Fix #478: required Scientist execution fails closed, never PASS on WARN - #490
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>
sdlc_validate's sandbox-execution step degraded ANY unverified result (no
runtime, no configured command, sandbox disabled) to a WARN check — and a
WARN never changed the overall verdict, so the builder's self-reported
tests_run stood unchallenged. A well-formed builder contract could PASS a
"required" stage with no command ever actually executed. Worse, when a
container DID run but hit an infrastructure problem (image pull failure,
daemon hiccup) rather than a real test failure, the non-zero exit was
recorded as an ordinary FAIL — burning a builder retry for something that
was never the builder's fault.
- sandbox.js: new `execution_policy` (required|optional|prohibited,
DEFAULT_SANDBOX_CONFIG default "optional" — a zero-config install's
behavior is byte-for-byte unchanged). resolveExecutionPolicy() resolves
task-level > per-stage > project-global, same precedence family as
resolveSandboxCommand. A per-stage config entry may now set
execution_policy alone, without its own command (opting a stage into
"required" while still relying on the project-global/task command).
- sandbox.js: new looksLikeInfraFailure() heuristic — a non-zero exit with
NO stdout at all and a known Docker/Podman error signature in stderr
("Cannot connect to the Docker daemon", "pull access denied", "Unable to
find image", etc.) means the command never actually ran. Reclassified to
the SAME unverified/observed shape as "no runtime present" instead of
FAIL — a real test failure (stdout present, or no matching signature)
is untouched.
- rstack-sdlc.ts: runValidationExecution now resolves the policy and, when
execution is "required" but unverified for ANY reason (disabled,
unconfigured, no runtime, infra-suspect failure), returns
`infraBlocked: true` with a `BLOCKED_INFRA` check status instead of WARN.
sdlc_validate's claim critical section short-circuits on infraBlocked
BEFORE writing validation.json or touching task.status/the claim nonce —
the task stays exactly as claimed, so re-running sdlc_validate once the
runtime/image/daemon issue is fixed re-attempts the SAME claim for free.
This deliberately never reaches classifyRetryDecision, so an infra block
can never consume an attempt-budget slot. Emits a distinguishable
`execution_blocked_infra` event and a structured tool response naming
the exact reason and the recovery action.
- config-validation.js: validateSandboxConfig recognizes execution_policy
at both the project-global and per-stage levels, flags invalid values,
and no longer requires a per-stage entry to carry a command when it sets
a valid execution_policy instead.
A genuine test FAIL/PASS is untouched by any of this regardless of policy
— required only changes what happens when a real result couldn't be
obtained at all. Test coverage: the infra-failure heuristic (matching and
non-matching signatures, stdout-present exemption, timeout exemption),
resolveExecutionPolicy's precedence chain, config validation, the full
runValidationExecution required/optional/prohibited matrix, and an
end-to-end sdlc_build_next → sdlc_validate integration test proving the
claim/nonce/attempt survive an infra block untouched and a second infra
block is recorded distinctly. 1651/1651 tests, typecheck clean, lint
clean, validate clean, security-audit clean.
Fixes #478.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Strix Security ReviewAll previously reported security findings have been resolved. 1 resolved finding
Review summaryReviewed all security-relevant changes in the PR, including Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 46 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 QodoFail-closed sandbox execution: required policy blocks on unverified + infra heuristics
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}
| * never consume a builder attempt or the claim itself — end-to-end through | ||
| * the real sdlc_build_next / sdlc_validate tools. | ||
| * | ||
| * owner: RStack developed by Richardson Gunde |
There was a problem hiding this comment.
1. Pii in owner comment 📜 Skill insight ⛨ Security
New test-file header comments include a full personal name (Richardson Gunde), which is PII in a code comment. This violates the requirement to avoid exposing sensitive data (including PII) in documentation/comments.
Agent Prompt
## Issue description
A full personal name is added in code comments (PII). The compliance rule requires documentation/code comments not expose sensitive data including PII.
## Issue Context
The PR adds header comments with `owner: RStack developed by Richardson Gunde` in multiple new/changed test files.
## Fix Focus Areas
- tests/execution-policy-478.test.js[1-8]
- tests/security-audit.test.js[1-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1158
to
+1161
| export async function runValidationExecution( | ||
| { projectRoot, task, stageIds = [], config, deps = {} }: | ||
| { projectRoot: string; task: any; stageIds?: string[]; config?: any; deps?: any; }, | ||
| ): Promise<{ record: any | null; check: any }> { | ||
| ): Promise<{ record: any | null; check: any; policy: string; infraBlocked: boolean }> { |
There was a problem hiding this comment.
2. runvalidationexecution() uses any 📜 Skill insight ✧ Quality
The updated runValidationExecution signature and return type use any, which weakens type safety and violates the TypeScript typing requirement. This can hide real shape mismatches for task, config, deps, and the returned check/record objects.
Agent Prompt
## Issue description
`runValidationExecution` introduces/retains `any` in its public TypeScript types, violating the rule that forbids `any`.
## Issue Context
The function now returns additional fields (`policy`, `infraBlocked`), so this is a good moment to define explicit interfaces/types for the function input and return shapes.
## Fix Focus Areas
- src/integrations/pi/rstack-sdlc.ts[1158-1161]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+108
to
+123
| 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(', ')}]`); | ||
| } | ||
|
|
||
| if (blocking.length) { | ||
| console.error('\nBlocking high/critical advisories in our dependency tree:'); | ||
| for (const b of blocking) { | ||
| console.error(` - ${b.name} (${b.severity})`); | ||
| for (const n of b.nodes || []) console.error(` ${n}`); | ||
| } | ||
| console.error('\nRun `npm audit` for details, then bump or override the offending package.'); | ||
| process.exit(1); | ||
| } | ||
| console.error('\nRun `npm audit` for details, then bump or override the offending package.'); | ||
| process.exit(1); | ||
|
|
||
| console.log(`\nSecurity baseline OK — ${tolerated.length} tolerated bundled advisory(ies), 0 blocking.`); |
There was a problem hiding this comment.
3. security-audit.mjs uses console.log 📜 Skill insight ✧ Quality
The script includes console.log statements, which are disallowed by the debug-logging compliance rule. Keeping these can introduce unwanted output in CI and violates the requirement to remove debug logging statements before merge.
Agent Prompt
## Issue description
Changed code includes `console.log(...)`, which is disallowed by the compliance rule banning debug logging statements.
## Issue Context
This is a CI-facing script; if output is necessary, prefer a sanctioned logging approach (or move informational output to `console.error` if allowed by project conventions).
## Fix Focus Areas
- scripts/security-audit.mjs[108-123]
ⓘ 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
+174
to
+189
| const INFRA_FAILURE_SIGNATURES = [ | ||
| /cannot connect to the docker daemon/i, | ||
| /unable to find image/i, | ||
| /no such image/i, | ||
| /manifest (unknown|for .* not found)/i, | ||
| /pull access denied/i, | ||
| /oci runtime (create|exec) failed/i, | ||
| /error response from daemon/i, | ||
| /image .* (not found|not exist)/i, | ||
| ]; | ||
|
|
||
| function looksLikeInfraFailure({ timedOut, exitCode, stdout, stderr }) { | ||
| if (timedOut || exitCode === 0) return false; | ||
| if (stdout.trim()) return false; // the command produced output — it ran | ||
| return INFRA_FAILURE_SIGNATURES.some((pattern) => pattern.test(stderr)); | ||
| } |
There was a problem hiding this comment.
Spoofable stderr heuristic lets untrusted sandbox code suppress real test failures
Resolved — no longer flagged as of 10ffafa.
Comment on lines
+1188
to
+1191
| const baseCheck = executionCheck(record, resolved.command); | ||
| const unverified = !record || record.tier === "unverified" || record.status === "observed"; | ||
| const infraBlocked = policy === "required" && unverified; | ||
| const check = infraBlocked ? { ...baseCheck, status: "BLOCKED_INFRA" } : baseCheck; |
There was a problem hiding this comment.
Optional policy preserves the downgraded result as a non-blocking check
…re signal Strix (HIGH, CWE-807) and qodo both found the same real gap in the infra-failure heuristic: it inferred "the command never ran" from "stdout is empty", but (a) a genuine test failure that only writes to stderr also has empty stdout — a false positive that could silently downgrade a real FAIL to a WARN — and (b) since the command runs via `sh -c` INSIDE the container, repository-controlled test code could deliberately print one of the known Docker/Podman error phrases to stderr and exit non-zero to suppress its own real failure (the untrusted code influencing a trust decision about itself). Fixed with an unspoofable signal instead of an inference: buildSandboxArgv now wraps the untrusted command so a TRUSTED start sentinel (`__rstack_sandbox_started__`) prints to stdout before the command ever runs. looksLikeInfraFailure now checks for the sentinel's presence in the RAW (untruncated) stdout buffer — not "is stdout empty" — so: - a real failure that writes only to stderr still shows the sentinel on stdout and correctly stays FAIL; - untrusted code has no way to make the sentinel ABSENT, since the trusted wrapper's printf already ran and was already captured before the untrusted command got control — the only lever left to the untrusted code is printing the SAME sentinel too, which is harmless (still proves the command started, still a real FAIL either way). The sentinel check runs on the full buffer before tail() truncates it, so a long-output command whose sentinel would otherwise fall out of the 8KB tail window is still correctly recognized as started. Also (qodo, real bug): validateSandboxConfig compared execution_policy values case-sensitively while sandbox.js's resolveExecutionPolicy normalizes via trim().toLowerCase() before comparing — an operator's "Required" would validate as invalid even though it's accepted and applied correctly at runtime. Added a matching normalizeExecutionPolicy helper used by both the top-level and per-stage checks. Also (qodo, minor): extracted the hardcoded `300` stderr-truncation length in the infra-failure evidence string into a named INFRA_FAILURE_EVIDENCE_MAX_CHARS constant. Test coverage: updated the pinned argv test for the new wrapped command format, two new tests proving a genuine stderr-only failure and a deliberately daemon-phrase-mimicking real failure both stay FAIL, a dedicated spoofing-attempt regression test, and case/whitespace insensitivity coverage for the config validator matching the resolver. 1654/1654 tests, typecheck clean, lint clean. Skipped as out of scope for this PR (originate from the already-open #473 branch merged in for CI): PII-in-owner-comment finding (the attribution line is this codebase's established, intentional convention, naming the real project owner — not accidental PII), console.log usage in security-audit.mjs. Skipped as not applicable given this file's pre-existing convention: `any` typing on runValidationExecution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@strix-security please re-run — the spoofable stderr infra-heuristic reported in the previous review has been fixed with an unspoofable trusted start-sentinel. |
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
…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>
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
Qodo (correctness, real): morphChildren used a plain object as the data-key → node map, so a state-derived key like "__proto__" silently failed to store (assignment hits the prototype slot) — the node lost keyed matching and its identity on reorder. Object.create(null) removes every special-key pitfall; a __proto__ round-trip case is pinned in the morph browser test, verified failing on the plain-object map first. Convention items (PR #490 precedent): browser-test timeouts are named constants; test names carry their #495 attribution. Refs #495 (PR #504 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
Qodo (convention, PR #490 precedent): humanizeDurationMs and formatSnapshotAge now branch on named constants (MS_PER_MINUTE, MINUTES_PER_HOUR, HOURS_PER_DAY) — the two formatters are meant to stay in step, and named thresholds make drift visible at a glance. Browser-test timeouts are named constants in both new test files. Refs #491 #492 #493 (PR #508 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
Qodo (convention, PR #490 precedent): the events-timeline depth cap and the rail-label goal length are named constants (EVENTS_TIMELINE_CAP — matching the rollup index's own timeline depth — and PROOF_SOURCE_GOAL_CHARS). Refs #496 #498 (PR #511 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
richard-devbot
pushed a commit
that referenced
this pull request
Jul 31, 2026
7 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.
Summary
Closes #478 (P0, Wave 0 of the #476 autonomy-hardening epic).
sdlc_validate's sandbox-execution step degraded ANY unverified result (no runtime, no configured command, sandbox disabled) to a WARN check — and a WARN never changed the overall verdict, so the builder's self-reportedtests_runstood unchallenged. A well-formed builder contract could PASS a "required" stage with no command ever actually executed. Worse, when a container DID run but hit an infrastructure problem (image pull failure, daemon hiccup) rather than a real test failure, the non-zero exit was recorded as an ordinary FAIL — burning a builder retry for something that was never the builder's fault.sandbox.js: newexecution_policy(required|optional|prohibited, defaultoptional— a zero-config install's behavior is byte-for-byte unchanged).resolveExecutionPolicy()resolves task-level > per-stage > project-global, same precedence family asresolveSandboxCommand. A per-stage config entry may now setexecution_policyalone, without its own command.sandbox.js: newlooksLikeInfraFailure()heuristic — a non-zero exit with NO stdout at all and a known Docker/Podman error signature in stderr (Cannot connect to the Docker daemon,pull access denied,Unable to find image, etc.) means the command never actually ran. Reclassified to the SAME unverified/observed shape as "no runtime present" instead of FAIL — a real test failure (stdout present, or no matching signature) is untouched.rstack-sdlc.ts:runValidationExecutionnow resolves the policy and, when execution isrequiredbut unverified for ANY reason, returnsinfraBlocked: truewith aBLOCKED_INFRAcheck status instead of WARN.sdlc_validate's claim critical section short-circuits oninfraBlockedbefore writingvalidation.jsonor touchingtask.status/the claim nonce — the task stays exactly as claimed, so re-runningsdlc_validateonce the infra issue is fixed re-attempts the SAME claim for free. This deliberately never reachesclassifyRetryDecision, so an infra block can never consume an attempt-budget slot. Emits a distinguishableexecution_blocked_infraevent and a structured tool response naming the exact reason and the recovery action.config-validation.js:validateSandboxConfigrecognizesexecution_policyat both the project-global and per-stage levels, flags invalid values, and no longer requires a per-stage entry to carry a command when it sets a validexecution_policyinstead.A genuine test FAIL/PASS is untouched by any of this regardless of policy —
requiredonly changes what happens when a real result couldn't be obtained at all.Test plan
npx tsx --test tests/sandbox-execution-452.test.js— the infra-failure heuristic (matching/non-matching signatures, stdout-present exemption, timeout exemption) andresolveExecutionPolicy's precedence chainnpx tsx --test tests/sandbox-validate-452.test.js— the fullrunValidationExecutionrequired/optional/prohibited matrix (no runtime, no command, disabled, infra-suspect failure, real FAIL, real PASS) andvalidateSandboxConfig's newexecution_policychecksnpx tsx --test tests/execution-policy-478.test.js— new end-to-end test via the real extension: arequiredtask with no authoritative command blocks on infra (never PASS, never a builder attempt — claim/nonce/task.status untouched, validation.json never written, a second infra block recorded distinctly), and the defaultoptionalpolicy is provably unchanged from pre-[P0][Scientist] Required execution must fail closed as BLOCKED_INFRA, never PASS on WARN #478 behaviornpm test— 1651/1651npm 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)
optional, deliberately. The issue's acceptance criteria describerequiredsemantics precisely; making it the global default would be a breaking change for every existing zero-config install (many currently-passing tests assert WARN-coexists-with-PASS behavior when no runtime/command exists). An operator opts a stage/task into the hard guarantee viasandbox.execution_policy,sandbox.per_stage.<id>.execution_policy, ortask.execution_policy.BLOCKED_INFRAfrom FAIL/PASS (theexecution_blocked_infraevent is emitted and ready for a future dashboard card, but no UI work is included in this PR).BLOCKED_INFRAonly ever appears as a check-level status (already free-text per the validator-contract schema) and is never written astask.statusorvalidation.status, so no schema changes were needed and the blast radius stays contained to the sandbox/validate wiring.🤖 Generated with Claude Code