fix: land #64, #65, #67 on rewritten master + README refresh - #73
Merged
Merged
Conversation
…wed sets normalize_loop_event() in .agent/tools/data_layer_export.py copies entry["event"], entry["status"], and entry["decision"] straight into the exported "action" and "result" fields with no validation. Those three keys are supposed to come from a small, supervisor-controlled finite set -- harness_manager/loops/runner.py only ever writes 10 distinct event names, 10 status names, and 3 decision names to runtime/loops/events.jsonl -- but nothing enforced that on the export path, so any row with an unexpected value for those fields (a bug upstream, a hand-edited events.jsonl, or a future loop kind that doesn't yet exist) would flow the raw string straight through into the dashboard/analytics surface. That matters more here than it would in a generic ETL script: this repo's own design intent for the loop event journal is that it's content-free by construction. task/prompt/command/output are excluded from events.jsonl entirely for exactly this reason -- see the existing test_exports_privacy_safe_loop_events_and_quality_counts coverage, which already asserts a "task" field never makes it into the export. event/status/decision were the one gap in that whitelist discipline: a closed, small vocabulary in the writer, treated as open text on the read side. Add VALID_LOOP_EVENTS/VALID_LOOP_STATUSES/VALID_LOOP_DECISIONS (mirrored directly from runner.py's own literals) and a small _allowed_or_unknown() helper. Anything outside the allowed set redacts to "unknown" -- the same fallback already used everywhere else in this file for missing or unrecognized values, so this isn't introducing a new convention, just applying the existing one to a field that was skipped. Adds one regression test alongside the existing loop-event privacy test, verified to fail against the pre-fix code (a script tag and a free-text string both land in the exported JSONL verbatim without the fix) and pass with it. (cherry picked from commit 5a2724e)
call_model()'s MiniMax path silently dropped the caller's max_tokens entirely on the OpenAI-compatible wire -- the chat.completions.create() call passed model/temperature/messages but nothing for output length. Every MiniMax call through this wire has been unbounded regardless of what max_tokens the caller specified. The MiniMax OpenAI-compatible Chat Completions API deprecated bare max_tokens in favor of max_completion_tokens; add max_completion_tokens=max_tokens to close the gap. The Anthropic wire is untouched -- it already forwards max_tokens correctly and Anthropic's API keeps that parameter name. Extends the existing test_call_model_openai_wire_global coverage with an assertion on the new kwarg. Verified in both directions: fails with KeyError against the pre-fix code, passes with the fix. (cherry picked from commit 7b88fcd)
_load_learn() stubs sys.modules["text"]/["cluster"] to isolate learn.py from its two sibling modules, but never removed the stubs after loading -- they stayed process-wide for the rest of the test run. Verified this is a real leak, not a theoretical one: after calling _load_learn() once, both "text" and "cluster" remain in sys.modules; any later test or import in the same process would silently get the throwaway lambda stand-ins instead of the real modules. Save whatever was previously in sys.modules for those two names (or note there was nothing) before stubbing, and restore that exact prior state in a finally block around exec_module() so the stubs never outlive the one learn.py load they exist for. Also adds encoding="utf-8" to the one read_text() call in this file that was using the platform default encoding, per this repo's own "force UTF-8 when reading/writing tracked text" convention (already followed everywhere else _episodic() and learn.py itself read files). The third test in this file, test_append_mirror_fails_open_on_write_error, is untouched: _append_episodic_mirror()'s fail-open behavior on a write error is this file's own explicit, documented design (its docstring says so directly, matching _lesson_already_appended's read-only fail-open posture) -- not a bug. (cherry picked from commit 20441c2)
claude_code_post_tool.py persisted raw absolute paths and raw edit/write content into AGENT_LEARNINGS.jsonl -- a log meant to be shared, diffed, and exported via the data flywheel: - file_path values went into action/reflection unnormalized, baking the operator's home directory into every entry that touched a file. - Edit/MultiEdit reflection used repr(old[:30])/repr(new[:30]) -- literal content excerpts, not just that an edit happened. - The generic _reflection() fallback (for tool types not explicitly handled -- Read, Grep, Glob, WebFetch, etc.) embedded raw json.dumps(tool_input) whenever it was under 80 chars, which is backwards: a short absolute path is exactly the case most likely to leak in full. Adds _normalize_path(), reusing the hook's own existing AGENT_ROOT resolution (no new dependency, no new config) to normalize to project-relative or ~-relative form. Replaces content/diff previews with char counts. Fixes the _reflection() fallback to normalize paths before its length check instead of dumping raw JSON. tests/test_claude_code_hook.py: new TestNoRawContentOrPathsPersisted-style coverage (8 new checks, section 10b) asserting no raw home path or file content survives into a persisted entry, for both Edit and Write -- 62/62 passing, up from 54/54. Fixes #66 (cherry picked from commit 81e0f1e)
The loop allowlist added in the previous commit missed three values the
supervisor really writes: runner.py uses the run status as the event name
on a breaker stop ("exhausted"), and process.py reports maker results as
"failed_to_start" or "timed_out". Those rows were exported as
"unknown". Add them and cover them in the redaction test.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 766b34e312
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+462
to
+466
| _fallback_input = dict(tool_input) if isinstance(tool_input, dict) else {} | ||
| for _key in ("file_path", "path", "new_path"): | ||
| if isinstance(_fallback_input.get(_key), str): | ||
| _fallback_input[_key] = _normalize_path(_fallback_input[_key]) | ||
| inp_str = json.dumps(_fallback_input) |
There was a problem hiding this comment.
When the hook receives a tool without an explicit branch—such as Claude Code's NotebookEdit—and its serialized input is under 80 characters, the fallback reflection persists this entire dictionary. Only file_path, path, and new_path are normalized, so inputs such as {"notebook_path":"/home/alice/x.ipynb","new_source":"API_KEY=secret"} retain both the absolute home path and raw content in episodic memory, which may later be committed or exported. Replace the fallback serialization with whitelisted metadata or explicitly redact content-bearing and path fields.
Useful? React with 👍 / 👎.
- claude_code_post_tool: the fallback reflection no longer dumps short tool inputs (NotebookEdit notebook_path/new_source leaked); it records the normalized path and input key names. Non-Bash detail stores output size instead of the first 150 chars of output (Read of a secrets file leaked), plus the first error line on failure. Paths outside both the project and home become <external>, not ../../<user>/... - llm: MiniMax OpenAI wire uses max_tokens; max_completion_tokens does not exist in the minimum supported openai==1.40.0 and raised TypeError. - data_layer_export: allow phase_started/running from the desktop supervisor. - README/CHANGELOG: 14 seed skills, not 15. Every new check fails on the previous code and passes now.
Failed non-Bash tool calls appended the first 150 chars of the error to the episodic entry, so an error echoing a path or file content bypassed the metadata-only detail. Store error_chars instead. The new check fails on the previous code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
masterhistory was rewritten after #64, #65, #67 and #68 were opened, so each of them shows ~31k lines / 231 files and conflicts. Each PR's real change is only its last 1–2 commits. This PR cherry-picks those commits (-x, original author kept) onto currentmaster.What
fix(data-layer): redact loop event/status/decision outside the allowed sets (cyre)fix(llm): forwardmax_tokenson the MiniMax OpenAI wire (cyre)fix(tests): restorelearn.pymodule stubs, force UTF-8 read (cyre)fix(claude-code): redact home paths and raw content in episodic logging, fixes Privacy: claude_code_post_tool.py persists raw absolute paths and file content/diffs into AGENT_LEARNINGS.jsonl #66 (cyre)runner.pyuses the run status as the event on a breaker stop (exhausted), andprocess.pyreportsfailed_to_start/timed_out. Without this, those rows exported asunknown. The values are added and the test covers them (the test fails without the fix).loops/, Copilot/Pi hooks,tests/), fix the Windsurf rule path, move the stranded install notes, drop the duplicate changelog section. CHANGELOG:[Unreleased]entry.Review follow-ups (second commit)
detailstores output and error size, not their text (Read and error leaks). Out-of-tree paths become<external>.max_tokens.max_completion_tokensraisesTypeErroron the supported minimumopenai==1.40.0(verified locally).phase_started/running(desktop supervisor).Not included
api.star-history.com/svg?...returns a valid chart today (May–Sep data, ~2K stars). The PR would also point the README image and link at an unknown third-party domain (star-history.dera.page). The official link stays.Verification
pytest: 183 passedtests/test_claude_code_hook.py: 69/69 (the 5 new privacy checks fail on the pre-fix code).agent/tools/test_learn_episodic_mirror.py: OKverify_codex_fixes.py: all regression checks passedSupersedes #64, #65, #67. Closes #66.
🤖 Generated with Claude Code
Note
Redact raw paths and tool content from Claude Code episodic entries and allowlist loop exports
_normalize_pathin claude_code_post_tool.py so episodic action labels, reflections, and details store project-relative or tilde paths instead of raw absolute paths, with outside paths labeled<external>and invalid values as?.unknown.max_tokensto the MiniMax OpenAI-wire request in llm.py, and adds regression tests for path/content redaction and loop-value allowlisting._detail/_reflectionfields inclaude_code_post_tool.pymust use the new counts and normalized paths; unrecognized loop event/status/decision text is dropped from exports.Macroscope summarized e6f8b95.
The PR appears safe to merge based on the reviewed changes.
Summary
The PR combines loop-export redaction, MiniMax token-limit forwarding, Claude Code hook privacy changes, test repairs, and documentation updates. Since the previous review, the hook also stops storing raw non-Bash error text. All five previous Greptile threads are resolved; no new actionable issue was established.
Reviews (3) · Last reviewed commit: "fix(claude-code): store error size, not ..."