Repository navigation
Requirement delivery: agrippa/v2 slots, checkpoints & loops, Codex reviewer, platform PRs - #5
Merged
Merged
Conversation
The requirement-delivery workflow (ADR-0010) needs three contracts shared across the API, SPA, engine, and executors without breaking dependency direction: a static executor catalog (the API may never import executor packages, so capability/provider metadata lives in core and the worker asserts its registrations against it at boot), zod schemas for the two structured interaction artifacts (agent questions and review reports) that drive the new input/review-gate checkpoints, and the checkpoint kind/severity vocabulary plus respond/comment request schemas. StepExecutionRequest gains optional iteration/agentSlot so executors and scripted fakes can distinguish loop rounds and agent slots; optional fields keep every existing executor source-compatible.
Schema groundwork for the requirement-delivery workflow (ADR-0010): - approvals becomes checkpoints (data-preserving RENAME) and grows kind (approval|input|review-gate), iteration, and a response jsonb column so a decision can carry structured data (answers, findings dispositions) back into the run's expression context — the old table could only say approved/rejected. - run_steps and artifacts gain iteration (default 1) so loop rounds get distinct idempotency/attribution rows without changing any non-loop behavior; run_steps_uq now includes it. - runs gains agent_bindings (per-slot faber+executor) and work_branch (set by the upcoming git.branch system action). faber_id/executor_id stay as primary-slot denormalization for list/usage queries. - new run_comments table for the team discussion thread on a run. decideApproval/findStrandedApprovalRuns are renamed to their checkpoint equivalents across engine/api/worker (behavior unchanged); route paths keep their /approvals shape until the API slice lands.
…ded loops, SCM actions ADR-0006 deferred loops and richer interaction to 'an explicit agrippa/v2 decision'; the requirement-delivery workflow is that decision (ADR-0010). The format needs three constructs the linear v1 graph cannot express: a review→fix cycle, per-step agent routing (Claude implements, Codex reviews), and human decisions that carry data back into the run. Template format: v2 documents declare spec.agents (slots binding a faber + executor, submit-overridable), checkpoint *steps* (approval | input | review-gate — the latter two auto-pass on an empty json source artifact), bounded kind:loop nodes (static maxIterations keeps compiler validation total), and git.branch/git.push/pr.open system actions with interpolable 'with' config. New expression roots: checkpoints.<id> (decided response; same-loop forward refs resolve to the previous iteration) and artifacts.<key> (latest inline content). v1 stays a supported authoring format: a pure upgradeV1ToV2 maps it onto the same IR (one 'main' slot, phase approvals become checkpoint steps), applied at compile time and when loading stored compiled rows — no data migration, no behavior change for existing templates (the whole compliance suite passes through the upgrade path). Engine: per-slot executor/faber bindings resolved at pickup (empty run.agent_bindings falls back to the run's primary faber/executor); checkpoint steps reuse the pause mechanics (pending row, CAS to waiting_approval, job completes) but can now sit mid-phase and store a structured response that re-enters the expression context; loops derive their resume iteration from persisted (stepId, iteration) rows so crash recovery needs no new state; SCM actions run through a new optional EngineDeps.scm seam, with pr.open composing the PR body plus an explicit waiver section for review findings the team accepted. Events gain iteration and the approval.required/decided pair becomes checkpoint.required/decided; loop.iteration.started/completed/exhausted, branch.created/pushed, and pr.opened are new.
…s, scm The v2 constructs need the same executable-contract treatment ADR-0005 gave the executor boundary: a fixture template exercising the whole requirement-delivery spine (Q&A loop with auto-pass, plan approval, review-fix loop with fix-selected vs accept-remaining, exhaustion- continue publish gate, exhaustion-fail, platform branch/push/PR with retry) so any future engine change is pinned against it. FakeExecutor learns iteration-scoped script keys (stepId@iteration) so loop rounds can behave differently — findings in round 1, clean in round 2 — without changing existing scripts. Two engine fixes fell out of writing these tests: PR-body waivers now accumulate across review rounds (latest human decision per finding id wins) instead of reading only the final gate row, which — being the auto-pass — always carried zero accepted findings; and loop lifecycle events consult the event log so resumes through a finished loop don't re-emit loop.completed. Engine-side patch generation is now keyed to (step, iteration) so a loop's fix round re-diffs the workspace instead of being skipped because the key existed from the implement phase.
The requirement-delivery workflow's reviewer slot needs a second real agent engine, and ADR-0005's promise — a new executor implements one interface and passes the existing compliance semantics — gets its first outside test. The adapter wraps codex exec --json (JSONL event shapes pinned against codex-cli 0.145.0 by live probes; samples in the README) and maps thread/item/turn events onto the normalized ExecutorEvent stream: exactly one terminal event, usage split so the cached-inclusive input_tokens never double-charges cache reads, thread_id as the same-step resume handle. Isolation goes through the existing seams rather than around them (ADR-0009): the subprocess env uses the shared allow-list scrubber (OPENAI_API_KEY/CODEX_API_KEY join the provider-auth allow-list and the redaction values), and containment maps toolPolicy.access onto Codex's native OS sandbox (read-only / workspace-write, network off, approval_policy=never) because codex exec has no per-tool-call hook for evaluateToolCall — the enforcement matrix is documented for ADR-0011. Read-only steps (the reviewer) cannot write artifact files, so a json artifact is synthesized from the final message's fenced json block and validated by the shared interaction schema downstream. The file-based artifact convention moves from executor-claude into executor-core (artifacts.ts) and both adapters share it. Tests drive the executor against a fixture binary replaying canned JSONL — argv/sandbox/resume construction, event and usage mapping, prompt assembly, read-only synthesis, error normalization, SIGTERM-on-abort, and env scrubbing.
…emo executor GitScmService implements the engine's new scm seam: branch creation with checkout -B (idempotent across retries/crash-resume), push against a credential-injected URL that never lands in .git/config, and PR/MR creation via the GitHub or GitLab REST API from the project-scoped repo connection — the PR link is contract-required, so it cannot depend on an optional MCP server or agent behavior. The connection/credential loading moves out of GitWorkspaceManager into shared helpers both use. AGRIPPA_SCM=fake wires the in-memory fake for token-free demos. The codex executor registers only when the CLI probe succeeds and auth is configured, and every registered executor is asserted against EXECUTOR_CATALOG at boot — the catalog is what the API and compiler trust for capability checks, so drift must fail fast, not surface as runtime template errors. (The catalog's fake entry stops over-promising resume, which the demo executor never supported.) DemoExecutor becomes round-aware: question/review json artifacts report findings in round 1 and come back clean in round 2, so the checkpoint loops of the requirement-delivery template demo realistically.
…errides One respond path serves every checkpoint kind: the payload is validated against the pending row's kind and snapshot (required answers against the question list, finding ids against the report), and the stored response carries full finding objects — templates interpolate checkpoints.<id>.selectedFindings directly, so the expression language never needs filtering. request_changes is refused outside loops (there is nothing to send the agent back to; the outcome would silently read as a pass — the engine now stamps loopId into checkpoint payloads for exactly this guard). The decision CAS, checkpoint.decided event, and audit row commit in one transaction before the run re-enqueues; the legacy /approvals decide route and inbox stay as thin delegates until the SPA switches over. Run comments insert with their comment.added timeline event in the same transaction, so the SSE stream and the thread can never disagree; members write, viewers read. Task submit resolves every agent slot to a concrete faber + executor (resolveAgentBindings): template defaults — the v1-upgrade sentinel maps to the deployment default, preserving pre-slot behavior exactly — then user overrides on overridable slots, capability checks against the executor catalog, and per-slot provider-filtered model resolution (a codex reviewer without a granted openai model fails at submit with model_unresolvable, not mid-run). Bindings and slot-keyed model resolution freeze onto the run; retry copies them. Run detail embeds checkpoints with decider names and per-slot faber/executor metadata; the task-type detail exposes slot pickers' data.
…ai model seeds The flagship agrippa/v2 template: a requirement goes from natural language to a reviewed PR through two cooperating agent slots — the implementer (Forge on Claude Code) and the reviewer (the new Arbiter faber on Codex, an exacting persona that must defend every severity and treats an empty findings list as a valid verdict). Flow: platform checkout + deterministic work branch; a bounded clarify loop (agent asks structured questions with recommended answers, empty list auto-passes); a plan loop whose approval gate supports request-changes with the comment fed back into the revision (exhaustion fails the run — implementing an unapproved plan is never acceptable); implementation on the pre-created branch; a review-fix loop where the user picks fix-selected vs accept-remaining per round and zero findings auto-pass; and a publish phase whose extra sign-off appears only when the loop exhausted right after an un-re-reviewed fix, before the platform pushes and opens the PR with the plan and accepted-findings waivers in the body. Seeds: arbiter faber, the requirement-delivery template head + task type, and openai codex model rows (ids/pricing to verify at rollout — registry rows are admin-editable). The API fixture now grants anthropic-only models so the no-openai-model submit failure stays testable.
…ers, inbox The SPA catches up with the interaction model. Submitting a task with an agent-slot template shows per-slot pickers (faber persona + executor engine from the core catalog, prefilled with template defaults, sparse overrides only). The run detail page gains a conversational Timeline tab built entirely from the replayed SSE stream — phase headers with loop-round chips, streaming agent turns tagged with the slot's faber and executor, inline interaction cards, teammate comments, and a PR card at the end — with the raw activity feed kept as its own tab. CheckpointPanel replaces ApprovalPanel with kind-specific bodies: the approval card adds Request changes (loop checkpoints only, comment required — it feeds the revision); input checkpoints render the agent's question form with one-click 'use recommendation' fills; review gates render a findings table with severity badges where checked findings go to the fix round and unchecked ones are explicitly accepted (the confirm dialog spells out what is being waived). Decided checkpoints show who decided them and when. Comments post from a composer pinned under the timeline; they arrive back through the event stream, so every watcher sees them live. The approvals inbox becomes a kind-aware 'waiting on you' list wired to /checkpoints/pending, and the legacy /approvals routes and schema are removed now that nothing consumes them. Model-resolution display and the phase rail understand slot-keyed resolutions and loop iterations. resolveAgentBindings gains demo mode: when the deployment default executor is 'fake' (the documented token-free demo switch), every slot binds to it — a demo install must never silently route a slot to a real, key-consuming engine. Verified end-to-end by driving the real requirement-delivery template through the engine with the demo executor: Q&A round, plan gate, review-fix loop, and platform PR all behave.
…elivery ADR-0010 records the agrippa/v2 decision ADR-0006 explicitly deferred — agent slots, checkpoint steps, and bounded loops, including the deliberate loosening that lets v2 templates read checkpoint responses — and ADR-0011 records the Codex executor (with its enforcement matrix: what the native OS sandbox covers and where evaluateToolCall cannot reach) plus the platform-side git write-path and PR waiver policy. The living design docs move with the implementation: 02 documents the v2 format and the compiled-IR/upgrade story, 03 the Codex CLI mapping and multi-executor catalog, 04 the checkpoint/loop runtime semantics, 05 the respond/comments/inbox API surface and its error vocabulary, 06 the run-timeline UX, checkpoint cards, and agent pickers. ARCHITECTURE.md, the bilingual manual (concepts + running tasks, both locales in this commit per the i18n rule), the changelog, and the README front door are updated to match.
…kpoints The render-level smoke was broken twice over: it still called the sign-up endpoint that invite-only onboarding closed (403 on boot), and it drove approvals through the removed legacy routes. It now bootstraps the first admin via the CLI + signs in, responds through the checkpoint endpoints, and — new coverage — seeds a requirement-delivery run against a file:// connection to this working tree (AGRIPPA_SCM=fake), answering the Q&A round and approving the plan so the capture lands on the round-1 review gate: the timeline, findings table, comment thread, and agent pickers all render under the console-error gate. Waiting for the next checkpoint polls the checkpoint list rather than the run status, which stays waiting_approval across a resume and made the old status-poll return before the worker had moved on. The timeline drops the duplicate phase header a resume re-announces for the round it paused in — spotted in the captures.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
hutusi has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds agrippa/v2 requirement-delivery orchestration with bounded loops, typed checkpoints, per-slot agent bindings, Codex execution, platform SCM operations, checkpoint APIs, and a timeline-centered web interface. ChangesRequirement delivery workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SubmitTaskPage
participant ExecutionAPI
participant RunEngine
participant CodexExecutor
participant CheckpointAPI
participant GitScmService
SubmitTaskPage->>ExecutionAPI: submit task with agent overrides
ExecutionAPI->>RunEngine: enqueue resolved run
RunEngine->>CodexExecutor: execute bound reviewer step
CodexExecutor-->>RunEngine: stream events and artifacts
RunEngine->>CheckpointAPI: persist pending checkpoint
CheckpointAPI->>RunEngine: validate and decide response
RunEngine->>GitScmService: push branch and open pull request
Possibly related PRs
🚥 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/features/useRunEvents.ts (1)
40-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
branch.*events don't trigger a run-query refresh, despite being newly subscribed.
branch.created/branch.pushedare added to the SSE subscription list (L86-87) but are missing from the refresh-trigger condition here. Any run-detail UI relying on the invalidated["run", runId]query (e.g. branch/PR metadata) will not update when a branch is created or pushed — only on a subsequentcheckpoint.*/loop.*/pr.opened/comment.addedevent. The AI summary for this diff states the refresh conditions were updated to includebranch.created/branch.pushed, which the code here does not show.🐛 Proposed fix
if ( parsed.type.startsWith("run.") || parsed.type.startsWith("step.") || parsed.type.startsWith("phase.") || parsed.type.startsWith("checkpoint.") || parsed.type.startsWith("loop.") || + parsed.type.startsWith("branch.") || parsed.type === "comment.added" || parsed.type === "pr.opened" || parsed.type === "artifact" ) { refresh(); }🤖 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 `@apps/web/src/features/useRunEvents.ts` around lines 40 - 55, Update the refresh-trigger condition in onAnyEvent to include the branch.created and branch.pushed event types alongside the existing run, step, phase, checkpoint, loop, comment, PR, and artifact events, while preserving the current deduplication and refresh behavior.
🧹 Nitpick comments (2)
README.md (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the real-run SCM configuration.
This paragraph explains
AGRIPPA_SCM=fakebut does not tell users what provider credentials or repository connection setup is required for real branch/push/PR operations. Add the required configuration or link to the SCM deployment documentation.🤖 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 `@README.md` at line 43, Update the README setup paragraph around AGRIPPA_SCM to document the required real-run SCM provider configuration, including credentials and repository connection setup needed for branch, push, and PR operations. Preserve the existing fake-SCM explanation and link to the SCM deployment documentation if the full configuration is documented elsewhere.apps/api/src/routes/catalog.ts (1)
76-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
fabriquery — fetch active fabers once and derivefabriOptions.
activeFabri(Line 76) andfabriOptions(Lines 100-103) issue two separate round-trips tofabriwith the identicalstatus = 'active'predicate. Hoist the query above theifblock and derivefabriOptionsfrom it to drop one DB round-trip per task-type detail load.♻️ Proposed consolidation
let agents: Record<string, unknown> | null = null; + // selectable fabri for overridable slots (members submit; registry CRUD is admin-only) + const activeFabri = await db.select().from(fabri).where(eq(fabri.status, "active")); + const bySlug = new Map(activeFabri.map((f) => [f.slug, f])); if (template?.latestPublishedVersionId) { ... inputs = compiled.spec.inputs; budgets = compiled.spec.budgets; - const activeFabri = await db.select().from(fabri).where(eq(fabri.status, "active")); - const bySlug = new Map(activeFabri.map((f) => [f.slug, f])); agents = Object.fromEntries(- // selectable fabri for overridable slots (members submit; registry CRUD is admin-only) - const fabriOptions = await db - .select({ id: fabri.id, slug: fabri.slug, nameI18n: fabri.nameI18n, avatar: fabri.avatar }) - .from(fabri) - .where(eq(fabri.status, "active")); + const fabriOptions = activeFabri.map((f) => ({ + id: f.id, + slug: f.slug, + nameI18n: f.nameI18n, + avatar: f.avatar, + }));🤖 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 `@apps/api/src/routes/catalog.ts` around lines 76 - 103, Hoist the active fabri query before the conditional block so it runs once, retaining the full records needed by the existing bySlug map. Replace the separate fabriOptions database query with a projection of that shared activeFabri result containing only id, slug, nameI18n, and avatar, while preserving the existing status-filtered options behavior.
🤖 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/web/src/features/runs/CheckpointPanel.tsx`:
- Line 1: The review-gate checkpoint can deadlock because evidence failure
disables both fix and accept actions. Update FindingsTable to retain disabled
for fix gating and add acceptDisabled for accept gating, then in
CheckpointPanel’s review-gate branch pass busy-only disabled for fix and
busy-or-evidenceFailed acceptDisabled, keeping accept blocked while fix remains
available.
- Around line 121-138: Update the review-gate branch in CheckpointPanel so
evidenceFailed disables only the blind accept action while keeping the fix
action available, analogous to the approval rejection behavior. Adjust the
FindingsTable action-specific disabled handling or props around onFix and
onAccept, preserving busy-state disabling and ensuring Accept remains blocked
when evidence loading fails.
In `@apps/web/src/features/runs/FindingsTable.tsx`:
- Around line 32-44: Split FindingsTable’s single disabled prop into separate
disabled states for the onFix and onAccept actions. Update the button usages in
FindingsTable and all callers such as CheckpointPanel so evidenceFailed disables
only the fix action while the accept/waiver action remains available; preserve
existing behavior for other disable conditions.
In `@apps/web/src/features/runs/QuestionsForm.tsx`:
- Around line 39-42: Update the Accept All Recommendations flow around
recommendable and its fill loop so required questions without a recommended
value remain missing. After applying recommendations, recompute or otherwise
validate the remaining required unanswered questions, and disable or prevent
submission when any remain, matching the primary Submit button’s missing-field
behavior.
In `@apps/web/src/features/runs/RunTimeline.tsx`:
- Around line 330-333: Update the auto-scroll useEffect in RunTimeline to
include itemCount in its dependency array so endRef scrolls into view whenever
streamed timeline items are appended; preserve the existing nearest-block scroll
behavior.
In `@apps/worker/src/deps/scm.ts`:
- Around line 29-80: Make openPullRequest resume-safe by checking for an
existing open head-to-base PR/MR before creating one, reusing its URL when
found. Add provider-specific lookup logic alongside openGithubPr and
openGitlabMr, and have both lookup paths use the same credentials, repository,
head, and base from PullRequestSpec; only POST when no matching open change
exists.
In `@ARCHITECTURE.md`:
- Line 30: Update the apps/api architecture entry to replace the stale
“approvals” terminology with “checkpoints,” including comments if that is part
of the exposed API surface, while preserving the rest of the listed
responsibilities.
- Line 7: Update the adjacent executor/system diagram in ARCHITECTURE.md so its
engine-path label matches the overview and visibly includes the OpenAI Codex CLI
alongside the Claude SDK; change only the diagram label or relevant diagram
node, preserving the existing architecture relationships.
In `@docs/adr/0011-codex-executor-and-platform-scm.md`:
- Around line 11-12: Update the Codex containment description around the
read-only reviewer claim to match the current reviewer behavior, which uses
workspace readWrite access. State the actual enforcement guarantee and remove or
qualify any assertion that reviewer-style steps cannot write artifacts, keeping
the existing unsupported per-tool-call enforcement caveat intact.
In `@docs/design/02-orchestration-template.md`:
- Around line 206-208: Enforce a non-empty comment for loop request_changes in
the shared approval validation/schema, then align the documented contract at
docs/design/02-orchestration-template.md:206-208,
docs/design/06-frontend.md:59-59, and docs/manual/en/02-concepts.md:21-21 so
each describes the enforced requirement and corresponding UI/API behavior.
In `@docs/design/05-api-and-auth.md`:
- Line 25: Update the “member” project-role capability description to qualify
request-changes as available only on loop checkpoints, while preserving the
other checkpoint actions and member permissions unchanged.
In `@docs/design/06-frontend.md`:
- Around line 73-75: Update the remaining frontend terminology in the design
document from “Approvals” to the checkpoint inbox wording, including sidebar and
dashboard navigation labels and route names. Keep the existing “Waiting on you”
terminology and checkpoint behavior unchanged.
In `@docs/manual/en/02-concepts.md`:
- Line 13: Update the “Agent slot & executor” documentation sentence to qualify
that slots are swappable only when the selected executor supports the step’s
required resources and the model is resolvable. Replace the unqualified “roles
are interchangeable by design” wording, and mention that Codex does not support
subagents, skills, or MCP.
In `@docs/manual/zh-CN/02-concepts.md`:
- Line 15: Update the “编排模板” definition to replace “人工审批节点” with “人工检查点,” and
explicitly mention the three supported checkpoint types: question-form,
review-gate, and approval. Keep the surrounding template definition unchanged.
In `@packages/db/src/seed/index.ts`:
- Around line 364-384: Remove the gpt-5.1-codex and gpt-5.1-codex-mini entries
from the seed model registry, or replace them with currently supported OpenAI
model IDs and verified pricing. Update the affected rows in the model seed
collection while preserving valid tier and context metadata for any
replacements.
In `@packages/executor-codex/src/executor.test.ts`:
- Around line 11-14: Update makeWorkspace to track each mkdtempSync directory,
then extend the existing afterEach hook to remove all tracked workspace paths
recursively and reset the tracking collection so every test cleans up its
temporary workspace.
In `@packages/executor-codex/src/executor.ts`:
- Around line 30-39: Update the read-only artifact synthesis in
collectStepArtifacts to reject multiple non-patch artifacts sharing the same
kind before assigning fallback content from finalMessage. Preserve the existing
single-artifact behavior, and return a clear error identifying the duplicated
artifact kind or keys instead of silently applying identical content.
In `@packages/orchestration/src/resolve.ts`:
- Around line 262-268: Update the slot resolution flow around resolveModelRoles
so each slot resolves only the roles used by its bound agent steps, rather than
the full compiled.spec.models set. Build the per-slot role set from the slot’s
bound agent steps, then pass that scoped role collection to resolveModelRoles
while preserving the existing providers constraint.
---
Outside diff comments:
In `@apps/web/src/features/useRunEvents.ts`:
- Around line 40-55: Update the refresh-trigger condition in onAnyEvent to
include the branch.created and branch.pushed event types alongside the existing
run, step, phase, checkpoint, loop, comment, PR, and artifact events, while
preserving the current deduplication and refresh behavior.
---
Nitpick comments:
In `@apps/api/src/routes/catalog.ts`:
- Around line 76-103: Hoist the active fabri query before the conditional block
so it runs once, retaining the full records needed by the existing bySlug map.
Replace the separate fabriOptions database query with a projection of that
shared activeFabri result containing only id, slug, nameI18n, and avatar, while
preserving the existing status-filtered options behavior.
In `@README.md`:
- Line 43: Update the README setup paragraph around AGRIPPA_SCM to document the
required real-run SCM provider configuration, including credentials and
repository connection setup needed for branch, push, and PR operations. Preserve
the existing fake-SCM explanation and link to the SCM deployment documentation
if the full configuration is documented elsewhere.
🪄 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 Plus
Run ID: 6166beae-b1ed-4e96-8b26-dc70262806fb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (83)
ARCHITECTURE.mdCHANGELOG.mdREADME.mdapps/api/src/routes/catalog.tsapps/api/src/routes/execution.tsapps/api/src/test/checkpoints.integration.test.tsapps/api/src/test/execution.integration.test.tsapps/web/src/components/shell/AppSidebar.tsxapps/web/src/components/submit/AgentSlotPicker.tsxapps/web/src/features/runs/ApprovalPanel.tsxapps/web/src/features/runs/CheckpointPanel.tsxapps/web/src/features/runs/FindingsTable.tsxapps/web/src/features/runs/PhaseTimeline.tsxapps/web/src/features/runs/QuestionsForm.tsxapps/web/src/features/runs/RunMetaCard.tsxapps/web/src/features/runs/RunTimeline.tsxapps/web/src/features/usePendingApprovals.tsapps/web/src/features/usePendingCheckpoints.tsapps/web/src/features/useRunEvents.tsapps/web/src/lib/types.tsapps/web/src/pages/ApprovalsPage.tsxapps/web/src/pages/DashboardPage.tsxapps/web/src/pages/RunDetailPage.tsxapps/web/src/pages/SubmitTaskPage.tsxapps/worker/package.jsonapps/worker/src/deps/demo-executor.tsapps/worker/src/deps/scm.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsapps/worker/tsconfig.jsondocs/adr/0010-agrippa-v2-slots-checkpoints-loops.mddocs/adr/0011-codex-executor-and-platform-scm.mddocs/design/02-orchestration-template.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/design/06-frontend.mddocs/manual/en/02-concepts.mddocs/manual/en/03-running-tasks.mddocs/manual/zh-CN/02-concepts.mddocs/manual/zh-CN/03-running-tasks.mdpackages/core/src/domain.tspackages/core/src/executors.tspackages/core/src/index.tspackages/core/src/interaction-schemas.tspackages/core/src/schemas.tspackages/db/drizzle/0005_requirement-delivery.sqlpackages/db/drizzle/meta/0005_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/runs.tspackages/db/src/seed/index.tspackages/executor-claude/src/executor.tspackages/executor-codex/README.mdpackages/executor-codex/package.jsonpackages/executor-codex/src/cli.tspackages/executor-codex/src/events.tspackages/executor-codex/src/executor.test.tspackages/executor-codex/src/executor.tspackages/executor-codex/src/index.tspackages/executor-codex/test/fixtures/fake-codex.tspackages/executor-codex/tsconfig.jsonpackages/executor-core/src/artifacts.tspackages/executor-core/src/fake-executor.tspackages/executor-core/src/index.tspackages/executor-core/src/isolation.tspackages/executor-core/src/types.tspackages/i18n/locales/en/catalog.jsonpackages/i18n/locales/en/runs.jsonpackages/i18n/locales/zh-CN/catalog.jsonpackages/i18n/locales/zh-CN/runs.jsonpackages/orchestration/src/compile.test.tspackages/orchestration/src/compile.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.tspackages/orchestration/src/resolve.tspackages/orchestration/src/template-schema.tsscripts/check-deps.tsscripts/screenshot.tstemplates/swdev/requirement-delivery.yamltsconfig.json
💤 Files with no reviewable changes (2)
- apps/web/src/features/runs/ApprovalPanel.tsx
- apps/web/src/features/usePendingApprovals.ts
Comment on lines
+262
to
+268
| const providers = entry?.providers ?? "*"; | ||
| modelResolution[slotId] = await resolveModelRoles( | ||
| db, | ||
| projectId, | ||
| compiled.spec.models, | ||
| providers, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which roles does each agent slot's steps actually use vs. all declared roles?
rg -nP -C3 'resolveModelRoles\(' packages/orchestration/src/resolve.ts
rg -nP -C2 '\brole\b' packages/orchestration/src/resolve.ts
# Inspect template role usage per slot
sed -n '50,209p' templates/swdev/requirement-delivery.yamlRepository: ainaive/agrippa
Length of output: 8200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolve.ts relevant ranges =="
sed -n '60,150p' packages/orchestration/src/resolve.ts
echo
sed -n '220,290p' packages/orchestration/src/resolve.ts
echo
echo "== templates files and reviewer binding usage =="
rg -n -C2 'slotId|resolveAgentBindings|agent_bindings|codex-cli|providers:\s*\[' templates swagger templates packages/orchestration -g '*.yaml' -g '*.yml' -g '*.ts' || true
echo
echo "== parse template roles and role references =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('templates/swdev/requirement-delivery.yaml')
text = p.read_text()
roles = []
for m in re.finditer(r'^( {6,}\S+):\s*$|^ (planning|coding|review):\s*{.*\n(?: .*|$)', text):
pass
for m in re.finditer(r'^ {6}(\w+):\s*{(.*)\n(?: {8}.*(?:$|\n)){1,2}', text):
role = m.group(1)
print(f"decl_role={role} first_line={m.group(0).replace(chr(10), ' '[:50])!r}")
refs = []
for i,line in enumerate(text.splitlines(),1):
m = re.search(r'model:\s*\{\s*role:\s*(\w+)', line)
if m:
refs.append((i, m.group(1), line.strip()))
for r in refs:
print(f"ref_line={r[0]} role={r[1]} line={r[2]!r}")
print('declared=', {m.group(1) for m in re.finditer(r'^ \s{6}(\w+):\s*{', text)})
print('references=', sorted(set(r[1] for r in refs)))
echo
echo "== resolveModelRoles implementation =="
rg -n -C8 "function resolveModelRoles|resolveModelRoles" packages/orchestration/src/resolve.ts
PYRepository: ainaive/agrippa
Length of output: 13634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all roles declarations and role references =="
python3 - <<'PY'
import re
from pathlib import Path
p = Path('templates/swdev/requirement-delivery.yaml')
text = p.read_text()
decl = {m.group(1): m.group(0).splitlines()[0] for m in re.finditer(r'^ {6}(\w+):\s*{(.*?)\n(?: {8}.*\n|\})', text)}
refs = []
for i, line in enumerate(text.splitlines(), 1):
m = re.search(r'model:\s*\{\s*role:\s*(\w+)', line)
if m:
refs.append((i, m.group(1), line.strip()))
print("declared_roles:")
for role in sorted(decl):
print(f" {role}: {decl[role]}")
print("template_role_refs:")
for line, role, txt in refs:
print(f" line {line} role={role}: {txt[:160]}")
print("declared_minus_refs:", sorted(set(decl) - set(r[1] for r in refs)))
print("refs_minus_decl": sorted(set(r[1] for r in refs) - set(decl)))
PY
echo
echo "== executor catalog codex-cli entry =="
rg -n -C4 'codex-cli' packages/orchestration -g '*.ts'
echo
echo "== model resolution tests related to roles/providers =="
rg -n -C4 'resolveModelRoles|model_resolution|planning|coding|review|provider' packages/orchestration/src packages/orchestration/test -g '*.ts' || true
echo
echo "== compile test for reviewer slot and executor =="
sed -n '160,230p' packages/orchestration/src/compile.test.tsRepository: ainaive/agrippa
Length of output: 359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
p = Path('templates/swdev/requirement-delivery.yaml')
text = p.read_text()
decl = {m.group(1): m.group(0).splitlines()[0] for m in re.finditer(r'^ {6}(\w+):\s*{(.*?)\n(?: {8}.*\n|\})', text, re.S)}
refs = []
for i, line in enumerate(text.splitlines(), 1):
m = re.search(r'model:\s*\{\s*role:\s*(\w+)', line)
if m:
refs.append((i, m.group(1), line.strip()))
print("declared_roles:")
for role in sorted(decl):
print(f" {role}: {decl[role]}")
print("template_role_refs:")
for line, role, txt in refs:
print(f" line {line} role={role}: {txt[:160]}")
print("declared_minus_refs:", sorted(set(decl) - set(r[1] for r in refs)))
print("refs_minus_decl:", sorted(set(r[1] for r in refs) - set(decl)))
PY
echo
echo "== executor catalog codex-cli entry =="
rg -n -C6 'codex-cli' packages/orchestration -g '*.ts'
echo
echo "== model resolution tests related to roles/providers =="
rg -n -C4 'resolveModelRoles|model_resolution|planning|coding|review|provider|model_unresolvable' packages/orchestration/src packages/orchestration/test -g '*.ts' || true
echo
echo "== compile test for reviewer slot and executor =="
sed -n '160,230p' packages/orchestration/src/compile.test.tsRepository: ainaive/agrippa
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
p = Path('templates/swdev/requirement-delivery.yaml')
text = p.read_text()
decl = {m.group(1): m.group(0).splitlines()[0] for m in re.finditer(r'^ {6}(\w+):\s*{(.*?)\n(?: {8}.*\n|\})', text, re.S)}
refs = []
for i, line in enumerate(text.splitlines(), 1):
m = re.search(r'model:\s*\{\s*role:\s*(\w+)', line)
if m:
refs.append((i, m.group(1), line.strip()))
print("declared_roles:")
for role in sorted(decl):
print(f" {role}: {decl[role]}")
print("template_role_refs:")
for line, role, txt in refs:
print(f" line {line} role={role}: {txt[:160]}")
print("declared_minus_refs:", sorted(set(decl) - set(r[1] for r in refs)))
print("refs_minus_decl:", sorted(set(r[1] for r in refs) - set(decl)))
PY
echo
echo "== executor catalog codex-cli entry =="
rg -n -C6 'codex-cli' packages/orchestration -g '*.ts'
echo
echo "== model resolution tests related to roles/providers =="
rg -n -C4 'resolveModelRoles|model_resolution|planning|coding|review|provider|model_unresolvable' packages/orchestration/src packages/orchestration/test -g '*.ts' || true
echo
echo "== compile test for reviewer.slot =="
sed -n '160,230p' packages/orchestration/src/compile.test.tsRepository: ainaive/agrippa
Length of output: 50371
Scope each slot’s model resolution to its used roles.
resolveModelRoles resolves every declared spec.roles entry while constrained by a single slot’s executor providers. The flagship test fixture matches this: reviewer binds to a fixed executor, yet the per-slot resolution stores the entire template model set; a provider-scoped executor will incorrectly fail on unused roles that belong to another slot. Build a per-slot role set from bound agent steps and resolve only those roles.
🤖 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/resolve.ts` around lines 262 - 268, Update the
slot resolution flow around resolveModelRoles so each slot resolves only the
roles used by its bound agent steps, rather than the full compiled.spec.models
set. Build the per-slot role set from the slot’s bound agent steps, then pass
that scoped role collection to resolveModelRoles while preserving the existing
providers constraint.
Contributor
Author
There was a problem hiding this comment.
Valid — acknowledged as a tracked follow-up (left unresolved on purpose): per-slot resolution should only cover the roles that slot's steps actually use, which needs the compiler to expose role→slot usage. Queued together with the other known follow-up (patch artifacts above the 64 KB inline cap failing the publish evidence check).
There was a problem hiding this comment.
@hutusi, acknowledged. I’ll leave this finding unresolved for the current PR. The follow-up should have the compiler expose role-to-slot usage so submit-time model resolution can be scoped to each slot’s bound agent steps.
Would you like me to open GitHub issues for this and the patch-evidence inline-size-cap follow-up?
✏️ Learnings added
Learnt from: hutusi
Repo: ainaive/agrippa PR: 5
File: packages/orchestration/src/resolve.ts:282-288
Timestamp: 2026-07-23T07:10:06.402Z
Learning: For the `agrippa/v2` requirement-delivery workflow, per-slot model resolution in `packages/orchestration/src/resolve.ts` should eventually resolve only model roles used by agent steps bound to that slot. This is intentionally deferred because the compiler must first expose role-to-slot usage. The related review finding remains unresolved in PR `#5`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…facts, unique branches Addresses PR #5 review findings 1, 2, 3, and 8 — they interlock (the schema tightening must ship with its enforcement, and branch uniqueness must land before pr.open dup-recovery can be added safely). Committed changes no longer vanish from patch artifacts: checkout pins the clone-time HEAD as refs/agrippa/base (a ref, not a marker file — gc-safe, cleanly testable) and diff() reports against it, so committed + staged + worktree changes all appear, with pathspec excludes keeping the sanitization deletions (.claude/.mcp.json/.agrippa) out of every patch. Old workspaces without the ref fall back to the previous worktree diff. The engine additionally fails the producing step — retryably, and before push/pr.open — when a required patch's auto-diff comes back empty. This also repairs the latent empty-patch bug in v1 bug-localize-fix with no template change. Malformed interaction artifacts no longer auto-pass their checkpoints: artifacts that drive an input/review-gate are validated against the shared schemas at store time, while the producing step's attempt is still open — so a broken review report fails the step with contract_violation and template retry/onFailure apply (the review step gains retry: {max: 1}). The checkpoint-time read becomes a strict backstop for resumed pre-fix rows: a review gate with an absent source fails the run (a gate without evidence must never pass), invalid or over-inline-limit sources fail with distinct messages, and only a genuinely empty questions/findings list still auto-passes. Question contracts are enforced end to end: select questions must carry options (a required select without options was an unanswerable form — input checkpoints have no reject, so the run deadlocked until timeout), recommendations must match the question kind (boolean allowed now), and the respond endpoint validates every answer against its snapshotted kind and options. Review-report caps tighten so typical reports stay inline-sized. Work branches stop colliding across tasks: run numbers restart per task, so the default branch name now appends the run id's random tail — the LAST hex chars of the UUIDv7; the first are timestamp bits shared by every run created in the same minute. The template drops its explicit name and uses the default. New real-git integration suite (temp repos, file:// remotes) covers the committed/staged/untracked diff, sanitization exclusion, branch+push, and the fallback path — the compliance suite's canned FakeWorkspace diff is exactly how finding 1 shipped unnoticed. Compliance additions: malformed report = retryable step failure that never becomes an artifact row, absent report = failed run with no PR, optionless select = failed producing step, and the default branch suffix.
…latest artifacts Addresses PR #5 review findings 4 and 5. Submission no longer accepts executors the deployment cannot run: the static core catalog says what CAN exist, and a new executor_registrations table (migration 0006) says what DOES — workers upsert their registered executor ids at boot and heartbeat them on the sweeper interval, and resolveAgentBindings rejects any FINAL resolved binding (template-pinned executors included, not just overrides) whose id has no recent registration, with executor_unavailable before model resolution so the user gets the more actionable of the two errors. An empty live set — fresh deployment, no worker booted yet — skips the check rather than blocking every submission. The task-type detail carries per-slot availability plus the live id list, and the submit page's executor picker disables engines this deployment lacks. Known limitation (documented): heterogeneous workers still race at pickup; per-executor queues are future work. Checkpoint panels stop presenting stale evidence: loop rounds re-produce the same artifact key, the artifacts endpoint returned rows unordered and without iteration, and the panel took the FIRST key match — after 'request changes' a responder could approve the previous round's plan or patch. The endpoint now orders by creation (id tiebreak) and exposes iteration, and the panel takes the last match — exact, because panels render only pending checkpoints and a paused run produces nothing while paused, so 'latest row per key' is precisely the responder's evidence.
Addresses PR #5 review findings 6 and 7, plus the documentation for the whole review round. pr.open survives retries: a lost provider response (or a crash between the API call and storing the URL) used to make the retry's POST fail with GitHub 422 / GitLab 409 even though the PR exists. Any 422/409 now triggers a lookup of the open PR by head/base — honoring the GHES api base — returning its URL when found and rethrowing the original error body when not (no message string-matching, no wasted lookup-first call). This is only safe because the previous commit made work branches unique per run; with the old colliding names the recovery could have returned another task's PR as this run's contract artifact. Unit-tested against a local fake forge scripting create → 422 → recover and 422 → nothing-to-recover; FakeScmService mirrors the semantics (same head/base re-open returns the same URL). Codex can no longer load capabilities the catalog says it lacks: every codex exec invocation passes --ignore-user-config (a user-level config.toml could register MCP servers, bypassing project resource governance despite mcp:false) and --ignore-rules (repo-shipped execpolicy rules are untrusted input). Auth still resolves from CODEX_HOME per the CLI docs. The boot probe now checks exec --help for the flags and refuses to register an older CLI — which would otherwise fail every step on 'unexpected argument', or worse, run unisolated if the flags were made conditional. Docs: changelog entry for the review round; design 03/04 record the executor-availability registry, the gate-without-evidence rule, and the branch-naming scheme; ADR-0011 gains a post-review addendum.
Found by re-running the browser harness after the review fixes: the empty-required-patch guard (correctly) fails a step whose diff is empty, and the demo executor never touched the workspace — so the delivery demo died at the implement step against a real checkout. A demo step that owes a patch now appends a line to AGRIPPA_DEMO.md in the workspace, keeping the guard meaningful and the demo diff honest.
There was a problem hiding this comment.
hutusi has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
Author
|
All eight review findings verified valid and addressed in
Verification: full gate green at each commit ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workspace.test.ts`:
- Around line 224-227: Await the final rejects assertion in the “422 with
nothing to recover” test so the test waits for scm.openPullRequest(runId, spec)
to reject with /422/. Keep the existing try/finally cleanup and assertion
behavior unchanged.
🪄 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 Plus
Run ID: d1e1edd8-b47e-48a2-a44e-e9853832feac
📒 Files selected for processing (36)
CHANGELOG.mdapps/api/src/lib/executors.tsapps/api/src/routes/catalog.tsapps/api/src/routes/execution.tsapps/api/src/test/checkpoints.integration.test.tsapps/web/src/components/submit/AgentSlotPicker.tsxapps/web/src/features/runs/CheckpointPanel.tsxapps/web/src/features/runs/QuestionsForm.tsxapps/web/src/lib/types.tsapps/web/src/pages/SubmitTaskPage.tsxapps/worker/src/deps/demo-executor.tsapps/worker/src/deps/scm.tsapps/worker/src/deps/workspace.test.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0011-codex-executor-and-platform-scm.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mdpackages/core/src/interaction-schemas.tspackages/db/drizzle/0006_executor-registrations.sqlpackages/db/drizzle/meta/0006_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/registry.tspackages/executor-codex/src/cli.tspackages/executor-codex/src/executor.test.tspackages/executor-codex/src/executor.tspackages/executor-codex/test/fixtures/fake-codex-cli.tspackages/i18n/locales/en/catalog.jsonpackages/i18n/locales/en/runs.jsonpackages/i18n/locales/zh-CN/catalog.jsonpackages/i18n/locales/zh-CN/runs.jsonpackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/fakes.tspackages/orchestration/src/resolve.tstemplates/swdev/requirement-delivery.yaml
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/i18n/locales/zh-CN/catalog.json
- packages/i18n/locales/en/catalog.json
- apps/web/src/features/runs/QuestionsForm.tsx
- apps/api/src/routes/catalog.ts
- packages/orchestration/src/engine/fakes.ts
- docs/adr/0011-codex-executor-and-platform-scm.md
- packages/i18n/locales/zh-CN/runs.json
- packages/i18n/locales/en/runs.json
- apps/web/src/features/runs/CheckpointPanel.tsx
- apps/worker/src/index.ts
- packages/executor-codex/src/executor.test.ts
- apps/api/src/test/checkpoints.integration.test.ts
- docs/design/03-executor-abstraction.md
- apps/web/src/pages/SubmitTaskPage.tsx
- docs/design/04-execution-runtime.md
- packages/executor-codex/src/executor.ts
- templates/swdev/requirement-delivery.yaml
- apps/worker/src/deps/scm.ts
- packages/orchestration/src/resolve.ts
- packages/orchestration/src/engine/engine.integration.test.ts
- apps/web/src/lib/types.ts
- apps/api/src/routes/execution.ts
- packages/orchestration/src/engine/engine.ts
hutusi
added a commit
that referenced
this pull request
Jul 23, 2026
…trict artifacts, 48-bit branches Addresses round-2 review findings 1, 2, 3, and 5 (PR #5). Findings 1 and 3 are mirror-image evidence-integrity holes the round-1 fixes introduced: the PR could silently DELETE sanitized files the patch never showed, and the patch could SHOW uncommitted work the push never shipped. The fix establishes one contract: the patch artifact and the pushed PR contain exactly the same changes, and the sanitized paths appear in neither, in either direction. Sanitized paths become invisible to git itself instead of being hidden from the diff: tracked .claude/.mcp.json/.agrippa entries get --skip-worktree before removal (so an agent's add -A/commit -a cannot ship the deletion — and reset --hard no longer casually restores repo-supplied .claude, an improvement over the plain rm), and the paths are appended to .git/info/exclude to cover the UNTRACKED side — which the platform itself populates (materialized skills under .claude/skills/, artifact scratch under .agrippa/artifacts/) and which skip-worktree alone cannot reach. The diff pathspec excludes are then dropped entirely. Push gains a platform finalizing commit: add -A (now safe by the above), staged-change detection via status --porcelain (diff --cached --quiet exits 1 into the throwing git helper), and a commit under an Agrippa identity with host gitconfig isolated (gpgsign/hooksPath must not leak). A branch whose HEAD still equals the clone base fails with 'nothing to publish' instead of an opaque provider 422 later. And because steps can touch the worktree AFTER the last patch was stored — the reviewer runs between fix and publish — the engine's git.push handler re-diffs and refreshes stale patch artifacts (with a refreshed-artifact event) before pushing. Interaction schemas turn strict with required arrays: {} and typo'd keys ({"findingz": …}) previously parsed as clean empty reports via defaults + zod's unknown-key stripping and auto-passed review gates. Top-level objects are now strictObject with required lists; nested finding/question objects stay tolerant of extra keys. Note for in-flight runs: leniently-stored artifacts re-read on resume now fail the gate-time backstop. The work-branch suffix widens from 8 to 12 hex chars (32 → 48 random bits of the UUIDv7 tail): ~1.2% birthday-collision odds at 10k runs is too high for a scheme pr.open's duplicate-recovery depends on. Real-git suite extended to pin the whole contract: agent commit-all excludes the sanitized deletion, the pushed branch retains .claude at origin, platform/agent files under sanitized paths appear in neither diff nor finalize commit, uncommitted work ships via the finalize commit, and empty runs refuse to publish. New core unit tests pin the strict-schema rejections; a new compliance test pins the reviewer-drift refresh.
hutusi
added a commit
that referenced
this pull request
Jul 23, 2026
Addresses round-2 review findings 4 and 6 (PR #5). In a heterogeneous fleet (one worker has codex, one doesn't) a run could land on the wrong worker and burn its pg-boss retries on a condition that isn't transient for THIS worker but is perfectly servable by another. The engine now throws a typed ExecutorUnavailableError — with a stable `code` so the worker can match across bundle boundaries — at the binding lookup, which happens before any status transition. The worker handler catches it, re-reads the run's status, and for queued/waiting_approval DECLINES the job: warn log, one `run.deferred` timeline event (surfaced in the SPA activity feed, both locales), job completes without consuming retries. The existing reconciliation sweepers re-enqueue both states, so the run bounces at most every ~60s until a capable worker claims it — no max-bounce, because that would convert a capacity problem into a run failure, and the submit-time availability gate already confines endless bouncing to reconfiguration windows. A `running` run still rethrows: nothing re-enqueues an unclaimed running run today (the execution lease is ADR-0009 future work). A compliance test pins the contract the worker depends on: typed error, stable code, thrown while the run is still queued. Docs are brought back in line with the runtime: ADR-0010's claim that input and review-gate both auto-pass on absent-or-empty is rewritten in place (the ADR was authored on this unmerged branch — rewriting is honest, amending would fake history) to the asymmetric semantics the engine actually enforces; design/03 replaces the known-limitation note with the deferral mechanism and the host-affinity boundary it exposes; design/04 cross-references it from the sweeper section. CHANGELOG gains the round-2 block.
There was a problem hiding this comment.
hutusi has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
Author
|
Second-round findings are all valid — thank you. Findings 1 and 3 were mirror-image holes in the round-1 evidence fixes (the PR could delete what the patch never showed; the patch could show what the push never shipped), so the fix establishes one contract: the patch artifact and the pushed PR contain exactly the same changes, and the sanitized paths appear in neither, in either direction. Resolved in 2 commits ( 1. Sanitized repository files silently deleted by the PR — fixed ( 2. Malformed artifacts still auto-pass ( 3. Patch evidence contains never-pushed work — fixed ( 4. Executor availability unsafe with heterogeneous workers — fixed ( 5. Branch names only probabilistically unique — fixed ( 6. Checkpoint docs contradict the runtime — fixed ( Full gate re-verified per commit ( |
hutusi
added a commit
that referenced
this pull request
Jul 23, 2026
…-only reviewer; evidence guards Addresses the code side of all round-3 review findings (PR #5); the ADR amendments and design-doc corrections follow separately. Finding 1 (P0): every platform git call — evidence diffs, the finalizing commit, the credentialed push — runs inside a directory the agent could write, including .git/**, and inherited the worker's FULL process.env. An agent-installed pre-push hook, a filter.*.clean, a diff textconv driver, or core.hooksPath in repo-local .git/config would have executed as the platform with DATABASE_URL, AGRIPPA_SECRET_KEY, and provider keys in scope; url.*.insteadOf could even rewrite the credentialed push URL and exfiltrate the repo token. The round-2 GIT_CONFIG_GLOBAL/SYSTEM nulling covered one call and never touched repo-local config. Three independent layers close it (each alone is whack-a-mole): the git() wrapper now spawns with the executor allow-list env scrub (buildScrubbedEnv — PATH/HOME/locale/TLS only) plus global/system config disabled on EVERY call; core.hooksPath and core.fsmonitor are neutralized on every invocation (and clone-sample hooks are removed at sanitize); and repo-local .git/config is snapshotted at provision into a platform sidecar (<runId>.platform — a SIBLING of the workspace, out of the write containment's reach) and restored before every diff and push, discarding agent-added filters/textconv/insteadOf/credential helpers with a warn log. The agent identity is pre-seeded into config at provision so agents never have a legitimate reason to touch it. The clone-base SHA moves into the same sidecar: diff() and the nothing-to-publish check trust it over the agent-writable refs/agrippa/base, closing the move-the-base-ref evidence blank-out found while verifying. A hostile-workspace real-git test drives the whole publish path with weaponized hooks, hooksPath, a clean filter, and an insteadOf redirect, and asserts none of it executed and the push landed at the real origin. Finding 2 (P1): the reviewer inherited the workspace's readWrite access, and the round-2 push-time refresh published post-approval drift — refreshed evidence is not approved evidence. v2 agent steps gain an optional per-step access override; the delivery template's review step declares readOnly (both executors already enforce it: claude via the isolation seam, codex via --sandbox read-only with fenced-json artifact synthesis). The push-time guard now FAILS the run (contract_violation) when the workspace differs from the stored patch instead of refreshing it; with a read-only reviewer the legit flow can never drift, so any hit is a real violation. The compliance drift test asserts failure-and-no-push, plus that reviewer steps run readOnly while implementer steps stay readWrite. Finding 3 (P1): a resumed run whose checkout succeeded on another host ran against the bare mkdir ensureDir() leaves — every step silently operating on nothing. WorkspaceManager gains isIntact(); when a succeeded workspace.checkout step has no repository behind it, the run fails fast with workspace_lost. Re-provisioning is deliberately not attempted: a fresh clone would lack the work branch and all agent commits, fabricating an empty-but-working state. contract_violation and workspace_lost get errors-namespace entries in both locales. Finding 5 (P2): the strict schemas accepted duplicate question and finding ids, but every consumer keys by id (answer records, selection sets, waiver maps, React keys) — duplicates were inseparable downstream. Both artifact schemas now reject duplicate ids.
There was a problem hiding this comment.
hutusi has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
Author
|
All five round-3 findings are valid — the P0 especially so, and verification turned up two aggravating details beyond the report: 1. [P0] Platform commit executes agent-controlled Git hooks with worker secrets — fixed (
2. [P1] Reviewer changes published after approval of different evidence — fixed ( 3. [P1] Cross-host resume operates without the repository — fixed ( 4. [P2] ADR-0010 rewritten despite the append-only rule — accepted ( 5. [P2] Duplicate interaction IDs — fixed ( Full gate re-verified per commit: |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CHANGELOG.md (1)
36-40: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winClarify the review-gate auto-pass rule.
Line 37 incorrectly groups
inputandreview-gateas auto-passing on empty sources. Only input checkpoints may auto-pass when absent; review gates require a present, schema-valid report with zero findings. Missing review evidence must fail the gate, as documented indocs/design/04-execution-runtime.md.Proposed wording
- - *`agrippa/v2` template format* ... checkpoint steps in three kinds (`approval` incl. request-changes, `input` question forms, `review-gate` findings decisions — the latter two auto-pass on empty sources), ... + - *`agrippa/v2` template format* ... checkpoint steps in three kinds (`approval` incl. request-changes, `input` question forms, `review-gate` findings decisions — input auto-passes on absent/empty questions, while review-gate auto-passes only on a present valid report with zero findings), ...🤖 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 `@CHANGELOG.md` around lines 36 - 40, Update the `agrippa/v2` template format changelog entry to state that only empty `input` checkpoints auto-pass; `review-gate` checkpoints require present, schema-valid review evidence with zero findings, and missing evidence fails the gate. Keep the existing descriptions of checkpoint kinds, loops, and other behavior unchanged.apps/worker/src/deps/scm.ts (1)
97-188: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd fetch timeouts and/or abort signals to the SCM API calls.
The GitHub and GitLab create/lookup calls in
apps/worker/src/deps/scm.tsdon’t passsignal; if the provider API stalls, these can hang. Thread in the run/step abort signal if available, or set per-call timeouts such asAbortSignal.timeout(30_000).🤖 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 `@apps/worker/src/deps/scm.ts` around lines 97 - 188, Add abort signals or 30-second timeouts to every GitHub and GitLab fetch in openGithubPr and openGitlabMr, including PR/MR creation and 422/409 lookup requests. Reuse the run/step signal if available; otherwise apply AbortSignal.timeout(30_000) so stalled provider calls terminate.
🤖 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/web/src/features/runs/RunActivityFeed.tsx`:
- Around line 101-108: Update the "run.deferred" case in RunActivityFeed to read
the event payload reason and interpolate it into the translated activity label,
following the existing payload-detail pattern used by cases such as
"workspace.ready"; preserve the current icon, warning tone, and event
sequencing.
In `@apps/worker/src/deps/workspace.test.ts`:
- Around line 193-205: Await the rejects assertion in the test “refuses to
publish a run with no commits and no changes” so the test waits for scm.push to
reject and verifies the “nothing to publish” error before completing.
In `@apps/worker/src/index.ts`:
- Around line 137-146: Update the error-code check in the run error-handling
branch around the executor-unavailable condition to safely access code when err
is null or undefined, using optional chaining while preserving the existing
ExecutorUnavailableError check and queued/waiting_approval handling.
- Around line 147-153: Add the missing activity.runDeferred translation key to
every packages/i18n/locales/*/runs.json catalog, using the existing activity
labels and locale conventions. Ensure RunActivityFeed.tsx can resolve this key
for run.deferred without changing the event payload or routing logic.
---
Outside diff comments:
In `@apps/worker/src/deps/scm.ts`:
- Around line 97-188: Add abort signals or 30-second timeouts to every GitHub
and GitLab fetch in openGithubPr and openGitlabMr, including PR/MR creation and
422/409 lookup requests. Reuse the run/step signal if available; otherwise apply
AbortSignal.timeout(30_000) so stalled provider calls terminate.
In `@CHANGELOG.md`:
- Around line 36-40: Update the `agrippa/v2` template format changelog entry to
state that only empty `input` checkpoints auto-pass; `review-gate` checkpoints
require present, schema-valid review evidence with zero findings, and missing
evidence fails the gate. Keep the existing descriptions of checkpoint kinds,
loops, and other behavior unchanged.
🪄 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 Plus
Run ID: 8c050556-a9b3-4946-96bd-0f5f775b5a91
📒 Files selected for processing (24)
CHANGELOG.mdapps/api/src/test/checkpoints.integration.test.tsapps/web/src/features/runs/RunActivityFeed.tsxapps/worker/src/deps/scm.tsapps/worker/src/deps/workspace.test.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0010-agrippa-v2-slots-checkpoints-loops.mddocs/adr/0011-codex-executor-and-platform-scm.mddocs/design/02-orchestration-template.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mdpackages/core/src/interaction-schemas.test.tspackages/core/src/interaction-schemas.tspackages/i18n/locales/en/errors.jsonpackages/i18n/locales/en/runs.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/i18n/locales/zh-CN/runs.jsonpackages/orchestration/src/engine/deps.tspackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/fakes.tspackages/orchestration/src/template-schema.tstemplates/swdev/requirement-delivery.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- docs/adr/0010-agrippa-v2-slots-checkpoints-loops.md
- docs/design/02-orchestration-template.md
- packages/i18n/locales/zh-CN/runs.json
- packages/orchestration/src/engine/deps.ts
- packages/i18n/locales/en/runs.json
- docs/design/03-executor-abstraction.md
- packages/orchestration/src/template-schema.ts
- apps/api/src/test/checkpoints.integration.test.ts
- templates/swdev/requirement-delivery.yaml
- packages/orchestration/src/engine/engine.ts
- packages/orchestration/src/engine/engine.integration.test.ts
|
Too many files changed for review. ( Bypass the limit by tagging |
Contributor
Author
|
Round 4 ( Two independent git metadata domains. Checkout moves the pristine It closes real residual holes in the round-3 design, each now pinned by a regression test:
Review verification: Known follow-ups recorded during review (not in this commit):
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/resources.test.ts`:
- Around line 22-25: Update the fixture setup around freshDir to write .mcp.json
first, then create the .claude directory and its hooks subdirectory before
writing .claude/settings.json and .claude/hooks/start.sh, ensuring all Bun.write
parent paths exist.
In `@apps/worker/src/deps/workspace.test.ts`:
- Around line 504-508: Await the rejects assertion in the test case for missing
trusted platform metadata by updating the workspace.diff(runId) expectation;
ensure the test waits for the promise to reject with the existing “trusted
platform git base is missing” matcher and preserves the fail-closed intact
check.
In `@docs/adr/0012-platform-owned-git-snapshots.md`:
- Line 22: Update the platform Git publish flow described in
docs/adr/0012-platform-owned-git-snapshots.md at lines 22 and 35 and
docs/design/03-executor-abstraction.md at line 110 so push() makes branch
publication concurrency-safe: replace the separate missing-existing commit and
ref-write operations with a create-if-absent/CAS update, or serialize publishing
per branch, while preserving idempotent reuse of matching refs and the
single-commit guarantee.
🪄 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 Plus
Run ID: 787a75f3-40b5-4497-9b31-719dd1410073
📒 Files selected for processing (23)
ARCHITECTURE.mdCHANGELOG.mdapps/worker/src/deps/resources.test.tsapps/worker/src/deps/resources.tsapps/worker/src/deps/scm.tsapps/worker/src/deps/workspace.test.tsapps/worker/src/deps/workspace.tsdocs/adr/0012-platform-owned-git-snapshots.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mddocs/design/09-testing-and-ci.mddocs/manual/en/03-running-tasks.mddocs/manual/en/06-operations.mddocs/manual/zh-CN/03-running-tasks.mddocs/manual/zh-CN/06-operations.mdpackages/executor-claude/src/executor.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.tstemplates/_shared/skills/git-workflow/SKILL.mdtemplates/swdev/requirement-delivery.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/orchestration/src/engine/deps.ts
- docs/manual/zh-CN/03-running-tasks.md
- packages/executor-claude/src/executor.ts
- packages/orchestration/src/engine/fakes.ts
- docs/manual/en/03-running-tasks.md
- templates/swdev/requirement-delivery.yaml
- packages/orchestration/src/engine/engine.integration.test.ts
- docs/design/04-execution-runtime.md
…trict schemas, 48-bit branches Addresses round-2 review findings 1, 2, 3, and 5 (PR #5). Findings 1 and 3 are mirror-image evidence-integrity holes the round-1 fixes introduced: the PR could silently DELETE sanitized files the patch never showed, and the patch could SHOW uncommitted work the push never shipped. The fix establishes one contract: the patch artifact and the pushed PR contain exactly the same changes, and the sanitized paths appear in neither, in either direction. Sanitized paths become invisible to git itself instead of being hidden from the diff: tracked .claude/.mcp.json/.agrippa entries get --skip-worktree before removal (so an agent's add -A/commit -a cannot ship the deletion — and reset --hard no longer casually restores repo-supplied .claude, an improvement over the plain rm), and the paths are appended to .git/info/exclude to cover the UNTRACKED side — which the platform itself populates (materialized skills under .claude/skills/, artifact scratch under .agrippa/artifacts/) and which skip-worktree alone cannot reach. The diff pathspec excludes are then dropped entirely. Push gains a platform finalizing commit: add -A (now safe by the above), staged-change detection via status --porcelain (diff --cached --quiet exits 1 into the throwing git helper), and a commit under an Agrippa identity with host gitconfig isolated (gpgsign/hooksPath must not leak). A branch whose HEAD still equals the clone base fails with 'nothing to publish' instead of an opaque provider 422 later. And because steps can touch the worktree AFTER the last patch was stored — the reviewer runs between fix and publish — the engine's git.push handler re-diffs and refreshes stale patch artifacts (with a refreshed-artifact event) before pushing. Interaction schemas turn strict with required arrays: {} and typo'd keys ({"findingz": …}) previously parsed as clean empty reports via defaults + zod's unknown-key stripping and auto-passed review gates. Top-level objects are now strictObject with required lists; nested finding/question objects stay tolerant of extra keys. Note for in-flight runs: leniently-stored artifacts re-read on resume now fail the gate-time backstop. The work-branch suffix widens from 8 to 12 hex chars (32 → 48 random bits of the UUIDv7 tail): ~1.2% birthday-collision odds at 10k runs is too high for a scheme pr.open's duplicate-recovery depends on. Real-git suite extended to pin the whole contract: agent commit-all excludes the sanitized deletion, the pushed branch retains .claude at origin, platform/agent files under sanitized paths appear in neither diff nor finalize commit, uncommitted work ships via the finalize commit, and empty runs refuse to publish. New core unit tests pin the strict-schema rejections; a new compliance test pins the reviewer-drift refresh.
Addresses round-2 review findings 4 and 6 (PR #5). In a heterogeneous fleet (one worker has codex, one doesn't) a run could land on the wrong worker and burn its pg-boss retries on a condition that isn't transient for THIS worker but is perfectly servable by another. The engine now throws a typed ExecutorUnavailableError — with a stable `code` so the worker can match across bundle boundaries — at the binding lookup, which happens before any status transition. The worker handler catches it, re-reads the run's status, and for queued/waiting_approval DECLINES the job: warn log, one `run.deferred` timeline event (surfaced in the SPA activity feed, both locales), job completes without consuming retries. The existing reconciliation sweepers re-enqueue both states, so the run bounces at most every ~60s until a capable worker claims it — no max-bounce, because that would convert a capacity problem into a run failure, and the submit-time availability gate already confines endless bouncing to reconfiguration windows. A `running` run still rethrows: nothing re-enqueues an unclaimed running run today (the execution lease is ADR-0009 future work). A compliance test pins the contract the worker depends on: typed error, stable code, thrown while the run is still queued. Docs are brought back in line with the runtime: ADR-0010's claim that input and review-gate both auto-pass on absent-or-empty is rewritten in place (the ADR was authored on this unmerged branch — rewriting is honest, amending would fake history) to the asymmetric semantics the engine actually enforces; design/03 replaces the known-limitation note with the deferral mechanism and the host-affinity boundary it exposes; design/04 cross-references it from the sweeper section. CHANGELOG gains the round-2 block.
…-only reviewer; evidence guards Addresses the code side of all round-3 review findings (PR #5); the ADR amendments and design-doc corrections follow separately. Finding 1 (P0): every platform git call — evidence diffs, the finalizing commit, the credentialed push — runs inside a directory the agent could write, including .git/**, and inherited the worker's FULL process.env. An agent-installed pre-push hook, a filter.*.clean, a diff textconv driver, or core.hooksPath in repo-local .git/config would have executed as the platform with DATABASE_URL, AGRIPPA_SECRET_KEY, and provider keys in scope; url.*.insteadOf could even rewrite the credentialed push URL and exfiltrate the repo token. The round-2 GIT_CONFIG_GLOBAL/SYSTEM nulling covered one call and never touched repo-local config. Three independent layers close it (each alone is whack-a-mole): the git() wrapper now spawns with the executor allow-list env scrub (buildScrubbedEnv — PATH/HOME/locale/TLS only) plus global/system config disabled on EVERY call; core.hooksPath and core.fsmonitor are neutralized on every invocation (and clone-sample hooks are removed at sanitize); and repo-local .git/config is snapshotted at provision into a platform sidecar (<runId>.platform — a SIBLING of the workspace, out of the write containment's reach) and restored before every diff and push, discarding agent-added filters/textconv/insteadOf/credential helpers with a warn log. The agent identity is pre-seeded into config at provision so agents never have a legitimate reason to touch it. The clone-base SHA moves into the same sidecar: diff() and the nothing-to-publish check trust it over the agent-writable refs/agrippa/base, closing the move-the-base-ref evidence blank-out found while verifying. A hostile-workspace real-git test drives the whole publish path with weaponized hooks, hooksPath, a clean filter, and an insteadOf redirect, and asserts none of it executed and the push landed at the real origin. Finding 2 (P1): the reviewer inherited the workspace's readWrite access, and the round-2 push-time refresh published post-approval drift — refreshed evidence is not approved evidence. v2 agent steps gain an optional per-step access override; the delivery template's review step declares readOnly (both executors already enforce it: claude via the isolation seam, codex via --sandbox read-only with fenced-json artifact synthesis). The push-time guard now FAILS the run (contract_violation) when the workspace differs from the stored patch instead of refreshing it; with a read-only reviewer the legit flow can never drift, so any hit is a real violation. The compliance drift test asserts failure-and-no-push, plus that reviewer steps run readOnly while implementer steps stay readWrite. Finding 3 (P1): a resumed run whose checkout succeeded on another host ran against the bare mkdir ensureDir() leaves — every step silently operating on nothing. WorkspaceManager gains isIntact(); when a succeeded workspace.checkout step has no repository behind it, the run fails fast with workspace_lost. Re-provisioning is deliberately not attempted: a fresh clone would lack the work branch and all agent commits, fabricating an empty-but-working state. contract_violation and workspace_lost get errors-namespace entries in both locales. Finding 5 (P2): the strict schemas accepted duplicate question and finding ids, but every consumer keys by id (answer records, selection sets, waiver maps, React keys) — duplicates were inseparable downstream. Both artifact schemas now reject duplicate ids.
Addresses the doc side of round-3 findings 3, 4, and 6-adjacent text. ADR-0010's semantics correction is put on the record as an appended amendment: the original decision text claimed symmetric absent-or- empty auto-pass, the second review round rewrote it in place, and the third round rightly objected — the append-only rule applies even to an ADR that has never been merged, because the branch history already published it for review. ADR-0011 gains an addendum recording this round's two consequential decisions (platform git distrusts the workspace; per-step access shipped with the read-only reviewer and the drift-fails-the-publish rule), superseding its 'per-step access modes are future hardening' consequence. design/03's claim that a cross-host resume 're-provisions its workspace from the repo' was simply false — the engine only mkdir'd an empty directory and skipped the succeeded checkout. The text now states the implemented behavior (isIntact probe, workspace_lost fail- fast, why re-provisioning is deliberately not attempted) plus the rationale for letting running-state pickups consume retries: each retry is a fresh pickup that may land on a capable worker, so retries are the routing mechanism. design/04's resumability section documents the probe; the CHANGELOG gains the round-3 block and the round-2 refresh clause is marked superseded.
Keep evidence and publication on a platform-owned gitdir and index so agent-controlled config, refs, indexes, excludes, symlinks, and special files cannot shape the pushed tree. Reset executor project configuration per attempt, fail closed on diff or evidence mismatches, publish one idempotent verified snapshot commit, and document the history and upgrade tradeoffs. Add real-Git and engine regressions for the reviewed attack paths.
…lback, deterministic publish Valid findings from the four CodeRabbit review batches on PR #5; false/stale ones are answered on the PR instead. Publish determinism (from the ADR-0012 concurrency thread): commit-tree now pins author/committer dates to the clone-base commit, so with identity, tree, parent, and message already fixed, the snapshot commit SHA is fully deterministic — a retry or racer that lost the local ref recreates the byte-identical commit and the expected-old update-ref (a CAS) chooses between equals rather than racing duplicates. The idempotency test now deletes the platform branch ref between pushes and asserts the recreated commit equals the first. Checkpoint evidence fallback: when the artifacts query fails, the panel previously disabled every action, so a review-gate could only expire (the findings live in the checkpoint snapshot — only the PREVIEW failed). Blind-waiver actions stay blocked (accept-all, approve, and a partial fix, whose unchecked findings are implicitly waived), but safe directions now work: fix-all, request-changes (comment required), reject — plus a retry button that re-fires the artifacts query (label in both locales). Contract enforcement: request_changes requires a non-empty comment at the schema level — the revision step interpolates the comment, so an empty one would send the agent back with no instructions; the UI already enforced this, the API now rejects it too (tests pin 400 before the kind check's 409). 'Accept all recommendations' could submit answer sets missing required answers (only recommendable questions were filled); it now fills-then-verifies and disables when a required question has no recommendation. Smaller correctness fixes: three un-awaited expect(...).rejects assertions in the real-git suite could pass without verifying (including the fail-closed diff check); the run timeline auto-scrolled only on mount instead of following the stream; run.deferred timeline entries now show the concrete reason the event already carried; the worker's executor-unavailable match no longer explodes on a null/undefined throw; codex executor tests clean up their temp dirs. Docs aligned where review caught drift: executor diagram lists Codex, request-changes qualified as loop-only in the RBAC text, Approvals nav name explained as the generalized checkpoint inbox, agent-slot interchangeability qualified by engine availability and granted models (manual, both locales), ADR-0012/design-03 record the CAS + determinism rationale.
hutusi
force-pushed
the
feat/requirement-delivery-workflow
branch
from
July 23, 2026 06:14
fa91cfd to
cbf360e
Compare
Contributor
Author
|
CI + review sweep. Two changes landed (with one history rewrite, hence the force-push): CI fix. The PR-range commitlint step was failing on
Fixed:
Dismissed (with reasons):
Deferred (tracked follow-ups):
Verification: full gate green ( |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
42-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the review-gate auto-pass description.
This says both input and review-gate checkpoints auto-pass on empty sources. Review-gates only auto-pass on a present, schema-valid report with zero findings; missing review evidence must fail the gate.
Proposed wording
- - ... the latter two auto-pass on empty sources ... + - ... input checkpoints auto-pass when questions are absent or validly empty; + review-gates auto-pass only on present, schema-valid zero-finding reports; + missing review evidence fails the gate ...🤖 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 `@CHANGELOG.md` around lines 42 - 43, Update the `agrippa/v2` template format changelog entry so only input checkpoints auto-pass on empty sources; specify that review-gate checkpoints auto-pass only when a present, schema-valid report contains zero findings, while missing review evidence fails the gate.
🤖 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.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 42-43: Update the `agrippa/v2` template format changelog entry so
only input checkpoints auto-pass on empty sources; specify that review-gate
checkpoints auto-pass only when a present, schema-valid report contains zero
findings, while missing review evidence fails the gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4aabdb0d-eeb5-4264-936d-1ae354837e67
📒 Files selected for processing (48)
ARCHITECTURE.mdCHANGELOG.mdapps/api/src/test/checkpoints.integration.test.tsapps/web/src/features/runs/CheckpointPanel.tsxapps/web/src/features/runs/FindingsTable.tsxapps/web/src/features/runs/QuestionsForm.tsxapps/web/src/features/runs/RunActivityFeed.tsxapps/web/src/features/runs/RunTimeline.tsxapps/web/src/pages/ApprovalsPage.tsxapps/web/src/pages/RunDetailPage.tsxapps/worker/src/deps/resources.test.tsapps/worker/src/deps/resources.tsapps/worker/src/deps/scm.tsapps/worker/src/deps/workspace.test.tsapps/worker/src/deps/workspace.tsapps/worker/src/index.tsdocs/adr/0010-agrippa-v2-slots-checkpoints-loops.mddocs/adr/0011-codex-executor-and-platform-scm.mddocs/adr/0012-platform-owned-git-snapshots.mddocs/design/02-orchestration-template.mddocs/design/03-executor-abstraction.mddocs/design/04-execution-runtime.mddocs/design/05-api-and-auth.mddocs/design/06-frontend.mddocs/design/09-testing-and-ci.mddocs/manual/en/02-concepts.mddocs/manual/en/03-running-tasks.mddocs/manual/en/06-operations.mddocs/manual/zh-CN/02-concepts.mddocs/manual/zh-CN/03-running-tasks.mddocs/manual/zh-CN/06-operations.mdpackages/core/src/interaction-schemas.test.tspackages/core/src/interaction-schemas.tspackages/core/src/schemas.tspackages/executor-claude/src/executor.tspackages/executor-codex/src/executor.test.tspackages/executor-core/src/isolation.tspackages/i18n/locales/en/errors.jsonpackages/i18n/locales/en/runs.jsonpackages/i18n/locales/zh-CN/errors.jsonpackages/i18n/locales/zh-CN/runs.jsonpackages/orchestration/src/engine/deps.tspackages/orchestration/src/engine/engine.integration.test.tspackages/orchestration/src/engine/engine.tspackages/orchestration/src/engine/fakes.tspackages/orchestration/src/template-schema.tstemplates/_shared/skills/git-workflow/SKILL.mdtemplates/swdev/requirement-delivery.yaml
🚧 Files skipped from review as they are similar to previous changes (41)
- packages/i18n/locales/zh-CN/errors.json
- packages/i18n/locales/en/errors.json
- apps/web/src/features/runs/RunActivityFeed.tsx
- apps/worker/src/deps/resources.test.ts
- apps/web/src/pages/ApprovalsPage.tsx
- docs/adr/0012-platform-owned-git-snapshots.md
- templates/_shared/skills/git-workflow/SKILL.md
- docs/manual/en/06-operations.md
- docs/design/09-testing-and-ci.md
- packages/core/src/interaction-schemas.test.ts
- docs/design/02-orchestration-template.md
- apps/web/src/features/runs/CheckpointPanel.tsx
- apps/worker/src/deps/resources.ts
- apps/web/src/features/runs/QuestionsForm.tsx
- docs/manual/en/02-concepts.md
- packages/i18n/locales/zh-CN/runs.json
- packages/executor-core/src/isolation.ts
- apps/web/src/features/runs/RunTimeline.tsx
- templates/swdev/requirement-delivery.yaml
- packages/core/src/schemas.ts
- apps/web/src/pages/RunDetailPage.tsx
- packages/orchestration/src/engine/fakes.ts
- packages/orchestration/src/engine/deps.ts
- docs/design/06-frontend.md
- docs/design/05-api-and-auth.md
- docs/adr/0010-agrippa-v2-slots-checkpoints-loops.md
- packages/executor-codex/src/executor.test.ts
- packages/executor-claude/src/executor.ts
- apps/worker/src/deps/scm.ts
- apps/worker/src/index.ts
- apps/api/src/test/checkpoints.integration.test.ts
- docs/adr/0011-codex-executor-and-platform-scm.md
- ARCHITECTURE.md
- docs/manual/zh-CN/03-running-tasks.md
- packages/core/src/interaction-schemas.ts
- packages/orchestration/src/engine/engine.integration.test.ts
- apps/worker/src/deps/workspace.test.ts
- docs/design/03-executor-abstraction.md
- packages/i18n/locales/en/runs.json
- apps/worker/src/deps/workspace.ts
- packages/orchestration/src/engine/engine.ts
The round-1 Added entry still described the original symmetric rule (input and review-gate both auto-passing on empty sources), which the second review round corrected everywhere else — an absent review report fails the gate. Unreleased ships as the release notes, so the entry must state the final semantics. Caught by CodeRabbit's re-review of the previous push. Also generalize the manual's template definition from approval-only checkpoints to all three kinds (both locales) — the last approval-era wording an open review thread pointed at.
hutusi
force-pushed
the
feat/requirement-delivery-workflow
branch
from
July 23, 2026 07:08
1abfb07 to
44abca0
Compare
This was referenced Jul 27, 2026
hutusi
added a commit
that referenced
this pull request
Jul 30, 2026
…ailing publish The git.push evidence check compared the fresh workspace snapshot against artifactValues, which holds "" for any patch artifact past the 64 KB inline threshold — so every run whose reviewed diff exceeded 64 KB died at publish with a phantom "workspace changed after the reviewed evidence". This was the remaining follow-up from PR #5's review rounds, and the sibling of the interaction-artifact fix earlier on this branch: same threshold, different consumer. A patch cannot get the interaction artifacts' raised inline allowance — patches are capped at 25 MB, far past anything Postgres should inline — so the check instead reads the stored bytes back via a new ArtifactStore.read(storageRef). The stored patch IS the approved evidence: drifted workspaces still fail exactly as before, and evidence that cannot be read back (lost volume, corrupted row) fails the push with a distinct contract_violation rather than publishing unverified. DiskArtifactStore.read refuses refs outside the storage root, so a corrupted row cannot become an arbitrary-file-read primitive; the in-memory fake persists spilled content across engine legs the way the real artifacts volume does.
This was referenced Aug 1, 2026
This was referenced Aug 16, 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.
Takes a natural-language requirement all the way to a reviewed pull request with two cooperating agents — an implementer (Claude Code / Forge) and a reviewer (OpenAI Codex / the new Arbiter faber) — with the user in the loop for clarification, plan confirmation, and per-finding review decisions, and the whole team watching and commenting live.
What's in here
agrippa/v2template format (ADR-0010 — the explicit v2 decision ADR-0006 reserved)implementer,reviewer) binding a faber + executor; the submitter can swap either on overridable slots. Bindings freeze onto the run at submit with capability checks against a static executor catalog in core and per-slot, provider-filtered model resolution (a Codex slot without a granted OpenAI model fails at submit, not mid-run).approval(incl. request changes inside loops, with the comment fed back into the revision),input(structured Q&A from an agent-produced questions artifact, with recommended answers), andreview-gate(fix-selected vs accept-remaining per finding). Empty sources auto-pass. Decisions store a structured response that re-enters the run as thecheckpoints.<id>expression root.maxIterations1–10,untilconditions) power clarify-Q&A, plan revision, and review-fix uniformly. Iteration is a column on steps/checkpoints/artifacts; resume derives its round from persisted rows, so crash recovery needs no new state.OpenAI Codex executor (ADR-0011)
packages/executor-codexwrapscodex exec --json(event shapes pinned against codex-cli 0.145.0 by live probes; samples in the package README): normalized event mapping, cache-aware usage splitting, same-step resume via thread ids, SIGTERM-on-abort.read-only/workspace-write, network off,approval_policy=never); the enforcement matrix is documented in the ADR. Read-only reviewer steps synthesize their json artifact from the final message's fenced block.Platform-side git write-path
git.branch/git.push/pr.openrun through an engine SCM seam (worker: credential-injected push that never touches.git/config, PR/MR via the GitHub/GitLab REST API). The PR link is a contract-required artifact, so it no longer depends on an optional MCP server or agent behavior.API + collaboration
POST /runs/:id/checkpoints/:checkpointId/respond(kind-discriminated, validated against the pending row's snapshot; CAS + event + audit in one transaction), a generalizedGET /checkpoints/pending"waiting on you" inbox, and run comments that commit together with theircomment.addedevent so the live timeline and the thread can't disagree. Legacy/approvalsroutes removed.UX
Flagship template —
swdev.requirement-delivery: clarify loop → plan loop (onMaxIterations: fail— an unapproved plan is never implemented) → implement on a platform-named branch → review-fix loop → publish, with an extra sign-off only when the loop exhausts right after a fix that was never re-reviewed.Schema —
approvals→checkpoints(data-preserving rename; + kind/iteration/response),iterationon run_steps/artifacts,agent_bindings/work_branchon runs, newrun_comments(migration0005_requirement-delivery).Verification
bun run check,bun test(160 tests incl. Postgres integration suites),bun run templates:validate,bun run build— all green at every commit.Notes for review
gpt-5.1-codex,gpt-5.1-codex-mini) should be verified against the current price list at rollout; registry rows are admin-editable.readWriteaccess (access is per-workspace, not per-step); the reviewer is instructed not to modify code — per-step access modes are noted as future hardening in ADR-0011.bug-localize-fix's MCP-basedopen-prstep to platformpr.openis a noted follow-up.AGRIPPA_EXECUTOR=fake, every slot binds to the demo executor (a demo install must never silently route to a key-consuming engine);AGRIPPA_SCM=fakefabricates branch/push/PR, so the whole workflow demos token-free.Summary by CodeRabbit