Skip to content

config: write the path the user named, and check every config write the same way - #4204

Merged
max-sixty merged 5 commits into
mainfrom
release
Sep 21, 2026
Merged

max-sixty merged 5 commits into
mainfrom
release

Conversation

@max-sixty

@max-sixty max-sixty commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Three simplifications to the config write paths, removing guards that second-guess an explicit argument or duplicate a stronger check.

--output writes the path it names

wt config update --output <path> refused three things: the config being migrated, an existing file without --yes, and an existing file with no terminal to prompt on. A named destination is the caller's choice — cp, curl -o, and a shell redirect all replace what is there — so all three go, along with the clobber/no-clobber branch at the write. This is the maintainer's stated policy for this PR, not an inference: a path the caller names is written rather than refused.

That also fixes --output for automation, which is what it exists for. The old code didn't skip the prompt without a TTY, it failed: … already exists; to overwrite it with the … migration, add --yes. So wt config update --output migrated.toml in CI worked the first time and errored every time after.

--yes keeps its meaning. It answers the in-place Apply updates? prompt, which renders the migration diff before rewriting the file.

Overwriting the config being migrated moves its approvals first

Writing the migration over the config it came from drops that config's approved-commands. An earlier revision of this PR left them dropped and printed the existing warning, whose remedy — "to migrate them to approvals.toml, run wt config update" — is false precisely in that case: the write has already removed them, so the follow-up run reports No deprecated settings found and the entries are gone.

copy_approved_commands_to_approvals_file had exactly one production caller, the in-place loop. That block is now move_approvals_out_of, and both writes that land a migration on top of the config it came from call it. Any other destination leaves the config alone, so its warning still names a wt config update with something to migrate.

One check on both config-mutation writes

ConfigFile::edited returns one of two candidate documents: the file with the edit applied, or the file with the load-path migrations applied and then the edit. It validated only the first, and with_locked_mutation then ran a weaker TOML-parse over whichever came back.

Both candidates now face the same test — the content loads back as the config the mutation asked for — and the parse check goes. It immediately caught a mutator in the suite that returned an edit it never made to the config beside it, which previously wrote a file loading as something other than what the mutation said.

One write primitive per situation

Both primitives stay, but no call site picks between them any more:

  • a path the caller named → write_atomically
  • a path wt observed absent → write_new_atomically

wt config create was on the wrong one, so a dangling ~/.config/worktrunk/config.toml symlink from a dotfile manager would have been replaced by a regular file rather than refused. Its error for that case said File exists (os error 17), naming neither the path nor the link, immediately after the existence check reported the path free. A dangling symlink now gets a message naming the link and what to do, as the rc-file write already did; any other write failure names the path it was writing. The rule is in CLAUDE.md so it doesn't drift back.

What stays

wt config update still re-reads each config after the prompt and fails if it changed. It is the only check here guarding content the user authored rather than a path they named: edit config.toml while the prompt is open, and without it that edit is silently reverted.

Changelog

#4153's 0.79.0 entry described behavior this removes, so it is replaced with one covering the net change from 0.78.0. 0.79.0 is merged to main but untagged, so this can still land inside it.

This was written by Claude Code on behalf of max-sixty

max-sixty and others added 2 commits September 20, 2026 15:31
…he same way

`wt config update --output <path>` refused three things: the config being
migrated, an existing file without `--yes`, and an existing file with no
terminal to prompt on. A named destination is the user's choice — `cp` and a
shell redirect replace what is there — so all three go, along with the
no-clobber/clobber branch at the write. Pointed at the config being migrated it
writes the migration there; `approved-commands` still reach approvals.toml only
via the in-place update, which is what the warning above the write says.

`ConfigFile::edited` checked only one of its two candidate documents — the
as-written one — and `with_locked_mutation` then ran a weaker TOML-parse check
over whichever it got. Both candidates now face the same test, that the file
loads back as the config the mutation asked for, and the parse check goes. It
caught a mutator in the suite returning an edit it never made to the config
beside it, which previously wrote a file loading as something else.

Two write primitives stay, one per situation, so no call site picks between
them: a path the user named is written, a path wt observed absent is created
no-clobber. `wt config create` was on the wrong one, where a dangling symlink a
dotfile manager owns would have been replaced by a regular file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Withholding the verdict rather than blocking: the diff loosens a gate on overwriting a config file, which .claude/skills/running-tend/references/review-pr.md ("Data-Loss Surface: Hold for Human Review") routes to a maintainer rather than to the bot. The design argument in the description is defensible — this is a call for you, not a finding against it. Two consequences the description doesn't cover:

--output names the destination but not the source, so the cp analogy only holds in one direction. cp src dst names both ends; wt config update --output dst names only dst, and which migration lands there is decided by which config happens to be deprecated. test_config_update_output_replaces_an_existing_file pins the shape: a user config holding [aliases] hi = "echo hi", a deprecated project config, and --output <user config path> replaces the user config wholesale with the project-config migration, reporting success. That's the case #4153's changelog entry described, and after this PR nothing stands between a user and it — no prompt, no --yes, no non-TTY refusal. The in-place path keeps its re-read guard because it is "the only check here guarding content the user authored rather than a path they named"; this destination is both at once.

