From 5fdf177643854274cb528e45f19629408c45a630 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 26 Jul 2026 01:46:15 +0200 Subject: [PATCH] ci(coverage): ratchet runtime boundaries --- .changes/electron-remote-static-paths.md | 7 ++ docs/architecture/validation-map.md | 25 ++++-- ...5-coverage-integrity-runtime-boundaries.md | 61 ++++++++++---- ...age-integrity-runtime-boundaries-design.md | 79 ++++++++++++++++--- tools/coverage/coverage-policy.json | 40 +++++++++- 5 files changed, 177 insertions(+), 35 deletions(-) create mode 100644 .changes/electron-remote-static-paths.md diff --git a/.changes/electron-remote-static-paths.md b/.changes/electron-remote-static-paths.md new file mode 100644 index 000000000..70313de33 --- /dev/null +++ b/.changes/electron-remote-static-paths.md @@ -0,0 +1,7 @@ +--- +type: fix +area: electron +--- + +The desktop remote-control server now blocks crafted static paths from escaping +bundled web files on Windows. diff --git a/docs/architecture/validation-map.md b/docs/architecture/validation-map.md index 139fea90a..0c839e812 100644 --- a/docs/architecture/validation-map.md +++ b/docs/architecture/validation-map.md @@ -55,11 +55,25 @@ Docs-only changes (Markdown, `docs/`, `.plans/`, `.codex/`, `.claude/`) and PRs no E2E validation runs in CI, which is intentional: they cannot affect app behavior. -| Tier | Rule | Validation | -| --- | --- | --- | -| A | Product/runtime Angular, Electron, backend, data-access, portal, playlist, workspace, playback, EPG, and shared UI code collects source coverage. | `pnpm run coverage:ci` | -| B | Validate behavior without percentage coverage, such as `website`, `packaging`, and Playwright E2E projects. | `pnpm nx test website`, `pnpm nx test packaging`, or the closest E2E target | -| C | Excluded from the source coverage baseline, such as mock servers, test helper libraries, and untested feature shells. | Validate through dependent flows, or add focused tests when changing behavior directly | +Tier A coverage is fail-closed. `coverage:unit:ci` relays Jest output but exits +nonzero on a `Failed to collect coverage` marker, a missing or invalid project +report, or a runtime-owning production TypeScript file absent from that report. +`coverage:merge` requires every configured Tier A report before replacing the +merged output. `coverage:health --require-report` independently repeats report +completeness checks plus aggregate and selected critical-file ratchets. + +Runtime-owning files are discovered from the TypeScript AST. Specs, +declarations, test setup and stubs, generated and environment files, `index.ts`, +type-only files, and pure re-export shims are excluded. Ratchets live under +`reporting.coverageRatchet` in `tools/coverage/coverage-policy.json`; update +them only from a fresh full `coverage:ci` report when every value stays level +or rises, and never lower one to accept a regression. + +| Tier | Rule | Validation | +| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------- | +| A | Product/runtime Angular, Electron, backend, data-access, portal, playlist, workspace, playback, EPG, and shared UI code collects source coverage. | `pnpm run coverage:ci` | +| B | Validate behavior without percentage coverage, such as `website`, `packaging`, and Playwright E2E projects. | `pnpm nx test website`, `pnpm nx test packaging`, or the closest E2E target | +| C | Excluded from the source coverage baseline, such as mock servers, test helper libraries, and untested feature shells. | Validate through dependent flows, or add focused tests when changing behavior directly | `apps/website` is an Astro marketing site. Its useful signal is a successful static build plus targeted output checks, not a merged code coverage percentage. @@ -69,6 +83,7 @@ Projects with a test target but no specs, such as `remote-control-web` and For local coverage inspection: ```bash +pnpm run coverage:tools:test pnpm run coverage:unit:ci pnpm run coverage:merge pnpm run coverage:health diff --git a/docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md b/docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md index d1224d9f8..671305242 100644 --- a/docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md +++ b/docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md @@ -4,10 +4,16 @@ **Goal:** Make Tier A coverage fail closed and add meaningful regression coverage for the HTTP server, remote control, settings, and download-file Electron boundaries. -**Architecture:** A shared pure Node module classifies runtime-owning TypeScript, validates per-project Istanbul reports, scans child output for collection failures, and evaluates aggregate/per-file ratchets. Existing Electron modules keep their public behavior; tests use real loopback/filesystem behavior where practical and mock only process, persistence, or native-shell boundaries. +**Architecture:** A shared pure Node module classifies runtime-owning TypeScript, validates per-project Istanbul reports, scans child output for collection failures, and evaluates aggregate/per-file ratchets. Existing Electron contracts stay unchanged except for the TDD-backed rejection of a discovered static-path traversal escape; tests use real loopback/filesystem behavior where practical and mock only process, persistence, or native-shell boundaries. **Tech Stack:** Node.js ESM and `node:test`, TypeScript compiler API, Istanbul coverage maps, Jest/ts-jest, Electron main-process IPC, Node HTTP streams, Angular Jest configuration, Nx, Playwright. +**Execution note (2026-07-26):** The branch was rebased onto +`origin/master` commit `a5bb8dc25ca1284320d93f14e17de58687984602` +before the final ratchet run. A real-loopback HTTP regression exposed a Windows +double-leading-slash traversal escape; the implementation adds a tested pure +resolver and a user-facing Electron fix note. + --- ## Scope And File Map @@ -37,7 +43,8 @@ Modify: - `libs/m3u-state/tsconfig.spec.json` — use bundler resolution and include the shared Electron window declaration so `effects.ts` instruments. - `apps/electron-backend/src/app/server/http-server.ts` — optional static-root - and server-factory seam; accept port `0` for loopback tests. + and server-factory seam, port `0` support for loopback tests, and a tested + static-root containment resolver. - `apps/electron-backend/src/app/events/remote-control.events.ts` — export the class for isolated instances while preserving the default singleton. - `apps/electron-backend/src/app/events/settings.events.spec.ts` — persistence, @@ -1615,6 +1622,10 @@ Expected: the provisional policy remains the only unstaged source change. `docs/architecture/validation-map.md` - Modify: `docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md` +- Modify: + `docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md` +- Create: + `.changes/electron-remote-static-paths.md` - [ ] **Step 1: Generate a complete fresh candidate report** @@ -1728,19 +1739,32 @@ Also include `pnpm run coverage:tools:test` in the local coverage command block. - [ ] **Step 5: Confirm documentation and release-note decisions** +Because the real-loopback regression exposed and fixed an observable Windows +static-path traversal escape, add `.changes/electron-remote-static-paths.md`: + +```markdown +--- +type: fix +area: electron +--- + +The desktop remote-control server now blocks crafted static paths from escaping +bundled web files on Windows. +``` + Run: ```bash pnpm exec prettier --check \ docs/architecture/validation-map.md \ - docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md + docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md \ + docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md \ + .changes/electron-remote-static-paths.md pnpm run release:notes:validate ``` -Expected: both pass. Do not add `.changes/*.md`: this PR changes tests, CI -integrity, and behavior-preserving testability seams, not user-visible -behavior. Because it touches runtime `.ts`, apply `no-release-note` to the PR -in Task 10. +Expected: both pass. The release note records the narrow user-visible security +fix; do not apply `no-release-note` in Task 10. - [ ] **Step 6: Commit ratchets and docs** @@ -1750,12 +1774,14 @@ Run: git add \ tools/coverage/coverage-policy.json \ docs/architecture/validation-map.md \ - docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md + docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md \ + docs/superpowers/plans/2026-07-25-coverage-integrity-runtime-boundaries.md \ + .changes/electron-remote-static-paths.md git commit -m "ci(coverage): ratchet runtime boundaries" ``` -Expected: the provisional policy is replaced by achieved values and the -worktree becomes clean. +Expected: the provisional policy is replaced by achieved values, the security +fix has a valid release note, and the worktree becomes clean. ### Task 9: Run Fresh Validation And Audit The Final Diff @@ -1921,7 +1947,7 @@ The body must contain: - runner, merge, health, and ratchet behavior; - exact validation commands/results; - docs update; -- no release note because behavior is unchanged; and +- the Electron fix release note for Windows static-path containment; and - deferred `database.worker.ts`, dashboard rails, and Stalker search. Use: @@ -1938,17 +1964,17 @@ gh pr create \ Provide the fully populated body on standard input; do not leave metric or validation placeholders. -- [ ] **Step 5: Apply the internal-only release-note label** +- [ ] **Step 5: Verify release-note metadata** Run: ```bash -gh pr edit --add-label no-release-note -gh pr view --json url,isDraft,headRefName,baseRefName,labels,commits,statusCheckRollup +gh pr view --json url,isDraft,headRefName,baseRefName,labels,files,commits,statusCheckRollup ``` -Expected: the PR is draft, head/base are correct, `no-release-note` is present, -and the command prints the PR URL. +Expected: the PR is draft, head/base are correct, +`.changes/electron-remote-static-paths.md` is included, `no-release-note` is +absent, and the command prints the PR URL. - [ ] **Step 6: Return the exact handoff** @@ -1960,5 +1986,6 @@ Return: - aggregate and selected-file before/after metrics; - exact command results and any explicit environment-only skip; - docs updated (`docs/architecture/validation-map.md`); -- release note skipped with `no-release-note`; +- release note added (`.changes/electron-remote-static-paths.md`) with no + `no-release-note` label; - deferred zero-coverage follow-ups. diff --git a/docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md b/docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md index 1357b2049..0fb2c9bb9 100644 --- a/docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md +++ b/docs/superpowers/specs/2026-07-25-coverage-integrity-runtime-boundaries-design.md @@ -4,8 +4,9 @@ Approved in the delegated coverage-audit task on 2026-07-25. -The implementation branch starts from fresh `origin/master` commit -`4e5132cb` (`ci(release): gate PRs on an authored release note (#1257)`). +Before recording the final ratchets, the implementation branch was rebased +onto fresh `origin/master` commit +`a5bb8dc25ca1284320d93f14e17de58687984602`. ## Goal @@ -18,31 +19,34 @@ percentage by testing type-only modules, re-export shims, or low-risk code. ## Fresh Baseline -`pnpm run coverage:ci` on `4e5132cb` completed successfully and reported: +An uncached `pnpm run coverage:ci` on +`a5bb8dc25ca1284320d93f14e17de58687984602` completed successfully and reported: | Metric | Covered / total | Percent | | ---------- | --------------: | ------: | -| Statements | 24,684 / 35,843 | 68.86% | -| Branches | 17,290 / 29,471 | 58.66% | -| Functions | 5,728 / 8,524 | 67.19% | -| Lines | 24,051 / 34,754 | 69.20% | +| Statements | 24,974 / 36,062 | 69.25% | +| Branches | 17,461 / 29,637 | 58.91% | +| Functions | 5,784 / 8,577 | 67.43% | +| Lines | 24,315 / 34,959 | 69.55% | The successful command nevertheless printed `Failed to collect coverage` for `libs/m3u-state/src/lib/effects.ts`. TypeScript could not resolve `@angular/material/snack-bar`, inferred the injected snack bar as `unknown`, and did not see the shared `window.electron` declaration under the project's -spec compiler configuration. The file was absent from both the project report -and `coverage/merged/coverage-final.json`. +spec compiler configuration (`TS2307` once, `TS2571` twice, and `TS2551` +once). The file was absent from both the project report and +`coverage/merged/coverage-final.json`. -All 30 Tier A projects produced a `coverage-final.json`, so the current merge -included 30 reports and still missed an executable file. This demonstrates +All 30 Tier A projects produced a `coverage-final.json`, so 31 report files +including the merged report existed. The current merge included all 30 project +reports and 709 files while still missing an executable file. This demonstrates that project-level report existence is necessary but not sufficient. The audit file status on the same baseline is: | File | Statement coverage | | --------------------------------------------------------------------------------------- | -----------------: | -| `apps/electron-backend/src/app/workers/database.worker.ts` | 0 / 205 (0%) | +| `apps/electron-backend/src/app/workers/database.worker.ts` | 0 / 207 (0%) | | `apps/electron-backend/src/app/events/remote-control.events.ts` | 0 / 115 (0%) | | `apps/electron-backend/src/app/server/http-server.ts` | 0 / 73 (0%) | | `apps/electron-backend/src/app/events/database/downloads.events.ts` | 69 / 147 (46.93%) | @@ -333,6 +337,57 @@ They must not depend on a previously generated workspace `coverage/` tree. - Deferred database worker, dashboard, and Stalker search coverage is listed in the draft PR. +## Implementation Outcome + +The fresh uncached implementation run merged all 30 required Tier A reports +into the 31st `coverage-final.json` and included 710 files, with no +collection-failure marker or missing runtime-owning source. The achieved +ratchets are: + +| Metric | Covered / total | Percent | +| ---------- | --------------: | ------: | +| Statements | 25,196 / 36,241 | 69.52% | +| Branches | 17,569 / 29,748 | 59.05% | +| Functions | 5,823 / 8,635 | 67.43% | +| Lines | 24,535 / 35,136 | 69.82% | + +The selected Electron boundary statement coverage changed as follows: + +| File | Baseline | After | +| -------------------------- | ----------------: | -----------------: | +| `http-server.ts` | 0 / 73 (0%) | 83 / 92 (90.21%) | +| `remote-control.events.ts` | 0 / 115 (0%) | 112 / 116 (96.55%) | +| `settings.events.ts` | 16 / 27 (59.25%) | 26 / 27 (96.29%) | +| `downloads.events.ts` | 69 / 147 (46.93%) | 86 / 147 (58.50%) | + +The integrity correction makes the denominator honest: +`libs/m3u-state/src/lib/effects.ts` is now present at 0 / 159 statements +instead of silently disappearing. The aggregate improvement therefore holds +even after surfacing those 159 previously unreported uncovered statements. + +Two production seams were sufficient for isolated contract tests: +`HttpServer` accepts an optional server factory and static distribution path, +and `RemoteControlEvents` is a named export while the production singleton is +unchanged. Real loopback HTTP tests also exposed a Windows +double-leading-slash traversal escape in the prior static-path normalization. +A direct failing regression preceded the narrow fix: production now uses the +pure, platform-testable `resolveStaticFilePath` resolver to decode once, reject +malformed or NUL paths, strip leading separators, resolve against the static +root, and enforce containment. This is a tested behavior and security fix, not +a behavior-preserving refactor. + +The release decision follows that observable fix: +`.changes/electron-remote-static-paths.md` records it as an Electron bug fix, +so the draft PR must not carry `no-release-note`. The canonical coverage +workflow is documented in `docs/architecture/validation-map.md`; `AGENTS.md` +and `CLAUDE.md` do not describe the changed coverage contract or HTTP path +logic and require no update. + +The deferred production files remain uncovered in this change: +`database.worker.ts` at 0 / 207, `workspace-dashboard-rails.component.ts` at +0 / 206, and `stalker-search.component.ts` at 0 / 166. They retain the +separate follow-up scopes described above. + ## Validation Ladder Run fresh, uncached validation in this order: diff --git a/tools/coverage/coverage-policy.json b/tools/coverage/coverage-policy.json index 2c1b8011e..4c6dbd224 100644 --- a/tools/coverage/coverage-policy.json +++ b/tools/coverage/coverage-policy.json @@ -3,7 +3,45 @@ "reporting": { "mergedCoverageDir": "coverage/merged", "e2eSummaryDir": "coverage/e2e", - "unitCodecovFlag": "unit" + "unitCodecovFlag": "unit", + "coverageRatchet": { + "merged": { + "statements": 69.52, + "branches": 59.05, + "functions": 67.43, + "lines": 69.82 + }, + "criticalFiles": [ + { + "path": "apps/electron-backend/src/app/server/http-server.ts", + "statements": { + "minimumCovered": 83, + "minimumPercent": 90.21 + } + }, + { + "path": "apps/electron-backend/src/app/events/remote-control.events.ts", + "statements": { + "minimumCovered": 112, + "minimumPercent": 96.55 + } + }, + { + "path": "apps/electron-backend/src/app/events/settings.events.ts", + "statements": { + "minimumCovered": 26, + "minimumPercent": 96.29 + } + }, + { + "path": "apps/electron-backend/src/app/events/database/downloads.events.ts", + "statements": { + "minimumCovered": 86, + "minimumPercent": 58.5 + } + } + ] + } }, "unitCoverage": { "description": "Tier A projects collect and trend source coverage. Tier B projects are validated without percentage coverage. Tier C projects are excluded from the source coverage baseline.",