Repo optimization: lifecycle & security fixes, release readiness, roadmap wave 1 - #18
Repo optimization: lifecycle & security fixes, release readiness, roadmap wave 1#18Liohtml wants to merge 4 commits into
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
📝 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 container PID 1 alive while session exists
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials across all workflows.
Each checkout retains the GitHub token while later steps execute repository or package code.
.github/workflows/ci.yml#L24-L25: setpersist-credentials: false..github/workflows/ci.yml#L46-L47: setpersist-credentials: false..github/workflows/release.yml#L15-L15: setpersist-credentials: false..github/workflows/release.yml#L32-L32: setpersist-credentials: false.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 24-25: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 2 files
.github/workflows/ci.yml#L24-L25(this comment).github/workflows/ci.yml#L46-L47.github/workflows/release.yml#L15-L15.github/workflows/release.yml#L32-L32
🤖 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 @.github/workflows/ci.yml around lines 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.
Source: Linters/SAST tools
| on: | ||
| push: | ||
| tags: ["v*"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the tag against the package version before publishing.
v* accepts arbitrary tags, and the workflow never checks that the tag equals v${package.json.version}. Add a pre-publish assertion to prevent stale or mistyped tags from publishing the wrong version.
🤖 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 @.github/workflows/release.yml around lines 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.
| checks: | ||
| name: typecheck & test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "22" | ||
| cache: npm | ||
| - run: npm ci | ||
| - run: npm run typecheck | ||
| - run: npm test |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the release artifact smoke test part of the release gate.
The release workflow publishes after source tests but does not validate the packed tarball.
.github/workflows/release.yml#L11-L22: add the clean-installnpm packsmoke test before publication.RELEASING.md#L25-L26: keep the “all checks” statement only after the release workflow runs that smoke test.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 15-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 16-16: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
📍 Affects 2 files
.github/workflows/release.yml#L11-L22(this comment)RELEASING.md#L25-L26
🤖 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 @.github/workflows/release.yml around lines 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.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "22" | ||
| cache: npm | ||
| - run: npm ci |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== release workflow =="
cat -n .github/workflows/release.yml
echo
echo "== cache-related entries =="
rg -n "cache: npm|actions/setup-node|npm ci|npm publish|npm_config_userconfig|NPM_TOKEN|NODE_AUTH_TOKEN|registry-url" .github/workflows/release.yml || true
echo
echo "== package files if present =="
fd -a '^(package\.json|package-lock\.json|npm-shrinkwrap\.json)$' . | sed 's#^\./##' | while read -r f; do echo "--- $f"; sed -n '1,160p' "$f"; doneRepository: Liohtml/agentbox
Length of output: 7654
🌐 Web query:
GitHub Actions actions/setup-node cache npm cache scope public fork pull request documentation
💡 Result:
When using GitHub Actions, pull requests from public forks present specific security considerations regarding cache access and modification [1][2]. By default, workflows triggered by a pull request—including those from public forks—can restore caches created in the base branch (such as the default main branch), but they cannot write to the base branch's cache [1][2]. Caches created by a workflow run triggered by a pull request are scoped to the merge ref (refs/pull/.../merge) [1][2]. This ensures that these caches are isolated and can only be restored by re-runs of that specific pull request [1][2]. Regarding actions/setup-node specifically: 1. Cache Poisoning Prevention: Because untrusted pull requests could theoretically modify caches to inject malicious dependencies, it is recommended to disable automatic dependency caching in publishing workflows or workflows with elevated privileges [3][4][5]. This is done by setting the input package-manager-cache to false [6][7][8]. 2. Read-Only Caching (Restore-Only): As of March 2026, an open pull request (#1508) for actions/setup-node introduces a cache-write input [9]. This input is designed to allow workflows to restore dependencies from the cache without writing back to it at the end of the run, effectively enabling a read-only cache mode [9]. When cache-write is set to false, the post-step cache save is skipped, which prevents untrusted pull requests from potentially poisoning the cache while still allowing them to benefit from existing cached dependencies [9]. Example usage provided in the pull request: - uses: actions/setup-node@v6 with: node-version: 20 cache: npm cache-write: ${{ github.event_name!= 'pull_request' }} If you are not using this proposed feature, standard practice for securing sensitive workflows is to explicitly disable automatic caching to avoid cache poisoning risks [3][4].
Citations:
- 1: https://docs.github.com/en/enterprise-server@3.9/actions/using-workflows/caching-dependencies-to-speed-up-workflows
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/dependency-caching
- 3: https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md
- 4: https://github.com/actions/setup-node?tab=readme-ov-file
- 5: https://github.com/marketplace/actions/setup-node-js-environment
- 6: https://github.com/actions/setup-node
- 7: https://github.com/actions/setup-node/blob/v6/README.md
- 8: https://github.com/actions/setup-node/tree/refs/heads/main
- 9: Add cache-write input for read-only cache mode actions/setup-node#1508
Disable npm caching in the release workflow steps.
Both .github/workflows/release.yml#L16-L20 and #L33-L38 rely on actions/setup-node npm caching for jobs that install before typecheck/test and npm publish. Pin trusted dependencies via the lockfile and avoid cache restore/write in these sensitive jobs; with this version, use a cached npm setup without restore/write if needed, or remove caching entirely.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 16-16: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
📍 Affects 1 file
.github/workflows/release.yml#L16-L20(this comment).github/workflows/release.yml#L33-L38
🤖 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 @.github/workflows/release.yml around lines 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.
Source: Linters/SAST tools
| - run: npm publish --provenance --access public | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching release docs/workflows:\n'
git ls-files | rg '(^\.github/workflows/.*release.*\.ya?ml$|RELEASING\.md|package\.json|npm-shrinkwrap\.json|package-lock\.json)$' || true
printf '\nrelease workflow excerpt:\n'
if [ -f .github/workflows/release.yml ]; then
nl -ba .github/workflows/release.yml | sed -n '1,180p'
fi
printf '\nRELEASING excerpt:\n'
if [ -f RELEASING.md ]; then
nl -ba RELEASING.md | sed -n '1,90p'
fi
printf '\nSearch for NPM_TOKEN/Trusted Publishing/permissions in relevant files:\n'
rg -n "NPM_TOKEN|Trusted Publishing|id-token|provenance|npm publish|registry-url|publish" .github/workflows/release.yml RELEASING.md package.json package-lock.json npm-shrinkwrap.json 2>/dev/null || true
printf '\nPackage publishing metadata relevant to Trusted Publishing prerequisites:\n'
python3 - <<'PY'
import json
try:
with open('package.json') as f:
p=json.load(f)
print(json.dumps({k:p.get(k) for k in ['name','version','repository','packageManager']}, indent=2))
except Exception as e:
print("package.json missing/unreadable:", e)
PYRepository: Liohtml/agentbox
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'release.yml excerpt:\n'
awk '{printf "%6d\t%s\n",NR,$0}' .github/workflows/release.yml | sed -n '1,180p'
printf '\nRELEASING.md excerpt:\n'
awk '{printf "%6d\t%s\n",NR,$0}' RELEASING.md | sed -n '1,90p'
printf '\nSearch relevant terms:\n'
grep -n "NPM_TOKEN|Trusted Publishing|id-token|provenance|npm publish|registry-url|publish" .github/workflows/release.yml RELEASING.md package.json package-lock.json npm-shrinkwrap.json 2>/dev/null || true
printf '\npackage.json metadata:\n'
python3 - <<'PY'
import json
try:
with open('package.json') as f:
p=json.load(f)
print(json.dumps({k:p.get(k) for k in ['name','version','repository','packageManager','publishConfig','engines'] if k in p}, indent=2))
except Exception as e:
print("package.json missing/unreadable:", e)
PYRepository: Liohtml/agentbox
Length of output: 3243
Align release auth with the documented path.
RELEASING.md says Trusted Publishing is supported, but .github/workflows/release.yml still always seeds NODE_AUTH_TOKEN before npm publish. Either configure the workflow to use npm Trusted Publishing exclusively and update the docs, or keep Trusted Publishing documentation removed/clarified as unsupported.
📍 Affects 2 files
.github/workflows/release.yml#L39-L41(this comment)RELEASING.md#L34-L35
🤖 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 @.github/workflows/release.yml around lines 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.
| if (target?.isSymbolicLink()) { | ||
| // Dotfile managers often symlink ~/.claude.json. Follow it: if the | ||
| // target exists we're done; if it dangles, create the target through | ||
| // the symlink (flag "w" — "wx" would fail with a confusing EEXIST on | ||
| // the link itself). | ||
| try { | ||
| fs.statSync(config.claudeJson); | ||
| } catch { | ||
| fs.writeFileSync(config.claudeJson, "{}\n", { flag: "w" }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Verify that the symlink target is a regular file.
A symlink targeting a directory makes statSync succeed and then returns early, allowing Docker to mount a directory at ~/.claude.json. Check isFile() on the followed target and fail with the same corrective guidance otherwise.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 288-288: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(config.claudeJson, "{}\n", { flag: "w" })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-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 `@src/docker/index.ts` around lines 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".
| function writeEntryScript(config: AgentboxConfig, tty: TtySize): string { | ||
| const script = buildEntryScript(config, tty); | ||
| mkdirSync(dirname(config.entryScriptPath), { recursive: true, mode: 0o700 }); | ||
| // Never write through a pre-existing symlink at the target path. | ||
| try { | ||
| if (lstatSync(config.entryScriptPath).isSymbolicLink()) { | ||
| rmSync(config.entryScriptPath); | ||
| } | ||
| } catch { | ||
| // does not exist — fine | ||
| } | ||
| writeFileSync(config.entryScriptPath, script, { encoding: "utf8" }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not trust an existing predictable temp directory.
Recursive mkdirSync leaves a pre-created /tmp/agentbox-<uid> unchanged. Its owner could replace or race entry.sh, which is then executed inside the container. Atomically create a unique directory, or verify the existing directory is owned by the current UID and has no group/world permissions before writing.
🤖 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 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.
| /** | ||
| * Launch the Claude lead with --dangerously-skip-permissions (teammates | ||
| * inherit it). Opt-in via --yolo — the disposable, resource-capped box is | ||
| * the environment that flag exists for, but it stays off by default. | ||
| */ | ||
| yolo: boolean; | ||
| /** Optional `docker run --gpus` value (e.g. "all", "device=0"). flag: --gpus. */ | ||
| gpus?: string; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the locked CLI contract for --yolo and --gpus.
AgentboxConfig now exposes both flags, but CONTRACT.md’s global flag surface omits them. The prose and machine-readable contract explicitly require agreement.
🤖 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 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.
| test("mounts workspace, claude dir/json, entry script and observer", () => { | ||
| const mounts = valuesOf(buildRunArgs(baseConfig(), TTY, NONE), "-v"); | ||
| assert.ok(mounts.includes(`/home/u/proj:${CONTAINER_WORKSPACE}`)); | ||
| assert.ok(mounts.includes("/home/u/.claude:/home/agent/.claude")); | ||
| assert.ok(mounts.includes("/home/u/.claude.json:/home/agent/.claude.json")); | ||
| assert.ok(mounts.includes(`/tmp/agentbox-1000/entry.sh:${CONTAINER_ENTRY}`)); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the observer bind mount named by this test.
The current assertions do not verify observerAsset, so removing that mount would go undetected.
Proposed assertion
assert.ok(mounts.includes(`/tmp/agentbox-1000/entry.sh:${CONTAINER_ENTRY}`));
+ assert.ok(mounts.includes("/repo/assets/observer.mjs:/agentbox/observer.mjs"));📝 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.
| test("mounts workspace, claude dir/json, entry script and observer", () => { | |
| const mounts = valuesOf(buildRunArgs(baseConfig(), TTY, NONE), "-v"); | |
| assert.ok(mounts.includes(`/home/u/proj:${CONTAINER_WORKSPACE}`)); | |
| assert.ok(mounts.includes("/home/u/.claude:/home/agent/.claude")); | |
| assert.ok(mounts.includes("/home/u/.claude.json:/home/agent/.claude.json")); | |
| assert.ok(mounts.includes(`/tmp/agentbox-1000/entry.sh:${CONTAINER_ENTRY}`)); | |
| }); | |
| test("mounts workspace, claude dir/json, entry script and observer", () => { | |
| const mounts = valuesOf(buildRunArgs(baseConfig(), TTY, NONE), "-v"); | |
| assert.ok(mounts.includes(`/home/u/proj:${CONTAINER_WORKSPACE}`)); | |
| assert.ok(mounts.includes("/home/u/.claude:/home/agent/.claude")); | |
| assert.ok(mounts.includes("/home/u/.claude.json:/home/agent/.claude.json")); | |
| assert.ok(mounts.includes(`/tmp/agentbox-1000/entry.sh:${CONTAINER_ENTRY}`)); | |
| assert.ok(mounts.includes("/repo/assets/observer.mjs:/agentbox/observer.mjs")); | |
| }); |
🤖 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/docker.test.ts` around lines 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.
| test("falls back to the default port when OBSERVER_PORT is not a valid port", () => { | ||
| assert.match(src, /Number\.isInteger\(RAW_PORT\)/); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete fallback contract.
This only checks for Number.isInteger(RAW_PORT), so it passes if the 1–65535 bounds or the 4317 fallback are broken. Match the full validation expression or exercise invalid, zero, and out-of-range inputs.
🤖 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 - 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.
2 participants
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
New Features
doctor.Bug Fixes
Documentation