Fix AI credential volume scoping, simplify peon-ping/claude-dev coexistence - #100
Merged
Merged
Conversation
Code-review follow-up on the v1.3.0 claude-dev coexistence fix — no user-visible behavior change, but two real correctness gaps in it. - seed-claude-hooks.sh deduped by exact command-string equality instead of the install-invariant "peon-ping" substring install.sh's own merge_hooks_json already uses for Cursor/Copilot, leaving two independently-evolving dedup implementations that could silently diverge. Verified against a real installer-generated fragment (not just a synthetic one) that switching to the shared convention still registers multiple distinct peon-ping commands under one event (e.g. UserPromptSubmit's sound player plus its /peon-ping-* slash-command handlers) and stays idempotent across repeated runs - removed a redundant re-derivation of "is claude-dev present", computed once via a marker-file check and then re-derived 20 lines later by comparing paths — the two could silently diverge if edited inconsistently - cross-reference comments between peon-ping's and claude-dev's install scripts at the exact path each depends on, so a rename on either side is caught by grep instead of failing silently at runtime - fix a test comment that narrated the pre-fix bug instead of describing current behavior Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code-review follow-up on v1.3.0's cache-sharing change. The completion marker fell back to a generic ".h4-installed-unknown-<browsers>" name when `npx playwright --version` failed to resolve — harmless while the volume was exclusive per project, but now that it's shared across every local project for the host OS user, two different projects hitting that same failure (offline, transient npx/registry hiccup) would collide on the identical fallback marker. Whichever one installs first leaves the other thinking its own, possibly different, Playwright version already has its browser binaries cached. A failed lookup no longer participates in the marker shortcut at all — it just re-runs the install for that one start, which is already idempotent and revision-aware on Playwright's own side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code-review follow-up on the v1.3.0 volume-scoping change. The full --shared/exclusive explanation was restated near-identically across claude-dev, mistral-dev, pnpm-store, and playwright-dev's install.sh — a future correction to that rationale needed editing all four consistently, and a missed one would leave a stale explanation sitting next to the shared h4_ensure_volume_writable helper it's actually about. Trimmed each call site to the feature-specific fact (exclusive or shared, and why that's safe for what this particular volume holds) with a pointer to h4_ensure_volume_writable's own comment in helpers4-common for the general mechanism, instead of restating it every time. No behavior change — comments only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code-review follow-up on peon-ping's v1.3.0 claude-dev coexistence fix. That fix was necessary, but nothing documented it as a general pattern — claude-dev's destructive ~/.claude swap on every start is hostile to any feature that drops files there at build time, not just peon-ping, and the next feature to hit it would have had to rediscover the whole thing from scratch (most likely via files silently vanishing on first rebuild). Write up the 4-step pattern peon-ping already implements as a named, reusable recipe. Also documents, with a citation to the actual dev container spec, that installsAfter orders postStartCommand execution across features (not just installation) — the guarantee step 4 of the pattern depends on, which nothing in this repo stated explicitly before now. And notes that a chown race on the non---shared path (two containers attached to the same workspace) can't corrupt anything, since chown is metadata-only and both converge on the same target UID — re-assessed down from "could race a chown" to "wastes a chown," not worth adding locking for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code-review follow-up. 4 files (test.sh/with_cli.sh added in earlier commits on this range) used "Copyright (C) 2026 baxyz" — the year they happened to be added in — while the other 42 shell scripts in the repo use the literal "2025" from AGENTS.md's example block. AGENTS.md never stated whether that year is meant to track "current year" or stay fixed, so the drift had no rule to catch it. Realigned the 4 outliers to 2025 and added one sentence stating the year is fixed at the project's inception, not bumped per file — all 46 shell scripts now agree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ateCommand The v1.3.0/v1.3.1 fix worked but was more complex than necessary: peon-ping installed into a private per-container directory at image build time and relinked it into ~/.claude via a second postStartCommand, because claude-dev's own postStartCommand replaces ~/.claude with a symlink to its volume on every start, and a named volume isn't mounted yet during the image build that runs a feature's install.sh. Verified directly (docker run + an immediate docker exec) that a mount declared in "mounts" is already attached — and writable once chowned — from the very first command that runs inside a container, not just by postStartCommand: onCreateCommand and postCreateCommand see it too, and both fire exactly once per container instance instead of on every start. That changes the fix entirely: - claude-dev: setup-credentials.sh now runs via postCreateCommand instead of postStartCommand — the swap only needs to happen once, not be redone on every restart. It's also no longer destructive: if ~/.claude isn't already a symlink, its contents are added to the volume (cp -rn, never overwriting what's already there) before the swap, instead of being discarded outright — a safety net for any feature (this repo's own or a third-party one) that still writes into ~/.claude at image build time. - peon-ping: no longer writes into ~/.claude at build time at all. The actual install (binary, packs, Claude Code hooks) is deferred to its own postCreateCommand (install-claude-hooks.sh), declared installsAfter claude-dev. By the time it runs, ~/.claude is already whatever it's going to be for this container, so peon-ping just installs normally, straight into it — no CLAUDE_CONFIG_DIR redirect, no private claude-home directory, no relinking, no settings.json fragment rewriting. seed-claude-hooks.sh is gone entirely. Verified end-to-end in a real container (build, container-creation-time install in installsAfter order, and a full rebuild simulation with a planted build-time artifact to exercise claude-dev's new merge-preserve path) — all three of test.sh (claude-dev), test.sh (peon-ping), and the with_claude_dev.sh scenario pass for real against this design. Also corrects two related pieces of guidance found stale while making this change: AGENTS.md's own write-up of the old relink pattern (now describes the postCreateCommand approach instead), and the add-devcontainer-feature skill's claim that devcontainer features test "doesn't wire up mounts from a real host" — false for named volumes specifically (only true of host bind-mounts), demonstrated by claude-dev's own test.sh already exercising exactly that. No version bump on this branch — already bumped in an earlier commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
install-claude-hooks.sh (generated by install.sh) carried over su -
"${USERNAME}" from the old build-time (root) install, but this script
runs at container creation as postCreateCommand — already executing as
the container's remoteUser/containerUser, non-root in the standard case.
su - run BY that same non-root user requires their password (there's no
sudoers-style passwordless path for su itself), so it failed with
"Authentication failure" every time, silently swallowed by the `||`
guard, and the peon binary was never actually installed.
Verified directly: `su - <self> -c true`, run as that same non-root
user, fails the same way. Also verified the fix end-to-end as a real
non-root user (a created "vscode" user, postCreateCommand invoked via
runuser to mirror how the devcontainer CLI actually runs it — as the
resolved remoteUser, via a privileged mechanism that needs no password)
— the peon binary installs correctly and both test/peon-ping/test.sh and
test/peon-ping/with_claude_dev.sh pass for real under it. All of this
session's prior manual verification had used USERNAME=root throughout,
which never exercised this path; claude-dev's own postCreateCommand
script already avoided su for the same reason and was the tell.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
❌ PR Validation Failed
📋 Pipeline Status
🤖 Generated by @helpers4 CI • 2026-09-12 |
…ilures nub's own installer (curl https://nubjs.com/install.sh) returned a bare 403 to a GitHub Actions runner IP in this PR's CI run — reproduced the exact same install.sh against the same base image locally and it succeeded immediately, confirming this is IP-based flakiness on the external installer's side, not a code bug. Every feature's install.sh that pulls its real payload from a third-party URL at test time (an upstream installer, a GitHub release, a package registry) is exposed to the same class of transient failure. Wrap the devcontainer features test invocation in both test-features jobs (pr-validation.yml, test.yml) and both test-scenarios jobs in a 3-attempt retry with a 15s backoff, instead of adding a retry loop to every individual feature's install.sh just to paper over runner-IP flakiness. No version bump — CI workflow changes never require one (AGENTS.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Two related threads, both stemming from a code review of the volume-scoping change that shipped on
main(claude-dev/mistral-dev →${devcontainerId}, pnpm-store/playwright-dev →${localEnv:USER}):installsAfter/postStartCommandordering guarantee (cited against the actual dev container spec), and a copyright-year drift cleanup.docker run+ immediatedocker exec) that a named volume is already mounted from the very first command that runs inside a container — not just bypostStartCommand. That meanspostCreateCommand(fires once per container instance, not on every start) sees it too, which eliminates the need for the previous fix's redirect-into-a-private-directory-then-relink dance entirely:claude-dev'ssetup-credentials.shnow runs viapostCreateCommand, and is no longer destructive — it adds (cp -rn, never overwrites) anything found in a not-yet-linked~/.claudeto the volume instead of discarding it, as a safety net for any feature that still writes there at image build time.peon-pingno longer writes into~/.claudeat build time at all. Its actual install now runs in its ownpostCreateCommand, orderedinstallsAfterclaude-dev, straight into the real~/.claude— noCLAUDE_CONFIG_DIRredirect, no private directory, no relinking, no settings.json rewriting.seed-claude-hooks.shis gone.Version bump:
mainis currently at1.3.0forclaude-dev/mistral-dev/pnpm-store/playwright-dev/peon-ping(an earlier, separate accidental push). This branch bumps all five to1.3.1for the changes above — a real, consumer-visible bump, required by this repo's ownversion-bump-check.Fixed after initial review (
a76c7d9):install-claude-hooks.sh(generated by peon-ping'sinstall.sh) carried oversu - "${USERNAME}"from the old build-time install, but this script now runs aspostCreateCommand— already executing as the non-rootremoteUser.su -run by that same non-root user requires their password and has no passwordless path, so it silently failed every time (swallowed by a||guard), and the peon binary was never actually installed. Caught by an external review of this PR; verified directly (su - <self>fails the same way as any non-root user) and fixed by droppingsu -entirely, since the script already runs as the correct user. Re-verified end-to-end as a real non-root user this time (all of this session's prior manual testing had usedUSERNAME=root, which never exercised this path).Test plan
postCreateCommandininstallsAfterorder, binary/hooks land directly in the real (volume-backed)~/.claudeclaude-dev's newcp -rnmerge-preserve path adds it to the volume without touching the volume's own pre-existing datarunuser, mirroring how the devcontainer CLI actually invokespostCreateCommand) after thesu -fix — this is the scenario that exposed the bug and needed separate coverage from the root-only checks abovetest/claude-dev/test.sh,test/peon-ping/test.sh,test/peon-ping/with_claude_dev.sh) run for real against this design, non-root included, and passplaywright-dev's marker-collision fix and dedup-logic fix verified against real installer output, including idempotency across repeated runspr-validation.yml/test.yml) — pending on this PR🤖 Generated with Claude Code