charles/claude-hooks
1
0
Fork 0
Code Issues 10 Pull requests Projects Releases Packages 1 Wiki Activity Actions

feat(container): bind-mount credentials directory, not file #58

Merged
code-lead merged 4 commits from dev/57 into main 2026-04-18 18:00:06 +00:00
Conversation 3 Commits 4 Files changed 10 +359 -48
dev commented 2026-04-18 17:39:26 +00:00
Collaborator
Copy link

Switch the Claude Code credentials bind from a single file to the containing directory so that claude login on the host (which atomic-renames .credentials.json to a new inode) is visible inside running containers without a restart.

Changes

  • src/container.ts: add defaultHostCredentialsDirPath(); replace credentialsHostPath: string with credentialsHostDir: string on ContainerConfig; update assertCredentialsBindSafe to check the directory against ~/.claude rather than the individual file
  • src/webhook-config.ts: resolve credentialsHostDir from credentials_host_dir (preferred) → dirname(credentials_host_path) (backwards compat) → defaultHostCredentialsDirPath() (default)
  • src/agent-runner.ts: pass credentialsHostDir to assertCredentialsBindSafe
  • justfile (containers-rebuild): read credentials_host_dir, fall back to dirname(credentials_host_path), mount the directory at /home/claude/.config/claude-code:ro instead of the file
  • src/webhook-config.test.ts (new): backwards-compat resolution coverage — explicit dir, file-path fallback, neither-set default, priority when both set
  • src/container.test.ts: add defaultHostCredentialsDirPath tests; update assertCredentialsBindSafe tests for directory semantics; update justfile↔container.ts agreement test
  • src/agent-runner.test.ts: update fixture to use credentialsHostDir
  • scripts/smoke-creds.sh (new): manual inode-propagation smoke test — simulates both in-place rewrite and atomic rename, verifies host and container see the same inode
  • CLAUDE.md: document directory-mount behaviour and smoke-test script

Backwards compatibility

No config changes required. Existing agents.json with container.credentials_host_path (file) automatically get dirname(credentials_host_path) as the directory mount source.

Closes #57

Switch the Claude Code credentials bind from a single file to the containing directory so that `claude login` on the host (which atomic-renames `.credentials.json` to a new inode) is visible inside running containers without a restart. ## Changes - **`src/container.ts`**: add `defaultHostCredentialsDirPath()`; replace `credentialsHostPath: string` with `credentialsHostDir: string` on `ContainerConfig`; update `assertCredentialsBindSafe` to check the directory against `~/.claude` rather than the individual file - **`src/webhook-config.ts`**: resolve `credentialsHostDir` from `credentials_host_dir` (preferred) → `dirname(credentials_host_path)` (backwards compat) → `defaultHostCredentialsDirPath()` (default) - **`src/agent-runner.ts`**: pass `credentialsHostDir` to `assertCredentialsBindSafe` - **`justfile`** (`containers-rebuild`): read `credentials_host_dir`, fall back to `dirname(credentials_host_path)`, mount the directory at `/home/claude/.config/claude-code:ro` instead of the file - **`src/webhook-config.test.ts`** (new): backwards-compat resolution coverage — explicit dir, file-path fallback, neither-set default, priority when both set - **`src/container.test.ts`**: add `defaultHostCredentialsDirPath` tests; update `assertCredentialsBindSafe` tests for directory semantics; update justfile↔container.ts agreement test - **`src/agent-runner.test.ts`**: update fixture to use `credentialsHostDir` - **`scripts/smoke-creds.sh`** (new): manual inode-propagation smoke test — simulates both in-place rewrite and atomic rename, verifies host and container see the same inode - **`CLAUDE.md`**: document directory-mount behaviour and smoke-test script ## Backwards compatibility No config changes required. Existing agents.json with `container.credentials_host_path` (file) automatically get `dirname(credentials_host_path)` as the directory mount source. Closes #57
feat(container): bind-mount credentials directory, not file
Some checks failed
qa / qa (pull_request) Failing after 47s
Details
qa / dockerfile (pull_request) Successful in 6s
Details
0a001a6d52 
Switch the Claude Code credentials bind from a single file to the
containing directory so that `claude login` on the host (which
atomic-renames `.credentials.json` to a new inode) is visible inside
running containers without a restart.

Changes:
- container.ts: add `defaultHostCredentialsDirPath()`; replace
  `credentialsHostPath: string` with `credentialsHostDir: string` on
  `ContainerConfig`; update `assertCredentialsBindSafe` to check the
  directory against `~/.claude` rather than the file
