Skip to content

feat(hooks): run stop-run-checks inside a docker-compose service (dogfood F3) - #91

Merged
tigers1997 merged 3 commits into
mainfrom
feat/container-aware-stop-run-checks
Jun 27, 2026
Merged

feat(hooks): run stop-run-checks inside a docker-compose service (dogfood F3)#91
tigers1997 merged 3 commits into
mainfrom
feat/container-aware-stop-run-checks

Conversation

@tigers1997

Copy link
Copy Markdown
Owner

What

F3 from the container dogfood (deferred from #88/#89). A containerized project's check loop no-op'd because the toolchain lives in the image, not on the host PATH — F2 (#89) warned about it; this makes it run.

Change

A CHECKS entry in stop-run-checks.sh gains an optional 3rd field — label|command|service. When a service is named, the Stop hook runs the check inside it:

  • docker compose exec -T <svc> <cmd> if the service is up, else
  • docker compose run --rm <svc> <cmd> — a throwaway instance of the existing service (no new container, no host rebuild, no path translation).

The host-PATH/manifest guards are bypassed for container checks. Fail-open: no docker/docker compose, or an undefined service → skip silently (stderr note); a non-zero container exit → reported as a normal check FAIL. Two-field label|command entries — including host commands containing a literal | — are unchanged (an entry is container-bound only when it has ≥2 pipes and a bare-token final field, so eslint . | tee log stays host-side). The F2 [ STACK WARNINGS ] note now points at this field.

Tests

New test/stop-run-checks/test-container-checks.sh via a PATH-prepended docker stub: run-vs-exec by service state, skip when compose/service absent, FAIL on non-zero exit, and 2-field stays host-side. configure.py --check clean; 106/106 fixtures; 5/5 persona snapshots (the template content change adds no files).

Scope

One feat (no fix+feat mix, per CONTRIBUTING.md:43). Deferred follow-up: a conditional intake field that detects docker-compose.yml and pre-populates the service per check (this format is the substrate). No tested_up_to / CC_VERSION bump.

🤖 Generated with Claude Code

tigers1997 and others added 2 commits June 26, 2026 21:46
…KS entry names one

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… service field; changelog

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jun 27, 2026

Copy link
Copy Markdown

VERDICT: COMMENT-ONLY

PR adds an optional 3rd label|command|service field to stop-run-checks.sh CHECKS entries so checks can run inside a docker-compose service rather than on the host PATH. The format-parsing heuristic (≥2 pipes + bare-token final field), the compose() availability check, exec-vs-run service-state branching, and the new test suite are all correct. No blocking issues found.

Advisory

docker compose run --rm is missing -T (templates/git-workflow/hooks/stop-run-checks.sh:128)

The exec branch at line 126 explicitly passes -T to disable pseudo-TTY allocation; the run branch at line 128 does not:

run_cmd="$cc exec -T ${service} ${cmd}"   # line 126 — -T present
run_cmd="$cc run --rm ${service} ${cmd}"  # line 128 — -T absent

docker compose run allocates a TTY by default. Claude Code hooks run in a non-interactive context (the test itself passes </dev/null). On Compose v2 < v2.2.0, the missing -T causes a pseudo-TTY to be unconditionally allocated; the "input device is not a TTY" warning is captured by out=$(eval "$run_cmd" 2>&1) and lands in the REPORT even when the check passes. Compose v2.2.0+ auto-detects non-TTY stdin, so this is harmless on modern installs — but adding -T explicitly closes the version gap, matches the exec branch by symmetry, and the test case A assertion (grep -q "run --rm backend") would need to be updated to grep -q "run --rm -T backend" (or -T --rm, depending on preferred flag order) to lock it in.

Test case A grep against a potentially absent $DOCKER_LOG (test/stop-run-checks/test-container-checks.sh:53–55)

run_hook FAKE_SERVICES="backend frontend" FAKE_RUNNING="" >/dev/null
grep -q "run --rm backend" "$DOCKER_LOG" \
  || { echo "FAIL A: ..."; exit 1; }

$DOCKER_LOG is only created when the fake docker's exec|run arm fires. If the hook silently skips for an unexpected reason (e.g. the service-defined check fails before reaching exec/run), $DOCKER_LOG never exists and grep exits 2 with No such file or directory rather than producing the FAIL A: message. A touch "$DOCKER_LOG" inside run_hook (after the rm -f) ensures the file always exists and the assertion failure is the expected one.

compose() probed once per container-bound entry (templates/git-workflow/hooks/stop-run-checks.sh:83–87)

compose() calls docker compose version (a daemon round-trip) each time a container-bound CHECKS entry is encountered. With N container-bound checks, docker compose version runs N times. Hoisting the call before the CHECKS loop — set cc=$(compose) || cc="" once, then [ -z "$cc" ] && { ... ; continue; } inside the loop — eliminates N-1 redundant probes at no cost to readability.

…t-A log assert (PR #91 review)

Three non-blocking review advisories: (1) add -T to the run-branch for
symmetry with exec and to suppress a 'not a TTY' line in the report on
compose < v2.2.0; (2) hoist the compose() probe to a single $CC before the
loop instead of once per container-bound check; (3) touch $DOCKER_LOG in the
test's run_hook so an unexpected skip yields FAIL A, not a grep file-not-found.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jun 27, 2026

Copy link
Copy Markdown

VERDICT: COMMENT-ONLY

PR #91 adds a 3rd |service field to CHECKS entries in stop-run-checks.sh, routing the check through docker compose exec -T or docker compose run --rm -T depending on service state, and bypassing the host-PATH/manifest guards. The pipe-counting + token-regex approach for distinguishing 2-field (host) from 3-field (container) entries is sound; the configure.py warning update and companion test via a fake docker stub are well-integrated. No blocking issues found.

Advisory

configure.py:1083 — warning example is Python-flavored for a stack-agnostic path
The container note always includes e.g. `test|pytest|backend` regardless of which stack triggered it (pnpm, cargo, Go, etc.). A pnpm user who copies the example literally gets pytest in their CHECKS entry instead of their actual test command. A generic placeholder like test|<your-test-cmd>|<service> or a pointer to the hook comment's existing examples would avoid the mismatch.

test/stop-run-checks/test-container-checks.sh:56,62 — negative tests (C, D) pass vacuously on hook crash
run_hook ends with || true, so a hook crash and a correct skip both leave no $DOCKER_MARKER. Tests C ("service undefined → skip") and D ("no compose → skip") assert only that the marker is absent — both pass whether the skip logic fired correctly or the hook exited 1 immediately. Tests A and B provide partial mitigation (they assert docker IS called on the positive path), but checking that $DOCKER_LOG is empty (rather than merely absent) — or asserting the hook's stderr skip message — would make C and D less vacuous.

@tigers1997
tigers1997 merged commit c29786d into main Jun 27, 2026
4 checks passed
@tigers1997
tigers1997 deleted the feat/container-aware-stop-run-checks branch June 27, 2026 13:59
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.

1 participant