Repository navigation
Various bug fixes + reliability improvements on the Framer fork - #24
Open
ErikBooijFR wants to merge 16 commits into
Open
ErikBooijFR wants to merge 16 commits into
ErikBooijFR wants to merge 16 commits into
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>
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI now merges SSM and inherited environment variables deterministically, passes wrapped-command arguments correctly, preserves execution exit codes, and forwards signals to the child. Tests use the race detector across all packages. Release permissions and action references are more restricted. ChangesRuntime behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant BuildEnvVars
participant MergeEnvVars
participant RunCommand
participant WrappedCommand
CLI->>BuildEnvVars: Build SSM environment entries
CLI->>MergeEnvVars: Merge inherited and SSM environments
MergeEnvVars-->>CLI: Return SSM-preferred environment
CLI->>RunCommand: Start wrapped command
RunCommand->>WrappedCommand: Execute with environment and arguments
WrappedCommand-->>RunCommand: Return status or signal
RunCommand-->>CLI: Return mapped exit status
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 5: Set the go.mod minimum Go version to 1.26.5 using the go directive
rather than relying on the toolchain directive, and update both CI job
Go-version entries to 1.26.5. Ensure all three configuration points consistently
enforce the required crypto/tls security baseline.
In `@main.go`:
- Around line 46-47: Configure the CLI application's OnUsageError handler
alongside the existing validateArgs handling so parser errors return cli.Exit
with errorPrefix(err) and status 125 instead of reaching log.Fatal. Update
main.go accordingly; README.md lines 147-153 document the expected behavior and
require no direct change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d229ca6e-8b0b-42af-9e99-4d40664e7762
📒 Files selected for processing (9)
.github/workflows/ci.ymlMakefileREADME.mddata.godata_test.gogo.modmain.gomain_test.gorunner.go
README documents exit code 125 for invalid usage, and validateArgs errors honor that, but a malformed flag never reaches the action: cli.Command.Run returns the parse error to main, where log.Fatal exits with status 1, breaking the documented contract. Set OnUsageError to route parse errors through the same cli.Exit(..., 125) path as other usage errors. This also reports the error once, in the ERROR: prefix style, instead of twice: previously cli printed "Incorrect Usage" plus a full help dump and logrus then repeated the message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The intent of requiring 1.26.5 (the crypto/tls fix for GO-2026-5856) was expressed as go 1.26 plus a toolchain directive, but the toolchain directive is only a suggestion: it is honored under the default GOTOOLCHAIN=auto and silently ignored under GOTOOLCHAIN=local, where a 1.26.0 toolchain would build a binary without the fix. The go directive is the mechanism that actually enforces a floor - an older toolchain either auto-upgrades or fails loudly. Raise the go directive to 1.26.5 and drop the now-redundant toolchain line. The module is not importable as a library, so the stricter directive burdens no dependents. CI stays on go-version: '1.26', which setup-go resolves to the latest 1.26.x patch, so release builds keep picking up future security fixes without manual bumps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
|
Hey @gmr, just checking in if there's something I can do to move this forward 🙏 |
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.
See #23
These are the changes we've implemented on our fork, after an automated audit of the codebase. Each individual commit, contains reasoning/explanation on the what and why.
If there are any you wish to opt-out of, feel free to let me know, and I'll drop them. Same if you'd prefer I split them into separate PRs (in that case, please let me know how you'd like them split).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
env-aws-paramsexit codes.