Approvals: one decision = one pending item — semantic dedup + realpath-canonical roots (#494, #505) - #509
Merged
Merged
Conversation
…#505) macOS's /tmp and /var are symlinks into /private, so the registry and the --project argument could name the same directory with two spellings. Lexical path.resolve kept both: the Hub registered one project twice — "2 projects" for one repo, doubled queue approvals, doubled feed rows and task counts (observed live during the #495 test work, and the reason #494's one-decision-one-card contract could not hold end to end). Root intake (state/roots.js sourceRoots + the injected-roots branch in buildFullState) now canonicalizes through fs.realpath before dedup, and project/run identity (state/identity.js resolveProjectDescriptor, runScopeKey, decorateRunIdentity) hashes the canonical path — identity is a function of the real filesystem node, never of the spelling that reached us. A missing directory degrades to its resolved lexical form instead of throwing, and only successful resolutions are cached so a root that appears later gets its real identity then. Test fixtures that pinned descriptor paths now derive expectations from the canonical (realpath) fixture base — same assertions, canonical spelling. Closes #505. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The event-derived gap-filler (approvalRequestsFromBlockedGates, #274) deduped against queue entries by raw id only, so a queue card whose id is not the recomputed canonical approvalQueueId — pre-#274 runs, hand-authored entries, the canonical fixtureBlockedRun card — produced a duplicate pending card, an inflated "Approvals 2 pending" topbar chip, and a Studio rail reading "holding 2 gate(s): guardrail-override:07-code, guardrail-override:07-code" for a single human decision. Dedup is now semantic: a decision is (runId, taskId, artifact), exported as approvalDecisionKey. The queue-backed entry wins (it is the actionable record); the derived request stays strictly a gap-filler — the no-card case still derives it, unrelated artifacts and other runs never suppress it, and canonical-id dedup keeps working (all pinned). The semantic Studio's governance deck had the sibling problem one level up: it lists blocked-gate records AND pending approvals, counting one decision twice. A gate whose every missing artifact is covered by a pending approval for the same task is now suppressed — the actionable approval represents the decision; uncovered gates still surface as observed truth. Browser-pinned end to end on fixtureBlockedRun (the exact repro): one pending approval in the snapshot, "1 pending" in the topbar, exactly one actionable card, and a Studio rail holding one gate. The end-to-end contract also needed #505 (the same queue card was being read through two spellings of one root). Closes #494. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strix Security Review1 open security finding on this PR:
Review summaryReviewed all security-relevant changes in the PR, including the updated dashboard state assembly, approval deduplication, root canonicalization, and the added regression coverage. One confirmed issue remains in the new semantic approval/governance dedup logic: approvals and blocked-gate governance items can be suppressed across different project roots when those roots share the same Fixed the findings? re-run the review, or tag Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 24 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 (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoDedup approvals by semantic decision key and canonicalize roots via realpath
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
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}
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Comment on lines
+42
to
+44
| export function approvalDecisionKey(entry) { | ||
| return [entry?.runId ?? '', entry?.taskId ?? '', entry?.artifact ?? ''].join('::'); | ||
| } |
There was a problem hiding this comment.
🟢 Cross-project approval suppression from run-scoped semantic dedup
Severity: LOW · CWE-863
The PR changes approval deduplication and Studio governance suppression to key decisions only by runId, taskId, and artifact. This ignores the repository/root dimension even though the dashboard explicitly supports identical runId values across different project roots. In a live local reproduction with two distinct roots sharing the same runId and the same (taskId, artifact), a queue-backed pending approval in one root suppresses the other root's blocked gate and derived pending approval in global state. When the second run is selected in Studio, its governance deck shows the first project's approval instead of the second project's blocked gate. This hides a real pending governance decision across projects and breaks approval integrity in multi-project views.
Suggested change
| export function approvalDecisionKey(entry) { | |
| return [entry?.runId ?? '', entry?.taskId ?? '', entry?.artifact ?? ''].join('::'); | |
| } | |
| export function approvalDecisionKey(entry) { | |
| return [ | |
| entry?.projectId ?? entry?.projectRoot ?? '', | |
| entry?.runId ?? '', | |
| entry?.taskId ?? '', | |
| entry?.artifact ?? '', | |
| ].join('::'); | |
| } |
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: Cross-project approval suppression from run-scoped semantic dedup
Severity: LOW
CWE: CWE-863
The PR changes approval deduplication and Studio governance suppression to key decisions only by `runId`, `taskId`, and `artifact`. This ignores the repository/root dimension even though the dashboard explicitly supports identical `runId` values across different project roots. In a live local reproduction with two distinct roots sharing the same `runId` and the same `(taskId, artifact)`, a queue-backed pending approval in one root suppresses the other root's blocked gate and derived pending approval in global state. When the second run is selected in Studio, its governance deck shows the first project's approval instead of the second project's blocked gate. This hides a real pending governance decision across projects and breaks approval integrity in multi-project views.
Location: src/observability/dashboard/state/approvals.js:42-44
Context: Scope semantic approval dedup to the project/root
```
// Before:
export function approvalDecisionKey(entry) {
return [entry?.runId ?? '', entry?.taskId ?? '', entry?.artifact ?? ''].join('::');
}
// After:
export function approvalDecisionKey(entry) {
return [
entry?.projectId ?? entry?.projectRoot ?? '',
entry?.runId ?? '',
entry?.taskId ?? '',
entry?.artifact ?? '',
].join('::');
}
```
Location: src/observability/dashboard/state/approvals.js:60-62
Context: Pass project scope into blocked-gate dedup
```
// Before:
const id = approvalQueueId({ runId: gate.runId, taskId: gate.taskId, artifact });
const key = approvalDecisionKey({ runId: gate.runId, taskId: gate.taskId, artifact });
if (existing.has(`id:${id}`) || existing.has(`key:${key}`)) continue;
// After:
const id = approvalQueueId({ runId: gate.runId, taskId: gate.taskId, artifact });
const key = approvalDecisionKey({
projectId: gate.projectId,
projectRoot: gate.projectRoot,
runId: gate.runId,
taskId: gate.taskId,
artifact,
});
if (existing.has(`id:${id}`) || existing.has(`key:${key}`)) continue;
```
Location: src/observability/dashboard/state/studio.js:354-367
Context: Prevent Studio governance suppression across projects
```
// Before:
const pendingApprovals = (state?.approvals ?? [])
.filter((item) => (!item?.runId || item.runId === runId) && String(item?.status ?? '').toLowerCase() === 'pending');
// #494: one decision = one governance item. A blocked-gate record whose
// every missing artifact is covered by a pending approval for the same
// task is the decision that approval card already represents — listing
// both read as "holding 2 gate(s)" for a single human call. Gates with no
// covering approval (or no missing list) still surface as observed truth.
const covered = new Set(pendingApprovals.map((item) => `${safeId(item.taskId) ?? ''}::${item.artifact ?? ''}`));
const gates = (state?.blockedGates ?? [])
.filter((item) => !item?.runId || item.runId === runId)
.filter((item) => {
const missing = Array.isArray(item?.missing) && item.missing.length ? item.missing : null;
if (!missing) return true;
return !missing.every((artifact) => covered.has(`${safeId(item.taskId) ?? ''}::${artifact}`));
// After:
const pendingApprovals = (state?.approvals ?? [])
.filter((item) => (!item?.runId || item.runId === runId) && String(item?.status ?? '').toLowerCase() === 'pending');
// #494: one decision = one governance item. A blocked-gate record whose
// every missing artifact is covered by a pending approval for the same
// task is the decision that approval card already represents — listing
// both read as "holding 2 gate(s)" for a single human call. Gates with no
// covering approval (or no missing list) still surface as observed truth.
const covered = new Set(pendingApprovals.map((item) => `${item.projectId ?? item.projectRoot ?? ''}::${safeId(item.taskId) ?? ''}::${item.artifact ?? ''}`));
const gates = (state?.blockedGates ?? [])
.filter((item) => !item?.runId || item.runId === runId)
.filter((item) => {
const missing = Array.isArray(item?.missing) && item.missing.length ? item.missing : null;
if (!missing) return true;
const scope = item.projectId ?? item.projectRoot ?? '';
return !missing.every((artifact) => covered.has(`${scope}::${safeId(item.taskId) ?? ''}::${artifact}`));
```
How to fix:
Include project identity in both semantic keys. The approval dedup key should incorporate a stable root/project discriminator such as `projectRoot` or `projectId` in addition to `runId`, `taskId`, and `artifact`. The Studio suppression set should likewise scope coverage by the same discriminator so one project's pending approval cannot cover another project's blocked gate. Preserve the existing legacy-id behavior, but only within the same project/root boundary.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
Comment on lines
60
to
+62
| const id = approvalQueueId({ runId: gate.runId, taskId: gate.taskId, artifact }); | ||
| if (existing.has(id)) continue; | ||
| existing.add(id); | ||
| const key = approvalDecisionKey({ runId: gate.runId, taskId: gate.taskId, artifact }); | ||
| if (existing.has(`id:${id}`) || existing.has(`key:${key}`)) continue; |
There was a problem hiding this comment.
Pass project scope into blocked-gate dedup
Suggested change
| const id = approvalQueueId({ runId: gate.runId, taskId: gate.taskId, artifact }); | |
| if (existing.has(id)) continue; | |
| existing.add(id); | |
| const key = approvalDecisionKey({ runId: gate.runId, taskId: gate.taskId, artifact }); | |
| if (existing.has(`id:${id}`) || existing.has(`key:${key}`)) continue; | |
| const id = approvalQueueId({ runId: gate.runId, taskId: gate.taskId, artifact }); | |
| const key = approvalDecisionKey({ | |
| projectId: gate.projectId, | |
| projectRoot: gate.projectRoot, | |
| runId: gate.runId, | |
| taskId: gate.taskId, | |
| artifact, | |
| }); | |
| if (existing.has(`id:${id}`) || existing.has(`key:${key}`)) continue; |
Comment on lines
+354
to
+367
| const pendingApprovals = (state?.approvals ?? []) | ||
| .filter((item) => (!item?.runId || item.runId === runId) && String(item?.status ?? '').toLowerCase() === 'pending'); | ||
| // #494: one decision = one governance item. A blocked-gate record whose | ||
| // every missing artifact is covered by a pending approval for the same | ||
| // task is the decision that approval card already represents — listing | ||
| // both read as "holding 2 gate(s)" for a single human call. Gates with no | ||
| // covering approval (or no missing list) still surface as observed truth. | ||
| const covered = new Set(pendingApprovals.map((item) => `${safeId(item.taskId) ?? ''}::${item.artifact ?? ''}`)); | ||
| const gates = (state?.blockedGates ?? []) | ||
| .filter((item) => !item?.runId || item.runId === runId) | ||
| .filter((item) => { | ||
| const missing = Array.isArray(item?.missing) && item.missing.length ? item.missing : null; | ||
| if (!missing) return true; | ||
| return !missing.every((artifact) => covered.has(`${safeId(item.taskId) ?? ''}::${artifact}`)); |
There was a problem hiding this comment.
Prevent Studio governance suppression across projects
Suggested change
| const pendingApprovals = (state?.approvals ?? []) | |
| .filter((item) => (!item?.runId || item.runId === runId) && String(item?.status ?? '').toLowerCase() === 'pending'); | |
| // #494: one decision = one governance item. A blocked-gate record whose | |
| // every missing artifact is covered by a pending approval for the same | |
| // task is the decision that approval card already represents — listing | |
| // both read as "holding 2 gate(s)" for a single human call. Gates with no | |
| // covering approval (or no missing list) still surface as observed truth. | |
| const covered = new Set(pendingApprovals.map((item) => `${safeId(item.taskId) ?? ''}::${item.artifact ?? ''}`)); | |
| const gates = (state?.blockedGates ?? []) | |
| .filter((item) => !item?.runId || item.runId === runId) | |
| .filter((item) => { | |
| const missing = Array.isArray(item?.missing) && item.missing.length ? item.missing : null; | |
| if (!missing) return true; | |
| return !missing.every((artifact) => covered.has(`${safeId(item.taskId) ?? ''}::${artifact}`)); | |
| const pendingApprovals = (state?.approvals ?? []) | |
| .filter((item) => (!item?.runId || item.runId === runId) && String(item?.status ?? '').toLowerCase() === 'pending'); | |
| // #494: one decision = one governance item. A blocked-gate record whose | |
| // every missing artifact is covered by a pending approval for the same | |
| // task is the decision that approval card already represents — listing | |
| // both read as "holding 2 gate(s)" for a single human call. Gates with no | |
| // covering approval (or no missing list) still surface as observed truth. | |
| const covered = new Set(pendingApprovals.map((item) => `${item.projectId ?? item.projectRoot ?? ''}::${safeId(item.taskId) ?? ''}::${item.artifact ?? ''}`)); | |
| const gates = (state?.blockedGates ?? []) | |
| .filter((item) => !item?.runId || item.runId === runId) | |
| .filter((item) => { | |
| const missing = Array.isArray(item?.missing) && item.missing.length ? item.missing : null; | |
| if (!missing) return true; | |
| const scope = item.projectId ?? item.projectRoot ?? ''; | |
| return !missing.every((artifact) => covered.has(`${scope}::${safeId(item.taskId) ?? ''}::${artifact}`)); |
…ache Strix (CWE-863, LOW — live-reproduced): the semantic decision key omitted the project dimension, so equal (runId, taskId, artifact) tuples in two different roots cross-suppressed — a queue card in project A could swallow project B's blocked gate. approvalDecisionKey now leads with projectId ?? projectRoot, the gate-side key passes the gate's project, and the Studio deck matches items to the run by project as well as run id for BOTH listing and gate suppression (legacy records without project info keep run-id matching). Two cross-project regression tests, verified failing first. Qodo (reliability, PR #143 precedent): canonicalPath/canonicalRoot swallowed every realpath failure — EACCES/ELOOP/EIO now log to stderr before degrading to the lexical form; only not-found stays quiet. The memoization cache is removed outright (realpath is microseconds, the root set is tiny, and a cache would freeze identities across symlink retargets) — which also resolves the staleness and vague-perf-claim review notes. Qodo (convention, PR #490 precedent): the browser test's timeouts are named constants. Refs #494 #505 (PR #509 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Review findings addressed in d463c18 — all three reviewers' items:
Suite 1722/1722, lint/typecheck clean. — Claude Main |
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bisected commits, TDD throughout (all tests verified failing on unfixed code first). The second bug was discovered blocking the first one's end-to-end contract — the E2E probe is in the story below.
Closes #494
Closes #505
Commit 1 — #505: realpath-canonical roots
macOS's
/tmpand/varare symlinks into/private, so the registry and--projectcould name one directory two ways; lexicalresolve()kept both and the Hub registered one project twice — "2 projects" for one repo, doubled queue approvals, doubled feed rows/counts. Root intake (state/roots.js+ the injected-roots branch ofbuildFullState) now canonicalizes throughfs.realpathbefore dedup, and identity (resolveProjectDescriptor,runScopeKey,decorateRunIdentity) hashes the canonical path — identity is a function of the real filesystem node, never the spelling. Missing directories degrade to the lexical form (no throw); only successful resolutions cache, so late-appearing roots get their real identity. Pinned by a real-symlink unit test (one root from two spellings; distinct dirs stay distinct; ghosts don't throw). Fixture-pinned tests now derive expectations from the canonical base — same assertions, canonical spelling.Commit 2 — #494: semantic approval dedup
The event-derived gap-filler (#274) deduped against queue entries by raw id, so any queue card whose id isn't the recomputed canonical
approvalQueueId(pre-#274 runs, hand-authored entries, the canonicalfixtureBlockedRuncard) duplicated: two pending cards, "Approvals 2 pending", and a Studio rail "holding 2 gate(s): guardrail-override:07-code, guardrail-override:07-code" for one human decision.(runId, taskId, artifact)(approvalDecisionKey, exported). The queue-backed entry wins; the derived request stays strictly a gap-filler — no-card still derives, unrelated artifacts/other runs never suppress, canonical-id dedup unchanged (all pinned).The E2E probe that found #505
After commit 2's unit tests went green, the browser E2E still saw 2 pending — a
/api/statedump showed the same queue card twice, once per root spelling (/var/...and/private/var/...): the derived duplicate was gone, and the remaining duplicate was #505. Tuple-dedup across projects would have been wrong (equal run ids in genuinely different projects must stay distinct — pinned by the existing collision tests), so the root fix went where it belongs.Verification
Branch is off current main — independent of PRs #504 and #508 (no shared files).
Held for Richardson's batch review — please verify before merge.
🤖 Generated with Claude Code