feat: add tiered sandbox testing system - #2734
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded a tiered sandbox-testing platform with schemas, routing, execution backends, escalation, CI workflows, reports, documentation, packaging, fixtures, and validation tests. Synchronized plugin and documentation metadata now reports 287 skills. ChangesSandbox testing platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
haelyra
changed the title
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
🤖 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 `@docs/design/sandbox-testing/BACKENDS.md`:
- Around line 52-56: Align the Apple-guest lifecycle lock documentation between
BACKENDS.md lines 52-56 and CONVENTIONS.md lines 185-188 by defining one
consistent acquisition, ownership, release, stale-lock, and manual-recovery
contract; if the implementation intentionally uses multiple steps, explicitly
document their relationship and recovery semantics in both records.
In `@docs/design/sandbox-testing/CONVENTIONS.md`:
- Around line 137-142: Update schemas/sandbox-report.schema.json so every
normalized layer-diff, install-diff, and artifact-validation path list enforces
a maximum of 1,000 items, matching the documented bound in CONVENTIONS.md.
Replace the broader stringArray limit or add the constraint to each relevant
array schema, ensuring oversized reports fail validation.
In `@docs/design/sandbox-testing/evidence/phase8-claude-code.json`:
- Around line 2-11: Correct the Claude acceptance evidence to match the routing
contract: in docs/design/sandbox-testing/evidence/phase8-claude-code.json (lines
2-11), either use the dedicated undeclared-capability denial fixture for
SRT-to-Podman escalation or revise the interpretation to describe a direct Tier
1 run; then update docs/design/sandbox-testing/PROGRESS.md (line 97) to describe
the same corrected fixture and route.
In `@docs/design/sandbox-testing/evidence/phase8-codex.json`:
- Line 9: Update the evidence record’s escalation entry to remove the invalid
srt -> podman transition, and ensure the JSON reflects the actual final route
for install-acme-cli-3-1-4; if no valid route is available, remove or mark the
stale record instead.
In `@docs/sandbox-testing.md`:
- Around line 48-51: Update the runtime installation guidance in the
sandbox-testing documentation to pin the tested ecc-universal version from the
package manifest or locked dependency set, and include --ignore-scripts when
installing runtime-only dependencies. Replace the unversioned global install
command while preserving the warning against running repository-local
ecc-sandbox binaries.
In `@hooks/hooks.json`:
- Around line 164-174: Update the Bash hook command for
post:bash-failure:sandbox-escalation-suggest to invoke
scripts/hooks/run-with-flags.js with the hook ID,
scripts/hooks/sandbox-escalation-suggest.js, and standard,strict arguments
instead of directly bootstrapping the hook. Configure a 30-second command
timeout for this hook while preserving its existing description and ID.
In `@images/sandbox/Containerfile.debian`:
- Around line 15-16: Remove the sudo package and unrestricted ecc NOPASSWD
sudoers rule from images/sandbox/Containerfile.debian lines 15-16,
images/sandbox/Containerfile.fedora lines 14-15, and
images/sandbox/Containerfile.ubuntu lines 15-16. If any privileged operation is
required, permit only that specific fixed command rather than granting
unrestricted root access.
In `@package.json`:
- Line 464: Update the test:sandbox script to include
tests/hooks/sandbox-escalation-suggest.test.js in its command chain, preserving
execution of all existing sandbox tests.
In `@scripts/hooks/sandbox-escalation-suggest.js`:
- Around line 75-77: Update shellQuote and the command construction around its
usage in the sandbox escalation suggestion so the CLI path is quoted according
to the host platform, including paths with spaces on Windows cmd.exe while
preserving correct POSIX quoting on macOS and Linux. Use Node.js platform
detection and keep the existing command behavior unchanged aside from
cross-platform path escaping.
In `@scripts/sandbox/backends/ci-native.js`:
- Around line 79-88: Update the explicitEnabled condition in the ci-native
execution gate so options.mock only enables mock mode when options.run is also
provided, while preserving the existing GitHub Actions and ECC_SANDBOX_CI_NATIVE
checks. Ensure callers with mock enabled but no injected runner remain blocked
rather than falling back to defaultRunner and reporting mock execution.
In `@scripts/sandbox/backends/ci.js`:
- Around line 289-298: Validate repository in the repository-resolution flow
before it is used by remoteHead to build the gh api
repos/${repository}/commits/... path. Require exactly the owner/repo shape with
non-empty segments and reject values containing additional slashes, dot
segments, or other invalid path components, returning fail with a clear error;
apply the same validation whether repository comes from options.repository,
ECC_SANDBOX_CI_REPO, or gh repo view output.
In `@scripts/sandbox/backends/lume.js`:
- Around line 35-50: Set LC_ALL=C in the spawn environment used by
readProcessIdentity, processGroupIsAlive, and readProcessTable so ps output
remains in the English format expected by their parsers; preserve the existing
environment while adding this locale override.
- Around line 117-288: Split startLume into smaller functions under 50 lines by
moving the lifecycle closures (ownership markers, helper discovery, and
stop/signal/force-stop methods) into a named attachLumeLifecycle(child, context)
helper. Keep startLume focused on launching the process, validating its PID,
verifying identity, and returning the result, passing the required state and
dependencies through context without changing behavior.
- Around line 159-172: Update the !identity branch in the launcher cleanup flow
to avoid process.kill(-pid, 'SIGTERM'), since ownership was never verified.
Signal the created child through its handle instead, matching the ownership-safe
behavior used by signalOwned and forceStop, then preserve the existing unref and
error result.
In `@scripts/sandbox/backends/podman.js`:
- Around line 158-334: Refactor executeContainer into focused helper functions
under 50 lines, separating runtime readiness, image resolution, container
create/start lifecycle, step execution, diff collection, and cleanup while
preserving the existing accumulators and report behavior. Reduce nesting in
executeContainer to no more than four levels, and have helpers communicate
through explicit arguments/results without changing execution, error, timeout,
or cleanup semantics.
- Around line 188-311: Wrap the container lifecycle in the backend’s main
execution flow with try/finally so cleanup always runs after createAttempted
becomes true, including when invoke, normalizeStep, parseContainerDiff, or
injected execution callbacks throw. Move the existing rm cleanup block into the
finally clause, preserving cleanupSucceeded handling, failure notes,
unsafe-state behavior, and normal execution results.
In `@scripts/sandbox/backends/srt.js`:
- Around line 126-271: Split executeSrt into helpers so each function remains
under 50 lines: extract temp-root/settings preparation, platform-specific
command execution, and final report assembly. Add a named helper for Windows
argv construction, and a note-building helper that returns a new array without
mutating shared state. Preserve the existing execution, cleanup, denial, and
exit-code behavior.
- Line 12: Bound the input passed to isSrtDenial before applying DENIAL_PATTERN,
rather than scanning the full joined stdout/stderr buffers. Also replace the
unbounded sandbox-to-denial gap in DENIAL_PATTERN with a finite maximum,
preserving detection of denial messages within that bound and all existing
denial terms.
In `@scripts/sandbox/backends/tart.js`:
- Around line 103-112: Update the startArgs function to remove the conditional
--net-host flag, or invert its condition so --net-host is included only when the
manifest explicitly lacks a network:* capability, while preserving the remaining
Tart arguments.
In `@scripts/sandbox/backends/vm-scan.js`:
- Around line 62-66: Update the Linux regex in the services classification
around bounded and present.filter to also match user-level systemd unit paths
under .config/systemd/user, while preserving the existing systemd/system,
init.d, rc.d, and macOS matching behavior.
In `@scripts/sandbox/backends/vm.js`:
- Around line 197-499: Decompose executeVm into small stage helpers, each under
50 lines and no deeper than four nesting levels, using an early-return pipeline
where each stage returns { pass, notes }; separate seed verification, preflight,
clone/configure, startup/readiness, ownership barrier, scanning, and step
execution while preserving existing state and report behavior. Extract the
entire finally cleanup sequence into cleanupVm(...), including launcher
shutdown, VM stop, deletion, reservation release, cleanup notes, and
executionError updates, then have executeVm orchestrate stages and build the
report.
- Around line 136-195: Make the stale-lock policy user-visible in the VM backend
flow that handles the failed result from acquireRunLock: when pass is false and
the lock is stale, output the returned note before terminating or reporting the
failed run. Preserve the existing manual-removal instruction and ensure it is
surfaced to users rather than only stored in notes or tests.
In `@scripts/sandbox/ecc-sandbox`:
- Around line 345-359: Update the outcome-building logic around the destination
and initial report assignments to avoid mutating either input parameter:
construct and return new outcome objects with the updated report values applied.
Apply the same immutable-copy pattern to all corresponding assignments at the
referenced paths, preserving existing report fields and behavior.
- Around line 576-583: Add the shared eligibility marker constant to the SRT
backend alongside executeSrt, then update the validation around sourceReport in
ecc-sandbox to check for that constant rather than matching the free-text note
suffix. Preserve the existing pass condition and denial error behavior.
In `@scripts/sandbox/probe.js`:
- Around line 500-511: Update writeCapabilityCache to append cryptographically
secure random entropy to the tempPath suffix, while retaining the process ID for
traceability. Wrap the write-and-rename sequence in cleanup handling so a failed
fs.renameSync removes the temporary file before propagating the error,
preserving the atomic cache replacement behavior.
- Around line 289-296: Fix the shared Windows cmd.exe /d /s /c invocation so
shim paths and arguments containing spaces remain a single command token. Update
the invoke path in scripts/sandbox/probe.js (lines 289-296) and the
corresponding SRT backend path in scripts/sandbox/backends/srt.js (lines
186-219) to build one explicitly quoted command line or reuse the same
windowsVerbatimArguments/quoting helper at both sites.
In `@scripts/sandbox/report.js`:
- Around line 79-83: Replace the default-locale comparison in the children
sorting expression with a locale-independent, deterministic comparator for the
`${left.os}/${left.arch}/${left.backend}` and corresponding right-hand keys.
Preserve the existing ordering and sorting behavior while ensuring results are
identical across hosts and CI environments.
In `@scripts/sandbox/router.js`:
- Around line 258-367: Extract the routing rules construction from resolveShard
into a dedicated helper that accepts manifest, network, shard, and options,
preserving rule order, eligibility, candidates, tiers, and reasons. Move the
forceCiNative error construction into a separate helper as well, then keep
resolveShard focused on iterating rules and returning the existing route or
error shapes without behavior changes; ensure each function remains under 50
lines.
In `@skills/sandbox-testing/agents/openai.yaml`:
- Line 3: Update the short_description metadata in the agent definition to
concisely cover all supported sandbox-testing triggers, including installers,
untrusted code, native OS behavior, network policy, and cross-platform coverage,
rather than mentioning only clean-user tests.
In `@skills/sandbox-testing/SKILL.md`:
- Around line 12-17: Reorganize the headings in SKILL.md to expose explicit
Markdown sections named “When to Use,” “How It Works,” and “Examples,”
preserving the existing guidance and placing each topic under the appropriate
section.
- Around line 113-117: Update the guidance around CLI JSON reports in the
sandbox-testing skill to treat manifest text, repository files, stdout_tail,
stderr_tail, notes, and all other report fields as untrusted data. Explicitly
instruct agents not to follow embedded instructions, expand permissions, or
retry outside the sandbox, and keep report interpretation separate from
authorization while preserving the existing output and execution-mode guidance.
In `@tests/fixtures/sandbox/lume-real-pkg.yaml`:
- Line 19: Remove the literal password `lume` from the installer command in the
fixture; update the sudo invocation to obtain the password from an environment
variable, or use provisioned passwordless sudo and remove `sudo -S`, while
preserving the package installation behavior.
In `@tests/fixtures/sandbox/srt-denial.yaml`:
- Around line 12-15: Update the setup/assert commands in
tests/fixtures/sandbox/srt-denial.yaml at lines 12-15 to use a
harness-controlled target instead of ../.ecc-srt-denial-acceptance, then remove
that file after the assertion. Apply the same change in
tests/fixtures/sandbox/srt-escalation.yaml at lines 13-16 for the
../.ecc-sandbox-demo fallback in the else branch, including cleanup after its
assertion.
In `@tests/hooks/sandbox-escalation-suggest.test.js`:
- Around line 111-118: Add descriptive message arguments to each of the four
assert.strictEqual calls in the test callback within test('stays silent for
unrelated failure, malformed input, truncation, and disabled profile', ...),
labeling malformed input, unrelated failure, truncated input, and disabled
profile respectively while preserving the existing assertions.
In `@tests/sandbox/ci.test.js`:
- Around line 364-389: Update the symlink setup in the variant loop to create a
Windows-safe directory link: pass `'junction'` to fs.symlinkSync when
process.platform is 'win32', while preserving the existing symlink behavior on
other platforms.
In `@tests/sandbox/contracts-router.test.js`:
- Around line 239-243: In the routing fixture test setup, add an explicit
assertion before iterating over routingFixtures.cases that the collection exists
under the expected key and contains at least one case. Keep the existing
per-case loop and assertions unchanged, ensuring an empty or renamed fixture
causes the suite to fail.
In `@tests/sandbox/microsandbox-escalation.test.js`:
- Around line 401-422: Update executeWithEscalation and its callers so
escalationBudget is returned in the outcome rather than mutating
executionOptions.escalationBudget. In runExecution, pass the returned remaining
budget to subsequent local routes, then reset it before CI routes to preserve
one escalation per run. Update the affected test assertions and setup to
validate the returned-budget flow and absence of caller-owned mutation.
In `@tests/sandbox/probe.test.js`:
- Around line 380-382: Replace the direct process.exit calls after the pass/fail
summaries with conditional process.exitCode assignments so stdout can flush:
update tests/sandbox/probe.test.js lines 380-382 and
tests/sandbox/report-srt.test.js lines 356-358 to set exitCode to 1 only when
failed > 0, leaving successful runs without an explicit exit code.
In `@tests/sandbox/vm.test.js`:
- Around line 29-39: Update the test harness around test() to support an
explicit SKIP result and maintain a separate skipped counter instead of counting
skipped cases as passes. Change the three non-macOS early returns in the Lume,
Lima, and Tart CLI tests to return SKIP, and include the skipped count in the
final summary.
- Around line 743-745: Update the lock assertions in the affected tests to use
process-scoped test lock names, including a `${process.pid}` suffix, instead of
`apple-macos-guests` or the fixed `apple-macos-guests-test` name. Ensure the
release assertion targets the same process-scoped lock acquired by the test,
avoiding production locks and collisions between concurrent test processes.
- Around line 642-643: Add a manifestWithTimeout helper that creates a new
manifest using object spreads, applies the timeout override, and passes the
result through validateManifest. Replace each short manifest() followed by
direct resources.timeout assignment, including the occurrences near lines 642,
673, and 722, with manifestWithTimeout(1).
In `@tests/skills/sandbox-testing.test.js`:
- Around line 57-76: Replace the repeated computed imports of the sandbox
contracts module in the test cases with one top-level literal require using the
established ../../scripts/sandbox/contracts specifier, then reuse the imported
contracts symbols throughout the tests, including loadManifest, validateReport,
and parseManifestText.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b21cc5f-5f1f-43d2-aded-5596767d3325
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (74)
.claude-plugin/marketplace.json.claude-plugin/plugin.json.github/workflows/sandbox-matrix.yml.github/workflows/sandbox-probe.yml.github/workflows/sandbox-tier1.ymlAGENTS.mdREADME.mdREADME.zh-CN.mddocs/design/sandbox-testing/BACKENDS.mddocs/design/sandbox-testing/CONVENTIONS.mddocs/design/sandbox-testing/PROGRESS.mddocs/design/sandbox-testing/evidence/phase8-claude-code.jsondocs/design/sandbox-testing/evidence/phase8-codex.jsondocs/sandbox-testing.mddocs/tr/AGENTS.mddocs/zh-CN/AGENTS.mddocs/zh-CN/README.mdexamples/sandbox/install-ecc-clean-user.yamlhooks/hooks.jsonimages/sandbox/Containerfile.debianimages/sandbox/Containerfile.fedoraimages/sandbox/Containerfile.ubuntumanifests/install-modules.jsonpackage.jsonschemas/sandbox-capabilities.schema.jsonschemas/sandbox-manifest.schema.jsonschemas/sandbox-report.schema.jsonscripts/hooks/sandbox-escalation-suggest.jsscripts/sandbox/backends/ci-native.jsscripts/sandbox/backends/ci.jsscripts/sandbox/backends/lima.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/microsandbox.jsscripts/sandbox/backends/podman.jsscripts/sandbox/backends/srt.jsscripts/sandbox/backends/tart.jsscripts/sandbox/backends/vm-scan.jsscripts/sandbox/backends/vm.jsscripts/sandbox/contracts.jsscripts/sandbox/ecc-sandboxscripts/sandbox/mock.jsscripts/sandbox/probe.jsscripts/sandbox/report.jsscripts/sandbox/router.jsskills/sandbox-testing/SKILL.mdskills/sandbox-testing/agents/openai.yamltests/fixtures/sandbox/agent-surface-failure.jsontests/fixtures/sandbox/agent-surface-output.schema.jsontests/fixtures/sandbox/ci-matrix.yamltests/fixtures/sandbox/container-native-escalation.yamltests/fixtures/sandbox/ecc-demo-tool.shtests/fixtures/sandbox/invalid-capability.yamltests/fixtures/sandbox/lima-native.yamltests/fixtures/sandbox/lume-cleanup-smoke.yamltests/fixtures/sandbox/lume-native.yamltests/fixtures/sandbox/lume-real-pkg.yamltests/fixtures/sandbox/podman-install.yamltests/fixtures/sandbox/routing-cases.jsontests/fixtures/sandbox/srt-benign.yamltests/fixtures/sandbox/srt-denial.yamltests/fixtures/sandbox/srt-escalation.yamltests/fixtures/sandbox/tart-native.yamltests/fixtures/sandbox/unknown-key.yamltests/fixtures/sandbox/valid.yamltests/hooks/sandbox-escalation-suggest.test.jstests/sandbox/ci.test.jstests/sandbox/contracts-router.test.jstests/sandbox/microsandbox-escalation.test.jstests/sandbox/podman.test.jstests/sandbox/probe.test.jstests/sandbox/report-srt.test.jstests/sandbox/vm.test.jstests/scripts/npm-publish-surface.test.jstests/skills/sandbox-testing.test.js
| - Lume and Tart share a stale-owner-aware ECC lifecycle lock and count both | ||
| backends' running guests. This serializes ECC macOS VM starts and avoids a | ||
| cross-tool race against Apple's two-running-guest ceiling. Lock acquisition | ||
| is an atomic prepared-directory rename; stale locks fail closed with an exact | ||
| manual recovery path rather than risking concurrent automatic takeover. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant docs/files =="
fd -a 'BACKENDS\.md|CONVENTIONS\.md|PROGRESS\.md|evidence.*claude-code\.json' docs/design/sandbox-testing 2>/dev/null || true
echo
echo "== git status/stat =="
git status --short
git diff --stat
echo
echo "== relevant doc snippets =="
for f in docs/design/sandbox-testing/BACKENDS.md docs/design/sandbox-testing/CONVENTIONS.md docs/design/sandbox-testing/PROGRESS.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '45,60p;175,195p;90,105p'
fi
done
echo
echo "== search lifecycle lock terms =="
rg -n "lifecycle lock|prepar(e|ed)-directory|owner file|random token|stale|release|two-running|Apple" docs/design/sandbox-testing -SRepository: affaan-m/ECC
Length of output: 504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate likely implementation files =="
git ls-files | rg -i -- '(^|/)(sandbox|lume|tart|apple|lifecycle|lock|evidence|evidence/phase8-claude-code|script|src|bin|scripts|dockerfile|compose)' | head -500
echo
echo "== implementation searches for lock keywords =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' -i \
"lifecycle|lock|rename|owner|stale|Apple|two-running|guest|sandbox|lume|tart" .Repository: affaan-m/ECC
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant sandbox design snippets =="
echo "--- docs/design/sandbox-testing/BACKENDS.md 45-62 ---"
awk 'NR>=45 && NR<=62 {printf "%6d %s\n", NR, $0}' docs/design/sandbox-testing/BACKENDS.md
echo "--- docs/design/sandbox-testing/CONVENTIONS.md 175-192 ---"
awk 'NR>=175 && NR<=192 {printf "%6d %s\n", NR, $0}' docs/design/sandbox-testing/CONVENTIONS.md
echo "--- docs/design/sandbox-testing/PROGRESS.md 90-100 ---"
awk 'NR>=90 && NR<=100 {printf "%6d %s\n", NR, $0}' docs/design/sandbox-testing/PROGRESS.md
echo "--- docs/design/sandbox-testing/evidence/phase8-claude-code.json 1-30 ---"
if [ -f docs/design/sandbox-testing/evidence/phase8-claude-code.json ]; then
awk 'NR>=1 && NR<=30 {printf "%6d %s\n", NR, $0}' docs/design/sandbox-testing/evidence/phase8-claude-code.json
else
echo "missing"
fi
echo
echo "== focused lock/Apple/guest references in repo, limited output =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' -i \
"lifecycle lock|lifecycle_lock|lifecycle-lock|prepar(ed|e)-directory|owner file|random token|token|stale|release|two-running|Apple|Apple's|Lume|Tart|macOS VM|guest" .
echo
echo "== candidate files near lock/Apple/lume/tart (file-level) =="
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' --files-with-matches -i \
"lifecycle lock|lifecycle_lock|lifecycle-lock|owner file|token|stale|Apple|Lume|Tart|macOS VM|guest" . | head -200Repository: affaan-m/ECC
Length of output: 50369
🌐 Web query:
site:GitHub.com affaan-m/ECC "lifecycle lock" "Apple" "Lume" "Tart"
💡 Result:
The query appears to conflate two distinct projects and concepts: 1. ECC (by affaan-m): This project [1][2] is a configuration/installation management tool that includes an install lifecycle service. It manages an install-state.json file [1][3] to support deterministic uninstall, doctor, and repair commands [1][2]. It explicitly focuses on supply-chain security, including removing install-time lifecycle scripts and enforcing exact-pin dependencies [4][5]. It does not mention "Lume Tart". 2. Tart and Lume: "Tart" is a virtualization tool for macOS and Linux on Apple Silicon [6]. "Lume" is another lightweight CLI tool for managing virtual machines on Apple Silicon [7]. Users often encounter "locking" issues with Tart (e.g., file locks during tart clone [8][9] or keychain locking after version upgrades [10]). There is no established project or technical concept combining these terms into an "ECC lifecycle lock Apple Lume Tart". The mention of a "lifecycle lock" likely refers to the lifecycle management systems in the ECC project [1][2] or the concurrency/file locking mechanisms used by the Tart virtualization CLI [8][9][11], rather than a unified security feature.
Citations:
- 1: Install-state + uninstall / doctor / repair lifecycle #443
- 2: https://github.com/affaan-m/ECC/blob/main/docs/SELECTIVE-INSTALL-ARCHITECTURE.md
- 3: 131f977
- 4: 754b8dd
- 5: https://github.com/affaan-m/ECC/blob/main/docs/security/supply-chain-incident-response.md
- 6: https://github.com/cirruslabs/tart
- 7: https://github.com/tuist/lume
- 8: https://github.com/cirruslabs/tart/blob/4bb248e7b4395e1c1d4683053b710ff6b87f92d8/Sources/tart/Commands/Clone.swift
- 9: tart clone: clone VM and generate MAC under a file lock openai/tart#215
- 10: tart gets locked out of its own passwords after a version upgrade openai/tart#184
- 11: https://github.com/sabatico/orchard_ui
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
echo "== exact tracked candidate snippets =="
for f in docs/design/sandbox-testing/BACKENDS.md docs/design/sandbox-testing/CONVENTIONS.md docs/design/sandbox-testing/PROGRESS.md docs/design/sandbox-testing/evidence/phase8-claude-code.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,130p' "$f" | awk '{printf "%6d %s\n", NR, $0}'
else
echo "--- $f missing ---"
fi
done
echo
echo "== exact lock-word hits under docs/design/sandbox-testing (case-insensitive, limited) =="
rg -n -i \
"lifecycle lock|stale|owner file|random token|released|release|rename|directory|Apple|two-running|VM start|guest" docs/design/sandbox-testing || true
echo
echo "== exact lock-word hits in tracked git files (case-insensitive, limited) =="
git ls-files -z |
xargs -0 rg -n -i \
"lifecycle lock|stale|owner file|random token|released|release|rename|directory|Apple|two-running|VM start|guest|Lume|Tart" \
|| trueRepository: affaan-m/ECC
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
files = [
pathlib.Path("docs/design/sandbox-testing/BACKENDS.md"),
pathlib.Path("docs/design/sandbox-testing/CONVENTIONS.md"),
pathlib.Path("docs/design/sandbox-testing/PROGRESS.md"),
pathlib.Path("docs/design/sandbox-testing/evidence/phase8-claude-code.json"),
]
for path in files:
print(f"EXISTS {path}: {path.exists()}")
print()
for path in files:
print(f"--- {path} ---")
print("\n".join(path.read_text(encoding="utf-8").splitlines()[:150]))
print()
PY
echo
echo "== strict focused lock-related strings in sandbox design doc tree =="
rg -n -i '\b(lifecycle lock|prepared-directory|random token|owner file|stale|release|rename|Apple|two-running|guest|Lume|Tart|sandbox-testing)\b' docs/design/sandbox-testing || trueRepository: affaan-m/ECC
Length of output: 42871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
backends = pathlib.Path("docs/design/sandbox-testing/BACKENDS.md").read_text(encoding="utf-8")
conventions = pathlib.Path("docs/design/sandbox-testing/CONVENTIONS.md").read_text(encoding="utf-8")
progress = pathlib.Path("docs/design/sandbox-testing/PROGRESS.md").read_text(encoding="utf-8")
evidence = pathlib.Path("docs/design/sandbox-testing/evidence/phase8-claude-code.json").read_text(encoding="utf-8")
terms = {
"prepared_directory_rename": "prepared-directory rename",
"rename_lock": "prepared-directory rename",
"owner_file": "atomically exclusive owner file",
"random_token": "random token",
"stale_owner_aware": "stale-owner",
"fails_closed": "fail closed",
"manual_recovery": "manual recovery",
"release_removes_matching": "release removes only the matching token",
}
for site, text in [(("docs/design/sandbox-testing/BACKENDS.md", 52), backends),
(("docs/design/sandbox-testing/CONVENTIONS.md", 185), conventions),
(("docs/design/sandbox_testing/PROGRESS.md", 97), progress),
(("docs/design/sandbox_testing/evidence/phase8-claude-code.json", 2), evidence)]:
file_label, line = site
print(f"--- {file_label}:{line} ---")
for term, needle in terms.items():
print(term, "PRESENT" if needle in text else "absent", "=>", needle)
print()
PYRepository: affaan-m/ECC
Length of output: 1956
Align the Apple-guest lifecycle lock contract with the implementation.
BACKENDS.md documents the lock as an atomic prepared-directory rename with manual recovery, while CONVENTIONS.md documents it as a portable owner file with a random token and token-bound release. Use one contract across both records, or explicitly document any multi-step relationship before manual recovery relies on it.
📍 Affects 2 files
docs/design/sandbox-testing/BACKENDS.md#L52-L56(this comment)docs/design/sandbox-testing/CONVENTIONS.md#L185-L188
🤖 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 `@docs/design/sandbox-testing/BACKENDS.md` around lines 52 - 56, Align the
Apple-guest lifecycle lock documentation between BACKENDS.md lines 52-56 and
CONVENTIONS.md lines 185-188 by defining one consistent acquisition, ownership,
release, stale-lock, and manual-recovery contract; if the implementation
intentionally uses multiple steps, explicitly document their relationship and
recovery semantics in both records.
| 22. Layer-diff reports include directory entries and cap every normalized list | ||
| at 1,000 paths. Executable and top-level home dotfile classifications include | ||
| deleted paths; `services_registered` includes only added/changed services, | ||
| while removed services remain explicit in `files_deleted`. Malformed or | ||
| truncated install evidence makes an `install-diff` run an error, never a | ||
| passing partial report. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only inspection; this does not execute repository code or modify files.
rg -n -C 4 'maxItems|1000|10000|validate.*report|artifact|install_diff' schemas scripts tests || trueRepository: affaan-m/ECC
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^docs/design/sandbox-testing/CONVENTIONS\.md$|^docs/design/sandbox-testing/BACKENDS\.md$|^schemas/sandbox-report\.schema\.json$|sandbox|sandbox-report|install-diff|install_diff|diff)'
echo
echo "== doc lines =="
if [ -f docs/design/sandbox-testing/CONVENTIONS.md ]; then
sed -n '125,175p' docs/design/sandbox-testing/CONVENTIONS.md
fi
echo
echo "== schema maxItems and stringArray =="
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('schemas/sandbox-report.schema.json')
if p.exists():
data=json.loads(p.read_text())
print(json.dumps({k:v for k,v in data.items()}, indent=2)[:8000])
print('--- stringArray references ---')
for k,v in data.get('$defs',{}).items():
print(k, v.get('type'), v.get('minItems'), v.get('maxItems'))
PY
echo
echo "== focused validator/schema mentions =="
rg -n -C 3 'sandbox-report|10000|1000|maxItems|install-diff|install_diff|artifact' docs scripts schemas tests | head -n 300Repository: affaan-m/ECC
Length of output: 42709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== report.js outline and focused lines =="
wc -l scripts/sandbox/report.js scripts/sandbox/contracts.js tests/sandbox/contracts-router.test.js
sed -n '1,260p' scripts/sandbox/report.js
echo
echo "== validateReport usages and artifact validation mentions =="
rg -n -C 4 'validateReport|validate artifact|artifact.*validate|schema validated|schema-validated|10000|1000|install_diff|files_added|files_changed|files_deleted|path_changes|services_registered|dotfiles_touched' scripts/sandbox scripts tests docs/design/sandbox-testing/CONVENTIONS.md docs/design/sandbox-testing/PROGRESS.md | head -n 300
echo
echo "== behavioral schema probe =="
node - <<'JS'
const fs = require('fs');
const schema = JSON.parse(fs.readFileSync('schemas/sandbox-report.schema.json', 'utf8'));
console.log('stringArray maxItems =', schema.definitions.stringArray.maxItems);
const sample = JSON.parse(fs.readFileSync('tests/fixtures/sandbox/agent-surface-failure.json', 'utf8'));
console.log('sample install_diff path length =', sample.install_diff.files_added.length + sample.install_diff.files_changed.length + sample.install_diff.files_deleted.length + sample.install_diff.path_changes.length + sample.install_diff.services_registered.length + sample.install_diff.dotfiles_touched.length);
console.log('sample notes length =', sample.notes.length);
JSRepository: affaan-m/ECC
Length of output: 29507
Keep the 1,000-path diff bound in both docs and validation.
docs/design/sandbox-testing/CONVENTIONS.md caps normalized layer-diff lists at 1,000 paths, but schemas/sandbox-report.schema.json allows stringArray up to 10,000 items. Add the same bound at the validation boundary, including install-diff lists and artifact validation paths, so oversized reports cannot pass schema validation.
🤖 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 `@docs/design/sandbox-testing/CONVENTIONS.md` around lines 137 - 142, Update
schemas/sandbox-report.schema.json so every normalized layer-diff, install-diff,
and artifact-validation path list enforces a maximum of 1,000 items, matching
the documented bound in CONVENTIONS.md. Replace the broader stringArray limit or
add the constraint to each relevant array schema, ensuring oversized reports
fail validation.
| "manifest_yaml": "name: install-acme-cli\nneeds:\n os: [linux]\n capabilities:\n - clean-home\n - pkg-install\n - network:*\n trust: first-party\n native: false\nresources:\n cpu: 2\n memory: 2GB\n timeout: 300\nsteps:\n setup:\n - npm install --global acme-cli@3.1.4\n assert:\n - command -v acme\n - acme --version | grep -q '^3\\.1\\.4$'\nreport: install-diff\n", | ||
| "interpretation": { | ||
| "result": "fail", | ||
| "backend": "podman", | ||
| "tier": 1, | ||
| "execution_mode": "real", | ||
| "first_failure": "acme --version | grep -q '^3.1.4$' exited 1 with empty bounded output; it is the first nonzero step and matching failed assertion.", | ||
| "escalation": "One recorded transition: srt -> podman at Tier 1 after pkg-install denial.", | ||
| "install_diff_complete": true, | ||
| "evidence_claim": "Real Tier 1 Podman evidence has a complete layer diff, but the version assertion failed. Isolation was degraded because Microsandbox was unavailable; this is not native or cross-platform evidence." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the Claude acceptance evidence with the routing contract.
The fixture declares pkg-install, but the documented router sends such manifests directly to Tier 1. The claimed SRT-to-Podman escalation is therefore not valid evidence for this manifest.
docs/design/sandbox-testing/evidence/phase8-claude-code.json#L2-L11: use the dedicated undeclared-capability denial fixture for escalation, or change the interpretation to a direct Tier 1 run.docs/design/sandbox-testing/PROGRESS.md#L97-L97: update the acceptance row to describe the corrected fixture and route.
📍 Affects 2 files
docs/design/sandbox-testing/evidence/phase8-claude-code.json#L2-L11(this comment)docs/design/sandbox-testing/PROGRESS.md#L97-L97
🤖 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 `@docs/design/sandbox-testing/evidence/phase8-claude-code.json` around lines 2
- 11, Correct the Claude acceptance evidence to match the routing contract: in
docs/design/sandbox-testing/evidence/phase8-claude-code.json (lines 2-11),
either use the dedicated undeclared-capability denial fixture for SRT-to-Podman
escalation or revise the interpretation to describe a direct Tier 1 run; then
update docs/design/sandbox-testing/PROGRESS.md (line 97) to describe the same
corrected fixture and route.
| { | ||
| "matcher": "Bash", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "node -e \"const p=require('path');const r=(function(){var p=require('path'),f=require('fs'),o=require('os');var e=process.env.CLAUDE_PLUGIN_ROOT;if(e&&e.trim())return e.trim();var d=p.join(o.homedir(),'.claude');function L(x){try{return require(p.join(x,'scripts','lib','resolve-ecc-root')).resolveEccRoot()}catch(_){return null}}var r=L(d);if(r)return r;var s=['ecc','ecc@ecc','marketplaces/ecc','everything-claude-code','everything-claude-code@everything-claude-code','marketplaces/everything-claude-code'];for(var i=0;i<s.length;i++){r=L(p.join(d,'plugins',s[i]));if(r)return r}try{var g=['ecc','everything-claude-code'];for(var j=0;j<g.length;j++){var c=p.join(d,'plugins','cache',g[j]);var O=f.readdirSync(c);for(var k=0;k<O.length;k++){var q=p.join(c,O[k]);var V=f.readdirSync(q);for(var m=0;m<V.length;m++){r=L(p.join(q,V[m]));if(r)return r}}}}catch(_){}return d})();const s=p.join(r,'scripts/hooks/plugin-hook-bootstrap.js');process.env.CLAUDE_PLUGIN_ROOT=r;process.argv.splice(1,0,s);require(s)\" node scripts/hooks/sandbox-escalation-suggest.js" | ||
| } | ||
| ], | ||
| "description": "Suggest the tiered sandbox workflow after Bash isolation denials without rerunning or changing permissions", | ||
| "id": "post:bash-failure:sandbox-escalation-suggest" | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the direct invocation against the run-with-flags wrapper contract.
set -euo pipefail
fd -t f 'run-with-flags.js' scripts/hooks --exec cat -n {}
# Confirm the hook id string matches between the config and the script.
rg -n 'post:bash-failure:sandbox-escalation-suggest' hooks/hooks.json scripts/hooks/sandbox-escalation-suggest.jsRepository: affaan-m/ECC
Length of output: 10914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== hook config around sandbox-escalation-suggest ==="
sed -n '150,185p' hooks/hooks.json | cat -n
echo
echo "=== sandbox-escalation-suggest outline and relevant sections ==="
ast-grep outline scripts/hooks/sandbox-escalation-suggest.js --view compact || true
echo
sed -n '1,140p' scripts/hooks/sandbox-escalation-suggest.js | cat -n
sed -n '140,280p' scripts/hooks/sandbox-escalation-suggest.js | cat -n
echo
echo "=== sibling hook entries timeout/use run-with-flags context ==="
rg -n '"timeout"|"scripts/hooks/run-with-flags.js"|"scripts/hooks/.*\.js' hooks/hooks.json
echo
echo "=== hook flags helpers ==="
fd -t f 'hook-flags.(js|ts)' scripts/lib --exec cat -n {}Repository: affaan-m/ECC
Length of output: 16231
Use run-with-flags.js for this hook and set a command timeout.
scripts/hooks/sandbox-escalation-suggest.js exports run(raw, options), so the bootstrap can pass the hook via node scripts/hooks/run-with-flags.js post:bash-failure:sandbox-escalation-suggest scripts/hooks/sandbox-escalation-suggest.js standard,strict. This adds dry-run preview, injected hook metadata, and a 30s legacy command guard. The direct command invocation never times out, but the hook reads stdin before returning.
🤖 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 `@hooks/hooks.json` around lines 164 - 174, Update the Bash hook command for
post:bash-failure:sandbox-escalation-suggest to invoke
scripts/hooks/run-with-flags.js with the hook ID,
scripts/hooks/sandbox-escalation-suggest.js, and standard,strict arguments
instead of directly bootstrapping the hook. Configure a 30-second command
timeout for this hook while preserving its existing description and ID.
Source: Coding guidelines
| && printf 'ecc ALL=(ALL) NOPASSWD: ALL\n' > /etc/sudoers.d/ecc \ | ||
| && chmod 0440 /etc/sudoers.d/ecc |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove unrestricted passwordless root access from the clean-user images.
The ecc user can run arbitrary commands through sudo. This invalidates the clean-user boundary and lets manifest commands modify protected container state as root. Remove sudo and its NOPASSWD rule unless a specific install step requires it. If a privileged operation is required, allow only that fixed command.
images/sandbox/Containerfile.debian#L15-L16: remove the unrestrictedeccsudoers rule and thesudopackage.images/sandbox/Containerfile.fedora#L14-L15: remove the unrestrictedeccsudoers rule and thesudopackage.images/sandbox/Containerfile.ubuntu#L15-L16: remove the unrestrictedeccsudoers rule and thesudopackage.
📍 Affects 3 files
images/sandbox/Containerfile.debian#L15-L16(this comment)images/sandbox/Containerfile.fedora#L14-L15images/sandbox/Containerfile.ubuntu#L15-L16
🤖 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 `@images/sandbox/Containerfile.debian` around lines 15 - 16, Remove the sudo
package and unrestricted ecc NOPASSWD sudoers rule from
images/sandbox/Containerfile.debian lines 15-16,
images/sandbox/Containerfile.fedora lines 14-15, and
images/sandbox/Containerfile.ubuntu lines 15-16. If any privileged operation is
required, permit only that specific fixed command rather than granting
unrestricted root access.
| console.log(`\nPassed: ${passed}`); | ||
| console.log(`Failed: ${failed}`); | ||
| process.exit(failed > 0 ? 1 : 0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Two sandbox harnesses terminate with process.exit and can truncate their own output. Both files print the per-test lines and the pass/fail summary, then call process.exit(failed > 0 ? 1 : 0). On a piped stdout, Node does not flush queued writes before exiting, so CI can lose the failure detail that the harness just printed. tests/sandbox/podman.test.js line 335 and tests/sandbox/vm.test.js line 750 already use the safe form.
tests/sandbox/probe.test.js#L380-L382: replaceprocess.exit(failed > 0 ? 1 : 0)withif (failed > 0) process.exitCode = 1;.tests/sandbox/report-srt.test.js#L356-L358: replaceprocess.exit(failed > 0 ? 1 : 0)withif (failed > 0) process.exitCode = 1;.
📍 Affects 2 files
tests/sandbox/probe.test.js#L380-L382(this comment)tests/sandbox/report-srt.test.js#L356-L358
🤖 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 `@tests/sandbox/probe.test.js` around lines 380 - 382, Replace the direct
process.exit calls after the pass/fail summaries with conditional
process.exitCode assignments so stdout can flush: update
tests/sandbox/probe.test.js lines 380-382 and tests/sandbox/report-srt.test.js
lines 356-358 to set exitCode to 1 only when failed > 0, leaving successful runs
without an explicit exit code.
| function test(name, fn) { | ||
| try { | ||
| fn(); | ||
| console.log(` ✓ ${name}`); | ||
| passed += 1; | ||
| } catch (error) { | ||
| console.log(` ✗ ${name}`); | ||
| console.log(` Error: ${error.stack || error.message}`); | ||
| failed += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The harness counts skipped tests as passes.
test() increments passed whenever fn returns without throwing. Three tests return early on non-macOS hosts (lines 201, 501, 540) and assert nothing. On Linux CI the summary reports them as passing. This hides zero coverage for the Lume, Lima, and Tart CLI paths.
Add an explicit skip channel and a separate counter.
♻️ Proposed skip channel
let passed = 0;
let failed = 0;
+let skipped = 0;
+const SKIP = Symbol('skip');
function test(name, fn) {
try {
- fn();
+ if (fn() === SKIP) {
+ console.log(` - ${name} (skipped)`);
+ skipped += 1;
+ return;
+ }
console.log(` ✓ ${name}`);
passed += 1;
} catch (error) {
console.log(` ✗ ${name}`);
console.log(` Error: ${error.stack || error.message}`);
failed += 1;
}
}Then return SKIP instead of bare return at lines 201, 501, and 540, and print skipped in the summary.
🤖 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 `@tests/sandbox/vm.test.js` around lines 29 - 39, Update the test harness
around test() to support an explicit SKIP result and maintain a separate skipped
counter instead of counting skipped cases as passes. Change the three non-macOS
early returns in the Lume, Lima, and Tart CLI tests to return SKIP, and include
the skipped count in the final summary.
| const short = manifest(); | ||
| short.resources.timeout = 1; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not mutate the validated manifest in place.
Line 643 assigns short.resources.timeout = 1 on the object returned by validateManifest. Lines 673 and 722 repeat the pattern. Two problems follow. The repository guidelines forbid in-place mutation in JavaScript. The mutated value also bypasses validateManifest, so an out-of-range timeout would reach the adapter unchecked.
Build a new manifest with the spread operator and re-validate.
♻️ Proposed helper
+function manifestWithTimeout(seconds, osName = 'macos') {
+ const base = manifest(osName);
+ return validateManifest({
+ ...base,
+ resources: { ...base.resources, timeout: seconds },
+ });
+}Then replace each const short = manifest(); short.resources.timeout = 1; pair with const short = manifestWithTimeout(1);.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const short = manifest(); | |
| short.resources.timeout = 1; | |
| function manifestWithTimeout(seconds, osName = 'macos') { | |
| const base = manifest(osName); | |
| return validateManifest({ | |
| ...base, | |
| resources: { ...base.resources, timeout: seconds }, | |
| }); | |
| } | |
| const short = manifestWithTimeout(1); |
🤖 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 `@tests/sandbox/vm.test.js` around lines 642 - 643, Add a manifestWithTimeout
helper that creates a new manifest using object spreads, applies the timeout
override, and passes the result through validateManifest. Replace each short
manifest() followed by direct resources.timeout assignment, including the
occurrences near lines 642, 673, and 722, with manifestWithTimeout(1).
Source: Coding guidelines
| const lock = acquireRunLock('apple-macos-guests'); | ||
| assert.strictEqual(lock.pass, true); | ||
| lock.release(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not acquire the production Apple VM lock from a test.
Line 743 acquires apple-macos-guests, the same lock name that real sandbox runs use. Two consequences follow. The assertion fails if a real Lume or Tart run holds the lock on the same host. The test also blocks a concurrent real run for the duration it holds the lock.
The test at line 327 has a milder version of the same problem: apple-macos-guests-test is a fixed name, so two concurrent test processes on one host collide. Line 341 already shows the correct pattern with ${process.pid}.
Assert release through a process-scoped name instead.
🐛 Proposed fix
- const lock = acquireRunLock('apple-macos-guests');
+ const lock = acquireRunLock(`apple-macos-guests-pre-ready-${process.pid}`);
assert.strictEqual(lock.pass, true);
lock.release();Note: this changes what the assertion proves. If the intent is to verify that executeLume released the real production lock, keep the production name but make the surrounding test opt-in on a dedicated host, and document that it must not run concurrently with real sandbox runs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lock = acquireRunLock('apple-macos-guests'); | |
| assert.strictEqual(lock.pass, true); | |
| lock.release(); | |
| const lock = acquireRunLock(`apple-macos-guests-pre-ready-${process.pid}`); | |
| assert.strictEqual(lock.pass, true); | |
| lock.release(); |
🤖 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 `@tests/sandbox/vm.test.js` around lines 743 - 745, Update the lock assertions
in the affected tests to use process-scoped test lock names, including a
`${process.pid}` suffix, instead of `apple-macos-guests` or the fixed
`apple-macos-guests-test` name. Ensure the release assertion targets the same
process-scoped lock acquired by the test, avoiding production locks and
collisions between concurrent test processes.
| results.push(test('demo manifest validates against the production contract', () => { | ||
| const { loadManifest } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); | ||
| const manifest = loadManifest(path.join(repoRoot, 'examples', 'sandbox', 'install-ecc-clean-user.yaml')); | ||
| assert.strictEqual(manifest.name, 'install-ecc-clean-user'); | ||
| assert.deepStrictEqual(manifest.needs.os, ['linux']); | ||
| assert.ok(manifest.needs.capabilities.includes('clean-home')); | ||
| assert.ok(manifest.steps.setup[0].includes('/workspace/source')); | ||
| assert.ok(manifest.steps.setup[0].includes('--target codex')); | ||
| })); | ||
|
|
||
| results.push(test('fresh-harness failure fixture validates against the production report contract', () => { | ||
| const { validateReport } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); | ||
| const report = validateReport(JSON.parse(read('tests/fixtures/sandbox/agent-surface-failure.json'))); | ||
| assert.strictEqual(report.result, 'fail'); | ||
| assert.strictEqual(report.escalations.length, 1); | ||
| assert.strictEqual(report.install_diff.complete, true); | ||
| })); | ||
|
|
||
| results.push(test('retained Claude Code and Codex eval outputs remain contract-correct', () => { | ||
| const { parseManifestText } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist the repeated contracts require to a single top-level literal import.
Lines 58, 68, and 76 each call require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')) for the same module. Two consequences:
- The duplication triples the maintenance surface for one dependency.
- The computed specifier produces three
detect-non-literal-requirewarnings from ast-grep. A literal relative specifier removes all three.
tests/sandbox/ci.test.js Line 13 already uses the literal form require('../../scripts/sandbox/contracts'). Match it.
♻️ Proposed fix
const yaml = require('yaml');
+const {
+ loadManifest,
+ parseManifestText,
+ validateReport,
+} = require('../../scripts/sandbox/contracts'); results.push(test('demo manifest validates against the production contract', () => {
- const { loadManifest } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'));
const manifest = loadManifest(path.join(repoRoot, 'examples', 'sandbox', 'install-ecc-clean-user.yaml')); results.push(test('fresh-harness failure fixture validates against the production report contract', () => {
- const { validateReport } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'));
const report = validateReport(JSON.parse(read('tests/fixtures/sandbox/agent-surface-failure.json'))); results.push(test('retained Claude Code and Codex eval outputs remain contract-correct', () => {
- const { parseManifestText } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'));
for (const harness of ['claude-code', 'codex']) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| results.push(test('demo manifest validates against the production contract', () => { | |
| const { loadManifest } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); | |
| const manifest = loadManifest(path.join(repoRoot, 'examples', 'sandbox', 'install-ecc-clean-user.yaml')); | |
| assert.strictEqual(manifest.name, 'install-ecc-clean-user'); | |
| assert.deepStrictEqual(manifest.needs.os, ['linux']); | |
| assert.ok(manifest.needs.capabilities.includes('clean-home')); | |
| assert.ok(manifest.steps.setup[0].includes('/workspace/source')); | |
| assert.ok(manifest.steps.setup[0].includes('--target codex')); | |
| })); | |
| results.push(test('fresh-harness failure fixture validates against the production report contract', () => { | |
| const { validateReport } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); | |
| const report = validateReport(JSON.parse(read('tests/fixtures/sandbox/agent-surface-failure.json'))); | |
| assert.strictEqual(report.result, 'fail'); | |
| assert.strictEqual(report.escalations.length, 1); | |
| assert.strictEqual(report.install_diff.complete, true); | |
| })); | |
| results.push(test('retained Claude Code and Codex eval outputs remain contract-correct', () => { | |
| const { parseManifestText } = require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts')); | |
| const yaml = require('yaml'); | |
| const { | |
| loadManifest, | |
| parseManifestText, | |
| validateReport, | |
| } = require('../../scripts/sandbox/contracts'); | |
| results.push(test('demo manifest validates against the production contract', () => { | |
| const manifest = loadManifest(path.join(repoRoot, 'examples', 'sandbox', 'install-ecc-clean-user.yaml')); | |
| assert.strictEqual(manifest.name, 'install-ecc-clean-user'); | |
| assert.deepStrictEqual(manifest.needs.os, ['linux']); | |
| assert.ok(manifest.needs.capabilities.includes('clean-home')); | |
| assert.ok(manifest.steps.setup[0].includes('/workspace/source')); | |
| assert.ok(manifest.steps.setup[0].includes('--target codex')); | |
| })); | |
| results.push(test('fresh-harness failure fixture validates against the production report contract', () => { | |
| const report = validateReport(JSON.parse(read('tests/fixtures/sandbox/agent-surface-failure.json'))); | |
| assert.strictEqual(report.result, 'fail'); | |
| assert.strictEqual(report.escalations.length, 1); | |
| assert.strictEqual(report.install_diff.complete, true); | |
| })); | |
| results.push(test('retained Claude Code and Codex eval outputs remain contract-correct', () => { | |
| for (const harness of ['claude-code', 'codex']) { |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 57-57: Avoid require with non-literal values
Context: require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'))
Note: [CWE-829] Inclusion of Functionality from Untrusted Control Sphere (dynamic require).
(detect-non-literal-require)
[warning] 67-67: Avoid require with non-literal values
Context: require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'))
Note: [CWE-829] Inclusion of Functionality from Untrusted Control Sphere (dynamic require).
(detect-non-literal-require)
[warning] 75-75: Avoid require with non-literal values
Context: require(path.join(repoRoot, 'scripts', 'sandbox', 'contracts'))
Note: [CWE-829] Inclusion of Functionality from Untrusted Control Sphere (dynamic require).
(detect-non-literal-require)
🤖 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 `@tests/skills/sandbox-testing.test.js` around lines 57 - 76, Replace the
repeated computed imports of the sandbox contracts module in the test cases with
one top-level literal require using the established
../../scripts/sandbox/contracts specifier, then reuse the imported contracts
symbols throughout the tests, including loadManifest, validateReport, and
parseManifestText.
Source: Linters/SAST tools
| "tier": 1, | ||
| "execution_mode": "real", | ||
| "first_failure": "acme --version | grep -q '^3.1.4$' is the first failing command.", | ||
| "escalation": "One recorded transition: srt -> podman due to pkg-install denial.", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(docs/(sandbox-testing\.md|design/sandbox-testing/evidence/phase8-codex\.json)|phase8-codex\.json|sandbox-testing\.md)'
echo
echo "== phase8-codex.json =="
cat -n docs/design/sandbox-testing/evidence/phase8-codex.json
echo
echo "== docs/sandbox-testing.md around policy =="
sed -n '110,170p' docs/sandboxtesting.md 2>/dev/null || sed -n '110,170p' docs/sandbox-testing.mdRepository: affaan-m/ECC
Length of output: 4322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== route mentions =="
rg -n "srt|SRT|package install|package-install|pkg-install|podman|router|route" docs sandbox-testing 2>/dev/null || true
echo
echo "== all phase evidence manifests =="
git ls-files 'docs/design/sandbox-testing/evidence/*.json' | sortRepository: affaan-m/ECC
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package install need documentation =="
sed -n '80,112p' docs/sandbox-testing.md
echo
echo "== srt/route implementation references =="
rg -n "pkg-install|package install|non-native|Tier 0|Tier 1|SRT|srt|router|route" -S --glob '!docs/**/*.md' --glob '!docs/**/*.json' .Repository: affaan-m/ECC
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== router SRT eligibility block =="
sed -n '120,145p' scripts/sandbox/router.js
echo
echo "== route fixtures around install evidence =="
rg -n "install-acme|install-ecc-clean|pkg-install|expected|notePattern|reasonPattern" scripts/sandbox tests/sandbox docs/design/sandbox-testing -SRepository: affaan-m/ECC
Length of output: 10330
Remove the invalid SRT-to-Podman escalation from this evidence record.
install-acme-cli-3-1-4 declares pkg-install, so tierZeroEligible() excludes it and routing does not start at SRT. The recorded srt -> podman transition contradicts the router policy documented in docs/sandbox-testing.md and Conventions. Update this JSON to reflect its actual final route or remove/stale this record.
🤖 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 `@docs/design/sandbox-testing/evidence/phase8-codex.json` at line 9, Update the
evidence record’s escalation entry to remove the invalid srt -> podman
transition, and ensure the JSON reflects the actual final route for
install-acme-cli-3-1-4; if no valid route is available, remove or mark the stale
record instead.
| An npm installation also exposes `ecc-sandbox` as a binary. ECC managed | ||
| content installs copy the skill but not its Node runtime dependencies; install | ||
| the trusted runtime once with `npm install --global ecc-universal`. Never run a | ||
| repository-local `ecc-sandbox` lookalike from an untrusted checkout. The npm |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## repo files related to ecc-universal / sandbox-testing"
git ls-files | rg '(^|/)(sandbox-testing\.md|package\.json|pnpm-lock\.yaml|yarn\.lock|package-lock\.json)$' || true
echo
echo "## docs/sandbox-testing.md relevant section"
if [ -f docs/sandbox-testing.md ]; then
cat -n docs/sandbox-testing.md | sed -n '35,70p'
fi
echo
echo "## package declarations / dependencies"
for f in $(git ls-files '*/package.json' 'package.json' | sort); do
echo "--- $f"
python3 - <<'PY' "$f"
import json, sys, pathlib
p = pathlib.Path(sys.argv[1])
try:
data=json.loads(p.read_text())
except Exception as e:
print(f"json error: {e}")
raise SystemExit
for key in ("dependencies","devDependencies","optionalDependencies","peerDependencies","lockfileVersion","executes");
if key in data:
print(key, data[key])
PY
done
echo
echo "## occurrences of ecc-universal / ecc-sandbox"
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'ecc-universal|ecc-sandbox|sandbox' . || trueRepository: affaan-m/ECC
Length of output: 2381
🌐 Web query:
npm install --global package runs lifecycle scripts lifecycle documentation
💡 Result:
Yes, running npm install --global does execute lifecycle scripts for the installed package [1][2]. Lifecycle scripts—including preinstall, install, and postinstall—are triggered during global installations [1][3]. These scripts run to perform setup tasks, such as compiling native addons via node-gyp, or other necessary environment configurations [1][3][2]. Security and Configuration: 1. Lifecycle Script Blocking: By default, modern versions of npm may restrict or require explicit approval for lifecycle scripts for security reasons [4][5][6]. The allow-scripts configuration (managed via the --allow-scripts flag,.npmrc settings, or the allowScripts field in package.json) controls which packages are permitted to run these scripts [4][7][5]. 2. Global Context: Because global installs (npm install -g) do not have a local package.json to store an allowlist, you can use the --allow-scripts CLI flag to specify authorized packages (e.g., npm install -g --allow-scripts=pkg-name) or set the preference in your user-level.npmrc [4][8][7]. 3. Disabling Scripts: If you wish to prevent scripts from running entirely, you can append the --ignore-scripts flag to your command (e.g., npm install --global --ignore-scripts) [9]. Execution Visibility: If you are concerned about whether scripts are running or what they are doing, you can use the --foreground-scripts flag. This forces lifecycle scripts to run in the foreground process, sharing standard input, output, and error with the main npm process, which makes their execution visible in your terminal [4][10][11].
Citations:
- 1: https://docs.npmjs.com/cli/v11/using-npm/scripts/
- 2: https://fastly.jsdelivr.net/npm/npm@12.0.2/docs/content/using-npm/scripts.md
- 3: https://docs.npmjs.com/cli/v6/using-npm/scripts/
- 4: https://docs.npmjs.com/cli/v11/commands/npm-install/
- 5: https://github.com/npm/rfcs/blob/main/accepted/0054-make-scripts-install-opt-in.md
- 6: [RFC] Make install scripts opt-in npm/rfcs#868
- 7: https://docs.npmjs.com/cli/v11/using-npm/config/
- 8: https://docs.npmjs.com/cli/v12/commands/npm-install-scripts/
- 9: https://cheatsheetseries.owasp.org/cheatsheets/NPM_Security_Cheat_Sheet.html
- 10: https://docs.npmjs.com/cli/v11/commands/npm-install/?v=true
- 11: [BUG] npm install does not run postinstall nor does it show name & version of installed pkg npm/cli#3647
🏁 Script executed:
#!/bin/bash
set -uxo pipefail
echo "## package manifests"
for f in package.json .opencode/package.json tests/fixtures/docker-plugin-project/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f"
fi
done
echo
echo "## dependency references"
rg -n 'ecc-universal|ecc-sandbox|sandbox' package.json .opencode/package.json tests/fixtures/docker-plugin-project/package.json yarn.lock package-lock.json .opencode/package-lock.json || true
echo
echo "## lockfile entries for ecc-universal"
python3 - <<'PY'
import json, pathlib, re
for name in ['package-lock.json', '.opencode/package-lock.json']:
p=pathlib.Path(name)
if not p.exists(): continue
data=json.loads(p.read_text())
print(f'--- {name}')
print('lockfileVersion=', data.get('lockfileVersion'))
packages=data.get('packages', {}) or {}
for k,v in packages.items():
if 'ecc-universal' in k or (v and any('ecc-universal' in str(x) for x in (v.get('dependencies') or {}))):
print(k, {kk:v.get(kk) for kk in ('version','resolved','integrity','dependencies','devDependencies','peerDependencies') if kk in v or kk in (v.get('dependencies') or {})})
break
else:
print('no ecc-universal dependency/package found')
PY
echo
echo "## docs line count"
wc -l docs/sandbox-testing.mdRepository: affaan-m/ECC
Length of output: 27044
🌐 Web query:
npm 9 --ignore-scripts default lifecycle scripts global install
💡 Result:
In npm 9, the --ignore-scripts configuration flag defaults to false [1][2]. When this flag is set to true, it instructs npm to not run lifecycle scripts defined in the package.json file of the packages being installed [1][2]. Regarding global installations specifically: 1. Default Behavior: By default, when you run an install command (including global installs via npm install -g), npm will execute lifecycle scripts (such as preinstall, install, and postinstall) associated with the packages being installed unless --ignore-scripts is explicitly passed [1][2]. 2. Scope of --ignore-scripts: When you use the --ignore-scripts flag during a global installation, it applies to the packages being installed in the same way it does for local installations—it prevents the execution of the lifecycle scripts defined in their package.json files [1][2]. 3. Important Nuance: While --ignore-scripts prevents the execution of package-defined lifecycle scripts, it does not guarantee that all scripts will be ignored in every scenario. For example, some operations, such as installing dependencies from git repositories, may still trigger certain internal processes (like pack) that are not fully suppressed by this flag in all versions [3]. Additionally, note that npm commands explicitly intended to run a specific script (such as npm start, npm stop, npm restart, npm test, and npm run-script) will still execute their designated script even if --ignore-scripts is enabled; however, the associated pre- and post-scripts will be skipped [1][2].
Citations:
- 1: https://docs.npmjs.com/cli/v9/using-npm/config/
- 2: https://docs.npmjs.com/cli/v9/commands/npm-install/
- 3: [RRFC] all commands (without context) should respect
--ignore-scriptsnpm/rfcs#709
Pin the documented runtime package.
npm install --global ecc-universal installs the package under the implicit ecc-universal@latest version, and global installs execute package lifecycle scripts on the host unless scripts are ignored. A compromised or incompatible release can replace the runtime that selects and enforces routes. Document the tested version and install from the locked dependency set used by this package manifest, or include --ignore-scripts for runtime-only deps.
🤖 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 `@docs/sandbox-testing.md` around lines 48 - 51, Update the runtime
installation guidance in the sandbox-testing documentation to pin the tested
ecc-universal version from the package manifest or locked dependency set, and
include --ignore-scripts when installing runtime-only dependencies. Replace the
unversioned global install command while preserving the warning against running
repository-local ecc-sandbox binaries.
|
Thanks for the executable reproduction — this was a real cleanup-ordering bug. Fixed in |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/sandbox/vm.test.js`:
- Around line 511-512: Update the exitOnly setup around manifest() to create a
new object containing the manifest fields and report value instead of assigning
to the object returned by manifest(). Preserve the existing manifest data while
avoiding in-place mutation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a11f3f36-fea6-4af8-a490-a50b72c48df1
📒 Files selected for processing (4)
docs/design/sandbox-testing/CONVENTIONS.mddocs/design/sandbox-testing/PROGRESS.mdscripts/sandbox/backends/vm.jstests/sandbox/vm.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (28)
- GitHub Check: Greptile Review
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (macos-latest, Node 18.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (macos-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (macos-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (macos-latest, Node 22.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/sandbox/vm.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/vm.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/sandbox/backends/vm.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/sandbox/backends/vm.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/sandbox/backends/vm.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Delegate complex, domain-specific, architectural, security, testing, review, build, and workflow tasks to the appropriate specialized agent; use parallel execution for independent operations.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: For new features and bug fixes, follow mandatory TDD: write a failing test first, implement minimally, then refactor and verify at least 80% coverage.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Never compromise security; validate all inputs and address critical security issues before proceeding.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Always create new objects and never mutate existing ones.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Plan complex features and refactoring before implementation, including dependencies, risks, and phases.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Before any commit, prevent hardcoded secrets, validate inputs, use parameterized SQL, sanitize HTML, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive data in errors.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and search for similar issues.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Organize code into many small, cohesive files by feature or domain rather than type; target 200–400 lines and allow at most 800 lines per file.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Handle errors at every level, provide user-friendly UI messages, log detailed server-side context, and never silently swallow errors.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Validate all external and user input at system boundaries with schema-based validation; fail fast with clear messages and never trust external data.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, handle errors properly, avoid hardcoded values, and use readable, well-named identifiers.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Provide unit tests for functions, utilities, and components; integration tests for APIs and database operations; and end-to-end tests for critical user flows.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: When troubleshooting test failures, check test isolation, verify mocks, and fix the implementation rather than tests unless the tests are incorrect.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Capture personal debugging notes and temporary context in auto memory, team knowledge in the existing project documentation structure, avoid duplicating code comments or generated docs, and ask before creating a new top-level file when no documentation location exists.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Use Conventional Commits with the format `<type>: <description>` and one of: feat, fix, refactor, docs, test, chore, perf, or ci.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: For pull requests, analyze the full commit history, provide a comprehensive summary and test plan, and push with the `-u` flag.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: API responses must use a consistent envelope containing a success indicator, data payload, error message, and pagination metadata.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Encapsulate data access behind a repository interface with `findAll`, `findById`, `create`, `update`, and `delete`; business logic must depend on the abstraction rather than the storage mechanism.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: For skeleton projects, find battle-tested templates, evaluate them in parallel for security, extensibility, and relevance, clone the best match, and iterate within its proven structure.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Avoid using the final 20% of the context window for large refactoring and multi-file features; higher utilization is acceptable for low-sensitivity single edits, documentation, and simple fixes.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: For build failures, use the appropriate build-error-resolver agent, analyze errors, fix incrementally, and verify after each fix.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:10.636Z
Learning: Success requires all tests to pass with at least 80% coverage, no security vulnerabilities, readable maintainable code, acceptable performance, and fulfilled user requirements.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:23.651Z
Learning: Güvenlik sorunu bulunursa durun; security-reviewer agentını kullanın, kritik sorunları düzeltin, ifşa edilen sırları döndürün ve benzer sorunlar için kod tabanını inceleyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:23.651Z
Learning: Bağımsız görevlerde agentları paralel çalıştırın; build sorunlarında build-error-resolver, C++/Go/Kotlin/Rust/Java/PyTorch sorunlarında ilgili resolver agentlarını kullanın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-10T07:07:23.651Z
Learning: Takım/proje bilgilerini mevcut dokümantasyon yapısında, kişisel veya geçici notları otomatik bellekte tutun; aynı bilgiyi birden fazla yerde çoğaltmayın.
| const exitOnly = manifest(); | ||
| exitOnly.report = 'exit-only'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid in-place manifest mutation.
exitOnly.report = 'exit-only' mutates the object returned by manifest(). Build exitOnly as a new object to preserve the immutable manifest contract.
Proposed fix
- const exitOnly = manifest();
- exitOnly.report = 'exit-only';
+ const exitOnly = { ...manifest(), report: 'exit-only' };As per coding guidelines: "Always create new objects, never mutate existing ones."
🤖 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 `@tests/sandbox/vm.test.js` around lines 511 - 512, Update the exitOnly setup
around manifest() to create a new object containing the manifest fields and
report value instead of assigning to the object returned by manifest(). Preserve
the existing manifest data while avoiding in-place mutation.
Sources: Coding guidelines, Learnings
|
Thanks—fixed in |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
| } | ||
|
|
||
| const execute = (command, assertion) => { | ||
| const remainingSeconds = Math.max(1, Math.ceil((deadline - clock()) / 1000)); |
There was a problem hiding this comment.
When guest readiness succeeds exactly as the manifest timeout expires, setup and assertion commands are still dispatched to the VM. Math.max(1, Math.ceil((deadline - clock()) / 1000)) converts a zero or negative remaining budget into a new one-second allowance. The focused Lume harness observed both commands sent with --timeout 1 after the clock reached the 1000 ms deadline. Check the remaining milliseconds before invoking driver.execArgs; if the deadline has elapsed, mark the run as an execution error and skip command dispatch.
Artifacts
Focused VM deadline hypothesis script
- Authored Node harness that mocks Lume readiness at the manifest deadline and records VM adapter calls, proving whether host and post-deadline execution boundaries hold.
Readiness failure at the deadline
- Executed `CASE=before` harness output shows a failed readiness probe at 1000 ms sends no manifest commands and creates no host marker.
Readiness success at the deadline still dispatches guest commands
- Executed `CASE=after` harness output shows two guest commands dispatched with one-second timeouts after readiness consumes the full deadline, while the host marker remains absent.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/sandbox/vm.test.js`:
- Around line 545-548: Update tests/sandbox/vm.test.js:545-548 in the
stop-failure test to assert that the second get command targets the cloned
guest, and update tests/sandbox/vm.test.js:581-586 in the already-stopped test
to assert that the second get command targets ecc-lume-already-stopped-test.
Keep the existing response behavior while ensuring cleanup status queries use
the expected guest name rather than accepting any target.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0bffc507-a393-4743-ac20-c13c8ea42ef1
📒 Files selected for processing (3)
scripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.jstests/sandbox/vm.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (24)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 22.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Coverage
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/sandbox/vm.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Delegate complex, domain-specific, architectural, security-sensitive, and review tasks to the appropriate specialized agent; run independent agents in parallel where practical.
Maintain at least 80% test coverage and include unit, integration, and end-to-end tests, including critical user flows.
Never hardcode secrets such as API keys, passwords, or tokens; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets.
Validate all external and user-provided input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Enable CSRF protection, verify authentication and authorization, apply rate limiting to all endpoints, and ensure errors do not leak sensitive data.
If a security issue is found, stop, invoke the security-reviewer agent, fix critical issues, rotate exposed secrets, and check for similar issues elsewhere.
Prefer many small, focused files organized by feature or domain, with high cohesion and low coupling; typical files should be 200–400 lines and never exceed 800 lines.
Keep functions under 50 lines, avoid nesting deeper than four levels, use readable well-named identifiers, avoid hardcoded values, and keep files focused.
Handle errors at every level, provide user-friendly UI messages, log detailed server-side context, and never silently swallow errors.
Encapsulate data access behind a repository interface with findAll, findById, create, update, and delete operations; business logic must depend on the abstraction, not the storage mechanism.
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{sql,js,jsx,ts,tsx,py,java,kt,go,rs,php}
📄 CodeRabbit inference engine (AGENTS.md)
Prevent SQL injection by using parameterized queries rather than interpolating untrusted values.
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{html,htm,js,jsx,ts,tsx,vue,svelte,php}
📄 CodeRabbit inference engine (AGENTS.md)
Prevent XSS by sanitizing untrusted HTML before rendering or inserting it into the DOM.
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
**/*.{js,jsx,ts,tsx,py,java,kt,go,rs,cs,cpp,h,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx,py,java,kt,go,rs,cs,cpp,h,hpp}: Preserve immutability: always create new objects and never mutate existing ones; return new copies with changes applied.
Use consistent API response envelopes containing a success indicator, data payload, error message, and pagination metadata where applicable.
Files:
tests/sandbox/vm.test.jsscripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/sandbox/backends/lume.jsscripts/sandbox/backends/vm.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: For complex features, plan before implementation; identify dependencies and risks and break work into phases.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: Use test-driven development for new features and bug fixes: write a failing test first, implement the minimum passing solution, then refactor.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: Use Conventional Commits with the format <type>: <description>, where type is one of feat, fix, refactor, docs, test, chore, perf, or ci.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: For pull requests, analyze the full commit history, draft a comprehensive summary, include a test plan, and push with the -u flag.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: Capture personal debugging notes and temporary context in auto memory; record team/project knowledge in the existing documentation structure, avoid duplicate documentation, and ask before creating a new top-level file when no location is obvious.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: For skeleton projects, find battle-tested templates, evaluate candidates in parallel for security, extensibility, and relevance, then clone and iterate on the best match.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: Avoid using the final 20% of the context window for large refactors and multi-file features; higher utilization is acceptable for low-sensitivity single edits, documentation, and simple fixes.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:45:48.875Z
Learning: Success requires all tests to pass with at least 80% coverage, no security vulnerabilities, readable maintainable code, acceptable performance, and fulfilled user requirements.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Karmaşık özellikler, yeniden düzenlemeler, mimari kararlar, güvenlik açısından hassas işler ve ilgili uzmanlık gerektiren görevler için uygun agentları proaktif olarak kullanın; bağımsız işleri paralel yürütün.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Uygulamadan önce test yazın ve en az %80 kapsama sağlayın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Her zaman yeni nesneler oluşturun; mevcut nesneleri değiştirmeyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Karmaşık özellikleri kodlamadan önce planlayın; bağımlılıkları ve riskleri belirleyip işi aşamalara bölün.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Commit öncesinde sabit kodlanmış sırları, doğrulanmamış girdileri, SQL injection, XSS, CSRF, kimlik doğrulama/yetkilendirme, hız sınırlama ve hassas hata mesajı sorunlarını kontrol edin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Sırları asla kaynak koda sabit kodlamayın; ortam değişkenleri veya sır yöneticisi kullanın ve ifşa edilen sırları hemen döndürün.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Güvenlik sorunu bulunursa çalışmayı durdurun, security-reviewer agentını kullanın, kritik sorunları düzeltin, ifşa edilen sırları döndürün ve benzer sorunlar için kod tabanını inceleyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Dosyaları küçük ve odaklı tutun; tipik olarak 200–400, en fazla 800 satır kullanın. Kodları tipe göre değil özelliğe veya alana göre düzenleyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Hataları her seviyede ele alın; UI'da kullanıcı dostu mesajlar sağlayın, sunucuda ayrıntılı bağlamı loglayın ve hataları sessizce yutmayın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Sistem sınırlarında tüm kullanıcı girdilerini şema tabanlı doğrulayın; net mesajlarla hızlı başarısız olun ve harici verilere güvenmeyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Fonksiyonları 50 satırdan kısa, dosyaları 800 satırdan kısa tutun; dört seviyeden fazla iç içe geçmeden kaçının, sabit kodlanmış değerleri azaltın ve okunabilir adlandırmalar kullanın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: TDD akışını uygulayın: önce başarısız test yazın (KIRMIZI), minimal uygulamayla başarılı hale getirin (YEŞİL), ardından yeniden düzenleyip %80+ kapsamayı doğrulayın (İYİLEŞTİR).
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Test başarısızlıklarında önce test izolasyonunu ve mockları kontrol edin; testler yanlış olmadıkça uygulamayı düzeltin, testleri değiştirmeyin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Yeni veya değiştirilmiş koddan sonra code-reviewer agentını kullanın ve kritik/yüksek önem taşıyan sorunları giderin.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Kişisel/geçici bağlamı otomatik bellekte, takım ve proje bilgisini mevcut proje dokümantasyonunda tutun; aynı bilgiyi farklı yerlerde çoğaltmayın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Commit mesajlarını `<type>: <description>` biçiminde Conventional Commits formatında yazın; geçerli tipler feat, fix, refactor, docs, test, chore, perf ve ci'dır.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: PR hazırlarken tam commit geçmişini analiz edin, kapsamlı özet ve test planı ekleyin; push işlemlerinde `-u` bayrağını kullanın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: API yanıtlarını başarı göstergesi, veri yükü, hata mesajı ve sayfalandırma metadatası içeren tutarlı bir zarf biçiminde döndürün.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Veri erişimini `findAll`, `findById`, `create`, `update` ve `delete` işlemlerini sağlayan standart bir Repository arayüzü arkasında kapsülleyin; iş mantığını depolama mekanizmasından ayırın.
Learnt from: CR
Repo: affaan-m/ECC
Timestamp: 2026-08-11T17:46:08.583Z
Learning: Büyük yeniden düzenlemeler ve çok dosyalı özelliklerde bağlam penceresinin son %20'sinden kaçının.
🔇 Additional comments (3)
tests/sandbox/vm.test.js (1)
574-575: Avoid in-place manifest mutation.Line 575 mutates the object returned by
manifest(). This is the same unresolved issue reported previously.As per coding guidelines: "Always create new objects, never mutate existing ones."
Source: Coding guidelines
scripts/sandbox/backends/lume.js (1)
10-24: LGTM!Also applies to: 42-48, 367-368, 381-386
scripts/sandbox/backends/vm.js (1)
432-440: LGTM!
| if (argv[0] === 'get') { | ||
| getCount += 1; | ||
| return result(0, getCount === 1 ? lumeSeedJson : '[{"status":"running"}]'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the target guest for each cleanup status query.
Both mock runners return a get response for any guest name. A regression that queries the seed instead of vmName during cleanup would still pass.
tests/sandbox/vm.test.js#L545-L548: Assert that the secondgetcommand targets the cloned guest in the stop-failure test.tests/sandbox/vm.test.js#L581-L586: Assert that the secondgetcommand targetsecc-lume-already-stopped-testin the already-stopped test.
📍 Affects 1 file
tests/sandbox/vm.test.js#L545-L548(this comment)tests/sandbox/vm.test.js#L581-L586
🤖 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 `@tests/sandbox/vm.test.js` around lines 545 - 548, Update
tests/sandbox/vm.test.js:545-548 in the stop-failure test to assert that the
second get command targets the cloned guest, and update
tests/sandbox/vm.test.js:581-586 in the already-stopped test to assert that the
second get command targets ecc-lume-already-stopped-test. Keep the existing
response behavior while ensuring cleanup status queries use the expected guest
name rather than accepting any target.
| } | ||
|
|
||
| const execute = (command, assertion) => { | ||
| const remainingSeconds = Math.max(1, Math.ceil((deadline - clock()) / 1000)); |
There was a problem hiding this comment.
- Bug
- An injected-clock Node harness ran executeVm with a one-second manifest timeout and made guest readiness succeed exactly at the deadline. The harness observed both setup and assertion commands dispatched afterward with execArgs..., 1 and one-millisecond runner timeouts, while the run reported success. This confirms that the timeout calculation grants expired work a new execution window instead of stopping command dispatch.
- Cause
- T-Rex reproduced this while running the changed behavior, but it did not return a separate root-cause sentence.
- Fix
- Update the changed code so this failing path is handled, then rerun the same T-Rex check to confirm it passes.
Artifacts
Runtime repro source for exact-deadline VM command dispatch
- The authored untracked Node.js harness sets readiness to succeed at the exact deadline and asserts the setup/assert dispatch and timeout values, demonstrating the exercised scenario and the takeaway is that expired-deadline commands are still invoked.
Observed exact-deadline VM command dispatch output
- The captured command output shows both setup and assert commands were dispatched with one-second `execArgs` values and one-millisecond runner timeouts after readiness reached the deadline, confirming the finding and the takeaway is that the deadline does not prevent command dispatch.
|
Superseded by the reviewable three-PR sandbox stack:
Each PR has its own focused workflow and base, while preserving the required merge order. The replacement stack is rebased on current |
2 participants
Summary
The product boundary is intentionally infrastructure-neutral: agents declare needs and ECC selects the cheapest truthful venue. Docker remains a detected fallback only and is never an installation requirement.
Safety boundaries
trust: first-partyplus explicitnetwork:*srt.cmd; mock mode launches no processEvidence
.pkgand launchd lifecycle passed with scan evidence, clone deletion, seed shutdown, and no attributable helper leakPost-main-sync manual acceptance
mainat446f02b0; GitHub reports the PR mergeable, with refreshed CI in progressecc-universalruntimee05c7fa2)origin/maintest codeReview focus
The highest-value review areas are the fail-closed routing table, Windows command boundary, VM ownership and cleanup, CI artifact identity binding, and the single mutable escalation budget.