feat(context): add Agent Review workflow#1795
Conversation
Gandy2025
left a comment
There was a problem hiding this comment.
Independent code review — Agent Review (Phase 2)
Reviewed at head ec47509a (the 8df2c7fe..ec47509a delta is test-only: one
mock fixture in api-small-routes-extra.test.ts, production code unchanged).
Scope: architecture intrusion, legacy_app compatibility / single verdict,
task-packet trust boundary, repair + exact-head merge contract, Phase-1
simulation fidelity, test coverage.
Note on review type: my gh identity is the PR author, so GitHub only allows a
COMMENT review here. Treat this as one should-fix (medium) + minor notes;
I did not find an architectural blocker.
Should-fix (medium) — malformed-metadata fail-closed is bypassed by a stack overflow
renderAgentTaskContextForLLM (packages/client/src/runtime/agent-io.ts:330)
is designed to fail closed: when contextReviewTaskMetadataSchema.safeParse(...)
returns success === false it emits the <first-tree-task-context-error> block
telling the Reviewer "Do not repair, publish, or merge … report the malformed
task input."
That guarantee does not hold for a deeply-nested reviewPacketV1. The size
guard decodedStringBytes (packages/shared/src/schemas/context-review.ts:110)
is unbounded recursion, and zod v4's own parse recurses too, so a deeply-nested
value throws RangeError: Maximum call stack size exceeded — which safeParse
does not convert to {success:false}. The !parsed.success branch never
runs; the call throws instead.
Empirical repro (standalone, zod 4.4.3 matching zod@^4.0.0):
depth=2000 jsonBytes=4058 -> returned success=false
depth=3000 jsonBytes=6058 -> THREW RangeError
depth=5000 jsonBytes=10058 -> THREW RangeError
Reachability:
- The trigger is
metadata.taskType === "context_tree_pr_review"— the only
gate before the recursive walk. Any chat participant who can attach message
metadata can set it;sendMessageSchema.metadatais
z.record(z.string(), z.unknown())with no depth bound, stored verbatim. - ~3000 nesting levels ≈ 6 KB of JSON — far under the 32 KiB packet guard
(which counts only string-leaf bytes and does not bound structural depth) and
under transport body limits. renderAgentTaskContextForLLMalso runs overprecedingMessages, so a
recipient with nothing to do with Context Review still walks the payload if a
prior message in the chat carries thattaskType.
Observed impact in the built paths: the batch-inject handlers (TUI / cursor /
codex) and the single-message toSDKUserMessage path all wrap
formatInboundContent in try/catch and log a format failure, so the crafted
message is silently dropped rather than surfaced as malformed. That is a
denial-of-delivery for that turn and a defeat of the advertised fail-closed
degradation — not a process crash or RCE, but exactly the untrusted-input
robustness class this trust boundary exists to prevent.
Suggested direction (not prescriptive): bound structural depth before / inside
the byte walk, and wrap the size-check + parse so any throw maps to the same
<first-tree-task-context-error> fail-closed block instead of propagating.
Minor — 32 KiB boundary is client-runtime-only, not server-side
contextReviewTaskMetadataSchema / CONTEXT_REVIEW_PACKET_MAX_BYTES are
referenced only in agent-io.ts; there is no server-side reference. That is
consistent with Phase 1 not being built yet, but any framing that says the
32 KiB limit is enforced "server-side pre-persistence" is inaccurate for this
PR — today the server stores the packet as generic untyped metadata and the
only validation locus is the recipient runtime. Worth stating precisely so
Phase 1 owns the ingestion-side check explicitly.
Minor — fail-closed test covers only shallow-malformed input
agent-io.test.ts "fails closed when Agent Review task metadata is malformed"
uses { schemaVersion: 1, pullRequest: 42 } (shallow), which correctly returns
success:false. It does not exercise the deep-nesting vector above, so the
regression that matters is untested. A depth-based case would lock the fix.
Confirmed sound (no action)
- Single verdict owner. Under
agent_review, the App webhook path returns
{handled:false, reason:"workflow_not_legacy_app"}
(context-reviewer-pr.ts:232) and App publication is rejected in
assertCurrentAuthority(context-reviewer-publisher.ts:430); the Agent path
never submits a GitHubAPPROVE. Two projections (PR comment + commit
status), one authority. Tests cover both guards. - Legacy compatibility. Stored two-field configs default to
legacy_app / human / squash;applyInputDeltapreserves workflow fields
across two-field writes; disabling nulls onlyagentUuid. Existing App
reviewer teams are unaffected.legacy_app + autonomousis rejected at both
input and storage refinement layers. - Packet trust boundary.
reviewPacketV1Schemais.strict();
repositoryPathSchemarejects absolute,\, and./..segments (no
traversal);expectedHeadis normalized 40-hex;repositoryisowner/name.
The runtime wrapper labels every value as untrusted and defers to live GitHub;
SKILL §6.1 requires live author ==requesterGithubLogin, managed marker,
and scope agreement before any write. Discovery-only, GitHub-authoritative. - Repair + exact-head merge. SKILL §6.3/§6.5 re-read live PR + source head
before edit/commit/push, scope-limit writes, forbid force-push/retarget,
restart from each successor head, never carryREADYacross a push, and merge
only viagh pr merge --match-head-commit <SHA>with no--auto/--admin.
Progress-gatedNEEDS_HUMAN; bounded check-wait with no watcher/job. Faithful
to the accepted design. - Phase-1 simulation fidelity. Because the runtime renders any sender's
taskType/reviewPacketV1metadata identically, injecting the real packet
through an ordinary task Chat exercises the true consumption boundary with no
test-only endpoint and no second packet shape. No hidden dependency. - Minimalism. No new table, endpoint, job, queue, coordinator, or merge
credential.contextReviewerextended in place;/agent/context-tree/info
reused for the read.
Overall: architecture is minimal and faithful to the R18-accepted trust model.
Recommend landing after the fail-closed hardening + its regression test; the two
minor items are precision/coverage, not blockers.
What changed
This implements the Phase 2 Agent Review module proposed in Context Lite V1 PR #749, independently of the unfinished Phase 1 Write workflow.
agent_reviewworkflow,human/autonomousgovernance, and merge method. Old stored settings continue to default to the legacy App workflow.context_tree_pr_review/reviewPacketV1task contract and a 32 KiB decoded UTF-8 boundary.context-tree-reviewSkill so one assigned Reviewer owns inspection, scoped repair, full re-review, current-head outcome, Human handoff, and optional expected-head merge. There is no fixed Auditor Agent.first-tree org context-tree review-configcommand.legacy_app; App ingress and publishing do not create a second verdict whileagent_reviewowns the Team workflow.Scope and architecture
This is deliberately a module on existing primitives: Agent, ordinary task Chat/Inbox, Context Tree binding, runtime proof, org settings, the existing review Skill, and Host
gh.It does not add a Job table, scheduler, watcher, Coordinator service, dedicated review endpoint, Cloud merge credential, or second PR-state store.
Phase 1 task creation is not implemented here. Until it ships, tests and dogfood create an ordinary task Chat and inject the exact opening body plus
taskType/reviewPacketV1metadata that Phase 1 will later emit. No private test-only endpoint or alternate packet shape is introduced.User and developer impact
Validation
pnpm checkpnpm typecheckpnpm validate:skillcontext-tree-auditfixture that timed out only under full parallel-suite load passes in isolation: 12/12.git diff --checkThe full
pnpm testrun was attempted twice. Each run reached unrelated pre-existing timing-sensitive QA/skill fixtures under parallel load; the affected fixtures passed when rerun independently. No Agent Review targeted test failed.Follow-up boundaries