Quality & Risk: a cache tier can no longer change the verdict (#527) - #530
Merged
Conversation
computeQualityRisk reads task.builder (risks, files_modified) — data the rollup index never persisted — so the SAME run scored 50/high on a full parse and 15/low once index-served, reporting "0 risks flagged" while the very same snapshot carried risk_count: 2 per task. Not an honest unknown: a false score under a reassuring green band, on the tier that serves most runs most of the time. The #525 dials made it louder — a confident green arc on a genuinely high-risk run. Index entries now persist a compact per-task `builder_risk` summary (severity + mitigation fields, files_modified) and computeQualityRisk reads it when the full builder is absent. Deliberately a DISTINCT key, not a partial `builder`: client-state renders builder.summary / memory_summary / tests_run, so rehydrating a stub builder would have traded this false score for a new set of false empties. Bounded and still EXACT — the complexity file component saturates at FILES_CAP (50) unique files, so the 50/task cap cannot change a score. INDEX_VERSION 9 → 10 forces the rebuild. Pinned by a new case in the canonical lite↔full parity guard: the whole Quality & Risk verdict (score, band, severities, mitigated, complexity, files, builder tasks) must be identical on both paths, plus a cross-check that the card can never report "no risks" while task.risk_count counts them. Verified failing first; the original probe now returns identical numbers on both tiers. Rider: that guard was itself flaky — buildFullState does not await the index write, so cycle 2 could read too early and the guard would silently test nothing. tests/helpers/index-settle.js waits for the run to really be in the index (one shared helper, per the #515 lesson). Note it does NOT cure the separate dashboard-command-pages flake — evidence and a narrowed mechanism handed to #517 rather than papered over. Closes #527. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix Quality & Risk parity by persisting builder_risk in rollup index
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+115
to
+158
| test('index parity: the Quality & Risk verdict is identical on both paths (#527)', async () => { | ||
| // computeQualityRisk reads task.builder (risks, files_modified) — data the | ||
| // index did not persist, so the SAME run scored 50/high fresh and 15/low | ||
| // once index-served: a false score under a reassuring green band, on the | ||
| // tier that serves most runs most of the time. | ||
| const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-qr-parity-')); | ||
| try { | ||
| await fixtureScoredRun(projectRoot); | ||
|
|
||
| const first = await buildFullState(projectRoot, { includeRegistry: false }); | ||
| const full = first.runs.find((run) => run.runId === 'run-fx-scored'); | ||
| assert.ok(full && !full.fromIndex, 'cycle 1 parses the run fully'); | ||
| assert.ok(Number.isFinite(first.qualityRisk?.risk?.score), | ||
| 'the scored fixture yields a real risk score, or this guard proves nothing'); | ||
|
|
||
| assert.ok(await waitForIndexedRun(projectRoot, 'run-fx-scored'), 'cycle 1 persisted the run to the index'); | ||
| const second = await buildFullState(projectRoot, { includeRegistry: false }); | ||
| const lite = second.runs.find((run) => run.runId === 'run-fx-scored'); | ||
| assert.ok(lite?.fromIndex, 'cycle 2 serves the run from the index'); | ||
|
|
||
| const verdict = (state) => ({ | ||
| risk: state.qualityRisk?.risk?.score ?? null, | ||
| band: state.qualityRisk?.risk?.band ?? null, | ||
| severities: state.qualityRisk?.risk?.by_severity ?? null, | ||
| mitigated: state.qualityRisk?.risk?.mitigated ?? null, | ||
| complexity: state.qualityRisk?.complexity?.score ?? null, | ||
| files: state.qualityRisk?.complexity?.files_touched ?? null, | ||
| builderTasks: state.qualityRisk?.complexity?.builder_tasks ?? null, | ||
| }); | ||
| assert.deepEqual(verdict(second), verdict(first), | ||
| 'a cache tier must never change the governance verdict — persist the risk/complexity inputs in entryFromRun (with an INDEX_VERSION bump) or report unknown, never a quieter score'); | ||
|
|
||
| // And the card can never claim "no risks" while the same snapshot counts them. | ||
| const countedRisks = (lite.tasks ?? []).reduce((sum, task) => sum + (task.risk_count ?? 0), 0); | ||
| if (countedRisks > 0) { | ||
| const severities = second.qualityRisk?.risk?.by_severity ?? {}; | ||
| const reported = Object.values(severities).reduce((sum, n) => sum + n, 0); | ||
| assert.ok(reported > 0, | ||
| `snapshot counts ${countedRisks} risk(s) via task.risk_count but the card reports none`); | ||
| } | ||
| } finally { | ||
| rmSync(projectRoot, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
3. #527 test not aaa-structured 📜 Skill insight ▣ Testability
The new test interleaves setup, execution, and assertions without clear Arrange/Act/Assert separation, making it harder to scan and maintain. This violates the required AAA test structure guideline.
Agent Prompt
## Issue description
The test `index parity: the Quality & Risk verdict is identical on both paths (#527)` mixes Arrange, Act, and Assert steps throughout the body without clear separation.
## Issue Context
The compliance checklist requires tests to follow the Arrange-Act-Assert (AAA) pattern, ideally separated by comments or blank lines.
## Fix Focus Areas
- tests/dashboard-index-parity.test.js[115-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+97
| function riskSource(task) { | ||
| return task?.builder ?? task?.builder_risk ?? null; | ||
| } | ||
|
|
||
| function scoredTasks(tasks) { | ||
| return (tasks ?? []).filter((task) => riskSource(task) !== null); | ||
| } |
There was a problem hiding this comment.
9. Malformed builder counts as signal 🐞 Bug ☼ Reliability
scoredTasks() treats any non-null task.builder/task.builder_risk as a valid risk source without verifying it’s an object carrying risks/files_modified; a malformed truthy value can make riskSignalPresent true and produce a fabricated 0 score instead of an honest null. This weakens the “honest nulls” contract in computeQualityRisk() for corrupted/partial task data.
Agent Prompt
## Issue description
`scoredTasks()` currently includes tasks as long as `riskSource(task) !== null`, even if the selected source is not an object with the expected arrays. This can incorrectly mark risk/complexity signals as present and yield a 0 score.
## Issue Context
`computeQualityRisk()` uses `builderTasks.length` in `riskSignalPresent` and uses `collectRisks()` which ignores non-array `risks`. A non-object-but-truthy source can therefore force a misleading “0” instead of “unknown”.
## Fix Focus Areas
- src/observability/dashboard/state/quality-risk.js[91-97]
- src/observability/dashboard/state/quality-risk.js[99-106]
- src/observability/dashboard/state/quality-risk.js[142-147]
## What to change
- Make `riskSource()` (or `scoredTasks()`) return/include only sources that are plain objects (and optionally validate that `risks` / `files_modified` are arrays when present).
- Example: `const src = task?.builder ?? task?.builder_risk; return (src && typeof src === 'object' && !Array.isArray(src)) ? src : null;`
- Update the complexity loop to use safe access (`riskSource(task)?.files_modified`) if you change filtering logic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo caught a genuine correctness error in the first cut, and my commit
message and PR body both asserted the opposite: I claimed the file
component "saturates at FILES_CAP (50)" — FILES_CAP is 200. So the 50-file
slice DID silently lower complexity for any task touching >50 unique
files, and the 100-risk slice lowered the risk score and severity chips
for busy runs: the exact cache-tier-dependent verdict this fix exists to
remove, merely at a higher threshold.
Redesigned so no cap can move a score:
- Risks persist as per-severity {total, mitigated} TALLIES via a shared
summarizeRiskSeverities() — exact for any number of risks, and the
projection sums the identical arithmetic whether the tally came from a
live builder contract or the index. No list, no truncation.
- Files persist as a run-level deduped UNION bounded by the imported
FILES_CAP (one definition, no drift) — identical up to the point the
component provably saturates, and smaller than per-task copies.
- risk.total now derives from the tallies rather than a risk array.
Pinned by a new parity case at volume: 150 unique files (>the old 50 cap)
and 130 risks (>the old 100 cap) must produce an identical verdict on both
tiers — verified failing against the first cut.
Also: named the settle helper's retry constants (#490 convention). The
literal caps Qodo flagged separately are simply gone with the redesign.
Refs #527 (PR #530 review follow-up).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
All four findings addressed — and Qodo caught a real correctness error that my own PR body asserted the opposite of. Recording that plainly: I wrote "the complexity file component saturates at FILES_CAP (50) unique files, so the 50/task cap cannot change a score." FILES_CAP is 200. So the 50-file slice genuinely did lower complexity for any task touching >50 unique files, and the 100-risk slice lowered both the risk score and the severity chips on busy runs — the same cache-tier-dependent verdict this PR exists to eliminate, just at a higher threshold. Findings 1 and 2 were correct; my "provably exact" claim was not. Redesigned so that no cap can move a score:
Pinned by a new parity case at volume: 150 unique files and 130 risks must yield an identical verdict on both tiers — verified failing against the first cut before the fix. Findings 3/4: the literal caps are gone with the redesign; the settle helper's retry defaults are named constants (#490 convention). Gates: core 1902/1902, browser 26/26, lint/typecheck/validate/security clean. The PR body's inaccurate "FILES_CAP (50)" sentence is corrected below. — Claude Main |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #527 · found while live-verifying the #525 dials.
The bug
computeQualityRiskreadstask.builder(risks,files_modified) — data the rollup index never persisted. So the same run produced two different governance verdicts depending only on which cache tier served it:highlowtask.risk_countin the same snapshot[2, 2][2, 2]Not an honest
unknown— a false score under a reassuring green band, on the tier that serves most runs most of the time. "0 risks flagged" rendered while the same snapshot counted them. The #525 dials made it louder: a confident green arc on a genuinely high-risk run.The fix
Index entries persist a compact per-task
builder_risksummary (severity + mitigation fields,files_modified);computeQualityRiskreads it when the full builder is absent.INDEX_VERSION9 → 10 forces the rebuild.Two deliberate calls:
builder.client-staterendersbuilder.summary/memory_summary/tests_run; rehydrating a stub builder would have traded this false score for a fresh set of false empties — the same honesty bug wearing a different hat.FILES_CAPwas 50; it is 200, so those caps really could lower a score (Qodo, findings 1-2). Risks now persist as per-severity tallies (exact at any volume, nothing truncated) and files as a run-level deduped union bounded by the importedFILES_CAP. Pinned by a volume parity case: 150 files + 130 risks, identical verdict on both tiers.Pinned
New case in the canonical lite↔full parity guard: the entire verdict (score, band, severities, mitigated, complexity, files, builder tasks) must match on both paths — plus a cross-check that the card can never report "no risks" while
task.risk_countcounts them. Verified failing first; the original probe now returns identical numbers on both tiers.Rider, and one honest non-fix
That new guard was itself flaky —
buildFullStatedoesn't await the index write, so cycle 2 could read too early and the guard would silently test nothing. Fixed withtests/helpers/index-settle.js(one shared helper, per the #515 lesson).I also tried the same cure on the pre-existing
dashboard-command-pagesflake — it did not work, so I reverted it rather than ship a fix that doesn't fix. A standalone probe runs that sequence 20/20 successfully, and the run signature covers mtimes, so the mechanism is signature-recompute under concurrent load, not an unwritten index. Evidence and the narrowed mechanism handed to #517.Verification
Core 1901/1901 (repeat runs; the only intermittent is the pre-existing #517 flake above, which fails identically on main) · browser 26/26 · lint 0 · typecheck 0 · validate 196 · security green.
Merging after green checks + reviewer bodies read.
🤖 Generated with Claude Code