fix(mt#4793): Converge the elision filler on one source of truth - #3548
Merged
Merged
Conversation
mt#4792 fixed the manufactured-match defect in prose-elision.ts. It did not
reach the modules that carry their own copy of the filler character.
The load-bearing find: .minsky/hooks/elision.ts is not one of seven peer
copies, it is a SECOND shared module imported by 12 hooks. Fixing its filler
corrects 12 detectors at once, so it is done first and its 5 sites now consume
the shared blankSameLength rather than restating " ".repeat(m.length).
ELISION_FILL and blankSameLength are exported for that purpose — one character,
one definition, so the next filler fix cannot miss a module the way this one did.
Also converted: claim-provenance-scan (citation parentheticals),
scripts/lib/citation-scope-matcher (landed after the spec was written), and
packages/domain/src/ask/external-refs.
external-refs is the one that was NOT benign, and the spec assumed it was.
NOTION_CUE contains [\s:—–-]*, a whitespace-tolerant separator, so a space
filler let the cue span a blanked code region and bind "notion" to an id it was
never adjacent to — appending a wrong URL into an ask body on a WRITE path. The
spec's severity note said "nothing here writes"; that was false.
output-label-tokens is classified benign and left alone with its reasoning
recorded: both consumers only count parens, and a space cannot manufacture a "(".
The five detectors that collide with open PR #3412 are deliberately not touched.
Regression the hooks suite caught: turn-end-retro-scan.test.ts spelled a blanked
span out as literal spaces, pinning the filler character into a test about
anchoring. Now derived from the elision itself.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The convergence to a single non-matching, same-length filler is well executed and property-tested for the shared elision module, and rationale/classifications are clearly recorded. However, two blocking gaps remain against the task spec: (1) SC2 requires a per‑module property test for each modified consumer (claim‑provenance‑scan, external‑refs, citation‑scope‑matcher); only the shared helper is tested. This leaves the manufactured‑match guarantee unpinned at those sites, including a write path (external‑refs). (2) SC1 is not fully satisfied: five detectors are deliberately deferred due to PR #3412, so not every site in the census either consumes the shared primitive or keeps a non‑matching filler with justification in this PR. Please add per‑module tests and reconcile SC1’s scope (convert the remaining sites or amend the spec/ATs). With those addressed, the change is otherwise coherent and high‑quality.
Findings
- [BLOCKING] .minsky/hooks/claim-provenance-scan.ts:371 — SC2 property tests missing for modified modules (manufactured-match guarantee not asserted)
This PR changes multiple modules to useblankSameLength(e.g.,.minsky/hooks/claim-provenance-scan.ts:371-375,packages/domain/src/ask/external-refs.ts:125-133,scripts/lib/citation-scope-matcher.ts:134-138) but only adds property tests for the shared.minsky/hooks/elision.tsmodule (.minsky/hooks/elision.test.ts). The task spec’s SC2 requires a per-module property test asserting that a pattern that does not match the raw text does not match the residual. Absent tests for each changed module leave the guarantee unpinned at these call sites. Please add SC2-style tests for each modified module or update the spec to narrow SC2 explicitly to the shared module. - [BLOCKING] .minsky/hooks/elision.test.ts:1 — Test scope pins only the shared elision module; modified consumers lack direct tests asserting their guarantees
.minsky/hooks/elision.test.tsadds robust property tests for the shared elision helper, but the PR also changes.minsky/.claude/hooks/claim-provenance-scan.ts(citation-parenthetical elision),packages/domain/src/ask/external-refs.ts(code-region elision in a write path), andscripts/lib/citation-scope-matcher.ts(quote elision). Per SC2, each module needs a property test (pattern absent in raw must stay absent post-elision; same-length preserved). In particular,external-refs.tsis on a write path — adding a wrong URL would be a user-visible bug; it needs a targeted test forNOTION_CUEnot spanning elided code. Please add tests for these modules or adjust scope/spec accordingly.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
SC0 — .minsky/hooks/elision.ts uses a non-matching filler and the 12 consumers are enumerated; this change is sequenced FIRST. |
Met | .minsky/hooks/elision.ts:28-44 imports blankSameLength from packages/domain/src/text/prose-elision and replaces all prior space-fillers at :41-46, :48-49, :51 with blankSameLength. Mirror updated in .claude/hooks/elision.ts:31-44, 44-54. Property tests added at .minsky/hooks/elision.test.ts pin the non-matching behavior. |
| SC1 — Every site in the corrected table either consumes a shared primitive, or keeps its own pass with a NON-matching filler and a comment saying why it is separate. | Not Met | This PR updates several sites (e.g., .minsky/.claude/hooks/elision.ts, .minsky/.claude/hooks/claim-provenance-scan.ts, packages/domain/src/ask/external-refs.ts, scripts/lib/citation-scope-matcher.ts). However, the five detectors named as colliding with PR #3412 (build-claim-injection, causal-premise, code-mechanism-assertion, pre-narration, substrate-bypass) remain space-filled and unmodified in this diff by design. Per the spec, all sites should either consume or justify; here, those five are explicitly deferred. Please either update SC1 to reflect staged delivery or include those edits in this PR. |
| SC2 — For each module, a property test: a pattern that does not match the raw text does not match the residual. | Not Met | Only .minsky/hooks/elision.test.ts was added, covering the 12-consumer module. No equivalent property tests were added for claim-provenance-scan, external-refs, or citation-scope-matcher, which this PR also modifies. Add per-module property tests or update the spec to scope SC2 to the shared elision module only. |
| SC3 — The consume-vs-keep decision is recorded per module with its reason. | Met | Rationale added in headers: .minsky/.claude/hooks/elision.ts:5-22 explains the filler SoT and why regexes remain local. .minsky/.claude/hooks/output-label-tokens.ts:123-131 records classification and why it remains space-filled. packages/domain/src/ask/external-refs.ts:125-133 documents the whitespace-tolerant separator class and the need for a non-matching, same-length filler. |
SC4 — output-label-tokens.ts:128 is classified: same defect, different shape, or benign. |
Met | Classification comment added above stripLiterals in both mirrors: .minsky/hooks/output-label-tokens.ts:123-131 and .claude/hooks/output-label-tokens.ts:127-135 explain why this site is benign and remains space-filled. |
| SC5 — The docblocks describing the shared pass as "same-length whitespace" are corrected where they describe a SHARED primitive; left alone where they correctly describe a module's own still-space-filled pass. | Met | packages/domain/src/text/prose-elision.ts header extensively updated to describe a same-length, non-matching filler and to export blankSameLength (lines 1-47, 61-96). .minsky/.claude/hooks/elision.ts headers similarly updated (lines 5-22/8-21). Modules left space-filled (e.g., output-label-tokens.ts) retain appropriate wording plus a classification note. |
| SC6 — Negative control: for at least one converted module, the SC2 test observed FAILING against its un-fixed filler. | Unverifiable | The PR body claims a revert-of-filler causes 9/13 tests to fail for .minsky/hooks/elision.test.ts, but the repository does not carry a toggled variant or a script to reproduce the failing run. There is no in-repo artifact demonstrating the failing state; verification depends on out-of-band steps in the PR description. |
SC7 — external-refs.ts:126 is fixed or explicitly cleared, with its offset-preservation constraint honoured (same-length, non-space) and the NOTION_CUED_ID_RE manufacture question answered. |
Met | packages/domain/src/ask/external-refs.ts:125-133 imports blankSameLength and switches the elider to it with an in-file rationale naming the whitespace-tolerant separator. The implementation preserves same length and uses a non-matching filler via blankSameLength. |
Documentation impact
- no-update-needed — Behavioral changes are internal to elision helpers and detectors; no public API surface or CLI changes. Verified
packages/domain/package.jsonexports already allow deep import of../text/prose-elisionand this PR only exports two additional symbols (ELISION_FILL,blankSameLength) from that module. No docs indocs/were updated or appear to describe filler character specifics for these modules. If there are docs explicitly stating "same-length whitespace" for the shared primitive elsewhere, they were not included in this diff; the in-code docblocks were updated accordingly.
… just the shared one Both BLOCKING findings were the same gap and both were right: SC2 asks for a property test PER MODULE and I had written one, for elision.ts. Class-not- instance, so all three remaining converted modules get one in this round rather than the flagged one only. Each is a differential pair — a control proving the fixture can fire at all, then the negative — because a passing assertion about an absent match is otherwise indistinguishable from an inert test. - external-refs: a Notion cue separated from an id by a code span must not linkify. This is the write-path defect: NOTION_CUE's [\s:—–-]* let a space filler bind "notion" to an id it was never adjacent to. - citation-scope-matcher: a SCOPE_MARKER split by a quoted span (`\bis\s+the\b`) must not be manufactured. - claim-provenance-scan: COLLISION_VERB_RE is whitespace-tolerant (`overlaps?\s+(?:on|with)`), so blanking a `(same file …)` citation to spaces re-manufactured the very verb the elision had just removed. Negative control, all four modules at once: because they now share one filler definition, setting ELISION_FILL to a space restores the pre-fix state everywhere. elision 9 fail, external-refs 2, citation-scope-matcher 1, claim-provenance-scan 1 — every new test is live.
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 fix commit adding SC2 property tests. The prior BLOCKING gaps (missing per-module property tests) are addressed: new differential tests were added for claim-provenance, external-refs, and citation-scope-matcher that assert the non-manufacture property and demonstrate offset preservation where relevant. I found no new defects introduced by these changes; tests are self-contained and avoid patched-collaborator shapes by asserting on return values. Several other success criteria (SC0/SC1/SC3/SC4/SC5/SC6) cannot be re-verified from this diff slice, but nothing here contradicts them. No documentation impact from this commit’s test-only additions. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
SC0 — .minsky/hooks/elision.ts uses a non-matching filler, and the 12 consuming modules are enumerated with any fire-rate change noted. |
Unverifiable | This round’s diff adds tests only. No changes to .minsky/hooks/elision.ts or the consumer enumeration appear in the shown files; cannot verify from this slice of the PR. |
| SC1 — Every site in the corrected table either consumes the shared primitive, or keeps its own pass with a NON-matching filler and a comment saying why it is separate. | Unverifiable | No code changes for the listed detector modules are in this diff window; only new tests were added. Compliance cannot be re-verified here. |
| SC2 — For each module, a property test: a pattern that does not match the raw text does not match the residual. | Met | Property tests were added for three converted modules: .minsky/hooks/claim-provenance-scan.test.ts (lines ~228-258 added block asserting no manufactured collision verb); packages/domain/src/ask/external-refs.test.ts (new describe block at file end asserting no manufactured Notion cue/link); scripts/lib/citation-scope-matcher.test.ts (new describe block at file end asserting no manufactured scope marker). All encode the negative-manufacture property as required. |
| SC3 — The consume-vs-keep decision is recorded per module with its reason. | Unverifiable | Decision recording is not part of the shown test additions. Not verifiable from this diff slice. |
SC4 — output-label-tokens.ts is classified: same defect, different shape, or benign. |
Unverifiable | No changes to .minsky/hooks/output-label-tokens.ts in this diff; cannot verify classification here. |
| SC5 — Docblocks describing the shared pass as “same-length whitespace” are corrected where applicable; left alone where still accurate. | Unverifiable | No documentation or docblock edits appear in the files shown in this round; cannot verify this criterion from the current diff subset. |
| SC6 — Negative control: for at least one converted module, the SC2 test observed FAILING against its un-fixed filler. | Unverifiable | The added tests assert the property post-fix but do not include a revert/negative-control subtest in these files. The claim is mentioned in the PR description, but not verifiable from the shown code. |
SC7 — packages/domain/src/ask/external-refs.ts:126 is fixed or explicitly cleared, with same-length, non-space filler and the NOTION_CUED_ID_RE manufacture question answered. |
Met | packages/domain/src/ask/external-refs.test.ts adds property tests ensuring: (a) elideCodeRegions preserves length and uses a filler outside [\s:—–-], and (b) a cue separated by a code span does not cause linkifyExternalRefs to append a URL, while genuinely adjacent cue+id still linkify (new describe block at file end). This demonstrates both offset preservation and non-manufacture against the cue’s separator class. |
Documentation impact
- no-update-needed — This commit adds property tests only (three new describe blocks in test files) to satisfy SC2/SC7. No production behavior, public API, or user-facing surface changed in this diff slice, and no docs files were modified.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verification against the current diff shows the prior BLOCKING items are addressed: claim-provenance-scan now uses the shared blankSameLength instead of space-filling, and a comprehensive SC2 property test suite was added for the elision helpers plus each newly converted module. The shared filler is exported from prose-elision.ts and consumed consistently across .minsky, .claude, scripts, and packages/domain. The change also corrects a real write-path risk in external-refs.ts with tests. I found no new critical defects introduced by these edits; pathing/imports resolve via existing package exports and relative paths, and newline preservation is verified by tests with the hooks suite reported green. One new export (ELISION_FILL) has no direct consumers yet, which is acceptable. Overall, this converges on one SoT for the filler without scope creep. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
SC0 — .minsky/hooks/elision.ts uses a non-matching filler, and the 12 consuming modules are enumerated with any fire-rate change noted. It is a SECOND shared module, not a peer copy: one change corrects 12 detectors, so it is sequenced FIRST. |
Met | .minsky/hooks/elision.ts:28-45,64-69 — now imports blankSameLength from packages/domain/src/text/prose-elision and replaces all prior space-fillers with it. Header comment documents the SoT rationale. Enumeration and rate notes are recorded in the PR body (out-of-code artifact). |
SC1 — Every site in the CORRECTED table either consumes the shared primitive, or keeps its own pass with a NON-matching filler (neither \s nor \w) and a comment saying why it is separate. |
Met | Converted: .minsky/hooks/elision.ts (all sites), .minsky/hooks/claim-provenance-scan.ts:372-376, .claude/hooks/* mirrors, scripts/lib/citation-scope-matcher.ts:134-136, and packages/domain/src/ask/external-refs.ts:125-136 now use blankSameLength. The explicitly-kept site .minsky/hooks/output-label-tokens.ts:123-133 carries an in-file justification comment explaining why space-fill is benign at that call site. |
| SC2 — For each module, a property test: a pattern that does not match the raw text does not match the residual. | Met | Added tests: .minsky/hooks/elision.test.ts (new file, 93 lines) covers the shared elision module; .minsky/hooks/claim-provenance-scan.test.ts:674-706 adds the non-manufacture property; scripts/lib/citation-scope-matcher.test.ts:132-167 adds a differential pair; packages/domain/src/ask/external-refs.test.ts:146-187 adds SC2/SC7 property tests. |
| SC3 — The consume-vs-keep decision is recorded per module with its reason. A module kept separate names the context class its pass covers that the shared one does not. | Met | Per-module rationale appears in code comments where applicable: .minsky/hooks/output-label-tokens.ts:123-133 explains the keep decision; .minsky/hooks/elision.ts header documents filler SoT and why regexes stay local. Additional rationale and the 12-consumer list are recorded in the PR body (out-of-code artifact). |
SC4 — output-label-tokens.ts:128 is classified: same defect, different shape, or benign. |
Met | .minsky/hooks/output-label-tokens.ts:123-133 and .claude/hooks/output-label-tokens.ts:127-137 — explicit comment: both consumers only count parens; a space cannot manufacture (, so space-fill is benign here. No code change required. |
| SC5 — The ~20 docblocks describing the shared pass as "same-length whitespace" are corrected where they describe the SHARED primitive; left alone where they correctly describe a module's own still-space-filled pass, until SC1 changes it. | Met | packages/domain/src/text/prose-elision.ts:50-78 — header now describes the SAME-LENGTH, NON-MATCHING filler properties and exports ELISION_FILL/blankSameLength. .minsky/.claude/hooks/elision.ts headers updated to the same non-matching semantics. Modules intentionally left space-filled (output-label-tokens) retain their local description with justification. |
| SC6 — Negative control: for at least one converted module, the SC2 test observed FAILING against its un-fixed filler. | Met | .minsky/hooks/elision.test.ts includes assertions that would have failed under a space filler (manufactured \s+ joins) and now pass. The tests are phrased against properties (not the literal character), and the PR body documents the fail/pass observation when toggling the filler. |
SC7 — external-refs.ts:126 is fixed or explicitly cleared, with offset-preservation honoured and the NOTION_CUED_ID_RE manufacture question answered against the actual regex. |
Met | packages/domain/src/ask/external-refs.ts:123-136 now uses blankSameLength for elideCodeRegions with an in-file comment on the whitespace-tolerant NOTION_CUE separators; packages/domain/src/ask/external-refs.test.ts:146-187 adds property tests showing no manufactured Notion links and offset preservation. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| @minsky/domain/text/prose-elision.blankSameLength | function | scripts/lib/citation-scope-matcher.ts:134 — replaces quoted spans via package import, packages/domain/src/ask/external-refs.ts:125 — uses for code-region elision, /.minsky/hooks/elision.ts:28,41,64 — imports relatively and applies across contexts, /.claude/hooks/elision.ts:31,44,67 — mirror import and usage, /.minsky/hooks/claim-provenance-scan.ts:372 — uses for citation parentheticals, /.claude/hooks/claim-provenance-scan.ts:375 — mirror usage | Adopted | Export introduced in packages/domain/src/text/prose-elision.ts and consumed across hooks, scripts, and domain modules. |
| @minsky/domain/text/prose-elision.ELISION_FILL | function | — | Missing consumers | Newly exported constant; current changes adopt blankSameLength as the SoT. No direct consumers of ELISION_FILL were added in this PR; leaving it exported is reasonable for future use but has no current adopters. |
Recommendation: file a follow-up adoption task to wire 1 missing consumer.
Documentation impact
- no-update-needed — Code changes converge internal elision filler usage and add property tests. Public APIs and user-facing commands remain unchanged. In-code docblocks were updated to reflect “same-length, non-matching filler,” but no docs under docs/ reference the specific filler character or behavior at this granularity. No documentation files were modified in this PR, and no behavior exposed to end users was added/changed/removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mt#4792 fixed the manufactured-match defect in the shared
prose-elision.tsprimitive: eliding tospaces lets a caller's own
\s+run through the blanked hole, so a clause that does not matchthe raw text matches the residual. It did not reach the modules carrying their own copy of the
filler character. This converges them.
The finding that reordered the work
The spec framed this as "seven divergent copies — change seven constants." The measurement says
otherwise:
.minsky/hooks/elision.tsis not a peer copy, it is a SECOND shared module,exporting four functions and imported by 12 hooks (
retrospective-trigger-scanner,turn-end-{retro,stale-state-assertion,unescalated-incident,untaken-action}-scan,secret-request-in-chat,constructed-identifier-batch,pre-narration,warn-unwired-task-relationship,require-execution-evidence-before-merge,ask-routing-deferral,operator-deferral).So one change corrects 12 detectors, and it explains why mt#4792 missed them: this module held a
private copy of the character.
ELISION_FILLandblankSameLengthare now exported and consumed —one character, one definition.
The regexes stay local, deliberately. Which contexts each detector elides is per-detector
calibration and changing it would move fire rates; the spec puts that out of scope. This converges
the FILLER only.
external-refs was NOT benign, and the spec assumed it was
The spec's severity section said "Nothing here writes." False.
packages/domain/src/ask/external-refs.tsuses the same blank-to-spaces shape, and
linkifyExternalRefsrewrites ask body text.Planning flagged it as "probably benign — confirm it." Confirming it inverted the answer:
NOTION_CUEis— a whitespace-tolerant separator class. A space filler let the cue span a blanked code region
and bind
notionto an id it was never adjacent to, appending a wrong Notion URL into an ask body.Same-length is preserved (matches are applied to the original by index, so offsets must survive);
·is in neither\snor the separator class.Deliberately not converted
output-label-tokens.ts:135— classified benign, reasoning recorded in-file. Its only twoconsumers (
unrendered-result-fields.ts:170,178) count parens over the residual; a spacecannot manufacture a
(. The filler's only requirement here is "not a bracket."build-claim-injection,causal-premise,code-mechanism-assertion,pre-narration,substrate-bypass). Planning read feat(mt#4639): Convert 614 log sites to getLoggableErrorSummary and flip the rule to error #3412's full313-file list; those five are in it. Their conversion is mechanically identical and is left for a
follow-up so this PR does not contend with a 313-file sweep that is already
mergeable_state: dirty.Note
pre-narrationstill gets the fix for the half it imports fromelision.ts.Deploy verification
isDeploySurfaceFilereturns true for 2 of 11 changed files —packages/domain/src/ask/external-refs.tsandpackages/domain/src/text/prose-elision.ts— run overthe actual diff, which is why this PR carries no
[no-deploy-impact]tag. Post-merge I will wait onthe deployment bound to this merge (
notBefore= merge time,expectCommitSha= merge SHA) andassert the health body's service identity rather than the status code.
Testing
Execution evidence:
SC0 / SC2 / AT2 — new property tests for the 12-consumer module. They assert what the filler must
DO, not what it IS, so they survive a future filler change that keeps the guarantee:
AT4 — full hooks suite green:
AT1 / SC1 — the census grep re-run over BOTH forms (
" ".repeat(and[^\n]/g, " "; the spec'sown grep matched only the first, which is why
claim-provenance-scan.ts:374looked absent). Whatremains is exactly the justified set — the five #3412-colliding detectors plus the benign
output-label-tokens:SC3 / SC4 — consume-vs-keep recorded per module: in this PR body above, in
elision.ts's header,and in the in-file comment on
stripLiterals.SC5 —
elision.ts's header no longer claims "same-length whitespace"; it states the twoproperties and why the filler is imported rather than restated. The remaining "whitespace" docblocks
correctly describe modules whose own pass is still space-filled, which is what SC5 asks for.
SC7 — covered by the
external-refssection above; the manufacture question was answered againstthe actual regex, not assumed.
Negative control — SC6, elision.ts filler: reverting all 5 sites to
" ".repeat(m.length)(the full pre-fix state) fails 9 of 13, and restoring returns 13/13.A regression the suite caught, worth naming.
turn-end-retro-scan.test.tsspelled a blanked spanout as literal spaces in its fixture, pinning the filler character into a test about anchoring.
That is the same class as the 11 assertions mt#4792 had to rewrite in the shared module. The fixture
now slices the real residual, so it is filler-independent. A class-not-instance scan for other
space-pinned fixtures found none (the two candidates are code indentation inside multi-line
fixtures).
Generated mirror —
.minsky/hooksis the source;.claude/hooksregenerated viabun run src/cli.ts compile(Target "claude-hooks": 186 file(s) written), verified by git statusand by grepping the mirror: 6
blankSameLengthoccurrences in.claude/hooks/elision.ts, and onlythe five deferred detectors still space-filled. No unrelated regeneration drift.
Typecheck clean across all 8 projects (
infraskipped with its documented reason).Lint 0 errors / 0 warnings across 4,297 files. Format
format:checkexit 0.