Fix replay safety and git security issues from architecture review - #6
Conversation
A deep review of architecture quality, soundness, type safety and edge cases, covering the subsystems that landed after the previous review: the daemon + HTTP API + React UI (7d500a7) and workflow task tracking (627b28e). 64 findings survived an adversarial refutation pass: 2 critical, 15 high, 31 medium, 16 low. The two criticals both sit in the write path and both end with a green run whose tree does not contain the patch -- `ctx.integrate`'s verify-refused re-execution serving its own nested apply step, and its unsynchronised rollback on conflict. The critical in `matchHuman` replays a recorded denial as an approval after a script edit. All three were reproduced by running them. The review groups the findings under three missing abstractions rather than listing them flat: ownership modelled per-run instead of per-tree, the keyless-ambiguity guard living in one match function instead of in step identity, and the integration working tree being the one shared resource with no coordinator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
Both end with a green run whose journal claims something the world does not agree with, and both were reproduced before being fixed. `matchHuman` resolved a replayed human step by content hash alone. Human steps carry no `key` -- ask/approve/review and gateStep expose none -- so two gates with the same question are identical by content, and the first unconsumed entry for a hash was handed over. Deleting one of two identically worded gates slid the survivor onto the deleted gate's answer, so a recorded denial replayed as an approval with nothing in the journal to say it happened. It now takes seq and positionsTrusted like `matchStep`, serves on position where positions are trusted, and otherwise journals `replay.diverged` and re-opens the request. Re-asking costs a wait; guessing costs the truth. `ctx.integrate` guards itself with verifyServe, but the snapshot and apply that do the work were nested journaled steps with no guard. When verifyServe correctly refused -- the tree no longer carried the patch -- the parent re-executed and the nested pair was served straight back, returning ok without touching the tree, then re-journaling `patch.merged` with resultTree equal to baseTree. The change was lost permanently, since the new completion matched the untouched tree from then on. A re-establishing pass now does the work directly. `reExecuting` is set only when a journaled completion was refused, so a resume mid-ask, which leaves the parent incomplete, keeps reusing the pre-conflict snapshot as before. Four regression tests, each verified to fail with the fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
`treeHash` and `integrationBaseCommit` shelled out to git directly, bypassing GitCli, which prefixes `-c core.fsmonitor=false` on every call it makes. A pathname-valued core.fsmonitor is a program git executes on any worktree scan, and these two helpers run `add -A .` on every write-step dispatch and inside every ctx.integrate -- so cloning an untrusted repository was enough to execute repository-configured code with no gate. Verified against git 2.43: the hook fires on `add -A` without the flag and does not fire with it. All ten bare invocations now go through one local helper that applies the same floor. Its safety vars are spread last, because these callers build their env from process.env and spreading them first would let an inherited hostile value win. Separately, `checkIdle()` iterated the live listener array while listeners unregister themselves through `offIdle`, which splices it -- shifting every later element under the loop index and skipping every second waiter. With two waiters one never learns the run went idle. It iterates a copy now. Four regression tests, each verified to fail with the fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
A human step is identified by its content alone -- question, detail, schema, risk, timeouts -- so two gates that ask the same thing in different contexts are indistinguishable. The previous commit made that case re-open both gates rather than guess between them, which is the safe answer but not a usable one: there was no way to tell weft the gates were different, because ask/approve/review exposed no `key`. They do now, folded into the identity hash the same way `AgentOptions.key` is. Two gates worded "ship?" for staging and for prod keep their own answers across an edit that deletes one of them, instead of both being re-asked. Regression test covers the keyed pair surviving exactly the edit that forces a re-ask when the keys are absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
`ctx.agent.detailed(..., { onError: "null" })` returned a bare null when
the suppression happened live, but its replay path revived the carrier
`{ value: null, ... }` -- so a workflow branching on `result === null`
took one path on the first run and the other on a resume. The value alone
cannot distinguish the two, because a schema may legitimately permit
`value: null`, so the revived carrier is marked in a process-local
WeakSet and the caller restores the bare null from that.
`createWeft` now writes `.weft/.gitignore` covering runs/, blobs/,
tasks/ and index.sqlite. `.weft/` lives inside the user's working tree
and weft's own tree helpers run `git add -A .` on every write-step
dispatch and inside every ctx.integrate, so a user's repo folded its
journal and blob store into a git object on the first write step and
copied them into every agent worktree. workflows/ is deliberately not
listed -- it is source. The file is written once and never overwritten.
(This repository already ignores those paths in its own .gitignore; the
gap was every repo weft is pointed at.)
Two regression tests; the replay-shape one verified to fail with the fix
reverted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
Both defects come from the same gap: ownership, abort and terminality are tree-wide properties, and only the single run was modelled. `drive()` appended run.completed/run.failed and released the ownership claim as soon as the workflow BODY settled, without waiting for steps it had stopped awaiting -- a Promise.race loser, a lane of a raw Promise.all whose sibling rejected, a floating step. Those kept spending, kept writing to the repository, and appended into a journal that was already terminal, while the freed claim let a second process resume and interleave into the same file. It now waits, bounded, for its own steps before the terminal record, and holds the claim if they outlive the window, which is the position cancel() and shutdown() already take. Note it deliberately does NOT abort first the way those two do: every run in a tree shares one AbortController, so a child completing normally would tear down its parent. Waiting is the part that matters here. `fenceLostRun` fenced only the runtime that noticed. Because of that same shared controller the rest of the tree saw an ordinary cancellation and durably journaled run.cancelled -- so a child losing its claim terminally cancelled its parent, the exact inversion of the fence's contract, firing precisely when the system is under the stress the fence exists for. Fencing now walks to the root and covers every live descendant, so the whole tree stays resumable for its next owner. Two regression tests, each verified to fail with its fix reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
The daemon can start runs, answer approval gates, cancel and resume, and it has no authentication behind the ten lines of Host/Origin middleware that guard it. Two holes in that middleware were reachable from a browser. Loopback was treated as one origin, so any page served from any other localhost port -- a dev server on :3000, another local tool -- passed the Origin test. A CORS-simple POST from such a page carries an honest loopback Origin and could start runs and satisfy human approval gates. Origin now has to name the same host AND port the request was addressed to, normalised through URL so localhost:4781 and http://localhost:4781 still compare equal. A no-cors cross-site GET -- an img, a script, a form -- carries no Origin at all, so neither check ever saw it, and this surface has a GET that writes: /api/workflows/:name/tasks registers a namespace. Browsers that omit Origin send Sec-Fetch-Site instead, so that is now required to be same-origin or none. This narrows the browser-reachable surface; it is not authentication. Any local process can still reach the API, which is the gap the review's per-daemon token would close and this does not. Two regression tests, both verified to fail against the old guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
Adds a Status table to the review naming each Now-list finding and the fix that closed it, so the document does not read as a list of open problems. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eabc17307
ℹ️ 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".
Two P1 findings from the Codex review on PR #6, both real, both gaps in the drain added by the previous commit. Each was reproduced against the current code before being fixed. The cancellation exit never drained. `drained` kept its `true` initializer, so the finally released the ownership claim. cancel() keeps the claim when it times out, but an abort makes a COOPERATIVE lane reject at once -- so the body can reach this branch while a step that ignores its signal runs on, cancel() sees a body that settled promptly and drains happily, and the claim goes free with a step still executing. `drainSteps` consulted only `liveStepCount()`, which excludes steps that called markWaiting(). A workflow that stops awaiting a ctx.sleep hit the zero fast path immediately; confirmed by observation that the abandoned timer then appends `timer.fired` and `step.completed` after `run.completed`. Draining on waits unconditionally was wrong, and the suite caught it: one counter serves two different waits, because a sub-workflow step bridges its child's suspension into the parent's `waitingSteps`. A terminal root with a descendant still suspended on a person is a supported state, not an abandoned wait -- that child's records land in the child's journal and its question must stay answerable. Live children are the only reason a wait legitimately outlives the body, so the wait drain is gated on the run having none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e0c915bac
ℹ️ 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".
| // precedes a `step.completed`, and releasing the claim frees the run for another | ||
| // process while this one is mid-step. `cancel()` and `shutdown()` already refuse to | ||
| // do that; this is the path every run takes. | ||
| drained = await drainSteps(rt, TERMINAL_DRAIN_MS, active.children.size === 0); |
There was a problem hiding this comment.
Do not disable all wait draining merely because active.children is nonempty. If a workflow abandons a suspended ctx.workflow(...) via Promise.race or an early return, the parent workflow step is marked waiting while the child remains in this set, so this call immediately reports drained, appends run.completed, and releases/deletes the parent. When the child is later answered, launchChild returns and the parent runStep appends step.completed to the already-terminal, unowned parent journal. The new active.children.size === 0 condition is fresh evidence that the previously reported durable-wait problem remains for child-bridged waits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced and real — but not something this PR introduced, and I'm leaving it rather than pushing a third reshaping of this code.
Probe: parent does Promise.race([ctx.workflow(child), fastAgent]), child suspends on a gate, agent wins.
PARENT JOURNAL: … step.completed step.settled run.completed
CLAIM STEALABLE: true
AFTER TERMINAL: step.completed ← after answering the abandoned child
Two things make this a design change rather than a fix:
It predates the PR. Before this branch there was no terminal drain at all, so every abandoned wait behaved this way. children.size === 0 narrowed that to the child-bridged case; it didn't create it.
The obvious remedy breaks a supported state. I tried unconditional wait draining first and the suite rejected it: a sub-workflow step bridges its child's suspension into the parent's waitingSteps, and packages/daemon/test/api.test.ts ("keeps a sibling's question in the queue after the root has failed") asserts a terminal root with a live suspended child. Draining there blocks for the whole window, leaves the run reading executing, and — because that child stays suspended indefinitely — the root would never release its claim at all.
Both cases have children.size > 0 and a standing bridge wait. Separating them means knowing whether the body still awaits that child, which waitingSteps cannot express: one counter serves ctx.sleep, signal waits, in-step gates and the child bridge.
Proposed patch, for @sergeyzenchenko to take or leave: give RunRuntime a bridgedWaits counter incremented only on the waitBridge path in executeChildRun, and have the terminal drain wait on hasPendingWaits() minus bridged, unconditionally — no children gate. That drains the abandoned-race case while leaving a genuinely-awaited child alone. It touches the runtime's wait accounting, so it wants its own change and its own tests rather than riding along here.
Generated by Claude Code
| const poll = setInterval(() => { | ||
| if (!outstanding()) done(true); | ||
| }, 10); | ||
| const timer = setTimeout(() => done(false), ms); |
There was a problem hiding this comment.
Return a nonterminal/fenced outcome when outstanding work survives this timeout instead of letting drive() append its terminal event unconditionally. For example, an abandoned 60-second ctx.sleep reaches this five-second timeout, after which run.completed is recorded and lease renewal is stopped; the filesystem lease expires after 15 seconds, so another process can acquire the run before the original timer appends timer.fired and step.completed. The fresh evidence is that the new drain now detects pending waits but converts that detection to false after a fixed deadline without preventing the subsequent terminal append.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The facts check out — fs lease TTL is 15s (packages/store-fs/src/journal.ts:615) and the finally clears the renewal timer unconditionally — but I'm not making this change here, because it isn't a defect in the diff; it's the posture the whole file already takes.
shutdown() says it outright: "a step that ignores its abort signal past the drain window forfeits the release and its claim TTL-expires instead." cancel() does the same, and documents it. A bounded drain that gives up and proceeds is the existing contract in all three exits, not something this PR introduced — before it, drive() didn't wait at all.
The suggested remedy is a real semantic change, not a tightening. Withholding run.completed when the deadline expires means a workflow whose body completed successfully, whose output passed its schema, and whose patches all integrated, is never recorded as finished — it stays open for another owner to resume, purely because an unrelated abandoned timer is still armed. That trades a narrow ordering artifact for losing the durable record of a genuinely completed run, and it would apply to run.failed and run.cancelled too.
If the ordering artifact is worth closing, the cheaper lever is the other half of the same finally: keep renewing the claim while drained === false instead of stopping renewal, so nobody else can acquire the run while a zombie is still appending. That keeps the terminal record honest and closes the acquisition window, at the cost of holding a claim until the process exits. Worth a deliberate decision by @sergeyzenchenko either way — both options change how a wedged step ends a run, which is exactly the kind of thing the comments in this file were written to pin down.
Flagging one process point: this is the third review round on this hunk, and each fix has drawn a reshaped finding on the trade-offs of the drain rather than on a defect. The two concrete bugs from round two are fixed and tested; I've stopped pushing here so the remaining design calls stay with the author.
Generated by Claude Code
2 participants
This PR addresses multiple critical issues identified in the architecture review at commit 627b28e, focusing on replay correctness, git security, and ownership claim management.
Summary
Fixes several interrelated bugs in workflow replay, human gate handling, git command execution, and run lifecycle management that could cause incorrect behavior during resume operations or when running untrusted repositories.
Key Changes
Replay and Human Gate Safety:
keyparameter to human step options (HumanAskOptions) to allow explicit stable identity, mirroring agent step behaviorReplayIndex.matchHuman()to report ambiguity rather than guess when script changes make position unreliableGit Security Hardening:
safeGit()wrapper function that disablescore.fsmonitorand sets safety environment variables on all git calls in tree helperstreeHash()andintegrationBaseCommit()which rungit add -Aon every write-step dispatchIntegration and Patch Idempotency:
replay.divergeddetection when patch merge baseTree equals resultTree (signature of buggy re-journaling)Run Lifecycle and Ownership:
drainSteps()function to wait for in-flight steps to complete before releasing ownership claimrootOf()andtreeOf()helpers to properly handle parent-child run relationshipsRuntime Improvements:
checkIdle()to iterate over a copy of idle listeners, preventing skipped listeners when callbacks unregister during iteration.weft/.gitignorecreation to prevent run state (journals, blobs) from being committed to gitTest Coverage:
https://claude.ai/code/session_01P8RM3ozqRYTHt6NAqct175