Repository navigation
Make merge_args_into_command's precondition its own extraction - #4047
Conversation
The inline comment justified declining the merge as preserving `args`. It no longer does: `[commit.generation]` has no `args` field, so `drop_unsupported_keys` removes an unmerged `args` a few rules later either way. Declining still matters, for a different reason. Joining `args = [1, "--ok"]` would drop the `1`, rewrite `command` with a value the user never wrote, and report neither; dropping the whole key shows up in the deprecation warning and in the `wt config update` diff. The docstring now says that, and the body collects the strings with a let-else so the precondition is the extraction — no separate invariant for a comment to state, and the four unwraps it guarded are gone. Behavior is unchanged. A new test pins the empty-`args` case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNRipTsuGxtr9SYcouv1wV
worktrunk-bot
left a comment
There was a problem hiding this comment.
The refactor holds: I walked each decline path against the old can_merge (args absent, non-array, non-string element, command absent, command non-string) plus both merge paths (empty args, empty command), and the let-else chain reproduces every outcome — including that args is still left in place for drop_unsupported_keys to remove and report on the decline paths. The docstring matches what each branch now does.
codecov/patch is red, and it is closeable rather than a pre-existing miss. Patch coverage is 29/30 lines (96.66%); the single miss is the if key == "args" guard on the new test's negative assertion. The closure does run — I confirmed it is invoked once, with CommitGeneration — but the UnsupportedKey pattern never matches, so the guard expression itself never executes and the line counts as a miss. Dropping the guard makes the assertion strictly stronger here (command is the only other key, and it is supported, so args is the only thing that could produce an UnsupportedKey) and matches the guard-free shape test_detect_list_task_timeout_absent already uses for the same kind of negative assertion. The suggestion below passes cargo test --lib test_migrate_empty_args_leaves_command_unchanged and cargo clippy --lib --tests locally.
Withholding approval only on that check — root CLAUDE.md → Coverage treats codecov/patch as a merge gate despite GitHub marking it non-required. Everything else on this commit is green.
`command` is the section's only other key and it is supported, so an `UnsupportedKey` report from this input can only name `args`. The guard never evaluated — the closure runs against the `CommitGeneration` kind, which fails the pattern before reaching it — leaving the line uncovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNRipTsuGxtr9SYcouv1wV
merge_args_into_commanddeclines to mergeargsintocommandunless every element is a string, and an inline comment justified that by saying the alternative would drop user data "whileargswas removed". Since #3994,drop_unsupported_keysremoves every key the destination struct has no field for, and[commit.generation]has noargsfield — so an unmergedargsis removed a few rules later regardless, and the comment described a preservation the code no longer performs.Declining still changes the outcome, just not the one the comment named. With the precondition,
[commit-generation] command = "claude", args = [1, "--ok"]migrates tocommand = "claude", and the removal is reported both as▲ User config: [commit-generation] args is no longer supported and will be removedand as-args = [1, "--ok"]in thewt config updatediff preview. Without it (checked by building the variant), it migrates tocommand = "claude --ok": the1disappears,commandgains a value the user never wrote, and nothing says so. So the precondition stays and the docstring now states the live reason.The body collects the strings with a
let-else, which makes the precondition the extraction itself. That removes the separate invariant a comment had to carry — and the fourunwrap()s it guaranteed.Behavior is unchanged; existing tests pin every branch. A new test covers
args = [], which had none.