* docs(plans): toml_edit migration implementation plan

Fifteen-task implementation plan for the toml_edit migration specced in
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md (PR #160).

Task order: scaffold ConfigEditor + tripwire test; env setters + comment
preservation tests (the feature this migration exists for); mount / agent
/ workspace editor methods; migrate 13 call sites across app/mod.rs,
app/context.rs, runtime/launch.rs, persist.rs; delete AppConfig::save
and the dead in-memory mutators; final verification gate before PR.

Every task is TDD-shaped (failing test → impl → green → commit) with
complete code in each step. No placeholders, no "TBD".

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): scaffold ConfigEditor with toml_edit round-trip

Introduces src/config/editor.rs with a ConfigEditor struct that owns a
toml_edit::DocumentMut. ConfigEditor::open loads and parses, save writes
atomically (.tmp + fsync + rename, 0o600 on unix) and returns a
freshly-deserialized AppConfig parsed directly from the written content
(bypassing load_or_init's builtin-agent sync to avoid clobbering the
toml_edit document with a serde round-trip).

No mutators yet — subsequent commits add typed setters and migrate call
sites off AppConfig::save.

Tripwire test (idempotent_save_is_byte_identical) pins that an open →
save with no mutations produces byte-identical output, including
hand-written comments. If this ever fails, toml_edit is round-tripping
lossily.

toml_edit 0.22.27 resolved in Cargo.lock.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* docs(config): clarify ConfigEditor::save validation-bypass invariant

Code-quality review flagged that save()'s docstring only mentioned the
cycle-avoidance reason for bypassing load_or_init, not the second
equally-important property: toml::from_str also skips validate_workspaces
and validate_reserved_names.

Document the invariant the bypass relies on: validation runs once at
load time (via open → load_or_init, or via AppConfig::edit_workspace for
structural workspace changes), and the editor's typed setters preserve
validity by construction.

No behavior change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::set_env_var with scope-table upsert

Adds set_env_var for all four EnvScope variants (Global, Agent,
Workspace, WorkspaceAgent). Intermediate tables are created as needed
via the private table_path_mut helper, so writing to
WorkspaceAgent { workspace: "new", agent: "new" } works even when
neither table exists.

Existing values are overwritten. Tests cover the upsert, cross-scope
path construction, and overwrite semantics.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::remove_env_var

Removes an env var from any scope, returning true if the key existed
and false if it did not (including when the scope's table is missing).
Does not create intermediate tables — removes-from-nothing is a no-op.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::set_env_comment

Sets or removes a "# comment\n" line immediately above an env var
entry via toml_edit's leaf_decor_mut on the key. The secrets screen
(PR 3 of this series) will use this to annotate ID-form op://
references with their human-readable name.

Tests cover: adding a new comment, replacing an existing one, removing
via None, and — critically — that mutating a sibling key does NOT
disturb the comment above an unrelated key in the same table.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* test(config): fixture round-trip + cross-table comment preservation

Adds a realistic fixture (mixed comments, quoted keys with slashes,
nested env tables) and asserts open → save is byte-identical. This is
the main smoke test that toml_edit's round-trip actually preserves
everything we care about in a real config.

Also adds a cross-table test: mutating workspaces.a's env does not
disturb comments attached to workspaces.b.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* test(config): pin ConfigEditor atomic-write parity

Asserts the saved config is 0600 on unix and that the .tmp sidecar does
not outlive a successful save. Matches the invariants of the existing
AppConfig::save body that ConfigEditor::save lifted.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::add_mount, remove_mount

Preserves the existing (name, mount, scope: Option<&str>) call shape so
call-site migrations stay mechanical. Unscoped writes [docker.mounts.N];
scoped writes [docker.mounts.scope_key] with name as the inner key —
matching AppConfig::add_mount's MountEntry::Scoped storage convention
(scope_key is the outer key, name is the inner key, not the reverse).
remove_mount returns whether the entry existed.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): remove_mount cleans up empty scope table

Matches AppConfig::remove_mount (mounts.rs:127-131) which removes the
scope_key entry from docker.mounts when the last named mount inside it
is removed. The editor version was leaving empty [docker.mounts.<scope>]
tables behind.

Two new tests pin the behavior:
- remove_mount_scoped_last_entry_deletes_scope_table: last mount → scope gone
- remove_mount_scoped_preserves_scope_when_siblings_remain: still has siblings → scope stays

Found when comparing against AppConfig::remove_mount during Task 7 spec
review. The plan's Task 7 code omitted this cleanup branch.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor agent mutators

Adds set_agent_trust, set_agent_auth_forward, set_global_auth_forward,
upsert_builtin_agent. The builtin upsert touches only git + trusted so
operator-owned [agents.X.claude] and [agents.X.env] overrides survive
the sync that runs on every load_or_init.

set_agent_trust(false) removes the trusted key entirely to match the
canonical serde shape (skip_serializing_if = is_false).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor workspace mutators

Adds create_workspace, edit_workspace, remove_workspace, set_last_agent.

create_workspace collision-checks then splats a serialized
WorkspaceConfig into [workspaces.<name>]. edit_workspace delegates to
AppConfig::edit_workspace's validated logic (mount upserts,
allowed_agents diffs, workdir validation) and then replaces the
[workspaces.<name>] table — comments inside the edited workspace are
consumed (that IS the change), but all other sections survive intact.

set_last_agent is a targeted insert that preserves every other field
on the workspace, including default_agent.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): create_workspace delegates to AppConfig for validation

Matches edit_workspace's delegation pattern. The editor's
create_workspace now runs AppConfig::create_workspace's full validation
(collision check, workdir/mount-destination relationship) before
splatting the result into the doc — previously it only checked
collision and skipped the workdir/mount validation that today's
CLI path (config.create_workspace) provides.

Without this, Task 10's call-site migration at app/mod.rs:386 would
have silently lost validation: users who asked jackin to create a
workspace with a workdir that doesn't line up with any mount dst would
have gotten a quietly-invalid config instead of the usual error.

New test create_workspace_rejects_invalid_workdir_mount_combo pins
the behavior.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::upsert_agent_source

Persists git + trusted from an AgentSource into [agents.<key>], keeps
any existing claude subtable intact, and never touches [agents.<key>.env].

Needed by the app/mod.rs trust and auth_forward call sites: they invoke
resolve_agent_source (which may insert a new agent entry into the
in-memory AppConfig), and the editor must persist that insert alongside
the trust or auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(app): route config writes through ConfigEditor

Migrates the ten config.mutate(); config.save() pairs in src/app/mod.rs
(mounts, trust, auth_forward, workspace create/edit/remove) to the new
ConfigEditor::open → mutate → save shape. AppConfig::save still exists
at this point; it gets removed in a later commit once all callers are
migrated.

Sites that first call resolve_agent_source (trust grant/revoke,
auth_forward) now also call ConfigEditor::upsert_agent_source so that
any newly-inserted agent entry gets persisted alongside the trust or
auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(app): route last-used-agent save through ConfigEditor

Pre-flush the in-memory config to disk (ensuring the directory exists
via ensure_base_dirs) then open a ConfigEditor, apply set_last_agent,
and replace the in-memory config with the freshly-deserialized result.
Preserves all five behavioral contracts: early-exit on load failure,
no-op when workspace_name is None, no-op when workspace is absent,
the existing save-failure warning, and adds an open-failure warning.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(app): remember_last_agent drops pre-flush, tests persist init state

The Task 11 pre-flush (config.save(paths) before opening the editor)
kept the lossy serde write alive on the production path, destroying
any user-written comments in the config on every successful launch —
the exact thing this migration exists to prevent.

Root cause was a test setup issue, not production code: the two tests
constructed an in-memory AppConfig and called remember_last_agent
without first persisting the config to disk. Production always has
config on disk (loaded at startup), so the pre-flush was solving a
test-only problem in the wrong place.

Fix: move the persistence step into a test helper
(persisted_config_with_workspace) that writes the initial AppConfig via
toml::to_string_pretty + fs::write. Revert the production path to just
open → set_last_agent → save, which now preserves comments as intended.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(launch): route newly-trusted agent save through ConfigEditor

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::normalize_deprecated_copy

Rewrites any auth_forward = "copy" to "sync" at the two paths
contains_deprecated_copy_auth_forward checks (global [claude] and
[agents.*.claude]). Used by load_or_init's deprecated-copy migration
path so user comments and blank lines around the changed keys survive.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(config): route load_or_init migrations through ConfigEditor

The builtin-sync and deprecated-copy migrations run in-memory on
AppConfig as before, but the resulting save goes through
ConfigEditor::upsert_builtin_agent and normalize_deprecated_copy
instead of AppConfig::save. User comments and blank lines in config
sections untouched by the migration now survive the first load after
upgrade.

