feat(sync): W1 measurement contract — source-attributed sync instruments (I1–I9) - #2033
Conversation
…nts (I1–I9) Attribute sync cost to its trigger. Today no exported metric can answer "which lane is consuming the store": `operation` (which encodes the admission source) never reaches an instrument, per-operation bytes are folded into an in-process sum that is never exported, the changelog lane contributes zero bytes, and `outcome` is inert for `sync-global`. #2003 solved source-attributed pressure on the instantaneous snapshot only. Adds nine instruments across both request lanes, attributed to the closed `SYNC_ADMISSION_SOURCES` vocabulary, plus generator-emitted query artifacts validated by a real PromQL parser. Also fixes a real shutdown defect found during implementation. Shipped code calls `stopTelemetry()` before `server.close()` and `agent.stop()`, so the providers are torn down and `rebuildMetrics()` rebinds `getMetrics()` to a no-op meter while parent-side sync is still running — terminal catch-up records export while the attempts, bytes and active time belonging to them are silently dropped. Terminating the catch-up worker does not quiesce that work: `handleInvoke` awaits agent methods with no signal and no cancel hook. Both A24 ordering tests fail against the shipped sequence, so this is a regression proof, not a synthetic mutant. Key decisions: - `source` reaches the record sites via AsyncLocalStorage, not a threaded parameter. Threading would have put `source` in the same scope as `syncPageFetchCoalescingKey`, so the mutant guarding the highest-severity constraint would have guarded a hazard the implementation created. Ambient context makes it structural, and the changelog lane's `runResync` fallback inherits the correct label instead of reporting `unspecified`. Follows the existing `chain/src/rpc-usage.ts` idiom. - `runSyncSingleFlight` takes an explicit source at the three generic scopes: they coalesce above the admission boundary, so an ambient read there would label every generic join `unspecified` and stop I6's cross-family check firing. - No `timeout` in the attempt-outcome vocabulary: the router and pool emit at least seven incompatible deadline shapes and the only classifiers are `.message.includes(...)`. Any pre-response rejection that is not caller cancellation is `transport_error`. - Validation rejection is marked with a non-enumerable tag, never a replacement error — `makeLegacySyncBusyError`'s message is matched by `isSyncBackoffWorthyError`, so replacing it would silently change backoff and `failedPhases` accounting. - The terminal flush is bounded per leg. `MetricReader.forceFlush()` applies no timeout without `timeoutMillis` and leaves the trailing exporter flush unwrapped; `BasicTracerProvider.forceFlush()` takes no arguments and must be raced. Per-leg rather than an outer race, so each bound stays independently observable. - Both subscribe mint sites return 503 + Retry-After before any id is generated during shutdown; dedupe and replay are unaffected. Verification: agent packet 9 files/185 tests, CLI 6/6 files/149 tests, node-ui 19/19, packet reachability gate 16/16, four observability commands green on Windows and in the Linux CI shape, promtool SUCCESS 66 rules via a pinned multi-arch digest. Instrumentation overhead 0.005-0.006 ms/page against a 1 ms ceiling. A17 measured against a pristine pre-W1 referent: 154 + 31 = 185 exactly. Refs #2018, #2006 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
The AsyncLocalStorage design was approved on conditions; this lands them.
R1 — the ambient source reaches every recording path. Four per-lane cases
(durable, changelog, shared_memory, swm_recovery) drive a real send inside the
admitted boundary and assert the EXACT expected literal, with
`matching(I1, {source: 'unspecified'})` empty. Plus the case that was the
headline argument for ALS over parameter threading and had never been tested: a
changelog responder returns `resync`, `runResync` re-enters the legacy lane, and
both the changelog and legacy attempts carry the one operation's source, with I2
bytes on the legacy leg. Under parameter threading that fallback has no source in
scope and would have reported `unspecified` — the silently-partial denominator.
R2 — no context-loss boundary between the scope and the record sites; the chain
from `withSyncAdmissionSource(source, work)` to `await params.send(...)` is a
plain await chain, with no emitter or pooled-connection callback between them.
R5 — audited: no detached sender inside the admitted boundary. The structural
form is stronger than enumerating spawns — only five files in the package
reference `sendSyncRequest`/`PROTOCOL_SYNC_CHANGELOG`, and none is in the
store-commit or verification tree, so detached work there fails the harmful
condition by construction.
These discriminate rather than merely passing: mutating `withSyncAdmissionSource`
to a pass-through kills 10 tests, including all four lane cases, the fallback and
the detached-spawn case.
The detached-spawn test is named and commented for what it actually proves. As
first written it claimed detached work "does NOT inherit its source" and that it
"pins the property by construction so one cannot be introduced silently" — but it
wraps the spawn in its own `withSyncAdmissionSource`, so it reported that source
because the test supplied one, not because detachment prevents inheritance. A
bare detached sender added later would inherit `catchup-foreground` and the test
would stay green. It now pins the MITIGATION, and says so. A test asserting the
bare case DOES inherit is deliberately absent: it would encode the hazard as
expected behaviour and make the eventual fix read as a regression.
The real protection is the audit plus the standing hazard note: a record site
running outside an established scope reports `unspecified` (thread boundary —
loud) or inherits a stale label (bare detached spawn — silent, and directional
when that label is an eligible family). That is ALS's one structural weakness
against a threaded parameter, where a missing source is a compile error.
Refs #2018, #2006
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Two defects found by the mutation matrix, plus the test hygiene they exposed. **§7.3's "zero `unspecified` samples" gate was unreachable.** `curator-meta-refresh.ts` fetches `plane=durable`/`phase=meta` — inside §7.2's decision filter — from outside any admission boundary, so every observation window on any node that had ever joined a private Context Graph was invalidated. Named rather than weakened. `unspecified` must keep meaning "we don't know", and the code's own comment already called this "the small control-plane subset", so the information was in hand and being thrown away. Restating the gate as a bounded share was rejected: it needs an unprincipled threshold and re-opens the exact hole the gate exists to close, by letting a genuinely unattributed sample hide underneath it. The label rule is now explicit: **`source` is the operation that TRIGGERED the traffic; `control-plane` is the trigger when, and only when, no sync operation is.** `refreshMetaFromCurator` has three callers and one runs nested inside an admitted changelog sync, so unconditional labelling would have overridden the enclosing source — moving bytes out of an eligible family and under-counting the very lane §7.3 evaluates. Wrong in the conservative direction is still wrong. The guard tests store PRESENCE, not the sentinel value. `'unspecified'` is produced both by "no scope" and by "an admitted operation whose caller omitted `source`" (`normalizeSyncAdmissionSource(undefined)`), so a value comparison would relabel genuinely unattributed traffic as `control-plane` — laundering a loud "we don't know" into a confident answer, inside the change made to stop that gate being unreachable. **A10 was true but unprotected.** M9 — swapping I9's buckets for `OP_DURATION_BUCKETS`, which tops out at 120 s — survived every suite in the repo. The only 305 s sample lived in an attribute-key sweep that never inspects `buckets.boundaries`, the CLI suite mocks `record`, and the harness exposed no bucket accessor at all. The buckets were correct; nothing would have noticed if they stopped being. It now asserts the top finite boundary against both observed jobs (305 s and 382 s), never an index, with a sample-count precondition so an empty histogram cannot satisfy it for the wrong reason. M9 dies at `expected 120000 to be greater than or equal to 382000`. Both discriminators were verified able to fail rather than assumed: the value-comparison variant reddens the presence-vs-value assertion only; the unconditional variant reddens that one and the nested-source assertion. Neither rides on the other. Also: blockers now release in `afterEach`, which takes M10's failure count from 12 to 3 by removing cascade from a process-global backpressure snapshot (honest kill set is 3 genuine, 9 cascade); and a fetch counter asserting `> 0` under a name promising a single-run count is tightened to an exact `toBe(2)`, measured rather than guessed, and proven to fire first under M5-durable. Refs #2018, #2006 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Found by review after the control-plane sweep landed. `3: the responder auth-lookup call site carries no ambient scope` referenced `authorizeSyncRequest` only in a comment. Its body defined a local closure, invoked it from the test body, and asserted that the test body had no ambient scope — a result determined entirely by its own setup. Wrapping the real responder path in `withSyncAdmissionSource(...)`, which is precisely the regression the name promised to catch, would have left it green. A test that claims a path is pinned when it is not is worse than an acknowledged gap: a gap gets revisited, a green test does not. So the assertion is removed and the numbering keeps its gap, so this file and the plan's five-assertion table stay in correspondence. The coverage argument for not writing a metrics-level responder test still holds and is preserved in the comment: the responder shares assertion 1's guard branch exactly — both reach the refresh with no ambient scope — so it would prove the same fact twice while reading as independent coverage. Closing it properly needs an integration harness driving the real `authorizeSyncRequest` with a dependency object that records `hasSyncAdmissionSource()`; that is a follow-up. Until then the responder half is reasoned from source rather than asserted, and §12 records it next to the A24 wiring residual instead of implying coverage that does not exist. Refs #2018, #2006 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Two pieces of residue from deleting the responder assertion. The `hasSyncAdmissionSource` import survived the removal of its only caller. The build does not catch it — the symbol still appears in a comment — and a dead import that exists because of a deleted test is exactly the residue that misleads the next reader into thinking the file still exercises that path. The residual comment also named `authorizeSyncRequest`. The real export is `authorizePrivateSyncRequest`. A comment naming a function that does not exist is the same class of defect as the assertion it replaced: an unverified claim stated confidently, which outlives anyone's memory of its provenance. Someone following that pointer to build the follow-up harness would have grepped for a symbol that isn't there. Also records what pricing the real fix revealed: the entry point takes a single ~15-field params object and `test/_helpers/sync-responder.ts` already exists, but reaching the `refreshMetaFromCurator` call requires passing signature recovery and replay/freshness checks — so it needs a genuinely signed envelope. That is a real harness, not a line, which is why it stays a follow-up rather than being built under commit pressure to close a base-case branch assertion 1 already exercises. Refs #2018, #2006 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| const base = inst.name.replaceAll('.', '_'); | ||
| const unit = UNIT_SUFFIX[inst.unit]; | ||
| if (unit === undefined) throw new Error(`w1: no Prometheus unit suffix known for unit '${inst.unit}' (${inst.name}) — add it to UNIT_SUFFIX consciously`); | ||
| const withUnit = unit ? `${base}_${unit}` : base; |
There was a problem hiding this comment.
🔴 Bug: W1 byte queries miss the standard suffixed Prometheus counter names
What's wrong
The generated W1 PromQL does not match the standard suffixed names for the new request/response byte counters. On Prometheus-compatible backends using the default suffixing translation, the byte-denominator queries can come back empty or inconclusive even though the node is exporting the metrics.
Example
For dkg.sync.attempt.request_bytes with unit By, a Prometheus-with-suffixes backend exposes dkg_sync_attempt_request_bytes_total. The generated selector dkg_sync_attempt_request_bytes(_bytes_total)? matches dkg_sync_attempt_request_bytes and dkg_sync_attempt_request_bytes_bytes_total, but not dkg_sync_attempt_request_bytes_total, so W1 byte gates and family byte shares can read empty data on that ingest path.
Suggested direction
Derive unit suffixes with the OpenTelemetry/Prometheus duplicate-unit rule instead of blindly appending _${unit}.
Confidence note
Verified against the OpenTelemetry Prometheus/OpenMetrics compatibility rule: unit suffixes should be added unless the metric name already ends with the unit, before type suffixes (https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/).
For Agents
Update tools/observability/lib/w1.mjs Prometheus spelling derivation to avoid adding a unit suffix when the normalized base name already ends in the Prometheus unit word, then regenerate W1 artifacts. Add/adjust verifier expectations so request_bytes_total and response_bytes_total are accepted and the duplicate bytes_bytes_total spelling is rejected.
There was a problem hiding this comment.
Confirmed and fixed in 737dd381f. You were right about the rule and right about the consequence, and the consequence was the worse half: the alternation covered a form nothing emits while missing the real one, so on a standard suffixed backend the byte gates read empty and §7.3 reports inconclusive indefinitely — with nothing in the output pointing at the cause.
The derivation now appends the unit word only when the normalized name does not already carry it as a token, matching the OTel Prometheus/OpenMetrics rule and what prometheus/otlptranslator implements:
const withUnit = unit && !base.split('_').includes(unit) ? `${base}_${unit}` : base;Token containment rather than endsWith — the faithful reading of the rule, and it keeps duration_ms correct: its tokens are […, duration, ms], which does not contain milliseconds, so that instrument still receives its suffix.
Regenerated artifacts now read dkg_sync_attempt_request_bytes(_total)?, matching both ingest routes. bytes_bytes is 0 occurrences across w1-rules.yaml and w1-queries.md.
On the verifier I did both halves of what you asked, plus one more. It accepts request_bytes_total/response_bytes_total, and it actively rejects the duplicated spelling with its own diagnostic — checked before the native/translated gate, so a selector matching only the doubled form is reported as the duplicated-unit bug rather than as an unknown metric. That rejection is not redundant with requiring translated: an alternation can match both, which is exactly what the original defect did.
Beyond the ask: the rejection can only discriminate on instruments whose name already ends in their unit word, so if a refactor ever dropped both By counters it would silently become a check over an empty set — green, and proving nothing. There is now an anti-vacuity gate that fails the verifier in that case.
| await steps.drainCatchupJobs(); | ||
| await steps.flushTelemetry(); | ||
| await steps.stopBackgroundWorkers(); | ||
| await steps.stopAgent(); |
There was a problem hiding this comment.
🔴 Bug: A rejecting agent stop skips the final telemetry shutdown
What's wrong
The new teardown order depends on every step being non-throwing, but agent.stop() can reject by design. Because the final telemetry shutdown is now after stopAgent, that rejection prevents telemetry providers and log exporters from being shut down.
Example
If closeRfc64PersistenceV1() fails during shutdown, DKGAgent.stop() rethrows at the end of stop. runProducerQuiescentTeardown then exits at await steps.stopAgent(), and steps.stopTelemetry() never runs, leaving the final telemetry shutdown skipped even though this PR moved it after agent shutdown to preserve I1-I6 emissions.
Suggested direction
Guarantee stopTelemetry runs in a finally-style path after stopAgent, or catch/log agent.stop() at the wired step the same way nearby worker stops are guarded.
For Agents
Look at buildProducerQuiescentTeardownSteps and runProducerQuiescentTeardown. Preserve the intended order, but make the agent-stop slot non-throwing or otherwise ensure stopTelemetry always runs after it. Add a test where stopAgent rejects and prove telemetry shutdown still executes while the rejection is logged or rethrown only after required cleanup.
There was a problem hiding this comment.
Confirmed, fixed in 737dd381f, and my original defence of this code was wrong in a way worth naming.
I had argued the chain was safe because every step absorbs its own failures. I had verified that only against the step this PR added (flushTelemetry), never against the ones already there. DKGAgent.stop() has no top-level try/catch, four unguarded awaits and three deliberate rethrows (dkg-agent.ts:1847-1854), documented there as returning a failed shutdown rather than a false success. It rejects by design — and this PR is what moved stopTelemetry after it, so this PR is what exposed telemetry shutdown to it.
The blast radius was also wider than the thread title. Back in lifecycle.ts the same rejection skipped managedOxigraph.stop() and dashDb.close() as well, surfacing only as one generic Shutdown cleanup error: line — not a hang, not an unhandled rejection, just a silent downgrade from graceful to abrupt.
runProducerQuiescentTeardown now runs every step in order regardless of earlier failures, collects them, and returns {failures} instead of throwing. Each failure is logged by name; the caller logs a summary. There is deliberately no per-step "fatal" flag: every step is cleanup, so no step exists for which aborting would be correct, and an option whose every use is "keep going" is dead policy that invites the next person to pick the other value for a bad reason. DKGAgent.stop() already follows exactly this shape — run all cleanup, then report — so the sequencer now matches the thing that motivated it.
Two things this is not. It is not laxer than aborting: a step whose contract says it cannot reject still has that violation reported by name in failures and in the log, rather than swallowed. And it does not mask flushTelemetry's "never throws into the shutdown path" mutant, which pins that contract where it lives — calling the function directly, without going through this sequencer at all.
Tests: a rejecting publisher stop still terminates the catch-up runner; the runner never throws, so the caller reaches its own remaining cleanup; every failing step is reported and all of them still run; a clean teardown reports none.
| // `/_shared_memory` and `/_private` planes, so everything this lane | ||
| // transfers is durable. Shared-memory content defers to `runResync`, | ||
| // which is separately instrumented as `transport=legacy`. | ||
| send: async (bytes) => { |
There was a problem hiding this comment.
🟡 Issue: Collapse the duplicated send-attempt telemetry state machine
What's wrong
The PR intentionally avoids reusing sendSyncRequest, but it goes too far in the other direction by duplicating the telemetry lifecycle inline. That makes the metric contract harder to maintain because the invariant now lives in two procedural nests instead of one small abstraction.
Example
Today a new attempt outcome, byte-leg rule, or cancellation classification has to be applied in both the legacy transport closure and the changelog send closure. The code already carries two parallel mini state machines with different local variables and comments explaining the same invariant.
Suggested direction
Make the code judo move here: do not route changelog through sendSyncRequest, but do extract the common lower-level telemetry bracket so both lanes share one implementation of the I1-I3 lifecycle.
For Agents
Look at packages/agent/src/p2p/sync-transport.ts and packages/agent/src/dkg-agent-lifecycle.ts around the W1 send instrumentation. Preserve the exact record ordering and lane-specific behavior, but extract a small shared helper in sync/attempt-telemetry.ts or a sibling module that owns the request-byte, terminal-attempt, and optional response-byte bracket. Keep transport-specific send/validation/cancellation details as callbacks or options, and prove both legacy and changelog tests still pass.
There was a problem hiding this comment.
Accepting the diagnosis and deferring the change, with the reasoning rather than a "later".
You're right that the invariant now lives in two procedural nests, and that a new outcome value or byte-leg rule requires two correct edits. That is a real defect, and the judo framing is right — keep the lanes separate (§4's reason for not routing changelog through sendSyncRequest stands), extract the bracket.
We designed it before deciding. The shape that holds §6.2's four-point contract is a single checkpoint hook called at all four abort points, with the helper classifying rather than the caller — because only the helper knows whether sendStarted/responded are set yet. Splitting it into checkBeforeSend/checkAfterReceipt pushes classification back to the callers and re-creates the duplication. All six contract points map cleanly, including §10.3, since the helper is the per-attempt bracket (legacy invokes it inside the withRetry closure, so one invocation = one attempt).
So it is tractable. We are still deferring it, for three reasons:
- Two 🔴s are landing in adjacent files right now. Refactoring the most contract-heavy code in the PR concurrently means a break wouldn't be cheaply attributable to one change.
- The extraction trades one hazard for another. Six hooks whose ordering is the contract replaces a visible duplication with an invisible sequencing hazard. Today the ordering is legible inline at both sites; afterwards it is implicit in a helper's internals. That is a different trade, not obviously a better one, and it deserves its own design review rather than a review-round patch.
- The trigger is rarer than it looks. The outcome vocabulary is deliberately closed — §10.1 rejected
timeouton stated grounds (seven mutually incompatible deadline shapes; the only classifiers are.message.includes(...)) — so adding an outcome is a plan-level change that gets reviewed anyway. Both sites are currently correct and each is pinned by mutants M1/M2/M3/M6.
Filing it as a follow-up with the design attached, so it starts from a shape rather than a complaint. If you think the sequencing-hazard argument is wrong, or that the two-edit cost is more likely to bite than we've assumed, say so and we'll reopen it — this is a judgement about when, not a disagreement about the finding.
There was a problem hiding this comment.
Filed as #2037, with the design attached rather than as a complaint — the checkpoint-hook shape, the reason classification has to live in the helper, and the sequencing-hazard counter-argument that makes this worth a design review rather than a review-round patch.
It is filed jointly with the catch-up ledger finding, because they are the same shape (one lifecycle spread across a caller and a helper) and the acceptance criteria are shared — in particular that the existing mutants must keep their disjoint kill sets, since an extraction that collapses two mutants onto one assertion loses coverage while the suite stays green.
There was a problem hiding this comment.
🟡 Issue: Sync telemetry policy is leaking into the lifecycle class
What's wrong
The PR creates a dedicated telemetry module, but the hard parts of the telemetry policy still live as manual branches and metadata plumbing inside the largest orchestration file. That makes the implementation fragile because the invariant is spread across unrelated sync paths instead of owned by one canonical abstraction.
Example
Adding another sync coalescing scope now requires the caller to know whether the ambient source is valid, pass scope, avoid putting source in the key, store owner metadata, record map-hit joins, and ensure operation timing wraps the right boundary. Those are telemetry policy rules leaking into lifecycle orchestration.
Suggested direction
Move the source ownership and join-recording rules behind a dedicated sync single-flight abstraction, and move operation timing/rejection wrapping behind a small admission-boundary helper. The lifecycle class should orchestrate sync work, not encode the telemetry bookkeeping rules for each scope.
For Agents
Focus on packages/agent/src/dkg-agent-lifecycle.ts and packages/agent/src/sync/attempt-telemetry.ts. Preserve the current labels and coalescing behavior, but extract telemetry-aware single-flight and operation boundary helpers so lifecycle call sites supply work identity and lane/source once without owning the recording mechanics. Keep the existing I4/I5/I6 tests green.
There was a problem hiding this comment.
🟡 Issue: The changelog lane duplicates the send-attempt telemetry bracket inline
What's wrong
This puts transport-instrumentation control flow into an already very large lifecycle/orchestration file and duplicates the same state machine added to sendSyncRequest. The comment explains why changelog should not route through the legacy transport, but it does not require duplicating the telemetry bracket itself.
Example
Both paths need the same invariant: record I2 before physical send, classify terminal outcome, then record I1 and conditional I3 in finally. Today a change to that invariant has to be replicated in two different orchestration files.
Suggested direction
Extract a small withSyncAttemptTelemetry/recordedSyncSend helper that owns the I1-I3 lifecycle and lets each transport provide only its send function and classification hooks.
For Agents
Extract an instrumentation-only helper around a physical send, probably under packages/agent/src/sync/attempt-telemetry.ts or a sibling module. Keep changelog off sendSyncRequest if its retry/validation semantics differ, but share the telemetry bracket so dkg-agent-lifecycle.ts only supplies transport='changelog', labels, byte length, and the actual send closure. Prove existing legacy and changelog telemetry tests still pass.
There was a problem hiding this comment.
🟡 Issue: Collapse the duplicated sync-attempt telemetry state machine
What's wrong
The PR correctly isolates metric record sites, but it leaves the hardest part of the implementation, the per-send terminal-state bookkeeping, duplicated across two call sites. That is a structural maintainability risk because the behavior is subtle and comment-heavy; the next label, outcome, or cancellation refinement will need to be mirrored manually.
Example
The legacy path and changelog path now both have to preserve the same invariants: record request bytes immediately before the physical send, record I1 exactly once in finally, record I3 only after a resolved send, and classify cancellation consistently. Any future tweak to that state machine must be made in two different transport implementations.
Suggested direction
Move the physical-send accounting bracket into a dedicated helper/module and have both legacy and changelog sends call it. The helper should own the sendStarted/responded/outcome bookkeeping so the invariant exists once.
For Agents
Look at packages/agent/src/p2p/sync-transport.ts and the send closure in runChangelogSyncForCg in packages/agent/src/dkg-agent-lifecycle.ts. Preserve the existing W1 labels and terminal classifications, but extract one shared recordedSyncAttempt/withSyncAttemptTelemetry helper that owns request/response byte accounting and final I1 emission; both transports should supply only labels, preflight/send behavior, and lane-specific validation hooks. Existing W1 attempt/changelog tests should continue to prove the behavior.
There was a problem hiding this comment.
🟡 Issue: Move single-flight attribution out of the lifecycle monolith
What's wrong
This adds another synchronization concern directly to an 8k-line lifecycle file and duplicates the core attribution mechanics in two places. The implementation relies on comments to keep the invariants aligned instead of making the map-hit/owner-source behavior a reusable abstraction.
Example
Both paths now have the same conceptual algorithm: check a map, record an I6 join with owner/joiner source on hit, store owner metadata beside the promise on miss, and delete only the entry that this generation created.
Suggested direction
Create one focused single-flight/coalescing abstraction that accepts the scope/source metadata and records joins uniformly. Then reuse it from the generic scopes and the page-fetch coalescer.
For Agents
Extract a small sync/single-flight.ts helper that owns Map storage, owner metadata, join recording, and identity-safe cleanup. Keep page-specific abort/waiter handling separate if needed, but do not leave the join attribution algorithm duplicated in dkg-agent-lifecycle.ts. Prove durable/shared/context-graph/page joins still emit the same I6 labels.
There was a problem hiding this comment.
🟡 Issue: The per-attempt telemetry bracket is duplicated inside transport callers instead of owned by one boundary helper.
What's wrong
This makes telemetry correctness depend on two manually synchronized miniature state machines embedded in busy networking/orchestration code. The long comments are doing the work a well-named boundary helper should do, and future changes to terminal classification or byte recording now have to be applied in multiple places without drift.
Example
The legacy path tracks sendStarted, responded, responseByteLength, and outcome; the changelog path tracks a separate outcome/responseByteLength flow and repeats request-byte, terminal-attempt, and response-byte recording inline.
Suggested direction
Extract a reusable recordInstrumentedSyncAttempt or changelog-specific transport helper that owns the request/response/outcome state machine once. Keep lane-specific policy at the call sites, but stop hand-rolling the recording bracket in both the legacy transport and the changelog callback.
For Agents
Look at packages/agent/src/p2p/sync-transport.ts and the changelog send callback in packages/agent/src/dkg-agent-lifecycle.ts. Preserve the exact terminal labels and pre-send abort behavior, but move the common send-attempt bracket into one small helper, with changelog/legacy supplying their transport labels and actual send function. Existing W1 attempt tests should keep proving the same labels/byte legs.
| // ENTRY (not the jobId) in the closure below: the ledger slot is released | ||
| // as soon as the job settles, and looking the entry up by id afterwards | ||
| // would find nothing and re-emit its terminal point. | ||
| const ledgerEntry = beginWalkCatchupJob(job); |
There was a problem hiding this comment.
🟡 Issue: Move the catch-up job ledger protocol out of the route
What's wrong
This adds more orchestration to a 2k-line route and spreads one lifecycle across route branches plus the ledger module. The new helper module exists, but the route still owns too many details of the telemetry protocol, which makes future changes easy to get subtly out of sync.
Example
The route now has to remember this whole protocol: record each route result, guard both mint sites, create synthetic jobs through a separate telemetry call, call beginWalkCatchupJob, assign entry.task, call recordTerminalOnce, then call releaseCatchupJob. Missing any one of those steps in the next catch-up path creates hidden telemetry/ledger drift.
Suggested direction
Give catch-up job creation a real boundary that owns the ledger lifecycle atomically. The route should not manipulate a mutable ledger entry or hand-code the terminal recording finally.
For Agents
Focus on packages/cli/src/daemon/routes/context-graph.ts and packages/cli/src/daemon/catchup-telemetry.ts. Preserve the existing subscribe outcomes, shutdown behavior, and job status transitions, but move job admission/minting/ledger wrapping behind a dedicated helper such as createSyntheticCatchupJob and runWalkCatchupJob(job, work). The route should decide HTTP/domain branches; the helper should own request/job metric emission, task retention, terminal-once recording, and release.
There was a problem hiding this comment.
Agreed on the diagnosis, deferring the refactor, and the reasoning is close to the one on the send-telemetry finding.
The protocol you list is real and is exactly the drift risk you name: record each route result, guard both mint sites, mint synthetics through a separate telemetry call, beginWalkCatchupJob, assign entry.task, recordTerminalOnce, releaseCatchupJob. Missing one in a future catch-up path is silent.
Two things make us hold rather than move it now.
The route is where the mint sites are, and the mint sites are the thing under test. The two admission guards must sit at exact positions — the walk guard before subscribeToContextGraph() (it persists a subscription, performs four gossipsub subscribes and invalidates caches, so a 503 after it strands a gossiping subscription with no job), and the synthetic guard after reusableDoneJob is computed but before the id is generated. Both windows are await-free by construction, which is what makes them atomic. A runWalkCatchupJob(job, work) boundary that owns admission would move those guards behind an abstraction whose atomicity is no longer visible at the call site. That is worth doing carefully, not quickly.
This PR is already changing that file's shutdown behaviour, and a same-PR structural refactor of the surrounding protocol would make the behavioural diff harder to review rather than easier.
What we have instead of a refactor, so the drift you describe is at least detectable today: M12c/M12f′ pin the synthetic guard, M12d pins the walk guard and that a 503 leaves no persisted subscription, M12e pins terminal-once through the shared idempotency bit, and M7/M8 pin that dedupe and replay mint nothing. Each has a disjoint kill set, so the sites are proven independently rather than by one assertion covering several.
Filing as a follow-up together with the send-telemetry extraction — they are the same shape (one lifecycle spread across a caller and a helper) and are better designed together than separately.
There was a problem hiding this comment.
Filed as #2037, together with the send-telemetry extraction — same shape, and the constraints interact.
The issue records what any such boundary has to preserve: both admission windows are await-free by construction, the walk guard before subscribeToContextGraph() and the synthetic guard after reusableDoneJob but before the id mint. That atomicity is currently visible at the call site, and a runWalkCatchupJob(job, work) that also owned admission would hide it — which is the part worth designing carefully rather than quickly.
There was a problem hiding this comment.
🟡 Issue: Centralize catch-up subscribe outcomes instead of recording metrics at every return
What's wrong
The subscribe handler now mixes HTTP response construction, shutdown admission, catch-up job minting, metric result classification, and ledger registration directly in one long control flow. The result vocabulary is closed, but the implementation does not make it structurally closed: it relies on scattered calls before individual returns. This makes the route more brittle and harder to extend without silently missing instrumentation or duplicating guard logic.
Example
A future branch that returns from the subscribe route must remember to call recordCatchupRequest(...) with the right result value, use the right shutdown guard position, and avoid minting job state incorrectly. That protocol is spread through the handler rather than encoded in one response/minting abstraction.
Suggested direction
Introduce a small result dispatcher for the subscribe route. The route should classify the request into a bounded result and job action, then a single exit path should record I7 and write the response. Job creation should be handled by focused helpers that also own I8/I9 ledger registration. That would delete repeated recordCatchupRequest calls and make the shutdown guard placement part of the minting API rather than route folklore.
For Agents
Focus on the POST subscribe path in packages/cli/src/daemon/routes/context-graph.ts and packages/cli/src/daemon/catchup-telemetry.ts. Preserve every current route response and metric result, but restructure the route around a typed outcome model or response helper so each branch returns a CatchupSubscribeOutcome and one place records I7 and emits the HTTP response. Keep job minting and ledger registration behind dedicated mintSyntheticCatchupJob / mintWalkCatchupJob helpers.
There was a problem hiding this comment.
🟡 Issue: Catch-up job policy is scattered through the subscribe handler
What's wrong
This PR adds more cross-cutting branches to an already large route handler instead of reducing the number of concepts the handler owns. The resulting structure makes the route the coordinator for HTTP, subscription mutation, shutdown admission, request telemetry, job telemetry, ledger lifecycle, and readiness classification all at once.
Example
Adding a new subscribe return path now requires the author to remember three separate concerns in the route body: whether it mints a job, whether shutdown should block it, and which I7 result label to emit. Missing any one of those is easy because the policy is scattered through the handler.
Suggested direction
Extract the catch-up request/job lifecycle into a dedicated abstraction that returns a small decision object for the route to serialize. The route should not manually interleave HTTP responses, shutdown admission, telemetry labels, job ledger registration, and terminal recording.
For Agents
Refactor packages/cli/src/daemon/routes/context-graph.ts and packages/cli/src/daemon/catchup-telemetry.ts. Preserve every existing response/status and metric label, but move catch-up admission, replay/dedupe/mint decisions, request-result recording, ledger registration, and terminal recording into a focused catch-up job service or tracker API. Tests should continue to drive the route, but assertions should prove the service owns the lifecycle decisions.
There was a problem hiding this comment.
Same territory as the ledger-boundary thread above, and this framing is sharper than the one I responded to before — so let me take the part that's new.
"The result vocabulary is closed, but the implementation does not make it structurally closed" is the right criticism, and it is a different point from "the route is long". A closed vocabulary enforced by scattered call sites is a convention; a typed outcome model with one exit path is a guarantee. That distinction is the same one this PR has been repeatedly corrected on in other places — a check that relies on someone remembering is not a check.
And this round produced direct evidence for you: the invalid-JSON finding was exactly the failure your example predicts. A route return existed that nobody remembered to instrument, and the result was a biased under-count of I7 on precisely the requests clients retry. One branch, out of the vocabulary, missed. That is one data point in favour of the dispatcher, and I'd rather record it than argue around it.
Still deferring, and the reason is the specific constraint I described earlier rather than general caution: the two admission guards must sit at exact positions and both windows are await-free by construction — the walk guard before subscribeToContextGraph() (which persists a subscription and performs four gossipsub subscribes, so a 503 after it strands a gossiping subscription with no job), and the synthetic guard after reusableDoneJob is computed but before the id is minted. A "single exit path records I7 and writes the response" model has to preserve that atomicity while moving the guards behind mintSyntheticCatchupJob / mintWalkCatchupJob. That is doable — it is your suggestion and I think it is right — but it is a restructuring of the exact code whose shutdown behaviour this PR changes, and it deserves its own diff.
Recorded in #2037 as part C's design, with the invalid-JSON case attached as the motivating example and the await-free-window requirement as an acceptance criterion.
There was a problem hiding this comment.
🟡 Issue: Centralize subscribe accounting instead of scattering it across returns
What's wrong
The subscribe route is already a large control-flow function, and this change makes it the manual orchestrator for I7 metrics, I8/I9 job accounting, shutdown admission, synthetic jobs, and the walk ledger. That creates a brittle protocol where every future early return must remember the right metric label and every new job path must update multiple state holders in the right order.
Example
The branch at recordCatchupRequest('bad_request', false) is one of many route returns that now has to remember its telemetry label manually; later the same route writes catchupTracker.jobs, latestByContextGraph, and beginWalkCatchupJob(job) as separate steps.
Suggested direction
Move the request-result metric emission and job ledger/tracker updates behind a route-level abstraction such as catchupSubscribeOutcome.* or CatchupJobRegistry. The route should decide the domain outcome; the helper should perform the consistent accounting/state updates.
For Agents
Look in packages/cli/src/daemon/routes/context-graph.ts and packages/cli/src/daemon/catchup-telemetry.ts. Preserve every current response/status/metric label, but introduce a small catch-up subscribe coordinator or typed outcome helper that owns request accounting and job mint/replay/dedupe/shutdown transitions in one place.
There was a problem hiding this comment.
🟡 Issue: Subscribe telemetry is scattered across route return sites
What's wrong
This adds another cross-cutting obligation to an already large route: every new return path must remember a separate metric call with the right label. That is exactly the kind of spaghetti growth that makes the route harder to modify safely.
Example
A future early return in this subscribe branch can produce a valid HTTP response while forgetting recordCatchupRequest(...); nothing in the structure or types forces the one-point-per-return invariant the comments describe.
Suggested direction
Collapse the route returns behind a single helper or decision object, for example returnCatchupSubscribe(res, { result, includeSharedMemory, status, body }), so the telemetry label and HTTP response are emitted together.
For Agents
Look at the subscribe branch in packages/cli/src/daemon/routes/context-graph.ts. Preserve the same response statuses/bodies and catch-up tracker mutations, but introduce a small typed response/decision helper that requires result and includeSharedMemory for every subscribe return. Existing catch-up telemetry route tests should still pass.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
There was a problem hiding this comment.
🟡 Issue: The subscribe route is accumulating telemetry and job-lifecycle side effects branch by branch
What's wrong
The implementation spreads one accounting invariant across many unrelated route branches in an already-large handler. That makes the route harder to scan and creates a maintenance trap: every new early return must manually pair HTTP behavior with the correct I7/I8/ledger behavior. The cleaner move is to make request outcome and job minting a typed model rather than scattered side effects.
Example
The same endpoint now has telemetry/job-lifecycle calls at lines 1729, 1740, 1766, 1784, 1835, 1837, 1883, 1925, and 2012. Adding another subscribe return path requires remembering both the HTTP response and the metric/job side effect in the right branch.
Suggested direction
Introduce a small catch-up admission/job lifecycle abstraction that owns request result recording, synthetic/walk job creation, ledger registration, and terminal recording. The route should mostly parse input, call that abstraction, and serialize the returned response.
For Agents
Look in packages/cli/src/daemon/routes/context-graph.ts and packages/cli/src/daemon/catchup-telemetry.ts. Keep response behavior unchanged, but move subscribe result accounting and job minting into a focused catch-up admission/lifecycle helper that returns a typed route outcome and emits telemetry from one boundary. Add focused tests around that helper plus thin route integration coverage.
| export async function runProducerQuiescentTeardown( | ||
| steps: ProducerQuiescentTeardownSteps, | ||
| ): Promise<void> { | ||
| steps.closeServer(); |
There was a problem hiding this comment.
🟡 Issue: Encode teardown resilience instead of relying on comments
What's wrong
The new teardown module is meant to make shutdown order explicit, but its most important maintenance rule is only in prose. That creates a brittle abstraction: adding or editing a step requires the caller to remember an undocumented-by-type requirement, and one missed catch changes the rest of the shutdown sequence.
Example
A future stopBackgroundWorkers change that lets stopPromoteWorker reject would skip stopAgent and stopTelemetry because runProducerQuiescentTeardown has no structural way to enforce its own stated invariant.
Suggested direction
Make runProducerQuiescentTeardown or buildProducerQuiescentTeardownSteps own the non-throwing contract it describes. A named step model with explicit catch/log behavior would make the sequence safer and easier to extend.
Confidence note
This is a maintainability finding about the abstraction boundary, not a claim that the current shutdown behavior is incorrect.
For Agents
Review packages/cli/src/daemon/teardown.ts and the builder call in lifecycle.ts. Preserve the step order, but encode the non-throwing policy in the teardown abstraction itself, either by wrapping named steps at the builder boundary or by making the runner consume named steps with a consistent catch/log policy. Tests should show that a rejecting step does not prevent later mandatory teardown steps from running, if that is the intended invariant.
There was a problem hiding this comment.
Fixed by the same change as the 🔴 above (737dd381f), and your framing is what made me see they were one problem rather than two: a maintainability finding and a live defect with the same root — a policy stated in prose that the abstraction had no way to enforce.
runProducerQuiescentTeardown now consumes named steps and applies one catch/log policy across all of them, so the invariant is structural instead of remembered.
One consequence worth flagging, because it changes the shape you reviewed: stopBackgroundWorkers is gone. The guard is per-entry, so an entry composing several sequential awaits would strand its own tail before the catch ever saw the failure — a rejecting publisher stop would have skipped the promote worker and left the catch-up runner's worker thread alive. That is precisely the stranding this sequencer exists to prevent, reproduced one level down inside a single step. It is now three separate entries (stopPublisherRuntime, stopPromoteWorker, closeCatchupRunner), with "one action per entry" documented as the rule for adding steps — which is the structural version of the "before adding a seventh step, make sure it cannot reject" comment you were right to call out as insufficient.
Your stopPromoteWorker example is now a direct test rather than a hypothetical.
| undefined, | ||
| true, | ||
| ); | ||
| const result = await (hasSyncAdmissionSource() |
There was a problem hiding this comment.
🟡 Issue: Responder-side control-plane attribution is not verified
What's wrong
This change attributes curator meta refresh traffic for three callers, including responder authorization, but the new tests only cover requester-side standalone/nested cases and an admitted no-source case. The responder path is one of the enumerated behaviors, and it currently has no regression test.
Example
A regression that wraps the real responder authorization path in an ambient sync source, or stops routing it through this refresh path, would still leave the standalone and nested refresh tests green because they never invoke authorizePrivateSyncRequest / authorizeSyncRequest.
Suggested direction
Close the acknowledged gap with a test that drives the responder auth lookup, not just a standalone refresh helper.
For Agents
Add an integration-style test around the real responder authorization path in packages/agent/src/sync/auth/request-authorize.ts. Use a dependency seam whose refreshMetaFromCurator records hasSyncAdmissionSource() or drive the real metrics path and assert responder-side curator meta refresh is control-plane with no ambient requester source.
There was a problem hiding this comment.
Closed in 737dd381f, by the integration route you suggested rather than the dependency seam.
Worth being explicit about the history, because it is why this took a detour. An earlier round had a test in this slot that claimed to cover the responder and executed no responder code: it defined a local closure, invoked it from the test body, and asserted that the test body had no ambient scope — a result determined entirely by its own setup. Wrapping the real responder path in withSyncAdmissionSource(...), which is exactly the regression its name promised to catch, would have left it green. I deleted it rather than leave it standing (6bf6fbb8d), on the grounds that an acknowledged gap gets revisited and a green test does not. Your finding is what sent me back to close it properly.
Assertion 3 now drives the production authorizePrivateSyncRequest. The envelope clears the auth preflight with a real signature over an injected digest, then fails the allowlist — which is what routes control into refreshMetaFromCurator. Nothing about the guard is simulated: the responder genuinely reaches the refresh with no ambient requester scope, and control-plane is read off a real I1 point rather than inferred from the call graph.
Two more, because assertion 3 on its own still could not fail for the right reason:
- 3b executes your regression directly — the responder path wrapped in an ambient
catchup-foregroundscope reports that label and notcontrol-plane. Without it, assertion 3 would pass whether or not the responder still lands on the base case. - 3c injects a transport failure, which is the branch that matters most in production: the whole point of a curator meta refresh is recovering metadata from a peer that may be down, so an unreachable curator is the normal case — yet every other assertion in the block drives a successful fetch. It also pins the I2-before-send leg, the one a naive failure path drops while silently shrinking the byte denominator, and that I3 is absent because the send never resolved.
That last one was "correct by construction and unasserted", which is the same shape as M9 in this PR: also correct, also unprotected, and it survived every suite in the repo until someone wrote the assertion.
Suite is green at 28.
| // `let` in this function's scope), and a job minted after this point would | ||
| // be queued against a runner whose worker is about to be terminated — its | ||
| // exit handler rejects every pending run. Both mint sites now 503 instead. | ||
| daemonState.catchupAcceptingJobs = false; |
There was a problem hiding this comment.
🟡 Issue: Shutdown-side catch-up admission closure is not verified
What's wrong
The user-facing behavior depends on shutdown closing catch-up admission before cleanup awaits begin. Current tests validate what happens after the global flag is already false, but they do not validate that shutdown() sets it, or that it remains the first operation.
Example
Delete line 3752, or move it after await removeApiPort(). The new subscribe-route tests that manually assign daemonState.catchupAcceptingJobs = false would still pass, but a real shutdown could still accept and mint a catch-up job during that early window.
Suggested direction
Test the actual shutdown entry point, or extract the shutdown-start state transition into a small tested seam, instead of only testing the route with the flag pre-set.
For Agents
Add a shutdown integration or extracted-seam test in the CLI package that starts the daemon shutdown path and asserts daemonState.catchupAcceptingJobs becomes false before the first awaited cleanup step resolves. Then issue a subscribe crossing that suspended first await and prove no job/subscription is minted.
There was a problem hiding this comment.
Fixed in 737dd381f. This was the strongest of the six findings, and your falsification was the reason: I ran both of your suggested regressions and both passed the entire existing suite, exactly as you predicted.
The gap is what you said it was — every test in that file sets catchupAcceptingJobs by hand, so all of them can only observe what happens once admission is already closed. The transition itself was unpinned.
One thing I want to flag, because it changed the fix: a seam owning only the flag would not have closed this. It kills "delete the write" but not "move it after await removeApiPort()" — the call site stays free to invoke the seam late. The contract here is an ordering, so the seam has to own the flag write and the first suspension point together. beginGracefulShutdown now does, and removeApiPort is real filesystem I/O, so the window is genuinely reachable by a concurrent request in production rather than a contrived one.
The test suspends inside removeApiPort and issues a real subscribe through the route from inside that window — which is the only vantage point from which the two orderings differ.
Both of your regressions verified as killed, each one restored by inverse edit afterwards:
| mutation | result |
|---|---|
delete catchupAcceptingJobs = false |
AssertionError: expected 200 to be 503 |
sink it below await removeApiPort() |
AssertionError: expected 200 to be 503 |
The 200 is the finding itself: in both cases the subscribe genuinely succeeded inside the shutdown window and minted a job against a runner about to be terminated.
Being straight about what that table does and does not show: both mutants break the same ordering contract, so one assertion catching both is by design, not two independent facts. What stops it from being vacuous is the paired positive control — the same subscribe with no shutdown in flight returns 200 and mints exactly one job. Without that, any 503 (an unsubscribable CG, a broken fixture) would read as proof the guard fired when it never ran. There is also an explicit precondition that admission is open going in, so the refusal is the transition rather than the state the test started in.
Suite green at 30 (was 28).
| if (tracerProvider) { | ||
| tasks.push(tracerProvider.forceFlush().catch(() => {})); | ||
| tasks.push(tracerProvider.shutdown().catch(() => {})); | ||
| await tracerProvider.forceFlush().catch(() => {}); |
There was a problem hiding this comment.
🟡 Issue: Tracer shutdown sequencing lacks a regression test
What's wrong
The production fix applies the flush-before-shutdown ordering to both metrics and traces, but the test only protects the metric provider. A trace-only regression could drop final spans on shutdown without failing the new test.
Example
Revert only the tracer branch to the old concurrent shape while keeping the meter branch sequential. The current shutdownTelemetry test would still pass because no tracer provider is installed in that test and no tracer prototype is observed.
Suggested direction
Mirror the meter sequencing test for the tracer provider so final-span flushing is covered by the same contract.
Confidence note
This is based on the diff: the shutdownTelemetry regression test initializes metrics only and spies MeterProvider, while the production change also sequences the tracer provider.
For Agents
In packages/node-ui/test/telemetry.test.ts, add a tracer-side analogue: initialize tracing, create an ended span so forceFlush has work, delay NodeTracerProvider/processor flush, and assert shutdown does not start until that flush promise resolves.
There was a problem hiding this comment.
Fixed in 737dd381f. Your diagnosis was exact, including the falsification: the test registered metrics only, so reverting just the tracer branch to the concurrent Promise.all shape passed.
Both signals are now registered, and each is asserted in its own test — which is the part I want to call out as deliberate rather than incidental. A single test covering both branches would be killed by a revert of either, giving the two mutants an identical kill set and therefore no evidence that either branch is pinned on its own. Split, a meter-only revert kills only the meter test and a tracer-only revert kills only the tracer test.
Implementation note for whoever touches this next: forceFlush/shutdown live on BasicTracerProvider.prototype, which NodeTracerProvider inherits — spying on the subclass prototype does not intercept the inherited call, which is a quiet way to write a test that observes nothing. The test reaches the base prototype via getPrototypeOf rather than taking a dependency on sdk-trace-base.
Each test also asserts that its branch actually ran (flushes === 1, shutdowns === 1) before asserting the ordering. Without that, an unregistered provider leaves the ordering flag undefined and the assertion is unreachable rather than satisfied — which is exactly how the tracer branch went uncovered in the first place.
Suite green at 21.
…resilience, and three unpinned contracts Addresses six review findings on #2033. Two 🔴: - The Prometheus spelling derivation appended the unit word unconditionally, producing `..._request_bytes_bytes_total` — a form no ingest route emits — while MISSING the real `..._request_bytes_total`. On a suffixed backend the byte gates read empty and §7.3 reports `inconclusive` indefinitely with nothing pointing at the cause. Now applies the OTel duplicate-unit rule via token containment. The verifier accepts the real spelling AND actively rejects the doubled one, with an anti-vacuity gate so the rejection cannot silently become a check over an empty set. - `runProducerQuiescentTeardown` was a bare await chain, defended on the claim that no step can reject. That claim was only ever checked against the step this PR added. `DKGAgent.stop()` rejects BY DESIGN, and since this PR moved `stopTelemetry` after it, a failed agent stop skipped telemetry shutdown — and, back in lifecycle.ts, the Oxigraph and dashboard-DB teardown too. It now runs every step, collects failures, and reports them by name. Composed steps are split one-action-per-entry, because the guard is per-entry and a composite would strand its own tail. Three contracts that were correct but unpinned: - Responder-side `control-plane` attribution now drives the real `authorizePrivateSyncRequest`, with a regression case (ambient scope wins) and a failure-injection case (an unreachable curator still attributes, with the I2-before-send byte leg intact and no I3). - Tracer shutdown sequencing gets its own test, separate from the meter, so the two branches have disjoint kill sets rather than one test that either revert would trip. - Shutdown-side catch-up admission closure is extracted as `beginGracefulShutdown`, owning the flag write AND the first await together. A seam owning only the flag would pin the write's existence while leaving the call site free to invoke it after the await; the contract is an ordering, so the test suspends inside `removeApiPort` and issues a real subscribe from that window. Verified against both regressions — deleting the write, and sinking it below the await — each returning 200 where 503 is required. Both break the same ordering, so one assertion catches both by design; the paired positive control proves the 503 comes from the guard and not the fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
…ng M17 #2029 asked for a deliberate choice between guarding each teardown step and enforcing "no step may reject" at the builder boundary, and predicted that the guard would mask M17 — the mutant removing the `.catch` around the bounded meter flush — on the assumption M17 was killed by the rejection propagating through this sequencer. Re-ran M17 against the guarded version: still killed, by three direct-call assertions in node-ui/test/telemetry.test.ts that never reach the sequencer. The invariant is pinned where it lives, so option 1 is safe to take. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| * Live walk jobs, keyed by jobId. Synthetic jobs are born terminal and never | ||
| * enter it — there is nothing to drain and their record is emitted at once. | ||
| */ | ||
| const ledger = new Map<string, CatchupJobLedgerEntry>(); |
There was a problem hiding this comment.
🟡 Issue: The catch-up ledger should not be a second hidden daemon singleton
What's wrong
This PR adds another process-global mutable state container next to daemonState and catchupTracker. Catch-up lifecycle is now split across the visible route tracker and an imported module singleton, which makes future changes easier to mis-wire.
Example
A fresh catchupTracker can still inherit pending ledger entries from a previous flow unless the caller also remembers the separate resetCatchupJobLedger() seam. The need for that seam is a sign the state owner is unclear.
Suggested direction
Make the ledger an explicit field on daemonState or CatchupTracker, so route state, shutdown state, and test reset semantics have one owner.
For Agents
Move the catch-up job ledger into daemonState or the CatchupTracker instance. Route job creation and shutdown drain through that explicit owner. Preserve current request/job telemetry behavior and prove independent trackers do not share ledger entries.
There was a problem hiding this comment.
Agreed on the diagnosis, deferring the move, and folding it into #2037 alongside the ledger-boundary finding from the earlier round — they are the same question ("who owns the catch-up job lifecycle") arriving from two directions, and answering them separately would produce two half-answers.
Your resetCatchupJobLedger() argument is the strongest part and I'll restate it as the acceptance criterion rather than paraphrase it away: a test seam whose existence is required for correctness is evidence the owner is unclear. That is a better signal than the singleton count.
Two things that constrain the move, both of which the issue now records:
The module-level ledger is not merely a second daemonState; it is deliberately NOT the tracker. catchupTracker.jobs prunes to 100 entries by oldest queuedAt regardless of status, so a long-running walk can be evicted from it while still in flight. The ledger holds the idempotency bit (terminalRecorded) and the retained continuation precisely because keying either off an evictable map would let a job be counted twice or not at all. Moving the ledger onto CatchupTracker is fine only if it does not inherit that pruning — which is easy to get wrong, because the natural implementation puts them in the same object.
daemonState vs CatchupTracker are not interchangeable here. The reason this module exists at all is that two modules which never import each other need the same object: the subscribe route mints jobs, lifecycle.ts drains them at shutdown. CatchupTracker is currently a plain data bag that route tests construct by hand — which is a property worth keeping — so daemonState is the likelier home of the two.
Your "independent trackers do not share ledger entries" test is the right acceptance check, and it is now written into #2037 as such.
There was a problem hiding this comment.
🟡 Issue: The catch-up ledger is a hidden second job registry
What's wrong
The code now has catchupTracker.jobs plus an independent process-global ledger tracking the same logical jobs for shutdown/telemetry. Keeping those in sync requires manual begin/release calls and a test-only reset seam, which is a sign the ownership boundary is wrong.
Example
If another daemon instance or test harness creates its own CatchupTracker, it still shares this module-global ledger unless it remembers to call the test-only reset seam. That makes the job lifecycle split across two registries with different ownership and cleanup rules.
Suggested direction
Make the drainable ledger explicit state owned by the daemon/catch-up tracker instead of a module singleton. That gives route and shutdown code the same object without invisible process-global coupling.
For Agents
Move the ledger into the object that already owns catch-up jobs, or introduce an explicit CatchupJobManager stored on daemon state and passed to both the route and lifecycle teardown. Preserve synthetic-vs-walk accounting and shutdown drain behavior. Tests should not need a process-global reset to isolate job state.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
There was a problem hiding this comment.
🟡 Issue: Avoid adding a second process-global catch-up job store beside the tracker
What's wrong
This creates a hidden parallel state machine for catch-up jobs. The comments explain why it must stay synchronized with route minting, tracker pruning, and shutdown drain order, but the ownership boundary does not enforce that. Tests also need explicit global resets, which is a sign the state is not naturally scoped to a daemon instance.
Example
const ledger = new Map<string, CatchupJobLedgerEntry>(); at line 81 creates another lifecycle store that must stay consistent with catchupTracker.jobs, the detached route continuation, and shutdown drain ordering.
Suggested direction
Make the ledger an owned dependency rather than a module singleton. The same object that owns job creation should own terminal recording and drain state, with lifecycle receiving that object explicitly.
For Agents
Move the ledger into the daemon-owned catch-up state, the catch-up tracker, or a constructed catch-up job service passed to both the route and lifecycle. Preserve exact-once terminal recording and shutdown drain behavior, and keep tests proving replay/dedupe do not mint new jobs.
There was a problem hiding this comment.
🟡 Issue: The catch-up job ledger is hidden process-global state instead of explicit daemon state.
What's wrong
The new module-global map creates an implicit coupling between the subscribe route and shutdown path. That makes lifecycle ownership harder to reason about, forces reset helpers for tests, and would be awkward if the daemon ever needs isolated instances in the same process.
Example
handleContextGraphRoutes receives catchupTracker explicitly, but the corresponding terminal-job ledger is hidden behind imports from catchup-telemetry.ts. Shutdown then drains whatever happens to be in that process-global map rather than an object owned by the daemon instance.
Suggested direction
Make the ledger an explicit dependency owned by daemon state or the catch-up tracker. The route can register jobs on that object and shutdown can drain that same object without a module singleton or test-only reset seam.
For Agents
Move the ledger behind an explicit object, either as part of daemonState or as a CatchupTelemetryLedger instance created by daemon startup and passed to the route/teardown builder. Preserve the current idempotency semantics for recordTerminalOnce, releaseCatchupJob, and shutdown drain behavior.
| return includeSharedMemory ? 'shared-memory' : 'durable'; | ||
| } | ||
|
|
||
| const syncAdmissionSourceContext = new AsyncLocalStorage<SyncAdmissionSource>(); |
There was a problem hiding this comment.
🟡 Issue: Ambient telemetry source makes attribution a hidden control-flow dependency
What's wrong
The implementation hides a core attribution input in dynamic async context. That reduces parameter churn, but it also makes correct attribution depend on readers preserving informal constraints around where async work may be spawned.
Example
A future send-capable detached task spawned under a foreground operation can keep catchup-foreground after the operation returns unless it explicitly establishes another source. That coupling is invisible in the function signature.
Suggested direction
Push source attribution into an explicit narrow abstraction instead of letting arbitrary async descendants read ambient state.
Confidence note
The diff argues for AsyncLocalStorage, so this is not a claim that behavior is wrong. The maintainability concern is that source attribution now depends on hidden dynamic state and audit-only constraints around detached async work.
For Agents
Review attempt-telemetry.ts, sync-transport.ts, and lifecycle sync call sites. Consider a scoped sync telemetry/sender abstraction created at runContextGraphSyncWithBackpressure and passed through the narrow send seams. Preserve current labels, keep source out of coalescing keys, and prove changelog-to-legacy fallback still inherits the operation source.
There was a problem hiding this comment.
Accepting the residual you describe, disagreeing with the direction, and I want to be precise about which part is which — because your confidence note already concedes behaviour is correct, so this is purely about where the constraint should live.
Why ambient, and why it is not just parameter-churn avoidance. The alternative is threading source through the send seams. Those seams are also the coalescing seams, and source must never reach a coalescing key: two callers with different sources joining the same in-flight fetch is the single-flight behaviour I6 exists to measure. Putting the value in the parameter list next to keys that are built from parameter lists is how it eventually ends up in one. The ambient path makes that mistake unavailable rather than merely discouraged — backpressure.ts:69 builds its key from label + source deliberately for admission, and the fetch-coalescing keys are source-free by construction. That is a correctness argument, not an ergonomics one.
Your example is the real residual, and it is sharper than "informal constraints". A detached, send-capable task spawned under an operation inherits that operation's source after the operation returns. Worth separating two cases:
- A detached continuation of that operation's own work keeping the label is correct — those bytes were caused by that admission, which is exactly what I1–I3 claim to attribute.
- A long-lived task that outlives its spawning operation and later serves unrelated work would be wrong, and nothing in the signature prevents it.
The second case does not exist today (the enumerated scopes are the admission choke point in runContextGraphSyncWithBackpressure, withSyncAdmissionSource at the curator-refresh guard, and the responder's unscoped base case — all verified in this round by thread 6's assertions 3/3b/3c). But "does not exist today" is a fact about the current code, not a property of the design, and you are right that the signature does not carry it.
Why not the scoped-sender abstraction now. It reintroduces source into the send seams as a constructor argument, which is the coupling above in a different shape — and the constructed sender then has to be plumbed to exactly the sites the ambient store reaches today, so the audit surface is the same size, just relocated. That is a real trade to evaluate, not an obvious win, and it lands in the same file as the send-telemetry bracket extraction already filed as #2037. Designing them together is materially better than either alone, since both change where the send seam's contract lives.
Folding this into #2037 with the two-case split above stated as the acceptance criterion: whatever shape wins must make the outliving case unrepresentable, not merely unused.
There was a problem hiding this comment.
🟡 Issue: The admission source is now hidden ambient state
What's wrong
This adds a magical cross-cutting dependency to the sync path: sends no longer reveal in their parameters which source they record under, and the comments/tests have to explain subtle scope-presence, sentinel, and detached-task behavior. That is a maintainability smell because the invariant is enforced by convention and audit, not by the local API shape.
Example
A future sync helper that starts a detached async send inside an admitted operation can silently keep the parent source after the operation has returned. The send signature will not show that dependency, so reviewers have to remember the AsyncLocalStorage rule and audit task lifetimes manually.
Suggested direction
Prefer making source ownership explicit at the operation boundary, for example by creating a SyncTelemetryContext/recorder bound in runContextGraphSyncWithBackpressure and passing that through transport seams instead of reading process-local ambient state in record helpers.
For Agents
Look at packages/agent/src/sync/attempt-telemetry.ts, runContextGraphSyncWithBackpressure, fetchSyncPages, and the changelog send path. Preserve the current metric labels and the “source must not enter coalescing keys” behavior, but try to introduce an explicit operation-scoped telemetry context or bound recorder that is passed only to send/record sites. Prove the changelog->legacy fallback still carries the intended source and detached work cannot accidentally inherit a stale one.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
| @@ -0,0 +1,520 @@ | |||
| // W1 sync-measurement decision queries — DATA ONLY, the one module that maps | |||
There was a problem hiding this comment.
🟡 Issue: The W1 observability generator is doing too many jobs in one module
What's wrong
The semantic W1 contract is interleaved with formatting and YAML/Markdown mechanics. That makes future W1 changes harder to audit and increases the chance of accidental drift hiding inside a large generator function.
Example
Adding a new W1 instrument requires editing the core metric declaration, generator catalog, verifier catalog, query construction, and generated artifacts. The generator currently mixes those model edits with renderer mechanics in one long module.
Suggested direction
Decompose the W1 artifact code around concepts: catalog, spelling helpers, query catalog, Markdown renderer, and rule renderer.
For Agents
Split tools/observability/lib/w1.mjs into a small catalog, Prometheus-name helpers, query construction, and renderers. Keep the verifier independent, but make its duplicated contract compact. Existing generator check and verifier should prove byte-for-byte preservation.
There was a problem hiding this comment.
🟡 Issue: Collapse the hand-copied W1 contract into a canonical catalog
What's wrong
This PR creates a large new telemetry contract but spreads its canonical facts across production metrics, agent label normalizers, the observability generator, and the verifier. The duplication is deliberate in comments, but structurally it makes every future W1 evolution a multi-file synchronization task rather than a type-checked change to one model. That is avoidable architectural drift, especially for values that are explicitly described as closed vocabularies.
Example
Adding a new sync metric label now requires updating the core metric declaration, the agent record-site normalizers, the W1 query catalog, the generated artifacts, and the verifier's duplicate inventory. The current structure depends on reviewers noticing every parallel table.
Suggested direction
Create a shared W1 telemetry contract module that owns instrument ids, names, units, label keys, closed vocabularies, and source families. Let the generator render from that contract and let record helpers normalize against the same contract. The verifier can still independently parse rendered artifacts and assert coverage without re-declaring the entire domain model.
For Agents
Look at packages/core/src/telemetry-api.ts, packages/agent/src/sync/attempt-telemetry.ts, tools/observability/lib/w1.mjs, and tools/observability/verify-w1-render.mjs. Preserve the generated artifact behavior, but move the W1 instrument metadata and source-family taxonomy into a small typed catalog that both record sites and observability generation can import. Keep one independent verifier layer for rendered syntax/coverage, but stop maintaining multiple hand-copied contract tables.
There was a problem hiding this comment.
Deferring this one, and unlike the other two in this round I'd argue it is the weakest of the six — worth saying plainly rather than filing it to look responsive.
The cost you name is real: adding an instrument means editing the core declaration, the generator catalog, the verifier catalog, the query construction, and the artifacts. But splitting w1.mjs into five modules does not reduce that number — the same five edits land in five files instead of three. The multi-edit cost comes from the deliberate duplication between generator and verifier, which you explicitly (and rightly) want to keep: the verifier is only meaningful because it states the contract independently rather than importing it. That duplication is the feature, and it survives any decomposition.
What the split would actually buy is readability inside one ~400-line generator. That is worth something, but it is bought at the cost of a refactor whose only acceptance criterion is "byte-for-byte identical artifacts" — a change with real regression surface and no behavioural upside, landing in the same PR as two other 🔴-adjacent fixes to the same file.
There is also a concrete near-term reason to hold: this round already changed promParts (the duplicated-unit rule) and added an anti-vacuity gate to the verifier. Those are exactly the "semantic contract" parts a decomposition would move. Doing both at once means a spelling regression would not be cheaply attributable to either.
Not filing this as an issue, because a standing "split this module" ticket with no forcing function tends to sit open and become noise. The right trigger is the next W1 instrument added — at that point the five-edit path gets walked for real and either hurts enough to justify the refactor or doesn't. If you think it should be tracked regardless, say so and I'll file it.
There was a problem hiding this comment.
🟡 Issue: The W1 observability contract is maintained as parallel hand-written tables
What's wrong
The PR intentionally duplicates the W1 instrument inventory and source-family vocabulary across generator and verifier code. That makes routine metric/source evolution expensive and brittle, and it normalizes a multi-file synchronized-edit workflow for a contract that should have one canonical representation.
Example
Adding a new SYNC_ADMISSION_SOURCES member now requires synchronized edits in the agent policy, the generator, the verifier, generated Markdown/YAML, and likely tests. The verifier catches drift after the fact, but the design still relies on maintaining parallel hand-written catalogs.
Suggested direction
Replace the generator/verifier copy-paste contract with a shared machine-readable contract plus targeted verifier checks against source declarations. The verifier should still avoid self-validating rendered output, but it does not need an entire second copy of the domain model.
For Agents
Introduce a small versioned W1 contract artifact, such as JSON or a pure data module, for instruments, labels, source families, filters, and windows. Have the generator consume it and have the verifier validate rendered artifacts against it plus independently check that the agent/core declarations match. Preserve the fail-loud behavior for unclassified future sources without duplicating the full table in multiple places.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
| /** I1 — exactly one terminal point per physically invoked send. */ | ||
| export function recordSyncAttempt( | ||
| attributes: SyncAttemptAttributes, | ||
| outcome: SyncAttemptOutcome | string, |
There was a problem hiding this comment.
🟡 Issue: The telemetry record API widens internal contracts to string too early
What's wrong
The new record API accepts arbitrary strings from ordinary internal callers, weakening the TypeScript boundary and forcing readers to rely on comments/tests instead of the signature for the valid label contract.
Example
A new internal call can pass lane: 'responder' or outcome: 'success'; TypeScript accepts it and the metric silently becomes unspecified. That hides a bad local call site behind the normalizer.
Suggested direction
Separate trusted internal record helpers from boundary-normalizing helpers so local call sites get useful type checking.
Confidence note
Defensive clamping is valuable at metric boundaries; the issue is that the loose contract is exposed to normal internal TypeScript callers too.
For Agents
In attempt-telemetry.ts, narrow internal record helper signatures to the explicit unions. If truly untyped boundary data still needs clamping, expose a separate boundary-normalizing helper and keep the existing clamping tests there.
There was a problem hiding this comment.
Fixed in 9e6dd5d30 — and this one was not just a typing nit. Applying it failed the build and surfaced a real measurement hole, which is the best possible argument for the change you asked for.
You were right that T | string collapses to string, so the unions were decorative and outcome: 'success' typechecked its way to unspecified. I narrowed all five record helpers to their exact unions. tsc then immediately rejected the I4/I5 call site:
src/dkg-agent-lifecycle.ts(1339,11): error TS2322:
Type 'SyncSchedulerLane' is not assignable to type 'SyncOperationLane'.
Type '"pre_authorization"' is not assignable to type 'SyncOperationLane'.
runContextGraphSyncWithBackpressure accepted SyncSchedulerLane (6 members) while I4/I5 recognise SyncOperationLane (4). The two extras — pre_authorization and responder — are absent from OPERATION_LANES, so either one would have clamped to unspecified and dropped that operation out of every per-lane denominator, silently.
Tracing it: both belong to the responder limiter in sync/responder/sync-handler.ts and reach admission through the priority queue, never through runContextGraphSyncWithBackpressure. So the code was correct — but nothing said so, and nothing checked it. That is precisely the "correct by construction and unasserted" shape that M9 had, and that thread 6's failure-injection case was added for.
So the parameter and the requester's ContextGraphSyncWork.lane are now SyncOperationLane, and the compiler proves the invariant instead of a comment asserting it. The genuinely shared admission types — PriorityAdmissionScheduling and acquire in backpressure.ts — keep the wide lane deliberately, because the responder really does use them; narrowing those would have been wrong.
On your confidence note: the clamping stays exactly where you suggested. It lives in the exported normalizers, which are tested directly (normalizeSyncAttemptOutcome('timeout') === 'unspecified'), so tightening these signatures cost no boundary coverage.
agent tsc clean, cli tsc clean, 158 agent W1 tests pass.
| # W1 queries are proven to follow --prom-node-label like every other | ||
| # profile-sensitive surface (the /tmp/render tree is produced by the | ||
| # step above). | ||
| - name: Verify the W1 sync-measurement artifacts |
There was a problem hiding this comment.
🟡 Issue: W1 semantic verification is not triggered by the source contracts it mirrors
What's wrong
The new W1 verifier gives good coverage when observability artifacts change, but the workflow does not run for changes to the production telemetry contract that the verifier is supposed to guard. That leaves an important stale-query regression unverified.
Example
A later PR renames dkg.sync.attempt.request_bytes or adds a new SYNC_ADMISSION_SOURCES member without touching tools/observability; the W1 queries stay stale, but this workflow does not run because the changed path is outside its filter.
Suggested direction
Expand the workflow path filter or add an equivalent CI check in the main test workflow so telemetry-contract-only edits cannot skip the W1 render verifier.
For Agents
Update .github/workflows/observability-artifacts.yml so the W1 verification job also runs when the production sources that define W1 metric names/units/source families change. At minimum include packages/core/src/telemetry-api.ts and packages/agent/src/sync/policy.ts; consider any other source files the verifier intentionally mirrors.
There was a problem hiding this comment.
Fixed in 9e6dd5d30. Same class as the packet-gate finding, and equally right: a verifier that mirrors a contract has to be triggered by that contract, or it is a check that cannot fire on the change that matters.
verify-w1-render.mjs holds its own copies of the W1 instrument inventory (names + units) and the eight-member source vocabulary — deliberately, so the artifacts are checked against an independent statement of the contract rather than against themselves. But that independence is exactly what makes the path filter wrong: a PR renaming dkg.sync.attempt.request_bytes or adding a SYNC_ADMISSION_SOURCES member without touching tools/observability/ would leave every W1 query stale and skip the only check that could say so.
Added both sources you named to the filter:
- 'packages/core/src/telemetry-api.ts'
- 'packages/agent/src/sync/policy.ts'Those are the two the verifier actually mirrors — instrument names/units come from telemetry-api.ts, and SYNC_ADMISSION_SOURCES is declared in sync/policy.ts (catchup-policy.ts and index.ts only re-export or consume it). The workflow already re-triggers on itself, which is the same principle applied to the gate.
Both workflow files re-parsed after the edit.
| * of the right number. Existence is still checked, so this strictly supersedes | ||
| * the old gate. | ||
| * | ||
| * node scripts/verify-w1-packet.mjs [repoRoot] |
There was a problem hiding this comment.
🟡 Issue: The new W1 packet reachability gate is not wired into automation
What's wrong
The script verifies exactly the failure mode it describes, but because nothing calls it, the changed verification behavior is not actually backed by CI or the normal test command.
Example
If a future W1 suite is removed from a package vitest.unit.config.ts include list, scripts/verify-w1-packet.mjs would detect the unreachable suite, but pnpm test and CI will not execute this script unless someone remembers to run it manually.
Suggested direction
Add an automated invocation for the packet verifier, or document and enforce it through the same CI path that claims W1 packet coverage.
For Agents
Wire scripts/verify-w1-packet.mjs into an existing automated path, such as test:scripts, a dedicated package script invoked by CI, or the workflow that runs the W1 packet. The test should fail if any named packet file exists but is not resolved by its pinned Vitest config.
There was a problem hiding this comment.
🟡 Issue: The W1 packet reachability gate is not run by CI or test scripts
What's wrong
The PR adds a script specifically to prevent named W1 suites from silently falling out of the Vitest resolver, but the script is only a standalone file. Without wiring it into CI or an existing test command, the validation evidence depends on manual execution.
Example
If test/sync-operation-telemetry.test.ts stops resolving under packages/agent/vitest.unit.config.ts, this script is designed to catch it. But because nothing invokes node scripts/verify-w1-packet.mjs, normal CI can stay green unless a reviewer remembers to run that standalone command manually.
Suggested direction
Add the new packet gate to an existing CI/test entrypoint, or add a small repository-script test that invokes it and is covered by pnpm run test:scripts.
For Agents
Wire scripts/verify-w1-packet.mjs into an automated path, such as a root script used by CI or a node --test wrapper under scripts/lib/__tests__, so the packet reachability check runs with the rest of repository validation.
There was a problem hiding this comment.
Fixed in 9e6dd5d30, and this is the finding I'm most glad you caught, because it is the same defect the script was written to prevent — one level up.
verify-w1-packet.mjs exists because a packet suite can be present on disk, tracked, and still contribute nothing: Vitest treats positionals as filters against include, never additions, so a suite missing from the include array is silently skipped and the command still exits 0. Shipping that gate uninvoked meant the check that catches "a check that cannot fire" could not itself fire. I had verified the script's logic and never verified that anything ran it.
Now pnpm verify:w1-packet, wired into the Build packages job — chosen because it is the one unsharded lane that installs the full workspace, which the gate needs to ask the real Vitest resolver (vitest list --filesOnly) what each pinned config would actually run.
Verified rather than assumed, using exactly your scenario. Dropped test/daemon-catchup-telemetry-shutdown.test.ts from packages/cli/vitest.unit.config.ts:
W1 packet gate FAILED:
- packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts: EXISTS BUT UNREACHABLE
— not matched by vitest.unit.config.ts's include. Vitest silently skips it
and the packet still exits 0. Add it to that include list.
ELIFECYCLE Command failed with exit code 1.
Restored; back to packet ok — all 16 named suites exist AND resolve under their pinned config, exit 0.
There was a problem hiding this comment.
🔴 Bug: Node UI packet filter uses the wrong package name
What's wrong
The newly added packet verifier runs Vitest through pnpm --filter, but this entry uses dkg-node-ui while the actual workspace package is scoped. That means the new CI step added in this PR does not target the intended package and can fail independently of the packet reachability it is supposed to validate.
Example
The node-ui entry becomes pnpm --filter dkg-node-ui exec vitest list --filesOnly test/telemetry.test.ts, but the workspace package name is @origintrail-official/dkg-node-ui, so the filter may match no package and the new CI gate fails or reports the packet unreachable for the wrong reason.
Suggested direction
Use the exact workspace package name for the Node UI packet entry, and consider surfacing pnpm's “no projects matched” case distinctly from a test include reachability failure.
Confidence note
I could not execute pnpm in this sandbox because pnpm is not installed, but the package-name mismatch is visible from packages/node-ui/package.json and this verifier constructs the pnpm filter directly from the pkg field.
For Agents
Update the node-ui packet entry in scripts/verify-w1-packet.mjs to use pkg: '@origintrail-official/dkg-node-ui'; rerun pnpm run verify:w1-packet and prove it resolves packages/node-ui/test/telemetry.test.ts through the package's default Vitest config.
There was a problem hiding this comment.
🟡 Issue: The W1 packet gate lacks negative-path verification
What's wrong
This PR adds a new verification gate whose purpose is to fail when a named packet suite is silently skipped, but the added validation only runs it on the current healthy tree. Without a negative fixture, the gate can regress into an always-green or success-only check while still giving CI confidence.
Example
A regression that removed the branch checking if (!resolved.has(file)) would still pass this PR’s CI as long as the current Vitest include arrays remain correct; the gate would stop detecting the exact “file exists but is unreachable” condition it was added for.
Suggested direction
Cover the gate itself with fixture tests or refactor the comparison logic into an importable function and test both directions of the set comparison.
Confidence note
I could not run pnpm run verify:w1-packet in this sandbox because pnpm is unavailable, so this is based on the diff and repository test wiring.
For Agents
Add a focused test for scripts/verify-w1-packet.mjs: build a temporary fake repo with a fake pnpm on PATH that returns controlled vitest list --filesOnly output, and assert success, missing-on-disk, exists-but-unreachable, and resolved-but-not-named cases. Put it somewhere reached by pnpm run test:scripts or a dedicated CI step.
There was a problem hiding this comment.
🔴 Bug: The W1 packet verifier filters node-ui by the wrong package name.
What's wrong
The new CI gate depends on this script, but the node-ui packet entry uses dkg-node-ui while the workspace package is named @origintrail-official/dkg-node-ui. That makes the added reachability check fail or resolve no tests for node-ui, so the new workflow can become red for the wrong reason and the telemetry packet reachability contract is not actually established.
Example
The new CI step runs pnpm run verify:w1-packet, whose node-ui arm is effectively pnpm --filter dkg-node-ui exec vitest list --filesOnly test/telemetry.test.ts. The actual workspace package is @origintrail-official/dkg-node-ui, so the filter will not target the intended package and the verifier cannot prove packages/node-ui/test/telemetry.test.ts is reachable.
Suggested direction
Use the actual scoped workspace package name in the packet entry, or filter by the package directory if that is the intended stable selector.
Confidence note
I could not run the verifier because pnpm is not installed in this sandbox, but the package metadata and existing workflow selectors show the filter mismatch.
For Agents
Update the node-ui PACKET entry in scripts/verify-w1-packet.mjs to target @origintrail-official/dkg-node-ui or a path selector, then verify pnpm run verify:w1-packet resolves the node-ui telemetry test alongside the agent and CLI entries.
| m.contextGraphCatchupRequestsTotal, | ||
| m.contextGraphCatchupJobsTotal, | ||
| ]); | ||
| const spies = [...counters].map((counter) => |
There was a problem hiding this comment.
🔴 Bug: Catch-up telemetry tests stub away the metric-name contract
What's wrong
The catch-up telemetry suite verifies values by spying on instrument objects instead of reading exported metrics. That means the tests prove the route called a property on the cached metrics object, but not that OpenTelemetry exports the public metric names the W1 queries use.
Example
Rename dkg.context_graph.catchup.requests_total in packages/core/src/telemetry-api.ts to dkg.context_graph.catchup.request_total. The route tests still pass because they spy on getMetrics().contextGraphCatchupRequestsTotal.add, but the generated W1 PromQL still queries dkg_context_graph_catchup_requests_total, so production dashboards/rules read empty data.
Suggested direction
Add at least one exporter-backed assertion for I7-I9 so a declaration-name typo or mismatch with the generated PromQL fails in tests.
For Agents
In packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts or a shared W1 metrics helper, install a real MeterProvider with InMemoryMetricExporter, drive one request/job/duration path, forceFlush(), and assert the exported metric descriptor names for I7, I8, and I9 plus their attributes. Keep spy-based tests only for never-throws behavior.
There was a problem hiding this comment.
🔴 Bug: Catch-up telemetry tests do not verify the I7/I8 instrument names
What's wrong
The new tests give false confidence for a core observability contract: they can pass even if request counts and job counts are recorded on the wrong OpenTelemetry instruments.
Example
Change recordCatchupRequest() to call contextGraphCatchupJobsTotal.add(1, { result, include_shared_memory }). In these tests, that still hits the same no-op counter spy and requests() still sees a result attribute, but a real telemetry backend would receive I7 data under the I8 metric name and dkg.context_graph.catchup.requests_total would be missing.
Suggested direction
Use exported metric points or distinct fake instruments so the tests prove each route/job record is emitted on the intended metric, not just with the intended attributes.
For Agents
In packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts, capture I7/I8 through a real MeterProvider/InMemoryMetricExporter as the agent W1 harness does, or otherwise make the two instruments distinct and assert the descriptor names dkg.context_graph.catchup.requests_total and dkg.context_graph.catchup.jobs_total. Preserve the existing route outcome assertions, but add a test that fails if either record site writes to the other counter.
There was a problem hiding this comment.
Confirmed and fixed in b2be6f815. Your falsification reproduced exactly, and I ran it rather than reasoning about it.
Applying your rename — dkg.context_graph.catchup.requests_total → ...request_total in packages/core/src/telemetry-api.ts:
cli suite: Tests 30 passed (30) ← green, exactly as you said
verifier: W1 RENDER VERIFY FAILED (1 violation)
- metric declaration: I7 "dkg.context_graph.catchup.requests_total" is queried
by the W1 artifacts but is NOT declared in packages/core/src/telemetry-api.ts
— the queries read a name nothing emits
The hole was worse than the suite. Each W1 name lives in four independent copies — the core declaration, the generator catalog in lib/w1.mjs, the verifier catalog in verify-w1-render.mjs, and the agent test helper. A rename in the declaration left the other three mutually consistent, so the artifacts regenerated unchanged, the verifier passed against its own transcription, and every query read a name the node no longer emits. The spy-based tests were one of four things that couldn't see it, not the only one.
So rather than only adding an exporter-backed assertion for I7–I9, I bound the verifier's transcription to the one copy that decides what is actually exported: the meter.create*('<name>') literals in the core declaration. That covers all nine instruments plus the two corroborating ones, including any that no test drives — which an exporter-backed test cannot do, since it only sees what a test path exercises.
Two properties I kept deliberately:
- It reads source text, it does not import the module. The verifier's independence is the reason it is worth anything; importing the generator's tables would make every assertion self-satisfying. A rename still has to be made consciously in both places — it just can no longer be made in only one.
- Anti-vacuity guard. If the declaration style ever changes (double quotes, a helper wrapper, a name built by concatenation), the regex matches nothing and every name would "pass" against an empty set. That fails now instead — the exact shape this check exists to remove.
It also composes with the path-filter fix from the previous round: the verifier now runs when telemetry-api.ts changes, which is what makes this binding reachable on the PR that would break it. Without that, this check would have been correct and unreachable.
On the exporter-backed assertion specifically: I did not add it, and want to be straight about the tradeoff rather than quietly substituting. packages/cli has no OpenTelemetry dependency at all, so it needs @opentelemetry/sdk-metrics as a devDependency plus a lockfile change. The agent package already has the right harness (test/_helpers/w1-metrics.ts, InMemoryMetricExporter, reading real names — it already pins I9 that way), but it cannot import the cli's catch-up recording code. So the cli version is a real dependency addition, and it would still only cover the three instruments a test path drives. Happy to add it if you want the declaration→export leg pinned too; say so and I'll do it in a follow-up with the dependency cost stated.
There was a problem hiding this comment.
🔴 Bug: Catch-up telemetry tests do not verify the metric being emitted
What's wrong
The test harness gives false confidence for the new I7/I8/I9 behavior: it confirms that some no-op instrument object was called with certain attributes, but not that the production record sites use the intended metric names.
Example
If recordCatchupRequest() accidentally emitted to contextGraphCatchupJobsTotal.add(1, { result, include_shared_memory }), these tests would still classify the point as a request because it has a result attribute, while production would export it under the wrong metric name.
Suggested direction
Use real exported metric points for at least one I7/I8/I9 route/drain scenario, so wrong-instrument regressions fail instead of being reclassified by attributes.
For Agents
In packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts, replace or supplement the spy-only captureCatchupMetrics() path with a real MeterProvider + in-memory exporter, like packages/agent/test/_helpers/w1-metrics.ts, and assert descriptors for dkg.context_graph.catchup.requests_total, dkg.context_graph.catchup.jobs_total, and dkg.context_graph.catchup.job_duration_ms plus their expected attributes.
There was a problem hiding this comment.
Fixed in 55af9362d, and your diagnosis was exact — including that the source verifier "does not bind a property to its public name".
The mechanism is worse than the suggested fix accounts for, which changed the implementation: installing a real SDK provider alone would not have killed your mutant. Both spies wrote into a single shared array, so even with two distinct instrument objects the points still merged. The sink split is the fix, not the provider — which is also why this needs no new packages/cli dependency.
Half A — each instrument now gets its own fake and its own sink, restored by assignment (exact, unlike nested mockRestore() ordering), with an anti-vacuity assertion that the two really are distinct objects. The attribute predicates survive only as a secondary shape check, never as the classifier. Your mutant — pointing recordCatchupRequest at contextGraphCatchupJobsTotal — now produces 11 failures, matching your verified figure exactly. It was 32/32 green before.
Half B — verify-w1-render.mjs now binds property → public name → factory kind, in both directions. Your I7/I8 literal swap produces 4 violations naming both senses.
This closes two instruments beyond the two you named. Sweeping the class: name-bound today are I1–I6 and I9 (the agent harness reads metric.descriptor.name). Bound by nothing were I7, I8, and P1 syncSchedulerQueueWaitMs and P2 backpressureOldestQueuedAgeMs — a repo-wide grep finds those two public names only in telemetry-api.ts, tools/observability/ and docs. All eleven are bound now.
Guards, since this check has several ways to quietly stop working: total-parse failure, partial parse (69 declarations must equal 69 meter.create*( call sites — a dropped declaration is never checked and looks healthy), duplicate property, duplicate name. kind is bound because it drives _total suffixing and the histogram _bucket/_sum/_count set, including I4's _sum strain denominator.
You were right that both halves are necessary: Half B cannot see a record site calling the wrong property, and Half A cannot see a rename in telemetry-api.ts.
There was a problem hiding this comment.
🟡 Issue: include_shared_memory is only type-checked, not value-checked
What's wrong
The new I7 metric contract includes a behaviorally meaningful include_shared_memory dimension, but the tests would pass if every request were attributed to the wrong boolean bucket.
Example
A regression that hard-codes recordCatchupRequest(result, true) for every parsed request would still satisfy typeof point.attrs.include_shared_memory === 'boolean', while durable-only subscribes with { includeSharedMemory: false } would be reported in the shared-memory bucket.
Suggested direction
Add table-driven route tests that send both default and durable-only subscribe bodies and assert the exact include_shared_memory label on the captured I7 points.
For Agents
In packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts, add exact assertions for include_shared_memory across at least default true, includeSharedMemory: false, includeWorkspace: false, and malformed-body false cases. Preserve the one-point-per-return assertions while checking the emitted attribute values, not just their type.
| const { includeWorkspace, includeSharedMemory } = parsed; | ||
| // Resolved before the first return so that EVERY I7 point carries a real | ||
| // `include_shared_memory` value; it depends only on the parsed body. | ||
| const shouldSyncSharedMemory = |
There was a problem hiding this comment.
🟡 Issue: Invalid JSON subscribe requests bypass catch-up request accounting
What's wrong
The new I7 accounting starts only after JSON.parse succeeds. Invalid JSON still produces a 400 response, but it skips the new request counter, so the metric no longer represents one point per subscribe-route return.
Example
POST /api/context-graph/subscribe with body { currently returns 400 through the top-level error mapper, but no dkg.context_graph.catchup.requests_total{result="bad_request"} point is emitted.
Suggested direction
Wrap the subscribe JSON parse in a local try/catch that records bad_request before returning 400, instead of relying on the outer daemon error mapper for this route.
For Agents
In packages/cli/src/daemon/routes/context-graph.ts, handle JSON parse failures inside the subscribe branch and record bad_request exactly once before returning the existing 400. Preserve the existing default include-shared-memory semantics or choose an explicit conservative boolean, and add a test for invalid JSON.
There was a problem hiding this comment.
Confirmed and fixed in b2be6f815. You're right that this breaks the module's own stated invariant — "I7 counts route returns" — and it breaks it for the requests most likely to be retried, so the under-count is biased rather than uniform.
JSON.parse now runs inside the subscribe branch:
let parsed: any;
try {
parsed = JSON.parse(body);
} catch {
recordCatchupRequest('bad_request', false);
return jsonResponse(res, 400, { error: 'Invalid JSON body' });
}On your open question about include_shared_memory: I took the explicit conservative option rather than the existing default. The usual value is (includeSharedMemory ?? includeWorkspace) !== false, i.e. true — but the field that would set it is precisely what could not be read, so defaulting to true would file every unparseable request into the shared-memory bucket and bias a denominator with requests that expressed no such intent. false is the honest reading of "no shared-memory intent was successfully expressed". It stays a real boolean, which the existing per-point assertion requires.
Test added, and I verified it discriminates rather than assuming: dropping the recordCatchupRequest line fails it with AssertionError: expected [] to deeply equal [ 'bad_request' ] — and [] is exactly the old behaviour you described. It needed a new subscribeRaw harness method, because the existing subscribe() helper JSON.stringifys its argument and therefore cannot send a malformed body — which is part of why this went unnoticed.
Suite green at 31.
There was a problem hiding this comment.
🟡 Issue: Oversized subscribe bodies bypass the new I7 request counter
What's wrong
The new catch-up request accounting is intended to emit one request point for each subscribe-route return, but oversized bodies can fail before any of the new recording calls run. That omits a legitimate bad-request outcome from requests_total, which skews the requests-vs-jobs accounting and can hide malformed or retry traffic at the API boundary.
Example
POST /api/context-graph/subscribe with a body larger than SMALL_BODY_BYTES: current behavior is HTTP 413 with no dkg.context_graph.catchup.requests_total sample. Expected behavior is still HTTP 413, but with exactly one I7 request sample, for example result="payload_too_large" and include_shared_memory=false because the body was not parsed.
Suggested direction
Handle PayloadTooLargeError around the subscribe route's readBody call, add a closed telemetry result for that return, record it, then preserve the existing 413 response shape.
Confidence note
I verified the control flow from the route body read to the daemon error mapper, but could not run the suite because pnpm is not installed in this environment.
For Agents
Look at handleContextGraphRoutes around the subscribe body read and the CatchupRequestResult vocabulary in packages/cli/src/daemon/catchup-telemetry.ts. Preserve the existing 413 response behavior, add an explicit closed result for oversized bodies, record one I7 point, and prove an over-limit body gets 413 without minting a job.
Centralize subscribe-route accounting instead of scattering record calls across every return
What's wrong
The PR introduces a route-level invariant, “one I7 point per subscribe-route return,” but implements it by hand at each early return. That makes the already-large route more branchy and leaves the metric contract coupled to incidental control flow instead of a single exit abstraction.
Example
A future subscribe-route branch can return jsonResponse(...) without recording I7 unless the author remembers this scattered convention. The invariant is documented and tested, but the implementation does not make it structural.
Suggested direction
Have the subscribe flow return a typed outcome like {status, body, catchupResult} and make one final responder record I7 and send the response. That would delete the repeated instrumentation calls and make new exits compile through the same accounting path.
For Agents
Look at the /api/context-graph/subscribe branch in packages/cli/src/daemon/routes/context-graph.ts. Preserve the existing response statuses/body shapes and I7 labels, but refactor the branch toward a typed subscribe result or small responder wrapper that records recordCatchupRequest exactly once at the boundary. Add/update focused tests so each result variant still maps to the same metric label.
There was a problem hiding this comment.
🟡 Issue: Subscribe telemetry is scattered across every return instead of centralized at the route boundary.
What's wrong
The route grew several special-case record calls in unrelated branches. This is brittle because the next return path can silently bypass I7 unless the author remembers the telemetry side effect. It also makes the already-large handler harder to scan because route behavior and accounting policy are interleaved throughout the flow.
Example
A new subscribe-route return now has to remember both jsonResponse(...) and the correct recordCatchupRequest(...) call. That is exactly the invariant the comments describe as one point per route return, but the code encodes it as scattered obligations.
Suggested direction
Have the subscribe handler produce a typed route outcome, or at least use a single response helper that records I7 and writes the response together. That deletes the repeated bookkeeping and makes “one point per route return” structural.
For Agents
Refactor only the subscribe route in packages/cli/src/daemon/routes/context-graph.ts. Preserve every current response body/status and emitted result label, but route all subscribe returns through a small typed responder, e.g. return subscribeResponse(result, includeSharedMemory, status, body, headers). The existing daemon catch-up telemetry tests should still pass with the same sequence of I7 result labels.
| } = seam; | ||
|
|
||
| const operations = Math.max(1, Math.ceil(opts.pages / opts.pagesPerOperation)); | ||
| const pagesPerOperation = Math.ceil(opts.pages / operations); |
There was a problem hiding this comment.
🟡 Issue: Benchmark can run more pages than it reports
What's wrong
For non-divisible --pages / --pages-per-operation values, the benchmark over-executes record sites and then divides by the requested page count. That inflates reported ms/page and can make the A18 budget fail even when the true per-page cost is within bounds.
Example
--pages 201 --pages-per-operation 20 computes operations=11 and pagesPerOperation=19, so the benchmark records 209 page attempts but reports elapsed time per 201 pages.
Suggested direction
Iterate over the remaining page count per operation, or divide by the actual number of recorded pages if overrun is intentional.
For Agents
In packages/agent/scripts/bench-sync-telemetry.mjs, make runRound execute exactly opts.pages attempts, for example by tracking remaining pages per operation. Add a small assertion/test or self-check for non-divisible page counts.
There was a problem hiding this comment.
Confirmed and fixed in b2be6f815. Your arithmetic is exactly right: --pages 201 --pages-per-operation 20 gave operations = ceil(201/20) = 11 and pagesPerOperation = ceil(201/11) = 19, so 209 attempts were recorded and divided by 201 — ~4% inflation, on cost the benchmark invented. For an A18 budget check that is the worst direction to be wrong in, because it fails a build for work that never happened.
Pages are now dealt from a remaining counter, so exactly opts.pages attempts run and the last operation is simply short:
const pagesPerOperation = Math.min(opts.pagesPerOperation, remainingPages);
remainingPages -= pagesPerOperation;
recordedPages += pagesPerOperation;I took the self-check option rather than dividing by the actual count, because the reported unit should be what was asked for; a silent divide-by-actual would have made the same class of error invisible again. It throws when recordedPages !== opts.pages, naming both numbers.
Verified to fire — restoring the old Math.ceil(pages / operations) form:
Error: bench-sync-telemetry: recorded 51 page attempts but reports per 50
(operations=3, pagesPerOperation=20) — ms/page would be wrong
Worth noting it tripped on the warmup round (50 pages) before the measured rounds, so the defect was inflating the warmup as well.
Both splits now pass: --pages 201 --pages-per-operation 20 → PASS, and the divisible --pages 200 → PASS.
| let responseBytes: Uint8Array; | ||
| // Resolved once per attempt so all three W1 points describe the same | ||
| // send, and so the ambient source is read once rather than three times. | ||
| const attributes = syncAttemptAttributes({ |
There was a problem hiding this comment.
🟡 Issue: Move the attempt-accounting state machine out of the transport bodies
What's wrong
The new attempt-telemetry.ts module provides low-level record helpers, but the actual attempt lifecycle still lives as ad-hoc flags and finally blocks at each send site. That tangles instrumentation policy with transport behavior and makes the code harder to scan: readers must verify each call site independently to know whether I1/I2/I3 are emitted consistently. This is exactly the kind of duplicated special-case branching a telemetry boundary should absorb.
Example
Both paths construct attributes, record request bytes before the physical send, remember whether a response arrived, classify the terminal outcome, and record I1/I3 in a finally block. The details differ, but the accounting lifecycle is the same concept and is now embedded in two transport call sites.
Suggested direction
Add a focused abstraction such as runInstrumentedSyncAttempt or an explicit SyncAttemptRecorder with recordRequest, recordResponse, and finish(outcome) methods. Keep legacy validation and changelog transport separate, but centralize the shared telemetry lifecycle so the next lane does not copy another finally-block state machine.
For Agents
Look at packages/agent/src/p2p/sync-transport.ts and the changelog send closure in packages/agent/src/dkg-agent-lifecycle.ts. Preserve the current outcome semantics and do not route changelog through legacy retry behavior. Extract a small attempt-accounting helper or recorder object in packages/agent/src/sync/attempt-telemetry.ts that owns begin/request-bytes/response/finalize mechanics while callers supply only transport-specific send and validation behavior.
There was a problem hiding this comment.
This is the third arrival of the send-telemetry extraction (round 1 on dkg-agent-lifecycle.ts:5055, round 2 on attempt-telemetry.ts:157, now here), and the repetition is itself a signal — so rather than repeat the deferral I want to state where it actually stands.
It is filed as #2037 with the design attached, not as a complaint: a single checkpoint hook invoked at all four abort points, with the helper classifying rather than the caller, because only the helper knows whether sendStarted/responded are set yet. Your SyncAttemptRecorder with recordRequest/recordResponse/finish(outcome) is the same shape with explicit methods, and I think it is the better spelling of it — I've added it to the issue as the preferred form.
The reason it is still not in this PR, stated once properly:
- The counter-argument is real, not procedural. Six hooks whose ordering is the contract replaces a visible duplication with an invisible sequencing hazard. Today the ordering is legible inline at both sites; afterwards it lives inside a helper. That is a different trade, not obviously a better one, and it is the kind of thing that gets designed wrong when it is bolted onto a review round.
- Both sites are currently correct and independently pinned — M1/M2/M3/M6, with disjoint kill sets. The acceptance criterion recorded in W1 follow-up: extract the duplicated send-telemetry bracket and the catch-up ledger boundary #2037 is that the extraction must preserve that disjointness: an extraction that collapses two mutants onto one assertion loses coverage while the suite stays green, which is the failure mode this PR has been repeatedly caught by.
- This round already changed the attempt-telemetry signatures (narrowing
T | stringto the real unions, which surfaced a live lane hole). Restructuring the same lifecycle in the same PR would make a regression hard to attribute to either change.
One thing your framing sharpens and I've taken: "readers must verify each call site independently to know whether I1/I2/I3 are emitted consistently" is a better statement of the cost than "two edits per change", because it describes the review burden rather than the typing burden. That is now the problem statement in #2037.
If you think the sequencing-hazard objection is wrong — specifically, that an explicit recorder object with named methods makes ordering more legible rather than less — that is the argument that would move this, and I'd rather have it now than after the extraction.
There was a problem hiding this comment.
🟡 Issue: Extract the attempt telemetry state machine from sendSyncRequest
What's wrong
The implementation bolts a telemetry state machine directly into the transport retry closure. This makes the hot path harder to reason about and makes future transport changes risky because moving a throwIfAborted, validator call, or catch block can silently change accounting semantics.
Example
A reader has to trace sendStarted, responded, and outcome through the send catch, validator catch, outer catch, and finally block to know which metric records fire for one attempt.
Suggested direction
Keep the transport path linear and push accounting into a dedicated helper that receives the labels and wraps a single physical send attempt. That would make the send flow read as request-build → send → validate → return, while the helper owns classification and final records.
For Agents
Refactor packages/agent/src/p2p/sync-transport.ts around sendSyncRequest. Preserve retry behavior and the exact terminal labels, but extract the telemetry state machine into a helper such as runMeasuredSyncAttempt or SyncAttemptRecorder with explicit events for request sent, response received, validation rejected, cancellation, and finish.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
There was a problem hiding this comment.
🟡 Issue: Extract the attempt telemetry state machine out of sendSyncRequest
What's wrong
The new accounting is structurally correct-looking but it makes an already-sensitive transport function much harder to reason about. The four mutable flags and nested handlers are incidental complexity caused by mixing instrumentation lifecycle with transport lifecycle inline. A helper would make the attempt state machine explicit and keep future transport changes from threading through telemetry branches.
Example
The per-attempt telemetry state machine begins at line 136 and runs through line 220 before the withRetry options resume. A reader has to trace multiple flags to understand when I1/I2/I3 are emitted.
Suggested direction
Keep sendSyncRequest as orchestration over span + retry + sync request metric, and move the I1/I2/I3 bracket into a focused helper that takes the already-built per-attempt inputs and returns the response bytes.
For Agents
Extract the per-attempt send bracket from packages/agent/src/p2p/sync-transport.ts into a small helper such as runInstrumentedSyncAttempt(params, attributes) or an attempt recorder object. Preserve the exact emission points and retry semantics; the existing sync-attempt telemetry tests should continue to drive sendSyncRequest.
There was a problem hiding this comment.
🟡 Issue: Extract the attempt-recording state machine instead of duplicating it across transport paths
What's wrong
The telemetry state machine is now spread across two implementations. The legacy path owns one copy in sendSyncRequest, while the changelog path gets a parallel inline copy in dkg-agent-lifecycle.ts. That is the opposite of the new attempt-telemetry.ts abstraction: the low-level record helpers are shared, but the harder orchestration remains duplicated and one copy lives in the wrong layer.
Example
Both lanes now locally manage attributes, request-byte recording, outcome, response length, and final I1/I3 emission. A future outcome or label change has to be applied in both places, one of them buried inside the 8k-line lifecycle class.
Suggested direction
Move the common “physical send attempt” lifecycle into a helper that accepts transport/plane/phase and an invocation callback, or move changelog sending into its own transport module that reuses the same bracket. Keep lane-specific retry and validation policy outside that helper.
For Agents
Look at sendSyncRequest in packages/agent/src/p2p/sync-transport.ts and the changelog send closure in packages/agent/src/dkg-agent-lifecycle.ts. Preserve the current retry/no-retry behavior and exact labels, but extract the shared attempt-recording bracket into a small transport-level helper or a dedicated changelog request module. Tests should still prove the legacy and changelog lanes emit the same I1/I2/I3 contract.
| @@ -0,0 +1,1055 @@ | |||
| // W1 §6.6/§6.7 — catch-up request/job accounting (I7–I9) and the | |||
There was a problem hiding this comment.
🟡 Issue: Decompose the new 1k-line shutdown telemetry suite
What's wrong
This PR adds a new test file over the 1,000-line threshold. The file is not one cohesive scenario; it bundles several independent subsystems and a large local harness. That makes future changes expensive because a reader has to load the whole shutdown telemetry world to edit one narrow behavior.
Example
The file starts by defining a fake worker and route harness, then later contains separate suites for request result metrics, shutdown admission, catch-up drain behavior, job idempotency, teardown ordering, teardown failure resilience, teardown wiring, and ledger-generation release behavior. Those are distinct maintenance units sharing one oversized file.
Suggested direction
Extract the fake worker, route harness, and metric capture into helpers under packages/cli/test/_helpers, then split the test file into focused suites. A good target would be one file for subscribe-route I7 behavior, one for catch-up job ledger/drain behavior, and one for teardown sequencing/wiring.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts before adding more cases. Move shared fake worker, HTTP route harness, and metric capture into test helpers, then separate suites by concern: subscribe request metrics/admission, catch-up drain ledger, and producer-quiescent teardown. Preserve existing assertions and config inclusion.
There was a problem hiding this comment.
🟡 Issue: The new daemon shutdown test file is too large to stay cohesive
What's wrong
This introduces a monolithic test module over the 1,000-line threshold. It combines several independent concerns, which makes future maintenance and review harder even if the behavioral coverage is useful.
Example
A change to only the teardown sequencer has to load and scan the same file that also owns HTTP subscribe fixtures, worker simulation, metric spies, and ledger idempotency scenarios. The shared setup becomes harder to reason about as each area evolves.
Suggested direction
Decompose the 1k+ line test into smaller files with shared helpers so each suite has one reason to change.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts into focused suites: subscribe request telemetry, catch-up ledger/drain, and producer-quiescent teardown. Move the fake worker, route harness, and metrics capture into _helpers modules. Preserve current scenario coverage; this is decomposition, not reducing tests.
There was a problem hiding this comment.
Agreed, and partly acted on already — with one correction to the premise worth making.
The file is not over 1,000 lines because of one PR's worth of scenarios; it is over because the harness is in it. The fake worker_threads module, the HTTP route harness and the metric capture together are a few hundred lines before any test. Your suggested extraction into packages/cli/test/_helpers is therefore the high-value half, and it is the half that doesn't risk anything: moving fixtures cannot change what is asserted. Splitting the suites is the half that can, since each new file re-establishes its own beforeEach/afterEach around process-global state (daemonState.catchupAcceptingJobs, the job ledger, the metric spies), and a subtly different reset is how a green split hides a lost assertion.
There is also a precedent to follow rather than invent: packages/agent/test/_helpers/w1-metrics.ts already does exactly this for the agent side.
What I did this round: the round-3 🔴 fix (binding queried names to the core declaration) went into verify-w1-render.mjs rather than into this file, and the invalid-JSON case is four lines in the existing I7 suite. So the file grew by ~20 lines, not by a new subsystem — I deliberately did not add the exporter-backed harness here, which would have been the thing that pushed it well past the threshold.
What I'm not doing in this PR: the split itself. This PR has changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics — the behavioural diff across those is what a reviewer needs to be able to read, and a simultaneous file reorganisation makes every one of them harder to verify. "Preserve existing assertions" is easy to claim and expensive to check when the diff is a move.
Your three-way split is the right target and I've recorded it — helpers to _helpers/, then subscribe-route I7/admission, catch-up drain/ledger, and producer-quiescent teardown/wiring — with the note that the helper extraction should land first and separately, so the suite split is reviewable as a pure move afterwards. Tracking it against the same follow-up rather than filing a bare "split this file" ticket, since the forcing function is the next change to this area.
There was a problem hiding this comment.
🟡 Issue: Split the new 1k+ shutdown telemetry suite
What's wrong
This PR adds a new test file over the 1k-line threshold and packs several distinct subsystems into it. That is a maintainability regression even if the coverage is valuable: the helpers are trapped inside one huge suite, the test intent is harder to scan, and future edits will keep growing the same file instead of landing near the behavior they exercise.
Example
A change to the fake worker or route harness now requires reading through the same 1k+ file that also owns shutdown-order tests and ledger-idempotency tests.
Suggested direction
Decompose this before merge: keep one concern per test file and extract reusable harness code so future changes do not have to navigate a monolithic suite.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts into focused suites. Preserve the existing assertions, but move shared pieces like workerControl, createHarness, and captureCatchupMetrics into _helpers, then separate request/result accounting, drain behavior, and teardown sequencing tests.
There was a problem hiding this comment.
🟡 Issue: New catch-up shutdown test file is already over 1k lines
What's wrong
The PR introduces a brand-new file past the 1000-line threshold. Even though it is test code, it bundles multiple independent concerns and creates a maintenance hotspot immediately.
Example
A maintainer looking for the shutdown order tests has to scan through route telemetry setup and ledger-release tests in the same file; a maintainer changing subscribe route behavior has to load teardown sequencing fixtures that are unrelated to the route surface.
Suggested direction
Decompose this into smaller focused suites, with shared fake-worker/route/metric harness code extracted once. The test intent will be clearer and future changes will not have to touch a 1k-line mixed-concern file.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts before merging. Extract reusable harnesses under packages/cli/test/_helpers/ if that pattern fits the repo, then separate route telemetry, catch-up drain/worker, teardown sequencing, and ledger identity into focused test files. Preserve the current assertions.
There was a problem hiding this comment.
Re-raised, and my position is unchanged — so rather than restate the same argument a third time, here is where this stands and what would move it.
Tracked in #2037, with the design, the constraints it must preserve, and explicit acceptance criteria — not as a bare "refactor later" ticket. The issue now also carries the concrete evidence this round produced in favour of these refactors: the invalid-JSON I7 gap was exactly the "a future branch forgets the protocol" failure predicted by the dispatcher finding, and it is written up there as the motivating example.
What this PR did instead, and why it is not nothing: every one of these findings is about a contract held by convention. This round replaced four of those conventions with mechanisms — the metric names are bound to the core declaration, the source vocabulary is bound to SYNC_ADMISSION_SOURCES, the teardown order is exhaustive by construction, and each label vocabulary now has exactly one declaration site. Those close the drift half of the concern without restructuring the code whose behaviour this PR changes.
What would move the remaining half: a specific argument that the extraction makes ordering more legible rather than relocated — for the recorder, that named methods beat an inline finally a reader can see; for the route dispatcher, that a typed outcome model preserves the await-free atomicity of both admission guards at the call site. I've asked for that directly in the thread on sync-transport.ts and I mean it: it is the argument I would act on, and I'd rather have it before the extraction than discover it after.
Deferring these is a judgement about sequencing, not a disagreement about the findings. This PR has already changed the shutdown ordering, the teardown sequencer, the admission seam and the ledger release semantics; a simultaneous structural refactor of the same code would make that behavioural diff materially harder to review, which is the thing most likely to let a real defect through.
There was a problem hiding this comment.
🟡 Issue: Split the new 1,117-line shutdown telemetry test file before it becomes the next test harness sink
What's wrong
This new file crosses the 1k-line health threshold immediately and mixes several distinct contracts into one broad harness. That makes the suite harder to scan, harder to change safely, and more likely to accumulate unrelated shutdown and telemetry cases over time.
Example
A failure in daemon-catchup-telemetry-shutdown.test.ts can now come from HTTP route setup, fake worker plumbing, catch-up request counters, job terminal accounting, graceful shutdown admission, teardown ordering, teardown wiring, or ledger release semantics. Those are separate concepts sharing one file-level fixture.
Suggested direction
Decompose the file around the production modules it exercises and share only the minimal test harness utilities.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts into focused suites, for example route/request accounting, catch-up job ledger/drain, teardown sequencing/wiring, and ledger identity unit tests. Move the fake worker and route harness into small helpers so each file exposes one contract and stays under the 1k-line threshold.
There was a problem hiding this comment.
🟡 Issue: Split the new 1,117-line test file before it becomes the catch-up telemetry dumping ground
What's wrong
This crosses the 1k-line smell immediately as a brand-new file and mixes several unrelated testing surfaces. Future changes to catch-up routes, worker shutdown, telemetry flushing, or ledger behavior will all collide in the same enormous fixture, making it harder to find the right setup and easier to add more broad, stateful tests instead of focused ones.
Example
The file starts as one combined W1 shutdown/catch-up telemetry suite at line 1, then later contains separate concerns such as worker drain ordering around line 605, teardown order around line 939, and ledger identity behavior around line 1050.
Suggested direction
Extract the fake worker, metric capture, and HTTP route harness into helpers, then move the describe blocks into smaller suites grouped by production boundary.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts into focused files, for example route request accounting, shutdown admission, catch-up drain/worker lifecycle, teardown sequencing/wiring, and ledger idempotency. Preserve the existing scenarios and keep shared harness code in a helper module.
There was a problem hiding this comment.
🟡 Issue: Split the new 1,138-line shutdown telemetry test file
What's wrong
This PR introduces a test file over 1k lines from scratch. Even though tests are not production code, this is a maintainability regression: the file mixes multiple subsystems and harness layers, making future changes expensive to reason about and increasing the chance that helpers accidentally couple unrelated cases.
Example
A maintainer touching catch-up request metrics has to scan through teardown-order tests and worker lifecycle fakes in the same file; a maintainer touching shutdown sequencing has to load the route harness and catch-up status helpers too.
Suggested direction
Decompose the file by responsibility and extract shared harness helpers. Keep the same assertions, but make each test module own one concept.
For Agents
Split packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts into focused suites such as daemon-catchup-route-telemetry.test.ts, daemon-catchup-ledger-drain.test.ts, and daemon-teardown-order.test.ts. Move the fake worker, HTTP route harness, and metric capture into _helpers modules so each suite reads as the behavior it owns.
…contract Review round 2 on #2033. Three findings, two of which were checks that could never fail — the class this PR has been fighting all along, and one of them was its own gate. 1. `scripts/verify-w1-packet.mjs` was invoked by nothing. It exists because a packet suite can be present, tracked, and still contribute nothing (Vitest treats positionals as FILTERS against `include`, so a missing entry is skipped and the command exits 0). Leaving the gate uninvoked reproduced that exact failure one level up. Now `pnpm verify:w1-packet`, run by the Build packages job — the one unsharded lane with the full workspace installed. Proven by dropping a packet suite from an include list: exits 1 naming the file. Restored, green again. 2. `observability-artifacts.yml` did not trigger on the sources its verifier mirrors. `verify-w1-render.mjs` holds its own copies of the instrument inventory and the eight-member source vocabulary, so renaming an instrument or adding a source in a PR that never touched tools/observability would leave every W1 query stale AND skip the only check that could say so. Added `packages/core/src/telemetry-api.ts` and `packages/agent/src/sync/policy.ts` to the path filter. 3. The record helpers took `T | string`, which collapses to `string` and made the unions decorative — a call passing `outcome: 'success'` typechecked and silently became `unspecified`. Narrowed to the exact unions. That third one was not just a typing nit: narrowing immediately failed the build and surfaced a real hole. `runContextGraphSyncWithBackpressure` accepted `SyncSchedulerLane` (6 members) while I4/I5 recognise `SyncOperationLane` (4). The two extra lanes — `pre_authorization`, `responder` — would have clamped to `unspecified` and dropped those operations out of every per-lane denominator. They belong to the responder limiter and never reach this path today, but nothing said so. The parameter and the requester's `ContextGraphSyncWork.lane` are now narrow, so the compiler proves it. The genuinely shared admission types (`PriorityAdmissionScheduling`, `acquire`) keep the wide lane on purpose. Clamping coverage is unaffected: it targets the exported normalizers directly, not these signatures. agent tsc clean, cli tsc clean, 158 agent W1 tests pass, both workflows parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| const i4count = (src) => selector(I.I4, { series: '_count', matchers: [...LANE, src, SEL] }); | ||
| const i5 = (src) => selector(I.I5, { matchers: [...LANE, src, SEL] }); | ||
|
|
||
| const bytesRate = (src) => `(sum(rate(${i2(src)}${W})) + sum(rate(${i3(src)}${W})))`; |
There was a problem hiding this comment.
🔴 Bug: Byte totals go blank when either byte counter has no series
What's wrong
The W1 byte queries treat an absent response-byte series as “cannot compute”, but absence of I3 is a normal, meaningful zero for attempts that sent a request and never got a response. PromQL binary + drops the result when one side is empty, so the evidence gate and per-family byte materiality can become inconclusive while real request bytes were recorded.
Example
In a 2h window with dkg_sync_attempt_request_bytes_total{source="reconcile"}=10MB and no dkg_sync_attempt_response_bytes* series because every send timed out before a response, sum(increase(I2[2h])) + sum(increase(I3[2h])) returns no data instead of 10MB.
Suggested direction
Zero-fill the I2 and I3 terms separately for byte additions, while keeping the existing coverage gates to detect a dead pipeline.
For Agents
Update tools/observability/lib/w1.mjs so each byte leg is zero-filled independently, regenerate tools/observability/w1/*, and add/prove a request-only window still produces a non-empty byte total.
There was a problem hiding this comment.
Confirmed and fixed in 904b2109a. My comment on that line argued the opposite and was wrong, so this is worth being precise about.
The old code carried an explicit justification for not zero-filling the byte legs: "there an empty operand genuinely means the total cannot be computed, and §7.3 reads that as inconclusive, never as a smaller confident number." That reasoning fails on your case. §6.2 records I3 only when the send RESOLVED, so a window in which every send timed out has real I2 bytes and no I3 series at all — an absent I3 there is a meaningful zero, not an unknown. The total was going blank precisely in the window an operator most needs it, and one where the answer is perfectly well known: the I2 sum.
Each byte leg is now zero-filled independently. The dead-pipeline concern the old comment was protecting still holds — "nothing exported at all" is caught by completed_durable_operations and export_coverage_min_samples, which is what makes zero-filling safe here rather than merely convenient.
Proven, not asserted. Added tools/observability/w1/w1-rules.test.yaml — real promtool rule unit tests, run in the same pinned container that already does check rules:
| window | before | after |
|---|---|---|
| request bytes only, no I3 series | no samples | 10485720 |
| completely silent | no samples | 0 |
| both legs present | 20971440 | 20971440 |
Stripping the zero-fills reproduces got: nil on the first two, so the tests discriminate rather than merely pass.
One thing the tests surfaced that is worth knowing on its own: the committed w1-rules.yaml is not directly evaluable against real data. Every selector carries instance=~"${node:regex}", a Grafana dashboard variable that matches nothing as PromQL — so my first run had all three cases returning 0 for the wrong reason, and would have "passed" vacuously had I written laxer expectations. The test step substitutes only that variable with .*, and greps afterwards to fail loudly if the substitution ever silently stops working. check rules never noticed because the placeholder is a syntactically valid regex.
… unparseable subscribes, fix bench page arithmetic Review round 3 on #2033. 1. RED — the metric-name contract was unverified in the direction that matters. The catch-up suites assert through the instrument OBJECT (`getMetrics().contextGraphCatchupRequestsTotal.add`), which survives any rename of the string handed to `meter.createCounter`. Each W1 name existed in FOUR independent copies (core declaration, generator catalog, verifier catalog, agent test helper), and nothing tied the first to the rest — so renaming an instrument in `packages/core/src/telemetry-api.ts` left the generator, the verifier and the artifacts mutually consistent while every query read a name the node no longer emits. `verify-w1-render.mjs` now binds its transcription to the `meter.create*` literals in the core declaration. It reads SOURCE TEXT rather than importing the module, so the independence that makes the verifier meaningful is kept: a rename must still be made consciously in both places, it just can no longer be made in only one. Includes an anti-vacuity guard — if the declaration style changes so the regex matches nothing, the check fails instead of passing every name against an empty set. Proven with the reviewer's own scenario: renaming `dkg.context_graph.catchup.requests_total` to `...request_total` leaves the cli suite at 30/30 green, exactly as they said, and now fails the verifier naming I7. Restored; verifier green. The verifier already runs in CI and — thanks to the path-filter fix from round 2 — is now triggered by changes to `telemetry-api.ts` itself, which is what makes this binding reachable on the PR that would break it. 2. Unparseable subscribe bodies bypassed I7. `JSON.parse` threw to the outer daemon error mapper, which returned the same 400 from outside the route — a real subscribe-route return with no point, breaking the one-point-per- return invariant the module documents, for precisely the requests a client is most likely to retry. Parsed locally now. `include_shared_memory` is reported `false` rather than the usual default, because the field that would set it is what could not be read. Test kills the mutant that drops the record (`expected [] to deeply equal [ 'bad_request' ]` — `[]` being the old behaviour). 3. The A18 benchmark over-executed on non-divisible splits: `--pages 201 --pages-per-operation 20` ran 11 x 19 = 209 attempts and divided by 201, inflating ms/page ~4% and able to fail the budget on invented cost. Pages are now dealt from a remaining counter, with a self-check that throws if recorded != reported — verified to fire on the old arithmetic (it caught the warmup round too). cli tsc clean, 31 cli tests pass, benchmark PASS on both divisible and non-divisible splits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
…he whole suite `releaseCatchupJob` has two halves: the `=== entry` identity check, and the delete it guards. D3's test pins the first and is structurally blind to the second — both of its jobs share one id, so the ledger holds 1 entry whether or not release deletes, and `drained`/`newerSettled` are satisfied by the slot merely staying occupied. Measured rather than argued: reducing `releaseCatchupJob` to `void entry;` left the suite **31/31 green**. The delete was entirely unprotected. The consequence is not a counting error — `recordTerminalOnce` is idempotent so I8 stays exact — it is unbounded growth: every settled walk job retained for the process lifetime, on a node whose shutdown drain may never come. The new test deliberately performs NO drain. `drainCatchupJobs` ends with `ledger.clear()`, so any ledger-size assertion after a drain is unfalsifiable — it reads 0 regardless of what release did. An earlier attempt at this control sat just below a drain and could not fail; it was removed rather than left as a check that confirms itself. That trap is now documented at the `clear()` site, because the next person will reach for the same placement. Kill verified and narrow: with the mutant applied exactly one test fails (`expected 1 to be +0`), the other 31 pass. Found by qa2, who also caught and corrected their own vacuous control. I had reported that control as merged; it was not in the file, and had it been, it would have been unfalsifiable where it sat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| { id: 'P1', name: 'dkg.sync.scheduler.queue_wait_ms', kind: 'histogram', unit: 'ms' }, | ||
| { id: 'P2', name: 'dkg.backpressure.oldest_queued_age_ms', kind: 'gauge', unit: 'ms' }, | ||
| ]; | ||
| const EXPECTED_FAMILIES = [ |
There was a problem hiding this comment.
🔴 Bug: The W1 source-vocabulary gate does not verify the real source union
What's wrong
This gives false confidence for exactly the drift the new workflow path filter says it covers. A future source added in production can be omitted from the W1 family mapping while CI still reports the semantic verifier green.
Example
Add a new member such as 'peer-repair' to SYNC_ADMISSION_SOURCES in packages/agent/src/sync/policy.ts without touching tools/observability. The workflow would run, but verify-w1-render.mjs would still compare the rendered queries against its own old EXPECTED_FAMILIES copy and pass, leaving the new source absent from W1 classification.
Suggested direction
Bind the verifier to packages/agent/src/sync/policy.ts the same way it already binds metric declarations to telemetry-api.ts, and fail when the source-family table is not exhaustive over the actual union.
For Agents
Update tools/observability/verify-w1-render.mjs around the source-family checks to parse packages/agent/src/sync/policy.ts and compare the actual SYNC_ADMISSION_SOURCES set exactly against the union of the verifier/generator family table. Add or extend a verifier self-test/mutation case for adding a source without updating W1 artifacts.
There was a problem hiding this comment.
🔴 Bug: The W1 source-family verifier is not bound to the production source union
What's wrong
The new verifier gives false confidence for the exact drift the workflow path filter is meant to catch. It validates source matchers against a duplicated expected list, but never checks that duplicated list still equals SYNC_ADMISSION_SOURCES.
Example
Add 'manual-repair' to SYNC_ADMISSION_SOURCES in packages/agent/src/sync/policy.ts without updating tools/observability/lib/w1.mjs or the W1 report. The workflow is triggered by the policy file, but verify-w1-render.mjs still compares the rendered rules to its own old EXPECTED_FAMILIES list, so it would not prove the new source was consciously classified or documented.
Suggested direction
Make the verifier read the production source declaration, similar to its metric declaration binding, and fail when policy.ts has any source not classified by the W1 families.
For Agents
In tools/observability/verify-w1-render.mjs, add an independent binding check that parses packages/agent/src/sync/policy.ts for SYNC_ADMISSION_SOURCES and compares it exactly to the verifier/report family vocabulary. Add a negative/self-test or fixture-style check proving an extra source in policy fails until the W1 families are updated.
There was a problem hiding this comment.
Confirmed and fixed in 904b2109a. Your framing — "false confidence for exactly the drift the new workflow path filter says it covers" — is the sharpest part, and it is correct in a way that makes this worse than an ordinary uncovered check.
In the previous round I added packages/agent/src/sync/policy.ts to the observability workflow's path filter because this verifier mirrors the source vocabulary. That change made a source-only PR run this job and report green — and nothing in the job had ever compared the two sets. So the fix I shipped last round actively manufactured the false confidence you're describing. A trigger without a corresponding assertion is worse than no trigger, because the green result now means something it didn't before.
The verifier now binds to SYNC_ADMISSION_SOURCES, exhaustively in both directions, since EXPECTED_FAMILIES is a decision per source (eligible / excluded / invalidating):
- a real member with no family → fails (
would be silently unclassified by every W1 query) - a family entry for a source that no longer exists → fails (
a dead family kept alive in the queries)
Verified with your exact scenario. Adding 'peer-repair' to policy.ts and touching nothing under tools/observability:
W1 RENDER VERIFY FAILED (1 violation(s)):
- source vocabulary: "peer-repair" is a real SYNC_ADMISSION_SOURCES member but
has NO family in this verifier's table — it would be silently unclassified by
every W1 query
Two anti-vacuity guards, because this check has two ways to quietly stop working: it fails if the export const SYNC_ADMISSION_SOURCES = [...] as const block cannot be located, and again if it parses to an empty set. Same discipline as the metric-declaration binding from the last round — that one had exactly one such guard and this one needed two, since it has both a locate step and a parse step.
It reads source text rather than importing the module, deliberately, for the same reason as the metric binding: importing would make the verifier's transcription self-satisfying, and the whole value of that table is that it is an independent statement of the contract.
There was a problem hiding this comment.
🟡 Issue: Avoid making the W1 contract a set of duplicated tables plus source-text regexes
What's wrong
This creates a brittle three-way maintenance model: production declarations, query generation, and verification each carry their own copy of the same contract, and the verifier compensates by depending on exact TypeScript source syntax. That is a lot of machinery for what wants to be a small typed data model, and it makes benign refactors of telemetry declaration style look like W1 contract failures.
Example
A future cleanup that changes metric declarations from meter.createCounter('name') to a small helper or double-quoted literal now breaks the verifier’s parser rather than the W1 contract itself changing. That is a tooling-shape dependency, not a domain invariant.
Suggested direction
Introduce one explicit W1 contract artifact for metric names, units, label vocabularies, and source-family classification. Generate both the production declarations and observability artifacts from it, and let the verifier assert rendered output against that contract.
For Agents
Look at tools/observability/lib/w1.mjs, tools/observability/verify-w1-render.mjs, packages/core/src/telemetry-api.ts, and packages/agent/src/sync/policy.ts. Preserve the emitted queries and validation behavior, but move the W1 metric/source contract into a small canonical data module or JSON manifest consumed by the telemetry declarations, generator, and verifier. Keep any independence check focused on comparing artifacts to that contract, not regex-parsing implementation source text.
| log: (message: string) => void = () => {}, | ||
| ): Promise<TeardownOutcome> { | ||
| const failures: TeardownStepFailure[] = []; | ||
| const order: TeardownStepName[] = [ |
There was a problem hiding this comment.
🟡 Issue: Make teardown ordering exhaustive by construction
What's wrong
The PR correctly extracts shutdown ordering, but the order is still a manually synced string list beside the interface. That leaves the most important invariant in this module dependent on future authors remembering to update two places.
Example
Adding closeFoo: () => Promise<void> to ProducerQuiescentTeardownSteps would compile while the order array stays unchanged, so the new cleanup step would never run.
Suggested direction
Represent teardown as an ordered tuple of step definitions, or add a typed exhaustiveness assertion that fails when keyof ProducerQuiescentTeardownSteps and the tuple members diverge. Avoid a plain TeardownStepName[], which only validates each element individually.
For Agents
In packages/cli/src/daemon/teardown.ts, make the ordered step list the canonical model or add a compile-time exactness check. Preserve the current order and failure-collection behavior.
There was a problem hiding this comment.
Agreed and fixed in 904b2109a. This is a real hole and it is the worst-shaped one a sequencer like this can have: adding a step to ProducerQuiescentTeardownSteps and forgetting the order array type-checks, ships, and simply never runs that teardown. Nothing throws, no test fails, the resource just leaks on every shutdown — in a function whose entire purpose is "no step is skipped".
TEARDOWN_ORDER is now as const satisfies readonly TeardownStepName[], with a compile-time exhaustiveness guard that resolves to a tuple naming the omission rather than a bare false, so the error text tells you which step you forgot:
error TS2322: Type 'boolean' is not assignable to type
'["TEARDOWN_ORDER is missing steps:", "stopSomethingNew"]'
Verified by adding an unlisted step to the interface, which produced exactly that. Restored; cli tsc clean, 32 tests pass.
Worth noting what this does not replace. The existing wiring test still matters: exhaustiveness proves every step is present, and says nothing about whether the order is right or whether each slot is wired to the resource it names. Those remain assertions, because they are judgements the type system has no view on — the satisfies guard only closes the "silently absent" case, which was the one nothing could see.
| /** One-to-one with the instrumented coalescing maps. */ | ||
| export type SyncSingleFlightScope = 'context-graph' | 'durable' | 'shared-memory' | 'page'; | ||
|
|
||
| const TRANSPORTS: ReadonlySet<string> = new Set<SyncAttemptTransport>(['legacy', 'changelog']); |
There was a problem hiding this comment.
🟡 Issue: Derive label unions from the runtime vocabularies
What's wrong
The closed label vocabulary is expressed twice: once as TypeScript unions and again as runtime Sets. That weakens the type boundary because the compiler cannot prove the accepted values and the clamped values stay identical.
Example
If a future edit adds a new transport member to SyncAttemptTransport but forgets to update TRANSPORTS, TypeScript will accept the new value at call sites while syncAttemptAttributes() records it as unspecified.
Suggested direction
Use one source of truth per closed vocabulary, for example export const SYNC_ATTEMPT_TRANSPORTS = ['legacy', 'changelog'] as const; export type SyncAttemptTransport = typeof SYNC_ATTEMPT_TRANSPORTS[number]; const TRANSPORTS = new Set<string>(SYNC_ATTEMPT_TRANSPORTS);. A small defineLabelVocabulary helper could remove the repeated pattern.
For Agents
In packages/agent/src/sync/attempt-telemetry.ts, derive each union type from a single as const vocabulary array, then build the Set from that array. Apply the same pattern to transports, planes, phases, outcomes, operation lanes, rejection reasons, and single-flight scopes.
There was a problem hiding this comment.
Agreed and fixed in a701c1a7f, applied to all seven vocabularies — transports, planes, phases, attempt outcomes, operation lanes, operation outcomes, rejection reasons and single-flight scopes — using exactly the shape you proposed.
Your failure mode is the one that made this worth doing rather than tidying: adding a member to the union but not the Set type-checks at every call site and then records unspecified. The label space stays valid, cardinality stays bounded, nothing throws — the data just quietly stops meaning what the type says it means. That is the hardest kind of drift to notice, because every visible signal stays healthy.
The clamp sets are now new Set<string>(SYNC_ATTEMPT_TRANSPORTS) and so on — built from the array, never re-listing members.
Worth being precise about what changed, because it is stronger than "now detected": there is no second place to forget, so the drift is unrepresentable rather than caught by a check. That means there is no mutant to demonstrate here — the failure mode no longer has a state to be in. I'd rather say that plainly than manufacture a mutation that only proves the compiler still works.
I skipped the defineLabelVocabulary helper. With as const + (typeof X)[number] the pattern is already two lines and fully inferable; a helper would need a generic signature to preserve the literal union and would make the vocabularies harder to read at a glance, for no additional guarantee. Say so if you'd rather have the uniformity.
agent tsc clean, cli tsc clean, 47 agent W1 tests pass.
| case '--request-bytes': opts.requestBytes = num('request-bytes'); break; | ||
| case '--response-bytes': opts.responseBytes = num('response-bytes'); break; | ||
| case '--json': opts.json = true; break; | ||
| // Host variance on a shared runner is expected, so CI records the numbers |
There was a problem hiding this comment.
🟡 Issue: The A18 benchmark is syntax-checked but never run
What's wrong
The new benchmark is itself part of the verification story for hot-path overhead, but the PR only demonstrates that the file parses. Its runtime contract, real-helper loading, and --ci reporting path can break without any automated signal.
Example
If loadRealSeam() starts failing because dist/sync/attempt-telemetry.js is not produced, an expected export is renamed, or the OpenTelemetry SDK constructor shape changes, node --check still passes and CI never records that A18 has no usable benchmark output.
Suggested direction
Wire the benchmark into an automated smoke path so the real seam load and JSON/report generation are validated, even if the performance budget remains non-gating in CI.
Confidence note
I only see syntax-check evidence and no repo reference invoking the benchmark; if there is an external, non-repo validation job for A18, that would reduce this to a documentation/wiring gap.
For Agents
Add a lightweight smoke invocation after the agent/core build, for example a package script that runs bench-sync-telemetry.mjs --ci --json with small --pages/--warmup/--rounds, or add a focused test for the runtime seam loading/report shape. Preserve non-gating budget behavior if host variance is the concern.
There was a problem hiding this comment.
🟡 Issue: The A18 overhead benchmark is not wired into CI or a package script
What's wrong
The PR adds substantial hot-path telemetry and a benchmark script for the A18 overhead budget, but the benchmark is not reachable from the new verification packet, package scripts, or workflows. That leaves the performance validation unproven by automation.
Example
A regression that makes recordSyncAttempt* add 2 ms/page of overhead can still merge green if no one manually runs node packages/agent/scripts/bench-sync-telemetry.mjs; the new --ci path is never exercised.
Suggested direction
Add a script/workflow invocation for the benchmark so the PR produces repeatable overhead evidence instead of relying on manual execution.
Confidence note
This is based on static search: rg found the benchmark only in its own file and no package script or workflow invocation.
For Agents
Wire packages/agent/scripts/bench-sync-telemetry.mjs --ci --json into a package script and CI step after the agent/core build, and archive or print the JSON so A18 has actual validation evidence. If CI must remain non-gating, still run it and fail only on harness/load errors.
There was a problem hiding this comment.
Fair, and fixed in a701c1a7f — your confidence note was right that there was no repo reference invoking it. There is no external A18 job either; it was genuinely unrun.
Wired as pnpm bench:w1-sync-telemetry:smoke in the Build packages job, placed after the build that produces the seam it loads.
I took your non-gating suggestion, and the flag already existed: --ci sets gated: false and records the verdict without failing. That is deliberate rather than a concession — a shared runner's timing variance must never fail a build, and a perf budget that flakes gets disabled, which would put us right back here. What is gated is the mechanism: the real seam loads out of packages/agent/dist, both arms run, and the JSON report is produced. Tiny --pages 20 --warmup 5 --rounds 2 keeps it a smoke test rather than a measurement.
Your loadRealSeam() example is not hypothetical — earlier on this branch the benchmark refused to run with:
bench-sync-telemetry: dist is STALE: src/sync/attempt-telemetry.ts is newer than
its build output, so this run would measure code that is not on disk.
exit 2. That is exactly the class of breakage this step now catches, and it is why the smoke belongs after the build step rather than in a standalone job with its own build.
Verified locally: exits 0, reports "pass": true, "gated": false.
There was a problem hiding this comment.
🟡 Issue: The A18 overhead benchmark is not exercised by any verification path
What's wrong
This PR adds hot-path sync telemetry and a benchmark that claims to validate the A18 overhead budget, but the benchmark is only a loose file. Because no package script or workflow runs it, the changed behavior has no automated validation for the stated performance budget; the rest of the tests can be green while overhead is never measured.
Example
A regression could make recordSyncAttempt* allocate heavily or add expensive SDK work. The new unit tests would still pass because they assert emitted points and labels, not overhead, and CI never runs node packages/agent/scripts/bench-sync-telemetry.mjs --ci --json.
Suggested direction
Add a script and CI invocation for the benchmark, or otherwise commit reproducible validation output from the real built package so the overhead claim is actually checked during review.
Confidence note
I searched the changed workflows and package scripts; the only references to this file are the script itself, so this is not currently exercised by CI or a named npm/pnpm script.
For Agents
Wire packages/agent/scripts/bench-sync-telemetry.mjs into a documented verification path, preferably via a package/root script and a CI step that runs after the agent/core build with --ci --json so the real built seam loads. Preserve the non-gated CI behavior if runner variance is expected, but make the run itself mandatory and archive/log the result.
…ardown order exhaustive by construction Review round 4 on #2033. Two RED, one YELLOW, all three the same underlying shape: something that looked checked and was not. 1. RED — byte totals went BLANK instead of reporting the request bytes. PromQL binary `+` yields an empty result when either operand is empty, and §6.2 records I3 only when a send RESOLVED — so a window in which every send timed out has real I2 bytes and no I3 series at all. `sum(I2) + sum(I3)` returned nothing, and §7.3 read that as `inconclusive`: blank in exactly the window an operator most needs the number for, and one where the answer is perfectly well known. The old comment argued this was correct ("an empty operand genuinely means the total cannot be computed"). That reasoning was wrong — an absent I3 is a meaningful zero, not an unknown. Each byte leg is now zero-filled independently. It still cannot mask a dead pipeline: that is caught by `completed_durable_operations` and `export_coverage_min_samples`. Proven, not asserted: added `w1-rules.test.yaml`, real promtool rule unit tests. Request-only window now yields 10485720 (was: no samples); silent window yields 0 (was: no samples); both-legs window is unchanged at 20971440. Stripping the zero-fills reproduces `got: nil` on the first two. The tests substitute the `${node:regex}` Grafana variable with `.*` first — it is a dashboard placeholder, not PromQL, and matches nothing against real series, so without that every rule evaluates empty and the suite would pass vacuously. That discovery is itself worth recording: the committed rules are not directly evaluable against real data. The CI step greps to prove the substitution happened. 2. RED — the source-vocabulary gate never compared against the real union. This was worse than an uncovered check: round 2 added `sync/policy.ts` to the observability workflow's path filter BECAUSE the verifier mirrors that vocabulary, so a green run on a source-only change read as "W1 classification verified" while nothing had compared the two sets. `verify-w1-render.mjs` now binds to `SYNC_ADMISSION_SOURCES`, exhaustively in both directions — a new production source with no family, and a family entry for a source that no longer exists, both fail. Verified with the reviewer's scenario: adding `'peer-repair'` to policy.ts alone now fails with that member named. Anti-vacuity guards on both the parse and the empty set. 3. The teardown order was a hand-maintained array beside the step interface. Adding a step and forgetting the array type-checked, shipped, and simply never ran that teardown — silent, in a sequencer whose entire purpose is "no step is skipped". `TEARDOWN_ORDER` is now `as const satisfies`, with a compile-time exhaustiveness guard that names the omission in the error text. Verified: adding an unlisted step fails with `["TEARDOWN_ORDER is missing steps:", "stopSomethingNew"]`. cli tsc clean, 32 cli tests pass, generator --check OK, verifier OK, promtool check rules 66/66, promtool test rules 3/3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| plane: 'durable', | ||
| phase: 'delta', | ||
| }); | ||
| recordSyncAttemptRequestBytes(attributes, bytes.byteLength); |
There was a problem hiding this comment.
🔴 Bug: Changelog telemetry records request bytes for pre-aborted sends
What's wrong
This new record site counts a changelog request before the transport has accepted the send. If shutdown has already aborted the signal, the underlying router fails before opening/writing the stream, but the W1 metrics still report request bytes and an attempted send. That corrupts the measurement denominator the PR is adding.
Example
During daemon shutdown, this.node.stopSignal can already be aborted when the next changelog page is attempted. Current behavior records request bytes and a cancelled attempt for transport=changelog; expected behavior is zero I1/I2 points because no physical send started and no request bytes crossed the boundary.
Suggested direction
Move the changelog I2/I1 emission behind the same pre-send cancellation gate used by sendSyncRequest, so cancelled-before-start work does not enter the attempt denominator.
For Agents
In runChangelogSyncForCg's send closure, mirror the legacy send bracket: check the stop signal before recording request bytes, or introduce a sendStarted guard so I1/I2 are emitted only after pre-send cancellation can no longer reject. Add a test where node.stopSignal is already aborted before the changelog send and assert no attempt/request-byte points are emitted.
There was a problem hiding this comment.
Fixed in 55af9362d. Confirmed exactly as you describe, and your sendStarted-guard framing is what the fix implements.
The bracket now reads the stop signal once and takes a pre-send boundary before building attributes or recording anything, throwing the coerced asSyncFetchAbortError (never throwIfAborted(), which throws reason raw — a non-AbortError reason would reach callers with the wrong name and turn a cancellation into an error).
Two refinements from tracing it, both of which narrow your example rather than contradict it:
- The changelog lane is entered only when the caller supplied no signal, so only
node.stopSignalcan reach here — never a caller signal. - An operation that starts aborted never gets this far; admission rejects it. The live window is intra-driver:
runChangelogSyncis abort-unaware, so a stop landing inside the fully-awaitedapplyPagesurfaces as the next round's pre-aborted send. Bounded at one per lane run.
Two things I changed beyond the suggestion:
- The signal is read once.
node.stopSignalis a getter over a controllerstop()nulls in its finally, so a second read could returnundefinedmid-shutdown. - The catch classifies from the ERROR (
isSyncOperationCancellation), not by re-reading the signal. TypeScript proved the old form unsound — after the guard it narrowedabortedtofalse | undefinedand flagged the comparison as dead, while at runtime the signal really does flip mid-flight. It also catches libp2p'scode: 'ABORT_ERR'DOMExceptions a signal check cannot see, and removes the race where a completing shutdown files a cancellation as a fabricated failure.
Three tests, mutation-proven: pre-aborted (all of I1/I2/I3 empty, sendToPeer never called, rejection carries {name:'AbortError', cause}), a mid-flight control (dispatched, aborts in flight → still records I1 cancelled with its bytes — without it the fix is indistinguishable from deleting the bracket), and a real-router premise test in packages/core proving a pre-aborted send performs zero admission calls.
Worth flagging the mutant that justifies asserting all three legs: moving the guard below the I2 record fails only the I2 assertion — I1 still passes. The obvious I1-only test would have shipped that variant, and it is the one that silently corrupts the byte panels.
| // only inputs the spelling derivation needs, so a unit change in core that is | ||
| // not mirrored here shows up as an empty query, which verify-w1-render.mjs | ||
| // turns into a hard failure rather than a silent zero. | ||
| const W1_INSTRUMENTS = [ |
There was a problem hiding this comment.
🟡 Issue: W1 observability contract is hand-copied across generator and verifier
What's wrong
This is a brittle boundary: the implementation relies on synchronized copies plus source-text regex parsing to keep metrics, generated PromQL, and verification aligned. That is a lot of machinery for what should be a single explicit contract.
Example
Adding a new sync admission source requires coordinated edits in the TypeScript source union, the generator family table, the verifier family table, and the rendered artifacts. The verifier explains this coupling, but the code still makes the contract live in several hand-maintained places.
Suggested direction
Make the W1 contract a reviewed data artifact and have generator/verifier/core checks consume or compare against it. Independence does not require duplicating hundreds of lines of contract data by hand.
Confidence note
The duplication is intentional in comments, but the current shape still creates a high-maintenance contract boundary that can be made explicit without losing reviewability.
For Agents
Look at tools/observability/lib/w1.mjs, tools/observability/verify-w1-render.mjs, and packages/core/src/telemetry-api.ts. Introduce a small machine-readable W1 contract manifest or typed contract module that declares instruments, units, labels, and source families. Have core/generator/verifier compare against that contract rather than hand-copying the tables. Keep the verifier independent by checking rendered artifacts and core declarations against the manifest.
There was a problem hiding this comment.
🟡 Issue: Replace duplicated observability contract tables with an explicit contract
What's wrong
The PR creates a fragile triangle of duplicated inventories and source-family tables, then adds regex-based source parsing to keep them synchronized. That is a lot of custom mechanism for data that wants a typed/schema-like home, and it makes future telemetry changes depend on remembering several parallel edits.
Example
Adding a W1 instrument or changing a source family currently requires coordinated edits across runtime metric declarations, the W1 renderer catalog, the verifier's duplicate catalog, and regex source parsing. The comments call this coupling out, but the implementation preserves it.
Suggested direction
Create one W1 contract artifact/module and make the generator and verifier consume it, while keeping a structured binding check to the runtime declarations.
For Agents
Introduce a checked-in, machine-readable W1 contract module/artifact for instrument ids, names, units, kinds, labels, and source-family classification. Have lib/w1.mjs render from that contract and have verification compare runtime declarations/artifacts against it using structured data rather than duplicating arrays plus regex parsers. Keep an independent check, but make the contract explicit instead of scattered.
…he A18 benchmark Two more from review round 4, both closing a gap between something declared and something checked. 1. Each closed label vocabulary was written twice — once as a TypeScript union, once as a runtime clamp `Set`. They could drift in the direction that hurts: adding a member to the union but not the `Set` type-checks at every call site and then records `unspecified`, so the label space stays valid while the data quietly stops meaning what the type says. All seven vocabularies (transports, planes, phases, attempt outcomes, operation lanes, operation outcomes, rejection reasons, single-flight scopes) are now declared once as `as const` arrays with both the union and the `Set` derived from them. The drift is now unrepresentable rather than merely detectable — there is no second place to forget. 2. The A18 benchmark was only syntax-checked, which proves nothing about what it exists to do: it loads the REAL record helpers out of `packages/agent/dist`, so a missing build output, a renamed export or a changed OTel constructor shape would leave A18 with no usable benchmark while `node --check` stayed green. Wired as `pnpm bench:w1-sync-telemetry:smoke` in the Build packages job, after the build that produces the seam it loads. `--ci` records the verdict WITHOUT gating on it, deliberately — a shared runner's timing variance must never fail a build. What is gated is the mechanism: real seam loads, both arms run, JSON report is produced. Tiny page/round counts keep it a smoke test rather than a measurement. The mechanism gate is not hypothetical: earlier in this branch the benchmark refused to run at all with `dist is STALE ... this run would measure code that is not on disk` and exit 2, which is precisely the class of breakage this step now catches in CI. agent tsc clean, cli tsc clean, 47 agent W1 tests pass, smoke exits 0 with `"gated": false`, both workflows parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
…cess path actionlint failed on the step added in 904b210, and fixing it surfaced a second, worse bug that CI had not reached yet. 1. SC2016 on `sed 's/\${node:regex}/...'`. Replaced with `[$]`, a literal dollar — `$` is only an anchor at end-of-pattern in BRE, so this is exact as well as lint-clean. Verified the substitution still removes every occurrence (grep count 0) and the rule unit tests still pass. 2. SC1072/SC1073: a comment line beginning with the linter's own name was parsed as a DIRECTIVE rather than prose. Reworded, with a note not to do it again — a comment had become code. 3. Found while rewriting, not reported by the linter: the vacuity guard was `grep -q X && { echo; exit 1; }`. Under `set -euo pipefail` that returns 1 from the whole list on the SUCCESS path — grep finds nothing, so the step would have failed exactly when the substitution worked. Rewritten as `if`, where errexit does not apply. actionlint clean on both workflows; promtool test rules 3/3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
| * Metadata only. It reaches no key, no wire envelope and no scheduling | ||
| * decision, and is clamped to the closed source set before use. | ||
| */ | ||
| sourceOverride?: SyncAdmissionSource; |
There was a problem hiding this comment.
🟡 Issue: Model catch-up attribution as a first-class admission value instead of an optional override
What's wrong
The override is a nullable mode-like flag whose semantics are maintained by comments and repeated helper calls. Because it is not part of a single admission model, every caller has to remember where it matters and where it must be excluded, which is exactly the kind of ad-hoc branching this telemetry work should avoid.
Example
syncContextGraphFromConnectedPeers now has to remember: include sourceOverride in runCatchupOverPeers, derive catchupAdmissionSource(mode, sourceOverride) for I6, but deliberately keep it out of contextGraphCatchupSingleFlightKey. That is a sign the model is split across call sites.
Suggested direction
Replace the optional override with an explicit catch-up admission/context object that separates scheduling identity from telemetry attribution once, then pass that object through the policy and join sites.
For Agents
Refactor catch-up admission into a first-class value such as { mode, source, priority } or { schedulingMode, admissionSource } produced once at the boundary. Preserve the current behavior: VM recovery uses background scheduling and vm-recovery attribution, and the coalescing key remains based on work identity only. The resulting code should remove repeated sourceOverride threading and “do not put this in the key” comments.
There was a problem hiding this comment.
🟡 Issue: Model catch-up attribution as a policy object, not an optional override
What's wrong
sourceOverride is a one-off flag that deliberately does not follow the normal meaning of mode, and the PR has to restate that rule at each boundary. That is a missing model: scheduling mode and attribution source are now separate dimensions, but the code represents the second one as an optional exception rather than as part of an explicit catch-up policy.
Example
VM recovery is modeled as mode: 'background' plus sourceOverride: 'vm-recovery', so every intermediate layer has to remember that the override affects telemetry but must not affect priority, peer selection, or the single-flight key.
Suggested direction
Compute a first-class catch-up admission context once and pass it through, rather than threading sourceOverride alongside mode and re-deriving source at several seams.
For Agents
Replace the loose override with a computed catch-up admission/policy value, for example { mode, priority, source }, produced once at the catch-up boundary. Pass that object to scheduling, admission, and join recording, while keeping key builders typed against only the work-identity fields. Existing VM-recovery tests should still show background priority and vm-recovery attribution.
PR #2033 merge-readiness reviewReviewed VerdictNot merge-ready: two introduced high-severity blockers remain. CI is fully green and the broader W1 packet passes from a fresh dependency-closure build, but one shutdown path violates the I1/I2 physical-send contract and the CLI evidence does not detect writing I7 observations to I8. The remaining maintainability concerns already tracked in #2037 are not re-raised here. Findings[P1 / high / introduced] Pre-aborted changelog calls mint a request and terminal attempt before the router rejects themThe changelog bracket records I2 before calling sequenceDiagram
participant W1 as Changelog W1 bracket
participant Router as Protocol router
participant Wire as Admission and transport
W1->>W1: Record I2 request bytes
W1->>Router: Send with pre-aborted signal
Router-->>W1: AbortError during preflight
Note over Router,Wire: No admission, dial, or stream work
W1->>W1: Record I1 cancelled
This is inconsistent with the PR's own legacy path, which checks That creates shutdown-only request bytes and attempts for work rejected before admission. Those points pollute the physical-attempt and byte denominators W1 is intended to make decision-grade. Recommended fix direction: put the same pre-send abort boundary on the changelog lane. Preserve The regression test should keep the real router and assert both Load-bearing locations:
[P1 / high / introduced] A wrong I7 record site survives every claimed CLI/observability checkThe CLI capture harness spies on the OpenTelemetry no-op meter's shared singleton counter object, then infers I7 versus I8 from attributes. That proves the points' shapes, but not which instrument property the production record site used. The source verifier independently checks only that all expected metric-name literals occur somewhere; it does not bind a property to its public name. I mutated the actual route record site from: getMetrics().contextGraphCatchupRequestsTotal.add(...)to: getMetrics().contextGraphCatchupJobsTotal.add(...)and proved the changed source body was the one Vitest loaded. On the current PR, the mutant survived both layers: In production this removes I7 request observations and inserts request-shaped points into I8, invalidating the requests-to-jobs relationship. This is a load-bearing W1 mapping, not only test style. Recommended fix direction: close both independent seams.
I verified the no-new-dependency variant by temporarily installing three distinct fakes on the cached metrics object, restoring the originals after every test, and changing the verifier to a property-to-name map. The clean suite stayed at 32/32. The record-site mutant then produced 11 failures, and an independent declaration mutant was rejected with: Both halves are necessary if this direction is applied literally: distinct fakes alone do not catch a public-name swap in Load-bearing locations:
[P2 / low / introduced] The PR body's byte anchors and test totals no longer describe the reviewed headThis is non-blocking for runtime behavior, but the body labels the values as “Artifact hashes at this commit” and uses the totals as review provenance. At the current head:
Recommended fix: refresh the PR description after the final code push. No source change is required. Executed evidenceThe first advertised partial build failed in a fresh worktree because lower packages had no Restored-head packet: Artifacts and overhead: Mutation audit:
Every mutation and candidate fix was restored afterward. Merge readinessThe PR head remained Recommendation: request changes / do not merge yet. Fix the two P1 items, add the mutation-killing regressions, rebuild and rerun the affected W1 packet, then refresh the PR body. With those changes and a clean delta review, the remaining #2037 work is appropriately deferred and should not block W1. |
…8 to their instruments
Closes both P1 blockers from the merge-readiness review. Each was independently
re-validated against source before implementing, and each fix is proven by a
mutation that was green before it.
## P1-A — pre-aborted changelog sends minted phantom attempts and bytes
The changelog bracket recorded I2 unconditionally and I1 in an ungated
`finally`. The legacy bracket does the opposite: `throwIfAborted` before
`sendStarted = true`, I2 after it, I1 gated on `sendStarted`. Its own comment
names this defect class ("a `finally` on the whole retry closure would fire on
five distinct states, three of which move zero bytes").
`ProtocolRouter.sendInner` rejects an already-aborted signal at
`protocol-router.ts:610`, one statement BEFORE `requirePeerAccepted` and far
before any dial — so nothing was physically invoked, violating I1's stated
contract ("exactly one terminal point per physically invoked send") and
inflating the very denominators W1 exists to make decision-grade.
Reachability is narrower than the review implied but real: the changelog lane is
only entered when the caller supplied no signal, so only `node.stopSignal` can
reach it, and an operation that STARTS aborted is stopped at the admission
boundary. The live window is intra-driver — `runChangelogSync` is abort-unaware,
so a stop landing inside the fully-awaited `applyPage` surfaces as the next
round's pre-aborted send. Bounded at one phantom per lane run.
Two further corrections found while implementing:
- The signal is now read ONCE. `node.stopSignal` is a getter over a controller
`stop()` nulls in its finally, so the second read could return undefined
mid-shutdown.
- The catch classifies from the ERROR (`isSyncOperationCancellation`), not by
re-reading the signal. TypeScript proved the old form unsound: after the
guard it narrowed `aborted` to `false | undefined` and flagged the comparison
as dead — while at runtime the signal really can flip mid-flight. Classifying
off the error also catches libp2p's `code: 'ABORT_ERR'` DOMExceptions, which
a signal check never could, and removes the race where a completing shutdown
files a cancellation as a fabricated FAILURE.
Throws the coerced `asSyncFetchAbortError`, never `signal.throwIfAborted()` —
that throws `reason` raw, and a non-AbortError reason would reach callers with
the wrong `name`, breaking `isSyncOperationCancellation` and turning a
cancellation into an error.
Three regressions, each load-bearing:
- pre-aborted: all of I1/I2/I3 empty, `sendToPeer` never called, and the
rejection carries `{name:'AbortError', cause: theOriginalError}`. The abort
uses a NON-AbortError reason on purpose — `controller.abort()` alone yields a
reason that already has that name, making the coercion assertion unfalsifiable.
- mid-flight control: the send IS dispatched, aborts during flight, and must
still record I1 `cancelled` plus its request bytes. Without it the fix is
indistinguishable from deleting the bracket.
- real-router premise (core): a pre-aborted `router.send` performs ZERO
admission calls. It lives in core because every agent telemetry test replaces
`agent.messenger` wholesale, so the real preflight is never exercised there.
Mutants, all verified: guard deleted → fails; guard moved BELOW the I2 record →
fails on the I2 assertion while I1 still passes (which is exactly why all three
legs are asserted — that variant corrupts the byte panels silently); raw
`stopSignal.reason` instead of the coercion → fails the `cause` assertion;
router preflight deleted → the core test fails on `admittedCalls`.
## P1-B — a wrong I7 record site survived everything
`packages/cli` has no OpenTelemetry dependency, so the API's no-op meter hands
back SINGLETON instruments: `contextGraphCatchupRequestsTotal` and
`contextGraphCatchupJobsTotal` are literally the same object. The harness knew
this and worked around it by classifying points by ATTRIBUTE — which proves
each point's shape and not which instrument produced it. Note this is stronger
than "install a real provider": even with distinct objects, both spies wrote to
one shared array. The SINK split is the fix.
Half A — each instrument now gets its own fake and its own sink, restored by
assignment. Attribute predicates remain only as a secondary shape assertion.
Mutant: point `recordCatchupRequest` at the I8 property → 11 failures, matching
the reviewer's verified figure exactly. It was 32/32 green before.
Half B — `verify-w1-render.mjs` bound only an unordered SET of names, so
swapping two `create*` literals left it byte-identical. It now binds
property → public name → factory kind for all ELEVEN instruments, in both
directions. That closes two instruments the review did not mention: P1
`syncSchedulerQueueWaitMs` and P2 `backpressureOldestQueuedAgeMs`, whose public
names no test in the repo asserts at all.
Guards: total-parse failure, and a PARTIAL parse (69 declarations must equal 69
`meter.create*(` call sites — a dropped declaration is simply never checked, and
looks healthy). Duplicate property and duplicate name both fail. `kind` is bound
because it drives `_total` suffixing and the histogram `_bucket/_sum/_count`
series set.
Mutant: swap the I7/I8 name literals → 4 violations naming both directions.
The two halves are complementary: Half B cannot catch a record site calling the
wrong property; Half A cannot catch a rename in `telemetry-api.ts`.
agent/cli/core tsc clean; agent W1 packet 164, cli 32, node-ui 21, core
protocol-router 65; verifier, generator --check and packet gate all OK.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Merge-readiness review — both P1 blockers fixed, P2 refreshedHead is now P1-A — pre-send abort boundary on the changelog lane ✅Confirmed verbatim: I2 unconditional at Where the report was wrong or incomplete:
Two things I changed beyond your recommendation, both found while implementing:
Three regressions, each load-bearing, all mutation-proven:
The second row is why all three byte legs are asserted: the obvious I1-only test would have shipped that variant, and it is the one that silently corrupts the byte panels. On your P1-B — both halves ✅Confirmed, and the mechanism is worse than you described. You proposed installing a real SDK provider as one option; that alone would not have killed the mutant. Both spies wrote into a single shared array, so even with two distinct instrument objects the points still merged. The sink split is the fix, not the provider. Half A — each instrument gets its own fake and its own sink, restored by assignment (exact, unlike nested Half B — the verifier now binds This closes two instruments you didn't mention. Sweeping the class rather than the instance: name-bound today are I1–I6 and I9 (via the agent harness reading Guards, because this check has more than one way to quietly stop working: total-parse failure, partial parse (69 declarations must equal 69 Also worth recording: the naive regex P2 — PR body refreshed ✅Measured, not projected:
My independently computed hashes match yours exactly. The old ones were CRLF at Two more the body needed:
Verification at
|
PR #2033 merge-readiness follow-up — round 2Reviewed only the delta from the last-reviewed head VerdictNot merge-ready yet: the two reported P1s are fixed, but their fix introduced one new P1 outcome-classification regression. The correction is narrow and verified; no other blocking issue remains in this delta. The #2037 maintainability work remains correctly deferred. Finding[P1 / high / introduced in
|
| Mutation | Result at 55af9362d |
|---|---|
| Delete changelog pre-send guard | Killed |
| Move guard below I2 | Killed specifically on the 62-byte I2 point |
Throw the non-AbortError reason raw |
Killed on {name, cause} |
| Delete real router abort preflight | Killed first on admittedCalls |
| Route I7 request points through the I8 property | Killed: 11 failed / 21 passed |
| Bind I7 property to the I8 public name | Killed by declaration verifier |
| Change I4's factory from histogram to counter | Killed by factory-kind binding |
The fixed pre-abort path reads the node signal once, records nothing before its guard, preserves error coercion, and keeps a genuine mid-flight send counted. The CLI harness now separates instruments by distinct object and sink, and the verifier binds property, public name, and factory kind with parse-parity/duplicate guards. Those close the original two findings.
Every mutation, reproduction, and candidate fix was restored with an inverse edit. git hash-object --path matched the committed head blob for all eight touched production/test/verifier files and final diffs were empty. The agent dist was rebuilt again from restored source before the final packet.
Other review comments
The new online red claiming pkg: 'dkg-node-ui' matches no workspace package is a false positive. I executed both selectors:
pnpm --filter dkg-node-ui exec node -p "process.cwd()"
C:\Projects\dkg-2006\packages\node-ui
pnpm --filter @origintrail-official/dkg-node-ui exec node -p "process.cwd()"
C:\Projects\dkg-2006\packages\node-ui
The exact packet gate also passes 16/16, and the node-ui suite ran 21 tests through the unscoped selector. No package-name change is required. The fresh yellow refactor/test-organization comments repeat the already-documented #2037 class and are nonblocking for W1.
Executed evidence
dependency closure: 17 packages built, exit 0
agent packet: 9 files, 201 passed
CLI packet: 6 files, 166 passed
node-ui telemetry: 1 file, 21 passed
core protocol-router: 1 file, 65 passed
packet reachability: 16/16 named suites exist and resolve
generator check: all generated artifacts match
render verifier: instruments=9, rules=66, selectors=106
check-mode verifier: 6/6 expected outcomes
benchmark smoke: 0.022840 ms/page delta, pass
benchmark full: 0.012082 ms/page delta, pass
The PR body's refreshed packet totals and Git-blob SHA-256 anchors now match this head.
All exact-head GitHub checks have also completed successfully, including the Windows SQLite lifecycle gate and the aggregate CI gate.
Merge readiness
Request changes / do not merge yet. Replace the broad error-class classifier with the captured-stop-signal classifier and add the deadline regression. After that small push, a delta-only rerun of the agent build, the three changelog cases, the W1 packet, and the final head/CI check should be sufficient. I found no reason to reopen #2037 or any other nonblocking review item.
…not the error class Fixes the P1 regression I introduced in 55af936. Replacing the signal read with `isSyncOperationCancellation(error)` fixed a TypeScript narrowing complaint and broke the measurement contract while doing it. `ProtocolRouter` coerces a deadline `TimeoutError` into an `AbortError`, so a predicate keyed on `name === 'AbortError'` / `code === 'ABORT_ERR'` reports a 45-second transport timeout as a caller cancellation. That rule is stated twice in the code I was editing, and I read both: attempt-telemetry.ts — "a deadline `TimeoutError` and a caller cancel land on the same `AbortError` differing only in message text ... Any pre-response rejection that is not caller cancellation is `transport_error`." sync-transport.ts — "the caller's own signal is the only non-textual evidence of caller cancellation that exists." Attempts and bytes were not lost, but the exported terminal state was false in the direction that matters most: it masks network failure as shutdown activity, which is precisely the distinction an operator would use I1's outcome label to make. Now classified from the CAPTURED signal: outcome = Boolean(stopSignal?.aborted) ? 'cancelled' : 'transport_error'; This keeps the good half of the previous commit — `node.stopSignal` is still read exactly once, so a shutdown completing between the rejection and the catch cannot downgrade a real cancellation into a fabricated failure. The captured `AbortSignal` stays aborted for its lifetime even after the node clears the controller behind the getter, which is what makes it durable causal evidence rather than live state. `Boolean(...)` rather than `=== true` because the pre-send guard narrows this to `false | undefined` and that narrowing is unsound — the mid-flight control proves the signal really does flip. Regression added for the deadline case specifically: an `AbortError` whose CAUSE is a `TimeoutError`, thrown while the node is NOT shutting down. It asserts `transport_error`, zero `cancelled`, positive I2 and empty I3. A generic `Error` transport test cannot discriminate this — the mutant that restores the error-class predicate fails ONLY this test, with the other 30 in the file still passing. Also swept the class rather than the instance: `isSyncOperationCancellation` has one other call site, the I4/I5 operation-level classifier. I tried the same correction there and REVERTED it. The two levels are governed differently — I1 has the explicit written rule above because it measures physical sends, while I4 measures a whole logical operation, has no such rule, and an existing test deliberately pins the error-class shape (an inner `AbortError` counts as a cancelled operation even with no admission signal aborted). Changing it is defensible but it is a semantic change to an instrument no reviewer has flagged, so it is documented at the site and raised for a decision instead of made unilaterally. agent tsc clean; agent packet 202, CLI packet 166, core protocol-router 65; verifier, generator --check and packet gate all OK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Merge-readiness round 2 — regression fixed at
|
| @@ -0,0 +1,1008 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
🟡 Issue: Split the new telemetry suites before landing thousand-line test files
What's wrong
The PR creates multiple test files over the 1k-line threshold. These are not single cohesive specifications; they aggregate several distinct telemetry and shutdown contracts into broad mega-suites. That makes future changes harder to localize and pushes reviewers toward editing a wall of fixtures and comments instead of a focused test module.
Example
sync-operation-telemetry.test.ts covers I4/I5 admission, I6 single-flight joins, changelog byte accounting, ambient source propagation, control-plane attribution, and VM recovery labels in one file. The CLI test similarly combines route accounting, shutdown admission, worker drain behavior, teardown sequencing, wiring, and ledger identity guards.
Suggested direction
Decompose these suites along the production boundaries they exercise, and keep common fixtures in focused helpers. This keeps the packet broad without making each file a long narrative artifact that is hard to scan or change safely.
For Agents
Split the new oversized suites by contract boundary. For agent, consider files such as operation-admission telemetry, single-flight telemetry, changelog-attempt telemetry, source-attribution, and VM-recovery attribution. For CLI, split subscribe request accounting, shutdown admission/drain, teardown ordering/wiring, and ledger identity. Preserve helper sharing through _helpers modules and keep the same Vitest include/packet reachability.
There was a problem hiding this comment.
🟡 Issue: The PR adds new test files over 1k lines instead of decomposed focused suites.
What's wrong
This crosses the file-size smell threshold immediately for new files. Even though the tests may be valuable, their current shape makes the W1 behavior harder to navigate, review, and update safely because unrelated contracts are packed into monolithic suites.
Example
sync-operation-telemetry.test.ts combines I4/I5 admission behavior, single-flight joins, changelog send accounting, ambient source propagation, control-plane attribution, and VM recovery labeling in one file. The CLI suite similarly mixes route result accounting, shutdown admission, runner-drain behavior, ledger identity, and teardown sequencing.
Suggested direction
Decompose these into smaller suites before merging. The current structure can keep the same assertions, but each file should own one concept and share setup helpers rather than accumulating the entire W1 matrix in two giant files.
For Agents
Split the new suites by behavioral surface: agent operation admission, single-flight attribution, changelog attempt accounting, source attribution, CLI subscribe accounting, catch-up ledger drain, and producer teardown. Keep shared fixtures in small _helpers modules so each suite stays focused and below the 1k-line threshold.
| try { | ||
| const metrics = getMetrics(); | ||
| metrics.contextGraphCatchupJobsTotal.add(1, { | ||
| status: terminalStatusFor(entry.job), |
There was a problem hiding this comment.
🟡 Issue: I8 status coverage only proves done and failed
What's wrong
The changed job telemetry claims to preserve terminal catch-up status, but the tests only verify successful and failed jobs. Several operator-visible terminal states could be collapsed to failed or omitted without failing the new suite.
Example
If TERMINAL_STATES accidentally dropped denied, a denied catch-up job would be recorded as failed; the current I8 assertions would still pass because they never create a denied/deferred/unreachable terminal job and inspect the metric point.
Suggested direction
Cover every terminal catch-up job status that I8 promises to export, especially denied, deferred, and unreachable.
For Agents
In packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts, add focused I8 tests for denied, deferred, and unreachable, either by driving the route classification with fake runner results or by constructing ledger entries and calling recordTerminalOnce. Assert the exact status label and one job point per job id.
PR #2033 merge-readiness follow-up — round 3Reviewed only the authored delta from VerdictNot merge-ready yet. The latest commit correctly fixes the I1 router-deadline regression and its new regression test is meaningful. However, the I4 question raised in the author’s disposition is a real, production-reachable P1 rather than a harmless difference between instrument levels. One narrow correction remains; I found no other blocker. Finding[P1 / high / introduced by PR #2033, pre-existing before this latest commit] I4 still reports an un-cancelled SWM-recovery deadline as
|
…t that hid it
Applies the same causal rule at the logical-operation level that the previous
commit applied at the attempt level:
outcome = Boolean(admissionBoundary.signal?.aborted) ? 'cancelled' : 'error';
`admissionBoundary.signal` combines the node stop signal and the caller's
`operationSignal` — the whole of the cancellation evidence at this level — and
is read before `dispose()` runs in the outer finally. Being the captured signal
it stays aborted for its lifetime, so a shutdown completing between the
rejection and the catch cannot downgrade a real cancellation.
I had deferred this in the previous commit on the grounds that an existing test
deliberately pinned error-class classification, and that reasoning was wrong on
both halves.
It is production-reachable. `recoverContextGraphSwmFromPeer` admits on the
`swm_recovery` lane and, unlike the durable driver, does NOT fold a router
rejection into a diagnostic result — so a `ProtocolRouter` deadline (coerced to
`AbortError` with a `TimeoutError` cause) escapes through the I4 boundary with
nothing aborted, and was exported as `cancelled`. Network strain reported as
shutdown activity, in the one label an operator would use to tell them apart.
And the test was not a decision. It hand-built `Error{name:'AbortError'}`
labelled "caller gave up", supplied no caller signal and aborted nothing, so it
never exercised cancellation at all — it asserted the classifier's mechanism.
A test written from the implementation rather than the contract will hold the
implementation still, including its bugs. Treating it as a deliberate contract
was the error; it is rewritten, not preserved.
The rewritten test is causal on both sides:
- a plain failure is `error` (retained)
- the router's real deadline shape with NOTHING aborted is `error`
- a caller signal genuinely aborted after work starts is `cancelled` — the
positive control, without which the fix is indistinguishable from deleting
the `cancelled` branch
- the requester-lane clamp assertions are retained
Plus an anti-vacuity witness on the REAL path: `recoverContextGraphSwmFromPeer`
driven with only its network fetch seam replaced, so the production admission
path, lane and I4 record site all execute. It asserts the lane produced a point
before asserting its outcome.
Mutation: restoring the error-class predicate fails BOTH the causal case
(`expected 'cancelled' to be 'error'`) and the real-path witness. It was green
on both before.
`isSyncOperationCancellation` now has no call site in this file; the import is
dropped. The helper remains exported and tested where it lives.
agent tsc clean; agent packet 203, CLI 166, core 65, node-ui 21; verifier,
generator --check and packet gate all OK.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
Merge-readiness round 3 — I4 made causal at
|
| // belongs to the enclosing operation and is read from the ambient | ||
| // context at the record site, which also keeps it structurally out of | ||
| // every coalescing key. | ||
| plane: syncPlaneFor(includeSharedMemory), |
There was a problem hiding this comment.
🟡 Issue: The shared-memory attempt plane is not verified through the real fetch path
What's wrong
This line is where production page fetches decide whether attempt bytes are labelled durable or shared-memory. The W1 decision queries explicitly filter durable bytes, so a wiring regression here would contaminate or hide the denominator. The current tests prove the helper maps booleans correctly, but they do not prove the real fetchSyncPages call passes the shared-memory branch into the record site.
Example
A regression like plane: 'durable' in fetchSyncPages would still pass the helper test (syncPlaneFor(true) === 'shared-memory') and the current real-fetch W1 tests, because they never run fetchSyncPages(..., includeSharedMemory=true, ...) and assert the emitted I1/I2 labels include plane: 'shared-memory'.
Suggested direction
Extend the W1 operation/attempt telemetry coverage to exercise fetchSyncPages with includeSharedMemory=true, not just the syncPlaneFor helper.
For Agents
Add a W1 metrics test that drives the real fetchSyncPages path with includeSharedMemory=true and a successful send, then assert I1/I2 carry transport=legacy, plane=shared-memory, and the expected phase/source. Keep the existing durable case so both sides of the branch are pinned at the production seam.
PR #2033 merge-readiness follow-up — round 4Reviewed only the authored delta from VerdictMerge-ready. The last blocking I4 defect is fixed: operation outcomes now use the captured combined cancellation signal, so an un-cancelled router deadline is There is one non-blocking P2 test-hardening gap below. The current production code is correct and I verified the missing branch directly, so I do not consider another implementation round necessary for merge. Prior blocker — fixed and mutation-provenThe load-bearing change is now: outcome = Boolean(admissionBoundary.signal?.aborted) ? 'cancelled' : 'error';That is the right evidence boundary. It does not infer cancellation from an I rebuilt the full dependency closure first, then generated the deadline with the real rebuilt core The authored causal test and real-lane witness are meaningful against the original regression. Reverting the source to error-shape classification made both fail: Deleting the cancellation branch instead made the real caller-abort positive control fail: This proves the tests reach the changed production source and discriminate both sides of the causal rule. Load-bearing locations:
Non-blocking finding[P2 / test hardening / introduced by the latest commit] The node-stop half of the combined signal is not pinned by the authored I4 testsThe implementation and its comment correctly say outcome = Boolean(operationSignal?.aborted) ? 'cancelled' : 'error';still passes both new authored tests: That mutant would report an in-flight node-shutdown cancellation as Recommended direction: add one positive I4 control with a node stop signal, no caller signal, abort after work starts, and assert Do not apply this literally by calling Executed evidence at restored headAll exact-head GitHub checks completed successfully, including the aggregate CI gate, Windows SQLite lifecycle, EVM integration, and generated-artifact gate. GitHub reports the commit mergeable; the remaining Every experiment was reversed with an exact inverse edit. The online node-UI package-filter comment remains a proven false positive, and the #2037 maintainability items remain nonblocking/deferred. Merge readinessApprove / merge when branch policy permits. The blocker is fixed, the fix is causally correct, the real core deadline reaches the real production lane with the correct I4 result, the meaningful inverse mutants die, the restored W1 packet and artifact gates are green, and CI is fully green. The node-stop I4 positive control is worthwhile follow-up hardening but does not require another merge-readiness round. |
2 participants
Summary
W1 makes sync cost attributable to its trigger. Today a node cannot answer "which
lane is consuming the store?" from any exported metric:
operation(which encodes theadmission source) never reaches an instrument, per-operation bytes are folded into an
in-process sum that is never exported, the changelog lane contributes zero bytes,
and
outcomeis inert forsync-global(alwaysreleased). #2003 solvedsource-attributed pressure on the instantaneous snapshot only — no window query can
compare consumed time by trigger.
This PR adds nine instruments (I1–I9) across both request lanes, attributes them to a
closed eight-value admission source, and ships the query artifacts that turn them into
a decision.
It also fixes a real shutdown defect, which was discovered during implementation and
is not merely telemetry. Shipped code calls
stopTelemetry()atlifecycle.ts:3774— before
server.close()(:3793) andawait agent.stop()(:3794). SincestopTelemetry()shuts the providers down and callsrebuildMetrics(), every latergetMetrics()binds to a no-op meter. Terminating the catch-up worker does notquiesce parent-side sync either: the runner's
handleInvokebridge awaits real agentmethods with only
{priority, source}— no signal, no cancel hook — so analready-started sync keeps running and keeps emitting into nothing. The current
release therefore exports terminal catch-up records while silently dropping the
attempts, bytes and active time that belong to them.
Key decisions, each recorded with its rationale in the plan:
sourcereaches the record sites viaAsyncLocalStorage, not a threadedparameter. Threading would have placed
sourcein the same scope assyncPageFetchCoalescingKey— so the mutation that guards the highest-severityconstraint (§3.1, "source never enters a coalescing key") would have been guarding a
hazard the implementation itself created. Ambient context makes that constraint
structural. It also closes a real gap: the changelog lane's
runResyncfallbackre-enters the legacy lane with no
sourcein scope, which would have attributedthose bytes to
unspecified— the silently-partial denominator the design exists toprevent. Follows the existing
packages/chain/src/rpc-usage.tsidiom.timeoutin the attempt-outcome vocabulary. The router and pool emit at leastseven mutually incompatible deadline shapes, and the only exported classifiers are
.message.includes(...). Any pre-response rejection that is not caller cancellationis
transport_error. Timeout does not participate in the decision rule, so nothingis lost.
error.
makeLegacySyncBusyError's message is matched byisSyncBackoffWorthyErrortoday; replacing it would silently change backoff,
failedPhasesaccounting and thedurable verifiable-prefix return — a behaviour change dressed as telemetry.
MetricReader.forceFlush()applies no timeout at all without
timeoutMillis, andonForceFlush()leaves thetrailing
_exporter.forceFlush()unwrapped.BasicTracerProvider.forceFlush()takesno arguments and must be raced instead. Each leg is bounded individually so each
bound is independently observable — an outer aggregate race would have made both
inner bounds unkillable, which the matrix later confirmed by killing each separately.
runSyncSingleFlighttakes an explicit source at the three generic scopes, ratherthan reading the ambient one. Those scopes coalesce above the admission boundary
(admission happens per-CG inside the factory), so an ambient read there would have
labelled every generic join
unspecifiedand stopped I6's cross-family check fromever firing. Not in the original design; found during implementation.
control-plane, and the label rule stated explicitly.sourceis the operation that triggered the traffic;
control-planeis the trigger when,and only when, no sync operation is. Without it §7.3's "zero
unspecifiedsamples"gate was unreachable:
curator-meta-refresh.tsfetchesplane=durable/phase=meta— inside the decision filter — from outside any admission boundary, so every window on
any node that had joined a private CG was invalidated. The gate was not weakened to
a bounded share; that needs an unprincipled threshold and re-opens the exact hole it
exists to close. The guard tests store presence, not the sentinel value, because
'unspecified'is produced both by "no scope" and by "an admitted operation whosecaller omitted
source" — conflating them would relabel genuinely unattributed trafficas
control-plane, laundering "we don't know" into a confident answer.Related
curator-to-peer binding". The ADR concluded the opposite — authority would have
covered ~0–1.7% of 593 observed CGs — and decomposed D1 into measurement (this PR),
update convergence, and a certificate gate that is parked. The title should be
corrected so nobody builds the anchor.
instantaneous snapshot to windowed instruments.
issue asked for a deliberate choice between guarding each step and enforcing the
no-reject contract at the builder boundary; this PR takes the first option. It also
predicted the guard would mask M17 — it does not: M17 stays killed by direct-call
assertions in
node-ui/test/telemetry.test.tsthat never reach the sequencer,re-verified by re-running the mutation against the guarded version.
Diagrams
Graceful shutdown — telemetry lifetime
Before:
sequenceDiagram participant Sig as SIGTERM participant Tel as Telemetry participant Run as CatchupRunner participant Agent Sig->>Tel: stopTelemetry() — providers DOWN, rebuildMetrics() Note over Tel: every later getMetrics() binds to a NO-OP meter Sig->>Run: close() == worker.terminate() Sig->>Agent: agent.stop() Agent-->>Tel: in-flight sync still emitting → discardedAfter:
sequenceDiagram participant Sig as SIGTERM participant Tel as Telemetry participant Run as CatchupRunner participant Agent Sig->>Sig: catchupAcceptingJobs = false Sig->>Run: drain retained jobs (worker ALIVE, capped) Run-->>Tel: terminal job record per jobId Sig->>Tel: flushTelemetry() — flush only, 2s cap, providers LIVE Sig->>Run: stop workers Sig->>Agent: agent.stop() — ends parent-side sync Agent-->>Tel: final attempts/bytes recorded Sig->>Tel: stopTelemetry() — LASTCatch-up subscribe crossing shutdown
Before:
sequenceDiagram participant C as Client participant R as /context-graph/subscribe participant T as catchupTracker C->>R: POST subscribe (during shutdown) R->>R: subscribeToContextGraph() — persists, gossips R->>T: mint job R-->>C: 200 queued Note over T: nothing will drain itAfter:
sequenceDiagram participant C as Client participant R as /context-graph/subscribe participant T as catchupTracker C->>R: POST subscribe (during shutdown) alt existing job (dedupe / replay) R-->>C: 200 with the existing jobId else would mint a NEW job R-->>C: 503 + Retry-After (before any side effect or id) endFiles changed
packages/core/src/telemetry-api.tsCATCHUP_DURATION_BUCKETS(30 min —OP_DURATION_BUCKETStops out at 120 s and observed jobs ran 305 s/382 s) andSYNC_OPERATION_DURATION_BUCKETSpackages/agent/src/sync/attempt-telemetry.tspackages/agent/src/p2p/sync-transport.tsfinallyafter classificationpackages/agent/src/sync/requester/page-fetch.tsplane/phase; reuse the existing response byte lengthpackages/agent/src/sync/error-tags.tsmarkSyncValidationRejection/isSyncValidationRejection— without itvalidation_rejectedwas unreachablepackages/agent/src/dkg-agent-lifecycle.tsworkclosure; I6 join metadatapackages/agent/src/sync/catchup-policy.ts,dkg-agent-swm-host.tsvm-recoveryfrom the VM-reconcile call sitepackages/cli/src/daemon/catchup-telemetry.tsrecordTerminalOncepackages/cli/src/daemon/teardown.tspackages/cli/src/daemon/routes/context-graph.tspackages/cli/src/daemon/lifecycle.ts,state.tscatchupAcceptingJobs;beginGracefulShutdowncall sitepackages/agent/src/curator-meta-refresh.tscontrol-planesource guard — covers requester AND responder refreshespackages/agent/src/sync/policy.tsSYNC_ADMISSION_SOURCESgainscontrol-plane(eighth member)packages/agent/src/sync/requester/ordered-sync.tsSyncOperationLane, not the wider scheduler lanepackages/agent/src/index.ts,packages/node-ui/src/index.tspackages/core/src/protocol-router.tspackages/node-ui/src/telemetry.tsflushTelemetry()with a real per-leg cap;shutdownTelemetry()made sequentialpackages/agent/scripts/bench-sync-telemetry.mjsb2be6f815tools/observability/lib/w1.mjs,verify-w1-render.mjs,verify-check-mode.mjs,w1/tools/observability/w1/w1-rules.test.yamlcheck rulesproves parsing, these prove what the expressions returnscripts/verify-w1-packet.mjspackage.jsonverify:w1-packetandbench:w1-sync-telemetry:smokescripts.github/workflows/ci.yml.github/workflows/observability-artifacts.ymlpromtool test rulespackages/agent/vitest.unit.config.ts,packages/cli/vitest.unit.config.tsincludeis silently skipped)packages/agent/test/:sync-attempt-telemetry,sync-operation-telemetry,sync-transport-metrics,core-fills-gap,_helpers/w1-metrics.ts·packages/cli/test/daemon-catchup-telemetry-shutdown.test.ts·packages/core/test/protocol-router.test.ts·packages/node-ui/test/telemetry.test.tsTable verified complete against
git diff --name-only origin/testnet-canary...HEAD(40 files). An earlier revision omitted 23 of them, including four agent/node-ui source files — two of which the Scope note above already named, so the table contradicted the prose.Test plan
Commands
Windows / Git Bash: that last line needs both an env guard and a Windows-form path,
or MSYS rewrites the container-side
/wand Docker rejects the mount:MSYS_NO_PATHCONV=1 docker run --rm -v "$(pwd -W)/tools/observability:/w" --entrypoint promtool prom/prometheus@sha256:6559acbd…e218 check rules /w/w1/w1-rules.yamlPowerShell and Linux need neither. Flagged because a reviewer following the plain form
on Windows gets exit 125 and will reasonably read it as a broken artifact rather than a
shell quirk.
Artifact hashes, so any later citation is checkable rather than assumed. Taken over
the LF blob at head
4ce70ec57. The convention matters and was previously unstated,which made the citation unreproducible even at the right commit: a Windows checkout
rewrites these files to CRLF and hashes differently. The earlier values were also taken
at
2456be4b1, eight commits before the head they were labelled with.Results
All figures below were re-derived against the final commit by a reviewer who did not
write the code, with a clean tree verified immediately before and after each command.
protocol-router(the P1-A premise)generate-observability.mjs --checkverify-w1-render.mjsverify-check-mode.mjspromtool check rules(pinned digest)pass=true. The earlier4.35–6.97 µs/pageis withdrawn —b2be6f815fixed page arithmetic that inflated ms/page on non-divisible splits, so that figure was produced by the very code this branch later called wrongpromtool's 66 andverify-w1-render'srules=66are derived independently and agree.A17 is measured, not asserted — and re-derived per file rather than by arithmetic on a
remembered total. A pristine
a97b99714worktree carrying only the vitest allowlistline yields 7 files / 154 tests. The same seven files on this branch:
154 + 42 = 196reconciles independently with the runner's own total. The CLI packetdecomposes the same way:
And the counts are corroborated by a structural check, because a matching total cannot
by itself rule out two offsetting edits:
All twelve pre-existing packet files are byte-identical to the W1 base. So 154
and 134 are not numbers that happen to match — those files could not have changed count,
because they did not change at all. The three W1 suites are confirmed genuinely new
(
git cat-file -efails for each at the base commit).Against the old referent this would have read 152 → 196 and the gap would have been
waved through as "new tests".
The verification packet itself was defective
§8.3's command named 7 agent files and ran 6, exiting 0. Vitest positionalargs filter the config's
includearray rather than extend it, andsync-transport-metrics.test.tswas absent from it. The pre-flight gate could not catchthis because it tested
fs.existsSync, and that file has been tracked sincea148062ad. The gate checked the wrong property. It is now a reachability gate(
scripts/verify-w1-packet.mjs) that asks the real Vitest resolver which files it wouldrun and compares the resolved set against the named set, in both directions.
A later pass found the packet also omitted the two new suites carrying 8 of 17
mutants — which would have produced eight false survivors, each reading as "the tests
are too weak". Reachability and completeness are different properties, and the gate can
only check the former, because it is fed from the packet.
What the ALS tests do and do not prove
Stated precisely, because the imprecise version is more flattering.
runResyncfallback test is the real content. A changelog responder returnsresync, the fallback re-enters the legacy lane, and both the changelog and legacyattempts carry the one operation's source, with I2 bytes on the legacy leg. This is
the concrete proof of the argument the ALS deviation was approved on — under
parameter threading that call site has no source in scope and would have reported
unspecified, the silently-partial denominator.runContextGraphSyncWithBackpressureuseslaneat three sites, all label-only, andnever branches on it;
laneis absent from the I1–I3 attempt path. So all fourtraverse an identical path and cannot have distinct kill sets — any mutation that
kills one kills all four. They are a cheap forward guard against a future
lane-specific divergence, not four present-tense proofs.
withSyncAdmissionSourceto a pass-through kills 10 tests — including thedetached-spawn test, which does not test that property. It removes the mechanism they
all depend on. Withdrawn as evidence of per-property coverage.
to
syncPageFetchCoalescingKey's result kills the page test only; appendingsourceOverridetocontextGraphCatchupSingleFlightKey's result kills thecontext-graph test only. Disjoint kill sets, so each scope is separately pinned —
which matters, because §3.1 (source must never enter a coalescing key) is the
highest-severity constraint in the design.
Known limitations
Stated because a change set with no disclosed caveats is less believable than one with
several.
swm_recoveryis not driven end-to-end through the production recovery driver.The per-lane test proves the ambient source reaches the record site under that lane
label; the real driver's call tree was exercised only via the single-flight path.
it is the one assertion here that could flake on a saturated runner. Suspect load
before code.
vitest.unit.config.tsrun neverfinished on the development box (>80 min against 40+ node processes) and is reported
as not run, not as passing.
daemon-http-behavior-extra.test.tsis likewise notrun — it boots Hardhat and collides with concurrent worktrees.
rfc64-public-catalog-native-gate1.integration.test.tsfails locally with aWindows
EBUSYunlinking a SQLite lease journal inafterEach, stranding the leaseand timing out every later test in the file. Evidence says pre-existing and unrelated:
the file and
src/rfc64/are unmodified on this branch, no module changed hereappears in its import graph, the lease code was last touched by
4306c6f14(fix(agent): persist finalization recovery in SQLite #1939),and the same 30 s expiries reproduce on a pristine pre-W1 tree. Three local
attempts at a clean verdict were confounded by machine load; CI is the arbiter. If it
is red there it gets its own issue rather than being absorbed here.
buildProducerQuiescentTeardownStepsis tested one step at a time for slotassignment, and the order is tested separately; nothing short of executing
runDaemonInnerproves the composition, and a source-text assertion onlifecycle.tswas rejected as brittle.(
withSyncAdmissionSource→monotonicNowMs→recordSyncOperationDuration), notthe boundary function, which has no extractable wrapper. The boundary's own cost is
admission and store work this PR does not add; including it would put a large constant
in both arms and inflate variance while measuring nothing new.
than guarded: a record site running outside an established scope reports
unspecified(thread boundary — loud, invalidates the window) or inherits a stale label (bare
detached spawn — silent). Neither exists today.
sync-verify-workerrecords nothing,and the only voided spawn in the package that can send —
runImmediatePostApprovalSyncatdkg-agent-lifecycle.ts:2911— sits in a gossipsubcallback bound at subscribe time, so it has no ambient scope to inherit and
establishes its own. Moving that call inside an admitted operation would make it live.
The durable half of the audit is structural: only four modules in the package can
send, and the store-commit and verification trees contain no sender at all.
Mutation matrix — how to read it
Every result carries its reason, not just its verdict, because three of them are
true-but-worthless in ways a results table cannot show:
calling the exporter when the span queue is empty — it would have passed with or
without the guard and been recorded as a kill that never happened. A precondition was
added (queue and end a real span, assert the exporter was reached); it now dies at
10 018 ms. A fabricated kill caught before it entered the record.
constthat is then discarded on the 503path — no tracker insert, no response field, no side effect. No test can
distinguish it. Withdrawn and replaced by M12f′ (invert the guard so it returns 200
without the tracker insert), which kills 2 tests. A survivor logged without
explanation would have read as "A21 coverage is weak", which is false.
moving it, producing a 7-element sequence — so it killed on "a duplicate step is
present", not on "the order is wrong". Redone as a genuine reorder.
Kill sets are compared explicitly, because identical sets prove one fact N times.
They must differ when the facts differ — two mutants breaking the same guard
should share a set. M12c/M12f′ correctly share one. M12a/M12b share one and should
not: A23 (drain while the worker is alive) and A24 (provider live during
agent.stop()) are different guarantees detected only by "the step sequence equals thisliteral array". Their consequences still follow — from that order plus the
separately-pinned behaviour of
close()(rejects every pending run) andstopTelemetry(disables the provider) — so the argument is complete but distributed, not
unverified. Nothing asserts either consequence end-to-end.
Where our evidence is narrower than our claim
Kept deliberately, because it tells a reviewer where to look rather than asking them to
trust a total. Several entries were found by the person who wrote the thing.
Closed
OP_DURATION_BUCKETSsurvived every suite in the repo: the only 305 s sample livedin an attribute-key sweep that never inspects
buckets.boundaries, the CLI suitemocks
record, and the harness had no bucket accessor. Now guarded, and verified inboth directions against two recorded
disthashes.in-window at all four coalescing scopes.
turning one real failure into eleven.
> 0assertion under a name promising a single-runcount, tightened to an exact
toBe(2)and proven to fire first at 4.Open
consumes I4's
sumandcount, never its distribution, so a collapsed floor cannotmove a verdict. Sketch for the follow-up — a 15 ms and a 45 ms sample must not land
in the same bucket;
harness.buckets(I4)already supplies the observable.control-planeresponder call site — the guard is proven on both branches;the responder's precondition is reasoned from source rather than executed. Stated
rather than covered.
Follow-ups filed
Not a live defect (
flushTelemetrycannot reject today); the invariant is enforcedinside one function rather than at the sequencer that depends on it.