- webhook-config.ts: resolve `credentialsHostDir` from
  `credentials_host_dir` (preferred) → `dirname(credentials_host_path)`
  (backwards compat) → `defaultHostCredentialsDirPath()` (default)
- agent-runner.ts: pass `credentialsHostDir` to `assertCredentialsBindSafe`
- justfile (containers-rebuild): read `credentials_host_dir`, fall back to
  `dirname(credentials_host_path)`, mount directory at
  `/home/claude/.config/claude-code:ro` instead of the file
- Tests: update container.test.ts, agent-runner.test.ts; add
  webhook-config.test.ts with backwards-compat resolution coverage
- scripts/smoke-creds.sh: manual inode-propagation smoke test
- CLAUDE.md: document directory-mount behaviour

Closes #57

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(ci): collapse multi-line expect to satisfy Biome line-length rule
All checks were successful
qa / qa (pull_request) Successful in 51s
Details
qa / dockerfile (pull_request) Successful in 16s
Details
06e5b56eab 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dev requested review from reviewer 2026-04-18 17:42:47 +00:00
reviewer requested changes 2026-04-18 17:44:23 +00:00
Dismissed
reviewer left a comment
Copy link

Review

CI: ✅ green (#1553, 1m 9s).

The core fix is correct and well-structured. Directory bind-mount vs. file bind-mount is the right solution to the inode-orphan problem. Backwards-compat resolution (credentials_host_dir → dirname(credentials_host_path) → default) is handled consistently in both webhook-config.ts and the justfile. assertCredentialsBindSafe now correctly blocks ~/.claude and any subdirectory. Tests are thorough.

One bug blocks approval.

Bug — scripts/smoke-creds.sh exits on the very first counter increment (set -e + arithmetic)

File: scripts/smoke-creds.sh

In bash, ((expr)) exits with status 1 when expr evaluates to zero. With set -e active (line 17: set -euo pipefail), any command that exits non-zero aborts the script.

Both counters start at 0:

pass=0
fail=0

The first time ((pass++)) is called with pass=0, the post-increment expression evaluates to 0 (the old value), so the command exits with status 1 → set -e kills the script. Same for ((fail++)). The smoke test never completes a single full agent check.

This affects three call sites:

  • Line ~49: { echo " ERROR: container not found"; ((fail++)); continue; }
  • Line ~68: ((pass++)) / ((fail++)) after in-place rewrite check
  • Line ~80: ((pass++)) / ((fail++)) after atomic-rename check

Fix — append || true to each arithmetic increment (or use += assignment):

# option A — safest, idiomatic with set -e
((pass++)) || true
((fail++)) || true

# option B — always safe, avoids the issue entirely  
pass=$((pass + 1))
fail=$((fail + 1))

Apply the fix to all six ((pass++)) / ((fail++)) occurrences in the script.

Everything else is good to go. Fix the smoke-script counter increments and this is ready to merge.

## Review **CI:** ✅ green (`#1553`, 1m 9s). The core fix is correct and well-structured. Directory bind-mount vs. file bind-mount is the right solution to the inode-orphan problem. Backwards-compat resolution (`credentials_host_dir` → `dirname(credentials_host_path)` → default) is handled consistently in both `webhook-config.ts` and the justfile. `assertCredentialsBindSafe` now correctly blocks `~/.claude` and any subdirectory. Tests are thorough. One bug blocks approval. --- ### Bug — `scripts/smoke-creds.sh` exits on the very first counter increment (`set -e` + arithmetic) **File:** `scripts/smoke-creds.sh` In bash, `((expr))` exits with status 1 when *expr evaluates to zero*. With `set -e` active (line 17: `set -euo pipefail`), any command that exits non-zero aborts the script. Both counters start at 0: ```bash pass=0 fail=0 ``` The first time `((pass++))` is called with `pass=0`, the post-increment expression evaluates to `0` (the old value), so the command exits with status 1 → `set -e` kills the script. Same for `((fail++))`. The smoke test never completes a single full agent check. This affects three call sites: - Line ~49: `{ echo " ERROR: container not found"; ((fail++)); continue; }` - Line ~68: `((pass++))` / `((fail++))` after in-place rewrite check - Line ~80: `((pass++))` / `((fail++))` after atomic-rename check **Fix** — append `|| true` to each arithmetic increment (or use `+=` assignment): ```bash # option A — safest, idiomatic with set -e ((pass++)) || true ((fail++)) || true # option B — always safe, avoids the issue entirely pass=$((pass + 1)) fail=$((fail + 1)) ``` Apply the fix to all six `((pass++))` / `((fail++))` occurrences in the script. --- Everything else is good to go. Fix the smoke-script counter increments and this is ready to merge.
scripts/smoke-creds.sh
@ -0,0 +46,4 @@
'.agents[$n].container.credentials_host_dir // empty' "$AGENTS_JSON")
if [[ -z "$creds_dir" ]]; then
creds_file_cfg=$(jq -r --arg n "$name" \
'.agents[$n].container.credentials_host_path // empty' "$AGENTS_JSON")
reviewer commented 2026-04-18 17:44:23 +00:00
Collaborator
Copy link

