mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(devcontainer): resolve local adversarial-review findings (low/nit)
Follow-up to a local branch review (run after the cloud review crashed before producing findings); all 5 confirmed findings were low/nit: - chown via `find -xdev -exec chown -h`: add -h so chown acts on a symlink ITSELF, not its target. Without it a dangling node_modules/.bin link aborted provisioning under `set -e`, and a cross-fs symlink target could be dereferenced/rewritten. Verified in a clean container (regular files still chowned; dangling link no longer aborts; cross-fs target untouched). Applied to install-deps.sh and post-create.sh; the inline comments are corrected to describe -xdev (descent bound) and -h (no deref) as the two distinct guards. - Reword the .cjs header claims from "lintable" to "unit-tested and prettier-checked": ESLint applies no rules to .cjs in this repo; CI only prettier-checks them. - README: the initializeCommand is `node ensure-host-config-dirs.cjs`, which creates the full bind-source set, not a bash `mkdir -p` of four dirs. - ci-devcontainer.yml: document that the x64 runner exercises only the amd64 Cursor branch; the arm64 sha/URL is hash-pinned (verified against the published artifact) but not built in CI. - Make the seed chmod-644 test meaningful: pre-create dst at 0o600 so only the explicit chmodSync can widen it (the prior assertion passed under the default umask regardless of whether the chmod ran). 25/25 config-transform tests pass; arm64 + x64 Cursor artifacts verified.
This commit is contained in:
parent
dab73c6e5d
commit
4169c50a13
7 changed files with 59 additions and 20 deletions
|
|
@ -174,7 +174,7 @@ That means:
|
|||
- **`gh` auth is shared.** `gh pr create`, `gh pr checks`, `gh issue create` work inside the container without re-authenticating.
|
||||
- **No per-workspace duplication.** All your devcontainers across all your projects see the same host CLI state, just like all your host shells do.
|
||||
|
||||
The bind mount source directories are guaranteed to exist by the `initializeCommand` (`mkdir -p $HOME/.claude $HOME/.codex $HOME/.cursor $HOME/.config/gh`), which runs on the host shell before container create.
|
||||
The bind mount source directories are guaranteed to exist by the `initializeCommand` (`node .devcontainer/ensure-host-config-dirs.cjs`), which runs on the host before container create. It's a Node script (not a shell one-liner) so the same command works on Windows `cmd.exe` and POSIX shells, and it creates the full set of bind-mount source dirs — all the `~/.claude`, `~/.codex`, `~/.cursor` shareable subdirs plus `~/.ssh`, `~/.docker`, `~/.aws`, `~/.azure`, `~/.config/{gh,git}` — not just the top-level CLI dirs.
|
||||
|
||||
### Trust boundary, concretely
|
||||
|
||||
|
|
|
|||
|
|
@ -19,17 +19,20 @@ echo "[install-deps] 1/4: chown workspace node_modules + npm cache mount points"
|
|||
# owned by the stale UID — npm install can't write. Re-chown
|
||||
# post-realignment; idempotent on subsequent runs.
|
||||
#
|
||||
# `find -xdev -exec chown` (same idiom as post-create.sh) rather than a bare
|
||||
# `chown -R`: -xdev keeps each chown ON its own volume filesystem, and `find`
|
||||
# does not follow symlinks during traversal — so a symlink committed in the
|
||||
# workspace tree (or dropped by a dependency postinstall on a rerun) can't
|
||||
# redirect the chown onto a host path outside the volume.
|
||||
# `find -xdev -exec chown -h` (same idiom as post-create.sh) rather than a bare
|
||||
# `chown -R`. Two distinct guards: `-xdev` bounds find's DESCENT to each
|
||||
# volume's own filesystem (it won't recurse into a sub-mounted host bind), and
|
||||
# `-h` makes chown act on a symlink ITSELF rather than dereferencing it. Without
|
||||
# `-h`, a symlink inside the tree (a dep postinstall dropping one, or a dangling
|
||||
# node_modules/.bin link) would either redirect the chown onto its cross-fs
|
||||
# target or abort provisioning under `set -e` with a dereference error. `-h` is
|
||||
# a no-op for regular files/dirs, so the intended ownership fix is unchanged.
|
||||
for d in /workspace/node_modules \
|
||||
/workspace/gitnexus/node_modules \
|
||||
/workspace/gitnexus-web/node_modules \
|
||||
/workspace/gitnexus-shared/node_modules \
|
||||
/home/node/.npm; do
|
||||
sudo find "$d" -xdev -exec chown node:node {} +
|
||||
sudo find "$d" -xdev -exec chown -h node:node {} +
|
||||
done
|
||||
|
||||
echo "[install-deps] 2/4: clear stale .husky/_ runtime cache"
|
||||
|
|
|
|||
|
|
@ -18,15 +18,18 @@ echo "[post-create] 1/2: chown AI CLI named-volume mount points"
|
|||
# install-deps.sh handles the workspace-side chown; this script handles
|
||||
# the AI CLI side so each lifecycle hook owns its own concern.
|
||||
#
|
||||
# `-xdev` keeps chown ON THE VOLUME filesystem and stops it descending into
|
||||
# the RW host bind mounts overlaid at sub-paths (plugins/marketplaces,
|
||||
# plugins/cache, skills, agents, memory, commands, codex/plugins, cursor/*).
|
||||
# Those binds are a DIFFERENT filesystem (9p/virtiofs/bind); recursing into
|
||||
# them would rewrite host file ownership on a non-UID-aligned Linux host and,
|
||||
# worse, an EPERM there would abort provisioning before credentials sync.
|
||||
# Two distinct guards. `-xdev` bounds find's DESCENT to the volume filesystem
|
||||
# so it won't recurse into the RW host bind mounts overlaid at sub-paths
|
||||
# (plugins/marketplaces, plugins/cache, skills, agents, memory, commands,
|
||||
# codex/plugins, cursor/*). Those binds are a DIFFERENT filesystem
|
||||
# (9p/virtiofs/bind); recursing into them would rewrite host file ownership on
|
||||
# a non-UID-aligned Linux host and, worse, an EPERM there would abort
|
||||
# provisioning before credentials sync. `-h` makes chown act on a symlink
|
||||
# ITSELF, never dereferencing it onto a cross-fs target and never aborting on a
|
||||
# dangling link under `set -e` (it's a no-op for regular files/dirs).
|
||||
for d in /home/node/.claude /home/node/.codex /home/node/.cursor \
|
||||
/home/node/.local /commandhistory; do
|
||||
sudo find "$d" -xdev -exec chown node:node {} +
|
||||
sudo find "$d" -xdev -exec chown -h node:node {} +
|
||||
done
|
||||
|
||||
echo "[post-create] 2/2: sync AI CLI credentials + identity from host"
|
||||
|
|
@ -105,8 +108,8 @@ sync_from_host /host/.codex/config.toml /home/node/.codex/config.toml 644
|
|||
# host's `installMethod` (e.g. "native") makes Claude probe ~/.local/bin/claude
|
||||
# and fail "claude command not found at /home/node/.local/bin/claude". The
|
||||
# transform (strip machine fields + force hasCompletedOnboarding, guarding a
|
||||
# non-object host file) lives in seed-claude-config.cjs so it is lintable and
|
||||
# unit-tested (translate-plugin-registries.test.cjs).
|
||||
# non-object host file) lives in seed-claude-config.cjs so it is unit-tested
|
||||
# and prettier-checked (translate-plugin-registries.test.cjs).
|
||||
node "$SCRIPT_DIR/seed-claude-config.cjs"
|
||||
|
||||
# Plugin registry path translation (Claude + Cursor). Both bake absolute
|
||||
|
|
@ -115,7 +118,7 @@ node "$SCRIPT_DIR/seed-claude-config.cjs"
|
|||
# on macOS — so the host versions can't be bind-mounted into the Linux
|
||||
# container (the CLI fails with `cache-miss` resolving a Windows path under
|
||||
# Linux). The translation (regex + deep rewrite + the REGISTRIES table) lives
|
||||
# in translate-plugin-registries.cjs so it is lintable and unit-tested. Codex
|
||||
# in translate-plugin-registries.cjs so it is unit-tested and prettier-checked. Codex
|
||||
# needs no translation — its enablement registry is config.toml with git URLs,
|
||||
# not filesystem paths, so its whole plugins/ dir is bind-mounted instead.
|
||||
node "$SCRIPT_DIR/translate-plugin-registries.cjs"
|
||||
|
|
|
|||
|
|
@ -9,8 +9,8 @@
|
|||
// its own method, and force hasCompletedOnboarding so the wizard is skipped
|
||||
// even on a first-time host.
|
||||
//
|
||||
// Extracted from a post-create.sh heredoc so the pure transform is lintable
|
||||
// and unit-tested (see seed-claude-config.test via translate-plugin-registries
|
||||
// Extracted from a post-create.sh heredoc so the pure transform is unit-tested
|
||||
// and prettier-checked (see seed-claude-config.test via translate-plugin-registries
|
||||
// test harness). DISABLE_AUTOUPDATER=1 (containerEnv) already neutralizes
|
||||
// runtime updates; this purely silences the doctor mismatch + native probe.
|
||||
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@
|
|||
// so its whole plugins/ dir is bind-mounted instead.)
|
||||
//
|
||||
// Extracted from a post-create.sh heredoc so the regex + deep rewrite are
|
||||
// lintable and unit-tested (the regex has had path-handling bugs before).
|
||||
// unit-tested and prettier-checked (the regex has had path-handling bugs before).
|
||||
|
||||
'use strict';
|
||||
|
||||
|
|
|
|||
|
|
@ -305,6 +305,30 @@ test('seed main: missing host file still writes a valid onboarding-bearing file'
|
|||
}
|
||||
});
|
||||
|
||||
test('seed main: chmodSync widens a pre-existing restrictive dst to 0o644', () => {
|
||||
// POSIX mode bits only. Under the CI default umask (022) a plain writeFileSync
|
||||
// already yields 0o644, so asserting 0o644 after a fresh write does NOT prove
|
||||
// the explicit chmodSync did anything. Pre-create dst at 0o600 first: a 'w'
|
||||
// write truncates content but PRESERVES an existing file's mode, so the file
|
||||
// can only reach 0o644 via seed-claude-config.cjs's chmodSync. This isolates
|
||||
// the chmod from the umask-default path (delete the chmodSync line and this
|
||||
// test fails, where the other seed test would still pass).
|
||||
if (process.platform === 'win32') return;
|
||||
const dir = tmp();
|
||||
try {
|
||||
const src = path.join(dir, 'host.claude.json');
|
||||
const dst = path.join(dir, 'out.claude.json');
|
||||
fs.writeFileSync(src, JSON.stringify({ userID: 'u' }));
|
||||
fs.writeFileSync(dst, '{}');
|
||||
fs.chmodSync(dst, 0o600);
|
||||
execFileSync(process.execPath, [SEED_SCRIPT, src, dst]);
|
||||
assert.equal(fs.statSync(dst).mode & 0o777, 0o644);
|
||||
assert.equal(JSON.parse(fs.readFileSync(dst, 'utf8')).userID, 'u');
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// --- ensurePaths: host bind-source bootstrap --------------------------------
|
||||
|
||||
test('ensurePaths: creates every DIR and FILE under a temp home, idempotently', () => {
|
||||
|
|
|
|||
9
.github/workflows/ci-devcontainer.yml
vendored
9
.github/workflows/ci-devcontainer.yml
vendored
|
|
@ -65,6 +65,15 @@ jobs:
|
|||
# This is the smoke that catches Dockerfile regressions + drift from the
|
||||
# canonical version pins. Lifecycle hooks (post-create.sh) are not run
|
||||
# here — they need the host config mounts, which CI has none of.
|
||||
#
|
||||
# ARCH COVERAGE: this runs on an x64 runner with no --platform/QEMU, so it
|
||||
# exercises the amd64 Cursor branch (CURSOR_SHA256_X64) only. The arm64
|
||||
# branch (CURSOR_SHA256_ARM64 + the arm64 tarball URL) is pinned by sha256
|
||||
# verified against the published artifact, but is not BUILT here. Cursor's
|
||||
# extract+symlink step is arch-independent, so the residual gap is a stale
|
||||
# arm64 URL/hash; add a linux/arm64 matrix leg (docker/setup-qemu-action +
|
||||
# `--platform`) if that becomes a concern.
|
||||
#
|
||||
# Version-pinned: bare `npx --yes @devcontainers/cli` resolves @latest at
|
||||
# run time, so a breaking or malicious publish could change CI behavior
|
||||
# (or how devcontainer.json is interpreted) with no diff. Bump
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue