Wave 4: runtime-host - #5
Conversation
|
Warning Review limit reached
Next review available in: 40 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds the ChangesRuntime host execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExecuteRequest
participant HostExecutor
participant ChildProcess
participant ArtifactDirectory
ExecuteRequest->>HostExecutor: execute(request)
HostExecutor->>ChildProcess: spawn command with request workspace and environment
ChildProcess-->>HostExecutor: logs and exit status
HostExecutor->>ArtifactDirectory: copy declared artifacts
ArtifactDirectory-->>HostExecutor: paths or copy errors
HostExecutor-->>ExecuteRequest: execution result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoWave 4: Add runtime-host HostExecutor with allowlist, env passing, timeout, and artifacts
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 70 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This pull request is currently not up to standards. While the functional intent to implement the HostExecutor is correctly aligned with the architecture plan, the current implementation contains high-severity security flaws, including path traversal vulnerabilities in artifact collection and potential Denial-of-Service (DoS) via memory exhaustion in the logging mechanism.
Additionally, the execute method in host-executor.ts has become overly complex (cyclomatic complexity of 14), making it difficult to maintain and test. Codacy analysis reports 315 new issues and several code clones, indicating that significant cleanup and refactoring are required before this can be safely merged. The core executor logic is identified as high-risk and requires improved test coverage to validate boundary conditions and security constraints.
About this PR
- The PR introduces 315 new static analysis issues and 8 code clones. A systemic cleanup is required to meet the project's quality standards. Additionally, the core host-executor.ts file is significantly more complex than recommended, which likely contributed to the high-severity logic errors found during review.
Test suggestions
- HostExecutor.canExecute returns false when disabled or given a non-host operation type
- HostExecutor.canExecute validates command against allowlist and ensures timeoutSeconds is positive
- Process execution captures combined stdout/stderr and returns success status on exit code 0
- Process execution returns failure status and correct exit code for non-zero exits
- Timeouts trigger SIGTERM followed by SIGKILL after the 2s grace period
- Environment variables from host are filtered through envAllowlist, while request env/credentials are fully forwarded
- Operation working directory relative to workspace is honored but blocked if it attempts to escape via '..'
- Constructor throws PRIVILEGE_ESCALATION if runAsUid is 0 or allowlist contains sudo/su
- Declared artifacts are copied to artifactDir and missing files are reported in the result error
- Logs are truncated and tagged with a notice when exceeding maxLogBytes
- Increase unit test coverage for HostExecutor logic paths (currently identified as uncovered and complex)
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Increase unit test coverage for HostExecutor logic paths (currently identified as uncovered and complex)
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Code Review by Qodo
1.
|
🤖 CodeAnt AI — Review Status
|
MergerNeeds Review The shipped host executor still has concrete high-risk flaws, including allowlist bypasses for absolute paths, artifact path traversal, and broken SIGKILL escalation via Commit |
There was a problem hiding this comment.
All reported issues were addressed across 33 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
7108fe3 to
b49a057
Compare
b49a057 to
56bd3d8
Compare
56bd3d8 to
8ffc6fd
Compare
8ffc6fd to
df89511
Compare
841ecfc to
ef84f33
Compare
7191fa4 to
aca7737
Compare
4c3b10a to
8d881e2
Compare
8d881e2 to
34f1f80
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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.
Inline comments:
In `@engdocs/architecture/wave-04-runtime-host-plan.md`:
- Around line 163-165: Update the working-directory containment logic in the
runtime host plan to canonicalize both request.workspace and the resolved
operation.workingDir with realpath before checking containment. Reject relative
results equal to "..", beginning with ".." plus the platform separator, or
absolute paths; add tests covering sibling-prefix paths and symlink escapes.
In `@package.json`:
- Line 19: Update the workspace-local tsdown declaration in
packages/runtime-docker/package.json to match the root package.json pin of
0.22.14, or ensure the lockfile enforces that single version across the
workspace. Preserve the existing build dependency configuration while preventing
the caret range from resolving a different version.
In `@packages/core/package.json`:
- Line 18: Update the lint command in the Nx target of project.json to match the
package.json lint script and remove the obsolete --ext .ts argument, while
preserving the existing eslint src invocation.
In `@packages/runtime-host/src/__tests__/host-executor.test.ts`:
- Around line 299-313: The HostExecutor output handling must enforce maxLogBytes
during collection, not after accumulating complete stdout and stderr. Update the
execute/log collection flow and truncateLogs logic to use a bounded buffer that
continues draining both streams, reserves space for the "[log truncated]"
notice, and never exceeds maxLogBytes; tighten the test assertion to require
result.logs.length to be at most maxLogBytes.
- Around line 115-127: Update HostExecutor timeout handling and spawnProcess
termination to track actual child process closure rather than relying on
child.killed, clear the SIGKILL grace timer when the process closes, and
escalate to SIGKILL for the process group or entire process tree when SIGTERM is
ignored. Ensure descendants are terminated so captured stdio closes, and add
coverage for both a SIGTERM-ignoring child and a persistent descendant.
In `@packages/runtime-host/src/allowlist.ts`:
- Around line 27-35: Update Allowlist.isAllowed so non-absolute entries match
only commands that are themselves bare names, never an absolute command path;
retain exact matching for absolute allowlist entries. Adjust the corresponding
allowlist test covering bare entries and absolute commands to verify
/workspace/tools/node is rejected when only "node" is allowed.
In `@packages/runtime-host/src/host-executor.ts`:
- Around line 55-56: Update the timeout validation around
operation.timeoutSeconds to reject non-finite values and values whose
millisecond conversion exceeds Node’s supported timer range before calling
spawnProcess. Preserve the existing rejection for undefined and non-positive
values, returning the same MISSING_TIMEOUT behavior for every unsupported
timeout.
- Around line 147-148: Update the path validation around relative() in the host
executor to use path-segment boundaries: reject only rel === "..", rel beginning
with ".." followed by the platform separator, or an absolute rel. Preserve valid
child paths such as "..cache" and use the existing separator/path utilities.
- Around line 204-211: Update the timeout logic around the child process so the
grace-period callback checks whether the child’s close event has fired, rather
than relying on child.killed. Track the close state from the child process close
handler and send SIGKILL after GRACE_PERIOD_MS when it remains open, preserving
the existing SIGTERM-then-grace-period escalation in the executor flow.
- Around line 27-47: The HostExecutor constructor currently validates runAsUid
but does not apply it to child processes. Update the child-process spawn call to
pass config.runAsUid through the uid option, and reject configurations with an
unset runAsUid when the executor is running as root. Add tests covering the
spawned UID and this fail-closed validation path.
- Around line 158-181: Restrict request.env in buildEnv before merging it into
the child environment by applying a dedicated request-environment allowlist.
Reject PATH, NODE_OPTIONS, and dynamic-loader variables such as LD_PRELOAD, and
reject any keys colliding with config.env or request.credentials so protected
values cannot be overwritten. Preserve the existing merge behavior only for
validated request environment entries.
In `@specs/05-runtime-host/spec.md`:
- Around line 313-315: Update the fenced command block under Commands in
specs/05-runtime-host/spec.md with blank lines immediately before and after the
fence. In host-executor.ts, update resolveCwd to realpath the workspace and
candidate, reject escaped relative paths including "..", parent-prefixed paths,
and absolute paths; resolve bare commands before buildEnv merges request.env.
Pass validated runAsUid to spawn with platform-specific validation and tests,
track actual child exit instead of child.killed before SIGKILL, bound
stdout/stderr capture before truncateLogs, and constrain artifact source and
destination paths to their request-scoped roots.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64b586cc-2410-4156-b69c-fc3cc44420fd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
engdocs/architecture/wave-04-runtime-host-plan.mdpackage.jsonpackages/checks/package.jsonpackages/cli/package.jsonpackages/compiler-earthly/package.jsonpackages/compiler-github/package.jsonpackages/compiler-gitlab/package.jsonpackages/core/package.jsonpackages/core/src/internal/plan.tspackages/findings/package.jsonpackages/ir/package.jsonpackages/planner/package.jsonpackages/policy/package.jsonpackages/runtime-docker/package.jsonpackages/runtime-host/package.jsonpackages/runtime-host/src/__tests__/allowlist.test.tspackages/runtime-host/src/__tests__/errors.test.tspackages/runtime-host/src/__tests__/helpers/fixtures.tspackages/runtime-host/src/__tests__/host-executor.test.tspackages/runtime-host/src/__tests__/public-api.test.tspackages/runtime-host/src/allowlist.tspackages/runtime-host/src/config.tspackages/runtime-host/src/errors.tspackages/runtime-host/src/host-executor.tspackages/runtime-host/src/index.tspackages/runtime-podman/package.jsonpackages/runtime-remote/package.jsonpackages/runtime/package.jsonpackages/sdk/package.jsonspecs/05-runtime-host/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🪛 ast-grep (0.45.1)
packages/runtime-host/src/host-executor.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
engdocs/architecture/wave-04-runtime-host-plan.md
[grammar] ~40-~40: Ensure spelling is correct
Context: ...ostExecutorError→HostTimeoutError, CommandNotAllowedError. - Public re-exports from src/index.ts...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~171-~171: Ensure spelling is correct
Context: ...alidate in constructor; do not actually setuid. ### Slice I — Artifacts + log truncation 18....
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.23.2)
specs/05-runtime-host/spec.md
[warning] 314-314: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
engdocs/architecture/wave-04-runtime-host-plan.md
[warning] 76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 96-96: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 103-103: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 104-104: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/2/3
(MD029, ol-prefix)
[warning] 108-108: Ordered list item prefix
Expected: 2; Actual: 4; Style: 1/2/3
(MD029, ol-prefix)
[warning] 113-113: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 114-114: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 116-116: Ordered list item prefix
Expected: 2; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
[warning] 118-118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 119-119: Ordered list item prefix
Expected: 1; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
[warning] 124-124: Ordered list item prefix
Expected: 2; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
[warning] 132-132: Ordered list item prefix
Expected: 3; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
[warning] 137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 138-138: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
[warning] 142-142: Ordered list item prefix
Expected: 2; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
[warning] 146-146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 147-147: Ordered list item prefix
Expected: 1; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
[warning] 152-152: Ordered list item prefix
Expected: 2; Actual: 13; Style: 1/2/3
(MD029, ol-prefix)
[warning] 156-156: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 157-157: Ordered list item prefix
Expected: 1; Actual: 14; Style: 1/2/3
(MD029, ol-prefix)
[warning] 163-163: Ordered list item prefix
Expected: 2; Actual: 15; Style: 1/2/3
(MD029, ol-prefix)
[warning] 167-167: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 168-168: Ordered list item prefix
Expected: 1; Actual: 16; Style: 1/2/3
(MD029, ol-prefix)
[warning] 171-171: Ordered list item prefix
Expected: 2; Actual: 17; Style: 1/2/3
(MD029, ol-prefix)
[warning] 173-173: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 174-174: Ordered list item prefix
Expected: 1; Actual: 18; Style: 1/2/3
(MD029, ol-prefix)
[warning] 179-179: Ordered list item prefix
Expected: 2; Actual: 19; Style: 1/2/3
(MD029, ol-prefix)
[warning] 181-181: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 182-182: Ordered list item prefix
Expected: 1; Actual: 20; Style: 1/2/3
(MD029, ol-prefix)
[warning] 183-183: Ordered list item prefix
Expected: 2; Actual: 21; Style: 1/2/3
(MD029, ol-prefix)
[warning] 184-184: Ordered list item prefix
Expected: 3; Actual: 22; Style: 1/2/3
(MD029, ol-prefix)
🔇 Additional comments (29)
package.json (2)
15-18: LGTM!Also applies to: 20-20, 22-22
21-21: 🎯 Functional CorrectnessVerify the
typescript-eslintdowngrade.Line 21 pins
typescript-eslintto8.66.0, but the previous range was^8.67.0. Confirm that this downgrade is intentional, that the lockfile resolves8.66.0, and that it works with ESLint9.39.5and TypeScript5.9.3.packages/checks/package.json (1)
18-18: LGTM!packages/cli/package.json (1)
18-18: LGTM!packages/compiler-earthly/package.json (1)
18-18: LGTM!packages/runtime-docker/package.json (2)
5-14: LGTM!Also applies to: 18-18
21-24: 🗄️ Data Integrity & IntegrationVerify the
workspace:*dependency rewrite before publishing.Lines 22-23 add
workspace:*dependencies to a package with public exports. Confirm that the pack/publish workflow rewrites them to concrete versions in the packed manifest. Otherwise, consumers outside the workspace may fail to install the package.packages/runtime-podman/package.json (1)
18-18: LGTM!packages/runtime-remote/package.json (1)
18-18: LGTM!packages/runtime/package.json (1)
18-18: LGTM!packages/sdk/package.json (1)
18-18: LGTM!packages/compiler-github/package.json (1)
18-18: LGTM!packages/compiler-gitlab/package.json (1)
18-18: LGTM!packages/core/src/internal/plan.ts (1)
2-2: LGTM!packages/findings/package.json (1)
18-18: LGTM!packages/ir/package.json (1)
18-18: LGTM!packages/planner/package.json (1)
18-18: LGTM!packages/policy/package.json (1)
18-18: LGTM!engdocs/architecture/wave-04-runtime-host-plan.md (1)
1-162: LGTM!Also applies to: 166-249
packages/runtime-host/src/__tests__/errors.test.ts (1)
1-54: LGTM!packages/runtime-host/src/__tests__/helpers/fixtures.ts (1)
1-60: LGTM!packages/runtime-host/src/__tests__/host-executor.test.ts (1)
1-114: LGTM!Also applies to: 128-298, 314-360
packages/runtime-host/package.json (1)
5-24: LGTM!packages/runtime-host/src/__tests__/public-api.test.ts (1)
1-47: LGTM!packages/runtime-host/src/errors.ts (1)
6-30: LGTM!packages/runtime-host/src/index.ts (1)
3-7: LGTM!packages/runtime-host/src/host-executor.ts (2)
214-219: Bound log capture during process execution.
stdoutandstderrremain unbounded untilclose.truncateLogscannot prevent memory exhaustion during a noisy command.
281-289: Contain artifact source and destination paths.
artifact.pathandartifact.namecan escapeworkspaceandartifactDir. Validate both resolved paths before copying.packages/runtime-host/src/__tests__/allowlist.test.ts (1)
1-45: LGTM!
34f1f80 to
26ef475
Compare
26ef475 to
d2d6fd9
Compare
d2d6fd9 to
d3ff091
Compare
HostExecutor implementation: allowlist enforcement, env/credential passing, artifact collection, log truncation, timeout + SIGKILL grace. 47 tests pass across 4 files (allowlist, errors, host-executor, public-api). Typecheck clean, build produces dist/index.mjs + dist/index.d.mts. Reviewer approved (sv-gab). Spec 05 amended to match built runtime contract. <details> - 47 tests pass - typecheck clean - build green - reviewer approved </details> Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Created eslint.config.mjs (TypeScript-aware, strict, ESM, ESLint 9 flat config) - Added typescript-eslint dependency - Fixed all per-package lint scripts: removed legacy --ext .ts flag (removed in ESLint 9) - Fixed dead imports: core/plan.ts (OperationOutcome), ir/validate.ts (PlanOperation), runtime-host/host-executor.ts (HostTimeoutError) - Relaxed no-unused-vars to warn for test files (standard practice) - Lint now passes repo-wide Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…omplexity; pin devDeps - Refactor HostExecutor.execute (62 lines → 17, complexity 14 → 3) by extracting validateRequest and finalizeResult helpers. - Pin devDependency versions to resolve Codacy dependency-hijack warning. - Fix missing OperationOutcome import in core/plan.ts from rebase.
d3ff091 to
5908184
Compare
|



User description
Summary
Test plan
Stacked on #3
Generated with Devin
CodeAnt-AI Description
Add a controlled host executor for running approved local commands
What Changed
Impact
✅ Controlled local command execution✅ Fewer stuck processes and oversized logs✅ Reduced environment leakage and workspace escapes🔄 Retrigger CodeAnt AI Review
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.