The dropped-approvals warning now names a remedy the same write removes. format_dropped_approvals_warning says to migrate them to approvals.toml, run wt config update. That holds while the destination is some other file — the source still carries approved-commands. Pointed at the config being migrated, the write deletes that key and copy_approved_commands_to_approvals_file never runs, so the follow-up wt config update the warning asks for reports No deprecated settings found and the entries are gone for good. test_config_update_output_writes_the_config_it_migrates asserts both halves — !written.contains("approved-commands") and a stderr line pointing at wt config update. Either copy the approvals first when the destination is the source (which is what would make "write the path the user named" non-lossy), or reword the warning for that case so it doesn't send the user at a no-op — "they re-approve" from the description is the honest version, and the message should say it rather than the opposite.

Comment thread src/commands/config/create.rs Outdated
max-sixty and others added 2 commits September 20, 2026 15:59
…igrated

The reviewer's hold found a real defect. `--output` pointed at the config being
migrated wrote the migration over it, dropping `approved-commands`, and printed
a warning whose remedy — "to migrate them to approvals.toml, run
`wt config update`" — was false exactly then: the write had already removed
them, so the follow-up run reports `No deprecated settings found` and the
entries are gone.

`copy_approved_commands_to_approvals_file` had one production caller, the
in-place loop. That block is now `move_approvals_out_of`, and both writes that
land a migration on top of the config it came from call it — the in-place
update, and `--output` naming that same path. Any other destination leaves the
config alone, so its warning still names a `wt config update` with something to
migrate. "Write the path the user named" is no longer lossy.

`wt config create` on a dangling symlink reported `File exists (os error 17)`,
naming neither the path nor the link, right after the existence check said the
path was free. It now says what occupies it and what to do, as the rc-file
write already did, with the symlink case covered by a test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three patch lines went uncovered. Two came from shape rather than behavior:

- `move_approvals_out_of` split the original `if … && let Some(…)` chain into
  two early returns, so "approvals.toml already exists" — a reachable,
  benign no-op — became its own line no test reached. It is back to the
  chain, one condition and one body.
- `wt config create` gave the file-appeared-in-the-window race its own message.
  That arm has no deterministic trigger, which is the sign it needn't be one;
  it is now an ordinary write failure, which names the path. Only the dangling
  symlink keeps a message of its own, since `exists()` reads false through it
  and a dotfile manager's link reaches the write every time.

The remaining arm, a write that fails for any other reason, is now covered by
a read-only config directory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2fd858f adds .tmp-codecov.json at the repo root — the raw Codecov API response for this PR, 180 KB on a single line, not matched by any .gitignore pattern (git check-ignore reports it unignored). The commit message covers the three coverage arms and doesn't mention it, so it reads as a leftover from the coverage investigation rather than something intended to ship. It also pins a snapshot of this PR's own coverage totals against 812347f, which is stale as of the commit that added it.

git rm .tmp-codecov.json is the whole fix. Happy to push that if you'd rather not spend a round-trip on it.

The verdict stays withheld for the reason the earlier review gave — the diff removes a gate on overwriting a config file, which .claude/skills/running-tend/references/review-pr.md ("Data-Loss Surface: Hold for Human Review") routes to you rather than to the bot. Nothing new on that front: the approvals defect that review raised is fixed by 812347f, and I've resolved the config/create.rs thread — the error now distinguishes the dangling-symlink case and has a test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 2e12077 into main Sep 21, 2026
37 checks passed
@max-sixty
max-sixty deleted the release branch September 21, 2026 00:05
max-sixty added a commit that referenced this pull request Sep 21, 2026
Verification passes against the v0.78.0 binary found five inaccuracies
in the 0.79.0 changelog. The tag publishes this text as the GitHub
release body, so each is corrected here rather than after the fact.

- **The `--output` entry described behavior 0.78.0 already had.**
Replacing an existing destination is not new; what #4204 changed is that
`--output` pointed at the config being migrated is no longer refused.
The entry now claims only that, and moves out of the shared bullet with
#4153.
- **`wt step prune` shares the integration-check defect**, not just `wt
remove` — both reach it through the same check, so both are named.
- **Any earlier command could poison the `has-added-changes` cache**,
not only the removal itself, so the condition reads "a Worktrunk command
run from a subdirectory" rather than "the command".
- **Only `submodule.<name>.ignore = all` triggers it.** `untracked` and
`dirty` leave a submodule pointer bump visible, so the value is named.
- **The headline "no longer changes what Worktrunk reads" overstated
completeness** — `git status` has no plumbing equivalent. Scoped to the
prompts and the integration check, which is what this release fixes.

Two further changes: "entirely" is restored to the `diff.relative`
condition, where a repro confirmed it load-bearing (a branch also
touching a file inside the subdirectory is retained), and `wt config
create`'s dangling-symlink fix from #4204 gets the entry it was missing.

Entry order within Fixed follows reader interest; no code changes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants