feat(#502): automated copy & duplication audit — static lint, projection invariants, browser sweep - #538
Merged
Merged
Conversation
…ion invariants, browser sweep All three layers the issue asked for, verified from scratch since none existed on disk despite the issue body's pre-checked acceptance boxes. Layer 1 — static style/copy lint (tests/hub-copy-lint-502.test.js): - Pins the #492 fix: the global `[hidden] { display: none !important; }` guard must exist verbatim, and no OTHER `display: ...!important` rule may exist (the only thing that can shadow an !important rule for the same property is another one). - Scans ui/pages/*.js for duplicate user-facing prose within one module (a regex-based heuristic distinguishing sentence-like copy from HTML fragments, CSS class lists, and SVG path data — refined iteratively against the real codebase until it produced zero false positives). Undocumented duplicates fail; a documented allowlist (tests/helpers/copy-audit-allowlist.json) names why each real one is designed repetition. A third test guards the allowlist itself against staleness. Layer 2 — projection invariant tests (tests/hub-projection-invariants-502.test.js): - Sweeps the general PROPERTY across every alert type / every readiness outcome / several approval-dedup id-shapes, rather than pinning one historical scenario the way #491/#493/#494's own tests already do. This found two REAL bugs neither of those specific tests had exercised: 1. src/observability/alerts/engine.js: the "High failure rate" alert still truncated its run id via `.slice(-12)` — a third occurrence of the #491 bug class the original fix wave missed. Fixed. 2. src/observability/dashboard/state/approvals.js: approvalRequestsFromBlockedGates deduped by a project-scoped `key:` AND an UNSCOPED `id:` (approvalQueueId doesn't include project) — so two gates in DIFFERENT projects sharing the same run id/task id/ artifact string (plausible: these are canonical stage conventions shared across every project) incorrectly suppressed each other via the bare id collision alone, a residual instance of the exact CWE-863 cross-project-suppression class Strix found and partially fixed in PR #509. Fixed by scoping the id-based dedup key the same way the semantic key already was. Both fixes mutation-tested (reverting each reproduces the real failure). Layer 3 — browser copy-audit sweep (tests/browser/dashboard-copy-audit-502.test.js): - Extends the #96 browser-regression harness: extracts visible text blocks from the real painted DOM (real Chromium via playwright-core) across all six destinations, fails on an undocumented exact-duplicate block within one page, and reports (does not gate on) near-duplicates across pages. - Found that a naive extraction flagged data VALUES (file paths, rendered timestamps, a project id hash derived from the fixture's own — random per test run — temp directory) as "duplicates." Excluded these structurally in the test itself rather than allowlisting them, since an exact-string allowlist entry for something random-per-run would go stale on the very next test run. The remaining real findings (stage ids shown in both a list and detail view, per-row fallback text, shared CTA copy) are genuinely designed repetition, documented in the same allowlist file under a browser_within_page key. - Mutation-tested (an injected genuine duplicate is caught). Verified: 1929/1929 core tests, 34/34 browser tests (including 12 pre-existing suites from #96/#525/#526/#533 unaffected), typecheck/lint/ security-audit/validate all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
Closed
4 tasks
PR Summary by QodoAutomated copy/duplication audit: static lint, projection invariants, browser sweep
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+60
to
+71
| async function extractTextBlocks(page, pageId) { | ||
| return page.evaluate((id) => { | ||
| const root = document.getElementById(`page-${id}`); | ||
| if (!root) return []; | ||
| const blocks = []; | ||
| const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT); | ||
| let node; | ||
| while ((node = walker.nextNode())) { | ||
| const text = node.textContent.replace(/\s+/g, ' ').trim(); | ||
| if (text) blocks.push(text); | ||
| } | ||
| return blocks; |
There was a problem hiding this comment.
4. Hidden text counted visible 🐞 Bug ☼ Reliability
extractTextBlocks() collects every text node under #page-${id} without checking whether the node
is actually visible, so hidden tab panels (e.g., run-workspace) can contribute text and trigger
false duplicate failures or flakes. This contradicts the test’s stated intent (“painted DOM”) and
will become brittle as hidden panels gain more copy.
Agent Prompt
### Issue description
The browser copy-audit claims to extract *visible/painted* text, but it currently walks all text nodes under the page root and does not exclude text inside `[hidden]`/`display:none` subtrees. This can cause false positives (duplicates in non-visible panels) and CI flakiness as hidden UI grows.
### Issue Context
The dashboard keeps tab panels in the DOM and toggles `panel.hidden` for non-active sections; CSS enforces `[hidden] { display:none !important; }`, so those nodes are not painted but are still present in the DOM.
### Fix Focus Areas
- tests/browser/dashboard-copy-audit-502.test.js[60-72]
- src/observability/dashboard/ui/pages/run-workspace.js[140-149]
- src/observability/dashboard/ui/styles.js[43-48]
### Implementation notes
In `page.evaluate`, skip a text node if its parent element is not visible, e.g.:
- `if (el.closest('[hidden]')) continue;`
- `const style = getComputedStyle(el); if (style.display==='none' || style.visibility==='hidden') continue;`
- optionally require geometry/paint: `el.offsetParent !== null` (with care for `position: fixed`) or `el.getClientRects().length > 0`.
This should ensure the test matches its “visible text” intent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+50
to
+55
| const DYNAMIC_VALUE_PATTERNS = [ | ||
| /\//, // file paths, e.g. .rstack/runs/<id>/tasks.json | ||
| /^[A-Z][a-z]{2} \d{2}, \d{4},.*[AP]M/, // rendered date/time, e.g. "Jul 01, 2026, 09:30:00 AM GMT+5:30" | ||
| /project-[0-9a-f]{6,}/, // hashed project id derived from the (random) temp root | ||
| /rstack-browser-[a-z0-9-]+-[A-Za-z0-9]{6}\b/, // mkdtemp-suffixed temp dir name embedded in a longer string | ||
| ]; |
There was a problem hiding this comment.
5. Overbroad slash dynamic filter 🐞 Bug ⚙ Maintainability
The browser copy-audit skips any text containing '/', which excludes real user-facing copy like “Proof / risks” and reduces the audit’s ability to catch duplicate-copy regressions for those strings. This is a silent coverage gap: the suite will pass while missing duplicates it intends to detect.
Agent Prompt
### Issue description
The browser copy-audit treats any string containing `/` as a dynamic value and excludes it. Many legitimate UI labels include slashes, so this reduces coverage and can let duplicate-copy regressions slip by.
### Issue Context
The intent is to exclude *file paths* and other data-like values, not general punctuation in authored prose.
### Fix Focus Areas
- tests/browser/dashboard-copy-audit-502.test.js[50-58]
- src/observability/dashboard/ui/pages/run-workspace.js[78-85]
- src/observability/dashboard/ui/pages/traceability.js[14-16]
### Implementation notes
Replace `/\//` with a more specific path detector, for example:
- strings that start with `/` or `./` or contain `/.rstack/`
- strings that look like a filesystem path segment pattern: `/(^|\s)(\.?\/|\/)[^\s]+\/[^\s]+/`
- or match known extensions when combined with slashes (e.g. `\.json`, `\.md`, `\.yaml`).
Goal: keep excluding real paths while still analyzing authored copy that happens to include slashes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
All three layers the issue asked for. Verified from scratch that none existed on disk despite the issue body's pre-checked acceptance boxes (standard lesson — pre-checked issue checkboxes are unverified until proven).
Layer 1 — static style/copy lint (
tests/hub-copy-lint-502.test.js): pins the #492 fix (the global[hidden] { display: none !important; }guard must exist, and no otherdisplay:...!importantrule may shadow it) and scansui/pages/*.jsfor duplicate user-facing prose within one module, with a documented allowlist for designed repetition.Layer 2 — projection invariant tests (
tests/hub-projection-invariants-502.test.js): sweeps the general property across every alert type / readiness outcome / approval-dedup id-shape, rather than one historical scenario at a time (which #491/#493/#494 already pin individually). This found two real bugs:.slice(-12)— a third occurrence of the Alerts: stalled-run detail mangles run id via slice(-12) and prints raw minutes #491 bug class the original fix wave missed.approvalRequestsFromBlockedGatesdeduped by a project-scopedkey:but an UNSCOPEDid:— two gates in different projects sharing the same run/task/artifact string incorrectly suppressed each other, a residual instance of the CWE-863 cross-project-suppression class Strix found in PR Approvals: one decision = one pending item — semantic dedup + realpath-canonical roots (#494, #505) #509.Both fixed and mutation-tested (reverting each reproduces the real failure).
Layer 3 — browser copy-audit sweep (
tests/browser/dashboard-copy-audit-502.test.js): extends the #96 harness — extracts visible text from the real painted DOM across all six destinations, fails on an undocumented exact-duplicate within one page, reports (doesn't gate on) near-duplicates across pages. Found that a naive extraction flagged data values (paths, timestamps, a project-id hash derived from the fixture's own random temp dir) as "duplicates" — excluded these structurally rather than allowlisting them (an exact-string entry for something random-per-run would go stale immediately). Remaining real findings are genuinely designed repetition, documented in the allowlist.Verification
Test plan
npm test— 1929/1929npm run test:browser— 34/34npm run typecheck/npm run lint— 0 errorsnode scripts/security-audit.mjs/npm run validate— clean🤖 Generated with Claude Code