Review Security Vulnerability Report

I have enough to give a confident assessment. Here's my finding.

Verdict: The report is valid — confirmed arbitrary file write → RCE

I traced the full data flow myself rather than taking the scanner's word for it. The core claim holds: SessionID read from checkpoint metadata is used to build filesystem write paths with no validation on the resume/rewind restore paths, and for at least two agent types the value is written verbatim, including absolute paths.

The data flow (confirmed)

1. SessionID is an unvalidated string in metadata — CommittedMetadata.SessionID (checkpoint/checkpoint.go), deserialized straight from the git blob in ReadSessionMetadata with no post-unmarshal checks.

2. The restore paths use it directly to build write targets, with no validation:

   • strategy/manual_commit_rewind.go:689 — RestoreLogsOnly (the primary path for both resume and rewind): sessionID := content.Metadata.SessionID → sessionFile := sessionAgent.ResolveSessionFile(sessionAgentDir, sessionID) → os.MkdirAll(filepath.Dir(sessionFile)) → WriteSession(...) writing attacker-controlled content.Transcript as NativeData.
   • rewind.go:708 restoreSessionTranscriptFromStrategy and :750 restoreSessionTranscriptFromShadow — take returnedSessionID from ReadRawSessionLogForCheckpoint (remote metadata) → resolveTranscriptPath → ResolveSessionFile, no validation.
   • resume.go:821 resumeSingleSession — same.
   • transcript.go:21 resolveTranscriptPath does no sanitization; it just calls agent.ResolveSessionFile.

3. The agent type (content.Metadata.Agent) is also attacker-controlled, and it picks the path-resolution behavior. The cleanest vector:

   • Pi (agent/pi/pi.go:172) and Codex (agent/codex/codex.go:93) both do if filepath.IsAbs(agentSessionID) { return agentSessionID } — they return an absolute path verbatim, no suffix. So SessionID = "/home/victim/.bashrc" + Agent = "pi" → writes attacker content straight to ~/.bashrc. Exactly the report's scenario.

Important nuances the report glosses over

• Suffix limits some agents, not all. Claude/FactoryAI append .jsonl, Gemini appends .json, Copilot appends /events.jsonl. For those, ../ traversal escapes the directory but the forced suffix prevents hitting an exact .bashrc. The "~/.bashrc" headline is specifically achievable via pi or codex (absolute path, no suffix), or against any *.jsonl-globbed target for the others. Either way it's arbitrary-location write of fully attacker-controlled bytes.

• Validation exists but isn't wired into this path. validation.ValidateSessionID / ValidateAgentSessionID (validation/validators.go) reject /, \, and non-[a-zA-Z0-9_-] chars. They're correctly applied on the write/creation path (checkpoint/committed.go:63, temporary.go) and in attach.go:537 — but never on the resume/rewind restore path. So the defense is built and just not called where it matters.

• Codex's guard is bypassable. Codex is the only agent implementing the validating RestoredSessionPathResolver (codex.go:108, calls ValidateAgentSessionID). But in RestoreLogsOnly (manual_commit_rewind.go:713-720), if ResolveRestoredSessionFile returns an error it just logs "using fallback path" and keeps the unvalidated sessionFile from the plain ResolveSessionFile — which for an absolute path already returned it verbatim. So the validation failure is non-fatal and the attack proceeds anyway. Pi has no guard at all.

• No user prompt gates the path. The only confirmation prompt (PromptOverwriteNewerLogs) is a timestamp/"local is newer" check and doesn't fire when the target isn't an existing transcript — so overwriting ~/.bashrc is silent.

Severity

I agree with Critical / arbitrary file write → RCE, with one caveat on the threat model: it requires the malicious checkpoint metadata to reach the victim's local entire/checkpoints/v1 (push access to a shared repo, a malicious collaborator/PR, or a compromised remote). Given the product explicitly fetches and shares this branch across machines, that's a realistic threat, not a contrived one.

Recommended fix (smallest correct change)

Validate at the choke point before any path construction — resolveTranscriptPath and the top of the RestoreLogsOnly per-session loop:

• Call validation.ValidateAgentSessionID(sessionID) (rejects /, \, .., absolute, non-path-safe) and skip the session on failure rather than falling back.
• In RestoreLogsOnly, make the ResolveRestoredSessionFile error path abort the session, not fall back to the unvalidated path.
• Defense in depth: after resolving sessionFile, assert it's still within sessionAgentDir (e.g. filepath.Rel has no .. prefix) before MkdirAll/WriteSession.

Want me to write a failing test reproducing the pi/codex absolute-path write and then apply the fix?

Done. Clean lint, all tests green.

Summary

Tests (written first, confirmed failing, then passing)

