signed/docs/REFACTOR_PLAN.md

Backend refactor plan

Branch: audit @ 6c4cc9f. Baseline: cargo check --workspace clean (~10s incremental), cargo test --workspace 111/111 passing.

Scope: signed_core, signed_git, signed_nostr, signed_state, utils, paths, settings (~13,000 lines). workspace, signed_ui, dock, desktop are touched only as call sites. .delta/ is ignored throughout.

Goals

1. Behavior lives in impl blocks on a named domain type. No free functions in domain crates.
2. Free functions are allowed only in utils (and paths, which is the same kind of crate) — pure, stateless helpers.
3. Comments only where they carry non-obvious "why": protocol rules, safety invariants, races, ordering guarantees. No summaries, no field-name restatements.
4. Tests only for protocol interop and user-visible contracts. Delete tests that lock internal structure.

Everything below is behavior-preserving except the explicit deletions in Phase 0.

---

1. Audit summary

Crate	Lines	Free fns (non-test)	Impl blocks	Comment lines	Tests
signed_core	2,226	47	6	~216	29
signed_git	3,743	52	3	~276	38
signed_nostr	257	3	1	~6	0
signed_state	6,206	53	~20	~460	25
utils	98	4	0	~3	3
paths	105	8	0	~26	0
settings	369	1	4	~44	5

Structural findings

• signed_git is 52 free functions and 3 impl blocks. gix::open is called 29×; nearly every entry point re-opens the repository. Eight near-duplicate worktree_*/repo_* pairs exist only to bridge &Path ↔ &gix::Repository (worktree_all_commits/all_commits, worktree_branches/repo_branches, worktree_ref_state/repo_ref_state, worktree_current_branch/current_branch, head_commit_id/head_commit, …). The worktree_ prefix means two different things (path-based vs &gix::Repository-based). GitCache is the one type done right — it is the template for the refactor.
• signed_state/backend.rs (2,020 lines) is a god object plus 24 free helpers. create_repository (L370–546) and publish_local_repo (L549–722) duplicate ~120 lines of validation/announce/push/retract/fan-out; push_repo_from (L752) is already the factored third variant. The grasp push loop (push_staged_to_grasps + 6 helpers) and the grasp URL family float freely. Backend::connect_repo_relays (method) calls a free fn of the same name (L1375).
• signed_core operates on foreign types. 47 free functions take &Event/&RepoAddr-alias first args or are module-bound constructors; inherent impls are impossible without introducing types. RepoAddr is a bare alias for the SDK's Coordinate, so it can carry no methods.
• signed_nostr::new_backend returns a tuple (Client, UniversalSigner) whose halves are always created and consumed together.
• Duplication across crates: three middle-truncation helpers (utils private truncate_middle, signed_ui::middle_truncate, workspace::truncate_label), two hand-rolled mbox envelope parsers, three git-CLI spawn patterns, and workspace/src/views/repo/mod.rs:1955 re-assembling by hand what worktree_snapshot should provide.

Dead code (grep-verified, zero callers outside own crate)

Item	Location	Note
login, login_with_new_identity, login_with_nsec, login_with_bunker, logout, with_master_key	backend.rs L917–1025, L1451	No UI path calls these; import_dialog::open is an empty stub (TODO.md confirms)
BackendEvent::SyncProgress pipeline (variant, sync_progress field + accessor, progress task)	backend.rs L43, L64, L1104, L1191–1263	No subscriber; TODO.md retains it for an unbuilt indicator
BackendEvent::Error	backend.rs L47	Emitted 9×, subscribed 0× — errors currently vanish
RepoStore::merge_pull_request + publish_applied_status	repo.rs L1195, L1499	Zero callers
Inbox::mark_read / mark_archived	inbox.rs L27, L43	Zero callers
InboxItem::kind	signed_core/inbox.rs L45	View reads root_event.kind directly
filters::notification_comments, filters::NOTIFICATION_KINDS, inbox::notification_root re-exports	signed_core	Internal-only despite pub + root re-export
paths::{home_dir, config_dir, data_dir} pub-ness	paths/lib.rs	Internal-only
local_repo_addr re-export	signed_state/lib.rs L19	Internal-only
wasm32 code paths (new_backend wasm variant, init wasm variant, cfg! guards, nostr-memory dep)	signed_nostr/backend.rs L25, signed_state/lib.rs L65, backend.rs L174, checkouts.rs L103/136/162	No wasm target exists in the workspace
Unused signed_core/lib.rs root re-exports	lib.rs L11–14	Callers use filters::/inbox:: module paths

