fix(test): audit batch 3 — resurrect integration gate, vacuous assertions, installer, build freshness, CI hygiene#164
Merged
Merged
Conversation
Two interlocking bugs made test/integration/session_lifecycle_compose_exec.bats — the empirical gate for REQ-N10 — completely dead: - scripts/test.sh defaulted to `bats test/`, which is non-recursive, so the file was never collected by the default run, ci.yml, or smoke.yml. - The file's guard called `bats_skip_file`, which does not exist in bats-core: without DRYDOCK_INTEGRATION=1 the file hard-errored (127) instead of skipping. Fix: default scripts/test.sh to `bats -r test/` and move the guard into setup_file() using the real bats-core (>= 1.5) whole-file skip mechanism. Verified on bats 1.13.0: without the flag all four L-A..L-D tests report as skips and per-test setup() (which touches the Docker daemon) never runs; with the gate flag set, setup_file falls through and the tests execute. Document the gate in CONTRIBUTING.md and refresh the stale `bats test/` references.
A bare `! cmd` line in a bats test is exempt from bats' errexit handling: unless it is the very last command of the test function, its failure is silently swallowed and the assertion can NEVER fail. A sweep of the test tree (grep '^\s*! ' over test/**/*.bats, classifying each hit by whether the next statement is the function's closing brace) found 7 genuinely vacuous mid-test negations guarding real contracts: - test/cmd_attach.bats (S-2B oneoff guard: no compose exec) - test/egress_smoke.bats (_cleanup: pre-existing networks not created/removed) - test/lib_commands.bats (cmd_sync: /src must not mount HOST_CLAUDE directly) - test/lib_compose.bats x4 (submount managed-block removal + the DRYDOCK_SKIP_ENV_WRITE no-op contract, where BOTH negative greps were mid-test) All 7 are converted to the effective house pattern (`run grep ...; [ "$status" -ne 0 ]`), plus 3 adjacent last-line negations converted alongside for consistency. Also rewrites the D-7 empty-uuid safety assertion in test/cmd_reap.bats, which was tautological even as a last line: mock-docker logs `echo "$*"`, so an empty pkill argument leaves no quote characters in the log and `! grep 'pkill -f ""'` could never match anything. The real failure signature of an empty-pattern pkill is a logged line ending at `pkill -f`; the assertion now matches that. Red-proven: temporarily making _reap_orphan_claude issue `pkill -f ""` fails the new assertion. The ~41 remaining `! cmd` lines in the tree are all effective final-line assertions and are left unchanged.
…clones, defer INV-5 sentinel Three installer fixes: - The rc-file PATH export line was hardcoded to ~/.local/bin while the prompt advertised "$DRYDOCK_BIN_DIR": accepting the prompt with a custom bin dir appended a line that does not put drydock on PATH. The line is now built from DRYDOCK_BIN_DIR — the default keeps the portable literal-$HOME form, a custom dir is embedded verbatim — and the non-interactive hint lines use the same computed line. - Re-running install.sh over an existing clone said "skipping clone" and left the old version in place. The re-run now fast-forwards the install branch (pull --ff-only); a clone on a different branch, detached HEAD, diverged history, or offline downgrades to warn-and-continue without touching local state. - ask_engram_mode wrote the ~/.config/drydock/engram-shared sentinel at question time, BEFORE do_clone/do_symlink: an install aborting after the question left a live INV-5 shared-mode opt-in behind with no drydock installed. The question stays where it was; the write moves to a new write_engram_sentinel step that runs only after the install steps succeed. All driven by test/install.bats (8 red tests before the fix; 35/35 green after), HOME-isolated against local bare-repo fixtures.
…n comment The Dockerfile comment on the github.com ssh-keyscan bake claimed the host keys were "Refreshed on every drydock build" — false. cmd_build passed no --pull or --no-cache, FROM debian:12-slim is an unpinned tag docker build never re-pulls on its own, and the keyscan layer therefore stays cached indefinitely. Minimal, honest fix: - cmd_build now passes --pull to both image builds (base-image freshness; cheap — re-downloads only when the tag's digest changes). - New `drydock build --no-cache` passthrough forwards --no-cache to both builds for a full layer rebuild (e.g. after a GitHub host-key rotation); unknown arguments fail loudly. Help text updated. - The Dockerfile comment now states the actual refresh semantics: cached until the base digest moves or the cache is bypassed with --no-cache. No date-based cache-bust ARG — that would rebuild half the image on every invocation. Driven by 3 red tests in test/lib_commands.bats (169/169 green after).
… widen shellcheck CI hygiene batch: - ci.yml and smoke.yml trigger on pushes to dev as well as main — dev is the integration branch, where two individually-green PRs can land and break it silently until the release PR. - The shellcheck gate now covers test/integration/test_projects_submount.sh (clean — no findings). - bats-core/bats-action is pinned to the commit behind the mutable 3.0.0 tag. - The shfmt v3.8.0 binary download is verified against the official sha256 from the release's sha256sums.txt before being made executable. - The lint, test, and smoke jobs carry an explicit 'permissions: contents: read' block. - CONTRIBUTING.md described the noexec bats-tmp redirect as ~/.cache/drydock/bats-tmp; scripts/test.sh actually uses ~/.bats-tmp. The doc now matches the code — the on-disk path is what container users already have, and relocating it would orphan existing ~/.bats-tmp dirs for no functional gain. Plus the [Unreleased] changelog entries for the audit batch (F1-F5).
jraicr
added
type:fix
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
Batch 3 (infra / test-harness) of the full-codebase audit of the v0.3.3 feature set: five fixes. The bulk of the diff is test coverage and a resurrected integration gate; ~100 lines of product/CI code.
size:exceptionper the convention used for batches 1 (#162) and 2 (#163).F1 — resurrect the dead REQ-N10 integration gate (
8e40c26)Two interlocking bugs made
test/integration/session_lifecycle_compose_exec.bats— the self-described "empirical gate for REQ-N10" — completely unrunnable: (a)scripts/test.shranbats test/, which is NON-recursive, so the file was never collected by the test wrapper,ci.yml, orsmoke.yml; (b) its skip guard calledbats_skip_file, which does not exist in bats-core — withoutDRYDOCK_INTEGRATION=1the file hard-errored (127) instead of skipping. Fixed:scripts/test.shnow collects recursively (bats -r test/); the guard uses the real mechanism (skipinsetup_file(), verified on bats 1.13 to mark all tests skipped AND preventsetup()from touching docker); the gate is documented in CONTRIBUTING.md. Without the flag the 4 gate tests now report as clean skips; with it they run.F2 — neutralize vacuous test assertions (
99c0105)Mid-test
! cmdlines are exempt from bats errexit and can never fail. A full sweep of the 53 candidates found 7 genuinely-vacuous mid-test negations guarding real contracts (cmd_attach, egress_smoke, lib_commands, the lib_composeDRYDOCK_SKIP_ENV_WRITEpair) — converted torun …; [ "$status" -ne 0 ]; 3 adjacent last-line negations converted for consistency; and thecmd_reapempty-uuid safety assertion, which was tautological even as a last line (mock-docker logsecho "$*", so the literalpkill -f ""it grepped for can never appear), rewritten to match the real failure signature (pkill -f[[:space:]]*$) and red-proofed against an injected empty-pattern bug. The remaining ~41 bare!lines are effective final-command assertions, left unchanged.F3 — install.sh: PATH line, clone updates, sentinel ordering (
ec272c1)Three fixes: (a) the appended shell-rc PATH line was hardcoded to
~/.local/binwhile the prompt advertised$DRYDOCK_BIN_DIR— now built from the chosen dir (portable$HOMEform for the default, literal absolute path for a custom one); (b) re-running the installer over an existing clone said "skipping clone" and left the old version — now does an--ff-onlypull when the clone is on the tracking branch, warn-and-continue otherwise (detached/diverged/dirty/offline, with the local HEAD proven untouched); (c) the engram-shared INV-5 opt-in sentinel was written before the clone, so an aborted install left the opt-in behind with no drydock — the write now happens only after clone + symlink succeed.F4 — Dockerfile keyscan honesty +
drydock buildfreshness (2bae3f1)The Dockerfile comment claimed the GitHub host-key bake was "Refreshed on every drydock build" — false, because
cmd_buildpassed no--pull/--no-cacheand the keyscan layer was cached indefinitely.cmd_buildnow always passes--pull(cheap base-image digest check; the only newly-broken case is a fully-warm offline rebuild, which was a no-op anyway), accepts--no-cache(forwarded to both image builds) and a--helpflag, and rejects unknown args. The comment now states the real cache semantics.F5 — CI hygiene (
864f57c)ci.ymlandsmoke.ymlnow also run ondevpushes (two individually-green PRs could otherwise breakdevsilently until the release PR);bats-core/bats-actionpinned to the commit SHA behind tag3.0.0(verified viagit ls-remote); theshfmtbinary download is sha256-verified against the officialv3.8.0release checksum;permissions: contents: readadded to the jobs that lacked it;test/integration/test_projects_submount.shadded to the shellcheck list; the CONTRIBUTING bats-tmp path corrected to the code's actual~/.bats-tmp.Verification
scripts/test.sh: 1250 ok, 0 failures, 11 skips (4 = the REQ-N10 gate skipping cleanly, 7 = pre-existingDRYDOCK_INTEGRATION-gated managed-settings tests).bash -n,shellcheck(full CI list + the newly-added integration.sh),shfmt -d,scripts/lint-commits.sh: clean.drydock build --help, a CHANGELOG### Fixeddedupe, the README build row) are applied in3868ee4.Notes / out of scope
--pullrecommendation kept as-is (no escape hatch): the repo's only protected offline scenario is "image already present, no rebuild", whichensure_imagenever routes throughcmd_build; a knob with no documented demand would violate the §4 minimalism doctrine.smoke.ymlnow builds on everydevpush (~5 min runner time) — an intentional mirror ofmain.