To break the recursion that would arise when the config file does not
yet exist (ConfigEditor::open calls AppConfig::load_or_init if absent),
a bootstrap AppConfig::save is issued first to materialize the file,
then the editor reopens it and applies comment-preserving mutations on
top.

Critically, config = editor.save()? at the end replaces the in-memory
config with a fresh parse from the on-disk file. This matters because
sync_builtin_agents clears [agents.X.env] on any updated builtin (it
constructs a fresh AgentSource with empty env), but upsert_builtin_agent
leaves env alone. Using editor.save()'s return value means operators
keep their [agents.X.env] across builtin syncs.

BUILTIN_AGENTS visibility changed from pub(super) to pub(crate) so
persist.rs can iterate it directly.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): gate load_or_init bootstrap save on missing file

The Task 13 implementation issued AppConfig::save(paths)? unconditionally
before opening the editor, to avoid ConfigEditor::open's recursion into
load_or_init when the file is missing. That regressed comment preservation
on the exact upgrade paths this migration protects: a user with
auth_forward = "copy" and hand-written comments would have those comments
destroyed by the bootstrap serde write before the editor got a chance to
preserve them.

Root cause: the recursion only happens when the file DOES NOT exist. On
the upgrade path (file exists, needs builtin-sync or copy→sync migration),
ConfigEditor::open reads the existing file directly without recursing.

Fix: gate the bootstrap config.save on !paths.config_file.exists(). First
run (no file) still materializes defaults via the lossy writer — fine,
because there are no user comments to preserve yet. Existing-file upgrades
skip the bootstrap and go straight to the comment-preserving editor path.

New regression test load_migration_preserves_user_comments pins the
behavior: a config with hand-written comments and deprecated "copy"
auth_forward retains its comments through the migration.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(config): remove AppConfig::save and dead mutator methods

AppConfig::save is removed from the public surface; its body is inlined
into load_or_init's bootstrap branch (the only remaining caller) with
full atomic-write semantics preserved (.tmp + fsync + rename + 0o600 on
unix). No save method survives at any visibility.

ConfigEditor is now the sole write path. The in-memory mutator methods
are demoted or deleted:

- trust_agent, untrust_agent, set_agent_auth_forward → pub(crate) with
  #[cfg_attr(not(test), allow(dead_code))]; test-only affordances used
  by the unit tests in config/agents.rs and config/mod.rs. Production
  callers use ConfigEditor.

- add_mount → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  test-only affordance used by launch/preview.rs and workspace/resolve.rs
  for in-memory AppConfig setup; does not persist to disk.

- remove_mount → deleted entirely; no test or production caller remained
  after the migration.

- create_workspace, edit_workspace → pub(crate); ConfigEditor::create_workspace
  and ConfigEditor::edit_workspace delegate here for validation, so these
  must remain reachable within the crate.

- remove_workspace → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  kept for the workspaces.rs unit test that validates error message shape.
  ConfigEditor::remove_workspace operates directly on the TOML document
  and does not delegate here.

The two test callers of config.save() in config/agents.rs are rewritten
to use toml::to_string_pretty + std::fs::write for their bootstrap writes.

tests/workspace_config_crud.rs (integration tests) is rewritten to use
ConfigEditor instead of AppConfig directly, since pub(crate) visibility
is not accessible from integration test crates. The tests retain the same
assertions; the only change is the creation/edit path goes through the
editor rather than bare in-memory mutation.

Unused assignments (config = editor.save()?;) in app/mod.rs are fixed by
dropping the lhs; the match arms already returned Ok(()) immediately after,
making every such write dead. These are the unused_assignments flagged in
Task 10 that Task 14 was responsible for cleaning up.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* style(config): run cargo fmt

CI's cargo fmt --check flagged formatting in src/config/editor.rs
(mostly short-vec splat-vs-single-line choices from the table-path
constructions added across Tasks 2–13). Pure formatting — no logic
change. Caught by CI, not locally, because the worktree was running
cargo check / cargo test but not cargo fmt --check as a pre-push gate.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* style(config): quiet clippy lints introduced by migration

CI runs cargo clippy -- -D warnings (lib target only). The toml_edit
migration introduced 21 new lint errors across src/app/mod.rs,
src/config/editor.rs, and src/config/agents.rs. All are clippy-suggested
mechanical transforms — no behavior change.

Categories fixed: map-unwrap-or → is_some_and, if-not-else inversion,
needless pass-by-value on EnvScope, collapsible-if, explicit-iter-loop,
const-fn for auth_forward_str, missing-backticks in doc comments.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

---------

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

…160)

Add the design spec for PR 1 of 3 in the launcher-workspace-manager
series. Replaces the current full-reserialize writer with a targeted
toml_edit patch path so hand-written comments, blank lines, and key
ordering survive every programmatic save.

Needed because the upcoming secrets screen (spec 3) annotates ID-form
1Password references with the human-readable name as a mid-document
comment. The current writer discards all comments on any save, so
annotations would not survive the next unrelated trust toggle or
last-used-agent update.

Introduces ConfigEditor with typed setters for the 13 existing write
sites plus forward-looking env methods for spec 3. AppConfig::save is
removed; load_or_init stays. No schema change, no operator-facing
migration.

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

* docs(plans): toml_edit migration implementation plan

Fifteen-task implementation plan for the toml_edit migration specced in
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md (PR #160).

Task order: scaffold ConfigEditor + tripwire test; env setters + comment
preservation tests (the feature this migration exists for); mount / agent
/ workspace editor methods; migrate 13 call sites across app/mod.rs,
app/context.rs, runtime/launch.rs, persist.rs; delete AppConfig::save
and the dead in-memory mutators; final verification gate before PR.

Every task is TDD-shaped (failing test → impl → green → commit) with
complete code in each step. No placeholders, no "TBD".

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): scaffold ConfigEditor with toml_edit round-trip

Introduces src/config/editor.rs with a ConfigEditor struct that owns a
toml_edit::DocumentMut. ConfigEditor::open loads and parses, save writes
atomically (.tmp + fsync + rename, 0o600 on unix) and returns a
freshly-deserialized AppConfig parsed directly from the written content
(bypassing load_or_init's builtin-agent sync to avoid clobbering the
toml_edit document with a serde round-trip).

No mutators yet — subsequent commits add typed setters and migrate call
sites off AppConfig::save.

Tripwire test (idempotent_save_is_byte_identical) pins that an open →
save with no mutations produces byte-identical output, including
hand-written comments. If this ever fails, toml_edit is round-tripping
lossily.

toml_edit 0.22.27 resolved in Cargo.lock.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* docs(config): clarify ConfigEditor::save validation-bypass invariant

Code-quality review flagged that save()'s docstring only mentioned the
cycle-avoidance reason for bypassing load_or_init, not the second
equally-important property: toml::from_str also skips validate_workspaces
and validate_reserved_names.

Document the invariant the bypass relies on: validation runs once at
load time (via open → load_or_init, or via AppConfig::edit_workspace for
structural workspace changes), and the editor's typed setters preserve
validity by construction.

No behavior change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::set_env_var with scope-table upsert

Adds set_env_var for all four EnvScope variants (Global, Agent,
Workspace, WorkspaceAgent). Intermediate tables are created as needed
via the private table_path_mut helper, so writing to
WorkspaceAgent { workspace: "new", agent: "new" } works even when
neither table exists.

Existing values are overwritten. Tests cover the upsert, cross-scope
path construction, and overwrite semantics.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::remove_env_var

Removes an env var from any scope, returning true if the key existed
and false if it did not (including when the scope's table is missing).
Does not create intermediate tables — removes-from-nothing is a no-op.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::set_env_comment

Sets or removes a "# comment\n" line immediately above an env var
entry via toml_edit's leaf_decor_mut on the key. The secrets screen
(PR 3 of this series) will use this to annotate ID-form op://
references with their human-readable name.

Tests cover: adding a new comment, replacing an existing one, removing
via None, and — critically — that mutating a sibling key does NOT
disturb the comment above an unrelated key in the same table.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* test(config): fixture round-trip + cross-table comment preservation

Adds a realistic fixture (mixed comments, quoted keys with slashes,
nested env tables) and asserts open → save is byte-identical. This is
the main smoke test that toml_edit's round-trip actually preserves
everything we care about in a real config.

Also adds a cross-table test: mutating workspaces.a's env does not
disturb comments attached to workspaces.b.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* test(config): pin ConfigEditor atomic-write parity

Asserts the saved config is 0600 on unix and that the .tmp sidecar does
not outlive a successful save. Matches the invariants of the existing
AppConfig::save body that ConfigEditor::save lifted.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::add_mount, remove_mount

Preserves the existing (name, mount, scope: Option<&str>) call shape so
call-site migrations stay mechanical. Unscoped writes [docker.mounts.N];
scoped writes [docker.mounts.scope_key] with name as the inner key —
matching AppConfig::add_mount's MountEntry::Scoped storage convention
(scope_key is the outer key, name is the inner key, not the reverse).
remove_mount returns whether the entry existed.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): remove_mount cleans up empty scope table

