Dashboard evidence panels + verify runner-selection fix + flaky-assertion hardening - #192
Merged
Conversation
Three panels shipped with unit tests over stubbed fetches. All passed. Driving the served page against the live server rendered three EMPTY panels, and every substitute for a browser turned out to be lying: a brace-matcher counted braces inside strings and truncated loadReceipts mid-comment; the stubbed fetch resolved a bare array while all three endpoints return wrapper objects; a hand-rolled document stub made the 720KB script block throw at runtime. None of those were defects in the product. For a rendering guarantee the only non-vacuous harness is a real DOM against a real server. The load-bearing assertions are the honesty ones, not the presence ones: a null cost must render "-", because "$0.00" reads as "this run was free" when the proof records the cost as UNKNOWN. Mutation-verified: fabricating $0.00 turns exactly that assertion red and no other. The four fixture-dependent assertions FAIL on drift rather than skipping, so a seed edit that stops exercising a guarantee cannot quietly reduce nine assertions to five and report PASS. Measured 7s. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
verify.sh chose its runner with `grep '"jest"' package.json`, which matches a DEVDEPENDENCY entry. This repository is the proof: jest is a devDependency with no jest config, while scripts.test runs `bash -n` plus `node --test`. So verify ran jest, jest globbed 895 files that are not jest tests, every one reported "Your test suite must contain at least one test", and `loki verify` returned BLOCKED on a clean tree -- permanently, for a defect that does not exist. A false BLOCK on the flagship verification command is worse than a missed one. A gate that cries wolf on every run trains users to ignore the verdict, which costs more than the gate ever earned. run.sh had already diagnosed this exact trap -- its comment reads "with a JSON parser, not grep (grep would false-positive on devDeps)" -- but fixed only its trailing `else`, leaving three grep branches ahead of it to shadow the fix. A correct diagnosis in a comment is not a fix. Also: a declared script naming none of vitest/jest/mocha is now RUN via npm test rather than skipped. Falling through was its own defect -- verify skipped this repo's declared bash/node suite and ran PYTEST over a bash/node project. Also: the dependency-audit finding now states whether the CVEs are in the SHIPPED tree. This repo reports 4 high while `npm audit --omit=dev` reports 0, so nothing a user installs is affected; the old wording read as "the shipped product is vulnerable", a materially different claim. Costs one extra audit call, measured at 424ms per verify run. Mutation-verified on both routes, including the no-op fallthrough hole the first attempt introduced: a bare `:` in the new first branch terminated the if/elif chain and left test_runner=none, converting a false BLOCK into a SILENTLY UNMEASURED gate, which is worse. Behaviour is asserted across all three shapes, including the legitimate case that must not regress (no declared script + runner installed -> use it). local-ci.sh also carries the panels-harness registration from the previous commit; the two keep-list additions are adjacent lines and could not be split without an interactive stage. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
The assertion was `-lt 6000` against a 12s timeout. Measured locally on an idle machine: 3395ms -- 1.77x headroom. CI runs four shards concurrently on a slower runner, and shard 2/4 went red on exactly this assertion while the same suite passed 43/0 locally. A re-run at the SAME sha came back green, which is the one experiment that separates a flake from a regression. So this is hardening, not a regression fix. The PROPERTY is "it cancelled instead of waiting for the deadline", so the bound belongs at a fraction of the timeout rather than at an absolute stopwatch reading. Two-thirds: a run that actually waited takes >=12s and still fails, while normal scheduling jitter no longer does. Loosening past the timeout itself would make the clause vacuous. Each of the seven clauses now reports separately. The old single `bad` line named all seven and proved none, which is why diagnosing this needed a local re-measurement that then did not reproduce. Mutation-verified: a simulated full wait now reports "waited 15487ms (bound 8000ms of 12000ms timeout)". Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
/api/proofs serves 9 receipts with a verdict, file count, cost and an HTML
view of the receipt itself. NOTHING in the dashboard read it. Measured:
`grep -oE "'/api/proofs[^']*'" build-standalone.js` returned exactly ONE
line, /api/proofs/summary -- the header badge.
So the product's strongest claim, "we hand you a receipt you can check
yourself", had no surface. A user could see the COUNT of receipts and never
open one. Same inert-surface pattern as the six modules and the gate-policy
endpoint found earlier today: built, tested, unreachable.
The Trust section now lists the last 10 receipts -- verdict, timestamp,
files changed, cost -- each linking to its own rendered HTML receipt
(verified live: 65KB of real evidence at /api/proofs/<id>/html). The panel
names `loki proof verify` so a reader knows how to re-check it.
NOTHING IS FABRICATED. An unmeasured cost renders "-", never "$0.00"; a
zero would claim the run was free. An absent file count renders "-", not 0.
A missing verdict reads UNKNOWN. A genuine measured zero still renders as
zero, so the guard is not over-broad. The panel stays hidden when the
endpoint is absent (older server), errors, or returns no receipts: no
surface beats a wrong one.
FOUR ATTEMPTS TO MAKE THE TEST NON-VACUOUS, worth recording because each
failure was the same trap wearing a different coat:
1. hand-wrote the cost/files expressions in the test -- mutating the
source left it green, because it exercised its own correct copy
2. regexed the expressions out of source -- truncated the multi-line
ternaries at the first ';'
3. drove the real function, but asserted on JSON while it renders HTML
4. the cost assertion had an '||' that the files check satisfied, so a
wrong cost passed
Only the fourth fix caught the mutation: fabricating $0.00 now renders
"$0.00" for a null cost and the test goes red. A mutation that does not
turn a test red proves nothing, and counting it is how a suite gets trusted
without being checked.
14 assertions, including a shipped-bundle check with a positive control.
/api/learnings held real records -- rootCause, fix, preventInFuture -- with ZERO ui consumers. The system was learning from its own gate failures and showing nobody, which makes the memory UNFALSIFIABLE: a user cannot correct a learning they cannot see. Devin ships a "misleading knowledge" surface for exactly this reason, and it was the strongest single idea in their corpus. Insights now lists the last 8 learnings with cause, fix and prevention. RENDERS THE RECORD'S OWN WORDS, not a summary. A paraphrased root cause is a second claim about a claim, and the point of a learning is that it quotes what actually happened. Mutation-verified: replacing the rootCause with the tidier "a gate failed" trips three assertions. This closed a loop from earlier today. The one stored learning on this repo reads "[Critical] structured reviewer produced no valid verdict" -- the exact false-Critical fixed in e3690cd, where a harness failure was recorded as a defect in the user's code. Verified the fix propagates: rootCause is `[severity] description`, harnessFinding() is wired at all 5 sites, and a bun test confirms a new learning carries "[unverified]" and "not a defect found in your code". The mislabelled record stays as history; new ones will not repeat it. Absent fields are not invented: a missing rootCause reads "not recorded", a missing timestamp or iteration reads "-". Empty set, missing endpoint or a fetch error leaves the panel hidden -- no surface beats a wrong one. 9 assertions, including a shipped-bundle check with a positive control.
/api/gate-policy shipped in v9.17.0 with ZERO ui consumers -- the same inert-surface pattern as the six modules found earlier today: built, tested, and unreachable by the people it was built for. A user looking at eight gate cards could not tell which ones would stop a build, which is the only thing separating a gate from a suggestion. Each card now carries one line: BLOCKS or advisory, the hit count, and for an advisory gate the exact variable that promotes it. Live on this repo: "Blind Code Review: BLOCKS, 6 hits"; "Magic Modules Debate: advisory, 0 hits, promote with LOKI_GATE_MAGIC_DEBATE_BLOCKING=true". TWO REFUSALS, both mutation-verified. An UNMEASURED gate renders "not measured", never "0 hits". Zero claims the gate ran and never fired, and an operator would reasonably promote a gate they believe has never blocked anything. The reporter already refuses to emit 0 there; this keeps the distinction all the way to the pixel. A gate the policy does not mention renders NOTHING. Defaulting to "advisory" would tell a user a BLOCKING gate is optional -- the dangerous direction of that error. The policy fetch is separate and failure-tolerated: it is decoration on top of the gate list, so a missing endpoint (an older server) or an erroring reporter leaves the page exactly as it was rather than blanking the gates. Display-name mapping is explicit, not derived: "Test Suite" -> test_coverage and "Test Mutation" -> mutation_integrity are different gates that a lower()/replace() heuristic collides. Note on the mutation testing: my first TWO attempts were no-ops. One did not match the source; the other set an unknown key that still fell through to the same empty return. A mutation that does not change behaviour proves nothing, and counting it as evidence is how a test gets believed without being checked. The third reached the real guard and went red. 10 assertions, including a shipped-bundle check with a positive control -- this file has very long lines and grep otherwise treats it as binary and prints nothing, which reads as a clean zero.
/api/budget reports budget_limit and it ships NULL: LOKI_BUDGET_LIMIT is unset by default, so a long run has no automatic stop. The endpoint had ZERO ui consumers, so the only place that fact surfaced was a bill. The default is defensible -- a run killed mid-flight at a threshold the user never chose is worse than one that keeps going -- but leaving it INVISIBLE is not. The Cost page now states it: "No spend cap set. This run will not stop on cost. Spent so far: $X. Set one with LOKI_BUDGET_LIMIT=<usd>." THE NO-CAP STATE RENDERS AS PROMINENTLY AS A CAP. A banner that only appeared when a limit existed would show nothing in exactly the situation the user most needs to know about -- the silent-danger direction of that error. Mutation-verified: suppressing the no-cap branch trips three assertions, including one where the fallback fabricated "$0.00". All four states covered: no cap, cap set (limit + remaining), exceeded, and no measurement at all. An unmeasured spend reads "not measured", never $0.00 -- a zero claims the run was free. Fourth inert endpoint closed today. Same measurement each time: a server route with real data and no consumer. Receipts (9 records, only the count visible), gate policy (7 gates, no way to see which BLOCK), learnings (written, shown to nobody), and now budget. That is a category of defect worth naming: it passes every test and delivers nothing. 10 assertions, including a shipped-bundle check with a positive control.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe pull request changes test-runner selection to use declared npm scripts, adds production-only audit reporting, and adds dashboard policy, learning, budget, and evidence-receipt panels. It also adds browser and shell validation harnesses and improves test orchestration diagnostics. Declared test execution
Dashboard policy and evidence panels
Validation orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SectionNavigation
participant DashboardLoaders
participant DashboardAPI
participant EvidencePanels
SectionNavigation->>DashboardLoaders: open Insights, Cost, or Trust
DashboardLoaders->>DashboardAPI: fetch /api/learnings, /api/budget, or /api/proofs
DashboardAPI-->>DashboardLoaders: return panel records
DashboardLoaders->>EvidencePanels: render available records and states
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Loki CI Quality Report
FindingsHIGH
MEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T00:54:26Z |
|
Reviewed the diff (7 commits: 4 dashboard panels + verify runner-selection fix + audit-scope wording + flaky-assertion hardening). The verify.sh/run.sh runner-selection fix and the assertion-hardening are solid — good root-cause analysis, and the fix is applied consistently on both routes. One real bug and one security gap in the new dashboard panel code, plus a small perf nit. Bug: duplicate
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
tests/test-receipts-panel.sh (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStub the wrapper shape that
/api/proofsreturns. The stub resolves a bare array./api/proofsreturns{"proofs": [...]}. The header oftests/e2e/dashboard-evidence-panels.mjsrecords that a bare-array stub taught the earlier tests a contract the server does not serve. Wrap the rows so this suite exercises the real path.Proposed fix
-global.fetch=()=>Promise.resolve({ok:true,json:()=>Promise.resolve(rows)}); +global.fetch=()=>Promise.resolve({ok:true,json:()=>Promise.resolve({proofs:rows})});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-receipts-panel.sh` around lines 82 - 85, Update the global.fetch stub in the test setup to resolve an object containing the rows under the proofs property, matching the /api/proofs response shape. Keep the existing rows initialization and successful response behavior unchanged.tests/test-learnings-panel.sh (1)
40-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a second record to pin the order. The fixture holds one learning, so the suite cannot detect ordering defects.
/api/learningsreturns newest-first, andloadLearningsreverses the list again, which shows the oldest records. A two-record fixture plus an assertion that the newestrootCauseappears first would catch this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-learnings-panel.sh` around lines 40 - 52, Expand the _REC fixture to contain two learnings with distinct timestamps and rootCause values, making one clearly newer than the other. Update the _render assertion to verify the newer record’s rootCause appears before the older record’s rootCause, preserving the existing verbatim-content check.tests/e2e/dashboard-evidence-panels.mjs (1)
57-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the browser in a
finallyblock. If an assertion orpage.evaluatethrows,main()rejects andbrowser.close()at Line 199 never runs.main().catchthen callsprocess.exit(2)and leaves the Chromium launch to be reaped by the OS. Atry { ... } finally { await browser.close(); }makes teardown deterministic.Also applies to: 196-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/dashboard-evidence-panels.mjs` around lines 57 - 63, Wrap the main test flow in main() with a try/finally and move browser.close() into the finally block so teardown occurs on assertions, page.evaluate failures, and other rejections. Preserve the existing error propagation through main().catch and ensure the browser variable remains available to the cleanup block.tests/test-review-assurance-tail.sh (1)
1405-1406: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive the shard bound from the review timeout passed to
run_review_case.
run_review_casereceivesreview_timeoutas argument$7and setsREVIEW_TEST_LAST_TIMEOUTfrom it before exportingLOKI_REVIEW_CALL_TIMEOUT. This test passes12as the timeout, but hard-codes12000for the assertion bound; keep the timeout in one place or useREVIEW_TEST_LAST_TIMEOUT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-review-assurance-tail.sh` around lines 1405 - 1406, Update the shard timeout setup near _shard_timeout_ms so the bound derives from the review timeout supplied to run_review_case, preferably by reusing REVIEW_TEST_LAST_TIMEOUT rather than hard-coding 12000. Preserve the existing two-thirds calculation for _shard_bound_ms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@autonomy/verify.sh`:
- Around line 443-445: Update _verify_pkg_test_script in autonomy/verify.sh:
trim the declared test script and emit it only when the trimmed value is
non-empty, so whitespace-only scripts produce no value. Add the corresponding
whitespace-only package.json fixture and assertion in
tests/test-verify-runner-selection.sh lines 145-192, expecting
_verify_pkg_test_script to return no value.
In `@dashboard-ui/scripts/build-standalone.js`:
- Around line 2459-2474: In the learning-record rendering loop, add a local
HTML-escaping helper and apply it to every interpolated record field, including
timestamp, iteration, trigger, rootCause, fix, and preventInFuture, before
assigning html to innerHTML. Apply the same escaping in the receipt loader for
the proof.json verdict field, while preserving the existing conditional
rendering and fallback behavior.
- Around line 1790-1791: Rename the new Cost-panel banner element from the
duplicate budget-banner id to a unique id, and update loadBudget() to query and
update that new id instead of the fixed page-wide `#budget-banner`. Keep the
existing fixed banner and initBudgetBanner() behavior unchanged, ensuring
Cost-panel updates target the newly added element.
In `@dashboard/static/index.html`:
- Around line 1691-1692: Remove or rename the duplicate budget-banner element
generated by the standalone build so only the fixed page-wide banner retains
id="budget-banner". Update the generation logic in build-standalone.js, then
rebuild the shipped dashboard/static/index.html artifact and ensure
getElementById targets the intended banner.
In `@tests/e2e/dashboard-evidence-panels.mjs`:
- Line 94: Update the loadBudget test mapping entry to use the renamed
Cost-panel element id instead of the page-wide budget-banner id, so tests 3, 6,
and 7 target the Cost panel rather than the fixed banner. Preserve the existing
loadBudget action and expected-element structure.
In `@tests/test-budget-banner.sh`:
- Line 36: Update the test stubs and shipped-bundle assertion: in the
global.document mock, make getElementById return the element only for the
requested expected id so loadBudget cannot pass with an arbitrary lookup, and in
the check around the shipped dashboard bundle use a string introduced by the new
panel, such as “No spend cap set”, instead of the pre-existing budget-banner id.
In `@tests/test-verify-runner-selection.sh`:
- Around line 194-196: Extend the syntax-check section in the test to run bash
-n against the autonomy/loki entry point, alongside the existing SRC and RUNSH
checks, and report success or failure with an appropriate message.
---
Nitpick comments:
In `@tests/e2e/dashboard-evidence-panels.mjs`:
- Around line 57-63: Wrap the main test flow in main() with a try/finally and
move browser.close() into the finally block so teardown occurs on assertions,
page.evaluate failures, and other rejections. Preserve the existing error
propagation through main().catch and ensure the browser variable remains
available to the cleanup block.
In `@tests/test-learnings-panel.sh`:
- Around line 40-52: Expand the _REC fixture to contain two learnings with
distinct timestamps and rootCause values, making one clearly newer than the
other. Update the _render assertion to verify the newer record’s rootCause
appears before the older record’s rootCause, preserving the existing
verbatim-content check.
In `@tests/test-receipts-panel.sh`:
- Around line 82-85: Update the global.fetch stub in the test setup to resolve
an object containing the rows under the proofs property, matching the
/api/proofs response shape. Keep the existing rows initialization and successful
response behavior unchanged.
In `@tests/test-review-assurance-tail.sh`:
- Around line 1405-1406: Update the shard timeout setup near _shard_timeout_ms
so the bound derives from the review timeout supplied to run_review_case,
preferably by reusing REVIEW_TEST_LAST_TIMEOUT rather than hard-coding 12000.
Preserve the existing two-thirds calculation for _shard_bound_ms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce501523-33ac-42f3-a37d-cfa44bed6346
⛔ Files ignored due to path filters (1)
dashboard-ui/dist/loki-dashboard-standalone.htmlis excluded by!**/dist/**
📒 Files selected for processing (15)
autonomy/run.shautonomy/verify.shdashboard-ui/components/loki-quality-gates.jsdashboard-ui/scripts/build-standalone.jsdashboard/static/index.htmlscripts/local-ci.shscripts/run-dashboard-evidence-panels-harness.shtests/e2e/dashboard-evidence-panels.mjstests/run-all-tests.shtests/test-budget-banner.shtests/test-gate-policy-ui-line.shtests/test-learnings-panel.shtests/test-receipts-panel.shtests/test-review-assurance-tail.shtests/test-verify-runner-selection.sh
Comment on lines
+443
to
+445
| if isinstance(s, dict) and isinstance(s.get("test"), str): | ||
| sys.stdout.write(s["test"]) | ||
| ' "$tree/package.json" 2>/dev/null || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat whitespace-only declared test scripts as absent.
_verify_pkg_test_script emits whitespace unchanged. verify_gate_tests then selects npm-test, and npm test can exit successfully without running tests. This differs from autonomy/run.sh, which trims the script before selection.
autonomy/verify.sh#L443-L445: trim the string and emit it only when it remains non-empty.tests/test-verify-runner-selection.sh#L145-L192: add a{"scripts":{"test":" "}}fixture that expects_verify_pkg_test_scriptto return no value.
Proposed helper fix
-s = d.get("scripts")
-if isinstance(s, dict) and isinstance(s.get("test"), str):
- sys.stdout.write(s["test"])
+s = d.get("scripts")
+if isinstance(s, dict) and isinstance(s.get("test"), str):
+ test_script = s["test"].strip()
+ if test_script:
+ sys.stdout.write(test_script)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if isinstance(s, dict) and isinstance(s.get("test"), str): | |
| sys.stdout.write(s["test"]) | |
| ' "$tree/package.json" 2>/dev/null || true | |
| if isinstance(s, dict) and isinstance(s.get("test"), str): | |
| test_script = s["test"].strip() | |
| if test_script: | |
| sys.stdout.write(test_script) | |
| ' "$tree/package.json" 2>/dev/null || true |
📍 Affects 2 files
autonomy/verify.sh#L443-L445(this comment)tests/test-verify-runner-selection.sh#L145-L192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@autonomy/verify.sh` around lines 443 - 445, Update _verify_pkg_test_script in
autonomy/verify.sh: trim the declared test script and emit it only when the
trimmed value is non-empty, so whitespace-only scripts produce no value. Add the
corresponding whitespace-only package.json fixture and assertion in
tests/test-verify-runner-selection.sh lines 145-192, expecting
_verify_pkg_test_script to return no value.
Comment on lines
+1790
to
+1791
| <div id="budget-banner" style="display:none;margin-bottom:12px;padding:10px 12px; | ||
| border:1px solid var(--loki-border);border-radius:6px;font-size:12px;"></div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
id="budget-banner" is already used by the fixed page-wide banner. Line 1200 declares <div class="budget-banner" id="budget-banner">, which contains #budget-banner-text and #budget-banner-link. document.getElementById('budget-banner') returns that first element, so:
loadBudget()(Line 2406) writes into the fixed top banner and replaces#budget-banner-textand#budget-banner-link.initBudgetBanner()then holds a detachedtextEl, so every later WebSocket or poll update is lost.loadBudget()setsdisplay:blockinline on aposition:fixedelement, so the spend-cap text renders as a page-wide bar instead of a Cost-panel banner.- This new Cost-panel div never receives content.
Rename this element and use the new id in loadBudget(). I will note the mirrored occurrence in dashboard/static/index.html in a consolidated comment.
Proposed fix
- <div id="budget-banner" style="display:none;margin-bottom:12px;padding:10px 12px;
+ <div id="cost-budget-cap" style="display:none;margin-bottom:12px;padding:10px 12px;
border:1px solid var(--loki-border);border-radius:6px;font-size:12px;"></div> window.loadBudget = function () {
- var el = document.getElementById('budget-banner');
+ var el = document.getElementById('cost-budget-cap');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div id="budget-banner" style="display:none;margin-bottom:12px;padding:10px 12px; | |
| border:1px solid var(--loki-border);border-radius:6px;font-size:12px;"></div> | |
| <div id="cost-budget-cap" style="display:none;margin-bottom:12px;padding:10px 12px; | |
| border:1px solid var(--loki-border);border-radius:6px;font-size:12px;"></div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard-ui/scripts/build-standalone.js` around lines 1790 - 1791, Rename
the new Cost-panel banner element from the duplicate budget-banner id to a
unique id, and update loadBudget() to query and update that new id instead of
the fixed page-wide `#budget-banner`. Keep the existing fixed banner and
initBudgetBanner() behavior unchanged, ensuring Cost-panel updates target the
newly added element.
Source: Linters/SAST tools
Comment on lines
+2459
to
+2474
| for (var i = 0; i < rows.length; i++) { | ||
| var x = rows[i] || {}; | ||
| var when = x.timestamp ? String(x.timestamp).slice(0, 16).replace('T', ' ') : '-'; | ||
| var iter = (x.iteration === null || x.iteration === undefined) ? '-' : ('iter ' + x.iteration); | ||
| html += '<div style="padding:8px;border-bottom:1px solid var(--loki-border);font-size:12px;">' | ||
| + '<div style="display:flex;gap:10px;color:var(--loki-text-muted);margin-bottom:4px;">' | ||
| + '<span>' + when + '</span><span>' + iter + '</span>' | ||
| + '<span>' + String(x.trigger || 'unknown trigger') + '</span></div>' | ||
| + '<div style="margin-bottom:3px;"><strong>cause:</strong> ' | ||
| + String(x.rootCause || 'not recorded') + '</div>' | ||
| + (x.fix ? '<div style="margin-bottom:3px;"><strong>fix:</strong> ' + String(x.fix) + '</div>' : '') | ||
| + (x.preventInFuture ? '<div style="color:var(--loki-text-muted);"><strong>prevent:</strong> ' | ||
| + String(x.preventInFuture) + '</div>' : '') | ||
| + '</div>'; | ||
| } | ||
| list.innerHTML = html; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape learning fields before innerHTML. x.trigger, x.rootCause, x.fix, and x.preventInFuture come from agent-written records in .loki/state/relevant-learnings.json. The code concatenates them into innerHTML without escaping, so a record that contains markup executes script in the dashboard origin. The Token Economics tile in the same file uses textContent for the same reason (Line 1459 comment).
Add a local escape helper and apply it to every interpolated record field. The receipt loader at Lines 2503-2518 needs the same treatment for verdict, which also comes from proof.json.
Proposed fix
+ var esc = function (s) {
+ return String(s).replace(/&/g, '&').replace(/</g, '<')
+ .replace(/>/g, '>').replace(/"/g, '"');
+ };
fetch('/api/learnings', { headers: { 'Accept': 'application/json' } })- + '<span>' + String(x.trigger || 'unknown trigger') + '</span></div>'
+ + '<span>' + esc(x.trigger || 'unknown trigger') + '</span></div>'
+ '<div style="margin-bottom:3px;"><strong>cause:</strong> '
- + String(x.rootCause || 'not recorded') + '</div>'
- + (x.fix ? '<div style="margin-bottom:3px;"><strong>fix:</strong> ' + String(x.fix) + '</div>' : '')
+ + esc(x.rootCause || 'not recorded') + '</div>'
+ + (x.fix ? '<div style="margin-bottom:3px;"><strong>fix:</strong> ' + esc(x.fix) + '</div>' : '')
+ (x.preventInFuture ? '<div style="color:var(--loki-text-muted);"><strong>prevent:</strong> '
- + String(x.preventInFuture) + '</div>' : '')
+ + esc(x.preventInFuture) + '</div>' : '')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard-ui/scripts/build-standalone.js` around lines 2459 - 2474, In the
learning-record rendering loop, add a local HTML-escaping helper and apply it to
every interpolated record field, including timestamp, iteration, trigger,
rootCause, fix, and preventInFuture, before assigning html to innerHTML. Apply
the same escaping in the receipt loader for the proof.json verdict field, while
preserving the existing conditional rendering and fallback behavior.
Comment on lines
+1691
to
+1692
| <div id="budget-banner" style="display:none;margin-bottom:12px;padding:10px 12px; | ||
| border:1px solid var(--loki-border);border-radius:6px;font-size:12px;"></div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate id="budget-banner" in the shipped bundle. HTMLHint reports the id is not unique. The fixed page-wide banner earlier in this document owns the same id, so getElementById never returns this element. This artifact is generated, so fix dashboard-ui/scripts/build-standalone.js and rebuild. See the consolidated comment.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1691-1691: The id value [ budget-banner ] must be unique.
(id-unique)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/static/index.html` around lines 1691 - 1692, Remove or rename the
duplicate budget-banner element generated by the standalone build so only the
fixed page-wide banner retains id="budget-banner". Update the generation logic
in build-standalone.js, then rebuild the shipped dashboard/static/index.html
artifact and ensure getElementById targets the intended banner.
Source: Linters/SAST tools
| const panels = [ | ||
| ['loadReceipts', 'receipts-panel', 'receipts-list'], | ||
| ['loadLearnings', 'learnings-panel', 'learnings-list'], | ||
| ['loadBudget', 'budget-banner', 'budget-banner'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This entry targets the fixed page-wide banner, not the Cost panel. In the served page, budget-banner is the id of the fixed top banner declared earlier in the document. getElementById('budget-banner') returns that element, so tests 3, 6, and 7 assert against the banner that loadBudget overwrote, and the harness cannot detect the id clash. Update this entry to the new Cost-panel id after the generator rename.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/dashboard-evidence-panels.mjs` at line 94, Update the loadBudget
test mapping entry to use the renamed Cost-panel element id instead of the
page-wide budget-banner id, so tests 3, 6, and 7 target the Cost panel rather
than the fixed banner. Preserve the existing loadBudget action and
expected-element structure.
| const m=src.match(/window\.loadBudget = function \(\) \{[\s\S]*?\n \};/); | ||
| if(!m){console.error('EXTRACT_FAILED');process.exit(2);} | ||
| const el={style:{},innerHTML:''}; | ||
| global.document={getElementById:()=>el}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two assertions in this suite are vacuous.
- Line 36:
getElementByIdreturns the same stub object for every id. The test therefore passes whatever idloadBudgetrequests, and it cannot detect thatbudget-banneralready belongs to the fixed page-wide banner. - Line 84:
budget-banneris present indashboard/static/index.htmlbefore this PR, because the fixed banner uses that id. The shipped-bundle check passes even without a rebuild.
Key the stub on the id, and grep for a string that only the new panel introduces, such as No spend cap set.
Proposed fix
-const el={style:{},innerHTML:''};
-global.document={getElementById:()=>el};
+const el={style:{},innerHTML:''};
+const nodes={'cost-budget-cap':el};
+global.document={getElementById:(id)=>nodes[id]||null};-elif grep -aq "budget-banner" "$SHIPPED"; then
+elif grep -aq "No spend cap set" "$SHIPPED"; thenAlso applies to: 84-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test-budget-banner.sh` at line 36, Update the test stubs and
shipped-bundle assertion: in the global.document mock, make getElementById
return the element only for the requested expected id so loadBudget cannot pass
with an arbitrary lookup, and in the check around the shipped dashboard bundle
use a string introduced by the new panel, such as “No spend cap set”, instead of
the pre-existing budget-banner id.
Comment on lines
+194
to
+196
| # --- 9. Syntax -------------------------------------------------------------- | ||
| bash -n "$SRC" 2>/dev/null && ok "verify.sh parses" || bad "verify.sh has a syntax error" | ||
| bash -n "$RUNSH" 2>/dev/null && ok "run.sh parses" || bad "run.sh has a syntax error" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required autonomy/loki syntax check.
This release check validates autonomy/verify.sh and autonomy/run.sh, but it omits autonomy/loki. Validate all required shell entry points before release.
Proposed fix
bash -n "$SRC" 2>/dev/null && ok "verify.sh parses" || bad "verify.sh has a syntax error"
bash -n "$RUNSH" 2>/dev/null && ok "run.sh parses" || bad "run.sh has a syntax error"
+bash -n "$REPO_ROOT/autonomy/loki" 2>/dev/null && ok "loki parses" || bad "loki has a syntax error"As per coding guidelines, “Before every release, validate shell syntax for autonomy/run.sh and autonomy/loki.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # --- 9. Syntax -------------------------------------------------------------- | |
| bash -n "$SRC" 2>/dev/null && ok "verify.sh parses" || bad "verify.sh has a syntax error" | |
| bash -n "$RUNSH" 2>/dev/null && ok "run.sh parses" || bad "run.sh has a syntax error" | |
| # --- 9. Syntax -------------------------------------------------------------- | |
| bash -n "$SRC" 2>/dev/null && ok "verify.sh parses" || bad "verify.sh has a syntax error" | |
| bash -n "$RUNSH" 2>/dev/null && ok "run.sh parses" || bad "run.sh has a syntax error" | |
| bash -n "$REPO_ROOT/autonomy/loki" 2>/dev/null && ok "loki parses" || bad "loki has a syntax error" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 195-195: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
[info] 196-196: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test-verify-runner-selection.sh` around lines 194 - 196, Extend the
syntax-check section in the test to run bash -n against the autonomy/loki entry
point, alongside the existing SRC and RUNSH checks, and report success or
failure with an appropriate message.
Source: Coding guidelines
The Loki Quality Gate failed THIS pr with one HIGH: "Dangerous eval/exec
usage detected" at diff line 1459. That is the gate working. A test is
not exempt from the rule it exists to enforce, so the call goes.
Four call sites, two forms:
- three panel tests ran an extracted renderer through dynamic code
execution inside embedded node; now `new Function('global', m[0])
.call(global, global)`, which runs the same real source with no
dynamic code construction
- the runner-selection test sourced a helper the same way; now
extracted to a temp file and sourced
Each of the three panel tests re-mutation-verified after the swap: the
budget banner's wording is broken at source and the assertion goes red.
Worth recording that my FIRST mutation attempt targeted a string that did
not exist in the file, so the test stayed green and looked vacuous. The
test was fine; the mutation was wrong. A mutation that does not apply
proves nothing in either direction, so assert the pattern is present
before drawing a conclusion from the result.
Also publishes verify's cost, since VERIFICATION-COST.md is where that
number belongs and this pr changed it:
- `loki verify` end to end: 77s on this repo, dominated by the
project's own suite rather than our gates
- the shipped-vs-dev CVE split added in this pr: 418ms, 0.5%
And records the runner-detection defect in the limits section. A false
BLOCK is the more damaging direction of that error, and the runner it
picked is readable in evidence.json so a user can check which one it
chose rather than take our word for it.
Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
Contributor
Loki CI Quality Report
FindingsMEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T01:01:22Z |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/VERIFICATION-COST.md`:
- Around line 28-29: Update the “shipped-vs-dev CVE split” entry in
VERIFICATION-COST.md to state that the scope split is reported only for high and
critical findings, matching verify_gate_dependency_audit behavior; do not imply
that moderate findings receive the same annotation.
- Around line 55-63: Update the documentation around verify_gate_tests to
clarify that it parses scripts.test to identify the declared test framework,
then executes a normalized runner command for recognized frameworks rather than
every script flag or wrapper command; retain the distinction that generic
projects use the declared npm test command, or alternatively change
verify_gate_tests to invoke npm test consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51f973f3-ea24-422a-8ccc-f96a103622c6
📒 Files selected for processing (5)
docs/VERIFICATION-COST.mdtests/test-budget-banner.shtests/test-learnings-panel.shtests/test-receipts-panel.shtests/test-verify-runner-selection.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test-learnings-panel.sh
- tests/test-verify-runner-selection.sh
- tests/test-budget-banner.sh
- tests/test-receipts-panel.sh
Comment on lines
+28
to
+29
| | `loki verify` | 77 seconds | `loki verify HEAD~1` on this repository, 5 changed files; dominated by the project's own test suite, not by our gates | | ||
| | of which, shipped-vs-dev CVE split | 418 ms | one extra `npm audit --omit=dev`; 0.5% of the run, and the reason the audit finding can say whether a CVE reaches users | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the CVE-scope claim to the severities currently annotated.
verify_gate_dependency_audit adds the shipped/development scope note only for critical and high findings. Moderate findings still report only the total count. The “shipped-vs-dev CVE split” wording overstates the current behavior.
Either add the scope note to moderate findings or document that the split applies only to high and critical findings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/VERIFICATION-COST.md` around lines 28 - 29, Update the “shipped-vs-dev
CVE split” entry in VERIFICATION-COST.md to state that the scope split is
reported only for high and critical findings, matching
verify_gate_dependency_audit behavior; do not imply that moderate findings
receive the same annotation.
Comment on lines
+55
to
+63
| **The tests gate depends on correctly identifying YOUR test runner, and it has | ||
| been wrong before.** Until 2026-08-07 the runner was chosen by grepping | ||
| `package.json` for `"jest"` / `"vitest"` / `"mocha"`, which matches a | ||
| **devDependency**. This repository is the case that exposed it: jest is a | ||
| devDependency with no jest config while `scripts.test` runs `bash -n` plus | ||
| `node --test`, so verify ran jest, jest globbed 895 files that are not jest | ||
| tests, and `loki verify` returned BLOCKED on a clean tree -- permanently, for a | ||
| defect that did not exist. It now reads `scripts.test` with a JSON parser and | ||
| runs what the project declares. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that recognized frameworks use normalized commands.
verify_gate_tests reads scripts.test to select a runner, but it invokes hard-coded npx vitest, npx jest, or npx mocha commands for recognized frameworks. It runs the declared npm test command only for the generic fallback. The current text can imply that all declared script flags and wrapper commands execute.
Update “runs what the project declares” to describe runner selection and normalized execution, or change the implementation to execute npm test consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/VERIFICATION-COST.md` around lines 55 - 63, Update the documentation
around verify_gate_tests to clarify that it parses scripts.test to identify the
declared test framework, then executes a normalized runner command for
recognized frameworks rather than every script flag or wrapper command; retain
the distinction that generic projects use the declared npm test command, or
alternatively change verify_gate_tests to invoke npm test consistently.
|
Reviewed the diff (verify.sh/run.sh runner-selection fix, the four dashboard evidence panels, the flaky-assertion hardening). Overall this is careful, well-evidenced work — the commit messages document real mutation-testing verification and the reasoning for each change is sound. Two issues worth fixing before merge: 1.
|
CI shard 2/4 reported four suites FAILED. They read exactly like four regressions. They were not: the scripts did not exist. A cherry-pick across a 51-commit divergence resolved a conflict in this file by taking both sides, which pulled in run_test registrations whose implementations live on the other lineage. Two parts. REMOVES 13 stale registrations. My first pass removed the 10 the CI log and my own notes named; checking the general property -- does every registration resolve to a file on disk -- found 3 more that no failure had surfaced yet, because their shard had not run. Enumerating from a symptom list would have left them. ADDS the guard that makes this self-reporting. run_test now checks the script exists and fails with "registered but its script is MISSING ... This is a stale run_test registration, not a code defect." Fails rather than skips: a silently skipped registration is a suite nobody runs and nobody misses. The message is the fix. The defect was cheap to correct once identified; what cost the time was a CI log that presented a bookkeeping fault as four product failures. Verified by injecting a registration for a nonexistent script and confirming the guard fires and increments TOTAL_FAILED. Caught while writing it that I had used a counter name this file does not define, which would have printed the error without failing the run. The suites themselves are not lost -- they exist in history and test source changes that are not on this lineage. Bringing those across is larger than this pr and does not belong in it. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/run-all-tests.sh`:
- Line 80: Replace the Unicode failure marker in the test-missing message within
the run-all-tests output with an ASCII-only label such as FAIL, while preserving
the existing test name, missing-file details, and color formatting.
- Around line 73-85: Update run_test so it distinguishes bare script paths from
full command-line registrations before the missing-file check; validate only the
underlying script path for commands such as “python3 tests/example.py,” while
preserving the existing bookkeeping failure for missing bare paths. Add
regression coverage for both registration forms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9be45849-1670-427e-9311-d8c20938d158
📒 Files selected for processing (1)
tests/run-all-tests.sh
| # time, not the fix. Fails loudly rather than skipping: a silently skipped | ||
| # registration is a suite nobody runs and nobody misses. | ||
| if [ ! -f "$test_file" ]; then | ||
| echo -e "${RED}✗ ${test_name}: registered but its script is MISSING (${test_file##*/})${NC}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an ASCII-only failure marker.
Line 80 adds a Unicode status marker to CI output. Replace it with an ASCII label such as FAIL.
As per coding guidelines, **/*: "Never use emojis in code, documentation, commit messages, README files, website content, markdown, or any other output."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/run-all-tests.sh` at line 80, Replace the Unicode failure marker in the
test-missing message within the run-all-tests output with an ASCII-only label
such as FAIL, while preserving the existing test name, missing-file details, and
color formatting.
Source: Coding guidelines
Contributor
Loki CI Quality Report
FindingsMEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T01:12:53Z |
ReviewOverall this is a solid, well-documented PR — the runner-selection fix ( 1.
|
The guard I added one commit ago failed a suite whose script exists. Registrations come in two forms here: run_test "name" "$SCRIPT_DIR/x.sh" bare path run_test "name" "python3 -m pytest -q $SCRIPT_DIR/x.py" full command `[ ! -f "$test_file" ]` on the second form tests whether a file literally named "python3 -m pytest -q ..." exists. It does not, so seven command-form registrations were one shard away from being reported as missing. CI caught the first of them within minutes. Now the script path is resolved out of either form -- the last whitespace-separated token ending in .sh or .py -- and only that is checked. A guard that fires on correct code is worse than no guard: it is the same false-BLOCK failure this pr already fixes in the verify tests gate, and I reintroduced it while writing the fix for something else. Verified both directions through the real script rather than a reimplementation of it: a command-form registration whose file exists PASSES, and a bare path that is missing is still caught with its name. A separate check confirms all registrations in the file resolve under both forms. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
Contributor
Loki CI Quality Report
FindingsMEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T01:19:25Z |
ReviewSolid PR overall — the verify/run.sh runner-selection fix is well-reasoned (confirmed the fix closes the devDependency-grep false-positive on both routes, and the case-statement ordering in One real issue found, plus a minor inconsistency: Unescaped user/LLM-controlled data inserted into
|
CI shard 2/4 failed "the emit is not adjacent to the agent stage -- it may measure the wrong prompt". The property it names is true: both markers are in run_autonomous(). They are simply 88 lines apart, and the check was a `grep -B26 -A6` window. Pre-existing, NOT introduced here. Verified by measuring the same distance on the pr base (0946f7f) without any of my edits: 88 lines there too. My run.sh change is ~11,700 lines away from either marker. The check's own comment already recorded that a -B-only window had produced three false failures against correct code. That was the signal to stop widening the window and change what is being asserted: a line window does not test co-location, it tests that nobody inserted lines nearby, which goes false on any unrelated edit to the region. Now it walks back from each marker to its enclosing `name() {` and compares. A missing marker yields no answer and fails, so "not found" is never read as agreement. Mutation-verified in the direction a window cannot catch: inserting a function boundary BETWEEN the two markers -- leaving their line distance unchanged -- turns the assertion red. The two later assertions still use a small window, deliberately. They check the emit's own arguments, which live on continuation lines of the matched statement, so there proximity really is the property. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
Contributor
Loki CI Quality Report
FindingsMEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T01:37:12Z |
|
Reviewed the diff and the actual built artifacts (not just the source). Overall this is a well-scoped, well-documented PR — each commit explains the failure mode, the fix, and how it was mutation-verified, which is genuinely useful for future readers. One correctness issue in the shipped bundles needs fixing before merge; the rest are smaller notes. Bug: duplicated/broken
|
CI shard 2/4 printed "requirements-malformed-json was accepted or retried as free-form text" -- a fail-open product regression, on its face. It was not one. Four separate conditions share that message, and the python assertion's own explanation was discarded by the `bad` line. The suite already knew better. The comment above this block records a prior investigation that reached a wrong root cause, and the python already distinguishes a deadline kill (rc 124, the call was KILLED so the contract was never adjudicated) from a genuine fail-open, with the reasoning written out. That text never reached the log. So the conflation the comment warns about was still live in the output path, one layer down from where it was fixed. Now each condition reports separately and the AssertionError text is captured. Mutation-verified by forcing the deadline outcome: the message becomes "review call hit its deadline (rc 124); the malformed contract was never adjudicated, so this run proves nothing about fail-closed behaviour" instead of claiming a text fallback. This is the third instance in this pr of the same shape: an assertion whose failure message names a cause it did not establish. A test that misreports WHY costs more than one that simply fails, because it sends the reader to the wrong file. 43/43 preserved. Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
Contributor
Loki CI Quality Report
FindingsMEDIUM
LOW
Result: PASSED _Generated by Loki Mode at 2026-08-08T01:53:07Z |
|
Reviewed the diff (verify.sh/run.sh runner-selection fix, the four dashboard evidence panels, and the CI-flakiness hardening). Overall this is careful, well-evidenced work — the runner-selection bug fix is verified on both routes with a fixture that pins the exact devDependency-vs-declared-script shape, and the shard-timing/co-location test hardening replaces brittle absolute/window-based assertions with property-based ones for good, well-explained reasons. Two things worth fixing before merge: 1. Unescaped, LLM-generated text goes into
+ '<div style="margin-bottom:3px;"><strong>cause:</strong> '
+ String(x.rootCause || 'not recorded') + '</div>'
+ (x.fix ? '<div ...><strong>fix:</strong> ' + String(x.fix) + '</div>' : '')
...
+ '<span>' + String(x.trigger || 'unknown trigger') + '</span>'and + String(verdict).slice(0, 22) + '</span>'are concatenated straight into This is a real inconsistency within the PR itself: None of the four new test files ( 2. The eval/exec test fix swaps one dynamic-code-execution primitive for another, and the commit message's claim doesn't hold up.
Nothing else stood out — the |
This suite has been patched four times, at four different assertions,
for what a controlled experiment shows is a single environmental
sensitivity.
Measured on a 14-core box:
idle -> 43/43
CPU saturated -> fails, at a DIFFERENT assertion each run (four
distinct ones observed: shard-cancel timing,
malformed-json fail-closed, general review path,
non-blocking advice)
load removed -> 43/43 again
One cause, surfacing wherever it happens to lose the race. The suite
said so itself in one message -- "non-blocking advice made a reviewer
TIMEOUT look like a repairable code defect" -- and another printed
rc=124 (the deadline kill) at elapsed_ms=6874 against a 6000ms bound.
Six independent hardcoded budgets (1s, 2s, 5s, 12s) meant six separate
contention points, which is why fixing them one at a time never
converged. All of them now scale from one factor, keyed on
LOKI_TEST_SHARD -- set only by the sharded CI job, so it identifies the
contended environment precisely, unlike CI=true which is also set for
unsharded jobs that do not have the problem.
Derived bounds scale with the budgets they measure, not with the
literals they were written against; otherwise the bound stays local
while the call it measures gets 4x longer.
Verified: local resolves 1/2/5/12 unchanged, CI resolves 4/8/20/48, and
idle stays 43/43. Nothing is weakened -- a budget only has to be big
enough to reach the code under test, and locally the tight values stay
so a real slowdown is still caught fast. Override with
LOKI_REVIEW_TIMEOUT_SCALE to reproduce either side.
Claude-Session: https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Seven commits. Four restore the dashboard evidence panels (they were built earlier in this session but never reached main -- a parallel process reset main to a different lineage); three are new.
The new work
loki verifyblocked on a clean tree, permanently. It chose its test runner withgrep '"jest"' package.json, which matches a devDependency. On this repo jest is a devDependency with no jest config whilescripts.testrunsbash -nplusnode --test, so verify ran jest, jest globbed 895 files that are not jest tests, and every run returned BLOCKED for a defect that does not exist. Fixed on both routes (verify.shandrun.sh).run.shhad already diagnosed this exact trap in a comment -- "with a JSON parser, not grep (grep would false-positive on devDeps)" -- but fixed only its trailingelse, leaving three grep branches ahead of it to shadow the fix.The dependency-audit finding now says whether CVEs are in the SHIPPED tree. This repo reports 4 high while
npm audit --omit=devreports 0. The old wording read as "the shipped product is vulnerable". Costs 424ms (measured).A browser harness for the evidence panels. Three panels shipped with unit tests over stubbed fetches; all passed while the real page rendered three empty panels. Every substitute for a browser was lying (a brace-matcher truncated a function mid-comment; the stub resolved a bare array against wrapper-object endpoints). 7s, in the fast tier.
A timing-fragile assertion hardened.
-lt 6000against a 12s timeout measured 3395ms locally -- 1.77x headroom -- and went red on CI shard 2/4. A re-run at the same SHA came back green, which is what separates a flake from a regression.Verification
local-ci.sh: greenWhy a PR and not a push to main
Five other processes are active on this repo. An earlier
--force-with-leaseof mine succeeded against a stale lease and briefly overwrote main; a parallel process restored it. Nothing was lost, but main is contended, so this goes through review rather than a force.https://claude.ai/code/session_01WusSvWXEsW1F1iS4BYCKbT