diff --git a/.project/cards/T-0147-truncating-a-context-bundle-overwrites-the-flag-sa.md b/.project/cards/T-0147-truncating-a-context-bundle-overwrites-the-flag-sa.md index 9e5e0f8..bafcceb 100644 --- a/.project/cards/T-0147-truncating-a-context-bundle-overwrites-the-flag-sa.md +++ b/.project/cards/T-0147-truncating-a-context-bundle-overwrites-the-flag-sa.md @@ -1,14 +1,20 @@ --- id: T-0147 title: Truncating a context bundle overwrites the flag saying it was truncated -status: backlog +status: done type: bug priority: low area: mcp tags: [mcp, truncation] created: 2026-08-03 -updated: 2026-08-04 +updated: 2026-08-07 origin: [T-0146] +scope: [packages/workfile/src/modules/mcp] +verified: + at: "2026-08-07T22:58:23.966Z" + method: local + commit: 9cfb0175194fc944ab34f527c800adf4c1b486d2 + digest: "sha256:cae81812f469d88981a5082dbb9a514cb0a7a5dea3d6cd3a5dc15cdea30befb5" --- `buildAgentContext` returns `truncated: boolean` — true when related records @@ -43,9 +49,18 @@ not an assumption to build a fix on. ## Acceptance criteria -- [ ] A test drives `project_agent_context` past `maxToolResultBytes` and pins +- [x] A test drives `project_agent_context` past `maxToolResultBytes` and pins what the two markers do to each other -- [ ] The byte-ceiling marker stops colliding with tool payload fields — a +- [x] The byte-ceiling marker stops colliding with tool payload fields — a distinct key, or a nested one -- [ ] `outputSchema` for `project_agent_context` matches whatever wins -- [ ] `docs/mcp.md` describes the marker that ends up shipping +- [x] `outputSchema` for `project_agent_context` matches whatever wins +- [x] `docs/mcp.md` describes the marker that ends up shipping + +## Activity + +- 2026-08-07 22:17Z illodev@local#42eb42f5 · claimed +- 2026-08-07 22:58Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 22:58Z illodev@local#42eb42f5 — local verification: Reproduced first, as the card asked: with the ceiling lowered, project_agent_context returned `truncated: {"records":1}` where its outputSchema declares a boolean. The byte-ceiling marker is `resultTruncated` now, declared in every tool's schema rather than merely allowed, and docs/mcp.md says which of the two is the transport speaking. Mutation-proven: restoring the shared key reports the object where a boolean belongs. Full gate green at 484 + 10 tests. diff --git a/.project/cards/T-0160-root-accepts-any-directory-and-answers-from-an-emp.md b/.project/cards/T-0160-root-accepts-any-directory-and-answers-from-an-emp.md index 799fe9c..eff984b 100644 --- a/.project/cards/T-0160-root-accepts-any-directory-and-answers-from-an-emp.md +++ b/.project/cards/T-0160-root-accepts-any-directory-and-answers-from-an-emp.md @@ -1,15 +1,20 @@ --- id: T-0160 title: --root accepts any directory and answers from an empty workspace -status: backlog +status: done type: bug priority: low area: core effort: S -scope: [packages/workfile/bin/workfile.ts, packages/workfile/src/core] +scope: [packages/workfile/bin/workfile.ts, packages/workfile/src/workspace] origin: [T-0159] created: 2026-08-04 -updated: 2026-08-04 +updated: 2026-08-07 +verified: + at: "2026-08-07T22:58:21.710Z" + method: local + commit: 9cfb0175194fc944ab34f527c800adf4c1b486d2 + digest: "sha256:e3afefb220be80685c23d420ec7ffb7e24b0f57d9ada1868fbf6dd3707a8c919" --- `workfile doctor --root packages/workfile` runs. It reports six @@ -48,8 +53,17 @@ resolving to a parent would be a second surprise rather than a fix. ## Acceptance criteria -- [ ] `--root` pointed at a directory with no workspace marker is an error -- [ ] The error names the directory and points at `init` or `--allow-new` -- [ ] `--allow-new` still accepts a not-yet-workspace directory -- [ ] Nothing writes a cache into a directory that failed the check -- [ ] `pnpm run check` green, doctor 0/0 +- [x] `--root` pointed at a directory with no workspace marker is an error +- [x] The error names the directory and points at `init` or `--allow-new` +- [x] `--allow-new` still accepts a not-yet-workspace directory +- [x] Nothing writes a cache into a directory that failed the check +- [x] `pnpm run check` green, doctor 0/0 + +## Activity + +- 2026-08-07 22:17Z illodev@local#42eb42f5 · claimed +- 2026-08-07 22:58Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 22:58Z illodev@local#42eb42f5 — local verification: The card's own reproduction now exits 2: `doctor --root packages/workfile` names the directory and both ways forward instead of reporting six issues and exiting 0. `card list` the same. `--allow-new` still accepts one, and it reaches that branch now — it never did, which is how an explicit root came to check nothing. A real workspace is unaffected, and nothing was written into the directory that failed. Strict rather than a walk, so a root inside a workspace is refused rather than silently resolved upward; both halves mutation-proven. diff --git a/.project/cards/T-0218-a-doctor-issue-never-says-which-module-produced-it.md b/.project/cards/T-0218-a-doctor-issue-never-says-which-module-produced-it.md index 1b5301b..16d163c 100644 --- a/.project/cards/T-0218-a-doctor-issue-never-says-which-module-produced-it.md +++ b/.project/cards/T-0218-a-doctor-issue-never-says-which-module-produced-it.md @@ -1,7 +1,7 @@ --- id: T-0218 title: A doctor issue never says which module produced it -status: backlog +status: done type: task priority: low area: core @@ -11,6 +11,11 @@ scope: [packages/workfile/src/modules/health] origin: [T-0213, LRN-0029] created: 2026-08-07 updated: 2026-08-07 +verified: + at: "2026-08-07T22:58:22.248Z" + method: local + commit: 9cfb0175194fc944ab34f527c800adf4c1b486d2 + digest: "sha256:61c9511a6a827f6742debb2f977250551ca11a5c12bfa39a84e008eb55a0a76d" --- Found while auditing T-0213. @@ -37,6 +42,20 @@ field, leave `code` alone, and have the CLI and the Health view group by it. ## Acceptance criteria -- [ ] A doctor issue carries the module that produced it, integrations included. -- [ ] An existing accepted baseline still matches, proven by a test. -- [ ] The CLI and `/api/v2/health` consumers can tell an integration's finding from a core one. +- [x] A doctor issue carries the module that produced it, integrations included. +- [x] An existing accepted baseline still matches, proven by a test. +- [x] The CLI and `/api/v2/health` consumers can tell an integration's finding from a core one. + +## Activity + +- 2026-08-07 22:17Z illodev@local#42eb42f5 · claimed +- 2026-08-07 22:58Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 22:26Z illodev@local#42eb42f5 — The card's premise was wrong and it changed where the fix goes. It says "every module that reports health returns `{ module, issues }`" — only the integration registry does. `diagnoseCards`, the docs, changelog and memory reports, `checkAgentInstructions` and `checkCiTemplates` all return `{ counts, ok, issues }` and no module at all, so stamping `report.module` onto each issue produced `module: undefined` on 1396 of the 1397 findings in a real workspace. Verified by running it against one rather than by reading. + +So the module is named where `runDoctor` calls each reporter, which is the only place that knows which one it is calling — one table, and the reports hanging off the shared index are tagged rather than mutated so the routes that also serve them are untouched. + +The other decision the card left open is settled the way it guessed: `module` is a field and `code` is untouched. Namespacing the code would read better and would change `issueIdentity`, which is what a baseline is matched by, so every baseline accepted with `--accept-baseline` would go stale at once for a cosmetic gain. Pinned by a test that fails if the identity grows the field. +- 2026-08-07 22:58Z illodev@local#42eb42f5 — local verification: Against Fube's 1397 findings: every one now names its module, 0 with none, where the first attempt produced `undefined` on 1396 because the card's premise about `{ module, issues }` was wrong. The CLI's summary groups by module and code, so `integration:/` is unmistakable. An accepted baseline still matches, pinned by a test that fails if `issueIdentity` grows the field — mutation-proven. diff --git a/.project/cards/T-0219-the-claim-board-carries-no-session-so-the-guard-mi.md b/.project/cards/T-0219-the-claim-board-carries-no-session-so-the-guard-mi.md index 0a93ef5..f35b971 100644 --- a/.project/cards/T-0219-the-claim-board-carries-no-session-so-the-guard-mi.md +++ b/.project/cards/T-0219-the-claim-board-carries-no-session-so-the-guard-mi.md @@ -1,7 +1,7 @@ --- id: T-0219 title: The claim board carries no session, so the guard misses a shared actor -status: backlog +status: done type: task priority: low area: core @@ -12,6 +12,11 @@ related: [T-0089] origin: [T-0206, LRN-0030] created: 2026-08-07 updated: 2026-08-07 +verified: + at: "2026-08-07T22:58:23.322Z" + method: local + commit: 9cfb0175194fc944ab34f527c800adf4c1b486d2 + digest: "sha256:89f0faee49e0513ddaf53fd252b98b26d1965433665d6a7341bf822cd8ab4cdf" --- The residual ADR-0020 left open, recorded in full in LRN-0030. @@ -40,7 +45,16 @@ T-0089 is already about board staleness and would pass through the same code. ## Acceptance criteria -- [ ] A board entry carries the session resolved for its claim, from either source. -- [ ] Both writers of the board — `rebuildClaimBoard` and the hook's `buildBoard` — produce the same entry for the same card, proven by a test. -- [ ] Two agents sharing an explicit `--actor` make the guard prompt, proven by driving the real hook. -- [ ] The hook's latency budget still holds. +- [x] A board entry carries the session resolved for its claim, from either source. +- [x] Both writers of the board — `rebuildClaimBoard` and the hook's `buildBoard` — produce the same entry for the same card, proven by a test. +- [x] Two agents sharing an explicit `--actor` make the guard prompt, proven by driving the real hook. +- [x] The hook's latency budget still holds. + +## Activity + +- 2026-08-07 22:17Z illodev@local#42eb42f5 · claimed +- 2026-08-07 22:58Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 22:58Z illodev@local#42eb42f5 — local verification: Driven through the real hook, four cases: the holding session stays silent; two agents with different sessions and the same explicit --actor now prompt, which is the case LRN-0030 left open; two session-free terminals with one actor stay silent because unproven is not a verdict; different actors prompt. The board carries the session from either source and both writers agree, pinned by a test that now compares the rule the guard applies rather than assuming null on both sides — it was agreeing by coincidence. The latency budget test passes, and the hot path gained one string operation and no I/O. Two mutations caught: dropping the field, and collapsing the guard back to comparing actors. diff --git a/.project/cards/T-0223-only-cards-notice-that-a-retitled-record-s-filenam.md b/.project/cards/T-0223-only-cards-notice-that-a-retitled-record-s-filenam.md index d8e69b0..a5ba6e7 100644 --- a/.project/cards/T-0223-only-cards-notice-that-a-retitled-record-s-filenam.md +++ b/.project/cards/T-0223-only-cards-notice-that-a-retitled-record-s-filenam.md @@ -1,16 +1,21 @@ --- id: T-0223 title: Only cards notice that a retitled record's filename went stale -status: backlog +status: done type: bug priority: low area: core tags: [health] effort: S -scope: [packages/workfile/src/modules/health] +scope: [packages/workfile/src/modules/health, packages/workfile/src/modules/memory, packages/workfile/src/modules/docs, packages/workfile/src/modules/changelog] origin: [LRN-0033] created: 2026-08-07 updated: 2026-08-07 +verified: + at: "2026-08-07T22:58:22.788Z" + method: local + commit: 9cfb0175194fc944ab34f527c800adf4c1b486d2 + digest: "sha256:5efda4a8354950bd3d335c443a3dc01b1322f0ea7a1ba1395fd32aacc719fd49" --- `diagnoseCards` reports `filename-stale` when a card's filename no longer matches its title, and `doctor --fix` renames it. The comment there states the reason plainly: the filename is the handle people and agents grep by, and a stale one misdirects long after anyone remembers the rename. @@ -21,8 +26,17 @@ Worth doing as one pass rather than per collection, and worth doing at all for t ## Acceptance criteria -- [ ] `doctor` reports a stale filename for a memory record, a managed document and a changelog fragment, as a warning, the way it does for a card. -- [ ] `doctor --fix` renames them, and a record whose id is referenced elsewhere keeps resolving. -- [ ] The rule is written once rather than per collection. -- [ ] A record whose filename is stale because it was renamed *by hand* to something legitimate is not fought over — state what happens. -- [ ] `pnpm run check` green, doctor 0/0. +- [x] `doctor` reports a stale filename for a memory record, a managed document and a changelog fragment, as a warning, the way it does for a card. +- [x] `doctor --fix` renames them, and a record whose id is referenced elsewhere keeps resolving. +- [x] The rule is written once rather than per collection. +- [x] A record whose filename is stale because it was renamed *by hand* to something legitimate is not fought over — state what happens. +- [x] `pnpm run check` green, doctor 0/0. + +## Activity + +- 2026-08-07 22:17Z illodev@local#42eb42f5 · claimed +- 2026-08-07 22:58Z illodev@local#42eb42f5 · released + +## Notes + +- 2026-08-07 22:58Z illodev@local#42eb42f5 — local verification: One record of each kind created, retitled and repaired end to end with the shipped binary: four `filename-stale` warnings attributed to cards, memory, docs and changelog, then `doctor --fix` renamed all four, ids still resolve and doctor is quiet. On Fube the rule finds 53 findings that were invisible — 35 docs, 18 memory. The exclusions were checked rather than assumed: an indexed README is not compared, and a released fragment cannot even be retitled — the protocol answers CHANGE_FRAGMENT_RELEASED — so that exclusion covers a hand edit. The rule caught my own hand-rename of LRN-0033 one commit after I made it, which is what closed criterion 5. diff --git a/.project/cards/T-0224-a-trailing-separator-strip-is-written-five-times-a.md b/.project/cards/T-0224-a-trailing-separator-strip-is-written-five-times-a.md new file mode 100644 index 0000000..f720eae --- /dev/null +++ b/.project/cards/T-0224-a-trailing-separator-strip-is-written-five-times-a.md @@ -0,0 +1,24 @@ +--- +id: T-0224 +title: A trailing-separator strip is written five times, as a regex +status: backlog +type: chore +priority: low +area: core +tags: [security] +effort: S +created: 2026-08-07 +updated: 2026-08-07 +--- + +CodeQL reported `js/polynomial-redos` against `routeRoots` in `docs/validation.ts`: `replace(/\/+$/, "")` retries the anchored `+` from every start position, so a value of N slashes costs O(N²). The alert was high and it blocked a pull request that had nothing to do with it. + +The spelling appeared five times across the package and once more in the hook, and only one of them was reachable from a declared value — which is why only one was reported. Replaced by `stripTrailingSlashes` in `core/glob.ts`, a linear loop, plus the hook's own copy since it imports nothing. + +Filed to record what is not done: nothing stops the sixth copy. The suite has no rule against an anchored quantifier over a value that comes out of config or a card, and CodeQL only reports the ones whose taint it can follow. + +## Acceptance criteria + +- [ ] A test or lint rule fails on an anchored `+`/`*` applied to a config or record value. +- [ ] It names the linear alternative rather than only refusing. +- [ ] The existing six sites pass it. diff --git a/.project/changelog/unreleased/CHG-0155-an-explicit-root-is-checked-the-way-a-discovered-one-always-was.md b/.project/changelog/unreleased/CHG-0155-an-explicit-root-is-checked-the-way-a-discovered-one-always-was.md new file mode 100644 index 0000000..e904f89 --- /dev/null +++ b/.project/changelog/unreleased/CHG-0155-an-explicit-root-is-checked-the-way-a-discovered-one-always-was.md @@ -0,0 +1,12 @@ +--- +id: CHG-0155 +title: An explicit --root is checked the way a discovered one always was +type: fixed +area: core +visibility: public +cards: [T-0160] +created: 2026-08-07 +updated: 2026-08-07 +--- + +`--root` pointed at a directory with no workspace marker is an error naming the directory, instead of a clean, empty, believable answer from somewhere that is not a workspace. `--allow-new` is the way through and reaches that branch now; it only ever reached the path that already refused one. diff --git a/.project/changelog/unreleased/CHG-0156-every-doctor-finding-says-which-module-produced-it.md b/.project/changelog/unreleased/CHG-0156-every-doctor-finding-says-which-module-produced-it.md new file mode 100644 index 0000000..440baac --- /dev/null +++ b/.project/changelog/unreleased/CHG-0156-every-doctor-finding-says-which-module-produced-it.md @@ -0,0 +1,12 @@ +--- +id: CHG-0156 +title: Every doctor finding says which module produced it +type: changed +area: core +visibility: public +cards: [T-0218, T-0223] +created: 2026-08-07 +updated: 2026-08-07 +--- + +A diagnostic returned by a repository's own healthCheck used to read exactly like one Workfile made itself. Each finding now carries its reporter and the CLI groups by it, so `integration:/` is unmistakable. Accepted baselines are unaffected: the identity a baseline matches on deliberately ignores the field. The stale-filename rule moved to the layer that holds every kind while this was open, so memory records, managed documents and unreleased changelog fragments are checked and repaired too — they derived their filenames from their titles all along and only cards ever noticed. diff --git a/.project/changelog/unreleased/CHG-0157-the-scope-guard-sees-two-agents-that-share-an-actor.md b/.project/changelog/unreleased/CHG-0157-the-scope-guard-sees-two-agents-that-share-an-actor.md new file mode 100644 index 0000000..af50c45 --- /dev/null +++ b/.project/changelog/unreleased/CHG-0157-the-scope-guard-sees-two-agents-that-share-an-actor.md @@ -0,0 +1,12 @@ +--- +id: CHG-0157 +title: The scope guard sees two agents that share an actor +type: fixed +area: core +visibility: public +cards: [T-0219] +created: 2026-08-07 +updated: 2026-08-07 +--- + +A claim board entry carries the session behind its claim, so the guard no longer has to recover one from the actor's tail — which a `claimed_by` written from an explicit `--actor` does not have. Two agents handed the same actor saw a string equal to their own and the guard stayed silent; it prompts now. The session that holds a claim is still free to work, and two session-free terminals sharing one actor are still left alone, because unproven is not a verdict. diff --git a/.project/changelog/unreleased/CHG-0158-the-mcp-byte-ceiling-stops-overwriting-a-tool-s-own-truncation-flag.md b/.project/changelog/unreleased/CHG-0158-the-mcp-byte-ceiling-stops-overwriting-a-tool-s-own-truncation-flag.md new file mode 100644 index 0000000..ab89c7a --- /dev/null +++ b/.project/changelog/unreleased/CHG-0158-the-mcp-byte-ceiling-stops-overwriting-a-tool-s-own-truncation-flag.md @@ -0,0 +1,12 @@ +--- +id: CHG-0158 +title: The MCP byte ceiling stops overwriting a tool's own truncation flag +type: fixed +area: mcp +visibility: public +cards: [T-0147] +created: 2026-08-07 +updated: 2026-08-07 +--- + +A reply degraded for exceeding maxToolResultBytes is marked `resultTruncated`, not `truncated`. The old key collided with what project_agent_context already meant by it: a large bundle replaced a boolean with an object, so a caller checking `=== true` survived only because an object is truthy, and a caller reading `truncated.records` on any other tool got `true` from that one. Both markers are declared in every tool's outputSchema. diff --git a/.project/memory/learnings/LRN-0033-a-cards-finding-is-evidence-its-scope-and-its-stated-fix-are-a-guess.md b/.project/memory/learnings/LRN-0033-a-card-s-finding-is-evidence-its-scope-and-its-stated-fix-are-a-guess.md similarity index 100% rename from .project/memory/learnings/LRN-0033-a-cards-finding-is-evidence-its-scope-and-its-stated-fix-are-a-guess.md rename to .project/memory/learnings/LRN-0033-a-card-s-finding-is-evidence-its-scope-and-its-stated-fix-are-a-guess.md diff --git a/packages/workfile/bin/workfile.ts b/packages/workfile/bin/workfile.ts index 3fc1a6d..e627d93 100644 --- a/packages/workfile/bin/workfile.ts +++ b/packages/workfile/bin/workfile.ts @@ -62,6 +62,7 @@ import { renderChangelog, renumberCard, reslugStaleCardFiles, + reslugStaleRecordFiles, reopenCard, runUpgrade, runDoctor, @@ -2633,7 +2634,15 @@ async function main() { // still wrote. `ensureWritable` is the guard that actually holds. const workspace = await loadWorkspace( explicitRoot - ? { root: explicitRoot, readOnly: has("--read-only") } + ? { + root: explicitRoot, + // `--allow-new` reaches both branches now. It did not, which + // is how `--root` came to check nothing at all: the flag that + // means "accept a directory that is not yet a workspace" was + // only wired to the path that already refused one (T-0160). + allowMissing: has("--allow-new"), + readOnly: has("--read-only") + } : { cwd: root, allowMissing: has("--allow-new"), @@ -2656,10 +2665,10 @@ async function main() { let fixed: | (Awaited> & { renamed: Awaited< - ReturnType + ReturnType >["moves"]; renameSkipped: Awaited< - ReturnType + ReturnType >["skipped"]; trails: Awaited< ReturnType @@ -2672,7 +2681,11 @@ async function main() { // Renaming runs after the ID repair: a card that just moved to a // fresh ID keeps the old title slug, and this is what brings the // whole filename back in step. - const renamed = await reslugStaleCardFiles(workspace, { actor }); + // Every kind, not only cards. Memory records, managed documents and + // unreleased changelog fragments derive their filenames from their + // titles the same way and had neither the rule nor the repair + // (T-0223). + const renamed = await reslugStaleRecordFiles(workspace, { actor }); // Last, because both repairs above rewrite whole files and this one // reads the body it finds afterwards. const trails = await healMisplacedTrailEntries(workspace, { actor }); @@ -2786,9 +2799,15 @@ async function main() { } // Grouped counts, so a wall of one repeated rule reads as one // problem rather than as hundreds. + // Keyed by module and code, not by code alone. A core `code` implies + // its module to anyone who knows the codebase; a diagnostic returned + // by a repository's own `healthCheck` implies nothing, and used to + // read exactly like one Workfile produced. `integration:/` + // is unmistakable (T-0218). const byCode = new Map(); for (const issue of shown) { - byCode.set(issue.code, (byCode.get(issue.code) || 0) + 1); + const key = issue.module ? `${issue.module}/${issue.code}` : issue.code; + byCode.set(key, (byCode.get(key) || 0) + 1); } if (byCode.size) { console.log("\nBy rule:"); diff --git a/packages/workfile/docs/mcp.md b/packages/workfile/docs/mcp.md index 2377bba..5a088e0 100644 --- a/packages/workfile/docs/mcp.md +++ b/packages/workfile/docs/mcp.md @@ -34,8 +34,18 @@ conflict. Every result carries the data once, in `structuredContent`; `content` is a one-line summary rather than a second copy of the payload. When a result would -exceed `maxToolResultBytes` it is truncated with a `truncated` marker instead of -failing the call, because a get-by-id has no query to narrow. +exceed `maxToolResultBytes` the server degrades it rather than failing the call, +because a get-by-id has no query to narrow — and says so with +**`resultTruncated`**: `{ records: }`, or +`{ bodyBytes: }` when a single record's body was clipped. + +That marker is the transport speaking, and it is deliberately not called +`truncated`. A tool may declare a `truncated` of its own meaning something else +entirely: `project_agent_context` returns `truncated: boolean` for relations +dropped to respect `limit`, and the two used to be one key — so a large bundle +replaced the boolean with an object, a caller checking `=== true` survived by +accident because an object is truthy, and a caller reading `truncated.records` on +any other tool got `true` from that one. ## Claude Code integration @@ -208,9 +218,11 @@ Every tool declares its full contract, so a caller never has to infer one: - **Defaults are declared where the implementation has one**, rather than left for the caller to discover by omitting the field. - **Every tool declares an `outputSchema`** matching the `structuredContent` it - returns. None of them is a closed object: a payload over `maxToolResultBytes` - gains a `truncated` marker, and a schema that forbade it would invalidate the - server's own degradation path. + returns, including `resultTruncated` — declared rather than merely allowed, so + a caller reads it from the schema instead of meeting it the first time a + payload gets large. None of them is a closed object either: the degradation + path adds a field, and a schema that forbade it would invalidate the server's + own answer. `project_card_release` is the one place where an enum is narrower than the protocol's: a released card cannot stay `doing`, so that value is refused as an diff --git a/packages/workfile/src/core/glob.ts b/packages/workfile/src/core/glob.ts index 12b6ee3..a43efe5 100644 --- a/packages/workfile/src/core/glob.ts +++ b/packages/workfile/src/core/glob.ts @@ -52,6 +52,22 @@ export function globToRegExp(pattern) { return compiled; } +/** + * Trailing separators removed, without a regex. + * + * `replace(/\/+$/, "")` is the obvious spelling and CodeQL is right about it: + * the engine retries the anchored `+` from every start position, so a value of N + * slashes costs O(N²). Nothing here is attacker-controlled — the values are + * config entries and repository paths a maintainer writes — but the loop is + * shorter than the argument for keeping the regex, and it was written five times + * across this package before this existed. + */ +export function stripTrailingSlashes(value: string): string { + let end = value.length; + while (end > 0 && value[end - 1] === "/") end -= 1; + return end === value.length ? value : value.slice(0, end); +} + export function normalizeRepoPath(value) { return String(value).split(sep).join("/").replace(/^\.\//, ""); } diff --git a/packages/workfile/src/index.ts b/packages/workfile/src/index.ts index 202ff33..73f408a 100644 --- a/packages/workfile/src/index.ts +++ b/packages/workfile/src/index.ts @@ -139,7 +139,8 @@ export { healDuplicateRecordIds, renumberCard, renumberRecord, - reslugStaleCardFiles + reslugStaleCardFiles, + reslugStaleRecordFiles } from "./modules/health/renumber.js"; export { HEALABLE_KINDS, diff --git a/packages/workfile/src/modules/cards/cards.ts b/packages/workfile/src/modules/cards/cards.ts index 31906cb..b35d4a3 100644 --- a/packages/workfile/src/modules/cards/cards.ts +++ b/packages/workfile/src/modules/cards/cards.ts @@ -332,28 +332,14 @@ export async function diagnoseCards({ `Filename must start with ${card.id}-` ) ); - } else if ( - card.id && - card.title && - basename(card.file || "") !== cardFileName(card.id, card.title) - ) { - // Creating a card derives the filename from the title; retitling it - // never revisited that, so a file could sit for months named after a - // title the card no longer has. The filename is the handle people - // and agents grep by, and a stale one misdirects long after anyone - // remembers the rename. A warning rather than an error: the record - // is intact and only its label has drifted, and renaming on every - // title edit would churn history and break open editor buffers — - // so the repair is `doctor --fix`, when the reader asks for it. - issues.push( - issue( - "warning", - "filename-stale", - card, - `Filename no longer matches the title; \`doctor --fix\` renames it to ${cardFileName(card.id, card.title)}` - ) - ); } + // The stale-filename rule used to sit here, and it only ever covered + // cards. It moved to `health/filenames.ts`, which is the layer that + // holds every kind: memory records, managed documents and unreleased + // changelog fragments derive their names from their titles identically + // and had no rule at all (T-0223). The same argument + // `duplicate-record-id` makes — a module sees one kind, and this + // question is about all of them. if ((card.title || "").length > 80) { issues.push( issue( diff --git a/packages/workfile/src/modules/cards/claims.ts b/packages/workfile/src/modules/cards/claims.ts index 18d541c..dfe3b95 100644 --- a/packages/workfile/src/modules/cards/claims.ts +++ b/packages/workfile/src/modules/cards/claims.ts @@ -142,7 +142,7 @@ function boardPath(workspace) { } /** The board's view of one card, or null when it holds no claim. */ -export function claimBoardEntry(card) { +export function claimBoardEntry(card, sessions: any[] = []) { if (!card?.claimed_by) return null; return { id: card.id, @@ -150,6 +150,21 @@ export function claimBoardEntry(card) { status: card.status, claimedBy: card.claimed_by, claimedAt: card.claimed_at, + /** + * The session this claim belongs to, resolved here because here is where + * the session files are in hand (T-0219). + * + * The board carried `claimedBy` and nothing else, so the scope guard — + * which reads only this file — could recover a session from the actor's + * tail and no other way. A `claimed_by` written from an explicit + * `--actor` has no tail, so two agents sharing one saw a string equal to + * their own and the guard stayed silent. That is the residual ADR-0020 + * left open, and LRN-0030 records it. + * + * `null` when there is none to find, which the guard has to treat as + * "unproven" rather than as "the same process". + */ + session: sessionForClaim(card, sessions), scope: Array.isArray(card.scope) ? card.scope : card.scope @@ -158,6 +173,26 @@ export function claimBoardEntry(card) { }; } +/** + * The session behind a claim: the one that named this card, else the one + * belonging to this actor, else whatever the actor's tail carries. + * + * The first two are the same two-step `claimState` and the activity snapshot + * take, in that order and for the reason T-0206 established — a session that + * names the card beats one that merely shares an actor, because two agents can + * share an actor. + */ +function sessionForClaim(card, sessions: any[]): string | null { + const match = + sessions.find((candidate) => candidate.cardId === card.id) || + sessions.find((candidate) => candidate.actor === card.claimed_by); + return ( + sessionDiscriminator(match?.sessionId) ?? + claimSession({ by: card.claimed_by }) ?? + null + ); +} + export async function readClaimBoard(workspace) { try { return JSON.parse(await readFile(boardPath(workspace), "utf8")); @@ -195,7 +230,7 @@ export async function updateClaimBoard(workspace, card, { now = new Date() } = { const claims = (board.claims || []).filter( (claim) => claim.id !== card.id ); - const entry = claimBoardEntry(card); + const entry = claimBoardEntry(card, await readAgentSessions(workspace, { now })); if (entry) claims.push(entry); return writeBoard(workspace, claims, now); }, @@ -205,7 +240,13 @@ export async function updateClaimBoard(workspace, card, { now = new Date() } = { /** The whole board from a listing, for session start and for repair. */ export async function rebuildClaimBoard(workspace, cards, { now = new Date() } = {}) { - return writeBoard(workspace, cards.map(claimBoardEntry).filter(Boolean), now); + // Read once for the whole sweep rather than per card. + const sessions = await readAgentSessions(workspace, { now }); + return writeBoard( + workspace, + cards.map((card) => claimBoardEntry(card, sessions)).filter(Boolean), + now + ); } /** diff --git a/packages/workfile/src/modules/cards/validation.ts b/packages/workfile/src/modules/cards/validation.ts index 4c08d7a..5be6da2 100644 --- a/packages/workfile/src/modules/cards/validation.ts +++ b/packages/workfile/src/modules/cards/validation.ts @@ -7,6 +7,7 @@ import { VERIFICATION_POLICY_DEFAULT_AREA, VERIFY_TIMEOUT_SECONDS_DEFAULT } from "../../config/defaults.js"; +import { stripTrailingSlashes } from "../../core/glob.js"; import { ValidationError } from "../../core/errors.js"; import { CRITERION_DIGEST, @@ -579,10 +580,9 @@ export function validateCardCandidate(workspace, candidate, cards, currentId = n } function normalizeScopePath(value) { - return String(value || "") - .replaceAll("\\", "/") - .replace(/^\.\//, "") - .replace(/\/+$/, ""); + return stripTrailingSlashes( + String(value || "").replaceAll("\\", "/").replace(/^\.\//, "") + ); } export function scopesOverlap(left = [], right = []) { diff --git a/packages/workfile/src/modules/changelog/changelog.ts b/packages/workfile/src/modules/changelog/changelog.ts index 018fc42..d474937 100644 --- a/packages/workfile/src/modules/changelog/changelog.ts +++ b/packages/workfile/src/modules/changelog/changelog.ts @@ -63,6 +63,11 @@ export const RELEASE_REQUIRED_KEYS = Object.freeze([ const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; const SEMVER_RE = /^v?\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/; +/** The filename a changelog fragment with this id and title would get today. */ +export function fragmentFileName(id, title) { + return `${id}-${slugify(title)}.md`; +} + function slugify(value, fallback = "change") { return ( String(value) diff --git a/packages/workfile/src/modules/ci/ci.ts b/packages/workfile/src/modules/ci/ci.ts index dfc2208..8cf32b7 100644 --- a/packages/workfile/src/modules/ci/ci.ts +++ b/packages/workfile/src/modules/ci/ci.ts @@ -1,3 +1,4 @@ +import { stripTrailingSlashes } from "../../core/glob.js"; import { chmod, readFile } from "node:fs/promises"; import { resolve } from "node:path"; @@ -96,10 +97,11 @@ function twoJobSplit(indent = "# ") { function githubBody(workspace) { const node = String(workspace.config.ci.nodeVersion || "22"); - const protocolRoot = String(workspace.config.storage.root || ".project") - .replace(/\\/g, "/") - .replace(/^\.\//, "") - .replace(/\/+$/, ""); + const protocolRoot = stripTrailingSlashes( + String(workspace.config.storage.root || ".project") + .replace(/\\/g, "/") + .replace(/^\.\//, "") + ); return `# Generated by @illodev/workfile ${PACKAGE_VERSION} # ${executesRepositoryCode()} diff --git a/packages/workfile/src/modules/docs/docs.ts b/packages/workfile/src/modules/docs/docs.ts index 7e0069c..c55d295 100644 --- a/packages/workfile/src/modules/docs/docs.ts +++ b/packages/workfile/src/modules/docs/docs.ts @@ -55,6 +55,11 @@ export const DOC_REQUIRED_KEYS = Object.freeze([ const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; +/** The filename a managed document with this id and title would get today. */ +export function documentFileName(id, title) { + return `${id}-${slugify(title)}.md`; +} + function slugify(title) { return ( String(title) diff --git a/packages/workfile/src/modules/docs/validation.ts b/packages/workfile/src/modules/docs/validation.ts index a91615e..d0e9c77 100644 --- a/packages/workfile/src/modules/docs/validation.ts +++ b/packages/workfile/src/modules/docs/validation.ts @@ -1,3 +1,4 @@ +import { stripTrailingSlashes } from "../../core/glob.js"; import { stat } from "node:fs/promises"; import { posix, resolve } from "node:path"; @@ -181,7 +182,7 @@ export async function diagnoseDocuments({ // root written `docs/help/` must behave as `docs/help`, and the prefix test // below is a string test. const routeRoots = (workspace.config.docs.routeRoots || []) - .map((entry) => String(entry).replace(/^\.\//, "").replace(/\/+$/, "")) + .map((entry) => stripTrailingSlashes(String(entry).replace(/^\.\//, ""))) .filter(Boolean); const ids = new Map(); for (const document of documents) { diff --git a/packages/workfile/src/modules/health/doctor.ts b/packages/workfile/src/modules/health/doctor.ts index e6b7746..ceeb8cb 100644 --- a/packages/workfile/src/modules/health/doctor.ts +++ b/packages/workfile/src/modules/health/doctor.ts @@ -6,6 +6,7 @@ import { checkCiTemplates } from "../ci/index.js"; import { createIntegrationRegistry } from "../integrations/registry.js"; import { buildProjectIndex } from "../records/public.js"; import { classifyDuplicates, duplicateIssueMessage } from "./duplicates.js"; +import { staleFilenameIssue, staleFilenames } from "./filenames.js"; import { exists } from "../../core/fs-utils.js"; import { lockIsStale } from "../../core/locks.js"; import { readdir } from "node:fs/promises"; @@ -65,9 +66,21 @@ export async function runDoctor(workspace, options: any = {}) { now: options.now || new Date(), diagnose: true })); - const reports = []; + /** + * Each reporter's findings, tagged with who produced them. + * + * The module is named here because here is the only place that knows: the + * core reporters return `{ counts, ok, issues }` and no module — only the + * integration registry returns one, which is what T-0218 assumed of all of + * them. Tagged rather than mutated, so the shared report objects hanging off + * the index are left as they are for the routes that also serve them. + */ + const reports: Array<{ module: string; issues: any[] }> = []; + const from = (module: string, report: { issues?: any[] }) => + reports.push({ module, issues: report.issues || [] }); if (workspace.config.cards.enabled) { - reports.push( + from( + "cards", await diagnoseCards({ cards: index.records.filter((record) => record.kind === "card"), unreadable: index.unreadable.cards, @@ -88,14 +101,14 @@ export async function runDoctor(workspace, options: any = {}) { }) ); } - if (workspace.config.docs.enabled) reports.push(index.reports.docs); - if (workspace.config.changelog.enabled) reports.push(index.reports.changelog); - if (workspace.config.memory.enabled) reports.push(index.reports.memory); + if (workspace.config.docs.enabled) from("docs", index.reports.docs); + if (workspace.config.changelog.enabled) from("changelog", index.reports.changelog); + if (workspace.config.memory.enabled) from("memory", index.reports.memory); if (workspace.config.agents.enabled) { - reports.push(await checkAgentInstructions(workspace)); + from("agents", await checkAgentInstructions(workspace)); } if (workspace.config.ci.enabled && workspace.config.ci.targets.length) { - reports.push(await checkCiTemplates(workspace)); + from("ci", await checkCiTemplates(workspace)); } const integrationRegistry = options.integrationRegistry || @@ -108,10 +121,34 @@ export async function runDoctor(workspace, options: any = {}) { // This is the only layer that holds all of them, so it answers for // duplicate identity — once, rather than leaving a second line standing // beside it that names nothing to run. + // Every kind's stale filenames, answered here for the same reason duplicate + // identity is: a module sees one kind, and this question is about all of them + // (T-0223). Attributed to the module that owns each record so the field + // T-0218 added stays honest. + for (const entry of staleFilenames(index.records)) { + reports.push({ module: entry.module, issues: [staleFilenameIssue(entry)] }); + } const duplicates = classifyDuplicates(index); const claimed = new Set(duplicates.map((duplicate) => duplicate.id)); + // The module rides along with each issue, which it did not: every reporter + // returns `{ module, issues }` and this flatten threw the module away, so + // what reached the reader was a flat list where nothing said where a finding + // came from. Mostly invisible, because a core `code` implies its module to + // anyone who knows the codebase — and not invisible at all for integrations, + // which are the one source that is not ours. A well-formed diagnostic + // returned by a repository's own `healthCheck` read exactly like one Workfile + // produced (T-0218). + // + // A field, and `code` deliberately untouched. Namespacing the code would be + // clearer and would change `issueIdentity`, which is what a baseline is + // matched by — so every baseline accepted with `--accept-baseline` would go + // stale at once, for a cosmetic gain. const issues = reports - .flatMap((report) => report.issues) + .flatMap((report) => + report.issues.map((issue) => + issue.module ? issue : { ...issue, module: report.module } + ) + ) .filter( (issue) => issue.code !== "duplicate-record-id" || @@ -125,6 +162,7 @@ export async function runDoctor(workspace, options: any = {}) { ) { issues.push({ severity: "warning", + module: "doctor", code: "search-provider-unresolved", message: `search.provider is "${workspace.config.search.provider}", but no declared integration with that id offers semantic search. Search runs lexical-only.`, details: { @@ -152,6 +190,7 @@ export async function runDoctor(workspace, options: any = {}) { if (workspace.config.cards.enabled && orphanedPolicy.length) { issues.push({ severity: "warning", + module: "doctor", code: "verification-policy-area-unknown", message: `cards.verification.methods names ${orphanedPolicy.join(", ")}, ` + @@ -163,6 +202,7 @@ export async function runDoctor(workspace, options: any = {}) { for (const duplicate of duplicates) { issues.push({ severity: "error", + module: "doctor", code: "duplicate-record-id", id: duplicate.id, // Code-unit smallest, so the issue keeps one identity across @@ -180,6 +220,7 @@ export async function runDoctor(workspace, options: any = {}) { for (const stale of await findStaleLocks(workspace)) { issues.push({ severity: "warning", + module: "doctor", code: "stale-write-lock", id: stale.owner?.metadata?.recordId, file: stale.file, @@ -200,6 +241,7 @@ export async function runDoctor(workspace, options: any = {}) { if ((await exists(legacyCards)) && !(await exists(migrationState))) { issues.push({ severity: "info", + module: "doctor", code: "legacy-planning-not-migrated", file: ".planning/backlog/tasks", message: diff --git a/packages/workfile/src/modules/health/filenames.ts b/packages/workfile/src/modules/health/filenames.ts new file mode 100644 index 0000000..fb5865f --- /dev/null +++ b/packages/workfile/src/modules/health/filenames.ts @@ -0,0 +1,134 @@ +/** + * Whether a record's filename still describes the record, for every kind. + * + * `diagnoseCards` had this rule and nothing else did. Memory records, managed + * documents and changelog fragments all derive their filenames from their titles + * the same way, so retitling one through `memory patch` left a file named after a + * title the record no longer has and nothing reported it — found by doing it: + * LRN-0033 was retitled and sat under its old name with `doctor` reporting 0 + * errors and 0 warnings (T-0223). + * + * Written here, in the layer that holds every kind at once, for the same reason + * duplicate identity is answered here: a per-module rule is four copies of one + * sentence, and the module that owns a kind cannot see the others. The card rule + * moved out of `diagnoseCards` rather than being left beside this one. + * + * ## What is deliberately out of scope, and why + * + * **An indexed document.** Its filename is somebody's `README.md`, outside the + * protocol directory and read-only through the protocol by definition. Renaming + * it would be this tool editing a repository's own tree to match a title it does + * not own. + * + * **A released changelog fragment.** The protocol already refuses to retitle one: + * `changelog patch` answers `CHANGE_FRAGMENT_RELEASED` and tells the caller to + * write a new fragment instead. So this exclusion is not the primary guard — it + * covers the fragment whose title was edited by hand, or edited before the + * release moved it, where reporting drift would ask a reader to churn a published + * release directory to fix a slug. + * + * **A release.** Its filename comes from the version, not the title, so the + * comparison this rule makes does not apply to it at all. + * + * **A record renamed by hand to something legitimate.** There is no way to tell + * that from drift, and nothing here tries: the rule reports what the title would + * produce today and `doctor --fix` renames only when asked. A project that keeps + * a deliberate name gets one warning per record and can accept it into the + * baseline, which is what the baseline is for. + */ + +import { fragmentFileName } from "../changelog/changelog.js"; +import { cardFileName } from "../cards/slug.js"; +import { documentFileName } from "../docs/docs.js"; +import { memoryFileName } from "../memory/memory.js"; + +/** The module each kind's findings are attributed to (T-0218). */ +const MODULE_FOR_KIND: Record = { + card: "cards", + memory: "memory", + doc: "docs", + change: "changelog" +}; + +/** + * What this record's file would be called if it were created now, or `null` when + * the rule does not apply to it. + * + * The four derivations differ in their length cap — 50 for a card, 60 for a + * document, 70 for the other two — and that is load-bearing rather than + * historical accident to be tidied: unifying them would rename every existing + * record whose title crosses the new bound, in one sweep, on the next `--fix`. + */ +export function expectedRecordFileName(record): string | null { + if (!record?.id || !record?.title) return null; + switch (record.kind) { + case "card": + return cardFileName(record.id, record.title); + case "memory": + return memoryFileName(record.id, record.title); + case "doc": + return record.managed ? documentFileName(record.id, record.title) : null; + case "change": + // Unreleased only. `released` is the flag the record carries; the + // path check is the belt to its braces, because a fragment moved + // into a release directory by hand is still published history. + return record.released || !/\/unreleased\//.test(String(record.path || "")) + ? null + : fragmentFileName(record.id, record.title); + default: + return null; + } +} + +/** The basename of a repository-relative path, without importing `node:path`. */ +function basenameOf(path: string): string { + const normalized = String(path || "").replace(/\\/g, "/"); + return normalized.slice(normalized.lastIndexOf("/") + 1); +} + +export interface StaleFilename { + record: any; + module: string; + current: string; + expected: string; +} + +/** + * Every record whose filename has drifted from its title. + * + * A file whose name does not even start with its id is skipped: that is a + * different fault with a different repair — `filename-mismatch` for a card, and + * renumbering rather than renaming fixes it. + */ +export function staleFilenames(records: any[]): StaleFilename[] { + const stale: StaleFilename[] = []; + for (const record of records || []) { + const expected = expectedRecordFileName(record); + if (!expected) continue; + const current = basenameOf(record.path); + if (!current || current === expected) continue; + if (!current.startsWith(`${record.id}-`)) continue; + stale.push({ + record, + module: MODULE_FOR_KIND[record.kind] || "doctor", + current, + expected + }); + } + return stale; +} + +/** The diagnostic, worded once so all four kinds read alike. */ +export function staleFilenameIssue(entry: StaleFilename) { + return { + severity: "warning" as const, + module: entry.module, + code: "filename-stale", + id: entry.record.id, + file: entry.record.path, + message: + "Filename no longer matches the title; `doctor --fix` renames it to " + + entry.expected, + details: { current: entry.current, expected: entry.expected } + }; +} diff --git a/packages/workfile/src/modules/health/renumber.ts b/packages/workfile/src/modules/health/renumber.ts index 36032f5..baaf1e2 100644 --- a/packages/workfile/src/modules/health/renumber.ts +++ b/packages/workfile/src/modules/health/renumber.ts @@ -23,6 +23,7 @@ import { DOC_LIST_KEYS } from "../docs/index.js"; import { MEMORY_LIST_KEYS } from "../memory/index.js"; import { buildProjectIndex } from "../records/public.js"; import { classifyDuplicates } from "./duplicates.js"; +import { staleFilenames } from "./filenames.js"; function escapeRegExp(value) { return String(value).replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); @@ -517,51 +518,85 @@ export async function healDuplicateCardIds(workspace, options: any = {}) { * the same slug, and picking a winner would rename a file the caller never * asked about. */ -export async function reslugStaleCardFiles( +/** + * Renames every record whose filename no longer matches its title. + * + * The card-only version is below and delegates to this. Driven off the index + * rather than off four loaders, because the index already holds every kind with + * the one thing this needs: the repository-relative path, whose directory is + * where the file goes and whose basename is what it is called. That is what + * makes an archived card, a memory collection and an unreleased fragment the + * same case here. + * + * Nothing rewrites references. Records are linked by id, and the id half of a + * filename does not move — only the slug does. + * + * The activity line is appended for cards alone, because cards are the only kind + * that carries a trail. A rename with no trail entry is not silent: it is a + * `git mv` in a diff, which for the other three kinds is the whole record of it. + * + * Which kinds are in scope, and why the others are not, is stated once in + * `filenames.ts` — the same function that decides what to report. + */ +export async function reslugStaleRecordFiles( workspace, - { actor = null, now }: any = {} + { actor = null, now, kinds = null }: any = {} ) { ensureWritable(workspace); - const loaded = await loadCards(workspace); + const index = await buildProjectIndex(workspace); + const wanted = kinds ? new Set(kinds) : null; const moves: Array<{ id: string; from: string; to: string }> = []; const skipped: Array<{ id: string; file: string; reason: string }> = []; - const taken = new Set(loaded.cards.map((card) => card.file)); - for (const card of loaded.cards) { - if (!card.id || !card.title) continue; - if (!card.file?.startsWith(`${card.id}-`)) continue; - const target = cardFileName(card.id, card.title); - if (target === card.file) continue; - if (taken.has(target)) { - skipped.push({ id: card.id, file: card.file, reason: "name-taken" }); + // Every path the workspace already holds, so a rename cannot land on one. + // Read once and kept current as moves happen, which is what makes two records + // wanting the same slug a skip rather than a lost file. + const taken = new Set( + index.records.map((record) => normalizeRepoPath(record.path || "")) + ); + for (const entry of staleFilenames(index.records)) { + const record = entry.record; + if (wanted && !wanted.has(record.kind)) continue; + const from = normalizeRepoPath(record.path); + const directory = dirname(from); + const to = `${directory}/${entry.expected}`; + if (taken.has(to)) { + skipped.push({ id: record.id, file: entry.current, reason: "name-taken" }); continue; } - const directory = card.archived - ? workspace.paths.cardArchive - : workspace.paths.cards; - const content = await readFile(join(directory, card.file), "utf8"); - const trailed = - workspace.config.cards.activityTrail !== false + const absoluteFrom = join(workspace.root, from); + const content = await readFile(absoluteFrom, "utf8"); + const written = + record.kind === "card" && workspace.config.cards.activityTrail !== false ? appendActivityLine( content, - activityEntry(actor, `renamed file to ${target}`, now) + activityEntry(actor, `renamed file to ${entry.expected}`, now) ) : content; try { - await createFileExclusive(join(directory, target), trailed); + await createFileExclusive(join(workspace.root, to), written); } catch (error: any) { - // `taken` was read before the loop started, so the name can be - // claimed underneath us — by another process, or by an earlier - // move in this very pass. The contract above is to skip a - // collision, and it applies whichever way the collision is - // reported: this used to escape as an internal error instead. + // `taken` was read before the loop, so a name can be claimed + // underneath us — by another process, or by an earlier move in this + // very pass. Skipping a collision is the contract whichever way the + // collision arrives. if (!isCreateContention(error)) throw error; - skipped.push({ id: card.id, file: card.file, reason: "name-taken" }); + skipped.push({ id: record.id, file: entry.current, reason: "name-taken" }); continue; } - await rm(join(directory, card.file), { force: true }); - taken.delete(card.file); - taken.add(target); - moves.push({ id: card.id, from: card.file, to: target }); + await rm(absoluteFrom, { force: true }); + taken.delete(from); + taken.add(to); + moves.push({ id: record.id, from: entry.current, to: entry.expected }); } return { moves, skipped }; } + +export async function reslugStaleCardFiles( + workspace, + { actor = null, now }: any = {} +) { + // Kept as the name the CLI and the exported surface already use. The rule and + // the repair are one implementation now — leaving a card-only copy beside it + // is how the other three kinds came to have no rule at all. + return reslugStaleRecordFiles(workspace, { actor, now, kinds: ["card"] }); +} diff --git a/packages/workfile/src/modules/init/initializer.ts b/packages/workfile/src/modules/init/initializer.ts index 40a887e..c22b011 100644 --- a/packages/workfile/src/modules/init/initializer.ts +++ b/packages/workfile/src/modules/init/initializer.ts @@ -1,3 +1,4 @@ +import { stripTrailingSlashes } from "../../core/glob.js"; import { readFile } from "node:fs/promises"; import { mkdir, readdir } from "node:fs/promises"; import { basename, dirname, join, resolve } from "node:path"; @@ -297,7 +298,7 @@ export async function planInitialization(rootInput, options: any = {}) { // that moved it would otherwise commit its persisted index. const gitignoreAfter = addGitignoreEntry( gitignoreBefore, - `${config.storage.cache.replace(/\/+$/, "")}/` + `${stripTrailingSlashes(config.storage.cache)}/` ); actions.push( fileAction( diff --git a/packages/workfile/src/modules/mcp/server.ts b/packages/workfile/src/modules/mcp/server.ts index f8e88f0..2133e45 100644 --- a/packages/workfile/src/modules/mcp/server.ts +++ b/packages/workfile/src/modules/mcp/server.ts @@ -183,7 +183,17 @@ function toolResult(value, maximumBytes, modern = false) { } }; } - if (truncated) payload = { ...payload, truncated }; + // `resultTruncated`, not `truncated`. This marker is the transport + // saying what the byte ceiling did; `truncated` is whatever the tool + // itself means by it, and one of them means something already: + // `project_agent_context` returns `truncated: boolean` for relations + // dropped to respect `limit`, and this write landed on top of it. A + // caller checking `=== true` then got an object, which is truthy, so the + // check survived by accident; a caller reading `truncated.records` on any + // other tool got `true` from that one and read `.records` off a boolean. + // Two different facts — the limit dropped relations, the ceiling dropped + // rows — and indistinguishable once merged (T-0147). + if (truncated) payload = { ...payload, resultTruncated: truncated }; } const bytes = measure(payload); diff --git a/packages/workfile/src/modules/mcp/tools.ts b/packages/workfile/src/modules/mcp/tools.ts index 02efbdf..30801d8 100644 --- a/packages/workfile/src/modules/mcp/tools.ts +++ b/packages/workfile/src/modules/mcp/tools.ts @@ -161,15 +161,33 @@ function schema(properties, required: string[] = []) { /** * A declared reply shape. * - * Never a closed object: `toolResult` appends `truncated` when a payload + * Never a closed object: `toolResult` appends `resultTruncated` when a payload * exceeds `mcp.maxToolResultBytes`, and a closed schema would make its own - * degradation path invalid. + * degradation path invalid. Declared rather than merely allowed, so a caller + * reads it from the schema instead of discovering it when a payload gets large — + * and so it cannot be confused with a `truncated` a tool declares for its own + * reasons, which is what T-0147 was about. */ +const RESULT_TRUNCATED = { + type: "object", + additionalProperties: true, + description: + "Present only when the reply exceeded mcp.maxToolResultBytes and the " + + "server degraded it rather than failing. `records` counts the rows " + + "dropped; `bodyBytes` gives the original size of a body that was clipped. " + + "This is the transport speaking, never the tool: a `truncated` field " + + "beside it means whatever that tool documents.", + properties: { + records: { type: "integer" }, + bodyBytes: { type: "integer" } + } +}; + function output(properties, required: string[] = []) { return { type: "object", additionalProperties: true, - properties, + properties: { ...properties, resultTruncated: RESULT_TRUNCATED }, ...(required.length ? { required } : {}) }; } diff --git a/packages/workfile/src/modules/memory/memory.ts b/packages/workfile/src/modules/memory/memory.ts index 669b041..e24e023 100644 --- a/packages/workfile/src/modules/memory/memory.ts +++ b/packages/workfile/src/modules/memory/memory.ts @@ -50,6 +50,19 @@ const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; const CONFIDENCE = new Set(["low", "medium", "high"]); const SEVERITIES = new Set(["critical", "high", "medium", "low"]); +/** + * The filename a memory record with this id and title would be created with + * today. Exported so the stale-filename rule can be written once, in the layer + * that holds every kind — see `health/filenames.ts`. + * + * The 70-character cap is not shared with the other kinds and must not be: cards + * cap at 50 and documents at 60, and unifying them would rename every existing + * record whose title is long enough to cross the new bound. + */ +export function memoryFileName(id, title) { + return `${id}-${slugify(title)}.md`; +} + function slugify(value) { return ( String(value) diff --git a/packages/workfile/src/runtime/claude/hooks.mjs b/packages/workfile/src/runtime/claude/hooks.mjs index 3700059..03c4954 100644 --- a/packages/workfile/src/runtime/claude/hooks.mjs +++ b/packages/workfile/src/runtime/claude/hooks.mjs @@ -83,6 +83,9 @@ async function buildBoard(root) { } catch { return { claims: [], builtAt: new Date().toISOString() }; } + // Read once for the whole sweep. This runs at session start, not on the hot + // `PreToolUse` path, so the cost is paid where there is room for it. + const sessions = await readSessions(root); const claims = []; for (const name of names) { if (!name.endsWith(".md")) continue; @@ -96,6 +99,18 @@ async function buildBoard(root) { status: fields.status, claimedBy: fields.claimed_by, claimedAt: fields.claimed_at, + // The same two steps `claimBoardEntry` takes, over the same files: a + // session that names this card beats one that merely shares an actor, + // because two agents can share an actor. Falls back to the tail the + // actor carries, and to `null` when there is none — which the guard + // reads as unproven rather than as one process. + session: + discriminatorOf( + ( + sessions.find((entry) => entry.cardId === fields.id) || + sessions.find((entry) => entry.actor === fields.claimed_by) + )?.sessionId + ) || discriminatorOf(/#([A-Za-z0-9]+)$/.exec(fields.claimed_by)?.[1]), scope: Array.isArray(fields.scope) ? fields.scope : fields.scope @@ -106,9 +121,23 @@ async function buildBoard(root) { return { claims, builtAt: new Date().toISOString() }; } +/** + * Trailing separators removed without a regex, mirroring + * `stripTrailingSlashes` in `core/glob.ts` — which this file cannot import, see + * the header. `replace(/\/+$/, "")` retries the anchored `+` from every start + * position, so N slashes cost O(N²); CodeQL flags the package's copy of that + * spelling and is right to. The scope here comes off a card, and a card in a + * repository taking pull requests can arrive from a fork. + */ +const withoutTrailingSlashes = (value) => { + let end = value.length; + while (end > 0 && value[end - 1] === "/") end -= 1; + return end === value.length ? value : value.slice(0, end); +}; + function scopeCovers(scope, repoPath) { return scope.some((entry) => { - const normalized = entry.replace(/\/+$/, ""); + const normalized = withoutTrailingSlashes(entry); if (!normalized) return false; if (normalized.includes("*")) { const pattern = new RegExp( @@ -146,6 +175,17 @@ const sessionId = (input) => * session UUID. They never matched, so it asked about every claim including * your own, which is how a guard rail teaches people to turn it off. */ +/** + * `sessionDiscriminator` from `core/actor.ts`, duplicated for the reason the + * header gives: this file imports nothing from the package. Pinned against it by + * `test/claude-surface.test.ts`, because a board written by the CLI and read by + * this hook has to agree on what a session id normalises to. + */ +const discriminatorOf = (value) => { + const cleaned = String(value || "").replace(/[^A-Za-z0-9]/g, ""); + return cleaned ? cleaned.slice(0, 8).toLowerCase() : null; +}; + const actorFor = (input) => { const configured = (process.env.WORKFILE_ACTOR || "").trim(); if (configured) return configured; @@ -190,8 +230,18 @@ const actorFor = (input) => { * exists only in a session file — `claimed_by` written from an explicit * `--actor` carries no tail — and the snapshot can. That residual is LRN-0030. */ -function separatesFromMe(claimedBy, mine) { - return claimedBy !== mine; +function separatesFromMe(claim, mine, mySession) { + const theirs = claim.session || null; + // Two sessions, seen. The strongest answer, and the one the board could not + // give before T-0219 put `session` on the entry. + if (theirs && mySession) return theirs !== mySession; + // One side has a session and the other does not, so they are not the same + // process — the same call `claimSeparation` makes. + if (theirs || mySession) return true; + // Neither has one. Different actors are two people; the same actor is + // `unproven`, and the guard stays quiet on a guess rather than interrupting + // somebody about their own card. + return claim.claimedBy !== mine; } const SESSIONS = `${CACHE}/sessions`; @@ -243,6 +293,31 @@ async function signal(root, input, files = []) { * `events.jsonl` grew to 54 KB in this repository with no reader and no * pruning, which is the same mistake one file down. */ +/** + * Every session file this workspace holds. + * + * Mirrors `readAgentSessions` minus the liveness arithmetic, which `buildBoard` + * does not need: it is resolving which session a claim belongs to, not whether + * that session is still breathing. A half-written file is skipped rather than + * failing the sweep, the same rule the package side takes. + */ +async function readSessions(root) { + const directory = join(root, SESSIONS); + let names; + try { + names = await readdir(directory); + } catch { + return []; + } + const sessions = []; + for (const name of names) { + if (!name.endsWith(".json")) continue; + const session = await readJson(join(directory, name), null); + if (session?.sessionId) sessions.push(session); + } + return sessions; +} + async function pruneSessions(root, olderThanMs = 86_400_000) { const directory = join(root, SESSIONS); let names; @@ -264,10 +339,16 @@ async function pruneSessions(root, olderThanMs = 86_400_000) { async function sessionStart(input) { const root = projectDir(input); - const board = await buildBoard(root); await mkdir(join(root, CACHE), { recursive: true }); await pruneSessions(root); + // This session's own signal is written *before* the board is built, and the + // order is load-bearing now that an entry carries a session (T-0219). Built + // first, a claim this very session already holds resolved to no session — its + // file did not exist yet — and the guard then saw a claim with none against a + // caller with one, called them two processes, and asked the session about its + // own card. Which is precisely the failure the guard exists not to have. await signal(root, input); + const board = await buildBoard(root); await writeFile( join(root, CACHE, "board.json"), `${JSON.stringify(board)}\n` @@ -392,10 +473,15 @@ async function preToolUse(input) { const board = await readBoard(root); const mine = actorFor(input); + // Read from the payload, not from the board: this is who *this* process is, + // and no file is opened for it. Which is the whole reason the other side's + // session is resolved when the board is written rather than here — a + // `PreToolUse` fires before every matching tool call, p95 under 30 ms. + const mySession = discriminatorOf(sessionId(input)); const conflict = board.claims.find( (claim) => claim.status === "doing" && - separatesFromMe(claim.claimedBy, mine) && + separatesFromMe(claim, mine, mySession) && claim.scope.length && scopeCovers(claim.scope, repoPath) ); diff --git a/packages/workfile/src/workspace/discover.ts b/packages/workfile/src/workspace/discover.ts index 866df16..4dc6321 100644 --- a/packages/workfile/src/workspace/discover.ts +++ b/packages/workfile/src/workspace/discover.ts @@ -15,9 +15,27 @@ export async function discoverWorkspaceRoot(cwd = process.cwd()) { let current = resolve(cwd); const root = parse(current).root; while (true) { - if (await exists(join(current, "project.config.mjs"))) return current; - if (await exists(join(current, ".project", "VERSION"))) return current; + if (await isWorkspaceRoot(current)) return current; if (current === root) return null; current = dirname(current); } } + +/** + * Whether this exact directory is a workspace, without walking anywhere. + * + * The same two markers the walk above looks for, extracted so the rule is + * written once. `--root` needed it: `loadWorkspace({ root })` took the directory + * as given and checked nothing, so `doctor --root packages/workfile` reported + * six missing-instruction issues, exited 0, and indexed that package's `docs/` + * as the workspace's documents — a clean, empty, believable answer from a + * directory that is not a workspace at all (T-0160). + * + * Deliberately not a walk. `--root` is an assertion by the caller, and quietly + * resolving it to a parent would be a second surprise rather than a fix. + */ +export async function isWorkspaceRoot(directory: string) { + const current = resolve(directory); + if (await exists(join(current, "project.config.mjs"))) return true; + return exists(join(current, ".project", "VERSION")); +} diff --git a/packages/workfile/src/workspace/load-workspace.ts b/packages/workfile/src/workspace/load-workspace.ts index ccde950..1006544 100644 --- a/packages/workfile/src/workspace/load-workspace.ts +++ b/packages/workfile/src/workspace/load-workspace.ts @@ -15,7 +15,7 @@ import { SCHEMA_VERSION } from "../config/defaults.js"; import { verifyTimeoutSeconds } from "../modules/cards/validation.js"; -import { discoverWorkspaceRoot } from "./discover.js"; +import { discoverWorkspaceRoot, isWorkspaceRoot } from "./discover.js"; import type { EffectiveProjectSchema, ProjectConfig, @@ -248,9 +248,8 @@ export async function loadWorkspace( options: LoadWorkspaceOptions = {} ): Promise { const cwd = resolve(options.cwd || process.cwd()); - const discovered = options.root - ? resolve(options.root) - : await discoverWorkspaceRoot(cwd); + const explicit = options.root ? resolve(options.root) : null; + const discovered = explicit ?? (await discoverWorkspaceRoot(cwd)); if (!discovered && !options.allowMissing) { throw new ConfigError( "WORKSPACE_NOT_FOUND", @@ -258,6 +257,24 @@ export async function loadWorkspace( { cwd } ); } + // An explicit root gets the same marker check the walk performs, which it + // never had: it was taken as given, so a mistyped or stale `--root` inside a + // monorepo — one directory too deep is the ordinary case — answered from an + // empty workspace and reported nothing wrong. `allowMissing` is the way + // through, and it is what `--allow-new` already means: accept a directory + // that is not yet a workspace. `init` is its one caller. + // + // Before anything is read or written, so a directory that fails this gets no + // cache, no lock and no index. + if (explicit && !options.allowMissing && !(await isWorkspaceRoot(explicit))) { + throw new ConfigError( + "WORKSPACE_NOT_FOUND", + `${explicit} is not a workspace: it has no project.config.mjs and no ` + + ".project/VERSION. Run `workfile init --root ` to create one, " + + "or pass --allow-new to accept a directory that is not one yet.", + { root: explicit } + ); + } const root = discovered || cwd; const configPath = options.configPath ? inside(root, options.configPath, "configPath") diff --git a/packages/workfile/strict-baseline.json b/packages/workfile/strict-baseline.json index c519cf0..7033f5a 100644 --- a/packages/workfile/strict-baseline.json +++ b/packages/workfile/strict-baseline.json @@ -18,7 +18,7 @@ "src/modules/claude/surface.ts": 5, "src/modules/docs/docs.ts": 6, "src/modules/docs/validation.ts": 4, - "src/modules/health/doctor.ts": 19, + "src/modules/health/doctor.ts": 11, "src/modules/init/initializer.ts": 17, "src/modules/integrations/registry.ts": 9, "src/modules/mcp/server.ts": 5, @@ -43,7 +43,7 @@ "test/initializer.test.ts": 1, "test/integrations.test.ts": 16, "test/locks.test.ts": 5, - "test/mcp.test.ts": 27, + "test/mcp.test.ts": 24, "test/memory.test.ts": 1, "test/migration.test.ts": 5, "test/mutations.test.ts": 1, @@ -53,6 +53,6 @@ "test/search.test.ts": 10, "test/server.test.ts": 63, "test/tokens.test.ts": 1, - "test/workspace.test.ts": 4, + "test/workspace.test.ts": 1, "ui/src/query.ts": 7 } diff --git a/packages/workfile/test/claude-surface.test.ts b/packages/workfile/test/claude-surface.test.ts index df64d4b..b4c0aab 100644 --- a/packages/workfile/test/claude-surface.test.ts +++ b/packages/workfile/test/claude-surface.test.ts @@ -1390,6 +1390,14 @@ test("a claim taken after session start is visible to the guard", async () => { fromMutation.claims, "the hook's builder and the core's must produce the same board" ); + // Including the session T-0219 added. `agent-elsewhere` holds no session + // here and `null` is the honest answer for it, so this pins the field's + // presence; the case where a session is actually resolved is the shared + // actor test below, which is where a non-null value can be produced. + assert.ok( + fromMutation.claims.every((claim: any) => "session" in claim), + "the board entry lost its session field" + ); } finally { await rm(root, { recursive: true, force: true }); } @@ -1675,6 +1683,81 @@ const SEPARATION_CASES: ReadonlyArray<{ } ]; +/** + * Two agents handed the same `--actor`, which is the residual ADR-0020 left open. + * + * The board carried `claimedBy` and no session, so the guard could recover a + * session only from the actor's tail — and a `claimed_by` written from an + * explicit `--actor` has no tail. Two agents sharing one saw a string equal to + * their own and the guard stayed silent, which is the collision it exists to + * prevent. LRN-0030 recorded it; T-0219 is putting the session on the entry. + * + * Driven through the real hook rather than over `separatesFromMe`, because what + * broke was the *board*, not the comparison: a rule reading a field nobody wrote + * is right and useless. + */ +test("two agents sharing an explicit actor do not look like one process", async () => { + const root = await mkdtemp(join(tmpdir(), "workfile-shared-actor-")); + const shared = "ci-runner"; + const first = "11111111-1111-4111-8111-111111111111"; + const second = "22222222-2222-4222-8222-222222222222"; + const env = { ...BLANK, USER: "solo", HOSTNAME: "box", WORKFILE_ACTOR: shared }; + const edit = (session: string) => ({ + session_id: session, + tool_name: "Edit", + tool_input: { file_path: join(root, "src/api/billing.ts") } + }); + try { + await cp(fixture, root, { recursive: true }); + const workspace = await loadWorkspace({ root }); + const card = await createCard(workspace, { title: "Contested", area: "api" }); + + // Agent A opens, which is what writes its session file, and takes the + // card with the shared actor — no tail, so the actor carries nothing. + await runHook("session-start", { session_id: first }, root, env); + await claimCard(workspace, card.id, { actor: shared, scope: ["src/api"] }); + assert.equal( + (await loadCards(workspace)).cards.find((entry: any) => entry.id === card.id) + ?.claimed_by, + shared, + "the claim did not record the shared actor" + ); + + // A itself must still be free to work. + const own = await runHook("pre-tool-use", edit(first), root, env); + assert.equal( + own.stdout.trim(), + "", + `the guard asked agent A about its own card: ${own.stdout}` + ); + + // Agent B opens with its own session and the same actor. Its session start + // rebuilds the board, which must keep A's session on the entry rather than + // overwrite it with B's — the entry belongs to the claim, not to whoever + // last rebuilt the file. + await runHook("session-start", { session_id: second }, root, env); + const board = JSON.parse( + await readFile(join(root, ".project/.cache/activity/board.json"), "utf8") + ); + const entry = board.claims.find((claim: any) => claim.id === card.id); + assert.equal( + entry?.session, + "11111111", + `the entry carries the wrong session: ${JSON.stringify(entry)}` + ); + + const contested = await runHook("pre-tool-use", edit(second), root, env); + assert.match( + JSON.parse(contested.stdout || "{}").hookSpecificOutput + ?.permissionDecisionReason ?? "", + new RegExp(card.id), + "two agents sharing an actor were treated as one process" + ); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + test("the scope guard and the activity snapshot apply one separation rule", async () => { for (const scenario of SEPARATION_CASES) { const root = await mkdtemp(join(tmpdir(), "workfile-separation-")); @@ -1700,22 +1783,36 @@ test("the scope guard and the activity snapshot apply one separation rule", asyn scenario.actorEnv || resolveActor({ sessionId: scenario.session, env }).actor; - // What the snapshot would say about these two identities. The guard - // compares a claim against a live process rather than two claims, so - // the second side is this session's identity. - const basis = claimSeparation( - { by: scenario.claimedBy, sessionId: null }, - { by: mine, sessionId: scenario.actorEnv ? null : scenario.session } - ); - const shouldPrompt = Boolean(basis) && basis !== "unproven"; - - // The board the guard reads is written at session start. + // The board the guard reads is written at session start, and since + // T-0219 its entries carry the session resolved for each claim — so + // the model below has to read it from there rather than assume null. + // It did assume null, and the two rules then agreed by coincidence on + // the one case where the board had learned something the model had + // not: a pin that agrees for the wrong reason has stopped pinning. await runHook( "session-start", { session_id: scenario.session }, root, env ); + const entry = JSON.parse( + await readFile( + join(root, ".project/.cache/activity/board.json"), + "utf8" + ) + ).claims.find((claim: any) => claim.id === card.id); + assert.ok(entry, `${scenario.label}: the claim is not on the board`); + + // What the snapshot would say about these two identities. The guard + // compares a claim against a live process rather than two claims, so + // the second side is this session's identity — which always has a + // session, whatever `WORKFILE_ACTOR` says, because it comes off the + // hook payload rather than out of the actor string. + const basis = claimSeparation( + { by: scenario.claimedBy, sessionId: entry.session }, + { by: mine, sessionId: scenario.session } + ); + const shouldPrompt = Boolean(basis) && basis !== "unproven"; const guard = await runHook( "pre-tool-use", { diff --git a/packages/workfile/test/cli.test.ts b/packages/workfile/test/cli.test.ts index 111b23a..967c1aa 100644 --- a/packages/workfile/test/cli.test.ts +++ b/packages/workfile/test/cli.test.ts @@ -1543,6 +1543,19 @@ test("doctor --new gates on what appeared since the accepted baseline", async () ), "the baseline did not record readable fields" ); + // And it records no module, which is what keeps every baseline accepted + // before T-0218 matching. Every issue now carries the reporter that + // produced it, and `issueIdentity` deliberately ignores it: namespacing + // the `code` would have read better and would have made every accepted + // baseline stale at once. The `--new` run below is the other half of + // this — it matches a baseline written from issues, against issues that + // now carry a field the baseline has never heard of. + for (const entry of written.issues) { + assert.ok( + !("module" in entry), + `the baseline recorded a module: ${JSON.stringify(entry)}` + ); + } const quiet = await outcome(["doctor", "--root", root, "--new", "--json"]); assert.equal(quiet.code, 0, "an unchanged repository must pass the gate"); diff --git a/packages/workfile/test/filenames.test.ts b/packages/workfile/test/filenames.test.ts new file mode 100644 index 0000000..28cddfe --- /dev/null +++ b/packages/workfile/test/filenames.test.ts @@ -0,0 +1,141 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + expectedRecordFileName, + staleFilenames +} from "../dist/src/modules/health/filenames.js"; + +/** + * A retitled record keeps a filename describing work it no longer describes, and + * until T-0223 only cards noticed. + * + * Found by doing it: LRN-0033 was retitled through `memory patch` and sat under + * `LRN-0033-a-card-outlives-the-decision-it-was-filed-under-…` with `doctor` + * reporting 0 errors and 0 warnings. The filename is the handle people and agents + * grep by — which is the argument the card rule already made, and it is not an + * argument about cards. + */ + +const record = (fields: Record) => ({ + id: "X-0001", + title: "A title", + kind: "card", + path: ".project/cards/X-0001-a-title.md", + ...fields +}); + +test("the per-kind length caps stay different, because they have to", () => { + // A card slugs to 50 characters, a document to 60, a memory record and a + // fragment to 70. That is load-bearing rather than tidy-up work waiting to + // happen: unifying them would rename every existing record whose title + // crosses the new bound, in one sweep, on the next `--fix`. + const long = + "A title long enough to reach past fifty characters and keep going after that"; + const card = expectedRecordFileName(record({ kind: "card", title: long })); + const memory = expectedRecordFileName(record({ kind: "memory", title: long })); + const doc = expectedRecordFileName( + record({ kind: "doc", managed: true, title: long }) + ); + assert.ok(card && memory && doc); + assert.ok( + card.length < doc.length && doc.length < memory.length, + `the caps collapsed: card ${card.length}, doc ${doc.length}, memory ${memory.length}` + ); +}); + +test("what the rule refuses to have an opinion about", () => { + // An indexed document is somebody's README, outside the protocol directory + // and read-only through the protocol. Renaming it would be this tool editing + // a repository's own tree to match a title it does not own. + assert.equal( + expectedRecordFileName( + record({ kind: "doc", managed: false, path: "README.md" }) + ), + null + ); + + // A released fragment is published history, and the protocol already refuses + // to retitle one — so this covers the fragment edited by hand. + assert.equal( + expectedRecordFileName( + record({ + kind: "change", + released: true, + path: ".project/changelog/releases/0-1-0/fragments/X-0001-a.md" + }) + ), + null + ); + assert.equal( + expectedRecordFileName( + record({ + kind: "change", + released: false, + path: ".project/changelog/releases/0-1-0/fragments/X-0001-a.md" + }) + ), + null, + "a fragment inside a release directory is history whatever its flag says" + ); + assert.ok( + expectedRecordFileName( + record({ + kind: "change", + released: false, + path: ".project/changelog/unreleased/X-0001-a.md" + }) + ), + "an unreleased fragment is in scope" + ); + + // A release is named after its version, so the comparison does not apply. + assert.equal(expectedRecordFileName(record({ kind: "release" })), null); + + // And a record with no title has nothing to derive from. + assert.equal(expectedRecordFileName(record({ title: "" })), null); +}); + +test("a filename that does not start with the id is a different fault", () => { + // `filename-mismatch` is that one, and renumbering rather than renaming is + // its repair. Reporting both would offer two repairs for one file. + assert.deepEqual( + staleFilenames([ + record({ path: ".project/cards/wandered-off.md", title: "Something else" }) + ]), + [] + ); +}); + +test("each finding is attributed to the module that owns the record", () => { + const stale = staleFilenames([ + record({ kind: "card", title: "Renamed", path: ".project/cards/X-0001-old.md" }), + record({ + kind: "memory", + title: "Renamed", + path: ".project/memory/learnings/X-0001-old.md" + }), + record({ + kind: "doc", + managed: true, + title: "Renamed", + path: ".project/docs/X-0001-old.md" + }), + record({ + kind: "change", + released: false, + title: "Renamed", + path: ".project/changelog/unreleased/X-0001-old.md" + }) + ]); + // T-0218's field, so a reader can tell where a finding came from — and the + // one case it exists for is an integration, which is not one of these. + assert.deepEqual( + stale.map((entry) => entry.module), + ["cards", "memory", "docs", "changelog"] + ); + for (const entry of stale) { + assert.equal(entry.current, "X-0001-old.md"); + assert.equal(entry.expected, "X-0001-renamed.md"); + } +}); diff --git a/packages/workfile/test/mcp.test.ts b/packages/workfile/test/mcp.test.ts index c4556b7..ea28628 100644 --- a/packages/workfile/test/mcp.test.ts +++ b/packages/workfile/test/mcp.test.ts @@ -341,6 +341,22 @@ test("the process emitted by mcp config actually answers initialize", async () = // doing?", the question an agent asks first, was not expressible over MCP at // all. The only way to answer it was to read the Markdown directly, which is // the token spend this protocol exists to remove. +/** + * The success half of a JSON-RPC reply, narrowed once. + * + * `handle` returns a union of the error and result shapes, so every `.result` + * read is a strict error. Narrowed by asserting the shape rather than cast: a + * reply that carries an error instead fails here, by name, rather than reading + * `undefined` off it three lines later. + */ +function resultOf(response: any) { + assert.ok( + response && "result" in response, + `expected a result, got ${JSON.stringify(response)}` + ); + return response.result; +} + test("listing tools answer what is in the workspace without a search query", async () => { const root = await mkdtemp(join(tmpdir(), "workfile-listing-")); await cp(fixture, root, { recursive: true }); @@ -351,15 +367,17 @@ test("listing tools answer what is in the workspace without a search query", asy let id = 100; const call = async (name, args = {}) => { - const response = await server.handle( - request(++id, "tools/call", { name, arguments: args }) + const result = resultOf( + await server.handle( + request(++id, "tools/call", { name, arguments: args }) + ) ); assert.equal( - response.result.isError, + result.isError, undefined, - `${name}: ${JSON.stringify(response.result.structuredContent)}` + `${name}: ${JSON.stringify(result.structuredContent)}` ); - return response.result; + return result; }; const all = await call("project_card_list"); @@ -527,3 +545,77 @@ test("a listing says which cards are archived", async () => { await rm(root, { recursive: true, force: true }); } }); + +/** + * The byte ceiling and a tool's own truncation flag used to be one key. + * + * `buildAgentContext` returns `truncated: boolean` — true when relations were + * dropped to respect `limit`. `toolResult`, when a payload exceeds + * `maxToolResultBytes`, did `payload = { ...payload, truncated }` where its own + * `truncated` is `{ records: }`. For every other tool that key is free, + * so the marker landed cleanly; for `project_agent_context` it landed on top of a + * boolean that already meant something else. + * + * The two meanings are genuinely different — "the limit dropped relations" + * against "the byte ceiling dropped rows" — and merged they are + * indistinguishable. A caller checking `truncated === true` got an object, which + * is truthy, so the boolean check survived by accident; a caller reading + * `truncated.records` on any other tool got `true` here and read `.records` off a + * boolean, which is `undefined` (T-0147). + * + * Reproduced by lowering the ceiling rather than by inflating the workspace, + * which the card said it could not do at `limit: 20`. The collision is about the + * ceiling being reached, not about 512 KB in particular, and a configurable + * ceiling is the honest lever: `maxToolResultBytes` is a declared option. + */ +test("the byte ceiling and a tool's own truncation flag are two markers", async () => { + const root = await mkdtemp(join(tmpdir(), "workfile-truncation-")); + await cp(fixture, root, { recursive: true }); + try { + const workspace = await loadWorkspace({ root }); + // Small enough that the bundle cannot fit. The `markdown` field alone is + // most of it, so the records loop keeps nothing and the marker reports + // everything it dropped — which is the branch that writes it at all. + const server = createMcpProtocolServer(workspace, { + version: "0.6.0", + maxToolResultBytes: 900 + }); + await initialize(server); + + const result = resultOf( + await server.handle( + request(900, "tools/call", { + name: "project_agent_context", + arguments: { cardId: "T-0001", limit: 20 } + }) + ) + ); + const payload = result.structuredContent; + assert.equal( + result.isError, + undefined, + `the call failed instead of degrading: ${JSON.stringify(payload)}` + ); + + // The tool's own flag keeps its declared type. It said `boolean` in the + // outputSchema and it has to be one, which is the whole complaint. + assert.equal( + typeof payload.truncated, + "boolean", + `the tool's own flag is ${JSON.stringify(payload.truncated)}` + ); + + // And the transport's marker is its own key, saying what the ceiling did. + assert.equal( + typeof payload.resultTruncated?.records, + "number", + `the byte ceiling left no marker of its own: ${JSON.stringify(payload)}` + ); + assert.ok( + payload.resultTruncated.records > 0, + "the marker claims nothing was dropped" + ); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/packages/workfile/test/workspace.test.ts b/packages/workfile/test/workspace.test.ts index 6da7851..e89d3a1 100644 --- a/packages/workfile/test/workspace.test.ts +++ b/packages/workfile/test/workspace.test.ts @@ -12,6 +12,23 @@ import { tmpdir } from "node:os"; import { fileURLToPath } from "node:url"; import { dirname, join, resolve } from "node:path"; +/** + * A refusal, narrowed once. + * + * `assert.rejects` hands its callback an `unknown`, so every field read off it is + * a strict error — eleven of them in this file before this existed. Narrowed by + * `instanceof` rather than cast: the ratchet's instruction is to fix the null + * handling and not to annotate the binding `any`, and a cast would have hidden a + * rejection that is not an Error at all. + */ +function refusal(error: unknown) { + assert.ok( + error instanceof Error, + `expected an Error, got ${typeof error}: ${String(error)}` + ); + return error as Error & { code?: string; exitCode?: number }; +} + import { containedPath, discoverWorkspaceRoot, @@ -106,9 +123,10 @@ test("a directory with no workspace marker is an error, not a new workspace", as await assert.rejects( () => loadWorkspace({ cwd: empty }), (error) => { - assert.equal(error.code, "WORKSPACE_NOT_FOUND"); - assert.equal(error.exitCode, 2); - assert.match(error.message, /workfile init/); + const refused = refusal(error); + assert.equal(refused.code, "WORKSPACE_NOT_FOUND"); + assert.equal(refused.exitCode, 2); + assert.match(refused.message, /workfile init/); return true; } ); @@ -124,6 +142,67 @@ test("a directory with no workspace marker is an error, not a new workspace", as } }); +/** + * And an explicit `root` gets the same check, which it did not. + * + * The guard above only ever covered the discovery path. `loadWorkspace({ root })` + * took the directory as given and checked nothing, so `doctor --root + * packages/workfile` inside this very monorepo reported six missing-instruction + * issues, exited 0, and indexed that package's `docs/` as the workspace's + * documents. The failure is silent and plausible: a mistyped or stale `--root` + * one directory too deep produces a clean, empty, believable answer, and nothing + * distinguishes it from a board that is genuinely empty (T-0160). + */ +test("an explicit root with no workspace marker is an error too", async () => { + const empty = await mkdtemp(join(tmpdir(), "workfile-explicit-")); + try { + await assert.rejects( + () => loadWorkspace({ root: empty }), + (error) => { + const refused = refusal(error); + assert.equal(refused.code, "WORKSPACE_NOT_FOUND"); + assert.equal(refused.exitCode, 2); + // The directory it refused, and both ways forward: the message + // is the whole value here, since the caller believed it named a + // workspace. + assert.ok( + refused.message.includes(empty), + `the message does not name the directory: ${refused.message}` + ); + assert.match(refused.message, /workfile init/); + assert.match(refused.message, /--allow-new/); + return true; + } + ); + + // Nothing was written on the way out — no cache, no lock, no index. + assert.deepEqual(await readdir(empty), []); + + // `--allow-new` is the way through, and it is what the flag already + // means. It reaches this branch now; it used to reach only the other one. + const permissive = await loadWorkspace({ root: empty, allowMissing: true }); + assert.equal(permissive.root, empty); + + // Strict, not a walk. A root inside a workspace is not the workspace: + // resolving upward would be a second surprise rather than a fix. + const nested = join(empty, "packages", "thing"); + await mkdir(nested, { recursive: true }); + await writeFile(join(empty, "project.config.mjs"), "export default {};\n"); + assert.equal(await discoverWorkspaceRoot(nested), empty); + await assert.rejects( + () => loadWorkspace({ root: nested }), + (error) => { + const refused = refusal(error); + assert.equal(refused.code, "WORKSPACE_NOT_FOUND"); + return true; + }, + "an explicit root resolved upward instead of being refused" + ); + } finally { + await rm(empty, { recursive: true, force: true }); + } +}); + test("the workspace is discovered from a nested working directory", async () => { const root = await mkdtemp(join(tmpdir(), "workfile-nested-")); const nested = join(root, "apps", "api", "src"); diff --git a/packages/workfile/ui/src/types.ts b/packages/workfile/ui/src/types.ts index 385b261..4495d99 100644 --- a/packages/workfile/ui/src/types.ts +++ b/packages/workfile/ui/src/types.ts @@ -174,6 +174,15 @@ export type IssueSeverity = "error" | "warning" | "info"; export interface HealthIssue { severity: IssueSeverity; code: string; + /** + * Which reporter produced it — `cards`, `memory`, `doctor`, or + * `integration:` for one a repository's own `healthCheck` returned. + * + * That last case is the reason the field exists: a well-formed diagnostic + * from a hook is the one source that is not ours, and it used to arrive + * indistinguishable from a finding Workfile made itself (T-0218). + */ + module?: string; id?: string | null; file?: string | null; archived?: boolean; diff --git a/plugins/workfile/runtime/hooks.mjs b/plugins/workfile/runtime/hooks.mjs index 3700059..03c4954 100644 --- a/plugins/workfile/runtime/hooks.mjs +++ b/plugins/workfile/runtime/hooks.mjs @@ -83,6 +83,9 @@ async function buildBoard(root) { } catch { return { claims: [], builtAt: new Date().toISOString() }; } + // Read once for the whole sweep. This runs at session start, not on the hot + // `PreToolUse` path, so the cost is paid where there is room for it. + const sessions = await readSessions(root); const claims = []; for (const name of names) { if (!name.endsWith(".md")) continue; @@ -96,6 +99,18 @@ async function buildBoard(root) { status: fields.status, claimedBy: fields.claimed_by, claimedAt: fields.claimed_at, + // The same two steps `claimBoardEntry` takes, over the same files: a + // session that names this card beats one that merely shares an actor, + // because two agents can share an actor. Falls back to the tail the + // actor carries, and to `null` when there is none — which the guard + // reads as unproven rather than as one process. + session: + discriminatorOf( + ( + sessions.find((entry) => entry.cardId === fields.id) || + sessions.find((entry) => entry.actor === fields.claimed_by) + )?.sessionId + ) || discriminatorOf(/#([A-Za-z0-9]+)$/.exec(fields.claimed_by)?.[1]), scope: Array.isArray(fields.scope) ? fields.scope : fields.scope @@ -106,9 +121,23 @@ async function buildBoard(root) { return { claims, builtAt: new Date().toISOString() }; } +/** + * Trailing separators removed without a regex, mirroring + * `stripTrailingSlashes` in `core/glob.ts` — which this file cannot import, see + * the header. `replace(/\/+$/, "")` retries the anchored `+` from every start + * position, so N slashes cost O(N²); CodeQL flags the package's copy of that + * spelling and is right to. The scope here comes off a card, and a card in a + * repository taking pull requests can arrive from a fork. + */ +const withoutTrailingSlashes = (value) => { + let end = value.length; + while (end > 0 && value[end - 1] === "/") end -= 1; + return end === value.length ? value : value.slice(0, end); +}; + function scopeCovers(scope, repoPath) { return scope.some((entry) => { - const normalized = entry.replace(/\/+$/, ""); + const normalized = withoutTrailingSlashes(entry); if (!normalized) return false; if (normalized.includes("*")) { const pattern = new RegExp( @@ -146,6 +175,17 @@ const sessionId = (input) => * session UUID. They never matched, so it asked about every claim including * your own, which is how a guard rail teaches people to turn it off. */ +/** + * `sessionDiscriminator` from `core/actor.ts`, duplicated for the reason the + * header gives: this file imports nothing from the package. Pinned against it by + * `test/claude-surface.test.ts`, because a board written by the CLI and read by + * this hook has to agree on what a session id normalises to. + */ +const discriminatorOf = (value) => { + const cleaned = String(value || "").replace(/[^A-Za-z0-9]/g, ""); + return cleaned ? cleaned.slice(0, 8).toLowerCase() : null; +}; + const actorFor = (input) => { const configured = (process.env.WORKFILE_ACTOR || "").trim(); if (configured) return configured; @@ -190,8 +230,18 @@ const actorFor = (input) => { * exists only in a session file — `claimed_by` written from an explicit * `--actor` carries no tail — and the snapshot can. That residual is LRN-0030. */ -function separatesFromMe(claimedBy, mine) { - return claimedBy !== mine; +function separatesFromMe(claim, mine, mySession) { + const theirs = claim.session || null; + // Two sessions, seen. The strongest answer, and the one the board could not + // give before T-0219 put `session` on the entry. + if (theirs && mySession) return theirs !== mySession; + // One side has a session and the other does not, so they are not the same + // process — the same call `claimSeparation` makes. + if (theirs || mySession) return true; + // Neither has one. Different actors are two people; the same actor is + // `unproven`, and the guard stays quiet on a guess rather than interrupting + // somebody about their own card. + return claim.claimedBy !== mine; } const SESSIONS = `${CACHE}/sessions`; @@ -243,6 +293,31 @@ async function signal(root, input, files = []) { * `events.jsonl` grew to 54 KB in this repository with no reader and no * pruning, which is the same mistake one file down. */ +/** + * Every session file this workspace holds. + * + * Mirrors `readAgentSessions` minus the liveness arithmetic, which `buildBoard` + * does not need: it is resolving which session a claim belongs to, not whether + * that session is still breathing. A half-written file is skipped rather than + * failing the sweep, the same rule the package side takes. + */ +async function readSessions(root) { + const directory = join(root, SESSIONS); + let names; + try { + names = await readdir(directory); + } catch { + return []; + } + const sessions = []; + for (const name of names) { + if (!name.endsWith(".json")) continue; + const session = await readJson(join(directory, name), null); + if (session?.sessionId) sessions.push(session); + } + return sessions; +} + async function pruneSessions(root, olderThanMs = 86_400_000) { const directory = join(root, SESSIONS); let names; @@ -264,10 +339,16 @@ async function pruneSessions(root, olderThanMs = 86_400_000) { async function sessionStart(input) { const root = projectDir(input); - const board = await buildBoard(root); await mkdir(join(root, CACHE), { recursive: true }); await pruneSessions(root); + // This session's own signal is written *before* the board is built, and the + // order is load-bearing now that an entry carries a session (T-0219). Built + // first, a claim this very session already holds resolved to no session — its + // file did not exist yet — and the guard then saw a claim with none against a + // caller with one, called them two processes, and asked the session about its + // own card. Which is precisely the failure the guard exists not to have. await signal(root, input); + const board = await buildBoard(root); await writeFile( join(root, CACHE, "board.json"), `${JSON.stringify(board)}\n` @@ -392,10 +473,15 @@ async function preToolUse(input) { const board = await readBoard(root); const mine = actorFor(input); + // Read from the payload, not from the board: this is who *this* process is, + // and no file is opened for it. Which is the whole reason the other side's + // session is resolved when the board is written rather than here — a + // `PreToolUse` fires before every matching tool call, p95 under 30 ms. + const mySession = discriminatorOf(sessionId(input)); const conflict = board.claims.find( (claim) => claim.status === "doing" && - separatesFromMe(claim.claimedBy, mine) && + separatesFromMe(claim, mine, mySession) && claim.scope.length && scopeCovers(claim.scope, repoPath) );