From d7d37c0ff2ecd756b453bcb2cf85cee104fdea5e Mon Sep 17 00:00:00 2001 From: difficcd Date: Wed, 14 Oct 2026 17:41:06 +0900 Subject: [PATCH] chore(process): measure the process, then fix what the measurement found A first Measure -> Analyze -> Change cycle, started from a written review of how this project is developed (incremental development on XP practices). The review's own order was baseline first, since it is the cheapest step and the one everything after is judged by, so that is where this began - and the changes come from what the numbers showed. Measure. scripts/process-metrics.mjs reads the measures straight out of GitHub and git: no tool to install, rerunnable each cycle. Baseline, all history / last 30 days: deployment frequency 32.8 / 39.7 merges a week CI pass ratio 96.9% / 99.7% defect escape rate 43.2% / 46.5% fixes carrying a test 43.6% / 60.4% Analyze. CI says yes to nearly everything while almost one fix lands per two changes. Fixes concentrate in App (63 fix-touches; the next file has 11). Over half of all fixes landed without a test. And two things found on the way: - CI ran four of the ten gates. ci.yml listed its steps by hand, and every gate added to `npm run check` since - helper index, unreachable, unused imports, stroke writes, native dialogs, i18n - bound only on a machine where someone ran it. - ARCHITECTURE.md did not name 44 of the 152 files under src/ (29%); export/ had no section. Change. - CI runs `npm run check` as one step, so the two lists cannot drift apart again. - scripts/fix-needs-test.mjs: a branch with a fix: commit must change something under test/, or say why in a No-Test: trailer. The targeted test-first the review asked for. - scripts/architecture-index.mjs: every file under src/ must be named in ARCHITECTURE.md. The 44 were added. - A needs-verification label, so deferred device checks are counted (it read 0 while #338 waited, because nothing recorded the wait). README, README.ko and CONTRIBUTING corrected where they had drifted too: a guard table with its last row stranded outside it, "six guards" when there were seven, squash-merge where the repository rebase-merges, and "npm run check is what CI runs", which was not true until now. The record, with what each change is expected to move at the next measurement, is in docs/process/2026-10-14.md. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 24 ++- ARCHITECTURE.md | 71 +++++++- CONTRIBUTING.md | 19 ++- README.ko.md | 6 +- README.md | 19 ++- docs/process/2026-10-14.baseline.json | 163 +++++++++++++++++++ docs/process/2026-10-14.md | 147 +++++++++++++++++ docs/process/README.md | 41 +++++ package.json | 2 +- scripts/architecture-index.mjs | 56 +++++++ scripts/commitKind.mjs | 13 ++ scripts/fix-needs-test.mjs | 68 ++++++++ scripts/process-metrics.mjs | 214 +++++++++++++++++++++++++ test/scripts/architectureIndex.test.js | 33 ++++ test/scripts/fixNeedsTest.test.js | 55 +++++++ 15 files changed, 905 insertions(+), 26 deletions(-) create mode 100644 docs/process/2026-10-14.baseline.json create mode 100644 docs/process/2026-10-14.md create mode 100644 docs/process/README.md create mode 100644 scripts/architecture-index.mjs create mode 100644 scripts/commitKind.mjs create mode 100644 scripts/fix-needs-test.mjs create mode 100644 scripts/process-metrics.mjs create mode 100644 test/scripts/architectureIndex.test.js create mode 100644 test/scripts/fixNeedsTest.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eacc57a3..3f392c0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,11 @@ jobs: # the six-hour default. timeout-minutes: 10 steps: + # Full history, not the default single commit: the fix-needs-a-test gate compares the branch + # against origin/main, and with one commit there is no merge base to compare from. - uses: actions/checkout@v7 + with: + fetch-depth: 0 - uses: actions/setup-node@v7 with: @@ -41,19 +45,13 @@ jobs: # added without committing the lockfile fails here rather than on someone else's machine. - run: npm ci - - name: Type check - run: npm run typecheck - - - name: Unit tests - run: npm test - - # Fails only when hook dependency warnings grow, i.e. when a change adds new stale-closure - # risk. See scripts/hook-baseline.mjs for why zero is not the target. - - name: Hook baseline - run: node scripts/hook-baseline.mjs - - - name: Build - run: npm run build + # The whole gate, as one command - the same one run locally. This used to list its steps by + # hand (type check, tests, hook baseline, build), and as gates were added to `npm run check` + # the copy here was never updated: by 2026-10-14 CI ran four of the ten, and the helper index, + # unreachable names, unused imports, stroke writes, native dialogs and i18n checks bound only + # on a machine where someone remembered to run them. One list cannot drift from itself. + - name: npm run check + run: npm run check # Separate from `check` on purpose: that job is the required gate and it finishes in half a # minute, so it must not wait behind a browser. This one is allowed to be the slow one. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index dd76091b..f0021093 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -9,7 +9,7 @@ Frame-by-frame MV/animation app. Vite + React 19. Capacitor wraps it for Android ## Files -App.tsx is around 2,100 lines; it is mostly wiring now — which hook owns what, and +App.tsx is around 2,200 lines; it is mostly wiring now — which hook owns what, and which component gets which props. **Put new pure logic in a module, not in App.tsx** — anything that is a function of its arguments belongs next to its tests. See [#234] for where the rest of App is going and why. @@ -52,6 +52,10 @@ know storage exists at all, because reading it is `useStored`'s decoder. write UI strings in Korean and add the English to the dictionary. Called `tr`, not `t` — `t` is a local variable in dozens of places. - `src/db.ts` — IndexedDB autosave (`saveAutosave`, `loadAutosave`, plus project CRUD). +- `src/i18n.ja.ts` — the Japanese dictionary, keyed by the same Korean text; a missing entry falls + back to English, not to the Korean key. +- `src/globals.d.ts` — browser and vendor globals the type checker does not know (the EyeDropper + among them), declared as possibly absent because the code checks before using them. **core/** — pure logic, all of it tested, all of it TypeScript (the first folder converted; the shapes the modules share are in `types.ts`, and the document's own are declared in @@ -117,6 +121,44 @@ index signature, so a misspelt one is an error. Ids there are `DocId` - a number to come: presets that use the room instead of zooming to make some, and a camera that can span more than one cut. +- `ids.ts` — `nextId`, `randomId`. These were `Date.now()` in nineteen places, and a millisecond + is long enough to make two things with one id. +- `camera.ts` — where the camera is looking at a moment in a cut: a window onto the artwork (a + centre and a zoom), its presets and paths. Applied at playback only. +- `clipping.ts` — `clipGroups`, `canClip`: a layer that only shows where the layer beneath it has + paint, which is how shading goes over flat colour. +- `shapeStroke.ts` — `rectPoints`, `ellipsePoints`: ruler shapes as ordinary point lists, which is + the whole idea - a shape is then just a stroke. +- `pathMotion.ts` — turning a drawn line into a path something can travel along evenly: smoothing + and resampling by length, since a pen piles points up wherever it slowed down. +- `brushSize.ts` / `viewZoom.ts` — the brush width range and the canvas zoom range, each in one + place after being written in two and three places that disagreed. +- `timelineZoom.ts` — timeline pixels to time and back (`timeAtX`, `xAtTime`), and zooming without + the content sliding out from under the cursor. +- `windowDrag.ts` — `dragOnWindow`: a drag run on window listeners, because the pointer leaves the + element it started on constantly. Ends on `pointercancel` too, which every hand-rolled one missed. +- `persist.ts` — `readStored`/`writeStored` and the codecs under `useStored`: the small preferences + kept in localStorage. +- `playbackRate.ts` / `playbackStart.ts` / `timeScale.ts` — the preview speed and that it is + remembered, where play begins when it is pressed, and baking a preview speed into the film's + real timings. +- `mediaReducer.ts` / `mediaEl.ts` — the audio and video tracks as one reducer, and the one thing + done to the elements playing them (`detachMedia`). +- `api.ts` / `probeBackoff.ts` — talking to the local project server (`apiFetch` adds the status + check fetch does not do; `retryDelayMs` honours a 429), and how often to ask whether it is there. +- `projectAssets.ts` — how each piece of a project is stored - as an asset, a blob or a data URL - + and how a stored one comes back. +- `splitProject.ts` — cutting one long project into sequential pieces that can be worked on apart + (#123). +- `textEdit.ts` — a text object's twenty-odd fields in one place (`textFromEdit`, `editFromText`) + instead of three hand-copied ones. +- `videoCuts.ts` — the arithmetic in the middle of a video import: extracted frames into cuts, + their durations and parts. +- `recordBitrate.ts` — the bitrate to ask the recorder for, by codec and canvas size; left unset, + Chrome settles near 2.5 Mbps whatever the size. +- `decodeBudget.ts` — how many decoded frames to hold, which to let go of, and which cached layers + a decode invalidates. + **engine/** — what a frame *is*, with no canvas anywhere near it. TypeScript; `Scene`, `EvaluatedCut` and `EvaluatedGroup` in `evaluateFrame.ts` are the renderer's whole input, named. @@ -155,6 +197,21 @@ frame is made of, and `SceneDeps` what the renderer needs from the app. - `warpRender.ts` / `shearSlices.ts` / `swayRender.ts` — a selection's rotate/skew/bend outline, the slice stack that draws a sheared bitmap, and the sway deformation. +- `canvasFactory.ts` — `makeCanvas`/`setCanvasFactory`: where the canvas modules get a canvas, which + is what lets the pixel tests run them in Node on `@napi-rs/canvas`. + +**export/** — turning the timeline into a file. No React; what only the app knows (the range, the +audio graph, the playback loop) stays in `hooks/useExport.ts`. + +- `recorder.ts` — `pickRecordingType` (MP4 first, asked for by a codec name browsers accept), + `frameSource`, `startRecorder`: recording the canvas, minus the parts that need the app. +- `gif.ts` — a GIF89a encoder, enough for an animation with a transparent background, which no + browser API produces. +- `zip.ts` / `byteWriter.ts` — a store-only ZIP writer (PNGs are already deflated, so compressing + them again buys nothing) and the growable byte buffer under it. +- `download.ts` — `downloadBlob`: handing a finished file to the browser, once rather than three + times. + **styles/** — the stylesheets, one per area (top bar, toolbar, canvas, the cut/layer panel, the timeline, the colour panel, touch, …), imported in their original cascade order by `styles/index.css`. The order is load-bearing: some rules override others by position alone. @@ -178,6 +235,13 @@ prompt, tool keys, project picker, progress overlay), all over `Modal.tsx`. And them stops the work underneath; that is the rule they share. - `NumField.tsx` — **use NumField for any new numeric field.** +- `Logo.tsx` — the app mark: three receding frames - the onion skin - with a play mark on the front. +- `dialogs/` — one file per dialog. `AskModal.tsx` is the app's own confirm and prompt (a browser + told to block dialogs makes `confirm` return silently, so the button looks dead). + `SettingsModal.tsx` / `ToolKeysModal.tsx` are the settings and the two keymap editors; + `VideoImportModal.tsx`, `SceneDetectModal.tsx`, `ExportRangeModal.tsx`, `ProjectPicker.tsx`, + `LinkPromptModal.tsx`, `ProgressOverlay.tsx` and `HelpModal.tsx` do what they are called. + **hooks/** — TypeScript. State that belongs together, lifted out of App so its wiring is somewhere with a name; each hook's dependency bag is an interface (`PlaybackDeps`, `ExportDeps`, `LayerCacheDeps`, …). Each takes what it cannot own as arguments, and the rule for what it cannot own is the same every time: `buildData` reads most of App's state and `restore` writes most of it, so a hook that @@ -236,6 +300,11 @@ needs a document takes those two functions rather than the document. call sites keep the shape `window.confirm` gave them. `AskModal` draws it. - `useDialogs.ts` — which dialog is open, and the rebinding two of them share. +- `useCutListUi.ts` — what is picked in the cut list and how much of it is unfolded, kept in one + hook so opening a document clears all of it at once (#241). +- `useTimelineView.ts` — how far the timeline is zoomed, which part is on screen, and the two + overlays drawn while a drag is happening. + - `server/` — the Express file-backed project DB on :8787, files under `server/data/`. `index.js` is the wiring (body limit, rate-limit tiers, mount, listen); `projects.js`, `backups.js` and `youtube.js` are one route family each; `paths.js` builds every path, so `safeId` cannot be diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f54eb518..5bd08812 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,7 +50,19 @@ git commit -m "fix: ..." git push -u origin HEAD gh pr create --fill # put "Closes #12" in the body gh pr checks # CI -gh pr merge --squash --delete-branch +gh pr merge --rebase --delete-branch # rebase, so each commit lands on main as written +``` + +**A `fix:` brings its test.** If a branch has a `fix:` commit, it has to change something under +`test/` - the test that fails without the fix. That is the moment a defect is best understood and +a test is cheapest to write, and it is what stops the same defect coming back: before this rule, +over half of all fixes had landed without one. When nothing can be observed by a test (a colour, +a config value), say so in the commit message and the gate accepts it: + +``` +fix: the panel border is visible in the light theme + +No-Test: colour only; nothing here a test can observe ``` ## `main` is protected @@ -76,7 +88,7 @@ Turning it back on is the part that gets forgotten, so do both in one sitting. ## Verifying ```bash -npm run check # typecheck → tests → hook-warning baseline → build +npm run check # typecheck → tests → nine static guards → build; CI runs exactly this ``` **Passing this proves less than it looks.** A component that returns `undefined` is legal @@ -107,7 +119,8 @@ evidence you will have. ## Where code goes The folders under `src/` say what a file is allowed to touch. The details are in -[ARCHITECTURE.md](ARCHITECTURE.md) — **read it before editing App.jsx.** +[ARCHITECTURE.md](ARCHITECTURE.md) — **read it before editing App.tsx.** Every file under `src/` +is named in it, and `npm run check` fails when a new one is not. ``` core/ pure logic — no React, no DOM, no canvas. Everything here has tests. diff --git a/README.ko.md b/README.ko.md index 18af6e7d..f4fd2cfc 100644 --- a/README.ko.md +++ b/README.ko.md @@ -96,7 +96,7 @@ npm run lint # eslint-plugin-react-hooks npm run smoke # 빌드된 앱을 헤드리스 브라우저에서 띄워 선 하나 긋기 ``` -`npm run check`가 관문입니다: 타입 검사, 단위 테스트, 정적 가드 여섯, 빌드. 같은 단계가 모든 푸시와 PR에서 CI로 돕니다. +`npm run check`가 관문입니다: 타입 검사, 단위 테스트, 정적 가드 아홉, 빌드. CI는 모든 푸시와 PR에서 이 명령 하나를 그대로 돌립니다 - 예전에는 단계를 손으로 나열했고, 10월에는 그 사본이 열 개 중 넷만 돌리고 있었습니다([docs/process/](docs/process/2026-10-14.md)). 코드는 TypeScript입니다 — JavaScript에서 폴더 하나씩 옮겼고(#268) 모든 파일이 `strict`로 검사됩니다. 목적은 주석을 타입으로 바꾸는 것이 아니라(JS도 이미 JSDoc으로 타입이 있었음) JSDoc이 줄 수 없던 @@ -115,9 +115,11 @@ npm run smoke # 빌드된 앱을 헤드리스 브라우저에서 띄워 선 | `scripts/unused-imports.mjs` | 파일 안 아무것도 쓰지 않는 import | | `scripts/stroke-writes.mjs` | `commitStroke`를 거치지 않고 레이어에 선을 추가하는 쓰기 | | `scripts/native-dialogs.mjs` | `src/` 안의 `alert`·`confirm`·`prompt` — 브라우저가 대화상자를 막으면 셋 다 조용히 실패함 | +| `scripts/architecture-index.mjs` | `ARCHITECTURE.md`가 이름을 대지 않는 `src/` 아래 파일 | +| `scripts/fix-needs-test.mjs` | `fix:` 커밋이 있는데 `test/` 아래를 하나도 바꾸지 않은 브랜치 (`No-Test:` 트레일러로 이유를 적으면 예외) | | `scripts/i18n-check.mjs` | 영어 항목 없는 `tr()` 리터럴, 또는 소스에 키가 더는 없는 사전 항목 | -단위 테스트 약 1,150개가 `src/core`, `src/canvas`, `src/engine`, `src/export`의 순수 모듈을 +단위 테스트 약 1,300개가 `src/core`, `src/canvas`, `src/engine`, `src/export`의 순수 모듈을 덮습니다 — 기하, 이징, 키프레임 샘플링, 컷 리듀서, 레이어 트리 이동, 올가미 오려내기, 타임라인 스냅, 시간 배율 굳히기, GIF·zip 라이터. DOM도 프레임워크도 필요 없어 Node 내장 러너를 씁니다. 실제로 그리는 함수들 — 선을 픽셀로, 노이즈, 모자이크, 채우기 — 는 `@napi-rs/canvas`(브라우저 diff --git a/README.md b/README.md index 2ee73a49..86710fbf 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,9 @@ npm run lint # eslint-plugin-react-hooks npm run smoke # boots the built app in a headless browser and draws a stroke ``` -`npm run check` is the gate: typecheck, the unit tests, then six static guards, then the build. -The same steps run in CI on every push and pull request. +`npm run check` is the gate: typecheck, the unit tests, then nine static guards, then the build. +CI runs that one command on every push and pull request - it used to list its steps by hand, and +by October that copy ran four of the ten (see [docs/process/](docs/process/2026-10-14.md)). The code is TypeScript, moved from JavaScript one folder at a time (#268), and every file is held to `strict`. The point was never the annotations - the JavaScript was already typed through @@ -119,15 +120,21 @@ dependency), and the bundler and `tsc` accept it. | `scripts/unused-imports.mjs` | an import nothing in the file uses | | `scripts/stroke-writes.mjs` | a write that adds a stroke to a layer without going through `commitStroke` | | `scripts/native-dialogs.mjs` | an `alert`, `confirm` or `prompt` in `src/` — a browser that blocks dialogs makes all three fail silently | +| `scripts/architecture-index.mjs` | a file under `src/` that `ARCHITECTURE.md` does not name | +| `scripts/fix-needs-test.mjs` | a branch with a `fix:` commit that changes nothing under `test/`, unless a `No-Test:` trailer says why | +| `scripts/i18n-check.mjs` | a `tr()` literal with no English entry, or a dictionary row whose key no longer appears in the source | -Two measuring tools, neither in the gate — they print numbers rather than pass or fail: +Measuring tools, none in the gate — they print numbers rather than pass or fail: `npm run bench` times the pure hot paths, and `node scripts/profile-frame.mjs` drives a real browser against a running dev server and reports frame time, 2d calls per frame attributed to the canvas they landed on, and image readbacks. The first cannot see the canvas; the second cannot see -raster and compositing. Both say so in their own headers. -| `scripts/i18n-check.mjs` | a `tr()` literal with no English entry, or a dictionary row whose key no longer appears in the source | +raster and compositing. Both say so in their own headers. `node scripts/video-dedupe-check.mjs` +imports a generated clip whose every frame differs and fails if any are merged (#124), and +`node scripts/process-metrics.mjs` reads the repository's own records into the process measures +- deployment frequency, lead time, test pass ratio, defect escape rate - that each +[process cycle](docs/process/) is judged against. -Around 1,150 unit tests cover the pure modules under `src/core`, `src/canvas`, `src/engine` and +Around 1,300 unit tests cover the pure modules under `src/core`, `src/canvas`, `src/engine` and `src/export` — geometry, easing, keyframe sampling, the cuts reducer, layer-tree moves, lasso cut-out, timeline snapping, the time-scale bake, GIF and zip writers. They use Node's built-in runner because none of it needs a DOM or a framework. The functions that draw - strokes into diff --git a/docs/process/2026-10-14.baseline.json b/docs/process/2026-10-14.baseline.json new file mode 100644 index 00000000..5d0ac977 --- /dev/null +++ b/docs/process/2026-10-14.baseline.json @@ -0,0 +1,163 @@ +{ + "all": { + "window": "all history", + "measuredAt": "2026-10-14", + "merges": 291, + "mergesByKind": { + "refactor": 108, + "fix": 73, + "feat": 54, + "docs": 17, + "chore": 10, + "perf": 7, + "test": 5, + "ci": 2, + "security": 2, + "deps": 2 + }, + "deploymentsPerWeek": 32.8, + "prLeadTimeHours": { + "median": 0, + "p90": 1.9 + }, + "issueLeadTimeDays": { + "median": 0, + "p90": 15.5, + "closed": 49 + }, + "ciPassRatio": 96.9, + "ciRunsJudged": 769, + "mainBranchFailures": 1, + "defectEscapeRate": 43.2, + "fixMerges": 73, + "introducingMerges": 169, + "mttrDays": { + "median": 0, + "bugsClosed": 11 + }, + "fixCommitsWithATest": 43.6, + "fixCommits": 101, + "risk": { + "untickedChecklistItems": 3, + "issuesAwaitingVerification": 2, + "reverts": 0, + "fixesOfARecentFix": 43, + "longestGapBetweenMergesDays": 9, + "openIssues": 3, + "oldestOpenIssueDays": 62.1 + }, + "defectHotspots": [ + [ + "src/App", + 63 + ], + [ + "src/ui/AnimPanels", + 7 + ], + [ + "src/canvasUtils", + 6 + ], + [ + "src/hooks/useExport", + 5 + ], + [ + "src/canvas/canvasUtils", + 5 + ], + [ + "src/core/layerOps", + 4 + ], + [ + "src/ui/TopBar", + 4 + ], + [ + "src/canvas/pixelEffects", + 4 + ] + ] + }, + "last30": { + "window": "last 30 days", + "measuredAt": "2026-10-14", + "merges": 170, + "mergesByKind": { + "refactor": 67, + "fix": 46, + "feat": 26, + "docs": 11, + "chore": 7, + "perf": 6, + "test": 3 + }, + "deploymentsPerWeek": 39.7, + "prLeadTimeHours": { + "median": 0, + "p90": 0.1 + }, + "issueLeadTimeDays": { + "median": 0, + "p90": 9.7, + "closed": 29 + }, + "ciPassRatio": 99.7, + "ciRunsJudged": 371, + "mainBranchFailures": 0, + "defectEscapeRate": 46.5, + "fixMerges": 46, + "introducingMerges": 99, + "mttrDays": { + "median": 4.9, + "bugsClosed": 5 + }, + "fixCommitsWithATest": 60.4, + "fixCommits": 48, + "risk": { + "untickedChecklistItems": 3, + "issuesAwaitingVerification": 2, + "reverts": 0, + "fixesOfARecentFix": 25, + "longestGapBetweenMergesDays": 5.2, + "openIssues": 3, + "oldestOpenIssueDays": 62.1 + }, + "defectHotspots": [ + [ + "src/App", + 23 + ], + [ + "src/ui/AnimPanels", + 7 + ], + [ + "src/hooks/useExport", + 5 + ], + [ + "src/canvas/canvasUtils", + 4 + ], + [ + "src/canvas/pixelEffects", + 4 + ], + [ + "src/canvas/strokes", + 3 + ], + [ + "src/core/layerOps", + 3 + ], + [ + "src/export/recorder", + 2 + ] + ] + } +} diff --git a/docs/process/2026-10-14.md b/docs/process/2026-10-14.md new file mode 100644 index 00000000..f5b1c2d7 --- /dev/null +++ b/docs/process/2026-10-14.md @@ -0,0 +1,147 @@ +# Process cycle 1 — 2026-10-14 + +The first pass through Measure → Analyze → Change. It starts from a written review of how this +project is developed, done for a software engineering course: it identified the process as +incremental development run on XP practices, checked it against the ten XP practices, and closed +with a risk plan and five steps to take. The review's own ordering was that **the measurement +baseline comes first** - it is the cheapest step and the one every other step is judged by - so +that is where this cycle began, and the changes were chosen from what the numbers showed rather +than from the list. + +Raw numbers: [`2026-10-14.baseline.json`](2026-10-14.baseline.json), produced by +`node scripts/process-metrics.mjs --json` (all history and `--since 30`). + +## 1. Measure + +The history runs from 2026-06-17 to 2026-10-14: 291 merged PRs, 52 issues, 430 commits. + +| measure | all history | last 30 days | +|---|---|---| +| deployment frequency | 32.8 merges/week (291) | 39.7 merges/week (170) | +| lead time, PR → deploy | median 0h, p90 1.9h | median 0h, p90 0.1h | +| lead time, issue → close | median 0d, p90 15.5d (49 issues) | median 0d, p90 9.7d (29) | +| automated test pass ratio | 96.9% of 769 CI runs; 1 failure on main | 99.7% of 371; 0 on main | +| defect escape rate | **43.2%** (73 fixes / 169 feat+perf+refactor) | **46.5%** (46 / 99) | +| MTTR, bug-labelled issues | median 0d over 11 bugs | median 4.9d over 5 | +| fixes carrying a test | **43.6%** of 101 fix commits | **60.4%** of 48 | + +| risk indicator (the review's monitoring list) | all history | last 30 days | +|---|---|---| +| estimation — deferred verification | 3 unticked checklist items; 0 issues labelled as waiting | same | +| tools — reverts | 0 | 0 | +| tools — a fix landing on a file another fix touched that week (App aside) | 43 | 25 | +| people — longest gap between merges | 9d | 5.2d | +| open issues | 3, oldest 62 days | | + +Defect hotspots, all history - the source files fix commits touched most: + +| fixes | file | +|---|---| +| 63 | `src/App` (`.jsx` then `.tsx`, counted as one) | +| 11 | `canvasUtils` (`src/` then `src/canvas/`; split up in #290) | +| 7 | `src/ui/AnimPanels` | +| 5 | `src/hooks/useExport` | +| 4 | `src/core/layerOps`, `src/ui/TopBar`, `src/canvas/pixelEffects` | + +### Corrections made before the numbers were trusted + +The first run gave a defect escape rate of 48.5%. Three things were wrong with how it was being +read, and each moved a number: + +- **55 PR titles were prose** ("hooks/ in TypeScript"), so they had no `feat:`/`fix:` prefix and + counted as neither. Most were the TypeScript migration - refactors - and leaving them out + shrank the denominator. Classifying a prefix-less PR by its commits, which rebase-merge puts on + main as written, took the rate to 43.2%. +- **Renames split one file's history.** `App.jsx` and `App.tsx` are the same component; counted + apart, the hotspot list understated it. +- **The translation tables ranked as hotspots** because every fix that changes a UI string adds + its translations. They are left out, as is CSS. + +And one indicator was redefined: "a fix touching a file another fix touched that week" read 77 +because nearly every change wires through App. An indicator that is always high cannot be +watched, so App is excluded from the overlap; it reads 43. + +## 2. Analyze + +**CI says yes, and defects ship anyway.** The pass ratio is 97-99.7% while nearly one fix lands +for every two changes. The review put this down to CI checking syntax rather than meaning; the +numbers agree, with one refinement - CI does run 1,300 unit tests, so the issue is not that it +only checks syntax but that the tests do not cover where the defects actually land. + +**Where they land is one file.** App took 63 fix-touches, against 11 for the next. This is the +"super component" the review named as the cost of building without design, now with a count. The +second hotspot, `canvasUtils`, no longer exists - it was split up in #290 - which is some evidence +the refactoring the review credited is the right remedy. + +**Test-after, measured.** 56% of all fixes landed without touching a test. That has improved - +40% over the last month - but a fix without a test is the clearest case of the test-after +development the review blamed, because nothing stops the same defect returning. + +**CI enforced four of its ten gates.** Found while wiring in the first change below. `ci.yml` +said it existed to run `npm run check` on every push, and then listed the steps by hand - type +check, tests, hook baseline, build. Every gate added locally since (helper index, unreachable +names, unused imports, stroke writes, native dialogs, i18n) bound only on a machine where someone +remembered to run it. The review's statement that the XP rule "every build must pass the tests" +was already implemented in CI was true for four tenths of it. CONTRIBUTING.md said "`npm run +check` is what CI runs", and was wrong. + +**The documents drift, and by how much.** `ARCHITECTURE.md` - the file whose own title says to +read it before editing - did not name 44 of the 152 files under `src/`, 29%. The `export/` folder +had no section at all; App's stated size was 2,100 lines against 2,198. A loose search (the bare +word, not the filename) found only 34 missing, because "zip" and "camera" appear in prose without +the files ever being named. The review's diagnosis was that keeping documents current is never +the cheapest next step in incremental development, which is why the cure has to be a check rather +than a resolution. + +**Some of the measurements cannot be trusted yet.** +- Only 11 of the 73 fixes had a bug-labelled issue, so MTTR describes about 15% of defects. +- PR lead time is near zero because PRs are opened after the work is done; the issue → close + figure is the meaningful one, and it is bimodal - most issues close the day they open, and the + rest wait weeks. +- "Deferred verification" read 0 while #338 was waiting on a device, because nothing recorded the + wait. + +## 3. Change + +Each change names the part of the review it answers and the number it should move. + +| # | change | answers | expected to move | +|---|---|---|---| +| C1 | **A fix brings its test.** `scripts/fix-needs-test.mjs`: a branch with a `fix:` commit must change something under `test/`, or carry a `No-Test:` trailer saying why. In the gate. | XP practice "test-first development" (not applied), adopted where the review said it should be: where defects happen. Risk plan: *semantic bugs reaching deploy - avoidance*. | fixes carrying a test: 60% → ~100%, waivers aside | +| C2 | **The map is checked.** `scripts/architecture-index.mjs`: every file under `src/` must be named in `ARCHITECTURE.md`. The 44 missing were added, with an `export/` section. In the gate. | The review's documentation step - make document freshness something verified, not hoped for. Risk plan: *black-box code accumulating - minimization*. | 152/152 named, and staying there | +| C3 | **CI runs the gate, all of it.** `ci.yml` now runs `npm run check` as one step instead of a hand-kept copy of it, so it cannot drift again. | XP practice "continuous integration", which the review counted as applied. | test pass ratio may *dip* at first - six more checks now count. That would be the gates working | +| C4 | **Deferred verification is recorded.** A `needs-verification` label, applied to #338 and #4, and counted by the metrics. | Risk monitoring, *estimation*: the backlog of untested work. | the indicator now reads 2; it should reach 0 as checks are done | +| C5 | **The measures themselves.** `scripts/process-metrics.mjs`, and this record. | The review's fifth step - a baseline before anything else. | - | + +Also corrected in passing, because they were the same kind of drift: README's guard table had its +last row stranded outside the table and said "six static guards" when there were seven; +CONTRIBUTING told contributors to squash-merge where the repository rebase-merges, and to read +ARCHITECTURE.md "before editing App.jsx". + +## 4. Not changed this cycle, and why + +- **A requirements specification** - the review's first step, derived backwards from what is + built and then checked by the customer. Its last step is the customer's own, so it is a draft + to write next rather than something to finish here. +- **Outside feedback** and **DevOps readiness** need a decision and a server respectively; there + is nothing to measure yet. +- **A testing cadence.** How often deferred checks must be closed is the owner's call. C4 makes + the backlog countable, so whatever period is chosen can be checked rather than remembered. +- **Labelling every fix with an issue,** which would make MTTR mean something. It is a habit, not + something a gate can enforce cheaply - flagged here so the next cycle can see whether it took. + +## 5. Next measurement + +Rerun `node scripts/process-metrics.mjs --since 30` at the end of the next cycle and compare it +with the `last30` block of the baseline file. What should have happened, written down now so it +cannot be fitted to the result afterwards: + +- **fixes carrying a test** up from 60.4%, towards 100%. If it does not move, C1 is being waived + rather than followed, and the waivers in history will say so. +- **fixes of a recent fix** down from 25, a little later - a test stops a defect *returning*, not + appearing. +- **defect escape rate** is the slowest to respond for the same reason. A rise would not on its + own mean C1 failed. +- **CI pass ratio** may fall below 99.7% for a while, since it now runs ten checks rather than + four. A fall here is the gate catching what it used to let through. +- **awaiting verification** down from 2. diff --git a/docs/process/README.md b/docs/process/README.md new file mode 100644 index 00000000..7b88ad6d --- /dev/null +++ b/docs/process/README.md @@ -0,0 +1,41 @@ +# Process cycles + +How the way this project is built gets improved, and the record of each time it was. + +Each cycle is **Measure → Analyze → Change**, then measure again. The measuring comes first and +is not optional: a process change judged by feel cannot be told apart from one that did nothing, +and without a baseline the next cycle cannot know what to change either. It is also the cheapest +step - every figure comes out of records the repository already keeps. + +## Running a cycle + +```bash +node scripts/process-metrics.mjs # all of history +node scripts/process-metrics.mjs --since 30 # the last 30 days - compare this one +node scripts/process-metrics.mjs --json # for diffing against a saved baseline +``` + +Then write a new file here, named for the date - **a new file each cycle, never an edit to an old +one**, so the trend stays readable (the same rule as `docs/health/`). Save the `--json` output +beside it, so the next cycle compares numbers rather than prose. + +A record says what was measured, what the numbers meant, what was changed because of them and +which number each change is expected to move - so the next cycle can check. + +## What the measures are + +| measure | from | note | +|---|---|---| +| deployment frequency | merged PRs | every merge to main deploys to Vercel | +| lead time | PRs, issues | PR lead time is near zero because PRs open after the work; issue → close is the real one | +| automated test pass ratio | Actions runs of `ci.yml` | cancelled runs are superseded, not verdicts, and are left out | +| defect escape rate | `fix:` merges ÷ feat + perf + refactor merges | nothing lands without passing CI, so each fix is a defect that passed it | +| MTTR | bug-labelled issues, open → close | only as good as the labelling | +| fixes carrying a test | fix commits touching `test/` | the test-after rate, directly | +| risk indicators | issues, git history | deferred verification, reverts, fixes of a recent fix, stalls | +| defect hotspots | files touched by fix commits | where a test-first rule pays most | + +## Records + +- [2026-10-14](2026-10-14.md) — cycle 1. The baseline; CI found running four of its ten gates; + ARCHITECTURE.md missing 29% of the files; fixes required to carry a test. diff --git a/package.json b/package.json index f39ef468..2b1e4912 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "bench": "node --experimental-strip-types --no-warnings=ExperimentalWarning scripts/bench.mjs", "typecheck": "tsc --noEmit", "lint": "eslint src", - "check": "npm run typecheck && npm test && node scripts/hook-baseline.mjs && node scripts/helper-index.mjs && node scripts/unreachable.mjs && node scripts/unused-imports.mjs && node scripts/stroke-writes.mjs && node scripts/native-dialogs.mjs && node scripts/i18n-check.mjs && npm run build", + "check": "npm run typecheck && npm test && node scripts/hook-baseline.mjs && node scripts/helper-index.mjs && node scripts/unreachable.mjs && node scripts/unused-imports.mjs && node scripts/stroke-writes.mjs && node scripts/native-dialogs.mjs && node scripts/architecture-index.mjs && node scripts/fix-needs-test.mjs && node scripts/i18n-check.mjs && npm run build", "smoke": "node test/smoke/boot.mjs && node test/smoke/draw.mjs" }, "dependencies": { diff --git a/scripts/architecture-index.mjs b/scripts/architecture-index.mjs new file mode 100644 index 00000000..14648ffc --- /dev/null +++ b/scripts/architecture-index.mjs @@ -0,0 +1,56 @@ +// Keeps ARCHITECTURE.md's file map honest. +// +// The map is the first thing to read before editing - its own title says so - and on 2026-10-14 +// it was missing 34 of the 152 files under src/, 22% of the codebase. Nothing had gone wrong in +// any one change. Each file was added by a PR that had something else to do, and the map was +// never on anyone's list, so it fell further behind with every module: the same drift the +// process review (docs/process/) put down to incremental development itself, where producing a +// document for every version is never the cheapest next step. +// +// HELPERS.md had the same problem and solved it by being checked, and this is that rule applied +// to the map. Add a file under src/ and forget to say what it is, and the gate fails. It checks +// that the file is named, not that the sentence beside it is good - the same line HELPERS.md +// draws, and for the same reason: whether a description is right is a review question, whether +// it exists at all is not. +// +// A file counts as named when its filename appears in backticks - `layerOps.ts`, or a path that +// ends in it. Basenames under src/ are unique, which is what makes the filename alone enough. + +import { readFileSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { pathToFileURL } from 'node:url'; + +const MAP = 'ARCHITECTURE.md'; + +/** + * The files the map does not name. + * + * @param {string} doc the map's text + * @param {string[]} files repository paths + * @returns {string[]} + */ +export function unnamed(doc, files) { + const named = new Set(); + for (const m of doc.matchAll(/`([^`\n]+)`/g)) { + // A backtick span can hold several names ("`a.ts` / `b.ts`" is two spans, but + // "`frameExport.ts`/`exportQueue.ts`" and a path like `src/core/x.ts` are common too). + for (const part of m[1].split(/[\s,/]+/)) if (/\.(tsx?|d\.ts)$/.test(part)) named.add(part); + } + return files.filter(f => !named.has(f.split('/').pop())); +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const files = execFileSync('git', ['ls-files', 'src'], { encoding: 'utf8' }) + .split('\n').filter(f => /\.(ts|tsx)$/.test(f)); + const missing = unnamed(readFileSync(MAP, 'utf8'), files); + if (missing.length) { + console.error(`${missing.length} file(s) under src/ that ${MAP} does not name:`); + for (const f of missing) console.error(` ${f}`); + console.error(` +${MAP} is read before editing, and a map that is missing a fifth of the territory teaches +people to stop reading it. Add a line under the file's folder saying what it is for - one +sentence is enough; the name alone is what this checks.`); + process.exit(1); + } + console.log(`Architecture map passed - all ${files.length} files under src/ are named in ${MAP}`); +} diff --git a/scripts/commitKind.mjs b/scripts/commitKind.mjs new file mode 100644 index 00000000..7ad94daf --- /dev/null +++ b/scripts/commitKind.mjs @@ -0,0 +1,13 @@ +// The conventional-commit type a subject line opens with: feat, fix, perf, refactor, ... +// +// Shared by the process metrics and the fix-needs-a-test gate, which must agree on what counts as +// a fix - if they did not, the gate could pass a change the metrics then count as an untested fix. + +/** + * @param {string} subject a commit subject or PR title + * @returns {string} the type in lower case, or 'other' when there is no prefix + */ +export function kindOf(subject) { + const m = /^(\w+)(\(.+?\))?!?:/.exec(String(subject || '').trim()); + return m ? m[1].toLowerCase() : 'other'; +} diff --git a/scripts/fix-needs-test.mjs b/scripts/fix-needs-test.mjs new file mode 100644 index 00000000..9c89213f --- /dev/null +++ b/scripts/fix-needs-test.mjs @@ -0,0 +1,68 @@ +// A fix has to bring the test that would have caught it. +// +// Measured on 2026-10-14 (docs/process/): 56% of all fix commits in this repository had landed +// without touching a test, and 40% of the last month's still did. A fix with no test is the +// cleanest case of test-after development - nothing stops the same defect coming back - and the +// process review put most of the semantic bugs that pass CI down to exactly that. +// +// Full test-first would be the textbook answer and is not realistic here. This is the targeted +// version the review asked for: the place where a test is cheapest to write and worth most is +// the moment a defect has just been understood, so that is where it is required. +// +// The rule: if a branch carries a `fix:` commit, the branch must change something under test/. +// A fix that genuinely cannot be tested - a stylesheet, a config value - says so with a trailer +// in its commit message, and the reason stays in history next to the change: +// +// No-Test: colour only; nothing here a test can observe +// +// Compared against origin/main, so it checks a branch and not the whole history. On main itself, +// or with no origin/main to compare against, there is nothing to check and it passes. + +import { execFileSync } from 'node:child_process'; +import { pathToFileURL } from 'node:url'; +import { kindOf } from './commitKind.mjs'; + +/** + * Whether a branch's fixes carry a test. + * + * @param {Array<{subject: string, body?: string}>} commits the branch's commits + * @param {string[]} files every path the branch changes + * @returns {{ok: boolean, why: string, fixes?: string[]}} + */ +export function verdict(commits, files) { + const fixes = commits.filter(c => kindOf(c.subject) === 'fix'); + if (!fixes.length) return { ok: true, why: 'no fix commits on this branch' }; + if (files.some(f => f.startsWith('test/'))) return { ok: true, why: `${fixes.length} fix commit(s), with a test` }; + for (const c of commits) { + const m = /^No-Test:[ \t]*(\S.*)$/mi.exec(c.body || ''); + if (m) return { ok: true, why: `waived - ${m[1].trim()}` }; + } + return { ok: false, why: 'fix without a test', fixes: fixes.map(c => c.subject) }; +} + +const git = (...a) => execFileSync('git', a, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + let base; + try { base = git('merge-base', 'origin/main', 'HEAD').trim(); } catch { + console.log('Fix-needs-a-test skipped - no origin/main to compare against.'); + process.exit(0); + } + const SEP = '\u001e'; + const commits = git('log', '--no-merges', `--format=%s%x1f%b${SEP}`, `${base}..HEAD`) + .split(SEP).map(x => x.trim()).filter(Boolean) + .map(x => { const [subject, body = ''] = x.split('\u001f'); return { subject, body }; }); + const files = git('diff', '--name-only', `${base}...HEAD`).split('\n').filter(Boolean); + const v = verdict(commits, files); + if (!v.ok) { + console.error(`This branch fixes something without changing a test:`); + for (const s of v.fixes) console.error(` ${s}`); + console.error(` +Add the test that fails without the fix - that is what keeps the defect from coming back. If +nothing here can be observed by a test, say why with a trailer in the commit message: + + No-Test: `); + process.exit(1); + } + console.log(`Fix-needs-a-test passed - ${v.why}.`); +} diff --git a/scripts/process-metrics.mjs b/scripts/process-metrics.mjs new file mode 100644 index 00000000..daeca130 --- /dev/null +++ b/scripts/process-metrics.mjs @@ -0,0 +1,214 @@ +// Process metrics, read straight out of the repository's own records. +// +// Until this script, "it is fast" and "it got more stable" were judged by feel. A process change +// can only be called an improvement against a number taken before it, so this produces that +// number - the baseline in a Measure -> Analyze -> Change cycle (docs/process/). +// +// Every figure comes from what GitHub and git already record: merged PRs, issues, Actions runs +// and commit history. Nothing to install and nothing to keep in sync, so it can be rerun at the +// end of each cycle and compared line by line. +// +// node scripts/process-metrics.mjs all of history +// node scripts/process-metrics.mjs --since 30 the last 30 days only +// node scripts/process-metrics.mjs --json machine-readable, for diffing two runs +// +// Needs the gh CLI, authenticated against this repository. + +import { execFileSync } from 'node:child_process'; +import { kindOf } from './commitKind.mjs'; + +const args = process.argv.slice(2); +const sinceDays = args.includes('--since') ? Number(args[args.indexOf('--since') + 1]) : null; +const asJson = args.includes('--json'); +const now = Date.now(); +const DAY = 86_400_000; +const since = sinceDays ? now - sinceDays * DAY : 0; + +const gh = (...a) => JSON.parse(execFileSync('gh', a, { encoding: 'utf8', maxBuffer: 64 << 20 })); +const git = (...a) => execFileSync('git', a, { encoding: 'utf8', maxBuffer: 64 << 20 }); + +const t = (iso) => new Date(iso).getTime(); +const inWindow = (iso) => iso && t(iso) >= since; +const median = (xs) => { if (!xs.length) return null; const s = [...xs].sort((a, b) => a - b); const m = s.length >> 1; return s.length % 2 ? s[m] : (s[m - 1] + s[m]) / 2; }; +const pct = (xs, p) => { if (!xs.length) return null; const s = [...xs].sort((a, b) => a - b); return s[Math.min(s.length - 1, Math.floor(p * s.length))]; }; +const hours = (ms) => ms == null ? null : Math.round(ms / 36e5 * 10) / 10; +const days = (ms) => ms == null ? null : Math.round(ms / DAY * 10) / 10; + + + +// --- the records ------------------------------------------------------------------------------- + +// Paged by hand through GraphQL rather than `gh pr list --json commits`. That command asks for +// every commit's authors as well, gh pages at 100 PRs, and 100 x 100 x 100 is twice GitHub's +// 500,000-node ceiling - it fails at any --limit. Asking for the commit headline alone is 2,000. +function mergedPrs() { + const { owner, name } = gh('repo', 'view', '--json', 'owner,name'); + const q = `query($after: String) { repository(owner: "${owner.login}", name: "${name}") { + pullRequests(states: MERGED, first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { number title createdAt mergedAt author { login } + commits(first: 20) { nodes { commit { messageHeadline } } } } } } }`; + const out = []; + for (let after = null; ;) { + const page = gh('api', 'graphql', '-f', `query=${q}`, ...(after ? ['-f', `after=${after}`] : [])) + .data.repository.pullRequests; + for (const n of page.nodes) out.push({ ...n, commits: n.commits.nodes.map(c => c.commit) }); + if (!page.pageInfo.hasNextPage) return out; + after = page.pageInfo.endCursor; + } +} +const prs = mergedPrs().filter(p => inWindow(p.mergedAt)); + +// A PR's kind is its title's prefix when it has one. 55 titles were written as prose instead - +// the TypeScript migration series ("hooks/ in TypeScript") is most of them - and counting those +// as "other" shrank the denominator of the escape rate. Their commits did carry a prefix, and +// rebase-merge puts exactly those commits on main, so they are the better witness. +const kindOfPr = (p) => { + const k = kindOf(p.title); + if (k !== 'other') return k; + for (const c of p.commits || []) { const ck = kindOf(c.messageHeadline); if (ck !== 'other') return ck; } + return 'other'; +}; +const issues = gh('issue', 'list', '--state', 'all', '--limit', '2000', '--json', 'number,title,state,createdAt,closedAt,labels,body'); +const runs = gh('run', 'list', '--workflow', 'ci.yml', '--limit', '2000', '--json', 'conclusion,event,createdAt,headBranch') + .filter(r => inWindow(r.createdAt)); + +const human = prs.filter(p => !/dependabot/i.test(p.author?.login || '')); +const byKind = {}; +for (const p of human) (byKind[kindOfPr(p)] ??= []).push(p); +const count = (k) => (byKind[k] || []).length; + +// --- DevOps measures, as the course defines them ------------------------------------------------ + +// Deployment frequency. Every merge to main deploys to Vercel, so a merge is a deployment. +const spanDays = sinceDays ?? Math.max(1, (now - Math.min(...prs.map(p => t(p.mergedAt)))) / DAY); +const deployPerWeek = Math.round(prs.length / spanDays * 7 * 10) / 10; + +// Lead time, PR opened -> merged (deployed). The issue -> close figure is separate because many +// changes never had an issue: they were asked for and done in the same sitting. +const prLead = human.map(p => t(p.mergedAt) - t(p.createdAt)); +const closedIssues = issues.filter(i => i.closedAt && inWindow(i.closedAt)); +const issueLead = closedIssues.map(i => t(i.closedAt) - t(i.createdAt)); + +// Automated test pass ratio. Cancelled runs are left out: the workflow cancels a branch's older +// run when a newer push arrives, so a cancellation is a superseded run, not a verdict. +const judged = runs.filter(r => r.conclusion === 'success' || r.conclusion === 'failure'); +const passRatio = judged.length ? judged.filter(r => r.conclusion === 'success').length / judged.length : null; +const mainRuns = judged.filter(r => r.event === 'push' && r.headBranch === 'main'); +const mainFailures = mainRuns.filter(r => r.conclusion === 'failure').length; + +// Defect escape rate. Nothing reaches main without passing CI, so every `fix:` that lands is a +// defect that passed CI and shipped - an escape, by the definition in the course. Measured +// against the changes that can introduce one. A proxy, and stated as one: a fix for something +// that was never shipped would count too, and that is rare here because work lands as it goes. +const introducing = count('feat') + count('perf') + count('refactor'); +const escapeRate = introducing ? count('fix') / introducing : null; + +// MTTR. Time a bug-labelled issue stayed open. MTTD - from the defect being introduced to it +// being noticed - would need blame on every fix and is not attempted; see the cycle notes. +const bugs = closedIssues.filter(i => i.labels.some(l => l.name === 'bug')); +const mttr = bugs.map(i => t(i.closedAt) - t(i.createdAt)); + +// --- risk indicators, as named in the risk-monitoring section ----------------------------------- + +// Estimation: verification that has been put off. Unticked checklist items in open issues, plus +// issues explicitly waiting on a check on a device. +const open = issues.filter(i => i.state === 'OPEN'); +const unticked = open.reduce((n, i) => n + (String(i.body || '').match(/^\s*[-*] \[ \]/gm) || []).length, 0); +const awaitingCheck = open.filter(i => i.labels.some(l => l.name === 'needs-verification')).length; + +// Tools: the agent undoing its own work. Reverts, and fixes landing on a file that another fix +// touched within the previous week - a fix that needed a fix. +const sinceArg = sinceDays ? [`--since=${sinceDays}.days`] : []; +const log = git('log', '--no-merges', '--format=@@%H%x09%ct%x09%s', '--name-only', ...sinceArg); +const commits = []; +for (const chunk of log.split('@@').slice(1)) { + const [head, ...files] = chunk.trim().split('\n'); + const [hash, ct, subject] = head.split('\t'); + commits.push({ hash, at: Number(ct) * 1000, subject, files: files.filter(Boolean) }); +} +const reverts = commits.filter(c => /^revert/i.test(c.subject)).length; +const fixes = commits.filter(c => kindOf(c.subject) === 'fix'); +// App is left out of the overlap. Nearly every change wires through it, so two fixes sharing it +// says nothing about one fix needing another - counted in, this read 77 and would never move. +const overlapFile = (x) => x.startsWith('src/') && !/^src\/App\.(jsx|tsx)$/.test(x); +let refix = 0; +for (const f of fixes) { + if (fixes.some(g => g !== f && g.at < f.at && f.at - g.at < 7 * DAY && g.files.some(x => overlapFile(x) && f.files.includes(x)))) refix++; +} + +// Test-after, measured. The plan blames semantic defects on tests written after the code; a fix +// that lands without touching a test is the clearest case of it, because nothing then stops the +// same defect coming back. +const fixesWithTest = fixes.filter(f => f.files.some(x => x.startsWith('test/'))).length; + +// People: work stalling. The longest gap between two merges, and how long open issues have sat. +const mergeTimes = prs.map(p => t(p.mergedAt)).sort((a, b) => a - b); +let longestGap = 0; +for (let i = 1; i < mergeTimes.length; i++) longestGap = Math.max(longestGap, mergeTimes[i] - mergeTimes[i - 1]); +const openAge = open.map(i => now - t(i.createdAt)); + +// Where the defects are. Source files touched most often by fixes - the place a test-first rule +// buys the most, which is the targeted adoption the plan calls for. +// Two corrections, or the list says the wrong thing. The TypeScript migration renamed App.jsx to +// App.tsx, so one file's history was split across two names; the extension is dropped so they +// count as one. And the translation tables are left out: every fix that changes a UI string adds +// its translations, so they rank high for touching, not for breaking. +const NOISE = /^src\/i18n(\.\w+)?\.(js|ts)$|\.css$/; +const stem = (x) => x.replace(/\.(jsx|tsx|js|ts)$/, ''); +const hot = {}; +for (const f of fixes) for (const x of f.files) if (x.startsWith('src/') && !NOISE.test(x)) hot[stem(x)] = (hot[stem(x)] || 0) + 1; +const hotspots = Object.entries(hot).sort((a, b) => b[1] - a[1]).slice(0, 8); + +// --- report ------------------------------------------------------------------------------------ + +const result = { + window: sinceDays ? `last ${sinceDays} days` : 'all history', + measuredAt: new Date(now).toISOString().slice(0, 10), + merges: prs.length, + mergesByKind: Object.fromEntries(Object.entries(byKind).map(([k, v]) => [k, v.length]).sort((a, b) => b[1] - a[1])), + deploymentsPerWeek: deployPerWeek, + prLeadTimeHours: { median: hours(median(prLead)), p90: hours(pct(prLead, 0.9)) }, + issueLeadTimeDays: { median: days(median(issueLead)), p90: days(pct(issueLead, 0.9)), closed: closedIssues.length }, + ciPassRatio: passRatio == null ? null : Math.round(passRatio * 1000) / 10, + ciRunsJudged: judged.length, + mainBranchFailures: mainFailures, + defectEscapeRate: escapeRate == null ? null : Math.round(escapeRate * 1000) / 10, + fixMerges: count('fix'), + introducingMerges: introducing, + mttrDays: { median: days(median(mttr)), bugsClosed: bugs.length }, + fixCommitsWithATest: fixes.length ? Math.round(fixesWithTest / fixes.length * 1000) / 10 : null, + fixCommits: fixes.length, + risk: { + untickedChecklistItems: unticked, + issuesAwaitingVerification: awaitingCheck, + reverts, + fixesOfARecentFix: refix, + longestGapBetweenMergesDays: days(longestGap), + openIssues: open.length, + oldestOpenIssueDays: days(Math.max(0, ...openAge)), + }, + defectHotspots: hotspots, +}; + +if (asJson) { + console.log(JSON.stringify(result, null, 2)); +} else { + const r = result; + console.log(`Process metrics - ${r.window}, measured ${r.measuredAt}\n`); + console.log('DevOps measures'); + console.log(` deployment frequency ${r.deploymentsPerWeek} merges/week (${r.merges} merges; each deploys)`); + console.log(` lead time, PR -> deploy median ${r.prLeadTimeHours.median}h, p90 ${r.prLeadTimeHours.p90}h`); + console.log(` lead time, issue -> close median ${r.issueLeadTimeDays.median}d, p90 ${r.issueLeadTimeDays.p90}d (${r.issueLeadTimeDays.closed} issues)`); + console.log(` automated test pass ratio ${r.ciPassRatio}% of ${r.ciRunsJudged} CI runs; ${r.mainBranchFailures} failed on main`); + console.log(` defect escape rate ${r.defectEscapeRate}% (${r.fixMerges} fixes / ${r.introducingMerges} feat+perf+refactor)`); + console.log(` MTTR (bug issues) median ${r.mttrDays.median}d over ${r.mttrDays.bugsClosed} bugs`); + console.log(` fixes carrying a test ${r.fixCommitsWithATest}% of ${r.fixCommits} fix commits`); + console.log(` merges by kind ${Object.entries(r.mergesByKind).map(([k, v]) => `${k} ${v}`).join(', ')}`); + console.log('\nRisk indicators'); + console.log(` estimation unticked checklist items in open issues: ${r.risk.untickedChecklistItems}; awaiting verification: ${r.risk.issuesAwaitingVerification}`); + console.log(` tools reverts: ${r.risk.reverts}; fixes landing on a file another fix touched that week (App aside): ${r.risk.fixesOfARecentFix}`); + console.log(` people longest gap between merges: ${r.risk.longestGapBetweenMergesDays}d; open issues: ${r.risk.openIssues}, oldest ${r.risk.oldestOpenIssueDays}d`); + console.log('\nDefect hotspots (src files most touched by fixes)'); + for (const [f, n] of r.defectHotspots) console.log(` ${String(n).padStart(3)} ${f}`); +} diff --git a/test/scripts/architectureIndex.test.js b/test/scripts/architectureIndex.test.js new file mode 100644 index 00000000..4e535019 --- /dev/null +++ b/test/scripts/architectureIndex.test.js @@ -0,0 +1,33 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { unnamed } from '../../scripts/architecture-index.mjs'; + +// On 2026-10-14 the map was missing 44 of the 152 files under src/. These pin down what "named" +// means, because a loose reading is how the drift was undercounted in the first place: a grep for +// the bare word found 34 missing, since "zip" and "camera" appear in prose without the files +// ever being named. + +test('a file named in backticks is named', () => { + assert.deepEqual(unnamed('- `layerOps.ts` — layer tree', ['src/core/layerOps.ts']), []); +}); + +test('a file mentioned only as a word in prose is not named', () => { + // The export folder was described as "zip, gif, the recorder" and none of its files appeared. + assert.deepEqual(unnamed('export/ turning the timeline into a file: zip, gif, the recorder.', ['src/export/zip.ts']), ['src/export/zip.ts']); +}); + +test('several names in one span, and paths, all count', () => { + const doc = '- `frameExport.ts` / `exportQueue.ts` — sizes\n- `src/App.tsx` — the component\n`a.ts/b.ts`'; + assert.deepEqual(unnamed(doc, ['src/core/frameExport.ts', 'src/core/exportQueue.ts', 'src/App.tsx', 'src/x/a.ts', 'src/x/b.ts']), []); +}); + +test('a declaration file is named by its full name', () => { + assert.deepEqual(unnamed('- `src/globals.d.ts` — vendor globals', ['src/globals.d.ts']), []); + assert.deepEqual(unnamed('- `globals` — vendor globals', ['src/globals.d.ts']), ['src/globals.d.ts']); +}); + +test('the extension is part of the name', () => { + // `App.jsx` in an old line does not cover App.tsx - that is exactly the stale reference the + // map is supposed to stop carrying. + assert.deepEqual(unnamed('- `App.jsx`', ['src/App.tsx']), ['src/App.tsx']); +}); diff --git a/test/scripts/fixNeedsTest.test.js b/test/scripts/fixNeedsTest.test.js new file mode 100644 index 00000000..d654ac8b --- /dev/null +++ b/test/scripts/fixNeedsTest.test.js @@ -0,0 +1,55 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { verdict } from '../../scripts/fix-needs-test.mjs'; +import { kindOf } from '../../scripts/commitKind.mjs'; + +// Measured before this gate existed: 56% of all fix commits, and 40% of the last month's, landed +// without touching a test. The cases below are the ones that decide whether it actually closes +// that gap or only looks like it does. + +const fix = (subject, body = '') => ({ subject, body }); + +test('a fix with no test is refused', () => { + const v = verdict([fix('fix: the pencil keeps answering pressure')], ['src/canvas/strokes.ts']); + assert.equal(v.ok, false); + assert.deepEqual(v.fixes, ['fix: the pencil keeps answering pressure']); +}); + +test('a fix that changes a test passes', () => { + const v = verdict([fix('fix: seek off the frame boundary')], ['src/canvas/videoFrames.ts', 'test/canvas/canvasHelpers.test.js']); + assert.equal(v.ok, true); +}); + +test('a branch with no fix on it is not this gate\'s business', () => { + assert.equal(verdict([fix('feat: drag the frame guide'), fix('docs: the map')], ['src/App.tsx']).ok, true); + assert.equal(verdict([], []).ok, true, 'main itself, or a branch with nothing on it'); +}); + +test('a scoped or breaking fix still counts as a fix', () => { + // Otherwise `fix(export):` would walk straight past the rule. + assert.equal(verdict([fix('fix(export): ask for mp4 by its real name')], ['src/export/recorder.ts']).ok, false); + assert.equal(verdict([fix('fix!: change the save format')], ['src/core/projectFormat.ts']).ok, false); +}); + +test('a waiver passes, and only with a reason', () => { + const waived = verdict([fix('fix: the panel colour', 'Slightly darker.\n\nNo-Test: colour only; nothing a test can observe')], ['src/styles/panel.css']); + assert.equal(waived.ok, true); + assert.match(waived.why, /colour only/); + // An empty trailer is not a reason. It would let the waiver become a habit with nothing in + // history saying why each one was allowed. + assert.equal(verdict([fix('fix: x', 'No-Test:')], ['src/a.ts']).ok, false); + assert.equal(verdict([fix('fix: x', 'No-Test: ')], ['src/a.ts']).ok, false); +}); + +test('a file merely named like a test does not count', () => { + // Only a change under test/ is a test. A source file with "test" in its name is not one. + assert.equal(verdict([fix('fix: x')], ['src/core/testHelpers.ts', 'scripts/smoke-test.mjs']).ok, false); +}); + +test('the gate and the metrics agree on what a fix is', () => { + // They share kindOf. If they drifted, the gate could pass a change the metrics then count + // as an untested fix, and the number this gate exists to move would stop meaning anything. + for (const [s, k] of [['fix: a', 'fix'], ['fix(scope): a', 'fix'], ['Fix: a', 'fix'], ['feat: a', 'feat'], ['hooks/ in TypeScript', 'other']]) { + assert.equal(kindOf(s), k, s); + } +});