Matches AppConfig::remove_mount (mounts.rs:127-131) which removes the
scope_key entry from docker.mounts when the last named mount inside it
is removed. The editor version was leaving empty [docker.mounts.<scope>]
tables behind.

Two new tests pin the behavior:
- remove_mount_scoped_last_entry_deletes_scope_table: last mount → scope gone
- remove_mount_scoped_preserves_scope_when_siblings_remain: still has siblings → scope stays

Found when comparing against AppConfig::remove_mount during Task 7 spec
review. The plan's Task 7 code omitted this cleanup branch.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor agent mutators

Adds set_agent_trust, set_agent_auth_forward, set_global_auth_forward,
upsert_builtin_agent. The builtin upsert touches only git + trusted so
operator-owned [agents.X.claude] and [agents.X.env] overrides survive
the sync that runs on every load_or_init.

set_agent_trust(false) removes the trusted key entirely to match the
canonical serde shape (skip_serializing_if = is_false).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor workspace mutators

Adds create_workspace, edit_workspace, remove_workspace, set_last_agent.

create_workspace collision-checks then splats a serialized
WorkspaceConfig into [workspaces.<name>]. edit_workspace delegates to
AppConfig::edit_workspace's validated logic (mount upserts,
allowed_agents diffs, workdir validation) and then replaces the
[workspaces.<name>] table — comments inside the edited workspace are
consumed (that IS the change), but all other sections survive intact.

set_last_agent is a targeted insert that preserves every other field
on the workspace, including default_agent.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): create_workspace delegates to AppConfig for validation

Matches edit_workspace's delegation pattern. The editor's
create_workspace now runs AppConfig::create_workspace's full validation
(collision check, workdir/mount-destination relationship) before
splatting the result into the doc — previously it only checked
collision and skipped the workdir/mount validation that today's
CLI path (config.create_workspace) provides.

Without this, Task 10's call-site migration at app/mod.rs:386 would
have silently lost validation: users who asked jackin to create a
workspace with a workdir that doesn't line up with any mount dst would
have gotten a quietly-invalid config instead of the usual error.

New test create_workspace_rejects_invalid_workdir_mount_combo pins
the behavior.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::upsert_agent_source

Persists git + trusted from an AgentSource into [agents.<key>], keeps
any existing claude subtable intact, and never touches [agents.<key>.env].

Needed by the app/mod.rs trust and auth_forward call sites: they invoke
resolve_agent_source (which may insert a new agent entry into the
in-memory AppConfig), and the editor must persist that insert alongside
the trust or auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(app): route config writes through ConfigEditor

Migrates the ten config.mutate(); config.save() pairs in src/app/mod.rs
(mounts, trust, auth_forward, workspace create/edit/remove) to the new
ConfigEditor::open → mutate → save shape. AppConfig::save still exists
at this point; it gets removed in a later commit once all callers are
migrated.

Sites that first call resolve_agent_source (trust grant/revoke,
auth_forward) now also call ConfigEditor::upsert_agent_source so that
any newly-inserted agent entry gets persisted alongside the trust or
auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(app): route last-used-agent save through ConfigEditor

Pre-flush the in-memory config to disk (ensuring the directory exists
via ensure_base_dirs) then open a ConfigEditor, apply set_last_agent,
and replace the in-memory config with the freshly-deserialized result.
Preserves all five behavioral contracts: early-exit on load failure,
no-op when workspace_name is None, no-op when workspace is absent,
the existing save-failure warning, and adds an open-failure warning.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(app): remember_last_agent drops pre-flush, tests persist init state

The Task 11 pre-flush (config.save(paths) before opening the editor)
kept the lossy serde write alive on the production path, destroying
any user-written comments in the config on every successful launch —
the exact thing this migration exists to prevent.

Root cause was a test setup issue, not production code: the two tests
constructed an in-memory AppConfig and called remember_last_agent
without first persisting the config to disk. Production always has
config on disk (loaded at startup), so the pre-flush was solving a
test-only problem in the wrong place.

Fix: move the persistence step into a test helper
(persisted_config_with_workspace) that writes the initial AppConfig via
toml::to_string_pretty + fs::write. Revert the production path to just
open → set_last_agent → save, which now preserves comments as intended.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(launch): route newly-trusted agent save through ConfigEditor

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* feat(config): ConfigEditor::normalize_deprecated_copy

Rewrites any auth_forward = "copy" to "sync" at the two paths
contains_deprecated_copy_auth_forward checks (global [claude] and
[agents.*.claude]). Used by load_or_init's deprecated-copy migration
path so user comments and blank lines around the changed keys survive.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(config): route load_or_init migrations through ConfigEditor

The builtin-sync and deprecated-copy migrations run in-memory on
AppConfig as before, but the resulting save goes through
ConfigEditor::upsert_builtin_agent and normalize_deprecated_copy
instead of AppConfig::save. User comments and blank lines in config
sections untouched by the migration now survive the first load after
upgrade.

To break the recursion that would arise when the config file does not
yet exist (ConfigEditor::open calls AppConfig::load_or_init if absent),
a bootstrap AppConfig::save is issued first to materialize the file,
then the editor reopens it and applies comment-preserving mutations on
top.

Critically, config = editor.save()? at the end replaces the in-memory
config with a fresh parse from the on-disk file. This matters because
sync_builtin_agents clears [agents.X.env] on any updated builtin (it
constructs a fresh AgentSource with empty env), but upsert_builtin_agent
leaves env alone. Using editor.save()'s return value means operators
keep their [agents.X.env] across builtin syncs.

BUILTIN_AGENTS visibility changed from pub(super) to pub(crate) so
persist.rs can iterate it directly.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* fix(config): gate load_or_init bootstrap save on missing file

The Task 13 implementation issued AppConfig::save(paths)? unconditionally
before opening the editor, to avoid ConfigEditor::open's recursion into
load_or_init when the file is missing. That regressed comment preservation
on the exact upgrade paths this migration protects: a user with
auth_forward = "copy" and hand-written comments would have those comments
destroyed by the bootstrap serde write before the editor got a chance to
preserve them.

Root cause: the recursion only happens when the file DOES NOT exist. On
the upgrade path (file exists, needs builtin-sync or copy→sync migration),
ConfigEditor::open reads the existing file directly without recursing.

Fix: gate the bootstrap config.save on !paths.config_file.exists(). First
run (no file) still materializes defaults via the lossy writer — fine,
because there are no user comments to preserve yet. Existing-file upgrades
skip the bootstrap and go straight to the comment-preserving editor path.

New regression test load_migration_preserves_user_comments pins the
behavior: a config with hand-written comments and deprecated "copy"
auth_forward retains its comments through the migration.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* refactor(config): remove AppConfig::save and dead mutator methods

AppConfig::save is removed from the public surface; its body is inlined
into load_or_init's bootstrap branch (the only remaining caller) with
full atomic-write semantics preserved (.tmp + fsync + rename + 0o600 on
unix). No save method survives at any visibility.

ConfigEditor is now the sole write path. The in-memory mutator methods
are demoted or deleted:

- trust_agent, untrust_agent, set_agent_auth_forward → pub(crate) with
  #[cfg_attr(not(test), allow(dead_code))]; test-only affordances used
  by the unit tests in config/agents.rs and config/mod.rs. Production
  callers use ConfigEditor.

- add_mount → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  test-only affordance used by launch/preview.rs and workspace/resolve.rs
  for in-memory AppConfig setup; does not persist to disk.

- remove_mount → deleted entirely; no test or production caller remained
  after the migration.

- create_workspace, edit_workspace → pub(crate); ConfigEditor::create_workspace
  and ConfigEditor::edit_workspace delegate here for validation, so these
  must remain reachable within the crate.

- remove_workspace → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  kept for the workspaces.rs unit test that validates error message shape.
  ConfigEditor::remove_workspace operates directly on the TOML document
  and does not delegate here.

The two test callers of config.save() in config/agents.rs are rewritten
to use toml::to_string_pretty + std::fs::write for their bootstrap writes.

tests/workspace_config_crud.rs (integration tests) is rewritten to use
ConfigEditor instead of AppConfig directly, since pub(crate) visibility
is not accessible from integration test crates. The tests retain the same
assertions; the only change is the creation/edit path goes through the
editor rather than bare in-memory mutation.

Unused assignments (config = editor.save()?;) in app/mod.rs are fixed by
dropping the lhs; the match arms already returned Ok(()) immediately after,
making every such write dead. These are the unused_assignments flagged in
Task 10 that Task 14 was responsible for cleaning up.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* style(config): run cargo fmt

CI's cargo fmt --check flagged formatting in src/config/editor.rs
(mostly short-vec splat-vs-single-line choices from the table-path
constructions added across Tasks 2–13). Pure formatting — no logic
change. Caught by CI, not locally, because the worktree was running
cargo check / cargo test but not cargo fmt --check as a pre-push gate.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

