BitSquare/Codeman
2
0
Fork 0
mirror of https://github.com/Ark0N/Codeman.git synced 2026-09-30 12:39:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(docker): address Ark0N's PR review on Update-Codeman.sh — fix the handoff, the build/down ordering, and default-clear the build volumes

Browse Source 
Three blockers, all fixed and verified by actually running the script (not
just string-matching it):

1. `exec "$script_dir/Start-Codeman.sh"` failed EACCES/exit 126 on every
   checkout, since Start-Codeman.sh is committed non-executable (100644) —
   the same fact my own second commit on this branch established. Fixed to
   `exec bash "$script_dir/Start-Codeman.sh"`.

2. `down` ran before `build --no-cache`, so Codeman and every session it was
   running were offline for the entire rebuild, and a build failure left the
   stack down with nothing to bring it back — the exact ordering mistake
   Start-Codeman.sh's own "Build BEFORE taking the stack down" comment exists
   to prevent. Reordered to build, then down, then hand off.

3. The default path could throw the rebuild away: codeman-node-modules/
   codeman-dist only re-seed from the image while EMPTY, Start-Codeman.sh
   only clears them when it detects the checkout's HEAD or package-lock.json
   moved, and neither condition is true for the Dockerfile-only change this
   script exists for — so a plain `bash docker/Update-Codeman.sh` rebuilt an
   image whose fresh node_modules/dist then sat unused behind the old
   volumes. Made clearing them the default; `--keep-volumes` opts out
   (replaces the old `--volumes`/`-v` flag, which is no longer needed since
   clearing is now the default).

Smaller items from the same review, also fixed:

- The --no-cache build now derives PUID/PGID from CODEMAN_APPDATA_PATH's
  owner first, via the identical owner_of() helper Start-Codeman.sh uses
  (parity-tested) — without it, the build used Compose's default 1000:1000
  regardless of the real appdata owner (99:100 on the Unraid layout
  docker/README.md documents), and Start-Codeman.sh's own correctly-PUID'd
  build during the handoff would then rebuild those layers anyway, so the
  --no-cache image never actually shipped.
- docker/README.md's "rebuilds ... only when it detects ... moved" wrongly
  described BOTH the rebuild and the volume-clearing as conditional;
  Start-Codeman.sh rebuilds on every start, only the volume-clearing is
  conditional. Corrected, and reworded around the new default.
- --help/-h now prints usage and exits 0 instead of falling into the
  unrecognised-argument branch.
- "the ONLY named volumes this stack declares" now says docker-compose.yaml
  specifically, since a docker-compose.override.yml could add more.

