Refactor: decompose the six god functions (~210–480 lines each)

[0xLeif]

Six production functions run ~210–480 lines and concentrate the pillar logic that is hardest to test (init/run/release/plugin flows). Decomposing them into smaller, unit-testable units would improve coverage and readability. A follow-up to the god-function finding in the review.

---

Filed from a skeptical staff-level review (Medium-severity tier). All Critical/High findings and the correctness/doc Medium findings were fixed in #432–442.

[0xGaspar]

TL;DR — the premise no longer holds, because this issue was already executed

The six god functions this issue describes were real when it was filed, and all six have already been decomposed — by six PRs that each cite (#446) in their titles:

PR	Function decomposed
#481	plugin/install.rs::install_plugin
#482	plugin/update.rs::update_plugins
#483	config_cmds.rs::handle_config
#484	review.rs::run
#485	main.rs::run
#486	config_cmds.rs::interactive_config_edit

Those six are exactly the top six by true body length at the filing commit (0c1f42d, 2026-07-02). Nothing is left to do under the original framing. Recommend closing this issue, optionally spinning out the one genuine residual (§5) and one bug this analysis surfaced (§6) as separate issues.

Details and the re-measured inventory below, since "is the premise still true" deserved evidence rather than a vibe.

---

1. Measurement method

A "distance to the next fn" scan is unusable in this repo — trailing #[cfg(test)] mod tests blocks are routinely larger than the production code they sit under, and src/lanes/tests.rs / src/release/tests.rs are whole test files.

I measured with a brace-matching pass over src/**/*.rs that blanks strings, raw strings, char literals and comments before counting braces, so { inside a format! or a doc comment can't skew the match. #[cfg(test)] mod bodies are range-excluded, and #[test]/#[tokio::test] functions are dropped. Body length = lines from the opening { line to its matching } line inclusive. Spot-checked against the file boundaries by hand.

Baseline for the whole tree: 557 production functions, median 17 lines, mean 32.4.

2. Then vs. now — the original six

Measured at 0c1f42d (the commit at issue-filing time) versus 6eb7a3e (current main):

Function	At filing	Today	What happened
main.rs::run	401	210	#485 extracted the CLI-subcommand → action mappings
config_cmds.rs::interactive_config_edit	329	gone	#486 split into keys table + menu render + edit dispatch; largest fn in the file is now edit_config_key at 151
plugin/update.rs::update_plugins	329	104	#482 extracted update_one_plugin (194) + rebuild_after_fetch (187) + envelope builder
plugin/install.rs::install_plugin	321	204	#481 extracted CapabilityFlags + display helpers
config_cmds.rs::handle_config	262	gone	#483 extracted list rendering + masked-key dedup
review.rs::run	252	113	#484 extracted panel resolution, execution, output

Distribution shift over the same window:

	at filing	today
longest production fn	401	219
fns ≥ 300 lines	4	0
fns ≥ 250 lines	6	0
fns ≥ 210 lines	9	3
fns ≥ 200 lines	9	4

Two notes on the issue text itself. The stated upper bound of ~480 doesn't reconcile with either measurement — true max at filing was 401, and a naive line-distance scan gives 402. The "~210–480" range was probably an eyeballed/rounded figure. It doesn't change the conclusion; the six functions named by #481–#486 are unambiguously the six the issue meant.

Second, the issue frames the targets as "init/run/release/plugin flows". Three of those four pillars had no god function even at filing time, and certainly don't now:

• src/run.rs — longest production fn is execute_task at 82 lines. There is nothing to decompose here.
• src/release/* — longest is release/mod.rs::run at 151.
• src/init.rs — longest is run_remote at 163.
• src/plugin/* — the only pillar that matched, and it's the one that got Refactor: decompose install_plugin — extract capability flags + display helpers (#446) #481/Refactor: decompose update_plugins — extract per-plugin update + envelope builder (#446) #482.

The actual concentration was in main.rs, config_cmds.rs and review.rs.

3. Current inventory — top 15 production functions

#	Lines	Location	Function	branch density*	Spec module	In-flight owner
1	219	src/spec/commands.rs:604-822	new_spec	6	spec	PR #505
2	215	src/lanes/community.rs:100-314	import_lanes	21	lanes	free
3	210	src/main.rs:62-271	run	27 (18 arms)	main	PR #505
4	204	src/plugin/install.rs:425-633	install_plugin	28	plugin	free
5	194	src/plugin/update.rs:125-322	update_one_plugin	25	plugin	free
6	187	src/validate.rs:77-263	validate_single	37	validate	free
7	187	src/plugin/update.rs:413-604	rebuild_after_fetch	26	plugin	free
8	187	src/spec/commands.rs:107-293	check	20	spec	PR #505
9	165	src/lanes/mod.rs:523-692	dry_run_lane	28	lanes	free
10	164	src/plugin/wasm.rs:219-382	setup_linker	5	plugin-wasm	free
11	163	src/init.rs:237-404	run_remote	16	init	free
12	163	src/plugin/create.rs:8-177	create_plugin	5	plugin	free
13	160	src/plugin/list.rs:96-255	audit_plugins	14	plugin	free
14	159	src/protocol/mod.rs:247-410	run_message_loop	22	plugin-protocol	free
15	153	src/lanes/validate.rs:16-168	validate_lanes	38	lanes	free

* count of if / match / for / while / match-arm tokens in the body — a rough proxy for "is this long because it branches, or long because it's flat".

4. Which of these are long-but-fine

Being honest about the ones that should be left alone, because that's most of them:

• new_spec (219) — leave it. Branch density 6 across 219 lines, max nesting depth 3. It builds five scaffold template strings and writes five files in sequence. Splitting it produces five functions that are each called exactly once and tested through the same tempdir fixture you'd use today. Pure indirection. It is also the single largest function in the tree, so it will keep showing up on any length-sorted list — that's a metric artifact, not a finding.
• setup_linker (164) and create_plugin (163) — leave them. Density 5 each. A wasmtime host-function registration builder and a file-scaffolder respectively. Same reasoning.
• main.rs::run (210) — leave it. Already decomposed once by Refactor: decompose main::run — extract CLI-subcommand → action mappings (#446) #485; what remains is an 18-arm dispatch match. The residual length is the shape of the CLI, not accidental complexity.
• bump_version_files (147, src/release/bump.rs) — leave it, or at most tidy it. Eight near-identical read → regex-replace → write → record blocks. High raw branch density (31) but it's repetition, not complexity, and src/release/tests.rs already covers it through the public entry with 9+ per-ecosystem tests. A data-driven [(filename, regex)] table would be nicer to read but buys no new test coverage.
• validate_single (187, src/validate.rs) — low priority. Genuinely branchy, but it's already a pure &Path -> Result<ValidationReport> with no prompts and no network, and src/validate.rs has 14 unit tests. Everything in it is reachable from a tempdir fixture today. Readability-only win.
• install_plugin (204) / update_one_plugin (194) / rebuild_after_fetch (187) — low priority, post-Refactor: decompose install_plugin — extract capability flags + display helpers (#446) #481/Refactor: decompose update_plugins — extract per-plugin update + envelope builder (#446) #482. These are the residue of decomposition, and it shows: install_plugin delegates to 12 named helpers already. Its remaining length is orchestration plus a rollback path repeated at ~10 exit points (remove_plugin_path(&plugin_dir).ok()). The one worthwhile move is an RAII rollback guard (InstallGuard that cleans up on drop unless .commit()ed), which would delete the repetition and make the rollback discipline testable in isolation. That's a nice change, but it's a correctness-hardening change wearing a refactor costume, and should be scoped as such. update_one_plugin is a decision tree whose length is mostly paired println!/json! literals; splitting it moves lines without creating a seam.

5. The one genuine residual: import_lanes (215, src/lanes/community.rs)

This is the only untouched function on the list that is long because it welds unrelated concerns together, and it has the clearest testability payoff.

It currently does, in one body: fledge.toml presence check → source parsing → trust-tier gating and interactive confirmation → GitHub API fetch → base64 decode → TOML parse → merge against local config → TOML text generation → safe-filename derivation → directory creation and write → two-branch JSON/pretty output for both the empty and non-empty cases.

The merge + content-generation core is completely pure and completely untested. The evidence that this matters is in the test file itself — src/lanes/tests.rs:791 merge_imported_lanes does not call production code. It reimplements the merge inline:

for (name, task) in imported.tasks {
    base.tasks.entry(name).or_insert(task);
}
for (name, lane) in imported.lanes {
    base.lanes.entry(name).or_insert(lane);
}

That is not what import_lanes does. Production doesn't merge into the base config at all — it emits a TOML string to a separate .fledge/lanes/<safe>.toml, tracks imported/skipped name lists, and skips the write entirely when imported is empty. So we have a passing test that asserts nothing about the shipped behaviour, and the written: false / all-skipped path (a documented --json contract in AGENTS.md) has no unit coverage at all.

Proposed extraction, all within src/lanes/:

1. fn plan_lane_import(remote: &FledgeFileWithLanes, existing: &FledgeFileWithLanes, display_source: &str) -> LaneImportPlan returning { content: String, imported: Vec<String>, skipped: Vec<String> }. Pure. Newly testable: task dedup, lane skip-on-conflict, the # Imported from header, TOML escaping of commands, and the empty-imported case.
2. fn import_target_name(owner: &str, repo: &str, subpath: Option<&str>) -> String — the safe_name derivation, currently inline and untested. Pure, and it's a path-construction routine derived from remote input, so it's worth having a seam on it.
3. fn fetch_remote_lane_config(owner, repo, subpath, git_ref, token) -> Result<FledgeFileWithLanes> — isolates the network + base64 + TOML-parse leg. Composes with the mock harness landing in PR test: mocking harness + $HOME isolation + publish coverage (#447) #504.

import_lanes then reduces to guard → confirm → fetch → plan → write → render, at roughly 70 lines. src/lanes/community.rs currently has zero inline unit tests; this creates the first real ones. Cost: one module (specs/lanes/), one spec update, one change record.

6. Bug found while measuring: duplicated cycle detection in lanes

Not what I was looking for, but worth its own issue. There are two task-dependency cycle detectors in src/lanes/, and only one is correct.

check_dep_cycle (src/lanes/mod.rs:506) is a correct DFS with backtracking — it does visiting.remove(name) on the way out, and src/lanes/tests.rs:204 validate_no_cycle_with_shared_deps covers the diamond case.

validate_lanes (src/lanes/validate.rs:122-138) reimplements it iteratively without backtracking:

let mut visited = HashSet::new();
let mut stack = vec![task_name.as_str()];
while let Some(current) = stack.pop() {
    if !visited.insert(current.to_string()) {
        report.errors.push(format!(
            "Circular dependency detected involving task '{task_name}'"
        ));
        break;
    }
    ...
}

visited is never popped, so any node reachable by two distinct paths reports a cycle. Given a -> [b, c], b -> [d], c -> [d] — a diamond, no cycle — d is visited twice and fledge lanes validate emits Circular dependency detected involving task 'a', a hard error. Trivially reproducible, and it means lanes validate false-fails on any config where two tasks share a dependency, which is a completely ordinary shape.

validate_lanes has no unit tests (the validate_circular_deps test exercises validate_lane, the other function). The fix is to delete the duplicate and call check_dep_cycle — which is exactly the extraction this issue is about, so it's a fair advertisement for the residual value in §5.

7. Constraints on any follow-up work

• PR feat: fledge spec lint — quality gate for the spec itself (#429) #505 (spec lint) owns src/main.rs, src/cli.rs, src/spec/*. That covers targets feat: core scaffolding engine and 5 built-in templates #1, Expand README and add template authoring guide #3 and CorvidLabs template collection and org defaults #8 — the three largest functions in the tree. Do not touch them until it lands. All three are in the "leave it alone" bucket anyway, so this costs nothing.
• PR test: mocking harness + $HOME isolation + publish coverage (#447) #504 (mocking harness) owns src/publish.rs, src/github.rs, src/llm.rs, src/doctor.rs, src/test_support.rs. No top-15 target lives there. Its HttpMock harness is a prerequisite for testing extraction (3) in §5, so §5 should land after it.
• Issue Feature: stream child output for interactive fledge run tasks #507 is adding --stream to src/run.rs. No collision — src/run.rs has no function over 82 lines and is not a target under any reading of this issue.
• Spec-sync cost. Every src/ change needs a matching module-spec update plus a governance change record, so a refactor spanning modules is disproportionately expensive here. Both §5 and §6 are contained entirely within specs/lanes/, which is the cheapest possible shape: one spec, one change record, one PR.

8. Recommendation

1. Close Refactor: decompose the six god functions (~210–480 lines each) #446. Its stated scope was completed by Refactor: decompose install_plugin — extract capability flags + display helpers (#446) #481–Refactor: decompose interactive_config_edit — extract keys table, menu render, edit dispatch (#446) #486.
2. Open a narrow follow-up for import_lanes (§5) — one module, one spec, real new coverage on a currently-faked test. Sequence it after PR test: mocking harness + $HOME isolation + publish coverage (#447) #504 lands so the network leg can use the mock harness.
3. Open a bug for the validate_lanes cycle-detection false positive (§6). That's a shipped correctness defect, not a refactor, and shouldn't wait behind a cleanup issue.
4. Do not open follow-ups for anything else in the top 15. Post-Refactor: decompose install_plugin — extract capability flags + display helpers (#446) #481–Refactor: decompose interactive_config_edit — extract keys table, menu render, edit dispatch (#446) #486 the tree's longest function is 219 lines with a branch density of 6; there is no god-function problem left to solve, and further length-driven splitting would trade readable code for indirection plus spec-sync overhead.