* style(config): quiet clippy lints introduced by migration

CI runs cargo clippy -- -D warnings (lib target only). The toml_edit
migration introduced 21 new lint errors across src/app/mod.rs,
src/config/editor.rs, and src/config/agents.rs. All are clippy-suggested
mechanical transforms — no behavior change.

Categories fixed: map-unwrap-or → is_some_and, if-not-else inversion,
needless pass-by-value on EnvScope, collapsible-if, explicit-iter-loop,
const-fn for auth_forward_str, missing-backticks in doc comments.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>

---------

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

…160)

Add the design spec for PR 1 of 3 in the launcher-workspace-manager
series. Replaces the current full-reserialize writer with a targeted
toml_edit patch path so hand-written comments, blank lines, and key
ordering survive every programmatic save.

Needed because the upcoming secrets screen (spec 3) annotates ID-form
1Password references with the human-readable name as a mid-document
comment. The current writer discards all comments on any save, so
annotations would not survive the next unrelated trust toggle or
last-used-agent update.

Introduces ConfigEditor with typed setters for the 13 existing write
sites plus forward-looking env methods for spec 3. AppConfig::save is
removed; load_or_init stays. No schema change, no operator-facing
migration.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Codex <codex@openai.com>

* docs(plans): toml_edit migration implementation plan

Fifteen-task implementation plan for the toml_edit migration specced in
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md (PR #160).

Task order: scaffold ConfigEditor + tripwire test; env setters + comment
preservation tests (the feature this migration exists for); mount / agent
/ workspace editor methods; migrate 13 call sites across app/mod.rs,
app/context.rs, runtime/launch.rs, persist.rs; delete AppConfig::save
and the dead in-memory mutators; final verification gate before PR.

Every task is TDD-shaped (failing test → impl → green → commit) with
complete code in each step. No placeholders, no "TBD".

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): scaffold ConfigEditor with toml_edit round-trip

Introduces src/config/editor.rs with a ConfigEditor struct that owns a
toml_edit::DocumentMut. ConfigEditor::open loads and parses, save writes
atomically (.tmp + fsync + rename, 0o600 on unix) and returns a
freshly-deserialized AppConfig parsed directly from the written content
(bypassing load_or_init's builtin-agent sync to avoid clobbering the
toml_edit document with a serde round-trip).

No mutators yet — subsequent commits add typed setters and migrate call
sites off AppConfig::save.

Tripwire test (idempotent_save_is_byte_identical) pins that an open →
save with no mutations produces byte-identical output, including
hand-written comments. If this ever fails, toml_edit is round-tripping
lossily.

toml_edit 0.22.27 resolved in Cargo.lock.

Co-authored-by: Claude <noreply@anthropic.com>

* docs(config): clarify ConfigEditor::save validation-bypass invariant

Code-quality review flagged that save()'s docstring only mentioned the
cycle-avoidance reason for bypassing load_or_init, not the second
equally-important property: toml::from_str also skips validate_workspaces
and validate_reserved_names.

Document the invariant the bypass relies on: validation runs once at
load time (via open → load_or_init, or via AppConfig::edit_workspace for
structural workspace changes), and the editor's typed setters preserve
validity by construction.

No behavior change.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::set_env_var with scope-table upsert

Adds set_env_var for all four EnvScope variants (Global, Agent,
Workspace, WorkspaceAgent). Intermediate tables are created as needed
via the private table_path_mut helper, so writing to
WorkspaceAgent { workspace: "new", agent: "new" } works even when
neither table exists.

Existing values are overwritten. Tests cover the upsert, cross-scope
path construction, and overwrite semantics.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::remove_env_var

Removes an env var from any scope, returning true if the key existed
and false if it did not (including when the scope's table is missing).
Does not create intermediate tables — removes-from-nothing is a no-op.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::set_env_comment

Sets or removes a "# comment\n" line immediately above an env var
entry via toml_edit's leaf_decor_mut on the key. The secrets screen
(PR 3 of this series) will use this to annotate ID-form op://
references with their human-readable name.

Tests cover: adding a new comment, replacing an existing one, removing
via None, and — critically — that mutating a sibling key does NOT
disturb the comment above an unrelated key in the same table.

Co-authored-by: Claude <noreply@anthropic.com>

* test(config): fixture round-trip + cross-table comment preservation

Adds a realistic fixture (mixed comments, quoted keys with slashes,
nested env tables) and asserts open → save is byte-identical. This is
the main smoke test that toml_edit's round-trip actually preserves
everything we care about in a real config.

Also adds a cross-table test: mutating workspaces.a's env does not
disturb comments attached to workspaces.b.

Co-authored-by: Claude <noreply@anthropic.com>

* test(config): pin ConfigEditor atomic-write parity

Asserts the saved config is 0600 on unix and that the .tmp sidecar does
not outlive a successful save. Matches the invariants of the existing
AppConfig::save body that ConfigEditor::save lifted.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::add_mount, remove_mount

Preserves the existing (name, mount, scope: Option<&str>) call shape so
call-site migrations stay mechanical. Unscoped writes [docker.mounts.N];
scoped writes [docker.mounts.scope_key] with name as the inner key —
matching AppConfig::add_mount's MountEntry::Scoped storage convention
(scope_key is the outer key, name is the inner key, not the reverse).
remove_mount returns whether the entry existed.

Co-authored-by: Claude <noreply@anthropic.com>

* fix(config): remove_mount cleans up empty scope table

Matches AppConfig::remove_mount (mounts.rs:127-131) which removes the
scope_key entry from docker.mounts when the last named mount inside it
is removed. The editor version was leaving empty [docker.mounts.<scope>]
tables behind.

Two new tests pin the behavior:
- remove_mount_scoped_last_entry_deletes_scope_table: last mount → scope gone
- remove_mount_scoped_preserves_scope_when_siblings_remain: still has siblings → scope stays

Found when comparing against AppConfig::remove_mount during Task 7 spec
review. The plan's Task 7 code omitted this cleanup branch.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor agent mutators

Adds set_agent_trust, set_agent_auth_forward, set_global_auth_forward,
upsert_builtin_agent. The builtin upsert touches only git + trusted so
operator-owned [agents.X.claude] and [agents.X.env] overrides survive
the sync that runs on every load_or_init.

set_agent_trust(false) removes the trusted key entirely to match the
canonical serde shape (skip_serializing_if = is_false).

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor workspace mutators

Adds create_workspace, edit_workspace, remove_workspace, set_last_agent.

create_workspace collision-checks then splats a serialized
WorkspaceConfig into [workspaces.<name>]. edit_workspace delegates to
AppConfig::edit_workspace's validated logic (mount upserts,
allowed_agents diffs, workdir validation) and then replaces the
[workspaces.<name>] table — comments inside the edited workspace are
consumed (that IS the change), but all other sections survive intact.

set_last_agent is a targeted insert that preserves every other field
on the workspace, including default_agent.

Co-authored-by: Claude <noreply@anthropic.com>

* fix(config): create_workspace delegates to AppConfig for validation

Matches edit_workspace's delegation pattern. The editor's
create_workspace now runs AppConfig::create_workspace's full validation
(collision check, workdir/mount-destination relationship) before
splatting the result into the doc — previously it only checked
collision and skipped the workdir/mount validation that today's
CLI path (config.create_workspace) provides.

Without this, Task 10's call-site migration at app/mod.rs:386 would
have silently lost validation: users who asked jackin to create a
workspace with a workdir that doesn't line up with any mount dst would
have gotten a quietly-invalid config instead of the usual error.

New test create_workspace_rejects_invalid_workdir_mount_combo pins
the behavior.

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::upsert_agent_source

Persists git + trusted from an AgentSource into [agents.<key>], keeps
any existing claude subtable intact, and never touches [agents.<key>.env].

Needed by the app/mod.rs trust and auth_forward call sites: they invoke
resolve_agent_source (which may insert a new agent entry into the
in-memory AppConfig), and the editor must persist that insert alongside
the trust or auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>

* refactor(app): route config writes through ConfigEditor

Migrates the ten config.mutate(); config.save() pairs in src/app/mod.rs
(mounts, trust, auth_forward, workspace create/edit/remove) to the new
ConfigEditor::open → mutate → save shape. AppConfig::save still exists
at this point; it gets removed in a later commit once all callers are
migrated.

Sites that first call resolve_agent_source (trust grant/revoke,
auth_forward) now also call ConfigEditor::upsert_agent_source so that
any newly-inserted agent entry gets persisted alongside the trust or
auth_forward change.

Co-authored-by: Claude <noreply@anthropic.com>

* refactor(app): route last-used-agent save through ConfigEditor