Speculative generality

• U: AsRef<str> generics on clone_repo, fetch_repo_refs, GitCache::ensure_clone — every call site passes &str/String.
• GraspSignals exposes 10 bools; consumers read 4.
• RepoStore::set_status and reply are pub but only used internally (draft-PR flow / comment).

---

2. Target types

New or promoted types that absorb the free functions. Each row is a home, not a new abstraction layer — they are mostly newtypes over data the code already passes around.

Crate	Type	Absorbs
signed_core	RepoAddr (newtype over Coordinate, replacing the alias)	repo_addr → RepoAddr::new; identifier_from_name → RepoAddr::identifier_from_name (NIP-34 identifier policy stays here, not utils); enables addr.announcement_filter() etc.
signed_core	PullRequest<'a>(&'a Event)	pull_request_patches, pull_request_patch, latest_update, private forward_series, patch_produces_commit
signed_core	trait Nip34Event (extension on Event)	current_commit_of, merge_base_of, clone_urls_of, branch_name_of, activity_subject, references_root, is_git_activity — needed because the same accessors apply to PR roots and updates, where PullRequest can't go
signed_core	RepoState { refs, head }	build_state → RepoState::builder(id), parse_state → RepoState::from_event; kills the anonymous tuple return
signed_core	ThreadResolver<'a> (wraps the event lookup closure)	resolve_thread_root, parent_id, nip10_root_id, first_e_id, first_uppercase_e_id, first_tag_id, e_tag_with_marker, notification_root
signed_core	Filters (unit struct)	the 12 filter constructors (announcement, state, activity, statuses_for, grasp_list, comments_for, notifications, authored_activity, all_announcements, all_states, deletions, deletions_for_repo) — repo-scoped ones become RepoAddr methods
signed_core	Announcement::forks_in (assoc fn)	fork_candidates
signed_nostr	NostrBackend { client, signer }	new_backend → NostrBackend::open(path); private with_database assoc fn; kills the tuple
signed_git	Repo (wrapper over gix::Repository)	~46 of the 52 free functions — see Appendix A. Repo::open, Repo::open_cached (object cache for history walks), Repo::init, Repo::clone. One gix::open per operation instead of 29 scattered calls. The worktree_*/repo_* pairs collapse
signed_git	GitCache (existing)	unchanged except open/ensure_clone return Repo
signed_state	GraspServer<'a>(&'a RelayUrl)	grasp_base_url, grasp_clone_url, grasp06_prs_url, pr_clone_urls, grasp_list_servers, latest_grasp_list_servers, user_grasp_list_servers
signed_state	GraspPush { client, signer, executor }	push_staged_to_grasps, sign_state_event, stage_event_on_relay, keep_newest, is_transient_grasp_denial, is_stale_advertisement_race (→ PushRejection::classify), PushOutcome — moved to a new push.rs
signed_state	BunkerCredential { uri, session_key }	extract_master_key, with_master_key (if the bunker login path survives Phase 0 — otherwise deleted)
signed_state	StatusIndex(HashMap<EventId, RepoStatus>)	resolve_statuses, status_of; RepoStore::status_of reads through it
signed_state	PatchSeries	patch_current_commit, split_patch_series consumers in repo.rs
signed_state	Mirrors (unit struct over the OnceLock<GitCache>)	the 6 free fns in git_store.rs
signed_state	methods on existing types	query_inbox/load_state/save_state/fetch_notifications/event_references → Inbox; pr_proposes_checkout, resolve_associations, checkout_status, checkout_push_status, compute_statuses → CheckoutsStore assoc fns; resolve_local_repos → LocalReposStore; next_update → impl UpdateEvent; retract_event, publish_best_effort, ensure_bootstrap_relays, subscribe_bootstrap_only, sync_bootstrap_only → Backend; publish_patch_series, comment_builder → RepoStore; url_identity, same_repo_url → utils
utils	consolidation	one middle_truncate (absorbing 3 copies — signed_ui gains a utils dep), flatten_whitespace, url_identity/same_repo_url, latest, sort_newest_first/sort_oldest_first

settings needs nothing: it is already plain data + one store. default_scan_paths folds into the Default impl.

---

3. Phases

Each phase ends with cargo check --workspace && cargo test --workspace green and its own commit, so review and bisection stay cheap. Call-site updates in workspace/signed_ui/desktop happen in the same commit as the API change.

