Repository navigation
test: live interaction coverage gate for the chat surface - #809
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 116 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a live, authenticated Playwright chat-coverage harness. It discovers application surfaces, proves control effects, validates exclusions and coverage floors, generates reports, and runs self-checks and live sweeps in CI. ChangesLive chat coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CI
participant Playwright
participant Auth
participant Chat
participant CoverageReports
CI->>Playwright: run self-check or gated live sweep
Playwright->>Auth: create authenticated browser state
Auth->>Chat: complete console and Hive sign-in flow
Playwright->>Chat: discover surfaces and interact with controls
Chat-->>Playwright: return UI, navigation, and network evidence
Playwright->>CoverageReports: write coverage ledgers and summaries
CI-->>CoverageReports: upload coverage artifacts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
Findings filed from this run, deduped against open issues:
Not filed, expected and already tracked: the model picker still listing the embedding, speech-to-text and text-to-speech aliases (#792, fix unmerged in #814). |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
apps/web-console/tests/e2e/support/live-auth.ts (1)
85-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
signInToChatexists twice with identical logic. Both files implement the same chat sign-in recipe: the same console probe, the samecontinue with hiveandapproveselectors, the same 4000/8000/8000 waits, and the samenew chatreadiness check. Two copies drift when an Open WebUI button name or timing changes, and only one of the two callers would then break.
apps/web-console/tests/e2e/support/live-auth.ts#L85-L129: keep this as the single implementation, sinceapps/web-console/e2e/chat-coverage/auth.setup.tsimports it.apps/web-console/tests/e2e/support/live-auth.mjs#L313-L350: remove the duplicated body and delegate to the shared implementation, or extract the shared steps into one module that both entry points import.🤖 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 `@apps/web-console/tests/e2e/support/live-auth.ts` around lines 85 - 129, Keep signInToChat in apps/web-console/tests/e2e/support/live-auth.ts lines 85-129 as the single implementation, preserving its console probe, selectors, waits, and New Chat readiness check. Update apps/web-console/tests/e2e/support/live-auth.mjs lines 313-350 to remove the duplicated sign-in body and delegate to or import the shared implementation so both callers use the same logic.apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts (1)
618-627: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe removed-route check accepts any page that contains the text "404".
body.includes("404")matches a version string, a request id, or a chat title that contains those digits. A route that still renders would then pass as removed. Tighten the check, for example by asserting on the HTTP status returned bypage.goto.🤖 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 `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts` around lines 618 - 627, Tighten the removed-route validation in the REMOVED.forbiddenRoutes loop so incidental “404” text cannot mark a still-rendered page as gone. Capture the response returned by page.goto and use its HTTP status, together with the existing not-found text or redirect checks, to determine whether the route is removed.apps/web-console/e2e/chat-coverage/lib.ts (1)
421-439: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
readStateandstateOfalready disagree oninput[type=file].
stateOfat lines 134-139 returnsnullfor a file input.readStatereturnsinput.valuefor it. The two readers describe the same concept, so a control read through one path and compared through the other can produce a false difference. Extract one shared in-page reader and call it from both places.🤖 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 `@apps/web-console/e2e/chat-coverage/lib.ts` around lines 421 - 439, Unify the duplicated control-state logic used by readState and stateOf by extracting a shared in-page reader, ensuring input[type=file] returns null in both paths. Update both functions to call the shared reader while preserving the existing handling for all other control types.
🤖 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 `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts`:
- Around line 479-525: Guard the existing coverage.json read and parse in the
tracker initialization before merging results: wrap fs.readFileSync and
JSON.parse in try/catch, and use an empty tracker when either operation fails.
Preserve the current controls extraction for valid files and leave the
subsequent cleanup, assertions, and report generation unchanged.
In `@apps/web-console/e2e/chat-coverage/lib.ts`:
- Around line 94-178: Update ENUMERATE to remove existing data-cov-id attributes
from all elements in the scoped root before assigning new IDs to the current
visible elements. Preserve the existing covId generation and locate behavior
while ensuring each current surface has no stale duplicate identifiers.
- Around line 261-288: Update proveByClickInner to perform all request,
download, filechooser, context page-listener, and MutationObserver cleanup in a
finally block. In proveByClick, signal cancellation when the budget timer wins
so the still-running inner operation stops handling stale events and cannot
affect later controls, while preserving the existing timeout Result.
In `@apps/web-console/e2e/chat-coverage/removed-surfaces.json`:
- Around line 5-14: Add /workspace/skills to the forbiddenRoutes array alongside
the existing workspace routes, keeping issue `#772`, unless the $comment
explicitly documents why Skills has no route. Ensure the forbidden control and
direct-navigation route checks remain aligned.
In `@apps/web-console/e2e/chat-coverage/results/2026-08-08-live-run.json`:
- Around line 5-9: The recorded coverage results violate the assertions in
chat-coverage.spec.ts because errors and unproven controls remain. Before
merging, update the affected surfaces to eliminate the failures, or add each
accepted control with a justification to inert-registry.json; alternatively gate
the suite so it is not a required check, ensuring no required test remains
failing.
- Around line 1-4: Rename the live-run results file to match its generatedAt
date, 2026-08-10. Align the JSON schema with the output written by
chat-coverage.spec.ts—using excused, surfaceErrors, and results—or explicitly
mark the artifact as hand-curated in note; preferably preserve the raw
coverage.run.json so tooling can compare future runs.
In `@apps/web-console/e2e/chat-coverage/surfaces.ts`:
- Around line 94-102: Update ensureSidebar to verify the sidebar reaches the
requested state after any toggle click, rather than silently succeeding when the
expected named toggle is absent. Try the available open/close toggle names,
assert the resulting sidebar state, and throw a distinct error when neither
toggle is found; preserve the existing settle delay after successful
interaction.
- Around line 256-284: Update the signature helper in enumerateSurface so
baseline and surface comparisons ignore enumerate’s trailing duplicate suffixes
such as `#2` and `#3`, while still removing the surface prefix. Use normalized
signatures for both baseline construction and delta filtering so repeated
controls are not incorrectly counted as new.
---
Nitpick comments:
In `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts`:
- Around line 618-627: Tighten the removed-route validation in the
REMOVED.forbiddenRoutes loop so incidental “404” text cannot mark a
still-rendered page as gone. Capture the response returned by page.goto and use
its HTTP status, together with the existing not-found text or redirect checks,
to determine whether the route is removed.
In `@apps/web-console/e2e/chat-coverage/lib.ts`:
- Around line 421-439: Unify the duplicated control-state logic used by
readState and stateOf by extracting a shared in-page reader, ensuring
input[type=file] returns null in both paths. Update both functions to call the
shared reader while preserving the existing handling for all other control
types.
In `@apps/web-console/tests/e2e/support/live-auth.ts`:
- Around line 85-129: Keep signInToChat in
apps/web-console/tests/e2e/support/live-auth.ts lines 85-129 as the single
implementation, preserving its console probe, selectors, waits, and New Chat
readiness check. Update apps/web-console/tests/e2e/support/live-auth.mjs lines
313-350 to remove the duplicated sign-in body and delegate to or import the
shared implementation so both callers use the same logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f088e664-7848-4426-a442-b4adfa9c992e
📒 Files selected for processing (11)
.gitignoreapps/web-console/e2e/chat-coverage/auth.setup.tsapps/web-console/e2e/chat-coverage/chat-coverage.spec.tsapps/web-console/e2e/chat-coverage/inert-registry.jsonapps/web-console/e2e/chat-coverage/lib.tsapps/web-console/e2e/chat-coverage/playwright.chat-coverage.config.tsapps/web-console/e2e/chat-coverage/removed-surfaces.jsonapps/web-console/e2e/chat-coverage/results/2026-08-08-live-run.jsonapps/web-console/e2e/chat-coverage/surfaces.tsapps/web-console/tests/e2e/support/live-auth.mjsapps/web-console/tests/e2e/support/live-auth.ts
|
Body updated and read back from the API to confirm it landed: the 196 of 222 figure, the per-surface table, how the denominator is derived, the four excluded surfaces and the fixture result are all present. Three of the four excluded surfaces (sidebar, chat-item-menu, chat-message-actions) become sweepable once #834 lands, which gives the Open WebUI sidebar toggle an accessible name. composer-controls has a separate cause and stays excluded. The earlier body write went missing because gh pr edit fails on a projects-classic GraphQL error without writing anything. Filed as #842 with the gh api -X PATCH workaround and the read-back rule. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts`:
- Around line 716-731: Update the test "no excluded surface waits on an issue
that already closed" to collect issue lookups whose GitHub responses are not
ok(), then fail the test with those failures instead of continuing to
expiredExclusions(EXCLUSIONS, closed). Preserve the existing missing-token skip
and closed-state collection, while ensuring 401, 403, 404, and rate-limit
responses cannot allow the expiry gate to pass silently.
In `@apps/web-console/e2e/chat-coverage/lib.ts`:
- Around line 615-623: Update the validation around the exclusion entry checks
near the issue/permanent rules so an issue is considered present only when it is
a positive safe integer. Reject or report zero, negative, non-integer, and
unsafe numeric issue identifiers before downstream GitHub validation, while
preserving the existing mutual-exclusion and non-permanent checks for valid
issue values.
- Around line 344-357: Update the response-listener flow around onResponse to
register a requestfailed listener that records meaningful transport failures
using Request.url(), method(), and failure(). Ensure this listener is removed
alongside the existing listeners, and move all listener cleanup into a finally
block covering settling and state inspection so handlers are always released
before returning proof.
In `@docs/proof/chat-interaction-coverage-2026-08-10/README.md`:
- Around line 31-42: Update the section heading to state five surfaces, add the
missing user-menu entry with its “click pass aborted” error, and reword the line
beginning with “#834” so it no longer starts with a hash while preserving the
deployment reference.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb3fe458-57ff-4c4e-b61e-101c62d3508a
📒 Files selected for processing (6)
apps/web-console/e2e/chat-coverage/chat-coverage.spec.tsapps/web-console/e2e/chat-coverage/lib.tsapps/web-console/e2e/chat-coverage/surface-exclusions.jsonapps/web-console/e2e/chat-coverage/surface-floors.jsondocs/proof/chat-interaction-coverage-2026-08-10/README.mddocs/proof/chat-interaction-coverage-2026-08-10/coverage.run.json
3525bf0 to
ab1ffa2
Compare
Stage 6, adversarial review, pipeline modeStreams selected by diff shape: this diff carries a live OAuth flow and a CI artifact upload, so the security stream is mandatory and it earns the Codex adversarial stream as well.
Two of the four available streams were blocked by their own infrastructure, so the coverage of this review is thinner than the pipeline calls for. That is stated plainly rather than being allowed to read as three green streams. Plain adversarial pass, findings1. A response proved a control that did not cause it. Blocking, fixed. 2. Three strings reached the ledger and the CI log unredacted. Blocking, fixed. Both were caught by asking the two questions the earlier reviews established for this file: where can a credential reach something durable, and where can a control be recorded as proven by something it did not cause. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
apps/web-console/e2e/chat-coverage/lib.ts (1)
105-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe doc comment above
NOT_MOUNTEDdescribes a different rule.The comment explains asset and socket.io request filtering, which
isMeaningfulRequestimplements at lines 122-127.NOT_MOUNTEDis a set of HTTP status codes that mark an endpoint as absent. A reader who edits the set based on this comment changes the wrong rule.♻️ Proposed fix
-/** - * Assets and background chatter never count as evidence that a control did - * something. socket.io is the important exclusion: Open WebUI long-polls it - * every few seconds, so counting it would hand a free pass to every control - * whose click happened to land near a poll. - */ +/** + * Statuses that say the endpoint a control called is not there, or failed + * outright. A click that produces one of these is not proof: it is a defect in + * the surface, reported ahead of every positive channel. + */ const NOT_MOUNTED = new Set([404, 405, 410, 501]);🤖 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 `@apps/web-console/e2e/chat-coverage/lib.ts` around lines 105 - 111, Update the doc comment above NOT_MOUNTED to describe its actual purpose: the HTTP status codes that indicate an endpoint is absent or not mounted. Remove the unrelated asset and socket.io request-filtering explanation, which belongs with isMeaningfulRequest.apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts (1)
605-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild a new tracker record instead of deleting keys in place.
Lines 605-609 delete keys from
tracker, and line 611 assigns into the same object. The repository guideline requires new objects instead of mutation, and append-only ledgers.coverage.jsonis a cross-slice ledger. A rebuilt record keeps the pruning rule explicit and keeps the read result immutable.♻️ Proposed refactor
- const sweptSurfaces = new Set(results.map((r) => r.surface)); - const seenKeys = new Set(results.map((r) => r.key)); - for (const key of Object.keys(tracker)) { - if (sweptSurfaces.has(tracker[key].surface) && !seenKeys.has(key)) { - delete tracker[key]; - } - } + const sweptSurfaces = new Set(results.map((r) => r.surface)); + const seenKeys = new Set(results.map((r) => r.key)); + // A surface swept in this run is fully described by this run, so its stale + // keys are dropped by rebuilding rather than by mutating what was read. + const kept: Record<string, TrackedResult> = Object.fromEntries( + Object.entries(tracker).filter( + ([key, entry]) => !(sweptSurfaces.has(entry.surface) && !seenKeys.has(key)), + ), + ); if (!partial) { - for (const r of results) tracker[r.key] = { ...r, lastSeen: generatedAt }; + const merged: Record<string, TrackedResult> = { + ...kept, + ...Object.fromEntries(results.map((r) => [r.key, { ...r, lastSeen: generatedAt }])), + }; fs.writeFileSync( trackerPath, JSON.stringify( { generatedAt, target: report.target, - summary: summarise(Object.values(tracker)), - controls: tracker, + summary: summarise(Object.values(merged)), + controls: merged, }, null, 2, ), ); }As per coding guidelines: "Create new objects instead of mutating existing ones; keep ledgers append-only."
🤖 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 `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts` around lines 605 - 611, Refactor the tracker update flow around the existing tracker-pruning loop to avoid deleting keys or assigning records in place. Build a new tracker object that retains unseen entries not matching swept surfaces, then merge new results into that object when partial is false, and use the rebuilt record for subsequent reads while preserving the existing pruning rule and append-only ledger behavior.Source: Coding guidelines
apps/web-console/e2e/chat-coverage/break-proof.spec.ts (1)
197-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid in-place accumulator mutation in these TypeScript tests.
apps/web-console/e2e/chat-coverage/break-proof.spec.ts#L197-L214: replaceclosed.add(...)andunreadable.push(...)with immutable accumulator updates.apps/web-console/tests/unit/chat-coverage-lib.test.ts#L181-L200: replaceseen.set(...)with a newly constructedMap.apps/web-console/tests/unit/chat-coverage-lib.test.ts#L207-L220: replacebelow.push(...)with a newly constructed array.As per coding guidelines, “Create new objects instead of mutating existing ones; keep ledgers append-only.”
🤖 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 `@apps/web-console/e2e/chat-coverage/break-proof.spec.ts` around lines 197 - 214, Replace in-place accumulator mutations in the indicated sites: in apps/web-console/e2e/chat-coverage/break-proof.spec.ts lines 197-214, update closed and unreadable immutably; in apps/web-console/tests/unit/chat-coverage-lib.test.ts lines 181-200, replace seen.set with a newly constructed Map; and in lines 207-220, replace below.push with a newly constructed array. Preserve the existing accumulation behavior and append-only ledger semantics.Source: Coding guidelines
🤖 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 @.github/workflows/chat-coverage.yml:
- Around line 17-19: Restrict the workflow-level token permissions in
.github/workflows/chat-coverage.yml to contents: read and issues: read,
preserving the self-check’s issue access. Update both checkout steps at the
referenced locations to set persist-credentials: false.
- Around line 62-69: The live-sweep job is currently skipped for ordinary pull
requests, so live coverage is not an enforced merge gate. Update the workflow
around live-sweep to run an always-present approved live sweep status for every
pull request, and configure the corresponding check as required branch
protection for the main branch so merges are blocked when it fails or is
missing.
In `@apps/web-console/e2e/chat-coverage/break-proof.spec.ts`:
- Around line 153-159: Update the SELF_TEST_HTML fixture to expose a click
counter or data-* marker for the “Delete Everything” control, then use that
marker around the checkWithoutFiring call and assert it remains unchanged
afterward. Keep the existing destructive.proven, destructive.proof, and
isDeferred assertions intact.
In `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts`:
- Around line 551-561: Update the excused filtering logic around results and
registryHits to require exact control-key identity instead of substring
matching: replace r.key.includes(key) with an exact comparison, or compare
identityOf(r) to identityOf the registry entry if that is the established key
model. Add identityOf to the existing ./lib imports only if using that approach,
while preserving the existing justification-length requirement and failure
filtering.
In `@apps/web-console/e2e/chat-coverage/lib.ts`:
- Around line 481-494: Add the same request-identity guard used by onResponse to
onRequestFailed: after the existing meaningful-request check, return when
mine.has(r) is false, so only failures belonging to the clicked request are
added to transportFailed.
In `@apps/web-console/e2e/chat-coverage/playwright.chat-coverage.config.ts`:
- Around line 77-82: Update the “chat-coverage-setup” project configuration to
set its runnable condition using the existing runnable symbol, so auth.setup.ts
is scheduled only when explicitly enabled. Preserve the project’s OAuth trace,
video, and screenshot settings and keep the credential-free break-proof project
runnable independently.
In `@apps/web-console/scripts/update-chat-coverage-floors.mjs`:
- Around line 38-45: Update parseArgs to validate every flag against
--allow-lower and --dry-run, and reject any unknown flag before returning. Also
reject more than one positional path, while preserving the existing default
ledger and recognized-flag behavior so main cannot read or write files with
invalid arguments.
In `@apps/web-console/tests/e2e/support/live-auth.ts`:
- Around line 163-165: Update the page.waitForURL predicate in the live
authentication flow to require both the Chat URL origin and a pathname other
than /auth. Keep the existing timeout and subsequent “New Chat” wait unchanged.
In `@docs/proof/chat-interaction-coverage-2026-08-10/README.md`:
- Around line 124-128: Update the shell command code fence in the README near
the e2e:chat-coverage:self-check example to declare the sh language after the
opening fence, preserving the commands and output unchanged.
---
Nitpick comments:
In `@apps/web-console/e2e/chat-coverage/break-proof.spec.ts`:
- Around line 197-214: Replace in-place accumulator mutations in the indicated
sites: in apps/web-console/e2e/chat-coverage/break-proof.spec.ts lines 197-214,
update closed and unreadable immutably; in
apps/web-console/tests/unit/chat-coverage-lib.test.ts lines 181-200, replace
seen.set with a newly constructed Map; and in lines 207-220, replace below.push
with a newly constructed array. Preserve the existing accumulation behavior and
append-only ledger semantics.
In `@apps/web-console/e2e/chat-coverage/chat-coverage.spec.ts`:
- Around line 605-611: Refactor the tracker update flow around the existing
tracker-pruning loop to avoid deleting keys or assigning records in place. Build
a new tracker object that retains unseen entries not matching swept surfaces,
then merge new results into that object when partial is false, and use the
rebuilt record for subsequent reads while preserving the existing pruning rule
and append-only ledger behavior.
In `@apps/web-console/e2e/chat-coverage/lib.ts`:
- Around line 105-111: Update the doc comment above NOT_MOUNTED to describe its
actual purpose: the HTTP status codes that indicate an endpoint is absent or not
mounted. Remove the unrelated asset and socket.io request-filtering explanation,
which belongs with isMeaningfulRequest.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc64bafe-56f2-42d6-a000-43e491a24823
📒 Files selected for processing (20)
.github/workflows/chat-coverage.yml.gitignoreapps/web-console/e2e/chat-coverage/auth.setup.tsapps/web-console/e2e/chat-coverage/break-proof.spec.tsapps/web-console/e2e/chat-coverage/chat-coverage.spec.tsapps/web-console/e2e/chat-coverage/data.tsapps/web-console/e2e/chat-coverage/lib.tsapps/web-console/e2e/chat-coverage/playwright.chat-coverage.config.tsapps/web-console/e2e/chat-coverage/removed-surfaces.jsonapps/web-console/e2e/chat-coverage/results/2026-08-10-morning-live-run.jsonapps/web-console/e2e/chat-coverage/surface-floors.jsonapps/web-console/e2e/chat-coverage/surfaces.tsapps/web-console/package.jsonapps/web-console/playwright-spec-manifest.jsonapps/web-console/scripts/update-chat-coverage-floors.mjsapps/web-console/scripts/verify-spec-collection.mjsapps/web-console/tests/e2e/support/live-auth.tsapps/web-console/tests/e2e/support/redact.tsapps/web-console/tests/unit/chat-coverage-lib.test.tsdocs/proof/chat-interaction-coverage-2026-08-10/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web-console/e2e/chat-coverage/removed-surfaces.json
- .gitignore
- apps/web-console/e2e/chat-coverage/auth.setup.ts
- apps/web-console/e2e/chat-coverage/surfaces.ts
Enumerates every focusable control the chat app renders, from the DOM rather than from a list, and requires each one to produce an observable effect against a running deployment. Toggles and settings additionally have to survive a reload, because Open WebUI keeps user settings in a persisted config blob and a control that flips visually while saving nothing looks correct until the next page load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coverage sweep re-establishes its surface before every control, so a click that changes what is on screen cannot be charged to the next control; proves an effect from a navigation, a network call, an overlay opening or closing, a new tab, a download, a file chooser, or a change in what is rendered; and requires every setting to survive a reload, with a reverted control re-checked on its own before it is called a defect. live-auth gains signInToChat. A minted session authenticates the console origin only: Open WebUI keeps its own session cookie that just its OAuth callback can set, so a context carrying only the minted session lands on /auth and reads as a broken login. The helper now carries the session through the real Continue with Hive hop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ixture The gate-can-fail check is a fixture with one wired button and one whose handler was never attached. Live sabotage was tried first and rejected: blocking the real Search button's events did stop its overlay, but the verdict still landed on a weaker signal, so the demonstration was not clean. The fixture is deterministic, runs in seconds, needs no deployment, and fails loudly the moment the prover stops telling the two apart, which is the property that matters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r its denominator This engine never registered a response listener at all, so every HTTP status was invisible to it. A click that answered 500 and rendered an error toast produced a meaningful request and a change to the render, and both of those were counted as proof, so a control that visibly did not work came back proven twice over. It now reads statuses. A response of 404, 405, 410, 501 or any 5xx means the endpoint is not mounted or the server failed on it. Any other 4xx means the server read the request and refused it. Either one is a failure, judged before any positive channel, so a refusal can no longer be mistaken for an effect. A change to the render whose only new content is an error surface is a failure for the same reason. No harnessSuppliedInput exception is carved out here, unlike the console gate. Nothing in this sweep feeds a control synthetic input before clicking it: typed values are proven by reading state back after a reload, not by clicking, so every 4xx a click provokes is unexpected by construction. surface-floors.json records the control count of every surface a run swept. A run that enumerates fewer controls on a surface than its floor fails, and so does a surface with no floor at all, because the ratio is proven over enumerated and a surface that renders less than it used to would otherwise shrink the denominator and hold the percentage up while the app degrades. surface-exclusions.json replaces the hand written note in the recorded ledger. composer-controls is excluded, owned, and tied to #844, and the gate fails once that issue closes, so the exclusion cannot outlive its reason. The self test now carries four buttons rather than two: one that works, one with no handler, one whose endpoint answers 500 and shows a toast, and one that only shows an error. The last two are the cases this change exists for and both must come back unproven.
… floors 248 of 270 proven, 91.9 percent, against the 196 of 222 recorded on 2026-08-08. Two controls the old predicate called proven are broken and are now unproven: Settings General "Admin Settings" fires a request that answers 404 and was counted as network proof, and the composer's "Upload Files" renders "File not found." and nothing else and was counted as dom proof. The ratio rose despite those two because the denominator changed shape: search enumerated 106 controls this run against 50 last time and all of them proved. The two runs are not directly comparable and the write up says so. Three surfaces still refuse to sweep after #834 deployed. sidebar and workspace:knowledge enumerate zero controls, and chat-item-menu and chat-message-actions time out opening. They stay as surface errors that fail the gate rather than becoming exclusions, because the gate should stay red until they sweep. A surface that enumerated zero controls no longer gets a floor written for it. A floor of zero is a bar nothing can fail, and it would go stale the moment the surface started rendering again.
…ge notes Two independent surfaces timed out under our own agent load on the same evening, the console box on seven routes and the Supabase magic link mint once, and both recovered unattended. Recorded rather than quietly re-run. The search surface is the chat list, so its control count is the number of chats the account happens to hold. That is why this run is not comparable to the previous one, and why the headline percentage is not yet a measure any decision should rest on.
The search surface enumerated 106 controls resolving to 14 unique names, because it is the chat list and every row contributes its own Chat Menu button and New Chat link. The ratio was therefore partly measuring how many chats the demo account happens to hold, much of which is litter from our own automated runs, rather than measuring the product. The primary figure is now distinct control identities proven over distinct identities enumerated. An identity is the enumerated key with its duplicate ordinal stripped, so 52 rows of the same chat menu collapse to one thing a user can do. The raw instance ratio stays in the report, labelled secondary and labelled not comparable between runs. The trade-off is that one identity could hide a control that works on row 1 and fails on row 7. Two things prevent that. An identity counts as proven only when every instance of it proved, so a single failing instance makes the identity unproven. And the gate already fails on any unproven result at all, so the failing instance fails the run whether or not its siblings passed. Each identity carries its instance count in the ledger. Restated under the new definition: 144 of 165 identities, 87.3 percent. The 248 of 270 instances figure this branch previously reported is superseded.
…rites Reworks the engine after an adversarial review returned BLOCK. Destructive controls are no longer clicked with their write aborted and then recorded as proven. An aborted request has no server verdict, so a button wired to an endpoint that is gone came back green, which is the exact defect class the gate exists to catch. They now get an assertion that they are present, enabled and named, recorded in their own "not-fired" category that is neither proof nor a failure. The network-level abort stays as a data-safety net only, and any control that trips it is recorded as not-fired for the same reason. Network proof is no longer awarded for background chatter. Only socket.io was excluded, by name, so any REST poll landing inside a control's settle window counted as that control's proof. Each surface is now sampled while idle first, and every request signature the page produces on its own is excluded from proof on that surface. The unmeasured "more than eight mutations" fallback is deleted; the rendered-text signature is the measured test and the count survives only in the failure detail. The in-test retry now only retries failures that say nothing about the control (surface would not re-open, control gone, no verdict within budget). A clean verdict of "does nothing" is final on the first attempt, instead of quietly reinstating the retries the config sets to zero. An unswept floor key is now a hard failure. The guard iterated only what was swept, so a surface that vanished took its own floor with it and left the denominator smaller with nothing said. Settings and workspace discovery throw instead of returning an empty list for the same reason. Credential hygiene: every URL this suite writes to a message, a ledger or a console line goes through redactUrl, which scrubs code, state and token parameters in the fragment as well as the query string. Style: the JSON data files are read through validators rather than a cast on JSON.parse, the window casts are replaced by a global declaration, and the non-null assertions in the click pass are gone.
The gate was wired to nothing. No workflow and no npm script referenced its config, and its spec files were invisible to the spec collection guard, so rebasing onto main would have turned the required Web console job red on two undeclared specs. Floors no longer ratchet downward. COV_FLOORS=update rewrote surface-floors.json from the run in progress and skipped checking the floors in that same pass, and this branch shipped the evidence of what that does: three surfaces that had genuinely degraded (Interface 56 to 48, General 17 to 16, Audio 8 to 7) had the smaller numbers written in as the new baseline with no reason recorded. The higher floors are restored from the 2026-08-08 ledger, the sweep now only ever reads them, and raising or lowering one is a separate program run against a recorded ledger, in its own commit, which refuses to lower anything without --allow-lower. A unit test holds the committed floors at or above what a recorded live run enumerated. The break proof no longer hides behind credentials. It needs no deployment, so it has its own project and its own file, and it runs in CI on every pull request through the new workflow, alongside the registry and exclusion checks. The exclusion-expiry check fails rather than skips when it cannot read issue state, so an exclusion cannot outlive its issue in silence. The fixture gained a fifth button: a dead control on a page that polls underneath it, which the old predicate proved on the page's own background traffic. Credential hygiene in the harness: traces and videos are off by default across the whole config and unconditionally off for the project that walks the OAuth hop. Both record the session cookies, and a trace of the sign-in hop records the callback's code and state, none of which tools/lint-no-token-in-proof-captures.mjs can see inside a binary. The proof directory is made true: the identity figure is now derived from the recorded run's own results by the gate's summarise(), with the command to reproduce it printed in the README, and the ledger carries the recomputed summary. The README's surface-error list matches the ledger's five entries rather than three, and the destructive controls counted as proven in that run are called out as the practice this change removes. live-auth.mjs keeps one implementation of the chat hop, in live-auth.ts, which is where the only caller imports it from. The duplicate added to the .mjs had no caller and is deleted.
Five findings from the automated review on this pull request, all in the same area of the engine. A transport failure emits request and then requestfailed, and no response at all, so a connection reset or an aborted write was credited as network proof with no verdict behind it. It is now its own listener and its own failure branch. Listener cleanup moves into a finally. Everything between registering the listeners and reading the verdict can throw, and the budget race can abandon the call outright, which used to leave five page listeners and a live MutationObserver attached for the rest of a 45 minute run. data-cov-id tags are cleared before each enumeration. The ids are positional, so an element tagged on a previous pass and no longer visible kept an id the next pass handed to a different element, and the locator then matched two nodes. ensureSidebar could not tell "already in the requested state" from "neither toggle is on screen". It now asserts the state it was asked for, which is the difference between a sidebar surface that enumerates zero controls and an error that says why. An exclusion's blocking issue must be a positive integer. A zero, a float or a string would be looked up as an issue that cannot exist, GitHub would answer 404, and the exclusion could never expire. /workspace/skills joins the forbidden routes: Skills was already forbidden as a control, and the route it opens was not checked.
The file called 2026-08-08-live-run.json carries generatedAt 2026-08-10T07:31Z. Both runs in this pull request are from 2026-08-10; the earlier one was mislabelled, and every reference to a 2026-08-08 run inherited that.
…unfireable controls Network proof now needs a response, not just a request. A control pointing at an endpoint that hangs emitted a request, never got an answer, and was proven for ever: the same missing verdict that made an aborted write look like proof. Proof is now a 2xx or 3xx response inside the settle window, and a request nothing answered is reported as exactly that. This costs no per-control inventory, which is why the earlier rejection of this finding was wrong. not-fired stops being a free pass. It was filtered out of the failure list, so a control the ledger proves dead would have gone green the moment its name matched the destructive pattern. It now fails the gate like any other unfireable control unless inert-registry.json carries a justification for its key, and it is still reported in its own column so it can never be read as coverage. The destructive classifier reads the control's own accessible name. It read the ambient label, which falls back to up to 140 characters of ancestor text, so one "Delete All Chats" heading in Settings > Data Controls made every button in that panel destructive: two controls that ledger proves dead would have been hidden and three genuinely proven controls thrown away. A disabled control is no longer proof. disabled-with-reason counted as coverage, so #846's Admin Settings could have been "fixed" by disabling it with a title attribute and the ratio would have RISEN. Disabled is now not-fired, and aria-describedby is no longer accepted as a reason, because it holds an element id rather than an explanation. Destructive stateful controls are checked rather than flipped. flip() fills every text input and clicks Save, and only the click pass consulted the classifier, so a "Reset to default" select was being activated for real. A sliced run no longer writes the cross-run tracker. It merged a slice into whatever a previous run had left and then printed summarise() over the mixture with no marker. The offline ledger-derivation script is deleted and the recorded ledger carries the summary that run actually wrote. No shipped code path could produce the 144/165 headline: it was derived from a pre-rework ledger whose proven count includes destructive clicks that are no longer proof.
…pin the floors The CI artifact uploaded a whole directory, and Playwright's JSON report was written into it. That report carries raw error text, and the sign-in hop walks an OAuth callback, so a sign-in that outran its ninety second wait would have published a live authorization code and state in a fourteen day artifact. Three changes, because one is not enough: the Playwright reports are written outside the uploaded directory, the upload names two files rather than a directory, and signInToChat now redacts the framework's own failure text at the point where the credential is still inside a string this repository owns. redactUrl could never have reached it, and the proof-capture lint only scans docs/proof/. Redaction moves into tests/e2e/support/redact.ts, one implementation for the suite and the sign-in helper, matching credential parameter names by pattern rather than by an exact list that missed id_token and provider_token, and covering the hash-router shape where the credential sits in the fragment's own query string. The floor pin test compares against every recorded run rather than one of them. Against the morning ledger alone, search was pinned at 50 while the committed floor says 106, so a fifth of the whole denominator could have been signed away with the test still green. A floor key missing entirely is now a failure rather than a skip. Four surfaces that do not sweep today (sidebar, chat-item-menu, chat-message-actions, workspace:knowledge) get a floor of 1, so deleting the link that reaches one of them fails the gate instead of quietly removing it from the denominator. That was a live route from red to green by making the app worse. Delta enumeration subtracts the baseline as a multiset. Matching full keys meant an overlay that injects or hides one identically-keyed element shifted every later ordinal, so a base control could survive subtraction and be clicked in the wrong context, or a genuinely new control could be cancelled by a base entry whose ordinal it inherited. The claim that the service-role key never enters the Playwright worker was wrong and is corrected in both places that made it. The key is ordinary job-level environment, the config reads it, and the spawn forwards all of process.env. Only its use is isolated. Cover for the branches this changes: the fixture gains a control whose request nothing answers, one whose request dies in transport, a destructive one and a disabled one, and the exclusion bookkeeping is now asserted against deliberately broken input rather than only against the valid committed file.
The 144 of 165 headline came from a run that predates this rework, whose proven count includes destructive controls clicked with the write aborted and controls proven by a request no server answered. Neither is proof any more. It was also derived by a script with none of the guards the floor updater has, from a ledger carrying five surface errors that violates four of the floors this pull request restores. The directory keeps the ledger, which is a real record, and states what it is worth. The figure returns when a sweep runs under the current rules and passes its own checks. The identity metric's ceiling is named rather than hidden: it keys on rendered names, and the chat list names its rows after the chats, so no number from that surface is a product measurement yet.
The search surface is the chat list, one row per chat, so it enumerated 50 controls on one run and 106 on the next with no product change between them. A floor there fails whenever somebody tidies the account, and the fix everyone reaches for when a floor is wrong for a reason nobody caused is to lower it, which is the ratchet this file exists to stop. It is now listed under dataDriven and carries no floor at all. The surface is still swept and every control on it still has to prove itself; only the denominator guard is off, and the file says why.
Removing the search floor entirely fixed the false red (its control count is one row per chat, so grooming the account moved it 50 to 106) and opened the hole this same review found elsewhere: with no floor key, the surface can be deleted outright and nothing notices. It keeps a floor of 1. The count is not pinned, so tidying chats cannot red the gate, but the surface still has to exist and render something. dataDriven now documents why that floor is a presence bar rather than a recorded count, and the pin test enforces both halves.
…edger's last raw URLs Two findings from this pull request's own adversarial pass. A response only proves a control when it answers a request that control made. Matching on the settle window alone credited a response to something the page had started before the click, which on a slow surface is exactly the coincidence the idle sample was added to remove. The request objects seen during the window are now held and the response has to belong to one of them. Three strings still reached the ledger unredacted: the intercepted-write list, the surface error list and the failing assertion's own message. The last is the worst of the three, because it lands in the CI log, the JSON report and the HTML report at once, and its contents are mostly Playwright's error text, which quotes the URLs a failed navigation walked through on a suite that signs in through an OAuth callback.
Fourteen casts inside the enumerator asserted an element type the compiler had no way to check, on nodes read straight out of whatever the deployment rendered. A select that is not a select answered undefined and the enumerator recorded the control's state as empty. instanceof asks the question rather than asserting the answer, and it costs nothing here.
5f5426d to
4f712ff
Compare
sakibsadmanshajib
left a comment
There was a problem hiding this comment.
Stage 6 adversarial review, pipeline mode, dynamic selector. Diff shape: a live OAuth flow plus a CI artifact upload, so the security stream is mandatory and the Codex adversarial stream is earned.
| stream | verdict |
|---|---|
| CodeRabbit CLI | SKIPPED, see inline comment |
| Codex adversarial | SKIPPED, see inline comment |
| Plain adversarial, ticket first | RAN, two blocking findings, both fixed, see inline comments |
| ecc:code-review, typescript-reviewer, security-reviewer | NOT RUN, capability gap, see inline comment |
Two of the four available streams were blocked by their own infrastructure and one group could not be dispatched from here. That is stated as blocked rather than allowed to read as green.
A transport failure now has to belong to the click. onRequestFailed lacked the request-identity check onResponse had, so an unrelated failure could produce a false 'its request never reached a server' verdict against whatever control happened to be under test. The sign-in no longer settles on the sign-in page. waitForURL matched on origin alone, and /auth is on the chat origin too, so a sign-in that had not actually completed satisfied it and the failure surfaced much later as a sweep that could not find the chat home. A registry excuse names one control. Keys matched as substrings, so an entry for 'settings::button::Save' also excused 'settings::button::Save and close'. Exact key, or the identity the key is an instance of. The destructive fixture control now records its own activation, and the test asserts the flag is still false. The claim under test is that the harness never fired it, and only the page can answer that; a verdict string is the harness marking its own homework. The floors updater rejects unknown flags. A typo in --allow-lower was read as its absence, which for the one flag that permits lowering a floor means doing the opposite of what was asked, silently. The workflow drops to contents:read plus issues:read and checks out without persisting credentials. Nothing in it writes to the repository, and a job that walks a live OAuth flow should not hold a token that could.
Live measurement against the deployed surface, and proof the gate can go redRun from a worktree at head Measured,
|
| surface | earlier commit on this branch | now | direction |
|---|---|---|---|
settings:Interface |
48 | 56 | raised |
settings:General |
16 | 17 | raised |
settings:Audio |
7 | 8 | raised |
Nothing was ratcheted downward to make a run pass. The 48, 16 and 7 were the defect: COV_FLOORS=update rewrote the floors from the run in progress and skipped checking them in the same pass, so a degraded run ratified itself. That path is deleted, the sweep only reads floors now, scripts/update-chat-coverage-floors.mjs refuses a partial ledger, refuses a ledger with surface errors and refuses to lower any floor without --allow-lower, and a unit test holds the committed file at or above the highest count any recorded run enumerated.
One floor was deliberately reduced and it is not a ratchet: search went from 106 to 1, a presence bar. Its control count is one row per chat, so it enumerated 50 on one run and 106 on the next with no product change; pinning that number reds the gate whenever the account is groomed, which is what teaches people to lower floors. The surface must still exist and render something, and dataDriven in the file records the reason.
…d raise the floor Three mutations, each run against the demo box or the real run report, each red. Ledger divergence: flipping C1's expected descriptor from input[email]#email to input[text]#email, which is exactly what shipping type=text on the sign-in field would produce, takes C24 red on the descriptor diff. The denominator guard is live rather than decorative. Collapsed run: a run report with the authenticated group skipped, which is what one renamed secret used to produce, scores 8 of 24 and the builder exits 1 on the floor. Before this change that scenario exited 0. Undeclared identity: a [C99] tag no ledger entry declares exits 1, so the ratio cannot be raised by deleting the entry for a control that regressed. Proof attribution. Both waiters are still armed before the action, because a waiter armed afterwards races the response, but the awaited response is now asserted to be the one belonging to the request the keypress issued, by object identity. PR #809 shipped the shape-match form and it was a real bug there: any qualifying response inside the window counted as proof. Floor raised from 18 to 19, the count a real run proves today. Nothing in this repository writes that number; it moves only in a commit like this one. Containment is now stated in the spec header: three writes, all named, on a surface that has no destructive control, with C23 and C24 failing on an unclaimed descriptor before anything could click one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ecks
tools/verify-spec-wiring.mjs: survivesOrdinaryPullRequest read a condition
like github.event_name != 'pull_request' as pull-request gated, because it
only checked for the presence of the event name plus the quoted string
pull_request, not the direction of the comparison. Added an explicit
exclusion check ahead of the presence check, and a fixture test
(tools/verify-spec-wiring.test.mjs) that reproduces the case, watched RED
against the unfixed classifier, and is GREEN now.
tools/lint-go-db-test-wiring.mjs: every go test invocation found in a step
was credited with every matrix leg's directory, with no regard for a shell
if/else inside that step's run text narrowing one invocation to one leg.
Two branches naming the same package pattern (the real control-plane/
edge-api RLS step shape) could therefore credit a leg whose branch never
runs that command. Added legsForLine, which narrows the legs attributed to
a go test line to the ones its enclosing `if [ "${{ matrix.KEY }}" = "VALUE" ]`
/ else block actually selects, falling back to every leg when it cannot
confidently parse the shape. A subprocess fixture test
(tools/lint-go-db-test-wiring.test.mjs) builds a two-module, one-leg-only
tree, watched it wrongly report both paired (RED), and is GREEN now. Both
new tests are wired into the required CI job next to the guards they cover.
Also: docs/TESTING-STANDARD.md's spec-wiring counts and reproducibility
claim brought current after merging #809's chat-coverage suite (14/19/3
of 36, was stale at 13/18/3 of 34), a new shape 16 naming the
effect-not-caused-by-the-control proof-attribution bug #809 shipped and
fixed in the same prover, dark-spec-allowlist.json's phase-19 wording and
its new chat-coverage entry, ci.yml's stale guard-script reference, and two
markdown/prose nits. All from CodeRabbit's second review pass on this PR.
… request Correction to the message this commit originally carried. It claimed the collapsed-run scenario exited 0 before this change and 1 after, crediting this commit with the floor. That is wrong, and an adversarial review caught it: the builder script is byte-identical to the previous commit, and the floor, not-run and hard-fail logic all landed there. Measured, each version run against one synthetic collapsed-run report with its own ledger: 86ff832 (this PR as first filed) exit 0 7/22 no floor 6d4a062 (first rework commit) exit 0 7/23 no floor 061bbfa (previous commit) exit 1 8/24 floor 18 297ad60 (this commit) exit 1 8/24 floor 19 So the collapsed run does go from exit 0 to exit 1 across this pull request, which is the comparison a reviewer deciding whether to merge it cares about, but that happened one commit earlier than claimed. What this commit actually does to the gate is raise the floor from 18 to 19, the count a real run proves. The rest of this commit stands: the create response is now asserted to be the one belonging to the request the keypress issued, by object identity, because PR #809 shipped the shape-match form and it was a real proof-attribution bug there. Containment is stated in the spec header. Nothing in this repository writes the floor; it moves only in a commit like this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tools/verify-spec-wiring.mjs: survivesOrdinaryPullRequest was patched twice
already for the "excludes pull_request but reads as included" defect class
(!= and github.event.action) and still missed two siblings in the same
class, found by ecc:code-review with direct execution against this file:
- A boolean `if: false` (YAML's unquoted false parses to a JS boolean, not
a string) was silently turned into "" by the caller before this function
ever saw it, so a disabled step was credited as pull-request coverage.
Extracted the caller's inline ternary into conditionOf, which now
round-trips a boolean through its string form, and taught
survivesOrdinaryPullRequest to reject the literal string "false".
- Negated contains()/startsWith()/endsWith() on github.event_name excluded
pull_request exactly as != does, and still passed the presence-only check
because they still mention the event name and the quoted string. Added
the same exclusion-operator treatment already applied to !=.
Both watched RED first (survivesOrdinaryPullRequest("false") and the three
negated-helper forms all returned true against the unfixed function),
fixed, watched GREEN. Six new cases in verify-spec-wiring.test.mjs, plus
conditionOf's own four.
tools/lint-go-db-test-wiring.mjs: legsForLine's own "known ceiling" comment
undersold what was actually broken. ecc:code-review found the backward scan
had no case for `elif` at all: it recognised `if` and `else` but walked
straight past an `elif [ matrix.KEY = VALUE ]` header, latching onto the
outer `if` instead and crediting that leg. Confirmed by extracting the
pre-fix function verbatim and running it against a 3-leg if/elif/else
chain: it returned mod-a's leg for the elif line where mod-b's was correct.
Rewrote legsForLine to recognise `elif` as its own branch header (a
positive match, same as `if`) and to accumulate every sibling condition in
a chain so a trailing `else` excludes all of them, not just the nearest
`if`. New fixture in lint-go-db-test-wiring.test.mjs gives each of three
legs a different package so the misattribution changes the pass/fail
outcome instead of hiding behind an identical pattern: confirmed RED by
temporarily restoring the pre-fix legsForLine and rerunning (mod-b's own
package reported dark, "no go test step ... names it", when a real line
does, misattributed), confirmed GREEN after restoring the fix.
docs/TESTING-STANDARD.md: the shape-16 addition from the previous commit
only updated the document's intro (line 5) and missed the section heading
and its adjoining sentence two lines below, which still said fifteen.
Fixed both, plus the two remaining un-hyphenated spec counts CodeRabbit's
LanguageTool pass caught (thirty-six, twenty-two). Also added the
floor/ratchet mechanism #809 shipped
(apps/web-console/e2e/chat-coverage/{data.ts,lib.ts},
scripts/update-chat-coverage-floors.mjs) as a new subsection under "How
coverage is counted": floors move only through a separate deliberate
commit against a recorded ledger, never the run doing the checking, and an
unsweepable surface carries a presence floor so deleting its entry point
fails the gate rather than shrinking the denominator. This document cites
#809 elsewhere and had no written trace of the mechanism most worth
generalizing from it.
Also corrected the PR's own description, which still stated the pre-#809
13/34 split; the gh api PATCH first attempt posted the literal string
"@<path>" as the body because -f does not expand @file the way --body-file
does, caught by reading the result back rather than trusting the call
succeeded, and redone with command substitution.
…wo highest ranked gaps (#803, #813) (#822) Opens #821, the tracking issue for making `Web E2E (full stack)` a required check. Fixes #803. Partially addresses #813 and #797. The owner's mandate is that every control surface is tested live, and that the full stack job becomes required once coverage is real and the suite is genuinely green. This lands the standard that defines "real", the honest measurement against it, and the two highest ranked gaps that measurement turned up. An adversarial review of the previous revision returned sixteen findings, and the core one was that a pull request whose thesis is honest measurement shipped a gameable metric and several false numbers. That is corrected here rather than argued with, and the third correction below is the useful half of this description. ## 1. The standard `docs/TESTING-STANDARD.md`. It encodes: - The one non negotiable rule: every test is watched failing against the broken state before it is trusted, and the pull request says what was broken to prove it. A test that has only ever been observed passing is an unvalidated claim. - What counts as proof of an effect per control type, including the reload-and-read-back requirement for anything persistent. Before PR #808 no spec in this repository called `page.reload()`, so no setting anywhere had a test that it survived one. - Coverage defined as proven controls over controls enumerated from the rendered DOM, with an explicit instruction not to invent a denominator. - The fifteen camouflage shapes that have each hidden a real defect here, each with a concrete instance and the assertion to write instead. - The live-session helper from #810 as the only sanctioned way to obtain a session against a deployed environment, and the rule that rotating a shared account password to get one is forbidden. The `demo_login.py` shape is named specifically so it is not reconstructed. - The rule that every figure in the document is one the tooling produces today, with superseded figures left visible beside the current ones. ## 2. #803, an invited owner could authorize a billing write `GetMembershipRole` resolved a role without constraining `account_memberships.status`. That predicate backs `IsWorkspaceOwner`, which gates `PermBillingWrite` on `PUT /api/v1/budgets/{ws}` and `POST /api/v1/spend-alerts/{ws}`, both mounted by #768. A row with `role='owner'` and `status='invited'` passed it. **Correction to the previous description.** It claimed that "every sibling query in the codebase constrains status". That is false, and the comment asserting it in `role_pgx.go` is deleted. `IsPlatformAdmin`, ten lines below the function this fixes, does not constrain status, and neither does `ListMembershipsByUserID` in `internal/accounts/repository.go`. The escalation in `IsPlatformAdmin` is a real separate defect and is owned by another change on its own branch; this one does not touch that function. The pre-existing unit tests stub `RoleStore`, so they prove the service maps the owner role to true and say nothing about the SQL underneath. The new tests drive the real pgx store against a migrated Postgres, and carry a positive control so a store that returned nothing for every input cannot pass as safe. **The fixture was also incoherent and is fixed.** `seedAccountMembership` set `accounts.owner_user_id` to the subject user and then inserted that same user as `status='invited'`, so the two rows contradicted each other: the account said the user owned it outright while the membership said the invitation was outstanding. A predicate reading `owner_user_id` instead of the membership row would have passed every case. The account now has a separate creator, so every case is one shape, a user added to somebody else's workspace, and the only variable is the role and status under test. ## 3. The `platform` package had never run in CI, and nothing stopped that recurring `role_rls_test.go` is gated on `HIVE_TEST_DB_URL`. The plain `go test` step does not have it, because the bootstrap step exports it afterwards, and `./internal/platform/...` was absent from the RLS step's explicit package list. So it skipped in one step and was never invoked in the other. Camouflage shape 1, same family as #659 and #708. Naming the package in the RLS step is what makes both the new tests and the existing `tenant_users` RLS tests execute at all. On its own that is a one line fix with nothing defending it: deleting `./internal/platform/...` again would silently skip all five new subtests while the suite stayed green. `tools/lint-go-db-test-wiring.mjs` closes that. It pairs every Go test file reading a `*_TEST_DB_URL` variable with a workflow step that both names its package and has that variable in scope at that point in the job, which is why the plain short step does not count. It runs in `Repo policy lints`, a required check that does not depend on the Go job it protects. Measured: twenty four control-plane packages and four edge-api packages carry such a gate. Eighteen package and variable pairs are properly wired. Ten control-plane packages are still dark and are carried by name as declared debt, which is #797's backlog. ## 4. #813, spec files that no workflow runs, measured honestly `tools/verify-spec-wiring.mjs` fails when a spec file is not selected by a pull request run and is not declared as debt. It runs in the required `Repo policy lints` job. **14 of 36 spec files run on a pull request** (as of `5206d914`; was 13/34 before `#809`'s chat-coverage merge added two specs, one of them, its self-check project, itself pull-request gated). 19 more run only on triggers a pull request cannot fire, meaning `owui-nightly.yml`'s and `chat-coverage.yml`'s schedule, manual dispatch and labelled runs. 3 run nowhere at all. The 22 that are not pull-request gated are declared in `apps/web-console/tests/dark-spec-allowlist.json`, grouped by shared cause, each group naming a reason, an owner and a tracking issue. ### Correction one: filename matching was wrong in both directions Filed openly because the mistake is inviting and it looked like it worked. A peer disproved the original numbers with `playwright test --list`. - **False positives.** `openai-sdk.spec.ts` and `performance/ttfb.spec.ts` were counted as wired because a workflow **comment** names them. A comment runs nothing. - **False negatives.** The nine `owui/NN-*.spec.ts` and two `owui/performance/*.spec.ts` are run by `owui-nightly.yml` via `npm run e2e:owui` and `e2e:owui:perf`, which select by `--project`. No filename ever appears in the workflow. It reported **6 wired / 27 dark**. Measured correctly at that commit the answer was **15 / 18**. Both are historical figures from a tree before #808 and before the phase-19 project was wired into the nightly. The root cause is structural: workflows select by project and config, so filename detection measures a quantity the runner does not use. That is camouflage shape 5, a weaker duplicate of the real predicate, occurring inside the tooling built to catch camouflage. ### Correction two: the invocation table was hardcoded, and went stale in a day The fix above replaced filename matching with `playwright test --list`, which was the right method, but kept a hardcoded table of the three invocations, each keyed on the exact `run:` line that triggered it. GitHub Actions checks out the merge commit, so this branch's CI saw `main` after #808 landed, which rewrote the line the table was keyed on. The guard failed with `invocation is no longer in its workflow` and called five genuinely wired specs dark. Correct alarm, wrong design. ### Correction three: it measured a real thing and still shipped a bad number This is the finding that mattered most, and it is three separate defects in the same file. **It was gameable.** The guard counted a nightly, a manual dispatch and a labelled run as identical to a pull request run. Eleven of the eighteen it called wired came from `owui-nightly.yml`, and **zero of the eighteen gated a pull request**. Anyone could improve the printed number without running a single additional test, by adding a `workflow_dispatch` workflow and deleting a ledger entry. A metric with a cheap fake move is not a metric. The fix is a three way split. Each spec measures as `pr`, `other` or `dark`, the ledger declares which of the last two applies, and a disagreement fails in **both** directions. Adding a dispatch-only workflow now moves a spec from `dark` to `other`, which fails until the ledger is edited, and it never moves the headline pull request figure at all. **Numerator and denominator came from different sets.** The denominator walked two hardcoded directories for `.spec.ts` only, while the numerator took anything the listing returned, including `../` relative paths, so the ratio could exceed one and a spec in a third directory was invisible forever. Hardcoding a denominator also violates the document's own rule at `TESTING-STANDARD.md`. Both sides now come from `playwright-spec-manifest.json`, which `verify-spec-collection.mjs` already pins to what is on disk, so the two sides cannot disagree and the denominator cannot go stale. **Arguments were discarded.** The previous version captured only an npm script's name and re-ran it bare, so a workflow running `npm run test:e2e -- --grep @smoke` was measured as if it ran everything. The earlier claim in this description that "the arguments cannot drift from CI's" was false. An npm script is now expanded into the argv it really runs, any appended arguments are kept, and a flag that narrows a run is **refused** rather than credited with everything its projects contain. ### Correction four, security: the guard executed workflow text The previous version ran every invocation it parsed out of a `run:` line, with `cwd` fixed at `apps/web-console` and no working directory awareness, and handed a `--config=<path>` parsed out of that same line to `npx playwright test --list`. **Playwright imports config files.** Editing a workflow line was therefore arbitrary code execution inside a required job. The guard now executes nothing. It resolves wiring from `playwright-spec-manifest.json`, which gains a `configs` section pinning each Playwright config to the projects it declares, verified from the same live listing by `verify-spec-collection.mjs` in the same required job. A workflow's `--config` and `--project` arguments resolve against that map, and a config the manifest does not pin is a hard failure rather than something to import and find out about. It also refuses an invocation whose working directory is not `apps/web-console`, and one that names a project its config does not declare, which Playwright would collect nothing for. Side effects of not executing anything: no placeholder credentials, no browsers, no `apps/web-console` dependencies, and the guard moves out of `Web console (type + unit + build)` into `Repo policy lints`, which is also required and runs on every non-docs change. ### What the measurement still cannot see Stated because the alternative is a number that flatters. Selection is not execution. A spec that skips every test on an unset variable counts as run here, and there are two live instances: five of the seven phase-19 specs skip themselves by name on `E2E_TENANT_B_ID`, `E2E_USER_A_SECOND_TENANT_ID`, `E2E_EXPIRED_JWT` and `E2E_ORPHAN_JWT`, which no environment provides, and `openai-sdk.spec.ts` skips on `EDGE_BASE_URL`, which `web-e2e` deliberately does not set. Both are recorded in the ledger and in the standard. Treat the wiring figure as an upper bound. The ledger also does not check that its tracking issues are still open. It cannot without a network call inside a required check, and a date based expiry would turn a required check red on a calendar rather than on a change. #813 was closed while eight entries still cited it, which is exactly this failure mode; every group now points at an open issue and the file says plainly that this is not enforced. ## Red proofs Every claim below was executed: run, observe the failure, restore, observe the pass. ### Go, #803 Against a `pgvector/pgvector:pg17` container carrying `.github/ci/test-db-bootstrap.sql` plus the full `supabase/migrations` chain, the same schema the CI RLS step builds. | Break | Result | | --- | --- | | Ran the suite against `role_pgx.go` with `AND status = 'active'` removed | **RED**, `invited_owner_is_not_owner`, `IsWorkspaceOwner` returned true for an invited owner | | Replaced the membership lookup with one reading `accounts.owner_user_id` | **RED**, the positive control fails. Under the old self contradicting fixture this predicate passed every case, which is what the fixture change buys | | Restored | **GREEN**, 5 subtests, alongside the 3 pre-existing `tenant_users` RLS tests | | Deleted `./internal/platform/...` from the RLS step's package list | **RED** in `lint-go-db-test-wiring.mjs`, naming both `internal/platform` and `internal/platform/db` | `gofmt`, `go vet` and `go test -short ./internal/platform/...` are clean. ### The spec wiring guard | Break | Result | | --- | --- | | Pointed `web-e2e` at the `probe` project instead of `chromium` | **RED**, thirteen specs undeclared, and the two probes reported as wired-now | | Declared a `dark` spec as `other` | **RED**, declared `other`, measured `dark` | | Added a `workflow_dispatch`-only workflow running the `probe` project | **RED**, declared `dark`, measured `other`. This is the gaming vector, and the headline pull request figure does not move | | Added a workflow **comment** naming a dark spec | **GREEN** and still dark. The false positive direction | | Added `--grep @smoke` to `npx playwright test --project=chromium` | **RED**, refuses to model a narrowed run | | Added `-- --grep @smoke` to `npm run e2e:owui` | **RED**, so npm script arguments are no longer discarded | | Pointed an invocation at a `--config` the manifest does not pin | **RED**, and nothing was imported to find out | | Named a project its config does not declare | **RED**, naming the projects that config does have | | Dropped `probe` from the manifest's `configs` | **RED** in the collection guard, from the live listing | ## Check state All six required checks pass at `64c34167`, run 31518377115. `Repo policy lints (tenant + audit)`, which now carries both new guards, prints the honest figures in its log: ``` spec wiring guard: OK, 13/34 spec files run on a pull request (1 invocation(s)); 18 run only on triggers a pull request cannot fire (3 invocation(s)); 3 run nowhere. go db test wiring OK: 18 package/variable pair(s) across 2 module(s), each named by a `go test` step that has the variable in scope. 10 more are carried as known debt. ``` `Go tests (control-plane)` shows the five #803 subtests executing rather than skipping, which is the point of section 3. `Web E2E (full stack)` **passes on this branch** at `64c34167` in that same run, and did so again at `d1915bda`, run 31515560185. Every job in run 31518377115 is green. **Correction to the previous description**, which claimed it passed on `00e75938` and left it at that. It failed on this pull request's own run at `2af0ba7f`, run 31358713495, on `container hive-control-plane-1 is unhealthy`. That is the shared session mode pooler contention tracked on #631 and not a change in this branch, and it is green on the run after the rebase. A description asserting a green check that was red on the same pull request is the shape this document exists to prevent. ## Coordination The detection contract this guard enforces is: **a spec counts as pull request gated only if a workflow that an ordinary pull request can trigger selects one of the projects the manifest pins it to**. Wiring a spec by adding it to a project removes it from the ledger automatically. Adding a new workflow invocation needs no change here, because invocations are discovered. ## What this pull request does not do - It does not change branch protection. The recommendation and its preconditions are in #821. - It does not wire the 21 specs that are not pull request gated. That is #813 and #708. - It does not wire the 10 dark control-plane Go packages. That is #797, and a suite that has never run is not known to pass, so turning ten of them on at once belongs in its own change. - It does not touch `IsPlatformAdmin`. Another branch owns that fix. Refs #797, #659, #708, #810, #819, #820, #843. ## Buglog entry To be appended to `.wolf/buglog.jsonl` in a buglog-only pull request after this merges. ```json {"date":"2026-08-11","title":"A coverage guard that executed workflow text and counted nightly-only runs as gating","error_message":"spec wiring guard reported 18/34 wired while zero of the eighteen gated a pull request","root_cause":"verify-spec-wiring.mjs parsed Playwright invocations out of workflow run: lines and executed them, including a --config path Playwright then imported, which is arbitrary code execution inside a required check. It also counted every workflow equally regardless of trigger, took its numerator and denominator from different sets, and dropped npm script arguments, so the printed number could be improved without any additional test running.","fix":"Moved the guard to tools/verify-spec-wiring.mjs, made it execute nothing by resolving wiring through playwright-spec-manifest.json (which gains a configs section verified by the collection guard against a live --list), split the report into pull-request gated, other-trigger and dark with a ledger that declares which, took both sides of the ratio from the manifest, expanded npm scripts into their real argv, and made an unmodelled narrowing flag a hard failure.","tags":["ci","testing","coverage-metric","required-check","code-execution","issue-813","issue-822"]} {"date":"2026-08-16","title":"Required check died on ERR_MODULE_NOT_FOUND: no job installed root dev deps","error_message":"Cannot find package yaml imported from tools/verify-spec-wiring.mjs","root_cause":"web-unit sets working-directory to apps/web-console and its only npm ci runs there. Two guard steps override working-directory to the repo root and import the root devDependency yaml, but nothing installed root node_modules in that job. The declaration existed, the install did not. Sibling root-scoped steps hid the gap because they import nothing.","fix":"Re-included the root package-lock.json in .gitignore per the rule its own comment states, committed the 15-package lockfile, added a root-scoped install step to web-unit ahead of both guard steps, and replaced the npm ci fallback chain in repo-policy-lints with plain npm ci, since falling back to npm install turned lockfile drift into a silent floating install.","tags":["ci","required-check","npm","lockfile","fail-open","issue-813","issue-822"]} ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added database-backed coverage for workspace ownership and membership access rules. * Added Playwright spec and configuration tracking for more reliable test coverage visibility. * Added a testing standard covering UI behavior, persistence, authentication, workflows, and measurement quality. * **Bug Fixes** * Added automated checks to detect missing or incorrectly wired browser and Go database tests in CI. * **Documentation** * Documented Playwright coverage gaps, ownership, tracking issues, and validation requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Enumerates the chat app's controls from the rendered DOM on every run and requires each one to produce an observable effect against a running deployment. Nothing here carries a hand written list of controls, so a control added upstream or by a Hive patch is enumerated the first time it renders and has to earn a proof or fail the gate.
Reworked after an adversarial review returned BLOCK on thirteen findings. What changed in that rework is in the last section; the rest of this description already reflects it.
No coverage figure is claimed
The 144 of 165 headline is withdrawn, and the script that derived it is
deleted. It came from a run that predates this rework, whose proven count
includes destructive controls clicked with the write aborted and controls
proven by a request no server ever answered, neither of which is proof any
more. It was derived by a script with none of the guards the floor updater
has, from a ledger carrying five surface errors that also violates four of the
floors this pull request restores. A number no shipped code path can produce
is not a measurement.
The figure returns when a sweep runs under the rules in this pull request,
writes its own summary, and passes its own floor and error checks. That run
was deliberately not taken here: the demo box is single tenant and another
agent is on it.
The ledger in
docs/proof/chat-interaction-coverage-2026-08-10/is kept forwhat it records, with the summary the run itself wrote.
What has been measured, live, by the shipped code path
Run from a worktree against
https://chat-hive.scubed.co, signed in throughthe shared live-auth helper, 2026-08-12T20:06:47Z. A slice, so it reports no
percentage, which is the partial-run rule working:
Five controls enumerated against a floor of 5, so the denominator guard was
satisfied exactly. Zero surface errors. Two controls proven by navigation,
three found dead, and the gate went red on them.
Two of those three are the rework changing verdicts rather than wording:
mutations=26andmutations=28would both have been proven under the oldmutations > 8fallback. Under the rules in this pull request they areunproven, because a mutation count nobody ever measured against an idle page is
not evidence of anything.
A full sweep has been attempted twice and does not complete against the box
today. It fails after the setup project signs in successfully, with the
coverage project finding itself on
/auth:That is the #782 class, a chat session dying under a token refresh, and it is
not caused by this branch. Stated rather than papered over: until it completes,
this gate is worth a real slice measurement plus a detector proven against nine
fixture controls, and it is not worth a whole-app number. No whole-app number
is claimed anywhere in the branch.
Destructive controls are checked, never fired, and never counted as proof
The first version clicked them with the write aborted at the network layer and then forced
proven: trueon the strength of the aborted request. That is worse than no coverage: an aborted request never reaches the server, so a button wired to an endpoint that is gone came back green, which is exactly the #846 defect class this gate exists to catch, and it was invisible.Now a control whose label says it destroys data is not clicked at all. It is asserted present, visible, enabled (or disabled with a stated reason) and carrying an accessible name, and recorded as
not-fired, a category that is neither proof nor failure and is reported on its own line. The network level abort survives only as a data safety net for a control whose label says nothing about what it does, and any control that trips it is recordednot-firedfor the same reason: there is no server verdict to read.The recorded run shows
0 not firedbecause the category did not exist when it was taken. The next live run will show a non-zero count and a correspondingly lower proven count. That is a more honest number, not a regression.Floors no longer ratchet downward
COV_FLOORS=updaterewrotesurface-floors.jsonfrom the run in progress and skipped checking the floors in the same pass. A bar the measured run can move is not a bar, and this branch shipped the proof: three surfaces that had genuinely degraded had the smaller numbers written in as the new baseline, with no reason recorded anywhere.The sweep now only ever reads the floors. Changing one is
scripts/update-chat-coverage-floors.mjs, run against a recorded ledger, in its own commit: it refuses a partial ledger, refuses a ledger with surface errors, and refuses to lower any floor without--allow-lower. A unit test holds the committed file at or above what a recorded live run enumerated, so this specific regression cannot come back quietly.An unswept floor key is now a hard failure too. The guard used to iterate only what was swept, so a surface that stopped opening took its own floor with it and left the denominator smaller with nothing said. Settings and workspace discovery throw instead of returning an empty list for the same reason, and the throw is recorded and the sweep continues, so the run still produces a ledger.
Proof is tied harder to the click
mutations > 8fallback is deleted. Nothing ever measured what an idle page emits in the same window, so it was a number that could pass a control on background churn. The rendered-text signature is the measured test; the count survives only in the failure detail.Still not done, and deliberately: proof is not tied to the specific request a given control should issue. That needs a per-control expectation, which is a hand written inventory, which is the thing this gate exists to avoid. The idle sample is the general form of the same guard.
Partial runs no longer report a total
COV_SURFACESslices a run so it can finish inside the session lifetime (#782). A sliced run now says so, prints identity counts for the swept surfaces only, and prints no percentage at all. Its floor check is scoped to the same slice, so a slice cannot fail on the floors it was never asked to sweep.Credential hygiene
codeandstate.tools/lint-no-token-in-proof-captures.mjsskips binaries by design and cannot see any of it.COV_TRACE=1turns them on for local debugging, with the artefact never to be attached anywhere.redactUrl, which scrubs the credential parameters in the fragment as well as the query string. That covers the "could not get back to the chat home" error, which used to interpolate a rawpage.url().live-auth.tskeeps the only implementation of the chat hop, which is where its only caller imports it from; the duplicate added tolive-auth.mjshad no caller and is deleted. The mint itself stays in the.mjschild process so the service-role key never enters a Playwright worker.Wired into CI
The gate previously ran nowhere: no workflow and no npm script referenced its config, and its specs were invisible to
scripts/verify-spec-collection.mjs, so a rebase onto main would have turned the required Web console job red on two undeclared spec files..github/workflows/chat-coverage.yml, job self-check: the prover break-proof, the inert-registry and exclusion validity check, and the exclusion-expiry check. Needs no deployment and no credentials, runs on every pull request. Green on this one: 3 passed.workflow_dispatchor arun-chat-coveragelabel, uploading the ledger as an artifact. Same gating shape as owui-nightly, for the same reason.playwright-spec-manifest.json, and the chat-coverage config is listed in the collection guard with placeholder env so its collection is deterministic everywhere.Exclusion expiry can actually fire now
It used to
test.skipon a missing token andcontinuepast any non-ok GitHub response, so both paths reported green and an exclusion could outlive its issue forever. Both now fail, and the CI job passesGITHUB_TOKEN.The removed-surfaces test no longer passes against a broken app
A settings modal that failed to open produced an empty list and counted as "nothing forbidden is present". It now asserts that each surface enumerated something before concluding anything is absent, and a route that redirects to
/authis a dead session rather than a removed route.Two controls the old predicate called proven are broken
Both were being reported as coverage until this run.
File not found.and nothing else. Was counted as dom proof.Both are defects in the surface, not gate artefacts.
Five surface errors, and the gate is red on all of them
#834 merged and deployed and was expected to unblock the first four. It did not.
These stay as surface errors that fail the gate rather than becoming exclusions. An exclusion is a decision that a surface will not be measured, and that is not the decision here: the gate should stay red until they sweep.
Verification
npx tsc --noEmit: clean.npx vitest run: 509 passed, 44 files, including 18 new tests over the gate's pure logic.npm run e2e:verify-collection: OK, 39 Playwright test files across 3 configs.npm run e2e:chat-coverage:self-check: 3 passed, including the new polling case. Without a token the expiry check fails as designed.node tools/lint-no-token-in-proof-captures.mjs: ok, 57 files scanned.The live sweep was deliberately not re-run: the demo box is single tenant and another agent is walking it. The headline figure is therefore derived rather than re-measured, and says so in both the README and above.
Second review pass, and what it changed
A second independent review returned BLOCK. Everything below is in the branch.
Proof now needs a server verdict.
requests.length > 0proved a controlpointing at an endpoint that hangs, for ever, which is the same missing verdict
that made an aborted write look like proof. Proof is a 2xx or 3xx response
inside the settle window; a request nothing answered is reported as exactly
that. My earlier rejection of this finding was wrong and is withdrawn: the rule
is generic and needs no per-control inventory.
not-fired no longer launders a dead control. Two problems, both fixed. It
matched
label, which falls back to up to 140 characters of ancestor text, soone "Delete All Chats" heading made every button in that panel destructive:
two controls this repository's own ledger proves dead would have gone green and
three genuinely proven ones would have been thrown away. It now matches the
control's own accessible name. And it was filtered out of the failure list
entirely; it now fails the gate like any other unfireable control unless
inert-registry.jsonjustifies the key, while still being reported in its owncolumn so it can never be read as coverage.
A disabled control is no longer proof.
disabled-with-reasoncounted ascoverage, so #846 could have been "fixed" by disabling Admin Settings with a
title attribute and the ratio would have risen. Disabled is now not-fired, and
aria-describedbyis no longer accepted as a reason because it holds anelement id.
Destructive stateful controls are checked rather than flipped.
flip()fills every text input and clicks Save, and only the click pass consulted the
classifier.
The artifact cannot carry the OAuth callback. The upload took a whole
directory, and Playwright's JSON report was written into it; that report
carries raw error text, and a sign-in that outran its ninety second wait quotes
the callback URL with a live
codeandstate. Three changes: the frameworkreports are written outside the uploaded directory, the upload names two files
rather than a directory, and
signInToChatredacts the framework's own failuretext where the credential is still inside a string this repository owns.
Redaction now lives in one place,
tests/e2e/support/redact.ts, matchesparameter names by pattern rather than an exact list that missed
id_tokenandprovider_token, and covers the hash-router shape.The floor pin test actually pins. It compared against the morning ledger
only, where
searchwas 50 against a committed floor of 106, so a fifth of thedenominator could have been signed away with the test still green. It now takes
the highest across every recorded run, and a missing floor key is a failure
rather than a skip.
Deleting an entry point no longer turns red into green. sidebar,
chat-item-menu, chat-message-actions and workspace:knowledge carry a floor of
fails on the unswept floor key instead of quietly shrinking the denominator.
The chat list carries no floor at all. It is one row per chat, so it
enumerated 50 controls one run and 106 the next with no product change. A floor
there reds the gate whenever someone grooms the account, and the fix everyone
reaches for is to lower it. It is listed under
dataDriven, still swept, stillrequired to prove every control.
Delta enumeration subtracts as a multiset. This is the finding I rejected
on the first pass and was wrong about: the point was ordinal SHIFT, not plain
stripping. An overlay that injects or hides one identically-keyed element moved
every later ordinal, so a base control could survive subtraction and be clicked
in the wrong context.
The service-role key claim is corrected. It is ordinary job-level
environment, the config reads it, and the spawn forwards all of
process.env.Only its use is isolated, and both comments that claimed otherwise now say so.
New cover for every branch above: a control whose request nothing answers,
one whose request dies in transport, a destructive one and a disabled one in
the browser fixture; exclusion bookkeeping asserted against deliberately broken
input rather than only against the valid committed file; the floor guard's
data-driven exemption and its missing-key failure.
Stage 6 review streams
Full verdicts are in a comment on this pull request. In short: CodeRabbit CLI SKIPPED (403, organization membership), the Codex adversarial stream SKIPPED (usage limit, produced no findings), the plain adversarial pass RAN and found two blocking issues that are fixed here (a response proving a control that did not cause it, and three strings reaching the ledger and CI log unredacted), and the three agent streams could not be dispatched from the process running this stage. Two blocked streams are reported as blocked rather than as passes.
Known gap, not fixed here
Neither new job is in
.github/branch-protection-main.json, so neither canblock a merge. That file is owner territory rather than something a builder
should quietly extend, and there is a trap worth recording before anyone does:
a job-level
if:skip SATISFIES a required context in GitHub, so makingChat coverage live sweeprequired would create a check that is permanentlyand silently green, since it skips on every ordinary pull request. If either
becomes required it should be
Chat coverage self-check, which has nojob-level
if:and runs everywhere.Buglog entry
To be appended to
.wolf/buglog.jsonlon main in a separate buglog-only pull request once this merges:{"id":"bug-2026-08-11-coverage-gate-forced-proof","date":"2026-08-11","area":"testing","error_message":"chat interaction coverage reported controls as proven that were never shown to work: destructive controls were clicked with their write aborted and then forced proven:true, background REST polling inside a control's settle window counted as that control's network proof, and COV_FLOORS=update rewrote the surface floors from the same run that was supposed to check them","root_cause":"three separate ways for the gate to manufacture a green: an aborted request has no server verdict but was treated as one, only socket.io was excluded from the request evidence channel while Open WebUI also polls REST, and setting a threshold and checking it happened in one pass so a degraded run ratified itself as the new baseline (Interface 56 to 48, General 17 to 16, Audio 8 to 7)","fix":"destructive controls are asserted present, enabled and named and never fired, in a not-fired category that is neither proof nor failure; each surface is sampled while idle and its own background request signatures are excluded from proof; floors are read-only during a sweep and changed by scripts/update-chat-coverage-floors.mjs against a recorded ledger, which refuses to lower one without --allow-lower, with a unit test holding the committed floors at or above a recorded live run","tags":["e2e","playwright","coverage","false-green","chat"]}