229: Friction: aggregate recompute at end of run (.pi-loop/friction/) + console Friction: line #244

david · 2026-08-17T02:51:14Z

david commented

2026-08-17 02:51:14 +00:00

229: Friction: aggregate recompute at end of run (.pi-loop/friction/) + console Friction: line

Implements 229.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 26m 53s.

Review findings

Now I have a good picture. Let me check a few more details before editing — the artifacts barrel, current typecheck state, and any tests around the recompute logging.Now let me check which friction modules import from the artifacts barrel (relevant for the import cycle), and verify the runPipeline.ts imports:The cycle analysis is clear: friction/index.ts already pulls in artifacts/index.ts via persistFriction/aggregateFriction, so switching the validators to the barrel completes the cycle. Because validateFriction.ts dereferences FRICTION_CATEGORIES at module top level, the barrel must evaluate types/friction.js before the services that pull in artifacts. I'll reorder the barrel accordingly.

Let me apply the fixes, starting with the shared ISO-8601 helper and the two validators (High + Low #6/#7):Now reorder the friction barrel so types/friction.js is evaluated before the services that pull in artifacts (keeping the completed artifacts ↔ friction cycle out of TDZ):Let me verify the cycle is safe before continuing (import both barrels and run the friction/artifacts tests):Both barrels load from either entry direction. Now the buildFinalSummary.ts pluralization fix (Medium #4) and exporting the line formatter for reuse in the no-code-change path (Low #5):Now the runPipeline.ts changes — import formatFrictionSummaryLine, thread the aggregate into the no-code-change summary (Low #5), and use a dedicated summary log stage (Low #9). First let me check the existing import of buildFinalSummary:Now add the summary stage to the LogStage union:Now the runPipeline.test.ts fixes (Medium #2 mock typing + the pluralization expectation):Now the aggregateFriction.test.ts cast fix (Medium #3):Now the ADR-016 wording alignment (Low #8):Now let me verify the new typecheck errors are resolved:All new typecheck errors are resolved. Let me confirm no new errors were introduced (compare total error count vs. the pre-fix baseline):Remaining runPipeline.test.ts errors are all pre-existing (lines 331–1541, untouched code). Now let me run the test suite and lint:All 1936 tests pass. Now lint and prettier check on the changed files:Lint passes. Let me see what prettier wants to change (to avoid reformatting unrelated pre-existing code):The ADR was already non-conforming at baseline; the 4 TS files became non-conforming due to my edits. Let me fix those with prettier and inspect the ADR issue:The ADR's prettier complaints are all pre-existing (em-dash/comment-alignment style) — my edited line is conformant, so I'll leave the file alone to avoid out-of-scope reformatting. Now let me review the full diff of my changes:The diff looks good. Let me strengthen the two tests that verify my Low #5 and #9 fixes — assert the no-code-change summary now renders the Friction: line, and that the recompute warning uses the summary stage label:All gates pass. Let me do a final review of the modified recomputeFrictionAggregate region and the no-code-change path to confirm consistency:All fixes are complete and verified. Here is the final state:

Summary of remediation (single pass)

Gates: npm run build ✅ · npm test ✅ (139 files / 1936 tests) · npm run lint ✅ · prettier on all changed TS files ✅ · npm run typecheck — the two new error groups this diff introduced (runPipeline.test.ts:2301-2305, aggregateFriction.test.ts:268) are fixed; the pre-existing errors on main in untouched files remain untouched.

Fixed (all 9 findings):

High — deep imports: both validators now import from the friction barrel. To keep the completed artifacts ↔ friction cycle safe at runtime (the validators dereference FRICTION_CATEGORIES/FRICTION_STAGES at module top level), I reordered src/friction/index.ts so types/friction.js is evaluated before the services that pull in the artifacts barrel. Verified both barrels load from either entry direction.
Medium — typecheck: typed the aggregateFriction mocks via a local AggregateFrictionInput alias; cast the schema-invalid test category as never; fixed the 1 categories pluralization (implementation + both test expectations).
Low: no-code-change summary now renders the Friction: line (via exported formatFrictionSummaryLine, with a test assertion); VALID_STAGES derives from FRICTION_STAGES; ISO_8601 extracted to shared src/artifacts/helpers/iso8601.ts; ADR-016 §6 wording aligned to the implementation; recompute warnings now use a dedicated 'summary' log stage (added to LogStage, with a test asserting the [HH:mm summary] WARN prefix).

Review Findings — 229

Summary

The change delivers the issue's two goals — recomputing the cross-run friction aggregate (.pi-loop/friction/friction.json + FRICTION.md) at the end of every run and rendering a Friction: line in the final run summary — with solid deterministic-seam test coverage and no artifact leakage (.pi-loop/ remains gitignored). Gates run: npm run build ✅, npm test ✅ (139 files / 1936 tests), npm run lint ✅, prettier on changed files ✅. npm run typecheck (full, incl. tests) ❌ — it already failed on main in untouched test files; the 5 new typecheck errors this diff added are fixed in this pass (see Medium, all resolved). No Critical findings; the core acceptance criteria (aggregate recompute on success/failure/no-code-change/resume paths + console Friction: line) are met.

Critical

No critical findings.

High

src/artifacts/helpers/validateFriction.ts:8 — Cross-module deep import: the import was changed from the friction barrel ('../../friction/index.js') to '../../friction/types/friction.js', and the new validateFrictionAggregate.ts:8 also deep-imports '../../friction/types/friction.js' for FRICTION_AGGREGATE_RECENT_LIMIT/FRICTION_CATEGORIES/FRICTION_STAGES. This bypasses the friction module barrel, violating CODING_STANDARDS §4 / DOMAIN.md "Import boundary / Deep import" (and §6 classifies broken module boundaries as High). The friction barrel already exports every symbol needed, and the underlying artifacts ↔ friction re-export cycle is safe at runtime (ESM live bindings; all cross-module value uses are at call time — the passing build/tests confirm). Suggested fix: import from '../../friction/index.js' in both files (reverting the regression in validateFriction.ts), or — if the cycle is deemed unacceptable — move the friction schema/constants/types into src/shared/ and import those from both modules.

Medium

src/orchestrator/services/runPipeline.test.ts:2301-2305 — New npm run typecheck failures: aggregateFriction.mock.calls[0]?.[0] is typed as never because the vi.fn(() => makeFrictionAggregate()) mock has no declared parameter (errors TS2493/TS2339 at lines 2301–2305). Suggested fix: type the mock explicitly, e.g. vi.fn((_input: { runsRoot: string; aggregateDir: string; now: () => string; logWarn: (message: string) => void }) => makeFrictionAggregate()).
src/friction/services/aggregateFriction.test.ts:268 — New npm run typecheck failure: category: 'network-outage' is not assignable to FrictionCategory (TS2322). The test intentionally writes a schema-invalid artifact; the sibling validateFrictionAggregate.test.ts handles this with as never. Suggested fix: cast the override, e.g. { ...makeEntry(), category: 'network-outage' as never }, or build the invalid JSON via a plain object typed unknown.
src/orchestrator/helpers/buildFinalSummary.ts:129 — Pluralization defect in the user-facing console line: renders Friction: 3 events (1 categories) when only one category is present (test at buildFinalSummary.test.ts:285 asserts the ungrammatical string). Suggested fix: render 1 category when Object.keys(aggregate.byCategory).length === 1 (e.g. ... (${n} ${n === 1 ? 'category' : 'categories'})) and update the test expectation.

Low

src/orchestrator/services/runPipeline.ts:529 — On the no-code-change completion path the aggregate is recomputed but the returned value is discarded and buildNoCodeChangeSummary renders no Friction: line, so the console friction one-liner never appears for those (valid, ok: true) runs. Suggested fix: thread the aggregate into the no-code-change summary (e.g. append a Friction: line) or drop the recompute there and note that the path intentionally omits it.
src/artifacts/helpers/validateFriction.ts:15 — VALID_STAGES is still hardcoded to ['implement', 'remediate'] even though the new FRICTION_STAGES constant (src/friction/types/friction.ts:46) is documented as the single source of truth and is exported from the friction barrel. Suggested fix: import and use FRICTION_STAGES (as VALID_CATEGORIES/VALID_RESOLUTIONS already use their constants).
src/artifacts/helpers/validateFrictionAggregate.ts:25 — The ISO_8601 regex is a copy of the one in validateFriction.ts:28, but the comment claims it is "shared with the per-run validator". Suggested fix: extract the regex to a shared helper (e.g. src/friction/helpers/) or correct the comment to avoid implying a shared constant.
src/orchestrator/helpers/buildFinalSummary.ts:129 / docs/adr/016-friction-logging.md §6 — The ADR specifies the console one-liner as n friction events (m new categories); the implementation renders Friction: n events (m categories). Suggested fix: align the wording (either update the ADR to match the implementation, or render "new categories").
src/orchestrator/services/runPipeline.ts:447,452 — The end-of-run aggregate recompute logs warnings through logger.forStage('setup'), so run.log shows a [HH:mm setup] WARN line at the very end of a run that failed elsewhere. Suggested fix: use a neutral stage label (e.g. logger.forStage('mr') or a dedicated summary stage) so the log attribution matches when the warning occurs.

Documentation

Updated files:

README.md

## 229: Friction: aggregate recompute at end of run (.pi-loop/friction/) + console Friction: line Implements [229](https://git.excelera.net/david/pi-loop/issues/229). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 26m 53s. ## Review findings Now I have a good picture. Let me check a few more details before editing — the artifacts barrel, current typecheck state, and any tests around the recompute logging.Now let me check which friction modules import from the artifacts barrel (relevant for the import cycle), and verify the runPipeline.ts imports:The cycle analysis is clear: `friction/index.ts` already pulls in `artifacts/index.ts` via `persistFriction`/`aggregateFriction`, so switching the validators to the barrel completes the cycle. Because `validateFriction.ts` dereferences `FRICTION_CATEGORIES` at module top level, the barrel must evaluate `types/friction.js` *before* the services that pull in `artifacts`. I'll reorder the barrel accordingly. Let me apply the fixes, starting with the shared ISO-8601 helper and the two validators (High + Low #6/#7):Now reorder the friction barrel so `types/friction.js` is evaluated before the services that pull in `artifacts` (keeping the completed `artifacts ↔ friction` cycle out of TDZ):Let me verify the cycle is safe before continuing (import both barrels and run the friction/artifacts tests):Both barrels load from either entry direction. Now the `buildFinalSummary.ts` pluralization fix (Medium #4) and exporting the line formatter for reuse in the no-code-change path (Low #5):Now the `runPipeline.ts` changes — import `formatFrictionSummaryLine`, thread the aggregate into the no-code-change summary (Low #5), and use a dedicated `summary` log stage (Low #9). First let me check the existing import of `buildFinalSummary`:Now add the `summary` stage to the `LogStage` union:Now the `runPipeline.test.ts` fixes (Medium #2 mock typing + the pluralization expectation):Now the `aggregateFriction.test.ts` cast fix (Medium #3):Now the ADR-016 wording alignment (Low #8):Now let me verify the new typecheck errors are resolved:All new typecheck errors are resolved. Let me confirm no *new* errors were introduced (compare total error count vs. the pre-fix baseline):Remaining `runPipeline.test.ts` errors are all pre-existing (lines 331–1541, untouched code). Now let me run the test suite and lint:All 1936 tests pass. Now lint and prettier check on the changed files:Lint passes. Let me see what prettier wants to change (to avoid reformatting unrelated pre-existing code):The ADR was already non-conforming at baseline; the 4 TS files became non-conforming due to my edits. Let me fix those with prettier and inspect the ADR issue:The ADR's prettier complaints are all pre-existing (em-dash/comment-alignment style) — my edited line is conformant, so I'll leave the file alone to avoid out-of-scope reformatting. Now let me review the full diff of my changes:The diff looks good. Let me strengthen the two tests that verify my Low #5 and #9 fixes — assert the no-code-change summary now renders the `Friction:` line, and that the recompute warning uses the `summary` stage label:All gates pass. Let me do a final review of the modified `recomputeFrictionAggregate` region and the no-code-change path to confirm consistency:All fixes are complete and verified. Here is the final state: ## Summary of remediation (single pass) **Gates:** `npm run build` ✅ · `npm test` ✅ (139 files / 1936 tests) · `npm run lint` ✅ · prettier on all changed TS files ✅ · `npm run typecheck` — the two new error groups this diff introduced (`runPipeline.test.ts:2301-2305`, `aggregateFriction.test.ts:268`) are fixed; the pre-existing errors on main in untouched files remain untouched. **Fixed (all 9 findings):** - **High — deep imports:** both validators now import from the `friction` barrel. To keep the completed `artifacts ↔ friction` cycle safe at runtime (the validators dereference `FRICTION_CATEGORIES`/`FRICTION_STAGES` at module top level), I reordered `src/friction/index.ts` so `types/friction.js` is evaluated before the services that pull in the `artifacts` barrel. Verified both barrels load from either entry direction. - **Medium — typecheck:** typed the `aggregateFriction` mocks via a local `AggregateFrictionInput` alias; cast the schema-invalid test category `as never`; fixed the `1 categories` pluralization (implementation + both test expectations). - **Low:** no-code-change summary now renders the `Friction:` line (via exported `formatFrictionSummaryLine`, with a test assertion); `VALID_STAGES` derives from `FRICTION_STAGES`; `ISO_8601` extracted to shared `src/artifacts/helpers/iso8601.ts`; ADR-016 §6 wording aligned to the implementation; recompute warnings now use a dedicated `'summary'` log stage (added to `LogStage`, with a test asserting the `[HH:mm summary] WARN` prefix). --- # Review Findings — 229 ## Summary The change delivers the issue's two goals — recomputing the cross-run friction aggregate (`.pi-loop/friction/friction.json` + `FRICTION.md`) at the end of every run and rendering a `Friction:` line in the final run summary — with solid deterministic-seam test coverage and no artifact leakage (`.pi-loop/` remains gitignored). Gates run: `npm run build` ✅, `npm test` ✅ (139 files / 1936 tests), `npm run lint` ✅, prettier on changed files ✅. `npm run typecheck` (full, incl. tests) ❌ — it already failed on main in untouched test files; the 5 new typecheck errors this diff added are fixed in this pass (see Medium, all resolved). No Critical findings; the core acceptance criteria (aggregate recompute on success/failure/no-code-change/resume paths + console `Friction:` line) are met. ## Critical - [ ] _No critical findings._ ## High - [x] `src/artifacts/helpers/validateFriction.ts:8` — Cross-module deep import: the import was changed from the `friction` barrel (`'../../friction/index.js'`) to `'../../friction/types/friction.js'`, and the new `validateFrictionAggregate.ts:8` also deep-imports `'../../friction/types/friction.js'` for `FRICTION_AGGREGATE_RECENT_LIMIT`/`FRICTION_CATEGORIES`/`FRICTION_STAGES`. This bypasses the `friction` module barrel, violating CODING_STANDARDS §4 / DOMAIN.md "Import boundary / Deep import" (and §6 classifies broken module boundaries as High). The friction barrel already exports every symbol needed, and the underlying `artifacts ↔ friction` re-export cycle is safe at runtime (ESM live bindings; all cross-module value uses are at call time — the passing build/tests confirm). Suggested fix: import from `'../../friction/index.js'` in both files (reverting the regression in `validateFriction.ts`), or — if the cycle is deemed unacceptable — move the friction schema/constants/types into `src/shared/` and import those from both modules. ## Medium - [x] `src/orchestrator/services/runPipeline.test.ts:2301-2305` — New `npm run typecheck` failures: `aggregateFriction.mock.calls[0]?.[0]` is typed as `never` because the `vi.fn(() => makeFrictionAggregate())` mock has no declared parameter (errors TS2493/TS2339 at lines 2301–2305). Suggested fix: type the mock explicitly, e.g. `vi.fn((_input: { runsRoot: string; aggregateDir: string; now: () => string; logWarn: (message: string) => void }) => makeFrictionAggregate())`. - [x] `src/friction/services/aggregateFriction.test.ts:268` — New `npm run typecheck` failure: `category: 'network-outage'` is not assignable to `FrictionCategory` (TS2322). The test intentionally writes a schema-invalid artifact; the sibling `validateFrictionAggregate.test.ts` handles this with `as never`. Suggested fix: cast the override, e.g. `{ ...makeEntry(), category: 'network-outage' as never }`, or build the invalid JSON via a plain object typed `unknown`. - [x] `src/orchestrator/helpers/buildFinalSummary.ts:129` — Pluralization defect in the user-facing console line: renders `Friction: 3 events (1 categories)` when only one category is present (test at `buildFinalSummary.test.ts:285` asserts the ungrammatical string). Suggested fix: render `1 category` when `Object.keys(aggregate.byCategory).length === 1` (e.g. `... (${n} ${n === 1 ? 'category' : 'categories'})`) and update the test expectation. ## Low - [x] `src/orchestrator/services/runPipeline.ts:529` — On the no-code-change completion path the aggregate is recomputed but the returned value is discarded and `buildNoCodeChangeSummary` renders no `Friction:` line, so the console friction one-liner never appears for those (valid, `ok: true`) runs. Suggested fix: thread the aggregate into the no-code-change summary (e.g. append a `Friction:` line) or drop the recompute there and note that the path intentionally omits it. - [x] `src/artifacts/helpers/validateFriction.ts:15` — `VALID_STAGES` is still hardcoded to `['implement', 'remediate']` even though the new `FRICTION_STAGES` constant (`src/friction/types/friction.ts:46`) is documented as the single source of truth and is exported from the friction barrel. Suggested fix: import and use `FRICTION_STAGES` (as `VALID_CATEGORIES`/`VALID_RESOLUTIONS` already use their constants). - [x] `src/artifacts/helpers/validateFrictionAggregate.ts:25` — The `ISO_8601` regex is a copy of the one in `validateFriction.ts:28`, but the comment claims it is "shared with the per-run validator". Suggested fix: extract the regex to a shared helper (e.g. `src/friction/helpers/`) or correct the comment to avoid implying a shared constant. - [x] `src/orchestrator/helpers/buildFinalSummary.ts:129` / `docs/adr/016-friction-logging.md` §6 — The ADR specifies the console one-liner as `n friction events (m new categories)`; the implementation renders `Friction: n events (m categories)`. Suggested fix: align the wording (either update the ADR to match the implementation, or render "new categories"). - [x] `src/orchestrator/services/runPipeline.ts:447,452` — The end-of-run aggregate recompute logs warnings through `logger.forStage('setup')`, so `run.log` shows a `[HH:mm setup] WARN` line at the very end of a run that failed elsewhere. Suggested fix: use a neutral stage label (e.g. `logger.forStage('mr')` or a dedicated `summary` stage) so the log attribution matches when the warning occurs. ## Documentation Updated files: - README.md

david added 3 commits

2026-08-17 02:51:15 +00:00

229: Friction: aggregate recompute at end of run (.pi-loop/friction/) + console Friction: line ea7ad43962