New tests: PUID/PGID derivation parity with Start-Codeman.sh's owner_of(),
--help handling, and — the one that actually catches blocker #1, which five
source-string-matching tests did not — a real end-to-end smoke test: a
synthetic deployment, a stub `docker` on PATH logging every invocation, the
real script executed via a real subprocess. Confirms the real command
sequence (build --no-cache, then down --volumes or plain down, then evidence
the handoff genuinely ran Start-Codeman.sh) and that a working handoff fails
honestly at Start-Codeman.sh's own later check rather than with EACCES.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N6eadpRyqpA9PD3i139cSD
This commit is contained in:
Devvyn
2026-09-21 15:19:00 +08:00
co-authored by Claude Sonnet 5
parent 3b714446b4
commit 5b4878df3b
4 changed files with 316 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/docker-entrypoint.test.ts
+201 -17
View File
@@ -21,7 +21,7 @@
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import { readFileSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import { readFileSync, mkdtempSync, rmSync, writeFileSync, mkdirSync } from 'node:fs';
import { execFileSync } from 'node:child_process';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
@@ -180,23 +180,32 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md
execFileSync('bash', ['-n', join(ROOT, 'docker/Update-Codeman.sh')]);
});
it('stops the stack, THEN force-rebuilds with --no-cache, THEN hands off to Start-Codeman.sh', () => {
const down = updateScript.indexOf('"${compose_command[@]}" down');
it('force-rebuilds with --no-cache BEFORE stopping the stack, THEN hands off to Start-Codeman.sh via `bash`', () => {
const build = updateScript.indexOf('"${compose_command[@]}" build --no-cache');
const handoff = updateScript.indexOf('exec "$script_dir/Start-Codeman.sh"');
expect(down).toBeGreaterThan(-1);
expect(build).toBeGreaterThan(down);
expect(handoff).toBeGreaterThan(build);
const down = updateScript.indexOf('"${compose_command[@]}" down');
const handoff = updateScript.indexOf('exec bash "$script_dir/Start-Codeman.sh"');
expect(build).toBeGreaterThan(-1);
expect(down).toBeGreaterThan(build);
expect(handoff).toBeGreaterThan(down);
// A bare `exec "$script_dir/Start-Codeman.sh"` fails EACCES — Start-Codeman.sh
// is committed non-executable (100644), same as this script.
expect(updateScript).not.toMatch(/exec "\$script_dir\/Start-Codeman\.sh"/);
});
it('--volumes (or -v) removes the named volumes on the way down; the default path does not', () => {
expect(updateScript).toMatch(/--volumes \| -v\)\s*\n\s*remove_volumes=1/);
it('clears the named volumes by DEFAULT; --keep-volumes opts out to a plain `down`', () => {
expect(updateScript).toMatch(/--keep-volumes\)\s*\n\s*keep_volumes=1/);
expect(updateScript).toMatch(/"\$\{compose_command\[@\]\}" down --volumes/);
// The unconditional call further down (the else branch) must stay a plain
// `down` — accidentally merging the two branches would silently start
// wiping the build-artefact volumes on every major update, not just when
// the flag is passed.
expect(updateScript).toMatch(/else\s*\n\s*"\$\{compose_command\[@\]\}" down\s*\n\s*fi/);
// The keep_volumes branch must stay a plain `down` — merging the two would
// silently start wiping the build-artefact volumes even when asked not to.
expect(updateScript).toMatch(
/if \[\[ "\$keep_volumes" == '1' \]\]; then\s*\n\s*"\$\{compose_command\[@\]\}" down\s*\n\s*else/
);
});
it('--help/-h prints usage and exits 0, rather than falling into the unrecognised-argument branch', () => {
expect(updateScript).toMatch(/--help \| -h\)\s*\n\s*printf 'Usage:/);
const helpBlock = updateScript.slice(updateScript.indexOf('--help | -h)'), updateScript.indexOf('*)'));
expect(helpBlock).toMatch(/exit 0/);
});
it('rejects an unrecognised argument rather than silently ignoring it', () => {
@@ -210,12 +219,187 @@ describe('Update-Codeman.sh (the scripted major-update path — docker/README.md
// endings on disk and not the other's (e.g. Start-Codeman.sh checked out before this
// script existed), which would fail a raw byte comparison for a reason that has
// nothing to do with the two scripts actually agreeing.
const normalise = (s: string) => s.replace(/\r\n/g, '\n');
const overrideBlock = (script: string) =>
script
.slice(script.indexOf('override_yml='), script.indexOf('compose_command=(docker compose'))
.replace(/\r\n/g, '\n');
normalise(script.slice(script.indexOf('override_yml='), script.indexOf('compose_command=(docker compose')));
expect(overrideBlock(updateScript)).toBe(overrideBlock(startScript));
});
it('derives PUID/PGID from the SAME owner_of() helper Start-Codeman.sh uses, so the --no-cache build gets the right build args', () => {
const normalise = (s: string) => s.replace(/\r\n/g, '\n');
const ownerOfBlock = (script: string) => {
const start = script.indexOf('owner_of() {');
const end = script.indexOf('\n}', start) + '\n}'.length;
return normalise(script.slice(start, end));
};
expect(ownerOfBlock(updateScript)).toBe(ownerOfBlock(startScript));
expect(updateScript).toMatch(/export PUID=\$\{owner_ids%%:\*\}/);
expect(updateScript).toMatch(/export PGID=\$\{owner_ids##\*:\}/);
// The build must come AFTER PUID/PGID are resolved and exported, or Compose
// falls back to its own default of 1000:1000 for the build args.
const puidExport = updateScript.indexOf('export PUID=');
const build = updateScript.indexOf('"${compose_command[@]}" build --no-cache');
expect(puidExport).toBeGreaterThan(-1);
expect(build).toBeGreaterThan(puidExport);
});
describe('end-to-end smoke test (a stub `docker` on PATH, logging every invocation)', () => {
/**
* Reproduces the exact scenario the review on PR #465 caught by hand: a bare
* `exec` of a non-executable script exits 126 with no further `docker` calls
* at all. Runs the REAL Update-Codeman.sh against a synthetic deployment,
* asserting the actual command sequence a shell would issue — string-matching
* the source (the tests above) cannot tell a working `exec bash "…"` apart
* from a silently-broken bare `exec "…"` the way actually running it can.
*
* The harness intentionally does NOT create a real Unix socket for
* DOCKER_SOCKET (net.createServer().listen(path) is unreliable off Linux —
* measured EACCES on this Windows sandbox even outside any container). So the
* handoff to Start-Codeman.sh is real and fully exercises this script's own
* build/down/handoff sequence, but Start-Codeman.sh's OWN socket check is
* expected to then fail — which is itself the proof the handoff worked: a
* process that failed to exec would never reach a Start-Codeman.sh-only error
* message, and would exit 126, not 1.
*/
// Windows join()/mkdtempSync() paths carry backslashes, which the stub
// `docker`'s naive `source "$envfile"` (a shortcut for `docker compose
// config --environment`'s own real parsing, which handles this fine) reads
// as bash ESCAPE characters and silently drops — `C:\Users\x` becomes
// `C:Usersx`. Forward slashes are accepted by git-bash/MSYS on Windows and
// by every POSIX shell, so normalising once here sidesteps a harness
// artifact that has nothing to do with the scripts under test.
const posix = (p: string) => p.replace(/\\/g, '/');
function runSmokeTest(args: string[]): { status: number; stderr: string; log: string[] } {
const dir = mkdtempSync(join(tmpdir(), 'codeman-update-smoke-'));
try {
const dockerDir = join(dir, 'docker');
mkdirSync(dockerDir);
writeFileSync(join(dockerDir, 'Update-Codeman.sh'), updateScript);
writeFileSync(join(dockerDir, 'Start-Codeman.sh'), startScript);
writeFileSync(join(dockerDir, 'docker-compose.yaml'), compose);
const appdataPath = join(dir, 'appdata');
const casesPath = join(dir, 'cases');
const socketPath = join(dir, 'docker.sock'); // deliberately NOT a real socket — see above
mkdirSync(appdataPath);
mkdirSync(casesPath);
writeFileSync(socketPath, '');
writeFileSync(
join(dockerDir, '.env'),
[
`CODEMAN_APPDATA_PATH=${posix(appdataPath)}`,
`CODEMAN_CASES_PATH=${posix(casesPath)}`,
`DOCKER_SOCKET=${posix(socketPath)}`,
'CODEMAN_RUNTIME_USER=codeman',
'CODEMAN_PORT=3000',
'CODEMAN_HOST=127.0.0.1',
'CODEMAN_PASSWORD=x',
'CODEMAN_USERNAME=admin',
'GEMINI_API_KEY=',
'CODEMAN_DOCKER_BRIDGE_HOOKS=',
'CODEMAN_DOCKER_DISABLE_SWAP_LIMIT=',
'TZ=UTC',
'CODEMAN_IMAGE=codeman:test',
'',
].join('\n')
);
// A stub `docker` that only understands the two `compose config` shapes
// both scripts actually issue, and logs every invocation verbatim —
// written and chmod+x'd from WITHIN one bash invocation (not
// fs.chmodSync, whose Win32 backing does not reliably set the bit this
// MSYS bash's own PATH lookup honours — measured, differs from a plain
// `chmod +x` issued by bash itself).
const binDir = join(dir, 'bin');
mkdirSync(binDir);
const stub = [
'#!/usr/bin/env bash',
'echo "docker $*" >> "$CMDLOG"',
'if [[ "$1" == "compose" ]]; then',
' shift',
' prev=""',
' envfile=""',
' for a in "$@"; do',
' if [[ "$prev" == "--env-file" ]]; then envfile="$a"; fi',
' prev="$a"',
' done',
' if [[ " $* " == *" config "* && " $* " == *" --environment "* ]]; then',
' source "$envfile"',
' echo "CODEMAN_APPDATA_PATH=$CODEMAN_APPDATA_PATH"',
' echo "CODEMAN_CASES_PATH=$CODEMAN_CASES_PATH"',
' echo "DOCKER_SOCKET=$DOCKER_SOCKET"',
' exit 0',
' fi',
' if [[ " $* " == *" config "* && " $* " == *" --format json "* ]]; then',
' echo \'{"name":"codeman"}\'',
' exit 0',
' fi',
' exit 0',
'fi',
'exit 0',
].join('\n');
const stubPath = join(binDir, 'docker');
writeFileSync(stubPath, stub);
execFileSync('bash', ['-c', `chmod +x '${stubPath}'`]);
const logPath = join(dir, 'cmdlog.txt');
writeFileSync(logPath, '');
let status = 0;
let stderr = '';
try {
execFileSync('bash', [join(dockerDir, 'Update-Codeman.sh'), ...args], {
env: { ...process.env, PATH: `${binDir}:${process.env.PATH}`, CMDLOG: logPath },
encoding: 'utf-8',
});
} catch (err) {
const e = err as { status?: number; stderr?: string };
status = e.status ?? 1;
stderr = e.stderr ?? '';
}
const log = readFileSync(logPath, 'utf-8')
.split('\n')
.filter((l) => l.trim());
return { status, stderr, log };
} finally {
rmSync(dir, { recursive: true, force: true });
}
}
it('default: build --no-cache, THEN down --volumes, THEN the handoff genuinely runs Start-Codeman.sh', () => {
const { status, stderr, log } = runSmokeTest([]);
const buildIdx = log.findIndex((l) => l.includes('build --no-cache'));
const downIdx = log.findIndex((l) => l.includes(' down --volumes') || l.endsWith(' down'));
expect(buildIdx).toBeGreaterThan(-1);
expect(downIdx).toBeGreaterThan(buildIdx);
expect(log[downIdx]).toContain('down --volumes');
// Proof the handoff really executed Start-Codeman.sh rather than dying
// with EACCES right after printing "Handing off...": more `docker`
// invocations appear AFTER the down, which only Start-Codeman.sh's own
// config-resolution lines would produce.
const configCallsAfterDown = log.slice(downIdx + 1).filter((l) => l.includes('config'));
expect(configCallsAfterDown.length).toBeGreaterThan(0);
// A working handoff fails HONESTLY at Start-Codeman.sh's own socket
// check (this harness deliberately supplies no real Unix socket) — never
// with an EACCES/126 from a broken `exec`.
expect(status).toBe(1);
expect(stderr).toMatch(/DOCKER_SOCKET is not a Unix socket/);
expect(stderr).not.toMatch(/permission denied/i);
});
it('--keep-volumes: a plain `down`, with no --volumes flag', () => {
const { log } = runSmokeTest(['--keep-volumes']);
const downLine = log.find((l) => / down(\s|$)/.test(l));
expect(downLine).toBeDefined();
expect(downLine).not.toContain('--volumes');
});
});
});
describe('git_head_commit resolves every ref layout a checkout can have', () => {