Phase 0 — Delete dead code and non-critical tests

The cheapest phase; shrinks every later diff.

1. Delete the login/logout family + with_master_key (backend.rs). Either delete the empty import_dialog stub and its menu entry, or leave the stub — decision D1.
2. Delete the SyncProgress pipeline (variant, field, accessor, progress task). TODO.md's planned indicator can re-add from git history when it is actually built — decision D2, default delete.
3. BackendEvent::Error: 9 emit sites, 0 subscribers. Either wire one subscriber in workspace (a banner/toast, ~15 lines, makes existing error emissions user-visible) or delete variant + emissions — decision D3, default: wire the subscriber (AGENTS.md requires errors reach the UI; the emissions already exist).
4. Delete merge_pull_request + publish_applied_status (recoverable from history if the merge UI is built) — decision D4, default delete.
5. Delete Inbox::mark_read/mark_archived, InboxItem::kind.
6. Demote to private: RepoStore::set_status, RepoStore::reply, Backend::restore_session, Backend::set_signer.
7. Privatize filters::notification_comments, NOTIFICATION_KINDS, inbox::notification_root, paths::{home_dir, config_dir, data_dir}; drop unused signed_core/signed_state root re-exports (local_repo_addr, GraspSignals path, etc.).
8. Remove wasm32 paths + nostr-memory target dep — decision D5, default remove (no wasm target exists). Revert is trivial if a web build materializes.
9. Replace U: AsRef<str> generics with concrete &[String]/&str signatures.
10. Delete all NON-CRITICAL tests (list in §5). Doing this before the structural phases means the structure changes never touch them.

Phase 1 — utils consolidation

• Add middle_truncate; delete signed_ui's private copy and workspace::truncate_label; shorten_pubkey calls it.
• Move in: flatten_whitespace, url_identity, same_repo_url, latest, sort_newest_first, sort_oldest_first.
• These are drop-in; domain crates already depend on utils.

Phase 2 — signed_core

Dependency bottom, consumed by everything else.

1. RepoAddr: alias → newtype with new, identifier_from_name, Display/From/FromStr delegation. Mechanical fallout across all crates (~30 sites).
2. Nip34Event extension trait for the tag accessors; PullRequest<'a> newtype; Announcement::forks_in.
3. RepoState (replaces the tuple return; one consumer in signed_state/repo.rs refresh).
4. ThreadResolver<'a> absorbing the seven inbox/threading helpers.
5. Filters unit struct + repo-scoped filters as RepoAddr methods; RepoStatus::resolve assoc fn.

Phase 3 — signed_nostr

NostrBackend::open / in_memory; signed_state::init constructs it. ~30 lines.

Phase 4 — signed_git → Repo

The biggest mechanical phase.

1. Introduce Repo wrapping gix::Repository with open, open_cached, init, clone.
2. Move every function from Appendix A into methods; collapse the 8 duplicate pairs; the worktree_ prefix disappears (repo.dirty(), repo.snapshot(), repo.branches() …).
3. Fold remote operations into methods (push_ref, push_all, fetch_refs, origin_url, ensure_origin, set_origin, remote_has_refs); one git-CLI spawn helper instead of three patterns.
4. One mbox envelope parser shared by split_patch_series/patch_commits (stays in signed_git as an impl on the patch parser, not utils — it is not generic).
5. GitCache::open/ensure_clone return Repo.
6. Extend Repo::snapshot() with branches/tags/current_branch so workspace/src/views/repo/mod.rs:1955 (load_repo_data) collapses into it.
7. Keep pure string parsing (sanitize_path_component, fork_namespace, split_patch_series, patch_diffs, patch_commits) as assoc fns on the owning type (Repo::sanitize_path_component, PatchParser::…) or move to utils if they carry no git semantics — Appendix A marks each.

Phase 5 — signed_state

1. New push.rs: GraspServer newtype + GraspPush + PushOutcome/PushRejection, moving ~400 lines out of backend.rs.
2. Factor the shared announce→authorize-push→retract-on-failure→fan-out orchestration out of create_repository/publish_local_repo, using push_repo_from as the template (~120 lines saved). Add publish_one (sign → send → require_relay_accepted, returning the Event) and collapse its 3 open-coded copies in repo.rs; merge the shadowed connect_repo_relays pair.
3. StatusIndex, PatchSeries, Mirrors; Inbox/CheckoutsStore/LocalReposStore/Backend/RepoStore method moves per §2.
4. Fold push_repository/push_checkout wrapper bodies into one helper.
5. Consider whether backend.rs (~2,020 → ~1,200 lines) still warrants a file split after the push extraction; stop there — the remaining methods are genuinely Backend's job.

