Skip to content

fix(config): three independent deprecation-layer correctness fixes - #2783

Merged
max-sixty merged 6 commits into
mainfrom
clawpatch-config
May 17, 2026
Merged

fix(config): three independent deprecation-layer correctness fixes#2783
max-sixty merged 6 commits into
mainfrom
clawpatch-config

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

Summary

Three independent correctness fixes in the config-deprecation layer, found during a static review pass. All touch only src/config/deprecation.rs (plus src/commands/config/update.rs for the approvals-copy fix), so they review and land cleanly on their own.

  • Preserve malformed deprecated sections and other [ci] keys — structural migration peeked-and-removed in a way that could drop a deprecated section that failed to parse as a table, and stripped the whole [ci] table instead of just the migrated platform key. Now it peeks before removing, only removes platform, and drops [ci] only when it is left empty — so unrelated [ci] keys and unparseable sections survive migration instead of being silently discarded.
  • Surface approvals-copy failures so config update aborts before rewritingcopy_approved_commands_to_approvals_file swallowed I/O errors; a failed copy (e.g. read-only directory) let the rewrite proceed and silently drop approvals. It now returns a Result and the caller propagates with ?, so a copy failure aborts the update instead of losing data.
  • Migrate deprecated template vars on the toml_edit tree, not raw text — variable renaming did a raw str::replace on the file text, which corrupted occurrences inside escaped strings. Migration now rewrites the parsed toml_edit document and serializes from the tree.

Test plan

  • cargo check clean on this branch off main
  • Per-fix regression tests added (malformed-section preservation, read-only-dir approvals-copy failure, escaped-quote template-var migration)
  • CI green

🤖 Generated with Claude Code

max-sixty and others added 3 commits May 17, 2026 14:31
Three migrations could silently drop user config when other migrations ran:

- migrate_commit_generation_doc / migrate_select_table removed the deprecated
  section before checking whether the value was a table. If it was malformed
  (e.g. `commit-generation = "string"`), `modified` stayed `false` and the
  function returned without re-emitting the doc — but if any sibling migration
  applied, the mutated (now empty) doc was serialized, silently dropping the
  malformed value.

- migrate_ci_doc removed the entire `[ci]` table after copying only `platform`,
  silently dropping any other keys in that section.

Peek before removing; remove only the migrated key from `[ci]` and drop the
section only when it becomes empty. Add regression tests for malformed
sibling-migration cases and for `[ci]` with extra keys, and tighten the
existing malformed-value tests to assert the value is preserved rather than
just absent from the new section.

Co-Authored-By: Claude <noreply@anthropic.com>
…rewriting

`copy_approved_commands_to_approvals_file` returned `Option<PathBuf>`,
collapsing read/parse/save errors into the same `None` used for the benign
"nothing to copy" no-op. `wt config update` then rewrote config.toml,
silently dropping the legacy approved-commands when the copy actually failed
(e.g. unwritable approvals.toml path).

Change the return type to `Result<Option<PathBuf>>` and propagate the error
from `update`, so a failed copy aborts the whole update with the underlying
error rather than dropping approvals. `Ok(None)` is kept for the genuine
no-op cases. Add a Unix regression test that makes the directory read-only
and asserts the failure is now `Err`.

Co-Authored-By: Claude <noreply@anthropic.com>
`replace_deprecated_vars_from_strings` modified the *decoded* TOML string
value and then did `content.replace(decoded, migrated)` against the raw file
text. When the source used escapes (e.g. `pre-start = "echo \"{{ repo_root
}}\""`), the decoded value (`echo "{{ repo_root }}"`) is not present
verbatim in the file, so the replace found nothing and the migration was
silently skipped — while detection still warned, so `config update` could
report a deprecated var yet write the file back unchanged.

Replace the raw-text path with `replace_deprecated_vars_in_doc`, which walks
the parsed `toml_edit` tree and rewrites each string value in place; toml_edit
re-serializes with correct escaping. `compute_migrated_content` now parses
once and threads one document through var + structural migration. Decor is
preserved per-string so unchanged strings keep their original representation.

Regression tests cover the escaped-quote case at both the helper and
`compute_migrated_content` level.

Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes May 17, 2026

@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.

