fix(observability): pool the quality friction denominator by scope count - #176
Conversation
quality_iters is a run-wide aggregate (once per task plus once per PR-fix round) but was compared against a single loop's cap (MAX_QUALITY_ITERATIONS), so a multi-task issue that cleared quality on iteration 2 of every task could saturate to 1.0 -- indistinguishable from an issue whose every quality loop actually exhausted the cap. Add a quality_scopes counter (incremented once per runQualityLoop invocation, not per iteration) and pool it into computeFriction's quality-stage cap via a new multiScopeField map, which also documents task_review_attempts and browser_iters as multi-scope aggregates not yet counted this way. A metrics blob with no quality_scopes field falls back to scopes=1, so pre-existing data scores unchanged. Corrects computeFriction's header comment, which claimed a uniform min(1, iters/cap) ratio across all seven stages and that a first-pass clean run scores 0 regardless of stage count -- both false given every stage field is written with += 1 semantics, so a first-try pass has always cost 1/cap. Refs #165
…ts, and per-call scope counter tests/friction.test.js: multi-scope all-capped saturates at 1.0, the dilution case (6 iters / 3 scopes) scores 0.4 instead of saturating, all-first-iteration (3/3) scores 0.2 with a comment recording that both sanctioned formulas agree on 0.2 (not the acceptance criterion's unsatisfiable literal "0"), single-scope behavior is unchanged, a metrics blob with no quality_scopes key scores as scopes=1, appending a first-try scope deliberately lowers the ratio (5/5 -> 6/10), a saturating fixture proves value/cap can exceed 1 while contribution clamps to it, scopes stays null on task-review/browser drivers, and both driver invariants (stage: contribution === min(1, value/cap) and cap === baseCap*(scopes ?? 1); signal: value*weight === contribution, no cap/scopes keys) hold across a mixed fixture. tests/compose-friction-churn.test.js: the :113 pass-through test gains a multi-scope quality fixture so the new driver keys survive composition unchanged. tests/quality-loop.test.js: quality_scopes assertions added to the existing two-calls, fully-capped-loop, and STOP-tripped scenarios. tests/harness.test.js: a new parity test regex-reads the metrics literal out of workflows/ticketmill.js and asserts its key set equals Object.keys(harness.freshMetrics()), giving the hand-sync contract teeth. Refs #165
Extends the quality-gate section of docs/architecture/gate-hygiene.md with quality_scopes as quality_iters' companion invocation counter, and adds "The friction denominator: pooled, not worst-scope" covering why pooled (mean) was chosen over worst-scope (max) — a max relocates this issue's opening complaint rather than fixing it — the accepted non-monotonicity cost and why dilution is safe (cap exhaustion is carried by ctx.quality_caps and gate_findings disposition, not quality_degrades), acceptance criterion 1's literal "scores 0" being unmet by both sanctioned formulas (both score 0.2), the corrected driver invariant pair, and why the other six stage drivers carry scopes: null rather than 1. docs/architecture/metrics.md is hash-frozen and stays byte-for-byte unmodified; a third provenance entry supersedes metrics.md:13-14 (now true of six stages, not seven) and notes metrics.md:15-17's "scores 0" claim was already inaccurate independent of this change. CLAUDE.md gains the matching frozen-passage paragraph, copied verbatim to AGENTS.md per the freeze-pair convention. index.md and CLAUDE.md's What-lives-here row get a refreshed gate-hygiene.md parenthetical. Issue #165.
Spec Review (Iteration 1)Verdict: approved Scoped diff (against Verified against the issue:
No engine-owned-path violations, no untracked task artifacts, no reopening of settled approach/plan adjudications. |
Code Review (Iteration 1)Verdict: changes_requested — no blocking findings. Two cheap, concrete minors below that would otherwise go unowned, one deferred follow-up, one optional nit. The change itself is correct, well tested, and lockstepped. Validation baseline (all green)
What I verified above that line
Findings1. Minor — The comment reads 2. Minor — PR body, "Acceptance criteria covered": criterion 1 is listed as covered when the shipped behavior is Seconding the Spec Review's nit rather than re-opening it — I agree with its substance and its severity, and I am not re-litigating the adjudicated formula. I flag it only because the recommended edit is still outstanding and currently belongs to no stage. The bullet reproduces the criterion verbatim ( 3. Deferred (not a change request — filed here so it has a written home) —
4. Nit (optional) — Not flagged, deliberatelyThe pooled-vs-worst-scope choice, the |
PR Review Fix (iteration 1)Commit: 8ee459c (no new code commit — code review raised no blocking findings; only the spec-review nit applied) Fixes applied:
|
Spec Review (Iteration 2)Verdict: approved Re-verified the scoped diff against the correct merge base ( Re-checked against the issue's own text, not just the prior review's summary:
Not my lane, noted for completeness only: the code review's Minor 1 (an engine comment at No engine-owned-path violations, no untracked task artifacts, no reopening of settled approach/plan adjudications. Consistent with the iteration-1 spec-review approval; nothing here reverses it. |
Code Review (Iteration 2)Verdict: changes_requested — one finding, comment-only, ~2 lines plus a Validation baseline (all green)
Finding1. Major (comment-only) — (a) New in this diff, not previously flagged — the module header at
Both halves are false, and verifiably so from this repo:
This is not a nit about phrasing. The claim "six of the seven stages run at most once per issue, so their denominator is that single loop's cap" asserts that the bug this PR exists to fix is now absent everywhere except quality, when in fact two of the other six carry the same aggregate-numerator-vs-single-loop-cap defect and are only left alone because they lack scope counters. A maintainer who reads the header and stops there concludes the invariant is repaired; it isn't. The contradiction is internal to this PR, which is what makes it cheap to see and cheap to fix:
So the header is the only one of the three artifacts that gets it wrong. (b) Carried from Iteration 1, finding 1 — still present verbatim at I am re-raising (b) rather than treating it as settled because it was never dispositioned. The PR Review Fix (iteration 1) comment closed it with "code review raised no blocking findings — only the spec-review nit applied." That reads the Iteration 1 verdict as if Fix direction: reword What I verified above the finding line
Not re-flagged
|
The module header claimed six of the seven capped stages run at most once per issue, and the multiScopeField comment called browser's scope unit "per-iteration". Both are wrong: task-review and browser are multi-scope aggregates in exactly the same sense quality is (only approach/plan/test/pr-review are genuinely single-scope), and browser's scope is the phase (implement/pre-merge), not the iteration -- iteration is the numerator. Reword both comments to match gate-hygiene.md and the code they sit beside.
PR Review Fix (iteration 2)Commit: 09c5113 Fixed the code review's finding: reworded the module header ( |
…er edit commit 09c5113 added 3 net lines to computeFriction's module header comment, shifting the seven `= iter`/`++` statements gate-hygiene.md cites as evidence for "iteration counting starts at 1, not 0" by +3 each. No functional change.
Spec Review (Iteration 3)Verdict: approved Re-verified the two commits added since iteration 2's approval:
Re-confirmed the invariants iteration 1/2 already established, nothing regressed:
No scope creep, no engine-owned-path violations beyond the required lockstep copy, no regressions from the last two commits. |
Code Review (Iteration 3)Verdict: changes_requested — three text-only findings, together one commit's worth of editing (two comments, one line citation). No behavioral defect. Finding 1 is iteration 2's accepted finding applied to two of its three sites: a third comment inside the same function still carries the corrected-away claim, and now contradicts the comment 21 lines above it. Validation baseline (all green)
Findings1. Major (comment-only) — // scopes stays null (not 1) for the six single-scope stages: a number
// here is a claim "this many invocations were pooled", and only
// quality currently has a counter backing that claim.Commit This is the one place where the four/two split actually changes what the code means. Not a re-flag: iteration 2's finding named Fix: reword 2. Minor —
The two drivers this test asserts on are precisely the two the same PR documents as not single-scope. Fixture and assertions are correct; only the name is wrong. Fix: "…for stage drivers with no scope counter — asserted on a task-review driver and a browser driver", or similar. 3. Minor —
What I verified above the finding line
Dispositioned, not re-flagged
Findings 1-3 are the last text-accuracy items I have; I searched exhaustively for further instances rather than leaving room for a fourth pass. With those three edits (plus |
Spec Review (Iteration 1)Verdict: approved Verified the scoped diff ( Files touched match the plan exactly: Acceptance criteria, checked against the current diff and tests:
Dependency note honored — diff touches only Documentation target. Lockstep verified directly: No engine-owned-path violations beyond the required lockstep copy, no untracked task artifacts. Noted, not blocking (not my lane): the PR's own commit history shows an unresolved code-review finding (three comment-accuracy nits — e.g. |
Code Review (Iteration 1)Verdict: changes_requested — no blocking findings, no behavioral defect. Three comment/label-only minors, all in one round of edits. The arithmetic, the counter placement, the lockstep, and the tests are correct and I re-verified them below. Finding 1 is a surviving third instance of the exact claim a prior review round already adjudicated as false and 09c5113 fixed in the other two places. Validation baseline (all green)
Verified above that line
Findings1. Minor — // scopes stays null (not 1) for the six single-scope stages: a number09c5113 corrected the module header ( Fix direction: reword to something scope-agnostic that matches what the code actually keys on — e.g. "for the six stages with no scope counter" — then 2. Minor —
The two drivers this test asserts on are exactly the two the PR documents as multi-scope-but-uncounted. The assertions are right; only the title is wrong. Fix direction: retitle to "for stages with no scope counter." 3. Minor — The comment cites No change required (recorded so it is not re-flagged)
|
Code review (iteration 1) found a third surviving instance of a claim 09c5113 already corrected in two other places: task-review and browser are multi-scope aggregates without a counter yet, not single-scope stages. Reword the computeFriction comment, the matching test title, and resync harness.js's stale :5127 line citation to :5130.
PR Review Fix (iteration 1)Commit: 24b2d17 Fixed all three code-review minors: reworded the |
Spec Review (Iteration 2)Verdict: approved Re-verified the fix commit added since this run's iteration-1 approval: Scope unchanged. Fix commit verified directly against the findings it claims to close:
Acceptance criteria, unchanged from iteration 1's determination, re-confirmed against live tests:
Frozen docs untouched. Lockstep and manifests. No engine-owned-path violations, no untracked task artifacts, no reopening of settled approach/plan adjudications. Consistent with this run's own iteration-1 approval; nothing here reverses it. |
Code Review (Iteration 2)Verdict: approved — all three iteration-1 minors are fixed, nothing new introduced, and I found no blocking, major, or minor findings on the current head. Reviewed Validation baseline (all green)
Iteration-1 findings, re-verified as fixed
Re-verified above the baseline (unchanged conclusions from iteration 1, re-checked on this head, not copied forward)
Carried forward from iteration 1, still not change requests
Ready to merge. |
Implementation CompleteBranch
No deferred follow-up suggestions were collected during implementation. |
1 participant
Closes #165
Summary
computeFriction's quality stage term compared a run-wide aggregate (ctx.metrics.quality_iters, incremented across every call torunQualityLoopfor an issue) against a single loop's cap (MAX_QUALITY_ITERATIONS = 5), viamin(1, quality_iters/cap). A multi-task issue that cleared quality on iteration 2 of each of 3 tasks accumulatedquality_iters=6, saturating the ratio to 1.0 — the same score as an issue whose every quality loop actually exhausted the cap. Every other capped stage incomputeFrictioncompares one loop's iterations against that loop's cap; quality alone compared an aggregate to a per-loop cap.Approach: pooled denominator (quality only)
Chosen over tracking the worst single scope's iteration count because pooled is a mean while worst-scope is a max — max does not fix the issue's opening complaint, it relocates it: one capped scope would pin a multi-task issue at 1.0 forever regardless of how well every other scope did.
workflows/ticketmill.jsmetrics literal gainsquality_scopes: 0.runQualityLoopincrementsctx.metrics.quality_scopesonce per invocation (if (iter === 1) ctx.metrics.quality_scopes++), below the STOP guard so a STOP'd entry touches nothing.quality_itersincrements are unchanged in meaning and value.computeFrictiongainsmultiScopeField = { quality: 'quality_scopes' }. Inside the existing generic stage loop:baseCap = caps[k] > 0 ? caps[k] : 1,scopes = tracked ? Math.max(1, Number(m[multiScopeField[k]]) || 0) : null,cap = baseCap * (scopes || 1),ratio = Math.min(1, iters / cap). All seven stage drivers now carrycapandscopes(scopesisnullfor the six single-scope stages, a number only forquality), socontribution === Math.min(1, value / cap)andcap === baseCap * (scopes ?? 1)hold for every stage driver. Drivervaluestays rawquality_iters;metrics.quality_itersis unchanged.tests/harness.jsfreshMetrics()synced withquality_scopes: 0(hand-synced by contract;tests/run-record.test.jswalks its key set)..claude/workflows/ticketmill.jslockstep copy updated vianode scripts/lint-engine.js --fix.docs/architecture/gate-hygiene.md(pre-existing on this branch from a concurrent issue) gains a third supersession entry documenting the corrected invariant and the pooled-vs-worst-scope rationale, rather than touchingdocs/architecture/metrics.md, which is off-limits per the issue body and whose fixture-pinned segment covers nearly the whole file.Key decisions
quality_scopesincrements once perrunQualityLoopinvocation, not per iteration, and only past the STOP guard.scopesisnull(not1) on the six single-scope stage drivers, keeping "no scope claim made" honest rather than implying they were counted.quality_itersmetric value and meaning are untouched for other consumers.docs/architecture/metrics.mdleft byte-for-byte unchanged per issue constraints; the correction lives ingate-hygiene.mdinstead.Acceptance criteria covered
0.2per scope (one iteration run against a five-iteration cap), not0— seegate-hygiene.md's "Acceptance criterion 1, in its strongest form" for why0.2is the sibling-consistent answer under this issue's ownmin(1, iters/cap)definition, not a shortfall against the literal criterion text.Token usage (approximate, this issue only): 417925 output tokens