feat(specialist): add lifecycle metadata - #106
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR extends the session system to track specialist execution metadata via ChangesSpecialist Metadata (Tag and Depth) Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
gnanam1990
left a comment
There was a problem hiding this comment.
Review — clean, approving
Tidy, well-scoped metadata groundwork, and nicely tested. Highlights:
- Hooks:
specialistStart/specialistStopevents added consistently —parseEventaccepts them with an updated error message, and the matcher check is refactored to a positiveeventSupportsMatcher(beforeTool/afterTool only) so the new lifecycle events correctly reject matchers too. Covered byTestLoadConfigRejectsMatchersOnLifecycleHooks+TestSpecialistHookEventsLoadAndSelect. - Sessions:
Tag/Depthflow throughMetadata/CreateInput/ChildInput, andCreateChildroutes throughCreate(), so theDepth < 0guard andTagtrimming apply to child sessions too (verified).omitemptykeeps existing JSON stable. Covered byTestStoreRejectsNegativeSessionDepth+TestPrepareExecPersistsSpecialistMetadataForNewSession. - CLI/contracts: tag/depth surfaced in
zero sessionsoutput and theSessionSnapshotcontract.
go build/vet/go test ./... all green. Approving.
Non-blocking notes
depthsemantics coordination (continuing the thread from #104):depthnow spans exec flags (#104),agent.Options.Depth(#104), and sessionMetadata.Depth(here). Worth making sure they all carry the same "specialist nesting level" meaning and converge cleanly with the upcoming task-runtime depth — so a child run's session depth == its agent nesting depth.childLinkPayloadcallsstrings.TrimSpace(input.Tag)whileCreate()also trimsTagfor the stored metadata — harmless redundancy, just noting.- As stated in the PR,
Tag/Depthare persisted/surfaced but not yet consumed by any runtime — fine as groundwork; flagging so it's a conscious staging step.
Nice incremental slice. 👍
🤖 Verified review via Claude Code.
…store, file locking (#112) * Session checkpoints & safe rewind: content-addressed blobs, atomic restore, file locking Module 4 of the runtime-core split. Scope: internal/sessions only. 3-way merged cleanly onto main's #106 Tag/Depth work (preserved); no new deps (filelock uses stdlib syscall); build/vet/-race/windows-cross-compile/full-suite green. - CaptureToolCheckpoint snapshots a tool's target files into content-addressed (sha256) blobs before a mutating tool runs; SnapshotForCheckpoint records file mode; oversize files are skipped, identical content deduped. - RestoreToSequence reverts to a target sequence: applies only the closest-to-target checkpoint per path, verifies each blob's sha256 before writing, preserves file mode, and confines paths with EvalSymlinks (no symlink escape). ApplyRewind runs restore+truncate+prune+marker atomically under one session lock. - Cross-process file lock (flock) serializes session mutations; Fork copies checkpoint blobs so a fork can rewind; writeMetadata uses a unique tmp suffix. FORWARD: this is the persistence layer; the CLI 'zero sessions rewind' and the TUI '/rewind' + pre-tool checkpoint capture wiring land in later modules. Exercised here by checkpoint_test.go / rewind_test.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address #112 review: capture-side confinement, atomic-only checkpoint API, Windows lock, test hygiene - [critical/human blocker] snapshotForCheckpoint now confines every capture target with resolveWithinWorkspace (same EvalSymlinks/no-".." guard as restore) BEFORE stat/read, so a traversal/symlink target can't read files outside the workspace into a blob. os.Stat failures are classified correctly: only os.IsNotExist -> Absent (restore deletes); permission/IO/symlink-loop -> Skipped (restore leaves alone), instead of treating every failure as Absent. Regression test added (TestCaptureRejectsTraversalTargets). - [major] SnapshotForCheckpoint unexported -> snapshotForCheckpoint: the blob writes and the referencing EventSessionCheckpoint must be committed atomically under one session lock, which only CaptureToolCheckpoint guarantees. Removes the unsafe snapshot-only external entry point (blob/event gap). - [major] Windows: real LockFileEx/UnlockFileEx (filelock_windows.go) replacing the no-op, so cross-process/cross-Store session serialization holds on Windows too. No new deps (x/sys already required for the unix flock). - [minor] checkpoint_test hygiene: check os.WriteFile/ReadDir/ReadFile/ReadEvents errors via helpers, add bounds checks before p.Files[0]/events[...] so setup failures fail at the source instead of panicking or passing spuriously. - rewind restore: documented the residual symlink-swap TOCTOU (boundary re-resolved immediately before the mutation; full closure needs openat2 RESOLVE_BENEATH / per-component O_NOFOLLOW, tracked for the CLI/TUI rewind-wiring work). build/vet/-race/full-suite + GOOS=windows build all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address #112 re-review: fail rewind on undecodable checkpoint payload [major] restoreToSequenceLocked silently continued past a checkpoint whose payload failed to decode, which let a NEWER checkpoint win for that path and restore a later state than the caller requested. Treat corruption as a hard rewind error. Regression test TestRestoreFailsOnCorruptCheckpointPayload (fails-open before, errors now). Re-review note: the earlier capture-target-validation (critical) and test bounds (minor) were already addressed in 600337b (resolveWithinWorkspace guard + len checks); those re-anchored threads are stale. The restore-side symlink-swap TOCTOU remains a documented heavy-lift residual (needs openat2/descriptor traversal), tracked for the rewind-wiring work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address #112 re-review: dedupe rewind on resolved path, not raw path [major] restoreToSequenceLocked deduped the per-path closest-to-target short-circuit on the RAW checkpoint path, so equivalent paths (./a.txt, dir/../a.txt, a symlink alias) keyed differently and a NEWER checkpoint could overwrite the closest-to-target snapshot for the same underlying file. Now resolve/confine FIRST and dedupe on the resolved workspace path (unresolvable targets dedupe on raw path). Regression test TestRestoreDedupesEquivalentRawPaths. This + the prior commit (24e9ba2, fail-hard on undecodable payload) address both correctness blockers @Vasanthdev2004 raised after CodeRabbit's pass. build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: KRATOS <kratos@KRATOSs-Mac-mini.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary:
Scope:
Tests:
Summary by CodeRabbit
Release Notes
TagandDepthmetadata, configurable via--tagand--depthcommand-line options.