Pre-flush the in-memory config to disk (ensuring the directory exists
via ensure_base_dirs) then open a ConfigEditor, apply set_last_agent,
and replace the in-memory config with the freshly-deserialized result.
Preserves all five behavioral contracts: early-exit on load failure,
no-op when workspace_name is None, no-op when workspace is absent,
the existing save-failure warning, and adds an open-failure warning.

Co-authored-by: Claude <noreply@anthropic.com>

* fix(app): remember_last_agent drops pre-flush, tests persist init state

The Task 11 pre-flush (config.save(paths) before opening the editor)
kept the lossy serde write alive on the production path, destroying
any user-written comments in the config on every successful launch —
the exact thing this migration exists to prevent.

Root cause was a test setup issue, not production code: the two tests
constructed an in-memory AppConfig and called remember_last_agent
without first persisting the config to disk. Production always has
config on disk (loaded at startup), so the pre-flush was solving a
test-only problem in the wrong place.

Fix: move the persistence step into a test helper
(persisted_config_with_workspace) that writes the initial AppConfig via
toml::to_string_pretty + fs::write. Revert the production path to just
open → set_last_agent → save, which now preserves comments as intended.

Co-authored-by: Claude <noreply@anthropic.com>

* refactor(launch): route newly-trusted agent save through ConfigEditor

Co-authored-by: Claude <noreply@anthropic.com>

* feat(config): ConfigEditor::normalize_deprecated_copy

Rewrites any auth_forward = "copy" to "sync" at the two paths
contains_deprecated_copy_auth_forward checks (global [claude] and
[agents.*.claude]). Used by load_or_init's deprecated-copy migration
path so user comments and blank lines around the changed keys survive.

Co-authored-by: Claude <noreply@anthropic.com>

* refactor(config): route load_or_init migrations through ConfigEditor

The builtin-sync and deprecated-copy migrations run in-memory on
AppConfig as before, but the resulting save goes through
ConfigEditor::upsert_builtin_agent and normalize_deprecated_copy
instead of AppConfig::save. User comments and blank lines in config
sections untouched by the migration now survive the first load after
upgrade.

To break the recursion that would arise when the config file does not
yet exist (ConfigEditor::open calls AppConfig::load_or_init if absent),
a bootstrap AppConfig::save is issued first to materialize the file,
then the editor reopens it and applies comment-preserving mutations on
top.

Critically, config = editor.save()? at the end replaces the in-memory
config with a fresh parse from the on-disk file. This matters because
sync_builtin_agents clears [agents.X.env] on any updated builtin (it
constructs a fresh AgentSource with empty env), but upsert_builtin_agent
leaves env alone. Using editor.save()'s return value means operators
keep their [agents.X.env] across builtin syncs.

BUILTIN_AGENTS visibility changed from pub(super) to pub(crate) so
persist.rs can iterate it directly.

Co-authored-by: Claude <noreply@anthropic.com>

* fix(config): gate load_or_init bootstrap save on missing file

The Task 13 implementation issued AppConfig::save(paths)? unconditionally
before opening the editor, to avoid ConfigEditor::open's recursion into
load_or_init when the file is missing. That regressed comment preservation
on the exact upgrade paths this migration protects: a user with
auth_forward = "copy" and hand-written comments would have those comments
destroyed by the bootstrap serde write before the editor got a chance to
preserve them.

Root cause: the recursion only happens when the file DOES NOT exist. On
the upgrade path (file exists, needs builtin-sync or copy→sync migration),
ConfigEditor::open reads the existing file directly without recursing.

Fix: gate the bootstrap config.save on !paths.config_file.exists(). First
run (no file) still materializes defaults via the lossy writer — fine,
because there are no user comments to preserve yet. Existing-file upgrades
skip the bootstrap and go straight to the comment-preserving editor path.

New regression test load_migration_preserves_user_comments pins the
behavior: a config with hand-written comments and deprecated "copy"
auth_forward retains its comments through the migration.

Co-authored-by: Claude <noreply@anthropic.com>

* refactor(config): remove AppConfig::save and dead mutator methods

AppConfig::save is removed from the public surface; its body is inlined
into load_or_init's bootstrap branch (the only remaining caller) with
full atomic-write semantics preserved (.tmp + fsync + rename + 0o600 on
unix). No save method survives at any visibility.

ConfigEditor is now the sole write path. The in-memory mutator methods
are demoted or deleted:

- trust_agent, untrust_agent, set_agent_auth_forward → pub(crate) with
  #[cfg_attr(not(test), allow(dead_code))]; test-only affordances used
  by the unit tests in config/agents.rs and config/mod.rs. Production
  callers use ConfigEditor.

- add_mount → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  test-only affordance used by launch/preview.rs and workspace/resolve.rs
  for in-memory AppConfig setup; does not persist to disk.

- remove_mount → deleted entirely; no test or production caller remained
  after the migration.

- create_workspace, edit_workspace → pub(crate); ConfigEditor::create_workspace
  and ConfigEditor::edit_workspace delegate here for validation, so these
  must remain reachable within the crate.

- remove_workspace → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  kept for the workspaces.rs unit test that validates error message shape.
  ConfigEditor::remove_workspace operates directly on the TOML document
  and does not delegate here.

The two test callers of config.save() in config/agents.rs are rewritten
to use toml::to_string_pretty + std::fs::write for their bootstrap writes.

tests/workspace_config_crud.rs (integration tests) is rewritten to use
ConfigEditor instead of AppConfig directly, since pub(crate) visibility
is not accessible from integration test crates. The tests retain the same
assertions; the only change is the creation/edit path goes through the
editor rather than bare in-memory mutation.

Unused assignments (config = editor.save()?;) in app/mod.rs are fixed by
dropping the lhs; the match arms already returned Ok(()) immediately after,
making every such write dead. These are the unused_assignments flagged in
Task 10 that Task 14 was responsible for cleaning up.

Co-authored-by: Claude <noreply@anthropic.com>

* style(config): run cargo fmt

CI's cargo fmt --check flagged formatting in src/config/editor.rs
(mostly short-vec splat-vs-single-line choices from the table-path
constructions added across Tasks 2–13). Pure formatting — no logic
change. Caught by CI, not locally, because the worktree was running
cargo check / cargo test but not cargo fmt --check as a pre-push gate.

Co-authored-by: Claude <noreply@anthropic.com>

* style(config): quiet clippy lints introduced by migration

CI runs cargo clippy -- -D warnings (lib target only). The toml_edit
migration introduced 21 new lint errors across src/app/mod.rs,
src/config/editor.rs, and src/config/agents.rs. All are clippy-suggested
mechanical transforms — no behavior change.

Categories fixed: map-unwrap-or → is_some_and, if-not-else inversion,
needless pass-by-value on EnvScope, collapsible-if, explicit-iter-loop,
const-fn for auth_forward_str, missing-backticks in doc comments.

Co-authored-by: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Codex <codex@openai.com>

…160)

Add the design spec for PR 1 of 3 in the launcher-workspace-manager
series. Replaces the current full-reserialize writer with a targeted
toml_edit patch path so hand-written comments, blank lines, and key
ordering survive every programmatic save.

Needed because the upcoming secrets screen (spec 3) annotates ID-form
1Password references with the human-readable name as a mid-document
comment. The current writer discards all comments on any save, so
annotations would not survive the next unrelated trust toggle or
last-used-agent update.

Introduces ConfigEditor with typed setters for the 13 existing write
sites plus forward-looking env methods for spec 3. AppConfig::save is
removed; load_or_init stays. No schema change, no operator-facing
migration.

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

* docs(plans): toml_edit migration implementation plan