Phase 6 — Comment stripping

Apply §4 across all backend crates. Mechanical; do it last so it never masks a behavioral diff.

Phase 7 — Final sweep

• cargo fmt, cargo clippy --workspace, full test run.
• Invariant grep: grep -rn '^\(pub \)\?\(async \)\?fn ' crates/{signed_core,signed_git,signed_nostr,signed_state}/src returns only lib.rs init and test helpers.
• Update docs/TODO.md (both entries are consumed by this plan) and this document's status.

---

4. Comment policy

Delete by default. Keep only (one line each, unless truly needed):

• Protocol/format rules: NIP-34 u-tag parsing workaround (model.rs L311 — the SDK's Nip34Tag doesn't model the u tag); subject-tag fallback; refs/heads publication form in state events; NIP-10 e vs NIP-22 uppercase E semantics; kind→root mapping table in inbox.rs L115–124; comments_for returning two filters because #E+#e would AND (filters.rs L109–112); ngit URL parity notes (grasp_base_url scheme mapping, /prs/ namespace, clone tag ordering).
• Safety invariants: sanitize_path_component untrusted-input contract; fast-forward-only branch updates; checkout_push_status never fetching checked-out refs.
• Races / retry taxonomy: grasp denial classification (backend.rs L1620–1638 — the single most valuable comment in the codebase); stale-advertisement race; same-second resend dedup on sign_state_event; convergence probe rationale.
• Ordering guarantees: commits_since oldest-first (git am order); connect-relays-before-send; state-event-authorizes-push; retract-on-failed-push; fan-out-after-landing.
• Non-obvious None/error contracts where callers depend on them (merge-base Ok(None) = unrelated history, unborn HEAD → None).
• Perf "why": object-cache rationale in history walks; commit cap for virtual lists.

Delete: every field doc that restates the field name, method summary one-liners, /// Get the profile.-style docs, organizational comments, test-helper docs, per-constant restatements, duplicated platform tables in paths.

Expected reduction: ~1,030 comment lines across backend crates → roughly 200.

---

5. Test disposition

Rule: a test earns its place if breaking the behavior would silently break a user (protocol wire formats, interop with git/ngit/nak, state machines users hit). Delete tests of internal ordering, trivial helpers, and coalescing machinery.

Keep (critical interop, ~73 tests)

