feat(agent): expose staged-buffer diagnostics for local_buffer_overflow (#4618) - #4642
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Adversarial red-team evidence at exact head
Verdict (author-signed, pending independent approval): needs-humanFocused local verification (lane worktree at 2a77ae9, review branch untouched)Also green: Red/green proof (byte-cap overflow, identical probe both sides)
Findings (no blockers)
Residual risk (accepted, non-blocking)
Merge was not performed. — |
|
@probepark @snowykr — requesting an independent exact-head review of this PR (head The full author-signed adversarial evidence, red/green proof (HEAD vs. base), and the eight audit findings are in the comment above. The contract verdict stays Review focus areas, in priority order: (1) prefix compatibility for session prefix-classification, (2) — |
|
Exact-head status — bounded CI+review hold (head CI classification at this head:
Verdict: — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review — merge blocked. The design is sound; this is a rebase + contract cleanup.
major — conflicts with #4610 on dev
packages/agent/src/agent-loop.ts and packages/agent/test/managed-attempt-transaction.test.ts conflict with dev commit 44d7b6ee0 (#4610), which inserts superseded-delta compaction immediately before both overflow throw sites. GitHub reports the branch as not mergeable.
When rebasing, the diagnostics must snapshot the retained post-compaction batch — otherwise the reported buffer shape describes deltas that #4610 already reclaimed, which is worse than no diagnostic.
minor — any casts
packages/agent/test/managed-attempt-transaction.test.ts:2256,2294 add as any, against the explicit AGENTS.md rule.
otherwise
Redaction is applied at the producer boundary and the new assertions genuinely fail against the old static error/receipt behavior. Rebase onto #4610, keep post-compaction counts, drop the casts, and this is mergeable.
2a77ae9 to
8863709
Compare
8863709 to
d6d9519
Compare
|
Reconstruction landed — exact head The branch was force-with-lease updated from stale head Every blocker from the CHANGES_REQUESTED review, addressed
Verification at this exact head (all green)
Verdict lineThe PR body carries exactly one current verdict line, @probepark — requesting re-review at the new head. Focus areas: (1) post-compaction snapshot ordering at both throw sites, (2) removal of the two casts, (3) unchanged legacy message prefix for session prefix-classification. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head d6d95193 — merge blocked. The diagnostic object itself is clean; the boundary that carries it to the parent is not.
prior findings resolved
Rebased onto current dev base b01f34527, so the #4610 conflict is gone — both overflow sites now compact superseded frames first, re-test the cap, discard(), then read the retained post-compaction shape via stagedShape(). Both as any casts from the previous head are removed.
major 1 — the parent boundary trusts a self-labeled error and forwards its free-form message
packages/coding-agent/src/task/executor.ts:2109-2113.
localErrorSummary is not shape-only by construction. managedFailureMessage (packages/agent/src/agent-loop.ts:374-405) copies a matching errorKind off any thrown object, and your own foreign-error test (packages/agent/test/managed-attempt-transaction.test.ts:1427-1448) demonstrates a stream error self-labeling as a local failure. The executor then runs that arbitrary errorMessage through regex redaction and the receipt embeds whatever survives, verbatim.
Regex redaction catches things that look like credentials and paths. It does not catch prompt text, tool arguments, or file content — which is exactly what an arbitrary error message from a foreign producer can contain. So a diagnostics feature whose whole justification is "closed vocabulary and numbers" ships a free-form string channel from child to parent receipt.
Require trusted producer identity at that boundary and propagate structured closed-vocabulary/numeric diagnostics instead of a message string.
To be explicit about what is fine: ManagedAttemptBufferOverflowError.overflow carries stage, stagedEventCount, stagedBytes, maxStagedEvents, maxStagedBytes — all closed or numeric, no payload. The unsafe field is downstream LocalErrorSummary.summary: string.
major 2 — the diagnostic cannot tell you which cap tripped
packages/agent/src/agent-loop.ts:1438-1447.
#wouldOverflow(bytes) tests stagedEventCount + 1 and stagedBytes + bytes. But #overflowShape() records only retained values — which by definition are still at or below both limits — and always emits both caps.
Two consequences. A single oversized event reports a tiny byte numerator against a large limit, which reads as "nowhere near the cap" for the event that just blew it. And the shape does not distinguish event-cap from byte-cap overflow, which the body and changelog both claim it does.
Add a closed exceeded discriminator plus prospective counts (or incomingEventBytes), and assert the relevant projected value actually exceeds its cap.
minor — the render path is entirely unpinned
packages/coding-agent/src/tools/subagent-render.ts:198-204. No test traverses subagentRunOutcomeFromSingleResult → AsyncJobManager → snapshot → markdown/TUI renderer; the added tests stop at SingleResult or construct a receipt directly. Dropped propagation, stale caching, and renderer sanitation regressions are all uncovered. Add failed async-subagent list/inspect/await tests over cached and streaming/dynamic rendering with tabs and over-width text.
minor — guidance mislabels one kind
packages/coding-agent/src/tools/subagent.ts:719-723 calls every localErrorSummary a staging-buffer limit, including local_snapshot_failure, which is a serialization failure. Render conditionally by kind.
audits that came back clean
Growth: no append-only collection. One summary per SingleResult/AsyncJob/receipt/snapshot, capped at 280 code points / 1,024 bytes; terminal jobs evict after five minutes; list output defaults to 10, caps at 50; renderer LRU caps at 128 bodies. Fan-out is linear in child count, one bounded item per child — not per overflow.
Renderer: both cached and dynamic/streaming bodies route through boundSubagentBodyLines() (replaceTabs() + truncateToWidth()), and snapshot production uses sanitizeText() with tab replacement, width truncation, and byte caps. Absolute paths are producer-redacted rather than shortenPath()-shortened. These are correct display safeguards — but they are display safeguards, so they do not substitute for the producer authentication in major 1.
Receipt versioning: TaskResultReceipt is persisted in tool-result/session details, participates in phase-rollup hashing, and is SDK-exported. The new field is optional and existing readers are structural/unknown-field tolerant, so the additive shape does not break old readers. The problem is trusting the field, not its shape.
sweep
No added any, ReturnType<>, inline await import(), or console.* in packages/coding-agent/. Both changelog entries directly under ## [Unreleased]. Workflow-skill and role-agent sets unchanged.
coverage
Both overflow-message tests are real pins driving real overflow paths, but neither asserts which cap tripped or the rejected event's projected size — the gap in major 2. The executor-propagation and parent-preview tests are pins for propagation and formatting but are synthetic: they inject a fabricated terminal message and never execute an overflow. The redaction test covers selected credential/path patterns and does not show arbitrary payload content cannot pass. The foreign-errorKind test pins kind normalization but not the actual leak case — a known local kind attached to a forged arbitrary message.
Nothing pins task/index.ts, async/job-manager.ts, tools/subagent.ts, or tools/subagent-render.ts.
Reviewed by @probepark — method: detached worktree at d6d95193, field-by-field leak audit of the overflow error and downstream summary, producer-trust trace from managedFailureMessage to receipt embedding, #wouldOverflow vs #overflowShape numeric comparison, growth/renderer/receipt-version audits, per-test base-vs-head discrimination. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:4675bc04698626e16cfe70492b3b85038f7951156b6c5034130639202fff1404 reviewer:human reviewer-id:probepark evidence:exact-head-d6d95193-free-form-message-channel-from-untrusted-producer-into-parent-receipt
d6d9519 to
6dfd619
Compare
|
Fix-forward for review 4961765443 — exact head Both majors and both minors are addressed. The diff now spans 16 files (+716/−11) — the three new files over the prior head are Major 1 — parent boundary no longer trusts a self-labeled error's free-form message
Major 2 — the diagnostic names which cap tripped
Minor — render path pinned end-to-endNew tests in Minor — guidance is kind-conditionalBoth the tool output ( Verification at this exact head
Verdict line updated to the exact new head/base/digest ( — |
|
@probepark — re-review requested at exact head
Local gates at this head: agent 789/0, coding-agent task/subagent/session suites 509/0, per-package checks + build + lint + schemas/plugins/tools + smoke all exit 0, — |
|
Exact-head status — head CI at this head (after rerunning the one infra-flaked job):
Blocker statement: everything inside this lane is complete — both majors and both minors from review 4961765443 are fixed with regression coverage, all local gates and exact-head CI are green. The only remaining step is a human action this account cannot perform for itself: an APPROVED review at Once that lands, the verdict flips to — |
6dfd619 to
7f4b805
Compare
|
Head advanced to Rebase was conflict-free and byte-identical in the diagnostic scope; all gates re-verified at the new head: agent 794/0, coding-agent task/subagent/session suites 420/0, per-package checks (ai/agent/coding-agent) all exit 0, @probepark @snowykr — the review request now targets — |
|
Status ping (no action needed beyond the pending review): head remains — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 7f4b8057 — merge blocked. Major 2 is properly fixed. Major 1 is not — it changed shape rather than closing.
major 2 (which cap tripped) — fixed
agent-loop.ts:1555-1583: #overflowShape() now computes eventsExceeded from stagedEventCount + 1 and bytesExceeded from stagedBytes + incomingEventBytes, emits a closed events | bytes | both, and carries the rejected event size alongside the retained post-compaction counters. Snapshotting the retained counters in discard() before clearing is the right ordering — the diagnostic reports post-compaction state while memory is released.
major 1 (trust boundary) — still open
task/executor.ts:2114-2118 calls createLocalErrorSummary(lastAssistant.errorKind, ..., lastAssistant.bufferOverflow) based only on presence of public message fields.
The producer-side instanceof check does not survive to the consumer as identity evidence:
agent-loop.ts:1143-1168spreads foreign/providersafeMetadataand deletes neithererrorKindnorbufferOverflow.packages/ai/src/types.ts:676-694,734-743exposes the object publicly and typesstageasstring.task/types.ts:441-457interpolates its fields with no runtime validation.
So a foreign stream can put arbitrary text in stage, in exceeded, or in string-valued nominally-numeric properties, and it is embedded verbatim in TaskResultReceipt.preview, errorSummary and localErrorSummary (receipt.ts:149-150,302-305). Your own new test at executor-subagent-reminders.test.ts:1689-1730 demonstrates the executor trusting a fabricated terminal shape.
Separately, local_snapshot_failure still routes a self-labeled free-form errorMessage through regex redaction (task/types.ts:481-487). Redaction catches things resembling credentials and paths; prompt text, tool arguments and file content pass through.
The body's "shape-only by construction" claim is therefore not accurate at this head.
Fix: strip local diagnostic fields from every foreign assistant message, attach them only on the private runtime-error path, validate closed stage/exceeded values and finite non-negative integer counters at the executor boundary, and use fixed or structured text for snapshot failures.
field-by-field leak audit
errorKind is closed by isAssistantLocalErrorKind, and LocalErrorSummary.kind normalizes to local_buffer_overflow / local_snapshot_failure / local. Good.
For overflow: stage is free-form at the receiving boundary; exceeded is a compile-time union only and free-form at runtime; all five counter/limit fields lack runtime numeric validation and can arrive as foreign strings or objects before interpolation. The resulting overflow summary is free-form and is not capped before receipt persistence.
For snapshot failure: summary is bounded to 280 code points / 1,024 bytes but stays free-form after pattern redaction.
Under the acceptance rule I set last round — any arbitrary-content channel from child to parent receipt is blocking — that is the block.
audits that came back clean
Bounding. #wouldOverflow() checks prospective count and bytes before append (:1549-1553); the collection is bounded at 10,000 events and 16 MiB. On failure discard() stores three numbers in #lastStagedShape, assigns #batch = [] and zeroes both counters (:1506-1517) — genuinely released, not merely uncounted.
Renderer. Cached and dynamic/streaming bodies both pass every line through boundSubagentBodyLines() (subagent-render.ts:111-113,132-144) applying replaceTabs() and truncateToWidth(); previews use shared limits via getPreviewLines(); snapshot production applies sanitizeText() with tab replacement, width truncation and code-point/byte caps (subagent.ts:865-870,965-970). Correct — but display truncation is not a substitute for the producer authentication above.
Persistence. localErrorSummary is optional (receipt.ts:45), lives in ordinary tool-result details/session JSON, participates in the phase-rollup hash, and the type stays exported. Readers are structural and tolerate unknown/absent fields, so no migration is needed. Same for optional AssistantMessage.bufferOverflow. The problem is authenticity, not compatibility.
Scope. 341 production, 373 test, 2 changelog additions across 16 files — all on the real cross-package path (agent producer, public AI type, executor/receipt/async/snapshot/render propagation, tests). No unrelated creep. The diagnostic shape is duplicated across agent/AI/coding-agent, which is maintainability debt worth consolidating later, not a blocker.
minor — the render path still is not pinned by a real overflow
test/tools/subagent.test.ts:201-267 traverses SingleResult -> SubagentRunOutcome -> AsyncJob -> snapshot -> renderer, which is the shape I asked for, but starts from a fabricated SingleResult.localErrorSummary and only exercises terminal cached inspect. No real overflow reaches the chain, no active-retry dynamic/streaming render, no tab or over-width assertion.
Also managed-attempt-transaction.test.ts:2379-2416 accepts exceeded=both for the event-cap case, so it would not catch a regression that stopped distinguishing an event-only trip. Require events and prove projected bytes stay under the byte cap.
coverage
Only tests 2 and 3 (real byte overflow, real event-count overflow) exercise the mechanism. Tests 4, 5, 7, 9, 10, 12 and 13 are synthetic — they inject a fabricated terminal shape or call the helper directly. Tests 1, 6 and 11 are base guards.
Nothing carries a real overflow through runSubprocess, receipt/job propagation, snapshot creation and parent rendering — which is exactly the path where major 1 lives.
Reviewed by @probepark — method: detached worktree at 7f4b8057, field-by-field classification of every diagnostic property as closed or free-form at the receiving boundary, trace of safeMetadata spreading to show the producer instanceof does not survive, bounding/renderer/persistence audits, per-test real-versus-synthetic and pin-versus-guard classification. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:e5d4012d0e269edb7145a5d32ee7247c8a81bccaffbbf3b01188fc827883e3db reviewer:human reviewer-id:probepark evidence:exact-head-7f4b8057-cap-discriminator-fixed-but-parent-still-trusts-unauthenticated-diagnostic-shape
|
Fix-forward at new exact head Major 1 (trust boundary) — closed at both ends:
Minor (render pin) — fixed:
Verification (this lane ran): Note on ancestry: head is Requesting fresh exact-head re-review at — |
1b0ee42 to
a38a157
Compare
|
Heads-up: branch rebased onto current dev — review target is now exact head
— |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head a38a1578 — merge blocked on one remaining major. The leak itself is closed.
the arbitrary-content channel is gone
This was the finding that mattered and it is properly fixed:
agent-loop.ts:1160-1161now doesdelete safeMetadata.errorKind; delete safeMetadata.bufferOverflow;, so foreign assistant metadata cannot carry local diagnostic fields through.task/types.ts:447-499validates the closed vocabulary at the executor boundary —stagelimited tooverflow.preMeasure|overflow.staged,exceededtoevents|bytes|both, and all five counters required to be non-negative safe integers.task/types.ts:557-564replaces the free-form snapshot-failure message with fixed text.
I re-audited the surface field by field: LocalErrorSummary.kind normalizes to a closed set, its summary is built only from validated vocabulary, validated numbers or fixed text, and the receipt's localErrorSummary / errorSummary / preview only duplicate that. No arbitrary prompt, tool-argument or file content can reach a parent receipt at this head.
The cap discriminator also held: #wouldOverflow() stays prospective and #overflowShape() computes exact events|bytes|both from projected values. The event-only test now requires exceeded === "events" and proves bytes stayed under the cap — the both-accepting weakness is fixed.
major — the producer authority is still not identity-gated
packages/agent/src/agent.ts:1982-1988:
err?.errorKind === "local_snapshot_failure" || err?.errorKind === "local_buffer_overflow"
? { errorKind: err.errorKind, ... }That copies the label off an arbitrary thrown object. Your own new test at managed-attempt-transaction.test.ts:1452-1475 throws a foreign error self-labeled local_buffer_overflow and asserts terminal.errorKind stays forged — so the test documents the hole rather than closing it.
The structured shape is identity-gated now, so no arbitrary text escapes. But the executor still emits the fixed diagnosis Local staging-buffer overflow; structured diagnostic unavailable. for what may be a provider or custom-stream failure. That is a false attribution surfaced to the parent, and it defeats the "attach only on the private runtime-error path" contract.
Expose one identity-checking helper for both private local error classes and use it in managedFailureMessage and the Agent catch. A forged error must receive neither errorKind nor bufferOverflow.
minor — the "real full chain" test still bridges around the chain
test/tools/subagent.test.ts:309-334 does produce a genuine overflow, which is progress, but then calls createLocalErrorSummary(...) by hand and fabricates a SingleResult. So it skips runSubprocess and receipt construction — a regression in executor extraction or receipt wiring would still pass.
Nothing yet carries one real overflow through runSubprocess → SingleResult → buildTaskReceipt → async job → snapshot → parent rendering. That is the path where the major above lives.
coverage
Real-overflow pins: the byte-overflow and event-cap diagnostic tests (both drive the mechanism and pin stage, discriminator, incoming size and limits). Executor-boundary validation, structured-only formatting, neutral fallback and the fixed snapshot sentence are all pinned, though synthetically. Guards: ordinary provider isolation, generic receipt preview, and the credential/path snapshot test (the previous regex implementation also removed those values, so it does not discriminate).
merge precondition
Conflicts with dev. Rebase preserving the post-compaction ordering and the trust fix, then re-review the resolved delta.
Reviewed by @probepark — method: detached worktree at a38a1578, field-by-field re-classification of every diagnostic property as closed or free-form at the receiving boundary, trace of safeMetadata stripping and executor validation, read of the Agent terminal catch against the new foreign-error test, per-test real-versus-synthetic classification. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:e8c69c2d3729807359b2854311cf9427b42d7b03a2a4a79d93951101601acd87 reviewer:human reviewer-id:probepark evidence:exact-head-a38a1578-leak-closed-but-agent-catch-still-copies-foreign-errorkind
a38a157 to
382ffb3
Compare
|
Rebased onto current dev — exact head The two PR commits were transplanted from base Focused validation at the new exact head (all green):
Security posture (unchanged from the prior fix-forward, re-verified): producer strip of Verdict line is exactly one, — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 382ffb33 — merge blocked. The leak stays closed; the false attribution is unchanged and it does reach the parent.
the leak is still closed
agent-loop.ts:1188-1189 still does delete safeMetadata.errorKind; delete safeMetadata.bufferOverflow;, task/types.ts:441-499 keeps the closed stage/exceeded vocabulary with safe-integer validation, and :529-562 uses fixed snapshot-failure text. No arbitrary prompt, tool or file text escapes through this path.
major — the forged label reaches a parent-visible receipt
Last round I said I would treat this as major only if the false attribution actually surfaces. I traced it this time, and it does:
agent.ts:2008-2010 still trusts a foreign error's self-declared label:
err?.errorKind === "local_snapshot_failure" || err?.errorKind === "local_buffer_overflow"
? { errorKind: err.errorKind, ... }then task/executor.ts:2122-2128 accepts the forged kind, task/types.ts:549-555 converts it to Local staging-buffer overflow; structured diagnostic unavailable., and task/receipt.ts:149-150,302-305 places that sentence in the parent-visible preview and error summary.
So a provider or custom-stream failure is reported to the parent as a local staging-buffer overflow. Whoever reads that receipt debugs the wrong subsystem.
Your own test documents the behavior rather than preventing it — managed-attempt-transaction.test.ts:1524-1528 asserts expect(terminal.errorKind).toBe("local_buffer_overflow") for a forged error while bufferOverflow is undefined.
The requested shared identity-checking extractor is still absent. managedBufferOverflowDiagnostic identity-gates the overflow shape but not errorKind. One extractor used by both managedFailureMessage and the Agent catch, and a forged error gets neither field.
The corrected pin asserts both terminal.errorKind and terminal.bufferOverflow are undefined for forged labels.
minor — the real-chain test still bridges around the chain
test/tools/subagent.test.ts:396-415 hand-calls createLocalErrorSummary(...) and fabricates singleResult, so it skips runSubprocess and receipt construction — the two boundaries where the major lives. Drive a genuine overflow through runSubprocess → SingleResult → buildTaskReceipt → async job → snapshot → renderer.
Reviewed by @probepark — method: detached worktree at 382ffb33, traced the forged errorKind from the Agent catch through executor acceptance and summary conversion into the receipt preview to determine whether the misattribution is parent-visible, and read the existing forged-error test to see which side of the contract it pins. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:1ee05906ec17c7dd0f4a6e0d17b59b2d59a6d3565a298d47f3d4cc97c588427b reviewer:human reviewer-id:probepark evidence:exact-head-382ffb33-forged-errorkind-reaches-parent-receipt-preview-as-a-local-overflow-diagnosis
…ow (#4618) ManagedAttemptBufferOverflowError surfaced as one static sentence, so a subagent killed by the provisional staging cap was indistinguishable from a provider or context-window failure — the exact misread reported in recordored." and the error text carried no stage, counts, or limits. The overflow now reports its shape everywhere it can reach: - agent: the typed error carries stage, staged event/byte counts at rejection, and both caps; the message keeps its stable prefix (session retry policy prefix-classifies on it) and appends a shape-only parenthetical stating this is a local staging-buffer limit that reproduces on re-issue, not a provider/context-window failure. Baked into the error itself because the non-retryable local exit path surfaces the thrown error, not the managedFailureMessage wrapper. - coding-agent: the executor retains a bounded, redaction-safe localErrorSummary (closed kind set, sanitized summary) from the subagent's terminal assistant error; receipt preview, errorSummary, subagent tool output, and the await renderer name the local kind and carry the diagnostic instead of the generic error preview. Does not touch cap configurability (#4602) or superseded-delta reclamation (#4610). Lore-id: 4618-buffer-diagnostics Constraint: message prefix must stay byte-identical for session prefix classification Constraint: diagnostics must be shape-only (no provider/prompt text can reach a parent receipt) Constraint: do not duplicate #4602 configurability or #4610 delta reclamation Rejected: enriching managedFailureMessage only | the non-retryable local exit surfaces the thrown error, not the wrapper Rejected: enlarging the caps | workaround path ships with #4602 Confidence: high Scope-risk: moderate Reversibility: trivial Tested: byte-cap + event-cap surfaced diagnostics (agent), propagation, redaction, foreign-kind degradation, generic-error fallback isolation (coding-agent) Not-tested: live provider delta-storm reproduction Closes: #4618
…eceipt (#4618) probepark's exact-head re-review of the staged-buffer diagnostics showed the parent still trusted an unauthenticated shape: a foreign provider payload could smuggle errorKind/bufferOverflow through the managed snapshot shell's safeMetadata spread, and the executor interpolated free-form stage/exceeded values and unchecked counters into TaskResultReceipt.preview/errorSummary. local_snapshot_failure also forwarded free-form child message text through regex redaction, which never covered prompt text, tool args, or file content. managedAssistantShell now strips errorKind/bufferOverflow from the snapshot spread (they attach only on the module's own runtime-error paths), and the executor boundary runtime-validates the structured diagnostic -- closed stage/exceeded literals, finite non-negative safe-integer counters, and consistency with the cap claimed to have tripped -- degrading anything else to a fixed neutral sentence. Snapshot and generic local failures render fixed sentences instead of redacted free-form text. The event-cap test now requires exceeded=events with projected bytes proven under the byte cap, and a genuinely tripped overflow pins the full chain through the executor boundary, async job/snapshot propagation, and both renderers with per-line width/tab bounds. Lore-id: 4a1c9e2d Constraint: parent receipts must never interpolate unauthenticated child-controlled text Constraint: legit identity-checked diagnostics from the runtime's own error paths must survive Rejected: regex redaction of free-form messages | cannot cover prompt text/tool args/file content Rejected: instanceof-only producer check | does not survive the cross-package receiving boundary Confidence: high Scope-risk: narrow Reversibility: clean-revert Tested: bun test task/tools suites (479), agent suite (793), package checks both packages Not-tested: full repo CI matrix (delegated to exact-head PR CI)
probepark's exact-head review at 382ffb3 traced a forged `errorKind: "local_buffer_overflow"` from the Agent run catch through the executor boundary into the parent-visible receipt preview: the structured `bufferOverflow` shape was identity-gated but the label was not, so a provider or custom-stream failure was reported to the parent as a local staging-buffer overflow and sent whoever read the receipt to debug the wrong subsystem. Both local-diagnostic fields now come from a single identity-checked extractor (`managedLocalErrorDiagnostic`) shared by both producers of a terminal assistant message. A foreign error gets neither the kind nor the shape, so the receipt degrades to ordinary provider-error text. The forged-label test previously documented the defect (`expect(terminal.errorKind).toBe("local_buffer_overflow")`); it now asserts both fields are undefined for both local kinds. The real-chain pin no longer bridges around `runSubprocess`/`buildTaskReceipt`: a genuine runtime overflow is driven through runSubprocess -> SingleResult -> buildTaskReceipt -> async job -> snapshot -> renderer. Lore-id: 4f21c7a9 Constraint: local diagnostic authority must survive a hostile child -- identity check, never a self-declared label Rejected: validate errorKind at the consumer only | the misattribution originates at the producer and every consumer would need the same check Rejected: keep the label, drop only the shape | a bare local kind still renders a local-overflow sentence in the parent preview Confidence: high Scope-risk: narrow Reversibility: clean-revert Tested: agent managed-attempt suite (67), coding-agent task+tools suites (397+38), both package checks, repo lint Not-tested: full repo CI matrix (delegated to exact-head PR CI)
382ffb3 to
007c4e6
Compare
|
Fix-forward for the major (forged label reaches a parent-visible receipt) — fixed at the producerThe review is exactly right: the shape was identity-gated, the label was not, so Both fields now come from one identity-checked extractor, as requested: // packages/agent/src/agent-loop.ts
export function managedLocalErrorDiagnostic(error: unknown): ManagedLocalErrorDiagnostic | undefined {
if (error instanceof ManagedAttemptBufferOverflowError) {
return { errorKind: "local_buffer_overflow", bufferOverflow: { /* shape */ } };
}
if (error instanceof ManagedAttemptSnapshotError) return { errorKind: "local_snapshot_failure" };
return undefined;
}Both producers of a terminal assistant message consume it — The pin was inverted as requested. The old test documented the defect ( expect(terminal.errorKind, kind).toBeUndefined();
expect(terminal.bufferOverflow, kind).toBeUndefined();minor (real-chain test bridged around the chain) — fixed
verification at the exact head
@probepark @snowykr — re-review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The producer-side identity gate and the independently validated, shape-only receipt rendering are well targeted. The exact PR metadata confirms this review covers head 007c4e6bf28d59a245e598cb81f188d75920fd77. Two changes are required before merge: do not silently remove a public agent export under a fix release, and add an end-to-end negative regression for the forged-label path this patch closes.
Findings / Required Changes
-
[P1] Public API removal must be intentional and release-visible —
packages/agent/src/agent-loop.ts:812-829replaces the exportedmanagedBufferOverflowDiagnosticwithmanagedLocalErrorDiagnostic. Becausepackages/agent/src/index.tsre-exports this module andpackages/agent/package.jsonexposes both the package root and./agent-loop, existing consumers importing the former helper will fail at type-check/module resolution. The package remains0.14.1, while the changelog presents this as a fix. Either restore the prior public export or explicitly make and document this as a breaking release; do not ship an undocumented public API removal. -
[P2] Cover the actual forged-label path through the parent receipt —
packages/coding-agent/test/task/executor-subagent-reminders.test.ts:1845-1870uses an unlabeled terminal error, so it verifies the ordinary provider-error fallback rather than the new Agent producer →runSubprocess→buildTaskReceiptboundary. Generate a foreign error that self-labels as a local failure through the real Agent path, then assert that the parent receives neitherlocalErrorSummarynor a local-overflow preview. The existing agent-level negative test and genuine-overflow positive chain do not exercise that parent-visible negative path together.
CI / Verification
- Exact head checked:
007c4e6bf28d59a245e598cb81f188d75920fd77. - Affected-path checks for the changed agent/task/tool tests passed.
PR contract bootstrapandValidate exact-head PR contractare failed due to the human-gated stale-verdict workflow; treated as non-blocking under the review instruction for human-CI failures.- No PR code was executed by the verification axis; assessment used the published CI artifacts and changed tests.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | PASS | Exact PR-to-head mapping confirmed; scope is focused on local diagnostic authority and the producer contract. |
| A2. Architecture / Correctness / Failure | PASS | Reviewed both terminal producers, identity gate, validation boundary, and receipt propagation. |
| A3. Security / Privacy / Trust | PASS | Reviewed provenance, snapshot stripping, closed-vocabulary validation, and parent-visible rendering. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | CI artifacts pass for affected tests; negative end-to-end forged-label coverage is missing. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | The exported managedBufferOverflowDiagnostic removal is an undocumented public API break. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 007c4e6b — approved. The forged-attribution blocker is fixed, and fixed by removing the trust rather than filtering it.
the fix
Local diagnostic authority now comes from a single identity check on a private class. agent-loop.ts:812-829 identity-checks both fields, and agent.ts:2008-2013 routes through that extractor instead of trusting err.errorKind directly.
That is the right shape. The previous version read a label off an arbitrary thrown value, so anything that could reach the error path could claim to be a local diagnostic. Now an object that merely looks like one carries no authority — which is why the pin at managed-attempt-transaction.test.ts:1505-1536 requires both terminal.errorKind and terminal.bufferOverflow to be undefined for a forged error. Asserting the absence of both authority fields, not just a wrong label, is what makes it discriminating.
disclosure surface is clean
I looked specifically for what a diagnostic feature tends to leak. Parent-visible summaries use only closed literals and checked numeric counters — no absolute paths, no username or tenant path segments, no buffer contents, no credentials, no prompts, no tool arguments. The parent receipt stays generic with no runtime label (executor-subagent-reminders.test.ts:1848-1868).
The shape is observational: genuine retry and fallback behavior is not driven by bufferOverflow or localErrorSummary, so the diagnostic cannot change the decision it is reporting on. That was my other concern with this class of change and it holds.
the coverage complaint is resolved
The synthetic bridge is gone. subagent.test.ts:433-466 creates a genuine overflow and sends its terminal message through runSubprocess before building the receipt; :468-505 carries it through AsyncJob, snapshot, and both render modes, including width bounds and absence of raw tabs.
A synthetic fixture proved the formatter; this proves the chain. Given the previous head asserted on a hand-built object, that distinction was the whole point.
rebase precondition satisfied
Ancestry from base 4ad3726c contains only the three PR commits for the reviewed paths, and there are no unresolved conflict markers in the changed package trees.
Reviewed by @probepark — method: detached worktree at 007c4e6b, traced diagnostic authority to its single identity check and confirmed the forged-error pin asserts absence of both authority fields, audited every parent-visible string for path/credential/content disclosure, and confirmed the overflow chain is driven by a real overflow rather than a constructed message.
gajae.pr-review-verdict.v1 merge-approved sha256:4a071ddd3e0e35628d153ad84d8246b6e8dd2f46855634140e593d98df179149 reviewer:human reviewer-id:probepark evidence:exact-head-007c4e6b-diagnostic-authority-via-private-class-identity-forged-pin-asserts-both-fields-undefined-real-overflow-chain
3 participants
Summary
ManagedAttemptBufferOverflowError(local_buffer_overflow) surfaced as one static sentence —Managed fallback attempt exceeded the provisional event buffer limit— with no stage, no staged event/byte counts, and no caps. In the parent session the whole failure collapsed toTask failed; error recorded., so a subagent killed by the local staging buffer was indistinguishable from a provider or context-window failure (#4618 reports exactly this misread against a 1M-context model).This PR implements only the diagnosability asks from the issue triage — the two items the repo owner explicitly listed as not yet covered by an open PR. Cap configurability stays with #4602 (
GJC_FALLBACK_MAX_STAGED_*); superseded-delta reclamation stays with #4610. Neither is duplicated here.Changes
packages/agent(agent-loop.ts)ManagedAttemptBufferOverflowErrornow carries a shape-onlyoverflowobject: rejectingstage,stagedEventCount/stagedBytesat rejection (retained acrossdiscard()), andmaxStagedEvents/maxStagedBytes..messagekeeps its byte-identical stable prefix (session retry policy prefix-classifies legacy messages on it) and appends a parenthetical naming the stage, counters, limits, and the fact that this is a local staging-buffer limit that reproduces on re-issue — not a provider or context-window failure.managedFailureMessagewrapper, because the non-retryable local exit path surfaces the thrown error (no transport facts → no retry decision →throw err), which is what reachesagent.state.error, the terminal assistant message, and parent task receipts.packages/coding-agenttask/types.ts:LocalErrorSummary+createLocalErrorSummary— kind normalized against the closed local set (foreignerrorKinddegrades to"local", so a hostile value cannot smuggle text into a receipt), summary routed through the existingcreateSetupFailureSummaryredaction/capping pipeline (credentials, tokens, local paths).task/executor.ts: at a terminalstopReason: "error"with a local error kind, retainlocalErrorSummaryintoSingleResult.task/receipt.ts: preview becomesTask failed; local failure (local_buffer_overflow): <summary>instead ofTask failed; error recorded.;errorSummaryand the receipt expose it too.async/job-manager.ts,task/index.ts,tools/subagent.ts,tools/subagent-render.ts: propagatelocalErrorSummarythrough the failed-run outcome into the job, snapshot, tool output, and await renderer (cache signature included).Shape-only by construction: every surfaced field is a closed-vocabulary stage literal or a locally synthesized number — no provider text, thinking, tool arguments, or prompt content can reach a parent receipt through this path.
Tests
managed-attempt-transaction.test.ts): byte-cap overflow namesstage=overflow.preMeasure, staged counts, byte limit, and the "not a provider or context-window failure" clause; event-cap overflow names the event limit with staged counts retained across discard.executor-subagent-reminders.test.ts): a terminallocal_buffer_overflowassistant error propagates intoSingleResult.localErrorSummarywith kind + diagnostic; an ordinary provider error leaveslocalErrorSummaryundefined (fallback isolation).receipt.test.ts): preview surfaces the local kind + summary;createLocalErrorSummaryredacts bearer tokens and local absolute paths and degrades foreign kinds; no-summary case keeps the generic preview.agent-session-fallback-attempt-transaction.test.ts, 20 pass — no retry, no chain charge, prefix classification intact).Verification
bun test packages/agent/test/— 784 passbun test packages/coding-agent/test/task/— 320 passbun test packages/coding-agent/test/agent-session-fallback-attempt-transaction.test.ts packages/coding-agent/test/tools/subagent.test.ts— 66 passbun run check:ts,check:rs,check:schemas,check:plugins,check:tools,check:publish-types,check:node20-baseline,check:public-sync,check:sdk-skills,check:sdk-closure,check:docker-context,check:gjc-ui,lint— all greenscripts/check-visible-definitions.ts,scripts/verify-g002-gates.ts,scripts/rebrand-inventory.ts --strict,default-gjc-definitions.test.ts— all greengit diff --check— cleanCloses #4618
GJC verdict
—
[repo owner's gaebal-gajae (clawdbot) 🦞]