fix(learn): resolve saved-trajectory path via shared get_trajectories_dir#247
Conversation
…_dir The learn Stop hook hardcoded `.evolve/trajectories/claude-transcript_<id>.jsonl` while the save-trajectory Stop hook resolved the same path from env. When EVOLVE_DIR was set the two hooks disagreed and the learn skill read a non-existent file, silently producing zero guidelines. Add get_trajectories_dir() to lib/entity_io.py that returns `get_evolve_dir() / "trajectories"` (absolute). Both Stop hooks now share this helper; learn's on_stop emits the resolved absolute path in its reason field. Update SKILL.md so the skill no longer assumes a relative path. Add platform-integration tests covering EVOLVE_DIR-set and default scenarios plus a parity check between the two hooks. Fixes AgentToolkit#246
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a shared ChangesTrajectory Path Resolution Unification
Sequence DiagramsequenceDiagram
autonumber
participant SaveHook as save-trajectory on_stop
participant LearnHook as learn on_stop
participant EntityIO as lib/entity_io.get_trajectories_dir
participant FS as Filesystem
SaveHook->>EntityIO: request trajectories dir for session
EntityIO->>FS: ensure <EVOLVE_DIR|.evolve>/trajectories exists (mode 0o700)
EntityIO-->>SaveHook: return resolved trajectories path
SaveHook->>FS: copy transcript -> {resolved}/claude-transcript_<session>.jsonl
SaveHook-->>Tester: "Trajectory saved: <resolved path>"
LearnHook->>EntityIO: request trajectories dir for session
EntityIO->>FS: ensure dir exists (if needed)
EntityIO-->>LearnHook: return resolved trajectories path
LearnHook-->>Tester: emit reason with "The saved trajectory path is: <resolved path>"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Fixes failing CI check: check-formatting (3.12)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/platform_integrations/test_stop_hooks_path_resolution.py (2)
38-46: ⚡ Quick winAdd
timeouttosubprocess.runto prevent indefinite CI hangs.If a hook regression causes it to stall (e.g., waiting for additional stdin), the test suite hangs with no recovery. A few seconds is more than enough for these lightweight scripts.
⏱️ Proposed fix
return subprocess.run( [sys.executable, str(script)], input=json.dumps(payload), capture_output=True, text=True, cwd=str(cwd), env=env, check=True, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/platform_integrations/test_stop_hooks_path_resolution.py` around lines 38 - 46, The subprocess invocation in the test currently calls subprocess.run([...], input=json.dumps(payload), capture_output=True, text=True, cwd=str(cwd), env=env, check=True) without a timeout, which can hang CI; add a timeout parameter (e.g., timeout=5 or another small number) to the subprocess.run call in this test (the call that uses variables script, payload, cwd, env) so the test fails quickly instead of blocking indefinitely.
66-72: ⚡ Quick winUse
temp_project_dirinstead oftmp_pathfor file operations in this subprocess-spawning test.The coding guidelines require
temp_project_dirfor all file operations in platform integration tests (tests/platform_integrations/), withtmp_pathonly permitted for pure-Python tests that spawn no subprocesses. Since_run_hookcallssubprocess.run, the exception does not apply here. Thecwdpassed to each subprocess should also usetemp_project_dirso the process's.evolve/resolution is fully within the managed sandbox.♻️ Proposed refactor
`@pytest.fixture` -def fake_transcript(tmp_path): +def fake_transcript(temp_project_dir): """Create a fake live transcript file matching Claude Code's layout.""" - src = tmp_path / "projects" / "fake-project" / f"{_SESSION_ID}.jsonl" + src = temp_project_dir / "projects" / "fake-project" / f"{_SESSION_ID}.jsonl" src.parent.mkdir(parents=True) src.write_text('{"type":"assistant","content":"hi"}\n') return srcAnd update each test to pass
temp_project_dirascwd:-def test_learn_uses_evolve_dir(tmp_path, fake_transcript): - custom = tmp_path / "my-evolve" +def test_learn_uses_evolve_dir(temp_project_dir, fake_transcript): + custom = temp_project_dir / "my-evolve" result = _run_hook( LEARN_ON_STOP, - cwd=tmp_path, + cwd=temp_project_dir, ... )(Apply the same
temp_project_dirsubstitution across all test functions and the parity test.)As per coding guidelines: "All file operations in platform integration tests must use
temp_project_dirfixture - never touch real files."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/platform_integrations/test_stop_hooks_path_resolution.py` around lines 66 - 72, Replace tmp_path with the temp_project_dir fixture for all file operations in this test: update the fake_transcript fixture to accept temp_project_dir instead of tmp_path and create the transcript file under temp_project_dir so the file and its parent dirs are inside the sandbox; also ensure calls to _run_hook (and any subprocess.run invocations) pass cwd=temp_project_dir so subprocesses resolve .evolve inside the managed temp_project_dir; apply the same substitution across all related test functions and parity checks referencing fake_transcript or tmp_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/platform_integrations/test_stop_hooks_path_resolution.py`:
- Around line 38-46: The subprocess invocation in the test currently calls
subprocess.run([...], input=json.dumps(payload), capture_output=True, text=True,
cwd=str(cwd), env=env, check=True) without a timeout, which can hang CI; add a
timeout parameter (e.g., timeout=5 or another small number) to the
subprocess.run call in this test (the call that uses variables script, payload,
cwd, env) so the test fails quickly instead of blocking indefinitely.
- Around line 66-72: Replace tmp_path with the temp_project_dir fixture for all
file operations in this test: update the fake_transcript fixture to accept
temp_project_dir instead of tmp_path and create the transcript file under
temp_project_dir so the file and its parent dirs are inside the sandbox; also
ensure calls to _run_hook (and any subprocess.run invocations) pass
cwd=temp_project_dir so subprocesses resolve .evolve inside the managed
temp_project_dir; apply the same substitution across all related test functions
and parity checks referencing fake_transcript or tmp_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8ca6880-03e4-4ae3-890f-ff9de1753153
📒 Files selected for processing (5)
platform-integrations/claude/plugins/evolve-lite/lib/entity_io.pyplatform-integrations/claude/plugins/evolve-lite/skills/learn/SKILL.mdplatform-integrations/claude/plugins/evolve-lite/skills/learn/scripts/on_stop.pyplatform-integrations/claude/plugins/evolve-lite/skills/save-trajectory/scripts/on_stop.pytests/platform_integrations/test_stop_hooks_path_resolution.py
SummaryThis PR hardens the filesystem backend against interrupted writes, aligns several sharing/sync stderr expectations, and tries to unify Claude stop-hook trajectory path Findings
Testing
|
|
@visahak Thanks for the reproducer. I'd prefer to keep For the "hook launched outside the repo root" case, I'd rather rely on the Claude Stop hook's cwd contract (which is project root today), or set Happy to open a follow-up if the cwd assumption breaks in the field. |
2 participants
Summary
learn/scripts/on_stop.pyhardcoded.evolve/trajectories/claude-transcript_<id>.jsonlwhilesave-trajectory/scripts/on_stop.pyresolved the same path fromEVOLVE_DIR. WhenEVOLVE_DIRwas set, the two hooks disagreed — the learn skill read a non-existent file and silently produced zero guidelines.get_trajectories_dir()inlib/entity_io.pyreturningget_evolve_dir() / "trajectories"(absolute). Both Stop hooks now share it; learn's hook emits the resolved absolute path in itsreasonfield so the skill canReadit directly.learn/SKILL.mdso the skill no longer assumes a relative path.Fixes #246.
Test plan
tests/platform_integrations/test_stop_hooks_path_resolution.py— 9 tests covering EVOLVE_DIR-set and default scenarios for each hook plus a parity check. All pass.tests/platform_integrations/test_entity_io.py— 11 tests, no regressions.tests/e2e/test_sandbox_learn_recall.py::test_learn_then_recall_flow— passed in 4m18s against the Docker sandbox.Summary by CodeRabbit
Refactor
Documentation
Tests