AI-driven round of code fixes - #4
Merged
Merged
Conversation
The forwarding goroutine did a single blocking channel read, forwarded one signal, and exited. signal.Notify stays registered for the life of the process, so every later SIGTERM/SIGINT was still intercepted by the runtime but never read: it neither reached the child nor terminated the wrapper. A second Ctrl-C, or a supervisor escalating its shutdown request, was silently swallowed until SIGKILL. Drain the channel in a loop instead, and grow the buffer from 1 to 32: signal.Notify sends without blocking and drops signals when the buffer is full, so bursts could otherwise be lost while a forward is in flight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kill(-getpid()) targets the process group whose ID equals the wrapper's own PID. That group only exists when the wrapper happens to be a group leader, e.g. PID 1 in a container or a job started by an interactive shell. Launched from a shell script or a supervisor, the kill failed with ESRCH and the child never received the signal at all. Where the group did exist, terminal-generated signals arrived twice (once from the kernel to the foreground group, once from us), and each forward re-signalled the wrapper itself. The log reported -cmd.Process.Pid, a group that never existed, while the code signalled a different one. Use cmd.Process.Signal to address exactly the process we started; it behaves identically in every launch context and delivers exactly once. Propagating further down the tree is the child's responsibility, the same contract as tini's default behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
signal.Notify ran after cmd.Start, so a signal arriving between the two killed the wrapper via default disposition and orphaned the just-started child. Supervisors that start and almost immediately stop a service (crash loops, instant rollbacks) can hit this window. Register interception before starting the child. Signals arriving before the forwarding goroutine is up queue in the buffered channel and are delivered once the child runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only SIGHUP, SIGINT, SIGTERM and SIGQUIT were forwarded. Any other signal, e.g. SIGUSR1 to rotate logs or SIGWINCH on resize, killed the wrapper through its default disposition and orphaned the child. An entrypoint wrapper should be transparent: whatever an operator sends to the visible PID must reach the application behind it. Subscribe to every catchable signal and forward, with two exceptions: SIGCHLD is addressed to the wrapper about its own child, and SIGURG is used continuously by the Go runtime for goroutine preemption. Demote the per-forward log line to debug; with SIGWINCH forwarded, a terminal resize would otherwise spam info-level logs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A signal-killed child has no exit code, so ExitCode() returns -1 and os.Exit truncated it to 255, collapsing SIGTERM, SIGKILL and SIGSEGV into one indistinguishable value. Translate to the shell convention of 128+N: 143 for SIGTERM, 137 for SIGKILL, the code supervisors and container tooling already interpret (137 is the well-known OOM-kill signature). Voluntary exits still pass through unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
urfave/cli v3 parses flags interspersed with positional arguments, so flags belonging to the wrapped command were claimed by the wrapper: "env-aws-params --prefix /x bash -c set" failed with "flag provided but not defined: -c" unless callers inserted "--" by hand. Set StopOnNthArg to 1: parsing stops at the first positional argument, and everything after the command passes through verbatim, matching how env(1), sudo and docker run treat their command tails. Wrapper flags must consequently appear before the command; trailing flags now belong to the child. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wrapper-side failures exited with 1 or 2 (validation), 255 (Parameter Store errors, via -1) and 128 (spawn failures). All of these collide with codes real children use, or with the 128+N signal range, so callers could not tell "the app failed" from "the plumbing failed". Adopt the shell and Docker convention: 125 for wrapper-side errors, 126 for a command that was found but could not be started, 127 for a command that was not found. exec.ErrNotFound covers failed PATH lookups; os.ErrNotExist covers explicit paths. A child may still exit with these codes itself; that ambiguity is inherent to the convention. validateArgs now returns only an error since both validation failures map to 125. Document the whole contract in the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The --sanitize, --strip and --upcase transforms are lossy: db-host and db_host both become DB_HOST. Both entries were emitted, and the effective winner was picked by os/exec's keep-last deduplication over a slice sorted as whole "KEY=value" strings, i.e. by lexical order of the values. Rotating a value could silently flip which parameter won. Iterate parameters in sorted-name order into a map keyed by the final env key: exactly one entry per key is emitted and the parameter whose name sorts last wins, independent of values. Collisions log a warning naming both parameters, the contested key and the winner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Builds that skip the -X ldflag, e.g. a plain go build or go install, printed an empty string for --version. The linker flag overwrites the initial value, so stamped release builds are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
govulncheck reports the Encrypted Client Hello privacy leak in crypto/tls (GO-2026-5856) as reachable from this binary through the SSM client's TLS calls. The fix ships with the toolchain rather than a module, so the floor is expressed as a toolchain directive: any Go since 1.21 fetches 1.26.5 automatically, and GOTOOLCHAIN=local fails loudly instead of silently producing a vulnerable binary. CI floats on 1.26.x and picks the patch up by itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
contents: write applied to the whole workflow, handing a repo-write token to the test and build jobs. Those run on every push and pull request and execute the most third-party code, yet only need to read the repository. Default the workflow token to contents: read and grant write solely to the release job, which creates GitHub Releases. A compromised action or dependency in the hot path can then no longer push commits, move tags or forge releases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A version tag is a mutable pointer: whoever controls, or compromises, softprops/action-gh-release can repoint v3, and every workflow using it executes the new code on its next run. This is exactly how the tj-actions/changed-files compromise spread (CVE-2025-30066). The action is third-party, runs in the only job holding contents: write, and uploads the release binaries users download directly, so a hijacked version could replace them. Pin to the commit v3.0.2 resolves to; the trailing comment keeps the version readable and lets Dependabot propose bumps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI runs go test -race ./... while make test ran a bare go test, so data races surfaced only in CI. Align the Makefile with CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The image publishing workflow is already gone and nothing builds or distributes this image any more; binaries ship through GitHub Releases. Drop the Dockerfile and the README's local image build instructions accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ErikBooijFR
force-pushed
the
fix/fable-driven-fixes
branch
from
August 10, 2026 11:45
285b8ba to
2b3ea92
Compare
ErikBooijFR
marked this pull request as ready for review
August 10, 2026 11:51
ankon
approved these changes
Aug 10, 2026
ankon
left a comment
ankon
left a comment
There was a problem hiding this comment.
These are amazing fixes!
For upstreaming them we may need to check with the author whether they want one PR per fix, and the usual discussion about AI-authored code.
|
FWIW: Not all of the fixes would apply in our environment, but they all look like they would be "very nice to have". |
Collaborator
Author
Yeah, agree, I think I'll try to get in touch with him before yeeting (a) PR(s) his way. |
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
2 participants
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.
Fable found a list of of things that could use a little improvement. Some nits, some in the realm of making it more "technically correct", some probably unintended bugs.
Best reviewed commit-by-commit.