KofTwentyTwo/license-tool
2
0 Fork 0
Code Issues 2 Pull Requests 9 Actions Packages Projects Releases 6 Wiki Activity

apply commit can include unrelated dirty working-tree changes #13

New Issue
Closed
opened 2026-06-04 16:19:16 +00:00 by KofTwentyTwo · 2 comments
KofTwentyTwo commented 2026-06-04 16:19:16 +00:00 (Migrated from github.com)
Copy link
Copy source

Finding

When dirty-tree writes are allowed and commit mode is requested, apply stages every repository change rather than only the files touched by the license operation.

Evidence

  • internal/applier/applier.go:144 to internal/applier/applier.go:151 commits after the write pass when commit mode is enabled.
  • internal/applier/applier.go:281 to internal/applier/applier.go:295 permits a dirty tree when the allow-dirty option is set.
  • internal/gitutil/gitutil.go:95 to internal/gitutil/gitutil.go:100 runs git add -A, which stages every change under the repo.

Reproduction

  1. Create a clean git repository with a configured source file.
  2. Add an unrelated working-tree file or edit an unrelated tracked file.
  3. Run license-tool apply <repo> with write, allow-dirty, and commit mode enabled.
  4. Inspect the resulting commit.
  5. The unrelated work appears in the license-standardization commit.

Why It Matters

A bulk license operation can capture unrelated work-in-progress. That violates user expectations for an atomic license commit and can leak unrelated changes into release or review history.

Suggested Fix

Track the exact paths changed by the apply run and stage only those paths. Alternatively, reject commit mode when the tree was dirty before the operation.

## Finding When dirty-tree writes are allowed and commit mode is requested, `apply` stages every repository change rather than only the files touched by the license operation. ## Evidence - `internal/applier/applier.go:144` to `internal/applier/applier.go:151` commits after the write pass when commit mode is enabled. - `internal/applier/applier.go:281` to `internal/applier/applier.go:295` permits a dirty tree when the allow-dirty option is set. - `internal/gitutil/gitutil.go:95` to `internal/gitutil/gitutil.go:100` runs `git add -A`, which stages every change under the repo. ## Reproduction 1. Create a clean git repository with a configured source file. 2. Add an unrelated working-tree file or edit an unrelated tracked file. 3. Run `license-tool apply <repo>` with write, allow-dirty, and commit mode enabled. 4. Inspect the resulting commit. 5. The unrelated work appears in the license-standardization commit. ## Why It Matters A bulk license operation can capture unrelated work-in-progress. That violates user expectations for an atomic license commit and can leak unrelated changes into release or review history. ## Suggested Fix Track the exact paths changed by the apply run and stage only those paths. Alternatively, reject commit mode when the tree was dirty before the operation.
KofTwentyTwo commented 2026-06-04 16:29:15 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — apply --write --allow-dirty --commit makes a single commit via git add -A, which stages the entire working tree, not just the files the license run touched.

Severity: high (matches an above-typical "bug" label). It is data-correctness/safety, not just cosmetic: unrelated WIP and untracked secrets/drafts get swept into a commit the user believes is scoped to license headers, polluting review/release history. Not "critical" only because it requires the opt-in combination --write --allow-dirty --commit (the dirty-tree gate is off by default).

Verified root cause: internal/gitutil/gitutil.go:Commit:96 runs git add -A, staging every change under the repo before committing at line 99. It is reachable because internal/applier/applier.go:Apply:144-151 calls gitutil.Commit(path, msg) whenever opts.Write && opts.Commit, and the dirty-tree guard internal/applier/applier.go:gateWrite:285-287 short-circuits (return nil) when AllowDirty is set, so a pre-existing dirty tree survives into the commit. CLI wiring confirms --allow-dirty (cmd/license-tool/commands.go:249) and --commit (:251) are independently combinable and passed straight through (:193-196).

Reproduction (confirmed against a build of HEAD): clean repo with main.go + tracked unrelated.txt; dirty the tree with an unrelated edit to unrelated.txt plus an untracked scratch.txt; run apply --write --allow-dirty --commit. The resulting chore: standardize license headers to MIT commit contained unrelated.txt (the WIP edit), scratch.txt (untracked), and .license-tool.yaml — only main.go was the tool's actual change.