set -e + ((post-increment)) kills the script on the first call when the counter is 0.

((fail++)) with fail=0 evaluates to 0 → exit status 1 → set -e aborts immediately. Same for every ((pass++)) call when pass=0.

Fix:

((fail++)) || true
# or
fail=$((fail + 1))

Apply to all six ((pass++)) / ((fail++)) occurrences in this file.

**`set -e` + `((post-increment))` kills the script on the first call when the counter is 0.** `((fail++))` with `fail=0` evaluates to `0` → exit status 1 → `set -e` aborts immediately. Same for every `((pass++))` call when `pass=0`. Fix: ```bash ((fail++)) || true # or fail=$((fail + 1)) ``` Apply to all six `((pass++))` / `((fail++))` occurrences in this file.
fix(smoke-creds): replace ((x++)) with x=$((x+1)) to survive set -e
Some checks failed
qa / qa (pull_request) Successful in 54s
Details
qa / dockerfile (pull_request) Failing after 5s
Details
7889958862 
((post-increment)) exits 1 when the old value is 0, which kills the
script immediately under set -e. Use arithmetic expansion instead,
which never has a non-zero exit status.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dev commented 2026-04-18 17:52:20 +00:00
Author
Collaborator
Copy link

Fixed in 7889958. Replaced all five ((pass++)) / ((fail++)) with pass=$((pass + 1)) / fail=$((fail + 1)) — $((expr)) never exits non-zero so it's safe under set -e.

Fixed in 7889958. Replaced all five `((pass++))` / `((fail++))` with `pass=$((pass + 1))` / `fail=$((fail + 1))` — `$((expr))` never exits non-zero so it's safe under `set -e`.
fix(ci): re-trigger after transient 504 on hadolint download
All checks were successful
qa / qa (pull_request) Successful in 48s
Details
qa / dockerfile (pull_request) Successful in 8s
Details
e37487fe62 
Previous run failed with curl: (22) 504 from GitHub releases — a
transient network error on the runner, not a code defect.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dev requested review from reviewer 2026-04-18 17:57:57 +00:00
reviewer approved these changes 2026-04-18 17:59:36 +00:00
reviewer left a comment
Copy link

Review — APPROVED

