fix(mt#4662): Repoint stale src/domain comment pointers from the packages/domain extraction - #3429
Conversation
…e packages/domain extraction mt#2108 moved the domain layer to packages/domain/src/. Source COMMENTS citing the old paths were never swept, so they point nowhere while the code beside them is correct — src/cockpit/sse-broker.ts is the clean illustration: its docblock @see says src/domain/mesh/postgres-channel-listener.ts while the import one line below already reads @minsky/domain/mesh/postgres-channel-listener. Not cosmetic. It is the class mt#4426 shipped a detector for: a confident pointer stops the next reader from looking, and a reader who follows one finds nothing and gets no signal that the file merely moved. The detector fired 18 times across these 13 files. The files carry 29 such occurrences in total, because the detector only records a path a CLAIM PHRASE governs — a stale path in ordinary prose is equally dead and never fires. Repointing only the 18 would have left 10 identical pointers in files already open for this defect, which is the fix-the-instance anti-pattern mem#503 records from this same extraction. So all 28 comment occurrences are swept; the fire count still drops by exactly 18 because the other 10 were never fires. One occurrence is deliberately untouched: tests/domain/memory/validation.test.ts line 69 is test DATA, a sample string exercising case-insensitive matching of the "THE FILE X" trigger, where the path is incidental filler. Rewriting a fixture to satisfy a detector inverts SC3's intent, and executable src/domain references are mt#2479 / mt#2354's subject. It is the only src/domain/ left in the 13 files. Chose per-file comment-scoped replacements over a repo-wide codemod for that reason: a blind sed would have rewritten exactly the executable references that must not move. Deploy verification: isDeploySurfaceFile was RUN over the 12 changed files and returns true for 7 — every packages/domain/src/** and src/** path. The predicate reads paths, not content, so a comments-only edit to application source is deploy surface. This commit therefore carries no `[no-deploy-impact]` claim, and §10 verification is owed post-merge.
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
Scope is tight and the edits are comment-only repoints as advertised; spot-checks of repointed targets under packages/domain/src looked sane. However, one stale pointer remains: .claude/hooks/record-subagent-invocation.ts’s header still cites @see src/mcp/subagent-dispatch-tracker.ts while neighboring @see entries were repointed to packages/domain paths. That leaves the same dead-pointer class this PR set out to remove and risks misleading readers. Please repoint (or justify) that entry. All success criteria rely on running the detector script; I cannot execute it here, so I recorded them as Unverifiable. Documentation impact: no-update-needed (comments-only).
Findings
- [BLOCKING] .claude/hooks/record-subagent-invocation.ts:19 — Stale
@see src/mcp/subagent-dispatch-tracker.tspath left untouched among repointed neighbors
The docblock near the top keeps@see src/mcp/subagent-dispatch-tracker.ts — DB write layerwhile the adjacent@seeentries for domain files were repointed topackages/domain/src/.... Given mt#2108 moved domain code and this task’s purpose is to repoint stale pointers, leaving this singlesrc/mcp/...reference creates an inconsistency and risks the same class of dead pointer the detector targets. Please repoint this to the current path (likelypackages/domain/src/mcp/subagent-dispatch-tracker.tsor confirm the correct location) or justify whysrc/mcp/remains correct here.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
1. bun scripts/measure-coverage-claim-paths.ts reports zero fires of the stale-src/domain/* class. The residual historical-incident false positive (deploy-surface-detector.ts:16) may remain; it is a known FP, not a defect. |
Unverifiable | This criterion depends on executing bun scripts/measure-coverage-claim-paths.ts before/after the sweep. Execution results are not part of the diff and cannot be verified from repo contents alone. |
| 2. Every rewritten pointer resolves — verified by the script's own existence check, not by eye. | Unverifiable | Requires running the measurement script’s existence-check over the changed files. I verified spot targets exist (e.g., packages/domain/src/mesh/postgres-channel-listener.ts; packages/domain/src/memory/validation.ts), but the criterion’s asserted verification is the script’s run, which is outside the diff. |
| 3. No pointer is deleted to silence the detector where the referenced file still exists; the fix is to repoint, not to remove. | Unverifiable | The diff shows comment-for-comment repoints (e.g., src/cockpit/sse-broker.ts: docblock @see repointed) and preserves one explicit test fixture string; however, the formal check the spec names is the detector’s measured count delta, which requires running the script — not verifiable from static diff. |
Documentation impact
- no-update-needed — This PR claims to be comments-only repointing of in-repo source comment paths. No public API, CLI, or behavior changed; only docblock references were updated. I spot-checked affected modules (e.g., sse-broker.ts, fake-* domain fakes) and found no behavior changes. No docs/ files were modified, and no user-facing semantics appear to change.
Cockpit PreviewPreview URL: https://cockpit-preview-production.up.railway.app Deployed from commit Updated by Cockpit Preview. |
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 finding (stale @see path in .claude/hooks/record-subagent-invocation.ts) has been addressed — the two domain @see entries are repointed to packages/domain/src/… while the src/mcp reference remains appropriately unchanged. I spot-checked several other repointed pointers; targets exist and no executable code changed. I found no new critical defects introduced by this comment-only sweep. Success criteria rely on running the detector script; I recorded them as Unverifiable, which does not block. Documentation impact: no-update-needed. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
1. bun scripts/measure-coverage-claim-paths.ts reports zero fires of the stale-src/domain/* class. The residual historical-incident false positive (deploy-surface-detector.ts:16) may remain; it is a known FP, not a defect. |
Unverifiable | This criterion requires executing bun scripts/measure-coverage-claim-paths.ts against the tree before and after. Execution output is not part of the diff; cannot be verified from static repo contents. |
| 2. Every rewritten pointer resolves — verified by the script's own existence check, not by eye. | Unverifiable | Verification depends on running the measurement script’s existence-check pass. I spot-checked targets referenced in the updated comments (e.g., packages/domain/src/mesh/postgres-channel-listener.ts; packages/domain/src/memory/validation.ts; packages/domain/src/configuration/sources/environment.ts) and they exist, but the criterion explicitly rests on the script’s check. |
| 3. No pointer is deleted to silence the detector where the referenced file still exists; the fix is to repoint, not to remove. | Unverifiable | The diff shows comment-for-comment repoints with no removals across the edited files, and it preserves an intentional test-fixture string. However, the task’s formal check is the detector’s measured count delta, which requires running the script — not verifiable from the diff alone. |
Documentation impact
- no-update-needed — Edits are confined to source comments/docblocks repointing internal file-path references from src/domain/* to packages/domain/src/*. No behavior, API, or CLI surface changed. No docs/ files are touched, and the updated pointers now reference existing in-repo files (spot-checked several targets).
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 (stale @see path in .claude/hooks/record-subagent-invocation.ts) is addressed — both the .claude and source .minsky hook headers now repoint the two domain references to packages/domain/src/… while correctly retaining the src/mcp reference. I swept the other touched files; all changes are to comments/docblocks only, and I verified the referenced targets exist under packages/domain/src/. No executable code was modified, and I see no new critical defects introduced by this fix. Success criteria rely on the detector script’s runtime output, which I cannot execute here, so they remain recorded as Unverifiable. Documentation impact: no-update-needed. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
1. bun scripts/measure-coverage-claim-paths.ts reports zero fires of the stale-src/domain/* class. The residual historical-incident false positive (deploy-surface-detector.ts:16) may remain; it is a known FP, not a defect. |
Unverifiable | This criterion requires executing the measurement script before/after. Execution output is not part of the diff; from static review I confirmed the repointed targets exist (e.g., src/cockpit/sse-broker.ts now points to packages/domain/src/mesh/postgres-channel-listener.ts, which exists), but the zero-fire check itself cannot be verified here. |
| 2. Every rewritten pointer resolves — verified by the script's own existence check, not by eye. | Unverifiable | The spec asks to verify via the script’s existence check. I spot-verified the referenced targets exist in the repo (e.g., packages/domain/src/persistence/fake-persistence-provider.ts, packages/domain/src/tasks/fake-task-service.ts, packages/domain/src/session/fake-session-provider.ts, packages/domain/src/configuration/sources/environment.ts, packages/domain/src/configuration/schemas/observability.ts), but I cannot run the script; marking Unverifiable per process. |
| 3. No pointer is deleted to silence the detector where the referenced file still exists; the fix is to repoint, not to remove. | Unverifiable | The diff shows comment-for-comment repoints (e.g., .claude/hooks/record-subagent-invocation.ts, .minsky/hooks/record-subagent-invocation.ts, packages/domain/src/git/fake-git-service.ts, src/cockpit/sse-broker.ts) and preserves an intentional test fixture string under tests/domain/memory/validation.test.ts. However, the formal check is the detector’s measured count delta, which requires running the script. |
Documentation impact
- no-update-needed — Comments-only sweep repointing in-source docblock/@see paths from src/domain/... to packages/domain/src/.... No behavior, API, or CLI surface changed. No docs/ files were touched by the PR, and affected code modules (hooks, domain fakes, cockpit SSE broker, scripts) retained identical runtime behavior.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verification pass: the prior BLOCKING finding about stale @see paths in record-subagent-invocation headers has been addressed — both .claude and .minsky variants now repoint domain references to packages/domain while correctly retaining the src/mcp reference. The rest of the diff is consistent comment-only repointing across the touched files; no executable code changed and I see no new defects introduced by this sweep. Success criteria rely on running the detector script and are recorded Unverifiable in this context. Documentation impact: no-update-needed. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
1. bun scripts/measure-coverage-claim-paths.ts reports zero fires of the stale-src/domain/* class. The residual historical-incident false positive (deploy-surface-detector.ts:16) may remain; it is a known FP, not a defect. |
Unverifiable | This criterion requires executing bun scripts/measure-coverage-claim-paths.ts on the repo before/after. Execution output is not part of the diff and cannot be verified here. |
| 2. Every rewritten pointer resolves — verified by the script's own existence check, not by eye. | Unverifiable | Verification hinges on the measurement script’s existence-check run. Static diff review shows all pointers repointed (e.g., packages/domain/src/git/fake-git-service.ts:5-12; src/cockpit/sse-broker.ts:14), but the scripted check cannot be executed in this review context. |
| 3. No pointer is deleted to silence the detector where the referenced file still exists; the fix is to repoint, not to remove. | Unverifiable | The diff shows comment-for-comment repoints across all touched files with no removals of pointer lines (e.g., .claude/hooks/record-subagent-invocation.ts:13-19; scripts/lib/pem-utils.ts:1-7). However, the spec’s formal proof is the script’s measured delta, which requires execution outside this review. |
Documentation impact
- no-update-needed — All changes are comment/docblock pointer updates within source files; no executable logic, public API, CLI, or runtime behavior changed. No files under docs/ were modified. I spot-checked several touched files (e.g., src/cockpit/sse-broker.ts; packages/domain/src/git/fake-git-service.ts; scripts/verify-mt1510-identity-routing.ts) and confirmed edits are confined to @see/path prose only.
1 participant
Summary
mt#2108 moved the domain layer to
packages/domain/src/. Source comments citing the old pathswere never swept, so they point nowhere — while the code beside them is correct.
src/cockpit/sse-broker.tsis the clean illustration: its docblock@seesaidsrc/domain/mesh/postgres-channel-listener.tswhile the import one line below already read@minsky/domain/mesh/postgres-channel-listener.This is the class mt#4426 shipped a detector for: a confident pointer stops the next reader from
looking, and a reader who follows one finds nothing and gets no signal that the file merely moved.
Why 28 occurrences and not 18
The detector fired 18 times across these 13 files. The files carry 29
src/domain/occurrences in total. That gap is not a discrepancy — the detector only records a path a CLAIM
PHRASE governs, so a stale path in ordinary prose is equally dead and simply never fires.
Repointing only the 18 would have left 10 identical pointers in files already open for this exact
defect — the fix-the-instance anti-pattern mem#503 records from this very extraction (mt#2108
broke 7 hook files; two successive fix-forward cycles each repaired one and swept neither). So all
28 comment occurrences are swept. AT1 is unaffected: the fire count still drops by exactly 18,
because the other 10 were never fires.
One occurrence deliberately NOT changed
tests/domain/memory/validation.test.ts:69:Test data, not a pointer — a sample string exercising case-insensitive matching of the
THE FILE Xtrigger, where the path is incidental filler. Rewriting a fixture to satisfy a detectorinverts SC3's intent, and executable
src/domainreferences are mt#2479 / mt#2354's subject. It isthe only
src/domain/left in the 13 files.What does NOT establish that I preserved it: the file's 39 tests pass either way, since the
assertion is about the trigger phrase and not the path. The evidence is the grep, below.
Approach
Per-file, comment-scoped replacements rather than a repo-wide codemod — deliberately. A blind
sed 's|src/domain/|packages/domain/src/|g'would have rewritten exactly the executable referencesthat must NOT move, which is the subject of the two open sibling tasks above.
Testing
Execution evidence:
AT1 — 22 → 4, exactly 18 fewer, and the stale-
src/domain/*class is at zero. AT3 — theremaining 4 are a strict subset of the pre-sweep set, and are precisely the out-of-scope residue the
spec names: the 3 deleted-script pointers plus the known historical false positive
(
deploy-surface-detector.ts:16citingservices/reviewer/railway.json). No unrelated fire wasintroduced.
AT2 — every rewritten pointer resolves. Checked across all 14 distinct targets, not sampled:
SC1 — the measurement above reports zero fires of the stale-
src/domain/*class; the residualrailway.jsonFP remains, which SC1 explicitly permits. SC2 — satisfied by AT2's existencecheck rather than by eye. SC3 — no pointer was deleted; all 28 were repointed. The diff is
29 insertions / 29 deletions across 12 files, i.e. line-for-line rewrites with no removals.
The single surviving occurrence is the test fixture, as intended.
Negative control — the pre-sweep measurement is the failing observation this change fixes:
Run on
mainbefore any edit and again after: 18 → 0 for the class. The probe demonstrablydistinguishes the fixed and unfixed trees, which is the property a control exists to establish.
Suite results:
Typecheck: 0 errors across 8 projects (
infra/skipped — deps not installed locally; CI covers it).Lint: 0 errors, 0 warnings over 4,152 files. Both run against the session workspace
(
validatedWorkspaceconfirmed). Format: clean.Deploy verification
This IS deploy surface, despite being comments-only.
isDeploySurfaceFilewas RUN over the 12changed files and returns true for 7:
The predicate reads PATHS, not content, so a comment-only edit to application source is deploy
surface exactly as a logic change would be. This PR carries no
[no-deploy-impact]claim, andpost-merge deploy verification will be run and reported per
/implement-task§10. Recorded because"it is only comments" is precisely the intuition that would have produced a false tag here — the
commit-msg guard also caught an earlier draft of the message that merely mentioned the tag.
Parallel work
Open PR #3412 (mt#4639, the 614-site
getLoggableErrorSummaryconversion) touches 1 of the 13citing files,
src/cockpit/sse-broker.ts. Not a conflict: its hunks are at line 22 (an import) andlines ~191 / ~236 (two
log.warnbodies); this change is at line 17, inside the module docblock.Disjoint regions. The other 12 files are untouched by it. mt#2479 and mt#2354, which also concern
stale
src/domainpaths, were verified disjoint by file —packages/domain/src/ask/transports/elicitation-containment.test.tsandpackages/domain/src/persistence/architecture.test.ts, neither among the 13.Adjacent finding, filed not folded
Planning gate (p)'s ADR grep surfaced 26 stale
src/domain/*paths indocs/architecture/adr-*.md— the same drift on a surface the detector structurally cannot scan (it returns early on anything
but
.ts/.tsx/.js/.jsx). Filed as mt#4716 rather than added here, because this task'sverification method is the detector's measurement, which could not check a markdown fix. Worth
knowing when reading the zero above: it is silent about the ADR corpus.
Consumer account
No signal-producing call is removed, and no executable line changes. The diff is 29 comment-line
rewrites plus the regenerated
.claude/hooks/mirror of the one edited hook source; every changedline is inside a comment.
bun run src/cli.ts compilewas run and reportedTarget "claude-hooks": 183 file(s) written, and the pre-commit compile-output check confirmed alltargets up to date.