Repo optimization: lifecycle & security fixes, release readiness, roadmap wave 1 - #18
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
…x + pack smoke test - Rename package to @liohtml/agentbox (bare 'agentbox' on npm is a security placeholder and cannot be claimed); bin command stays 'agentbox'. - Move tsx to dependencies so the published package runs via npx/global install without dev setup; add prepublishOnly guard + publishConfig (public access, provenance). - CI: Node 22+24 matrix, npm pack smoke test that installs the tarball and runs the CLI as an end user; narrow push triggers to main. - Add tag-triggered release workflow (npm publish --provenance). - Add SECURITY.md (threat model + private reporting), CODE_OF_CONDUCT.md, PR template, issue-template config, dependabot config, CHANGELOG.md. - Replace one-time PUBLISH.md (contained personal email) with generic RELEASING.md; README: badges + corrected npx instructions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
…launcher hardening Consolidated fixes from a code audit + upstream research: - Run the container detached with the entry script as PID 1 and attach via 'docker exec tmux attach'. Previously the tmux attach client was PID 1, so Ctrl+b d (detach) killed the lead, all teammates and the Observer, and --rm deleted the container — the opposite of the documented behavior. - Launch the lead with --teammate-mode tmux: since Claude Code v2.1.179 the default teammateMode is 'in-process', which silently disabled the split-pane team layout agentbox is built around. - Fail fast with clear errors on invalid --port/--memory/--cpus, unknown flags, stray positionals, ':' in mount paths, and a nonexistent explicitly requested workspace (no more silently created empty mounts). - rebuild refuses while the box is running (used to rebuild then silently reattach to the old container); reattach warns when tuning flags are ignored; stale stopped containers are removed before docker run. - down surfaces real 'docker rm -f' failures instead of claiming success. - ensureHostMounts: clear error when ~/.claude.json is a directory; follow symlinked configs (dotfile managers); benign EEXIST race handling. - Entry script moves to a per-user 0700 temp subdir (was a fixed world-shared /tmp path: tamper/TOCTOU + collision); never write through a pre-existing symlink. - Observer: HTML-escape project/session names (stored XSS), handle EADDRINUSE with a readable message, remain-on-exit for the observer tmux window, port sanity fallback, drop dead tail() code. - Launcher: invoke local tsx via node directly (no shell) — survives paths with spaces, removes cmd.exe metacharacter injection on Windows. - doctor: warn on host uid != 1000 (bind-mount write failures). - Dockerfile: CLAUDE_CODE_VERSION/TSX_VERSION build args for reproducible builds. - Contract & README updated; tests: 45 → 65 (new cli router suite, config validation, observer + session assertions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
…rwarding, --gpus, arch check - --yolo: launch the lead with --dangerously-skip-permissions (teammates inherit it). Strictly opt-in with a visible warning; documented under Security & trust. - Persistent tool-cache named volumes (agentbox-npm-cache, agentbox-cargo-registry, agentbox-pip-cache): boxes stay --rm-disposable, package downloads survive. Mount points pre-created agent-owned in the image so first use isn't root-owned. - Host identity forwarding: ~/.gitconfig and ~/.config/gh bind-mounted READ-ONLY when present; gh CLI installed in the image. Same philosophy as Claude auth: borrow host identity, never store it. - --gpus passthrough to docker run. - doctor: image-vs-host architecture check (warns about qemu-emulated images on e.g. Apple Silicon). - docker run argv construction extracted as pure buildRunArgs() and covered by a new Docker-free test suite (the audit's top missing-coverage item). - Docs: README flags/mounts/security, CONTRACT mounts + cache volumes, ROADMAP wave 1 marked done, CHANGELOG. Tests: 65 → 76, all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe PR hardens Agentbox configuration, Docker startup, tmux session management, Observer rendering, and CLI validation. It also adds package publishing metadata, CI and release workflows, dependency automation, security documentation, repository templates, and project documentation. ChangesAgentbox runtime
Repository delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant User
participant AgentboxCLI
participant Docker
participant Tmux
User->>AgentboxCLI: Run agentbox up with validated options
AgentboxCLI->>Docker: Start detached container
Docker->>Tmux: Create or reuse session
AgentboxCLI->>Tmux: Poll for session readiness
AgentboxCLI->>Tmux: Attach client to session
User->>Tmux: Detach client
Tmux-->>Docker: Keep PID 1 alive while the session exists
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 @.github/workflows/ci.yml:
- Around line 24-25: Disable persisted checkout credentials for every checkout
step: add persist-credentials: false to .github/workflows/ci.yml lines 24-25 and
46-47, and .github/workflows/release.yml lines 15 and 32.
In @.github/workflows/release.yml:
- Around line 11-22: The release gate must validate the packed artifact before
publication. In .github/workflows/release.yml lines 11-22, add the existing
clean-install npm pack smoke-test step after the source checks and before the
publish job or step; in RELEASING.md lines 25-26, retain the “all checks”
statement only after the release workflow’s smoke test is included.
- Around line 3-5: Update the release workflow’s tag-triggered publishing flow
to compare the pushed tag with v${package.json.version} before publishing. Add a
pre-publish assertion that fails on stale, mistyped, or otherwise mismatched
tags, while preserving the existing v* trigger.
- Around line 39-41: Align the release authentication strategy across
.github/workflows/release.yml (lines 39-41) and RELEASING.md (lines 34-35):
configure the npm publish step to use Trusted Publishing exclusively and update
the documentation accordingly, or remove/clarify the Trusted Publishing
documentation as unsupported while retaining NODE_AUTH_TOKEN-based publishing.
- Around line 16-20: Disable npm caching in both setup-node steps at
.github/workflows/release.yml lines 16-20 and 33-38. Remove the cache
configuration entirely, or configure caching without restore/write, while
preserving lockfile-based npm ci installation and the existing typecheck/test
and publish flows.
In `@assets/Dockerfile`:
- Around line 48-50: Update the CLAUDE_CODE_VERSION and TSX_VERSION
build-argument defaults in the Dockerfile to reviewed exact immutable versions
instead of latest, while preserving the arguments so upgrades remain intentional
overrides for the npm install command.
In `@bin/agentbox.mjs`:
- Around line 28-31: Remove the Windows shell-based global fallback from the
spawn logic in bin/agentbox.mjs, so process.argv.slice(2) is never passed with
shell: true or routed through cmd.exe. Use the local tsx runtime dependency
directly and fail clearly when it is unavailable, while preserving inherited
stdio and environment behavior.
In `@CHANGELOG.md`:
- Around line 61-64: Update the “Git identity + gh auth forwarding” changelog
entry to replace “never entering the image” with wording that clarifies
credentials are not baked into the image, while explicitly stating that
runtime-mounted credentials may remain readable inside the running container.
In `@CODE_OF_CONDUCT.md`:
- Around line 30-35: Update the “Enforcement” section of CODE_OF_CONDUCT.md to
name a maintainer contact or dedicated private channel for sensitive conduct
complaints, while retaining the GitHub issue option for non-sensitive matters.
Reserve the GitHub Security Advisories link specifically for security
vulnerability reports.
In `@src/cli/index.ts`:
- Around line 106-117: The reattach warning in the `up`/`rebuild` flow uses
invocation-time `config.yolo` instead of the existing container’s state. Persist
the startup yolo mode as a container label or environment marker when creating
the container, inspect that marker before reattaching, and base the warning on
the persisted state so both `up --yolo` and reattaching without `--yolo` report
accurately; update the related logic around `tuningFlagsGiven` as well.
- Around line 85-104: Restrict the startup validation block around
validateConfig to only the up and rebuild commands, so down remains usable with
invalid runtime settings. Update cmdDoctor to call validateConfig explicitly and
report every returned problem while retaining its checklist behavior. Keep the
explicit workspace existence check within the up/rebuild validation flow.
In `@src/cli/parser.ts`:
- Line 11: Update the argument parsing flow around VALUE_FLAGS to reject any
value-consuming flag, including --gpus, when it has no following value before
resolveConfig runs. Ensure bare flags produce the existing invalid-argument
behavior rather than being recorded as true and converted to undefined, while
preserving valid values for all flags in VALUE_FLAGS.
In `@src/docker/index.ts`:
- Around line 281-291: Update the symbolic-link handling block to inspect the
followed target with statSync and require isFile() before returning. If the
target exists but is not a regular file, reject it using the same corrective
guidance already used for invalid config targets; retain dangling-link creation
through fs.writeFileSync with flag "w".
- Around line 124-127: The Docker run configuration in src/docker/index.ts lines
124-127 must stop using automatic container removal so failed startup containers
and their logs remain available. Update the polling logic around the CLI startup
flow in src/cli/index.ts lines 214-222 to detect an early container exit, report
diagnostics from the retained container, and avoid waiting for the full timeout;
apply these changes using the relevant Docker argument construction and
container startup/polling symbols.
In `@src/session/index.ts`:
- Around line 136-147: The writeEntryScript path must not rely on a pre-existing
predictable temporary directory. Update the directory setup around mkdirSync so
it atomically creates a unique directory, or validates an existing directory is
owned by the current UID and has no group/world permissions before writing
entryScriptPath.
In `@src/types.ts`:
- Around line 75-82: Update the global flag surface in CONTRACT.md to include
the AgentboxConfig options yolo (--yolo) and gpus (--gpus), matching their
existing types and semantics. Keep the prose and machine-readable contract
entries consistent with each other and with the declarations in AgentboxConfig.
In `@tests/docker.test.ts`:
- Around line 59-65: Extend the “mounts workspace, claude dir/json, entry script
and observer” test to assert that the observer asset bind mount is included in
the mounts returned by buildRunArgs. Use the expected observerAsset source and
its corresponding container destination, preserving the existing assertions.
In `@tests/observer.test.ts`:
- Around line 60-62: Strengthen the test in “falls back to the default port when
OBSERVER_PORT is not a valid port” to verify the complete contract: integer
validation, inclusive 1–65535 bounds, and fallback to port 4317. Either match
the full validation expression in src or add cases covering invalid, zero, and
out-of-range values.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 775f86f3-e039-4799-a1c8-57ca7b43ef3c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.github/ISSUE_TEMPLATE/config.yml.github/PULL_REQUEST_TEMPLATE.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRACT.mdPUBLISH.mdREADME.mdRELEASING.mdROADMAP.mdSECURITY.mdassets/Dockerfileassets/observer.mjsbin/agentbox.mjspackage.jsonsrc/cli/config.tssrc/cli/help.tssrc/cli/index.tssrc/cli/parser.tssrc/docker/index.tssrc/session/index.tssrc/types.tstests/cli.test.tstests/config.test.tstests/docker.test.tstests/observer.test.tstests/session.test.ts
💤 Files with no reviewable changes (1)
- PUBLISH.md
…r lifecycle, remaining edge cases Release workflow: - persist-credentials: false on every checkout (ci.yml, release.yml). - release.yml: verify the pushed tag matches package.json's version before publishing; run the npm-pack tarball smoke test as a gate before publish; drop dependency caching (a poisoned cache must never influence what gets published — CI keeps caching, only the sensitive publish workflow doesn't). - RELEASING.md: stop implying npm Trusted Publishing is configured — the workflow authenticates with NODE_AUTH_TOKEN only. - Dockerfile: pin CLAUDE_CODE_VERSION/TSX_VERSION defaults to reviewed exact versions instead of `latest`, so a plain rebuild is reproducible. - Launcher: drop the shell-based global-tsx-on-PATH fallback — it forwarded raw argv through a shell (cmd.exe metacharacter risk on Windows); tsx is a runtime dependency now, so a missing local install is a clear error instead. Container lifecycle: - Stop using --rm: a container that exits early (entry-script crash) now stays around for `docker logs` to inspect instead of vanishing instantly. `up` removes a stale stopped container before starting fresh (already implemented); `down` still removes it explicitly; waitForSession also gives up early if the container itself exits, instead of polling the full timeout. - Reattaching now reports the RUNNING container's actual --yolo state via a container label, instead of echoing the current invocation's flag (which is ignored on reattach and could claim the wrong thing in either direction). Input handling: - A value flag given with no value (e.g. trailing --gpus) is now a hard error instead of silently doing nothing. - validateConfig (+ workspace-must-exist) now gates only up/rebuild — down/ shell stay usable with a currently-invalid MEM/CPUS/workspace. doctor reports every validateConfig problem explicitly in its checklist instead of a narrower, separate ad hoc port check. Other fixes: - ~/.claude.json symlinked to a directory is now a clear error (previously it passed through silently and Docker mounted a directory). - The entry-script temp directory's ownership/permissions are verified after creation (mkdirSync's mode arg is a no-op on a pre-existing dir), closing a race where another local user could pre-create it. - CODE_OF_CONDUCT: separate conduct-report contact from the security channel. - Doc wording fixes (CHANGELOG credential-mount phrasing; all --rm mentions updated to match the new lifecycle). Tests: 76 → 87, all green; typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
…ded path CI's npm-pack smoke test failed: when the tarball is installed as a dependency in another project, npm hoists tsx to the CONSUMER's top-level node_modules instead of nesting it under agentbox's own node_modules, so the previous `<here>/../node_modules/tsx/dist/cli.mjs` path didn't exist. Fix: use createRequire().resolve() to locate tsx the way Node itself would (walking up node_modules, handling hoisting correctly in every install shape — repo clone, hoisted consumer install, or nested global install). Resolves "tsx/package.json" rather than "tsx/dist/cli.mjs" directly: tsx's package.json declares an `exports` map that only allows a few subpaths, and "./dist/cli.mjs" isn't one of them, so resolving it by specifier would throw ERR_PACKAGE_PATH_NOT_EXPORTED even though the file exists on disk. Deriving the cli.mjs path from the resolved package.json's directory sidesteps that: it's a plain filesystem join, not a specifier resolution. Verified locally by reproducing the CI job exactly: npm pack, install the tarball into a scratch consumer project, run npx agentbox --help/doctor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/docker/index.ts (1)
269-317: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat only
ENOENTas a missing config target.Both
lstatSyncand the followedstatSynccatch every filesystem error.EACCES,ELOOP, and similar failures are then treated as missing paths and trigger a write attempt, masking the original failure. Re-throw errors other thanENOENTin both catches.Proposed fix
- } catch { + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err; ... - } catch { + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/docker/index.ts` around lines 269 - 317, Update the lstatSync and followed statSync catch blocks in the Claude config setup to treat only ENOENT as a missing path. Re-throw any other filesystem error, including EACCES or ELOOP, before proceeding with directory creation or symlink-target writing; preserve the existing missing-target behavior.
🧹 Nitpick comments (2)
tests/observer.test.ts (1)
60-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood improvement over the prior loose regex check;
new Function()flag here is a false positive.The OpenGrep hint about dynamic code execution doesn't apply meaningfully — the evaluated code is extracted from the project's own trusted source file, not external input. That said, the two-line regex extraction is a bit fragile: any reformatting of the
RAW_PORT/PORTlines inobserver.mjswould break the match rather than the intended bounds logic. Exporting the port-resolution as a small pure function reusable by both the runtime and this test would remove the eval/regex coupling entirely.🤖 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/observer.test.ts` around lines 60 - 73, Replace the regex extraction and new Function usage in resolvePort with a shared pure port-resolution function exported from observer.mjs. Update the runtime to use this function and have the test import and invoke it directly, preserving the existing integer, range, and 4317 fallback behavior.Source: Linters/SAST tools
src/session/index.ts (1)
160-167: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNarrow the catch so a failed
rmSyncisn't silently treated as "doesn't exist".If
lstatSyncsucceeds (symlink found) butrmSyncthen throws for a real reason (e.g. permission/immutable flag), the sharedcatchswallows it andwriteFileSyncproceeds to write through the still-present symlink — defeating the intent of this guard. Split the lookup from the removal so only the lstat's ENOENT is treated as "fine".♻️ Proposed fix
- try { - if (lstatSync(config.entryScriptPath).isSymbolicLink()) { - rmSync(config.entryScriptPath); - } - } catch { - // does not exist — fine - } + let existing; + try { + existing = lstatSync(config.entryScriptPath); + } catch { + existing = null; // does not exist — fine + } + if (existing?.isSymbolicLink()) { + rmSync(config.entryScriptPath); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/index.ts` around lines 160 - 167, Update the symlink guard around config.entryScriptPath so lstatSync handles only an ENOENT as the missing-path case, while rmSync runs outside that catch and propagates removal errors. Preserve the existing behavior of removing an existing symlink before writeFileSync proceeds.
🤖 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 `@src/types.ts`:
- Around line 177-183: Preserve an explicit unavailable state for container
labels instead of treating Docker inspect failures as empty labels. In
src/types.ts lines 177-183, update containerLabels to return the chosen
unavailable or tagged result; in src/docker/index.ts lines 355-369, return that
state for inspect and JSON failures, and update reattach handling to report
“startup mode unknown” rather than inferring non-YOLO mode from a missing
LABEL_YOLO.
In `@tests/session.test.ts`:
- Around line 158-176: Update both writeEntryScript tests to wrap their setup,
assertions, and invocation in try/finally blocks, keeping the existing directory
cleanup in finally so it always runs when writeEntryScript or an assertion
fails. Preserve each test’s current assertions and behavior, including the
world-writable directory rejection.
---
Outside diff comments:
In `@src/docker/index.ts`:
- Around line 269-317: Update the lstatSync and followed statSync catch blocks
in the Claude config setup to treat only ENOENT as a missing path. Re-throw any
other filesystem error, including EACCES or ELOOP, before proceeding with
directory creation or symlink-target writing; preserve the existing
missing-target behavior.
---
Nitpick comments:
In `@src/session/index.ts`:
- Around line 160-167: Update the symlink guard around config.entryScriptPath so
lstatSync handles only an ENOENT as the missing-path case, while rmSync runs
outside that catch and propagates removal errors. Preserve the existing behavior
of removing an existing symlink before writeFileSync proceeds.
In `@tests/observer.test.ts`:
- Around line 60-73: Replace the regex extraction and new Function usage in
resolvePort with a shared pure port-resolution function exported from
observer.mjs. Update the runtime to use this function and have the test import
and invoke it directly, preserving the existing integer, range, and 4317
fallback behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a70fa907-b695-48c1-8efc-9e9ce942a1a6
📒 Files selected for processing (21)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRACT.mdREADME.mdRELEASING.mdROADMAP.mdSECURITY.mdassets/Dockerfilebin/agentbox.mjssrc/cli/index.tssrc/cli/parser.tssrc/docker/index.tssrc/session/index.tssrc/types.tstests/cli.test.tstests/docker.test.tstests/observer.test.tstests/parser.test.tstests/session.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ROADMAP.md
- README.md
- CONTRACT.md
Comment on lines
+177
to
+183
| /** | ||
| * Labels of a container (e.g. LABEL_YOLO), or {} if the container is | ||
| * missing / docker is unavailable. Used on reattach to report the | ||
| * RUNNING container's actual startup mode instead of the current | ||
| * invocation's flags, which are ignored on reattach. | ||
| */ | ||
| containerLabels(name: string): Promise<Record<string, string>>; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve an explicit unknown state for container labels.
The interface and implementation collapse Docker inspect failures into {}, while src/cli/index.ts interprets a missing LABEL_YOLO as false. This can tell users that prompts are active when the running box was actually started with --yolo.
src/types.ts#L177-L183: changecontainerLabelsto return an explicit unavailable state or tagged result.src/docker/index.ts#L355-L369: return that state for inspect/JSON failures instead of{}, and make reattach output report “startup mode unknown”.
📍 Affects 2 files
src/types.ts#L177-L183(this comment)src/docker/index.ts#L355-L369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types.ts` around lines 177 - 183, Preserve an explicit unavailable state
for container labels instead of treating Docker inspect failures as empty
labels. In src/types.ts lines 177-183, update containerLabels to return the
chosen unavailable or tagged result; in src/docker/index.ts lines 355-369,
return that state for inspect and JSON failures, and update reattach handling to
report “startup mode unknown” rather than inferring non-YOLO mode from a missing
LABEL_YOLO.
Comment on lines
+158
to
+176
| posixOnly("writeEntryScript succeeds when it creates the temp dir itself", () => { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "agentbox-test-")); | ||
| fs.rmSync(dir, { recursive: true, force: true }); // let writeEntryScript create it fresh | ||
| const scriptPath = path.join(dir, "entry.sh"); | ||
| const written = session.writeEntryScript(baseConfig({ entryScriptPath: scriptPath }), TTY); | ||
| assert.equal(written, scriptPath); | ||
| assert.ok(fs.existsSync(scriptPath)); | ||
| fs.rmSync(dir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| posixOnly("writeEntryScript refuses a pre-existing world-writable temp dir", () => { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "agentbox-test-")); | ||
| fs.chmodSync(dir, 0o777); // simulate another local user's permissive pre-created dir | ||
| const scriptPath = path.join(dir, "entry.sh"); | ||
| assert.throws( | ||
| () => session.writeEntryScript(baseConfig({ entryScriptPath: scriptPath }), TTY), | ||
| /refusing to write the entry script/, | ||
| ); | ||
| fs.rmSync(dir, { recursive: true, force: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up temporary directories in finally blocks.
If writeEntryScript or an assertion fails, fs.rmSync is skipped, leaving test directories behind. Wrap both test bodies in try/finally so repeated or watch-mode runs do not leak directories.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 169-169: Do not give 777 permissions to a file
Context: fs.chmodSync(dir, 0o777)
Note: [CWE-732] Incorrect Permission Assignment for Critical Resource.
(chmod-permissions-typescript)
🤖 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/session.test.ts` around lines 158 - 176, Update both writeEntryScript
tests to wrap their setup, assertions, and invocation in try/finally blocks,
keeping the existing directory cleanup in finally so it always runs when
writeEntryScript or an assertion fails. Preserve each test’s current assertions
and behavior, including the world-writable directory rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Maintenance batch from a full repo audit (code audit + OSS-maturity audit + ecosystem research), in three commits:
1. Critical fixes
docker exec. Previously the tmux attach client was PID 1, soCtrl+b dkilled the lead, all teammates and the Observer, and--rmdeleted the container — the opposite of the documented behavior.--teammate-mode tmux; since Claude Code v2.1.179 the upstream default isin-process, which silently disabled agentbox's signature split-pane layout (verified against the Agent Teams docs).EADDRINUSEhandling, entry script moved to a per-user0700temp dir (tamper/TOCTOU), fail-fast validation for--port/--memory/--cpus/unknown flags,rebuildrefuses while the box runs,downreports real failures,~/.claude.jsonsymlink/directory edge cases, shell-free launcher (Windows injection),doctoruid warning.2. Release readiness
@liohtml/agentbox— the bare npm nameagentboxis an npm security placeholder and can't be published to;npx agentboxoutside the repo was a footgun (README fixed).tsxmoved to runtime dependencies (the published package was unrunnable for end users),publishConfigwith provenance, tag-triggered release workflow.npm packtarball smoke test; SECURITY.md (incl. the~/.claudesettings/hooks container-to-host escape channel), CODE_OF_CONDUCT, PR/issue templates, Dependabot, CHANGELOG, RELEASING (replaces PUBLISH.md, which contained a personal email).3. Roadmap wave 1 (from ecosystem research, see
ROADMAP.md)--yolo(opt-in--dangerously-skip-permissionsfor the lead), persistent npm/cargo/pip cache volumes, read-only~/.gitconfig+~/.config/ghforwarding withghin the image,--gpuspassthrough,doctorimage/host arch check.Scope
src/cli/— arg parsing, help, config, command routersrc/docker/— image build, container run/exec, mountssrc/session/— tmux entry script, TTY sizingassets/— Dockerfile / Observersrc/types.ts/CONTRACT.md— contract change (explain below!)Checklist
npm run typecheckpassesnpm testpasses (45 → 76 tests)tsxintentionally became a runtime dependency so the published package works via npx (CONTRIBUTING's no-build constraint is unchanged)DockerModulegainedexecOk/imageArchitecture;runContaineris now detached; the entry script idles as PID 1 instead ofexec tmux attach; new config fieldsyolo/gpus/gitconfigPath/ghConfigDir. CONTRACT.md §5/§6 updated accordingly.Notes
Touches all modules because it consolidates an audit; the three commits are separable if needed. The weekly "Research digest" routine will file follow-up issues; remaining roadmap items are tracked in
ROADMAP.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01RfjQjysq5JF7gQCJB3RQtP
Generated by Claude Code
Summary by CodeRabbit
--gpuspassthrough and opt-in--yolopermission-skip mode.doctorwith architecture, mount-safety, and config checklist reporting.upcleans stale stopped boxes and detaching doesn’t tear down the sandbox.@liohtml/agentbox, refreshed help/contract/security info, and expanded release/contribution guidance.