Skip to content

feat(observability): bind execution traces and task-cost metrics to exact attempts (#2676) - #2790

Merged
zaxbysauce merged 6 commits into
mainfrom
fix/issue-2676-execution-attempt-tracing
Sep 15, 2026
Merged

zaxbysauce merged 6 commits into
mainfrom
fix/issue-2676-execution-attempt-tracing

Conversation

@zaxbysauce

@zaxbysauce zaxbysauce commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Closes #2676

PR head: b142e88
(history: 2699b5b reviewed → rebased to dcc875c (base-drift) → PR-review feedback round bbdc5c5 → final-critic rounds 2f51e42 + b142e88)

Summary

Workstream D16: binds execution traces and task-cost measurements to exact attempts and outcomes.

  • New execution_attempt_recorded event kind (65th catalogued, full contract registration): one event per execution attempt, joined to the exact task / call / invocation / generation identity the producer holds, with the closed attempt-class vocabulary denial | attempt | result | duplicate | late | cancelled | provider_failed, explicit capture-coverage lists (captured/unknown), and duplicateOf (required for duplicate) / generation (required for late) enforced fail-open by the recorder.
  • Envelope correlation axes callId + invocationId added to WorkflowIdsSchema (13 → 15 recognized IDs; additive-optional, schema version stays 1); extractWorkflowIds + KNOWN_TELEMETRY_KEYS extended; Stage A's PascalCase sessionID/callID normalized at the recorder.
  • Per-attempt cost block (buildTaskAttemptCost): the six axes latencyMs, inputTokens, outputTokens, cacheReadTokens, estimatedCostUsd, billedCostUsd are number | null — an axis the producer did not hold is strictly null AND listed in unavailable; zero is only ever a known value. The delegation terminal carries tokens only when the provider's own payload attested usage (cost_source === 'reported'), so the legacy zero-default fold cannot leak known-zeros; estimated-vs-billed ride cost_source.
  • Producers wired: delegation lifecycle begin (attempt) and terminal (result/cancelled, latency from dispatch start); Stage A gate route (denial ×4 routes, result for valid_pass, late with the correlation's generation cursor threaded from the pending-gate-task map; duplicate_result stays a disclosed fail-open drop because the seam holds no original record identity).
  • Cohort machinery (task-cohort.ts): deep-copied population snapshot (post-snapshot mutation cannot change a reported denominator), frozen provenance manifests for trigger/WAL/event/host-status (bounded 64 KiB digests; missing sources explicit unavailable), config/version strata, a computed uncertainty channel (present when the sample is below threshold, any cost axis is unavailable, or any outcome is missing), and an explicit causal-rate qualification verdict. Manifests persist under .swarm/observability/cohorts/ (FIFO-20, retention-registry row observability-cohorts).
  • /swarm report gains the "Task attempts (cohort)" section (--json schemaVersion 2, additive); registry details + docs/commands.md regenerated. Existing pairing/savings sections are unchanged and documented as descriptive operational counts — the new section is the qualified-claims surface.
  • Docs: docs/execution-attempt-tracing.md defines the vocabulary, manifest schema, cohort inclusion rules, and the operational-counts vs causal-rate-claims boundary; the event-contract doc gains the kind section and the two new axes; release fragment included.

Disclosed residual: the legacy delegation_end/cost-fold zero-defaults are tracked by follow-up issue #2789 (the new surface is immune by construction and by type).

Invariant audit

  • 1 (plugin init): not touched — no init-path work added; execution-attempt emits are hook-path bounded and fail-open (same construction as the [Workstream A] PR 09 of 12: Keep mandatory lifecycle bookkeeping active and record every Stage A attribution route #2664 Stage A route events); the cohort snapshot runs only on report invocation. Evidence: src/observability/execution-attempt.ts emits via the existing fire-and-forget telemetry seam; no new init registrations.
  • 2 (runtime portability): not touched beyond additive zod fields — no bun: imports anywhere in the new modules (src/observability/execution-attempt.ts, task-cohort.ts use node:crypto/node:fs/node:path only); bun run typecheck clean; emit-line-parity test passes (legacy projection byte-identical).
  • 3 (subprocesses): not touched — no new spawns; cohort manifest reads are bounded synchronous file reads with try/catch fail-open.
  • 4 (.swarm containment): touched — new .swarm/observability/cohorts/ store written only via the explicit directory parameter (report passes the project root); registered in scripts/retention-registry.data.ts (row observability-cohorts) + docs/observability-retention-registry.md; bun run check:retention passes (124 rows).
  • 5 (plan durability): not touched — the receipt WAL is only stat+digest-read for provenance manifests; no mutation of ledger/receipt state; bun run check:registry-citations exit 0 + tests/unit/utils/atomic-write-ratchet.test.ts pass.
  • 6 (test_runner safety): not touched.
  • 7 (test writing): touched — three new bun:test files under 500 lines using canonicalMkdtemp + safeRmRecursive, no real clocks in tests, no new mock.module targets; check:test-file-cap, check:test-tmpdir, check:test-clock, check:mock-cleanup all pass.
  • 8 (session state): not touched — the new modules hold no session-keyed globals.
  • 9 (guardrails/retry): not touched.
  • 10 (chat/system msg): not touched.
  • 11 (tool registration): touched at the event-catalog level — full six-surface kind registration (union member, CATALOG_SOURCE entry with real producer/consumer citations, prose counts 64→65 in code + docs, #### execution_attempt_recorded contract section, envelope-roundtrip fixture); bun run check:events passes ("65 catalogued event kinds, coherent across the TelemetryEvent union, producers, consumers/owners, retention, documentation, tests, and OTel mapping"); check:core-events allowlists the provenance-digest read with a reason class; drift:check --enforce reports only the pre-existing local CRLF artifact (verified local-only on a clean origin/main worktree — the file is untouched by this branch).
  • 12 (release/cache): touched — docs/releases/pending/2676-execution-attempt-tracing.md shipped; version files untouched (release-please owns them).

Test plan

Frozen acceptance checks (base 7aa8b9f8b → head 2699b5b2e, independently re-run by the implementation reviewer and the final critic):

  • C1 (AC1 — joins + record classes): base RED ("no 'execution_attempt_recorded' kind; no callId/invocationId axes") → head GREEN — tests/unit/observability/execution-attempt-2676.test.ts 17 pass / 0 fail (vocabulary, joins, late-requires-generation, duplicate-requires-identity, casing normalization, extractWorkflowIds mapping, real Stage A handler drive, provider_failed fixture drive).
  • C2 (AC2 — known-vs-unavailable cost + manifests): base RED → head GREEN — execution-attempt-cost-2676.test.ts 9 pass / 0 fail (null+unavailable, zero-as-known, invalid→unknown, re-fold stability, real delegation-terminal drive, frozen-manifest persistence + FIFO).
  • C3 (AC3 — stable cohort): base RED → head GREEN — task-cohort-2676.test.ts 9 pass / 0 fail (snapshot-then-mutate, deep-mutation immunity, manifest set + explicit unavailable, uncertainty, qualification, FIFO-20).

Quality gates: bun run typecheck clean; check:events (65 kinds); check:retention (124 rows); check:registry-citations exit 0; retention-registry-rows + atomic-write-ratchet 24 pass; check:core-events OK (allowlisted provenance-digest); check:mock-cleanup, check:test-clock, check:invariants, check:test-file-cap (0 violations), check:test-tmpdir, check:cross-contamination, check:bash-portability, check:gate-portability, check:pending-fragment all pass; scoped biome ci src tests scripts clean (4 pre-existing warnings also on main).

Regression suites: report + report-adversarial (13 pass; the determinism test normalizes the cohort's wall-clock capturedAt exactly as it already normalized importedThisSync), guardrails stage-a family (24 pass), delegation-lifecycle family (30 pass), catalog-contract + envelope-roundtrip + emit-line-parity (731 pass).

Mutation probes (implementation reviewer, tier L): removing unavailable.push → 6 tests RED; inverting the provider-attestation gate → known-zero leak caught; removing the snapshot deep copy → mutation-leak tests RED. All restored GREEN.

Acceptance Criteria -> Evidence

  • AC1 (denial/attempt/result/outcome distinctions and duplicate/late records have explicit joins): frozen check C1 — base RED / head GREEN at 2699b5b (probe: catalog kind + callId/invocationId axes; tests/unit/observability/execution-attempt-2676.test.ts 17 pass / 0 fail: vocabulary, joins, late-requires-generation, duplicate-requires-identity, casing normalization, extractWorkflowIds mapping, real Stage A handler drive, provider_failed fixture drive).
  • AC2 (per-task cost distinguishes known from unavailable; frozen corpus manifests preserve provenance): frozen check C2 — base RED / head GREEN (execution-attempt-cost-2676.test.ts 9 pass / 0 fail: null+unavailable, zero-as-known, invalid-to-unknown, re-fold stability, real delegation-terminal drive, manifest persistence + FIFO-20).
  • AC3 (causal-rate claims require stable cohort, denominator, strata, uncertainty): frozen check C3 — base RED / head GREEN (task-cohort-2676.test.ts 9 pass / 0 fail: snapshot-then-mutate, deep-mutation immunity, manifest set with explicit unavailable, uncertainty, qualification, FIFO-20); /swarm report cohort section with --json schemaVersion 2.

Waivers (or none)

None. Scoped decisions (not waivers), all disclosed: legacy zero-default fold tracked by follow-up #2789; Stage A duplicate_result stays a fail-open drop (no original identity at the seam); computePairing/computeSavings documented as descriptive operational counts (docs/execution-attempt-tracing.md section 5).

PR-review feedback round (post-publication)

A full swarm-pr-review run (6 base lanes + 11 risk-family micro-lanes, 2 independent reviewer shards, critic challenge) on the published head verified 10 actionable findings and rejected 11 with evidence (transparency ledger in the run artifacts). Fixes shipped in bbdc5c5 + b142e88:

  • PRR-001 (CRITICAL, CI-red): the new modules sat in src/observability/, violating the directory's issue-[Observability PR 01/23] Define canonical event contract, correlation, and stream inventory #2029 no-I/O contract (no node:fs, ./-sibling imports only) — red on unit shards ubuntu-4 + macos-4. Fixed by relocating execution-attempt.ts and task-cohort.ts to src/services/ (tests to tests/unit/services/), repointing all citations (catalog producer, registry rows, gate allowlist, docs), and amending the frozen acceptance checks C1–C3 via sanctioned CHECK_WRONG amendments with fresh base-RED → head-GREEN replays. no-io.test.ts untouched and green (112 pass).
  • PRR-003 (HIGH): /swarm report cohort section had zero content coverage — new integration test seeds real attempt rows through the DB and asserts the fold (denominator, per-class counts, cost known/unavailable split, uncertainty, qualification) in both JSON and markdown.
  • PRR-002 (Copilot's inline comment): initially rejected on a false premise (the reviewer believed the Node sidecar never reaches report.ts). The final critic disproved that rejection — the plugin bundle builds --target node — and then found the deeper bug: the bun→node runtime fallback relied on .trim() truthiness, so under Node it yielded the version-less "bun" and sha256'd it into the host-status digest. Fixed with a pure deriveRuntimeLabel branching on bun presence, the derived label persisted into snapshot.strata, and a digest-tie test (digest === sha256(persisted label), never the constant sha256("bun")), verified under real Node v24.16.0 by the critic.
  • PRR-004/005/009/014/015/016/020: stale 64-entry doc prose, report-header honesty, FIFO eviction-order pinning + per-file unlink tolerance, registry citation drift, generation:0 / empty-duplicateOf boundary tests, emitDelegationBegin producer test.

Gates after the round: no-io 112 pass, contract 731 pass, all suites green, typecheck/events/retention/citations/core-events/invariants/test-clock/file-cap clean, biome ci clean (4 pre-existing warnings), frozen checks C1–C3 GREEN after CHECK_WRONG amendments. Independent reviewer APPROVE (per-item) + final critic APPROVE (Round 3, real-Node-verified) at head b142e88 / tree e181290bd65fb6949818fe4c87d49242a1fb2f8c.

@github-actions

Copy link
Copy Markdown
Contributor

Drift check report

Found 2 drift finding(s): 0 error, 0 warning, 2 notice.

required-check-contract (2)

  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not yet required by the captured ruleset
  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not present for every expected event in captured external workflow evidence

Test User added 3 commits September 15, 2026 03:22
…xact attempts (issue #2676)

Workstream D16: one execution_attempt_recorded event per attempt with the
closed denial/attempt/result/duplicate/late/cancelled/provider_failed
vocabulary, callId+invocationId envelope join axes, an unknown-honest
per-attempt cost block (null + unavailable, never 0), a snapshot-qualified
task-cohort report with frozen provenance manifests (FIFO-20), and the
/swarm report task-attempts cohort section (json schema v2).
…lt records the late class (issue #2676 review finding)

The guardrails pending-gate-task map holds the correlation's workflow
generation at the toolAfter seam; pass it through StageAGateRouteEventInput
so production late_result routes become recorded late attempts instead of
fail-open drops. duplicate_result still holds no original identity and
stays a disclosed fail-open drop.
…p surface, provider_failed fixture, duplicate disclosure)

- report registry details + docs/commands.md now describe the task-attempts
  cohort section and --json schema v2 (approved-plan item 12, dropped in the
  first commit without disclosure)
- provider_failed class gets a recorder-level fixture drive (AC1 detail)
- the duplicate class row and the release fragment disclose that production
  duplicate_result routes stay fail-open drops (no duplicateOf at the seam)
- stage-a-route comment updated for the threaded generation
@zaxbysauce
zaxbysauce force-pushed the fix/issue-2676-execution-attempt-tracing branch from 2699b5b to dcc875c Compare September 15, 2026 08:24
@zaxbysauce
zaxbysauce requested a balanced review from Copilot September 15, 2026 08:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change touches invariant-sensitive guardrail hook paths, the telemetry catalog, and the delegation lifecycle, adds a new filesystem write to the previously read-only report command, and carries subtle unknown-vs-zero provenance semantics (including a runtime-mislabel bug) that warrant final human review.

Pull request overview

This PR implements Workstream D16 (issue #2676): it makes execution traces and task-cost measurements bindable to the exact attempt and outcome that produced them, so aggregate reporting can distinguish honest "known" values from "unknown" ones and only make causal-rate claims over a stable, snapshotted cohort. It adds a new execution_attempt_recorded telemetry kind (fully registered across the event catalog, envelope, legacy adapter, retention registry, and docs), two new envelope correlation axes (callId/invocationId), producer wiring in the delegation lifecycle and Stage A gate route, cohort-snapshot machinery with frozen provenance manifests, and a new /swarm report "Task attempts (cohort)" section.

Changes:

  • New execution-attempt.ts recorder (closed attempt-class vocabulary, unknown-honest number|null cost block, fail-open validation) plus producers in delegation-lifecycle.ts and stage-a-route.ts.
  • New task-cohort.ts snapshot/report/qualification module with deep-copied population, bounded provenance manifests, and FIFO-20 persistence under .swarm/observability/cohorts/.
  • Catalog/envelope/legacy/retention-registry registration (64→65 kinds, 13→15 workflow IDs), /swarm report v2 JSON + cohort section, and supporting docs/release fragment.
File summaries
File Description
src/observability/execution-attempt.ts New recorder; builds cost block and emits the new event fail-open
src/observability/task-cohort.ts New cohort snapshot, report, manifest capture, FIFO persistence
src/observability/envelope.ts Adds callId/invocationId correlation axes to WorkflowIdsSchema
src/observability/legacy.ts Maps new axes in extractWorkflowIds; adds KNOWN_TELEMETRY_KEYS entry
src/observability/catalog.ts Registers 65th kind; reflows shifted producer line citations
src/telemetry.ts Adds execution_attempt_recorded to the TelemetryEvent union
src/background/delegation-lifecycle.ts Wires begin (attempt) and terminal (result/cancelled) producers
src/hooks/guardrails/stage-a-route.ts Route→class mapping and Stage A execution-attempt producer
src/hooks/guardrails/index.ts Threads the pending-gate-task generation cursor for late records
src/commands/report.ts Folds attempt rows into a snapshot cohort; adds v2 section (runtime-strata issue)
src/commands/registry.ts / docs/commands.md Documents the new /swarm report cohort section
scripts/retention-registry.data.ts / docs/observability-retention-registry.md Registers the observability-cohorts store
scripts/check-core-events-usage.ts Allowlists the cohort provenance-digest read of events.jsonl
docs/execution-attempt-tracing.md / observability-event-contract.md / releases/pending Defines vocabulary, manifest schema, cohort rules; release fragment
tests/unit/observability/*, tests/unit/commands/report.test.ts New/updated tests for recorder, cost, cohort, envelope fixture, determinism
Review details
  • Files reviewed: 23/23 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/commands/report.ts Outdated
const taskAttemptSnapshot = snapshotTaskAttemptCohort({
tasks: taskAttemptPopulation,
directory,
strata: { runtime: `bun ${process.versions.bun ?? ''}`.trim() },

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against source and closed in the feedback round (commits bbdc5c5 + b142e88):

The premise was partially right and the conclusion was fully right. You were correct that report.ts hardcodes bun ${process.versions.bun ?? ''} and that this poisons the frozen host-status provenance. Two corrections to the record, both caught by our final critic and verified empirically under real Node v24.16.0:

  1. The first fix attempt (dropping the caller-supplied stratum to rely on the module's bun→node fallback) did NOT cure it: the fallback (\bun ${process.versions.bun ?? ''}`.trim() || `node ...`) is dead code under Node — \"bun ".trim()` is the truthy literal "bun", so the node branch never fired and the digest stayed `sha256("bun")`.
  2. The shipped fix extracts a pure deriveRuntimeLabel(versions) that branches on process.versions.bun !== undefined (bun-first, since Bun also defines versions.node), persists the derived label into the snapshot strata, and a new digest-tie test asserts host.digest === sha256(persisted label) and !== sha256("bun") — the exact poisoned constant you flagged.

Also closed in the same round: the report cohort section had no content-level test coverage (new integration test seeds real attempt rows through the DB and asserts the fold + qualification rendering), and 7 smaller docs/test-hardening findings.

Thanks for the catch — the runtime-mislabel was real and is fixed at head b142e88.

Test User added 3 commits September 15, 2026 04:18
…rvices and close PR-review findings (PRR-001..020)

PR-review round on #2790: the no-io.test.ts directory contract (issue #2029)
forbids I/O in src/observability/ — relocate execution-attempt.ts and
task-cohort.ts (plus their tests) to src/services/, repoint producer/consumer
citations, registry rows, gate allowlist, and docs. Frozen acceptance checks
C1-C3 amended via CHECK_WRONG with fresh base-RED/head-GREEN replays.

Also closes: PRR-003 (report cohort integration tests), PRR-004 (stale
64-entry prose), PRR-005/020 (report header honesty), PRR-009 (FIFO eviction
order pinned + per-file unlink tolerance), PRR-014 (registry citation drift),
PRR-015 (generation:0 / empty-duplicateOf boundary tests), PRR-016
(emitDelegationBegin attempt-class test).
… hardcoding bun (final-critic round on PRR-002)

The reviewer's PRR-002 rejection was wrong: the plugin bundle builds
--target node (package.json), so /swarm report executes inside the OpenCode
Node sidecar where process.versions.bun is undefined — the hardcoded
'bun <undefined>' stratum fed a sha256('bun') host-status digest. Drop the
caller-supplied stratum and let captureHostStatusManifest derive the real
runtime via its bun-to-node fallback; regression test pins the derived
label is never the bare literal 'bun'.
…esence (final-critic round 2)

The bun-to-node fallback relied on .trim() truthiness — 'bun '.trim() is the
truthy 'bun' under Node, so the node branch was dead code and the host-status
digest stayed sha256('bun') on the Node sidecar. Extract deriveRuntimeLabel
branching on process.versions.bun presence, persist the derived label into
snapshot.strata (serialized-manifest honesty), and replace the vacuous
regression test with helper branch tests plus a digest-tie assertion
(digest === sha256(persisted label), never the constant sha256('bun')).
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

🤖 Multi-Stage PR Review

Pipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize)
Commit reviewed: dcc875c013ca


PR Reviewer — opencode-swarm

🔍 PR Intent

Reconstructed obligation list from PR description, issue #2676, and diff.

  • O-001 Bind each execution attempt to exact task/call/invocation/generation identity with a closed vocabulary (denial | attempt | result | duplicate | late | cancelled | provider_failed)
  • O-002 Add callId + invocationId to WorkflowIdsSchema (13 → 15)
  • O-003 Per-attempt cost block: six axes (latencyMs, inputTokens, outputTokens, cacheReadTokens, estimatedCostUsd, billedCostUsd) as number | null; unknown axes strictly null PLUS listed in unavailable, never 0
  • O-004 Wire producers: delegation lifecycle (begin → attempt, terminal → result/cancelled), Stage A gate route (denial ×4, valid_passresult, late_resultlate, duplicate_resultduplicate)
  • O-005 Cohort machinery: deep-copy snapshot, frozen provenance manifests (trigger/WAL/event/host-status), config/version strata, uncertainty channel, causal-rate qualification
  • O-006 /swarm report gains Task attempts (cohort) section, --json schema v2
  • O-007 Register new .swarm/observability/cohorts/ store in retention registry
  • O-008 Register execution_attempt_recorded as 65th catalog kind
  • O-009 Add provenance-digest allowlist entry to check-core-events-usage.ts

📦 Implementation Summary

The PR adds a new execution_attempt_recorded telemetry event (65th catalog kind), bound to exact task/call/invocation/generation identity. It wires three producers: delegation lifecycle begin/terminal and Stage A gate route. The per-attempt cost block uses a null+unavailable pattern for missing axes. A new cohort module (task-cohort.ts) provides deep-copy snapshots, frozen 64 KiB SHA-256 provenance manifests, and causal-rate qualification. /swarm report folds the new rows through the cohort and renders a "Task attempts (cohort)" section (schema v2). The new .swarm/observability/cohorts/ FIFO-20 store is registered in the retention registry.


✅ / ⚠️ / ❌ Intended vs Actual

Obligation Status Evidence (file:line)
O-001 SUPPORTED src/observability/execution-attempt.ts:1-271 — closed 7-class vocabulary, exact join axes, explicit captured/unknown coverage
O-002 SUPPORTED src/observability/envelope.ts:151-163callId + invocationId added to WorkflowIdsSchema; legacy.ts:655-663 extracts both
O-003 SUPPORTED src/observability/execution-attempt.ts:73-95buildTaskAttemptCost: null + unavailable push; knownCostValue at :61-67 rejects invalid inputs
O-004 SUPPORTED src/background/delegation-lifecycle.ts:261-272 (attempt), :351-390 (terminal); src/hooks/guardrails/stage-a-route.ts:83-109 (route→class mapping), :129-145 (emit)
O-005 SUPPORTED src/observability/task-cohort.tssnapshotTaskAttemptCohort deep-copies (structuredClone at :127); captureFileManifest bounded 64 KiB digests at :136-162; buildTaskCohortReport at :261-338
O-006 SUPPORTED src/commands/report.ts:72 (SCHEMA_VERSION = 2), :57-80 (cohort collection), :322-329 (snapshot call), :412 (cohort render)
O-007 SUPPORTED scripts/retention-registry.data.ts:4444-4468observability-cohorts row with FIFO-20, derived-rebuildable, retention-registry doc updated
O-008 SUPPORTED src/observability/catalog.ts:1292-1318 — new execution_attempt_recorded entry; src/telemetry.ts:177 union addition
O-009 SUPPORTED scripts/check-core-events-usage.ts:58-61provenance-digest entry for task-cohort.ts

🚨 Confirmed Findings

[LOW] Filename collision under concurrent same-millisecond snapshots with identical manifest content

  • Location: src/observability/task-cohort.ts:152
  • Why it matters: If two snapshotTaskAttemptCohort calls occur within the same millisecond AND produce manifest entries with byte-identical digests (e.g., all sources unavailable in both calls, same host_status digest), manifestFileName produces the same filename and writeFileSync silently overwrites the earlier manifest.
  • Evidence: Line 152: const stamp = Number.isFinite(epoch) ? epoch : 0; where epoch = Date.parse(capturedAt). Date.parse operates at millisecond precision. The suffix (createHash('sha256').update(combined).digest('hex').slice(0, 12)) is identical for identical manifest content. The persistence at :175 uses writeFileSync (no flag: 'x'), so the second write overwrites the first.
  • Fix direction: Add a monotonic counter (e.g., _counter in a closure, persisted in the filename) or use the ISO string's sub-millisecond component (capturedAt.split('T')[1].replace(/[:.]/g, '')) as a tiebreaker. Alternatively, open with flag: 'wx' (Bun/Node ≥22) to fail-fast on collision rather than overwrite.
  • Confidence: Low. Practical likelihood is minimal: callers control now, the manifest content (4 sources including live file stats) typically differs between snapshots, and the window is one millisecond. The overwrite is semantically equivalent to FIFO eviction. The cohort store is described as "provenance evidence," not authoritative state.

🔬 Unverified but Plausible Risks

  • Risk: structuredClone in deepCopyRecord (task-cohort.ts:127) falls back to JSON.parse(JSON.stringify(record)). The fallback loses undefined values, BigInt, Symbol, and circular references — but TaskAttemptPopulationRecord contains only plain JSON-compatible fields (strings, numbers, objects, arrays), so this is not a practical concern for the actual data shapes.
  • Risk: The execution_attempt_recorded producer paths add emit() calls inside existing hook catch blocks (delegation lifecycle emitDelegationBegin/emitDelegationCostObservation, Stage A recordStageAGateRoute) that were already fail-open. Since the execution-attempt recorder is itself fail-open, the nesting preserves fail-open semantics — but no new isolation is added.

🧪 Test / Coverage Gaps

  • Gap: No test for duplicate class + duplicateOf through recordExecutionAttempt end-to-end (only the "no duplicateOf → refused" case is covered; the "with duplicateOf → recorded" path is only exercised by the provider_failed fixture test at a different call site). Minor: the code path is trivially simple.
  • Gap: No concurrent snapshot test (would validate the filename-collision finding above, but the practical likelihood is low enough that a test would be testing a theoretical race condition).

📋 Shipped-vs-Claimed Gaps


📝 Merge Recommendation

APPROVE

This is a thorough, well-tested observability implementation. The single confirmed finding (filename collision) is low-severity, has a clear fix path, and does not affect any authoritative state. The implementation correctly:

  • Uses strict null + unavailable list for unknown cost axes (never 0)
  • Deep-copies the cohort population before returning
  • Captures manifests BEFORE returning the snapshot (the correct order)
  • Maps Stage A generation cursor into late execution-attempt records
  • Qualifies cohort reports explicitly (no silent false causal claims)
  • Registers the new store with a compliant retention-registry row (FIFO-20, keyed by content-addressed name)
  • Wires the catalog entry, envelope schema, legacy extractor, and CI gates in lockstep
Check Result
No CRITICAL findings
No unresolved STEALTH_CHANGE
No UNSUPPORTED obligations
Test coverage adequate
No hardcoded secrets
All async errors handled
Input validation present
No broken agent role boundaries
Prompt format contracts intact
Lockfile consistent ✅ (no lockfile changes)

🔁 Validation provenance

Item Decision Reason
Filename collision (task-cohort.ts:152) KEPT (LOW) Confirmed: Date.parse millisecond precision + identical manifest digests → filename collision. Practically unlikely but structurally real. Fix path clear.
duplicateOf double-bounded in execution-attempt.ts:242-243 DROPPED Trivially redundant, not incorrect. No + line introduced it — the function is self-contained.
duplicate class never produced at delegation terminal DROPPED By design: the seam holds no original record identity (disclosed in PR).
unknown as default outcome for non-terminal non-cancelled statuses DROPPED By design: "never guess" per docs and the provider_failed class comment.
structuredClone fallback for non-cloneable values DROPPED Not a regression; the data shapes (strings, numbers, objects) are JSON-compatible.
stage-a-route.ts _internals exports nothing DROPPED Pre-existing; no + line introduced it.
Missing duplicateOf in Stage A route execution-attempt emit DROPPED By design: Stage A duplicate_result routes stay fail-open drops (disclosed); duplicateOf is only for callers who supply it.
Missing --json cohort test in report.test.ts DROPPED execution-attempt-2676.test.ts and task-cohort-2676.test.ts cover the cohort machinery directly; the report test validates the full integration with deterministic-strip for volatile fields.
Blind-spot: FIFO transient (write count > 20 before trim) DROPPED Brief window (one call) before trim runs; harmless.
Blind-spot: report.ts JSON.parse on untrusted payload DROPPED The execution_attempt_recorded rows are produced by the local emit() function — not external input. Quarantine is still applied upstream in the sink.

🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

PR review round — closure ledger (swarm-pr-review → swarm-pr-feedback)

Full run: 6 base lanes + 11 risk-family micro-lanes (all attested), 35 raw candidates → 21 deduped, 2 independent reviewer shards, critic challenge on HIGH/CRITICAL. Review verdict: REQUEST_CHANGES → fixes shipped → reviewer APPROVE → final critic APPROVE (3 rounds) at head b142e88.

id source item outcome evidence
PRR-001 no-io CI red + TEST_COVERAGE lane src/observability I/O-contract violation (node:fs + import scope) FIXED relocated to src/services/ (+ tests), citations repointed, frozen checks amended CHECK_WRONG with fresh RED→GREEN; no-io 112 pass untouched
PRR-002 Copilot inline runtime strata hardcoded bun / mislabel under Node FIXED (2 rounds) initial fix incomplete (dead-code fallback); final critic caught it; deriveRuntimeLabel bun-presence branch + strata write-back + digest-tie test; verified under real Node v24.16.0
PRR-003 OBLIGATION lane report cohort section zero content coverage FIXED tests/unit/commands/report-cohort-2676.test.ts (JSON fold + markdown + empty cohort)
PRR-004 DOCS lane stale 64-entry prose (lines 6, 238) FIXED 65-entry + corrected math
PRR-005 unclassified UR-1 report header 'Read-only' vs manifest write FIXED header reworded 'Read-only over the stores it queries'
PRR-006 RUNTIME lane stage-a recorder throw into toolAfter INVALID emit() body fully try/catch'd ('must never throw'), warn()/builder cannot throw; empirically probed 4 invalid inputs — no throw
PRR-007 WIRING lane captured/unknown lists not consumed by fold BY DESIGN per-axis coverage already aggregated via cost.unavailable; lists are event-contract fields
PRR-008 WIRING/API lanes callId/invocationId not SQLite columns BY DESIGN payload_json serves the fold; indexed columns = future migration if needed
PRR-009 RUNTIME lane FIFO eviction order unpinned; unlink abort FIXED newest-20 survival asserted by stamp; per-file unlink try/catch
PRR-010 RUNTIME lane deepCopyRecord circular-ref throw INVALID records derive from JSON.parse — cycles unrepresentable; primary call site safe
PRR-011 WIRING lane invocationId/retryIndex/knowledgeTraceId unused BY DESIGN forward-compat axes; honestly listed in unknown
PRR-012 OBLIGATION lane provider_failed fixture-only INVALID AC1 requires a fixture drive (exists); producer not mandated; disclosed
PRR-013 OBLIGATION lane duplicate_result handler path untested INVALID refusal exercised recorder-level + mapping table; disclosed on 3 surfaces
PRR-014 DOCS lane registry citation :187 drift FIXED repointed (now :209 post-shift), gates green
PRR-015 micro TI generation:0 / duplicateOf:'' boundaries FIXED both boundary tests added
PRR-016 OBLIGATION lane emitDelegationBegin producer untested FIXED attempt-class producer test added
PRR-017 DOCS lane invariants doc 'thirteen IDs' INVALID historical #2029 record, immutable by convention
PRR-018 RUNTIME lane startedAt NaN INVALID contained by construction (verified end-to-end → null + unavailable)
PRR-019 micro concurrency FIFO concurrent over-eviction ACCEPTED (LOW) provenance-only, regenerated per report; per-file tolerance reduces blast
PRR-020 unclassified UR-4 byte-identical claim vs capturedAt FIXED header scoped to markdown; JSON capturedAt normalization disclosed
PRR-021 WIRING-DUP lane (CRITICAL claim) duplicate_result cannot produce records INVALID disclosed on 4 surfaces (docs table, lifecycle comment, commit message, release fragment); issue's own design contract

External signals

  • Copilot inline comment → PRR-002 → FIXED (see PR thread reply; initial rejection rationale corrected — the critic disprove the reachability premise but confirmed the defect via the dead-code fallback).
  • CI unit ubuntu-4 + macos-4 red → PRR-001 → FIXED.
  • CI unit ubuntu-6 red (evidence-summary-adversarial, not in diff) → flake: 25 pass / 0 fail standalone at this head → rerun.
  • Drift bot 2 notices → pre-existing RULESET_DIVERGENCE, non-blocking.

Gates after fixes

no-io 112 · moved+new suites 54 · contract 731 · stage-a/lifecycle 37 · report+cohort all green · typecheck/events/retention/citations/core-events/invariants/test-clock/file-cap clean · biome ci clean (4 pre-existing warnings) · frozen C1–C3 GREEN after CHECK_WRONG amendments · reviewer APPROVE per-item · final critic APPROVE round 3 (real-Node-verified) at b142e88 / tree e181290bd65fb6949818fe4c87d49242a1fb2f8c.

@zaxbysauce
zaxbysauce added this pull request to the merge queue Sep 15, 2026
Merged via the queue into main with commit 114338f Sep 15, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Workstream D] PR 16 of 17: Bind execution traces and task-cost metrics to exact attempts and outcomes

2 participants