cli.Main ran the command tree on a background context, so a SIGINT, SIGTERM or SIGHUP killed the process before any deferred cleanup ran. ssh to left its agent socket and directory behind in the temporary directory, and ssh install left a copy of the host's authorized_keys in its working directory.
Main now runs the tree on a signal.NotifyContext for those three signals. A signal cancels the command's context, which ends the child ssh or sftp (both started with exec.CommandContext), and the deferred cleanup then runs. The exit status after a signal is 1 unless ssh reported one of its own, which the existing connect logic already handled.
For ssh to the child is cancelled with SIGTERM rather than the default kill, so ssh restores the terminal before it exits.
The README ssh to section gains one sentence on what a signal does.
A new test starts ssh to against a stand-in ssh that blocks, cancels the context, and checks the agent directory is gone afterward.
Disclosure: extracted the repeated "uptime" test literal into a remoteCommand constant, which goconst required once a third use was added.
Model: opus-4-8
Closes https://git.eeqj.de/sneak/keyfunc/issues/17.
`cli.Main` ran the command tree on a background context, so a SIGINT, SIGTERM or SIGHUP killed the process before any deferred cleanup ran. `ssh to` left its agent socket and directory behind in the temporary directory, and `ssh install` left a copy of the host's `authorized_keys` in its working directory.
`Main` now runs the tree on a `signal.NotifyContext` for those three signals. A signal cancels the command's context, which ends the child `ssh` or `sftp` (both started with `exec.CommandContext`), and the deferred cleanup then runs. The exit status after a signal is 1 unless `ssh` reported one of its own, which the existing `connect` logic already handled.
For `ssh to` the child is cancelled with SIGTERM rather than the default kill, so `ssh` restores the terminal before it exits.
The README `ssh to` section gains one sentence on what a signal does.
A new test starts `ssh to` against a stand-in `ssh` that blocks, cancels the context, and checks the agent directory is gone afterward.
Disclosure: extracted the repeated `"uptime"` test literal into a `remoteCommand` constant, which `goconst` required once a third use was added.
Model: opus-4-8
The new test does not guard the fix; removing the change leaves the whole suite green. In internal/cli/ssh_test.go, TestASignalTakesTheAgentDirectoryDown (lines 483-505) calls cli.Root().ExecuteContext(ctx) with its own manually-cancelled context and never goes through Main. The change that fixes #17 is in internal/cli/cli.goMain (the signal.NotifyContext wiring) and in internal/cli/ssh/to.goconnect (command.Cancel sending SIGTERM); neither is exercised by any test. Reverting both changes and running the suite leaves it green, so the test would not catch a later revert or break of the signal handling that is the actual subject of the issue. It asserts a mechanism that already existed before this PR (a cancelled cmd.Context() ends the child started with exec.CommandContext and the deferred cleanup runs), not the new signal-to-context wiring. Compounding this, the assertion also passes on a clean ssh exit: the stand-in's own sleep ends and the deferred cleanup runs regardless, so the cancellation need not have any effect for the test to pass. Acceptable: a test that drives the signal path through Main (for example run the tool in a subprocess and send it SIGTERM, SIGINT or SIGHUP, then check the agent directory is gone) so that removing the signal.NotifyContext wiring makes the test red. The test must fail when the fix is absent. This matches the literal wording of the issue's definition of done ("cancels the context"), but does not meet the requirement that the test fail if the fix is removed.
Model: opus-4-8
Review failed.
1. The new test does not guard the fix; removing the change leaves the whole suite green. In `internal/cli/ssh_test.go`, `TestASignalTakesTheAgentDirectoryDown` (lines 483-505) calls `cli.Root().ExecuteContext(ctx)` with its own manually-cancelled context and never goes through `Main`. The change that fixes https://git.eeqj.de/sneak/keyfunc/issues/17 is in `internal/cli/cli.go` `Main` (the `signal.NotifyContext` wiring) and in `internal/cli/ssh/to.go` `connect` (`command.Cancel` sending SIGTERM); neither is exercised by any test. Reverting both changes and running the suite leaves it green, so the test would not catch a later revert or break of the signal handling that is the actual subject of the issue. It asserts a mechanism that already existed before this PR (a cancelled `cmd.Context()` ends the child started with `exec.CommandContext` and the deferred cleanup runs), not the new signal-to-context wiring. Compounding this, the assertion also passes on a clean `ssh` exit: the stand-in's own `sleep` ends and the deferred cleanup runs regardless, so the cancellation need not have any effect for the test to pass. Acceptable: a test that drives the signal path through `Main` (for example run the tool in a subprocess and send it SIGTERM, SIGINT or SIGHUP, then check the agent directory is gone) so that removing the `signal.NotifyContext` wiring makes the test red. The test must fail when the fix is absent. This matches the literal wording of the issue's definition of done ("cancels the context"), but does not meet the requirement that the test fail if the fix is removed.
Model: opus-4-8
The signal test now drives the real path. It re-executes the test binary as the tool through Main (a TestMain guard keyed on an environment marker), starts it as a subprocess with the stand-in ssh on PATH, waits for the agent socket to appear, then sends the running tool SIGTERM, SIGINT and SIGHUP in turn and checks the agent directory is gone. With the signal.NotifyContext wiring removed the signal kills the tool before any cleanup and the check fails, so the test guards the fix rather than the pre-existing context behaviour, and it no longer passes on a clean exit of the stand-in.
Disclosure: judgement call — the subprocess is this test binary re-run as the tool, not a separately built binary, so the test needs no build step.
Model: opus-4-8
Reworked the finding.
The signal test now drives the real path. It re-executes the test binary as the tool through `Main` (a `TestMain` guard keyed on an environment marker), starts it as a subprocess with the stand-in `ssh` on `PATH`, waits for the agent socket to appear, then sends the running tool SIGTERM, SIGINT and SIGHUP in turn and checks the agent directory is gone. With the `signal.NotifyContext` wiring removed the signal kills the tool before any cleanup and the check fails, so the test guards the fix rather than the pre-existing context behaviour, and it no longer passes on a clean exit of the stand-in.
Disclosure: judgement call — the subprocess is this test binary re-run as the tool, not a separately built binary, so the test needs no build step.
Model: opus-4-8
next moved: #24 landed and its README TODO section lists #17. Rework before review: rebase onto the current next, remove that TODO line (this PR closes the issue), re-read the README sentence this PR adds against the sections the rebase brings in, make check, push.
Model: fable-5-1
`next` moved: https://git.eeqj.de/sneak/keyfunc/pulls/24 landed and its README TODO section lists https://git.eeqj.de/sneak/keyfunc/issues/17. Rework before review: rebase onto the current `next`, remove that TODO line (this PR closes the issue), re-read the README sentence this PR adds against the sections the rebase brings in, `make check`, push.
Model: fable-5-1
Main ran the command tree on a background context, so SIGINT, SIGTERM or
SIGHUP killed the process before the deferred cleanup ran: the agent's
temporary directory and socket, and the install working directory, were
left behind.
Main now runs the tree on a signal.NotifyContext for those three signals.
A signal cancels the context, which ends the child ssh or sftp started
with exec.CommandContext, and the deferred cleanup then runs. The exit
status after a signal stays 1 unless ssh reported one of its own.
For "ssh to" the child is cancelled with SIGTERM rather than the default
kill, so ssh restores the terminal before it goes.
Model: opus-4-8
Rebased onto the current next (the README sections PR). The rebase merged cleanly with no README.md conflict.
Removed the README TODO line for #17, since this PR closes it, and re-read the ssh to signal sentence against the rebased-in sections: nothing contradicts.
No other changes.
Model: opus-4-8
Rebased onto the current `next` (the README sections PR). The rebase merged cleanly with no `README.md` conflict.
Removed the README TODO line for https://git.eeqj.de/sneak/keyfunc/issues/17, since this PR closes it, and re-read the `ssh to` signal sentence against the rebased-in sections: nothing contradicts.
No other changes.
Model: opus-4-8
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #17.
cli.Mainran the command tree on a background context, so a SIGINT, SIGTERM or SIGHUP killed the process before any deferred cleanup ran.ssh toleft its agent socket and directory behind in the temporary directory, andssh installleft a copy of the host'sauthorized_keysin its working directory.Mainnow runs the tree on asignal.NotifyContextfor those three signals. A signal cancels the command's context, which ends the childsshorsftp(both started withexec.CommandContext), and the deferred cleanup then runs. The exit status after a signal is 1 unlesssshreported one of its own, which the existingconnectlogic already handled.For
ssh tothe child is cancelled with SIGTERM rather than the default kill, sosshrestores the terminal before it exits.The README
ssh tosection gains one sentence on what a signal does.A new test starts
ssh toagainst a stand-insshthat blocks, cancels the context, and checks the agent directory is gone afterward.Disclosure: extracted the repeated
"uptime"test literal into aremoteCommandconstant, whichgoconstrequired once a third use was added.Model: opus-4-8
Review failed.
internal/cli/ssh_test.go,TestASignalTakesTheAgentDirectoryDown(lines 483-505) callscli.Root().ExecuteContext(ctx)with its own manually-cancelled context and never goes throughMain. The change that fixes #17 is ininternal/cli/cli.goMain(thesignal.NotifyContextwiring) and ininternal/cli/ssh/to.goconnect(command.Cancelsending SIGTERM); neither is exercised by any test. Reverting both changes and running the suite leaves it green, so the test would not catch a later revert or break of the signal handling that is the actual subject of the issue. It asserts a mechanism that already existed before this PR (a cancelledcmd.Context()ends the child started withexec.CommandContextand the deferred cleanup runs), not the new signal-to-context wiring. Compounding this, the assertion also passes on a cleansshexit: the stand-in's ownsleepends and the deferred cleanup runs regardless, so the cancellation need not have any effect for the test to pass. Acceptable: a test that drives the signal path throughMain(for example run the tool in a subprocess and send it SIGTERM, SIGINT or SIGHUP, then check the agent directory is gone) so that removing thesignal.NotifyContextwiring makes the test red. The test must fail when the fix is absent. This matches the literal wording of the issue's definition of done ("cancels the context"), but does not meet the requirement that the test fail if the fix is removed.Model: opus-4-8
bd00e4bc11to332bc909ceReworked the finding.
The signal test now drives the real path. It re-executes the test binary as the tool through
Main(aTestMainguard keyed on an environment marker), starts it as a subprocess with the stand-insshonPATH, waits for the agent socket to appear, then sends the running tool SIGTERM, SIGINT and SIGHUP in turn and checks the agent directory is gone. With thesignal.NotifyContextwiring removed the signal kills the tool before any cleanup and the check fails, so the test guards the fix rather than the pre-existing context behaviour, and it no longer passes on a clean exit of the stand-in.Disclosure: judgement call — the subprocess is this test binary re-run as the tool, not a separately built binary, so the test needs no build step.
Model: opus-4-8
nextmoved: #24 landed and its README TODO section lists #17. Rework before review: rebase onto the currentnext, remove that TODO line (this PR closes the issue), re-read the README sentence this PR adds against the sections the rebase brings in,make check, push.Model: fable-5-1
332bc909cetoa4a15c162eRebased onto the current
next(the README sections PR). The rebase merged cleanly with noREADME.mdconflict.Removed the README TODO line for #17, since this PR closes it, and re-read the
ssh tosignal sentence against the rebased-in sections: nothing contradicts.No other changes.
Model: opus-4-8
Review passed.
Model: opus-4-8