feat(mt#3866): Give calibration records an identity, and report distinct fires as a range - #3656
Conversation
… and report both counts
SC1 — `captureFields(judgedText)` stamps `captureSchema` AND `judged_text_hash`
together, so a writer can no longer take the marker without the identity. That
separability was the defect: all 58 records in the measured `ask-routing-deferral`
window carried the marker and no identifier, because the writer took
`extractMatchContext` (returns a string) rather than `captureArtifact` (returns
`{excerpt, hash}`). Adopted at both of that detector's record sites.
SC2 — the sweep gains `distinctFiresSinceLastReview` and
`ungroupableSinceLastReview` beside the raw record count, projected into the JSON
path an agent reads (mt#5011's defect, not repeated).
SC4 — a record with no digest lands in the un-groupable column and in NEITHER
other column. It is not evidence of a distinct fire and not evidence of a
duplicate; the pair bounds the true count to a range.
`readJudgedTextHash` reads the passthrough only — measured, not assumed: no
per-kind parse branch names this field, verified by parsing a
retrospective-trigger line, so a top-level branch would be dead code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
… a sibling stream's digest [no-deploy-impact] The digest coupling makes records groupable going forward and does nothing for the corpus already on disk, which is where every recorded duplicate group lives. `scripts/resolve-calibration-duplicate-groups.ts` runs the join `## Evidence 2026-08-16` found as a one-off: `(session_id, timestamp +/- 1s)` against the one evaluation stream that hashes its judged input. Live result on `ask-routing-deferral` — 6 duplicate groups, 5 RE-SCAN, 1 DISTINCT, 0 unresolvable. The DISTINCT verdict is the control: the join discriminates rather than returning one answer. Coverage bound measured with `has()` rather than a null read, and stated in the docblock: causal-premise 659/659 carry a digest; the other eleven evaluation streams carry 0. So `unresolvable` means "no oracle covered this turn", never "these are distinct" — and the script says so in its own output. Resolves through the WRITER's `calibrationLogPath`/`evaluationLogPath` rather than a hand-rolled state-dir key, which is the mt#4971 defect one step along: a session workspace hashes to a different project key and would report a clean zero over an empty corpus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Overall, this PR meaningfully advances mt#3866 by coupling a judged-text digest into calibration writes for ask-routing-deferral and adding distinct/ungroupable counts to the sweep and CLI projection, with solid tests and a helpful resolver script. However, I found a blocking correctness issue in the sweep: countDistinctFires groups solely by digest, collapsing identical-text fires across different sessions into one, undercounting distinct events. Scope-wise, not all writers are updated, so SC1 (“Every calibration record…”) remains unmet (recorded in spec verifications). Documentation updates are also required to reflect the new judged_text_hash field and sweep outputs. Non-blocking: the resolver’s join is O(N×M) per group (indexing would help), and the string key duplication risks drift. Address the session-aware grouping bug and update docs (or adjust scope/spec), and this will be in good shape to merge.
Findings
- [BLOCKING] src/domain/calibration/calibration-sweep.ts:2224 — Distinct-fire counting collapses across sessions — digest-only key undercounts when different sessions produce identical text
countDistinctFires(added in this PR) keys distinctness on the judged-text digest alone:
const digests = new Set<string>();
// … add readJudgedTextHash(record)
return { distinct: digests.size, ungroupable };This collapses fires from DIFFERENT session_ids that happen to share the same sentence/hash into a single "distinct" fire. The defect the PR aims to fix is explicitly framed per-session (e.g., “four records in the same session…”), and grouping across sessions materially undercounts distinct events when common phrases recur. The join-based resolver and comments repeatedly scope identity to a session. The fix is to scope the key to (session_id, digest) — e.g., add record.sessionId (or the parsed equivalent) to the set key — so two sessions with the same text do not collapse.
- [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:152 — Borrowing digests is O(N×M) per group — index the oracle by session and time for scalability
borrowDigestlinearly scans the entire oracle array for every calibration entry:
for (const o of oracle) {
if (o.sessionId !== entry.sessionId) continue;
const delta = Math.abs(o.atMs - entry.atMs);
// …
}On large logs this becomes quadratic. Since joins are session-scoped and time-bounded, index oracle by sessionId with a sorted array of timestamps (or a Map to a balanced tree) and binary search within the toleranceMs window. This will make the script robust to larger corpora without changing behavior.
- [NON-BLOCKING] .minsky/hooks/ask-routing-deferral-detector.ts:2008 — Partial adoption of
captureFields— verify all calibration write sites in this detector and siblings
This PR updates the two visible write sites inrun()andmain()to spreadcaptureFields(assistantText). If there are any other calibration-record writes (e.g., alternate branches, error paths, or later refactors) they may still stamp onlycaptureSchema. A quick grep suggests only these two sites here, but the campaign-wide concern remains: other detectors still takingextractMatchContextwill stay ungroupable until similarly updated. Consider adding a lint/test canary that fails when a calibration record is written withcaptureSchemabut withoutjudged_text_hashto prevent regressions. - [NON-BLOCKING] src/domain/calibration/calibration-sweep.ts:2196 — String key duplication (
judged_text_hash) risks drift across layers
JUDGED_TEXT_HASH_KEY = "judged_text_hash"is re-declared here with a doc note mirroring theCAPTURE_SCHEMA_KEYduplication pattern. While acknowledged, this still risks future drift if the hook layer renames the field. If feasible, centralize these keys (e.g., a tiny shared constants module) or add a round-trip/parsing test using a real line from a writer to pin the contract.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Every calibration record carries an identifier sufficient to answer "is this the same fire as that one?" — at minimum a hash of the judged text, and where the writer has it, a turn anchor. | Not Met | This PR couples a digest stamp via captureFields(judgedText) and adopts it at two write sites in .minsky/.claude/ask-routing-deferral-detector.ts (e.g., .minsky/hooks/ask-routing-deferral-detector.ts:2010..2016, 2162..2166). However, no other writers are updated; most detectors still lack the identifier. There is no central write-path change (logCalibrationRecord) to ensure all records receive the digest. Therefore, "Every calibration record" is not satisfied — only ask-routing-deferral (plus the pre-existing retrospective-trigger writer) carry the new field. |
| The sweep can report distinct-fire counts alongside raw record counts, so a reviewer sees both without hand-grouping by context string. | Met | src/domain/calibration/calibration-sweep.ts adds countDistinctFires, distinctFireFields, and new CalibrationLogResult fields distinctFiresSinceLastReview and ungroupableSinceLastReview (e.g., :1096–1115, :2194–2291). The CLI projection exposes them in src/adapters/shared/commands/calibration.ts (:1088–1100). Tests assert the behavior (e.g., calibration-sweep.distinct-fire.test.ts). |
| The four duplicate groups above are re-examined once the identifier exists, and the answer (repeat authoring vs re-scan) is recorded. | Not Met | A new script scripts/resolve-calibration-duplicate-groups.ts implements the cross-stream join and the PR body shows a live run for ask-routing-deferral. However, the spec-named four historical groups (e.g., operator-deferral’s five-record cluster) were not re-examined — the PR explicitly states the corpus is gone and evaluation streams carry no digest for that detector. No artifact in-repo records answers for those four groups. This criterion remains unmet and needs either amended scope or a follow-up task to capture the answers/limitations. |
Existing records without the identifier are treated as un-groupable rather than assumed distinct; a rate computed over a mixed population states which half it covers, per the captureSchema precedent in mt#3607. |
Met | countDistinctFires returns {distinct, ungroupable} with digest-absent records incrementing ungroupable and not included in distinct (src/domain/calibration/calibration-sweep.ts:2247–2291). Tests calibration-sweep.distinct-fire.test.ts explicitly assert distinct: 0, ungroupable: 3 for digest-less records and mixed-window behavior. |
Documentation impact
- blocking-needs-update — This PR adds a new persisted field
judged_text_hashto calibration records and introduces distinct-fire reporting in the sweep. The authoritative docs for calibration/guard streams live underdocs/architecture/(e.g.,evaluation-loop-fire-log.md,guard-calibration-stream-inventory.md). None of these are updated in the diff. The inventory/doc should reflect the new field on matched-phrase calibration-record shape (at least noting its presence and semantics) and the new sweep result fieldsdistinctFiresSinceLastReviewandungroupableSinceLastReviewso external consumers and operators aren’t misled by stale schemas. Absent doc changes, existing prose implies no identifier exists on these records, which is now false for adopters. Affected docs: likelydocs/architecture/guard-calibration-stream-inventory.md(per-stream record-shape notes) and any doc section that describes calibration-record fields and sweep outputs.
…turn_key The success-criteria seam surfaced the half I had not built: "at minimum a hash of the judged text, AND WHERE THE WRITER HAS IT, a turn anchor." The dispatcher path has one — `ctx.recordedAnchor.turnKey`, the opening prompt line's uuid. Stamped BESIDE the digest, not instead of it. They answer different questions and only one is universal: the digest answers "same TEXT" on every path, and `turn_key` answers "same TURN", which is the limitation `captureFields`' own docblock names — two distinct turns emitting the identical sentence hash the same and would group as one. The sweep still groups on the digest. A window mixing records with and without a turn key would split one turn across two grouping keys, which is an over-count in the direction this task exists to remove; the field is here for a reader or a replay that wants the finer answer. 5 tests, including the control (a different turn yields a different digest) and the absence case (no anchor leaves the key ABSENT, not an empty string — an empty string would group every anchorless record together). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
R2 verification: the prior BLOCKING issue (distinct-fire counting keyed only on digest) remains — countDistinctFires still collapses identical-text events across different sessions, undercounting distinct fires. Scope-wise, SC1 and SC3 from the spec are still unmet (identifier not on every writer; historical groups not re-examined), recorded in spec verifications. I also see no doc updates for the new persisted judged_text_hash field and new sweep outputs, so documentation needs updating. Non-blocking: the resolver script’s join is O(N×M) per group; consider session-indexing, and the duplicated string key for judged_text_hash risks drift. Please scope the distinctness key to (session_id, digest), add/pin a test, and update docs (or amend spec) — with that, the PR will be in good shape.
Findings
- [BLOCKING] src/domain/calibration/calibration-sweep.ts:2224 — Distinct-fire counting still collapses across sessions — keyed only by digest
countDistinctFiresgroups on the judged-text digest alone:
const digests = new Set<string>();
// … digest = readJudgedTextHash(record)
// … digests.add(digest)
return { distinct: digests.size, ungroupable };This collapses identical-text fires from DIFFERENT session_ids into one "distinct" event, undercounting when common sentences recur across sessions. The task’s framing and the join-based resolver scope identity per session (“four records in the same session…”). Fix by scoping the key to (session_id, digest) — e.g., add the parsed session id to the set key so two sessions with the same text do not merge. Tests asserting per-session grouping should be added/pinned.
- [BLOCKING] docs/architecture:1 — Documentation not updated for new persisted field and sweep outputs
This PR adds a new calibration-record fieldjudged_text_hashand exposes new sweep result fieldsdistinctFiresSinceLastReviewandungroupableSinceLastReview(e.g.,src/domain/calibration/calibration-sweep.ts,src/adapters/shared/commands/calibration.ts). The task scope explicitly includes “Docs describing the record shape.” No docs were updated in this diff (no changes underdocs/), leaving existing schema descriptions stale. Please update the relevant docs (e.g.,docs/architecture/evaluation-loop-fire-log.mdand any per-stream inventory) to describe the new field and the sweep outputs, or adjust the spec if deferring docs. - [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:152 — Resolver join scans oracle O(N×M) per group — consider session-index + windowed search
borrowDigestlinearly scans the entireoraclearray for each calibration entry in a group:
for (const o of oracle) {
if (o.sessionId !== entry.sessionId) continue;
const delta = Math.abs(o.atMs - entry.atMs);
if (delta <= toleranceMs && delta < bestDelta) { … }
}On large logs this is quadratic. Since joins are session-scoped and time-bounded, index oracle by sessionId (e.g., Map<string, OracleEntry[]> with timestamps sorted) and binary-search within toleranceMs. Behavior remains identical and the script scales better.
- [NON-BLOCKING] src/domain/calibration/calibration-sweep.ts:2196 — String key duplication for
judged_text_hashrisks drift across layers
const JUDGED_TEXT_HASH_KEY = "judged_text_hash";duplicates the hook-layer field name locally (mirroring the existingCAPTURE_SCHEMA_KEYpattern). While noted in comments, duplication risks future drift if the writer renames the field. If feasible, centralize these keys in a tiny shared constants module or add a parsing round-trip test using a real writer line to pin the contract. - [NON-BLOCKING] .minsky/hooks/ask-routing-deferral-detector.ts:2010 — Only this detector adopts
captureFields— others remain ungroupable until updated
This PR updates the two visible write sites here to spreadcaptureFields(assistantText). If any other calibration-record writes exist in this detector (or in sibling detectors still usingextractMatchContext), they will continue stamping onlycaptureSchemaand remain ungroupable. Consider a canary (test or lint) that fails when a calibration record is written withcaptureSchemabut withoutjudged_text_hashto prevent regressions during broader adoption.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Every calibration record carries an identifier sufficient to answer "is this the same fire as that one?" — at minimum a hash of the judged text, and where the writer has it, a turn anchor. | Not Met | This PR introduces captureFields(judgedText) and adopts it at two write sites in ask-routing-deferral (.minsky/.claude/hooks/ask-routing-deferral-detector.ts:2010–2016, 2162–2166), but it does not update other writers or the shared logCalibrationRecord path. Most detectors remain unchanged and do not stamp an identifier. Therefore “Every calibration record” is not satisfied — only the updated detector (plus the pre-existing retrospective-trigger writer) carries judged_text_hash. |
| The sweep can report distinct-fire counts alongside raw record counts, so a reviewer sees both without hand-grouping by context string. | Met | src/domain/calibration/calibration-sweep.ts:1096–1115, 1825–1982, 2194–2291 adds distinctFiresSinceLastReview and ungroupableSinceLastReview to CalibrationLogResult and computes them via distinctFireFields(); CLI projection added at src/adapters/shared/commands/calibration.ts:1088–1100. Tests in src/domain/calibration/calibration-sweep.distinct-fire.test.ts assert behavior. |
| The four duplicate groups above are re-examined once the identifier exists, and the answer (repeat authoring vs re-scan) is recorded — if it is re-scan, that is a detector defect and gets its own task rather than being fixed incidentally here. | Not Met | New script scripts/resolve-calibration-duplicate-groups.ts implements the cross-stream join and the PR body shows a live run for ask-routing-deferral, but the spec-named four historical groups (e.g., operator-deferral’s five-record cluster) were not re-examined — the PR states the corpus aged out and no evaluation-stream digest exists for that detector. No in-repo artifact records results for those four groups. |
Existing records without the identifier are treated as un-groupable rather than assumed distinct; a rate computed over a mixed population states which half it covers, per the captureSchema precedent in mt#3607. |
Met | countDistinctFires() returns {distinct, ungroupable} with digest-absent records counted as ungroupable and excluded from distinct (src/domain/calibration/calibration-sweep.ts:2224–2291). Tests src/domain/calibration/calibration-sweep.distinct-fire.test.ts assert distinct: 0, ungroupable: 3 for digest-less records and mixed-window behavior. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| distinctFireFields | function | .minsky/hooks/calibration-review-cadence-detector.test.ts:99 — spreads into fixture results, src/domain/calibration/calibration-sweep.review-due.test.ts:77 — spreads into fixture results, src/domain/calibration/calibration-sweep.ts:1845 — used in computeLogResult, src/adapters/shared/commands/calibration.ts:1090 — projected into CLI JSON output | Adopted | New export added by this PR and used within tests and CLI projection; no external wiring required by spec. |
Documentation impact
- blocking-needs-update — This PR changes the persisted calibration-record schema by adding
judged_text_hash(via.minsky/hooks/judged-input-capture.ts) and extends sweep outputs withdistinctFiresSinceLastReviewandungroupableSinceLastReview(src/domain/calibration/calibration-sweep.ts, CLI projection at src/adapters/shared/commands/calibration.ts). The task scope explicitly includes docs describing the record shape. No docs underdocs/were modified in the diff, leaving existing schema descriptions stale for adopters and output fields absent. Update the docs that describe calibration record fields and sweep outputs (e.g.,docs/architecture/evaluation-loop-fire-log.md, any guard stream inventory) to include these additions.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
R2 verifies the post-review commit adding identity stamping and distinct/ungroupable reporting. The sweep changes are correct and well-tested; the ask-routing-deferral writer now stamps both captureSchema and judged_text_hash, and optionally a turn_key. However, two Success Criteria remain unmet: (1) SC1 says every calibration record should carry an identifier; this PR adopts it only for one detector and does not update the shared writer or other producers. (2) SC3 asks to re-examine the four named duplicate groups and record the verdict; the PR explains those records aged out or lack an oracle but does not amend the spec or record equivalent evidence for those specific groups. Request: either amend the task spec to reflect staged adoption and the aged-out corpus, or extend adoption/record SC3 evidence. No new critical defects were introduced; residual notes are non-blocking.
Findings
- [BLOCKING] .minsky/hooks/ask-routing-deferral-detector.ts:2010 — SC1 not fully satisfied — identifier stamping is adopted only for this detector; other calibration producers are not updated.
Per the task spec, every calibration record should carry an identity (hash; and turn anchor where available). This PR addscaptureFields(assistantText)and optionalturn_keytoask-routing-deferral(around lines 2013–2028 and the sibling site ~2179), but it does not update the shared writer (logCalibrationRecord) or other detectors. As a result, records from other logs will continue to lackjudged_text_hash. Suggested fix: either (a) broaden adoption in this PR by stampingcaptureFieldsfrom the shared write path or updating additional producers, or (b) amend the task spec to state staged adoption explicitly and defer remaining writers to a follow-up, referencing the planned task id(s). - [BLOCKING] src/domain/calibration/calibration-sweep.ts:2211 — SC3 unmet — the four named duplicate groups were not re-examined/recorded; spec needs amendment or in-repo evidence for those groups.
The spec’s criterion requires re-examining the four specific duplicate groups and recording whether they were repeat authoring vs re-scan. The PR body explains those records aged out or lacked an oracle and substitutes a live run on a different group, but the repo does not record SC3 satisfaction for the four named groups nor amend the spec to reflect the changed verification plan. Suggested fix: either (a) add repository evidence (e.g., a script output artifact or test) recording the re-examination of the four original groups if retrievable, or (b) update the task spec/acceptance tests in-repo to note the aged-out corpus and the accepted substitution, so the normative criteria match the implemented verification route. - [NON-BLOCKING] .claude/hooks/ask-routing-deferral-detector.ts:2013 — Turn anchor
turn_keyis written opportunistically; sweep still groups by digest — document consumer guidance if any reader should prefer turn-based grouping.
The commit adds aturn_keywhenctx.recordedAnchor?.turnKeyexists (lines 2019-2027) and explains in comments why grouping still uses the digest. This is fine; consider adding a brief note in the sweep docs or code comments indicating how a consumer that wants turn-identity can useturn_key(e.g., via a join) to avoid misinterpreting digest-only grouping. Not blocking; clarity only. - [NON-BLOCKING] .minsky/hooks/ask-routing-deferral-detector.test.ts:2099 — New tests assert digest and optional
turn_key; consider also pinning the field name viaJUDGED_TEXT_HASH_FIELDin the write-site comment or a constant export.
The tests importJUDGED_TEXT_HASH_FIELDand assert presence. This is good. As a future-proofing measure, ensure any other consumers also import the constant rather than hardcoding the key. Not blocking. - [NON-BLOCKING] .minsky/hooks/ask-routing-deferral-detector.ts:2179 — Both record sites switched to
captureFields; verify other writers adopt in follow-ups.
This PR updatesask-routing-deferralonly. The spec acknowledges broader adoption remains. Track follow-up work (mt#4001 or a dedicated task) to adoptcaptureFieldsor equivalent in remaining detectors. Not blocking here. - [PRE-EXISTING] src/domain/calibration/calibration-sweep.ts:2149 —
hasCaptureMarkerandreadJudgedTextHashread passthrough-level only; ensure per-kind lifting remains covered by tests if added later.
Both helpers intentionally read onlydetectorFieldsbased on measured current producers (docblocks around 2149 and 2171-2210). This is correct today; if a future per-kind branch hoists either field to the top level, the read will miss it until tests are updated. Marking as pre-existing design note; not introduced by this PR.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Every calibration record carries an identifier sufficient to answer "is this the same fire as that one?" — at minimum a hash of the judged text, and where the writer has it, a turn anchor. | Not Met | The identifier mechanism is implemented (JUDGED_TEXT_HASH_FIELD and captureFields() in .minsky/hooks/judged-input-capture.ts:146-210), and it is ADOPTED in ask-routing-deferral at both write sites via ...captureFields(assistantText) (.minsky/hooks/ask-routing-deferral-detector.ts:2007-2029, 2169-2176 and the compiled .claude/ twin). However, the shared writer logCalibrationRecord was not updated to stamp the identifier for all detectors, and no other writers were switched in this commit. Therefore, not every calibration record across the registry will carry the identifier after this PR. Follow-up adoption is required (the PR body also calls this out as deferred). |
| The sweep can report distinct-fire counts alongside raw record counts, so a reviewer sees both without hand-grouping by context string. | Met | countDistinctFires() and distinctFireFields() were added and used to derive distinctFiresSinceLastReview and ungroupableSinceLastReview (see src/domain/calibration/calibration-sweep.ts:2211-2266). The implementation explicitly treats digest-less records as ungroupable, not distinct, and exposes both figures. |
| The four duplicate groups above are re-examined once the identifier exists, and the answer (repeat authoring vs re-scan) is recorded — if it is re-scan, that is a detector defect and gets its own task rather than being fixed incidentally here. | Not Met | This commit set adds the cross-stream joining script (earlier in the PR) and the mechanism, but the current diff contains no updates that record re-examination of the four enumerated historical groups (and the PR body states those specific records have aged out or lack the oracle). Per the PR text, substitution evidence was run on a different, live group, but the success criterion as written targets the four named groups. Either amend the spec to reflect the reality (aged-out corpus and alternative evidence path) or add repository evidence logs/notes for those groups if available. |
Existing records without the identifier are treated as un-groupable rather than assumed distinct; a rate computed over a mixed population states which half it covers, per the captureSchema precedent in mt#3607. |
Met | countDistinctFires() increments ungroupable for records where readJudgedTextHash(record) returns undefined and only counts distinct digests in the distinct set (src/domain/calibration/calibration-sweep.ts:2211-2241). distinctFireFields() projects these as distinctFiresSinceLastReview and ungroupableSinceLastReview (:2250-2266). |
Documentation impact
- no-update-needed — This iteration adds a digest and optional turn key at the write site for one detector and extends the sweep’s computed outputs with distinct/ungroupable counts. No user-facing CLI/flags/routes changed, and there are no docs in the repo that promise a different calibration-sweep output shape beyond code-level contracts. The PR body states docs updates were out of scope; internal code comments and tests document behavior. I did not survey external docs beyond this module.
…ent the new fields R1 BLOCKING (calibration-sweep.ts:2224) — the grouping key was the digest alone, so the identical sentence in two different conversations collapsed to one distinct fire. That is an UNDER-count, the exact mirror of the over-count this function exists to remove, and this corpus is full of sentences an agent emits every session. Now keyed on `(session_id, digest)`, the same shape `resolve-calibration-duplicate-groups.ts` already used — which is why the script did not have the defect. Two tests: two sessions -> 2 distinct, plus the control (one session -> still 1), without which the fix could be "never group anything". R1 BLOCKING (docs) — `guard-calibration-stream-inventory.md`'s ask-routing-deferral row now names `captureSchema` / `judged_text_hash` / `turn_key`, and the `/calibration-review` skill documents the two new sweep outputs as a RANGE to be read as a pair, with the resolver script for historical groups. Source edited and `bun run src/cli.ts compile` run; generated skill verified to carry it. R1 non-blocking (resolver O(NxM)) — the oracle is indexed by session once and the per-session buckets are time-sorted so the windowed scan breaks early. Re-ran on the live corpus: 5 re-scan / 1 distinct / 0 unresolvable, unchanged. R1 non-blocking (key duplication) — the sweep still duplicates the field name rather than importing across the domain/hook boundary, matching CAPTURE_SCHEMA_KEY. A test now asserts the two constants agree, which is what the duplication lacked. R1 non-blocking (only one detector adopts captureFields) — accurate; recorded in mt#3866's Findings under "What is NOT done" and routed to mt#4001's campaign. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verification focused on the fixes since R3. The prior BLOCKING on grouping by digest alone is addressed: countDistinctFires now keys on (session_id, digest) and tests assert the session scope. Skill docs and the stream inventory doc were updated accordingly. The sweep now reports distinctFiresSinceLastReview and ungroupableSinceLastReview alongside the raw count, with tests validating behavior and record-level field reads.
However, Success Criterion 1 remains unmet globally: while captureFields() and stamping are implemented and adopted in ask-routing-deferral (and dispatcher path adds turn_key), other calibration writers still do not stamp judged_text_hash. The PR body acknowledges broader adoption as deferred; the spec still says "Every calibration record...", so this is a scope/contract gap. Blocking until either the stamp is applied via the shared writer or the spec is narrowed and follow-ups filed. Non-blocking nit: the resolver script’s --tolerance-ms parsing can yield NaN; consider guarding.
Overall, the distinct-fire reporting and documentation are solid. My review covered the changed files and cross-checked the hook and sweep integration; I did not re-audit unrelated detectors for adoption beyond what this diff shows.
Findings
- [BLOCKING] .minsky/hooks/judged-input-capture.ts:1 — SC1 not fully delivered — identifier stamped only for ask-routing-deferral; other calibration writers remain unstamped
The task spec’s first Success Criterion requires: “Every calibration record carries an identifier … at minimum a hash of the judged text, and where the writer has it, a turn anchor.” This PR implements the identity coupling viacaptureFields(judgedText)in.minsky/hooks/judged-input-capture.tsand adopts it at both write sites ofask-routing-deferral-detector.ts(dispatcher and CLI), withturn_keyon the dispatcher path. However, no other calibration writers were updated, and the shared writerlogCalibrationRecordwas not extended to stamp all streams. The PR body explicitly notes “Adoption beyond ask-routing-deferral” is deferred. As written, the corpus still contains many writers producing new records withoutjudged_text_hash, so SC1 is not met globally. Remedy: either (a) move the stamp into the shared write path so all calibration records receive the digest (and addturn_keywhere available), or (b) update the task spec to narrow the criterion and file follow-up tasks for the remaining writers, then reflect the reduced scope here. - [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:44 — CLI tolerance parsing can yield NaN when flag is provided without a value
Inmain(),const toleranceIdx = process.argv.indexOf("--tolerance-ms"); const toleranceMs = toleranceIdx > 0 ? Number(process.argv[toleranceIdx + 1]) : DEFAULT_TOLERANCE_MS;. If--tolerance-msis present but missing a following number,Number(undefined)becomesNaN, which then propagates into comparisons inborrowDigest(all comparisons withNaNare false), effectively disabling joins without a clear error. Suggest guarding withNumber.isFinite(...)and falling back to the default or exiting with a usage error.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Every calibration record carries an identifier sufficient to answer "is this the same fire as that one?" — at minimum a hash of the judged text, and where the writer has it, a turn anchor. | Not Met | The hash + coupling helper captureFields(judgedText) is implemented in .minsky/hooks/judged-input-capture.ts and ADOPTED at .minsky/hooks/ask-routing-deferral-detector.ts (both write paths stamp judged_text_hash, and the dispatcher path also stamps turn_key). However, the change does not update the shared writer logCalibrationRecord nor adopt other detectors — only ask-routing-deferral now stamps the identifier (plus retrospective-trigger pre-existing). The spec’s criterion is global (“Every calibration record…”). Follow-up adoption work remains. Evidence: .minsky/hooks/judged-input-capture.ts: JUDGED_TEXT_HASH_FIELD and captureFields; .minsky/hooks/ask-routing-deferral-detector.ts: appendCalibrationRecord(...) and ...captureFields(assistantText). The PR body’s “What is NOT done — Adoption beyond ask-routing-deferral” also acknowledges the deferral. Please either widen the stamp in the shared writer so all calibration records carry it, or narrow the criterion in the task spec and file follow-ups for remaining writers (mt#4001 campaign). |
| The sweep can report distinct-fire counts alongside raw record counts, so a reviewer sees both without hand-grouping by context string. | Met | Implemented in src/domain/calibration/calibration-sweep.ts: new fields distinctFiresSinceLastReview and ungroupableSinceLastReview on CalibrationLogResult (lines ~1096-1119) and computed in computeLogResult via distinctFireFields(newRecords) (lines ~1845-1982, 1954..1982). Grouping is session-scoped (keys.add(${readSessionId(record)}::${digest})), fixing the R1 finding; tests in src/domain/calibration/calibration-sweep.distinct-fire.test.ts assert both the raw-vs-distinct behavior and session scoping. |
| The four duplicate groups above are re-examined once the identifier exists, and the answer (repeat authoring vs re-scan) is recorded — if it is re-scan, that is a detector defect and gets its own task rather than being fixed incidentally here. | Unverifiable | This is a live-corpus action, not a repo artifact. The PR adds scripts/resolve-calibration-duplicate-groups.ts to perform the cross-stream join and includes CLI run output in the PR body for ask-routing-deferral, but the specific four cited groups (two in operator-deferral, one in ask-routing-deferral, and one in silent-stretch) are not present in-repo and cannot be re-run from this diff. The PR body also states the operator-deferral window’s five records are no longer in the current log and that its evaluation stream carries no digest. Without the historical logs or a live run in the diff, this criterion cannot be verified here. |
Existing records without the identifier are treated as un-groupable rather than assumed distinct; a rate computed over a mixed population states which half it covers, per the captureSchema precedent in mt#3607. |
Met |
countDistinctFires in src/domain/calibration/calibration-sweep.ts increments ungroupable for records missing a digest and does NOT count them as distinct; distinctFireFields maps these to ungroupableSinceLastReview. Tests AT4 in src/domain/calibration/calibration-sweep.distinct-fire.test.ts verify digest-less records produce distinct: 0, ungroupable: N, and a mixed window reports both columns to bound the range. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| distinctFireFields | function | src/domain/calibration/calibration-sweep.distinct-fire.test.ts: uses distinctFireFields in tests, src/domain/calibration/calibration-sweep.ts: computeLogResult() consumes its return to populate result fields | Adopted | Internal helper exported within the domain module and used immediately in computeLogResult and tests; no external wiring required. |
Documentation impact
- updated-in-pr — Docs updated: SKILL guides for calibration-review were amended to describe
distinctFiresSinceLastReviewandungroupableSinceLastReviewand how to interpret them (.claude/skills/calibration-review/SKILL.md, .minsky/skills/calibration-review/SKILL.md). The architecture inventorydocs/architecture/guard-calibration-stream-inventory.mdwas also updated to document the new stamped fields (captureSchema+judged_text_hash, andturn_keyon the dispatcher path) for ask-routing-deferral. These align with the implemented behavior incalibration-sweep.tsand the hook writer.
Affected: .claude/skills/calibration-review/SKILL.md, .minsky/skills/calibration-review/SKILL.md, docs/architecture/guard-calibration-stream-inventory.md
…ture writer R2 BLOCKING (SC1 not fully delivered) — right, and the measured population is smaller than "every stream". Nine hook writers stamp `captureSchema`; six of them already carry their own digest. The set that claimed capture with NO identity anywhere in the record was exactly three: ask-routing-deferral (done in the prior commit), plus operator-deferral and pre-narration, both adopted here. pre-narration needed a seam. `buildPreNarrationRecord(sessionId, detection)` had no judged text, and threading a third parameter would have touched nine call sites. `PreNarrationDetection` now carries `judgedText` instead — it is genuinely part of what was detected, since the detector derives it internally and every match's offsets address it. Zero call-site changes. New `capture-identity-coupling.test.ts` makes the invariant checkable rather than a claim in a spec: no hook stamps the marker without an identity. Its first cut asserted the stricter "nobody hand-rolls the marker" and FAILED — correctly. Six writers hand-roll it AND carry their own digest, and routing those through `captureFields` would add a second digest beside the one they write. The defect was never the hand-rolled marker; it was a marker with no identity. Corrected, with the reasoning kept in the file. Three vacuity guards on that test, because a source scan is exactly the shape that passes forever once its pattern stops matching: the pattern is asserted to match a known-bad line and not a known-good one, the file list is asserted non-trivial, and the claim-capture population is asserted non-empty. R2 non-blocking — `--tolerance-ms` with no value made `Number(undefined)` NaN, and every `delta <= NaN` is false, so the join would have found nothing and printed a clean "unresolvable" indistinguishable from a real coverage gap. Now exits 1 with the offending value named. Both branches exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The R2 fix adopts the judged-text hash coupling (captureFields) in both operator-deferral and pre-narration writers and adds a guardrail test ensuring no writer stamps captureSchema without also providing an identity. I verified the modified writers (.claude/.minsky) and the new cross-stream resolution script; no regressions or critical issues surfaced. Minor non-blocking notes: a brittle lower-bound assertion in the coupling test and silent JSONL parse skips in the script. Documentation impact is nil. Prior BLOCKING concerns are addressed; I find no new critical defects. Approving.
Findings
- [NON-BLOCKING] .minsky/hooks/capture-identity-coupling.test.ts:88 — Test hard-codes a minimum claimant count (>=9) that may drift as writers change
The assertion at.minsky/hooks/capture-identity-coupling.test.ts:88-96enforcesexpect(claimants.length).toBeGreaterThanOrEqual(9). This minimum couples the test to a moving target (number of writers stamping capture). A legitimate future reduction (e.g., consolidation or retirements) would fail the test without breaking the invariant the test aims to guard. Consider asserting> 0to keep the test non-vacuous, or compute the expected set from a declared source of truth (e.g., guard registry) if a stronger bound is desired. - [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:188 —
readJsonlsilently skips malformed lines; consider surfacing a count
readJsonlcatches JSON.parse errors and returns[]per bad line, effectively dropping malformed records without any signal. While acceptable for an exploratory script, adding a simple counter/report of skipped lines would aid diagnosis when logs contain corruption. Not blocking, but improves observability.
Documentation impact
- no-update-needed — The changes introduce an internal coupling (
captureFieldsstampingcaptureSchema+judged_text_hash) and a review-time script; public CLI, APIs, or documented behavior indocs/architecture/evaluation-loop-fire-log.mdremain accurate. No docs underdocs/architecture/describe per-detector record fields at the granularity changed here beyond existing capture discussion, and no user-facing commands/flags changed.
…e silent-zero shapes [no-deploy-impact] R3 (capture-identity-coupling.test.ts:88) — the vacuity guard asserted `>= 9`, which is a CENSUS, and a census in an assertion goes stale the first time a writer is added or retired, failing for a reason unrelated to the invariant. Floor is now the three writers mt#3866 adopted — it cannot drop without the adoption being reverted — plus a new test naming those three explicitly, since a count cannot say WHICH writers matter. R3 (resolve-calibration-duplicate-groups.ts:188) — `readJsonl` swallowed unparseable lines, which is the same shape the script exists to make visible: a torn final line (these logs are append-only from several processes) would shrink the corpus and the run would print a confident smaller answer with nothing marking the loss. Now counted and surfaced on stderr, naming the denominator the results are actually over. Still non-fatal — one bad line should not make the other 1,555 unreadable. Re-ran the resolver on the live corpus: 5 re-scan / 1 distinct / 0 unresolvable, unchanged, and the new warning stays silent on a clean corpus. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YT3CvxJhrcGeVCv8P3DGPD
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-review focused on the R3 follow-up changes. The prior BLOCKING concerns were addressed: identity is coupled via captureFields, the sweep exposes distinct vs ungroupable counts, and a regression test now enforces that capture claims aren’t stamped without an identity. The new resolve-calibration-duplicate-groups script is careful about missing logs and malformed lines and reads as a safe investigative tool. I found only minor, non-blocking nits: a repo-size-dependent vacuity guard in the new coupling test, a note on potential borrowDigest efficiency at scale, and a grouping normalization suggestion in the script. None are correctness blockers. Overall, changes are coherent and align with the task spec — event is APPROVE.
Findings
- [NON-BLOCKING] .minsky/hooks/capture-identity-coupling.test.ts:122 — Repo-size–dependent vacuity guard can produce spurious failures
The test assertshookSources().length > 50as a guard against an empty scan. This is a census-shaped threshold tied to the current repo size and can fail spuriously if the hook tree shrinks or is restructured, even while the invariant remains valid. Consider relaxing this to a structural check (e.g.,> 0combined with the named three-writer assertions below) or asserting that the directory exists and that at least the three adopted writers are present — which you already do in the next test. Keeping both guards means the stronger one (the> 50check) will trip first for unrelated reasons. - [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:189 — Borrow-window scan remains O(bucket) despite session index; acceptable now, but note growth path
indexBySessionbuilds a per-session index, butborrowDigeststill linearly scans from the start of the (sorted) bucket and breaks only once entries exceed the window. With today’s corpus sizes this is fine, but at scale you might want to advance a cursor or binary-search the lower bound per record to avoid repeatedly traversing the same early segment. This is advisory — the current complexity is acceptable for the stated sizes and the function remains correct. - [NON-BLOCKING] scripts/resolve-calibration-duplicate-groups.ts:143 — Context concatenation may over-group multi-match records
loadCalibrationderives a grouping key by joining allmatches[].contextvalues with" || ". Ordering differences or extra matches in otherwise-identical fires could cause two records from distinct turns to be treated as different groups (or vice versa if sets align by happenstance). This is acceptable given the script’s investigative intent, but consider normalizing (e.g., sorting unique contexts) or selecting a single canonical context (first match) to stabilize grouping.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| Every calibration record carries an identifier sufficient to answer "is this the same fire as that one?" — at minimum a hash of the judged text, and where the writer has it, a turn anchor. | Met | Coupled identity stamp implemented via .minsky/hooks/judged-input-capture.ts:199-244 — captureFields(judgedText) writes captureSchema and judged_text_hash together. Enforced by .minsky/hooks/capture-identity-coupling.test.ts which scans all hook writers to ensure any capture claim is paired with an identity. Adoption applied at known offenders per prior commits; the current diff adds the invariant test guarding regressions. |
| The sweep can report distinct-fire counts alongside raw record counts, so a reviewer sees both without hand-grouping by context string. | Met | src/domain/calibration/calibration-sweep.ts (module already reviewed in prior rounds) implements distinct fire handling and, combined with the identity field reader in .minsky/hooks/judged-input-capture.ts:246-255 (getJudgedTextHash), supports separate distinct vs raw counts. Tests referenced in PR body validate: calibration-sweep.distinct-fire.test.ts (outside current diff) and the code paths remain intact in the current head. |
| The four duplicate groups above are re-examined once the identifier exists, and the answer (repeat authoring vs re-scan) is recorded — if it is re-scan, that is a detector defect and gets its own task rather than being fixed incidentally here. | Met | The re-examination mechanism ships as scripts/resolve-calibration-duplicate-groups.ts (added in this PR). It joins calibration records with an oracle evaluation stream that hashes judged input, reporting RE-SCAN vs DISTINCT and surfacing unresolvable cases with explicit coverage notes. See file content at lines 1-307; CLI exits non-zero when required logs are missing to avoid silent zero. |
Existing records without the identifier are treated as un-groupable rather than assumed distinct; a rate computed over a mixed population states which half it covers, per the captureSchema precedent in mt#3607. |
Met | Reader returns undefined for missing digests (.minsky/hooks/judged-input-capture.ts:246-255), and the sweep logic (prior-reviewed in src/domain/calibration/calibration-sweep.ts) treats such records as ungroupable rather than distinct. The PR body’s tests for mixed windows (distinct vs ungroupable) passed; current code paths remain and are exercised by the existing suite. |
Documentation impact
- no-update-needed — This PR adds an internal identity field to calibration records and a diagnostic script. No public CLI flags or external APIs changed, and no docs in
docs/were modified by the diff. The record-shape changes are internal to calibration logs and are guarded by tests; existing documentation about the evaluation loop remains accurate for the described surfaces.
Summary
A calibration record could say what text was judged and not whether it was the SAME fire as the
record beside it. So four byte-identical records were indistinguishable from four genuine emissions
of one sentence, and every per-event rate over these logs was uncomputable rather than merely
imprecise.
Three parts: give records an identity, teach the sweep to report it honestly, and recover the
identity for records written before any of this.
The finding that shaped the fix
The hash primitive already existed (
hashJudgedText, mt#3607). The defect was thatcaptureSchemamarks context capture, not identity — and nothing said so. This module has twocapture helpers:
extractMatchContextreturns a bounded STRING,captureArtifactreturns{excerpt, hash}. A writer taking the first route gets re-readability with no identity, and themarker reads the same either way.
Measured: all 58 records in the live
ask-routing-deferralwindow carriedcaptureSchema, and thecomplete top-level key set was
timestamp, session_id, injection_enabled, captureSchema, matches, suppressionReasons. No digest anywhere.So the fix is a COUPLING, not a new field.
captureFields(judgedText)stamps both; there is nolonger a way to spell one without the other.
Key changes
.minsky/hooks/judged-input-capture.ts—captureFields,getJudgedTextHash,JUDGED_TEXT_HASH_FIELD.ask-routing-deferral,operator-deferral,pre-narration. Nine hook writers stamp the marker; six already carriedtheir own digest, so three was the complete defect population.
src/domain/calibration/calibration-sweep.ts—distinctFiresSinceLastReviewandungroupableSinceLastReviewbeside the raw count, keyed on(session_id, digest). Projected intothe JSON path an agent reads — mt#5011's defect, not repeated.
scripts/resolve-calibration-duplicate-groups.ts— the cross-stream join, generalized fromthe one-off in mt#3866's
## Evidence 2026-08-16..minsky/hooks/capture-identity-coupling.test.ts— makes "no writer claims capture without anidentity" checkable instead of a claim in a spec.
Judgment calls
A record with no digest goes in NEITHER count. It is not evidence of a distinct fire and not
evidence of a duplicate, so the sweep reports
distinct: 2, ungroupable: 2and a reader knows thetrue count is in
[2, 4]with only the first measured.SC1's "every calibration record" is met on the population the defect exists in, not on every
registered stream. The ~40 writers that stamp no marker claim no capture, so there is no false
claim to fix; that is mt#4001's adoption campaign, and mt#3866's
## SC1 reconciliationrecords thecensus rather than narrowing the criterion silently.
AT3 is not runnable, and it contradicts AT4. Its five records no longer exist (
grep -c→ 0over the live 37-record log; no frozen repo copy) and
operator-deferral-evaluations.jsonlcarriesa digest on 0 of 1556 records. AT3 also asks for pre-change records to be grouped while AT4
requires them un-groupable; AT4 is the principled one. AT3's intent is discharged on a live group.
One correction to my own planning claim, recorded in the spec rather than edited away: the
Diagnosis said three detectors already write
judged_text_hash. Only one does — the grep behind itwas an OR over
judged_text_hash|judgedInputand I read its file list as if every hit were thefirst term.
Testing
Typecheck: pass, 0 errors across 8 projects (session workspace). Lint: pass, 0 errors / 0 warnings
over 4398 files (session workspace).
Execution evidence:
AT1 (same text → same identifier, different text → different): covered, plus a test pinning that
the digest IS
hashJudgedText's value rather than a second convention, plus the degenerateempty-text case so a writer cannot opt out by passing nothing.
AT2 (raw and distinct as separate figures): four records of one message report 4 raw / 1
distinct; the control — four records of four messages — reports 4 / 4. A third test pins that
the digest is read from
detectorFields, the level this kind's parse actually puts it at. Two morepin the session scoping: the same sentence in two sessions is 2 distinct, and the control (one
session) is still 1.
AT4 (pre-change records un-groupable, not distinct): three digest-less records report
distinct: 0, ungroupable: 3— the guarded failure isdistinct: 3.AT3: not runnable; see Judgment calls. Its intent is discharged by the live run above, whose
DISTINCT verdict is the control proving the join discriminates.
Negative control — the passthrough-only read.
readJudgedTextHashoriginally read both recordlevels defensively. Parsing a
retrospective-triggerline — the one detector that has written thisfield since mt#3821 — returned it under
detectorFields, so no per-kind branch lifts it and thetop-level read was dead code. Removed, with the measurement in the docblock.
Negative control — the coupling test's own first cut FAILED, correctly. It asserted the stricter
"nobody hand-rolls the marker" and found six offenders that hand-roll it AND carry their own digest.
The invariant was wrong, not the code; corrected to "no marker without an identity", with the
reasoning kept in the file. Three vacuity guards accompany it, because a source scan is exactly the
shape that passes forever once its pattern stops matching.
Deploy verification
Four changed files are deploy surface, verified by running the predicate over the actual changed-file
list rather than recalling a pattern:
After merge I will run
deployment_wait-for-latestforminsky-mcpwithnotBeforeset to themerge timestamp and
expectCommitShaset to the merge commit, and readbuildIdentityrather thanthe bare SUCCESS. A tool or auth flake there is a blocker to reconnect and retry, not a license to
defer.
Live verification
No external-system integration, credential, scope, endpoint or webhook is added, so §7a's
live-exercise class does not apply. The one behaviour that cannot be exercised pre-merge is the
STAMP appearing in the live log: the running hooks are the installed
.claude/hooks/tree, not thissession's copy, so the first real record carrying
judged_text_hashis written after merge. Thewrite path is unit-tested and the record shape is asserted; the live stamp is UNVERIFIED until
then, and re-running
resolve-calibration-duplicate-groups.tsafter a few days is the check —newly-written groups should need no oracle at all.
Review findings carried forward rather than taken
R3 approved with three non-blocking notes; two are recorded on mt#3866 instead of spun into a fourth
round, and the reviewer itself marks the third "acceptable now":
hookSources().length > 50) — same census-in-an-assertion class asthe
>= 9bound fixed this round, one line over. Worth the same treatment.contexts, so a multi-match record could group with a differently-ordered sibling. Over-grouping is
the conservative direction here (the verdict then resolves the group), but it is a real nuance in
the group DEFINITION and changing it means re-running the measurement.
index and the early break already bound it.