Fifteen-task implementation plan for the toml_edit migration specced in
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md (PR #160).

Task order: scaffold ConfigEditor + tripwire test; env setters + comment
preservation tests (the feature this migration exists for); mount / agent
/ workspace editor methods; migrate 13 call sites across app/mod.rs,
app/context.rs, runtime/launch.rs, persist.rs; delete AppConfig::save
and the dead in-memory mutators; final verification gate before PR.

Every task is TDD-shaped (failing test → impl → green → commit) with
complete code in each step. No placeholders, no "TBD".


* feat(config): scaffold ConfigEditor with toml_edit round-trip

Introduces src/config/editor.rs with a ConfigEditor struct that owns a
toml_edit::DocumentMut. ConfigEditor::open loads and parses, save writes
atomically (.tmp + fsync + rename, 0o600 on unix) and returns a
freshly-deserialized AppConfig parsed directly from the written content
(bypassing load_or_init's builtin-agent sync to avoid clobbering the
toml_edit document with a serde round-trip).

No mutators yet — subsequent commits add typed setters and migrate call
sites off AppConfig::save.

Tripwire test (idempotent_save_is_byte_identical) pins that an open →
save with no mutations produces byte-identical output, including
hand-written comments. If this ever fails, toml_edit is round-tripping
lossily.

toml_edit 0.22.27 resolved in Cargo.lock.


* docs(config): clarify ConfigEditor::save validation-bypass invariant

Code-quality review flagged that save()'s docstring only mentioned the
cycle-avoidance reason for bypassing load_or_init, not the second
equally-important property: toml::from_str also skips validate_workspaces
and validate_reserved_names.

Document the invariant the bypass relies on: validation runs once at
load time (via open → load_or_init, or via AppConfig::edit_workspace for
structural workspace changes), and the editor's typed setters preserve
validity by construction.

No behavior change.


* feat(config): ConfigEditor::set_env_var with scope-table upsert

Adds set_env_var for all four EnvScope variants (Global, Agent,
Workspace, WorkspaceAgent). Intermediate tables are created as needed
via the private table_path_mut helper, so writing to
WorkspaceAgent { workspace: "new", agent: "new" } works even when
neither table exists.

Existing values are overwritten. Tests cover the upsert, cross-scope
path construction, and overwrite semantics.


* feat(config): ConfigEditor::remove_env_var

Removes an env var from any scope, returning true if the key existed
and false if it did not (including when the scope's table is missing).
Does not create intermediate tables — removes-from-nothing is a no-op.


* feat(config): ConfigEditor::set_env_comment

Sets or removes a "# comment\n" line immediately above an env var
entry via toml_edit's leaf_decor_mut on the key. The secrets screen
(PR 3 of this series) will use this to annotate ID-form op://
references with their human-readable name.

Tests cover: adding a new comment, replacing an existing one, removing
via None, and — critically — that mutating a sibling key does NOT
disturb the comment above an unrelated key in the same table.


* test(config): fixture round-trip + cross-table comment preservation

Adds a realistic fixture (mixed comments, quoted keys with slashes,
nested env tables) and asserts open → save is byte-identical. This is
the main smoke test that toml_edit's round-trip actually preserves
everything we care about in a real config.

Also adds a cross-table test: mutating workspaces.a's env does not
disturb comments attached to workspaces.b.


* test(config): pin ConfigEditor atomic-write parity

Asserts the saved config is 0600 on unix and that the .tmp sidecar does
not outlive a successful save. Matches the invariants of the existing
AppConfig::save body that ConfigEditor::save lifted.


* feat(config): ConfigEditor::add_mount, remove_mount

Preserves the existing (name, mount, scope: Option<&str>) call shape so
call-site migrations stay mechanical. Unscoped writes [docker.mounts.N];
scoped writes [docker.mounts.scope_key] with name as the inner key —
matching AppConfig::add_mount's MountEntry::Scoped storage convention
(scope_key is the outer key, name is the inner key, not the reverse).
remove_mount returns whether the entry existed.


* fix(config): remove_mount cleans up empty scope table

Matches AppConfig::remove_mount (mounts.rs:127-131) which removes the
scope_key entry from docker.mounts when the last named mount inside it
is removed. The editor version was leaving empty [docker.mounts.<scope>]
tables behind.

Two new tests pin the behavior:
- remove_mount_scoped_last_entry_deletes_scope_table: last mount → scope gone
- remove_mount_scoped_preserves_scope_when_siblings_remain: still has siblings → scope stays

Found when comparing against AppConfig::remove_mount during Task 7 spec
review. The plan's Task 7 code omitted this cleanup branch.


* feat(config): ConfigEditor agent mutators

Adds set_agent_trust, set_agent_auth_forward, set_global_auth_forward,
upsert_builtin_agent. The builtin upsert touches only git + trusted so
operator-owned [agents.X.claude] and [agents.X.env] overrides survive
the sync that runs on every load_or_init.

set_agent_trust(false) removes the trusted key entirely to match the
canonical serde shape (skip_serializing_if = is_false).


* feat(config): ConfigEditor workspace mutators

Adds create_workspace, edit_workspace, remove_workspace, set_last_agent.

create_workspace collision-checks then splats a serialized
WorkspaceConfig into [workspaces.<name>]. edit_workspace delegates to
AppConfig::edit_workspace's validated logic (mount upserts,
allowed_agents diffs, workdir validation) and then replaces the
[workspaces.<name>] table — comments inside the edited workspace are
consumed (that IS the change), but all other sections survive intact.

set_last_agent is a targeted insert that preserves every other field
on the workspace, including default_agent.


* fix(config): create_workspace delegates to AppConfig for validation

Matches edit_workspace's delegation pattern. The editor's
create_workspace now runs AppConfig::create_workspace's full validation
(collision check, workdir/mount-destination relationship) before
splatting the result into the doc — previously it only checked
collision and skipped the workdir/mount validation that today's
CLI path (config.create_workspace) provides.

Without this, Task 10's call-site migration at app/mod.rs:386 would
have silently lost validation: users who asked jackin to create a
workspace with a workdir that doesn't line up with any mount dst would
have gotten a quietly-invalid config instead of the usual error.

New test create_workspace_rejects_invalid_workdir_mount_combo pins
the behavior.


* feat(config): ConfigEditor::upsert_agent_source

Persists git + trusted from an AgentSource into [agents.<key>], keeps
any existing claude subtable intact, and never touches [agents.<key>.env].

Needed by the app/mod.rs trust and auth_forward call sites: they invoke
resolve_agent_source (which may insert a new agent entry into the
in-memory AppConfig), and the editor must persist that insert alongside
the trust or auth_forward change.


* refactor(app): route config writes through ConfigEditor

Migrates the ten config.mutate(); config.save() pairs in src/app/mod.rs
(mounts, trust, auth_forward, workspace create/edit/remove) to the new
ConfigEditor::open → mutate → save shape. AppConfig::save still exists
at this point; it gets removed in a later commit once all callers are
migrated.

Sites that first call resolve_agent_source (trust grant/revoke,
auth_forward) now also call ConfigEditor::upsert_agent_source so that
any newly-inserted agent entry gets persisted alongside the trust or
auth_forward change.


* refactor(app): route last-used-agent save through ConfigEditor

Pre-flush the in-memory config to disk (ensuring the directory exists
via ensure_base_dirs) then open a ConfigEditor, apply set_last_agent,
and replace the in-memory config with the freshly-deserialized result.
Preserves all five behavioral contracts: early-exit on load failure,
no-op when workspace_name is None, no-op when workspace is absent,
the existing save-failure warning, and adds an open-failure warning.


* fix(app): remember_last_agent drops pre-flush, tests persist init state

The Task 11 pre-flush (config.save(paths) before opening the editor)
kept the lossy serde write alive on the production path, destroying
any user-written comments in the config on every successful launch —
the exact thing this migration exists to prevent.

Root cause was a test setup issue, not production code: the two tests
constructed an in-memory AppConfig and called remember_last_agent
without first persisting the config to disk. Production always has
config on disk (loaded at startup), so the pre-flush was solving a
test-only problem in the wrong place.

Fix: move the persistence step into a test helper
(persisted_config_with_workspace) that writes the initial AppConfig via
toml::to_string_pretty + fs::write. Revert the production path to just
open → set_last_agent → save, which now preserves comments as intended.


* refactor(launch): route newly-trusted agent save through ConfigEditor


* feat(config): ConfigEditor::normalize_deprecated_copy

Rewrites any auth_forward = "copy" to "sync" at the two paths
contains_deprecated_copy_auth_forward checks (global [claude] and
[agents.*.claude]). Used by load_or_init's deprecated-copy migration
path so user comments and blank lines around the changed keys survive.


* refactor(config): route load_or_init migrations through ConfigEditor

The builtin-sync and deprecated-copy migrations run in-memory on
AppConfig as before, but the resulting save goes through
ConfigEditor::upsert_builtin_agent and normalize_deprecated_copy
instead of AppConfig::save. User comments and blank lines in config
sections untouched by the migration now survive the first load after
upgrade.

To break the recursion that would arise when the config file does not
yet exist (ConfigEditor::open calls AppConfig::load_or_init if absent),
a bootstrap AppConfig::save is issued first to materialize the file,
then the editor reopens it and applies comment-preserving mutations on
top.

Critically, config = editor.save()? at the end replaces the in-memory
config with a fresh parse from the on-disk file. This matters because
sync_builtin_agents clears [agents.X.env] on any updated builtin (it
constructs a fresh AgentSource with empty env), but upsert_builtin_agent
leaves env alone. Using editor.save()'s return value means operators
keep their [agents.X.env] across builtin syncs.

BUILTIN_AGENTS visibility changed from pub(super) to pub(crate) so
persist.rs can iterate it directly.


* fix(config): gate load_or_init bootstrap save on missing file

The Task 13 implementation issued AppConfig::save(paths)? unconditionally
before opening the editor, to avoid ConfigEditor::open's recursion into
load_or_init when the file is missing. That regressed comment preservation
on the exact upgrade paths this migration protects: a user with
auth_forward = "copy" and hand-written comments would have those comments
destroyed by the bootstrap serde write before the editor got a chance to
preserve them.

Root cause: the recursion only happens when the file DOES NOT exist. On
the upgrade path (file exists, needs builtin-sync or copy→sync migration),
ConfigEditor::open reads the existing file directly without recursing.

Fix: gate the bootstrap config.save on !paths.config_file.exists(). First
run (no file) still materializes defaults via the lossy writer — fine,
because there are no user comments to preserve yet. Existing-file upgrades
skip the bootstrap and go straight to the comment-preserving editor path.

New regression test load_migration_preserves_user_comments pins the
behavior: a config with hand-written comments and deprecated "copy"
auth_forward retains its comments through the migration.


* refactor(config): remove AppConfig::save and dead mutator methods

AppConfig::save is removed from the public surface; its body is inlined
into load_or_init's bootstrap branch (the only remaining caller) with
full atomic-write semantics preserved (.tmp + fsync + rename + 0o600 on
unix). No save method survives at any visibility.

ConfigEditor is now the sole write path. The in-memory mutator methods
are demoted or deleted:

- trust_agent, untrust_agent, set_agent_auth_forward → pub(crate) with
  #[cfg_attr(not(test), allow(dead_code))]; test-only affordances used
  by the unit tests in config/agents.rs and config/mod.rs. Production
  callers use ConfigEditor.

- add_mount → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  test-only affordance used by launch/preview.rs and workspace/resolve.rs
  for in-memory AppConfig setup; does not persist to disk.

- remove_mount → deleted entirely; no test or production caller remained
  after the migration.

- create_workspace, edit_workspace → pub(crate); ConfigEditor::create_workspace
  and ConfigEditor::edit_workspace delegate here for validation, so these
  must remain reachable within the crate.

- remove_workspace → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  kept for the workspaces.rs unit test that validates error message shape.
  ConfigEditor::remove_workspace operates directly on the TOML document
  and does not delegate here.

The two test callers of config.save() in config/agents.rs are rewritten
to use toml::to_string_pretty + std::fs::write for their bootstrap writes.

tests/workspace_config_crud.rs (integration tests) is rewritten to use
ConfigEditor instead of AppConfig directly, since pub(crate) visibility
is not accessible from integration test crates. The tests retain the same
assertions; the only change is the creation/edit path goes through the
editor rather than bare in-memory mutation.

Unused assignments (config = editor.save()?;) in app/mod.rs are fixed by
dropping the lhs; the match arms already returned Ok(()) immediately after,
making every such write dead. These are the unused_assignments flagged in
Task 10 that Task 14 was responsible for cleaning up.


* style(config): run cargo fmt

CI's cargo fmt --check flagged formatting in src/config/editor.rs
(mostly short-vec splat-vs-single-line choices from the table-path
constructions added across Tasks 2–13). Pure formatting — no logic
change. Caught by CI, not locally, because the worktree was running
cargo check / cargo test but not cargo fmt --check as a pre-push gate.


* style(config): quiet clippy lints introduced by migration

CI runs cargo clippy -- -D warnings (lib target only). The toml_edit
migration introduced 21 new lint errors across src/app/mod.rs,
src/config/editor.rs, and src/config/agents.rs. All are clippy-suggested
mechanical transforms — no behavior change.

Categories fixed: map-unwrap-or → is_some_and, if-not-else inversion,
needless pass-by-value on EnvScope, collapsible-if, explicit-iter-loop,
const-fn for auth_forward_str, missing-backticks in doc comments.


---------

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

…160)

