Fix #479: enforce single-active-stage and predecessor ordering at the claim gate - #489
Merged
Merged
Conversation
…#472) npm audit reported 5 high-severity advisories, all tracing to one root: a DoS/ReDoS bug in brace-expansion (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg), reachable via eslint -> minimatch@3.1.5 -> brace-expansion@1.1.16. npm's only reported fix was eslint@10 (isSemVerMajor), which pulls in a new no-useless-assignment rule in @eslint/js's recommended config, flagging 15 pre-existing dead-store patterns across src/ -- too invasive for a security-only fix. Instead: add a root `overrides` entry (this repo already uses the same mechanism for esbuild/protobufjs/ws) forcing brace-expansion to the patched 5.0.8 line. Verified this reaches eslint's own tree (minimatch@3.1.5's brace-expansion now 5.0.8) while, as documented, leaving @earendil-works/pi-coding-agent's shrinkwrapped copy (5.0.6) untouched -- npm honors a shrinkwrap absolutely, so overrides can't reach it regardless. That shrinkwrapped copy is the only remaining high-severity finding, and it's exactly the node this gate's BUNDLED_ROOTS/ TOLERATED_BUNDLED_ADVISORIES allowlist already exists to tolerate -- added the second GHSA id to that allowlist (the gate is deliberately strict per-advisory-id, so a newly surfaced GHSA for an already-tolerated package still fails until consciously added). Verified: npm run lint unchanged (0 errors, same 4 pre-existing warnings -- confirms minimatch@3.1.5 works fine with the newer brace-expansion at runtime), npm run typecheck clean, gate logic simulated against real npm audit output now reports 0 blocking.
Qodo flagged four issues on the eslint/brace-expansion security fix: - no test coverage for the TOLERATED_BUNDLED_ADVISORIES allowlist change - brace-expansion@5.0.8 declares engines.node "20 || >=22", excluding the Node 18 CI lane - the ">=5.0.8" override is unbounded and can float to a future major - the script comment mis-attributed the version bump to an eslint 9->10 bump that never happened (the real mechanism is the root override) Fix: bound the override to an exact "5.0.8" pin; export the pure tolerate/block decision logic from security-audit.mjs (evaluateAdvisories, isFullyBundled, advisoryIds) behind a main-guard so it's unit-testable without shelling out to npm audit, and add tests/security-audit.test.js covering the tolerate/block/allowlist-miss/severity-threshold paths, plus a pinned-allowlist regression test; correct the stale comment to document the real mechanism and the accepted Node 18 engines trade-off (no brace-expansion release fixes both GHSAs while keeping Node 18 support; it's a devDependency-only path and no engine-strict is set, so this doesn't fail CI in practice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… claim gate
sdlc_build_next's claim selector picked the next FAIL/BLOCKED/PENDING/READY
task without ever checking whether another task was already IN_PROGRESS, and
the Definition-of-Ready gate only checked pending decisions, never
predecessor-stage completion. The model-free pipeline runner already refused
both cases in its own planning layer (planNextAction validates the active
task before claiming more; parallel_groups authorization gates concurrent
claims) — but a direct sdlc_build_next call bypassed that layer entirely,
since the check lived only in the runner, never in the claim gate itself.
Repeated calls could claim multiple stages concurrently with no parallel
authorization, and a later stage could be claimed while an earlier one had
never passed.
- pipeline-state.js: new findUnmetPredecessorTask(tasks, candidate) — the
first task whose canonical stages are entirely before the candidate's
earliest stage and whose status isn't yet passed. Returns null when the
candidate has no canonical stage mapping (adopted/custom tasks aren't
subject to this ordering) or every true predecessor has passed.
- parallel-benchmark.js: new isAuthorizedParallelClaim({activeStageIds,
candidateStageIds, parallelGroups}) — reuses the existing
checkDataIndependence primitive to decide whether one specific
active/candidate PAIR may run concurrently. Distinct from
pipeline-run.js's planParallelGroup (which asks "is the ENTIRE fresh
frontier one authorized group", used to batch-claim a whole group at
once) — this is the narrower question the claim gate needs for a single
pair, backed by the same config + independence check either way.
- rstack-sdlc.ts: sdlc_build_next's claim critical section now (a) refuses a
second claim while another task is IN_PROGRESS unless the pair is
authorized parallel work, and (b) refuses a fresh (never-claimed)
PENDING/READY task if a true predecessor hasn't passed — UNLESS that
predecessor is itself authorized to run concurrently with the candidate
(the same parallel_groups check, so an authorized pair is never blocked by
its own sibling). Retries (FAIL/BLOCKED reclaims) are exempt from the
predecessor check — they already passed it on their first claim, and the
existing #265 claim-order policy deliberately lets a retry jump the queue.
Both refusals return a structured, distinguishable reason
(active_stage_exists / dependency_unmet, never a bare "no pending task")
and emit claim_refused_active_stage / claim_refused_dependency events.
Test coverage: unit tests for both new pure helpers, plus an end-to-end
integration test reproducing the exact audit repro (two sdlc_build_next
calls without validating the first → refusal, not a second IN_PROGRESS
task), the predecessor-ordering case, and the authorized-parallel-pair case.
1640/1640 tests, typecheck clean, lint clean, validate clean, security-audit
clean.
Fixes #479.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Strix Security ReviewAll previously reported security findings have been resolved. 1 resolved findingReview summaryReviewed the security-relevant changes in the claim-gate hardening flow, including the new active/candidate parallel authorization helper, exhaustive predecessor detection, the Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 52 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 ignored due to path filters (1)
📒 Files selected for processing (9)
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 QodoEnforce single-active-stage and canonical ordering in sdlc_build_next
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}
Comment on lines
+48
to
52
| export function advisoryIds(info) { | ||
| return (info.via ?? []) | ||
| .filter((entry) => typeof entry === 'object' && entry?.url) | ||
| .map((entry) => String(entry.url).split('/').pop()); | ||
| } |
There was a problem hiding this comment.
2. advisoryids not verb-noun 📜 Skill insight ⚙ Maintainability
The function name advisoryIds does not follow the required verb-noun naming convention, making intent less explicit (e.g., extract/get vs. a noun phrase). Renaming improves consistency and discoverability across the codebase.
Agent Prompt
## Issue description
`scripts/security-audit.mjs` exports `advisoryIds`, which is a noun phrase rather than a verb-noun action name.
## Issue Context
Compliance requires JS/TS function names to start with a verb (e.g., `get`, `extract`, `build`, `calculate`). This function extracts GHSA IDs from audit entries, so a name like `extractAdvisoryIds` or `getAdvisoryIds` would comply.
## Fix Focus Areas
- scripts/security-audit.mjs[48-52]
- scripts/security-audit.mjs[87-96] (update call sites)
- tests/security-audit.test.js[15-25] (update imports/usage)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2250
to
+2268
| const activeTask = taskState.tasks.find((t: any) => t.status === "IN_PROGRESS" && t.id !== task.id); | ||
| // Loaded once and reused by both #479 checks below, so an authorized | ||
| // pair reads the SAME config/independence verdict for both "is a | ||
| // second active stage allowed" and "is this predecessor exempt". | ||
| const parallelGroups = (activeTask || task.status === "PENDING" || task.status === "READY") | ||
| ? await loadProjectParallelGroups(projectRoot) | ||
| : null; | ||
| if (activeTask) { | ||
| const authorized = isAuthorizedParallelClaim({ | ||
| activeStageIds: taskStageIds(activeTask), | ||
| candidateStageIds: taskStageIds(task), | ||
| parallelGroups, | ||
| }); | ||
| if (!authorized) { | ||
| return { | ||
| taskState, task: null, | ||
| missing: [] as string[], requiredApprovals: [] as string[], readiness: null as any, guardrailCheck: null as any, approvalAuditEvents: [] as any[], | ||
| activeStageBlock: { active_task_id: activeTask.id, candidate_task_id: task.id }, | ||
| }; |
There was a problem hiding this comment.
4. New any types in claim gate 📜 Skill insight ✧ Quality
The new claim-gate logic introduces any annotations and as any casts for task objects, weakening type safety in a critical claim-ordering/concurrency control path and increasing the risk of runtime errors. This violates the compliance requirement to avoid any in TypeScript MCP server code and use explicit types instead.
Agent Prompt
## Issue description
New claim-gate code in `src/integrations/pi/rstack-sdlc.ts` introduces TypeScript `any` (e.g., `find((t: any) => ...)` and `as any` casts) for task objects within the `sdlc_build_next` claim gate logic, violating compliance rules that require avoiding `any` in MCP server TypeScript code.
## Issue Context
Compliance explicitly forbids the use of `any` in TypeScript (PR Compliance IDs 1399957 and 1400551). This code path is a critical gate for claim ordering / concurrency enforcement and relies on task fields such as `id` and `status`; using `any` removes compile-time verification that these fields exist and match expected types, increasing the chance of runtime errors.
## Fix Focus Areas
- src/integrations/pi/rstack-sdlc.ts[2250-2268]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+108
to
+111
| if (tolerated.length) { | ||
| console.log('Tolerated advisories (vendored/bundled, unpatchable downstream — track upstream):'); | ||
| for (const t of tolerated) console.log(` - ${t.name} (${t.severity}) [${t.ids.join(', ')}]`); | ||
| } |
There was a problem hiding this comment.
5. security-audit.mjs uses console.log 📜 Skill insight ✧ Quality
The modified security audit script writes output using console.log, which is disallowed by the no-debug-logging compliance rule. This can lead to accidental debug output persisting in committed source files.
Agent Prompt
## Issue description
Changed code in `scripts/security-audit.mjs` uses `console.log(...)`, which violates the "no debug logging statements" rule.
## Issue Context
This file is a CI/security gate script; it can still emit user-facing output, but it should do so via the project's logging abstraction (or another approved mechanism) rather than direct `console.log`.
## Fix Focus Areas
- scripts/security-audit.mjs[108-111]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Comment on lines
+2281
to
+2287
| if (task.status === "PENDING" || task.status === "READY") { | ||
| const unmetPredecessor = findUnmetPredecessorTask(taskState.tasks, task); | ||
| const predecessorExempt = unmetPredecessor && isAuthorizedParallelClaim({ | ||
| activeStageIds: taskStageIds(unmetPredecessor), | ||
| candidateStageIds: taskStageIds(task), | ||
| parallelGroups, | ||
| }); |
There was a problem hiding this comment.
Parallel-group predecessor exemption bypasses NEEDS_CONTEXT human gate in sdlc_build_next
NEEDS_CONTEXT human gate in sdlc_build_nextResolved — no longer flagged as of 32e5b6b.
…sors Two reviewers (qodo, Strix) found real correctness bugs in the claim gate's #479 checks, all stemming from the same root cause: checking only ONE item (via .find()) when the invariant actually requires checking EVERY item. 1. (qodo, Action required) Active-stage check used .find() to grab a single IN_PROGRESS task and authorize the candidate against just that one. With overlapping parallel_groups (e.g. [A,B] and [A,C]), a candidate authorized against A (found first) could be wrongly allowed even though a DIFFERENT active task B was not authorized against it. Fixed: .filter() to get every active task, refuse if ANY of them is unauthorized with the candidate. 2. (qodo, Action required) Predecessor check only ever looked at the FIRST unmet predecessor (findUnmetPredecessorTask), and if that one happened to be exempt via parallel-group authorization, a second, non-exempt unmet predecessor elsewhere in the plan was never even checked. Fixed: new findUnmetPredecessorTasks (plural) returns every unmet predecessor; the claim gate finds the first one that ISN'T exempt, and refuses if any such predecessor exists. The singular function is now a thin wrapper (`[0]`) kept for its existing narrow callers/tests. 3. (Strix, HIGH, CWE-862) The predecessor exemption only checked parallel_groups CONFIG authorization, never whether the predecessor was actually running. Strix reproduced this live: validate an early stage into NEEDS_CONTEXT (its parallel_groups sibling is still configured as authorized), then call sdlc_build_next directly for that sibling — it claimed anyway, bypassing the runner's intended stop-for-human-context behavior. Fixed: exemption now requires `predecessor.status === "IN_PROGRESS"` in addition to authorization — a predecessor merely CONFIGURED into a group but not actually running (NEEDS_CONTEXT, BLOCKED, FAIL) is never exempt. Authorization only ever widens what may run alongside a genuinely active peer; it never excuses a predecessor that isn't running at all. Also (qodo DRY finding): extracted the repeated "no task granted" return object into one `refusal()` helper shared by the three return sites (plain no-task, active-stage block, dependency block) instead of duplicating the same seven-field literal three times. Bonus fix found while writing the adversarial tests below: the dependency_unmet tool response's `details` was missing `predecessor_status` (present in the emitted event, but not in what the caller sees) — added. Test coverage: findUnmetPredecessorTasks (plural) unit tests, plus three new end-to-end tests reproducing each finding exactly — overlapping active-task authorization, multiple unmet predecessors with only one exempt, and the Strix NEEDS_CONTEXT bypass. 1645/1645 tests (1 known, unrelated, pre-existing temp-dir-cleanup flake in dashboard-env-write.test.js that passes in isolation), typecheck clean, lint clean. Skipped as out of scope for this PR (originate from the already-open #473 branch merged in for CI): security-audit.mjs advisoryIds naming, console.log usage. Skipped as not applicable given this file's pre-existing, pervasive convention: `any` typing in the claim gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@strix-security please re-run — the NEEDS_CONTEXT predecessor-exemption bypass reported in the previous review has been fixed (exemption now requires the predecessor to be actively IN_PROGRESS, not just configured into a parallel group). |
This was referenced Jul 31, 2026
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.
Summary
Closes #479 (P0, Wave 0 of the #476 autonomy-hardening epic).
sdlc_build_next's claim selector picked the next FAIL/BLOCKED/PENDING/READY task without ever checking whether another task was already IN_PROGRESS, and the Definition-of-Ready gate only checked pending decisions, never predecessor-stage completion. The model-free pipeline runner already refused both cases in its own planning layer (planNextActionvalidates the active task before claiming more;parallel_groupsauthorization gates concurrent claims) — but a directsdlc_build_nextcall bypassed that layer entirely, since the check lived only in the runner. Repeated calls could claim multiple stages concurrently with no authorization, and a later stage could be claimed while an earlier one had never passed — exactly the audit's reproduction steps.pipeline-state.js: newfindUnmetPredecessorTask(tasks, candidate)— the first task whose canonical stages are entirely before the candidate's earliest stage and whose status isn't yet passed. Returnsnullwhen the candidate has no canonical stage mapping (adopted/custom tasks aren't subject to this ordering) or every true predecessor has passed.parallel-benchmark.js: newisAuthorizedParallelClaim({activeStageIds, candidateStageIds, parallelGroups})— reuses the existingcheckDataIndependenceprimitive to decide whether one specific active/candidate pair may run concurrently. Distinct frompipeline-run.js'splanParallelGroup(which asks "is the ENTIRE fresh frontier one authorized group", used to batch-claim a whole group at once) — this is the narrower question the claim gate needs for a single pair, backed by the same config + independence check either way.rstack-sdlc.ts:sdlc_build_next's claim critical section now:active_stage_exists/dependency_unmet, never a bare "no pending task") and emitclaim_refused_active_stage/claim_refused_dependencyeventsTest plan
npx tsx --test tests/parallel-benchmark.test.js— unit coverage forisAuthorizedParallelClaim(disabled config, authorized pair, partial-membership, non-independent group, bundled multi-stage task)npx tsx --test tests/pipeline-state.test.js— unit coverage forfindUnmetPredecessorTask(unmet predecessor, satisfied predecessors, later-stage tasks ignored, bundled tasks, no-canonical-mapping exemptions)npx tsx --test tests/stage-ordering-479.test.js— new end-to-end integration test via the real extension: the exact audit repro (secondsdlc_build_nextcall refused, never a second IN_PROGRESS task), validating unblocks the next claim again, predecessor-ordering refusal + resolution, and an authorizedparallel_groupspair claiming concurrently without being blocked by its own siblingnpm test— 1640/1640npm run typecheck— cleannpm run lint— clean (pre-existing unrelated warnings only)npm run validate— cleannode scripts/security-audit.mjs— clean (branch includes the Fix Security baseline gate: override brace-expansion in eslint's dependency tree #473 brace-expansion fix so CI is green regardless of merge order)Scope notes (read before merge)
parallel_groupsconfig (.rstack/rstack.config.json) andcheckDataIndependence— this PR does not introduce a new scheduler-issued/expiring group-token mechanism (that richer model belongs with [P0][Durability] Add immutable attempt ledger, BUILT state, leases, CAS transitions, and transactional outbox #481's transactional foundation, per the epic's own dependency map). It does close the concrete Wave-0 gap: the SAME authorization source the runner already trusts is now consulted by the claim gate itself, so it can't be bypassed by callingsdlc_build_nextdirectly.pipeline run --dry-runparity with this exact claim-gate logic is not addressed here — they already shareplanNextAction, which independently implements equivalent (but not code-identical) active-task/parallel-group logic. Unifying them onto one function is#479's own "dashboard next-action and pipeline run --dry-run consume the same claim-planning function" acceptance criterion; flagging as a reasonable follow-up rather than folding a runner refactor into this fix.🤖 Generated with Claude Code