Repository navigation
Conversation
|
Automatically computed PR review signals (deterministic) Complexity and branch coverage help focus review on functions and their tests; these signals are advisory. Complexity and test coverage13 changed JS/TS files analyzed · 4 flagged functions.
🟢 Improving · 🟠 Needs attention · 🔴 High risk · 🟣 Highest risk — = unavailable · n/a = not applicable · * = incomplete tests Isobar repository mapThis fixed repository map locates changed files and their possible reach through static links. Signals
Legend
|
78587c5 to
47c64e4
Compare
36d0714 to
3b11740
Compare
JWThewes
left a comment
There was a problem hiding this comment.
Please see the 3 inline finding(s) below.
| const result = { ...env }; | ||
| for (const [name, value] of Object.entries(sandboxEnv)) { | ||
| if (value !== outerEnv[name]) result[name] = value; | ||
| } | ||
| for (const name of Object.keys(outerEnv)) { | ||
| if (!(name in sandboxEnv)) delete result[name]; |
There was a problem hiding this comment.
[P1] Enforce credential environment restrictions after restoration
The command wrapper starts from the full invocation environment but only deletes names present in the safe outer environment. Provider credentials are intentionally absent from that outer environment, so an SRT credentials.envVars deny rule removes the variable and this code immediately restores its original secret. Masking likewise cannot work because SRT's worker never receives the real variable from which to build a sentinel.
Suggested fix: Enforce credential environment rules while reconstructing the harness environment, or reject credential-environment settings until they can be supported without exposing secrets to setup helpers. Test both deny and mask with a secret supplied only through the invocation environment.
Evidence: The launcher starts the worker with childEnvironment({}, process.env), excluding inference credentials, while this function begins with {...env} and only deletes Object.keys(outerEnv). A direct reproduction with KIRO_API_KEY present only in env and absent after SRT still returned KIRO_API_KEY with its original secret.
| }) => | ||
| new Promise((resolve, reject) => { | ||
| const capture = captureStderrTail > 0; |
There was a problem hiding this comment.
[P2] Allow sandbox cleanup before terminating the worker
Sandbox enablement puts the worker on process-group termination paths that use SIGKILL for timeouts and credential cancellation. SIGKILL prevents runSandboxWorker's finally block and SRT's process-exit cleanup from removing host-side mount-point placeholders, which can leave ghost files or directories in a persistent workspace.
Suggested fix: Cancel the worker gracefully through IPC or SIGTERM, wait for its cleanup to finish, and retain group SIGKILL only as a delayed fallback. Add timeout and credential-cancellation tests that verify SRT-created host paths are removed.
Evidence: A focused sandboxed captureChild timeout sent SIGKILL to the negative worker PID. The pinned SRT 0.0.81 implementation documents that absent deny paths create persistent host mount points and that cleanupAfterCommand or its exit handler must remove them; neither runs after SIGKILL.
|
|
||
| const exitCode = result?.exitCode ?? 0; | ||
| logger.error('cli exit', { | ||
| logger[exitCode === 0 ? 'info' : 'error']('cli exit', { |
There was a problem hiding this comment.
[P2] Do not log a failed spawn as a successful exit
An asynchronous child spawn error returns exitCode null, but runStageAttempt normalizes that value to zero before this new severity selection. The event is therefore logged at INFO as a successful exit, removing the previous ERROR-level signal while downstream logic also treats the launch as successful.
Suggested fix: Preserve null as a launch failure, emit an error-level cli_error, and fail the stage instead of defaulting it to zero. Add a test that emits the child-process error event rather than close with an integer code.
Evidence: A direct runChild reproduction with an asynchronous ENOENT-style error returned {exitCode:null}; the current normalization produced exit code 0 and selected the info logger. The added severity tests cover only close codes 0 and 2.
3b11740 to
c915412
Compare
c915412 to
7a9098c
Compare
7a9098c to
52b176d
Compare

Collaborative AIDLC agents need to run commands, read source files, install dependencies and execute repository code without asking for approval at every step. Those capabilities also let an agent misled by prompt injection, or a malicious dependency it executes, read accessible secrets, modify unrelated files or send data to an unintended destination.
We want platform admins to control which files and network destinations agents can access through one platform-wide configuration, while preserving their ability to work autonomously. Disabling individual tools is insufficient: blocking the AWS CLI, for example, still leaves a Python script using the AWS SDK. The restriction needs to apply to the underlying resource access, including child processes.
Why SRT
Anthropic Sandbox Runtime (SRT) fits this requirement because:
SRT's JSON settings also give us one configuration format for the initial admin editor and the later visual UI. It complements IAM and MCP/Gateway authorization; it does not prevent misuse of files or destinations explicitly allowed by its settings.
This is the first of three planned PRs, with an optional fourth. It adds the runtime foundation and troubleshooting logs. Sandboxing remains disabled by default, so this PR alone does not mitigate the risk for normal intents; the next PR connects admin configuration and enables enforcement.
What this PR adds
allowNestedUserNamespaces: true. Tools requiring nested namespaces need a separate execution environment.powertools_log_levelcontrols filtering.Planned stack
Each step should be independently mergeable after its predecessors. JSON configuration provides a complete usable feature before the visual editor arrives. The separate MCP/Gateway migration remains outside this PR; compatibility with its final integration must be validated before enabling sandboxing for normal jobs.
Validation and limits
./scripts/agent-e2e-testing.sh --sandboxpassed: protected reads/writes, direct TCP and blocked HTTP succeed without SRT and are blocked with restrictive SRT settings; allowed workspace writes still succeed. Both launch helpers are exercised. The 0.0.81 image tests also verify that a parent write deny defeats a nested write allow and that nested user/mount namespaces are blocked. These are deterministic image tests, not live model calls.--versionpassed), but its model/tool test is pending a test credential. The earlier successful four-harness qualification used 0.0.79; it is not evidence of a Kiro model/tool pass on 0.0.81. The standalone lab uses its documented CLI versions; these runs do not qualify full application flows.See the testing guide for setup, the test matrix and logging details.