1. TestResolveTranscriptPath_RejectsTraversalSessionID (transcript_test.go) — table test at the choke point: absolute paths, ../ traversal, backslash, embedded separators, and empty all must be rejected. Before the fix, ../../../../../../tmp/entire-pwned resolved to /tmp/entire-pwned.jsonl.
2. TestResolveTranscriptPath_AllowsLegitSessionID — regression guard: a normal UUID still resolves correctly inside the session dir.
3. TestResumeSingleSession_RejectsPathTraversalSessionID (resume_test.go) — end-to-end proof through the real resumeSingleSession: a sentinel file outside the session dir must survive. Before the fix the test output literally showed Session restored to: .../victim/secret.jsonl — the sentinel was overwritten with attacker content. Now the write is refused and the sentinel is untouched.

Fix (centralized, not per-agent)

To your question — no, we don't fix each agent individually, and that's the important design call. The per-agent ResolveSessionFile methods are the sinks, but the bug is that a remote-trust-boundary value reaches them unvalidated. Fixing each agent would be fragile: every new agent re-introduces the bug, and codex already demonstrated a guard (ResolveRestoredSessionFile calling ValidateAgentSessionID) that was bypassable because RestoreLogsOnly fell back to the unvalidated path on error. So I validated at the two places where the remote-supplied session ID enters path construction:

• cli/transcript.go resolveTranscriptPath — guards resumeSingleSession, all rewind.go restore functions, and attach.
• strategy/manual_commit_rewind.go RestoreLogsOnly (write loop) + the status-collection loop — guards the multi-session resume/rewind path that builds paths directly.

Both call validation.ValidateSessionID (rejects /, \, empty). I deliberately reused the same validator that the write path already enforces (checkpoint/committed.go:63), so the fix re-asserts an existing invariant at the read boundary — it can't reject any legitimately-created checkpoint, while closing every separator-based traversal (the only escape primitives filepath.Join/IsAbs honor are / and \). The codex fallback-bypass becomes moot since a malicious ID never reaches that code now.

The strategy/resolve_transcript.go resolver (live SessionState, locally-derived/trusted) was intentionally left untouched.

Want me to also add a strategy-package test that exercises RestoreLogsOnly directly with a hand-crafted malicious checkpoint blob (the multi-session path), or is the resume-level end-to-end proof sufficient coverage for you?

I've now verified the key flows directly. Here's the consolidated review.

Audit: {file}path.Join + disk-write flows

Method: mapped all 284 non-test Join sites and ~190 write/mutation ops, then fanned out three read-only agents over strategy/+checkpoint/, agent/, and everything else — filtering for the only pattern that matters: an untrusted path component reaching a write. I verified each real finding against source myself. Threat model: an attacker who can push to the shared entire/checkpoints/v1 branch, plus the local agent/hook boundary.

Headline: the working-tree restore is genuinely defended (mostly)

ManualCommitStrategy.Rewind restores files from a checkpoint tree via osroot.WriteFile(repoRootHandle, f.Name, …) (manual_commit_rewind.go:433). osroot is built on Go 1.24 os.Root, which enforces containment at the kernel level at read/restore time (osroot.go:40). So even a maliciously-crafted tree entry name like ../../../etc/cron.d/x cannot write content outside the repo — the write errors out. This is the correct defense (the first agent justified it via write-time normalizeGitTreePath, which is the wrong reason — a malicious remote never uses our writer — but the os.Root layer makes the conclusion right anyway).

Findings worth acting on

1. os.MkdirAll in the restore loop is not containment-guarded (low) manual_commit_rewind.go:419-423:

A tree entry name with ../ makes os.MkdirAll create empty directories outside the repo before the subsequent osroot.WriteFile refuses the escaping write and aborts the rewind. No attacker-controlled content escapes, but arbitrary empty-dir creation + a failed/partial rewind is a real defense-in-depth gap. The comment even flags it: "MkdirAll not available on os.Root." Fix: lexically reject f.Name containing ../absolute before the join (or derive the dir inside the root).

2. posttodo hook path skips SessionID validation (low) hooks_claudecode_posttodo.go:91 takes input.SessionID unvalidated → GetNextCheckpointSequence → SessionMetadataDirFromSessionID (paths.go:222 — bare string concat ".entire/metadata/" + sessionID) → os.ReadDir (state.go:646). A hook payload with session_id: "../../.." makes ReadDir list an arbitrary directory. It's a read/listdir only (the count feeds a sequence number), the source is the local same-privilege agent, and it reaches no write — so impact is minimal. But every other hook entry (lifecycle.go:111/283/357) validates SessionID here and this one doesn't. Fix: add the same validation.ValidateSessionID guard at the top, for consistency.

Latent / defense-in-depth (not currently reachable — the central fix covers them)

