[#199] chore: resolve audit findings T1-T10 (tech-debt ledger) by rucka · Pull Request #311 · foomakers/pair

rucka · 2026-07-11T23:34:09Z

Summary

What Changed

Resolves all findings T1–T10 from the 2026-04-17 codebase audit in a single PR, per maintainer directive (overrides the issue's "incremental small PRs" wording). Includes the two non-trivial refactors T3 (LinkProcessor) and T4 (copyPathOps) inline — not split into separate cards.

Closes #199.

Why This Change

#199 is the audit resolution ledger. Clearing the catalogued debt in one pass lets the ledger be closed as the single source of truth, with no dangling audit doc and no card-per-finding pollution of the backlog.

Story Context

Story: #199 — Tech-debt resolution ledger (2026-04-17 audit). Parent epic #209 (Code Quality & Testing foundations). P1, XL.

Findings resolved (T1–T10)

#	Finding	Resolution	Commit
T1	CI/script silent-failure masks	`ci.yml` `\|\| echo` dropped; knowledge-hub `vitest \|\| check:links` → `&&` and dropped `--coverage \|\| true`; content-ops `adm-zip` + `@types/adm-zip` moved to `catalog:` (drift fix)	`5afead2`
T2	`cli.e2e.test.ts` monster file (1507 LOC)	Split into 7 per-command e2e files + shared `cli-e2e-helpers.ts`; full pair-cli suite green (869 tests)	`f671afc`
T3	`LinkProcessor` static-only class	Converted to a module of named function exports; callers, barrels, and tests updated. Public API preserved (content-ops-internal seam)	`e3fdbb6`
T4	`copyPathOps` god-module	Split into `copy-file` / `copy-directory` / `copy-directory-transforms` / `copy-types` + a slim orchestrator. Public API preserved	`4444fbc`
T5	No coverage thresholds	brand: 80% thresholds (currently measures ~85%); website: realistic baseline (statements/branches/functions/lines) + ADL, committed `reports/coverage-baseline.json`	`050dfd3`
T6	Non-null assertions in move ctx	`MoveCtx` made a total type; dropped `ctx.source!`. Discriminated union N/A (branch unknown at ctx-build time) — documented in code	`e1831e5`
T7	`:any` usage	No work — zero real `:any` in `src`; the audit's match was a string literal. Noted for the ledger	—
T8	brand `dev/App.tsx` oversized	Split into `dev/sections/*` + primitives + barrel; `App.tsx` 456 → 51 LOC; `dev/App.test.tsx` green	(in `050dfd3`..range)
T9	`in-memory-fs` test util oversized	Split into state + read/write/seed modules; thin delegating class; private `resolvePath` test access repointed to state	`64aa83d`
T10	No duplication scanner	jscpd v5 wired into the quality gate via `dup:check` + `.jscpd.json` (threshold 5%, current ~1.42%); noted in `tech-stack.md`	`0327ef6`

Key decisions (reviewer: these are the load-bearing calls)

Website coverage is a realistic baseline, not 80%. website is a Playwright-tested Next.js app; a blanket 80% unit-coverage floor would be misleading. Baseline recorded in reports/coverage-baseline.json and rationale in ADL .pair/adoption/decision-log/2026-07-12-website-coverage-baseline.md. brand stays at 80% (its dev harness is unit-tested, measures ~85%).
Refactors are behavior-preserving. T3/T4/T9/T8 and the T2 e2e split preserve public APIs. The safety net is the pre-existing green suites exercised through public seams — I intentionally did not add redundant per-file unit tests for pure internal decompositions. Diff churn is high but semantically a move/rename.
jscpd threshold set to 5% against a current ~1.42% baseline — headroom without ratcheting churn into this PR.

Testing

turbo ts:check (9 pkgs) ✅   turbo lint (5 pkgs) ✅
vitest: content-ops 618 ✅  pair-cli 869 ✅  brand 78 ✅  website 31 ✅  knowledge-hub 56 ✅
check:links ✅   hygiene:check ✅   docs:staleness ✅ (35 skills / 8 cmds)
dup:check 1.42% ✅ (threshold 5%)
brand test:coverage ~85% ✅   website test:coverage baseline ✅

Not run in this gate: Playwright ct/e2e (requires playwright install chromium) — out of scope for these changes.

Reviewer Guide

Diffs are dominated by file splits (moves/renames). Focus review on: (a) the public-API surface of content-ops (src/index.ts, src/markdown/index.ts) is unchanged after T3/T4; (b) the website coverage baseline + ADL is a reasonable call; (c) ci.yml no longer swallows failures.
To test locally:

git checkout chore/#199-tech-debt-ledger
pnpm install
pnpm --filter @pair/content-ops build   # self-imports need build first
pnpm --filter @pair/content-ops exec vitest run
pnpm --filter pair-cli exec vitest run

Follow-up

On merge: tick Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 Task Breakdown T1–T10 + DoD and close Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 as the single source. (T7 = already-fixed note.)

🤖 Generated with Claude Code

- ci.yml: drop `|| echo` on ts:check/build/lint (fail loudly) - knowledge-hub: `vitest || check:links` -> `&&` (both real gates), drop `--coverage || true` - content-ops: adm-zip + @types/adm-zip -> catalog: - Task: T1 (P0.1 + P0.2 + P1.1) Refs: #199

- copy-file.ts: single-file copy path - copy-directory.ts: non-transform directory copy path - copy-directory-transforms.ts: flatten/prefix transform copy path - copy-types.ts: shared CopyPathOpsResult/TransformOpts - copyPathOps.ts: slim orchestrator, re-exports public API (unchanged) - Public API (copyPathOps, copyDirectoryWithTransforms, CopyPathOpsResult) preserved - Task: T4 (P0.5) Refs: #199

- Replace `Partial<...>` MoveCtx + ctx.source!/ctx.normSource! with a total context type built once after setup (all fields required) - Discriminated union not applicable: branch unknown at ctx-build time - Task: T6 (P1.3) Refs: #199

- Convert 13 static methods to module-level named functions; private statics become module-private functions (no behavior change) - extractLinks/detectLinkStyle collapse duplicate standalone wrappers - applyReplacements/processFileWithLinks re-exported from replacement-applier - Update callers: replacement-applier, replacement-generator (aliased), link-rewriter, link-batch-processor; barrels src/index + markdown/index - Update tests to named imports (LinkProcessor was content-ops-internal) - Task: T3 (P0.4) Refs: #199

- in-memory-fs-state.ts: shared mutable state + path primitives - in-memory-fs-seed.ts: constructor seeding - in-memory-fs-read.ts / in-memory-fs-write.ts: operation groups - in-memory-fs.ts: thin InMemoryFileSystemService delegating to the above - Public API unchanged; test's private resolvePath access repointed to state - Task: T9 (P2.3) Refs: #199

- brand: 80% global thresholds (measures 84.5% with tested dev harness) - website: realistic baseline floors (9/9/40/60), not 80% — Playwright-tested Next.js app; documented via ADL 2026-07-12-website-coverage-baseline - both: coverage block extends coverageConfigDefaults.exclude + json-summary - reports/coverage-baseline.json: committed baseline for trend tracking (P1.4) - Task: T5 (P1.2 + P1.4) Refs: #199

- add jscpd v5 (root dev dep) + .jscpd.json (threshold 5%, ts/tsx, src-scoped) - `dup:check` script (jscpd apps packages) appended to quality-gate - current duplication ~1.4% lines / ~2.2% tokens (17 clones) — well under gate - note jscpd in tech-stack.md - Task: T10 (P2.4) Refs: #199

- 11 showcase sections + Header moved to dev/sections/*.tsx - shared helpers (Section/LogoVariant/ColorSwatch/TypoBlock/UtilityCard) in dev/sections/primitives.tsx; barrel dev/sections/index.ts - App.tsx now a slim shell (456 -> 51 LOC); dev/App.test.tsx unchanged, green - brand coverage 85% (still >= 80%) - Task: T8 (P2.2) Refs: #199

- extract shared fixtures to cli-e2e-helpers.ts - cli.e2e.test.ts (1507 LOC) -> 7 focused files: validate-config, install, update, link, errors, packaging, kb-validate - behavior-preserving; full pair-cli suite green (869 tests, 77 files) - Task: T2 (P0.3) Refs: #199

rucka · 2026-07-11T23:40:17Z

Code Review — PR #311

Review Information

PR: [#199] chore: resolve audit findings T1-T10 (tech-debt ledger) #311 — [#199] chore: resolve audit findings T1-T10 (tech-debt ledger)
Reviewer: Independent reviewer (Claude Code)
Review Date: 2026-07-12
Story/Epic: Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 (tech-debt resolution ledger) / Epic Code Quality & Testing foundations #209
Review Type: Refactor + Config (multi-finding batch)
Head reviewed: f671afc (origin/chore/Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199-tech-debt-ledger)

Review Summary

Overall Assessment

Approved with Comments — minor issues noted, can merge

Key Changes

Resolves all 10 catalogued audit findings (T1–T10) in one PR: silent-failure removal (T1), monster-e2e split (T2), LinkProcessor class→module (T3), copyPathOps god-module split (T4), coverage thresholds + baseline (T5), type-safe MoveCtx (T6), :any (T7 = no-op, verified), brand dev/App.tsx split (T8), in-memory-fs split (T9), jscpd gate (T10). Diff is dominated by behavior-preserving moves/renames.

Business Value

Clears the tracked debt so #199 can close as the single source of truth. No functional/product surface changed.

Independent Verification (gates re-run on the pushed head)

Check	Result
content-ops build + `tsc --noEmit`	PASS
content-ops vitest	618/618 PASS
pair-cli `tsc --noEmit`	PASS
pair-cli vitest	869/869 PASS
brand `test:coverage` (80% thresholds)	PASS
website `test:coverage` (baseline thresholds)	PASS
knowledge-hub `test` (now `&& check:links`)	PASS
`dup:check` (jscpd)	1.42% < 5%, exit 0
`hygiene:check`	PASS
`docs:staleness`	PASS (35 skills / 8 cmds)

The behavior-preservation claim for the T3/T4/T6/T9 refactors is corroborated: content-ops public barrels (src/index.ts, src/markdown/index.ts) re-export the new named functions, no stale LinkProcessor / ctx.source! references remain in code, and the full pre-existing suites pass through the public seams. T7 verified: the only : any grep hit is a string literal ('... 1: any check fails ...') — no real :any, matches the PR claim.

Acceptance Criteria

AC1 (each finding resolved or consciously deferred + ticked): met — all T1–T10 resolved; website-80% consciously adjusted with an ADL; T7 a verified no-op.
AC2 (PR references Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199, suggested batching): met on reference; batching deviated (single PR) — see finding M4.
AC3 (refactors may be promoted to own card): satisfied — T3/T4 done inline, which AC3 permits ("may").
AC4 (Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 closed as single source): deferred to post-merge follow-up (correct — cannot close before merge).

Detailed Review Comments

Positive

Refactors are genuinely behavior-preserving and the barrels line up exactly with the new named exports; verified by 618 + 869 green tests.
T6 sensibly overrides the story's "discriminated union" suggestion with a total-type + an in-code rationale (branch unknown at ctx-build time) — correct call, no non-null assertions remain.
Website coverage decision is properly recorded as an ADL with alternatives considered and a committed baseline for trend tracking.

Minor Issues

[package.json:37] — jscpd added as pinned "jscpd": "^5.0.12" while every other root devDependency (husky, turbo, @changesets/cli) uses catalog:. This re-introduces exactly the catalog drift that T1 removes elsewhere in the same PR. Recommendation: add jscpd to the workspace catalog and reference catalog:.
[packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/design-rules.md:44,59,108] — The DR-2 guideline still presents class LinkProcessor (18 static methods) as present-tense "Evidence" and an open migration item. T3 resolved it, so the shipped guideline now cites code that no longer exists. docs:staleness does not catch this (it only checks skill/command sync). Recommendation: mark DR-2 resolved or convert the example to historical framing.
[.github/workflows/ci.yml] — T10's jscpd (and pre-existing hygiene:check / docs:staleness) are wired into the local quality-gate only; CI runs ts:check/build/lint as discrete steps and never invokes quality-gate, so the new duplication gate is not enforced in CI. Not a regression (matches prior hygiene/docs behavior), but T10's "gate" intent is only half-realized. Recommendation: consider running dup:check (or quality-gate) in CI.
[PR-wide] — All T1–T10 landed in a single PR, deviating from the story's "incremental small PRs / suggested batching" wording. The PR cites a maintainer directive that is not visible in the issue or an ADL. Within AC latitude (batching is "suggested", AC3 refactor-promotion is "may", debt "never blocks"), so non-blocking; noting the traceability gap. Recommendation: none required; optionally record the directive.
[reports/coverage-baseline.json] — P1.4 asked to persist coverage-summary.json; the author committed a hand-authored aggregate instead. Satisfies the trend-baseline intent. No action needed.

Risk Assessment

Technical risk: Low. Pure decomposition/config; public APIs preserved; all suites green on the head.
Business risk: Low. No product/runtime surface changed.

Decision

Approved with Comments. No critical or major issues; all AC met (website-80% consciously deferred via ADL); all quality gates independently verified green. The listed Minor items are non-blocking cleanups.

🤖 Independent review generated with Claude Code

…le DR doc) - jscpd -> workspace catalog + catalog: ref in root (kills catalog drift) - CI: add hygiene:check / docs:staleness / dup:check discrete steps - design-rules.md: reframe DR-1/DR-2/DR-3 evidence + migration plan as historical (all originating instances resolved in #199) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

rucka · 2026-07-11T23:46:28Z

Remediation — round 1 (commit `d5c1a83`)

Per finding:

[Minor] jscpd pinned as root devDependency instead of catalog: (package.json:37) → added jscpd: ^5.0.12 to the workspace catalog and changed the root devDependency to "jscpd": "catalog:"; lockfile now resolves it via catalog exactly like husky/turbo — catalog drift removed. pnpm-workspace.yaml, package.json, pnpm-lock.yaml
[Minor] DR-2 KB guideline cites class LinkProcessor as present-tense evidence / open migration item (design-rules.md:44,59,108) → reframed as historical: DR-2 Evidence now states the class was converted to named exports in Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199, the illustrative code block is labelled "not live code", and the Migration Plan row is marked "Resolved in Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199". Applied the same fix to DR-1 and DR-3 for coherence — Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 also resolved their cited code (copyPathOps / cli.e2e.test.ts / dev/App.tsx / in-memory-fs splits; MoveCtx assertions dropped), so leaving them present-tense would reintroduce the identical staleness. The "Migration Plan (existing violations) … seed for assess-* skills are output-only; record-decision is the sole adoption writer (no scan-mode) #224" note is retitled to "originating instances", all marked resolved. design-rules.md
[Minor] jscpd/hygiene/docs:staleness gates only in local quality-gate, not CI (.github/workflows/ci.yml) → added three discrete CI steps after Run lint: Run code-hygiene check (pnpm hygiene:check), Run docs-staleness check (pnpm docs:staleness), Run duplication check (pnpm dup:check). Did not invoke the full quality-gate script in CI on purpose — it runs prettier:fix/mdlint:fix (mutating) and re-runs ts:check/test/lint already covered by existing steps; discrete non-mutating steps match this workflow's per-script convention. .github/workflows/ci.yml

Not changed (escalated): none

Quality gates: PASS — ts:check (9 pkgs, via pre-commit hook), full quality-gate incl. dup:check 1.42% (via pre-push hook), hygiene:check, docs:staleness, knowledge-hub mdlint + check:links, prettier --check on changed config files.

→ Re-review requested.

rucka · 2026-07-11T23:49:20Z

Code Review — PR #311 (Re-review, round 2)

Review Information

PR: [#199] chore: resolve audit findings T1-T10 (tech-debt ledger) #311 — [#199] chore: resolve audit findings T1-T10 (tech-debt ledger)
Author: Gianluca Carucci
Reviewer: Independent reviewer (Claude Code)
Review Date: 2026-07-12
Story/Epic: Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 (tech-debt resolution ledger) / Epic Code Quality & Testing foundations #209
Review Type: Refactor + Config (multi-finding batch) — re-review after remediation round 1
Head reviewed: d5c1a83 (origin/chore/Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199-tech-debt-ledger)

Review Summary

Overall Assessment

Approved with Comments — prior findings resolved; only documented/by-design notes remain

Key Changes Summary

Re-review of the remediation commit d5c1a83 on top of the T1–T10 batch. The remediation touches only ci.yml, package.json, pnpm-workspace.yaml, pnpm-lock.yaml, and design-rules.md (KB dataset) — no source code. The prior independent review verified the source head (f671afc) green (content-ops 618, pair-cli 869, brand/website coverage, dup:check 1.42%); that source is unchanged by this remediation, so the behavior-preservation guarantee carries forward.

Prior Findings — Resolution Verification

#	Prior finding	Status	Evidence
1	jscpd pinned as root devDep instead of `catalog:` (package.json:37)	RESOLVED	`package.json:37` now `"jscpd": "catalog:"`; `pnpm-workspace.yaml:40` adds `jscpd: ^5.0.12`; lockfile resolves via catalog like husky/turbo. Catalog drift removed.
2	DR-2 guideline cites `class LinkProcessor` as present-tense evidence / open migration item	RESOLVED (+ extended)	`design-rules.md` DR-2 code block labelled "Illustrative anti-pattern (not live code)"; Evidence reframed "historical — resolved in #199". Same fix correctly applied to DR-1 and DR-3 (their cited code — copyPathOps / cli.e2e / App.tsx / in-memory-fs / MoveCtx — was also resolved in #199). Migration Plan retitled "originating instances", every row "Resolved in #199". No stale present-tense claims remain.
3	jscpd/hygiene/docs:staleness gates only in local `quality-gate`, not CI	RESOLVED	`ci.yml` adds three discrete steps after "Run lint": `Run code-hygiene check` (`pnpm hygiene:check`), `Run docs-staleness check` (`pnpm docs:staleness`), `Run duplication check` (`pnpm dup:check`). All three scripts exist in `package.json`. jscpd `.jscpd.json` sets `threshold: 5` (non-zero exit on breach), so the CI gate is real. Not invoking full `quality-gate` in CI is a correct call — it runs mutating `prettier:fix`/`mdlint:fix`.

Independent Verification (re-confirmed on `d5c1a83`)

No stale LinkProcessor references remain in apps//packages/ source (only the historical KB example).
No ctx.source!/ctx.target! non-null assertions remain in content-ops/src.
content-ops public barrels (src/index.ts, src/markdown/index.ts) re-export the new named functions; public API surface unchanged.
CI-referenced scripts (hygiene:check, docs:staleness, dup:check) all present.

Detailed Review Comments

Positive

The DR-doc remediation was applied coherently beyond the single flagged rule: DR-1 and DR-3 were reframed too, pre-empting the identical staleness the reviewer would otherwise have flagged next.
jscpd catalog fix is complete across all three files (manifest + workspace + lockfile), not just the manifest.
Remediation is scoped to config/docs — zero source churn keeps the test-verified head intact.

Minor Issues

[reports/coverage-baseline.json / vitest configs] — The T5 coverage thresholds (brand 80%, website baseline floors) are enforced only on explicit test:coverage runs; CI and quality-gate both run turbo test (not test:coverage), so these thresholds do not gate PRs. By-design: the website coverage ADL explicitly records this ("these thresholds bite only on explicit coverage runs / trend tracking — they do not gate normal PRs"), and story T5 required thresholds in the configs + a committed baseline, not CI wiring. Non-blocking; noted for transparency. Recommendation (optional): wire test:coverage into CI when raising the website floor toward target.
[PR-wide] — All T1–T10 landed in one PR, deviating from the story's "incremental small PRs / suggested batching" wording, citing a maintainer directive not visible in the issue or an ADL. Within AC latitude (batching "suggested", AC3 refactor-promotion "may", debt "never blocks"). Non-blocking; optionally record the directive.

Acceptance Criteria

AC1 (each finding resolved or consciously deferred + ticked): met — T1–T10 resolved; website-80% consciously adjusted via ADL; T7 verified no-op.
AC2 (PR references Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199, suggested batching): met on reference; batching deviated (single PR) — non-blocking (see above).
AC3 (refactors may be promoted): satisfied — T3/T4 done inline (AC3 permits "may").
AC4 (Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 closed as single source): correctly deferred to post-merge.

Risk Assessment

Technical risk: Low. Remediation is config/docs only; source head unchanged and previously verified green.
Business risk: Low. No product/runtime surface changed.

Decision

Approved with Comments. All three prior findings genuinely resolved (jscpd catalog, DR-doc staleness reframed + extended to DR-1/DR-3, CI gates wired). Remaining items are documented/by-design and non-blocking.

🤖 Independent re-review generated with Claude Code

…sion CI now runs test:coverage for brand+website after the test step, so their vitest coverage.thresholds actually gate PRs instead of biting only on explicit runs. Amends the 2026-07-12 ADL to record the reversal per explicit user direction.

Story #199's T1-T10 batched into a single PR was flagged by review as a batching deviation; user confirmed this is intentional and should be the default. Adds the convention to way-of-working.md (consulted convention) and an ADL recording the why (historical record).

…review) ADL exemption for *.e2e.test.ts co-location was descriptive, not mechanically verifiable. Add 3 checkable criteria (named after a real flow, additive not sole coverage, genuine cross-module interaction) to the ADL and mirror into file-structure.md (root + dataset copies).

…311 review) Root of src/ isn't the right place for e2e-only test infra. Follows the pair-cli's own existing #test-utils/ precedent (test-helpers.ts, test-setup.ts) instead of inventing a new convention.

…s (PR #311 review) Both were split (#199 T4/T9) into sibling files, but only one entry point is externally consumed in each case (copyPathOps / copyDirectoryWithTransforms; InMemoryFileSystemService). Group each split under its own folder with an index.ts barrel so the directory signals public surface vs internal collaborators: - content-ops/src/ops/copyPathOps.ts + copy-{file,directory, directory-transforms,types}.ts -> ops/copy/ + index.ts - content-ops/src/test-utils/in-memory-fs.ts + in-memory-fs-{state, read,write,seed}.ts -> test-utils/in-memory-fs/ + index.ts Public API unchanged (same import specifiers, now resolving through the folder). package.json's "./test-utils/in-memory-fs" subpath export updated to the new dist path. content-ops (618) + pair-cli (869) tests green.

…idelines (PR #311 review) KB guideline files ship to every adopting project via pair install/update; they must never cite this repo's own paths or issue numbers as "evidence" (meaningless/stale for external consumers). - design-rules.md: remove the 3 "Evidence (historical — #199)" paragraphs (DR-1/2/3) and the whole "Migration Plan" section (also now factually stale re: scan-mode removed by #224/#310); Recognition + Don't + Do already fully define each rule without a citation. Overview's rule enumeration updated accordingly. - Add DR-4 "Split Files Without a Barrel" (companion rule to this PR's ops/copy/ and test-utils/in-memory-fs/ folder-module refactors), generic illustrative snippets only, no repo citation. - shared-config-packages.md: genericize the apps/pair-cli, apps/website, packages/brand code-comment examples to placeholder project names (apps/api, apps/web, packages/ui-lib) so the guideline doesn't imply the reader's project has those directories. Repo-wide grep confirms these were the only two guideline files with this pattern.

…ps to module tests, consolidate to single cli.e2e.test.ts Per-test classification of the 7-file cli.e2e split found only 1/51 tests genuinely e2e (real cross-command state hand-off); the rest were single-module tests wearing e2e clothing. Deletes 35 exact/near duplicates, moves 15 genuine gaps into install/update/kb-validate handler.test.ts and registry/validation.test.ts, keeps the 1 genuine e2e test in a new cli.e2e.test.ts, and removes the now-unused cli-e2e-helpers.ts.

…-entry-point rule The PR #311 checkable-criteria amendment was satisfiable on paper without genuine cross-module flows, as proven by the pair-cli 7-file split it approved (only 1/51 tests were actually e2e). Replace it with the rule confirmed with the user: e2e defaults to one file per entry point; splitting requires the production code itself to be refactored into isolated modules with true unit tests, not just moved test files. Cites the #199 reorg as precedent.

…uideline The strategy-level e2e-testing guideline already said e2e should focus on critical paths, not every scenario, but didn't point to the concrete file-organization consequence (one file per entry point, split only with genuine module isolation) landed by this story's test reorg. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

rucka · 2026-07-12T15:12:27Z

Remediation update — all 11 review comments now reflected in the pushed diff

Pushed d5c1a83..77576e5 (9 commits). All 11 inline review comments above have replies; this comment confirms every fix they reference is now actually live on this PR (previously the replies pointed at local commits not yet pushed).

Summary of what changed since `d5c1a83`

7 e2e-naming comments: superseded my own earlier reply. A per-test classification (51 tests across the 7 cli-*.e2e.test.ts files) found only 1 was genuinely e2e; the other 50 were single-module tests wearing e2e clothing. Resolution (confirmed via a grilling session, not unilateral): 35 exact/near-duplicate tests deleted, 15 genuine coverage gaps moved into the relevant commands/*/handler.test.ts / registry/validation.test.ts, the 7 files consolidated into a single apps/pair-cli/src/cli.e2e.test.ts containing only the 1 real cross-command test. pair-cli: 869→834 tests, 77→71 files. file-structure.md and the 2026-07-08 ADL corrected to state the rule properly (one e2e file per entry point by default; split only if production code is genuinely refactored into isolated modules). Cross-linked from the e2e-testing strategy guideline too.
cli-e2e-helpers.ts placement: moved to apps/pair-cli/src/test-utils/cli-e2e-helpers.ts, matching the test-utils/ convention already used elsewhere in the monorepo.
copyPathOps.ts / in-memory-fs.ts folder-module suggestion: both refactored into folder modules with an index.ts barrel (ops/copy/, test-utils/in-memory-fs/); content-ops's package.json exports map updated accordingly. Added as a new rule, DR-4 in design-rules.md ("Split Files Without a Barrel"), generic/illustrative, no project citations.
design-rules.md project-code references: all 3 "Evidence (historical — resolved in Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199)" citations and the entire "Migration Plan" section removed (KB guidelines ship to every adopting project — citing this repo's own file paths/issue numbers was meaningless and already going stale, e.g. it referenced assess-debt's scan-mode which assess-* skills are output-only; record-decision is the sole adoption writer (no scan-mode) #224/[#224] refactor: assess-* output-only; record-decision sole adoption writer #310 removes). Also genericized incidental project paths found in shared-config-packages.md while auditing for the same pattern.

Additional fixes landed in this push (beyond the 11 comments)

CI coverage gate: test:coverage now wired into ci.yml for brand+website — the 80% thresholds from T5 previously only bit on manual runs, never gated a PR. Reverses the "trend-only" ADL; verified locally that a real threshold violation would now fail CI.
"One PR per story" recorded as the default: written into way-of-working.md (the live convention), with an ADL tracing the rationale — this PR's own T1-T10-in-one-PR shape is the precedent.

Gates (re-verified at the final pushed head, `77576e5`)

pair-cli:        71 files / 834 tests — pass
content-ops:      42 files / 618 tests — pass
knowledge-hub:    56/56 — pass
check:links:      all valid
mdlint:           pass
docs:staleness:   PASS — 35 skills, 8 commands in sync
hygiene:check:    PASS
dup:check:        1.43% (threshold 5%)
ts:check / lint:  pass (pair-cli, content-ops, knowledge-hub)

🤖 Generated with Claude Code

…oom sentinel (PR #311 review) Split copyPathOps.test.ts (5 describes, 1 file) into per-production-file tests: copy-file.test.ts, copy-directory.test.ts, copy-directory-transforms.test.ts; copyPathOps.test.ts keeps only its own dispatch-level concerns (absolute-path validation, source-existence check). Reuses existing createTestFileService helper to simplify repeated fs setup in the transforms file. Also removes the dead err.message.includes('boom') test-sentinel from copy-file.ts and copy-directory.ts — no test references it anymore, so the catch blocks now always wrap via createError.

… review) Split in-memory-fs.test.ts (16 describes, 1 file) into per-production-file tests: in-memory-fs-state.test.ts, in-memory-fs-read.test.ts, in-memory-fs-write.test.ts, in-memory-fs-seed.test.ts. in-memory-fs.test.ts keeps only the class's own constructor wiring, accessSync (no-op), and one end-to-end complex-scenario smoke test. Extracts test-fixtures.ts (createFs/DEFAULT_DIR) to remove the repeated moduleDir/workingDir='/app' boilerplate across the split files.

rucka added 9 commits July 12, 2026 00:59

github-actions Bot temporarily deployed to Website Preview July 11, 2026 23:34 Inactive

github-actions Bot temporarily deployed to Website Preview July 11, 2026 23:46 Inactive

rucka added 2 commits July 12, 2026 11:31