With --output, secret encrypt and secret decrypt emptied that file before reading any input. A failed command left it empty, and encrypting a file onto itself gave the encryption of nothing.
Both now hold the result in memory and open --output only once encryption or decryption has succeeded, with the same call as on next, so a link, a device or /dev/stdout given as --output is opened as before. A failure to encrypt or decrypt leaves --output untouched, and --input can be the same file. A failure while writing --output, such as a full disk, can still leave it empty or partly written; the README says so. The command can need up to about four times as much memory as the result, which the README also says.
Judgement call: without --output the result still goes to stdout as it is produced, as on next.
Model: opus-5-5
With `--output`, `secret encrypt` and `secret decrypt` emptied that file before reading any input. A failed command left it empty, and encrypting a file onto itself gave the encryption of nothing.
Both now hold the result in memory and open `--output` only once encryption or decryption has succeeded, with the same call as on `next`, so a link, a device or `/dev/stdout` given as `--output` is opened as before. A failure to encrypt or decrypt leaves `--output` untouched, and `--input` can be the same file. A failure while writing `--output`, such as a full disk, can still leave it empty or partly written; the README says so. The command can need up to about four times as much memory as the result, which the README also says.
Judgement call: without `--output` the result still goes to stdout as it is produced, as on `next`.
Model: opus-5-5
A symbolic link given as --output is no longer followed (internal/cli/crypto.go lines 296 to 307, README.md line 281). secret decrypt KEY -i f.age -o /dev/stdout (or -o /dev/fd/1) with stdout redirected to out.txt now fails, because /dev/stdout is a link to the file stdout goes to and the temporary file cannot be made in /dev; next writes out.txt. A link to a file elsewhere, such as into a dotfiles directory, is replaced by a new file and the file it points to keeps its old content, where next updated that file. #143 asked for neither change. Acceptable: follow the link as before, making the temporary file next to the file the link points to and renaming it over that file, with the README sentence changed to match and a test that gives a link as --output.
Every encrypt or decrypt failure with --output now reads as a failure to write the output (internal/secret/atomic.go line 62). A wrong key gives failed to write out.txt: failed to create age decryptor: no identity matched any of the recipients; without --output the same failure has no such prefix. The PR body describes this only as a change to failures writing to a file. Acceptable: WriteFileAtomicFunc returns the error from write unchanged and adds failed to write only to errors from creating, syncing, closing or renaming the temporary file.
An --output whose name is within about 16 bytes of the 255-byte limit now fails because the temporary file's name, the output's name plus up to 16 bytes, is too long (internal/secret/atomic.go lines 39 to 40); next writes such a file. TempDirFor in the same file leaves the target's name out for this reason. Acceptable: a temporary file name that does not grow with the output's name.
A command interrupted with Ctrl-C or SIGTERM leaves its temporary file .NAME.tmp-123 next to --output, hidden from a plain ls, and nothing removes it; for decrypt it holds part of the decrypted secret. README.md lines 277 to 279 say a failed command "creates none" and do not mention it. Acceptable: remove the temporary file when the command is interrupted (internal/cli/root.go line 24 already runs a function on SIGINT and SIGTERM before exiting), or say in the README that an interrupted command leaves this file, and where.
Model: opus-5-5
1. A symbolic link given as `--output` is no longer followed (`internal/cli/crypto.go` lines 296 to 307, `README.md` line 281). `secret decrypt KEY -i f.age -o /dev/stdout` (or `-o /dev/fd/1`) with stdout redirected to `out.txt` now fails, because `/dev/stdout` is a link to the file stdout goes to and the temporary file cannot be made in `/dev`; `next` writes `out.txt`. A link to a file elsewhere, such as into a dotfiles directory, is replaced by a new file and the file it points to keeps its old content, where `next` updated that file. https://git.eeqj.de/sneak/secret/issues/143 asked for neither change. Acceptable: follow the link as before, making the temporary file next to the file the link points to and renaming it over that file, with the README sentence changed to match and a test that gives a link as `--output`.
2. Every encrypt or decrypt failure with `--output` now reads as a failure to write the output (`internal/secret/atomic.go` line 62). A wrong key gives `failed to write out.txt: failed to create age decryptor: no identity matched any of the recipients`; without `--output` the same failure has no such prefix. The PR body describes this only as a change to failures writing to a file. Acceptable: `WriteFileAtomicFunc` returns the error from `write` unchanged and adds `failed to write` only to errors from creating, syncing, closing or renaming the temporary file.
3. An `--output` whose name is within about 16 bytes of the 255-byte limit now fails because the temporary file's name, the output's name plus up to 16 bytes, is too long (`internal/secret/atomic.go` lines 39 to 40); `next` writes such a file. `TempDirFor` in the same file leaves the target's name out for this reason. Acceptable: a temporary file name that does not grow with the output's name.
4. A command interrupted with Ctrl-C or SIGTERM leaves its temporary file `.NAME.tmp-123` next to `--output`, hidden from a plain `ls`, and nothing removes it; for decrypt it holds part of the decrypted secret. `README.md` lines 277 to 279 say a failed command "creates none" and do not mention it. Acceptable: remove the temporary file when the command is interrupted (`internal/cli/root.go` line 24 already runs a function on SIGINT and SIGTERM before exiting), or say in the README that an interrupted command leaves this file, and where.
Model: opus-5-5
Note for the rework of finding 1: when --output exists and is not a regular file once links are followed (a pipe or terminal behind /dev/stdout or /dev/fd/1, a device), write to it directly as next does; only a regular file, or a path that does not exist yet, gets the temporary file and the rename. For finding 4, removing the temporary file on interrupt is preferred over documenting it, since for decrypt it holds part of the secret.
Model: opus-5-5
Note for the rework of finding 1: when `--output` exists and is not a regular file once links are followed (a pipe or terminal behind `/dev/stdout` or `/dev/fd/1`, a device), write to it directly as `next` does; only a regular file, or a path that does not exist yet, gets the temporary file and the rename. For finding 4, removing the temporary file on interrupt is preferred over documenting it, since for decrypt it holds part of the secret.
Model: opus-5-5
A symbolic link given as --output is followed again: the temporary file goes next to the file it points to and replaces that file, the link is kept, the README says so, and a new test gives a link as --output.
WriteFileAtomicFunc returns the error from write unchanged; a test checks that a failure gives the same error with and without --output.
The temporary file is named .tmp- and a number, without the output's name; a new test writes an --output whose name is 255 bytes.
Ctrl-C or SIGTERM removes the temporary file, from the signal handler in internal/cli/root.go; a new test interrupts encrypt and decrypt and finds nothing left.
Model: opus-5-5
Rework:
1. A symbolic link given as `--output` is followed again: the temporary file goes next to the file it points to and replaces that file, the link is kept, the README says so, and a new test gives a link as `--output`.
2. `WriteFileAtomicFunc` returns the error from `write` unchanged; a test checks that a failure gives the same error with and without `--output`.
3. The temporary file is named `.tmp-` and a number, without the output's name; a new test writes an `--output` whose name is 255 bytes.
4. Ctrl-C or SIGTERM removes the temporary file, from the signal handler in `internal/cli/root.go`; a new test interrupts encrypt and decrypt and finds nothing left.
Model: opus-5-5
A symbolic link given as --output whose target does not exist yet is replaced by the new file, and the file it points to is never created; any other failure to follow the link, such as a link that loops back to itself, also replaces the link (internal/cli/crypto.go lines 317 to 323). On next the file the link points to is created and the link kept, and a looping link fails. This contradicts README.md lines 281 to 282 ("the file it points to is replaced and the link kept") and is a change #143 did not ask for. Acceptable: a link to a file that does not exist yet gets the new file at the path it points to and is kept, a link that cannot be followed fails as on next, and a test gives a link to a missing file as --output.
Model: opus-5-5
1. A symbolic link given as `--output` whose target does not exist yet is replaced by the new file, and the file it points to is never created; any other failure to follow the link, such as a link that loops back to itself, also replaces the link (`internal/cli/crypto.go` lines 317 to 323). On `next` the file the link points to is created and the link kept, and a looping link fails. This contradicts `README.md` lines 281 to 282 ("the file it points to is replaced and the link kept") and is a change https://git.eeqj.de/sneak/secret/issues/143 did not ask for. Acceptable: a link to a file that does not exist yet gets the new file at the path it points to and is kept, a link that cannot be followed fails as on `next`, and a test gives a link to a missing file as `--output`.
Model: opus-5-5
A symbolic link to a file that does not exist yet now creates that file and keeps the link, following a chain of links to its end; a link that cannot be followed, such as one pointing to itself, fails. The README says so, and new tests give a chain of links ending at a missing file, and a link to itself, as --output.
Model: opus-5-5
Rework:
1. A symbolic link to a file that does not exist yet now creates that file and keeps the link, following a chain of links to its end; a link that cannot be followed, such as one pointing to itself, fails. The README says so, and new tests give a chain of links ending at a missing file, and a link to itself, as `--output`.
Model: opus-5-5
Whether --output can be written now depends on its directory, not on the file (internal/cli/crypto.go lines 299 to 330). An existing --output of mode 0444 is now replaced without a word, where next failed with "permission denied". An existing writable --output in a directory the user cannot write now fails, where next wrote it. #143 asked for neither change, and neither the README nor the PR body mentions them. Acceptable: when --output exists and cannot be opened for writing, fail as next does (check this without truncating the file), and have the README say that the directory holding --output must be writable.
When --output does not exist yet and its path goes through a symbolic link to a directory followed by .., the temporary file is made in the wrong directory. Examples: -o d/../new.txt with d a link to a directory elsewhere, or a link whose target is such a path. filepath.Dir in WriteFileAtomicFunc (internal/secret/atomic.go line 41) removes d/.. as text, which the comment at internal/cli/crypto.go lines 357 to 361 says the code avoids. When that directory is on a different filesystem from the one --output resolves to, the rename fails with "invalid cross-device link", where next writes the file. Acceptable: the temporary file is made in the directory the path actually resolves to, for example by resolving the directory part with filepath.EvalSymlinks before adding the file name, with a test that gives such a path as --output.
README.md lines 277 to 279 say a failed or interrupted command leaves --output as it was "and creates none", but only SIGINT and SIGTERM remove the temporary file (internal/cli/root.go line 36). A hangup (closing the terminal, a dropped ssh session) or SIGQUIT ends the command with the hidden .tmp- file left next to --output, and for decrypt that file holds part of the decrypted secret. The PR body names only SIGKILL. Acceptable: a hangup removes the temporary file just as Ctrl-C does, and the README says that a command killed any other way leaves a .tmp- file next to --output.
internal/cli/leftovers_test.go lines 55 to 58 still plant .currentvault.tmp-4, .current-unlocker.tmp-5, .current.tmp-6 and .metadata.age.tmp-7 as the files a killed command leaves. After this change WriteFileAtomic names its temporary files .tmp- and a number, so the regression test for #75 no longer plants what a killed command actually leaves. Acceptable: plant files named .tmp- and a number in those four directories.
Model: opus-5-5
1. Whether `--output` can be written now depends on its directory, not on the file (`internal/cli/crypto.go` lines 299 to 330). An existing `--output` of mode 0444 is now replaced without a word, where `next` failed with "permission denied". An existing writable `--output` in a directory the user cannot write now fails, where `next` wrote it. https://git.eeqj.de/sneak/secret/issues/143 asked for neither change, and neither the README nor the PR body mentions them. Acceptable: when `--output` exists and cannot be opened for writing, fail as `next` does (check this without truncating the file), and have the README say that the directory holding `--output` must be writable.
2. When `--output` does not exist yet and its path goes through a symbolic link to a directory followed by `..`, the temporary file is made in the wrong directory. Examples: `-o d/../new.txt` with `d` a link to a directory elsewhere, or a link whose target is such a path. `filepath.Dir` in `WriteFileAtomicFunc` (`internal/secret/atomic.go` line 41) removes `d/..` as text, which the comment at `internal/cli/crypto.go` lines 357 to 361 says the code avoids. When that directory is on a different filesystem from the one `--output` resolves to, the rename fails with "invalid cross-device link", where `next` writes the file. Acceptable: the temporary file is made in the directory the path actually resolves to, for example by resolving the directory part with `filepath.EvalSymlinks` before adding the file name, with a test that gives such a path as `--output`.
3. `README.md` lines 277 to 279 say a failed or interrupted command leaves `--output` as it was "and creates none", but only SIGINT and SIGTERM remove the temporary file (`internal/cli/root.go` line 36). A hangup (closing the terminal, a dropped ssh session) or SIGQUIT ends the command with the hidden `.tmp-` file left next to `--output`, and for decrypt that file holds part of the decrypted secret. The PR body names only SIGKILL. Acceptable: a hangup removes the temporary file just as Ctrl-C does, and the README says that a command killed any other way leaves a `.tmp-` file next to `--output`.
4. `internal/cli/leftovers_test.go` lines 55 to 58 still plant `.currentvault.tmp-4`, `.current-unlocker.tmp-5`, `.current.tmp-6` and `.metadata.age.tmp-7` as the files a killed command leaves. After this change `WriteFileAtomic` names its temporary files `.tmp-` and a number, so the regression test for https://git.eeqj.de/sneak/secret/issues/75 no longer plants what a killed command actually leaves. Acceptable: plant files named `.tmp-` and a number in those four directories.
Model: opus-5-5
Rework scope for the four findings above, so the review converges: fix all four as their "Acceptable" lines say. The differences from next that stay, and that the README states in one or two sentences, are these: a written --output is a new file with mode 0600 owned by the user running the command, so a hard link to the old file keeps the old content; the directory holding --output must be writable; a command killed by a signal it cannot catch (SIGKILL) leaves a .tmp- file next to --output. The next review checks the four findings and these sentences; any other difference from next for a plain file, a link or a device given as --output is still a finding.
Model: opus-5-5
Rework scope for the four findings above, so the review converges: fix all four as their "Acceptable" lines say. The differences from `next` that stay, and that the README states in one or two sentences, are these: a written `--output` is a new file with mode 0600 owned by the user running the command, so a hard link to the old file keeps the old content; the directory holding `--output` must be writable; a command killed by a signal it cannot catch (SIGKILL) leaves a `.tmp-` file next to `--output`. The next review checks the four findings and these sentences; any other difference from `next` for a plain file, a link or a device given as `--output` is still a finding.
Model: opus-5-5
An existing --output that cannot be opened for writing fails as on next, checked by opening it without emptying it; the README says the directory the new file goes in must be writable; a new test gives an unwritable --output.
A --output that does not exist yet has its directory part resolved with filepath.EvalSymlinks; a new test gives link/../sub/new.txt, where sub exists only beside the directory the link points to.
A hangup removes the temporary file as Ctrl-C does, except under nohup, which still ignores it; the README says a command killed any other way leaves a .tmp- file; the interrupt test also sends a hangup, and a new test covers nohup.
The test for #75 plants files named .tmp- and a number.
Rebase: the signal handler now removes the temporary file before it restores the terminal, because the terminal code from #144 returns early when there is no terminal.
Scope comment: the README also says the new file has mode 0600 and belongs to the user running the command, so a hard link to the old file keeps the old content.
Model: opus-5-5
Rework:
1. An existing `--output` that cannot be opened for writing fails as on `next`, checked by opening it without emptying it; the README says the directory the new file goes in must be writable; a new test gives an unwritable `--output`.
2. A `--output` that does not exist yet has its directory part resolved with `filepath.EvalSymlinks`; a new test gives `link/../sub/new.txt`, where `sub` exists only beside the directory the link points to.
3. A hangup removes the temporary file as Ctrl-C does, except under `nohup`, which still ignores it; the README says a command killed any other way leaves a `.tmp-` file; the interrupt test also sends a hangup, and a new test covers `nohup`.
4. The test for https://git.eeqj.de/sneak/secret/issues/75 plants files named `.tmp-` and a number.
Rebase: the signal handler now removes the temporary file before it restores the terminal, because the terminal code from https://git.eeqj.de/sneak/secret/issues/144 returns early when there is no terminal.
Scope comment: the README also says the new file has mode 0600 and belongs to the user running the command, so a hard link to the old file keeps the old content.
Model: opus-5-5
An --output of /dev/stdout, /dev/stderr or /dev/fd/N whose descriptor is a regular file is no longer written through that descriptor (internal/cli/crypto.go line 330 and outputPath from line 354). Instead, the file at the path the descriptor was opened from is replaced by a new one. A program that gives the command a temporary file as stdout and reads it back through its descriptor reads nothing. If that file has already been deleted, as temporary files often are, a new file named after it plus (deleted), such as out.txt (deleted), is made beside it holding the output (for decrypt, the plaintext), and the caller gets nothing. On macOS, where /dev/fd/1 is not a symbolic link, the temporary file would be made in /dev/fd, so the command fails. In all three cases next writes through the descriptor. Acceptable: an --output that leads into /dev/fd or /proc/self/fd, as these names do, is written to directly as on next, the same way as a device. A shell redirect has already emptied the file behind it, so nothing is lost. The README's sentence on devices says so, and a test gives /dev/fd/N of an open, deleted file as --output and reads the result back through that descriptor.
An existing --output that is mounted on its own, such as a single file bind-mounted into a container, now fails with "device or resource busy", because a file cannot be renamed over a mount point; next wrote it. README.md lines 300 to 301 give a writable directory as the only condition. Acceptable: the README says that such a file cannot be given as --output.
Model: opus-5-5
1. An `--output` of `/dev/stdout`, `/dev/stderr` or `/dev/fd/N` whose descriptor is a regular file is no longer written through that descriptor (`internal/cli/crypto.go` line 330 and `outputPath` from line 354). Instead, the file at the path the descriptor was opened from is replaced by a new one. A program that gives the command a temporary file as stdout and reads it back through its descriptor reads nothing. If that file has already been deleted, as temporary files often are, a new file named after it plus ` (deleted)`, such as `out.txt (deleted)`, is made beside it holding the output (for decrypt, the plaintext), and the caller gets nothing. On macOS, where `/dev/fd/1` is not a symbolic link, the temporary file would be made in `/dev/fd`, so the command fails. In all three cases `next` writes through the descriptor. Acceptable: an `--output` that leads into `/dev/fd` or `/proc/self/fd`, as these names do, is written to directly as on `next`, the same way as a device. A shell redirect has already emptied the file behind it, so nothing is lost. The README's sentence on devices says so, and a test gives `/dev/fd/N` of an open, deleted file as `--output` and reads the result back through that descriptor.
2. An existing `--output` that is mounted on its own, such as a single file bind-mounted into a container, now fails with "device or resource busy", because a file cannot be renamed over a mount point; `next` wrote it. `README.md` lines 300 to 301 give a writable directory as the only condition. Acceptable: the README says that such a file cannot be given as `--output`.
Model: opus-5-5
clawbot
changed title from Write encrypt and decrypt output only once they succeed to Hold the encrypt and decrypt result in memory until it is written (closes #143)2026-10-15 02:24:28 +02:00
Rework, rebuilt to the changed plan in #143 (comment): the result is held in memory and --output is opened only once the command has succeeded, with the same call as on next. The temporary file, the rename, the signal handling, the .tmp- test changes, the tests for links, pipes and long names, and the README sentences about them are gone. Both findings below are answered by this change.
/dev/stdout, /dev/stderr and /dev/fd/N are written through their descriptor as on next, since nothing replaces --output any more; a new test gives /dev/fd/N of an open, deleted file as --output and reads the result back through it.
A file mounted on its own is written as on next, so the README needs no sentence about it.
Model: opus-5-5
Rework, rebuilt to the changed plan in https://git.eeqj.de/sneak/secret/issues/143#issuecomment-141311: the result is held in memory and `--output` is opened only once the command has succeeded, with the same call as on `next`. The temporary file, the rename, the signal handling, the `.tmp-` test changes, the tests for links, pipes and long names, and the README sentences about them are gone. Both findings below are answered by this change.
1. `/dev/stdout`, `/dev/stderr` and `/dev/fd/N` are written through their descriptor as on `next`, since nothing replaces `--output` any more; a new test gives `/dev/fd/N` of an open, deleted file as `--output` and reads the result back through it.
2. A file mounted on its own is written as on `next`, so the README needs no sentence about it.
Model: opus-5-5
README.md lines 299 to 300, the comments at internal/cli/crypto.go lines 192 to 194 and 284 to 286, the commit message and the PR body say a failed command leaves --output untouched. That holds only when encryption or decryption fails. When writing --output itself fails (a full disk, a quota, a file size limit), writeOutputFile has already emptied it, and it is left empty or holding the start of the result, which for decrypt is part of the plaintext. Acceptable: these sentences say that a failure to encrypt or decrypt leaves --output untouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.
writeOutputFile (internal/cli/crypto.go line 320) throws away the error from closing the file it has just written. A write error that the system reports only on close, as NFS does for a full disk or quota, ends the command with success and a short --output. Acceptable: when the write succeeded, return the error from closing, wrapped like the write error, as WriteFileAtomic in internal/secret/atomic.go does.
TestDecryptToDescriptorOfDeletedFile (internal/cli/crypto_test.go line 131) reads the result from the descriptor's current position. On macOS, which the README lists as fully supported, opening /dev/fd/N gives a copy of the descriptor that shares its position, so once the command has written, the position is at the end and the test reads nothing and fails. Acceptable: read from the start of the file, for example with file.ReadAt at offset 0, or a seek to the start before reading.
The comment on resolveEncryptionKey (internal/cli/crypto.go lines 111 to 112) says Encrypt "streams its input and output unlocked"; with --output it no longer streams its output. Acceptable: say that it reads its input and writes its output without the lock.
Model: opus-5-5
1. `README.md` lines 299 to 300, the comments at `internal/cli/crypto.go` lines 192 to 194 and 284 to 286, the commit message and the PR body say a failed command leaves `--output` untouched. That holds only when encryption or decryption fails. When writing `--output` itself fails (a full disk, a quota, a file size limit), `writeOutputFile` has already emptied it, and it is left empty or holding the start of the result, which for decrypt is part of the plaintext. Acceptable: these sentences say that a failure to encrypt or decrypt leaves `--output` untouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.
2. `writeOutputFile` (`internal/cli/crypto.go` line 320) throws away the error from closing the file it has just written. A write error that the system reports only on close, as NFS does for a full disk or quota, ends the command with success and a short `--output`. Acceptable: when the write succeeded, return the error from closing, wrapped like the write error, as `WriteFileAtomic` in `internal/secret/atomic.go` does.
3. `TestDecryptToDescriptorOfDeletedFile` (`internal/cli/crypto_test.go` line 131) reads the result from the descriptor's current position. On macOS, which the README lists as fully supported, opening `/dev/fd/N` gives a copy of the descriptor that shares its position, so once the command has written, the position is at the end and the test reads nothing and fails. Acceptable: read from the start of the file, for example with `file.ReadAt` at offset 0, or a seek to the start before reading.
4. The comment on `resolveEncryptionKey` (`internal/cli/crypto.go` lines 111 to 112) says Encrypt "streams its input and output unlocked"; with `--output` it no longer streams its output. Acceptable: say that it reads its input and writes its output without the lock.
Model: opus-5-5
The README, both comments, the commit message and the PR body now say that a failure to encrypt or decrypt leaves --output untouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.
writeOutputFile returns the error from closing --output when the write succeeded, wrapped like the write error; a new test makes closing fail and expects decrypt to fail.
TestDecryptToDescriptorOfDeletedFile seeks to the start of the file before reading the result.
The comment on resolveEncryptionKey says Encrypt reads its input and writes its output without the lock.
Model: opus-5-5
Rework:
1. The README, both comments, the commit message and the PR body now say that a failure to encrypt or decrypt leaves `--output` untouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.
2. `writeOutputFile` returns the error from closing `--output` when the write succeeded, wrapped like the write error; a new test makes closing fail and expects decrypt to fail.
3. `TestDecryptToDescriptorOfDeletedFile` seeks to the start of the file before reading the result.
4. The comment on `resolveEncryptionKey` says Encrypt reads its input and writes its output without the lock.
Model: opus-5-5
internal/cli/crypto_test.go line 49: the comment on TestFailedCryptoLeavesOutputUnchanged says "A failed command must leave an existing file as it was and create none". But a failure while writing --output, such as a full disk, leaves it empty or partly written, as the README and the comments in internal/cli/crypto.go now say. Acceptable: "A failure to encrypt or decrypt must leave an existing file as it was and create none".
README.md lines 320 to 323: the paragraph that covers both commands sits under the secret decrypt heading. There, "both" has nothing to refer to, and someone reading the secret encrypt section never sees it. Acceptable: move the paragraph directly under ### Encryption Operations, before the two command headings, and name secret encrypt and secret decrypt in it.
README.md lines 320 to 323 leave out part of what the plan in #143 (comment) says the README states: that the command needs about as much memory as the result. Only the PR body says this. Acceptable: add that sentence to the paragraph.
Model: opus-5-5
1. `internal/cli/crypto_test.go` line 49: the comment on `TestFailedCryptoLeavesOutputUnchanged` says "A failed command must leave an existing file as it was and create none". But a failure while writing `--output`, such as a full disk, leaves it empty or partly written, as the README and the comments in `internal/cli/crypto.go` now say. Acceptable: "A failure to encrypt or decrypt must leave an existing file as it was and create none".
2. `README.md` lines 320 to 323: the paragraph that covers both commands sits under the `secret decrypt` heading. There, "both" has nothing to refer to, and someone reading the `secret encrypt` section never sees it. Acceptable: move the paragraph directly under `### Encryption Operations`, before the two command headings, and name `secret encrypt` and `secret decrypt` in it.
3. `README.md` lines 320 to 323 leave out part of what the plan in https://git.eeqj.de/sneak/secret/issues/143#issuecomment-141311 says the README states: that the command needs about as much memory as the result. Only the PR body says this. Acceptable: add that sentence to the paragraph.
Model: opus-5-5
The comment on TestFailedCryptoLeavesOutputUnchanged now says a failure to encrypt or decrypt must leave an existing file as it was and create none.
The README paragraph sits directly under ### Encryption Operations, before both command headings, and names secret encrypt and secret decrypt.
The paragraph ends with: the command needs about as much memory as the result.
Model: opus-5-5
Rework:
1. The comment on `TestFailedCryptoLeavesOutputUnchanged` now says a failure to encrypt or decrypt must leave an existing file as it was and create none.
2. The README paragraph sits directly under `### Encryption Operations`, before both command headings, and names `secret encrypt` and `secret decrypt`.
3. The paragraph ends with: the command needs about as much memory as the result.
Model: opus-5-5
README.md line 321 and the PR body say the command needs about as much memory as the result. With --output it needs two to four times as much. The result is held in a buffer that grows by doubling, so while it grows the old and new copies are both in memory: decrypting a 130 MiB file takes more than 500 MiB, and encrypting it more than twice its size. Someone deciding whether a large file will fit is told a quarter of what decrypt can need. Acceptable: the README and the PR body give the real figure, such as "up to about four times as much memory as the result"; or the code is changed so that the current sentence is true.
Model: opus-5-5
1. `README.md` line 321 and the PR body say the command needs about as much memory as the result. With `--output` it needs two to four times as much. The result is held in a buffer that grows by doubling, so while it grows the old and new copies are both in memory: decrypting a 130 MiB file takes more than 500 MiB, and encrypting it more than twice its size. Someone deciding whether a large file will fit is told a quarter of what decrypt can need. Acceptable: the README and the PR body give the real figure, such as "up to about four times as much memory as the result"; or the code is changed so that the current sentence is true.
Model: opus-5-5
The README and the PR body now say the command can need up to about four times as much memory as the result; the code is unchanged.
Model: opus-5-5
Rework:
1. The README and the PR body now say the command can need up to about four times as much memory as the result; the code is unchanged.
Model: opus-5-5
With --output, secret encrypt and secret decrypt emptied that file before
reading any input, so a failed command left it empty, and encrypting a file
onto itself gave the encryption of nothing.
With --output they now hold the result in memory and open --output, with the
same call as before, only once encryption or decryption has succeeded. A
failure to encrypt or decrypt leaves --output untouched, and --input can be the
same file. A failure while writing --output, such as a full disk, can leave it
empty or partly written; an error from closing it is reported. Without
--output the result still goes to stdout as it is produced.
Model: opus-5-5
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.
With
--output,secret encryptandsecret decryptemptied that file before reading any input. A failed command left it empty, and encrypting a file onto itself gave the encryption of nothing.Both now hold the result in memory and open
--outputonly once encryption or decryption has succeeded, with the same call as onnext, so a link, a device or/dev/stdoutgiven as--outputis opened as before. A failure to encrypt or decrypt leaves--outputuntouched, and--inputcan be the same file. A failure while writing--output, such as a full disk, can still leave it empty or partly written; the README says so. The command can need up to about four times as much memory as the result, which the README also says.Judgement call: without
--outputthe result still goes to stdout as it is produced, as onnext.Model: opus-5-5
A symbolic link given as
--outputis no longer followed (internal/cli/crypto.golines 296 to 307,README.mdline 281).secret decrypt KEY -i f.age -o /dev/stdout(or-o /dev/fd/1) with stdout redirected toout.txtnow fails, because/dev/stdoutis a link to the file stdout goes to and the temporary file cannot be made in/dev;nextwritesout.txt. A link to a file elsewhere, such as into a dotfiles directory, is replaced by a new file and the file it points to keeps its old content, wherenextupdated that file. #143 asked for neither change. Acceptable: follow the link as before, making the temporary file next to the file the link points to and renaming it over that file, with the README sentence changed to match and a test that gives a link as--output.Every encrypt or decrypt failure with
--outputnow reads as a failure to write the output (internal/secret/atomic.goline 62). A wrong key givesfailed to write out.txt: failed to create age decryptor: no identity matched any of the recipients; without--outputthe same failure has no such prefix. The PR body describes this only as a change to failures writing to a file. Acceptable:WriteFileAtomicFuncreturns the error fromwriteunchanged and addsfailed to writeonly to errors from creating, syncing, closing or renaming the temporary file.An
--outputwhose name is within about 16 bytes of the 255-byte limit now fails because the temporary file's name, the output's name plus up to 16 bytes, is too long (internal/secret/atomic.golines 39 to 40);nextwrites such a file.TempDirForin the same file leaves the target's name out for this reason. Acceptable: a temporary file name that does not grow with the output's name.A command interrupted with Ctrl-C or SIGTERM leaves its temporary file
.NAME.tmp-123next to--output, hidden from a plainls, and nothing removes it; for decrypt it holds part of the decrypted secret.README.mdlines 277 to 279 say a failed command "creates none" and do not mention it. Acceptable: remove the temporary file when the command is interrupted (internal/cli/root.goline 24 already runs a function on SIGINT and SIGTERM before exiting), or say in the README that an interrupted command leaves this file, and where.Model: opus-5-5
Note for the rework of finding 1: when
--outputexists and is not a regular file once links are followed (a pipe or terminal behind/dev/stdoutor/dev/fd/1, a device), write to it directly asnextdoes; only a regular file, or a path that does not exist yet, gets the temporary file and the rename. For finding 4, removing the temporary file on interrupt is preferred over documenting it, since for decrypt it holds part of the secret.Model: opus-5-5
90bfbec5cbtoaccfd5878fRework:
--outputis followed again: the temporary file goes next to the file it points to and replaces that file, the link is kept, the README says so, and a new test gives a link as--output.WriteFileAtomicFuncreturns the error fromwriteunchanged; a test checks that a failure gives the same error with and without--output..tmp-and a number, without the output's name; a new test writes an--outputwhose name is 255 bytes.internal/cli/root.go; a new test interrupts encrypt and decrypt and finds nothing left.Model: opus-5-5
--outputwhose target does not exist yet is replaced by the new file, and the file it points to is never created; any other failure to follow the link, such as a link that loops back to itself, also replaces the link (internal/cli/crypto.golines 317 to 323). Onnextthe file the link points to is created and the link kept, and a looping link fails. This contradictsREADME.mdlines 281 to 282 ("the file it points to is replaced and the link kept") and is a change #143 did not ask for. Acceptable: a link to a file that does not exist yet gets the new file at the path it points to and is kept, a link that cannot be followed fails as onnext, and a test gives a link to a missing file as--output.Model: opus-5-5
accfd5878ftoe08c29438aRework:
--output.Model: opus-5-5
Whether
--outputcan be written now depends on its directory, not on the file (internal/cli/crypto.golines 299 to 330). An existing--outputof mode 0444 is now replaced without a word, wherenextfailed with "permission denied". An existing writable--outputin a directory the user cannot write now fails, wherenextwrote it. #143 asked for neither change, and neither the README nor the PR body mentions them. Acceptable: when--outputexists and cannot be opened for writing, fail asnextdoes (check this without truncating the file), and have the README say that the directory holding--outputmust be writable.When
--outputdoes not exist yet and its path goes through a symbolic link to a directory followed by.., the temporary file is made in the wrong directory. Examples:-o d/../new.txtwithda link to a directory elsewhere, or a link whose target is such a path.filepath.DirinWriteFileAtomicFunc(internal/secret/atomic.goline 41) removesd/..as text, which the comment atinternal/cli/crypto.golines 357 to 361 says the code avoids. When that directory is on a different filesystem from the one--outputresolves to, the rename fails with "invalid cross-device link", wherenextwrites the file. Acceptable: the temporary file is made in the directory the path actually resolves to, for example by resolving the directory part withfilepath.EvalSymlinksbefore adding the file name, with a test that gives such a path as--output.README.mdlines 277 to 279 say a failed or interrupted command leaves--outputas it was "and creates none", but only SIGINT and SIGTERM remove the temporary file (internal/cli/root.goline 36). A hangup (closing the terminal, a dropped ssh session) or SIGQUIT ends the command with the hidden.tmp-file left next to--output, and for decrypt that file holds part of the decrypted secret. The PR body names only SIGKILL. Acceptable: a hangup removes the temporary file just as Ctrl-C does, and the README says that a command killed any other way leaves a.tmp-file next to--output.internal/cli/leftovers_test.golines 55 to 58 still plant.currentvault.tmp-4,.current-unlocker.tmp-5,.current.tmp-6and.metadata.age.tmp-7as the files a killed command leaves. After this changeWriteFileAtomicnames its temporary files.tmp-and a number, so the regression test for #75 no longer plants what a killed command actually leaves. Acceptable: plant files named.tmp-and a number in those four directories.Model: opus-5-5
Rework scope for the four findings above, so the review converges: fix all four as their "Acceptable" lines say. The differences from
nextthat stay, and that the README states in one or two sentences, are these: a written--outputis a new file with mode 0600 owned by the user running the command, so a hard link to the old file keeps the old content; the directory holding--outputmust be writable; a command killed by a signal it cannot catch (SIGKILL) leaves a.tmp-file next to--output. The next review checks the four findings and these sentences; any other difference fromnextfor a plain file, a link or a device given as--outputis still a finding.Model: opus-5-5
e08c29438atoe4a866949bRework:
--outputthat cannot be opened for writing fails as onnext, checked by opening it without emptying it; the README says the directory the new file goes in must be writable; a new test gives an unwritable--output.--outputthat does not exist yet has its directory part resolved withfilepath.EvalSymlinks; a new test giveslink/../sub/new.txt, wheresubexists only beside the directory the link points to.nohup, which still ignores it; the README says a command killed any other way leaves a.tmp-file; the interrupt test also sends a hangup, and a new test coversnohup..tmp-and a number.Rebase: the signal handler now removes the temporary file before it restores the terminal, because the terminal code from #144 returns early when there is no terminal.
Scope comment: the README also says the new file has mode 0600 and belongs to the user running the command, so a hard link to the old file keeps the old content.
Model: opus-5-5
e4a866949btobde833193cAn
--outputof/dev/stdout,/dev/stderror/dev/fd/Nwhose descriptor is a regular file is no longer written through that descriptor (internal/cli/crypto.goline 330 andoutputPathfrom line 354). Instead, the file at the path the descriptor was opened from is replaced by a new one. A program that gives the command a temporary file as stdout and reads it back through its descriptor reads nothing. If that file has already been deleted, as temporary files often are, a new file named after it plus(deleted), such asout.txt (deleted), is made beside it holding the output (for decrypt, the plaintext), and the caller gets nothing. On macOS, where/dev/fd/1is not a symbolic link, the temporary file would be made in/dev/fd, so the command fails. In all three casesnextwrites through the descriptor. Acceptable: an--outputthat leads into/dev/fdor/proc/self/fd, as these names do, is written to directly as onnext, the same way as a device. A shell redirect has already emptied the file behind it, so nothing is lost. The README's sentence on devices says so, and a test gives/dev/fd/Nof an open, deleted file as--outputand reads the result back through that descriptor.An existing
--outputthat is mounted on its own, such as a single file bind-mounted into a container, now fails with "device or resource busy", because a file cannot be renamed over a mount point;nextwrote it.README.mdlines 300 to 301 give a writable directory as the only condition. Acceptable: the README says that such a file cannot be given as--output.Model: opus-5-5
bde833193cto110e3d5e69110e3d5e69to34f3ff17c034f3ff17c0to9a74d2e18cWrite encrypt and decrypt output only once they succeedto Hold the encrypt and decrypt result in memory until it is written (closes #143)Rework, rebuilt to the changed plan in #143 (comment): the result is held in memory and
--outputis opened only once the command has succeeded, with the same call as onnext. The temporary file, the rename, the signal handling, the.tmp-test changes, the tests for links, pipes and long names, and the README sentences about them are gone. Both findings below are answered by this change./dev/stdout,/dev/stderrand/dev/fd/Nare written through their descriptor as onnext, since nothing replaces--outputany more; a new test gives/dev/fd/Nof an open, deleted file as--outputand reads the result back through it.next, so the README needs no sentence about it.Model: opus-5-5
9a74d2e18cto81360431ef81360431eftob47403b927README.mdlines 299 to 300, the comments atinternal/cli/crypto.golines 192 to 194 and 284 to 286, the commit message and the PR body say a failed command leaves--outputuntouched. That holds only when encryption or decryption fails. When writing--outputitself fails (a full disk, a quota, a file size limit),writeOutputFilehas already emptied it, and it is left empty or holding the start of the result, which for decrypt is part of the plaintext. Acceptable: these sentences say that a failure to encrypt or decrypt leaves--outputuntouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.writeOutputFile(internal/cli/crypto.goline 320) throws away the error from closing the file it has just written. A write error that the system reports only on close, as NFS does for a full disk or quota, ends the command with success and a short--output. Acceptable: when the write succeeded, return the error from closing, wrapped like the write error, asWriteFileAtomicininternal/secret/atomic.godoes.TestDecryptToDescriptorOfDeletedFile(internal/cli/crypto_test.goline 131) reads the result from the descriptor's current position. On macOS, which the README lists as fully supported, opening/dev/fd/Ngives a copy of the descriptor that shares its position, so once the command has written, the position is at the end and the test reads nothing and fails. Acceptable: read from the start of the file, for example withfile.ReadAtat offset 0, or a seek to the start before reading.The comment on
resolveEncryptionKey(internal/cli/crypto.golines 111 to 112) says Encrypt "streams its input and output unlocked"; with--outputit no longer streams its output. Acceptable: say that it reads its input and writes its output without the lock.Model: opus-5-5
b47403b927tob405224775Rework:
--outputuntouched, and that a failure while writing it, such as a full disk, can leave it empty or partly written.writeOutputFilereturns the error from closing--outputwhen the write succeeded, wrapped like the write error; a new test makes closing fail and expects decrypt to fail.TestDecryptToDescriptorOfDeletedFileseeks to the start of the file before reading the result.resolveEncryptionKeysays Encrypt reads its input and writes its output without the lock.Model: opus-5-5
b405224775to324cfebe54internal/cli/crypto_test.goline 49: the comment onTestFailedCryptoLeavesOutputUnchangedsays "A failed command must leave an existing file as it was and create none". But a failure while writing--output, such as a full disk, leaves it empty or partly written, as the README and the comments ininternal/cli/crypto.gonow say. Acceptable: "A failure to encrypt or decrypt must leave an existing file as it was and create none".README.mdlines 320 to 323: the paragraph that covers both commands sits under thesecret decryptheading. There, "both" has nothing to refer to, and someone reading thesecret encryptsection never sees it. Acceptable: move the paragraph directly under### Encryption Operations, before the two command headings, and namesecret encryptandsecret decryptin it.README.mdlines 320 to 323 leave out part of what the plan in #143 (comment) says the README states: that the command needs about as much memory as the result. Only the PR body says this. Acceptable: add that sentence to the paragraph.Model: opus-5-5
324cfebe54toeef05ee429Rework:
TestFailedCryptoLeavesOutputUnchangednow says a failure to encrypt or decrypt must leave an existing file as it was and create none.### Encryption Operations, before both command headings, and namessecret encryptandsecret decrypt.Model: opus-5-5
README.mdline 321 and the PR body say the command needs about as much memory as the result. With--outputit needs two to four times as much. The result is held in a buffer that grows by doubling, so while it grows the old and new copies are both in memory: decrypting a 130 MiB file takes more than 500 MiB, and encrypting it more than twice its size. Someone deciding whether a large file will fit is told a quarter of what decrypt can need. Acceptable: the README and the PR body give the real figure, such as "up to about four times as much memory as the result"; or the code is changed so that the current sentence is true.Model: opus-5-5
eef05ee429to73ec3035c0Rework:
Model: opus-5-5
73ec3035c0to1d06a633b9Review passed.
Model: opus-5-5