diff --git a/README.md b/README.md index 0454be2..53a5ef2 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,7 @@ Returns exit code 0 if the commit is atomic, 1 if violations are found. | `--dependants` | Include direct dependants when using `--committable` | | `--commit-msg ` | Generate commit message using LLM agent (claude, codex, mistral, opencode) | | `--prompt-file ` | Custom prompt file for `--commit-msg` (default: built-in Conventional Commits prompt) | +| `--single-changeset` | Fail if staged files contain multiple independent changesets that could be separate atomic commits | ### Progressive commit workflow @@ -170,6 +171,35 @@ Files are sorted **lexicographically** by path. The first file that is independe Transitive dependants (dependants of dependants) are excluded to maintain atomicity. +### Single-changeset detection + +`--single-changeset` runs *after* the atomicity check succeeds. It partitions the staged `.go` files into connected components based on direct symbol dependencies (in either direction) between staged files, and fails with exit code 1 when more than one component exists. + +Atomicity guarantees a commit is *complete* — every symbol it references is available. `--single-changeset` guarantees a commit is *cohesive* — it describes one logical change rather than several. A commit can be atomic without being cohesive: `{alpha.go, beta.go}` and `{service.go, types.go}` may each be self-contained, but bundling them into one commit forces reviewers to mentally split the diff. + +Example failure output: + +``` +Commit is atomic but contains 2 independent changesets. Split into separate commits: + + Changeset 1: + alpha.go + beta.go + + Changeset 2: + service.go + types.go + +To fix, run: + git reset + git add alpha.go beta.go + git commit -m "..." + git add service.go types.go + git commit -m "..." +``` + +See [ADR-003: Single-changeset detection for cohesive commits](docs/decisions/003.md) for the algorithm and rationale. + ### Git pre-commit hook ```bash @@ -184,7 +214,7 @@ Darna exits non-zero on violations, blocking the commit. 1. **Git status** - categorize files as staged, unstaged, or untracked using `git status --porcelain -z`. 2. **Package loading** - load all Go packages with full type information via `golang.org/x/tools/go/packages`. For partially staged files (status `MM`), an overlay with `git show :path` content ensures analysis reflects what will actually be committed. 3. **Dependency graph** - walk `types.Info.Defs` and `types.Info.Uses` to build a bidirectional symbol dependency graph with transitive reachability. -4. **Violation detection** - for each symbol in a staged file, check whether its dependencies are all satisfied by staged or committed files. Report any that require unstaged files. +4. **Violation detection** - for each symbol in a staged file, check whether its dependencies are all satisfied by staged or committed files. Report any that require unstaged files. When `--single-changeset` is set, an additional pass builds an undirected graph over staged files (edge = direct symbol dependency between them) and reports connected components — failing when more than one exists. 5. **Commit message generation (optional)** - when `--commit-msg` is used, invoke the specified LLM agent with `git diff --cached` content and the prompt (default or custom via `--prompt-file`), extract the first line of output, and return as the commit message. ## Project structure diff --git a/docs/decisions/003.md b/docs/decisions/003.md new file mode 100644 index 0000000..5781db5 --- /dev/null +++ b/docs/decisions/003.md @@ -0,0 +1,112 @@ +# Single-changeset detection for cohesive commits + +Darna's atomicity check (ADR-000) guarantees that a commit is *complete* — every symbol it references is defined either in the same commit or in a previous one. But completeness is not the same as *cohesion*. A commit can pass the atomicity check while still bundling two logically unrelated changes: e.g. `{alpha.go, beta.go}` refactors one subsystem and `{service.go, types.go}` refactors another, and the two sets have no symbol dependency between them. **This ADR introduces an opt-in `--single-changeset` flag that fails when the staged files form more than one independent changeset**, catching the "atomic but not cohesive" case that reviewers flag by hand today. + +## Why cohesion needs its own check + +### Atomicity ≠ cohesion + +The two properties are orthogonal: + +| Property | Question it answers | Enforced by | +|----------|--------------------|-------------| +| **Atomicity** | Can this commit be reviewed and reverted in isolation without breaking the build? | Default `darna` (ADR-000) | +| **Cohesion** | Does this commit describe *one* change, not several? | `--single-changeset` (this ADR) | + +A commit that stages two unrelated refactors is atomic (nothing is missing) but not cohesive (two things are mixed). Reviewers must mentally split it into "the alpha/beta part" and "the service/types part" — precisely the work that atomic commits are supposed to avoid. + +### Reviewer effort is the cost + +Every mixed-concern commit forces reviewers to reconstruct the split themselves. That cost is invisible to the author, who saw both changes as they were made. `--single-changeset` shifts detection to commit time, when splitting is cheap (`git reset` + two `git add`s). + +## Architectural decisions + +| Aspect | Decision | Rationale | +|--------|----------|-----------| +| **Opt-in flag** | `--single-changeset` off by default | Non-breaking for existing hooks and workflows; matches every other darna mode (`--committable`, `--dependants`, `--commit-msg`) | +| **Runs after atomicity** | Skipped when the atomicity check reports violations | Two failure modes wouldn't make sense together — fix atomicity first, then check cohesion | +| **Graph unit** | File-level undirected graph | The commit unit is the file; a symbol-level graph would over-report (methods calling each other would look like separate concerns) | +| **Edge criterion** | Direct `OutEdges` between staged symbols | BFS already provides transitivity across a chain `A→B→C`; using `TransitiveDeps` would be O(V·E) redundancy | +| **Component discovery** | BFS with deterministic start-node order | Matches ADR-001's lexicographic-sort precedent | +| **Return shape** | `[][]string` — one relative-path slice per component | Lets the caller print, sort, or serialize as needed; the API can't be shoehorned into `ValidateAtomicCommit`'s `[]Violation` shape | + +## Algorithm + +``` +1. Run the standard atomicity pipeline (git status → categorize → overlay → + load packages → build symbol graph). +2. Build adj: map[stagedFile]set[stagedFile]. For each staged file F, for each + symbol S in F, for each direct OutEdge S→D whose target file G is also + staged and G != F: add the undirected edge F—G. +3. Ensure every staged file is present in adj (isolated files matter — they + are their own changeset). +4. BFS connected components, iterating start files in the stagedGo order for + deterministic component discovery. +5. Sort each component lexicographically; sort components by first element. +6. Convert to relative paths. +``` + +The connectivity relation is undirected because commit cohesion is symmetric: if `beta.go` calls `AlphaFunc` from `alpha.go`, both files belong to the same logical change regardless of who calls whom. + +## Alternatives considered + +### Default behavior instead of opt-in + +Making `--single-changeset` the default would enforce cohesion for every commit — a stronger guarantee. Rejected because: + +- It's a breaking change for every pre-commit hook that already installed darna. +- The mixed-concern case is a *code review* concern, not a *build correctness* one — some users legitimately don't want it enforced automatically. +- Every other mode in darna is opt-in; consistency wins. + +### Use `TransitiveDeps` for connectivity + +Building edges from transitive dependencies would guarantee that any two staged files linked through *any* chain end up in the same component. Rejected because: + +- BFS over direct edges already provides transitivity: `A→B` and `B→C` become the component `{A, B, C}` via the two edges. +- `TransitiveDeps` is O(V·E) per starting symbol; direct edges are O(E) total. +- The only difference would be transitivity through *unstaged* code, which is exactly what atomicity was supposed to prevent — if we're here, that case is already impossible. + +### Symbol-level components instead of file-level + +Grouping symbols instead of files would find intra-file splits (e.g. `consumer.go` has `ConsumeConstant` in one changeset and `ConsumeVariable` in another). Rejected because: + +- The commit unit is the file — git can't stage half a function anyway. +- Every non-trivial file would look like multiple changesets because unrelated methods rarely reference each other. +- False positives would drown out the true "two unrelated files" case this flag is designed to catch. + +### Bool return on `ValidateAtomicCommit` + +Adding a `single bool` parameter to the existing function and returning an extra flag. Rejected because: + +- The `[]Violation` return type can't represent a *list of file groups*. +- Callers who don't ask for changeset detection shouldn't pay the extra graph traversal. +- A separate `DetectChangesets` function keeps the two concerns independent and testable in isolation. + +## Consequences + +**Positive:** + +- Catches commits that would otherwise silently bundle unrelated concerns. +- Composes with `--commit-msg`: a cohesive commit produces a cleaner, more focused generated message. +- Pre-commit hooks can layer it selectively on projects where cohesion matters, without forcing it project-wide. + +**Negative:** + +- Runs the analysis pipeline twice (once for atomicity, once for changesets) when the flag is set. Acceptable for hook latency on typical repos; a shared `loadAnalysis` helper is a future optimization. +- Only inspects `.go` files. Non-Go files staged alongside are ignored (same limitation as the atomicity check). + +**Neutral:** + +- Off by default — users who don't opt in see no behavior change. + +## Future enhancements + +- **Shared analysis pipeline**: extract a `loadAnalysis` helper so `ValidateAtomicCommit`, `FindCommittableSet`, and `DetectChangesets` share one git-status/overlay/package-load pass. Right now each call runs its own; deferred to keep this change's blast radius small. +- **MCP tool**: expose `detect_changesets` alongside the tools in ADR-004 so agents can surface the split proactively rather than reacting to a hook failure. +- **Integration with `--committable`**: suggest commit order when a mixed-concern commit is detected — e.g. "commit changeset 1 first because changeset 2 depends on it." + +## References + +- ADR-000: Atomic commit validation — the foundation this check layers on. +- ADR-001: `--committable` mode and lexicographic-sort precedent for deterministic ordering. +- ADR-004: MCP tool exposure — the future path for surfacing changeset detection to agents.