The agent/ audit confirmed our resume/rewind choke-point fix neutralizes these, but the agent implementations are unsafe by themselves if ever called with unvalidated input:

• codex (codex.go:93) and pi (pi.go:172) return the session ID verbatim when filepath.IsAbs — no suffix, full arbitrary path.
• copilot (copilotcli.go:78) uses the ID as a directory component (Join(dir, id, "events.jsonl")) — ../ escapes.
• gemini (gemini.go:91) interpolates the ID into a filepath.Glob pattern — glob metacharacters, not traversal.

Worth a cheap hardening: have validation.ValidateSessionID also reject filepath.IsAbs (closes the Windows C:-relative edge that /+\ checks miss), and/or make codex/pi reject absolute IDs in ResolveSessionFile so the interface is safe-by-default rather than relying on every caller validating.

Confirmed safe (spot-checked)

doctor bundle --out (user-chosen output, by design), plugin_store symlink/rename (constrained to managed bin dir), session/state.go (validates on Load/Save/Clear), lifecycle.go writes (validates first), settings/logging (hardcoded paths), clean.go (deletes via validated cleanup items).

Net: no new arbitrary-content-write equivalent to the SessionID bug — the os.Root layer holds the line on the scary path. Two low-severity consistency gaps (MkdirAll guard, posttodo validation) plus the safe-by-default agent hardening are worth fixing.

Want me to (a) fix the two low-severity gaps + add IsAbs to ValidateSessionID, with tests, or (b) just the agent safe-by-default hardening, or both? I'd lean toward both since they're small and tighten the same class of bug.

All touched packages lint clean and tests pass. Here's the complete picture.

What I changed

1. Strengthened the shared validator (validation/validators.go)

ValidateSessionID now also rejects bare ./.. (separator-free but still traverse when used as a path segment — e.g. Copilot uses the ID as a directory component) and filepath.IsAbs (Windows drive-path edge the /+\ check missed). This is the single highest-leverage "safe-by-default" change: every trust boundary that builds a path from a session ID calls this, and it's the same function the write path uses, so legit UUID-style IDs are unaffected. Tests added for ., .., and the a..b (allowed) case.

