refactor(src): split-then-directory layout with lib/bin split and test siblings - #138
Merged
Merged
Conversation
One payload shape for every commit-message drafting path: staged first draft and plan-time pre-draft both wrap diffs identically, with hunk slicing for planned batches.
BoxFuture, Resolver, Prompt, BatchPlanner, and CommitMessenger now live in the shared-type module so no workflow module owns another's seam vocabulary, keeping the module graph acyclic.
main.rs carried full workflow implementations alongside CLI dispatch. Move the commit Run and resolve workflow into run.rs and resolve.rs with seams bundled as RunDeps/ResolveDeps, and relocate prompt_yes_no to input. No behavior change.
Replace run_commit_workflow_impl/run_resolve_workflow_impl calls with commit_run/default_run/resolve_run taking RunDeps/ResolveDeps structs, re-point imports to run/resolve/types modules, and drop the now-unused unreachable_resolver helper.
Add glossary entry for src/resolve.rs (resolve_run, ResolveDeps, run→resolve dependency) and drop it from the avoid list
Pure code move: classify_line, reasoning_rows, loading_rows and the tagged wrap engine now live in markdown.rs, leaving progress.rs with only the terminal renderer. No behavior change; imports and doc links updated.
Documents src/markdown.rs as ADR 0013's pure line-to-styled-rows painter and its boundary with the reasoning feed and progress::ReasoningRenderer
New plan_from_diffs builds a BatchPlan directly from per-file workdir diffs in repo order, using conservative default config. Serves as the Run's fallback when the LLM plan fails validation.
An invalid LLM batch plan is a model malfunction, not a user error. Warn and regroup hunks deterministically via grouping::plan_from_diffs instead of failing the Run.
Issue #34 contract flipped: an empty LLM batch plan is now an LLM malfunction handled by warning and regrouping with the deterministic engine, not a hard rejection. Re-pin the e2e test to assert the fallback notice, per-batch drafts, two commits, and a clean tree.
Update module docs and CONTEXT.md: plan_from_diffs now regroups and completes the Run when the LLM plan fails validation, making the engine a live fallback rather than unwired foundation.
Move strip_code_fence, parse_json_response, and classify_retry from llm.rs into a new parse module so both API and CLI-agent backends share the same tolerant parsing and retry classification without reverse-importing from a sibling. Also relocate LlmError to cli_agent.rs where its CLI failures belong.
Replace the setup wizard's verify_preflight and inline LLM struct literals with ResolvedConfig::from_parts -> validate -> to_llm, reusing the Run path's validation. Make LLM fields private behind a constructor and derive the default provider from Provider::default.
Replace the free function with a Draft method and use Provider::default() instead of hardcoded OpenAI in provider_label, provider_submenu_items, and finalize.
Move CommitType/ParsedMessage vocabulary and all color decisions out of types.rs into their own modules; types.rs keeps only workflow seam aliases. No behavior change.
Centralizes success, emphasis, added/removed, and pending colors in the palette module so theme and contrast rules live in one place instead of ad-hoc Style chains.
The conflict resolve-flow UI renders through these same primitives instead of a parallel styling path — one seam for all output.
…ict module Conflict UI becomes free functions over Display so the conflict domain owns its vocabulary while Display keeps render primitives. Callers in resolve.rs and run.rs updated; finalize_hint and its test move too.
Add reasoning_sink() as the single module entry point returning the renderer as a ReasoningSink. Renderer's inherent methods become private so the trait impl is the only public protocol and the two doors cannot drift apart.
…t siblings
Adopt the ADR-0015 layout policy:
- src/lib.rs becomes the module root; main.rs is a thin dispatch (~60
lines). e2e stays in-crate on lib (it reads cfg(test) pub(crate)
helpers); resolve_workflow/default_workflow widen to pub for the bin.
- setup.rs (2108 lines) splits into setup.rs + setup/{provider,
cli_flow,verify,finalize,tests}.rs along its natural sub-flows;
wizard/menu state machine stays in the parent.
- git.rs (1550 lines) splits into git.rs + git/{status,diff_view,tests}.rs
(ADR-0016, superseding ADR-0006's one-file reading). The coupled core
ADR-0006 defended — handle, run_git, index refresh, add, stage_hunks,
commit, guards — never leaves the parent; no seam introduced.
- Test mods of every module at or past ~1000 total lines move to sibling
tests.rs files (config, cli_agent, llm, display, markdown, progress,
decoder, setup, git), so product files read as product.
- docs: ADR-0015, ADR-0016, revision note on research-src-layout.md,
CONTEXT.md Run-module entry now names lib.rs as module root.
CLI surface, crate:: paths, and the crate's public face are unchanged
(two workflow fns widened for the bin target). 402 tests + 2 doctests
green, rustfmt clean.
CaicoLeung
force-pushed
the
refactor/src-layout-split-then-directory
branch
from
August 15, 2026 06:12
796fa80 to
da1620b
Compare
Per review of #138: the modern parent-file style left src/ holding ~30 flat product files beside test-only directories, which reads as half-moved modules. Switch to one uniform rule (ADR-0015 rule 2): every module is foo/mod.rs, so src/ holds only lib.rs, main.rs, and module directories. Pure git mv (31 renames detected) except one include_bytes! path in update/ that gained a ../ level. - docs: ADR-0015 rules reworded (mod.rs style; parent-file style moved to rejected alternatives), ADR-0016 and the research-src-layout revision note updated, CONTEXT.md path refs now cite src/foo/. - cargo test: 402 + 2 doctests green, rustfmt clean.
The research doc's Recommendation 1: a grouped, commented module list in the crate root as the findability fix — the src/ root and this list are the crate's table of contents. Pure comments; no path changes.
…nder/workflow) Navigation-driven regrouping, owner's call (ADR 0015 rule 1 amended): src/ reads as five domains instead of 32 siblings. Coupling explicitly not a goal — 55% of use-crate edges cross group lines by design (rust-analyzer precedent: topic index, not dependency firewall). 29 module dirs moved via git mv (rename detection 84-100%); ~232 crate::/aic:: path sites rewritten; crate::git::* unchanged (group root is the git module itself). ADR 0015/0016 amended, research doc second-revision note added, CONTEXT.md paths updated.
CaicoLeung
commented
Aug 15, 2026
CaicoLeung
left a comment
CaicoLeung
left a comment
Owner
Author
There was a problem hiding this comment.
Verdict: net positive; the redesign is reasonable. (Posted as COMMENT — GitHub blocks self-approval on your own PR; content is an approval-with-comments review. Full artifact: .claude/PRPs/reviews/pr-138-review.md.)
Design assessment
- lib/bin split —
lib.rsis a 16-line module map;main.rsa thin dispatch (verified:aic --helpoutput unchanged). Real incremental-compile win. - Five domain roots as navigation, explicitly not a coupling firewall — ADR-0015 says this out loud (55% of edges still cross group lines). Avoids the "directories = architecture" cargo cult the research doc measured against; the owner's overrule of the anti-regrouping finding is documented with the measurement left standing. Honest.
- git split (ADR-0016) is the sharpest move: refutes ADR-0006's implicit "one module = one file" using Rust's actual semantics — the module spans
mod.rs+ child files with zero seams, and the coupled core (run_git,index(),stage_hunks) never leftmod.rs. Verified: 12/3/9 fns acrossmod.rs/status.rs/diff_view.rs. - e2e in-crate keeps
pub(crate)fixtures private; onlydefault_workflow/resolve_workflowwentpub, exactly as claimed. - Riding series (
RunDeps/ResolveDepsseams) is itself good: e2e now drives workflows with stubs directly; the e2e resolve diff is a clean args→struct migration with assertions untouched.
Findings
MEDIUM
- Behavior change in a refactor-titled PR: 918b293 + 437a434 make an invalid LLM batch plan warn + regroup deterministically instead of failing the Run. Disclosed via "rides in", but the body's What/Acceptance sections list layout only — add one line + a release note so bisectors aren't surprised. (The change itself is good: valid partition by construction, defensive re-validation, e2e pin in d5a65f9.)
LOW
src/e2e/mod.rs:3-4doc comment still namesrun_resolve_workflow_impl/run_commit_workflow_impl— those fns no longer exist. One-line fix.pub mod coreshadows the built-incorecrate under uniform paths (futureuse core::…→ E0659, needs::core::…). Zero such imports today; footgun note only.
Validation
| Check | Result |
|---|---|
cargo fmt --check |
Pass |
cargo clippy --all-targets |
Pass, 0 findings |
cargo test |
402 + 2 doctests, 0 failed (matches claim) |
| CI lint/test(ubuntu+windows)/deny | All SUCCESS |
| Rename history | Verified per-commit: 30/31 at 100% similarity (31st = include_bytes! path fix); regrouping commit's renames track at default threshold due to use-path rewrites |
The e2e module doc still described the suite as resolve-only and named
the removed `run_{resolve,commit}_workflow_impl` fns; rewrite it for the
deps-struct entries (`default_run`/`commit_run` via `RunDeps`,
`resolve_run` via `ResolveDeps`) and the full suite scope, including
the scripted confirmation-menu/editor stubs the old header omitted.
Also record in `core/mod.rs` that `pub mod core` shadows the built-in
`core` crate on unqualified `use core::…` paths (local child silently
wins on name collision; `::core::…` escapes), verified empirically —
today's children collide with nothing, so this is a note, not a rename.
Owner
Author
|
Review follow-up — findings resolved (71cec77 + body edit)
Gates after 71cec77: |
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.
Adopts the ADR-0015 layout policy (merges the previously planned PR1/2/3 into one, per review decision; layout style revised to
foo/mod.rsdirectories per follow-up review).What
src/lib.rsis now the module root (31pub mod+#[cfg(test)] mod e2e);main.rsis a thin dispatch. e2e stays in-crate — it readscfg(test)pub(crate)helpers (crate::git::tests,crate::conflict::tests), sotests/would force them public.foo/mod.rs):src/holds onlylib.rs,main.rs, and module directories — product code, children, and tests live together inside each module's directory. 31 pure renames (100% rename detection; oneinclude_bytes!path inupdate/gained a../level).setupsplit:setup/mod.rs(wizard/menu state machine) +setup/{provider,cli_flow,verify,finalize,tests}.rs.gitsplit (ADR-0016, supersedes ADR-0006):git/mod.rskeeps the coupled core ADR-0006 defended — handle,run_git,index()refresh,add,stage_hunks,commit, commit guards — plusgit/{status,diff_view,tests}.rs. One module, several files; no seam, no trait, no path change for anycrate::git::*user.foo/tests.rs(config, cli_agent, llm, display, markdown, progress, decoder, setup, git).research-src-layout.md(anti-regrouping evidence stands; it never measured splits), CONTEXT.md path refs citesrc/foo/.Behavior changes riding in (the architecture series below)
This PR is titled a layout refactor, but the riding architecture commits include one user-visible behavior change — called out so bisects and release notes find it here:
validate_batch_planrejects the model's plan, the Run now warns ("LLM batch plan invalid (…); regrouping deterministically") and regroups via the deterministic grouping engine (grouping::plan_from_diffs— a valid partition by construction, defensively re-validated) instead of erroring out. Everything else in the series is behavior-preserving seam/module extraction.Acceptance (from the design review)
cli/untouched by content) — except the fallback-regroup behavior change above.cargo test: 402 passed + 2 doctests, 0 failed;cargo fmt --checkclean; zero warnings.blame -C).main.rsnow recompiles only the bin.crate::paths unchanged; crate public face unchanged exceptresolve_workflow/default_workflowwidened topub(required by the lib/bin split, approved in review).Note
This branch sits atop the in-flight architecture series (run/resolve/parse/markdown/grouping …) rebased onto main (#137); the restructure depends on those modules existing, so they ride in this PR.