Repository navigation
fix(runner): classify every Docker failure once in the client - #73
Merged
Merged
Conversation
added 3 commits
October 4, 2026 22:37
The Docker client now attaches one classified failure to every result (not-found, permission, daemon-down, cli-missing, timeout, cancelled, other; null on exit 0), from the one classifier in failure.ts. The ops branch on that field: the tolerated failures are not-found outcomes and stderr only goes into the message, so ops.ts matches no stderr pattern. An image inspect pulls only on not-found and stops on any other failure. A discovery failure reaches the health check as the cli-missing value instead of discovery prose to match, and health maps each class to its wire problem name. A client-only `docker info` answer runs its engine errors through the same classifier. The client default, the health timeout, the stop grace, and the login-shell probe join the ops timeouts in one TIMEOUTS table, a leaf module (timeouts.ts) every Docker module reads without an import cycle. The ops and health tests feed classified results; one classifier test owns the patterns and drives the real client against a scripted CLI. Closes #66
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.
Intent
Deliver puck issue #66, "runner: one Docker failure classifier" (#66). The Docker client classifies every result once (not found, permission, daemon down, CLI missing, timeout, other) so the ops branch on a field instead of matching stderr, the health check consumes the same classification instead of matching the discovery module's prose, and the three timeout constants become the one table. Acceptance criteria: no stderr regular expression remains in the ops module, and every tolerated failure is a classified outcome; a discovery failure reaches the health check as a value, not a message, and the literal message assertion in the health test is gone; all Docker timeouts are read from the one table; the ops and health tests feed classified results, and one classifier test owns the patterns. Closes #66.
What Changed
src/puck-runner/docker/failure.tsholds the only stderr patterns (classifyStderr). The Docker client now setsDockerResult.failureon every result to one ofnot-found,permission,daemon-down,cli-missing,timeout, orother. A failed CLI discovery becomescli-missing, and the oldtimedOut/abortedflags are removed. An exit 0 that races the timeout guard or the abort signal now counts as a success.ops.ts) no longer match stderr. They checkfailure, and only anot-foundresult is tolerated (missing container, volume or image when inspecting or removing). One behavior change:docker image inspectused to try a pull after any failure. Now it pulls only onnot-foundand stops with an "Inspecting image" error for anything else, such as the Docker daemon being down. The health check (health.ts) mapsfailureto its existingDockerProblemwire names instead of matching the discovery module's error text.ServerErrorsfromdocker infogo through the same classifier.TIMEOUTStable in a new leaf module,timeouts.ts: the client default,docker info, every op, thedocker stopgrace, and the login-shell probe in discovery.AGENTS.mdand the line reference in the delivery spec now point to it. A newrunner-docker-failure.test.tsowns the stderr patterns. The ops and health tests (and the other tests that buildDockerResults) now pass classified results, and the ops tests also check each command's timeout against the table.Closes #66.
🤖 Generated with Claude Code
Risk Assessment
✅ Low: A contained refactor of the runner's Docker module: every result is classified once in the client, ops and health branch on the field with behavior equivalent to the old stderr checks (the one intentional change, failing fast when an image inspect fails for a reason other than not-found, is tested), all Docker timeouts come from one table, every test fake sets the new field consistently, and the new tests exercise a real scripted CLI rather than source text.
Testing
I built the runner bundle and drove its real
configcommand against the local Docker 29.5.2 engine. I changed the Docker state for each health class, and the user saw the right message for every one: healthy (version, CPUs and memory), CLI missing (discovery's own explanation), daemon down, socket permission, and timeout after 20 s. I also drove DockerOps with the real client through a temporary probe, since removed. On a healthy engine it treated a missing container, volume or image as absent (not-found). With the engine down it raised the daemon-down error and never tried a pull. Transcripts are in the evidence directory. The real-Docker runner suite couldn't run because this engine hosts the user's own running Puck environment, which the suite refuses to touch. The three targeted unit test files pass. I removed the build output, temp dirs and the probe file afterwards.Evidence: Runner config.sh Docker health transcript across 7 Docker states
Evidence: Live DockerOps probe: real docker results, their classification, and ops outcomes
Evidence: Live DockerOps probe run
Evidence: Real-Docker runner suite (blocked by a foreign Puck environment on this engine)
Evidence: Health check results by Docker state
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/puck-runner/docker/failure.ts:19- The intent enumerates six failure classes: "not found, permission, daemon down, CLI missing, timeout, other".failure.tsadds a seventh,cancelled(also set at client.ts:105 and client.ts:148, and mapped at health.ts:26). No consumer in src reads it (control.ts:350 maps every DockerError to the generic 'docker' code; the oldabortedflag had no reader either), and the intent's own 'other' class ("anything else; stderr carries the detail") would cover an aborted command with stderr 'cancelled'. It is a deliberate, low-stakes extension that preserves the distinction the removedabortedflag carried, so this is the author's call: either keep it as an intentional addition or fold it into 'other'. Removing it is the smaller change.test/unit/runner-docker-health.test.ts:110- The acceptance criterion says one classifier test owns the stderr patterns, but the health test's client-only case still asserts that the literal 'Cannot connect to the Docker daemon' (fed throughServerErrors) yields problem 'daemon-down', so a pattern change in failure.ts would break two tests. This is defensible as a behavior test of the ServerErrors routing in classifyInfo (health.ts:63-64) rather than of the pattern, and there is no way to prove the routing without some classifiable text; noting it as a known overlap, no action needed.🔧 Fix applied.
2 infos still open:
test/unit/runner-docker-health.test.ts:110- The acceptance criterion says one classifier test owns the stderr patterns, but the health test's client-only case still asserts that the literal 'Cannot connect to the Docker daemon' (fed throughServerErrors) yields problem 'daemon-down', so a pattern change in failure.ts would break two tests. This is defensible as a behavior test of the ServerErrors routing in classifyInfo (health.ts:63-64) rather than of the pattern, and there is no way to prove the routing without some classifiable text; noting it as a known overlap, no action needed.src/puck-runner/docker/ops.ts:128- Behavior change beyond a pure refactor, consistent with the intent's "every tolerated failure is a classified outcome":image()used to treat anydocker image inspectfailure as "not cached" and fall through todocker pull; it now tolerates onlynot-foundand throwsInspecting image <img>: ...for anything else (daemon-down, permission, timeout, other). Concrete trace: an engine that is busy enough forimage inspectto hit the 20 s guard (TIMEOUTS.inspect) now aborts the create with a timeout error, where before the 30-minutepullwould have been attempted and likely succeeded. The failure is loud, classified, and covered by the new ops test ("pulls only an image that is not found, and stops on any other inspect failure"), so this is an acknowledged tradeoff rather than a defect.src/puck-runner/docker/failure.ts:23- Existing behaviour, not caused by this change (base health.ts had the same patterns): if DOCKER_HOST points at a missing socket whose filename isn't docker.sock, the health check reports 'unknown' instead of 'daemon-down'. Docker 29 words this error as 'failed to connect to the docker API … if the daemon is running', which no daemon-down pattern matches. The default socket paths (Docker Desktop, colima, /var/run) all end in docker.sock and classify correctly.npm run build:runner, thennode .webpack/runner/puck-runner.cjs config --url http://127.0.0.1:9 --token PRT_… --unattendedwith Docker healthy,PUCK_DOCKER_BIN=/nonexistent/docker,DOCKER_HOST=unix://…/gone/docker.sock,DOCKER_HOST=tcp://127.0.0.1:9,DOCKER_HOST=unix://…/missing.sock, a mode-000docker.sock, and adocker.sockthat accepts connections but never answersTemporary live probe (removed afterwards) that runsDockerOpswithrealDockeron Docker 29.5.2: raw classification of inspect, rm and info results;state,volumesPresentanddeleteof a missing environment;image()for a missing and a present image; the same operations with the engine down;list()npx vitest run --config vitest.docker.config.ts test/docker/runner.test.ts(could not run on this host: the engine hosts a Puck environment the suite did not create)npx vitest run test/unit/runner-docker-failure.test.ts test/unit/runner-docker-health.test.ts test/unit/runner-docker-ops.test.ts✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.