2. New os.Root primitive + fixed the rewind MkdirAll (low-sev gap #1)

Added osroot.MkdirAll (osroot/osroot.go) — creates parents one level at a time via os.Root.Mkdir, so the kernel enforces containment (os.Root lacks a native MkdirAll). Rewired the working-tree restore (manual_commit_rewind.go:418) from the raw os.MkdirAll(filepath.Join(repoRoot, f.Name)) to osroot.MkdirAll(repoRootHandle, path.Dir(f.Name), …). Now both filesystem operations in that loop (mkdir + the already-present osroot.WriteFile) are containment-checked — a crafted .. tree entry name can no longer create directories outside the repo. Traversal/idempotency/no-op tests added in osroot_test.go.

3. Closed the posttodo validation gap (low-sev gap #2)

GetNextCheckpointSequence (state.go:640) now validates both sessionID and taskToolUseID before building the os.ReadDir path; an unsafe value just starts the sequence at 1 instead of letting ../.. redirect the directory listing.

4. Agent safe-by-default — contract + regression tests (not per-agent sanitization)

I deliberately did not push sanitization into each agent's ResolveSessionFile: that's the fragile per-agent approach we rejected, the method has no error channel, and Codex/Pi intentionally return absolute recorded transcript paths (and I confirmed resolve_transcript.go:52-54 only ever feeds them a filepath.Base, so the footgun branch is unreachable with tainted data post-fix). Instead:

• Documented the security contract on the Agent.ResolveSessionFile interface (agent.go): callers with untrusted IDs must validate first; the resume/rewind choke points do.
• Added two contract-pinning regression tests: agent/codex/security_contract_test.go (absolute-verbatim footgun ↔ validator rejects it) and agent/copilotcli/security_contract_test.go (.. dir-component escape ↔ validator rejects it). These fail if either the agent behavior or the validator guard regresses.

"Are there more places we should use os.Root?"

I surveyed every raw os.{WriteFile,ReadFile,MkdirAll,Create,OpenFile,Rename,Remove} in checkpoint/ and strategy/. The honest answer: the rewind MkdirAll was the one clear win, now done. The rest fall into two buckets that os.Root doesn't fit:

• No enclosing root to scope to — git-hook args (commitMsgFile in manual_commit_hooks.go), go-git config paths (configloader.go), and agent transcript reads/writes (opts.TranscriptPath, state.TranscriptPath) are absolute paths in .git/ or ~/.claude/ outside the repo. There's no directory to root them under.
• Path component already validated or non-remote — session_state.go (validated session ID), condensation.go prompt files (validated ID), and the common.go/content_overlap.go reads use paths from local git status output (the working tree), not remote-crafted data.

So I'd call os.Root adoption complete for the remote-attacker threat model. If you want belt-and-suspenders, scoping session_state.go's .git/entire-sessions/ writes to an os.Root handle would be a reasonable defense-in-depth follow-up, but it's guarding already-validated IDs — low value.

Status

• Touched packages: 0 lint issues, all tests pass (osroot, validation, strategy, cli, agent/codex, agent/copilotcli, agent/pi).
• ⚠️ Full mise run lint fails on three untracked scratch files at the repo root (test_fileiter.go, test_tree_names.go, test_tree_walk.go — duplicate package main, dated before this session). They're not mine and not tracked, so CI won't see them, but they break the local root-package typecheck. I didn't touch them — want me to remove them, or are they your active scratch work?

Done. Everything is committed, builds, tests green, and lints clean.

Branch soph/fix-checkpoint-path-traversal — 6 commits

Each commit is self-contained and builds — I split manual_commit_rewind.go cleanly across commits 1 and 3 (temporarily reverting the os.Root part so the core fix stands alone), and ordered the validator hardening (commit 2) before the agent contract tests (commit 5) that depend on it.

On the os.Root follow-up specifically

session/state.go Save now routes its atomic rename through os.Root (Load/Clear already did); the strategy-side session_state.go writers (StoreModelHint, StoreAgentTypeHint, ClaimSessionStartBanner), readers, and ClearSessionState all go through a scoped root via two small helpers. Go 1.26's os.Root natively supports Rename/OpenFile(O_EXCL)/WriteFile, so the atomic-write and first-writer-wins semantics are preserved exactly. I left the sibling entire-session-locks/ alone — different directory, and its flock is inode-bound so re-rooting would be fragile for no real gain. As noted, this is genuine belt-and-suspenders (the IDs are already validated), so it's all on harden: commits, not fix:.

Notes

• The verification you saw earlier where the root scratch files (test_*.go) vanished: they were never in any of my commits and are gone from disk — not by anything I ran (no rm), likely your own cleanup. Worth confirming they were disposable.
• Nothing pushed — local branch only, per the usual "commit only when asked" rule. Want me to push and open a PR, or hold?

Two things I did not do, by design — flag if you'd want either: (1) a strategy-level integration test that hand-crafts a malicious checkpoint tree to exercise the rewind MkdirAll containment end-to-end (heavy; the osroot unit tests + WriteFile/MkdirAll traversal tests cover the property), and (2) touching the lower-value raw reads on git-status/transcript paths outside any repo root.

/simplify complete. Build, tests, and lint all clean.

Applied (3 fixes)

1. osroot.MkdirAll — replaced 20-line hand-rolled loop with native os.Root.MkdirAll (Reuse + Simplification, flagged by 2 agents). Go 1.26 has a native (*os.Root).MkdirAll, so the manual per-segment Mkdir loop, strings.Trim/path.Join logic, and 5 added imports all collapse to a one-line delegation. This also dissolved the simplification nits about the dead "." early-return and the per-segment loop, and is more efficient (native stats before creating). Kept the thin wrapper so call sites stay consistent with the sibling osroot.WriteFile/Remove in the rewind loop. Dropped the now-obsolete TestMkdirAll_EmptyOrDotIsNoop (tested the removed Trim logic) and fixed the stale package doc that listed MkdirAll as unsupported.

2. Dead guard in the rewind restore loop (Simplification): if dir != "." && dir != "" → if dir != ".". path.Dir never returns "".

3. (Folded into #1) Stale package doc corrected.

Skipped (with reasons)

• searchTranscriptInProjectDirs missing validation (Altitude, "High") — false positive, verified by reading the call graph. runAttach validates at validateAttachPreconditions (:537) before any resolve, and the fallback is only reached after resolveTranscriptPath (which now validates) succeeds. Guarded twice over.
• Hoist hint/marker writes into session.StateStore / centralize os.Root ownership across packages (Reuse + Altitude, "Medium") — real observation, but a cross-package refactor well outside this hardening diff's scope; the current split works correctly. Worth a separate cleanup PR.
• ClearSessionState delegate to StateStore.Clear (Reuse) — would add cross-package coupling for ~6 near-identical lines; marginal.
• Merge openSessionStateRoot + ...ForRead into one bool-param function (Simplification) — two named functions with different return semantics (nil-on-missing) read clearer than a createIfMissing flag argument.
• Remove filepath.IsAbs from ValidateSessionID (Simplification — "redundant with separator check") — true that it's currently unreachable, but kept deliberately as defense-in-depth in a security validator: if the separator check is ever loosened, IsAbs is the backstop. One harmless branch, documented intent.
• Shared setupOsRoot(t) test helper (Simplification) — the existing osroot tests each do their own setup; adding a helper only for the new ones would be inconsistent.
• The efficiency agent found no regressions (one extra openat per hook is dwarfed by hook subprocess cost; GetGitCommonDir is cached; restore-loop MkdirAll count is unchanged from before).

Net: −44/+10 lines, all in commit 3's territory (osroot + rewind). The changes are uncommitted in the working tree — want me to fold them into commit ff58f37 (the os.Root commit) via rebase, or add them as a follow-up cleanup commit?

Committed. Working tree is clean.

Branch soph/fix-checkpoint-path-traversal — 7 commits

The /simplify cleanup landed as commit 8dcbb11 (a follow-up rather than folded into ff58f37, since git rebase -i isn't available here). It logically belongs to the os.Root commit — if you want a tidy history, this branch is a good candidate for squash-merge, or I can leave the 7 commits as-is.

Nothing is pushed. Want me to push and open a PR?

Verified the claim against the code. My assessment:

It's accurate, and the misleading nolint makes it worse

The ordering gap is real. The hook flow is:

1. hook_registry.go:149 → handler.ParseHookEvent(...) runs first
2. Inside Pi's ParseHookEvent, the HookNameAgentEnd case calls captureTranscript(ctx, sessionID, payload.SessionFile) (lifecycle.go:175)
3. captureTranscript builds dst := filepath.Join(dir, sessionID+".json") and writes the file (lifecycle.go:295)
4. Only then does the returned event reach DispatchLifecycleEvent, which validates event.SessionID at lifecycle.go:111

So validation happens a full step after the path is built and written. And the //nolint:gosec // G703: dst constructed from validated session ID comment on line 295 is factually wrong — the ID is not validated at that point. A wrong "this is safe" comment is arguably worse than no comment, because it discourages exactly this scrutiny.

captureTranscript itself has no validation; it only guards against empty strings. A sessionID of ../../../../foo would write Pi's transcript bytes to an arbitrary .json path outside .entire/tmp/pi/.

But severity is low — and that matters for how we frame it

The taint source is Pi's hook stdin payload (or readCachedSessionID, which reads a file we wrote locally under gitignored .entire/tmp/). That's the local Pi extension running as the user — not remote checkpoint metadata. There's no remote-attacker → victim RCE vector like the original report. The marginal capability is "local code that already controls the hook payload can write a local transcript to an arbitrary path." It's the same threat class and severity as the posttodo hook gap I already fixed in this branch (GetNextCheckpointSequence) — local hook input, same-privilege, defense-in-depth.

Recommendation: yes, fix it here — it's trivial and on-theme

The branch is literally fix-checkpoint-path-traversal and already hardens one pre-existing local-hook path (posttodo), so this is coherent. The fix is one guard at the right altitude (inside captureTranscript, the choke point where the ID becomes a path — covers both call sites), plus correcting the bogus comment:

Returning "" matches the function's existing "couldn't capture" contract, so dispatch behavior is unchanged. I checked the sibling cacheSessionID/clearCachedSessionID — those write to the fixed constant pi-active-session, not an ID-derived path, so they're already safe; captureTranscript is the only sink. Legit Pi session IDs are UUID-style and pass ValidateSessionID, so no breakage.

Want me to apply it (new fix: commit on this branch), or would you rather keep this branch scoped to the original report and track the Pi gap separately?

Committed as 6f28057. Working tree is clean.

Done — Pi captureTranscript gap fixed

The fix (cmd/entire/cli/agent/pi/lifecycle.go): validate the session ID at the top of captureTranscript — the choke point where the ID becomes a filesystem path, covering both agent_end call sites. Unsafe IDs return "" (the function's existing "no capture" signal), so dispatch behavior is unchanged. Also corrected the gosec comment that falsely claimed the ID was "validated" at the write.

Regression test (lifecycle_test.go): TestCaptureTranscript_RejectsTraversalSessionID confirms ../, absolute, bare .., and embedded-separator IDs are all refused and the sentinel outside the cache dir is untouched.

Why it was real but low severity: verified the ordering — hook_registry.go:149 runs ParseHookEvent (which writes the transcript) before DispatchLifecycleEvent validates at lifecycle.go:111. But the taint is the local Pi hook payload (same-privilege), not remote metadata, so no victim-RCE vector — defense-in-depth consistent with the posttodo hook fix already in this branch. Confirmed the sibling cacheSessionID/clearCachedSessionID write a fixed filename (pi-active-session), not an ID-derived path, so captureTranscript was the only sink needing a guard.

Branch state — soph/fix-checkpoint-path-traversal, 8 commits

Nothing pushed yet. Worth a mise run test:ci before pushing — want me to run the full pre-push checklist (fmt && lint && test:ci) and then push + open a PR?

Done — you were right, and it pointed at a genuine altitude problem rather than a one-off.

What I found and fixed

handleLifecycleTurnEnd built .entire/metadata/<event.SessionID>/ via os.MkdirAll + os.WriteFile inline with no validation (lifecycle.go:536), while its siblings SessionStart/TurnStart/ToolUse each validated. ModelUpdate/Compaction/SubagentEnd were also unguarded, surviving only because the strategy layer (MutateSessionState/StoreModelHint) validates internally — but TurnEnd's inline path construction had no such backstop.

The real lesson: per-handler validation is the wrong altitude — it's exactly the check a handler forgets, which is what happened. So rather than patch just TurnEnd, I added one guard in DispatchLifecycleEvent that validates every non-empty event.SessionID before routing. That covers all nine handlers and any future one. Empty IDs still pass through to each handler's own empty-handling (e.g. TurnEnd's "unknown" fallback — confirmed path-safe).

Test: TestDispatchLifecycleEvent_RejectsTraversalSessionID asserts a ../-laden ID is rejected for TurnEnd, ModelUpdate, Compaction, SubagentEnd, and SessionEnd — before any handler/FS access (so it's parallel-safe, no repo needed).

Verification: full cli package, lint, and all 388 integration tests pass — confirming the guard rejects nothing real.

I left the now-redundant per-handler ValidateSessionID calls in place as harmless defense-in-depth (noted in the commit; consolidating them is separate cleanup coupled to their empty-checks).

Branch — soph/fix-checkpoint-path-traversal, 9 commits

This branch has now swept the resume/rewind, checkpoint-tree, session-state, posttodo, Pi-capture, and lifecycle-dispatch paths. A reasonable point to stop and run the full mise run check (fmt + lint + test:ci) before pushing. Want me to run that and push + open a PR, or keep auditing?

Done — committed as b1338e1. Honest verdict on your flag:

CalculateTotalTokenUsage — fragile, not live

It builds agent-<id>.jsonl and reads it with no local validation of agentID. But the only source — ExtractSpawnedAgentIDs → extractAgentIDFromText — already restricts the ID to [a-zA-Z0-9], so it's not currently exploitable. The real issue is the one you're pointing at structurally: a file read's path-safety depends on the incidental character set of a parser two calls away. Relax that parser (e.g. to accept hyphenated UUIDs) and it silently becomes a traversal-read.

I enforced the invariant at the choke point — ExtractSpawnedAgentIDs (both claudecode and factoryaidroid) now drops any ID failing validation.ValidateAgentID — so all six downstream consumers (token usage + modified-files, both agents) are path-safe by construction. Committed as harden: (not fix:) and flagged in the message that it's a no-op for today's inputs, with no test added because the guarded branch is unreachable through the real extractor.

While in there, I mapped the sibling agent-<id>.jsonl sinks — one is worth a closer look

That last row is the one I'd prioritize over the token-usage read: event.SubagentID is hook input (like the cases we've fixed), and metadata.go:102 builds a write destination from it. I haven't verified whether handleLifecycleSubagentEnd/metadata.go validate SubagentID before that write. Want me to investigate that path next? It's higher severity than the read I just hardened.

Branch state — 10 commits

Committed as 98f8e37. Here's the full result of the checkpoint-field audit you asked for.

Audit: every field we read from checkpoint metadata.json

I traced all CommittedMetadata / CheckpointSummary / SessionContent fields to their consumers, classifying each as display/logic-only, validated-at-read, or unvalidated-path-sink.

Validated at read — safe:

• CheckpointID — NewCheckpointID/JSON unmarshal enforces exactly 12 lowercase hex. Safe as the tree/disk path component it becomes.
• InvestigateRunID — investigate/manifest.go runs validateRunID (12-hex) before using it in filepath.Glob.

Display / logic / comparison only — never reach a path, git ref, or command: Branch (display, map keys, URL query param — never a git command), Agent (only agent.GetByAgentType registry lookup), FilesTouched (display + git tree reads via tree.File(), never a filesystem path), Kind, ReviewSkills, ReviewPrompt, InvestigateTopic, TurnID, TranscriptIdentifierAtStart, Model, Strategy, CLIVersion, plus the structured blobs (Summary, SessionMetrics, SkillEvents, InitialAttribution, PromptAttributions, TokenUsage).

One asymmetry, but not live:

• ToolUseID is validated on the write path (committed.go:66, temporary.go:285 via ValidateToolUseID) but not re-validated on read. However, every checkpoint-read consumer is display-only (committed.go:1286/common.go:740 → CommittedInfo → rewind/explain output). No read path reconstructs tasks/<ToolUseID>/ from the metadata value — task content is located by tree walking, not by rebuilding the path from the field. So it's a latent write/read asymmetry with no live sink. I deliberately did not add read-side validation: unlike the agentID case (which had an actual path construction far from its guard), there's no path sink here to protect, so it would be pure dead code.

Verdict on the scoped question

The checkpoint-read path is clean — nothing read from metadata.json reaches an unvalidated filesystem/git/command sink.

What the trace did surface (and I fixed)

The one genuinely actionable gap was adjacent, not a checkpoint read: handleLifecycleSubagentEnd built a subagent transcript path from hook-supplied event.SubagentID and read it before validation (a .. → arbitrary agent-*.jsonl read; local hook input, read-only → low severity). I extended the central DispatchLifecycleEvent guard to validate event.ToolUseID and event.SubagentID alongside SessionID, so all three identifiers are checked once before any handler turns them into a path. The task-checkpoint write paths (CapturePreTaskState, WriteTemporaryTask) were already validated. Full cli + 388 integration tests pass.

Branch — soph/fix-checkpoint-path-traversal, 11 commits

The identifier-validation surface (SessionID, AgentSessionID, ToolUseID, AgentID/SubagentID, CheckpointID, InvestigateRunID) is now consistently guarded at read/dispatch boundaries. Reasonable stopping point — want me to run the full mise run check and push + open a PR?

Confirmed — fallbackToolUseID returns "factorytask_" + hex(sha256...), a path-safe hash. So factoryaidroid's fallback path is safe regardless of session ID content.

That completes the double-check. All five items are covered; no new code changes needed this turn.

1–4: confirmed present in current HEAD

5: sweep of os.WriteFile/Open/OpenFile/Rename/Symlink/Create

Every sink classified by path provenance — all safe:

os.Open reads: the remaining un-validated ones (claudecode/lifecycle.go:245, codex/transcript.go:97/130) read a transcript path the local agent reports via its own hook payload — inherent local same-privilege trust, and read-only. On the restore path those go through validated resolveTranscriptPath. Not a cross-boundary concern.

Net

No unguarded attacker-influenceable write/open/rename/symlink sink remains. The identifier-validation surface (SessionID, AgentSessionID, ToolUseID, AgentID/SubagentID, CheckpointID, InvestigateRunID) is consistently enforced at read/dispatch/function boundaries, with os.Root as defense-in-depth on the two highest-value write loops.

Branch unchanged at 11 commits. Want me to run the full mise run check and push + open the PR now?

Done — pushed and PR opened: https://github.com/entireio/cli/pull/1365

You asked for proposed PR text — here's what I set as the body (happy to revise and update the PR if you want changes):

Title: Fix checkpoint/session path-traversal: validate IDs at read/dispatch boundaries

Summary

Closes a path-traversal / arbitrary-file-write class across the checkpoint, session, and agent-lifecycle paths. Root issue: identifiers read from the shared entire/checkpoints/v1 branch (attacker-influenceable) or from agent hook input flowed into filesystem paths without validation at the read/restore boundary.

Original vulnerability: entire session resume / entire checkpoint rewind used the metadata SessionID to build the transcript write path with no validation. A crafted absolute or ../-laden session ID (or, for Codex/Pi, an absolute ID returned verbatim) let attacker-controlled transcript bytes overwrite arbitrary files (e.g. ~/.bashrc) → RCE on the next resume, no prompt.

What changed (11 commits)

• Core fix: validate session IDs at the two restore choke points (resolveTranscriptPath, RestoreLogsOnly) before path construction.
• Validator hardening: ValidateSessionID also rejects ., .., absolute paths.
• Lifecycle dispatch (right altitude): validate SessionID/ToolUseID/SubagentID once in DispatchLifecycleEvent so no handler can forget (covers handleLifecycleTurnEnd's metadata write and handleLifecycleSubagentEnd's subagent read).
• os.Root containment: rewind dir creation via new osroot.MkdirAll; .git/entire-sessions/ writes scoped to os.Root.
• Other boundaries: Pi captureTranscript, GetNextCheckpointSequence (posttodo), ExtractSpawnedAgentIDs subagent-ID filtering; documented ResolveSessionFile contract + Codex/Copilot regression tests.

Audit results

• All CommittedMetadata fields traced: CheckpointID/InvestigateRunID validated at read; rest display/logic-only.
• All os.WriteFile/Open/OpenFile/Rename/Symlink/Create sinks swept: every attacker-influenceable path is validated, hash-derived, or os.Root-contained.

Testing

• Regression tests at each boundary; mise run check green (fmt + lint + unit + 388 integration + Vogon canary).

Two notes for reviewers' benefit you might want to weave in: (1) most of the lifecycle/hook gaps are local same-privilege (hook input) → low severity, vs. the resume/rewind one which is the remote→RCE headline; (2) the ToolUseID write/read asymmetry is documented as latent (no live read→path sink) rather than fixed. Want me to fold those severity callouts into the PR body, or leave it as-is?

Confirmed — PR is open and ready for review:

• PR #1365: "Fix checkpoint/session path-traversal: validate IDs at read/dispatch boundaries"
• OPEN, soph/fix-checkpoint-path-traversal → main, 11 commits
• https://github.com/entireio/cli/pull/1365

Everything's pushed, the gate is green (mise run check passed on re-run after the known go-git flake), and the body has the summary + audit results + testing notes.

The one open question from my last message still stands if you want it: shall I fold the two severity callouts into the PR body — (1) that the lifecycle/hook gaps are local same-privilege (low severity) vs. the resume/rewind remote→RCE headline, and (2) that ToolUseID's write/read asymmetry is documented-as-latent rather than fixed? Say the word and I'll update it.

I have the full diagnosis. Here it is.

Short answer: not a code regression, and not from your PR branch

1ea54e2's keyring.MockInit() is intact (global_test.go:20) and works — but it's process-global within the test process only. It fundamentally cannot affect a spawned subprocess, and that's where your prompt comes from.

Why you're getting prompts "again"

test:ci (and mise run check, which I ran several times) has two halves:

The canary builds that binary with mise run build = go build ./cmd/entire — no -tags=authfilestore (mise-tasks/build). So the spawned entire uses the real OS keychain (auth/store.go defaults to keyringBackend; tokenstore defaults to keyringStore). MockInit in the test process can't reach it. So 1ea54e2 made the in-process half keychain-free but never covered the canary subprocess — that's the gap, matching its own stated goal ("no keychain during test:ci") only partially.

Confirmed: nothing since 1ea54e2 — including your entire soph/fix-checkpoint-path-traversal branch — touches auth/, tokenstore/, the build task, or e2e auth (empty diffs). So neither the PR nor a recent code change caused it.

There's also a second, simpler source unrelated to tests: every git commit/git push in this repo triggers entire's own checkpoint-push hook → real auth → keychain. I committed/pushed ~12 times this session, so some prompts were almost certainly that.

Proposed fix (separate from PR #1365 — it's test infra)

Make the canary/e2e entire binary keychain-free like the integration subprocess already is:

1. Build it with -tags=authfilestore (the canary's mise run build currently omits it).
2. Set ENTIRE_TEST_AUTH_STORE_FILE=<tmp> in the e2e env — the auth file backend honors that env var in authfilestore builds (store_filebackend.go:24).
3. Caveat: internal/entireclient/tokenstore only has an in-process UseFileBackendForTesting hook (no env-var subprocess selector). If the canary's keychain hit goes through that store rather than auth, we'd also add a small env-var file-backend hook to tokenstore so the subprocess is fully keychain-free.

Want me to implement that on a separate branch (e.g. soph/e2e-canary-keychain)? I'd keep it off PR #1365 since it's unrelated test infrastructure. I can first reproduce by running the canary and watching which store triggers the prompt, to confirm whether step 3 is needed.

Done. The e2e keychain fix is on its own branch and PR, separate from the security work.

PR #1368 — https://github.com/entireio/cli/pull/1368

soph/e2e-canary-keychain → main, 1 commit, OPEN.

What it does

• Root cause: 1ea54e2's MockInit() is process-local and intact, but the e2e suite drives the real entire binary as a subprocess (which a subprocess can't inherit an in-process mock). Two stores were reachable: tokenstore, and the legacy auth.NewStore().GetToken() fallback in contexts.go that queries the keychain even with no token present — that's why you saw prompts during mise run check/test:ci (the canary stage) and on any e2e run.
• Fix (two layers, both stores):
  1. New shared build:e2e task builds entire with -tags=authfilestore; the canary, roger-roger, and default e2e tasks now use it. Production mise run build is left untagged.
  2. e2e TestMain sets ENTIRE_TOKEN_STORE=file + _PATH (covers tokenstore, no tag needed) and ENTIRE_TEST_AUTH_STORE_FILE (covers the auth keyring store under the tag), into the run's artifact dir — inherited by the binary and the git hooks it spawns.
• Verified: canary is green and now keychain-free by construction (resolveBackendLocked → fileStore; chooseBackend → file backend, so no keyring call can execute).

Net answer to your question

Not a regression in 1ea54e2 or in the security PR — it's a coverage gap that MockInit fundamentally can't close (subprocess), now fixed at the e2e build + env layer. Some of the prompts you saw during this session were also just the normal entire checkpoint-push hook firing on my git commits (real auth), which is expected and separate.

You now have two open PRs:

• #1365 — checkpoint/session path-traversal hardening (11 commits + the Copilot-review fixes).
• #1368 — this e2e keychain fix.

The test expects errMsg "volume separator" but I changed the message to "volume reference". Let me update the test: