From 243278808f526c0b378bbf77b9d7cbbfad5d0e22 Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Thu, 3 Sep 2026 10:04:54 +0200 Subject: [PATCH 1/3] close supervisor mutation survivors: implemented related tests for batch bounds and response budget --- ...g-surfaced-in-platformos-mcp-supervisor.md | 64 ++++++++++ ...ates-warnings-not-only-errors-and-infos.md | 78 ++++++++++++ ...ther-the-file-count-branch-is-reachable.md | 95 ++++++++++++++ ...performance-guard-not-a-correctness-one.md | 81 ++++++++++++ .gitignore | 11 ++ .../src/result/response-budget.spec.ts | 36 +++++- .../src/validate/batch-bounds.spec.ts | 119 ++++++++++++++++++ .../src/validate/batch-bounds.ts | 5 + .../src/validate/validate-buffers.ts | 6 + vitest.config.mjs | 8 +- 10 files changed, 498 insertions(+), 5 deletions(-) create mode 100644 .backlog/tasks/task-100 - Close-what-mutation-testing-surfaced-in-platformos-mcp-supervisor.md create mode 100644 .backlog/tasks/task-100.1 - Assert-that-response-budget-truncates-warnings-not-only-errors-and-infos.md create mode 100644 .backlog/tasks/task-100.2 - Give-batch-bounds-its-own-spec-and-settle-whether-the-file-count-branch-is-reachable.md create mode 100644 .backlog/tasks/task-100.3 - Record-that-worthReading-is-a-performance-guard-not-a-correctness-one.md create mode 100644 packages/platformos-mcp-supervisor/src/validate/batch-bounds.spec.ts diff --git a/.backlog/tasks/task-100 - Close-what-mutation-testing-surfaced-in-platformos-mcp-supervisor.md b/.backlog/tasks/task-100 - Close-what-mutation-testing-surfaced-in-platformos-mcp-supervisor.md new file mode 100644 index 00000000..798a1daa --- /dev/null +++ b/.backlog/tasks/task-100 - Close-what-mutation-testing-surfaced-in-platformos-mcp-supervisor.md @@ -0,0 +1,64 @@ +--- +id: TASK-100 +title: Close what mutation testing surfaced in platformos-mcp-supervisor +status: Done +assignee: [] +created_date: '2026-09-03 06:30' +updated_date: '2026-09-03 07:48' +labels: + - testing + - platformos-mcp-supervisor + - mutation-testing +dependencies: [] +priority: medium +ordinal: 75000 +--- + +## Description + + +A local Stryker 10 run over 11 of the supervisor's 29 source files (551 mutants, 82.4% score) found guards that can be deleted with the whole suite still green, plus two design facts that are correct in behaviour but recorded nowhere. + +WHY THIS MATTERS: none of these is a bug today. Each is a promise the suite does not keep — a defence that could be removed or inverted in a future refactor without a single test noticing — or a fact the next reader will have to re-derive from scratch. + +CONTEXT A FUTURE IMPLEMENTER NEEDS: Stryker is NOT part of this repository. It was installed locally and its configs (`packages/*/stryker.config.mjs`, `packages/*/vitest.stryker.config.mjs`) are gitignored, so you cannot reproduce these findings by running a project script. You do not need to. Every item below states the exact one-line change that survives today, and CLAUDE.md already requires the same discipline: apply the change by hand, confirm the new test fails, then revert. That is the verification, and it is enough. + +If you do want the tool back: + yarn add -W -D @stryker-mutator/core@^10 @stryker-mutator/vitest-runner@^10 + git checkout -- package.json yarn.lock + +READ THE REPORT WITH THIS CAVEAT. The mutation run excludes `test/integration/**` and `test/guards/**`, because those specs cannot run inside Stryker's sandbox — `stdio-smoke.spec.ts` shells out to `yarn build`, and the guards scan the repository's own layout. Code covered ONLY by those specs therefore shows as uncovered when it is nothing of the sort. That already produced one false finding: the `validate_code` tool-callback body (`transport/validate-code.ts`, the `content: [{ type: 'text', … }]` envelope) reported four no-coverage mutants, and `test/integration/stdio-smoke.spec.ts` in fact calls the tool over a real stdio transport and asserts that exact envelope. No work is needed there. Check the integration specs before believing any no-coverage result. + +Deliberately OUT of scope, reviewed and set aside: `assemble.ts` empty-list assertions; the malformed-request sub-branches at `validate-code.ts:289/290/313`; and roughly 50 string-literal mutants that survive because tests derive expected messages from the code under test rather than pasting them, which is this repository's deliberate anti-rot choice and should stay that way. At least one of those is provably unkillable — `Buffer.byteLength(s, '')` equals `Buffer.byteLength(s, 'utf8')`, measured. + + +## Implementation Notes + + +All three subtasks Done. Files changed, all in `packages/platformos-mcp-supervisor`: + M src/result/response-budget.spec.ts (+1 test, +`bytesOf` helper that `diagnosticBytes` now reuses) + A src/validate/batch-bounds.spec.ts (new, 7 tests) + M src/validate/batch-bounds.ts (comment only, +13 lines, 0 removed) + M src/validate/validate-buffers.ts (comment only, +18 lines, 0 removed) +No production behaviour changed. `validate-code.spec.ts` was not touched. + +The branch also carries two pre-existing uncommitted hygiene changes that are NOT part of this task: `.gitignore` (Stryker artefacts) and `vitest.config.mjs` (excluding `.stryker-tmp` so a leftover sandbox cannot be collected as a second copy of every spec running against mutated code). + + +## Final Summary + + +All three subtasks closed. Two new tests' worth of promises the suite was not keeping, one branch decision settled, and one guard documented — no production behaviour changed anywhere. + +**100.1 — the warnings truncation is now pinned.** `response-budget.ts` could return the `warnings` bucket unsliced with the entire suite green. One test in `response-budget.spec.ts` drives 40 warnings against a budget derived from the fixture to pay for exactly 12, and asserts the whole returned result in one equality. The errors and infos slices turned out to be pinned already, by the contiguity test and the severity-order test respectively — confirmed by sabotaging all three, not assumed. + +**100.2 — `batch-bounds.ts` has a spec, and the file-count branch has an answer.** New `batch-bounds.spec.ts`, seven tests against `batchTooLarge` directly: both caps at their exact boundaries in both directions, bytes-not-characters, precedence between the two caps, and that the two refusals do not collapse into one message. The file-count branch is unreachable over MCP (`VALIDATE_CODE_INPUT.files` caps the array first, and `runValidateCode` is not on the package's public surface) and is **kept** as defence-in-depth, with that reasoning now written at the branch itself so its zero coverage is not read as a gap. + +**100.3 — `worthReading` is documented as a performance guard.** Its six survivors are explained by `warm()` only pre-warming a promise impact awaits itself; inverting it cannot change a verdict. + +**Verification.** Every claim was measured rather than carried over from the report: both survivors reproduced before any test was written, the file-count branch proved unreachable by planting a `throw` in it, and ten hand-applied mutations across the three files each killed by the intended test and no other. Package suite 549/549 (542 before), type-check clean, prettier clean, build confirms the new spec does not ship in `dist`. + +**One correction to the report itself**, recorded in 100.3's notes: the neighbouring `--no-impact` "costs NOTHING" claim was described as wholly untested. It is untested in half — the impact adapter never being called IS asserted in `validate-code.spec.ts`; the project read being skipped is not. The comment states that split rather than the flattened version. + +**Out of scope and untouched, as the task specified:** `assemble.ts` empty-list assertions, the malformed-request sub-branches, and the ~50 string-literal mutants that survive because tests derive expected messages from the code under test. + diff --git a/.backlog/tasks/task-100.1 - Assert-that-response-budget-truncates-warnings-not-only-errors-and-infos.md b/.backlog/tasks/task-100.1 - Assert-that-response-budget-truncates-warnings-not-only-errors-and-infos.md new file mode 100644 index 00000000..4887b566 --- /dev/null +++ b/.backlog/tasks/task-100.1 - Assert-that-response-budget-truncates-warnings-not-only-errors-and-infos.md @@ -0,0 +1,78 @@ +--- +id: TASK-100.1 +title: 'Assert that response-budget truncates warnings, not only errors and infos' +status: Done +assignee: [] +created_date: '2026-09-03 06:30' +updated_date: '2026-09-03 07:37' +labels: + - testing + - platformos-mcp-supervisor +dependencies: [] +references: + - packages/platformos-mcp-supervisor/src/result/response-budget.ts + - packages/platformos-mcp-supervisor/src/result/response-budget.spec.ts +parent_task_id: TASK-100 +priority: high +ordinal: 1000 +--- + +## Description + + +`src/result/response-budget.ts` exists to stop one `validate_code` answer eating the agent's context — the module's own comment records an unbounded call measured at ~336,000 tokens. It slices three buckets when a result is over budget: `errors`, `warnings`, `infos`. + +Only two of the three are asserted. MEASURED: replacing + + warnings: result.warnings.slice(0, taken.warnings), // line ~158 + +with + + warnings: result.warnings, + +leaves 289/289 tests passing across `src/result/` and `src/transport/`. The warnings truncation can be deleted and nothing notices. + +This is the highest-risk gap of the set because warnings are the COMMON severity — a file with hundreds of them is exactly the tail this module defends against, and losing the slice reopens the unbounded response the budget was written for. + +There is an existing test for the `errors` bucket; mirror it rather than inventing a new shape. + + +## Acceptance Criteria + +- [x] #1 A test drives a result whose `warnings` exceed the allocated budget and asserts the returned `warnings` array is sliced to exactly the allocated count +- [x] #2 The same test asserts the `truncated` field reports the true pre-truncation total for the warnings bucket, not the returned count +- [x] #3 Assertions use whole-value equality on the returned result per the repo's test guidelines — not a `length` check or a per-property read +- [x] #4 SABOTAGE-VERIFIED: replacing `result.warnings.slice(0, taken.warnings)` with `result.warnings` makes the new test fail; the change is reverted afterwards and the suite is green +- [x] #5 The existing errors and infos truncation tests still pass unchanged + + +## Implementation Plan + + +1. Reproduce the survivor first: replace `result.warnings.slice(0, taken.warnings)` with `result.warnings` and confirm the spec is still green (it was — 13/13). +2. Add a `bytesOf` helper to the spec that bills a diagnostic list the same way `costOf` does, and fold the existing `diagnosticBytes` onto it rather than duplicating the arithmetic. +3. Add one test that derives its budget from the entries (`bytesOf(warnings.slice(0, 12))`) so "twelve fit" is arithmetic, not a tuned constant, and asserts the whole returned shape in one equality. +4. Sabotage-verify, revert, then run the whole package suite, type-check and prettier. + + +## Implementation Notes + + +Why the gap existed, which is worth recording because it is not obvious from the file: the errors slice IS pinned (by 'closes a bucket at the first entry that does not fit', which asserts the returned lines are exactly [1]) and the infos slice IS pinned (by 'spends the budget on errors before any info', which asserts infosReturned is exactly 0). Every warnings assertion in the file read a TOTAL or a PRESENCE — `truncated.warnings.total`, `truncated.warnings !== undefined` — and both of those survive returning the whole unsliced list. Confirmed by sabotage on all three slices, not inferred. + +The budget is derived from the fixture (`bytesOf(warnings.slice(0, admitted))`) rather than hand-tuned. `diagnostic()` embeds the line number in the message, so entries on lines 1-9 and 10-40 differ in cost; a literal budget would be a magic number that quietly changes meaning if the fixture is touched. Paying exactly the cost of the first twelve makes the thirteenth fail `spent + cost > budgetBytes` by construction. + +The generated `truncated.note` string is deliberately NOT asserted, per the repo's rule against pasting a message the code under test produces. + + +## Final Summary + + +Closes the highest-risk mutation survivor in the supervisor's response budget: the `warnings` bucket could be returned unsliced with the entire suite still green. + +**What changed** — one test in `src/result/response-budget.spec.ts` drives a single file with 40 warnings against a budget that pays for exactly 12 of them, and asserts the whole returned result in one equality: the returned `warnings` array is `warnings.slice(0, 12)` element-for-element, `truncated.warnings` is `{ returned: 12, total: 40 }`, and the untouched buckets come back empty. A `bytesOf` helper now bills a diagnostic list the way `costOf` does; the pre-existing `diagnosticBytes` was folded onto it instead of keeping two copies of the same arithmetic. + +**Why it matters** — warnings are the common severity, so a file with hundreds of them is exactly the tail the module was written for (its own comment records an unbounded call measured at ~336,000 tokens). Losing that slice reopens the unbounded response. + +**Verification** — sabotage-verified both ways: with `warnings: result.warnings` substituted for the slice, exactly the new test fails and the other 13 pass; reverted, all 14 pass. Full package suite 542/542 green, `type-check` clean, prettier clean. No source file changed. + diff --git a/.backlog/tasks/task-100.2 - Give-batch-bounds-its-own-spec-and-settle-whether-the-file-count-branch-is-reachable.md b/.backlog/tasks/task-100.2 - Give-batch-bounds-its-own-spec-and-settle-whether-the-file-count-branch-is-reachable.md new file mode 100644 index 00000000..68741cc0 --- /dev/null +++ b/.backlog/tasks/task-100.2 - Give-batch-bounds-its-own-spec-and-settle-whether-the-file-count-branch-is-reachable.md @@ -0,0 +1,95 @@ +--- +id: TASK-100.2 +title: >- + Give batch-bounds its own spec, and settle whether the file-count branch is + reachable +status: Done +assignee: [] +created_date: '2026-09-03 06:31' +updated_date: '2026-09-03 07:45' +labels: + - testing + - platformos-mcp-supervisor +dependencies: [] +references: + - packages/platformos-mcp-supervisor/src/validate/batch-bounds.ts + - packages/platformos-mcp-supervisor/src/transport/validate-code.ts + - packages/platformos-mcp-supervisor/src/adapter-input.spec.ts +parent_task_id: TASK-100 +priority: medium +ordinal: 2000 +--- + +## Description + + +`src/validate/batch-bounds.ts` is the weakest file measured — 48% mutation score — and the reason is simple: it has NO spec file. It is reached only incidentally through `validate-code.spec.ts`, which exercises it as a side effect of testing something else. It is a pure function over `BufferToValidate[]`, so a direct spec needs no adapters, no temp project, and runs in milliseconds. + +Two distinct problems, both to be settled here. + +1. THE BYTE CAP BOUNDARY IS UNTESTED. Changing `bytes > MAX_BATCH_BYTES` to `bytes >= MAX_BATCH_BYTES` survives — no test sends a request of exactly `MAX_BATCH_BYTES` (272,384 bytes / 266 KiB, derived from the deadline ceiling). Note `adapter-input.spec.ts` already pins its own cap exactly ("accepts a buffer exactly at the limit"), so this is an inconsistency with house style rather than a deliberate omission. + +2. THE FILE-COUNT BRANCH HAS ZERO COVERAGE, AND MAY BE UNREACHABLE. `if (buffers.length > MAX_BATCH_FILES)` is never executed by any test. This is NOT simply a testing oversight: `VALIDATE_CODE_INPUT.files` in `transport/validate-code.ts` caps the array with `.max(MAX_BATCH_FILES)`, so the MCP protocol boundary refuses an oversized request before the handler runs. Nothing arriving over the wire can reach that branch; only a direct in-process call to `runValidateCode`/`batchTooLarge` can. + +That is a decision, not a test: EITHER keep the branch as defence-in-depth for direct callers and cover it with a direct unit test, OR conclude it is dead and say so. Whichever is chosen must be written down in `batch-bounds.ts`, because the next reader will otherwise re-derive it from scratch — or, worse, read the zero coverage as an oversight and "fix" it without noticing the schema already refuses. + +Do not paste refusal message text into assertions; this repo derives expected messages from the function under test so a reworded message cannot rot a spec. + + +## Acceptance Criteria + +- [x] #1 `src/validate/batch-bounds.spec.ts` exists and exercises `batchTooLarge` directly, without going through `runValidateCode` +- [x] #2 The byte cap is asserted at the exact boundary: a request totalling exactly `MAX_BATCH_BYTES` is admitted, and one byte more is refused +- [x] #3 SABOTAGE-VERIFIED: changing `bytes > MAX_BATCH_BYTES` to `>=` fails the new spec; the change is reverted afterwards +- [x] #4 A decision is recorded in a comment in `batch-bounds.ts` about the file-count branch — either that it is defence-in-depth for direct callers (and a direct test covers it), or that it is unreachable through MCP because `VALIDATE_CODE_INPUT.files` caps the array first +- [x] #5 If the branch is kept, a test covers it by calling `batchTooLarge` with more than `MAX_BATCH_FILES` buffers +- [x] #6 Expected refusal reasons are derived from the function under test rather than pasted as literals +- [x] #7 The existing batch tests in `validate-code.spec.ts` still pass unchanged + + +## Implementation Plan + + +1. Reproduce both survivors before writing anything: `bytes > MAX_BATCH_BYTES` -> `>=` (whole package suite still 542/542 green) and a `throw` planted at the top of the file-count branch (never reached, 542/542 green). Neither was inferred. +2. Settle the file-count question by reading the call graph rather than guessing: `runValidateCode` has exactly one non-spec caller (the tool callback), it receives schema-validated args, and it is not re-exported from `src/index.ts` — nor reachable by deep import, since the package `exports` map exposes only `.`. Unreachable over MCP; kept as defence-in-depth. +3. Write `src/validate/batch-bounds.spec.ts` against `batchTooLarge` directly: both caps at their exact boundaries, a multi-byte case, precedence between the two, and that the two refusals do not collapse into one sentence. +4. Record the decision in `batch-bounds.ts` at the branch itself. +5. Sabotage-verify six mutations, then run the whole package suite, type-check, prettier, and a build to confirm the new spec does not ship. + + +## Implementation Notes + + +DECISION ON THE FILE-COUNT BRANCH: kept, and the reasoning is now in `batch-bounds.ts` at the branch. Unreachable over MCP — `VALIDATE_CODE_INPUT.files` has `.max(MAX_BATCH_FILES)`, the tool callback is `runValidateCode`'s only non-spec caller, and `runValidateCode` is neither re-exported from `src/index.ts` nor deep-importable (the package `exports` map exposes only `.`). Kept anyway because the two bounds live a file apart and only this one is inside the function whose name promises it; a schema loosened for an unrelated reason must not silently un-cap the batch. + +SABOTAGE LOG — six mutations, each killed by the intended test and no other: + bytes `>` -> `>=` -> only 'ADMITS a request totalling exactly MAX_BATCH_BYTES' + bytes `>` -> `<` -> 4 tests (cap fully inverted) + files `>` -> `>=` -> only 'ADMITS exactly MAX_BATCH_FILES files' + `Buffer.byteLength(.,'utf8')` -> `.length` -> only the multi-byte test + file-count branch deleted -> 3 tests, incl. 'REFUSES one file more than MAX_BATCH_FILES' + the two caps' order swapped -> only 'answers with the FILE-COUNT refusal when a request breaks BOTH caps' + +WHY THE PROSE IS NOT PINNED, unlike `adapter-input.spec.ts` which pins `bufferTooLarge`'s message verbatim: there the message is an oracle for a different subject; here `batchTooLarge` IS the subject, so restating its own sentence asserts only that it was copied correctly. What is asserted instead is what a caller acts on. The precedence test derives its expectation by asking the same function about a request that breaks the COUNT cap alone at the same file count, so no sentence is written down. + +EVERY FIXTURE CARRIES ITS OWN CONTROL, so no case can be answered by the wrong cap: the file-count tests use one-byte buffers and assert in the same equality that they are under the byte cap; the byte-cap tests assert the measured total alongside the verdict, so 'exactly at the cap' cannot decay into 'comfortably under it'. + +Process note for whoever repeats this: `git checkout --` is the wrong way to revert a sabotage while the same file carries uncommitted work — it took the new comment with it twice. Copy the good file aside first. + + +## Final Summary + + +Gives the supervisor's weakest-measured file (48% mutation score, no spec of its own) a direct spec, and settles the file-count branch question rather than leaving it to the next reader. + +**New: `src/validate/batch-bounds.spec.ts`** — seven tests against `batchTooLarge` as a pure function, no adapters and no temp project: +- the byte cap at its exact boundary, both sides: a request totalling exactly `MAX_BATCH_BYTES` is admitted, one byte more is refused (this is the `>` vs `>=` survivor, and it now matches the house style `adapter-input.spec.ts` already sets for `MAX_BUFFER_BYTES`); +- the file-count cap at its exact boundary, both sides; +- bytes rather than string length, via multi-byte content that a `.length` cap would admit; +- precedence — a request breaking both caps answers with the file-count refusal; +- the two caps do not give the same advice, which matters because both carry `code: 'too_large'` and only the prose names the bound that was hit. + +**Changed: `src/validate/batch-bounds.ts`** — a comment at the file-count branch, no behavioural change. The branch is unreachable over MCP (`VALIDATE_CODE_INPUT.files` caps the array with `.max(MAX_BATCH_FILES)`, and `runValidateCode` is not on the package's public surface), so its zero coverage is the schema working, not a gap. It is **kept** as defence-in-depth, because only this bound sits inside the function whose name promises it, and the spec now exercises it directly. + +**Verification** — six mutations applied by hand and reverted; each was killed by the intended test and no other (log in the implementation notes). Whole package suite 549/549 green (542 before, +7), `type-check` clean, prettier clean, and a build confirms the new spec does not reach `dist`. `validate-code.spec.ts` was not touched. + diff --git a/.backlog/tasks/task-100.3 - Record-that-worthReading-is-a-performance-guard-not-a-correctness-one.md b/.backlog/tasks/task-100.3 - Record-that-worthReading-is-a-performance-guard-not-a-correctness-one.md new file mode 100644 index 00000000..f374593d --- /dev/null +++ b/.backlog/tasks/task-100.3 - Record-that-worthReading-is-a-performance-guard-not-a-correctness-one.md @@ -0,0 +1,81 @@ +--- +id: TASK-100.3 +title: 'Record that worthReading is a performance guard, not a correctness one' +status: Done +assignee: [] +created_date: '2026-09-03 06:31' +updated_date: '2026-09-03 07:47' +labels: + - documentation + - platformos-mcp-supervisor +dependencies: [] +references: + - packages/platformos-mcp-supervisor/src/validate/validate-buffers.ts +parent_task_id: TASK-100 +priority: low +ordinal: 3000 +--- + +## Description + + +In `src/validate/validate-buffers.ts` (around line 366) the `worthReading` guard decides whether the project scan is pre-warmed: + + const worthReading = + ctx.impactEnabled !== false && + lintable.some((buffer) => canHaveDependants(...)); + + warm: () => (worthReading ? scan.sources().catch(() => undefined) : Promise.resolve()), + +SIX mutants survive on that one condition, including replacing it outright with BOTH `true` and `false`, plus `.some` becoming `.every`. Read cold, that looks alarming — a guard nothing observes. + +It is not a bug, and the reason is worth writing down. `warm()` only PRE-warms: impact later awaits the same memoized `scan.sources()` promise itself, so an inverted guard costs either a wasted project read or a colder path, never a wrong answer. That is why no assertion moves when it flips, and it is why this sits at the bottom of the priority order despite the survivor count. + +This task is DOCUMENTATION ONLY. No behavioural change, no new test. The point is that the next reader — very possibly the next person to run mutation testing here — should not have to re-derive the same conclusion, and should not "fix" a guard that is doing exactly what it should. + +Note the neighbouring claim in the same file, that `--no-impact` "costs NOTHING", is likewise unverified by any test. Pinning it is deliberately NOT in scope here; the comment should be accurate about what is and is not proven. + + +## Acceptance Criteria + +- [x] #1 A comment at the `worthReading` declaration states that it is a performance guard and that inverting it cannot change a verdict +- [x] #2 The comment names the mechanism: `warm()` only pre-warms, and impact awaits the same memoized `scan.sources()` promise itself +- [x] #3 The comment does not claim the `--no-impact` cost behaviour is tested, because it is not +- [x] #4 No behavioural change and no new test in this task; the suite is green and unchanged + + +## Implementation Plan + + +1. Verify the mechanism before writing it down, rather than restating the task: confirm `sources()` really memoizes (`pending ??= readEdgeSources(...)` in `impact/project-scan.ts`) and that impact really awaits that same promise itself (`impact.ts:146`, `dependants.ts:71` — the only two other callers). +2. Establish what is actually tested about the neighbouring `--no-impact` claim before writing about it. +3. Add the comment at the `worthReading` declaration, below the existing intent comment rather than replacing it. +4. Confirm the change is additive-only (`git diff` removes zero lines), the suite is unchanged, and prettier and type-check are clean. + + +## Implementation Notes + + +The mechanism was verified rather than taken from the task description. `createProjectScan` memoizes with `pending ??= readEdgeSources(...)`, and `scan.sources()` has exactly three call sites: `warm()` here, `impact.ts:146` and `dependants.ts:71`. The latter two await it themselves, so `warm()` is genuinely only a head start. + +ONE CORRECTION TO THE TASK'S FRAMING, worth recording because it is the kind of compound claim CLAUDE.md warns about. The task says the neighbouring `--no-impact` "costs NOTHING" claim is "likewise unverified by any test". It is unverified in HALF. That sentence makes two claims, and they have different evidence: + - "the two extra lint passes never start" IS pinned — `validate-code.spec.ts`, 'reports disabled, and never calls the impact adapter at all', asserts `called: false` against an injected impact adapter. + - "`projectScan` declines to read" is NOT pinned by anything. No test observes the filesystem, and with `projectDir: '/srv/app'` on a `NodeFileSystem` a stray read would be swallowed by `warm()`'s own `.catch`, so nothing would notice. +The comment states exactly that split instead of flattening it to "untested", since the flattened version is itself inaccurate. + +No behavioural change: `git diff` on the file removes zero lines. Suite unchanged at 549/549. + + +## Final Summary + + +Documentation only — one comment block in `src/validate/validate-buffers.ts` at the `worthReading` declaration, added below the existing intent comment rather than replacing it. Zero lines removed, no behavioural change, no new test. + +**What it records** — that `worthReading` is a performance guard and inverting it cannot change a verdict, plus the mechanism that makes that true: `warm()` only pre-warms, `scan.sources()` memoizes its promise, and impact awaits that same promise itself. A guard stuck open buys a project read nobody needed; stuck shut it moves the read from "alongside the lint" to "when impact asks". The same map is read at most once either way. + +**Why** — six mutants survive on that single condition (including replacing it with both `true` and `false`, and `.some` becoming `.every`). Read cold that looks like a defence nothing observes, and the next person to run mutation testing here would otherwise re-derive the same conclusion, or "fix" a guard that is doing exactly what it should. + +**On the neighbouring `--no-impact` claim** — the comment is explicit that neither claim is pinned by a test, and corrects the framing while it is there: "costs NOTHING" is two claims with different evidence. The impact adapter never being called IS asserted; the project read being skipped is not asserted anywhere, and could not be noticed if it happened, since `warm()` swallows the failure a read against the test's non-existent `projectDir` would produce. + +**Verification** — `git diff` removes zero lines from the file; package suite 549/549 green and unchanged; type-check and prettier clean. + diff --git a/.gitignore b/.gitignore index 4cc9f7c5..987756b5 100644 --- a/.gitignore +++ b/.gitignore @@ -27,3 +27,14 @@ packages/theme-graph/playground/src/graph.json # pos-supervisor v1 reference material (recoverable at git 69aa9e4) — not part of PRs docs/mcp-supervisor/salvage/ .mcp.json + +# Stryker mutation testing — LOCAL ONLY, deliberately not part of this repository. +# The configs live on disk but are never committed, so `yarn install` will eventually +# prune the packages they need. To set it back up: +# yarn add -W -D @stryker-mutator/core@^10 @stryker-mutator/vitest-runner@^10 +# git checkout -- package.json yarn.lock # keep the manifests clean +.stryker-tmp/ +packages/*/reports/ +packages/*/stryker.config.mjs +packages/*/vitest.stryker.config.mjs +.pos diff --git a/packages/platformos-mcp-supervisor/src/result/response-budget.spec.ts b/packages/platformos-mcp-supervisor/src/result/response-budget.spec.ts index 8ead6424..44d1ed2b 100644 --- a/packages/platformos-mcp-supervisor/src/result/response-budget.spec.ts +++ b/packages/platformos-mcp-supervisor/src/result/response-budget.spec.ts @@ -40,13 +40,15 @@ const resultWith = ( const many = (count: number, severity: ValidateCodeDiagnostic['severity'] = 'error') => Array.from({ length: count }, (_, index) => diagnostic(index + 1, severity)); +/** What a diagnostic list costs in the response, billed the way `costOf` bills it. */ +const bytesOf = (entries: ValidateCodeDiagnostic[]): number => + entries.reduce((bytes, entry) => bytes + Buffer.byteLength(JSON.stringify(entry), 'utf8') + 1, 0); + /** Serialized size of the diagnostics a capped result actually carries. */ const diagnosticBytes = (results: Map): number => { let bytes = 0; for (const result of results.values()) { - for (const bucket of [result.errors, result.warnings, result.infos]) { - for (const entry of bucket) bytes += Buffer.byteLength(JSON.stringify(entry), 'utf8') + 1; - } + bytes += bytesOf(result.errors) + bytesOf(result.warnings) + bytesOf(result.infos); } return bytes; }; @@ -126,6 +128,34 @@ describe('Unit: capToBudget', () => { }).toEqual({ errors: undefined, warnings: true, infos: undefined }); }); + it('slices the WARNINGS bucket to exactly what the budget bought, and reports the true total', async () => { + // The one bucket the rest of this file leaves undefended: every other warnings assertion + // reads a total or a presence, and both survive returning the whole list unsliced. The + // budget is derived from the entries, so "twelve fit" is arithmetic rather than a guess. + const warnings = many(40, 'warning'); + const admitted = 12; + const results = new Map([['a.liquid', resultWith([], warnings)]]); + + const capped = capToBudget(results, bytesOf(warnings.slice(0, admitted))); + const result = capped.get('a.liquid')!; + + expect({ + status: result.status, + must_fix_before_write: result.must_fix_before_write, + errors: result.errors, + warnings: result.warnings, + infos: result.infos, + truncated: result.truncated!.warnings, + }).toEqual({ + status: 'warning', + must_fix_before_write: false, + errors: [], + warnings: warnings.slice(0, admitted), + infos: [], + truncated: { returned: admitted, total: warnings.length }, + }); + }); + it('spends the budget on errors before any info, across every file', async () => { // Severity-major allocation. A file that is nothing but infos must not consume // the budget a LATER file's errors need — note the noisy file is listed first, so diff --git a/packages/platformos-mcp-supervisor/src/validate/batch-bounds.spec.ts b/packages/platformos-mcp-supervisor/src/validate/batch-bounds.spec.ts new file mode 100644 index 00000000..4fe3f063 --- /dev/null +++ b/packages/platformos-mcp-supervisor/src/validate/batch-bounds.spec.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest'; + +import { MAX_BATCH_BYTES, MAX_BATCH_FILES, batchTooLarge } from './batch-bounds.js'; +import type { BufferToValidate } from './validate-buffers.js'; + +/** + * The request-level caps, tested directly. `validate-code.spec.ts` reaches them only + * incidentally, which left both boundaries unpinned. + * + * Refusal prose is not asserted: `batchTooLarge` is the subject here, so restating its own + * sentence would prove only that it was copied correctly. + */ + +const buffer = (index: number, content: string): BufferToValidate => ({ + filePath: `app/views/pages/p${index}.liquid`, + content, +}); + +/** Total bytes, by the same measure the cap bills. */ +const bytesOf = (buffers: readonly BufferToValidate[]): number => + buffers.reduce((total, entry) => total + Buffer.byteLength(entry.content, 'utf8'), 0); + +/** Buffers totalling EXACTLY `bytes`, split in two — these caps only run above one buffer. */ +const totalling = (bytes: number): BufferToValidate[] => { + const second = Math.floor(bytes / 2); + return [buffer(0, 'a'.repeat(bytes - second)), buffer(1, 'a'.repeat(second))]; +}; + +/** `count` buffers of one byte each, so nothing but the file count can refuse them. */ +const tinyFiles = (count: number): BufferToValidate[] => + Array.from({ length: count }, (_, index) => buffer(index, 'x')); + +describe('Unit: batchTooLarge', () => { + describe('the total-byte cap', () => { + it('ADMITS a request totalling exactly MAX_BATCH_BYTES', () => { + // Inclusive boundary, matching `bufferTooLarge`'s. The measured total rides along so a + // drifting fixture cannot quietly weaken this into "comfortably under the cap". + const buffers = totalling(MAX_BATCH_BYTES); + + expect({ total: bytesOf(buffers), refusal: batchTooLarge(buffers) }).toEqual({ + total: MAX_BATCH_BYTES, + refusal: undefined, + }); + }); + + it('REFUSES a request one byte over MAX_BATCH_BYTES', () => { + const buffers = totalling(MAX_BATCH_BYTES + 1); + + expect({ total: bytesOf(buffers), refusal: batchTooLarge(buffers)?.code }).toEqual({ + total: MAX_BATCH_BYTES + 1, + refusal: 'too_large', + }); + }); + + it('counts BYTES, not string length, so multi-byte content cannot slip past', () => { + // '€' is 3 bytes, so a `content.length` cap would admit three times the intended size — + // and every file in a request that big comes back `timed_out`, unchecked. + const perBuffer = Math.floor(MAX_BATCH_BYTES / 6) + 1; + const buffers = [buffer(0, '€'.repeat(perBuffer)), buffer(1, '€'.repeat(perBuffer))]; + const characters = buffers.reduce((total, entry) => total + entry.content.length, 0); + + expect({ + aLengthCapWouldAdmitIt: characters <= MAX_BATCH_BYTES, + theByteCapDoesNot: bytesOf(buffers) > MAX_BATCH_BYTES, + refusal: batchTooLarge(buffers)?.code, + }).toEqual({ aLengthCapWouldAdmitIt: true, theByteCapDoesNot: true, refusal: 'too_large' }); + }); + }); + + describe('the file-count cap', () => { + // One-byte buffers, with `wellUnderTheByteCap` asserted alongside, so only the count can + // be what answered. + + it('ADMITS exactly MAX_BATCH_FILES files', () => { + const buffers = tinyFiles(MAX_BATCH_FILES); + + expect({ + files: buffers.length, + wellUnderTheByteCap: bytesOf(buffers) <= MAX_BATCH_BYTES, + refusal: batchTooLarge(buffers), + }).toEqual({ files: MAX_BATCH_FILES, wellUnderTheByteCap: true, refusal: undefined }); + }); + + it('REFUSES one file more than MAX_BATCH_FILES', () => { + const buffers = tinyFiles(MAX_BATCH_FILES + 1); + + expect({ + files: buffers.length, + wellUnderTheByteCap: bytesOf(buffers) <= MAX_BATCH_BYTES, + refusal: batchTooLarge(buffers)?.code, + }).toEqual({ files: MAX_BATCH_FILES + 1, wellUnderTheByteCap: true, refusal: 'too_large' }); + }); + }); + + it('answers with the FILE-COUNT refusal when a request breaks BOTH caps', () => { + // No single-cap request can show precedence, and it is not cosmetic: splitting by BYTES + // can still leave too many files. The oracle is the same function asked about a count-only + // violation at the same file count, non-null asserted so it cannot silently be `undefined`. + const overBoth = [...totalling(MAX_BATCH_BYTES + 1), ...tinyFiles(MAX_BATCH_FILES)]; + const byCountAlone = batchTooLarge(tinyFiles(overBoth.length))!; + + expect({ refusal: batchTooLarge(overBoth), oracle: byCountAlone.code }).toEqual({ + refusal: byCountAlone, + oracle: 'too_large', + }); + }); + + it('gives the two caps DIFFERENT reasons, since both carry the same `code`', () => { + // Both carry `too_large`, so only the prose names which bound was hit. Collapsed into one + // message, a 51-file request gets told to get under a byte limit it is already far below. + const byCount = batchTooLarge(tinyFiles(MAX_BATCH_FILES + 1))!; + const byBytes = batchTooLarge(totalling(MAX_BATCH_BYTES + 1))!; + + expect({ + codes: [byCount.code, byBytes.code], + reasonsDiffer: byCount.reason !== byBytes.reason, + }).toEqual({ codes: ['too_large', 'too_large'], reasonsDiffer: true }); + }); +}); diff --git a/packages/platformos-mcp-supervisor/src/validate/batch-bounds.ts b/packages/platformos-mcp-supervisor/src/validate/batch-bounds.ts index d8adf46c..125cc566 100644 --- a/packages/platformos-mcp-supervisor/src/validate/batch-bounds.ts +++ b/packages/platformos-mcp-supervisor/src/validate/batch-bounds.ts @@ -44,6 +44,11 @@ export const MAX_BATCH_BYTES = maxBytesWithin(MAX_LINT_DEADLINE_MS); * validating a subset would report a changeset as checked when it was not. */ export function batchTooLarge(buffers: readonly BufferToValidate[]): Declined | undefined { + // UNREACHABLE OVER MCP — `VALIDATE_CODE_INPUT.files` caps the array with + // `.max(MAX_BATCH_FILES)` first, and `runValidateCode` is not on the package's public + // surface — so zero coverage here is the schema working, not a gap. Kept as defence-in-depth + // because only this bound lives inside the function that promises it, and asked FIRST + // because splitting a request by bytes can still leave too many files. if (buffers.length > MAX_BATCH_FILES) { return { code: 'too_large', diff --git a/packages/platformos-mcp-supervisor/src/validate/validate-buffers.ts b/packages/platformos-mcp-supervisor/src/validate/validate-buffers.ts index 7b53d250..42f78ba8 100644 --- a/packages/platformos-mcp-supervisor/src/validate/validate-buffers.ts +++ b/packages/platformos-mcp-supervisor/src/validate/validate-buffers.ts @@ -362,6 +362,12 @@ function projectScan( // Nothing in this changeset can HAVE dependants — every buffer is a YAML file, or sits in // no platformOS directory — so impact will never consult the scan and reading the project // would be pure waste. Decidable from the paths alone, before any I/O. + // + // A PERFORMANCE GUARD, NOT A CORRECTNESS ONE, which is why six mutants survive on it. + // `warm()` only PRE-warms: `scan.sources()` memoizes its promise and impact awaits that + // same promise itself, so inverting this buys a wasted read or a colder path, never a + // different answer. Nothing pins it — nor the `--no-impact` claim above, where what is + // tested is that the impact ADAPTER goes uncalled, not that the project read is skipped. const worthReading = ctx.impactEnabled !== false && lintable.some((buffer) => diff --git a/vitest.config.mjs b/vitest.config.mjs index 8c4ffea8..6de69eeb 100644 --- a/vitest.config.mjs +++ b/vitest.config.mjs @@ -10,9 +10,13 @@ const ciExclude = ['./packages/prettier-plugin-liquid']; export default defineConfig({ test: { + // `.stryker-tmp` is a COPY of a package with its sources mutated. Stryker removes it on + // a clean run and KEEPS it when one errors or is interrupted, so a leftover sandbox is + // the normal aftermath of a Ctrl-C — and without this every spec is then collected + // twice, the second copy running against deliberately broken code. exclude: CI - ? [...configDefaults.exclude, '**/dist/**', ...ciExclude] - : [...configDefaults.exclude, '**/dist/**'], + ? [...configDefaults.exclude, '**/dist/**', '**/.stryker-tmp/**', ...ciExclude] + : [...configDefaults.exclude, '**/dist/**', '**/.stryker-tmp/**'], // Spec files must run one at a time. That is not a preference here: // `config/load-config.spec.ts` installs mock packages into this package's REAL // `node_modules` to exercise sibling extension discovery, so a concurrently From 4f1e7a2e86b26579e1be0d93416ec7e321db0396 Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Thu, 3 Sep 2026 10:24:53 +0200 Subject: [PATCH 2/3] fix: clarify stream handling in response budget documentation --- .../src/result/response-budget.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/platformos-mcp-supervisor/src/result/response-budget.ts b/packages/platformos-mcp-supervisor/src/result/response-budget.ts index 6a3aa054..3ea487b4 100644 --- a/packages/platformos-mcp-supervisor/src/result/response-budget.ts +++ b/packages/platformos-mcp-supervisor/src/result/response-budget.ts @@ -17,9 +17,11 @@ * Within a file and bucket, entries are taken from the FRONT, so what survives is the head * of a list already ordered by line and column — where the root cause of a cascade is. * - * A STREAM THAT CANNOT FIT IS CLOSED, not skipped: once a bucket's next entry does not fit - * it takes nothing further, so the returned list stays a contiguous head rather than a - * scattered sample. + * A STREAM THAT CANNOT FIT TAKES NOTHING FURTHER, so the returned list stays a contiguous + * head rather than a scattered sample. That comes from `taken` not advancing — a retried + * entry costs the same and `spent` only grows, so it can never fit later. Closing the + * stream only saves the retry, and earns its keep there: 1.9 ms against 61 ms on a batch of + * blocked files, same output. * * ONE ERROR PER FILE IS GUARANTEED, budget or not — a blocked write with an empty `errors` * list names a problem and then declines to say what it is. Bounded: one entry per file, From bd7bb099793de9d40d0ba4f6c621c8455f6df2ce Mon Sep 17 00:00:00 2001 From: Filip Klosowski Date: Thu, 3 Sep 2026 12:24:24 +0200 Subject: [PATCH 3/3] test: add assertion for scheme anchoring in uriFromPathOrUri function --- ...th-containing-word-is-treated-as-a-URI.md" | 121 ++++++++++++++++++ .../platformos-common/src/os-path.spec.ts | 20 +++ 2 files changed, 141 insertions(+) create mode 100644 ".backlog/tasks/task-101 - hasSchemes-anchor-is-load-bearing-and-untested-\342\200\224-a-path-containing-word-is-treated-as-a-URI.md" diff --git "a/.backlog/tasks/task-101 - hasSchemes-anchor-is-load-bearing-and-untested-\342\200\224-a-path-containing-word-is-treated-as-a-URI.md" "b/.backlog/tasks/task-101 - hasSchemes-anchor-is-load-bearing-and-untested-\342\200\224-a-path-containing-word-is-treated-as-a-URI.md" new file mode 100644 index 00000000..2a3d18a9 --- /dev/null +++ "b/.backlog/tasks/task-101 - hasSchemes-anchor-is-load-bearing-and-untested-\342\200\224-a-path-containing-word-is-treated-as-a-URI.md" @@ -0,0 +1,121 @@ +--- +id: TASK-101 +title: >- + hasScheme's anchor is load-bearing and untested — a path containing "word:" is + treated as a URI +status: Done +assignee: [] +created_date: '2026-09-03 09:54' +updated_date: '2026-09-03 10:05' +labels: + - testing + - platformos-common + - cross-platform + - mutation-testing +dependencies: [] +references: + - packages/platformos-common/src/os-path.ts + - packages/platformos-common/src/os-path.spec.ts + - packages/platformos-check-common/src/ignore.ts +modified_files: + - packages/platformos-common/src/os-path.spec.ts +priority: medium +ordinal: 76000 +--- + +## Description + + +`uriFromPathOrUri` in `platformos-common/src/os-path.ts` decides whether its argument is a filesystem path or a URI, and routes it to `uriFromPath` or `normalizeUri` accordingly. That decision is made by one regex: + + function hasScheme(pathOrUri: string): boolean { + return /^[a-z][a-z0-9+.-]+:/i.test(pathOrUri); + } + +The `^` is load-bearing, and nothing asserts it. + +MEASURED — dropping the anchor changes the answer for a legal filename: + + input anchored unanchored + app/views/pages/index.liquid false false + C:\repo\app\x.liquid false false + /home/u/.../time_12:30.liquid false false + notes/TODO: rewrite.md false TRUE <-- diverges + file:///c:/a/x.liquid true true + mock-fs:/app/x.liquid true true + +A colon preceded by two or more `[a-z0-9+.-]` characters anywhere in the string is enough. `TODO:` qualifies; `a:` does not (the regex needs two characters before the colon), which is why a drive letter still reads as a path — the case the function's own docblock calls out. + +WHY IT MATTERS: a path misclassified as a URI goes to `normalizeUri` instead of `uriFromPath`, producing a plausible-looking URI for a different location. That is precisely the failure CLAUDE.md's three-normalizer rule exists to prevent, in the one function documented to accept "a path of unknown provenance" — a CLI argument, an `ignore` subject. `check-common/src/ignore.ts` calls it on every ignore subject, and `find-root.ts` on its input. + +MEASURED — nothing kills the mutant. With the anchor removed and platformos-common rebuilt, `os-path.spec.ts`, `find-root.spec.ts` and `check-common/src/ignore.spec.ts` all pass: 66 tests, 0 failures. + +The behaviour is CORRECT today. This task adds the assertion that keeps it correct. + +HOW THIS WAS FOUND, and what a future implementer needs: a local Stryker run over platformos-common's path and parsing primitives. Stryker is NOT part of this repository — its configs are gitignored — and you do not need it. Apply the one-line change by hand, confirm the new test fails, revert. That is the verification. + +ONE TRAP THAT COST TIME HERE: `check-common` imports `@platformos/platformos-common` from `dist`, not `src`. A cross-package sabotage that skips `yarn workspace @platformos/platformos-common build` tests the OLD code and reports a false "no test catches this". Rebuild after mutating and again after reverting. + + +## Acceptance Criteria + +- [x] #1 A test asserts that `uriFromPathOrUri` treats a WINDOWS-shaped path whose later segment contains `word:` as a PATH, returning the `uriFromPath` spelling rather than the `normalizeUri` one — corrected from the original wording, which named a posix example that cannot distinguish the two branches (see notes) +- [x] #2 A control in the same test asserts a genuine URI (a non-file scheme such as `mock-fs:/…`) is still treated as a URI, so the assertion cannot pass by classifying everything as a path +- [x] #3 A drive letter (`c:\\project\\app\\x.liquid`) is still treated as a path, pinning the two-character minimum the docblock relies on +- [x] #4 SABOTAGE-VERIFIED: removing `^` from the regex in `hasScheme` makes the new test fail, and no other test in platformos-common or platformos-check-common changes result; the change is reverted afterwards and both suites are green +- [x] #5 Assertions use whole-value equality on the returned URI per the repo's test guidelines, not a boolean `hasScheme` probe — `hasScheme` is private and the contract belongs to `uriFromPathOrUri` +- [x] #6 platformos-common and platformos-check-common suites pass, plus type-check and format:check + + +## Implementation Plan + + +1. Measure what the two branches actually RETURN for candidate inputs, not just how the regex classifies them. +2. Pick fixtures from that measurement rather than from the task description. +3. Add one test to the existing `uriFromPathOrUri` describe, with a scheme control in the same equality. +4. Sabotage with a rebuild of platformos-common (check-common imports `dist`), across BOTH package suites. + + +## Implementation Notes + + +THIS TASK'S OWN PREMISE WAS HALF WRONG, and finding that out changed the fixtures. The description asserted that dropping the `^` misroutes `notes/TODO: rewrite.md`. The REGEX does classify it differently — that part was measured. But the two branches then return the SAME STRING for a posix path: + + uriFromPath('/home/u/project/notes/TODO: rewrite.md') -> 'file:///home/u/project/notes/TODO: rewrite.md' + normalizeUri('/home/u/project/notes/TODO: rewrite.md') -> 'file:///home/u/project/notes/TODO: rewrite.md' + +So a posix fixture passes with the anchor deleted, and a test built on the description would have been decorative. This is the same error twice in one sitting: measuring the CLASSIFICATION and assuming the RESULT. Two claims, one measured. + +WHERE IT IS ACTUALLY OBSERVABLE — measured across the candidates, only Windows-shaped paths distinguish the branches: + + 'C:\\repo\\notes\\TODO: rewrite.md' anchored 'file:///c:/repo/notes/TODO: rewrite.md' unanchored 'C:/repo/notes/TODO: rewrite.md' + 'app\\views\\pages\\TODO: x.liquid' anchored 'file:///app/views/pages/TODO: x.liquid' unanchored THROWS UriError + '/home/u/…/notes/TODO: rewrite.md' identical either way + '../notes/TODO: rewrite.md' identical either way + 'C:\\repo\\app\\x.liquid' identical either way (one char before the colon, no match) + +The second case is the strongest statement of the contract: without the anchor, a function whose docblock says it takes anything crossing a public API THROWS on a path. Both fixtures are in the test, and the comment says why they are Windows-shaped so nobody 'simplifies' them to posix and quietly makes the test vacuous. + +HONEST NOTE ON REACHABILITY: a colon is illegal in a Windows filename, so these exact strings will not arrive from a Windows filesystem walk. They can arrive as an `ignore` pattern, a CLI argument, or a config authored elsewhere — which is exactly the 'unknown provenance' input this function exists for. The anchor is a guard on a narrow path, not a live bug. + +SABOTAGE: anchor removed and platformos-common rebuilt — platformos-common 1 failed / 588 passed, the failure being only the new test; platformos-check-common 1816/1816 still passed, so nothing anywhere else in the monorepo catches this. Reverted and rebuilt: 589 and 1816 green. + + +## Final Summary + + +Pins the `^` in `hasScheme`, which decides whether `uriFromPathOrUri` treats its argument as a path or a URI and which nothing in the monorepo asserted. One test added to `os-path.spec.ts`; **no source file changed**. + +**The fixtures are not the ones this task asked for, and that is the substance of the change.** The description claimed the anchor is observable on `notes/TODO: rewrite.md`. It is not: the regex classifies that path differently without `^`, but `uriFromPath` and `normalizeUri` then return the *same string* for any posix path, so a test written to the description would have passed with the anchor deleted. The premise measured the classification and assumed the result. + +Measured across candidates, only Windows-shaped inputs distinguish the branches, and both are now in the test: + +- `C:\repo\notes\TODO: rewrite.md` — correct `file:///c:/repo/notes/TODO: rewrite.md`, unanchored `C:/repo/notes/TODO: rewrite.md` (no scheme, drive not lowercased). +- `app\views\pages\TODO: x.liquid` — correct `file:///app/views/pages/TODO: x.liquid`, unanchored **throws `UriError`**. That is the sharper statement of the contract: the function's docblock says it takes anything crossing a public API, and without the anchor it throws on a path. + +A `mock-fs:` control rides in the same equality so the test cannot pass against a function that stopped recognising schemes altogether, and the comment records *why* the fixtures are Windows-shaped — a later "simplification" to a posix path would silently make the test vacuous. + +**Verification.** Anchor removed and platformos-common rebuilt (check-common imports `dist`, not `src`): platformos-common 1 failed / 588 passed, the failure being only the new test; platformos-check-common 1816/1816 still passed, confirming nothing else in the monorepo catches this. Reverted and rebuilt: platformos-common 589/589, platformos-check-common 1816/1816, type-check and prettier clean. + +**Scope note kept deliberately honest:** a colon is illegal in a Windows filename, so these strings cannot arrive from a filesystem walk. They can arrive as an `ignore` pattern, a CLI argument, or a config authored on another OS — the unknown-provenance input this function exists for. The anchor is a guard on a narrow path, not a live bug, and the test is worth its four lines on that basis rather than a stronger one. + diff --git a/packages/platformos-common/src/os-path.spec.ts b/packages/platformos-common/src/os-path.spec.ts index e732f4ad..ba7ac6ca 100644 --- a/packages/platformos-common/src/os-path.spec.ts +++ b/packages/platformos-common/src/os-path.spec.ts @@ -103,6 +103,26 @@ describe('uriFromPathOrUri', () => { expect(uriFromPathOrUri('c:\\project\\app\\x.liquid')).toBe('file:///c:/project/app/x.liquid'); }); + it('anchors the scheme at the START, so a colon deeper in a path is not one', () => { + // `TODO:` satisfies `[a-z][a-z0-9+.-]+:` on its own, so only the `^` keeps these on the + // path branch. Both fixtures are Windows-shaped ON PURPOSE: for a posix path the two + // branches happen to return the same string, so a posix fixture passes with the anchor + // deleted. Measured — without it the first comes back with no scheme and an + // un-lowercased drive (`C:/repo/...`), and the second THROWS `UriError`, out of the + // function documented to take anything crossing a public API. + expect([ + uriFromPathOrUri('C:\\repo\\notes\\TODO: rewrite.md'), + uriFromPathOrUri('app\\views\\pages\\TODO: x.liquid'), + // Control: a real scheme is still a scheme, or the two above would pass just as well + // against a function that had stopped recognising schemes at all. + uriFromPathOrUri('mock-fs:/app/TODO: x.liquid'), + ]).toEqual([ + 'file:///c:/repo/notes/TODO: rewrite.md', + 'file:///app/views/pages/TODO: x.liquid', + 'mock-fs:/app/TODO: x.liquid', + ]); + }); + it('normalizes a URI it is handed', () => { expect([ uriFromPathOrUri('file:///c%3A/project/app/x.liquid'),