feat(mt#4639): Convert 614 log sites to getLoggableErrorSummary and flip the rule to error - #3412
Open
minsky-ai[bot] wants to merge 4 commits into
Open
minsky-ai[bot] wants to merge 4 commits into
minsky-ai[bot] wants to merge 4 commits into
Conversation
…lip the rule to error custom/prefer-loggable-error-summary shipped in mt#4632 registered off, because this repo runs a zero-tolerance warning gate and the rule flagged hundreds of pre-existing sites. This is the cleanup that makes error shippable. The population was larger than the spec said. 590 was measured over src/packages/services/scripts; the gate that has to pass is lint:strict, which runs eslint over the whole repo and saw 642 - the extra 49 being .minsky/hooks plus its generated .claude/hooks mirror, tests, and one site in eslint-rules itself. 614 converted, 10 carved out, 18 handled by regenerating the mirror. Conversion is done by scripts/codemod-loggable-error-summary.ts, driven by the rule's own JSON report rather than a regex over source, so the codemod's population is the rule's population by construction. Every replacement is shape-validated before it is applied and anything unmatched is skipped and reported; 0 were skipped, and the script aborts on an accounting mismatch. The CLI-facing console sub-category the rule's off-comment left open was converted rather than exempted, on a measurement: getLoggableErrorSummary returns a byte-identical string to err.message for a plain Error and to String(err) for a non-Error, differing only when the error carries a cause or the message exceeds 2000 chars. There was no terseness to protect. Two carve-outs remain, both via ESLint's documented files-scoped config object rather than a bespoke rule option: test fixtures, and eslint-rules itself. Verified: lint:strict exits 0, typecheck clean across 8 projects, 5412 tests across 399 files pass, hooks suite 6572 tests across 179 files pass.
Minsky Reviewer StatusReview skipped — concurrent_inflight |
Cockpit PreviewPreview URL: https://cockpit-preview-production.up.railway.app Deployed from commit Updated by Cockpit Preview. |
Owner
|
/review |
edobry
added a commit
that referenced
this pull request
Aug 28, 2026
…e that decays across turns
`claim-confidence.mdc` shipped a two-axis claim vocabulary in mt#2921 and **nothing has ever checked
it**. Grep the hook tree for `verified-1a` / `strong-evidence` / `inferred` and every hit is guidance
text a detector EMITS — `build-claim-injection-detector.ts:480`,
`require-deploy-verification-before-merge.ts:511`, `code-mechanism-assertion-detector.ts:2397`. No
detector MATCHES on it.
RFC `3a0937f0` (Accepted 2026-07-18) predicted exactly this and reserved the fix as its Phase 3
candidate — *"a cheap high-precision companion detector … gives the vocabulary a falsifier instead of
relying on self-assessment alone."* It was never filed. This is that falsifier, on the
**labeled-uncertain-then-asserted** axis; the RFC's own *labeled-verified-unbacked* axis is now filed
as mt#4704.
## The premise this task was filed on was false, and was corrected at planning
The spec claimed every detector in the family evaluates one turn in isolation, so the pair was
"structurally invisible". Two shipped detectors already evaluate multi-turn windows —
`pre-narration-detector.ts` (`TRAILING_WINDOW_TURNS = 12`, mt#2671) even correlates a claim's named PR
against in-window evidence via `buildIdentityEvidence`. The real gap was a missing **predicate**, not a
missing scope. That correction is what turned this from "build a conversation-scoped claim ledger"
into "reuse the window that exists" — v1 adds **no persistent state at all**.
## The load-bearing design choice
The suppressor counts a subject resolved only by a tool call **strictly after** the hedge turn —
window `(hedgeTurn, assertionTurn]`. Measured against the originating transcript: the hedge turn's own
`memory_get {id: "mem#1323"}` returned `sourceAgentId: null`, which is the call that *created* the
uncertainty. Counting it would make the detector inert on the case that produced it (mem#704). Turns
4–5 carry zero tool calls naming the subject → the turn-5 restatement fires; turn 6's correction runs
the discriminating grep in its own turn → correctly suppressed.
## Key changes
- **`packages/domain/src/detectors/cross-turn-hedge.ts`** (new) — the matcher. Elider injected per
ADR-024 Rung 1 (a domain module must not import from the hooks tree); subject keys restricted to
decidable entity refs; two separately-measured marker legs.
- **`.minsky/hooks/cross-turn-hedge-detector.ts`** (new) — the adapter. Segments the window, supplies
`elideBlocksAndQuotes`, writes an evaluation record for every window scanned.
- **`.minsky/hooks/transcript.ts`** — `windowSlice` **hoisted** from `pre-narration-detector.ts` into
the module whose own header calls itself "the single definition of the turn-boundary logic".
- **Six registries**, each found by its own failing gate (mem#1206's documented pattern):
`GUARD_REGISTRY` via `registry-prompt-scan-guards.ts`, `interceptor-descriptions.ts`,
`interceptor-coordinates.ts`, `hook-module-inventory.md` (two tables + three counts), the **derived**
dispatcher timeout in `.claude/settings.json` (230s → 240s), and the env-var pair.
`enforcement-mapping.ts` needs no entry — that census covers `settings.json`-registered hooks, and no
sibling dispatcher-guard appears there.
## Two defects the tests forced, kept rather than papered over
- A `\b`-anchored file-path pattern silently dropped the leading dot from `.minsky/…` paths, recording
`minsky/hooks/x.ts` — a key that no longer denotes the file it came from.
- `windowSlice(lines, 0)` collapses to the **last turn**, not the whole transcript — the opposite of
the safe direction for a look-back. Documented and pinned by a test rather than "fixed", so
migrating `pre-narration-detector.ts` onto the hoisted export stays a deletion rather than a
semantic change.
## Spec-decision reconciliation
**SC4 is partially met and this is the honest account.** It asks that the window size be "derived from
measured hedge→restatement distance in the calibration data". There is no calibration data yet — this
PR is what starts collecting it. The one measured instance is a gap of **2** (the originating
incident). `WINDOW_TURNS = 8` is therefore *reasoned*, not measured, and is labelled provisional in its
own docblock with the reasoning and the single data point named. `hedgeGapTurns` is recorded on every
evaluation precisely so the first review has the distribution to set it from. Deliberately not copied
from either shipped constant (12, 5) — those were derived for different questions.
## Parallel-work reframe
`session_start` was denied by `parallel-work-guard`: open **PR #3412** changes
`pre-narration-detector.ts`. Confirmed against its actual 313-file changed-file list; `transcript.ts` is
not among them. Disposition was **REFRAME**, not an override — hoisting to `transcript.ts` reaches the
same design end state on a file with no collision. `pre-narration-detector.ts` keeps its private
`windowSlice` until #3412 merges; migration is tracked in the spec with a 2026-09-01 threshold.
## Testing
Execution evidence:
**SC1 / SC5 — the matcher, both legs, and the finding shape:**
```
$ bun test --preload ./tests/setup.ts packages/domain/src/detectors/cross-turn-hedge.test.ts
21 pass 0 fail Ran 21 tests across 1 file. [78.00ms]
```
**SC2 / SC3 / AT1–AT6 — the adapter, the window, and every negative control:**
```
$ bun test --preload ./tests/setup.ts ./.minsky/hooks/cross-turn-hedge-detector.test.ts
19 pass 0 fail Ran 19 tests across 1 file. [104.00ms]
```
AT1 (replay fires, citing both turns) is `evaluateWindow — fires on the hedge/restatement pair with no
intervening lookup`, asserting `hedgeGapTurns === [2]`. AT2 (probe between → no finding) is `a
post-hedge lookup suppresses`. AT3, AT4 and the two elision controls are the `negative controls` block
in the matcher suite. The self-report control — turn 6's *"I flagged that inference as uncertain, then
restated it as fact a message later"*, which carries a hedge marker AND an entity ref in one sentence
and is the **exemplary** behaviour — is `a self-report that re-hedges while naming the subject is not
an assertion`.
**SC6 — calibration-first, both graduation obligations bound:**
```
$ bun test --preload ./tests/setup.ts ./.minsky/hooks/canary-runner.test.ts
35 pass 0 fail Ran 35 tests across 1 file. [4.63s]
```
The canary asserts `calibration`, not `additionalContext`, so a later `INJECTION_ENABLED` flip gains an
outcome rather than breaking one. Both obligations bind through `calibrationLog: "cross-turn-hedge"`
and are now stated in the module header.
**The registry census gate — the one that actually found the six registries:**
```
$ bun scripts/run-related-tests.ts .minsky/hooks/cross-turn-hedge-detector.ts
2921 pass 0 fail Ran 2921 tests across 61 files. [13.78s]
61 related test file(s) passed
```
**Hooks tree + full gated suite:**
```
$ bun run test:hooks
6678 pass 0 fail Ran 6678 tests across 182 files. [23.25s]
$ bun scripts/run-tests-gated.ts
1871 pass 0 fail Ran 1871 tests across 156 files. [92.53s]
run-tests-gated.ts: all test steps passed.
```
**Typecheck** — clean across 8 projects (`.`, `packages/domain`, `packages/shared`,
`services/reviewer`, `services/site`, `src/cockpit/web`, `tsconfig.hooks.json`,
`tsconfig.scripts.json`), validated against the session workspace. `infra/tsconfig.json` skipped with
its documented reason. **Lint** — 0 errors, 0 warnings across 4147 files.
Negative control — the suppressor's hedge-turn exclusion is load-bearing, and the tests discriminate on it.
The central design claim is that the hedge turn's OWN lookup must not count as resolving. Flipping
`turnIndex <= hedge.turnIndex` to `turnIndex <` (i.e. counting it) and re-running both suites:
```
(fail) detectCrossTurnHedgeDecay — the hedge turn's OWN tool call does not suppress
(fail) evaluateWindow — fires on the hedge/restatement pair with no intervening lookup
(fail) evaluateWindow — the hedge turn's own memory_get does not count as resolving it
37 pass 3 fail
```
Reverted; 40 pass. This is the mem#704 check applied to the detector itself — a probe that returns the
same answer whether or not the mechanism works is not evidence.
Deploy verification: this PR changes deploy surface. `isDeploySurfaceFile` was RUN over the changed
files rather than recalled — it returns **true** for `packages/domain/src/detectors/cross-turn-hedge.ts`
and `packages/domain/src/configuration/sources/environment.ts`, and **false** for every
`.minsky/hooks/**`, `.claude/settings.json` and docs path. So no `[no-deploy-impact]` tag. The change
adds a new module and one env-var registration with no behavioural change to any request path — but
that is a claim about intent, not evidence, so post-merge I will wait on the deployment bound to this
merge (`notBefore` = merge time, `expectCommitSha` = merge SHA) and assert the health body's service
identity rather than the status code.
## Follow-ups filed, not deferred silently
- **mt#4704** — the RFC's other falsifier axis (`verified-*` with no backing call), which this PR
surfaced as never-filed.
- Migrating `pre-narration-detector.ts` onto the hoisted `windowSlice` once PR #3412 merges — tracked
in mt#4701's spec with a dated escalation threshold rather than left as a code comment.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Aug 29, 2026
…ages/domain extraction
## Summary
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` said
`src/domain/mesh/postgres-channel-listener.ts` while 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`:
```ts
expectIssue("THE FILE src/domain/memory/types.ts exports MemoryRecord", "code");
```
Test **data**, not a pointer — 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.
**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 references
that must NOT move, which is the subject of the two open sibling tasks above.
## Testing
Execution evidence:
```
$ bun scripts/measure-coverage-claim-paths.ts | grep -cE '^\S+:[0-9]+$'
4 # AT1: was 22 — a drop of exactly 18
$ bun scripts/measure-coverage-claim-paths.ts | grep 'cited:' | ... # AT3
1 scripts/cleanup-tasks-embeddings-uuid-orphans.ts
1 scripts/consolidate-policy-coverage-logs.ts
1 scripts/smoke-proxy.ts
1 services/reviewer/railway.json
```
**AT1** — 22 → 4, exactly 18 fewer, and the stale-`src/domain/*` class is at zero. **AT3** — the
remaining 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:16` citing `services/reviewer/railway.json`). No unrelated fire was
introduced.
**AT2** — every rewritten pointer resolves. Checked across all 14 distinct targets, not sampled:
```
packages/domain/src/auth/token-provider.test.ts OK
packages/domain/src/configuration/schemas/observability.ts OK
packages/domain/src/configuration/sources/environment.ts OK
packages/domain/src/git/fake-git-service.ts OK
packages/domain/src/memory/validation.ts OK
packages/domain/src/mesh/postgres-channel-listener.ts OK
packages/domain/src/persistence/fake-persistence-provider.ts OK
packages/domain/src/repository/github-pr-review.test.ts OK
packages/domain/src/session/current-invocation-marker.ts OK
packages/domain/src/session/fake-session-provider.ts OK
packages/domain/src/setup/github-app/pem-utils.ts OK
packages/domain/src/subagent/transcript-metrics.ts OK
packages/domain/src/subagent/workspace-classifier.ts OK
packages/domain/src/tasks/fake-task-service.ts OK
```
**SC1** — the measurement above reports zero fires of the stale-`src/domain/*` class; the residual
`railway.json` FP remains, which SC1 explicitly permits. **SC2** — satisfied by AT2's existence
check 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.
```
$ grep -rn 'src/domain/' <the 13 files> | grep -v 'packages/domain/src/'
tests/domain/memory/validation.test.ts:69: expectIssue("THE FILE src/domain/memory/types.ts exports MemoryRecord", "code");
```
The single surviving occurrence is the test fixture, as intended.
Negative control — the pre-sweep measurement is the failing observation this change fixes:
```
$ bun scripts/measure-coverage-claim-paths.ts # BEFORE, on main
22 fires
18 src/domain/* <- the class this PR closes
3 scripts/* (deleted scripts — out of scope)
1 services/reviewer/railway.json (known FP)
```
Run on `main` before any edit and again after: 18 → 0 for the class. The probe demonstrably
distinguishes the fixed and unfixed trees, which is the property a control exists to establish.
Suite results:
```
$ bun test --preload ./tests/setup.ts ./tests/domain/memory/validation.test.ts
39 pass 0 fail
$ bun run test:hooks
6696 pass 0 fail Ran 6696 tests across 182 files. [27.72s]
```
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
(`validatedWorkspace` confirmed). Format: clean.
## Deploy verification
**This IS deploy surface, despite being comments-only.** `isDeploySurfaceFile` was RUN over the 12
changed files and returns **true for 7**:
```
true packages/domain/src/git/fake-git-service.ts
true packages/domain/src/observability/braintrust.ts
true packages/domain/src/session/fake-session-provider.ts
true packages/domain/src/tasks/fake-task-service.ts
true packages/domain/src/workspace/fake-workspace-utils.ts
true src/adapters/shared/commands/memory/derivation-validator.ts
true src/cockpit/sse-broker.ts
false .claude/hooks/record-subagent-invocation.ts
false .minsky/hooks/record-subagent-invocation.ts
false scripts/lib/pem-utils.ts
false scripts/verify-mt1510-identity-routing.ts
false tests/domain/memory/validation.test.ts
```
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, and
post-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 `getLoggableErrorSummary` conversion) touches **1 of the 13**
citing files, `src/cockpit/sse-broker.ts`. Not a conflict: its hunks are at line 22 (an import) and
lines ~191 / ~236 (two `log.warn` bodies); 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/domain` paths, were verified disjoint by file —
`packages/domain/src/ask/transports/elicitation-containment.test.ts` and
`packages/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 in `docs/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's
verification 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 changed
line is inside a comment. `bun run src/cli.ts compile` was run and reported
`Target "claude-hooks": 183 file(s) written`, and the pre-commit compile-output check confirmed all
targets up to date.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Aug 31, 2026
…command registry
## Summary
The cockpit could not change a memory in any way — the widget transport was `app.get` only, and no memory widget called a mutating method. This PR adds `PATCH`/`POST`/`DELETE` routes for memory curation (retag, edit name/description, supersede, hard delete, bulk retag, bulk delete) plus the row-level and bulk-selection UI to drive them.
## The architectural decision (mt#4766 Planning Audit, ADR-004)
The routes call the **shared command registry** (`memory.update` / `memory.supersede` / `memory.delete`), never `MemoryService` directly. Two reasons, both verified rather than assumed:
1. Memory's guardrails live in the command layer, not the service. `grep -n` over `src/adapters/shared/commands/memory/index.ts` finds `checkDerivation` (:982), `validateAssociations` on **both** create (:997) and update (:1100 — retag is an update), and `extractTrackingTaskRefs` (:1018). The same grep over `memory-service.ts` returns zero hits. A route calling the service directly would silently skip all four.
2. **ADR-004 (Two-Phase Command Execution, ACCEPTED)** — the shared command registry is the framework-enforced place these guarantees are meant to live, specifically because "remember to validate first" is a bug class convention cannot close. `asks.ts`'s domain-function-direct pattern is not a counter-example: `respondAndCloseAsk` IS the domain function carrying the ask's preconditions; memory has no equivalent, so its preconditions only run when the command layer's `execute` is what gets invoked.
`src/cockpit/routes/memories.ts`'s docblock explains why commands are constructed **per-request** (a fresh throwaway `SharedCommandRegistry` injected with the cockpit's cached `MemoryServiceSurface`) rather than registered once at module load: registering once with a pre-bound service would pin it past a persistence-pool recycle (mt#3638), and registering once without one would require threading a DI container the cockpit doesn't otherwise have.
## AT2 — the discriminating test
> Attempt to write an association with a key outside ADR-012's closed vocabulary and confirm it is REJECTED.
This is the test that tells the two architectures apart — a route calling `MemoryService.update` directly has no vocabulary check to fail against. Result: **PASS**, both in the unit suite and live (see Live verification below).
```
PATCH /api/memories/:id { associations: { notARealAssociationType: ["mt#1"] } }
→ 400 { error: "\"notARealAssociationType\" is not an ADR-012 association type. ..." }
```
And the write never happens — the persisted record's `associations` is confirmed unchanged.
## Actor attribution (mt#2898 applied to memory)
mt#2898 documents that `POST /api/asks/:id/resolve` reads `responder` from the request body, making `responder: "operator"` forgeable by any caller, and that this became load-bearing once a permission bridge started trusting it. These routes never read `sourceAgentId`/`sourceSessionId` from the client at all — they aren't in the `supersede` route's field whitelist, so a request that includes either is rejected outright (400) rather than silently accepted-and-ignored. A supersede's replacement record is always ascribed `COCKPIT_OPERATOR_SOURCE_AGENT_ID` server-side. Verified both in the unit suite and live: a normal request gets the ascribed constant (`"cockpit-operator"`), and a request carrying `sourceAgentId: "attacker-controlled"` is rejected 400 with no record ever created.
## Key changes
- `src/cockpit/routes/memories.ts` (new) — `mountMemoryRoutes(app, options)`, following `asks.ts`'s structural pattern (own file, explicit status-code contract, whitelisted body fields — not the widget dispatcher's uniform `200 + {state:"degraded"}`).
- `src/cockpit/server.ts` — mounts the new routes alongside the other write-route mounts.
- `src/cockpit/web/lib/memory-mutations.ts` (new) — fetch wrappers + TanStack Query `useMutation` hooks that invalidate the `memories-list`/`-search`/`-stats`/`-detail` widget queries on success.
- `src/cockpit/web/widgets/MemoryDetail.tsx` — a curation action bar (Edit tags / Edit name-description / Supersede / Delete), each a controlled Dialog. The delete dialog states plainly that this is a **hard delete** and that memory has **no short-id tombstone table** (unlike tasks' `deleted_task_ids`), so a deleted `mem#N` can be reissued — the operator sees this before confirming.
- `src/cockpit/web/widgets/MemoriesList.tsx` — per-row + select-all-on-page checkboxes and a bulk actions bar (retag / delete), each opening a dry-run-first dialog: preview exactly which records change, then confirm to execute.
- `src/cockpit/routes/memories.test.ts` (new) — 24 tests.
- `docs/cockpit-ui.md` — a "Memory curation" section documenting all five routes (methods, request bodies, status codes), the actor-attribution rule, and the hard-delete/no-tombstone consequence.
## Bulk operations — dry-run-first + a record cap
Per `operational-safety-dry-run-first`, both bulk routes preview the exact record set before any write (`execute: false` by default) and require an explicit `execute: true` to act. Judgment call, stated here: rather than requiring a separate task wrapper for >10-record bulk mutations (the discipline's normal trigger), the routes simply refuse a selection over `BULK_RECORD_CAP` (10) with a 400 naming that discipline — a UI-driven, per-click bulk action doesn't have a natural place to attach a task reference, so capping the direct-execution size was the simpler and equally-safe choice for this surface.
## Testing
Execution evidence:
```
bun test --preload ./tests/setup.ts --timeout=15000 src/cockpit/routes/memories.test.ts
24 pass, 0 fail, 55 expect() calls
```
Covers AT2 (association-vocabulary rejection), AT5 (auth-gating — see below), AT6 (bulk-retag preview matches selection exactly; execute changes exactly that selection; over-cap selection refused), AT7 (malformed/wrong-typed/unknown body fields → 400, never 500), and the two actor-attribution tests above. Also: 404 on a missing record, 200 + persisted-state assertions on update/supersede/delete, invalid type/scope rejection on supersede.
`validate_typecheck` and `validate_lint` both pass clean across every workspace, including `src/cockpit/web`.
### AT5 — real unauthenticated requests against both deployment-mode gates
R1 finding: the original version of this test asserted `isPublicPath()` returns `false` for each route path — a check against a config table, not a behavior, that would keep passing if the gate were removed, misconfigured, or mounted in the wrong order. Replaced with genuine unauthenticated HTTP requests against the real `createCockpitServer` app (the same factory `server.ts` uses in production) for **both** deployment-mode gates:
- Public Railway deployment (`requirePasskeySession`, path-based, the same gate `/api/shares` relies on) — no session cookie → `401` on all five routes.
- Local daemon (`mutationAuthMiddleware`, method-based — every non-GET needs a bearer token or the loopback cookie) — no token → `401` on all five routes.
Also verified LIVE against a real running local daemon (see below): an unauthenticated `DELETE` returns `401` with `{"error":"Missing or invalid cockpit auth token"}`; the identical request with a valid bearer token returns `200`.
## Live verification
Live-exercised every route against a real, DB-connected cockpit daemon, plus screenshots of the resulting UI at 1440×1000. This required correcting course from an earlier attempt (see "How this got unblocked" below).
**Setup:** `pgvector/pgvector:pg16` in a disposable Docker container, migrated via `bun src/cli.ts persistence migrate --execute` with `MINSKY_POSTGRES_URL` pointed at it (confirmed by read-back BEFORE writing anything — see below), then `bun src/cli.ts cockpit start --port 18080 --dev` against that same database.
**AT2, live** (association vocabulary):
```
$ curl -X PATCH http://127.0.0.1:18080/api/memories/<id> \
-H "Authorization: Bearer <token>" \
-d '{"associations":{"notARealAssociationType":["mt#1"]}}'
→ HTTP 400
{"error":"\"notARealAssociationType\" is not an ADR-012 association type. Valid types:
tracksTask, relatedTask, originatesRule, originatesSkill, informsAsk, extractedFromSession,
extractedFromTranscript, citedInReview. ..."}
```
**Retag (valid), live:** `PATCH .../:id {"tags":["seed-mt4766","retagged-live"]}` → `200`, response `record.tags` updated, confirmed in Postgres.
**Actor attribution, live:**
```
POST .../:id/supersede { ..., "sourceAgentId": "attacker-controlled" }
→ HTTP 400 {"error":"Unknown field(s): sourceAgentId"}
POST .../:id/supersede { ...valid body, no sourceAgentId }
→ HTTP 200, replacement.sourceAgentId = "cockpit-operator", replacement.sourceSessionId = null
old.supersededBy = replacement.id (confirmed in Postgres)
```
**Delete, live:** authenticated `DELETE .../:id` → `200 {"deleted":true,"id":"..."}`; row count for that id in Postgres afterward: `0`.
**Bulk retag/delete, live (AT6):** preview (`execute:false`) over 3 seeded records returned exactly those 3 with `currentTags`/`newTags`, no write (confirmed via a follow-up `SELECT` — tags unchanged); `execute:true` then changed exactly those 3 rows (confirmed via `SELECT` — all 3 updated, an untouched 4th record unaffected); bulk delete likewise removed exactly the 3 targeted rows.
**Screenshots (1440×1000, headless Chrome via chrome-devtools-mcp against the live daemon above):**
- `/memories` — list with the new select-all/per-row checkboxes, one seeded record visible, stats/facets rendering normally.
- `/memory/:id` — the curation action bar (**Edit tags** / **Edit name / description** / **Supersede** / **Delete**) rendered above the metadata panel, no layout defects.
- The delete confirmation dialog — states plainly: *"This permanently deletes the row and its embedding. There is no undo, and unlike tasks, memory has no short-id tombstone table — this record's short id can be reissued to a different, unrelated memory in the future."*
- `/memories` with one row selected — the bulk actions bar ("1 selected", **Retag**, **Delete**, "Clear selection") rendering correctly above the table.
No defects found in any of the four renders. Cancelled the delete dialog rather than confirming it (no need to destroy the demo record to prove the dialog renders correctly). All scratch infrastructure (container, cockpit process, headless Chrome, temp files) torn down after verification — nothing was left running, and none of this touched the project's real database at any point.
### How this got unblocked (for the record)
My first attempt set `DATABASE_URL`, which this system's config resolution does not read at all — the CLI silently fell through to the project's real Supabase-hosted database, and I stopped before running anything mutating (documented in this PR's prior revision). The coordinator pointed me at `MINSKY_VERIFY_DATABASE_URL`, registered in `packages/domain/src/configuration/sources/environment.ts` as a `test-fixture`. Checking that registration directly: it is a narrowly-scoped fixture read **only** by `scripts/verify-driven-session-conversations.ts` (mt#4323) — confirmed empirically too (`persistence migrate --dry-run` with only that variable set still reported the Supabase pooler host). The variable actually wired into general persistence-config resolution (`persistence.postgres.connectionString`, the path `persistence migrate` and the cockpit daemon both read) is `MINSKY_POSTGRES_URL` (or `MINSKY_PERSISTENCE_POSTGRES_URL`), documented in `persistence-config.ts` as "the canonical escape hatch." Using that, the dry-run's read-back showed `postgres://***:***@127.0.0.1:<port>/minsky_scratch` with `schema=missing` (a fresh, empty database) — confirmed safe before running `--execute`.
## Out of scope (per spec)
- Creating a memory from the cockpit (mt#4743's permanent-association derivation defect).
- `memory_patch` section editing (mt#4612's open `replace`-mode defect).
- Dedup execution (mt#1619).
- The stats widget (mt#4767) — this PR only adds it to the invalidated-query list for consistency.
## Duplicate check
No other open PR touches `src/cockpit/routes/memories.ts`, `src/cockpit/web/widgets/MemoriesList.tsx`, or `src/cockpit/web/widgets/MemoryDetail.tsx` (per the mt#4766 Planning Audit's parallel-work sweep, PR #3412 touches unrelated files via a mechanical catch-block rewrite).
## Deploy verification
Seven deploy-surface files: the new `routes/memories.ts` and its test, `server.ts` (route
mounting), `scope-census.ts` (the R2 allowlist entry), the mutation hooks, and the two widgets.
No migration and no domain change — the risk shape is a boot failure from the new route mounting,
not a schema change.
After merge I will run `mcp__minsky__deployment_wait-for-latest --service cockpit` with `notBefore`
set to the merge timestamp and `expectCommitSha` set to the merge SHA, and require SUCCESS. The
cockpit deploys from a pushed image, so `buildIdentity` will read `indeterminate` — expected for
this service and not treated as a pass on its own.
Because an indeterminate build identity says nothing about WHICH build is serving, the
change-produced assertion is the route itself: `OPTIONS`/unauthenticated `PATCH` against
`/api/memories/:id` on the deployed cockpit must return a real status from the new handler
(401 under the auth gate) rather than the SPA fallback a pre-merge bundle would serve. That is
immune to which deployment record the wait returns, and it exercises the mounting — the specific
thing that could break on boot.
A tool or auth flake on the deployment probe is a blocker to reconnect and retry, not a licence to
downgrade the completion claim.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Aug 31, 2026
…e working tree ## Summary Two writers were still appending into the repo working tree after mt#4748's merge (2026-08-30 19:44), with mtimes to prove it — one of them advancing during the session that found it. One was broken rather than merely misplaced. **`ask-form-lint` was broken.** It is a `CALIBRATION_LOG_REGISTRY` member (`calibration-sweep.ts:255`), so `/calibration-review` resolves it through `resolveCalibrationStatePath` — the state dir. The writer still did `resolve(workspacePath, ...)`. Measured before the fix: **17,187 bytes in the repo copy and no state-dir copy at all**, so the sweep read an empty corpus and would report *"this guard never fired."* Same silent-zero shape mt#4780 fixed on the docs side, arriving from the write side, which mt#4780 explicitly scoped out. **`warn-main-workspace-mutation` was not broken.** Reader and writer both used `deriveHookRepoRoot()`, so it worked — it just deposited a baseline file into whatever project the agent was in, which is the condition mt#4748's SC2 forbids. ## Key changes - `appendAskFormLintCalibrationRecord` calls the **reader's own resolver**, so writer and reader agree by construction rather than by two derivations staying in sync. It became async for that; its one production caller was already inside an async handler. - `warn-main-workspace-mutation` resolves via the project-keyed state dir, **importing** `projectStateKey` rather than recomputing it. - Neither adds a derivation of the project key. (There are already four — `dispatcher.ts:344`, `ingest-runtime.ts:70`, `calibration.ts:236`, and one inside `calibration-review-cadence-detector.ts:134` whose comment explains the choice.) ## Execution evidence: ``` $ bun test ./src/adapters/shared/commands/ask-form-lint-calibration.test.ts 7 pass / 0 fail $ bun run test:hooks 6787 pass / 0 fail / 14171 expect() calls [185 files] $ bun scripts/run-related-tests.ts src/adapters/shared/commands/ask-form-lint-calibration.ts src/adapters/shared/commands/asks.ts 676 pass / 0 fail ``` Typecheck clean across 8 projects; lint 0 errors / 0 warnings over 4,246 files. **SC1** — neither writer resolves against the repo root; `find .minsky -newermt <merge>` no longer returns either file. **SC2** — verified live, below. **SC3** — migrated, see Live verification. **SC4** — no manifest change needed, and the reason is the finding: `ask-form-lint` was **already** declared `location: "state-dir"` in `stream-sources.ts:89`. Reader and manifest agreed with each other and both disagreed with the writer, so this aligns the writer to a standing declaration. `main-workspace-mutation-baseline` is not an ingested stream and has no row. **SC5 — NOT met in this PR. `[sc5-deferred: mt#4816]`.** The criterion itself says to coordinate one check rather than add a third independent one, and the implementation findings sharpened why: the population is defined by BEHAVIOUR (resolves a repo-rooted telemetry path and appends to it), not by directory, so a check scoped to `.minsky/hooks/**` would reproduce the blind spot that hid `ask-form-lint-calibration.ts` in `src/adapters/shared/commands/` from every prior sweep. mt#4816 is the last task in the family and carries the coordinated check in its own SC5. Deferring rather than shipping a fourth directory-scoped check is the point, not an omission. **Negative control.** Reverting only the path resolution turns the new assertions red: ``` $ perl -0777 -pi -e 's/await resolveCalibrationStatePath\(...\)/resolve(workspacePath, ASK_FORM_LINT_CALIBRATION_LOG)/' ask-form-lint-calibration.ts $ bun test ./src/adapters/shared/commands/ask-form-lint-calibration.test.ts 5 pass / 2 fail # restored ``` The new test asserts the path via `resolveCalibrationStatePath` — the same function the sweep calls — rather than a hand-written path. A hand-written expectation would have passed happily while reader and writer disagreed, which is precisely the state being fixed. ## Live verification SC3's disposition of the pre-existing corpus, run against the real state dir: ``` $ key = sha256(/Users/edobry/Projects/minsky)[0:16] -> a0809beec3ba7e98 src lines/bytes: 51 / 17187 dest lines/bytes: 51 / 17187 IDENTICAL — removing source $ bun -e 'resolveCalibrationStatePath(cwd, ".minsky/ask-form-lint-calibration.jsonl")' reader resolves to: ~/.local/state/minsky/projects/a0809beec3ba7e98/ask-form-lint-calibration.jsonl exists: true records: 51 ``` 51 records migrated byte-identically, source removed (so it does not become a 62nd orphan for mt#4777), and the reader now finds them. **The same call before this change found nothing.** ## Deploy verification: Ran the predicate over the actual changed files rather than recalling the pattern set: `ask-form-lint-calibration.ts`, `asks.ts` and the test are `true`; both `warn-main-workspace-mutation.ts` copies are `false`. So this IS deploy surface. I will run `deployment_wait-for-latest` after merge with the merge timestamp as `notBefore` and the merge commit as `expectCommitSha`, require a health body whose `service` matches, and — since minsky-mcp is image-source and returns `buildIdentity: indeterminate` by construction — settle identity by correlating the Deploy MCP workflow run's `head_sha` against the merge commit. ## Three corrections found while implementing 1. **`calibration-review-cadence-detector` is not broken.** The spec's carve-out listed it among three still-repo-rooted writers. mt#4748 fixed it (`:146`), and the mtimes that put it on my list predate that merge. Two writers, not three. 2. **`verify-subagent-model` was started as a fold-in and reversed.** It is `family: "special"`, which `resolveStreamPath` (`ingest-runtime.ts:74-91`) keeps FLAT rather than project-keyed by deliberate design — so moving it is a decision about what that family means, not a path edit. Filed as mt#4816. It is the last `location: "repo"` row. 3. **The class is defined by behaviour, not directory.** This is the fourth task finding "one more writer" (mt#4752, mt#4778, this, mt#4816) — see SC5 above. ## Parallel work **PR #3412** (mt#4639, 614-log-site sweep) also modifies `ask-form-lint-calibration.ts`. Found on **page 3** of its 313-file list — page 1 read as clean, so a single-page check would have recorded a false negative. It rewrites the error-formatting call *inside* the catch block; this changes the path resolution *above* it, so the edits are line-disjoint and the rebase is trivial. My other three in-scope files are untouched by it. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 1, 2026
…ent-protocol parity
## Summary
`@modelcontextprotocol/sdk` (v1) **never implements the MCP 2026-07-28 revision at any published version** — npm carries exactly one dist-tag, `latest` → `1.30.0` (published 2026-07-27, one day *before* the spec), whose `LATEST_PROTOCOL_VERSION` is `2025-11-25`, with no `inputResponses`/`resultType` anywhere in it. Support ships in a separate **v2 package family** (`@modelcontextprotocol/core|client|server|node`, all `2.0.0`).
This migrates every SDK import site to v2 **at current-protocol parity**: no wire change, no serving-entry change. Adopting 2026-07-28 stays with the parent mt#4608, gated on a decision ask#11232 deliberately deferred. This is the vendor's own staging — `upgrade-to-v2.md` and `support-2026-07-28.md` are two separate guides.
## Key changes — what the codemod did vs. what was hand-edited (SC4)
**Codemod (`bunx @modelcontextprotocol/codemod v1-to-v2`), reviewed not merged blind.** Its dry run matched the planned file set exactly — 34 changes across 10 files + `package.json` — and every rewrite was checked against the SDK's own `importMap.ts`: `Server`/spec types → `/server`, `StdioServerTransport` → `/server/stdio`, `StreamableHTTPServerTransport` → `NodeStreamableHTTPServerTransport` from `/node` (Express hands us Node `req`/`res`, matching the guide's decision rule), `JSONRPCMessageSchema` → `/core`, `Client` + `InMemoryTransport` both from `/client` (the guide requires one package per linked pair), `McpError`/`ErrorCode` → `ProtocolError`/`ProtocolErrorCode`.
**Hand-edited — three things the codemod flagged but could not resolve** (8 warnings, 6 `@mcp-codemod-error` markers, all discharged):
1. **`diagnostic-capture.ts`** — v1's flat `extra` became v2's structured `ctx`, and the two fields this capture reads moved *differently*: `sessionId` stayed top-level, `_meta` moved to `ctx.mcpReq._meta` (per the SDK's `contextPropertyMap.ts`). Reading `ctx._meta` would not throw — it would silently capture `undefined` forever, and nothing asserts on this research output.
2. **`server.ts` `tools/list`** — v2 types `Tool.inputSchema` as `{ type: "object"; … }` where v1 took a bare `object`. The `{}` fallback became `{ type: "object" }`. **Not a wire change:** the only production `addTool` caller (`command-mapper.ts:508`) always supplies an `inputSchema`, so the fallback is unreachable there. Corrected rather than cast away — a bare `{}` was never spec-valid.
3. **Four test suites** — v2 routes `tools/call` through `_invokeInputRequiredCapableHandler`, which reads `ctx.mcpReq.requestState()` before delegating, so the bare `{}` these tests passed as `extra` now throws *inside the SDK*. Fourteen reaches into the SDK's private `_requestHandlers` map collapse into one `src/mcp/test-support/tools-call-handler.ts` helper — which also reduces mt#4844's migration from fourteen sites to one.
## Review response
**R1 BLOCKING — "`@modelcontextprotocol/sdk` still present in bun.lock".** The finding is correct and **my criterion was wrong.** `@modelcontextprotocol/inspector@0.16.2` (a devDependency, out of scope) and its three sub-packages all declare `"@modelcontextprotocol/sdk": "^1.17.0"`, so a v1 lockfile entry is not removable by this task at all. `package.json` itself declares none.
It pointed at something real that the wording hid: bun hoists the transitive v1 to `node_modules/@modelcontextprotocol/sdk`, so a stray `import "@modelcontextprotocol/sdk"` would still **resolve and typecheck** — the migration had no regression guard. Fixed with an ESLint `no-restricted-imports` ban on the package and its subpaths, which is now what enforces SC1. AT4 amended to its achievable form.
**R1 NON-BLOCKING (notify)** — correct, and latent rather than hypothetical: `server.ts:1471` passes `ctx.mcpReq.notify` to `buildProgressReporter` whenever a request carries a `progressToken`. Added an async no-op default. Class scan run: `grep -rn 'ctx\.mcpReq\.'` over `src`/`packages`/`scripts` shows `notify` is the **only** other member our source reaches, so the helper now supplies exactly the reachable set.
**R1 NON-BLOCKING (private `_requestHandlers`)** — acknowledged, no change. That is mt#4844's scope; this PR shrank the surface from fourteen reaches to one to make that a single edit.
**R2 NON-BLOCKING — "caller override order is inverted".** Correct, and it was a comment contradicting its own code: the literal `mcpReq` sat *after* the outer spread, so a caller-supplied `mcpReq` was silently discarded while the docblock claimed caller-first. Fixed by spreading the caller's `mcpReq` **over** the defaults, and covered by a new test file so the contract is enforced rather than asserted.
## Spec deviations recorded
- **SC1 corrected (12 → 10 files).** The planning enumeration used `grep -l` on the package *string*, which counts mentions as imports. `src/commands/mcp/start-command.ts`, `services/reviewer/src/mcp-client.ts` and `scripts/mt2677-live-verify-progress-notifications.ts` only *name* the SDK in comments — the reviewer client's matched line literally reads *"Plain fetch only — no `@modelcontextprotocol/sdk` dependency"*. The codemod independently confirmed 10.
- **SC3 + AT4 withdrawn/replaced.** SC3 required the two families to *coexist* because the reviewer client would stay on v1 pending mt#2856. It was never a consumer, so v1 is removed outright. AT4 then had to be corrected a second time per R1 above.
## Known overlap, acknowledged rather than discovered at merge
**PR #3412** (mt#4639, open since 2026-08-27) touches **3** of this PR's files: `src/mcp/server.ts` (its import block), `scripts/deploy-minsky-mcp.ts`, `scripts/verify-mcp-hidden-params.ts`. Proceeding deliberately: #3412 already owes a rebase regardless of this PR (#3532 merged and modified two of its files), the edits are semantically independent (catch bodies vs. import specifiers), and blocking a 10-file migration on a 313-file PR inverts the cost.
## Testing
Execution evidence:
**SC1** — no v1 import remains anywhere:
```
$ grep -rn '@modelcontextprotocol/sdk' --include='*.ts' src packages services scripts | grep import
NONE
```
Every remaining specifier is v2: `/server` (Server, spec types, ProtocolError/ProtocolErrorCode, LATEST_PROTOCOL_VERSION), `/server/stdio` (StdioServerTransport), `/client` (Client, InMemoryTransport), `/node` (NodeStreamableHTTPServerTransport), `/core` (JSONRPCMessageSchema).
**SC2 — no wire change, measured live rather than assumed:**
```
BEFORE (v1, main's node_modules/@modelcontextprotocol/sdk/dist/esm/types.js):
LATEST_PROTOCOL_VERSION = '2025-11-25'
AFTER (v2, session's node_modules/@modelcontextprotocol/core/dist):
LATEST_PROTOCOL_VERSION = "2025-11-25"
$ bun scripts/verify-mcp-hidden-params.ts
[verify-mcp-hidden-params] handshake ok at protocolVersion 2025-11-25 (requested 2025-11-25)
```
**SC5** — the serving entry is untouched; the 2026-07-28 opt-in is NOT introduced:
```
$ grep -rn 'createMcpHandler\|serveStdio\|versionNegotiation' --include='*.ts' src packages scripts
NONE
```
**Tests** — each `src/mcp` file in its own process. Passing several to one `bun test` invocation reproduces the Bun defect CLAUDE.md documents (no summary, exit 0), which is a non-answer, not a pass, and is not counted here:
```
src/mcp/server.test.ts 46 pass 0 fail
src/mcp/server-tool-name-resolution.test.ts 4 pass 0 fail
src/mcp/client-capabilities.test.ts 23 pass 0 fail
src/mcp/drift-gate.test.ts 28 pass 0 fail
src/mcp/server-in-flight-tool-calls.test.ts 1 pass 0 fail
src/mcp/server-response-size-guard.test.ts 3 pass 0 fail
src/mcp/test-support/tools-call-handler.test.ts 4 pass 0 fail (new this PR)
$ bun test --preload ./tests/setup.ts packages/domain/src/errors/mcp-structured-errors.test.ts \
src/commands/mcp/start-command.test.ts scripts/deploy-minsky-mcp.test.ts
96 pass
0 fail
236 expect() calls
Ran 96 tests across 3 files. [16.65s]
```
`validate_typecheck` clean across 8 projects; `validate_lint` clean across 4296 files.
**Bundle-boot smoke** — asserting the health body's service identity, not just the status code (per mt#3148):
```
$ bun run build → dist/minsky.js, 26,048,532 bytes
$ bun run dist/minsky.js mcp start --http --host=127.0.0.1 --port=48799
$ curl http://127.0.0.1:48799/health
HTTP 200
{"status":"ok","service":"minsky-mcp","server":"Minsky MCP Server","transport":"http",
"persistence":{"mode":"connected"},"ready":true,"db":"ok"}
```
Negative control — three, each run and observed rather than asserted:
```
(1) the v2 ctx fix, round 1 — src/mcp/server.test.ts before the fix:
161 pass / 8 fail / 1 error
TypeError: undefined is not an object (evaluating 'ctx.mcpReq.requestState')
at _invokeInputRequiredCapableHandler (@modelcontextprotocol/server/dist/mcp-DXXb3Vv3.mjs:878)
(2) the v2 ctx fix, round 2 — caught by the pre-push gated suite, NOT by my targeted run:
src/mcp/drift-gate.test.ts 26 pass / 2 fail
src/mcp/server-in-flight-tool-calls.test.ts 0 pass / 1 fail
src/mcp/server-response-size-guard.test.ts 0 pass / 3 fail
— same TypeError in all six
(3) the R1 lint ban — a scratch file importing v1, deleted after the run:
1:1 error '@modelcontextprotocol/sdk/server/index.js' import is restricted ... no-restricted-imports
2:1 error '@modelcontextprotocol/sdk' import is restricted ... no-restricted-imports
✖ 2 problems (2 errors, 0 warnings)
— fires on both the bare specifier and the subpath form
(4) the R2 override fix — caller spread removed from the helper, new suite re-run:
(fail) getToolsCallHandler (mt#4854) > a caller-supplied mcpReq member WINS over the default, and the others survive
3 pass / 1 fail
— exactly the discriminating test fails; the other three are insensitive to the ordering by construction
```
Round 2 is worth naming for the reviewer: those three files break from the **dependency** change while the related-test selector keys on the files the **diff** changed, so it could not reach them by construction. A dependency upgrade's blast radius is its dependents, not its diff. Filed as mt#4862.
Deploy verification: `src/mcp/server.ts` ships in `minsky-mcp`, so this is deploy surface. After merge I will run `deployment_wait-for-latest` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, read `buildIdentity`, and assert the `/health` body's `service` field, per §10.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
minsky-ai Bot
pushed a commit
that referenced
this pull request
Sep 1, 2026
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.
edobry
added a commit
that referenced
this pull request
Sep 1, 2026
## Summary
mt#4792 fixed the manufactured-match defect in the shared `prose-elision.ts` primitive: eliding to
spaces lets a caller's own `\s+` run through the blanked hole, so a clause that does **not** match
the 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.ts` is 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_FILL` and `blankSameLength` are 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.ts`
uses the same blank-to-spaces shape, and `linkifyExternalRefs` **rewrites ask body text**.
Planning flagged it as "probably benign — confirm it." Confirming it inverted the answer:
`NOTION_CUE` is
```
\bnotion\b[\s:—–-]*(?:page[\s:—–-]*)?(?:id[\s:—–-]*)?["'`]?
```
— a **whitespace-tolerant separator class**. 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 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 `\s` nor the separator class.
## Deliberately not converted
- **`output-label-tokens.ts:135`** — classified benign, reasoning recorded in-file. Its only two
consumers (`unrendered-result-fields.ts:170,178`) **count parens** over the residual; a space
cannot manufacture a `(`. The filler's only requirement here is "not a bracket."
- **The five detectors that collide with open PR #3412** (`build-claim-injection`, `causal-premise`,
`code-mechanism-assertion`, `pre-narration`, `substrate-bypass`). Planning read #3412's full
313-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-narration` still gets the fix for the half it *imports* from `elision.ts`.
## Deploy verification
`isDeploySurfaceFile` returns **true** for 2 of 11 changed files —
`packages/domain/src/ask/external-refs.ts` and `packages/domain/src/text/prose-elision.ts` — run over
the actual diff, which is why this PR carries no `[no-deploy-impact]` tag. Post-merge I will wait on
the deployment bound to this merge (`notBefore` = merge time, `expectCommitSha` = merge SHA) and
assert 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:
```
$ bun test --preload ./tests/setup.ts ./.minsky/hooks/elision.test.ts
13 pass
0 fail
25 expect() calls
Ran 13 tests across 1 file. [76.00ms]
```
**AT4** — full hooks suite green:
```
$ bun run test:hooks
6973 pass
0 fail
14584 expect() calls
Ran 6973 tests across 188 files. [56.50s]
```
**AT1 / SC1** — the census grep re-run over BOTH forms (`" ".repeat(` and `[^\n]/g, " "`; the spec's
own grep matched only the first, which is why `claim-provenance-scan.ts:374` looked absent). What
remains is exactly the justified set — the five #3412-colliding detectors plus the benign
`output-label-tokens`:
```
.minsky/hooks/build-claim-injection-detector.ts:348,351,353
.minsky/hooks/causal-premise-detector.ts:344,348,351
.minsky/hooks/code-mechanism-assertion-detector.ts:714,717
.minsky/hooks/pre-narration-detector.ts:496,498,499
.minsky/hooks/substrate-bypass-detector.ts:603,607,610
.minsky/hooks/output-label-tokens.ts:135 # classified benign
```
**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 two
properties 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-refs` section above; the manufacture question was answered against
the 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.
```
reverted 5 sites to space filler
(fail) elideQuotedContexts: code span
(fail) elideQuotedContexts: multi-backtick span
(fail) elideQuotedContexts: a blockquote line
(fail) elideDoubleQuotedSpans: straight double quote
(fail) elideDoubleQuotedSpans: curly double quote
(fail) elideQuotedAndCodeContexts (the composed pass): code span
(fail) elideQuotedAndCodeContexts (the composed pass): prose quote
(fail) elideQuotedAndCodeContexts (the composed pass): quote nested in code
(fail) the filler is neither whitespace nor a word character
4 pass
9 fail
```
**A regression the suite caught, worth naming.** `turn-end-retro-scan.test.ts` spelled a blanked span
out 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/hooks` is the source; `.claude/hooks` regenerated via
`bun run src/cli.ts compile` (`Target "claude-hooks": 186 file(s) written`), verified by git status
and by grepping the mirror: 6 `blankSameLength` occurrences in `.claude/hooks/elision.ts`, and only
the five deferred detectors still space-filled. No unrelated regeneration drift.
**Typecheck** clean across all 8 projects (`infra` skipped with its documented reason).
**Lint** 0 errors / 0 warnings across 4,297 files. **Format** `format:check` exit 0.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
The reviewer's operator alerting is wired to the wrong tracker. mt#2363 (the cockpit Ask) and mt#2364 (the external sink) both hang off one emit point — `sweeper.circuit_breaker_tripped`, gated on the `reviewer_submission_failures` circuit breaker from mt#2350. That tracker is written from exactly one place: the `catch` around `submitReview` in `guarded-submit.ts:123` (verified — it is the only non-test call site). A review that dies **before** it can submit creates no tracker row, so the circuit never opens, so no ask and no external alert is ever produced. Measured against prod 2026-09-01, re-verified 2026-09-02: **88 `failed_at_reviewer` rows in 30 days** against **16 tracker rows EVER**, the newest from 2026-08-08. mt#1596 escalated this exact family in 2026-06 and its two children shipped DONE — the family sentence is still true, because the path that shipped reads a different table than the one that fills up. ## What the planning pass corrected before implementation Four things the spec asserted did not survive checking, and each changed the fix: 1. **9 of the 88 are not pre-submit** — they are the mt#3852 submit-path 422, which *does* write a tracker row (their dates line up one-for-one with the tracker's own, and 2026-08-08's are `alerted = true`). Uncovered population is **79**, and a naive "alert on every `failed_at_reviewer`" would double-alert on the one class already covered. That became SC8. 2. **There are two `failed_at_reviewer` writers, not one** — 51 rows carry `stage: "reviewer"` (`server.ts`), 33 carry `stage: "boot_recovery"` (`boot-recovery.ts`). Four write sites in total, since each file has both a `.then` and a `.catch`. A seam at `review_error`/`webhook_processing_failed` would have covered `server.ts` only and left ~38% of the class dark. 3. **"No PR-visible signal" was false** — `server.ts`'s catch writes `buildErrorBody` to the PR status comment on every thrown failure. What is actually wrong is sharper: that comment is a single update-in-place comment, so the next review overwrites it. On `edobry/peezombie.me` PR #2 it was created at 17:21:37 (the minute of the first failure) and now reads "APPROVED", updated 22:59:03. 4. **The durable in-PR signal is missing.** `publishCheckRun` has exactly two call sites, both in `review-finalize.ts`, and the error one is reached only via `finalizeReviewError` — the empty-output and CoT-leakage cases. A throw is caught at `review-worker.ts:942`, which records unrecovered timing and rethrows, so it never gets there. The two dominant classes set **no check-run conclusion at all**. ADR-030 assigns "status, convergence, and liveness of record" to the check-run channel and names failure/liveness surfacing as its open follow-up; the spec cited it nowhere. That became SC7. ## Key changes - **`services/reviewer/src/failure-alert.ts` (new)** — the seam. All four `failed_at_reviewer` write sites now call `recordReviewFailure` instead of `updateOutcome`, so the outcome write and the alert cannot drift apart. It classifies the error (10 classes, every one derived from the measured population), suppresses duplicates, carries the aggregation facts, and skips what the circuit breaker already owns. Fail-open throughout — alerting must never affect a review. - **`ask-emitter.ts`** — `emitReviewFailureAlert` + `ReviewFailureAlertContext`, alongside the existing circuit-breaker method. Dedup for this path lives in the caller, which is what mt#1596 asked for ("these emit points need their own dedup design"). - **`server.ts` / `boot-recovery.ts`** — the four call sites, plus the emitter built from the same domain container the sweeper uses. A skip resolved after dispatch keeps its existing `failed_at_reviewer` write and is deliberately *not* alerted (`concurrent_inflight` is in the measured 88 for exactly this reason). - **`check-run-publisher.ts`** — `ConvergenceState.roundNumber` widened to `number | null`. On the thrown path the round is genuinely unknown (nothing has ingested the prior reviews), so `null` renders without the round parenthetical rather than fabricating "round 1". Output for a numeric round is byte-identical to before, asserted by test. - **`scripts/replay-failure-alerting.ts` (new)** — the §7a verification artifact; see Live verification. ## The judgment call the replay forced The dedup key started as `(owner, repo, pr_number, head_sha, error_class)` — what the spec proposed. Replaying the real corpus showed that collapsing **88 failures to 60 asks**, which is barely a reduction: a repo-wide outage fails each PR exactly once, so a per-PR key dedups nothing. 30 of those 60 were already flagged systemic. So the key is now adaptive. The failure that *tips* a condition over the distinct-PR threshold still alerts, carrying the repo-wide signal; every later PR hit by the same condition inside the window is suppressed. Same corpus: **37 asks (~1.2/day)**. This is a deviation from the spec's proposed key, recorded here and in the code, and it is the reason the artifact exists rather than a nice-to-have. **Thresholds are grounded in observed cadence, not round numbers.** The 60-minute suppression window comes from the originating burst (4 failures on one PR spanning 17:21-18:15, a 54-minute burst); the systemic threshold of 3 distinct PRs comes from the measured day-buckets splitting cleanly into single-PR days (1 PR) and repo-wide days (6, 11, 13 PRs) with nothing in between. ## Testing Execution evidence: **AT1 / AT3 / AT4 / AT5 / AT6, SC1-SC4, SC8 — the seam (38 tests, new file):** ``` $ cd services/reviewer && bun test src/failure-alert.test.ts 38 pass 0 fail 85 expect() calls Ran 38 tests across 1 file. [154.00ms] ``` - **AT1** — `AT1: a pre-submit failure creates an ask naming the repo, PR, and error class` - **AT3** — `AT3: a burst on one (PR, class) produces exactly one ask` (20 attempts -> 1 ask) - **AT4** — `AT4: 5 distinct PRs with one class read as a systemic condition, not 5 one-offs` (5 failures -> 3 asks, the third flagged systemic, the first two not overclaiming) - **AT5 / SC8** — `AT5/SC8: a failure already owned by the circuit breaker does NOT double-alert` - **AT6** — `AT6: a boot_recovery-stage failure alerts on the same seam, carrying its stage` - **AT7 / SC7** — `a thrown failure yields conclusion=failure with no fabricated round`, plus `a numeric round renders byte-identically to the pre-change output` as the regression guard on the existing path - **AT2** — the negative control below **SC5 (no backfill)** — nothing to run: forward-only by construction, no migration, no backfill script. The 88 rows are historical and their PRs are resolved. **SC6** — `[sc6-deferred: mt#4881]` — the live before/after needs the deployed service; discharged post-merge in §10, see Live verification. Negative control — AT2: the tests can fail, and did. Restoring the **full** pre-mt#4881 behavior (write the outcome row, alert nobody) rather than reverting a single line, per the mt#4512 discipline: ``` $ MT4881_NEGATIVE_CONTROL=1 bun test src/failure-alert.test.ts (fail) recordReviewFailure > AT1: a pre-submit failure creates an ask naming the repo, PR, and error class (fail) recordReviewFailure > AT3: a burst on one (PR, class) produces exactly one ask (fail) recordReviewFailure > a DIFFERENT error class on the same PR is not suppressed by the first (fail) recordReviewFailure > AT4: the same class across 5 distinct PRs reports a systemic condition (fail) recordReviewFailure > AT5/SC8: a failure already owned by the circuit breaker does NOT double-alert (fail) recordReviewFailure > AT6: a boot_recovery-stage failure alerts on the same seam, carrying its stage (fail) recordReviewFailure > SC3: an empty message still alerts, classified as the empty class (fail) recordReviewFailure > fail-open: an emitter that throws does not propagate (fail) recordReviewFailure > fail-open: a DB that throws on the aggregation query does not propagate 25 pass 9 fail ``` The 25 that still pass are the pure-function tests (classifier, coordinate extraction, aggregation, check-run payload), which correctly do not depend on the emit. The scaffold was removed before commit. **What this control does NOT buy:** it ran in the real runtime against the real code, and it reverted the whole fix rather than one line — but it establishes nothing about coverage of the failure **class**. That axis is covered by the replay below, which is why the replay exists. **Full reviewer suite — no regression:** ``` $ cd services/reviewer && bun test 2426 pass 0 fail 5312 expect() calls Ran 2426 tests across 92 files. [4.57s] ``` **Repo-wide lint at the CI gate, and format:** ``` $ bun run lint:strict # eslint . --max-warnings=0 LINT_EXIT=0 $ bun run format:check FORMAT_EXIT=0 ``` **Typecheck** — clean across all 8 projects (`.`, `packages/domain`, `packages/shared`, `services/reviewer`, `services/site`, `src/cockpit/web`, `tsconfig.hooks.json`, `tsconfig.scripts.json`), validated against the session workspace. `infra/tsconfig.json` skipped with its documented reason. **On the gated runner, stated plainly so it is not over-read:** `bun scripts/run-tests-gated.ts` reports `Ran 0 tests across 0 files` and `all test steps passed` for this diff. That is correct and expected, not coverage — `run-tests-main.ts`'s `ROOTS` does not include `services/`, so the gated runner structurally cannot execute a reviewer test. CI runs `services/reviewer` in its own job (`.github/workflows/ci.yml:194`), which is what the 2426-test run above corresponds to. Do not read the gated pass as evidence about this change. ## Live verification `services/reviewer/scripts/replay-failure-alerting.ts` is the verification artifact. It is **read-only** — no row written, no Ask created, no GitHub call — and replays the real historical `failed_at_reviewer` population through the exact shipped classifier and `aggregatePriorFailures`, rather than re-deriving the rule locally (a replay that re-implements the rule measures the copy, and the copy is what drifts). Run here over an **88-row production export** of the same 30-day window the spec measured, via the `--fixture` path (this session has no Postgres URL in env — probed: `MINSKY_PERSISTENCE_POSTGRES_URL`, `MINSKY_SESSIONDB_POSTGRES_URL`, `MINSKY_POSTGRES_URL`, `DATABASE_URL` all absent; the sanctioned loader is a secret-emitting script and is guard-blocked from direct invocation, so the rows were exported through the Supabase MCP instead): ```json { "source": "fixture:...mt4881-fixture.json", "windowDays": 30, "failuresReplayed": 88, "rowsWithoutUsableCoordinates": 0, "classification": { "distribution": { "provider_timeout": 19, "provider_unavailable": 18, "tls_self_signed": 16, "provider_credits_exhausted": 12, "github_submit_rejected": 9, "github_diff_too_large": 4, "provider_token_limit": 4, "unclassified": 2, "network_socket_closed": 2, "unclassified_empty": 2 }, "unclassified": 4, "coveragePct": 95.5 }, "alerting": { "suppressionWindowMinutes": 60, "systemicDistinctPrThreshold": 3, "wouldAlert": 37, "wouldSuppress": 51, "systemicAlerts": 5, "asksPerFailure": 0.42, "upperBoundCaveat": "excludes the circuit-breaker suppression; production volume is lower by the github_submit_rejected count" } } ``` Two things this establishes that no unit test could: the classifier names **95.5%** of the real corpus with **0** rows whose coordinates could not be extracted, and the dedup rule turns 30 days of failures into **37 asks**, not 88. `wouldAlert` is an upper bound — the replay cannot see the circuit-breaker check, which removes the 9 `github_submit_rejected` in production. **UNVERIFIED — the end-to-end live exercise (SC6) is deferred to §10 post-deploy, because the ask is created by the deployed service against the live asks substrate and no failing review has been induced against it yet.** Deploy-SUCCESS will not settle this: the alert path is fail-open by design, so a wired-but-broken emitter and a healthy one are indistinguishable from the deploy signal. Post-merge I will induce a pre-submit failure and confirm an operator ask appears, and report that result rather than the deploy. Deploy verification: this PR changes deploy surface — `isDeploySurfaceFile` returns true for all 8 changed files (run over the actual diff, not recalled). Post-merge I will wait on the deployment bound to this merge (`notBefore` = merge time, `expectCommitSha` = merge SHA), read `buildIdentity`, and assert the health body's service identity rather than the status code. ## Coordination **mt#2719** ("Reviewer auth-health: surface sustained GitHub auth failure as a cockpit operator Ask") was absent from the spec's duplicate check and substantially overlaps this: its 2026-07-31 extension scans the same `review_error` stream for sustained provider failure, covering ~30 of these 79 rows. Neither subsumes the other, so they coordinate — this task owns per-failure emission and the aggregation facts; mt#2719 owns the global health trackers (`auth-health.ts` is not per-PR and writes no `failed_at_reviewer` row) and the severity/paging escalation built on those facts, which this task deliberately does not set. A dependency edge and a coordination note were recorded on both specs. Both tasks add a method to `AskEmitter`; landing this first avoids two concurrent edits to that interface. **mt#4118** was being planned concurrently by another agent during this work. Zero file overlap (its scope is `src/adapters/shared/commands/` and the merge-coordination skill), and its `## Out of scope` names mt#4881 explicitly — reconciled in both directions. Two of its findings are adopted here: that `error_details.message` is best-effort (2 of 143 rows are empty, the mt#2465 class — the classifier degrades to `unclassified_empty` rather than assuming a message), and its independent reading of ADR-030, which corroborates SC7's placement. **Open PR #3412** (mt#4639) touches `ask-emitter.ts`, `server.ts`, `sweeper.ts`, `auth-health.ts` as a mechanical `getLoggableErrorSummary` conversion. It is `mergeable_state: dirty` and 6 days old. Textual conflict risk only — it changes logging-call arguments, not control flow — and this change's primary seam files (`failure-alert.ts`, `webhook-events.ts`, `review-finalize.ts`, `check-run-publisher.ts`, `boot-recovery.ts`) are untouched by it. Full planning audit, corrected measurements, per-criterion gate verdicts and ref-drift dispositions: mt#4881. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_0146ufGg5jbbdFTUP37MCeFE Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…to run
## Summary
`runReview` returns `{ status: "skipped", reason: "concurrent_inflight" }` from `review-worker.ts:508` — **before** `review-finalize.ts`, the only path that publishes on the reviewed and errored outcomes. So a refusal set no check-run conclusion at all, and `session_pr_wait-for-review` reported `reviewerCheckRunState: { status: "absent" }`.
That is byte-identical to genuine reviewer silence, which is a documented **bypass-merge condition**. An agent walking the documented ladder honestly arrives at "reviewer absent >5 minutes" on a PR the bot REFUSED rather than missed — mem#1093, PR #3107: three consecutive waits over ~35 minutes, every one reading `absent`, on a healthy service. Nothing bad happened only because both bypass paths' preconditions incidentally held.
This is the **last leg of ADR-030 follow-up 1**. `review-finalize` already published on the errored outcome; mt#4881 took the thrown-review leg this morning; this takes the skip.
## Key changes
- **`publishTerminalCheckRunSafe`** (`server.ts`) — one function for both terminal outcomes that never reach `review-finalize.ts`: a review that THREW (mt#4881) and one that was DECLINED (this task). One function because it was one defect: neither set a conclusion, so both read as `absent`. Fail-open — a check-run write must never affect the review path, and fork PRs legitimately lack the permission.
- **`buildCheckRunPayload` gains `skipReason`** → conclusion `skipped`. The branch sits **above** the `blockingCount` derivation deliberately: a declined review carries no findings, so falling through would emit a green `success` asserting the code was reviewed when nothing looked at it. The round-1 negative control below confirms that is exactly what happens without it.
- **The summary rules the bypass out in words** rather than describing the state, because its reader is partway down a ladder whose next documented step is a bypass merge. The reason-specific remedy is asserted only for the reason it is true of (see Round 2).
- **`/implement-task` §9's zero-review branch** gains a step that reads the check-run conclusion **before** retriggering. The old ordering wasted a round: `skipped` means declined, `failure` means ran-and-failed (mt#4881), and only `absent` is consistent with silence — and even then, confirm through mt#4118's delivery record, whose `verdict.isSilence` is the authoritative discriminator.
**Nothing below the reviewer changes.** `packages/domain/src/repository/github-checks-run.ts:84` already accepts `skipped`, and **mt#1307 (DONE)** taught pr-watch the full conclusion enum — so the one downstream consumer of a new conclusion value already understands it.
Planning audit, the corrected line references, and the gate walk: mt#4271.
## Why `skipped`, with the vendor citation
`minsky-reviewer/findings` is a **required** check (`resolution-note-guard.ts:19`, `:232`), so the conclusion must not block the PR. GitHub, *About protected branches*, verbatim:
> Required status checks must have a `successful`, `skipped`, or `neutral` status before collaborators can make changes to a protected branch.
Both `skipped` and `neutral` satisfy a required check — they are **symmetric** for this purpose, contrary to the spec's original framing that they "are not symmetric and [this] has changed over time." `skipped` is chosen on semantics: the state being reported is a skip. `failure` is ruled out because it would turn a transient refusal into a merge blocker, which is worse than the silence this ends. The REST reference (`docs.github.com/en/rest/checks/runs`) enumerates the values but says nothing about branch-protection interaction, so it is not the page that answers this.
## Round 2 — reviewer findings addressed
Both blocking findings from review `5086970933` were correct.
**R1-1 (blocking) — no test proved the server's skip branch actually invokes the publisher.** This is the caller-direction check `/implement-task` §7 item 8 names, and I had skipped it: the payload builder was unit-tested and the wiring was not — precisely the shape that ships a publisher nobody calls while every payload test stays green, reproducing the `absent` state this change exists to end.
The seam has to sit **above** `createOctokit`, not below: the real path mints an App installation token no test holds, so a seam placed after that call is unreachable and would verify nothing. `createApp` gains an optional `terminalCheckRunPublisher`, undefined in production.
**Class, not instance:** mt#4881's `publishFailureCheckRunSafe` had the identical untested-wiring gap two functions up, so both now route through one `publishTerminalCheckRunSafe` rather than only mine gaining a seam. That is a behavior-preserving refactor of code merged this morning; its own suite (`failure-alert.test.ts`) passes unchanged.
**R1-2 (blocking) — the skip summary asserted `concurrent_inflight`'s remedy for every reason.** "Clears on a new head; a retrigger is refused" is true of one reason and nothing else yet; stating it for a future one — a permission, a rate limit — would send an operator confidently the wrong way, which is the same shape of error as the `absent` this whole change fixes, one layer up. The remedy is now conditional; the reason-independent half stays unconditional.
**R1-3 (non-blocking) — brittle phrasing assertions.** Narrowed from four exact phrases to the two carrying the contract, and replaced the rest with a test of the actual invariant: the remedy appears for `concurrent_inflight` and not for another reason.
## Testing
Typecheck: 0 errors across 8 projects, validated against the session workspace. Lint: 0 errors, 0 warnings over 4,316 files.
Execution evidence:
```
$ bun --cwd services/reviewer test --preload ../../tests/setup.ts \
src/check-run-publisher.test.ts src/server.test.ts src/failure-alert.test.ts
103 pass
0 fail
216 expect() calls
Ran 103 tests across 3 files. [495.00ms]
# the ten cases this task adds, all passing:
(pass) publishCheckRun: octokitOverride seam > skip path publishes with conclusion 'skipped'
(pass) buildCheckRunPayload: declined-to-run path > conclusion is 'skipped' — not 'failure', which would block a required check
(pass) buildCheckRunPayload: declined-to-run path > conclusion is NOT 'success' — a declined review must not read as reviewed
(pass) buildCheckRunPayload: declined-to-run path > the summary names the reason and marks the state as not-silence
(pass) buildCheckRunPayload: declined-to-run path > the concurrent_inflight remedy is asserted ONLY for that reason
(pass) buildCheckRunPayload: declined-to-run path > no round parenthetical when the round is unknown
(pass) buildCheckRunPayload: declined-to-run path > a real failure outranks a skip when both are somehow set
(pass) buildCheckRunPayload: declined-to-run path > negative control: the same params WITHOUT skipReason still derive normally
(pass) terminal check-run publication wiring > a DECLINED review publishes a skip check-run carrying the reason
(pass) terminal check-run publication wiring > a REVIEWED review publishes no terminal check-run — finalize owns that path
$ bun scripts/run-related-tests.ts services/reviewer/src/server.ts \
services/reviewer/src/check-run-publisher.ts .minsky/skills/implement-task/skill.ts
65 pass
0 fail
5 related test file(s) passed: .minsky/hooks/deploy-surface-detector.test.ts,
.minsky/hooks/require-deploy-verification-before-merge.test.ts,
services/reviewer/src/check-run-publisher.test.ts,
services/reviewer/src/server.test.ts,
tests/domain/implement-task-expected-head-sha.test.ts
```
**Acceptance tests, by the spec's own numbering.**
- **AT4 — RUN and passing.** "A review that proceeds normally still publishes its findings check-run exactly as before, with the same conclusion it produces today." Covered by the pre-existing suite (success / neutral / failure derivations, annotation mapping, convergence summaries) plus two explicit cases: `negative control: the same params WITHOUT skipReason still derive normally` → `conclusion: "success"`, and `a REVIEWED review publishes no terminal check-run`.
- **AT1, AT2, AT3 — UNVERIFIED pre-merge, and not for want of trying.** All three require a check-run actually published by the DEPLOYED reviewer against a contended in-flight marker. Two probes, both negative: publishing one needs the reviewer App's installation token, which the service mints internally via `createOctokit(cfg)` and the agent does not hold; and `forge_branch_protection_get` on `main` returns `Resource not accessible by integration` — this integration cannot read the required-checks list at all. **Deferred to the §10 post-deploy live exercise, not treated as done.** `[at1-deferred: mt#4271]` `[at2-deferred: mt#4271]` `[at3-deferred: mt#4271]`
**Success criteria.** SC1 (vendor citation recorded in `## Context`, choice justified against the must-not-block requirement) — done, quoted above and in the spec. SC2 (the skip publishes a check-run naming the reason) — implemented, unit-covered, and as of round 2 its **wiring** is covered too; its live half rides with AT1. SC4 (a live marker still produces this state, an expired one no longer can) — composes with mt#4267, DONE; nothing here touches the acquire path. SC5 (§9's ladder names this state as distinct from silence, against PR #3114's merged text) — done, written against the current merged §9.
**SC3 is the one I cannot fully satisfy, and it says so explicitly:** *"The published conclusion does not fail the required `minsky-reviewer/findings` check — demonstrated against branch protection, not asserted."* Branch protection is unreadable from here (probe above). The strongest evidence available, and it is live rather than asserted: **PR #3504 merged** at 2026-08-31T02:12:38Z with its head `a6633cb92dcd6d04b86c2bde55bef3ef399297ed` carrying `minsky-reviewer/findings` at `conclusion: neutral`, and `forge_check_runs_list` on that sha reports 13/13 passed, 0 failed. A non-`success` conclusion from the same documented sentence already does not block a merge in this repo. That is a class argument, not the direct `skipped` observation SC3 asks for — hence `[sc3-deferred: mt#4271]`, discharged at §10.
Negative control — round 1, the conclusion branch: reverted the whole `skipReason` branch from `buildCheckRunPayload`'s derivation, back to `failureSummary || blockingCount > 0 ? "failure" : deriveConclusion(levels)`. Three tests went red, and the received value is the dangerous one rather than merely absent — `Expected: "skipped" Received: "success"`, i.e. a green check-run claiming a review that never ran. Restored.
Negative control — round 2, the wiring: removed the `publishSkipCheckRunSafe` call from the skip branch in `server.ts` — the whole of R1-1's fix at the call site — and re-ran. The new wiring test failed with exactly the gap the reviewer named:
```
Expected length: 1
Received length: 0
(fail) terminal check-run publication wiring (mt#4271) > a DECLINED review publishes a skip check-run carrying the reason
26 pass
1 fail
```
Restored; 103/103. Note the sibling control stayed green through this, which is what makes the pair meaningful rather than one assertion twice.
## Live verification
**UNVERIFIED — the live exercise is deferred to §10 post-deploy, because the code that publishes the check-run only runs inside the deployed reviewer service and requires a contended in-flight marker to reach.** This integration is NOT confirmed working until that §10 exercise runs and succeeds.
Probes run rather than assumed: the reviewer App's installation token is minted inside the service (`createOctokit(cfg)`) and is not reachable from the agent; `forge_branch_protection_get main` → `Resource not accessible by integration`. Neither AT1's publish nor SC3's branch-protection read is available pre-merge.
What §10 will do: after the reviewer deploy lands, force a `concurrent_inflight` skip (two triggers against one head while the marker is held) and assert (a) a `minsky-reviewer/findings` check-run exists on that sha with `conclusion: skipped`, (b) its output names `concurrent_inflight`, (c) `session_pr_wait-for-review` reports a `reviewerCheckRunState` that is not `absent`, and (d) the PR's merge state is not blocked by it.
## Deploy verification
`isDeploySurfaceFile` run over this PR's actual changed files: **true** for `services/reviewer/src/check-run-publisher{,.test}.ts` and `services/reviewer/src/server{,.test}.ts`; **false** for the two skill files. This is a **reviewer-service** deploy-surface PR and does not claim `[no-deploy-impact]`.
Post-merge I will run `deployment_wait-for-latest` for `reviewer` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, read `buildIdentity`, and — since the reviewer is an image-source service where that comes back `indeterminate` — correlate `deploy-reviewer.yml`'s workflow run against the merge SHA and assert the health body's service identity. Then the §10 live exercise above.
## Coordination
**PR #3412 (mt#4639)** is open and touches `services/reviewer/src/server.ts`, which this PR also touches; its 313-file list was read via `get_files` and it does **not** touch `check-run-publisher.ts` or `review-worker.ts`. Its changes are mechanical `err.message` → `getLoggableErrorSummary` rewrites at log sites and do not occupy the skip branch — a rebase risk, not a correctness one. **PR #774 (mt#1263)** touches only `review-worker.test.ts` and `eslint.config.js`; its new `runReview` end-to-end tests all take the normal review path with the marker uncontended, so the skip return is untouched by them.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01U6kmDQENKCHPspxL5CZ5ks
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…e, not just the marker ## Summary The reviewer could already record that it was structurally unable to review; it could not tell anyone. mt#4881 shipped the classified failure stream and an inbox ask. This adds the tier above it: the two conditions **only the operator can clear** — sustained GitHub App auth failure, and sustained provider credit exhaustion — now create a `severity: "incident"` + `forceImmediate` ask **and page the principal**, with the remediation URL in the notification body. Originating incident: mt#3433 — ~4h of reviewer downtime the principal found by reading chat scroll, because remediation (add credits) was operator-only by construction. ## The defect this had to work around Setting `severity: "incident"` on the reviewer's emit path **would have paged nobody.** - `CreateAskInput` carries `severity` and `forceImmediate` (`packages/domain/src/ask/repository.ts:262,270`), so the naive change typechecks and looks complete. - The page is fired by `pagePrincipalForAsk`. A repo-wide grep finds exactly **one** non-test importer: `src/adapters/shared/commands/asks.ts`. - That call sits inside `createAsk`, after `persistRouteOutcome` — not in `repo.create`. Neither `create` implementation calls anything paging-related. - The reviewer's `DomainAskEmitter` calls `repo.create` **directly** and never calls `createAsk`, by design. So the marker would have been written to a row no paging code reads: typechecks, deploys, returns healthy, inert in production. The mt#2435 shape. Two findings that shaped the fix: the reviewer's original reason for bypassing `createAsk` is itself stale (mt#3491 made an explicit `routingTarget: "operator"` win over the kind→target default), and mt#3851 already forces operator routing for any `severity: "incident"` ask. But `createAsk` lives in the adapter layer, which the reviewer does not import — hence the extraction rather than a call. ## Key changes - **`packages/domain/src/ask/principal-page-dispatch.ts` (new)** — the production page dispatch, moved out of `asks.ts` so both producers reach one seam. A pure relocation: every collaborator (`notifyPrincipal`, `resolvePersistenceProvider`, `emitSystemEventFromProvider`, `pagePrincipalForAsk`) was already domain-side. `createAsk`'s behaviour is unchanged — it calls the extracted function exactly where it called its private copy. - **`AskEmitter.emitOperatorIncidentAlert`** — one method, one discriminated `OperatorIncidentContext`, for both sources. See the SC1 amendment in the spec for why this replaced the spec's named `emitAuthHealthAlert` plus a near-identical sibling. - **`auth-health.ts`** — pages on trip, deduped by the tracker's existing `tripped` flag (no new dedup state). Additive to the mt#2717 alert sink; the three surfaces degrade independently. - **`failure-alert.ts`** — escalates an operator-actionable class on the threshold **crossing**, evaluated *before* mt#4881's per-PR suppression so a single-PR outage still reaches the count that proves it sustained. `provider_unavailable` / `provider_timeout` never page — they self-heal. - **`services/reviewer/scripts/smoke-operator-incident-page.ts` (new)** — dry by default; `--execute` persists a real ask and sends a real page. Thresholds reuse mt#4881's 60-minute window and auth-health's count of 3 rather than minting a third number for the same judgment (SC8). ## Spec criteria | Criterion | Evidence | | --- | --- | | SC1 (amended) | `emitOperatorIncidentAlert` + `DomainAskEmitter` impl; deviation recorded in the spec | | SC2 | `configureGithubAuthHealthAskEmitter` called at boot, `server.ts` | | SC3 | one-shot via the tracker's `tripped` flag; fail-open on no-repo / throwing repo | | SC4 | 13 emitter tests, 2 auth-health tests, 9 escalation tests, 7 dispatch tests | | SC5 | consumes mt#4881's `ReviewFailureClass`; no second scan over `review_error` | | SC6 | AT3 + the live smoke below | | SC7 | AT5 + the regression test on the rendered page body | | SC8 | `PROVIDER_ESCALATION_THRESHOLD` derivation in its docblock | ## Testing Execution evidence: ``` $ bun test --preload ./tests/setup.ts packages/domain/src/ask/principal-page-dispatch.test.ts 7 pass / 0 fail / 12 expect() calls — Ran 7 tests across 1 file. [197.00ms] $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/ask-emitter.test.ts 13 pass / 0 fail (AT3, AT5, and the page-body regression) $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/failure-alert.test.ts 47 pass / 0 fail / 121 expect() calls (AT4 + 9 new escalation cases) $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/auth-health.test.ts 18 pass / 0 fail (AT1, AT2) $ cd services/reviewer && bun test --preload ../../tests/setup.ts 2446 pass / 0 fail / 5369 expect() calls — Ran 2446 tests across 92 files. [9.40s] $ bun scripts/run-related-tests.ts src/adapters/shared/commands/asks.ts packages/domain/src/ask/principal-page-dispatch.ts 685 pass / 0 fail — Ran 685 tests across 38 files. [22.53s] (includes asks.severity-page.test.ts, which covers the block that moved) ``` AT1/AT2 — auth-health trip with an emitter creates exactly one operator-routed ask; with no emitter wired it still logs and does not throw. AT3 — see the negative control below. AT4 — a sustained `provider_credits_exhausted` run crosses the threshold and emits exactly one incident; a `provider_unavailable` run 8 long emits none. AT5 — the remediation URL is asserted by substring, in the rendered page rather than only the ask. Negative control — AT3, the dispatch is what pages: Reverted the `await dispatchPrincipalPage(repo, ask, this.pageDeps)` call and re-ran: ``` Expected length: 1 Received length: 0 (fail) DomainAskEmitter.emitOperatorIncidentAlert (mt#2719) > actually pages — the assertion the whole task turns on 11 pass / 1 fail ``` The other 11 still passed, which is the defect in miniature: the ask is created, correctly marked `severity: "incident"`, `routingTarget: "operator"` — and nothing pages. Negative control — the escalation must precede the suppression: Moved the escalation after mt#4881's duplicate-suppression return and re-ran: ``` (fail) recordReviewFailure operator escalation (mt#2719) > escalates even when the ordinary alert is suppressed as a duplicate 46 pass / 1 fail ``` Both files were restored and verified byte-identical before committing. ## Live verification The dry smoke, run against the **real** production Telegram credentials (read into shell variables from the linked Railway project, never printed): ``` $ TELEGRAM_BOT_TOKEN=… TELEGRAM_CHAT_ID=… bun services/reviewer/scripts/smoke-operator-incident-page.ts channel resolved: configured via env PASS (dry): emitter → repo.create → dispatchPrincipalPage → send reached ask.severity=incident routingTarget=operator page title: Incident — needs you ``` `configured via env` confirms the resolution order the design depends on — `resolvePrincipalChannel` reads `TELEGRAM_*` before falling back to Pulumi, which a container cannot do. **This run found a real defect that every unit test missed.** `buildPageMessage` excerpts the ask's question at 300 chars; the remediation URL sat at the end of the body and was cut from the notification the principal actually reads, while remaining present in the ask. SC7 was silently untrue. Fixed by leading with the remediation (not by widening the shared excerpt, which bounds every page and is not one caller's to move), plus a regression test that asserts the rendered page body rather than the ask. **`--execute` is UNVERIFIED, deliberately, and its risky part is exercised.** A full run sends a real notification to the principal's phone, consumes one of the substrate's 3-per-24h page budget (`principal-page.ts` `PAGE_RATE_LIMIT_MAX`), and — since R1 below — writes a real ask row. That is an outward-facing action I have not taken unilaterally; it needs the operator's go-ahead. Per §7a's dual-mode rule the branch's own imports must still resolve at runtime (the mt#2760 class), so that was exercised directly and bounded: ``` $ bun -e 'import "reflect-metadata"; …' factory.resolvePersistenceProvider: function repository.DrizzleAskRepository: function listByClassifierVersion on prototype: function ``` `scripts/verify-ask-principal-page.ts` (mt#3595) already proves the final transport leg end-to-end. ## Review rounds **R1 — BLOCKING, `--execute` used `FakeAskRepository` while announcing a real ask.** Correct finding, and a pointed one: it is this task's own defect class — a path reporting success for work it did not do — reproduced inside the script written to catch that class. Fixed by changing the behaviour rather than the wording (`be53c506e`): `--execute` now resolves the real persistence provider, builds a `DrizzleAskRepository`, and reads the row BACK before claiming success, asserting the persisted `severity` and `principalPagedAt` instead of trusting the emit's return value. It skips cleanly when no provider or connection is available. This also closed the gap the reviewer named — with the fake repo the mode exercised neither the insert nor the `severity`/`forceImmediate` columns, so it could not have caught a persistence-layer rejection. ## External preconditions Verified provisioned, no new provisioning required. `TELEGRAM_CHAT_ID`, `TELEGRAM_BOT_TOKEN` present and `ALERT_SINK_TYPE=telegram` on the production `minsky-reviewer` Railway project (key names projected, values never rendered). Note `infra/index.ts:338-345` declares these conditionally as a per-stack opt-in — a stack without `reviewer-telegram-chat-id` degrades to a logged `PageDecisionReason` rather than failing loudly, which is correct but means "the page fired" must never be inferred from "the ask was created." Deploy verification: this PR touches `services/reviewer/src`, `packages/domain` and `src/adapters/shared/commands` — all deploy surface per `isDeploySurfaceFile` (checked with the predicate, not from memory). After merge I will run `deployment_wait-for-latest` for the reviewer service with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge commit, and read `buildIdentity` rather than treating SUCCESS alone as proof. ## Parallel work Open PR #3412 (mt#4639, IN-REVIEW) touches `ask-emitter.ts`, `auth-health.ts` and `server.ts` — a mechanical `err.message` → `getLoggableErrorSummary` sweep, established from its actual changed-file list. It does not touch `failure-alert.ts`, `principal-page.ts` or `ask/repository.ts`, where the substantive work here lives. Overlap is additive; a watch is armed on #3412. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…jected seams ## Summary mt#4271 shipped the `skipped` check-run for a review the reviewer DECLINES, and merged with three acceptance tests and one success criterion unverified — forcing a skip looked like it needed two colliding agents on a live PR. mt#4895 reframed that correctly: **the contention is a Postgres row, not an interaction between agents**, so the layers separate. This PR closes the two that are reachable, and records a corrected premise for the one that is not. Where the five layers stand after this PR: L1 already covered (twice), **L2 covered here**, L3 covered by PR #3563, **L4 harness shipped here** (live integration run deferred to mt#4897), L5 recorded as unverified with its reason. ## Key changes - **`RunReviewDeps` gains three seams** — `octokitFactory`, `prContextFetcher`, `appIdentityFetcher` — so the marker branch can be driven with no network. Optional fields with real defaults; every existing caller is unaffected. - **`src/runreview-concurrent-inflight.test.ts`** (new) — 4 cases covering the skip return, the log shape, the skip-path timing write, and SC1's negative control. - **`scripts/inflight-skip-harness.ts`** (new, round 2) — the L4 harness's decision core: stdout collection with a completion promise, event extraction, and the pass/fail derivation. Pure functions over values, covered by 14 tests. - **`scripts/smoke-concurrent-inflight-skip.ts`** (new) — the imperative shell around it: spawn, post, poll, clean up. Modelled on `kill-test.ts`, with a `--negative-control` mode. ### Injected, not module-patched — and this was a decision, not a default mt#4895's spec left the seam choice open, conditional on the still-open PR #774 landing its `mock.module` approach. **ADR-036 §2 rule 2 settles it:** where a seam can be added by changing one production file with no exported-type change ("an optional `deps` parameter with a real default counts as no change"), patching is *banned at that site*. That is exactly this site. PR #3563 — the mt#4271 PR merged this morning — used the same shape for `terminalCheckRunPublisher`, with the reasoning this task needs verbatim: *"The seam has to sit above `createOctokit`, not below."* Consequence: **this PR does not depend on PR #774**, and does not touch `review-worker.test.ts`, which is the only file #774 shares. Recorded on mt#1263 as material to the merge decision it is blocked on: #774 is `mergeable_state: dirty` and its ESLint carve-out is the mechanism ADR-036 now prohibits. ### A spec premise this PR corrects mt#4895's `## The precedent for L4` claimed the skip log is emitted "before any GitHub interaction", so an L4 script needs "no real PR, no model call, and no reviewer App token." **One of those three survives.** The marker is keyed on `pr.headSha`, so `fetchPullRequestContext` (`github-client.ts:201`, a real `pulls.get`) necessarily precedes `acquireMarker`. `createOctokit` is *not* the blocker — it is a pure constructor that validates nothing — but the PR fetch is: against a synthetic PR it throws, and `runReview` dies before the marker, so no contention is possible and the log can never fire. Only "no model call" holds. This is the same error SC1 had already corrected one section earlier; the L4 section was never updated to match. Both the spec and the script header now say so. ### The look-alike the tests discriminate `runReview` has TWO returns carrying `status: "skipped"`, and the routing one comes **first** — `decideRouting` short-circuits a Tier-1 PR before the marker exists. Asserting `status === "skipped"` alone would pass for the wrong reason, so every assertion pins `reason`, and the fixture PR body carries the tier-3 marker so routing resolves to `shouldReview: true`. ## Round 2 — reviewer findings addressed All three R1 findings were correct as stated. Two were real defects in the new script; the third is answered below with what changed rather than with an argument alone. **R1-1 (blocking) — racy stdout collection.** Correct, and it would have made SC2/SC3 intermittently wrong in the worst way: a real skip reported as a failure. `collectStdoutLines` returned only the array, with no completion signal, so the script killed the process and parsed `lines` while the background reader was still draining — and a terminal event is exactly what arrives last. It now returns `{ lines, done }`, and the shell awaits `done` after the process exits and **before** anything reads a line. **R1-2 (blocking) — `process.exit` inside the try bypassed cleanup.** Correct. Every failure path inside `main`'s try now throws (`bail`) and a top-level catch sets the exit code after the `finally` has run, so the spawned server is terminated and the Postgres handle closed. **Class, not instance:** the reviewer cited `:320`, but the same shape was on every in-try failure path — all of them converted, not just the one named. The env-gate `skip()` calls deliberately keep `process.exit`, and the code now says why: they run before anything is spawned or connected, so there is no cleanup to bypass. **Both fixes are provable rather than asserted.** The decision logic moved into `scripts/inflight-skip-harness.ts` — ADR-036 §3's functional core / imperative shell, applied to a script — so the harness's own logic is covered by the suite instead of resting on a live run that has not happened. One test reproduces R1-1 directly by asserting the line buffer is **incomplete before** `done` and complete after; without the fix there is nothing to await and the assertion cannot be written at all. **R1-3 (blocking) — missing live-run evidence.** The finding is factually right: there is no live run, and I am not claiming otherwise or asking for it to be waived as a false positive. What changed is how much it covers. - The credentials probe stands (all seven absent, listed below), which is §7a's documented override branch (b), "the author lacks live-target access." - Independently of credentials, delivery A starts a **real** review of a real PR and posts to it. That is a shared-state change on a PR this task does not own — the authorization mt#4271 surfaced rather than took, and it is not mine to grant. - **What was unproven is now much smaller.** R1 was right that shipping a harness whose primary path had never executed is weak. Round 2 covers its decision logic with 14 tests, and the failure path was exercised live end to end (below). What remains unproven is the *integration* — real GitHub, real Postgres, real contention — which is precisely what mt#4897 owns. I have not merged past this. If the reviewer still considers the live run a merge precondition, that is a legitimate call and mt#4897 is the gate; say so and the PR waits. Execution evidence: ``` $ bun test --preload ../../tests/setup.ts src/runreview-concurrent-inflight.test.ts (pass) a HELD marker makes runReview return the concurrent_inflight skip [8.13ms] (pass) the skip records a skip-path timing row (mt#2088) rather than skipping the write [0.27ms] (pass) the skip log carries the delivery id, which is what correlates it to a delivery [0.21ms] (pass) negative control: an AVAILABLE marker does not return the skip — execution passes the gate [1.25ms] 4 pass / 0 fail / 13 expect() calls $ bun test --preload ../../tests/setup.ts scripts/inflight-skip-harness.test.ts # round 2 (pass) collectStdoutLines > R1-1 regression: lines are INCOMPLETE before `done` resolves and COMPLETE after [3.54ms] (pass) collectStdoutLines > flushes a trailing segment that never got a newline [2.40ms] (pass) collectStdoutLines > reassembles a JSON object split across chunk boundaries [2.63ms] (pass) collectStdoutLines > a null stream yields no lines and an already-resolved done [0.05ms] (pass) findEvents > returns only objects whose event matches, in order [0.08ms] (pass) findEvents > non-JSON banner lines are skipped rather than throwing [0.04ms] (pass) deriveVerdict — contention mode > passes when B skipped, A did not, and a conclusion was read back (pass) deriveVerdict — contention mode > a FAILED publish still counts as ATTEMPTED — that is the SC3 contract (pass) deriveVerdict — contention mode > fails when neither publish signal is present — the skip never surfaced (pass) deriveVerdict — contention mode > fails when B did not skip at all (pass) deriveVerdict — contention mode > fails when A ALSO skipped — A holds the marker, so a skip on A means something else took it (pass) deriveVerdict — negative-control mode > passes when no skip is observed, which is the whole point of the control (pass) deriveVerdict — negative-control mode > FAILS when a skip is still observed — the harness cannot discriminate (pass) deriveVerdict — negative-control mode > the publish question is N/A here, not false 14 pass / 0 fail / 27 expect() calls $ bun run test # full reviewer suite 2474 pass / 0 fail / 5431 expect() calls — Ran 2474 tests across 94 files. [4.64s] $ validate_typecheck # 8 projects: root, packages/domain, packages/shared, services/reviewer, # services/site, src/cockpit/web, tsconfig.hooks.json, tsconfig.scripts.json 0 errors (infra/ skipped — deps not installed locally; CI runs it with its own install step) $ validate_lint # services/reviewer, 228 files 0 errors, 0 warnings ``` **R1-2 failure path, exercised live.** Dummy credentials + an unreachable Postgres, so the spawned server never becomes healthy and `bail` fires from inside the try: ``` $ INFLIGHT_TEST_PORT=34612 ... bun scripts/smoke-concurrent-inflight-skip.ts inflight-skip: mode=contention owner=edobry repo=minsky pr=1 port=34612 FAIL: server did not become healthy within 20s script exit=1 $ curl -s -m 2 -o /dev/null -w "http_code=%{http_code}\n" http://127.0.0.1:34612/health http_code=000 # nothing listening — the spawned server is gone ``` **What that does and does not prove.** It proves the throw path reaches the top-level handler and exits 1 with the right message. It does **not** by itself prove `finally` killed the server: the server also failed to boot in this run, so an empty port is consistent with it having exited on its own. The cleanup guarantee rests on control flow that is deterministic rather than probed — `bail` throws, and the `finally` is attached to the same `try`. Recording the distinction rather than letting the port check read as stronger evidence than it is. **Acceptance tests, by mt#4895's own numbering.** - **AT1 — RUN and passing.** *"`runReview` with a held marker returns the skip; with an available marker it does not. Both assertions in one test file, run with no network."* Both halves are in the 4-case run above; no network — the three GitHub calls are injected. - **AT2 — NOT run.** *"the script, run against a local Postgres, reports PASS and names which delivery was skipped. Run it twice."* Needs App credentials that can read a real PR, plus operator authorization to act on one. `[at2-deferred: mt#4897]` - **AT3 — NOT run.** *"Negative control for L4 — with the marker released between the two deliveries, the script observes NO skip."* Implemented as `--negative-control`; its decision half is covered by the two negative-control cases in the harness suite. Same blockers as AT2 for the live half. `[at3-deferred: mt#4897]` - **AT4 — DONE.** *"whatever verdict SC4 reaches is written into this spec's `## Outcome` with its evidence, including the case where it stays unverified."* Written; see `## Live verification`. **Success criteria.** SC1 — done. SC2 — the runnable script is shipped and its decision logic is covered; its live invocation is AT2. SC3 — the script asserts the publish was ATTEMPTED, and the observable is deliberately two-sided: a `minsky-reviewer/findings` conclusion read back off the sha when the publish succeeds, or the `review_skip_check_run_failed` warn when it fails, because both prove the branch ran *through* the publish call rather than returning before it. Three harness tests pin that contract, including the failed-publish case. SC4 — verdict recorded below. SC5 — discharge written to mt#4271's spec; the four markers live in PR #3563's merged **body**, not its spec, which mt#4895's spec now says. (R1 reported SC5 Unverifiable because the mt#4271 edit is outside this diff — correct per the contract; the record is in mt#4271's `## Deferred-marker discharge` section.) **Dual-mode script — both branches exercised (mt#2776).** Running only the safe branch leaves the other's code unexecuted, and imports are hoisted, so each run below proves the whole module — `@octokit/rest`, `@octokit/auth-app`, `postgres`, `@octokit/webhooks-methods`, and now `./inflight-skip-harness` — resolves at runtime: ``` contention, no env -> SKIP: MINSKY_REVIEWER_APP_ID is not set exit 0 --negative-control, no env -> SKIP: MINSKY_REVIEWER_APP_ID is not set exit 0 dummy env, INFLIGHT_TEST_PR=not-a-number -> FAIL: INFLIGHT_TEST_PR must be a positive integer exit 1 dummy env, REVIEWER_PROVIDER=cohere -> SKIP: not one of openai|google|anthropic exit 0 dummy env, unreachable Postgres -> FAIL: server did not become healthy within 20s exit 1 (the R1-2 path) ``` SC1 — negative control: an AVAILABLE marker does not return the skip, and the injected `appIdentityFetcher` is observed called exactly once, proving execution reached the first call past the marker gate rather than merely not skipping. Negative control — production log event, suite liveness: renamed `runReview.skipped_concurrent_inflight` to `runReview.MUTATED_CONTROL` in `review-worker.ts` and re-ran. ``` (fail) a HELD marker makes runReview return the concurrent_inflight skip error: expect(received).not.toBeNull() (fail) the skip log carries the delivery id, which is what correlates it to a delivery error: expect(received).toBe(expected) 2 pass / 2 fail ``` The two that do not assert on the log stayed green, which is what makes the pair meaningful rather than one assertion twice. Restored; 4/4. ## Live verification **UNVERIFIED — the reason is a missing specimen plus an authorization, not a missing mechanism.** Publishing `skipped` shipped with PR #3563 at 2026-09-02T08:05:26Z. All six `concurrent_inflight` occurrences on record predate it, so no historical sha can answer the question — the specimen has to be made, and this PR ships the thing that makes it. Probes run rather than assumed. Checked for presence, never values: `MINSKY_REVIEWER_APP_ID`, `MINSKY_REVIEWER_INSTALLATION_ID`, `MINSKY_REVIEWER_PRIVATE_KEY`, `MINSKY_REVIEWER_WEBHOOK_SECRET`, `MINSKY_PERSISTENCE_POSTGRES_URL`, `MINSKY_POSTGRES_URL`, `OPENAI_API_KEY` — **all seven absent.** `forge_branch_protection_get main` re-probed → `Resource not accessible by integration`, the same result mt#4271 got; that is `verified-1a` for the ForgeBackend channel and `inferred` for the capability, not evidence that no channel can read it. **mt#4897** owns the live run and the L5 observation, and its spec carries both blockers with the probe results. Best evidence standing, unchanged: PR #3504 merged with `minsky-reviewer/findings` at `conclusion: neutral`, 13/13 checks passed. `neutral` and `skipped` are named in the same sentence of GitHub's protected-branches documentation, so a non-`success` conclusion from that sentence demonstrably does not block a merge here — a **class argument, not the direct observation**, which is the distinction SC4 exists to keep visible. ## Deploy verification `isDeploySurfaceFile` run over this PR's actual changed files: **true** for all five. This is a reviewer-service deploy-surface PR and does **not** claim `[no-deploy-impact]`. Post-merge I will run `deployment_wait-for-latest` for `reviewer` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge SHA, read `buildIdentity`, and — since the reviewer is an image-source service where that returns `indeterminate` — correlate `deploy-reviewer.yml`'s workflow run against the merge SHA and assert the health body's service identity rather than the status code. Note the change is seams-plus-tests: the three new `deps` fields are undefined in production, so the deployed behaviour is unchanged by construction. That is a reason to expect a clean deploy, not a reason to skip verifying it. ## Coordination **PR #774 (mt#1263)**, open — `get_files` reads exactly `eslint.config.js` and `services/reviewer/src/review-worker.test.ts`. This PR touches **neither**; the L2 case is a sibling file specifically so the conflict surface stays at zero while #774 sits unmerged (and conflicted). **PR #3412 (mt#4639)**, open — touches `services/reviewer/src/server.ts`, which this PR does not touch. Recent merges, one `git_log --path` per path, both `pathMatched: true`: `review-worker.test.ts` — 0 commits in 7 days; `services/reviewer/scripts` — 6 commits, none colliding with a new filename. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LJGtdNqy7Yq9n6QXkV5fjp Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…m contextRefs ## Summary Two advisory `ask-form-lint` checks judged the `question` body in isolation, so both fired on asks that **already contained the remedy the warning asked for**. Measured in the 2026-09-02 `/calibration-review` window (8 unreviewed records, 9 matches): 2 of the 5 false positives were these. - **`domain-jargon`** warned *"say what it means, or move the reference to contextRefs"* on ask#10650, whose first sentence read *"Decide whether ADR-042 — the record of which planning gates get a mechanical backstop — should be marked Accepted."* The author said what it means, in the same clause. Its `metadata.formWarningDisposition` records the rebuttal contemporaneously, including the second half: ADR-042 is the decision's SUBJECT, so the "move it" branch was unavailable too. - **`unlinkified-reference`** warned *"the reader cannot open them from the ask. Supply the full URL"* on ask#10647, which cited `Notion 34f937f0` in the body and carried `https://app.notion.com/p/34f937f03cb48108a95bdf3813f5ca84` in `contextRefs` — the full URL for that exact id. The two checks gave contradictory advice because neither could see what the other recommended. ## Key changes **`domain-jargon` credits an inline gloss at first use** (`form-lint.ts`). The rule is structural and punctuation-anchored — a parenthetical, or a matched dash-pair apposition — and **requires CLOSURE**. An unclosed opener ("ADR-042 — required measuring its trigger") stays a fire, which is what stops the rule from suppressing the shape the check exists to catch. Deliberately **not** a semantic gloss detector: `form-lint.ts:170-176` already records that an unbounded natural-language surface is the axis ADR-024 assigns to embedding rather than regex. **The ask's own `contextRefs` become a resolution source** (`external-refs.ts`, `asks.ts`). A truncated cued id that is a unique prefix of a Notion URL the ask carries is resolved and the URL appended, so the **persisted body** gains a working link. Two design constraints worth calling out, because they were the whole planning finding: - **The fix is at the TEXT, not at the check.** `asks.ts:1239-1254` (mt#2918, PR #2755 R1) already decided this: *"any present or future check that reads for a URL inherits the same mismatch, so the fix belongs at the text, not at the check."* `FormLintInput` therefore gains **no field**, and the `unlinkified-reference` branch itself is **unmodified**. Teaching the check to look away would have silenced the warning while leaving the body unreadable — defeating mt#2918's stated goal that *"every downstream reader … carries the URL rather than a bare page id."* - **Harvesting is scoped to Notion-hosted URLs.** A Notion page id and a Minsky ask/memory/workspace id are the same shape, so a candidate set built from every id-shaped run could resolve a truncated cue against a Minsky entity and append a dead URL. An ambiguous prefix resolves to nothing rather than guessing. Idempotence across both call paths is carried by harvesting candidates from the text itself as well as from `contextRefs`: `computeFormLintMatches` re-runs the transform with no options over the already-normalized question, and without that second source it would re-report a reference the normalization step had just fixed. `scripts/replay-ask-form-lint-calibration.ts` replays the corpus against the current matchers. It labels each row with body provenance (`as-filed` / `original-content` / `edited-since`) because an ask body is mutable — mt#3584 lost a real false positive to exactly that conflation. ## Testing Execution evidence: **SC1 + SC2 + SC3 — 17 new tests, 100 pass / 0 fail across the three suites** (`bun test packages/domain/src/ask/external-refs.test.ts packages/domain/src/ask/form-lint.test.ts src/adapters/shared/commands/asks.external-refs.test.ts`): ``` (pass) domain-jargon — inline gloss at first use (mt#4901) > AT1: a dash-pair gloss at first use suppresses the ADR/RFC class (pass) domain-jargon — inline gloss at first use (mt#4901) > a parenthetical gloss suppresses it the same way (pass) domain-jargon — inline gloss at first use (mt#4901) > AT3: a bare ADR reference with no gloss still fires (pass) domain-jargon — inline gloss at first use (mt#4901) > AT3: a bare ADR reference mid-body still fires (pass) domain-jargon — inline gloss at first use (mt#4901) > AT4: a possessive use is not a gloss (pass) domain-jargon — inline gloss at first use (mt#4901) > an UNCLOSED dash opener is not a gloss — closure is what bounds the rule (pass) domain-jargon — inline gloss at first use (mt#4901) > the gloss must sit at the FIRST use, not a later one (pass) domain-jargon — inline gloss at first use (mt#4901) > a later bare use is ordinary prose once the term is glossed on introduction (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > AT2: the prefix resolves against a contextRefs URL and the URL lands in the text (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > AT2 negative control: with no refs supplied, the same body still reports it (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > idempotent across call paths: a second pass with NO options reports nothing (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > an AMBIGUOUS prefix resolves to nothing rather than guessing between two pages (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > only a NOTION-hosted URL is a resolution source — a bare id elsewhere is not (pass) linkifyExternalRefs — resolving a truncated cue ... (mt#4901) > collectNotionIdsFromUrls reads app.notion.com and notion.so, and nothing else (pass) createAskWithFormLint — contextRefs as a resolution source (mt#4901) > a short id prefix in the body resolves against a contextRefs URL (pass) createAskWithFormLint — contextRefs as a resolution source (mt#4901) > negative control: the same body with NO contextRefs still warns (pass) createAskWithFormLint — contextRefs as a resolution source (mt#4901) > a contextRef that is not a Notion URL is not a resolution source 100 pass 0 fail Ran 100 tests across 3 files. ``` **AT1** = the first row; **AT2** = the `linkifyExternalRefs` rows plus the `createAskWithFormLint` rows (which read the PERSISTED question back out of the repository, so they assert the URL is in the body rather than only that the warning fell silent — the rejected fix shape would pass the weaker assertion); **AT3** = the two "still fires" rows; **AT4** = the possessive row. **No regression across the ask family** — 950 pass / 0 fail across 52 files, including `form-lint.jargon-and-lede.test.ts` (the mt#4516 suite that owns this check): ``` 950 pass 0 fail Ran 950 tests across 52 files. [11.66s] ``` **Typecheck** — 0 errors across 8 projects (`.`, `packages/domain`, `packages/shared`, `services/reviewer`, `services/site`, `src/cockpit/web`, `tsconfig.hooks.json`, `tsconfig.scripts.json`). **Lint** — 0 errors, 0 warnings over 4324 files. Negative control: reverted all three source files (`external-refs.ts`, `form-lint.ts`, `asks.ts`) via `git stash push`, leaving the tests in place, and observed the new tests FAIL. ``` (fail) createAskWithFormLint — contextRefs as a resolution source (mt#4901) > a short id prefix in the body resolves against a contextRefs URL (fail) domain-jargon — inline gloss at first use (mt#4901) > AT1: a dash-pair gloss at first use suppresses the ADR/RFC class (fail) domain-jargon — inline gloss at first use (mt#4901) > a parenthetical gloss suppresses it the same way (fail) domain-jargon — inline gloss at first use (mt#4901) > a later bare use is ordinary prose once the term is glossed on introduction SyntaxError: Export named 'collectNotionIdsFromUrls' not found in module '.../external-refs.ts' 69 pass 5 fail Ran 74 tests across 3 files. ``` Reported honestly rather than as a clean 17-for-17: the `external-refs.test.ts` file does not LOAD against the pre-fix tree (it imports a symbol the fix introduces), so its 6 tests are *unrunnable* there rather than *failing*. The 4 named failures are the real control. Note also that the recall-floor tests (AT3, AT4) pass **both** before and after — correct, since they must not change. This is a full revert of the fix, not a one-line revert (mt#4512). ## Live verification **AT5 / SC4** — replayed over the real 51-record corpus at `~/.local/state/minsky/projects/a0809beec3ba7e98/ask-form-lint-calibration.jsonl`, against the live ask store: ``` Corpus: 51 records; 5 carry domain-jargon / unlinkified-reference 2026-08-27T15:11:32.568Z ask#10647 [as-filed] domain-jargon: STILL FIRES; unlinkified-reference: cleared 2026-08-27T15:12:28.035Z ask#10650 [edited-since] domain-jargon: cleared 2026-08-27T15:23:43.123Z ask#10657 [original-content] domain-jargon: STILL FIRES 2026-08-27T15:29:45.684Z ask#10662 [original-content] domain-jargon: STILL FIRES 2026-08-31T02:06:38.515Z ask#11095 [as-filed] domain-jargon: STILL FIRES cleared: 2 still fires: 4 unfetchable: 0 ``` **cleared: 2** are exactly the two matches classified false in the calibration window. **still fires: 4** are the three true positives plus ask#10647's `ask-kind name` class, which the pass classified *uncertain* and this change deliberately does not touch. 0 unfetchable, so every record was actually re-judged. `[edited-since]` on ask#10650 is the honest label: that ask was edited after its record, and the edit touched `metadata` only (its `editHistory` records `fields: ["metadata"]`), so the replayed question is the judged text. **One thing the replay could not do from this session:** `resolveCalibrationLogDir` keys on the current working directory, so run from a session workspace it resolves to that workspace's own empty project key. That is **mt#4885**, which already owns the general fix; the script takes an explicit `--log` override rather than re-solving it here, and refuses to silently report a clean zero. ## Deploy verification `isDeploySurfaceFile` returns **true** for 6 of the 7 changed files — run over this diff, not ``` true packages/domain/src/ask/external-refs.ts true packages/domain/src/ask/form-lint.ts true src/adapters/shared/commands/asks.ts true packages/domain/src/ask/external-refs.test.ts true packages/domain/src/ask/form-lint.test.ts true src/adapters/shared/commands/asks.external-refs.test.ts false scripts/replay-ask-form-lint-calibration.ts ``` So this is a deploy-surface PR and no `[no-deploy-impact]` claim is made. Post-merge I will run `deployment_wait-for-latest` with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge commit, and read `buildIdentity` rather than treating SUCCESS alone as verification. No new external-system integration: no new permission, scope, credential, outbound host, or webhook. ## Notes - **Coordinates with mt#4389**, which owns `missing-force-immediate` in the same file — adjacent branches, different causes. Not merged into this change; that task additionally carries a reserved `SEVERITY_TRANSPORT_CHECK_KINDS` question for the operator. - **Open PR #3412** also touches `src/adapters/shared/commands/asks.ts`. Verified line-disjoint against its real diff: 8 single-line edits (an import plus seven catch-block `error:` lines), none inside `normalizeQuestionForLint`, `validateFormLintNotViolated`, or `createAskWithFormLint`. Whoever lands second rebases. - **Unrelated flake observed:** the first push was blocked by `src/cockpit/sweepers.test.ts > an overrunning tick is not run concurrently with the next tick (mt#4335)` (`maxInFlight` 2 vs 1). It passes 3/3 in isolation and passed on the gated re-run that let this push through; it fails only under the 64-file parallel partition. Not caused by this diff — no file it touches is in this change — and no task currently owns it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_017FKnD9tAMUFGYWg36keBvz Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 4, 2026
…eturns results
## Summary
`knowledge search --sources <x>` builds `filters: { sourceName: params.sources }`, and `PostgresVectorStorage` renders a filter key as a bare column name — but `sourceName` is a member of `knowledge_embeddings.metadata`, **not a column**. Postgres folds the unquoted identifier to `sourcename` and raises 42703, and the command's `catch` turns that throw into `{ chunks: [], degraded: true }`.
Reproduced against prod rather than inferred:
```
SELECT count(*) FROM knowledge_embeddings WHERE sourceName = 'minsky-design'
ERROR: 42703: column "sourcename" does not exist
```
Found by mt#4937's audit of the shared vector-search path, which was looking for a *recall* defect and established that one is latent. This is the different, live defect in the one caller that passes filters at all.
## Correction found while implementing — this fix is necessary and NOT sufficient
Verifying the end-to-end command showed that `knowledge.search` returns `{ chunks: [], backend: "none", degraded: true }` for **every** query — filtered *and* unfiltered:
```
knowledge_search { query: "minsky architecture", sources: ["minsky-design"] } → backend: "none", degraded: true
knowledge_search { query: "minsky architecture" } → backend: "none", degraded: true
```
That is a *different, prior* defect: `registerKnowledgeCommands(targetRegistry, deps?)` reads its vector storage from `deps`, and the only production caller (`src/adapters/shared/commands/index.ts:135`) invokes it with **no arguments**. So the command has never reached the vector store at all, and the 42703 sits behind that. Filed as **mt#4946** — the mt#2508 caller-direction wiring defect in its purest form.
**This does not weaken the fix here.** The 42703 is verified by direct SQL against the real schema and would fire on every `--sources` query the moment the wiring lands; mt#4944 merging first is what keeps that from happening. But it does mean the task's own criterion 1 is not satisfiable by this PR, and it has been amended in the spec to say so rather than left to read as covered.
`[sc1-deferred: mt#4946]`
## Key changes
**1. `buildFilterConditions` gains a dotted key form.** `metadata.sourceName` renders `metadata->>$n` with the **member bound as a parameter**, so only the column half is ever SQL text and `rawIdentifier` still guards it. A key with more than one dot is refused rather than guessed at — nested access needs `#>>` and a path array, and there is no caller for it.
This stays domain-agnostic per ADR-013 — *"it filters by whatever column is named"* — widening "column" to "column or JSONB member". No knowledge-specific concept enters that layer.
**2. Array values do set membership.** `{ k: ["a","b"] }` now renders `k IN ($1,$2)` instead of binding an array to a scalar `=`, which matches nothing and reports no error — the same silent-zero shape. Empty array emits no predicate, symmetric with the existing `*Exclude` branch. **The caller's `z.array(z.string())` signature is unchanged**: planning ruled out narrowing it to a scalar, since that is a command-parameter contract change with four consumers and unrelated to the defect.
**3. The catch carries its reason.** `KnowledgeSearchResponse` gains optional `degraded` / `degradedReason` / `backend`, populated via `getLoggableErrorSummary` — **not** `getErrorMessage`, because a `DrizzleQueryError`'s `.message` is `Failed query: …` and the actual PG error (here the 42703) lives on `.cause`. A reason that omits the diagnosis satisfies the criterion's letter and none of its point.
## Planning narrowed the fix shape, and ADR-013 is why
Criterion 2 arrived as a free three-way menu (JSONB predicate / domain post-filter / promoted column). ADR-013 decides it: its prohibition — *"do not denormalize the mutable field into the shared index"* — is scoped to a **mutable** field copied from another table's source of truth, and it explicitly preserves the store's capability. `sourceName` is written at index time from the document itself with no second row to drift from, so none of the three scope conditions bind.
## Coordination
Touches `src/adapters/shared/commands/knowledge/index.ts` at the same `catch` line **PR #3412** rewrites. That PR is `mergeable_state: dirty` and parked on an operator decision about its unreviewable 313-file diff, so waiting would block a live defect indefinitely. The conflict is now nearly content-identical: #3412 converts that line to `getLoggableErrorSummary`, which this PR also does, for its own reason.
## Testing
**Execution evidence:**
```
bun test --preload ./tests/setup.ts packages/domain/src/storage/vector/postgres-vector-storage.test.ts
23 pass / 0 fail / 64 expect() calls (was 17 before this change)
```
Across all four affected suites, including `sql-generation-proof.test.ts` — which reaches this module only through a dynamic import and which the related-test selector therefore cannot see (mt#4945, filed for that gap and applied here by running it explicitly):
```
65 pass
0 fail
228 expect() calls
Ran 65 tests across 4 files. [200.00ms]
```
Related-test gate on all three source files: 335 / 17 / 207 tests, all passing. `lint:strict` (`--max-warnings=0`) 0 errors 0 warnings; typecheck 0 errors across 8 projects; format clean.
**SC1** — `[sc1-deferred: mt#4946]`. Not satisfiable here; see the correction section above. The evidence this task can produce is below.
**SC2** — the dotted form, narrowed to ADR-013's default at planning, mapping recorded in the spec's `## Planning Audit (READY)`.
**SC3** — the `IN` branch; signature deliberately unchanged.
**SC4** — `degradedReason` on the response, via `getLoggableErrorSummary` so it carries the PG error rather than Drizzle's wrapper.
**AT2 / AT3 — the predicate executed live, read-only, against prod.** The fixed shape against the same table that raises 42703 for the old one:
```
SELECT count(*) FROM knowledge_embeddings WHERE metadata->>'sourceName' IN ('minsky-design')
→ 111
SELECT count(*) FROM knowledge_embeddings WHERE metadata->>'sourceName' IN ('definitely-not-a-source')
→ 0 (no error — an empty result that is genuinely empty)
```
111 is the full corpus, the expected count: `knowledge_embeddings` holds 111 rows carrying exactly one distinct `sourceName`. The two cases are distinguishable at the SQL layer, which is the layer this PR changes.
Negative control: reverted BOTH new mechanisms — `filterTarget`'s dotted form and the array `IN` branch — and re-ran.
```
17 pass
6 fail
(fail) JSONB member targets > a dotted key targets a JSONB member, with the member name BOUND not interpolated
(fail) JSONB member targets > a key with more than one dot, or an empty member, is refused rather than guessed at
(fail) array set membership > an array value renders IN, not a scalar equality against an array
(fail) array set membership > set membership composes with a JSONB member target — the real knowledge shape
(fail) array set membership > a single-element array still uses IN, so one and many behave the same way
(fail) array set membership > an empty array emits no predicate, symmetric with the *Exclude branch
```
6 of 6 new tests red, all 17 pre-existing green — the control fires on exactly the new properties and nothing else. Restored before commit. Per mt#4512 this was a full revert of both mechanisms, not just the line I believed was load-bearing.
**One error typecheck caught, recorded rather than quietly fixed:** I declared `backend` as `"vector" | "none"` from memory; the emitted value is `"embeddings"`. Four type errors across two projects. Corrected after reading the call sites.
## Deploy verification
All four changed files are deploy surface — `isDeploySurfaceFile` returns `true` for each, run over the actual changed-file list rather than recalled. After merge I will run `deployment_wait-for-latest` with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge SHA, read `buildIdentity` rather than treating SUCCESS alone as proof, and correlate the deploy workflow run against the merge SHA when identity comes back `indeterminate` (expected for an image-source service). A tool or auth flake is a blocker to retry, not a licence to defer.
I will **not** claim a working `knowledge search --sources` post-deploy — mt#4946 is what makes that observable, and asserting it here would be the deploy-SUCCESS-equals-feature-works error this repo has already paid for.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01F1LhVjJvmY8oX8KxUK9vaA
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
This was referenced Sep 4, 2026
Merged
edobry
added a commit
that referenced
this pull request
Sep 4, 2026
… calibration writers ## Summary Two calibration writers resolve `const repoRootDir = findRepoRoot(input.cwd)` and pass that value as **`projectDir`** — the top, AUTHORITATIVE tier of `calibrationLogPath`'s ladder, which outranks `CLAUDE_PROJECT_DIR`. Both files asserted in a comment that the value *"IS authoritative — it is resolved by the caller, not a raw shell cwd."* That is false: the caller resolves it **from** the raw cwd. `calibrationLogPath`'s own docblock names this exact mistake: > The migrating writers are why the parameter exists: each hand-rolled > `resolve(findRepoRoot(input.cwd), <literal>)`, which silently treats the raw cwd as > authoritative. **Passing that cwd as `projectDir` would preserve the bug through the migration** > — the whole point is that it lands in the lower tier. In a session workspace `findRepoRoot` returns the **clone** — a real repo root, and the wrong project — so the record is keyed to a transient workspace instead of the project. **This fixes the TIER only.** With `CLAUDE_PROJECT_DIR` unset the ladder still falls through to this value and still keys to the clone. That is a separate defect (which root a session workspace *should* resolve to) and is tracked as mt#4954, not silently absorbed here. ## Key changes - `.minsky/hooks/gate-walk-provenance.ts` — 1 call site demoted; the false "IS authoritative" comment replaced with the correction and its basis. - `.minsky/hooks/require-execution-evidence-before-merge.ts` — 5 call sites demoted (`appendAtCoverageCalibration` plus the SC-coverage, test-first, render-path and consumer-account surfaces); same comment correction on its docblock. - `.minsky/hooks/calibration-root-tier.test.ts` — **new**, pins the tier for both writers. - Generated `.claude/hooks/` twins regenerated by the pre-commit step, not hand-edited. **Class-not-instance scan.** The only other `projectDir` passer is `coverage-claim-path-detector.ts:261`, which uses `deriveHookRepoRoot()` (`findRepoRoot(import.meta.dir)`) — a stable root, not cwd-derived — so its use of the top tier is correct and is left alone. The two `dispatcher.ts` sites are pass-throughs of the caller's option, not sources. ## Spec verification - **SC1** — discharged during planning, not by this diff. The reversibility check ran: 6 of 11 project keys hash to session-workspace paths, so the spec's own "this task is void if they are other real projects" branch does not apply. Recorded in mt#4885 `## Findings (2026-09-03)`. - **SC2** — this PR. Verified by reading all six call sites, not by the absence of new stray keys. - **SC3–SC6** — moved to **mt#4954** with their evidence and blockers, and mt#4885's scope is narrowed to SC1+SC2 in the same change, so its DONE-on-merge is honest rather than closing four unmet criteria silently. Markers, one per criterion, since a per-criterion deferral is what the coverage gate reads: [sc3-deferred: mt#4954] [sc4-deferred: mt#4954] [sc5-deferred: mt#4954] [sc6-deferred: mt#4954] ## Testing Typecheck: 0 errors across 8 projects (`validatedWorkspace` = the session; `infra/tsconfig.json` skipped, dependencies not installed locally — CI covers it). Lint: **0 errors, 0 warnings** over 4371 files. Execution evidence: ``` $ bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/calibration-root-tier.test.ts (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > gate-walk-provenance: CLAUDE_PROJECT_DIR outranks the caller's findRepoRoot(input.cwd) (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > require-execution-evidence: same tier, same outcome (pass) mt#4885 — cwd-derived roots land in the fallbackCwd tier > with CLAUDE_PROJECT_DIR unset the caller's root is still used — the tier is a ladder, not a redirect 3 pass 0 fail 5 expect() calls ``` ``` $ bun test --preload ./tests/setup.ts ./.minsky/hooks/calibration-root-tier.test.ts ./.minsky/hooks/gate-walk-provenance.test.ts ./.minsky/hooks/require-execution-evidence-before-merge.test.ts 267 pass 0 fail 491 expect() calls Ran 267 tests across 3 files. ``` ``` $ bun run test:hooks 7033 pass 0 fail 14679 expect() calls Ran 7033 tests across 190 files. [27.79s] ``` sc2 — negative control: reverted the FULL behavioural change (all 6 sites, `fallbackCwd:` → `projectDir:`, via sed) and re-ran; 2 of 3 cases went red. ``` $ sed -i '' 's/fallbackCwd: repoRootDir/projectDir: repoRootDir/g' <both files> reverted sites: gate-walk-provenance.ts:1 require-execution-evidence-before-merge.ts:5 error: expect(received).toBe(expected) Expected: "at-coverage" Received: undefined (fail) require-execution-evidence: same tier, same outcome 1 pass 2 fail ``` The third case stays green under the control **by design** — it is the `CLAUDE_PROJECT_DIR`-unset path, which is tier-independent, and it exists to guard the other direction (demoting the tier must not break the ordinary case). Restored afterwards; `grep -c 'projectDir: repoRootDir'` returns 0 in both files. **Why a dedicated test file rather than assertions in the two existing suites.** Those 264 tests pass *identically* before and after this fix, because they run with `CLAUDE_PROJECT_DIR` unset — and with an empty top tier, `projectDir: root` and `fallbackCwd: root` resolve to the same path. They cannot see this defect. The discriminating condition has to be constructed: `CLAUDE_PROJECT_DIR` **set**, to a directory that is not the one the caller passes. A test that cannot fail against the defect is not evidence about it (mem#704). ## Deploy verification `isDeploySurfaceFile` run over the actual changed set — `.minsky/hooks/gate-walk-provenance.ts`, `.minsky/hooks/require-execution-evidence-before-merge.ts`, `.minsky/hooks/calibration-root-tier.test.ts`, and the generated `.claude/hooks/` twins — returns **false for every one**, so `[no-deploy-impact]` is a checked claim rather than a remembered pattern list. No post-merge deploy verification is owed. ## Parallel work **PR #3253** (mt#3854) touches `.minsky/hooks/dispatcher.ts` and `.minsky/hooks/coverage-receipt.ts` — verified by reading its changed-file list, not its title. **Neither file is touched here**: this PR's surface (`gate-walk-provenance.ts`, `require-execution-evidence-before-merge.ts`) is entirely clear of it. The blocked work is SC4, which moved to mt#4954; a merge watch on #3253 was armed 2026-09-04T03:34:47Z. PR #3412 was also checked — 10 hook files, none in scope. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 4, 2026
…ub refuses it ## Summary `minsky-reviewer[bot]` fetched three things in one `Promise.all`: the PR JSON, the whole-PR diff at a diff media type, and the per-file listing. **Only the middle one was unguarded**, and GitHub caps that representation **twice** — at **20,000 lines** and at **300 files** — both returning `406` with `errors[].code === "too_large"`. `Promise.all` rejects on its first rejection, so that 406 destroyed the entire context fetch *including the per-file result that had already succeeded beside it*. The service then posted "Review failed — an internal error occurred. Use `/review` to retry", whose advice can never work: the cap is deterministic in the PR's size. Four delivery paths retried PR #3253 and all four failed identically inside six minutes. **The per-file path did not need building.** `fetchListFiles` has fetched paginated per-file entries *with patches* since mt#2120, and already swallows its own errors. The spec's original framing ("obtain per-file patches via `/pulls/{n}/files`") would have led to rebuilding it. What was missing was a guard on its sibling and an assembly step. That correction is recorded in the task's `## Diagnosis`. ## Key changes - **`diff-reconstruction.ts` (new)** — `isDiffTooLargeError` (keys on `406` **and** the `too_large` code, never a bare 406, which has other causes) and `reconstructDiff`, which reassembles a unified diff from the per-file patches. - **`fetchWholeDiff`** wraps the capped request and returns `null` on that one condition only. Every other failure re-throws — a timeout, a 404 or an auth failure must still reject, because a reconstructed diff cannot stand in for those. - **Both caps are covered by keying on the error code rather than a size**, so a third cap GitHub adds would route the same way. The two live fixtures bracket them: PR #3253 is 188 files / 85,606 insertions (line cap); PR #3412 is 313 files / 1,504 insertions (file cap). - **The size refusal now names itself.** `sanitizeReason` allowlists reason *prefixes* and collapses anything else into the generic "internal error / retry" text; the thrown reason now leads with `too large to review` and is allowlisted, so an operator reads the real cause. - **Both paths unavailable → a loud, named failure** rather than an empty diff, which would reach the model as a PR with no changes. **Vendor guidance — this MATCHES it.** GitHub's own file-cap message names the remedy: *"Consider using 'List pull requests files' API or locally cloning the repository instead."* No deviation to justify. **Anchoring is the real constraint on the output.** `pr.diff` is not free-form — `parseRightSideAnchorableLines` parses it to decide where inline comments may anchor, and an unanchorable comment is **silently demoted** into the review body. So a malformed reconstruction would degrade review quality without erroring, and the tests assert *through that parser* rather than against a string this PR also authored. ## Testing Execution evidence: **SC2 + AT2 (classification half) + AT3 — the trigger, including the discriminating negatives.** ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/diff-reconstruction.test.ts 13 pass 0 fail 27 expect() calls ``` Covers both real production error strings (copied from the logs for #3253 and #3412), the message-only fallback, and — the cases that matter — that a **bare 406 does NOT match** (AT3) and that a non-406 mentioning `too_large` does not either. **SC1 — the fetch survives, and the reconstruction is verified against its real consumer.** ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts \ src/status-comment.test.ts src/github-client.test.ts src/diff-reconstruction.test.ts 98 pass 0 fail 216 expect() calls ``` The reconstruction tests run `parseRightSideAnchorableLines` over the emitted diff and assert the exact anchorable line numbers — `/dev/null` on the correct side for adds and deletes, the old path on a rename, and a patch-less file recorded rather than dropped without derailing the file after it. **SC3 — the reason survives rendering.** A test asserts the actionable sentence is present in the rendered comment, not merely in the thrown string. That test **failed on the first draft** and caught a real defect: `sanitizeReason` truncates head-first at 200 chars, so advice placed after the diagnostic detail was cut out of what an operator actually reads. The message now leads with the imperative. **Full sweep:** `run-related-tests.ts` → 3 related files passed. `validate_typecheck` 0 errors across 8 projects. `validate_lint` **0 errors / 0 warnings** across 4,372 files. `format:check` clean. Negative control — the fix reverted in full, per mt#4512 Disabling `isDiffTooLargeError` restores **both** halves of the fix at once (the guard re-throws AND the fallback becomes unreachable), which is the pre-fix `Promise.all` rejection exactly: ``` (fail) survives a too_large 406 and reconstructs the diff from per-file patches error: Sorry, the diff exceeded the maximum number of files (300). …"code":"too_large" (fail) fails loudly when BOTH paths are unavailable (pass) uses GitHub's own diff when the fetch succeeds — the fallback stays dormant (pass) re-throws a NON-size failure instead of silently degrading 2 pass 2 fail ``` The control is **discriminating, not a blanket break**: the two that pass are exactly the two that *should* pass pre-fix, and the first failure is GitHub's verbatim production error. Restored and re-verified green (98 pass). Negative control — the status-comment allowlist An unrelated reason (`ECONNREFUSED … password=secret`) must still collapse to the generic fallback and must not leak. Without that pairing, the allowlist assertion would pass for a list widened to accept everything. ## Criteria not fully shipped Both are recorded in the spec's `## Implementation reconciliation`, with the reason: - `[sc4-deferred: mt#4955]` / `[at4-deferred: mt#4955]` — sweeper retrigger suppression. - **SC3 shipped in half** — the reason is now legible; routing it through `buildSkippedBody` so the `/review` footer stops advertising a retry is also **mt#4955**. **Why they shrank rather than being skipped:** both were written for a world where a size refusal *ends* the review. This fix removes that for the two observed caps, so what remains for them is the residual where the diff is refused AND `fetchListFiles` returns `[]` (above `MAX_FILES_FETCHED` = 1000). Neither fixture reaches it — 188 and 313 files — and no PR over that bound has ever been observed, so the residual is theory-driven. It is filed, not waved off. **mt#4879's deferred SC6 is deliberately NOT absorbed.** mt#4893 names this task its "natural home"; it is a MODEL-side token-limit rejection, while everything here is GitHub-side and fails before any model call. Same shape, no shared mechanism. ## Live verification **UNVERIFIED — AT1 and SC5 ("PR #3253 receives a real review") run post-deploy.** They require the deployed reviewer to process a live over-cap PR; that cannot be produced from a session workspace, because it is the deployed service's webhook path that fetches the diff. Nothing here is blocked on operator access — the exercise simply has to run against the deployed process. Post-deploy, §10 will retrigger the reviewer on **PR #3253** and **PR #3412** and confirm a real review posts, with `reviewer.diff_reconstructed` in the logs carrying a non-zero `filesWithPatch`. Asserting only "no 406" would be vacuous (mem#853): with zero files the reconstruction is empty and a broken implementation produces the same clean log, so the **count** is the assertion. Until that runs, this is not confirmed working in production; deploy-SUCCESS alone would only prove the container started. Deploy verification: required and not waived. `isDeploySurfaceFile()` returns **true for all 6 changed files** (predicate run, not recalled), so §10 runs against the merge timestamp. ## Context Authorized by **ask#9809**, which the principal answered "Fix the review bot first (mt#4434); PR #3253 waits". Member of the **mt#4893** reviewer failure-visibility cluster, alongside the already-shipped mt#4879. Closes mt#4434. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
![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: unknown
One blocking issue remains: src/mcp/inspector-launcher.ts still logs error.message directly in the child-process 'error' handler, violating the new prefer-loggable-error-summary rule and likely failing lint:strict. Please replace it with getLoggableErrorSummary(error). All other touched sites in this chunk appear correctly converted. No docs impact identified. Review limited to the files listed for this chunk.
Findings
- [BLOCKING] packages/domain/src/git/clone-operations.ts:127 — Raw error message still emitted in thrown error (leaks SQL and loses cause chain) after switching logs to getLoggableErrorSummary
At the catch-all incloneImpl(packages/domain/src/git/clone-operations.ts:121-141), the code logs usinggetLoggableErrorSummary(error)(good) but then throws a newErrorbuilt fromgetErrorMessage(error):throw new Error(Failed to clone git repository: ${getErrorMessage(error)});. This reintroduces the same risk this PR aims to remove: for DB-shaped errors (e.g.,DrizzleQueryError)getErrorMessagecan include full SQL and bound params, and the thrownErroralso discards the originalcausedetails. Given the PR’s purpose, this is a behavior regression relative to the logging change. Suggested fix: either (a) rethrow the originalerror(preserving stack/cause) or (b) wrap with a domain error that setscause: errorand uses a safe summary for the message, e.g.,throw new MinskyError('Failed to clone git repository', { cause: error });or at least usegetLoggableErrorSummary(error)for the message and pass the original as cause if using nativeError(new Error(message, { cause: error })). - [BLOCKING] packages/domain/src/session/session-workspace-service.ts:66 — Error message in thrown SessionNotFoundError still uses getErrorMessage (stringifying the error) instead of the new getLoggableErrorSummary
In the updated catch block (packages/domain/src/session/session-workspace-service.ts:58-80), the log line correctly usesgetLoggableErrorSummary(error), but the subsequent thrownSessionNotFoundErrorembedsgetErrorMessage(error)into the user-facing message. The PR's stated goal is to replace bareerr.message-style renderings at log sites withgetLoggableErrorSummary(err)to avoid leaking verbose SQL/params and to surface causes. While this line is not a log site, it regenerates the same noisy/low-signal string that motivated the change, and it creates inconsistency: logs will show a summarized cause, while the thrown error will carry the old long message. ReplacegetErrorMessage(error)withgetLoggableErrorSummary(error)in the thrown error message for consistency with the rule’s intent and to avoid reintroducing the verbose message elsewhere via error propagation. - [BLOCKING] scripts/codemod-loggable-error-summary.ts:190 — New import insertion can break shebang scripts by placing imports before the hashbang
insertNewImportfalls back to prefixing the file when there are no existing imports and no"\n\n"early-blank sentinel (seescripts/codemod-loggable-error-summary.ts:210-223). This will putimport { getLoggableErrorSummary } …ahead of a leading#!hashbang, which invalidates the shebang (it must be at byte 0, line 1) and breaks executability of scripts. Many repo scripts use a shebang (including this codemod itself). Fix by: (a) explicitly detecting a leading shebang (/^#!.*\n/) and inserting after it; and (b) more generally, scanning past any initial shebang + comment block rather than assuming a blank line exists. Without this, running the codemod on shebang-bearing files will silently corrupt them. - [BLOCKING] scripts/codemod-loggable-error-summary.ts:170 — Import-insertion guard tests for identifier presence anywhere in the file, not for an existing named import
AtprocessFile, the checkif (!new RegExp(\b${HELPER}\b).test(original)) { ... add import ... }keys off the raw source containinggetLoggableErrorSummaryanywhere, including comments, variable names, or unrelated identifiers. This can cause a false negative: when the identifier appears in a comment or another scope but is not actually imported, the codemod will skip adding the required import, leaving the rewritten call sites to fail typecheck/compile. The guard should detect an existing named import from the resolved errors module (or a namespaced import exposing the helper), not mere textual presence. Consider: (1) parse existing import statements (you already havefindImportStatements) and confirmgetLoggableErrorSummaryis among the named imports from the correct module; or (2) always attempt to add to an existing errors import (idempotently) and, if none, insert a new one. - [BLOCKING] scripts/verify-guard-canary-persistence.ts:170 — One remaining raw error logged instead of using getLoggableErrorSummary — inconsistent with the rule flip and others in this PR
At the bottom of the file,main().catch((err) => { console.error("[verify-guard-canary-persistence] FAILED:", err); ... })still logs the rawerrobject. This bypassesgetLoggableErrorSummaryand likely violates the now-erroringcustom/prefer-loggable-error-summaryrule this PR is flipping on.
Evidence (tail of file):
main().catch((err) => {
console.error("[verify-guard-canary-persistence] FAILED:", err);
process.exit(1);
});
Other log sites in this same file were converted (e.g., the table-existence branch uses (${getLoggableErrorSummary(err)})), so this looks like a missed spot. Please replace the err argument with getLoggableErrorSummary(err) to match the rest:
console.error("[verify-guard-canary-persistence] FAILED:", getLoggableErrorSummary(err));
This is blocking because it leaves a rule violation and contradicts the PR’s claim that lint:strict passes across the whole repo.
- [BLOCKING] src/adapters/mcp/session-files.ts:1 — Inconsistent error rendering in the same response: logs use getLoggableErrorSummary but response body still uses getErrorMessage
The catch blocks for bothsession.move_fileandsession.rename_filenow log witherror: getLoggableErrorSummary(error)(good), but they still returncreateErrorResponse(getErrorMessage(error), ...)for the tool response. This creates divergent error semantics for the same failure: operators see the improved cause-aware summary, while MCP clients (including tests and downstream callers) receive the oldmessage-derived string that can lose.causeand exceed practical length. The PR’s stated goal is repo-wide conversion togetLoggableErrorSummaryat error-reporting surfaces; keeping the old formatter in these user-visible tool responses is a behavior inconsistency.
Requested change: use a single, consistent error summarizer on both channels. Either (a) change the createErrorResponse first argument to getLoggableErrorSummary(error) to match the log, or (b) provide a clear rationale in code comments and spec for why client-facing responses should remain message-exact while logs are summarized. Given other adapters in this PR also switch their returned error payloads (e.g., config list/show and compile commands continue to surface getErrorMessage in the returned object), please align intentionally: if the client contract elsewhere expects the raw message, justify it; otherwise migrate to getLoggableErrorSummary here too.
- [BLOCKING] src/adapters/shared/commands/refs.ts:185 — Missed conversion:
unavailableMessagestill logsgetErrorMessage(cause)instead of usinggetLoggableErrorSummary
The goal of this PR is to stop logging rawError.message(especiallyDrizzleQueryError.message, which includes SQL and params) and pass the error object throughgetLoggableErrorSummary. Insrc/adapters/shared/commands/refs.ts, the helperunavailableMessage(...)still does:
log.error("refs.status: ref lookup did not complete", {
ref,
kind,
error: getErrorMessage(cause),
});This is a structured log site newly in-scope for this conversion (same file where other lines were converted to getLoggableErrorSummary in this PR), and it will continue to log SQL/params for DB-related failures. Please change the error field here to getLoggableErrorSummary(cause) to align with the rule's intent and the rest of this PR's conversions. Also check sibling uses in this module to ensure no other log site renders getErrorMessage or .message into error: fields.
- [BLOCKING] src/adapters/shared/commands/rules/list-search-commands.ts:40 — CLI error path still uses
getErrorMessageinstead ofgetLoggableErrorSummary
Inrules.index-embeddings'sexecutecatch block, the code does:
const message = getErrorMessage(error);
if (Boolean(params.json) || ctx?.format === "json") {
return { success: false, error: message };
}
log.cliError(`Failed to index rule embeddings: ${message}`);
throw error;Per this PR's goal, CLI/user-facing surfaces were also converted because getLoggableErrorSummary is byte-identical to err.message for plain Error and differs usefully when .cause is present or messages are huge. This path should use getLoggableErrorSummary(error) so JSON and CLI both carry the improved summary and avoid leaking raw SQL/params from DrizzleQueryError.message.
Please replace getErrorMessage with getLoggableErrorSummary here. The module already imports it.
-
[BLOCKING] src/adapters/shared/commands/tasks/crud-commands.ts:171 — Missed conversions: log sites still render
getErrorMessage(...)(leaky/informationally-poor) instead ofgetLoggableErrorSummary(...)
This PR's stated goal is to convert log sites togetLoggableErrorSummary(...), but several warn/debug calls in this file still incorporategetErrorMessage(...)(or its result) into the logged message: -
TasksCreateCommand.execute— dependency add failure path:const msg = getErrorMessage(depErr); log.warn( \[tasks.create] Failed to add dependency ${dep}: ${msg}`
);` -
TasksCreateCommand.execute— dependency provider catch:const msg = getErrorMessage(providerErr); log.warn( \[tasks.create] Could not add dependencies: ${msg}`
);` -
TasksCreateCommand.execute— parent set failure:const msg = getErrorMessage(parentErr); log.warn( \[tasks.create] Failed to set parent ${params.parent}: ${msg}`
);`
These are LOG sites and should use getLoggableErrorSummary(...) for the rendered error value to avoid leaking SQL/bound params and to surface the underlying cause when present. Please replace these with getLoggableErrorSummary(...) (either inline in the template or by computing summary = getLoggableErrorSummary(err) in scope) to meet the new lint rule's intent.
-
[BLOCKING] src/cockpit/shared-persistence.ts:1095 — Missed conversions: remaining
err.message/String(err)renderings not usinggetLoggableErrorSummary(rule regression)
This file importsgetLoggableErrorSummaryand converts several sites, but at least three error renderings still directly useerr.message/String(err), which contradicts the PR’s stated goal and will re-triggercustom/prefer-loggable-error-summarywhen the rule is set to error: -
In
closeAbandonedService(...)’s catch block, a localmessageis derived aserr instanceof Error ? err.message : String(err)and passed to twolog.warn(...)calls (one for the "REJECTED" path and one for the "abandoned" path). Both should usegetLoggableErrorSummary(err)instead (either inline or by derivingmessageviagetLoggableErrorSummary). -
In
startDbRetryBackoff(...)’s catch block,const msg = err instanceof Error ? err.message : String(err)is embedded into the warn string. This should begetLoggableErrorSummary(err). -
In
getSharedPersistenceService(...)’s orphan-close handler, alog.debugembedsString(closeErr)inside the template literal; this should usegetLoggableErrorSummary(closeErr).
These are in-scope for this PR and appear to be the same class of site converted elsewhere in this file. Please update these remaining occurrences to pass the error object (or its getLoggableErrorSummary(...)) and satisfy the lint rule consistently.
Evidence (excerpt from this file’s tail):
.catch((err: unknown) => {
const elapsedMs = Date.now() - startedAt;
const message = err instanceof Error ? err.message : String(err);
if (!isRecycleCloseDeadline(err)) {
log.warn("[shared-persistence] close of recycled pool REJECTED", { message, elapsedMs, ... });
return;
}
log.warn("[shared-persistence] abandoned close of recycled pool — ...", { message, elapsedMs, ... });
});
...
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
log.warn(`[shared-persistence] DB retry failed (${msg}); next attempt in ${intervalMs}ms`);
}
...
Promise.resolve(svc?.close?.()).catch((closeErr) =>
log.debug(`[shared-persistence] orphan close() after init-timeout failed: ${String(closeErr)}`)
);- [BLOCKING] src/commands/mcp/scheduler-wiring.ts:1 — Missed conversion:
onErrorstill logsgetErrorMessage(error)instead ofgetLoggableErrorSummary(error)
TheKnowledgeSyncScheduleris constructed with anonErrorhandler that callslog.error(..., { error: getErrorMessage(error) }). Per this PR's goal and the eslint rule flip, log sites should pass the error object throughgetLoggableErrorSummaryto avoid leaking SQL/params and to surface.causedetails.
Evidence (near the scheduler construction):
const scheduler = new KnowledgeSyncScheduler({
sources,
deps,
onError: (sourceName, error) => {
log.error(`[scheduler] Sync error for source "${sourceName}"`, {
error: getErrorMessage(error),
});
},
});
Replace getErrorMessage(error) with getLoggableErrorSummary(error). After updating, drop the now-unused getErrorMessage import from this module.
- [BLOCKING] src/commands/mcp/start-command.ts:940 — Missed conversion:
/registerhandler logsgetErrorMessage(err)rather thangetLoggableErrorSummary(err)
In the OAuth DCR/registerroute, the catch block computesconst message = getErrorMessage(err);and then logs it:log.error("DCR /register error", { error: message });. Per this PR's stated goal and the lint rule flip, log sites should render errors viagetLoggableErrorSummaryto avoid leaking sensitive details and to include.causeinformation.
Evidence (inside startHttpServer):
app.post("/register", async (req, res) => {
...
} catch (err) {
const message = getErrorMessage(err);
log.error("DCR /register error", { error: message });
// RFC 7591 response follows
res.status(400).json({
error: "invalid_client_metadata",
error_description: message,
});
}
});
Fix: keep message for the client response if desired, but change the log call to log.error("DCR /register error", { error: getLoggableErrorSummary(err) });. Ensure getLoggableErrorSummary is imported (it already is at the top of this file).
- [BLOCKING] src/commands/mcp/start-command.ts:1760 — Missed conversion: retry controller logs
getErrorMessage(result.error)instead ofgetLoggableErrorSummary(result.error)
Inside theRetryingInitController'sonAttemptSettledcallback, the error path logs:
log.error("[mt#1962] container.initialize() failed", {
attempt: result.attempt,
consecutiveFailures: result.consecutiveFailures,
error: getErrorMessage(result.error),
});
Per this PR's objective and the flipped ESLint rule, structured log sites should render errors with getLoggableErrorSummary so .cause details are surfaced and oversized messages are truncated. Replace getErrorMessage(result.error) with getLoggableErrorSummary(result.error). The file already imports getLoggableErrorSummary.
- [BLOCKING] src/mcp/inspector-launcher.ts:116 — Unconverted
error.messagelogged — violates newprefer-loggable-error-summaryposture and likely fails lint:strict
Insrc/mcp/inspector-launcher.ts, the process error handler still logs a bare message string:
inspectorProcess.on("error", (error) => {
log.error("MCP Inspector process error", {
error: error.message,
stack: error.stack,
});
});Per this PR’s goal and the flipped rule to "error", this site must pass the error object through getLoggableErrorSummary(error) rather than error.message. Every other converted site in this file and across the PR follows that rule. Leaving this will leak unredacted/verbose strings and should be flagged by bun run lint:strict (the rule targets exactly this shape). Please change to:
error: getLoggableErrorSummary(error),and keep the stack line as-is if desired. This is a correctness-of-the-PR issue (the central lint gate) rather than style — it blocks merge until converted.
- [NON-BLOCKING] .claude/hooks/causal-premise-detector.ts:56 — Import path includes "/index" subpath; consider using the package export root for stability/consistency
The new import uses@minsky/domain/errors/index(e.g.,.claude/hooks/causal-premise-detector.ts:56).packages/domain/package.jsonexports already expose"./errors": "./src/errors/index.ts", and there is a catch-all"./*"as well. Prefer@minsky/domain/errorsover@minsky/domain/errors/indexfor consistency with subpath exports and to avoid depending on the file name. This is stylistic — the current import does resolve via the pattern export — but aligning on the shorter form reduces churn if the file layout changes. - [NON-BLOCKING] eslint.config.js:1285 — Carve-out for tests/fixtures/** may be broader than needed
The rule exemption targets the entiretests/fixtures/**tree, but the PR description cites a concrete reason specific totests/fixtures/typescript/large-service.ts(9 sites). Disabling across all fixtures could mask future legitimate violations in other fixtures. Consider narrowing thefilesglob to the specific known path(s) or adding an inline allowlist comment at the exact fixture(s) that require it. - [NON-BLOCKING] eslint.config.js:1300 — eslint-rules/** carve-out may be overly broad
The exemption disablescustom/prefer-loggable-error-summaryfor the entireeslint-rules/**tree, but the rationale cites a specific need foreslint-rules/no-unregistered-minsky-env-var.js:107. Broadly disabling the rule could mask genuine violations in other rule modules that don’t import app code. Consider narrowing thefilesglob to the specific file(s) that must remain standalone or scoping by a more precise pattern (e.g., just the env-var rule file), keeping the rest ofeslint-rules/**covered. - [NON-BLOCKING] packages/domain/src/git/git-core-operations.ts:1 — Duplicate named imports from the same module — consider consolidating for consistency
This file now imports from../errors/indexacross multiple lines (e.g.,import { MinskyError, getLoggableErrorSummary } from "../errors/index";and then separate lines forgetErrorMessageandValidationError). While valid, our codebase generally consolidates named imports from the same module into a single statement for readability and to ease future maintenance. Consider merging these into oneimport { MinskyError, getLoggableErrorSummary, getErrorMessage, ValidationError } from "../errors/index";block unless an intentional grouping convention applies here. - [NON-BLOCKING] packages/domain/src/repository/review-state-labels.ts:200 — Prefer structured error field over string interpolation for log messages
This change switches togetLoggableErrorSummary(error)inside a string template (e.g., inapplyReviewStateLabelaround the "Failed to list labels on PR"/"Failed to add/remove review-state label" messages). For observability and downstream parsing, consider using a structured log with anerrorfield instead of embedding the summary into the message string:
log.warn("Failed to add review-state label", {
prNumber,
targetLabel,
error: getLoggableErrorSummary(error),
});Many call sites in this repo already follow the structured { error: … } pattern; applying it here would keep logs consistent and machine-parsable. This is a suggestion, not a merge blocker.
- [NON-BLOCKING] packages/domain/src/session/session-cleanup.ts:11 — Inconsistent import specifier for errors module inside domain package
This file imports from"../errors"(packages/domain/src/session/session-cleanup.ts:11), whereas most other files in this chunk import from"../errors/index". The spec amendment in the PR body calls for relative imports inside the domain package (which both forms satisfy), but keeping a single convention within the same tree reduces churn and avoids future toggling by automated codemods. Consider standardizing on one form (most files here use"../errors/index"). - [NON-BLOCKING] packages/domain/src/workspace/local-workspace-backend.ts:1 — Possibly unused import
getErrorMessageremains after conversion
The import now readsimport { getErrorMessage, getLoggableErrorSummary } from "../errors/index";but onlygetLoggableErrorSummaryis used in the changed log site (and I couldn't find any other reference togetErrorMessagein this file based on the current content). IfgetErrorMessageis no longer referenced, please remove it to avoid an unused import warning and keep the module tidy. - [NON-BLOCKING] scripts/codemod-loggable-error-summary.ts:124 —
isErrorsModulematcher is overly broad and may hijack unrelated imports named “…/errors”
isErrorsModulereturns true for any specifier that matches/(^|\/)errors(\/index)?$/, which will also match unrelated local modules like"../../utils/errors". In that case,addToExistingImportwill appendgetLoggableErrorSummaryto the wrong module's named imports, producing a compile-time error (the symbol won't exist there). Consider tightening this to compare against the exact intended specifier: either equality with thewantedvalue for this file, or path-equivalence after resolving relative specifiers. Alternatively, restrict the regex to the domain package's errors module path shape only. - [NON-BLOCKING] scripts/codemod-loggable-error-summary.ts:146 — Path handling likely non-portable on Windows (mixes POSIX-style checks with OS-native separators)
collectSitesderivesrelviapath.relative(repoRoot, entry.filePath), which on Windows yields backslashes. The exclusion prefixes (e.g.,".claude/hooks/") and laterhelperSpecifierForlogic assume POSIX-style forward slashes and test withstartsWith, so they will fail to match on Windows (causing generated folders not to be excluded, and domain-internal files not to be detected). Similarly,helperSpecifierForbuilds paths withresolve("/", …)and compares/massages withstartsWith("."), which is brittle across platforms. Consider normalizing all paths to POSIX (path.posix) when working with repo-relative paths and specifiers, or replace separators with/before prefix tests. - [NON-BLOCKING] scripts/codemod-loggable-error-summary.ts:128 — Import statement parser misses valid shapes (multi-line
import …\nfrom …;and side-effect imports)
findImportStatementsuses/^import\s[\s\S]*?from\s+["']([^"']+)["'];[ \t]*$/gm, which only matches single-lineimport … from "…";statements. It will not detect split imports like:
import {
a,
b,
} from "x";or side-effect imports import "reflect-metadata"; (no from). As a result: (1) imports.length may be 0 even when the file has imports, causing insertNewImport to inject a new import at the top and potentially before a required side-effect import; and (2) adding to an existing named import will fail for multi-line-with-from cases not matched by the regex. Consider using a simple TS/JS parser (or a more robust regex) to collect imports, and treat side-effect imports as anchors when choosing insertion points (e.g., insert after any shebang and after the last import of any kind).
-
[NON-BLOCKING] scripts/replay-retrospective-trigger-corpus.ts:501 — Inconsistent failure prefix vs. neighboring scripts may hinder log grepability
This script logs only the summarized error (console.error(getLoggableErrorSummary(error))), while most other scripts in this chunk include a consistent"FAIL:"or bracketed prefix (e.g.,"[probe] FAILED:","FAIL after …ms:"). If downstream tooling or operators grep for those prefixes, this script would be an outlier. Consider standardizing the prefix (e.g.,console.error(FAIL: ${getLoggableErrorSummary(error)})) for consistency across scripts. -
[NON-BLOCKING] services/reviewer/src/adoption-sweeper.ts:611 — Telemetry
errorfield now usesgetLoggableErrorSummary— verify downstream consumers aren’t relying on prior plain-message format
This change switches the emittederrorvalue fromerr.message(orString(err)) togetLoggableErrorSummary(emitErr)(services/reviewer/src/adoption-sweeper.ts:611). While the new helper is strictly better for diagnostics (handlescausechains and truncation), any downstream consumer that performs exact-match comparisons or parsing against the prior plain message string could be impacted (e.g., alert grouping, dashboards, or tests matching substrings). Please double-check consumers of theadoption_sweeper.event_emit_failedevent to ensure they either treaterroras opaque text or remain compatible with the new format. If any consumers depend on a specific shape, consider emitting both fields temporarily or documenting the contract explicitly. -
[NON-BLOCKING] src/adapters/shared/commands/tasks/crud-commands.ts:1 — Unconverted error rendering inside
this.debug(...)still useserror.message/String(error)
In theTasksGetCommand.executecatch block, the call tothis.debug("tasks.get execution failed", { error: error instanceof Error ? error.message : String(error), duration })still renders the rawerror.messagerather than usinggetLoggableErrorSummary(error). While this flows through a helper (this.debug) before reachinglog.debug, it still ultimately logs the message string and can reintroduce the very overlong/under-informative.messagecaptured bycustom/prefer-loggable-error-summary. Consider changing theerrorfield there (and similarthis.debugsites in this file like thetasks.createfailure path) togetLoggableErrorSummary(error)to match the rest of this PR's intent. Marking as pre-existing-within-file shape; this PR modified other log sites here but this particular call remained unchanged. -
[PRE-EXISTING] services/reviewer/src/server.ts:1 — Residual error.message indirections remain in this file (not converted to getLoggableErrorSummary)
This PR correctly converts many sites here togetLoggableErrorSummary, but several catch paths in the same module still derive amessagestring viaerr instanceof Error ? err.message : String(err)and pass that into structured logs (losing.causeand risking oversized messages). Examples visible in this file: -
startDetachedReview(...).catch(...)— logsreview_errorandwebhook_processing_failedwitherror: message. -
/issue_comment.createdhandler catch — logscomment_command.pr_fetch_failedwitherror: message. -
/retriggerroute catch — logsretrigger.errorwitherror: message. -
/alert-testroute sink catch — logsalert_test.sink_errorwitherror: message. -
Webhook dispatch catch — logs
webhook_dispatch_errorwitherror: message.
These are pre-existing lines not modified by this diff, but they preserve the exact failure the rule targets (PG errors hidden on .cause, long .message bodies). Suggest a follow-up sweep to replace these error: message payloads with error: getLoggableErrorSummary(err) and drop the intermediate message variable where it exists. The module already imports getLoggableErrorSummary, so the change is local.
Classification: PRE-EXISTING — not introduced by this diff; raising for completeness to avoid blind spots the lint rule cannot catch due to indirection.
Documentation impact
- no-update-needed — This chunk only changes logging calls to route errors through getLoggableErrorSummary and adjusts imports; there are no user-visible behavior or CLI/API changes. No docs in docs/ reference these internals, and no documentation files are part of this diff.
edobry
added a commit
that referenced
this pull request
Sep 4, 2026
…th maskConnectionString ## Summary Six sites hand-rolled a connection-string userinfo redaction instead of calling `maskConnectionString`, in three different spellings. Every one of them required a **non-empty username AND a non-empty password**, so a connection string with either half empty matched nothing — and a regex that matches nothing returns its input unchanged, printing the credential. The **empty-username** form is the severe one: `postgresql://:realpassword@host/db` goes through verbatim with a **live password**. The empty-password form (`postgresql://user:@host/db`, which Postgres accepts) exposes a username and host. All six were also non-global, so a second connection string embedded in the same text survived regardless of which halves were populated. This is the failure mode `terminal-command-best-practices.mdc §Secret handling` names outright — *"a pattern matching nothing emits its input UNCHANGED, indistinguishable from a redaction that fired"* — and the repo has already leaked a production password exactly this way (mem#808). ## Key changes Six sites converted to `maskConnectionString`: | Site | Leaked on | | --- | --- | | `packages/domain/src/persistence/validation-operations.ts:103` | empty user, empty pass, 2nd occurrence | | `packages/domain/src/persistence/postgres-migration-operations.ts:613` | same | | `scripts/verify-persistence-self-heal.ts:111` | 2nd occurrence only | | `scripts/smoke-task-id-reuse.ts:73` | empty pass | | `scripts/smoke-task-kinds.ts:66` | empty pass | | `scripts/smoke-memory-domain-routing.ts:51` | empty pass | `validation-operations.ts` already imported `maskConnectionString` and called it 55 lines above the weak copy — that site was a one-token fix. **Mask vs. host, decided once for all six:** `maskConnectionString`, not `connectionTargetHost`. The latter returns `new URL(cs).host` — no database name — and every one of these lines exists to answer "which database?" (three say `Database:` outright). Masking keeps host/port/db while replacing both userinfo halves globally, so it reaches the same safety without dropping the detail that makes the line worth printing. `verify-persistence-self-heal.ts` gains a second benefit: its old rendering was `://***@`, and `credential-shape-check.ts` recognizes exactly one masked form — `maskConnectionString`'s own output. `://***@` was not a form the checker could confirm safe; `://***:***@` is. ## Scope found during planning The task spec named **three** sites. Its verification grep matched only the `:\/\/`-prefixed spelling, so it was blind to the other two spellings — four of the six sites. The widened grep found them. Full inventory and the corrected grep are in [mt#4910](https://github.com/edobry/minsky/blob/main/README.md)'s `## Planning Audit` and `## Implementation` sections. ## Out of scope — filed as mt#4963 **The sanctioned detector shares this exact blind spot.** `CREDENTIAL_SHAPES`'s `postgres-url-credentials` regex (`credential-scrubber.ts:151`) also requires non-empty halves, and it feeds three consumers: the transcript-ingest scrubber, `minsky security check-credentials`, and (by the same shape) `.gitleaks.toml`. Measured by running them: ``` scrubText("postgresql://:hunter2@db.example.com:5432/minsky") -> passed through verbatim checkForUnmaskedCredentials("postgresql://:hunter2@host:5432/db") -> no hit checkForUnmaskedCredentials("postgresql://user:@host:5432/db") -> no hit ``` That is why AT3 below asserts by reading the line rather than with `check-credentials`: on this defect class the checker cannot discriminate a fixed tree from a broken one. Filed as mt#4963 rather than absorbed — different subsystem, different files. ## Testing The task's `## Acceptance Tests` numbering is used verbatim below. Execution evidence: **AT1 — the divergence, reproduced** (discharged during planning; both functions run, not read): ``` --- EMPTY password: postgresql://user:@host:5432/db helper : postgresql://***:***@host:5432/db inline : postgresql://user:@host:5432/db <-- UNCHANGED --- EMPTY username: postgresql://:hunter2@host:5432/db helper : postgresql://***:***@host:5432/db inline : postgresql://:hunter2@host:5432/db <-- UNCHANGED, live password exposed ``` **AT2 / SC3 — the widened grep returns zero leaking sites.** The form the spec originally specified (`[^)]*`) cannot span a `)`, so it could not see the sanctioned helper's own regex — a probe that could not fully fail, which is this task's own defect class. Corrected to `[^,]*`: ``` $ grep -rnE '\.replace\(/[^,]*@' --include='*.ts' packages src scripts services packages/domain/src/persistence/providers/postgres-provider.ts:551: return connectionString.replace(/(@[^/?]*):6543(?=\/|$|\?)/, "$1:5432"); packages/domain/src/persistence/providers/postgres-provider.ts:1022: const displayString = connectionString.replace(/\/\/[^@]+@/, "//***@"); packages/domain/src/persistence/connection-string.ts:23: return input.replace(/(:\/\/)[^:/@]*:[^@]*@/g, "$1***:***@"); packages/domain/src/rules/rule-mention-parser.ts:55: .replace(/(?:^|[\s])@[a-zA-Z0-9_-]+(?=[\s]|$)/g, " ") src/hooks/deploy-domain-detector.ts:161: h = h.replace(/^[^@/]*@/, ""); // strip user:pass@ scripts/smoke-setup-db.ts:63: const fragment = pgUrl.replace(/^postgres(ql)?:\/\/[^@]*@/, ""); ``` Six hits, none leaking: the sanctioned helper (line 23 — expected, and what makes this grep able to fail); the `:6543`→`:5432` port swap; `getConnectionInfo`, which masks the empty-half cases correctly and is parked on mt#4963 behind PR #3412 and mt#3497; an `@mention` stripper in rule prose; and two sites that STRIP userinfo to build a host or substring needle rather than to print a haystack. **AT3 — `persistence check` against the live database**, asserting the mask fired rather than printing the raw line: ``` $ bun src/cli.ts persistence check Testing connection to: postgresql://***:***@aws-0-us-west-2.pooler.supabase.com:6543/postgres $ <output> | bun src/cli.ts security check-credentials --quiet # exit 0 (clean) $ <output> | grep -cE 'postgres(ql)?://[^:/@*]+:[^@*]+@' # 0 unmasked shapes ``` **AT5 / related tests:** ``` $ bun scripts/run-related-tests.ts <the 7 changed files> 382 pass / 0 fail / 976 expect() calls across 16 files [15.73s] incl. postgres-migration-operations.test.ts, validation-operations.test.ts, setup-db.test.ts ``` Typecheck 0 errors across 8 projects (session workspace); lint 0 errors / 0 warnings across 4376 files. Negative control — AT4, per site, against the un-fixed patterns: Each pre-fix pattern was run verbatim against the failing inputs alongside the new helper. Sites 1–2 leave BOTH empty-half forms unchanged; sites 4–6 leave the empty-password form unchanged; all three spellings drop the second occurrence. ``` ### validation-operations.ts:103 + postgres-migration-operations.ts:613 empty username (LIVE password) PRE-FIX : postgresql://:hunter2@db.example.com:5432/minsky <-- UNCHANGED, LEAKS POST-FIX : postgresql://***:***@db.example.com:5432/minsky empty password PRE-FIX : postgresql://user:@db.example.com:5432/minsky <-- UNCHANGED, LEAKS POST-FIX : postgresql://***:***@db.example.com:5432/minsky two in one string PRE-FIX : failed "postgresql://***:***@h1/d1"; retry "postgresql://b:s2@h2/d2" POST-FIX : failed "postgresql://***:***@h1/d1"; retry "postgresql://***:***@h2/d2" ### smoke-task-id-reuse.ts:73 + smoke-task-kinds.ts:66 + smoke-memory-domain-routing.ts:51 empty password PRE-FIX : postgresql://user:@db.example.com:5432/minsky <-- UNCHANGED, LEAKS POST-FIX : postgresql://***:***@db.example.com:5432/minsky two in one string PRE-FIX : failed "postgresql://a:<REDACTED>@h1/d1"; retry "postgresql://b:s2@h2/d2" POST-FIX : failed "postgresql://***:***@h1/d1"; retry "postgresql://***:***@h2/d2" ``` **On the added test, stated plainly:** the multi-occurrence test in `setup-db.test.ts` (SC4) covers the `g` flag, the one case the existing five-case suite did not. It exercises the HELPER, which this PR does not change, so it passes on `main` too and has no negative control of its own — the control above is at the six call sites, which is where the behaviour changed. **SC5 — no per-site behavioural test was added, and the reason is the code shape.** Each site is `template(mask(connectionString))`: a log or format line whose only behaviour IS which function it names. The value is already tested at the helper across all six edge cases, so a per-site test could only assert *which function is called* — and the honest instrument for that is AT2's grep. Reaching the two domain lines behaviourally would need a live Postgres (`getPostgresMigrationsStatus` opens a connection before computing `maskedConn`) or a `spyOn(log, "cli")`, which `/implement-task` §6 names as design feedback rather than a test to write, and which `TEST_LOGGER_SILENCED_FLAG` would silence anyway. **Runtime import checked, not just typechecked.** The `@minsky/domain/persistence/connection-string` subpath was confirmed to resolve at runtime via a `bun -e` import returning the working function — the mem#577 / mt#2760 hazard where a subpath passes tsconfig `paths` and fails bun's package `exports`. Deploy verification: three of the seven changed files are deploy surface per `isDeploySurfaceFile` (`packages/domain/src/persistence/{postgres-migration-operations,validation-operations}.ts` and `packages/domain/src/setup-db.test.ts`; the four `scripts/` files are not). The post-merge deploy will be verified per `/implement-task` §10 with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge commit. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 4, 2026
…ting out the timeout ## Summary `session_pr_wait-for-review` treated every `expectedHeadSha` mismatch identically: poll until the timeout, then report `expectedHeadShaUnreached`. That is correct for the case it was built for — a push still in flight — and wrong for a sha the caller constructed, which can never arrive. The two are separable on the **first poll that observes a remote head**. A push-lag mismatch has an entirely different commit as the observed head. A caller-error mismatch shares a long common prefix and then diverges, because the caller started from a real abbreviated sha and extended it. This matters beyond the wasted time: a wait that returns nothing after its full budget is, per `/implement-task` §9, the documented lead-in to the bypass ladder. The failure mode is an agent reading its own mis-armed wait as reviewer silence. **Originating incident** (2026-09-04, PR #3635 / mt#4897): `session_commit` returned `commitHash: "f76e55628"` (9 chars). The caller passed `f76e556285ff4d6a4e0d21b0ba1e0a54ba7d2e0f` — the 9 real characters plus 31 invented ones — believing the parameter required a full 40-character sha. It does not; prefix matching is by design (mt#4039). The real head was `f76e556281b76e51949a057834f279e73d03a8e0`. Both existing boundary checks admitted the value: it is hexadecimal and well over the 7-character floor. ## Key changes - **`classifyHeadShaMismatch`** (`pr-wait-for-review-subcommand.ts`) returns `divergent-prefix` when the two share `>= MIN_ABBREVIATED_SHA_LENGTH` (7) characters and then differ, `push-pending` otherwise, and `null` when there is nothing to compare. - **`expectedHeadShaUnreached` carries `classification`.** The field is populated from the same `headSha` closure the payload reports, so the verdict can never describe a different observation than the `lastObservedHeadSha` printed beside it. - **`divergent-prefix` returns immediately**, through `finalizeTimeout` rather than a bare `buildTimeoutResult`. That path is already bounded by its own short budget and still attaches the fresh reviews list and `reviewerCheckRunState`; it cannot mis-report a review as a match here because its `finalMatch` is gated on `remoteIsServingExpectedHead()` — the predicate that is false. Verified by reading it, not assumed. - **The text-mode message branches.** On `divergent-prefix` the generic "two causes, opposite remedies" line is *replaced* — one of its two causes has been ruled out by evidence, so leaving it in asks the reader to weigh a possibility already eliminated. The new text names the padding mistake and the remedy (pass `commitHash` verbatim; do not extend it to 40 characters). - Parameter description and the regenerated `completion-manifest.json` updated to match. ## Judgment calls **Gate (g) collision, resolved as coordinate rather than wait.** Open PR #3412 (mt#4639, the 614-site `getLoggableErrorSummary` conversion) also touches `pr-wait-for-review-subcommand.ts`. I read its actual changed-file list and its patch for this file rather than judging by title: the overlap is **+3 −2** — one import line and two `getErrorMessage` substitutions inside catch-block `log.debug` calls (~L703, ~L1166). No hunk overlaps the matcher, the result type, or the poll loop, and the test file is untouched. Blocking a two-file change on an eight-day-old 100+-file mechanical conversion was the worse trade. The `parallel-work-open-pr` guard fired at `session_start` and I cleared it with an audit-logged `grant-guard-override.ts` grant carrying this reasoning. **Whoever lands second rebases.** **SC5 needed two assertion edits, and the criterion says "tests pass untouched".** Adding a field to `expectedHeadShaUnreached` breaks two exact-match `toEqual` assertions in `pr-wait-for-review-push-lag.test.ts`. I updated both to include `classification: "push-pending"`. The *semantics* SC5 protects are unchanged — those tests still assert the wait polls on and times out — but the payload shape changed, so the criterion is met in substance and not literally. Calling that out rather than letting it read as untouched. **A known false negative, deliberately not fixed.** A fabricated sha extending an *older* head's abbreviation, on a PR whose head has since advanced, shares ~0 characters with the current head and is reported as `push-pending`. That degrades to exactly today's behaviour rather than to a wrong verdict; catching it would require retaining head history this wait does not keep. Recorded in the code and in the spec's planning audit. The same reasoning covers a sha stranded by a rebase (mem#1013) — five recorded incidents, cause fixed upstream by mt#4046, and not the shared-prefix shape. ## Testing New file `pr-wait-for-review-sha-classification.test.ts` (a sibling, not an extension of `pr-wait-for-review-push-lag.test.ts`, which sits near the 400-line `max-lines` WARN threshold that this repo's zero-tolerance warning gate makes unshippable — mem#833's extract-a-sibling precedent). Execution evidence: AT1 — a 40-char value sharing its first 9 characters with the observed head, diverging after, classified `divergent-prefix`, returning on poll 1 of a 600s budget; message names the cause (the AT1 message half is asserted in `pr-wait-for-review-command.test.ts`, "names the extended-abbreviation cause"). AT2 — a true abbreviated prefix still matches (mt#4039 regression). AT3 — a sha sharing fewer than 7 characters classified `push-pending` and polls >50 times. AT4 — a backend with no `getPullRequestHeadSha` is unchanged and reports nothing new. SC1/SC2/SC5 are the AT1-vs-AT3 contrast above; SC3 is the adapter message test; SC4 is the threshold test plus the fixture check named in its own test title. ``` $ bun test --preload ./tests/setup.ts packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts packages/domain/src/session/commands/pr-wait-for-review-push-lag.test.ts (pass) classifyHeadShaMismatch (mt#4995) > the originating incident classifies as divergent-prefix (pass) classifyHeadShaMismatch (mt#4995) > an unrelated sha is push-pending — the wait-it-out case (pass) classifyHeadShaMismatch (mt#4995) > a value that MATCHES is not a mismatch at all (pass) classifyHeadShaMismatch (mt#4995) > an absent side yields no classification, never a guess (pass) classifyHeadShaMismatch (mt#4995) > the threshold is exactly MIN_ABBREVIATED_SHA_LENGTH, checked from both sides (pass) classifyHeadShaMismatch (mt#4995) > classification normalizes case and whitespace like the matcher does (pass) classifyHeadShaMismatch (mt#4995) > SC4 negative control: no existing test fixture trips the discriminator (pass) ... > AT1: a fabricated extension returns on the first poll, not at timeout (pass) ... > AT1: the review was visible all along — this was never reviewer silence (pass) ... > AT2: a true abbreviated prefix still matches (mt#4039 regression) (pass) ... > AT3: a sha sharing fewer than 7 characters waits, exactly as before (pass) ... > AT4: with no observable head, nothing is classified and nothing is reported 26 pass / 0 fail / 57 expect() calls (2 files) $ bun test --preload ./tests/setup.ts src/adapters/shared/commands/session/pr-wait-for-review-command.test.ts (pass) formatTimeoutMessage > names the extended-abbreviation cause, and drops the wait-for-the-push remedy 18 pass / 0 fail / 64 expect() calls $ bun scripts/run-related-tests.ts <the 4 changed source files> 598 pass / 0 fail / 1447 expect() calls — 36 related files, incl. src/hooks/completion-manifest-regen.test.ts ``` Negative control: neutralized the discriminator itself (`>= MIN_ABBREVIATED_SHA_LENGTH` → `>= 999`), a full revert of the decision logic rather than just deleting the early return, per mt#4512. Result: **4 fail / 8 pass** — the originating-incident test, the threshold test, the case/whitespace test and AT1 all went red, while AT2, AT3 and AT4 stayed green. The control therefore discriminates the new claim from the behaviour SC5 requires preserved, rather than merely proving the file can fail. ``` (fail) classifyHeadShaMismatch (mt#4995) > the originating incident classifies as divergent-prefix (fail) classifyHeadShaMismatch (mt#4995) > the threshold is exactly MIN_ABBREVIATED_SHA_LENGTH, checked from both sides (fail) classifyHeadShaMismatch (mt#4995) > classification normalizes case and whitespace like the matcher does (fail) ... > AT1: a fabricated extension returns on the first poll, not at timeout (pass) ... > AT2 / AT3 / AT4 — unchanged behaviour, still green 8 pass / 4 fail ``` Typecheck: 0 errors across 8 projects (`.`, `packages/domain`, `packages/shared`, `services/reviewer`, `services/site`, `src/cockpit/web`, `tsconfig.hooks.json`, `tsconfig.scripts.json`); `infra/` skipped for uninstalled deps. Lint: 0 errors, 0 warnings over 4382 files. `format:check` clean. Deploy verification: this PR touches deploy surface — I initially tagged the commit `[no-deploy-impact]` from assumption and the `commit-msg` guard correctly denied it, naming all 7 staged files as deploy surface. The tag is removed and the claim retracted here. After merge I will run `deployment_wait-for-latest` for `minsky-mcp` with `notBefore` set to the merge timestamp and `expectCommitSha` set to the merge SHA, and read `buildIdentity` rather than treating SUCCESS alone as sufficient. ## Live verification Not a structural change under `/implement-task` §7a — no new persistence path, model-output channel, external-system probe, deploy-target wiring, or schema migration; the behaviour is fully determined by the injected `SessionPrWaitForReviewDependencies` seam and is covered by the tests above, including a negative control. No new external-system integration, so no live-exercise requirement. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 5, 2026
…ually arrived
## Summary
`packages/domain/src/ai/embedding-service-openai.test.ts` AT1 asserted
`expect(isRequestTimeoutError(err)).toBe(true)`. That fails with `Expected: true, Received: false`
and says **nothing** about what actually rejected. On 2026-09-04 it failed once under the full gated
suite and passed 3/3 in isolation, blocking a `session_pr_create` on unrelated work — and the only
record it left was those two words, so the cause could not be determined afterwards and the flake is
too rare to reproduce on demand.
This ships the diagnostic, not a fix. `timeoutVerdict(err)` returns `"TimeoutError"` on the passing
path and `"<name>: <message>"` otherwise, so the same failure names its own cause.
**It does not make the flake less likely.** It makes the next occurrence self-describing. A green run
after this change is therefore consistent with the flake still being present — AT4 states that bound
rather than reading green as fixed.
## The task's own premise was falsified during planning
The spec asserted a SLOW path — *"the wall-clock budget … can be crossed by scheduling delay."* The
failure line it pasted reads `[5.56ms]` against a `TIMEOUT_MS` of **150**: the request rejected ~27x
**faster** than the timeout it should have hit, so the abort never fired and nothing crossed a
budget. The falsifier was inside the spec's own transcribed quote. The cause is now deliberately
UNSTATED and the scope is written to finding it.
Both fixes the spec originally proposed were also unsound and were dropped: injecting a clock does
not apply (the timeout is a real `AbortSignal` on a real socket), and asserting on a constructed
error contradicts the suite's own docblock — *"These deliberately do NOT mock `fetch`: … a mocked
rejection would prove nothing about whether the fix works."*
## Key changes
- `timeoutVerdict(err)` — reads only the error the test already caught. No clock dependency, no
mock, real stalled server preserved.
- Applied to **both** real-socket assertions: AT1 and its batch sibling AT3. **Class, not instance** —
the file's other two `isRequestTimeoutError` sites construct a `DOMException` directly, are
deterministic, and are not in this class, so they are untouched.
- A deterministic test pinning what the diagnostic reports.
## Testing
Execution evidence:
**AT1** — the instrumentation's own check, with the classification forced to fail. Deterministic; it
does not wait on the flake to recur:
```
(pass) OpenAIEmbeddingService request timeout (mt#3444) > mt#4985: the timeout verdict names the
error that actually arrived [0.15ms]
```
**AT3** — passes in isolation (23 pass, up from 22 with the new test):
```
23 pass
0 fail
57 expect() calls
Ran 23 tests across 1 file. [26.36s]
```
**AT4** — the full main suite, the partition the incident occurred in:
```
17776 pass
10 skip
0 fail
50981 expect() calls
Ran 17786 tests across 1148 files. [346.55s]
(pass) mt#4985: the timeout verdict names the error that actually arrived [0.12ms]
(pass) AT1: a stalled single request rejects within the bound instead of hanging [155.12ms]
(pass) AT3: a stalled BATCH request rejects within the bound too [153.50ms]
```
Note the change-scoped runner is NOT sufficient evidence here: `run-tests-gated.ts` selected 1 of
1148 files for this diff, so it never exercises the load condition. The run above is
`run-tests-main.ts` unscoped.
**AT2 — negative control:** AT1 pointed at a dead port (`http://127.0.0.1:1/v1`), stalled server
left running so only the failure mode changes, and the test observed FAILING.
```
expect(timeoutVerdict(err)).toBe("TimeoutError");
Expected: "TimeoutError"
Received: "Error: Unable to connect. Is the computer able to access the url?"
(fail) AT1: a stalled single request rejects within the bound instead of hanging [4.01ms]
```
**The control corroborated the direction rather than just proving the probe can fail.** Two findings
the planning pass could only hypothesize:
1. **Timing matches the incident.** The control failed in **2.22ms** and **4.01ms** across two runs,
against the incident's **5.56ms** — same band, ~40x below the 150ms bound. The healthy path in
those same runs sits at **152–155ms**, i.e. exactly at the bound. A connection-shaped failure
reproduces the incident's timing signature; a slow path cannot.
2. **The connection error's `name` is plain `Error`, not `TypeError`** — which is exactly why
`isRequestTimeoutError` (a `name === "TimeoutError"` check) rejects it silently. The deterministic
test now asserts this VERBATIM observed string rather than an invented one.
**What the control does not buy.** It proves a connection failure produces this shape and this
timing. It does **not** prove that is what happened on 2026-09-04 — that run was not reproduced. The
instrumentation is what settles it, on the next occurrence, for free. Fix restored and re-verified
after the control.
Typecheck clean across 8 projects; lint clean over 4,399 files; prettier clean.
## Deploy verification
`isDeploySurfaceFile` returns **true** for the changed file — run as the predicate over the actual
changed-file list, not recalled from a pattern list:
```
true packages/domain/src/ai/embedding-service-openai.test.ts
```
That is a surprising result for a test file and is precisely why the tag was not written. **No
`[no-deploy-impact]` claim is made anywhere in this PR or its commit message.** Post-merge I will
wait on the deployment bound to this merge (`notBefore` = merge time, `expectCommitSha` = merge SHA)
and assert the health body's service identity rather than the status code.
## Parallel work
No collision. `git_log --path` over the changed file and over `request-resilience.ts` for 7 days:
`pathMatched: true`, no commits. Two open PRs were real candidates and both were read by
changed-file list rather than title: **PR #3590** (mt#3575, test-suite order-independence and
randomization) touches 15 files, none under `packages/domain/src/ai/`; **PR #3412** (mt#4639,
614-site log conversion) touches **313** files — enumerated across all four pages, since a single
100-item page is a truncated list whose "no hit" is worth nothing. Six of its files are under
`packages/domain/src/ai/`, and the changed file is not among them.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 8, 2026
… the Detail column
## Summary
The cockpit's providers table rendered `lastValidationDetail` in its Detail column. That string is
written for **transient post-action feedback** — a sentence shown inline, untruncated, to someone
who just pressed Validate or Add. The column asks a different question ("what is currently true of
this credential") and is 276px wide, so two providers' strings were silently cut to roughly half.
The principal reported it after storing a subscription token:
> "in the detail column for the claude code one, I see this text, which is both cut off and also weird"
**This is mt#5027's AT4 acceptance judgment, and it is negative.** That task deferred the copy's
acceptance to the principal's reading; this is that reading. Its wording is correct for the surface
it was designed for and wrong for the surface the principal saw.
### It was a class, and the reported instance was not the worst one
Measured live against the operator's cockpit before the change:
| provider | chars | needs | column | truncated |
| --- | --- | --- | --- | --- |
| telegram | **95** | 523px | 276px | **yes, ~47% lost** |
| claude-code-token | **84** | 465px | 276px | **yes, ~41% lost** |
| github | 46 | 276px | 276px | no — exactly at the boundary |
| railway / google / supabase | 18–20 | 276px | 276px | no |
Telegram's is longer than the reported one and is an *instruction* ("send your bot one message
so…"), which is even less appropriate for a status column. A `claude-code-token`-only fix would
have repeated mt#5027's original mistake of treating a class as an instance.
### Why shortening the copy could not have worked
mt#5027 bounded the new string at **"under 15 words"**. The shipped string is ~13 words — it passed
— and 84 characters. The column is bounded in **pixels**. The unit was wrong, so the check that
existed to prevent this could not see it.
## Key Changes
- **`CredentialCheckResult.status`** — a short state form beside the existing `detail`, persisted as
`lastValidationStatus`. Optional, and absence is safe by construction.
- **`credentialStatusLine`** (`web/lib/credentials-api.ts`) — the column now reads this and never
`lastValidationDetail`. Five rungs: provider status → nothing for a presence-only row → nothing
for an unconfigured one → a *short* stored detail (back-compat) → a derived state. **A provider
that supplies no status, or a stored detail over budget, cannot truncate the column** — that is
the structural guarantee, not a convention.
- **All eight providers** supply a status. Two got distinct ones (`stored, unverified`;
`valid; no chats discovered yet`); the rest already read as states and repeat theirs.
- **`MAX_STATUS_LENGTH = 40`**, derived from the 276px measurement rather than chosen.
- **`listPresence()`** keeps carrying `detail`, with the reasoning recorded in code: nothing
truncates that payload, so the constraint motivating `status` does not apply there.
- **The full sentence stays reachable** as a `title` tooltip, and is unchanged on both transient
surfaces (Settings form and the masked entry form on a credential-request ask).
### Spec reconciliation: the budget is 40, where SC1 says ≤ 46
SC1 derives a ≤46-char working budget from the measurement. I implemented **40**, which satisfies
that criterion (strictly tighter) but is a deliberate departure worth naming, because it has a
visible consequence. 46 is the ceiling *at a 1440px viewport with zero headroom* — the measured
GitHub row needed exactly the full 276px — so a 46-char value truncates in any narrower window. 40
leaves room.
The cost: GitHub's *stored* pre-split detail is 46 chars, so the back-compat rung declines it and
that row reads `validated` until its next recheck. Recorded in `## Live verification` below rather
than left to be discovered.
## Testing
Execution evidence:
```
$ bun test --preload ./tests/setup.ts packages/domain/src/credentials/providers/status-line.test.ts
11 pass 0 fail 28 expect() calls Ran 11 tests across 1 file.
$ bun test <components invocation> ./src/cockpit/web/lib/credentials-api.test.ts \
./src/cockpit/web/widgets/Credentials.test.tsx
25 pass 0 fail Ran 25 tests across 2 files.
$ bun test --preload ./tests/setup.ts packages/domain/src/credentials/
222 pass 0 fail 468 expect() calls Ran 222 tests across 13 files.
$ validate_typecheck -> pass, 8 projects, 0 errors
$ validate_lint -> pass, 0 errors, 0 warnings, 4412 files
```
**AT1** — rendered the real table at 1440x1000 and measured `scrollWidth` vs `clientWidth` per cell
rather than eyeballing a screenshot: **zero rows truncated** (see `## Live verification`). This is
SC1's mechanical half.
**AT2** — `the transient surface still shows the sentence in full (SC3)`: drives the form and
asserts the 84-char string is rendered *complete* inline. This is the half a "just shorten the copy"
fix would break while passing every column assertion.
**AT3** — `Credentials widget — Detail column states`: all four states through the rendered row —
provider status, pre-split long detail, configured-never-checked (SC4), presence-only. Plus a
document-wide negative that the 84-char prose appears in no cell at all, and a `title` assertion
for SC6.
**AT4** — read back `GET /api/credentials` from a cockpit running this build; every column-bound
value is a state. Numbers in `## Live verification`.
**AT5** — principal's reading. Outstanding by design; this is subjective-quality acceptance and
theirs, so the render is presented without a verdict.
**SC2** is structural, per `credentialStatusLine` above, plus a class sweep asserting every
provider's status literal is within budget. **SC5** is the back-compat rung, tested with the two
real pre-split strings and documented in `CredentialMeta`. **SC7** is recorded in
`request-resolver.ts`.
Negative control — the render rule: reverted `credentialStatusLine` to the full pre-fix behaviour
(return the stored detail directly). **5 of 10 failed**, including "the two REAL over-budget details
are never rendered" and "every value fits the budget". Rungs 2–4 passed under the control, because
the old code coincidentally agreed on those inputs — so those three assertions are not
discriminating on their own, and I am not claiming them as such.
Negative control — the component wiring: reverted `CredentialRow` to render `lastValidationDetail`
directly. **Both new component tests failed.** This is the control that matters for the actual
defect: a green rule test is compatible with a row that never calls the rule.
## Live verification
Built and ran this branch's cockpit from the session workspace on :3941 — health body confirms
`"service":"minsky-cockpit"` and `"commit":"6f664fd15"`, so the probe is against this build, not the
operator's. Same CDP measurement as the before-table, same viewport, the operator's real credential
store.
| provider | before | after | truncated after |
| --- | --- | --- | --- |
| claude-code-token | 84 chars, cut at 276/465px | `validated` | no |
| github | `authenticated as @edobry; …scope present` | `validated` | no |
| anthropic | *(empty cell)* | `configured, never checked` | no |
| supabase | `4 projects visible` | `4 projects visible` | no |
| google | `50 models accessible` | `50 models accessible` | no |
| railway | `railway:Eugene Dobry` | `railway:Eugene Dobry` | no |
**Zero truncated rows.** Two things in that table are worth stating plainly rather than leaving to
be noticed:
- **github reads `validated` rather than its identity line** — the budget consequence described
under Spec reconciliation. It self-heals to `authenticated as @edobry` on that credential's next
recheck. I did not force a recheck: that writes to the operator's credential store and makes live
API calls with their tokens, which this task does not authorize.
- **telegram could not be verified live, and its row is absent above for that reason.** In the
session environment its `isConfigured()` shells out to Pulumi with no stack selected, so it reads
`configured: false` (confirmed: `false` on :3941 vs `true` on the operator's :3737) and takes the
unconfigured rung. **My live probe therefore cannot show telegram's real post-fix rendering** —
that path is covered by unit tests using the real 95-char string, not by this run.
Deploy verification: every changed file returns `true` from `isDeploySurfaceFile` (ran the
predicate over the diff rather than recalling the pattern set), so this is deploy surface and §10
applies. I will confirm the post-merge deploy with `notBefore` = the merge timestamp and
`expectCommitSha` = the merge sha, and re-read the rendered column on the operator's cockpit once
the tray rebuild picks it up.
## Notes
The reported bug's sibling — a UI flicker when pressing Add, reported in the same message — is
filed separately as mt#5032 and deliberately not touched here. Both edit
`src/cockpit/web/widgets/Credentials.tsx`, so they should land sequentially.
PR #3412 (the 614-site logging sweep) also touches `request-resolver.ts`, in a different function
~25 lines from this change. No line-level overlap; whichever lands second rebases trivially.
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 9, 2026
## Summary
ADR-002 listed *"PostgreSQL may or may not have pgvector extension available"* as an environmental
constraint and decided a runtime-probing factory that returns a vector provider **or** a plain one.
ADR-027 re-affirmed that axis. **Neither branch was reachable.** The schema declares `vector(1536)`
columns in six tables plus six HNSW indexes, and the fresh-DB bootstrap snapshot's first statement
is `CREATE EXTENSION IF NOT EXISTS vector` — so a Postgres without pgvector fails at migrate with
SQLSTATE `0A000`, exit 1, zero tables written, before any provider is constructed (measured on a
stock `postgres:17` during mt#5016).
Two Accepted ADRs therefore documented an optionality the product does not offer. The principal
settled it via [ask#11882](minsky://ask/38b1c0de-0000-0000-0000-000000000000) — **"Require
pgvector"** — with the cost stated and accepted: this closes off "runs on any Postgres", which
matters if a customer's managed Postgres lacks the extension.
## The judgment call: the probe is converted, not deleted
The task's `## DECIDED` section said to retire *"the `SELECT EXISTS` check, the three-way outcome,
and the `pgvectorVerified: false` construction path"* as one unit. Reading the code, those are three
different things, and **the first should not be retired**:
- `PostgresVectorPersistenceProvider.initialize()` skips its own re-probe precisely when the factory
reports one already ran (mt#2973, `postgres-provider.ts:1077`). Hardcoding `pgvectorVerified: true`
would therefore remove the **last** pgvector check on *both* paths — asserting a capability nothing
verified, which is the opposite of requiring it.
- `PostgresPersistenceProvider` is the **base class** of the vector provider, not merely the degraded
branch. "Retire the branch" can only mean the factory stops constructing it directly.
- mt#3833's inconclusive/absent distinction matters **more** under a hard requirement, not less:
both now fail, so collapsing them would misreport the cause and invite the wrong remedy.
So the branch retires and the query stays, as a **precondition assertion**. This is an agent-level
call — it names no reserved category, the principal's decision (the ADR reconciliation direction) is
unchanged — and it is recorded in the spec as contestable.
| probe outcome | before | after |
| --- | --- | --- |
| `present` | vector provider | vector provider (unchanged) |
| `absent` | base provider, silently degraded | `VectorExtensionAbsentError` — **not** retryable |
| `inconclusive` | `VectorCapabilityProbeInconclusiveError` | unchanged — still retryable |
## Key changes
- **`docs/architecture/adr-002-…md`** — dated addendum naming the three superseded statements
(`:30` constraint, `:40-42` framing, `:237` "same code works in dev (no pgvector) and prod"), the
mt#5016 evidence, what is *not* retired, and the decision provenance. Extends the file's existing
addendum convention (it already carries ADR-027 2026-07-07 and ADR-035 2026-08-03).
- **`docs/architecture/adr-027-…md`** — matching addendum. Its core claim (the axis is
intra-Postgres, not cross-backend) is unchanged and strengthened; only "supported deployment
choice" is superseded. `§Deferred`'s PGlite note is explicitly unaffected — PGlite is
pgvector-capable, which is exactly what this now requires.
- **`vector-capability-probe.ts`** — new `VectorExtensionAbsentError`, sibling to the inconclusive
error. The docblock states the axis that makes them two classes: **retryability**.
- **`postgres-provider-factory.ts`** — branch → assertion; return type narrowed to
`Promise<PostgresVectorPersistenceProvider>`; the `catch` no longer logs a probe that *answered*
as a probe that *failed*.
- **`postgres-provider.ts`** — `pgvectorVerified`'s docblock now says it is a cold-boot signal, not
a capability verdict, and why its `false` default is still load-bearing on the standalone path.
## Testing
Execution evidence:
**SC4 / SC5 / AT2 — factory asserts instead of branching, and the two errors stay distinct:**
```
$ bun test --preload ./tests/setup.ts --timeout=15000 \
packages/domain/src/persistence/providers/postgres-provider-factory.test.ts \
packages/domain/src/persistence/vector-capability-probe.test.ts \
packages/domain/src/persistence/providers/postgres-provider.test.ts
79 pass
0 fail
Ran 79 tests across 3 files. [1097.00ms]
```
**SC6 / AT4 — return type narrowed; no caller depended on a degraded provider:**
```
validate_typecheck(task: "mt#5037") → status: pass, errorCount: 0
workspaces: [".", "packages/domain", "packages/shared", "services/reviewer",
"services/site", "src/cockpit/web", "tsconfig.hooks.json", "tsconfig.scripts.json"]
validatedWorkspace: /Users/edobry/.local/state/minsky/sessions/0ebe13c4-…
skippedProjects: [infra/tsconfig.json — deps not installed locally; CI runs it with its own install]
```
AT3 — negative control: the three new factory tests against the pre-fix factory
`git restore --source=HEAD~1` on the factory alone, then the same test file:
```
(pass) a readable 'present' answer yields the vector provider
(fail) a readable 'absent' answer is now a fault — pgvector is a prerequisite
(fail) 'absent' is distinguishable from 'inconclusive' — different errors, different remedies
(fail) the probed client is ended when the extension is absent, so the pool does not leak
(pass) throws … when the probe returns zero rows
(pass) throws … a row with no exists column
(pass) throws … a row whose exists is null
(pass) throws … a row whose exists is the string 'f'
(pass) the probed client is ended when the probe is inconclusive
(pass) the string 'f' throws rather than being read as truthy
7 pass
3 fail
```
Exactly the 3 new tests fail and all 7 pre-existing ones still pass — the control **discriminates**
rather than breaking the suite wholesale, which is what distinguishes a faithful revert from an
inert test (mt#4502). Restored to `HEAD` afterwards: `git status --porcelain` clean, 10 pass / 0 fail.
**SC1 / SC2 / SC3 / AT1 — both addenda present, citing mt#5016's measurement:** in the diff;
`adr-002` and `adr-027` each carry a `2026-09-09, mt#5037` block naming SQLSTATE `0A000` on a stock
`postgres:17`.
**AT1's live half is UNVERIFIED** — running `minsky persistence migrate` against a plain
`postgres:17` container was not exercised in this session. Stated rather than implied: the *refusal*
behaviour it asserts is mt#5016's already-merged preflight, unchanged by this PR, and the ADR half
is checkable by reading the diff. The new factory assertion is covered by unit tests above.
Deploy verification: this PR changes deploy surface (`packages/domain/src/persistence/**` is
application source). Post-merge I will wait on the deployment bound to this merge
(`notBefore` = merge time, `expectCommitSha` = merge SHA), read `buildIdentity`, and assert the
health body's `service` identity rather than the status code.
## Planning provenance, including a gate failure
The `/plan-task` pass recorded gate (g) as PASS and it was a **false negative** — the
`parallel-work` guard caught PR #3412 (a 614-site repo-wide refactor) touching
`postgres-provider.ts` ninety seconds later. The gate's open-PR sweep narrowed 18 PRs with a
subsystem keyword filter, and a repo-wide mechanical refactor names no subsystem token by
construction. Corrected in the spec's audit with the generalizable form. Overridden via
`grant-guard-override.ts` on evidence: #3412 is `mergeableState: dirty`, carries 14 unaddressed
BLOCKING findings since 2026-09-04, and last saw a real commit on 2026-08-27 — so it is not a
bounded wait, and it must be fully rebased by whoever revives it regardless.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_012aTeW8XXjVuEF5GykykSwq
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 11, 2026
…he peer advisory
## Summary
`warn-peer-task-activity` could only say "a `session.started` you may not have caused", because the row carried no writer. On 2026-09-10 a conversation read exactly that as its own subagent's doing — the session had been started three minutes after it dispatched an opinion subagent — when a different, principal-launched conversation working a second succession package had created it. That attribution reached five durable records before the subagent's transcript falsified it (mt#5055, CLOSED with the `PROBLEM STATEMENT FALSIFIED` banner). This PR makes the inference unnecessary.
## Key Changes
- **`session.start` records who created the session.** A hidden, server-injected `callerActorId` parameter (`cliHidden` + `mcpHidden`, same shape as `tasks.claims.release`'s) is added to `sessionStartCommandParams`; `session.start` joins `CALLER_ACTOR_ID_TOOL_NAMES` in `src/mcp/server.ts`; `emitSessionStartedEvent` writes `resolveCallerActorId(callerActorId)` as the event's `actor`, and null stays null — never a fabricated id.
- **The advisory says whose it is.** `TaskEventRow` gains `actor`; a new pure `relateSessionActor` compares a conversation-scoped writer id (`…:conv:<uuid>`) against the hook input's `session_id` and `decidePeerActivity` renders `started by <actor> (this conversation)` / `(ANOTHER conversation — not yours, not your subagent's)` / `(comparison to your own id not made — see below)` / `no writer id on this row`. When a row is another conversation's, a paragraph states why that rules out a subagent: a subagent's MCP calls carry its parent's id (`resolveLiveConversationAgentId` takes only the harness pid and the spawn-time env — verified in source during planning). When the id is `proc:`-scoped (the shim's fallback with no pid→conversation mapping), the hook prints it, says the comparison was not made, and points at the reader's own `claimedBy`. Every fire now closes by naming the falsifier for "my subagent did it": `<session-dir>/subagents/agent-<id>.jsonl`.
- **Nothing else moves.** The cwd-based self-suppression runs first and is unchanged; `task.status_changed` rows stay unattributed; the advisory still never denies (mt#4788 owns the posture flip and is sequenced to consume this field). `decidePeerActivity` gains a trailing optional parameter, so `turn-end-stale-state-assertion-scan`'s call is untouched. One static import into the hook — `conversationIdFromAgentId` from `agent-identity/format.ts`, a leaf module (`format.ts` → `kinds.ts` → nothing), outside what `domain-bootstrap.ts` layer 1 guards.
- Docs: `docs/architecture/hooks/warn-peer-task-activity.md` gains the rendering table and incident; `hook-observers.mdc` entry updated and recompiled (`.claude/rules/hook-observers.md`, `.cursor/rules/hook-observers.mdc` both carry it — verified by grep, not by exit code).
- Spec reconciliation: SC2's label wording ("another process" / "this process") was amended in the spec to the conversation grain the writer id actually carries; the amendment and its basis (planning audit premise (i)) are recorded on the criterion itself.
## Testing
Execution evidence:
- `bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/warn-peer-task-activity.test.ts` → **27 pass / 0 fail** (8 new cases under `mt#5086 — a session.started row names its writer…`).
- **AT2** — `relateSessionActor pins each branch`; `AT2 — a row started by ANOTHER conversation is labelled so, and the subagent inference is ruled out` (replays the 00:14Z row with conversation `e7da3c7d`'s id against reader `dd1a36b5`); `AT2 — a row started by THIS conversation is labelled so`; `AT2 — a row with actor null renders the unattributed form`; plus the `proc:` "not compared" case and the no-caller-id case.
- **AT3** — `the cwd-based self-suppression still wins over the actor label`, and every pre-existing case (mt#4439 replay, PR #3281 R1 window/suppression cases) unchanged and passing.
- **AT4** — live `tools/list` on a server built from this branch: `session.start` params are `sessionId,task,description,branch,repo,quiet,noStatusUpdate,skipInstall,packageManager,recover`; `callerActorId advertised: false`. Contract-level: the `collectMcpHiddenParamKeys` machinery is mt#4579's, exercised by `src/mcp/mcp-hidden-params.test.ts`.
- **AT1** — see `## Live verification`: a `session.start` over MCP against this branch's server wrote a `session.started` row with `actor` populated with the id the server resolved for that client.
- **SC1** — AT1's row (`actor: "unknown:hash:e988ed544601b00c"` — the Layer-1 id for a raw-SDK client whose name is no known harness; the resolver's OUTPUT is what is stamped, whatever scope it yields) and the CLI-path null branch in `emitSessionStartedEvent` (`...(actor ? { actor } : {})`). **SC2** — the AT2 label cases. **SC3** — `SC3 — every fire names the subagent transcript as the falsifier, even a plain status-change fire`. **SC4** — `SESSION_START_TOOL_NAME` in `CALLER_ACTOR_ID_TOOL_NAMES` + AT4.
- `bun scripts/run-related-tests.ts` over the four changed source files → **491 pass / 0 fail across 34 files** plus **46 pass / 0 fail** (`src/mcp/server.test.ts`, `basic-commands.test.ts`, `claims-release.test.ts`, `session-parameter-family-parity.test.ts`, `hook-module-inventory.test.ts`, `interceptor-coordinates.test.ts`, `turn-end-stale-state-assertion-scan.test.ts` among them).
- **R1 (registration invariant, BLOCKING)** — `src/mcp/server-tool-name-resolution.test.ts` gains `mt#5086 — session.start is REGISTERED under its dotted id, so an alias caller still hits the callerActorId injection`: it builds the real `createSessionStartCommand`, registers it through `CommandMapper.addCommand({ name: command.id })`, asserts `tools.get("session_start").name === "session.start"`, then calls the tool as **`session_start`** over an in-memory client with a spoofed `callerActorId` and asserts the handler received a resolved id instead. The chain it cites: `registerToolsCommandsWithMcp` → `addCommand({ name: command.id })` (`shared-command-integration.ts`), `normalizeMethodName` strips only `[^a-zA-Z0-9._-]` (a dot survives), `addTool` maps both spellings to one object. Run: **5 pass / 0 fail**. `src/mcp/mcp-hidden-params.test.ts` gains a case over the REAL `sessionStartCommandParams` (`collectMcpHiddenParamKeys` → `["callerActorId"]`): **11 pass / 0 fail**.
- `validate_typecheck` (session): 0 errors across 8 projects. ESLint on the changed source files: clean (a `prefer-template` error and two magic-string warnings were fixed before commit).
Negative control — R1's alias-injection test: with `SESSION_START_TOOL_NAME` commented out of `CALLER_ACTOR_ID_TOOL_NAMES`, the same test fails with `Expected: not "spoofed-by-caller"` (the caller's value reached the handler); restored, 5 pass.
Negative control — the advisory: with `.minsky/hooks/warn-peer-task-activity.ts` reverted to `main` (`git stash` of that one file) and the same peer-conversation row passed to `decidePeerActivity` with the reader's conversation id, the pre-fix message contained neither `started by` nor `ANOTHER conversation` — it rendered `session.started 3m ago — session bc12bbc4-…` and nothing about the writer. The new test file against the reverted hook: 0 pass / 1 fail (import of `relateSessionActor` missing).
## Live verification
Server built from this branch (`bun run <session>/src/cli.ts mcp start --http --port=39111`, cwd = main workspace, throwaway static bearer token), driven with `@modelcontextprotocol/sdk`'s `StreamableHTTPClientTransport` (client name `mt5086-at1-smoke`):
```
session.start {task: "mt#5055", noStatusUpdate: true, skipInstall: true} → session 0cee7472-75ef-4605-9c79-396eb39f1a45
events.list {relatedTaskId: "mt#5055", eventType: "session.started"}:
"actor": "unknown:hash:e988ed544601b00c",
"relatedTaskId": "mt#5055",
"relatedSessionId": "0cee7472-75ef-4605-9c79-396eb39f1a45",
"createdAt": "2026-09-11T01:28:05.280Z"
```
The scratch session was deleted afterwards (`session_delete`, override reason recorded); mt#5055 stayed CLOSED (`noStatusUpdate`). The one `session.started` row on mt#5055's ledger is the residue of this check.
Deploy verification: `src/mcp/server.ts`, `basic-commands.ts` and `session-parameters.ts` are deploy surface (`isDeploySurfaceFile` → true; the seven hook/rule/doc files → false). After merge I will run `deployment_wait-for-latest` for `minsky-mcp` with `notBefore` = the merge timestamp and `expectCommitSha` = the merge sha, read `buildIdentity`, and assert the health body's `service` is `minsky-mcp`. Local daemon: the next `session_start` from any conversation after the rebuild should write an `actor`; I will read one back via `events_list`.
## Sequencing
mt#4788 (flip this advisory to DENY) wants the field this PR adds; its spec now carries a sequencing note. PR #3412 (614 log-site rewrite, stale since 2026-09-04) touches `basic-commands.ts` and `server.ts` mechanically; recorded as proceed-acknowledged in mt#5086's planning audit — whoever moves second rebases, and the hunks do not intersect.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01VuuiE8iQYtmaajWTbJmhzK
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 11, 2026
… with one proximity check
## Summary
`wall-of-text-detector`'s `DEPTH_REQUEST_PATTERNS` withholds the over-budget reminder when the principal recently asked for depth. Eight of its ten entries also matched the NEGATION of their phrase — "dont walk me through everything, just the summary", "no need to give me the full breakdown", "please do not go into more detail" — so the reminder was suppressed exactly when the principal asked for brevity (the unsafe direction by the list's own narrowness note). Verified at planning: all 8 return `matched: true` on the shipped list.
Measured in the live calibration log (174 records since 2026-08-30, the only copy on disk): 16 depth-suppressed records, 13 replayable through the hook's own window logic, **all 13 `help-me-understand`, 0 negated** — so this ships as a latent robustness fix, sized accordingly.
## Key changes
- `.minsky/hooks/wall-of-text-detector.ts` — `isNegatedDepthRequest(text, index)` + `DEPTH_REQUEST_NEGATOR_RE`: a match is rejected when a negator (`don't`/`dont`/`do not`/`no need to`/`never`/`not`/`rather than`/`instead of`/`without`) ends within three tokens before the phrase in the same clause (boundaries: `,;:.!?()—–`, newline, and contrastive `but`). `detectDepthRequest` now scans every occurrence of every entry, so a negated first mention does not hide a later genuine request. **One mechanism at the seam, not eight regex edits** (mt#4070's drift argument), and **not per-entry anchoring** like `tell-me-more`: 9 of the 13 live suppressions are mid-sentence ("…and also, help me understand…", "proceed, but first help me understand…") and the anchored form misses 4 of 6 live prompt shapes — anchoring would un-suppress most genuine requests, which is the friction incident mt#3112 exists to prevent. The negator list grows only on a calibration record naming a missed negator, per the list's own evidence-before-expansion discipline (docblock).
- `.minsky/hooks/wall-of-text-depth-request.test.ts` — `mt#5052` describe block, 7 tests.
- `.claude/hooks/wall-of-text-detector.ts` — regenerated mirror.
## Testing
Spec ATs use the task's own numbering.
Execution evidence:
- **AT1** — the spec's eight negated prompts return `matched: false` (`AT1 — each negated prompt from the spec's table is left unmatched`).
- **AT2** — every entry's calibrated phrase still matches under the same name, plus the mid-sentence live shapes and the benign-negator shape "I'm not sure I follow — help me understand the peez thing" (`AT2 — …` ×2, `the guard's window is tight …`).
- **AT3** — `bun scripts/measure-depth-request-widening.ts --log <state-dir>/wall-of-text-calibration.jsonl` → `window: head -120 -> 120 records; 75 injected / 45 suppressed`, `AT1 — newly suppressed: 3 (expected exactly 3)`, `PASS — AT1, AT2, AT3 hold on this window`. (The script exercises the entries directly, so this proves the list is untouched; the guard's effect on the live matches is the planning replay — 0 of 13 negated — plus the mid-sentence fixtures above.)
- **AT4** — `bun scripts/run-guard-canaries.ts` → `[PASS] wall-of-text-detector (registry, expects=calibration)`; `Total: 75 Passed: 73 Failed: 0 Missing: 2` (the two missing are pre-existing, unrelated to this guard).
- **AT5** — the negative control below.
```
$ bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/wall-of-text-depth-request.test.ts ./.minsky/hooks/wall-of-text-detector.test.ts ./.minsky/hooks/wall-of-text-turn-window.test.ts
169 pass
0 fail
Ran 169 tests across 3 files. [80.00ms]
$ bun scripts/run-related-tests.ts .minsky/hooks/wall-of-text-detector.ts
Ran 2899 tests across 61 files. [23.90s]
run-related-tests.ts: 61 related test file(s) passed
```
`validate_typecheck` (session): 0 errors across 8 projects. ESLint on the two source files at `--max-warnings=0`: clean. `bun run format:check`: clean.
Negative control: with the guard disabled in place (`if (true || !isNegatedDepthRequest(…))`) and the new tests kept:
```
(fail) mt#5052 — negated depth requests do not suppress > AT1 — each negated prompt from the spec's table is left unmatched
(fail) mt#5052 — negated depth requests do not suppress > the guard's window is tight — a negator outside it does not defeat a request
(fail) mt#5052 — negated depth requests do not suppress > the guard reaches a negator up to three tokens before the phrase
11 pass
3 fail
Ran 14 tests across 1 file.
```
The AT2 tests (genuine phrases, mid-sentence shapes) pass in both states, as pinned; restored afterwards (`grep -c "NEGATIVE CONTROL"` → 0, 169/169).
`[no-deploy-impact]` — `isDeploySurfaceFile` returns `false` for all three changed files (run over the diff, not recalled).
## Not in this PR
The `.codex/hooks` mirror (PR #3253 makes it a compile output); PR #3412's disjoint edits to the same file (an import and `main()`'s catch) — no conflict with this region.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01TPNyYTreM4F7PRbvh4xA5b
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 11, 2026
## Summary
Phase 4 of the compile-pipeline convergence (mt#2293 / ADR-016). The mt#3058 cutover moved `claude.md` / `agents.md` / `claude-rules` onto the new-pipeline `runCompileCheck` and left the legacy `runRulesCompileCheck` as a no-op shell still registered as pre-commit Step 9 under the instrumented name `rules-compile-check`. This deletes the remainder, so one compile-staleness step remains.
## Changes
- `src/hooks/pre-commit.ts` — Step 9 registration and `runRulesCompileCheck` deleted; the surviving `compile --check` step is now Step 9 and the only compile-staleness step, over all seven targets, still carrying `MINSKY_SKIP_SIZE_BUDGET`. `classifyCompileCheckError` drops its `kind` parameter (the only caller passed `"compile"`; the legacy prefix has no emitter in the hook any more) and its stale "legacy only" comment on the size-budget branch is corrected — `packages/domain/src/compile/size-budget-report.ts:93,119` emits both budget markers under `[compile --check]` since the cutover. Its stale-case hint now reads `minsky compile --target <t>`, the same shape as the compile CLI's own hint (R1). Comments naming the deleted method updated.
- `src/hooks/rules-compile-check.test.ts` → `src/hooks/compile-check.test.ts` — same 37 cases, markers repointed at `[compile --check]`; plus (R1) a pin on the hint shape and a two-test pin that pre-commit wires `compile-check` exactly once and the retired name not at all, and that the guard roster carries one and retires the other.
- Interceptor registries: `rules-compile-check` moved to the RETIRED stratum rather than deleted (R1) — `RETIRED_GUARD_NAMES` entry (lastSeen 2026-09-11), a retired-stratum description with `provenance: [KNOWN_NAMES]`, retired coordinates, and the authored-point manifest in `interceptor-coordinates.test.ts` — so its 4,435 historical fire-log rows keep resolving instead of reading as anomalies. The `compile-check` description now says it covers the rule-edit gap and its provenance names the repointed test. Generated `.claude/hooks/` copies and `src/generated/interceptor-catalog.json` regenerated.
- Docs: ADR-016 gains `### Phase 4 shipped (mt#2993)`; `docs/architecture.md:330` and a `crud-operations.ts` comment no longer name the deleted method. `docs/architecture/evaluation-loop-phase2.md`'s two mentions are a dated measurement record and are left as history.
## Deploy verification
`isDeploySurfaceFile` flags `src/hooks/pre-commit.ts`, `src/generated/interceptor-catalog.json` and the comment-only `crud-operations.ts`; the runtime behaviour change is confined to the local pre-commit hook. After merge: `mcp__minsky__deployment_wait-for-latest` for the affected services with `notBefore` = merge time, workflow-run correlation at the merge SHA, and a health read — SUCCESS plus runtime started, recorded on the task.
## Execution evidence
Per success criterion:
- **SC1** — `grep -rn runRulesCompileCheck src/hooks/` returns nothing (only ADR-016's historical Context and its new Phase-4 note mention the name repo-wide); pinned by the new test.
- **SC2** — repointed, not removed: `bun test --preload ./tests/setup.ts src/hooks/compile-check.test.ts` → `39 pass / 0 fail` (37 original + 2 R1 pins).
- **SC3** — one step: the new test asserts `this.instrumented("compile-check", …)` appears exactly once in `pre-commit.ts` and `rules-compile-check` not at all; the step's log line names all seven targets (`pre-commit.ts:2386`, unchanged). (The commit's own pre-commit run also wrote one `compile-check` fire-log record and zero `rules-compile-check` — a local observation, now superseded by the in-repo pin.)
- **SC4** — the `kind="rules"` branch is removed (parameter deleted), with the reason in the classifier docblock.
- **SC5** — `MINSKY_SKIP_SIZE_BUDGET` threaded on the surviving step's registration and consulted inside `runCompileCheck` — verified, not assumed.
- **SC6** — `claude-agents` is in the surviving step's target list; `pre-commit.ts` records that mt#2497 reconciled the drift and subsumed mt#1654. Nothing carried forward.
- **SC7** — `bun run test:hooks`: `7177 pass / 0 fail` (195 files, includes the interceptor census tests, re-run after R1); `bun scripts/run-related-tests.ts` over the changed sources: `424 pass / 0 fail` (24 files); `validate_typecheck` 0 errors across 8 projects (`infra/` skipped, not installed locally); `validate_lint` clean on the changed files; prettier unchanged.
Acceptance tests: **AT1** as SC1. **AT2** — appended a line to `CLAUDE.md` in the session workspace: `compile --check --target claude.md` → exit 1, `[compile --check] Target "claude.md" is STALE`; restored → exit 0. **AT3** as SC3.
**Negative control (test-first, mt#3244):** the repointed `compile-check.test.ts` run against `main`'s `pre-commit.ts` (classifier defaulting to the legacy prefix) → `25 pass / 12 fail`; all pass against this branch.
## R1 (review round 1)
- **BLOCKING — hint spelling.** `bun run minsky …` does run here (package.json `minsky` script) but is repo-only; the compile CLI (`compile-commands.ts:233,266`) and pre-commit's setup hint (`:2998`) both say `minsky …`. Aligned; pinned in the staleness test (`Run "minsky compile --target agents.md"`, and `not.toContain("bun run minsky")`).
- **Census counts** — the first push had removed the name outright; the census's "zero silent drops" test then failed on the `RETIRED_GUARD_NAMES` entry, and the authored-point manifest on the coordinates. Both are the append-only manifests the repo uses for exactly this, now updated; `test:hooks` 7177 pass.
- **Remediation-text assertion** — added (above).
- **Sweep beyond the updated files** — `grep -rn 'rules-compile-check\|runRulesCompileCheck' src packages docs .minsky scripts tests`: only ADR-016's historical Context lines (`:11`, `:21`, describing the pre-convergence state the ADR decided against) and `evaluation-loop-phase2.md`'s dated measurement; no operational guidance names the step.
- **Runtime one-step check** — added as a source-text pin (the step roster is inline `this.instrumented(…)` calls, not data), plus the roster/retired-set assertion.
- **Out-of-repo fire-log evidence** — superseded by the in-repo pin; the observation is kept in SC3 as what prompted it.
## Parallel-work note
Open PR #3253 (mt#3854) also edits `src/hooks/pre-commit.ts`, in the `compileCheckTargets` region (+15 lines of its own, adding a `codex` presence field); this PR touches that region only in one docblock sentence. Its branch predates mt#4866/mt#4986/mt#5003 and needs a rebase regardless. PR #3412 (mt#4639) lists the same files but makes no change of its own to them (three-dot diff empty).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01UG8FZC1RHk7otPDDfs2zrr
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 13, 2026
…versation date ## Summary `pre-narration-detector` (log-only) suppresses a claimed PR outcome only on evidence it can read from the parent transcript's tool NAMES and tool INPUTS. That leaves the orchestrator pattern unsuppressible: an implementer subagent creates the PR in ITS transcript, its report reaches the parent, and the parent's "PR #3723 is up for mt#4959" fires. Measured over the detector's whole post-mt#4810 corpus (2026-08-30 → 09-13, 265 records): **12 unsuppressed, 12 false** — 7 relays of a subagent-created PR, 3 claims dated before their conversation began, one relay of a subagent's research report, one undated catch-up-table entry. **One correction to the spec's mechanism, found at planning and folded into the criteria:** the subagent's report is NOT the `Agent` tool's `tool_result`. For a background agent that result is the launch stub (`Async agent launched successfully…` — all 10 in `f290bb69`); the report arrives as a **string-content user line** carrying a `<task-notification>` envelope (`<summary>Agent "…" finished</summary>` … `<result>PR created: **#3723** …</result>`). Implementing SC1 as originally written would have suppressed 0 of 7. Replaying the 7 relays through the detector's own `windowSlice`: 7 of 7 have such a line naming the PR in window, 0 have it in any `tool_result`. ## Key changes - `.minsky/hooks/pre-narration-detector.ts` - **`relayed-subagent-report`** (fourth source): the claim's PR number appears PR-shaped (`PR #N`, `PR N`, `#N`, `pull/N`, `changeset/N`; a lone `#N` may not follow a word character — `\w`, underscore included after R1 — so `mt#4959` / `mem#1386` / `foo_#123` never read as PRs) in a subagent report within the window — (a) a `<task-notification>` user line whose `<summary>` opens `Agent`, or (b) the `tool_result` of an `Agent` / `SendMessage` / `TaskOutput` call. `Background command …` / `MCP task …` envelopes are excluded (the parent's own command coming back). Identity-scoped: a report naming the WRONG number is the pre-narration this detector exists for. Applies to any category whose claim names a PR. - **`dated-historical`** (fifth source, `merged` family only): the claim's own sentence carries an ISO date strictly before the day of the conversation's first timestamped line. Anchored to the conversation, not to the presence of a clock time — the coarser signal mt#4256 measured and rejected as "would suppress genuine current claims that happen to cite when they were observed"; a claim dated inside the conversation's span keeps firing. `(?!\d)` rather than a trailing `\b` after the day, because `2026-09-02T21:19:45Z` (1 of the 3 measured) is followed by `T`. "The conversation" is the PARENT transcript on both call paths (`ctx.transcriptLines` via `resolveParentTranscriptLines`; `main()` via `resolveParentTranscriptLinesForPath`). - Both sources come LAST in the chain, so the three older reasons report exactly what they did. - `buildSuppressionEvidence(lines, windowTurns)` assembles every input once — `run()`, `main()` and the replay script all call it, which is what makes "the replay evaluates on the detector's own terms" hold by construction. `detectPreNarrationWithSuppression` gains an optional 4th parameter (`RelayEvidence`); omitting it disables both new sources (the safe direction). `#3412`'s two `console.error` lines in `main()` are untouched. - `scripts/diagnose-pre-narration-window.ts` — `replayCurrentDetector` routes through the shared builder; new `--reasons` mode replays EVERY record in the window (suppressed ones included) and tallies reasons by record (comparable to the log's `suppressionReasons`) and by match (the invariant). - `.minsky/hooks/pre-narration-detector.test.ts` — 18 tests; `.claude/hooks/pre-narration-detector.ts` regenerated by pre-commit. ## Testing Typecheck: 8 projects clean (`validatedWorkspace` = the session dir). Lint: 0 errors / 0 warnings over 4490 files (re-run after R1). Execution evidence: - **AT1** (`bun test --preload ./tests/setup.ts --timeout=15000 ./.minsky/hooks/pre-narration-detector.test.ts ./.minsky/hooks/suppression-contract.test.ts ./scripts/diagnose-pre-narration-window.test.ts`, at head `75abc4be3`): ``` 115 pass 0 fail Ran 115 tests across 3 files. [161.00ms] ``` The fixture is the `f290bb69` envelope verbatim in shape (ids shortened), no `session_pr_create` call, final turn "PR #3723 is up for mt#4959 …" → suppressed under `relayed-subagent-report`. Negative controls: (i) the envelope naming #3724 → fires; (ii) the same number inside a `Background command "Wait for CI build on PR 3723" completed` notification → fires. Second positive: the report as a foreground `Agent` `tool_result` → suppressed under the same reason. Also: a relayed `merged` claim (the `acd0c445` steelman shape) suppresses; a same-turn tool call still outranks a report; a claim naming no PR is never report-backed; omitting the relay evidence disables both sources; `extractPrShapedNumbers` reads every PR spelling and no task/memory short id, and (R1) `foo_#123` / `bar9#456` are not PRs while `(#123)` / `foo #456` are; report evidence is window-scoped. - **AT2**: "PR #34861 merged **2026-07-21** — one second before the issue closed." in a conversation whose first timestamped line is 2026-09-10 → suppressed under `dated-historical`; the same sentence with the date removed → fires; the same sentence dated on the conversation's first day → fires; "PR #3581 merged 2026-09-02T21:19:45Z." in a conversation begun 2026-07-01 → fires (later than start); a dated `pr-created` claim → fires (merged family only); no timestamped line → exclusion off; the datetime form is asserted directly in the helper test. - **AT3 / SC3 / SC4 — paired replay** (`bun scripts/diagnose-pre-narration-window.ts --reasons --until 2026-09-13T12:45:00Z --log <main checkout's log>`; the BEFORE side ran the identical harness with the detector restored to `origin/main` and the two old inputs; the AFTER side re-run at `75abc4be3`, unchanged by R1): ``` === BEFORE (detector at origin/main): today's detector says: 265 records, 12 unsuppressed, 0 unreplayable by MATCH: 270 same-turn-tool-call 53 window-tool-call 3 identity-scoped-tool-call === AFTER (this branch): today's detector says: 265 records, 1 unsuppressed, 0 unreplayable by RECORD: 203 same-turn-tool-call 53 window-tool-call 8 relayed-subagent-report 3 identity-scoped-tool-call 3 dated-historical by MATCH: 270 same-turn-tool-call 53 window-tool-call 8 relayed-subagent-report 3 identity-scoped-tool-call 3 dated-historical ``` Both sides reproduce the log's own record-level tally exactly (`log says: 265 / 12 unsuppressed / 202 / 53 / 3`), so the replay is faithful. **SC3: 12 → 1** — the survivor is the `d9c7c2b0` catch-up-table entry ("PR #3715 merged" inside a time-stamped table, undated, backed only by Bash reads), as the spec predicts. **SC4: by MATCH the three older reasons are equal, 270 / 53 / 3.** The record-level `same-turn-tool-call` 202 → 203 is the `#251` record: its turn also carried a same-turn-suppressed `review-approved` match, which becomes visible in the record's reason set once its live `merged` match is suppressed — the spec's SC4 was amended to state the invariant at match level and name this instance. - **SC1 / SC2**: the two reason strings are distinct constants with their own docblocks; `relayed-subagent-report` reaches 8 (7 relays + the steelman), `dated-historical` 3 (#3581, #251, #34861). Negative control: the new test file run against the un-fixed detector (`git restore --source=origin/main -- .minsky/hooks/pre-narration-detector.ts`, whole file): ``` SyntaxError: Export named 'SUPPRESSION_DATED_HISTORICAL' not found in module '…/.minsky/hooks/pre-narration-detector.ts'. 0 pass 1 fail ``` The discriminating control is the BEFORE half of the paired replay above: the same `--reasons` harness, fed the old detector's two inputs, leaves all 12 unsuppressed. Negative control — R1 underscore: the pinning test against the pre-R1 detector (`git restore --source=96a3c2bbe -- .minsky/hooks/pre-narration-detector.ts`, whole file, then restored): ``` (fail) mt#5109 — report evidence helpers > PR #3750 R1: an underscore is a word character too — `foo_#123` is not a PR 95 pass 1 fail ``` ## Live verification The paired replay IS the live exercise: 265 real calibration records replayed against their real session transcripts through the real-wired `buildSuppressionEvidence` → `detectPreNarrationWithSuppression` path that `run()` calls. No mock, no fixture, 0 unreplayable. Deploy verification: `[no-deploy-impact]` — `isDeploySurfaceFile` is `false` for all four changed files (`.minsky/hooks/pre-narration-detector.ts`, its test, the `.claude/hooks` mirror, `scripts/diagnose-pre-narration-window.ts`); `findAffectedServices` → `[]`. The detector is a Claude Code hook; it takes effect at the next hook run on a checkout that has the merge. Documentation impact: the detector's mechanism is documented in its own docblocks (each new reason constant, the two evidence builders, the chain ordering). `hook-observers.mdc`'s Pre-narration entry is an index line (trigger + status + override, "no detail page yet — owned by mt#4992") and enumerates no suppression sources, so it is unchanged. ## Review rounds **R1** (review 5190779986, on `96a3c2bbe`): BLOCKING — the lone-`#N` lookbehind was `[A-Za-z0-9]`, omitting `_`. Verified true against the docblock's own intent; fixed to `\w` with a pinning test observed failing pre-fix. Non-blocking 1 (lookbehind support): kept; hooks run under Bun/JSC, which supports it, and three sibling hooks already use lookbehind — noted in the docblock. Non-blocking 2 (`conversationStartDay` on "flattened parent+subagent lines"): the premise does not hold — `ctx.transcriptLines` is resolved by `resolveParentTranscriptLines`, which re-parses the PARENT alone whenever subagent candidates exist, and `main()` uses `resolveParentTranscriptLinesForPath`; documented in the function's docblock rather than narrowed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VuuiE8iQYtmaajWTbJmhzK Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 15, 2026
…ore the daemon cwd
## Summary
Closes mt#5155. On the shared local daemon (ADR-038) every read tool resolved its project scope from `process.cwd()` — the spawner's directory, for every caller — so `config_show --workspace …/flotato`, `tasks_list --workspace …/flotato` and `session_list --repo edobry/flotato` all answered for minsky. Eight sites carried the same `resolveProjectIdentity({ repoPath: process.cwd() })` block; the caller's explicit `workspace`/`repo` went unread (and `session.list`'s `repo` was dropped outright by `listSessionsImpl`).
All eight now go through ONE helper, `resolveReadScope` (`packages/domain/src/project/read-scope.ts`), with the precedence ADR-021 already records for writes: `allProjects` → `workspace` → `repo` (a path, else an `owner/name` slug through `projects.slug`) → process cwd.
**Design decision, recorded in the spec:** the explicit rungs do not fail open. ADR-021's "unidentified → ALL_PROJECTS" default is kept byte-for-byte on the cwd rung (a misconfigured ambient cwd still sees something). An argument the caller NAMED that resolves to no project is `unresolved` with a reason and the read returns nothing — it scopes to `NO_PROJECT_SCOPE` (the nil uuid, which `projects.id` never equals), so the query layer's `uuid | ALL_PROJECTS` contract is untouched. "Everything" is the wrong answer to "show me X"; it is how the onboarding session's `tasks_list --workspace …/flotato` returned 1016 minsky tasks. The ADR-021 amendment rides with mt#5168 (the implicit `_meta` cwd supplier this helper enables).
## Changes
- `packages/domain/src/project/read-scope.ts` (new): `resolveReadScope`, `readScopeToProjectScope`, `summarizeReadScope`, `NO_PROJECT_SCOPE`, `repoLooksLikePath`, `slugFromRepoArgument`. Deps injectable (identity resolver, path probe, cwd) — the cwd seam is what mt#5168 will fill.
- `tasks.list` (`query-commands.ts`): honours `workspace`/`repo`; `onScopeResolved` seam so the adapter reports `projectScope` (and `Found 0 tasks: <reason>` in text) without a second resolution. `tasks.similar`/`tasks.search`: same, via `taskContextParams` they already declared.
- `session.list`: `repo` now selects a project (it was accepted and dropped); optional `workspace` added; result carries `projectScope`.
- `memory.search/list/similar`, `asks.list`, `transcripts.search/search-text/similar`, `tasks.pointings.declare/list`, `tasks.expire-remainder`: optional `workspace` added (they declared none, so SC2 was unachievable for them without it — the spec's gate-h text claimed `memory.*` "already advertised" it; corrected in the spec).
- `src/mcp/server.ts` `resolveProjectIdBestEffort(args)`: presence claims and session attachments take the tool call's own `workspace`/`repo`; an explicit argument naming no project stamps `undefined` (a write; the nil-uuid sentinel is read-side only).
- `config.show/list/get/validate/doctor`: `getConfigProviderForWorkspace` (`config/helpers.ts`) builds a `CustomConfigFactory` provider rooted at the named workspace; else the global. mt#5154's doctor checks build on this.
- `read-scope-census.ts` + test: walks `src/` and `packages/` for the cwd-resolving call shape (whitespace/trailing-comma tolerant) and expects zero sites. `scripts/` excluded — a one-shot script's process IS its caller.
- `scripts/verify-read-scope-from-workspace.ts`: the live daemon-from-A/workspace-B run (AT1–AT3, AT5).
- Generated: `src/generated/completion-manifest.json` picks up the new `--workspace` flags and `session list --repo`'s new description.
## Contract changes (additive)
Optional `workspace` added to 11 commands (never required). `session.list --repo` changes meaning from ignored to a project selector. `tasks.list` and `session.list` results gain an optional `projectScope: { kind, source, projectId?, value?, reason? }`. No parameter renamed or made required; no env var, config key, or deployed-environment artifact.
## Test evidence
**Live, this branch (verified-1b).** Bundle built from this branch; scratch HTTP daemon started from the minsky session checkout (project `3ac3d147…`) on 127.0.0.1:48799 with a scratch bearer token. `bun scripts/verify-read-scope-from-workspace.ts --workspace /Users/edobry/Projects/flotato --slug edobry/flotato --task mt#5151 --expect-project-id 3ec1323d-8d31-4a5c-9ef0-420ba377bb62` — **11/11 PASS** (2026-09-14T23:49Z):
```
PASS AT1 config_show workspace.mainPath is the named workspace — workspace.mainPath=/Users/edobry/Projects/flotato harness=claude-code
PASS AT1 config_show project source comes from the named workspace — project source path=/Users/edobry/Projects/flotato/.minsky/config.local.yaml; repository.url=https://github.com/edobry/flotato.git
PASS AT2 tasks_list --workspace B scopes to B — projectScope={"kind":"scoped","source":"workspace","projectId":"3ec1323d-…"} count=3
PASS AT2 tasks_list --workspace B includes mt#5151 — ids=mt#5151,mt#5167,mt#5165
PASS AT2 control: tasks_list with no argument scopes to the daemon's cwd — projectScope={"kind":"scoped","source":"cwd","projectId":"3ac3d147-…"} count=2000
PASS AT2 B's rows are disjoint from the daemon's project's rows — B=3 A=2000 overlap=0
PASS AT2 tasks_list --workspace <no project> is unresolved and empty — projectScope={"kind":"unresolved","source":"workspace",…} count=0
PASS AT3 session_list --repo edobry/flotato scopes to B — projectScope={"kind":"scoped","source":"repo-slug","projectId":"3ec1323d-…"} sessions=1
PASS AT3 session_list --repo /Users/edobry/Projects/flotato scopes to B — projectScope={"kind":"scoped","source":"repo-path",…}
PASS AT3 session_list --repo nonexistent/repo is unresolved with a reason, 0 rows — reason: no project is registered under "nonexistent/repo"
PASS AT5 the presence claim this call refreshed is stamped with B's project — newest={…,"projectId":"3ec1323d-…"}
ALL CHECKS PASSED
```
**Negative control (verified-1b).** Same script against a PRE-FIX daemon (main's bundle, started from `/Users/edobry/Projects/minsky` on :48798): **11/11 FAIL** — config from minsky, 500 minsky tasks for `--workspace flotato`, 200 minsky sessions for `--repo edobry/flotato`, 20 rows for `nonexistent/repo`, claim stamped `3ac3d147…`. The script fails on the defect and passes on the fix.
**Census negative control.** A planted multi-line `resolveProjectIdentity({\n repoPath: process.cwd(),\n})` under `src/` made `read-scope-census.test.ts` fail naming the file; removed.
**Unit (verified-1a).** `read-scope.test.ts` 18 pass; `read-scope-census.test.ts` 4 pass; `config/helpers.test.ts` 40 pass (4 new); 697 tests across the 42 directly-affected files pass (`bun test --preload ./tests/setup.ts` over `packages/domain/src/project`, `commands/transcripts`, `commands/config`, `commands/memory`, session basic-commands, asks, similarity, `tests/domain/project-scope-acceptance.test.ts`, …). Typecheck clean across all 8 projects; lint clean. The full gated suite runs in CI.
`/Users/edobry/Projects/flotato` was read only (config_show, tasks_list, tasks_get); nothing there was modified.
## Execution evidence
Execution evidence:
- SC1 (inventory): recorded in the spec's `## Inventory` (2026-09-14) — 12 rows covering the eight cwd sites, `config.show/list/get/validate/doctor`, the two already-correct argument-taking commands, and the two write-side seams left unchanged.
- SC2 (one helper, explicit argument wins, empty not foreign): `bun test --preload ./tests/setup.ts packages/domain/src/project/read-scope.test.ts` → `18 pass 0 fail`; `read-scope-census.test.ts` → `4 pass 0 fail` (zero cwd-resolving sites in `src/`+`packages/`); live AT2 rows above (`--workspace flotato` → 3 rows, overlap with the daemon's project 0; `--workspace <no project>` → `unresolved`, 0 rows). CLI path unchanged: `resolveReadScope({}, …)` with no argument → `{ scoped, source: "cwd" }` (unit) and the live no-argument control → `{scoped, cwd, 3ac3d147}`.
- SC3 (`session.list --repo` as path / slug / unresolvable): live AT3 rows above — `repo-slug` → `3ec1323d`, `repo-path` → `3ec1323d`, `nonexistent/repo` → `unresolved`, `reason: no project is registered under "nonexistent/repo"`, 0 rows.
- SC4 (per-workspace config provider): live AT1 rows above — `config_show --workspace …/flotato --sources` → `workspace.mainPath /Users/edobry/Projects/flotato`, project source `…/flotato/.minsky/config.local.yaml`, `repository.url https://github.com/edobry/flotato.git`; `src/adapters/shared/commands/config/helpers.test.ts` → `40 pass 0 fail` (4 new: workspace → `createProvider({ workingDirectory })`, none/blank → global).
- SC5 (daemon-from-A / workspace-B test): `scripts/verify-read-scope-from-workspace.ts` → `ALL CHECKS PASSED` (11/11) against the fixed daemon; `11 CHECK(S) FAILED` against the pre-fix daemon; the structural half is `read-scope-census.test.ts`, whose negative control named a planted site.
- SC6 (`resolveProjectIdBestEffort` takes the caller's workspace): live AT5 row above — newest claim after `tasks_get taskId:mt#5151 workspace:…/flotato` carries `projectId 3ec1323d-…`; pre-fix daemon: `3ac3d147-…`.
- AT4 CLI half (no argument from cwd B): covered by the helper's cwd-rung unit tests and the live no-argument control; not separately run from a flotato terminal.
## Deploy verification
Touches `src/mcp/server.ts` and shared adapters → minsky-mcp (hosted) redeploys. On the hosted server no tool call carries a cwd-shaped workspace, so the cwd rung's ADR-021 default is unchanged there; the change is inert for hosted callers that pass no `workspace`/`repo`.
After merge I will run `mcp__minsky__deployment_wait-for-latest` for `minsky-mcp` with `notBefore` = merge time and `expectCommitSha` = the merge SHA, require `SUCCESS`, and confirm the runtime started by reading the health body (`service: minsky-mcp`, `status: ok`) and a change-produced assertion — the hosted daemon's `session_list` tool advertising the new `workspace` parameter. The outcome will be recorded here and in the task spec.
## Review round 1 (`efc671fcd`)
- **[BLOCKING] `session.list` `repo`/`workspace` schemas** — changed to `z.string().optional()` to match the memory/asks spelling. The mechanism claim ("calls without `--workspace` fail") did not hold: the registry treats `required: false` as optional regardless of the zod shape — the same `z.string()` + `required: false` shape is this file's pattern for `task`, `since`, `until`, `offset` and eight more, and every live `session_list` call above carried no `workspace`. Harmonized anyway; one spelling is better than two.
- **No DB handle + explicit argument widened to ALL** — fixed at all five sites (tasks.list, session.list, memory, transcripts, asks): the handle now goes to the helper unchecked, null included; the helper classifies it (`invalid-db-handle` → explicit rungs `unresolved`, cwd rung `all`). New unit test pins both halves.
- **`repoLooksLikePath` could misclassify a slug that exists as a relative directory** — accepted: a bare `owner/name` is now always a slug, no on-disk probe; a relative directory is spelled `./owner/name`. On the shared daemon a relative path would have resolved against the spawner's cwd anyway, so nothing legitimate is lost. Tests updated; `session list --repo` description says which spellings are paths.
- **Census skipped any dir named `generated`** — narrowed to the `src/generated` path.
- **`tasks.pointings.candidates` not threaded** — it resolves no scope: it takes a pointing id and reads the row's own project. Only `declare`/`list`/`expire-remainder` resolve a scope, and all three are threaded.
- **Stale transcripts header; call-site notes in `server.ts`** — both done.
## Related
mt#2391 (umbrella) · mt#5168 (implicit `_meta` cwd supplier + ADR-021 amendment; depends on this helper) · mt#5154 (doctor project-scope checks; consumes `getConfigProviderForWorkspace`) · mt#4808 / mt#4758 / mt#4772 (write-side precedent) · mt#1427 (global provider staleness — separate) · mt#4639 / PR #3412 (stale log-conversion PR overlapping 8 files; rebase note left).
Had Claude implement and verify this; the design decision above is recorded in the task spec.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
This branch has not been deployed
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.
custom/prefer-loggable-error-summaryshipped in mt#4632 registered"off"— not because it isunreliable, but because this repo runs a zero-tolerance ESLint warning gate (mt#1097, no override)
and the rule flagged hundreds of pre-existing sites. This is the cleanup that makes
"error"shippable, on the mt#3312 / mt#3313 model.
The defect the rule catches is real and measured: a
DrizzleQueryError's.messageisFailed query: <sql>\nparams: <params>and the actual Postgres error lives on.cause, so a logsite rendering
err.messagearound a DB call records the SQL and every bound parameter and noneof the diagnosis. During the 2026-08-25 degradation window that produced a 4,685-character
errorfield with no PG error in it.
The population was bigger than the spec said
The spec's
590was measured oversrc/packages/services/scripts. The gate that actuallyhas to pass is
bun run lint:strict(eslint . --max-warnings=0,.github/workflows/ci.yml:105),which lints the whole repo:
src packages services scripts(what 590 measured; 593 by 2026-08-27).minsky/hooks(source).claude/hooks(generated mirror of those same 18 sites)tests/eslint-rules/eslint .sees614 converted, 10 carved out, 18 handled by converting the source and regenerating the mirror.
Note
bun run lintandbun run validate-allboth use the NON-strict form and would have passed a"warn"posture — they are not the gate, which is why the success criterion now nameslint:strict.Key changes
scripts/codemod-loggable-error-summary.ts(new) — drives the conversion off the rule's ownJSON report rather than a regex over source. The rule already decides the hard question (which
bare renderings flow into a log call from inside an error handler; a raw grep finds 878, of which
279 are throws/returns that must NOT be converted), so the codemod rewrites exactly the AST ranges
it reports and the two populations cannot drift. Every replacement is shape-validated before it is
applied — the ternary's expression must appear identically in all three positions, and a
getErrorMessage(...)call has only its callee renamed so any argument expression survivesbyte-identically — and anything unmatched is skipped and reported rather than guessed at. It also
adds the import (298 files) and removes a now-orphaned
getErrorMessageimport (44 files).eslint.config.js— posture"off"→"error", plus twofiles:-scoped carve-outs..claude/hooksmirror.The judgment call the spec required, decided on a measurement
The spec left one sub-category open: CLI user-facing
console.error, where "a terse message isarguably the right output for a human at a terminal." Measured rather than argued —
getLoggableErrorSummaryreturns a byte-identical string toerr.messagefor a plainErrorand to
String(err)for a non-Error. It differs in exactly two cases: the error carries a.cause(rendered
outer — caused by: inner, which is what the person at the terminal wants), or themessage exceeds 2000 chars (truncated, which helps a terminal). There was no terseness to protect,
so all 85
console.*sites were converted. Also converted: the 14 inservices/reviewer, wherestdout IS the diagnostic log (the premise of mt#2463 / mt#2465), and the 18 in
.minsky/hooks,whose per-prompt import cost was measured at ~20ms before deciding.
Sites deliberately NOT converted
Both use ESLint's documented
files:-scoped config objectrather than a bespoke option on the rule — the vendor documents this mechanism and ADR-036 §Decision
records the same convention for this repo.
tests/fixtures/typescript/large-service.tstests/integration/helpers/edit-test-helpers.ts:380). Not log sites, and converting would make a fixture depend on@minsky/domain.eslint-rules/no-unregistered-minsky-env-var.js:107Testing
Execution evidence:
SC2 / AT1 / AT2 — the rule at
"error", whole repo, zero-warning gate:Negative control — the gate CAN fail, and did before this change. Flipping the posture to
"error"on unconverted main and running the same command reported the population this PR removes:and mid-conversion, before the formatter ran, the same gate was still red on real findings:
SC1 — flagged population reduced to zero:
SC4 / AT4 — conversions pass the ERROR, not
err.message(a no-op wearing a fix's clothing):Spread sample of 10 converted sites, all passing the error object:
postgres-channel-listener.ts:203 (err),agent-transcript-ingest-service.ts:1182 (err),summary-pipeline.ts:303 (err),github-issues-api.ts:153 (error as Error),elicitation.ts:176 (err),sync-runner.ts:153 (err),clone-operations.ts:127 (accessErr),session-auto-repair-provider.ts:94 (error),pr-get-subcommand.ts:309 (error),guard-health-tracker.ts:448 (err).Import-specifier distribution (SC4's amended rule — package specifier outside the domain package,
relative inside it, matching the 20+ pre-existing adopters):
SC5 / AT5 — hooks converted at SOURCE, mirror regenerated not hand-edited:
AT3 — non-converted sites enumerated with reasons: in the task spec's
## Outcome, and in thetable above.
Codemod accounting (0 skipped, and it aborts on a mismatch):
Full suites:
Typecheck — clean across all 8 projects (
.,packages/domain,packages/shared,services/reviewer,services/site,src/cockpit/web,tsconfig.hooks.json,tsconfig.scripts.json), validated against the session workspace.infra/tsconfig.jsonskippedwith its documented reason (deps not installed locally; CI runs it with its own install step).
No unrelated churn:
format:allruns a repo-wideeslint . --fix, so every changed file waschecked — all 313 minus
eslint.config.jsand the new codemod script carrygetLoggableErrorSummary. Zero unrelated files touched.Deploy verification: this PR changes deploy surface —
isDeploySurfaceFilereturns true for 223of the 313 changed files (application source under
packages/domain,src/mcp,src/cockpit,services/reviewer). The change is a logging-call substitution with no behavioral change to anyrequest path, but that is a claim about intent, not evidence, so post-merge I will wait on the
deployment bound to this merge (
notBefore= merge time,expectCommitSha= merge SHA) and assertthe health body's service identity rather than the status code.
Spec amendments made during implementation
@minsky/domain/errors/index, which isunsatisfiable for the 100+ converted files inside
packages/domain/src/**— a package cannotself-reference, and every pre-existing adopter uses a relative specifier. Amended to state the
real, tree-dependent rule; the measured distribution is above.
bun run lint, which carries no--max-warningsflag and would have passed a
"warn"posture. Amended tolint:strict, the command CI runs.Forward hazard, not owned by this PR
PR #3253 / mt#3854 will re-introduce 18 violations when it merges. It makes
.codex/a trackedcompile output,
.codex/**is not ineslint.config.js'signores, and its committed.codex/hooks/*.tswere generated 2026-08-22 — before this conversion. Once"error"is on main,merging that branch without re-running
compileputs 18 errors on main. The fix is onecompilerun on that branch after it rebases; pre-commit's auto-regen fires on STAGED hooks sources, which a
rebase does not stage, so it will not happen by itself.