All three fixes look right and the tests pin the previously-broken behavior. One non-blocking nit on the new permission test (inline).

Comment thread src/config/deprecation.rs

@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.

codecov/patch is failing at 91.54% vs 97.05% threshold — 17 uncovered lines on src/config/deprecation.rs. Most are easy to close; a few are defensive branches that may or may not be worth testing. Dismissing my approval per the CLAUDE.md codecov rule until this is resolved.

Easy to cover with one or two extra cases:

  • InlineTable arm in walk_value (lines 262–267) — never exercised. Adding a config that puts a deprecated var inside an inline table (e.g. entry = { cmd = "{{ repo_root }}" }) would hit it.
  • with_context on load_from_config_file failure (lines 1119–1123) — needs a test where the load itself fails. Writing a config with a malformed [projects."..."] block (e.g. approved-commands = "not a list") would surface the error from the strict serde path.
  • compute_migrated_content no-op return (line 1380, the content.to_string() branch) — a test passing a fully-clean config (no deprecations) would hit it.

Defensive / probably-unreachable (worth a quick decision):

  • ArrayOfTables arm in walk_item (lines 238–243) and the trailing _ => {} — worktrunk configs don't use [[arr]], but the existing collect_strings_from_edit_item walker has the same arm. Either keep symmetric and add a #[cfg(test)] exercise via a synthetic [[arr]] doc, or drop the arm (and the symmetric one in collect_strings_from_edit_item for consistency).
  • _ => None in into_table (line 453) — guarded by is_table_like, so never hit. Could be replaced with unreachable!() (and the expect call sites simplified), which removes the branch from coverage entirely.
  • Test helper's invalid-TOML fallback (line 1719) — all current callers pass valid TOML. Could collapse replace_deprecated_vars to call .parse().unwrap() and drop the early return.

Happy to push a coverage-fix commit if you'd like — let me know.

@worktrunk-bot
worktrunk-bot dismissed their stale review May 17, 2026 21:49

codecov/patch failing (91.54% vs 97.05% threshold); see follow-up comment for the uncovered lines

max-sixty and others added 2 commits May 17, 2026 14:50
Adds unit tests for the deprecation infrastructure paths flagged by
codecov/patch on the config-fix branch — the general tree-walker arms
(array-of-tables, inline-table, non-string scalar), the into_table
non-table contract, the compute_migrated_content no-op branch, the
replace_deprecated_vars parse-error guard, and the approvals-copy
read/parse failure path. Single-deprecation-specific branches (empty
[ci] cleanup) are intentionally left out of scope.

Co-Authored-By: Claude <noreply@anthropic.com>
Root ignores directory permissions, so the 0o555 read-only tempdir is a
no-op and the Err assertion would spuriously fail on root environments
(Claude Code web, Docker). Probe-and-skip, matching the established
pattern in tests/integration_tests/approval_save.rs.

Co-Authored-By: Claude <noreply@anthropic.com>
@max-sixty
max-sixty enabled auto-merge (squash) May 17, 2026 22:09
@max-sixty
max-sixty merged commit 2286d30 into main May 17, 2026
34 checks passed
@max-sixty
max-sixty deleted the clawpatch-config branch May 17, 2026 22:10
max-sixty added a commit that referenced this pull request May 19, 2026
…s scalar

Three data-loss edges in `compute_migrated_content`:

- `commit = "x"` + `[commit-generation]`: `doc.contains_key("commit")` was
  true so no table was created, then `doc["commit"].as_table_mut()` was None,
  so the migrated `[commit.generation]` was never inserted — but the source
  was already removed. Same shape for `switch = "x"` + `[select]`.
- `args = [1, "--ok"]`: the string filter silently discarded the non-string
  element while removing `args`.

Add `can_host_subtable` (parent absent or table) and gate source removal on
it for both top-level and project-level commit-generation and for select.
Require every `args` element be a string before merging; otherwise preserve
`args` unchanged. Regression tests for all three.

Reconstructed against current main: the original clawpatch implementation
sat on a divergent #2783 baseline and its 4-commit Group C stack would not
3-way-merge cleanly on top of #2806/#2783 main without hand-merging
config-migration logic — the exact code where silent corruption is the
defect. Re-derived as one focused change.

Co-Authored-By: Claude <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