• signed_core/model.rs: announcement parsing, malformed-value dropping, u-tag fork semantics (3), maintainer authority (2), patch-set chain assembly (3), latest_update author scoping (2).
• signed_core/inbox.rs: threading/root resolution (5) + group thread-merging.
• signed_core/filters.rs: both (NIP-22 K vs NIP-34 k).
• signed_core/status.rs, state.rs: all (kind-30618 wire format, latest-wins, authority, round-trip).
• signed_git/tests.rs: split_patch_series…, patch_commits…, parses_real_format_patch_output, push_all_mirrors…, remote_has_refs…, fetch_repo_refs_imports_heads_under_a_prefix, repo_ref_state_lists…, root_commit_reports…, blocks_parent_components, merge_base_finds…, working_copy_cloned_from_the_mirror…, head_commit_and_commits_since… (pins git am order), plus fast_forward_branches_moves_the_mirror_and_keeps_local_work — kept as a safety invariant, not structure.
• signed_git/nip34.rs: all 10 (on-disk marker interop with nak/ngit).
• signed_state/backend.rs: the 6 URL-format/denial-classification tests.
• signed_state/repo.rs: all 3 (format-patch header, NIP-22 comment builder, maintainer filters).
• signed_state/checkouts.rs: same_repo_url…, checkout_status_reports_ahead_branches_only, checkout_push_status_counts_unpushed_commits_only.
• signed_state/local_repos.rs: the 2 sidebar-contract tests (own announcement dropped / other owner's linked).
• settings: save_and_load_roundtrip, partial_json_merges_with_defaults (breaking these silently resets user settings).

Delete (~38 tests, ~1,100 lines)

• signed_git/tests.rs (16): scan walker, init_repository contents, set_origin, worktree_last_commits (2), all_commits_lists_every_commit, find_readme_prefers_markdown, current_branch_tracks_checkout, worktree_snapshot_reflects…, diff display (4), worktree_dirty…, worktree_commits_ahead….
• signed_state (10): push_outcome_reports_partial_failures, resolve_orders_remembered_freshest_first, resolve_deduplicates_paths…, the 4 local_repos trivial pass-through/name tests, all 4 refresh.rs coalescing tests.
• signed_core (9): the 4 InboxReadState policy tests, 2 fork_candidates ordering/policy tests, 3 utils formatting tests.
• settings (3): keep only the 2 named above.

Consolidate retained helpers while there: three git-env helpers → one, five bare-server git init --bare blocks → one fixture, delete the inline run closure at tests.rs L634.

Out of scope but flagged: signed_ui/pixel_avatar (identicon determinism — borderline, visual), dock/tests/render_smoke.rs (UI smoke).

---

6. Risks

• PullRequest<'a> lifetimes at pull_requests/detail.rs call sites — mitigated by keeping it a borrow newtype, not an owning copy.
• Repo wrapper vs background tasks: RepoStore spawns git work via cx.background_spawn. The wrapper must stay Send when moved into tasks exactly as gix::Repository is today; no interior mutation planned, so this is mechanical.
• Phase 2 RepoAddr newtype touches ~30 call sites in one commit — small each, many files; consider impl From<Coordinate> bridges to keep the diff mechanical.
• Backend push orchestration dedup (Phase 5.2) is the only phase that reorders logic; land it alone in its own commit behind the existing tests (URL-format + denial-classification + push_all/fetch_repo_refs interop tests are the net).

7. Open decisions

#	Question	Default
D1	Delete the empty import_dialog stub + menu entry, or keep the shell?	Keep shell (UI territory, 0 cost)
D2	SyncProgress pipeline	Delete; re-add with the feature
D3	BackendEvent::Error (9 emitters, 0 subscribers)	Wire one workspace subscriber
D4	merge_pull_request (unwired feature)	Delete; recover from history when merge UI is built
D5	wasm32 code paths + nostr-memory dep	Remove (no wasm target in workspace)
D6	paths: keep as a second function-crate, or fold into utils?	Keep separate (no nostr dep for path users)

---

Appendix A — signed_git function → Repo method mapping

Current free fn	Becomes
merge_base, head_commit_id, commits_since, root_commit, refs_with_prefix, delete_refs_with_prefix	Repo::merge_base, head, commits_since, root_commit, refs_with_prefix, delete_refs_with_prefix
init_repository	Repo::init
worktree_current_branch/current_branch	Repo::current_branch
worktree_ref_exists	Repo::ref_exists
worktree_branches/repo_branches	Repo::branches
worktree_ref_state/repo_ref_state	Repo::ref_state
repo_tags	Repo::tags
fast_forward_branches	Repo::fast_forward_branches
worktree_checkout_branch/worktree_checkout_tag	Repo::checkout_branch / checkout_tag
worktree_snapshot	Repo::snapshot (+ branches/tags/current_branch, absorbing load_repo_data)
worktree_dirty	Repo::is_dirty
worktree_commits_ahead	Repo::commits_ahead
worktree_all_commits/all_commits	Repo::all_commits
worktree_last_commits/last_commits	Repo::last_commits
worktree_commit_range_commits	Repo::commit_range
worktree_commit	Repo::commit
worktree_commit_diff/commit_diff	Repo::commit_diff
worktree_commit_range_diff	Repo::range_diff
worktree_entries, worktree_read, find_readme	Repo::entries, read, find_readme
clone_repo	Repo::clone
push_commit_ref, push_main, push_all	Repo::push_ref, push_main, push_all
remote_has_refs	Repo::remote_has_refs
ensure_origin, set_origin, origin_url	Repo::ensure_origin, set_origin, origin_url
fetch_repo_refs, fetch_all	Repo::fetch_refs, fetch
apply_patch, format_patch_between	Repo::apply_patch, format_patch_between
detect_nip34, set_nostr_repo	Repo::nip34_binding, set_nostr_repo
find_git_repos	stays free in scan.rs (directory-tree scan, no repo opened) — acceptable exception, or utils
sanitize_path_component, fork_namespace	assoc fns on GitCache (they build its layout)
split_patch_series, patch_diffs, patch_commits	assoc fns on a small PatchParser impl (pure parsing, no Repo)
is_grasp_url + private nip34 url helpers	private assoc fns of the NIP-34 detection impl
repository_signature, open_with_cache, resolve_commit, move_head, force_checkout, edit_local_config	private methods/assoc fns on Repo