feat(sdk,surface): journaled agent artifacts, artifact_exists gate, predicate gates - #449
Conversation
…redicate gates Three pieces that together make "the agent must have written this file" a journal-honest, replayable check for authored flows. Artifacts move into the worker. #434 measured them in the authored step, after the journal entry was written, so a gate could not read them and a resume could not reproduce them. `runAgentCli` (the process that spawns the CLI in its cwd) now snapshots that directory before the spawn and content- diffs it after, and `WorkerCliResult.artifacts` rides in the step's journaled `output` on the CliResult path. `AgentResult.artifacts` is read from that entry — no second scan, no guess about which host wrote what. Relay transport journals none (the agent ran elsewhere); an agent whose final message is a JSON object owns its output shape, which is left untouched, so such a step journals no artifacts either (documented; gate those on a deterministic check). `agent-artifacts.ts` and its unit tests are kept as-is. `artifact_exists` named gate: `{ type: 'artifact_exists', path }` end to end — surface `NamedGate`, SDK `NamedDataGate`, validation (`gate_path_invalid`: relative POSIX path, no empty/./.. segments, no NUL), lowering to a deterministic `<step>.gate` step whose command judges only `input.output.artifacts` from FLOWS_INPUT, `unknown_gate_kind` message, preflight refusal coverage, regenerated JSON schema. Predicate gates: `.gate(fn, because?)` was refused as `unsupported_gate` although SURFACE.md §6 specified running it as runtime control flow. The executor now runs the closure once, after the step, on the journaled value, and journals the verdict as a lowered `<step>.gate` deterministic step (`{"gate":"predicate","step","verdict","because"}`; exit 0/1), so resume and replay read the recorded verdict and never re-run author code. A false or throwing predicate fails the run as `gate_failed` naming the step and the reason; `flows run`/`resume` report it as a run failure, not a protocol one. One `.gate()` per step. The function is never serialized; `flows check` prints no gate line for it (runtime-only, by construction — documented). Tests: worker artifacts through a mocked spawn; gate validation, lowering (command exercised against FLOWS_INPUT), `flows check` inspection; #434's loopback integration adapted to the journaled contract; and a live test that invokes the built CLI with Cloud's exact argv (`run --json --data-dir … --local-agent x.flow.ts --input …`) against a real daemon and a wrapper CLI that writes `review/*.md`, asserting `output.artifacts` in the journal, both gate forms passing, and both negative cases failing the run with the gate named. Examples: pr-review-pipeline uses `artifact_exists` per lens and a predicate for consensus; examples/README no longer calls it BLOCKED on the budget header (accepted since #306). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (31)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 4 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| operation.predicateGate = { predicate: configOrPredicate, ...(_because === undefined ? {} : { because: _because }) }; | ||
| return step; |
There was a problem hiding this comment.
🔴 Helper predicate gates pass unchecked
A predicate on any helper, Slack, MCP, or plugin Step is stored but never evaluated. These gates pass when predicates return false or throw.
Learn more
Step<T> is the common surface returned by core steps and journaled effect helpers. This branch now accepts predicate gates for every AuthoredFlowOperation, but only the run, llm, and agent start closures invoke applyPredicateGate. The Slack, generated helper, MCP, and plugin closures execute their effects and resolve without consulting predicateGate. The operation therefore fulfills even when its accepted predicate would reject the returned receipt.
Example: await f.slack.post(...).gate(() => false) records the Slack effect and returns successfully. The author expects gate_failed, but the predicate never runs.
Recommended fix: Apply the predicate-gate wrapper centrally inside AuthoredFlowOperation.begin, or wrap every operation-producing start closure consistently. Add runtime tests for false and throwing predicates on Slack, generated helpers, MCP, and plugins.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 5cb417a — applied centrally in begin, exactly as recommended; no operation can carry a gate that never runs. Covered by a live test where a false predicate on a non-agent step fails the run gate_failed.
| try { | ||
| verdict = gate.predicate(value) === true; | ||
| } catch (error) { | ||
| verdict = false; | ||
| detail = error instanceof Error ? error.message : String(error); | ||
| } | ||
| const record = JSON.stringify({ | ||
| gate: 'predicate', step: id, verdict: verdict ? 'pass' : 'fail', | ||
| ...(gate.because === undefined ? {} : { because: gate.because }), | ||
| ...(detail === undefined ? {} : { threw: detail }), | ||
| }); | ||
| const literal = `'${record.replaceAll("'", "'\\''")}'`; | ||
| const command = verdict ? `printf '%s' ${literal}` : `printf '%s' ${literal} >&2; exit 1`; |
There was a problem hiding this comment.
🔴 Predicate gates rerun on resume
After a predicate gate completes but before root completion, resume makes applyPredicateGate execute the predicate again. A changed verdict alters the child spec under its existing admission key, so the resumed root fails.
Learn more
A durable authored resume re-executes the pinned body and relies on stable child admission keys to reuse completed child runs. Here the closure runs before lowerDeterministic opens the child gate run. The gate command embeds the newly computed verdict and detail, so a different result changes the child spec while retaining the same admission key. Even a stable result repeats any side effects inside the predicate, contrary to the recorded-verdict contract.
Example: A predicate based on Date.now() passes and its gate run completes. The process dies before the authored root completes. On resume the predicate returns false, producing a failing command where the admission key already identifies the passing command; run.start rejects the spec drift.
Recommended fix: Persist the predicate verdict before it can be lost, then discover and consume that record on resume without invoking author code. The child gate identity and spec must be recoverable without recomputing the closure.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 5cb417a: the verdict is appended to the root run's predicate-gates stream before the gate run is opened. On resume the executor reads that stream first and reuses the recorded verdict for the step — the closure is never re-run and the gate spec is byte-identical under its admission key. Test reads the record back from the root run.
| const artifactRoot = mode === 'agent' ? (cwd ?? process.cwd()) : undefined; | ||
| const before = artifactRoot === undefined ? undefined : await snapshotWorkspaceFiles(artifactRoot); | ||
| const withArtifacts = async (result: WorkerCliResult): Promise<WorkerCliResult> => { | ||
| if (before === undefined || artifactRoot === undefined) return result; | ||
| return { ...result, artifacts: diffWorkspaceFiles(before, await snapshotWorkspaceFiles(artifactRoot)) }; | ||
| }; |
There was a problem hiding this comment.
🟡 Concurrent agents share artifacts
With AgentWorker capacity above one, runAgentCli snapshots a shared cwd around overlapping executions. Each result can include another agent's files, allowing the wrong artifact_exists gate to pass.
Learn more
Artifact attribution uses a process-wide directory snapshot, not writes associated with one child process. AgentWorker is public and accepts capacities above one, so two dispatched agents can run concurrently against the same cwd. Each after-snapshot includes all writes since its own before-snapshot, including the other agent's writes.
Example: Agents A and B start together in /repo. B writes review/security.md; A writes nothing. A's after-snapshot still differs at that path, so A journals it and A's artifact_exists gate passes.
Recommended fix: Serialize the full snapshot-execute-snapshot interval per canonical working directory, isolate each execution in a distinct workspace, or collect writes using per-process filesystem instrumentation. Add an overlapping capacity-2 test where only one agent writes the gated path.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 5cb417a: the snapshot-spawn-snapshot interval is serialized per canonical cwd inside runAgentCli, so overlapping agents in one directory are measured one at a time. Test starts two executions together where only the first writes; the second journals [].
| wrapperEnvironment(process.env), | ||
| wrapperLimits, | ||
| signal, | ||
| )), effectiveModel); | ||
| )), effectiveModel)); |
There was a problem hiding this comment.
🟡 Wrapper artifacts scan wrong directory
runAgentCli snapshots the requested cwd, but wrapper sessions still spawn in the worker process directory. Relative wrapper writes land elsewhere, so artifacts misses them and artifact_exists fails.
(Refers to this code)
Learn more
The direct provider branch passes cwd to spawnInvocation, so its subprocess and artifact scanner share a root. The wrapper branch passes no cwd to runWrapperSession, and executePinnedWrapper spawns with the inherited process directory. The scanner nevertheless uses the caller's requested cwd, so the journal does not describe the wrapper's actual writes.
Example: The worker runs from /service, while an authored agent requests cwd: '/repo'. A wrapper writes review.md relatively under /service; the scanner checks /repo and journals no artifact.
Recommended fix: Thread cwd through runWrapperSession and executePinnedWrapper, and set it on the wrapper spawn. Keep the scanner root identical to the effective subprocess working directory and test a wrapper with cwd !== process.cwd().
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 5cb417a — same change as the sibling thread: cwd reaches the wrapper spawn; scanner root and subprocess cwd are the same directory. Tested at the worker level with a real wrapper.
…or resume, isolate artifact intervals, run wrappers in cwd Review round on #449 (Cursor, Devin), each addressed: - A predicate accepted on a helper/MCP/plugin Step was stored and never evaluated. The applier now lives on the lifecycle and runs in `AuthoredFlowOperation.begin` for every operation kind; a runtime without an applier refuses the gate instead of skipping it. - On resume the closure re-ran and could change the gate step's spec under its admission key. The verdict is now appended to the root run's `predicate-gates` stream before the gate run opens; a resumed body finds the recorded verdict for that step and reuses it, never re-running author code, so the gate spec is identical. - Two agents in one cwd with worker capacity > 1 could attribute each other's writes. The snapshot-spawn-snapshot interval is now serialized per canonical working directory. - Wrapper sessions spawned in the worker's process directory while the scanner measured the requested `cwd`. `cwd` is threaded through `runWrapperSession`/`executePinnedWrapper` to the spawn. Tests: helper-step predicate failing the run; `predicate-gates` stream record read back from the root run; capacity-2 overlap attributing the write to the agent that made it; a real wrapper measured in the requested cwd. (`f.agent({ cwd })` is refused by the kernel spec today — unknown field — so that last one is a worker-level test.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review round addressed in 5cb417a (all 6 threads replied): predicate gates now apply to every operation via a lifecycle hook; verdicts are recorded on the root run's Full SDK suite on this head against a fresh kernel: 1802 passed, 0 real failures — two tests ( One pre-existing gap surfaced while testing: |
Review swarm: maintainabilityNo fresh transcript was produced for run |
Review swarm: historyNo fresh transcript was produced for run |
Review swarm: structureNo fresh transcript was produced for run |
Review swarm: FAILED
Cloud run: |
…ates on resume `recordedVerdict` assigned an empty map before its `stream.read` resolved, so a second `.gate(fn)` in flight at the same time (Promise.all) saw the map as empty, re-ran its closure and appended a second record — and a different verdict would have changed the lowered gate command under its existing admission key. The in-flight load promise is now what is memoized; every concurrent gate awaits the same read. Test: two parallel gates on a resumed root with slow-answered recorded verdicts — closures never called, nothing appended, both gate specs carry the recorded pass; red on the previous source, green now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the worker-side artifact journaling over #434's step-side snapshot (both landed on main meanwhile) and main's f.agent permissions option. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c38af4. Configure here.
…e `<step>.gate` runs and lowered named-gate steps The Bun/Node standalone verifier (`verifyAuthoredNodeResult`) required `journalSteps.length === N` from `complete-N` and every id to end in `-<n>`, so a predicate-gated flow — whose gate is journaled as its own `<step>.gate` child run — was refused as having no durable completion and the root terminalized `worker_error`. Missed locally because the Bun suite was skipped (Bun 1.3.14 vs pinned 1.4.0). Decision, per SURFACE.md §6: a gate is subordinate to the step it judges. A named gate lowers to `<step>.gate` INSIDE the step's spec and has never consumed an ordinal; the predicate gate keeps the same `<step>.gate` shape as a separate run only because the closure must run between the step's completion and the gate's. `complete-N` counts the operations the author wrote, so `.gate` ids are set aside from the count and contiguity check — not exempted from verification: every gate must name a claimed parent, cannot be the terminal, and is held to the same durable completion evidence as any other child (completed run, `done` state, `step.completed` success, spec named `<flow>/<id>`). While running the suite under a real Bun 1.4.0, a second pre-existing gap in the same verifier surfaced: it assumed one step per child spec, but a NAMED gate lowers a second `<id>.gate` step into that spec, so any named-gated authored step was refused too. The verifier now accepts `[step, step.gate]` and requires the gate step to have completed. Tests: unit (no Bun) — predicate gate accepted and verified; orphan gate, non-success gate, gate without durable completion, gate as terminal, and a count that includes gates all refused; named-gate spec shape accepted with a completed gate and refused otherwise or with a foreign second step. Runtime — new `authored-node-runtime` case runs a predicate-gated + named-gated flow through the standalone CLI and resumes it; the whole Bun suite passes under Bun 1.4.0 (`FLOWS_BUILD_BUN`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Makes "the agent must have written this file" a journal-honest, replayable check in authored flows. Three pieces, one contract: the worker that spawned the agent measures the artifacts and journals them; gates and
AgentResultread the journal, never the disk.1. Artifacts move into the worker (what changed vs #434)
#434 diffed the cwd inside the authored step, after the journal entry existed — so a gate couldn't read the list and a resume couldn't reproduce it. Now
runAgentCli(worker-cli.ts, the one process provably sharing the agent's filesystem) snapshots the cwd before the spawn, content-diffs after, andWorkerCliResult.artifactslands in the step's journaledoutput.authored-worker-step.tsreadsoutput.artifactsback; no second scan.agent-artifacts.ts+ its unit tests are unchanged; #434's loopback integration test is adapted to the journaled contract.Honest edges, documented in
SURFACE.mdandAgentResult: relay transport journals none (agent ran elsewhere); an agent whose final message is a JSON object owns its output shape — it is left byte-identical, so such a step journals no artifacts (gate it on a deterministic check). Injecting a key into author-owned JSON brokesummaryandjson_schemacontracts in the existing suite, so that path is deliberately untouched.2.
artifact_existsnamed gate.gate({ type: 'artifact_exists', path: 'review/security.md' })end to end: surfaceNamedGate, SDKNamedDataGate, validation (gate_path_invalid: relative POSIX, no empty/./..segments, no NUL), lowering to a deterministic<step>.gatestep whose command judges onlyinput.output.artifactsfromFLOWS_INPUT,unknown_gate_kindmessage, preflight coverage, regeneratedflows.schema.json.flows checkprints it as a kernelexit_codegate.3. Predicate gates, per SURFACE.md §6
.gate(fn, because?)was refusedunsupported_gatealthough §6 already specified running it as runtime control flow. The executor now runs the closure once, after the step, on the journaled value, and journals the verdict as a lowered<step>.gatedeterministic step — so resume/replay read the verdict and never re-run author code. False or throwing → run failsgate_failednaming the step and the reason (flows run/resumereport it as a run failure, notprotocol_error). One.gate()per step; the function is never serialized;flows checkprints no line for it (runtime-only by construction — documented, see "could not do" below).Examples/docs
examples/pr-review-pipelineusesartifact_existsper lens and a predicate for consensus (workspace annotations dropped — still refused, unchanged);examples/README.mdno longer says BLOCKED on the budget header (accepted since #306).docs/SURFACE.md§6 rewritten to describe what now happens.Evidence — journal of a real run (built CLI, real daemon,
--local-agent, wrapper CLI that writesreview/security.md)Test plan
tests/artifact-gates.test.ts(5): worker artifacts via mocked spawn (created + changed, dotdirs/node_modules skipped, empty list, none for llm); gate validation/refusal kinds; lowering with the command executed againstFLOWS_INPUT(present/absent/no-list/JSON-owned output);flows checkinspection.tests/agent-artifacts-live.test.ts(3): Cloud's exact argv through the built CLI + real daemon; assertsoutput.artifactsin the journal, both gate forms pass,artifact_existsmiss →step_failednamingagent-1.gate, predicate false →gate_failedwith the author reason and"verdict":"fail"journaled.authored-agent-artifacts.test.tsadapted;authored-flow.test.tsnow asserts predicate acceptance + one-gate-per-step;preflight.test.tscoversgate_path_invalid.packages/sdksuite against a freshly built kernel: 1799 passed, 0 failed (onecli-watch5 s timeout under load, passes alone;authored-node-runtimeskipped: Bun 1.3.14 vs pinned 1.4.0). Surface 34/34. Examples typecheck.packages/schemaparity: 76/77 —step-memory.flow.yamlhits bun's 5 s timeout, identically on an untouchedmaincheckout; pre-existing, not this change.🤖 Generated with Claude Code
Note
Medium Risk
Changes authored execution, worker output shape, gate lowering, and Node result verification—core run/resume/replay paths—with broad test coverage but non-trivial behavioral surface area.
Overview
Journal-honest agent file checks and predicate
.gate(fn)for authored flows: the worker that spawns the agent now measures cwd changes and journalsoutput.artifacts;AgentResultand gates read that journal entry instead of re-scanning disk.Adds
.gate({ type: 'artifact_exists', path })end to end (surface, schema, validation, lowering to a deterministic step that checksFLOWS_INPUT.output.artifacts). Predicate.gate(fn, because?)is no longerunsupported_gate: the closure runs once on the completed value, the verdict is appended to a rootpredicate-gatesstream and lowered as<step>.gate, and resume/replay reuse the recorded verdict. Failures surface asgate_failedon run/resume; one gate per step. Concurrent agent runs in the same cwd are serialized around the artifact snapshot window.Verifier and IPC treat
<step>.gatechildren as non-ordinal journal steps (named gates may live inside the parent spec).pr-review-pipelineis updated toartifact_exists+ predicate gates; docs/examples reflect runnable budget headers and §6 behavior.Reviewed by Cursor Bugbot for commit e2ac931. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Makes "the agent must have written this file" a journal-honest, replayable check: the worker that spawned the agent measures and journals artifacts, and gates read that journal, never the disk. Predicate
.gate(fn)gates were previously refused asunsupported_gate; they now run and their verdict is journaled, and the standalone result verifier accepts gate child runs so gated flows no longer fail verification.Artifacts and
artifact_existsAgentResult.artifactsreads that entry instead of re-scanning..gate({ type: 'artifact_exists', path })named gate lowers to a deterministic step that checks the journaled list, soflows checkcan preflight it.cwd, matching the directory that gets scanned.Predicate gates
<step>.gatestep so resume/replay never re-run author code.gate_failed, naming the step and the author's reason..gate()per step; the function is never serialized.complete-Nordinal; a named gate lowered inside a child spec must complete too.Written for commit e2ac931. Summary will update on new commits.