Suggested fix: Track the exact paths the run wrote and stage only those, mirroring how the v0.2.1 universal-shebang fix narrowed behavior to exactly the correct surface. The data is already in hand: Apply (applier.go:120-134) knows every file it wrote — those with action != "none"/"skip"/"" and no Err — and model.FileResult.Path holds the scan-root-relative path; the managed LICENSE / LICENSES/<id>.txt entries from ManageLicenseFiles likewise carry their Path. Add a gitutil.CommitPaths(path, message string, paths []string) (or change Commit's signature) that runs git add -- <paths...> instead of git add -A, and have Apply pass the collected written paths. Unlike the shebang precedent there is no filetype table to touch here; the two surfaces that must stay in sync are (1) gitutil.Commit's staging step and (2) applier.Apply's commit call site that must now supply the path list (and include managed-license-file paths so license --commit-style flows stay complete). Keep the 100% gate green: internal/gitutil/gitutil_test.go:TestCommit and internal/applier/applier_test.go (TestApplyWriteWithLicenseAndCommit, TestApplyWriteCustomCommitMessage, TestApplyCommitErrorWithNothingToCommit) must be updated, and a new test should assert that an unrelated dirty/untracked file is NOT in the commit. Note the empty-path edge: if the run wrote nothing, git add -- with no paths must not degenerate into staging everything — guard it and preserve the existing "nothing to commit" error path.

Corrections to the issue: Line references are accurate. Two refinements: (1) the body cites applier.go:281-295 for the allow-dirty permit; the precise short-circuit is gateWrite:285-287 (the 285-301 function spans further). (2) The body frames impact as "unrelated tracked changes"; it is actually broader — untracked files are also captured by git add -A (reproduced above), and .license-tool.yaml itself if uncommitted. Worth stating explicitly.

Open questions: (a) When commit mode runs but the run wrote zero files, should commit be a no-op (silent), an error, or unchanged from today's "nothing to commit" git error? (b) Should the alternative "reject --commit when the tree was dirty before the operation" be offered as a flag instead of / in addition to scoped staging — i.e. is partial staging of a dirty tree acceptable, or should --commit + dirty be refused outright? (c) Should managed LICENSE/LICENSES/ files always be included in the scoped add even when only those changed?

Overlaps: none identified from this issue's surface. A maintainer should confirm against any open issue touching gitutil.Commit or apply-commit semantics.

Automated maintainer review.

**Status**: confirmed — `apply --write --allow-dirty --commit` makes a single commit via `git add -A`, which stages the entire working tree, not just the files the license run touched. **Severity**: high (matches an above-typical "bug" label). It is data-correctness/safety, not just cosmetic: unrelated WIP and untracked secrets/drafts get swept into a commit the user believes is scoped to license headers, polluting review/release history. Not "critical" only because it requires the opt-in combination `--write --allow-dirty --commit` (the dirty-tree gate is off by default). **Verified root cause**: `internal/gitutil/gitutil.go:Commit:96` runs `git add -A`, staging every change under the repo before committing at line 99. It is reachable because `internal/applier/applier.go:Apply:144-151` calls `gitutil.Commit(path, msg)` whenever `opts.Write && opts.Commit`, and the dirty-tree guard `internal/applier/applier.go:gateWrite:285-287` short-circuits (`return nil`) when `AllowDirty` is set, so a pre-existing dirty tree survives into the commit. CLI wiring confirms `--allow-dirty` (`cmd/license-tool/commands.go:249`) and `--commit` (`:251`) are independently combinable and passed straight through (`:193-196`). **Reproduction (confirmed against a build of HEAD)**: clean repo with `main.go` + tracked `unrelated.txt`; dirty the tree with an unrelated edit to `unrelated.txt` plus an untracked `scratch.txt`; run `apply --write --allow-dirty --commit`. The resulting `chore: standardize license headers to MIT` commit contained `unrelated.txt` (the WIP edit), `scratch.txt` (untracked), and `.license-tool.yaml` — only `main.go` was the tool's actual change. **Suggested fix**: Track the exact paths the run wrote and stage only those, mirroring how the v0.2.1 universal-shebang fix narrowed behavior to exactly the correct surface. The data is already in hand: `Apply` (`applier.go:120-134`) knows every file it wrote — those with `action != "none"/"skip"/""` and no `Err` — and `model.FileResult.Path` holds the scan-root-relative path; the managed LICENSE / `LICENSES/<id>.txt` entries from `ManageLicenseFiles` likewise carry their `Path`. Add a `gitutil.CommitPaths(path, message string, paths []string)` (or change `Commit`'s signature) that runs `git add -- <paths...>` instead of `git add -A`, and have `Apply` pass the collected written paths. Unlike the shebang precedent there is no filetype table to touch here; the two surfaces that must stay in sync are (1) `gitutil.Commit`'s staging step and (2) `applier.Apply`'s commit call site that must now supply the path list (and include managed-license-file paths so `license --commit`-style flows stay complete). Keep the 100% gate green: `internal/gitutil/gitutil_test.go:TestCommit` and `internal/applier/applier_test.go` (`TestApplyWriteWithLicenseAndCommit`, `TestApplyWriteCustomCommitMessage`, `TestApplyCommitErrorWithNothingToCommit`) must be updated, and a new test should assert that an unrelated dirty/untracked file is NOT in the commit. Note the empty-path edge: if the run wrote nothing, `git add --` with no paths must not degenerate into staging everything — guard it and preserve the existing "nothing to commit" error path. **Corrections to the issue**: Line references are accurate. Two refinements: (1) the body cites `applier.go:281-295` for the allow-dirty permit; the precise short-circuit is `gateWrite:285-287` (the `285-301` function spans further). (2) The body frames impact as "unrelated tracked changes"; it is actually broader — untracked files are also captured by `git add -A` (reproduced above), and `.license-tool.yaml` itself if uncommitted. Worth stating explicitly. **Open questions**: (a) When commit mode runs but the run wrote zero files, should commit be a no-op (silent), an error, or unchanged from today's "nothing to commit" git error? (b) Should the alternative "reject `--commit` when the tree was dirty before the operation" be offered as a flag instead of / in addition to scoped staging — i.e. is partial staging of a dirty tree acceptable, or should `--commit` + dirty be refused outright? (c) Should managed LICENSE/`LICENSES/` files always be included in the scoped add even when only those changed? **Overlaps**: none identified from this issue's surface. A maintainer should confirm against any open issue touching `gitutil.Commit` or apply-commit semantics. _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:07 +00:00 (Migrated from github.com)
Copy link
Copy source

Resolved on develop in commit 742c124e6ec2afa9312075db3112bc2def3c91ff.

Validation:

Closing as fixed.

Resolved on `develop` in commit `742c124e6ec2afa9312075db3112bc2def3c91ff`. Validation: - CI: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977377097 - CodeQL: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977376421 - Coverage gate: 100% (2087/2087) Closing as fixed.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
bug
Something isn't working
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 go
Pull requests that update go code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#13