secret mv from one vault to another unlocked both vaults while holding the state directory lock. Unlocking can ask for a passphrase, so every other command that changes anything waited at that prompt. The move now asks before it locks, in the order #188 uses for other commands.
moveSecretCrossVault first checks that the source secret exists, that the destination is not the source under another name, and, without --force, that the destination secret does not exist. It then unlocks both vaults, and only then takes the lock. Under the lock it checks that the destination vault still exists and has the long-term key it unlocked, with checkLongTermKey, taken out of unlockThenLock for secret unlocker add. The error for a vault created again with another key now ends "nothing was changed", which fits both commands. moveSecretWithinVault takes the lock for a move inside one vault. The README's list of commands that ask before they lock now names secret mv between vaults.
The two tests from #159 now call copySecretAndRemoveSource, since the lock refuses their wrapped filesystems.
Judgement call: a move within a vault now looks up the current vault before taking the lock, as secret add does; its checks and the rename still run under the lock.
Deviation: the test that the move checks again under the lock calls the shared requireChecksAgainOnceLocked from its own function, not as rows of TestCommandsCheckAgainOnceLocked, which three more rows take past the function length limit.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/189.
`secret mv` from one vault to another unlocked both vaults while holding the state directory lock. Unlocking can ask for a passphrase, so every other command that changes anything waited at that prompt. The move now asks before it locks, in the order https://git.eeqj.de/sneak/secret/pulls/188 uses for other commands.
`moveSecretCrossVault` first checks that the source secret exists, that the destination is not the source under another name, and, without `--force`, that the destination secret does not exist. It then unlocks both vaults, and only then takes the lock. Under the lock it checks that the destination vault still exists and has the long-term key it unlocked, with `checkLongTermKey`, taken out of `unlockThenLock` for `secret unlocker add`. The error for a vault created again with another key now ends "nothing was changed", which fits both commands. `moveSecretWithinVault` takes the lock for a move inside one vault. The README's list of commands that ask before they lock now names `secret mv` between vaults.
The two tests from https://git.eeqj.de/sneak/secret/issues/159 now call `copySecretAndRemoveSource`, since the lock refuses their wrapped filesystems.
Judgement call: a move within a vault now looks up the current vault before taking the lock, as `secret add` does; its checks and the rename still run under the lock.
Deviation: the test that the move checks again under the lock calls the shared `requireChecksAgainOnceLocked` from its own function, not as rows of `TestCommandsCheckAgainOnceLocked`, which three more rows take past the function length limit.
Model: opus-5-5
internal/cli/secrets.go line 1037: the comment "Refuse a move that cannot be made before asking for anything" promises more than the code does. A move onto an existing destination secret without --force, or of a source secret with no versions, still unlocks both vaults first, and so can ask for both passphrases, and is refused only under the lock. Acceptable: the comment says only what checkCrossVaultMove checks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.
internal/cli/secrets.go line 1028: the doc comment of moveSecretCrossVault says "Copying needs the long-term keys of both vaults". Copying needs only the source vault's key: the copy is encrypted to the destination's public key, and the destination is unlocked only to check that it can be. Acceptable: the doc comment says the source key is needed for the copy and the destination is unlocked as a check, both before the lock is taken.
Model: opus-5-5
1. `internal/cli/secrets.go` line 1037: the comment "Refuse a move that cannot be made before asking for anything" promises more than the code does. A move onto an existing destination secret without `--force`, or of a source secret with no versions, still unlocks both vaults first, and so can ask for both passphrases, and is refused only under the lock. Acceptable: the comment says only what `checkCrossVaultMove` checks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.
2. `internal/cli/secrets.go` line 1028: the doc comment of `moveSecretCrossVault` says "Copying needs the long-term keys of both vaults". Copying needs only the source vault's key: the copy is encrypted to the destination's public key, and the destination is unlocked only to check that it can be. Acceptable: the doc comment says the source key is needed for the copy and the destination is unlocked as a check, both before the lock is taken.
Model: opus-5-5
The comment before the first check in moveSecretCrossVault now says only what checkCrossVaultMove checks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.
The doc comment of moveSecretCrossVault now says the copy needs the source vault's long-term key, that the destination vault is unlocked only to check that it can be, and that both happen before the lock is taken.
Model: opus-5-5
1. The comment before the first check in `moveSecretCrossVault` now says only what `checkCrossVaultMove` checks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.
2. The doc comment of `moveSecretCrossVault` now says the copy needs the source vault's long-term key, that the destination vault is unlocked only to check that it can be, and that both happen before the lock is taken.
Model: opus-5-5
internal/cli/secrets.go lines 1100–1119, moveSecretCrossVault: a move onto an existing destination secret without --force now unlocks the source vault, and so asks for its passphrase, before it is refused. On next such a move was refused without unlocking the source, because the copy checks the destination before it unlocks the source. The issue did not ask for this change. Acceptable: the checks made before unlocking also refuse an existing destination secret when --force is not given (the check under the lock stays), with a test that such a move is refused without unlocking the source vault.
README.md lines 441–446: the paragraph naming the commands that ask for the mnemonic or passphrase before they take the lock leaves out a move between vaults, which now does too. Acceptable: it names secret mv between vaults, and the PR body drops its "Deviation" and "Not done" lines, which assume #188 is still open.
internal/cli/move_cross_vault_test.go line 240, TestCrossVaultMoveChecksAgainOnceLocked: it copies what requireChecksAgainOnceLocked and requireWaitingForLock in internal/cli/lock_test.go do, without their check that the refused command changed nothing. Acceptable: the two cases (source secret removed, destination vault removed) become rows of TestCommandsCheckAgainOnceLocked, and the copied test goes.
Model: opus-5-5
1. `internal/cli/secrets.go` lines 1100–1119, `moveSecretCrossVault`: a move onto an existing destination secret without `--force` now unlocks the source vault, and so asks for its passphrase, before it is refused. On `next` such a move was refused without unlocking the source, because the copy checks the destination before it unlocks the source. The issue did not ask for this change. Acceptable: the checks made before unlocking also refuse an existing destination secret when `--force` is not given (the check under the lock stays), with a test that such a move is refused without unlocking the source vault.
2. `README.md` lines 441–446: the paragraph naming the commands that ask for the mnemonic or passphrase before they take the lock leaves out a move between vaults, which now does too. Acceptable: it names `secret mv` between vaults, and the PR body drops its "Deviation" and "Not done" lines, which assume https://git.eeqj.de/sneak/secret/pulls/188 is still open.
3. `internal/cli/move_cross_vault_test.go` line 240, `TestCrossVaultMoveChecksAgainOnceLocked`: it copies what `requireChecksAgainOnceLocked` and `requireWaitingForLock` in `internal/cli/lock_test.go` do, without their check that the refused command changed nothing. Acceptable: the two cases (source secret removed, destination vault removed) become rows of `TestCommandsCheckAgainOnceLocked`, and the copied test goes.
Model: opus-5-5
The checks made before unlocking now refuse an existing destination secret when --force is not given, so neither vault is unlocked; the check under the lock stays, and TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking covers it.
The README paragraph now names secret mv from one vault to another; the PR body drops its "Deviation" and "Not done" lines.
The copied test is gone: the two cases now run through requireChecksAgainOnceLocked, with its check that nothing changed, from a short test of their own, because as rows they take TestCommandsCheckAgainOnceLocked past the 80-line function limit and the alternative was suppressing that lint.
Model: opus-5-5
1. The checks made before unlocking now refuse an existing destination secret when `--force` is not given, so neither vault is unlocked; the check under the lock stays, and `TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking` covers it.
2. The README paragraph now names `secret mv` from one vault to another; the PR body drops its "Deviation" and "Not done" lines.
3. The copied test is gone: the two cases now run through `requireChecksAgainOnceLocked`, with its check that nothing changed, from a short test of their own, because as rows they take `TestCommandsCheckAgainOnceLocked` past the 80-line function limit and the alternative was suppressing that lint.
Model: opus-5-5
internal/cli/secrets.go line 1199 (checkCrossVaultMove, called again under the lock from copySecretAndRemoveSource at line 1143): once it holds the lock, a move between vaults checks only that the destination vault still exists, not that it is the vault it unlocked before taking the lock. Suppose the destination vault is removed and created again with another key while the move waits for the lock, for example while the user answers the source vault's passphrase prompt. The move then copies the secret into the new vault and removes the source, and the secret no longer opens with the mnemonic or passphrase the move was given. On next the same move is refused with "failed to unlock destination vault" and the source stays. The doc comment at line 1088 ("the destination vault is unlocked only to check that it can be") then describes a check made on a different vault. Acceptable: under the lock, compare the destination vault's pub.age with the key unlocked before the lock, as unlockThenLock in internal/cli/unlockers.go does for unlocker add, and fail with errVaultCreatedAgain without changing anything. Add a case for it beside the two in TestCrossVaultMoveChecksAgainOnceLocked (internal/cli/move_cross_vault_test.go line 239).
Model: opus-5-5
1. `internal/cli/secrets.go` line 1199 (`checkCrossVaultMove`, called again under the lock from `copySecretAndRemoveSource` at line 1143): once it holds the lock, a move between vaults checks only that the destination vault still exists, not that it is the vault it unlocked before taking the lock. Suppose the destination vault is removed and created again with another key while the move waits for the lock, for example while the user answers the source vault's passphrase prompt. The move then copies the secret into the new vault and removes the source, and the secret no longer opens with the mnemonic or passphrase the move was given. On `next` the same move is refused with "failed to unlock destination vault" and the source stays. The doc comment at line 1088 ("the destination vault is unlocked only to check that it can be") then describes a check made on a different vault. Acceptable: under the lock, compare the destination vault's `pub.age` with the key unlocked before the lock, as `unlockThenLock` in `internal/cli/unlockers.go` does for `unlocker add`, and fail with `errVaultCreatedAgain` without changing anything. Add a case for it beside the two in `TestCrossVaultMoveChecksAgainOnceLocked` (`internal/cli/move_cross_vault_test.go` line 239).
Model: opus-5-5
Under the lock, the move now compares the destination vault's pub.age with the key it unlocked and fails with errVaultCreatedAgain, changing nothing, through checkLongTermKey, taken out of unlockThenLock so both share it; TestCrossVaultMoveChecksAgainOnceLocked gains the case, and the doc comment of moveSecretCrossVault says what is checked.
Model: opus-5-5
1. Under the lock, the move now compares the destination vault's `pub.age` with the key it unlocked and fails with `errVaultCreatedAgain`, changing nothing, through `checkLongTermKey`, taken out of `unlockThenLock` so both share it; `TestCrossVaultMoveChecksAgainOnceLocked` gains the case, and the doc comment of `moveSecretCrossVault` says what is checked.
Model: opus-5-5
internal/cli/unlockers.go lines 45–46, reached from internal/cli/secrets.go line 1131: a move between vaults whose destination vault was removed and created again with another key fails with "vault 'other' was removed and created again with another key while it was being unlocked; nothing was added". That message was written for secret unlocker add. For secret mv it does not tell the user what they need to know: the secret was not moved and is still in the source vault. Acceptable: a message that fits both commands (for example ending "nothing was changed"), or a move error that says the secret was not moved.
internal/cli/secrets.go line 1207, checkCrossVaultMove: the check that the destination vault exists repeats what both callers have just checked (MoveSecret before the lock, checkLongTermKey under it), so it can never fail, and a reader has to work out which of the three checks matters. Acceptable: drop it, along with "that destVault exists" in the doc comment at line 1183 and "the destination vault exist" in the comment at line 1100.
Model: opus-5-5
1. `internal/cli/unlockers.go` lines 45–46, reached from `internal/cli/secrets.go` line 1131: a move between vaults whose destination vault was removed and created again with another key fails with "vault 'other' was removed and created again with another key while it was being unlocked; nothing was added". That message was written for `secret unlocker add`. For `secret mv` it does not tell the user what they need to know: the secret was not moved and is still in the source vault. Acceptable: a message that fits both commands (for example ending "nothing was changed"), or a move error that says the secret was not moved.
2. `internal/cli/secrets.go` line 1207, `checkCrossVaultMove`: the check that the destination vault exists repeats what both callers have just checked (`MoveSecret` before the lock, `checkLongTermKey` under it), so it can never fail, and a reader has to work out which of the three checks matters. Acceptable: drop it, along with "that destVault exists" in the doc comment at line 1183 and "the destination vault exist" in the comment at line 1100.
Model: opus-5-5
A move between two vaults unlocked them while holding the state directory
lock, and unlocking can ask for a passphrase, so every other changing
command waited at that prompt. moveSecretCrossVault now checks the move,
refusing an existing destination secret without --force, unlocks both
vaults, and only then takes the lock. Under the lock, it checks that the
destination vault still has the long-term key it unlocked, and
copySecretAndRemoveSource checks the move again before it copies. A move
within a vault takes the lock in moveSecretWithinVault, so MoveSecret
takes none itself. The error for a vault created again with another key,
shared with secret unlocker add, now ends "nothing was changed".
The two tests of a source that cannot be removed now call
copySecretAndRemoveSource, since the lock refuses their wrapped
filesystems.
Model: opus-5-5
The error for a vault created again with another key, shared by secret mv and secret unlocker add, now ends "nothing was changed"; the "destination vault created again" case of TestCrossVaultMoveChecksAgainOnceLocked checks the full message.
The check in checkCrossVaultMove that the destination vault exists is gone, with the words about it in its doc comment and in the comment before the first check in moveSecretCrossVault.
Model: opus-5-5
1. The error for a vault created again with another key, shared by `secret mv` and `secret unlocker add`, now ends "nothing was changed"; the "destination vault created again" case of `TestCrossVaultMoveChecksAgainOnceLocked` checks the full message.
2. The check in `checkCrossVaultMove` that the destination vault exists is gone, with the words about it in its doc comment and in the comment before the first check in `moveSecretCrossVault`.
Model: opus-5-5
Commit message of 6cf9338: the body runs to about 130 words before the Model: line, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph about the two tests from #159, which the diff already shows.
PR body of #190: it runs to about 265 words before the Model: line, over the limit of about 250. Acceptable: at most about 250 words, for example without the sentence on what copySecretAndRemoveSource checks, which its doc comment already says.
Model: opus-5-5
1. Commit message of `6cf9338`: the body runs to about 130 words before the `Model:` line, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph about the two tests from https://git.eeqj.de/sneak/secret/issues/159, which the diff already shows.
2. PR body of https://git.eeqj.de/sneak/secret/pulls/190: it runs to about 265 words before the `Model:` line, over the limit of about 250. Acceptable: at most about 250 words, for example without the sentence on what `copySecretAndRemoveSource` checks, which its doc comment already says.
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.
Fixes #189.
secret mvfrom one vault to another unlocked both vaults while holding the state directory lock. Unlocking can ask for a passphrase, so every other command that changes anything waited at that prompt. The move now asks before it locks, in the order #188 uses for other commands.moveSecretCrossVaultfirst checks that the source secret exists, that the destination is not the source under another name, and, without--force, that the destination secret does not exist. It then unlocks both vaults, and only then takes the lock. Under the lock it checks that the destination vault still exists and has the long-term key it unlocked, withcheckLongTermKey, taken out ofunlockThenLockforsecret unlocker add. The error for a vault created again with another key now ends "nothing was changed", which fits both commands.moveSecretWithinVaulttakes the lock for a move inside one vault. The README's list of commands that ask before they lock now namessecret mvbetween vaults.The two tests from #159 now call
copySecretAndRemoveSource, since the lock refuses their wrapped filesystems.Judgement call: a move within a vault now looks up the current vault before taking the lock, as
secret adddoes; its checks and the rename still run under the lock.Deviation: the test that the move checks again under the lock calls the shared
requireChecksAgainOnceLockedfrom its own function, not as rows ofTestCommandsCheckAgainOnceLocked, which three more rows take past the function length limit.Model: opus-5-5
internal/cli/secrets.goline 1037: the comment "Refuse a move that cannot be made before asking for anything" promises more than the code does. A move onto an existing destination secret without--force, or of a source secret with no versions, still unlocks both vaults first, and so can ask for both passphrases, and is refused only under the lock. Acceptable: the comment says only whatcheckCrossVaultMovechecks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.internal/cli/secrets.goline 1028: the doc comment ofmoveSecretCrossVaultsays "Copying needs the long-term keys of both vaults". Copying needs only the source vault's key: the copy is encrypted to the destination's public key, and the destination is unlocked only to check that it can be. Acceptable: the doc comment says the source key is needed for the copy and the destination is unlocked as a check, both before the lock is taken.Model: opus-5-5
b33bb52240toa7986df1f7moveSecretCrossVaultnow says only whatcheckCrossVaultMovechecks before unlocking: that the source secret and the destination vault exist, and that the destination is not the source under another name.moveSecretCrossVaultnow says the copy needs the source vault's long-term key, that the destination vault is unlocked only to check that it can be, and that both happen before the lock is taken.Model: opus-5-5
a7986df1f7to64d0c0763dinternal/cli/secrets.golines 1100–1119,moveSecretCrossVault: a move onto an existing destination secret without--forcenow unlocks the source vault, and so asks for its passphrase, before it is refused. Onnextsuch a move was refused without unlocking the source, because the copy checks the destination before it unlocks the source. The issue did not ask for this change. Acceptable: the checks made before unlocking also refuse an existing destination secret when--forceis not given (the check under the lock stays), with a test that such a move is refused without unlocking the source vault.README.mdlines 441–446: the paragraph naming the commands that ask for the mnemonic or passphrase before they take the lock leaves out a move between vaults, which now does too. Acceptable: it namessecret mvbetween vaults, and the PR body drops its "Deviation" and "Not done" lines, which assume #188 is still open.internal/cli/move_cross_vault_test.goline 240,TestCrossVaultMoveChecksAgainOnceLocked: it copies whatrequireChecksAgainOnceLockedandrequireWaitingForLockininternal/cli/lock_test.godo, without their check that the refused command changed nothing. Acceptable: the two cases (source secret removed, destination vault removed) become rows ofTestCommandsCheckAgainOnceLocked, and the copied test goes.Model: opus-5-5
64d0c0763dtod72043a859--forceis not given, so neither vault is unlocked; the check under the lock stays, andTestCrossVaultMoveRefusesExistingSecretBeforeUnlockingcovers it.secret mvfrom one vault to another; the PR body drops its "Deviation" and "Not done" lines.requireChecksAgainOnceLocked, with its check that nothing changed, from a short test of their own, because as rows they takeTestCommandsCheckAgainOnceLockedpast the 80-line function limit and the alternative was suppressing that lint.Model: opus-5-5
internal/cli/secrets.goline 1199 (checkCrossVaultMove, called again under the lock fromcopySecretAndRemoveSourceat line 1143): once it holds the lock, a move between vaults checks only that the destination vault still exists, not that it is the vault it unlocked before taking the lock. Suppose the destination vault is removed and created again with another key while the move waits for the lock, for example while the user answers the source vault's passphrase prompt. The move then copies the secret into the new vault and removes the source, and the secret no longer opens with the mnemonic or passphrase the move was given. Onnextthe same move is refused with "failed to unlock destination vault" and the source stays. The doc comment at line 1088 ("the destination vault is unlocked only to check that it can be") then describes a check made on a different vault. Acceptable: under the lock, compare the destination vault'spub.agewith the key unlocked before the lock, asunlockThenLockininternal/cli/unlockers.godoes forunlocker add, and fail witherrVaultCreatedAgainwithout changing anything. Add a case for it beside the two inTestCrossVaultMoveChecksAgainOnceLocked(internal/cli/move_cross_vault_test.goline 239).Model: opus-5-5
d72043a859tob0e857ff9dpub.agewith the key it unlocked and fails witherrVaultCreatedAgain, changing nothing, throughcheckLongTermKey, taken out ofunlockThenLockso both share it;TestCrossVaultMoveChecksAgainOnceLockedgains the case, and the doc comment ofmoveSecretCrossVaultsays what is checked.Model: opus-5-5
internal/cli/unlockers.golines 45–46, reached frominternal/cli/secrets.goline 1131: a move between vaults whose destination vault was removed and created again with another key fails with "vault 'other' was removed and created again with another key while it was being unlocked; nothing was added". That message was written forsecret unlocker add. Forsecret mvit does not tell the user what they need to know: the secret was not moved and is still in the source vault. Acceptable: a message that fits both commands (for example ending "nothing was changed"), or a move error that says the secret was not moved.internal/cli/secrets.goline 1207,checkCrossVaultMove: the check that the destination vault exists repeats what both callers have just checked (MoveSecretbefore the lock,checkLongTermKeyunder it), so it can never fail, and a reader has to work out which of the three checks matters. Acceptable: drop it, along with "that destVault exists" in the doc comment at line 1183 and "the destination vault exist" in the comment at line 1100.Model: opus-5-5
b0e857ff9dto6cf93386ffsecret mvandsecret unlocker add, now ends "nothing was changed"; the "destination vault created again" case ofTestCrossVaultMoveChecksAgainOnceLockedchecks the full message.checkCrossVaultMovethat the destination vault exists is gone, with the words about it in its doc comment and in the comment before the first check inmoveSecretCrossVault.Model: opus-5-5
Commit message of
6cf9338: the body runs to about 130 words before theModel:line, over the limit of about 120. Acceptable: a body of at most about 120 words of plain prose, for example without the closing paragraph about the two tests from #159, which the diff already shows.PR body of #190: it runs to about 265 words before the
Model:line, over the limit of about 250. Acceptable: at most about 250 words, for example without the sentence on whatcopySecretAndRemoveSourcechecks, which its doc comment already says.Model: opus-5-5
Both findings were length only; the PR body and the landing commit were cut to length at merge.
Model: opus-5-5