feat(config): toml_edit migration (PR 1 of 3)#162
Merged
Conversation
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com>
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>
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>
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>
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>
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>
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>
This was referenced Apr 23, 2026
donbeave
added a commit
that referenced
this pull request
Apr 24, 2026
* docs(specs): workspace manager TUI (PR 2 of 3) Design spec for PR 2 of the launcher-workspace-manager series. Adds an interactive workspace manager screen to the jackin launcher — list, create, edit, and delete workspaces without dropping to CLI. Reached via `m` from the existing Workspace picker; Esc returns to the launcher. Launch path stays keystroke-identical. Key design decisions from brainstorming (all settled, no open questions): - Entry model: separate Manager screen on `m` keypress; launch path unchanged. - Editor tab set: General · Mounts · Agents · Secrets-stub. Secrets placeholder locks in the final tab strip so PR 3 fills in the panel without a visual reshuffle. - Text-edit UX: modal push — centered overlay, one reusable TextInput widget. - Staging: explicit save via `s`. Pending changes drive dirty markers; Esc with pending opens Discard/Save/Cancel. - Create flow: mounts-first wizard — file browser for host source, dst auto-defaulted to the same absolute path as src (host-path mirror), workdir picked from mount dsts + ancestors (never free-text), name last with live uniqueness check. - Delete UX: single-line Y/N confirm modal. - Style: reuses jackin's existing digital_rain (src/tui/animation.rs), step_shimmer, spin_wait, and landing-page color tokens from docs/src/components/landing/styles.css. One new area-bounded rain widget extracted from animation.rs. Three new reusable widgets emerge (TextInput, FileBrowser, Confirm) that PR 3's Secrets tab will consume unchanged. All persisted writes flow through ConfigEditor (established in PR 1, merged in #162). Non-goals: per-(workspace × agent) env overrides (PR 3), global mount management (CLI only), agent lifecycle from manager (CLI only), CLI surface changes, CHANGELOG. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com> * docs(specs): lock third-party widget choices for PR 2 Amends the workspace manager TUI spec with a Third-party dependencies subsection that names the three ratatui ecosystem crates we'll adopt: - ratatui-textarea (v0.9.x) — single-line TextInput (ratatui-org owned) - ratatui-explorer (v0.3.x) — FileBrowser with folders-only wrapper - tui-widget-list (v0.15.x) — WorkdirPick list mechanics All three require the ratatui unstable-widget-ref feature flag. Rejected with rationale so reviewers don't re-litigate: tui-input (superseded by ratatui-textarea), tui-confirm-dialog / tui-overlay (Confirm modal is cheaper hand-rolled), rat-widget (too opinionated), throbber-widgets-tui / ratatui-cheese (we have spin_wait already), ratatui-toaster (banner is ~30 LOC with step_shimmer), tui-logger (jackin has no log or tracing framework today). Also updates Rollout section — "no new dependencies" was no longer accurate. 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>
8 tasks
donbeave
added a commit
that referenced
this pull request
May 6, 2026
* 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>
donbeave
added a commit
that referenced
this pull request
May 6, 2026
* docs(specs): workspace manager TUI (PR 2 of 3) Design spec for PR 2 of the launcher-workspace-manager series. Adds an interactive workspace manager screen to the jackin launcher — list, create, edit, and delete workspaces without dropping to CLI. Reached via `m` from the existing Workspace picker; Esc returns to the launcher. Launch path stays keystroke-identical. Key design decisions from brainstorming (all settled, no open questions): - Entry model: separate Manager screen on `m` keypress; launch path unchanged. - Editor tab set: General · Mounts · Agents · Secrets-stub. Secrets placeholder locks in the final tab strip so PR 3 fills in the panel without a visual reshuffle. - Text-edit UX: modal push — centered overlay, one reusable TextInput widget. - Staging: explicit save via `s`. Pending changes drive dirty markers; Esc with pending opens Discard/Save/Cancel. - Create flow: mounts-first wizard — file browser for host source, dst auto-defaulted to the same absolute path as src (host-path mirror), workdir picked from mount dsts + ancestors (never free-text), name last with live uniqueness check. - Delete UX: single-line Y/N confirm modal. - Style: reuses jackin's existing digital_rain (src/tui/animation.rs), step_shimmer, spin_wait, and landing-page color tokens from docs/src/components/landing/styles.css. One new area-bounded rain widget extracted from animation.rs. Three new reusable widgets emerge (TextInput, FileBrowser, Confirm) that PR 3's Secrets tab will consume unchanged. All persisted writes flow through ConfigEditor (established in PR 1, merged in #162). Non-goals: per-(workspace × agent) env overrides (PR 3), global mount management (CLI only), agent lifecycle from manager (CLI only), CLI surface changes, CHANGELOG. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com> * docs(specs): lock third-party widget choices for PR 2 Amends the workspace manager TUI spec with a Third-party dependencies subsection that names the three ratatui ecosystem crates we'll adopt: - ratatui-textarea (v0.9.x) — single-line TextInput (ratatui-org owned) - ratatui-explorer (v0.3.x) — FileBrowser with folders-only wrapper - tui-widget-list (v0.15.x) — WorkdirPick list mechanics All three require the ratatui unstable-widget-ref feature flag. Rejected with rationale so reviewers don't re-litigate: tui-input (superseded by ratatui-textarea), tui-confirm-dialog / tui-overlay (Confirm modal is cheaper hand-rolled), rat-widget (too opinionated), throbber-widgets-tui / ratatui-cheese (we have spin_wait already), ratatui-toaster (banner is ~30 LOC with step_shimmer), tui-logger (jackin has no log or tracing framework today). Also updates Rollout section — "no new dependencies" was no longer accurate. 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>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* 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>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* docs(specs): workspace manager TUI (PR 2 of 3) Design spec for PR 2 of the launcher-workspace-manager series. Adds an interactive workspace manager screen to the jackin launcher — list, create, edit, and delete workspaces without dropping to CLI. Reached via `m` from the existing Workspace picker; Esc returns to the launcher. Launch path stays keystroke-identical. Key design decisions from brainstorming (all settled, no open questions): - Entry model: separate Manager screen on `m` keypress; launch path unchanged. - Editor tab set: General · Mounts · Agents · Secrets-stub. Secrets placeholder locks in the final tab strip so PR 3 fills in the panel without a visual reshuffle. - Text-edit UX: modal push — centered overlay, one reusable TextInput widget. - Staging: explicit save via `s`. Pending changes drive dirty markers; Esc with pending opens Discard/Save/Cancel. - Create flow: mounts-first wizard — file browser for host source, dst auto-defaulted to the same absolute path as src (host-path mirror), workdir picked from mount dsts + ancestors (never free-text), name last with live uniqueness check. - Delete UX: single-line Y/N confirm modal. - Style: reuses jackin's existing digital_rain (src/tui/animation.rs), step_shimmer, spin_wait, and landing-page color tokens from docs/src/components/landing/styles.css. One new area-bounded rain widget extracted from animation.rs. Three new reusable widgets emerge (TextInput, FileBrowser, Confirm) that PR 3's Secrets tab will consume unchanged. All persisted writes flow through ConfigEditor (established in PR 1, merged in #162). Non-goals: per-(workspace × agent) env overrides (PR 3), global mount management (CLI only), agent lifecycle from manager (CLI only), CLI surface changes, CHANGELOG. Co-authored-by: Claude <noreply@anthropic.com> * docs(specs): lock third-party widget choices for PR 2 Amends the workspace manager TUI spec with a Third-party dependencies subsection that names the three ratatui ecosystem crates we'll adopt: - ratatui-textarea (v0.9.x) — single-line TextInput (ratatui-org owned) - ratatui-explorer (v0.3.x) — FileBrowser with folders-only wrapper - tui-widget-list (v0.15.x) — WorkdirPick list mechanics All three require the ratatui unstable-widget-ref feature flag. Rejected with rationale so reviewers don't re-litigate: tui-input (superseded by ratatui-textarea), tui-confirm-dialog / tui-overlay (Confirm modal is cheaper hand-rolled), rat-widget (too opinionated), throbber-widgets-tui / ratatui-cheese (we have spin_wait already), ratatui-toaster (banner is ~30 LOC with step_shimmer), tui-logger (jackin has no log or tracing framework today). Also updates Rollout section — "no new dependencies" was no longer accurate. 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>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* 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>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* docs(specs): workspace manager TUI (PR 2 of 3) Design spec for PR 2 of the launcher-workspace-manager series. Adds an interactive workspace manager screen to the jackin launcher — list, create, edit, and delete workspaces without dropping to CLI. Reached via `m` from the existing Workspace picker; Esc returns to the launcher. Launch path stays keystroke-identical. Key design decisions from brainstorming (all settled, no open questions): - Entry model: separate Manager screen on `m` keypress; launch path unchanged. - Editor tab set: General · Mounts · Agents · Secrets-stub. Secrets placeholder locks in the final tab strip so PR 3 fills in the panel without a visual reshuffle. - Text-edit UX: modal push — centered overlay, one reusable TextInput widget. - Staging: explicit save via `s`. Pending changes drive dirty markers; Esc with pending opens Discard/Save/Cancel. - Create flow: mounts-first wizard — file browser for host source, dst auto-defaulted to the same absolute path as src (host-path mirror), workdir picked from mount dsts + ancestors (never free-text), name last with live uniqueness check. - Delete UX: single-line Y/N confirm modal. - Style: reuses jackin's existing digital_rain (src/tui/animation.rs), step_shimmer, spin_wait, and landing-page color tokens from docs/src/components/landing/styles.css. One new area-bounded rain widget extracted from animation.rs. Three new reusable widgets emerge (TextInput, FileBrowser, Confirm) that PR 3's Secrets tab will consume unchanged. All persisted writes flow through ConfigEditor (established in PR 1, merged in #162). Non-goals: per-(workspace × agent) env overrides (PR 3), global mount management (CLI only), agent lifecycle from manager (CLI only), CLI surface changes, CHANGELOG. * docs(specs): lock third-party widget choices for PR 2 Amends the workspace manager TUI spec with a Third-party dependencies subsection that names the three ratatui ecosystem crates we'll adopt: - ratatui-textarea (v0.9.x) — single-line TextInput (ratatui-org owned) - ratatui-explorer (v0.3.x) — FileBrowser with folders-only wrapper - tui-widget-list (v0.15.x) — WorkdirPick list mechanics All three require the ratatui unstable-widget-ref feature flag. Rejected with rationale so reviewers don't re-litigate: tui-input (superseded by ratatui-textarea), tui-confirm-dialog / tui-overlay (Confirm modal is cheaper hand-rolled), rat-widget (too opinionated), throbber-widgets-tui / ratatui-cheese (we have spin_wait already), ratatui-toaster (banner is ~30 LOC with step_shimmer), tui-logger (jackin has no log or tracing framework today). Also updates Rollout section — "no new dependencies" was no longer accurate. --------- Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com> Co-authored-by: Claude <noreply@anthropic.com>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* 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>
donbeave
added a commit
that referenced
this pull request
May 7, 2026
* docs(specs): workspace manager TUI (PR 2 of 3) Design spec for PR 2 of the launcher-workspace-manager series. Adds an interactive workspace manager screen to the jackin launcher — list, create, edit, and delete workspaces without dropping to CLI. Reached via `m` from the existing Workspace picker; Esc returns to the launcher. Launch path stays keystroke-identical. Key design decisions from brainstorming (all settled, no open questions): - Entry model: separate Manager screen on `m` keypress; launch path unchanged. - Editor tab set: General · Mounts · Agents · Secrets-stub. Secrets placeholder locks in the final tab strip so PR 3 fills in the panel without a visual reshuffle. - Text-edit UX: modal push — centered overlay, one reusable TextInput widget. - Staging: explicit save via `s`. Pending changes drive dirty markers; Esc with pending opens Discard/Save/Cancel. - Create flow: mounts-first wizard — file browser for host source, dst auto-defaulted to the same absolute path as src (host-path mirror), workdir picked from mount dsts + ancestors (never free-text), name last with live uniqueness check. - Delete UX: single-line Y/N confirm modal. - Style: reuses jackin's existing digital_rain (src/tui/animation.rs), step_shimmer, spin_wait, and landing-page color tokens from docs/src/components/landing/styles.css. One new area-bounded rain widget extracted from animation.rs. Three new reusable widgets emerge (TextInput, FileBrowser, Confirm) that PR 3's Secrets tab will consume unchanged. All persisted writes flow through ConfigEditor (established in PR 1, merged in #162). Non-goals: per-(workspace × agent) env overrides (PR 3), global mount management (CLI only), agent lifecycle from manager (CLI only), CLI surface changes, CHANGELOG. * docs(specs): lock third-party widget choices for PR 2 Amends the workspace manager TUI spec with a Third-party dependencies subsection that names the three ratatui ecosystem crates we'll adopt: - ratatui-textarea (v0.9.x) — single-line TextInput (ratatui-org owned) - ratatui-explorer (v0.3.x) — FileBrowser with folders-only wrapper - tui-widget-list (v0.15.x) — WorkdirPick list mechanics All three require the ratatui unstable-widget-ref feature flag. Rejected with rationale so reviewers don't re-litigate: tui-input (superseded by ratatui-textarea), tui-confirm-dialog / tui-overlay (Confirm modal is cheaper hand-rolled), rat-widget (too opinionated), throbber-widgets-tui / ratatui-cheese (we have spin_wait already), ratatui-toaster (banner is ~30 LOC with step_shimmer), tui-logger (jackin has no log or tracing framework today). Also updates Rollout section — "no new dependencies" was no longer accurate. --------- Signed-off-by: Alexey Zhokhov <alexey@zhokhov.com> Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the spec at
docs/superpowers/specs/2026-04-23-toml-edit-migration-design.md(merged in #160).Series: PR 1 of 3 — this PR · PR 2 of 3 — workspace manager TUI (#166) · PR 3 of 3 — Environments tab + 1Password picker (#171)
ConfigEditorinsrc/config/editor.rsowns atoml_edit::DocumentMutand exposes typed setters for every mutation the app performs. Reads still useAppConfig::load_or_init(serde +toml); writes go throughConfigEditor::open → mutate → save.AppConfig::saveis removed; the in-memory mutators (trust_agent,add_mount,create_workspace, etc.) are deleted or demoted topub(crate). Nothing outsidesrc/config/can change persisted state except through the editor.src/app/mod.rs(10),src/app/context.rs,src/runtime/launch.rs, andsrc/config/persist.rs(load-time migrations).What the plan specced vs. what shipped
Five deviations from the written plan emerged during execution and are worth flagging:
ConfigEditor::savedoesn't callload_or_init. The plan's original code would have created asave → load_or_init → sync_builtin_agents → AppConfig::savecycle.save()usestoml::from_strdirectly and documents the validation-bypass invariant (validation runs at load, the editor's typed setters preserve validity).[docker.mounts.<name>.<scope>]; the realAppConfig::add_mountuses[docker.mounts.<scope>.<name>]. Fixed during Task 7; tests pin the correct shape.remove_mountcleans up empty scope tables to matchAppConfig::remove_mount's behavior — the plan omitted this branch.create_workspacedelegates toAppConfig::create_workspacefor the workdir/mount validation, matchingedit_workspace's pattern. Without this, the CLI migration would have silently lost validation.load_or_initbootstrap save is gated on!file.exists()— the implementer's first pass ran an unconditional lossy save before opening the editor, destroying user comments on upgrade. Regression testload_migration_preserves_user_commentspins the fix.There's also one forward-looking method (
upsert_agent_source) that wasn't in the original spec but emerged as necessary becauseresolve_agent_sourcemutates in-memoryAppConfigbefore the editor opens.Behavioral improvement noted
AppConfig::sync_builtin_agents(serde path) clears[agents.X.env]whenever it updates a builtin entry (constructs a freshAgentSourcewith empty env).ConfigEditor::upsert_builtin_agentpreserves env. The finalconfig = editor.save()?;inload_or_initmeans operators now keep their[agents.X.env]across builtin syncs — previously, updating a builtin's git URL (or first-run registration) silently wiped operator-owned env overrides.Test plan
cargo test -p jackin --all-targets— all 533 tests greencargo clippy -p jackin --all-targets -- -D warnings— cleansrc/config/editor.rsload_migration_preserves_user_commentsinsrc/config/persist.rs(regression for the bootstrap-gate fix)# commentin~/.config/jackin/config.tomlsurvives ajackin config mount addor any other save-triggering command.🤖 Generated with Claude Code