feat(#452): wire Transient Sandbox into sdlc_validate — The Scientist feeds the Critic (PR 2/3)#463
Conversation
…ce mapping The Scientist gains the pieces the validate gate needs: - resolveSandboxConfig / loadSandboxConfig read a `sandbox` block from .rstack/rstack.config.json (sibling to guardrails; malformed → safe defaults). - resolveSandboxCommand resolves the command to execute from TRUSTED sources only (task.test_command written by the planner, per-stage config, or the project-global command) — NEVER the untrusted builder's tests_run, or a builder could declare tests_run:["true"] and mint its own green. Returns null → honest 'observed' degrade, never a false PASS. - executionCheck maps a runInSandbox record → a validation check; on FAIL the evidence IS the actual captured logs (not a summary), shaped for #451. - config-validation names unknown keys / bad types / typo'd per-stage ids so a misconfigured command can't silently leave execution unverified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire The Scientist into the gate: - runValidationExecution runs the authoritative command in the transient container and returns a validation check + raw execution record. - sdlc_validate folds the REAL exit code into the verdict, replacing the builder's self-reported tests_run signal: FAIL fails validation (its evidence carrying the actual logs), PASS upgrades testsRunOk to observed truth, no runtime / no command degrades to a WARN (never a false green). Runs inside the tasks.json lock (validation is the attempt's serialization point). - The execution record is persisted into validation.json and pinned as an execution_recorded event (tier + exit code). - priorCritiqueBlock now surfaces the failed run's REAL captured output at the top of the retry prompt — the Scientist feeding the Critic — bounded so a huge tail can't dominate, with the duplicate log-dump bullet dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- New tests/sandbox-validate-452.test.js: config resolution, authoritative command priority (never builder tests_run), execution→check mapping with real logs on FAIL, runValidationExecution wiring via injected spawn/runtime (FAIL/ PASS/no-runtime/disabled/no-command), the Scientist→Critic end-to-end through priorCritiqueBlock, and config validation. - Switch the fake child's setImmediate → setTimeout(…,0) here and in the PR1 file: setImmediate isn't in the eslint globals allow-list (a latent lint error that slipped through PR1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 41 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 (3)
📝 WalkthroughWalkthroughSandbox configuration is now validated and resolved into authoritative commands. SDLC validation executes configured sandbox tests, records real execution evidence, gates failures, and exposes failure output to retry critique. Tests cover configuration, execution outcomes, wiring, and asynchronous process behavior. ChangesSandbox validation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant sdlc_validate
participant SandboxConfig
participant SandboxRuntime
participant validation_json
sdlc_validate->>SandboxConfig: Load and resolve sandbox settings
sdlc_validate->>SandboxConfig: Resolve authoritative command
sdlc_validate->>SandboxRuntime: Run configured test command
SandboxRuntime-->>sdlc_validate: Return execution record
sdlc_validate->>validation_json: Store check and execution evidence
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/harness/config-validation.js (1)
131-174: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
limitssub-fields and the timeout upper bound are unchecked, contradicting this function's own stated goal.The doc comment above (lines 122-124) explicitly frames the risk here as silent failure that must be "named" — but
sandbox.limits.{memory,pids,cpus}are never type/shape-validated (only the key itself is allowlisted), and there's no warning whentimeout_msexceedsMAX_TIMEOUT_MS.resolveSandboxConfiginsrc/core/harness/sandbox.jssilently drops bad-typed limits and clamps oversized timeouts with no diagnostic surfaced back to the user — a config typo likelimits: { pids: "many" }or an oversizedtimeout_mspasses validation cleanly and is silently altered downstream.♻️ Proposed addition
if (sandbox.limits != null) { if (!isPlainObject(sandbox.limits)) { issues.push({ field: 'sandbox.limits', problem: 'must be an object of { memory, pids, cpus }' }); } else { if (sandbox.limits.memory != null && (typeof sandbox.limits.memory !== 'string' || !sandbox.limits.memory.trim())) { issues.push({ field: 'sandbox.limits.memory', problem: 'must be a non-empty string (e.g. "512m") — the default applies' }); } for (const key of ['pids', 'cpus']) { if (sandbox.limits[key] != null && !isNonNegativeNumber(sandbox.limits[key])) { issues.push({ field: `sandbox.limits.${key}`, problem: `must be a non-negative number, got ${JSON.stringify(sandbox.limits[key])} — the default applies` }); } } } } + if (timeout != null && isNonNegativeNumber(timeout) && Number(timeout) > MAX_TIMEOUT_MS) { + issues.push({ field: 'sandbox.timeout_ms', problem: `exceeds the hard cap — it will be clamped` }); + }🤖 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 `@src/core/harness/config-validation.js` around lines 131 - 174, Extend validateSandboxConfig to validate sandbox.limits sub-fields memory, pids, and cpus with the types and non-negative constraints expected by resolveSandboxConfig, reporting invalid values instead of only allowlisting the limits key. Also validate timeout_ms/timeoutMs against MAX_TIMEOUT_MS and add an issue when the value exceeds that bound, while preserving the existing alias handling and lower-bound validation.src/core/harness/sandbox.js (1)
255-286: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
task.test_commandbranch ignores matching per-stage image/network/timeout overrides.When
task.test_commandis set (source 1), the returned config always uses the globalimage/network/timeoutMs, even ifstageIdsmatches aperStageentry configured with a different image (e.g. a Python image forpytest). A planner setting a task-level command for a stage that also has a per-stage image override will silently run in the wrong (defaultalpine:3.20) image, producing spurious failures unrelated to code correctness. No test covers image/network/timeout for this branch — only.commandis asserted (tests/sandbox-validate-452.test.js:81-86).♻️ Proposed fix
const taskCommand = typeof task?.test_command === 'string' && task.test_command.trim() ? task.test_command.trim() : null; if (taskCommand) { - return { command: taskCommand, image: config.image, network: config.network, timeoutMs: config.timeoutMs, limits: config.limits }; + const matchedStage = stageIds.map((id) => config.perStage?.[id]).find(Boolean); + return { + command: taskCommand, + image: matchedStage?.image ?? config.image, + network: matchedStage?.network ?? config.network, + timeoutMs: matchedStage?.timeoutMs ?? config.timeoutMs, + limits: config.limits, + }; }Separately: please confirm
task.test_commandtruly has no path from a builder-writable field (e.g. no PATCH/merge path lets a builder set it on the persisted task) — the "authoritative, never builder-controlled" guarantee this function documents depends entirely on that upstream invariant holding.🤖 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 `@src/core/harness/sandbox.js` around lines 255 - 286, Update resolveSandboxCommand so the task.test_command branch preserves matching perStage image, network, and timeoutMs overrides while still using the task-level command; fall back to global config values when no stage override exists. Add coverage for these overrides and verify upstream task persistence/merge paths prevent builder-controlled fields from setting task.test_command.
🤖 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.
Nitpick comments:
In `@src/core/harness/config-validation.js`:
- Around line 131-174: Extend validateSandboxConfig to validate sandbox.limits
sub-fields memory, pids, and cpus with the types and non-negative constraints
expected by resolveSandboxConfig, reporting invalid values instead of only
allowlisting the limits key. Also validate timeout_ms/timeoutMs against
MAX_TIMEOUT_MS and add an issue when the value exceeds that bound, while
preserving the existing alias handling and lower-bound validation.
In `@src/core/harness/sandbox.js`:
- Around line 255-286: Update resolveSandboxCommand so the task.test_command
branch preserves matching perStage image, network, and timeoutMs overrides while
still using the task-level command; fall back to global config values when no
stage override exists. Add coverage for these overrides and verify upstream task
persistence/merge paths prevent builder-controlled fields from setting
task.test_command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 940b8bb8-dbd4-4bdb-bd15-02b7ef40c849
📒 Files selected for processing (5)
src/core/harness/config-validation.jssrc/core/harness/sandbox.jssrc/integrations/pi/rstack-sdlc.tstests/sandbox-execution-452.test.jstests/sandbox-validate-452.test.js
…eRabbit #463) validateSandboxConfig only allowlisted the `limits` key and never flagged a timeout above the hard cap — contradicting its own goal of NAMING every silent failure. resolveSandboxConfig silently drops bad-typed limits and clamps oversized timeouts, so a typo like limits:{pids:"many"} or an over-cap timeout_ms passed validation clean and was quietly altered. Now each is named, against the single MAX_TIMEOUT_MS exported from the executor it bounds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
#452 PR 2/3 — wire the Transient Sandbox into
sdlc_validate(The Scientist feeds the Critic)Builds on PR 1 (the sandbox core, #461 + #462). PR 1 could run untrusted code and author execution evidence; PR 2 makes the gate actually use it — the loop now runs → observes → learns.
What changed
sdlc_validatenow runs the authoritative test command in the transient container and folds the real exit code into the verdict, upgrading the builder's self-reportedtests_runsignal to observed truth:evidenceis the actual captured logs (per the posture: "the Evidence must be the actual log output, not a summary").testsRunOkbecomes container-verified, not builder-claimed.observed), never a false green. Same honest degrade as PR 1.resolveSandboxCommandtakes the command fromtask.test_command(planner-written), per-stage config, or the project-globalsandbox.command— never the untrusted builder'stests_run, or a builder could declaretests_run: ["true"]and mint its own green. This is a correctness/security invariant, not a preference.priorCritiqueBlock, so the retrying builder sees exactly what broke (bounded so a huge tail can't dominate;validation.jsonkeeps the full 8 KB tail).sandboxblock in.rstack/rstack.config.json(enabled/image/command/network/timeout_ms/per_stage/limits), validated field-by-field (typo'd stage/command named, never silently unverified). Execution record persisted intovalidation.json; pinnedexecution_recordedevent (tier + exit code) for the ledger/dashboard/doctor.Posture (Richardson's sign-off this session)
unverified. Auto-upgrade to container whenever Docker/Podman is present. Network OFF by default. No unconfined tier — the "no runtime" case degrades, it does not run on the host.Honest scope / notes
/tmptmpfs; suites that must write into the repo need a project image/config that accommodates that. The defaultalpine:3.20runs shell only — real suites setsandbox.image(e.g.node:20-alpine).Verification
git diff --checkclean.tests/sandbox-validate-452.test.js(config, authoritative-command priority incl. "never builder tests_run", execution→check mapping with real logs,runValidationExecutionvia injected spawn/runtime for FAIL/PASS/no-runtime/disabled/no-command, and the Scientist→Critic loop throughpriorCritiqueBlock).Next — PR 3/3:
doctorreports the active sandbox tier (container vs. unverified) + opt-in bounded auto-start (podman machine start/open -a Docker, never a hidden GUI launch mid-gate). Then #453 (Quality & Risk Index) rides on this real signal.Refs #452 #451
Summary by CodeRabbit
New Features
Bug Fixes
Tests