Skip to content

test(docker): give the Update-Codeman.sh smoke harness a non-root appdata owner - #738

Open
JDProfresh wants to merge 2 commits into
Ark0N:masterfrom
JDProfresh:test/docker-smoke-root-runner
Open

JDProfresh wants to merge 2 commits into
Ark0N:masterfrom
JDProfresh:test/docker-smoke-root-runner

Conversation

@JDProfresh

Copy link
Copy Markdown
Contributor

Under a root test runner, two of the Update-Codeman.sh smoke tests in test/docker-entrypoint.test.ts fail because the harness creates its appdata directory with mkdirSync, so it is owned by root, and the script correctly refuses a root-owned appdata directory before it ever reaches build --no-cache.

Problem

I hit this when the suite ran as root inside an agent container. Two tests fail there, and only there:

  • default: build --no-cache, THEN a plain down, ... fails with AssertionError: expected -1 to be greater than -1 at expect(buildIdx).toBeGreaterThan(-1)
  • --keep-volumes: a plain down, with no --volumes flag fails with AssertionError: expected undefined to be defined at expect(downLine).toBeDefined()

The stub docker log ends after the two compose config calls and the collision-guard docker ps, with no build and no down. Under uid 0 the script's stderr is Error: CODEMAN_APPDATA_PATH is owned by root: .../appdata.

Cause

Both docker/Update-Codeman.sh and docker/Start-Codeman.sh derive PUID:PGID from the owner of CODEMAN_APPDATA_PATH and exit when that owner is uid 0. That guard is right. The smoke harness makes the directory with mkdirSync, which gives it the runner's uid, so as a normal user it passes and as root it trips the guard. The three other smoke tests are unaffected because they stop at the config or collision checks, which run before the ownership guard.

What it does

When the runner is root (process.getuid?.() === 0), the harness chowns the appdata directory to 99:100, the owner docker/README.md's own chown -R 99:100 example sets up, which is what a real deployment would have. Nothing changes for a non-root runner, including GitHub's ubuntu-latest runners, so CI behaves exactly as before. No script changes.

Verified

All root runs were in a node:22 container started with -u 0 (Node 22.23.4, bash 5.2) with the checkout bind-mounted:

  • Unmodified master: 2 failed, 28 passed. The failures are the two tests above, with exactly those assertions.
  • With this change: 30 passed.
  • With this change but the chownSync line removed: the same 2 failures again.
  • As my normal user with this change: 30 passed, and the chown branch is not taken.
  • Full npm test as my normal user: everything passes except test/tui/tui-switch-guard.test.ts, which fails on my host's tmux 3.2a and has nothing to do with this change (it only touches the docker test).

Neither script's refuse-root branch has a behavioural test today, before or after this change. I left that out to keep this one small, and can follow up with one if you want it.

…data owner

Both Update-Codeman.sh and Start-Codeman.sh derive PUID:PGID from the CODEMAN_APPDATA_PATH owner and refuse a root-owned directory. The smoke harness created that directory with mkdirSync, so under a root test runner (CI or agent containers) it was owned 0:0 and the script exited at that guard before its `build --no-cache` and `down`, failing the default and --keep-volumes smoke tests with an empty build/down log. The guard is correct; the harness now chowns appdata to 99:100 (docker/README.md's own example) when running as root, matching a real deployment.
@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thanks @JDProfresh, and thank you for the careful root-runner measurements. This PR makes the Update-Codeman.sh end-to-end smoke tests pass under a root test runner by giving the harness's appdata directory the non-root owner a real deployment has, instead of the root owner mkdirSync leaves behind.

I reproduced it as real root in a node:22 container: master fails exactly the two tests you named with exactly those assertions, and this branch passes all 30. Non-root and Windows runners take the old path, so CI is unaffected.

One small ask:

  • test/docker-entrypoint.test.ts:318: uid 0 does not always mean the process can chown to 99:100. As root with --cap-drop ALL (which is how Codeman's own Docker case containers are created, src/docker-hosts.ts:550), chownSync throws EPERM inside runSmokeTest, so all five smoke tests fail instead of master's two: the three that stop at the config and collision checks pass on master and now die in harness setup. A single-uid user namespace (unshare -r) gives EINVAL for the same call. Making the chown best-effort keeps the change from ever being worse than master:
if (process.getuid?.() === 0) {
  try {
    chownSync(appdataPath, 99, 100);
  } catch {
    // No CAP_CHOWN, or a single-uid user namespace: leave it root-owned and
    // let the scripts' own guard refuse, exactly as before this change.
  }
}

I ran that version as root with --cap-drop ALL (back to master's two failures) and as root with default capabilities (all Update-Codeman.sh tests pass).

If you push that, great; otherwise I will apply it on top when merging. Thanks again for tracking this down to the ownership guard rather than loosening the scripts.

Root without CAP_CHOWN (--cap-drop ALL, as Codeman's own Docker case containers run) or a single-uid user namespace makes chownSync throw, which failed all five smoke tests in harness setup instead of master's two.
@JDProfresh

Copy link
Copy Markdown
Contributor Author

Thanks, good catch. It's in ee97daa, your version with the comment naming both cases.

I checked it the same way, as root in node:22:

  • --cap-drop ALL: the previous commit fails all five smoke tests with EPERM ... chown '<tmp>/appdata', this one is back to master's two (expected -1 to be greater than -1 and expected undefined to be defined). --cap-drop CHOWN alone gives the same two.
  • Default capabilities: 30 of 30.

@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thanks @JDProfresh for pushing the follow-up so quickly. This PR makes the Update-Codeman.sh smoke tests in test/docker-entrypoint.test.ts pass under a root test runner by giving the harness's appdata directory the non-root 99:100 owner a real deployment has.

ee97daa4 is exactly the best-effort chown I asked for, so there is nothing left to apply on top. I checked all three cases again as real root in a node:22 container: default capabilities pass every Update-Codeman.sh test where master fails two, --cap-drop ALL is back to master's two, and a single-uid user namespace (unshare -r) also matches master instead of the five EINVAL failures of the first commit.

This merges as soon as the CI test job finishes. Thanks again for tracking it down to the ownership guard rather than loosening the scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants