Repository navigation
Security & correctness hardening (P0 + P1) - #2
Merged
Merged
Conversation
…rub secrets The Claude executor only path-checked Write/Edit/NotebookEdit, so a Bash call could write anywhere on the worker; the containment test even asserted Bash was allowed. The check also used `startsWith(writeRoot)`, so a sibling directory `<workspace>-evil` passed. `workspace.access: readOnly` was declared in the template schema and plumbed to checkout but never enforced, so "read-only" templates still had full write + shell. `settingSources: ["project"]` loaded `.claude/settings.json` (hooks, permission overrides) and `.mcp.json` straight from the checked-out — untrusted — repository, and the SDK subprocess inherited the full worker environment including AGRIPPA_SECRET_KEY (the master key that decrypts every stored credential) and the datastore URLs, all reachable from a Bash `env`. The worker container also ran as root on a shared volume. Introduce one execution-isolation seam in executor-core (`evaluateToolCall`, `isWithin`, `buildScrubbedEnv`) that the SDK adapter must route every decision through, rather than reimplementing containment inline: - read-only workspaces deny shell and confine writes to `.agrippa/artifacts`; read-write workspaces allow shell (OS-sandboxed when available) and confine writes to the workspace, boundary-safe against the sibling-prefix bug; - `ToolPolicy.access` is now required and wired from the template workspace spec; - the adapter enables the SDK `sandbox` (bubblewrap, graceful when absent), passes `strictMcpConfig`, an explicit `skills` allowlist, and a scrubbed subprocess env that drops the master key and datastore URLs while keeping the Anthropic auth vars the SDK needs; - the worker strips repo-supplied `.claude`/`.mcp.json` after checkout so the project setting source can only load platform-controlled skills; - the worker image runs as the non-root `bun` user. Full Bash containment in read-write workspaces still relies on the OS sandbox / non-root worker — the static layer cannot bound arbitrary shell writes; this is called out for the execution-isolation deep module (docs/design/03).
The artifact store resolved an executor-controlled source path lexically and read it whole, following symlinks. An agent could `ln -s /proc/self/environ` (or another run's files under the shared /work volume) into `.agrippa/artifacts/`, and the linked bytes were ingested as this run's artifact and served by the download endpoint to any run viewer — a filesystem and secret disclosure. Resolve the source through realpath and require the real target to sit inside the workspace (reusing the isolation seam's `isWithin`); missing/broken sources yield no content instead of a zero-byte row. Adds the first worker-adapter tests: normal file stored, escaping symlink rejected, missing file is not an artifact.
… to the project Two cross-tenant authorization holes shared one root cause: the worker re-resolved mutable global resources that submission never fully authorized. - repoRef was only shape-validated (a UUID), and the worker loaded the repo connection by raw id with no project predicate. A member of project A could submit project B's repoConnectionId and the worker would clone B's private repo with B's stored token. Submission now rejects a repoConnectionId that is not owned by the project (verifyRepoRefs), and the worker loads the connection scoped to the run's projectId as defence in depth. - optional skills/MCP skipped the grant check at submit, and the worker resolved them from the global registry with the platform credential — a project with no GitHub grant still received the shared GitHub token when autoOpenPr enabled the optional server. Submission now pins an authorized resource manifest (required grants enforced, optional resources included only when granted) onto the run; the engine resolves skills/MCP only from that manifest, never the global registry. Ungranted optional resources are treated as unavailable, so the dependent step is skipped. Adds runs.resource_manifest (migration 0002) and regression tests: a cross-project repoConnectionId is refused, and an ungranted optional MCP server is never resolved even when it exists in the registry.
…ble approvals Run status changes, event-seq allocation, and approval decisions were spread across the API, engine, and worker and none were atomic: - run status was written with an id-only WHERE, so a late worker finalize could overwrite a concurrent cancellation; - event seq was max(seq)+1 seeded into an in-memory counter in both the API and the engine, so two writers could collide on the unique (run_id, seq) index; - approval decisions were check-then-update with no status='pending' guard, so a user decision and the expiry worker could overwrite each other; and the status was committed before the resume enqueue, so an enqueue failure stranded the run in waiting_approval forever (the sweeper only re-enqueued 'queued' runs). Introduce a run-lifecycle module owning all three as atomic operations: transitionRun (compare-and-swap on the expected `from`), appendRunEvent (seq allocated inside the INSERT, retried on the rare unique-index race), and decideApproval (CAS on status='pending'). The engine finalize bails when its CAS loses, so it never clobbers another terminal outcome; the API and expiry worker route approvals through decideApproval; and the sweeper now re-enqueues waiting_approval runs whose approval is already decided, recovering any lost resume enqueue. Adds lifecycle CAS/seq integration tests.
A worker that died mid-step left the step row 'running'; on resume the engine marked it failed and started the next attempt at previous+1. For a step with no template-level retry that made maxAttempts=1 and startAttempt=2, so the retry loop ran zero iterations and the engine proceeded with the step neither executed nor failed — silently skipped when its output was optional, or surfacing later as a spurious contract_violation when its artifact was required. The new attempt row also never carried the prior session id, so resumeSessionId was unreachable. Treat a crash as an interrupted attempt rather than a consumed retry: count crashed attempts (error.code='crashed') during initialize and add one extra attempt each, so a crashed no-retry step re-executes. Carry the crashed row's executorSessionId onto the recovery attempt so the executor resumes its session. The existing crash test only exercised run-tests (retry: 2), which masked this; adds a no-retry crash test asserting re-execution and session resume.
… resume The submit gate counted the current month's project usage while the engine's mid-run quota check summed all-time usage — two different windows for the same limit. Worse, the engine seeded the budget meter with this run's persisted spend AND subtracted that same spend inside the headroom it checked against, so on resume the run's own usage was counted twice and could trip the quota early (spend > limit - spend). And the headroom was snapshotted once at start, so concurrent runs each measured only their own increment and could jointly overspend. Scope the engine's headroom query to the current month (matching the submit gate), exclude the run's own rows (the meter already carries them), and re-read it at every step boundary via a new BudgetMeter.refreshQuota so a run reacts to other runs' spend instead of a stale snapshot. Adds a resume regression test proving a run under quota is not failed by double-counting, and corrects the usage.ts comment that overstated the old enforcement.
The patch-exclusion filter in the artifact instructions matched a substring
("(patch)") the generated lines never contain, so patch steps were told to
hand-write .agrippa/artifacts/patch. When the agent did, the executor collected
it as kind 'file', the engine saw the key already produced and skipped its own
git-diff generation, and the stored "patch" was the model's file rather than the
real diff. The executor also collected every file in the directory with a kind
guessed from the extension, so keys/kinds were never checked against the
contract, files left by earlier steps were re-emitted on later ones, and missing
files still created zero-byte artifact rows.
Scope collection to the step's declared artifacts: emit only contracted keys,
with the contracted kind, once each, skipping patch keys (the engine owns those)
and files from other steps. The engine additionally drops any artifact event
whose key is not in step.produces and refuses to store a source that produced no
bytes (so a required-but-missing artifact still fails the contract instead of
becoming an empty row). Adds an executor test for contract-scoped collection.
The events stream replayed Postgres history and only then subscribed to the bus. An event committed and published in the window between those two steps was in neither the replayed rows nor the (not-yet-active) subscription, so it surfaced live only at the terminal replay — a live viewer of a long-running run could miss an event for the run's entire duration, contradicting ADR-0007's gap-free guarantee. Subscribe before the initial replay so live events buffer while history is read, then flush the buffer deduped against the replay by seq. The no-bus DB-polling path is unchanged. Strengthens the SSE test to assert delivered seqs are unique and strictly increasing, guarding the dedup this reorder depends on.
Update the authoritative design docs, ADRs, manual, and changelog to match the P0/P1 fixes: - ADR-0009 records the three deep-module decisions (execution-isolation seam, authorized run-manifest, run-lifecycle module). - design/03 documents the isolation seam, env scrubbing, OS sandbox, repo-config stripping, and contract-scoped artifact collection; 04 documents repoRef ownership + the pinned manifest at submit, CAS transitions, DB-allocated event seq, crash-recovery of no-retry steps, the corrected quota window/double-count, approval CAS + recovery, and subscribe-before-replay SSE; 05 notes repoRef authorization and approval CAS; 09 replaces the false frontend-test claim with the tests that now exist (isolation, worker-adapter, lifecycle, quota-resume). - ARCHITECTURE.md gains invariants for the isolation seam, the trusted manifest, and atomic lifecycle mutations. - The bilingual manual notes that optional resources need a grant to be used and adds troubleshooting rows; CHANGELOG [Unreleased] summarizes the security and fixed entries.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds project-scoped repository authorization, pins authorized resources per run, centralizes execution isolation, hardens workspace and artifact containment, and introduces atomic run lifecycle operations for approvals, events, recovery, quotas, and SSE delivery. ChangesExecution security and lifecycle hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionAPI
participant Orchestration
participant Database
participant Worker
participant EventBus
Client->>ExecutionAPI: Submit task with repoRef and resources
ExecutionAPI->>Orchestration: Verify repo ownership and authorize resources
Orchestration->>Database: Persist run and resource_manifest
Worker->>Orchestration: Process pinned run
Orchestration->>Database: Append event with allocated seq
Orchestration->>EventBus: Publish event
Client->>ExecutionAPI: Subscribe to run events
ExecutionAPI->>Database: Replay events after cursor
EventBus-->>ExecutionAPI: Deliver live wake-up
ExecutionAPI-->>Client: Emit deduplicated ordered SSE frames
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orchestration/src/engine/engine.ts (1)
908-923: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTerminal status becomes visible before the terminal event, allowing SSE to close early.
packages/orchestration/src/engine/engine.ts#L908-L923: atomically persist the terminal CAS, metadata, and terminal event before publishing.apps/api/src/routes/execution.ts#L446-L467: do not terminate the stream based on status until terminal-event persistence is guaranteed.🤖 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 `@packages/orchestration/src/engine/engine.ts` around lines 908 - 923, The terminal completion flow around Engine’s transition, database update, and emit calls must persist the CAS status, terminal metadata, and terminal event atomically or as one guaranteed operation before exposing the terminal status, preventing SSE consumers from closing early. In packages/orchestration/src/engine/engine.ts lines 908-923, update the terminal persistence/event ordering or transaction boundary accordingly; in apps/api/src/routes/execution.ts lines 446-467, stop terminating the stream solely from observing terminal status and wait until the terminal event is guaranteed.
🤖 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.
Inline comments:
In `@apps/api/src/routes/execution.ts`:
- Around line 304-316: Update the approval flow around decideApproval so the
status CAS, approval.decided event, and audit record are persisted within one
database transaction, rolling back all three if any write fails. Publish the
event and enqueueRun only after the transaction commits; preserve the existing
conflict handling for approvals already decided.
In `@apps/worker/src/index.ts`:
- Around line 128-136: Update the stranded approvals query using the
runs/approvals selection to replace the innerJoin and non-pending filter with a
NOT EXISTS subquery that checks no pending approval remains for the run. Retain
the waiting_approval run-status filter and select each run only when all
associated approvals are decided.
In `@docs/design/04-execution-runtime.md`:
- Line 111: Update the execution runtime documentation around the reconnection
and replay description to remove the claim that there is no polling. State that
Redis is optional and that periodic database polling in bus-less deployments
preserves correctness, while retaining the existing explanation of replay and
subscription ordering.
- Around line 9-17: Update the execution flow description to state that the task
and run rows are committed before enqueueRun inserts the pg-boss job. Describe
enqueueing as a post-commit step with a mitigated dual-write window, and
document that the sweeper repairs jobs lost between commit and queue insertion;
remove the claim that both operations occur in one transaction or that no window
exists.
In `@docs/manual/en/04-administration.md`:
- Line 24: Update the Resources descriptions in
docs/manual/en/04-administration.md:24-24 and
docs/manual/zh-CN/04-administration.md:24-24 to state that unauthorized
resources are withheld, while skipping applies only to explicitly optional MCP
requirements. Preserve the existing explanation that required ungranted
resources fail submission, and make the Chinese wording equivalent to the
English clarification.
In `@infra/Dockerfile.worker`:
- Around line 25-26: Update the Dockerfile worker setup around the
`mkdir`/`chown` command so it grants `bun` ownership only of runtime storage
directories such as `/work/runs` and `/work/artifacts`; remove `/app` from the
recursive ownership change, and keep `/app` root-owned while retaining `USER
bun`.
In `@packages/executor-claude/src/executor.ts`:
- Around line 148-163: Update the artifact discovery loop around kindByKey to
construct each expected filename from its key and contracted kind, then emit
only when the directory entry exactly matches that filename. Remove
extension-stripping and emitted-key deduplication so files with alternate or
duplicate extensions are ignored rather than selected based on readdirSync
order; preserve the existing exclusion of patch artifacts and emitted artifact
shape.
- Around line 97-101: Ensure Bubblewrap is available in the worker image by
installing bwrap in infra/Dockerfile.worker, or change the sandbox configuration
in the executor initialization to fail when Bubblewrap is unavailable. Preserve
the existing graceful fallback only for environments where unsandboxed execution
is explicitly intended.
In `@packages/executor-core/src/isolation.ts`:
- Around line 67-80: The write-boundary validation in the WRITE_TOOLS path must
resolve filesystem symlinks before allowing a target. Update the target handling
around path.resolve and isWithin to canonicalize the existing parent (or reject
symlink components), then verify the canonical location remains within writeRoot
while preserving the existing denial behavior.
- Around line 105-109: Update looksSecret so it no longer exempts the entire
ANTHROPIC_ or CLAUDE_ namespaces. Allowlist only the exact SDK authentication
variable names required by the executor, then apply the existing secret-pattern
check to all other keys, including names such as CLAUDE_ADMIN_TOKEN and
ANTHROPIC_PRIVATE_KEY.
In `@packages/orchestration/src/engine/engine.ts`:
- Around line 671-685: Update persistExecutorEvent so artifact contract
validation occurs before the event is emitted or persisted, rejecting
uncontracted keys without exposing their contents or path. For accepted
artifacts, resolve the declared contract kind and emit the normalized event
using that kind, then continue storing it through the existing artifact path.
- Around line 152-168: Update the failed-claim branch in the run transition flow
around transition and current.status: when another worker has advanced the run
to running, stop this worker instead of assigning current.status and continuing
execution. Preserve terminal-status handling, and ensure resuming an
already-running run requires the existing explicit lease/claim mechanism before
execution proceeds.
In `@packages/orchestration/src/engine/run-lifecycle.ts`:
- Line 41: Update the self-transition branch in the transition-checking function
so from === to still queries the database and confirms the persisted row matches
the expected state before returning true. Preserve the compare-and-swap behavior
and return false when the database has moved to a different status.
---
Outside diff comments:
In `@packages/orchestration/src/engine/engine.ts`:
- Around line 908-923: The terminal completion flow around Engine’s transition,
database update, and emit calls must persist the CAS status, terminal metadata,
and terminal event atomically or as one guaranteed operation before exposing the
terminal status, preventing SSE consumers from closing early. In
packages/orchestration/src/engine/engine.ts lines 908-923, update the terminal
persistence/event ordering or transaction boundary accordingly; in
apps/api/src/routes/execution.ts lines 446-467, stop terminating the stream
solely from observing terminal status and wait until the terminal event is
guaranteed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e40b9d8-1d02-4bb4-9ca7-a4b660216598
📒 Files selected for processing (36)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/lib/usage.tsapps/api/src/routes/execution.tsapps/api/src/test/execution.integration.test.tsapps/worker/src/deps/artifacts.test.tsapps/worker/src/deps/artifacts.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0009-security-correctness-deep-modules.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/design/09-testing-and-ci.mddocs/manual/en/04-administration.mddocs/manual/en/06-operations.mddocs/manual/zh-CN/04-administration.mddocs/manual/zh-CN/06-operations.mdinfra/Dockerfile.workerpackages/db/drizzle/0002_run-resource-manifest.sqlpackages/db/drizzle/meta/0002_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/runs.tspackages/executor-claude/src/executor.test.tspackages/executor-claude/src/executor.tspackages/executor-core/src/budget.tspackages/executor-core/src/index.tspackages/executor-core/src/isolation.test.tspackages/executor-core/src/isolation.tspackages/executor-core/src/types.tspackages/orchestration/src/engine/deps.tspackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/run-lifecycle.tspackages/orchestration/src/index.tspackages/orchestration/src/resolve.ts
|
|
||
| - **Members** — add by email (the person must have an account), change roles, remove. A project always keeps at least one admin; the platform blocks demoting or removing the last one. | ||
| - **Resources** — the grant toggles per registry type. This is the gate: a template requirement that isn't granted here makes submission fail fast with a named error. | ||
| - **Resources** — the grant toggles per registry type. This is the gate: a template requirement that isn't granted here makes submission fail fast with a named error. **Optional** resources are also gated — an optional skill or MCP server that a template can use (for example the GitHub server behind an "open a PR" step) is only made available to the run when it's granted; without the grant that step is simply skipped rather than run with a shared credential. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align both manuals with actual optional-resource behavior.
docs/manual/en/04-administration.md#L24-L24: say unauthorized resources are withheld; skipping applies to explicit optional MCP requirements.docs/manual/zh-CN/04-administration.md#L24-L24: apply the equivalent clarification in Chinese.
📍 Affects 2 files
docs/manual/en/04-administration.md#L24-L24(this comment)docs/manual/zh-CN/04-administration.md#L24-L24
🤖 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 `@docs/manual/en/04-administration.md` at line 24, Update the Resources
descriptions in docs/manual/en/04-administration.md:24-24 and
docs/manual/zh-CN/04-administration.md:24-24 to state that unauthorized
resources are withheld, while skipping applies only to explicitly optional MCP
requirements. Preserve the existing explanation that required ungranted
resources fail submission, and make the Chinese wording equivalent to the
English clarification.
Comment on lines
+67
to
+80
| if (WRITE_TOOLS.has(toolName)) { | ||
| const target = (input.file_path ?? input.path ?? input.notebook_path) as string | undefined; | ||
| if (target === undefined) return { behavior: "allow" }; | ||
| const resolved = path.resolve(workspaceDir, target); | ||
| if (!isWithin(writeRoot, resolved)) { | ||
| return { | ||
| behavior: "deny", | ||
| message: `writes outside the run workspace are not permitted (${target})`, | ||
| }; | ||
| } | ||
| if ( | ||
| policy.access === "readOnly" && | ||
| !isWithin(path.join(writeRoot, ARTIFACT_SUBDIR), resolved) | ||
| ) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Resolve symlinks before allowing writes.
Lexical path.resolve() does not contain filesystem writes. A checked-out symlink such as workspace/link -> /app/templates makes Write(link/x) pass this check while modifying /app/templates/x.
Canonicalize the target’s existing parent and verify it remains inside writeRoot, or reject symlink components before execution.
🤖 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 `@packages/executor-core/src/isolation.ts` around lines 67 - 80, The
write-boundary validation in the WRITE_TOOLS path must resolve filesystem
symlinks before allowing a target. Update the target handling around
path.resolve and isWithin to canonicalize the existing parent (or reject symlink
components), then verify the canonical location remains within writeRoot while
preserving the existing denial behavior.
…le & artifact contract Follow-up to the code review on the hardening PR: - The stranded-approval sweeper matched *any* non-pending approval via an inner join, so a multi-checkpoint run with an earlier decided approval and a current pending one was re-enqueued every tick. Replace with a `not exists (… pending)` guard, extracted as run-lifecycle.findStrandedApprovalRuns with a regression test for the multi-approval case. - The approval decision, its `approval.decided` event, and the audit row now commit in one transaction (publish + enqueue after commit), so a partial write can't leave the timeline/audit missing a decision a retry would then skip. Adds a DbOrTx type so lifecycle/audit helpers can run inside a caller's transaction. - transitionRun no longer trusts a self-transition (`from === to`) blindly; it verifies the row is still in that status, and initialize now stops (rather than proceeding) when it loses the run-claim to another live worker, avoiding duplicate side effects. (A full lease for the running↔running overlap is noted as future work.) - Artifact collection matches the exact contracted filename (key + kind's extension), so a `json` artifact `report` is no longer satisfied by `report.md`.
…wnership, artifact events Second round of review follow-ups, all security-tagged: - Write containment was purely lexical, so a checked-out symlink component (workspace/link -> /app) let a Write escape. The executor now canonicalizes the real write target (realWriteContained) and denies a write that resolves outside the workspace; fail-closed on resolution errors. - The env scrubber exempted the whole ANTHROPIC_/CLAUDE_ namespace, so a stray ANTHROPIC_PRIVATE_KEY or CLAUDE_ADMIN_TOKEN would ride along. Replace the namespace exemption with an explicit allowlist of the SDK auth variables and apply the secret heuristic to everything else. - The worker Dockerfile chowned /app to bun, letting agent commands modify worker code/deps/templates and persist into later runs. Keep /app root-owned; only /work (runtime storage) is writable. Also install bubblewrap so the SDK command sandbox actually engages in the container instead of silently degrading. - Uncontracted artifact events were emitted to run_events/SSE before the contract check dropped the store, leaking their inline contents. Validate the contract before emit, and emit the normalized contract kind without the inline body (the content lives in the artifacts table).
…wording Review follow-ups on the docs: - design/04: the pg-boss job is enqueued after the task+run transaction commits, not inside it — describe the mitigated post-commit dual-write window (repaired by the sweeper) instead of claiming there is none. - design/04: drop the "No polling anywhere" line that contradicted the bus-less DB-polling fallback; state that Redis is optional and polling preserves correctness. - manual (both locales): an ungranted optional resource is withheld, not resolved with a shared credential; a step that requires it is skipped, one that merely could use it runs without it.
|
| Filename | Overview |
|---|---|
| packages/executor-core/src/isolation.ts | New module: lexical tool-call evaluation, symlink-following containment, environment scrubbing allowlist, and secret redactor. Core security seam is well-structured and comprehensively tested. |
| packages/executor-claude/src/executor.ts | Integrates isolation module: two-layer containment (lexical + symlink-following), env scrubbing, sandbox, artifact contract enforcement, and stale-file clearing. The symlink-following second layer uses writeRoot (workspace root) for both readOnly and readWrite modes — a readOnly symlink within the artifact dir pointing to source still passes (flagged in prior review thread, still open). |
| packages/orchestration/src/engine/run-lifecycle.ts | New module: CAS status transitions, DB-allocated seq via UPDATE…RETURNING, atomic finalizeRun transaction, decideApproval CAS, and stranded-approval sweeper query. The two-step UPDATE+INSERT in appendRunEvent is not wrapped in a transaction outside finalizeRun; a crash between them burns a seq number (gap in events), but SSE delivery via > cursor handles gaps correctly. |
| apps/api/src/routes/execution.ts | Approval decision path rewritten as an atomic transaction (decideApproval CAS + allocateRunEvent + audit), bus publish and enqueue moved post-commit. SSE handler corrected to subscribe-then-replay with bus as wake-up only. verifyRepoRefs added at submit. |
| packages/orchestration/src/engine/engine.ts | Major correctness work: crash recovery (extra attempt per crash, session resume), quota refreshed per step, manifest-scoped resource authorization, secret redaction of event payloads, finalizeRun atomic terminal transition, and RunClaimLost guard. |
| apps/worker/src/deps/artifacts.ts | Symlink-following containment via resolveContainedPath, size cap, empty-source guard, and byte-exact disk streaming for large/binary artifacts. First realpath(workspaceDir) call has no try/catch — a missing workspace throws unexpectedly rather than returning null. |
| apps/worker/src/deps/workspace.ts | Scopes repo-connection lookup to projectId (cross-tenant IDOR fix) and calls sanitizeWorkspace after clone to strip .claude/.mcp.json/.agrippa before any agent runs. |
| apps/worker/src/index.ts | markRunFailed now calls finalizeRun (terminal event appended, CAS from current status); approval expiry uses decideApproval CAS; sweeper gains findStrandedApprovalRuns. Bus publish on retry-exhaustion failure is still absent (terminal event is in run_events; SSE falls back to 2s polling). |
| packages/orchestration/src/resolve.ts | authorizeResources replaces verifyResourceGrants: required resources must be granted (submit fails otherwise), optional resources included only when granted. verifyRepoRefs validates every repoRef param against the submitting project. |
| packages/orchestration/src/engine/bus.ts | Subscription type gains a ready Promise that resolves once the transport is active; InProcessEventBus resolves immediately, RedisEventBus resolves on SUBSCRIBE acknowledgment — enabling subscribe-before-replay for gap-free SSE. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant API as API (execution.ts)
participant DB as PostgreSQL
participant Bus as EventBus (Redis/InProcess)
participant Worker as Worker (index.ts)
participant Engine as RunEngine (engine.ts)
participant Executor as ClaudeExecutor
Note over API,Executor: Submit path
API->>DB: verifyRepoRefs (scoped to projectId)
API->>DB: authorizeResources → resourceManifest
API->>DB: INSERT run (with resourceManifest pinned)
API->>Worker: enqueueRun(runId)
Note over Worker,Executor: Execution path
Worker->>Engine: executeRun(runId)
Engine->>DB: transitionRun CAS (queued→running)
Engine->>DB: sanitizeWorkspace + git clone (scoped by projectId)
loop Per step
Engine->>DB: refreshQuota (exclude own spend, month-scoped)
Engine->>Engine: authorizedSkillRefs / authorizedMcpRefs (manifest filter)
Engine->>Executor: buildRequest (scrubbed env, sandbox, strictMcpConfig)
Executor->>Executor: evaluateToolCall (lexical) + realContained (symlink-safe)
Executor-->>Engine: ExecutorEvent stream
Engine->>DB: appendRunEvent (UPDATE next_event_seq + INSERT)
Engine->>Bus: publish(seq, type, redacted payload)
end
Note over API,Bus: SSE streaming (subscribe-before-replay)
API->>Bus: "subscribe(runId, () => notify?.())"
Bus-->>API: subscription.ready
API->>DB: "replay() WHERE seq > cursor ORDER BY seq"
loop Until terminal
Bus-->>API: wake-up notification
API->>DB: replay() (ordered drain)
end
Note over API,DB: Approval decision (atomic)
API->>DB: "transaction { decideApproval CAS + appendRunEvent + audit }"
API->>Bus: publish(approval.decided)
API->>Worker: enqueueRun(runId)
Note over Worker,DB: Sweeper backstop
Worker->>DB: findStrandedApprovalRuns
Worker->>Worker: enqueueRun for each stranded run
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant API as API (execution.ts)
participant DB as PostgreSQL
participant Bus as EventBus (Redis/InProcess)
participant Worker as Worker (index.ts)
participant Engine as RunEngine (engine.ts)
participant Executor as ClaudeExecutor
Note over API,Executor: Submit path
API->>DB: verifyRepoRefs (scoped to projectId)
API->>DB: authorizeResources → resourceManifest
API->>DB: INSERT run (with resourceManifest pinned)
API->>Worker: enqueueRun(runId)
Note over Worker,Executor: Execution path
Worker->>Engine: executeRun(runId)
Engine->>DB: transitionRun CAS (queued→running)
Engine->>DB: sanitizeWorkspace + git clone (scoped by projectId)
loop Per step
Engine->>DB: refreshQuota (exclude own spend, month-scoped)
Engine->>Engine: authorizedSkillRefs / authorizedMcpRefs (manifest filter)
Engine->>Executor: buildRequest (scrubbed env, sandbox, strictMcpConfig)
Executor->>Executor: evaluateToolCall (lexical) + realContained (symlink-safe)
Executor-->>Engine: ExecutorEvent stream
Engine->>DB: appendRunEvent (UPDATE next_event_seq + INSERT)
Engine->>Bus: publish(seq, type, redacted payload)
end
Note over API,Bus: SSE streaming (subscribe-before-replay)
API->>Bus: "subscribe(runId, () => notify?.())"
Bus-->>API: subscription.ready
API->>DB: "replay() WHERE seq > cursor ORDER BY seq"
loop Until terminal
Bus-->>API: wake-up notification
API->>DB: replay() (ordered drain)
end
Note over API,DB: Approval decision (atomic)
API->>DB: "transaction { decideApproval CAS + appendRunEvent + audit }"
API->>Bus: publish(approval.decided)
API->>Worker: enqueueRun(runId)
Note over Worker,DB: Sweeper backstop
Worker->>DB: findStrandedApprovalRuns
Worker->>Worker: enqueueRun for each stranded run
Reviews (3): Last reviewed commit: "fix: guard the artifact-size env and mou..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e atomic Follow-up review (round 3) — the security & lifecycle enforcement half. Isolation seam (S1): - The tool policy only gated writes and shell, so Read/Grep/Glob could open any absolute path — /proc/self/environ (the kept ANTHROPIC_API_KEY), another run's /work/runs/<id>, the shared artifact store. evaluateToolCall now confines the read tools to the workspace too (with the same symlink-real check as writes); a read with no path still defaults to the workspace cwd. - Event payloads were persisted and streamed verbatim, so a secret the agent echoed reached run_events/SSE. A SecretRedactor (built from the env secret values + the run's resolved MCP tokens) now scrubs every event in emit. This makes the redaction the design doc already claimed real. Lifecycle seam (S2): - Event seq was max(seq)+1 with retry; inside the approval transaction the first unique violation aborted the tx, so the retry could never recover. Replace it with an atomic per-run counter (runs.next_event_seq, migration 0003 backfilled to max(seq)) allocated via UPDATE … RETURNING — collision-free and tx-safe. - finalize now commits the status CAS + finishedAt/totals + terminal event in one transaction and publishes to the bus post-commit, so a crash can't leave an unrepairable half-finalized run; it also re-checks cancelRequested so a late cancel wins over a success. - markRunFailed (retry-exhaustion) routes through transitionRun instead of an id-only update, so it can't clobber a concurrent transition.
…, SSE, compose Follow-up review (round 3) — the correctness & wiring half. Artifacts (S3): - A zero-byte source now fails the contract instead of passing it (the skip keyed on inline===null, so an empty file still produced a row). - The artifact dir is cleared before each agent-step attempt (new WorkspaceManager.clearArtifacts), so a failed attempt's stale file can't be re-collected as a later attempt's result. - `file`-kind artifacts are read as bytes and stored byte-exact on disk, not decoded as UTF-8 (which corrupted binary content). Resources & budget (S4): - `requires.skills` is now validated by the compiler and enforced by the engine: a step requiring an ungranted/unavailable skill is skipped, not run without it. - Resume rebuilds per-phase spend (token_usage joined to run_steps by phase), so per-phase budgets survive a crash instead of resetting. - scripts/backfill-manifest.ts backfills the resource manifest for non-terminal runs that predate migration 0002 (documented upgrade step). SSE (S5): - The stream awaits the subscription being live (RedisEventBus.subscribe now exposes a ready promise for the SUBSCRIBE ack) before replaying, closing the residual gap; and re-replays Postgres periodically to recover a dropped pub/sub message mid-run rather than only at terminal. Compose (S6): - The api service mounts /work (large-artifact downloads) and receives AGRIPPA_EXECUTOR (the API picks the executor at submit). Docs (ADR-0009, design 03/04, ARCHITECTURE, CHANGELOG) updated; the deferred architectural items (per-run container isolation, provider-key proxy, execution lease, quota reservations) are recorded as known limitations.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orchestration/src/engine/engine.ts (1)
307-325: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake hard-stop quota admission atomic.
Concurrent runs can read the same remaining headroom before either records usage, so both may proceed and collectively exceed the quota.
packages/orchestration/src/engine/engine.ts#L307-L325: introduce an atomic reservation or serialized quota debit instead of aggregate-only headroom calculation.packages/orchestration/src/engine/engine.ts#L363-L368: reserve the permitted spend when admitting the step rather than independently checking a snapshot.CHANGELOG.md#L20-L20: remove the “can't jointly overspend” guarantee unless atomic enforcement is implemented.🤖 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 `@packages/orchestration/src/engine/engine.ts` around lines 307 - 325, Make hard-stop quota admission atomic: replace the aggregate-only headroom logic in Engine.quotaHeadroom with an atomic reservation or serialized quota debit, and update the step-admission flow around Engine’s quota check to reserve permitted spend rather than rely on a snapshot. In packages/orchestration/src/engine/engine.ts lines 307-325 and 363-368, implement the enforcement; in CHANGELOG.md line 20, remove the “can't jointly overspend” guarantee unless the atomic enforcement is actually provided.
♻️ Duplicate comments (1)
packages/orchestration/src/engine/engine.ts (1)
184-198: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDo not treat
running → runningas an exclusive claim.A delivery that initially reads
runningpasses the self-transition assertion without acquiring ownership. Multiple redeliveries can therefore resume concurrently, duplicate side effects, and mark each other’s steps as crashed. Recovery needs an explicit worker lease/claim token.🤖 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 `@packages/orchestration/src/engine/engine.ts` around lines 184 - 198, Replace the self-transition-based ownership check in the run-resume flow around transition and RunClaimLost with an explicit worker lease/claim token. Ensure every resumed delivery atomically acquires or renews ownership before proceeding, including runs already in "running" status, and reject deliveries that fail to obtain the lease so concurrent redeliveries cannot execute side effects or alter one another’s steps.
🧹 Nitpick comments (1)
packages/orchestration/src/engine/bus.ts (1)
39-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDelete empty listener sets during unsubscribe.
The current callback leaves one empty
Setper historical run ID, causing unbounded map growth in long-lived in-process workers.Proposed fix
- return { unsubscribe: () => set.delete(listener), ready: Promise.resolve() }; + return { + unsubscribe: () => { + set.delete(listener); + if (set.size === 0) this.listeners.delete(runId); + }, + ready: Promise.resolve(), + };🤖 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 `@packages/orchestration/src/engine/bus.ts` around lines 39 - 43, Update the unsubscribe callback returned by subscribe so it removes the listener and deletes the corresponding runId entry from this.listeners when the set becomes empty. Preserve the existing listener removal behavior and retain the set while other listeners remain subscribed.
🤖 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.
Inline comments:
In `@apps/worker/src/index.ts`:
- Around line 111-120: Update the retry-exhaustion transaction in the worker
flow around transitionRun to append the corresponding run.failed terminal event
before the transaction completes, using the same event persistence pattern as
RunEngine.finalize. Publish the event only after the transaction commits,
preserving the existing failed status and error update.
In `@packages/executor-core/src/isolation.ts`:
- Around line 188-210: Update buildScrubbedEnv to construct the child
environment from an explicit allowlist rather than copying variables that pass
SECRET_ENV_KEYS and looksSecret. Include only the required system variables and
exact executor configuration/authentication keys, preserving SDK_AUTH_ALLOW
entries while excluding unlisted credentials, DSNs, and execution-control
variables such as NODE_OPTIONS.
In `@packages/orchestration/src/engine/engine.ts`:
- Around line 487-490: Update the attempt flow around runAgentStep to stage
discovered artifacts privately rather than publishing them immediately. Defer
artifact events, database inserts, and producedArtifacts updates until the
attempt has completed successfully; validate that sources are non-empty before
emitting or storing anything. On failure, discard the staged artifacts so
retries cannot reuse them or satisfy required-output validation.
In `@scripts/backfill-manifest.ts`:
- Around line 41-49: Update the backfill logic around template and
db.update(runs) to derive resourceManifest through the same project-scoped
authorization path used during submission. Use the run’s project grants to
filter or reject unauthorized MCP servers and skills before persisting the
manifest, while preserving the existing authorized reference format.
- Around line 33-34: Update the filtering logic in the backfill loop to handle
legacy resourceManifest values of {} before accessing mcpServers.length or
skills.length. Treat missing mcpServers and skills as empty collections, while
preserving the existing continue behavior for manifests containing either
resource type.
---
Outside diff comments:
In `@packages/orchestration/src/engine/engine.ts`:
- Around line 307-325: Make hard-stop quota admission atomic: replace the
aggregate-only headroom logic in Engine.quotaHeadroom with an atomic reservation
or serialized quota debit, and update the step-admission flow around Engine’s
quota check to reserve permitted spend rather than rely on a snapshot. In
packages/orchestration/src/engine/engine.ts lines 307-325 and 363-368, implement
the enforcement; in CHANGELOG.md line 20, remove the “can't jointly overspend”
guarantee unless the atomic enforcement is actually provided.
---
Duplicate comments:
In `@packages/orchestration/src/engine/engine.ts`:
- Around line 184-198: Replace the self-transition-based ownership check in the
run-resume flow around transition and RunClaimLost with an explicit worker
lease/claim token. Ensure every resumed delivery atomically acquires or renews
ownership before proceeding, including runs already in "running" status, and
reject deliveries that fail to obtain the lease so concurrent redeliveries
cannot execute side effects or alter one another’s steps.
---
Nitpick comments:
In `@packages/orchestration/src/engine/bus.ts`:
- Around line 39-43: Update the unsubscribe callback returned by subscribe so it
removes the listener and deletes the corresponding runId entry from
this.listeners when the set becomes empty. Preserve the existing listener
removal behavior and retain the set while other listeners remain subscribed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dd5052e-91a0-44c7-a8df-744ab550a706
📒 Files selected for processing (34)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/lib/audit.tsapps/api/src/routes/execution.tsapps/worker/src/deps/artifacts.test.tsapps/worker/src/deps/artifacts.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0009-security-correctness-deep-modules.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mddocs/manual/en/04-administration.mddocs/manual/zh-CN/04-administration.mdinfra/Dockerfile.workerinfra/docker-compose.ymlpackages/db/drizzle/0003_run-next-event-seq.sqlpackages/db/drizzle/meta/0003_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/client.tspackages/db/src/schema/runs.tspackages/executor-claude/src/executor.test.tspackages/executor-claude/src/executor.tspackages/executor-core/src/isolation.test.tspackages/executor-core/src/isolation.tspackages/orchestration/src/compile.test.tspackages/orchestration/src/compile.tspackages/orchestration/src/engine/bus.tspackages/orchestration/src/engine/deps.tspackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/fakes.tspackages/orchestration/src/engine/redis-bus.tspackages/orchestration/src/engine/run-lifecycle.tsscripts/backfill-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/manual/en/04-administration.md
- docs/adr/0009-security-correctness-deep-modules.md
- packages/orchestration/src/engine/deps.ts
- infra/Dockerfile.worker
- packages/db/src/schema/runs.ts
- apps/worker/src/deps/artifacts.ts
- apps/worker/src/deps/workspace.ts
- packages/db/drizzle/meta/_journal.json
- ARCHITECTURE.md
- packages/executor-claude/src/executor.ts
- packages/executor-claude/src/executor.test.ts
- docs/design/04-execution-runtime.md
- packages/orchestration/src/engine/engine.integration.test.ts
- docs/design/03-executor-abstraction.md
- apps/api/src/routes/execution.ts
Comment on lines
487
to
490
| // start each attempt from a clean artifact dir so a prior attempt's | ||
| // stale file isn't collected as this attempt's result | ||
| await this.deps.workspace.clearArtifacts(this.run.id); | ||
| const output = await this.runAgentStep(phase, step, row, attempt); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Commit artifacts only after the attempt succeeds.
Artifact events, database rows, and producedArtifacts are updated before the terminal step result. If the attempt subsequently fails, its artifacts survive retries and can satisfy the required-output contract. Empty sources also leave a persisted artifact event because emission precedes storage validation.
Stage artifacts per attempt, then publish/insert/mark them produced only when that attempt completes successfully.
Also applies to: 718-745, 769-772
🤖 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 `@packages/orchestration/src/engine/engine.ts` around lines 487 - 490, Update
the attempt flow around runAgentStep to stage discovered artifacts privately
rather than publishing them immediately. Defer artifact events, database
inserts, and producedArtifacts updates until the attempt has completed
successfully; validate that sources are non-empty before emitting or storing
anything. On failure, discard the staged artifacts so retries cannot reuse them
or satisfy required-output validation.
Round 4 (Codex re-review) — commit 1 of 2: the critical data-loss bug and the finalization races, both by deepening the seams rather than patching around them. Artifact-attempt (R4-A): - CRITICAL: `clearArtifacts` did a recursive `rm` of `<run>/.agrippa/artifacts`, and `.agrippa` wasn't stripped at checkout — a committed `.agrippa -> /work` symlink turned that into deletion of the shared artifact store (Codex reproduced it). Strip `.agrippa` at checkout, remove the whole-dir clear entirely, and instead have the executor delete only *this step's* expected files at attempt start — refusing to act if the artifact dir resolves outside the workspace (an agent-created symlink). Fixes the stale-attempt bug without cross-step deletion or the escape. - `DiskArtifactStore` stats the source before reading and rejects anything over a size cap, then streams accepted files to disk instead of buffering whole files into memory (OOM guard). Run-lifecycle (R4-B): - One `finalizeRun` in the lifecycle module owns terminal transitions: a single tx does the status CAS (optionally requiring cancel_requested=false), finishedAt/totals, and the terminal event. The engine's late-cancel check is now atomic (a cancel committed after the read still wins), and `markRunFailed` routes through it — so a queued run whose setup threw transitions queued→failed (now a legal transition) instead of throwing on an illegal one and stranding the run, and both paths always emit the terminal event.
…ose volumes
Round 4 (Codex re-review) — commit 2 of 2: resources, streaming, and wiring.
Resource-resolution (R4-C):
- `requires.skills` only checked authorization, but skill resolution *threw* when
no active version existed — so a step failed/retried instead of skipping. Give
`skills` the same `{ resolved, missing }` shape as `mcpServers`: an unavailable
required skill fails the step, an unavailable optional one is skipped.
- scripts/backfill-manifest.ts reconstructed the manifest from the full template
(over-granting) and couldn't tell a legacy row from a valid empty one. It now
reconstructs from project grants via authorizeResources (idempotent: a valid
empty run recomputes empty), skipping runs whose grant was since revoked.
SSE (R4-D):
- Direct bus delivery advanced a high-water cursor, so a dropped event followed
by a later one moved the cursor past the gap and skipped it forever (even on
Last-Event-ID reconnect). The bus is now only a wake-up: every event is
delivered by an ordered Postgres replay, so the cursor advances contiguously
and a dropped message is recovered by the next replay.
Compose (R4-E):
- Mounting one /work volume on the root API risked initializing it root-owned so
the bun worker couldn't write. Split into a worker-only `workspaces` volume and
a shared `artifacts` volume; the API runs as bun and mounts only `artifacts`,
so ownership is consistent whichever service creates the volume.
Docs (ADR-0009, design/04, CHANGELOG) updated.
…st backfill
Round 5 — the two open items from the PR review (CodeRabbit/Greptile); the
rest of their findings were already addressed in rounds 3–4.
- buildScrubbedEnv was a denylist, so any worker variable that evaded the secret
name heuristic reached the agent subprocess — including NODE_OPTIONS (code
injection) and future DSNs/credentials. Switch to an explicit allow-list: only
the SDK auth variables and a fixed set of system essentials (PATH/HOME/locale/
TLS-trust) pass through; everything else is dropped.
- backfill-manifest.ts guards against a hand-set `{}` manifest (optional-chained
length checks) and its comment no longer claims migration 0002 produces `{}`
(it backfills the full empty shape).
Artifact per-attempt staging (committing artifacts on the emitting event rather
than promoting them on step success) is deferred and documented in ADR-0009 —
it's moot with the current executors, which collect only on step.completed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/orchestration/src/engine/run-lifecycle.ts (1)
60-78: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake sequence allocation and event insertion atomic for
Dbcallers.Because this accepts
DbOrTx, a directDbcall can commit the increment on Line 63 before the insert fails. That permanently skips a sequence number and breaks contiguous SSE replay. Wrap both operations in one transaction or use a single writable CTE; add a regression covering insert rollback.🤖 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 `@packages/orchestration/src/engine/run-lifecycle.ts` around lines 60 - 78, Update appendRunEvent so the runs sequence increment and runEvents insertion execute atomically for both Db and DbOrTx callers, using a transaction or single writable CTE; ensure insertion failure rolls back the increment. Add a regression test covering rollback after insert failure and contiguous sequence allocation.packages/executor-claude/src/executor.ts (2)
123-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the workspace root when validating read targets.
writeRootis the contracted artifact directory in read-only mode, so this symlink check denies legitimate repository reads. UseworkspaceDirfor read tools andwriteRootfor write tools.Proposed fix
if ((isWriteTool(toolName) || isReadTool(toolName)) && target !== undefined) { const resolved = path.resolve(req.workspaceDir, target); - if (!(await realContained(req.toolPolicy.writeRoot, resolved))) { + const containmentRoot = isWriteTool(toolName) + ? req.toolPolicy.writeRoot + : req.workspaceDir; + if (!(await realContained(containmentRoot, resolved))) {🤖 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 `@packages/executor-claude/src/executor.ts` around lines 123 - 126, Update the containment-root selection in the record target validation around isWriteTool, isReadTool, and realContained: use req.toolPolicy.writeRoot for write tools and req.workspaceDir for read tools. Preserve the existing resolved target and symlink validation behavior.
209-223: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun artifact cleanup inside the executor’s
try/finally.
realpathSync()orrmSync()can throw here—for example, when an expected artifact path is a directory. Because cleanup currently precedes thetry, the executor emits no failure event and leaves the abort listener registered.🤖 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 `@packages/executor-claude/src/executor.ts` around lines 209 - 223, Move the clearExpectedArtifacts(req.workspaceDir, req.expectedArtifacts) call into the executor’s existing try/finally scope, while preserving its current pre-execution ordering. Ensure cleanup exceptions flow through the existing failure-event handling and the finally block removes the abort listener.scripts/backfill-manifest.ts (1)
43-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the backfill to provably legacy runs.
An empty manifest is also a valid pinned result for a new run. During a live upgrade, this script can recompute such a run from newer grants and add optional resources that were unauthorized at submission. Select rows using a migration cutoff/legacy marker, or require submissions to be quiesced while this runs.
🤖 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 `@scripts/backfill-manifest.ts` around lines 43 - 56, Restrict the backfill loop around authorizeResources to runs provably created before the migration, using the existing migration cutoff or legacy marker in the run-selection query. Do not treat an empty resourceManifest as sufficient eligibility, so newly submitted runs retain their pinned empty result during live upgrades.
🤖 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.
Inline comments:
In `@apps/worker/src/deps/artifacts.ts`:
- Around line 10-12: Update maxArtifactSize to parse AGRIPPA_MAX_ARTIFACT_BYTES
once and reject invalid configuration values, including non-positive,
non-finite, and non-integer results. Preserve the existing 25 MiB default for an
unset variable, and ensure callers always receive a valid positive integer so
artifact-size comparisons remain enforced.
In `@infra/docker-compose.yml`:
- Around line 22-25: Update the API service’s artifacts volume mount to use
read-only mode, preserving the existing shared artifacts path while preventing
the API container from modifying or deleting worker-produced artifacts.
---
Outside diff comments:
In `@packages/executor-claude/src/executor.ts`:
- Around line 123-126: Update the containment-root selection in the record
target validation around isWriteTool, isReadTool, and realContained: use
req.toolPolicy.writeRoot for write tools and req.workspaceDir for read tools.
Preserve the existing resolved target and symlink validation behavior.
- Around line 209-223: Move the clearExpectedArtifacts(req.workspaceDir,
req.expectedArtifacts) call into the executor’s existing try/finally scope,
while preserving its current pre-execution ordering. Ensure cleanup exceptions
flow through the existing failure-event handling and the finally block removes
the abort listener.
In `@packages/orchestration/src/engine/run-lifecycle.ts`:
- Around line 60-78: Update appendRunEvent so the runs sequence increment and
runEvents insertion execute atomically for both Db and DbOrTx callers, using a
transaction or single writable CTE; ensure insertion failure rolls back the
increment. Add a regression test covering rollback after insert failure and
contiguous sequence allocation.
In `@scripts/backfill-manifest.ts`:
- Around line 43-56: Restrict the backfill loop around authorizeResources to
runs provably created before the migration, using the existing migration cutoff
or legacy marker in the run-selection query. Do not treat an empty
resourceManifest as sufficient eligibility, so newly submitted runs retain their
pinned empty result during live upgrades.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51dcda1a-076e-4363-8974-f545d42d08c6
📒 Files selected for processing (23)
CHANGELOG.mdapps/api/src/routes/execution.tsapps/worker/src/deps/artifacts.test.tsapps/worker/src/deps/artifacts.tsapps/worker/src/deps/resources.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0009-security-correctness-deep-modules.mddocs/design/04-execution-runtime.mdinfra/Dockerfile.apiinfra/docker-compose.ymlpackages/core/src/run-status.test.tspackages/core/src/run-status.tspackages/executor-claude/src/executor.test.tspackages/executor-claude/src/executor.tspackages/executor-core/src/isolation.test.tspackages/executor-core/src/isolation.tspackages/orchestration/src/engine/deps.tspackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/fakes.tspackages/orchestration/src/engine/run-lifecycle.tsscripts/backfill-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/adr/0009-security-correctness-deep-modules.md
- apps/worker/src/deps/artifacts.test.ts
- apps/worker/src/index.ts
- packages/executor-core/src/isolation.ts
- packages/executor-core/src/isolation.test.ts
- docs/design/04-execution-runtime.md
- apps/api/src/routes/execution.ts
- packages/orchestration/src/engine/engine.ts
…d-only Two quick-wins from the latest PR review (CodeRabbit); the review's other threads were stale re-anchors of issues already fixed in rounds 3–5. - AGRIPPA_MAX_ARTIFACT_BYTES=invalid parsed to NaN, and `size > NaN` is always false, silently disabling the artifact size cap. Parse once and fall back to the default on any non-finite / non-positive value. - The API mounts the shared artifact volume but only reads it, so mount it read-only — a compromised API process can't modify or delete worker artifacts.
hutusi
added a commit
that referenced
this pull request
Jul 18, 2026
Pushes of PR merge commits fail the push-time commitlint step because "Merge" parses as the commit type (type-case + type-enum), leaving main red and skipping Test/Build — every PR merge (#2, #3, #4) hit this. The PR-range step already lints the branch's real commits; the merge subject is packaging metadata, so commitlint now ignores /^Merge: / (kept narrow so malformed types still fail).
This was referenced Jul 27, 2026
hutusi
added a commit
that referenced
this pull request
Jul 31, 2026
…, written-bytes digest Four findings from the second codex review of this branch. Three are fixed; the first is confirmed and documented as the accepted residual it restates. P1 (accepted, documented): the evidence digest is reachable from a read-write agent that recovers worker credentials via /proc/1/environ — same UID, same PID namespace, no functional inner sandbox under Docker, and Yama does not guard PTRACE_MODE_READ. Confirmed. No in-place hardening can close it: with DATABASE_URL an attacker forges approvals outright (the worker role must keep UPDATE on checkpoints for expiry), and overrides digests without UPDATE because resume replays artifact rows by createdAt. This is the documented container-is-the-boundary posture (design 08, Top Risks #2); comments, design docs, and the CHANGELOG now say "tamper-resistance within the posture, not a boundary" instead of implying agents cannot reach Postgres. Per-run isolation stays the M2 work (issue draft prepared). P1: worker_ok() counted any ready row with a fresh heartbeat, so a foreign container on the same database — debug docker run, out-of-band scale leftover, second stack, or an old container beating just before replacement — could satisfy the count while a replacement wedged. The query is now scoped to the hostnames of this fleet's running containers ({{.Config.Hostname}}, by construction not convention), and aliveness uses a sliding 90s window: the fixed post-deploy stamp let a single beat mask a later wedge for the rest of verification. The sweeper now beats first in its tick so a failing sweep cannot skip the heartbeat. P1: the worker's boot-time insert into worker_heartbeats raced the api's on-boot migration on exactly the deploy that ships the table; the resulting crash-loop moved RestartCount and rolled back a good deploy. The compose worker now gates on api health (depends_on service_healthy — the api is healthy only after migrate/seed/publish complete; mirrors the VM unit's /healthz ExecStartPre), and both `up -d` call sites are bounded by timeout 600, because the gate makes compose's wait open-ended when an api crash-loops (start_period resets per restart) and an unbounded hang would run to the Janus unit's SIGKILL with no rollback. P2: file-kind artifact digests were computed by re-reading the mutable source after the copy; the spill path now hashes and size-counts the exact byte stream being written in a single pass, enforcing the size cap mid-stream and deleting partial files on abort.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the two critical security groups and the high-severity correctness issues from the M1 code review, implemented as three enforceable deep modules rather than scattered patches (ADR-0009). Scope is P0 + P1; the P2/P3 items (single-chunk SPA, model-pricing null, template-validate 200, RBAC cancel, compose defects, DB constraints, release workflow, unimplemented-but-advertised API surface) are catalogued for a fast follow and are out of scope here.
Security (P0)
packages/executor-core/isolation.ts). Every write-capable tool including Bash is now contained (previously only Write/Edit/NotebookEdit were, and thestartsWithcheck admitted a sibling<workspace>-evilpath).workspace.access: readOnlyis enforced (deny shell, confine writes to.agrippa/artifacts) instead of being inert. The SDK subprocess runs with the masterAGRIPPA_SECRET_KEYand datastore URLs stripped from its environment, the OSsandboxenabled where available,strictMcpConfig, and repo-supplied.claude/.mcp.jsonremoved at checkout so a checked-out repo can't inject hooks or permission overrides. The worker image runs non-root.realpathand rejects escapes, closing a symlink disclosure (ln -s /proc/self/environ …) that could exfiltrate secrets or other runs' files via the download endpoint.repoConnectionIdnot owned by the project, and the worker loads connections project-scoped. An authorized-resource manifest is pinned onto the run at submit (required grants enforced, optional included only when granted); the engine resolves skills/MCP only from it, so an ungranted project can no longer receive the platform's global credential (e.g. the shared GitHub token).Correctness (P1)
run-lifecycle.ts): CAS status transitions, DB-allocated event seq, approval CAS onpending, and sweeper recovery of runs left paused by a lost resume enqueue.Verification
bun run check,bun test(111 pass against local Postgres — integration suites ran, not skipped),bun run templates:validate,bun run buildall green.repoConnectionIdrefused, ungranted optional MCP not resolved, no-retry crash re-execution + session resume, lifecycle CAS/seq, quota resume double-count, contract-scoped artifact collection, and SSE dedup.0002_run-resource-manifest(applies cleanly). Docs (docs/design/03/04/05/09, ADR-0009, ARCHITECTURE.md, bilingual manual) andCHANGELOG.mdupdated to match.Residual / follow-up
Full Bash containment in read-write workspaces still relies on the OS sandbox / non-root worker — the static layer can't bound arbitrary shell writes; container-level isolation remains future work. P2/P3 items above are not addressed here.
Summary by CodeRabbit