Add the design spec for PR 1 of 3 in the launcher-workspace-manager
series. Replaces the current full-reserialize writer with a targeted
toml_edit patch path so hand-written comments, blank lines, and key
ordering survive every programmatic save.

Needed because the upcoming secrets screen (spec 3) annotates ID-form
1Password references with the human-readable name as a mid-document
comment. The current writer discards all comments on any save, so
annotations would not survive the next unrelated trust toggle or
last-used-agent update.

Introduces ConfigEditor with typed setters for the 13 existing write
sites plus forward-looking env methods for spec 3. AppConfig::save is
removed; load_or_init stays. No schema change, no operator-facing
migration.

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>

* docs(plans): toml_edit migration implementation plan

Fifteen-task implementation plan for the toml_edit migration specced in
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md (PR #160).

Task order: scaffold ConfigEditor + tripwire test; env setters + comment
preservation tests (the feature this migration exists for); mount / agent
/ workspace editor methods; migrate 13 call sites across app/mod.rs,
app/context.rs, runtime/launch.rs, persist.rs; delete AppConfig::save
and the dead in-memory mutators; final verification gate before PR.

Every task is TDD-shaped (failing test → impl → green → commit) with
complete code in each step. No placeholders, no "TBD".


* feat(config): scaffold ConfigEditor with toml_edit round-trip

Introduces src/config/editor.rs with a ConfigEditor struct that owns a
toml_edit::DocumentMut. ConfigEditor::open loads and parses, save writes
atomically (.tmp + fsync + rename, 0o600 on unix) and returns a
freshly-deserialized AppConfig parsed directly from the written content
(bypassing load_or_init's builtin-agent sync to avoid clobbering the
toml_edit document with a serde round-trip).

No mutators yet — subsequent commits add typed setters and migrate call
sites off AppConfig::save.

Tripwire test (idempotent_save_is_byte_identical) pins that an open →
save with no mutations produces byte-identical output, including
hand-written comments. If this ever fails, toml_edit is round-tripping
lossily.

toml_edit 0.22.27 resolved in Cargo.lock.


* docs(config): clarify ConfigEditor::save validation-bypass invariant

Code-quality review flagged that save()'s docstring only mentioned the
cycle-avoidance reason for bypassing load_or_init, not the second
equally-important property: toml::from_str also skips validate_workspaces
and validate_reserved_names.

Document the invariant the bypass relies on: validation runs once at
load time (via open → load_or_init, or via AppConfig::edit_workspace for
structural workspace changes), and the editor's typed setters preserve
validity by construction.

No behavior change.


* feat(config): ConfigEditor::set_env_var with scope-table upsert

Adds set_env_var for all four EnvScope variants (Global, Agent,
Workspace, WorkspaceAgent). Intermediate tables are created as needed
via the private table_path_mut helper, so writing to
WorkspaceAgent { workspace: "new", agent: "new" } works even when
neither table exists.

Existing values are overwritten. Tests cover the upsert, cross-scope
path construction, and overwrite semantics.


* feat(config): ConfigEditor::remove_env_var

Removes an env var from any scope, returning true if the key existed
and false if it did not (including when the scope's table is missing).
Does not create intermediate tables — removes-from-nothing is a no-op.


* feat(config): ConfigEditor::set_env_comment

Sets or removes a "# comment\n" line immediately above an env var
entry via toml_edit's leaf_decor_mut on the key. The secrets screen
(PR 3 of this series) will use this to annotate ID-form op://
references with their human-readable name.

Tests cover: adding a new comment, replacing an existing one, removing
via None, and — critically — that mutating a sibling key does NOT
disturb the comment above an unrelated key in the same table.


* test(config): fixture round-trip + cross-table comment preservation

Adds a realistic fixture (mixed comments, quoted keys with slashes,
nested env tables) and asserts open → save is byte-identical. This is
the main smoke test that toml_edit's round-trip actually preserves
everything we care about in a real config.

Also adds a cross-table test: mutating workspaces.a's env does not
disturb comments attached to workspaces.b.


* test(config): pin ConfigEditor atomic-write parity

Asserts the saved config is 0600 on unix and that the .tmp sidecar does
not outlive a successful save. Matches the invariants of the existing
AppConfig::save body that ConfigEditor::save lifted.


* feat(config): ConfigEditor::add_mount, remove_mount

Preserves the existing (name, mount, scope: Option<&str>) call shape so
call-site migrations stay mechanical. Unscoped writes [docker.mounts.N];
scoped writes [docker.mounts.scope_key] with name as the inner key —
matching AppConfig::add_mount's MountEntry::Scoped storage convention
(scope_key is the outer key, name is the inner key, not the reverse).
remove_mount returns whether the entry existed.


* fix(config): remove_mount cleans up empty scope table

Matches AppConfig::remove_mount (mounts.rs:127-131) which removes the
scope_key entry from docker.mounts when the last named mount inside it
is removed. The editor version was leaving empty [docker.mounts.<scope>]
tables behind.

Two new tests pin the behavior:
- remove_mount_scoped_last_entry_deletes_scope_table: last mount → scope gone
- remove_mount_scoped_preserves_scope_when_siblings_remain: still has siblings → scope stays

Found when comparing against AppConfig::remove_mount during Task 7 spec
review. The plan's Task 7 code omitted this cleanup branch.


* feat(config): ConfigEditor agent mutators

Adds set_agent_trust, set_agent_auth_forward, set_global_auth_forward,
upsert_builtin_agent. The builtin upsert touches only git + trusted so
operator-owned [agents.X.claude] and [agents.X.env] overrides survive
the sync that runs on every load_or_init.

set_agent_trust(false) removes the trusted key entirely to match the
canonical serde shape (skip_serializing_if = is_false).


* feat(config): ConfigEditor workspace mutators

Adds create_workspace, edit_workspace, remove_workspace, set_last_agent.

create_workspace collision-checks then splats a serialized
WorkspaceConfig into [workspaces.<name>]. edit_workspace delegates to
AppConfig::edit_workspace's validated logic (mount upserts,
allowed_agents diffs, workdir validation) and then replaces the
[workspaces.<name>] table — comments inside the edited workspace are
consumed (that IS the change), but all other sections survive intact.

set_last_agent is a targeted insert that preserves every other field
on the workspace, including default_agent.


* fix(config): create_workspace delegates to AppConfig for validation

Matches edit_workspace's delegation pattern. The editor's
create_workspace now runs AppConfig::create_workspace's full validation
(collision check, workdir/mount-destination relationship) before
splatting the result into the doc — previously it only checked
collision and skipped the workdir/mount validation that today's
CLI path (config.create_workspace) provides.

Without this, Task 10's call-site migration at app/mod.rs:386 would
have silently lost validation: users who asked jackin to create a
workspace with a workdir that doesn't line up with any mount dst would
have gotten a quietly-invalid config instead of the usual error.

New test create_workspace_rejects_invalid_workdir_mount_combo pins
the behavior.


* feat(config): ConfigEditor::upsert_agent_source

Persists git + trusted from an AgentSource into [agents.<key>], keeps
any existing claude subtable intact, and never touches [agents.<key>.env].

Needed by the app/mod.rs trust and auth_forward call sites: they invoke
resolve_agent_source (which may insert a new agent entry into the
in-memory AppConfig), and the editor must persist that insert alongside
the trust or auth_forward change.


* refactor(app): route config writes through ConfigEditor

Migrates the ten config.mutate(); config.save() pairs in src/app/mod.rs
(mounts, trust, auth_forward, workspace create/edit/remove) to the new
ConfigEditor::open → mutate → save shape. AppConfig::save still exists
at this point; it gets removed in a later commit once all callers are
migrated.

Sites that first call resolve_agent_source (trust grant/revoke,
auth_forward) now also call ConfigEditor::upsert_agent_source so that
any newly-inserted agent entry gets persisted alongside the trust or
auth_forward change.


* refactor(app): route last-used-agent save through ConfigEditor

Pre-flush the in-memory config to disk (ensuring the directory exists
via ensure_base_dirs) then open a ConfigEditor, apply set_last_agent,
and replace the in-memory config with the freshly-deserialized result.
Preserves all five behavioral contracts: early-exit on load failure,
no-op when workspace_name is None, no-op when workspace is absent,
the existing save-failure warning, and adds an open-failure warning.


* fix(app): remember_last_agent drops pre-flush, tests persist init state

The Task 11 pre-flush (config.save(paths) before opening the editor)
kept the lossy serde write alive on the production path, destroying
any user-written comments in the config on every successful launch —
the exact thing this migration exists to prevent.

Root cause was a test setup issue, not production code: the two tests
constructed an in-memory AppConfig and called remember_last_agent
without first persisting the config to disk. Production always has
config on disk (loaded at startup), so the pre-flush was solving a
test-only problem in the wrong place.

Fix: move the persistence step into a test helper
(persisted_config_with_workspace) that writes the initial AppConfig via
toml::to_string_pretty + fs::write. Revert the production path to just
open → set_last_agent → save, which now preserves comments as intended.


* refactor(launch): route newly-trusted agent save through ConfigEditor


* feat(config): ConfigEditor::normalize_deprecated_copy

Rewrites any auth_forward = "copy" to "sync" at the two paths
contains_deprecated_copy_auth_forward checks (global [claude] and
[agents.*.claude]). Used by load_or_init's deprecated-copy migration
path so user comments and blank lines around the changed keys survive.


* refactor(config): route load_or_init migrations through ConfigEditor

The builtin-sync and deprecated-copy migrations run in-memory on
AppConfig as before, but the resulting save goes through
ConfigEditor::upsert_builtin_agent and normalize_deprecated_copy
instead of AppConfig::save. User comments and blank lines in config
sections untouched by the migration now survive the first load after
upgrade.

To break the recursion that would arise when the config file does not
yet exist (ConfigEditor::open calls AppConfig::load_or_init if absent),
a bootstrap AppConfig::save is issued first to materialize the file,
then the editor reopens it and applies comment-preserving mutations on
top.

Critically, config = editor.save()? at the end replaces the in-memory
config with a fresh parse from the on-disk file. This matters because
sync_builtin_agents clears [agents.X.env] on any updated builtin (it
constructs a fresh AgentSource with empty env), but upsert_builtin_agent
leaves env alone. Using editor.save()'s return value means operators
keep their [agents.X.env] across builtin syncs.

BUILTIN_AGENTS visibility changed from pub(super) to pub(crate) so
persist.rs can iterate it directly.


* fix(config): gate load_or_init bootstrap save on missing file

The Task 13 implementation issued AppConfig::save(paths)? unconditionally
before opening the editor, to avoid ConfigEditor::open's recursion into
load_or_init when the file is missing. That regressed comment preservation
on the exact upgrade paths this migration protects: a user with
auth_forward = "copy" and hand-written comments would have those comments
destroyed by the bootstrap serde write before the editor got a chance to
preserve them.

Root cause: the recursion only happens when the file DOES NOT exist. On
the upgrade path (file exists, needs builtin-sync or copy→sync migration),
ConfigEditor::open reads the existing file directly without recursing.

Fix: gate the bootstrap config.save on !paths.config_file.exists(). First
run (no file) still materializes defaults via the lossy writer — fine,
because there are no user comments to preserve yet. Existing-file upgrades
skip the bootstrap and go straight to the comment-preserving editor path.

New regression test load_migration_preserves_user_comments pins the
behavior: a config with hand-written comments and deprecated "copy"
auth_forward retains its comments through the migration.


* refactor(config): remove AppConfig::save and dead mutator methods

AppConfig::save is removed from the public surface; its body is inlined
into load_or_init's bootstrap branch (the only remaining caller) with
full atomic-write semantics preserved (.tmp + fsync + rename + 0o600 on
unix). No save method survives at any visibility.

ConfigEditor is now the sole write path. The in-memory mutator methods
are demoted or deleted:

- trust_agent, untrust_agent, set_agent_auth_forward → pub(crate) with
  #[cfg_attr(not(test), allow(dead_code))]; test-only affordances used
  by the unit tests in config/agents.rs and config/mod.rs. Production
  callers use ConfigEditor.

- add_mount → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  test-only affordance used by launch/preview.rs and workspace/resolve.rs
  for in-memory AppConfig setup; does not persist to disk.

- remove_mount → deleted entirely; no test or production caller remained
  after the migration.

- create_workspace, edit_workspace → pub(crate); ConfigEditor::create_workspace
  and ConfigEditor::edit_workspace delegate here for validation, so these
  must remain reachable within the crate.

- remove_workspace → pub(crate) with #[cfg_attr(not(test), allow(dead_code))];
  kept for the workspaces.rs unit test that validates error message shape.
  ConfigEditor::remove_workspace operates directly on the TOML document
  and does not delegate here.

The two test callers of config.save() in config/agents.rs are rewritten
to use toml::to_string_pretty + std::fs::write for their bootstrap writes.

tests/workspace_config_crud.rs (integration tests) is rewritten to use
ConfigEditor instead of AppConfig directly, since pub(crate) visibility
is not accessible from integration test crates. The tests retain the same
assertions; the only change is the creation/edit path goes through the
editor rather than bare in-memory mutation.

Unused assignments (config = editor.save()?;) in app/mod.rs are fixed by
dropping the lhs; the match arms already returned Ok(()) immediately after,
making every such write dead. These are the unused_assignments flagged in
Task 10 that Task 14 was responsible for cleaning up.


* style(config): run cargo fmt

CI's cargo fmt --check flagged formatting in src/config/editor.rs
(mostly short-vec splat-vs-single-line choices from the table-path
constructions added across Tasks 2–13). Pure formatting — no logic
change. Caught by CI, not locally, because the worktree was running
cargo check / cargo test but not cargo fmt --check as a pre-push gate.


* style(config): quiet clippy lints introduced by migration

CI runs cargo clippy -- -D warnings (lib target only). The toml_edit
migration introduced 21 new lint errors across src/app/mod.rs,
src/config/editor.rs, and src/config/agents.rs. All are clippy-suggested
mechanical transforms — no behavior change.

Categories fixed: map-unwrap-or → is_some_and, if-not-else inversion,
needless pass-by-value on EnvScope, collapsible-if, explicit-iter-loop,
const-fn for auth_forward_str, missing-backticks in doc comments.


---------

Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
Co-authored-by: Claude <noreply@anthropic.com>