CI: ✅ green (run #1560, success, 56 s on head e37487f)

Acceptance criteria (issue #57)

Criterion Status
credentials_host_dir config key accepted ✅ webhook-config.ts priority chain
Backwards compat: credentials_host_path → dirname() auto ✅ both webhook-config.ts and justfile
Default falls back to ~/.config/claude-hooks/claude-credentials ✅ defaultHostCredentialsDirPath()
Container launched with directory bind (:ro) ✅ justfile --volume "$creds_dir:/home/claude/.config/claude-code:ro"
Read-only preserved ✅
Security invariant broadened to block any path under ~/.claude ✅ startsWith(\${forbidden}/`)`
Unit tests for all 3 resolution paths ✅ src/webhook-config.test.ts (new, 7 tests)
assertCredentialsBindSafe tests updated for directory semantics ✅ subdirectory case added
defaultHostCredentialsDirPath tests ✅ parent-dir relationship + dedicated-path assertions
Smoke-test script ✅ scripts/smoke-creds.sh — covers both in-place rewrite and atomic rename
CLAUDE.md documents directory-mount behaviour ✅

Code observations

src/container.ts — The @deprecated JSDoc on defaultHostCredentialsPath() is a nice touch; it keeps the backwards-compat path discoverable while signalling intent to callers. The security check credentialsHostDir === forbidden || credentialsHostDir.startsWith(\${forbidden}/`)correctly extends the guard to subdirectories of~/.claude`, not just the exact path.

src/webhook-config.ts — Resolution priority is clean and the three-branch if/else if/else mirrors the justfile comment block exactly, which makes the two sides easy to audit together.

justfile — ${creds_file:+$(dirname "$creds_file")} safely handles an empty jq result without passing a bare dirname "" to the shell. Good defensive Bash.

scripts/smoke-creds.sh — The two-stage simulation (in-place cat src > dst then atomic cp + mv) precisely reproduces both failure modes from the issue. Note that stat -c %i is Linux-only (GNU coreutils); since this service runs on a Linux desktop that's fine, but a one-line comment near the top (# Linux only — uses GNU stat -c) would help anyone who tries to run it on macOS.

src/webhook-config.test.ts — The tmpDirs + afterEach cleanup pattern is correct (join(p, "..") normalises to the temp dir, not the file). All three resolution paths plus the priority-when-both-set case are covered.

Minor (non-blocking)

  • scripts/smoke-creds.sh: consider adding # Linux only — uses GNU stat -c near the top so macOS users aren't surprised. Not blocking.

No logic bugs, no unhandled error paths, no security issues, no scope creep. The implementation is complete and correct.

## Review — APPROVED **CI:** ✅ green (run #1560, `success`, 56 s on head `e37487f`) ### Acceptance criteria (issue #57) | Criterion | Status | |---|---| | `credentials_host_dir` config key accepted | ✅ `webhook-config.ts` priority chain | | Backwards compat: `credentials_host_path` → `dirname()` auto | ✅ both `webhook-config.ts` and `justfile` | | Default falls back to `~/.config/claude-hooks/claude-credentials` | ✅ `defaultHostCredentialsDirPath()` | | Container launched with directory bind (`:ro`) | ✅ justfile `--volume "$creds_dir:/home/claude/.config/claude-code:ro"` | | Read-only preserved | ✅ | | Security invariant broadened to block any path under `~/.claude` | ✅ `startsWith(\`${forbidden}/\`)` | | Unit tests for all 3 resolution paths | ✅ `src/webhook-config.test.ts` (new, 7 tests) | | `assertCredentialsBindSafe` tests updated for directory semantics | ✅ subdirectory case added | | `defaultHostCredentialsDirPath` tests | ✅ parent-dir relationship + dedicated-path assertions | | Smoke-test script | ✅ `scripts/smoke-creds.sh` — covers both in-place rewrite and atomic rename | | `CLAUDE.md` documents directory-mount behaviour | ✅ | ### Code observations **`src/container.ts`** — The `@deprecated` JSDoc on `defaultHostCredentialsPath()` is a nice touch; it keeps the backwards-compat path discoverable while signalling intent to callers. The security check `credentialsHostDir === forbidden || credentialsHostDir.startsWith(\`${forbidden}/\`)` correctly extends the guard to subdirectories of `~/.claude`, not just the exact path. **`src/webhook-config.ts`** — Resolution priority is clean and the three-branch `if/else if/else` mirrors the justfile comment block exactly, which makes the two sides easy to audit together. **`justfile`** — `${creds_file:+$(dirname "$creds_file")}` safely handles an empty `jq` result without passing a bare `dirname ""` to the shell. Good defensive Bash. **`scripts/smoke-creds.sh`** — The two-stage simulation (in-place `cat src > dst` then atomic `cp + mv`) precisely reproduces both failure modes from the issue. Note that `stat -c %i` is Linux-only (GNU coreutils); since this service runs on a Linux desktop that's fine, but a one-line comment near the top (`# Linux only — uses GNU stat -c`) would help anyone who tries to run it on macOS. **`src/webhook-config.test.ts`** — The `tmpDirs` + `afterEach` cleanup pattern is correct (`join(p, "..")` normalises to the temp dir, not the file). All three resolution paths plus the priority-when-both-set case are covered. ### Minor (non-blocking) - `scripts/smoke-creds.sh`: consider adding `# Linux only — uses GNU stat -c` near the top so macOS users aren't surprised. Not blocking. No logic bugs, no unhandled error paths, no security issues, no scope creep. The implementation is complete and correct.
code-lead deleted branch dev/57 2026-04-18 18:00:06 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
reviewer
Labels
Clear labels   
area:agents
Agent types, pool scheduling, per-instance config

  
area:dashboard
Dashboard UI and observability surfaces

  
area:database
DB layer — schema, migrations, ORM, raw SQL

  
area:design
UI/UX mockup work — routes to designer agent

  
area:design-review
Design review dispatch — routes to design-reviewer agent

  
area:flows
Flow runner — YAML loader, executor, op registry, expression eval

  
area:infra
Deployment, isolation, containers, systemd units

  
area:meta
Tracking, scaffolding, project setup

  
area:security
Security — routes to reviewer-security (opus)

  
area:sessions
Session-id store, Claude SDK resume logic

  
area:webhook
Forgejo webhook routing and handlers

  
area:workdir
Clone cache, worktrees, git identity

  
security
Security-sensitive issue

  
type:bug
Bug

  
type:chore
Chore

  
type:meta
Tracking or decisions, not implementation work

  
type:user-story
User story

No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/claude-hooks!58
No description provided.