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

apply dry run omits the promised unified diff #15

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

Finding

Dry-run apply computes per-file diffs, but the command output does not show the unified diff that the README and security policy describe as the primary review step before writes.

Evidence

  • README.md:72 to README.md:80 describes dry-run apply as printing a unified diff.
  • SECURITY.md:39 to SECURITY.md:42 repeats that dry-run prints a unified diff and write mode is explicit.
  • internal/applier/applier.go:120 to internal/applier/applier.go:127 stores the computed diff on FileResult.
  • internal/report/report.go:412 onward renders the text report summary, but does not emit the stored diffs.
  • cmd/license-tool/commands.go:232 to cmd/license-tool/commands.go:234 makes license print only path and action.

Reproduction

  1. Create a clean git repo with a headerless supported source file.
  2. Run license-tool apply <repo> without write mode.
  3. Observe that stdout contains an audit-style summary and no unified diff hunk.

Why It Matters

Dry-run output is the operator's chance to review exactly what will be rewritten. Without diffs, users must either trust a summary or run write mode blind.

Suggested Fix

Render FileResult.Diff for dry-run apply and license operations. Add command tests that assert unified diff headers and header content appear in dry-run output when a change is pending.

## Finding Dry-run apply computes per-file diffs, but the command output does not show the unified diff that the README and security policy describe as the primary review step before writes. ## Evidence - `README.md:72` to `README.md:80` describes dry-run apply as printing a unified diff. - `SECURITY.md:39` to `SECURITY.md:42` repeats that dry-run prints a unified diff and write mode is explicit. - `internal/applier/applier.go:120` to `internal/applier/applier.go:127` stores the computed diff on `FileResult`. - `internal/report/report.go:412` onward renders the text report summary, but does not emit the stored diffs. - `cmd/license-tool/commands.go:232` to `cmd/license-tool/commands.go:234` makes `license` print only path and action. ## Reproduction 1. Create a clean git repo with a headerless supported source file. 2. Run `license-tool apply <repo>` without write mode. 3. Observe that stdout contains an audit-style summary and no unified diff hunk. ## Why It Matters Dry-run output is the operator's chance to review exactly what will be rewritten. Without diffs, users must either trust a summary or run write mode blind. ## Suggested Fix Render `FileResult.Diff` for dry-run apply and license operations. Add command tests that assert unified diff headers and header content appear in dry-run output when a change is pending.
KofTwentyTwo commented 2026-06-04 16:29:52 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — dry-run apply/license compute and store a unified diff on FileResult.Diff, but no output format ever renders it; stdout is the audit summary only.

Severity: medium (matches the bug label; not high because it is a documentation/observability gap, not a write-safety or correctness defect — --write still behaves correctly, and git diff after a write is the real undo/review path). The promise in SECURITY.md under "Safe By Default" does inflate the impact slightly, since the diff is the advertised pre-write review surface.

Verified root cause (two render sites, both omit the stored diff):

  • internal/report/report.go:renderText:457-460 — the source files: block prints only fileLine(fr) (path + bracketed detected-license note). fileLine (report.go:480-491) never references fr.Diff. A repo-wide grep finds zero .Diff references anywhere in internal/report/ or cmd/.
  • cmd/license-tool/commands.go:newLicenseCmd:232-234 — the license command prints only "%s: %s\n" (path + action), discarding fr.Diff.
  • The data is present: applier.go:ApplyFile:174 computes diff = unifiedDiff(...), Apply:127 sets fr.Diff = diff, and report.Build:106 preserves it via append([]model.FileResult(nil), files...). So the diff reaches the model.Report and is simply never emitted by any renderer.
  • The JSON renderer drops it too: jsonFile (report.go struct) has no Diff/diff field, so even --format json cannot surface it.

Reproduction (built from HEAD, go build ./cmd/license-tool): a clean git repo with a headerless main.go + .license-tool.yaml (MIT). license-tool apply (no --write) prints the audit report and main.go [no managed header] — no ---/+++/@@ hunk and no SPDX-License-Identifier header text. Confirmed.

Suggested fix:

  • Render fr.Diff for the dry-run path in renderText (gate on !opts.Write && fr.Diff != ""), and in newLicenseCmd's result loop. The report must know it is a dry run — model.Report does not currently carry a Write/DryRun flag, so either thread one through report.Build or have the command decide whether to print diffs (the command already knows f.write). Threading a flag onto the report is cleaner and keeps license and apply consistent.
  • Add a diff field to jsonFile (omitempty) so JSON consumers get parity.
  • This is a render-layer fix; it does not touch the preserve-first boundaries. The v0.2.1 universal-shebang precedent ("touch both render.preserveBoundary and detect.PreserveBoundary + the filetype table") does not apply here — that pattern is for boundary/splice behavior, and this defect is purely about emitting already-correct data. Do not let the issue's framing pull a fixer into the boundary code.
  • Keep the 100% gate green: add command/report tests asserting unified-diff headers (---/+++/@@) and at least one line of header content appear in dry-run output, and assert they are absent under --write (so the new branch's both sides are covered).

Corrections to the issue:

  • Line refs are accurate: README.md:72 and README.md:75 (not 80) carry the "prints a unified diff" promise; SECURITY.md:39 carries it. applier.go:120-127 (the fr.Diff = diff assignment is at line 127) and commands.go:232-234 are correct.
  • Minor: commands.go:233 prints "%s: %s" (path-colon-action), not "path and action" as separate columns — cosmetic, not load-bearing.
  • The impact is slightly broader than the body implies in one respect: --format json also omits the diff (no Diff field on jsonFile), so the gap is not text-renderer-only.

Open questions:

  • Scope: should the diff print in all formats (text + markdown + json) or text-only? JSON parity argues for adding the field everywhere.
  • Should diffs print on every dry-run by default (potentially large output across a whole repo), or behind a --diff/--no-diff flag with a sensible default? This is a UX/behavior decision the README's blanket "prints a unified diff" doesn't settle for large trees.
  • Should license and apply share the exact same diff-rendering path, or does license (whole-file LICENSE/LICENSES rewrites) warrant different treatment?

Overlaps: none found in the cited code; this is an output-rendering gap independent of the v0.2.1 shebang fix (#5).

Automated maintainer review.

**Status**: confirmed — dry-run `apply`/`license` compute and store a unified diff on `FileResult.Diff`, but no output format ever renders it; stdout is the audit summary only. **Severity**: medium (matches the `bug` label; not high because it is a documentation/observability gap, not a write-safety or correctness defect — `--write` still behaves correctly, and `git diff` after a write is the real undo/review path). The promise in `SECURITY.md` under "Safe By Default" does inflate the impact slightly, since the diff is the advertised pre-write review surface. **Verified root cause** (two render sites, both omit the stored diff): - `internal/report/report.go:renderText:457-460` — the `source files:` block prints only `fileLine(fr)` (path + bracketed detected-license note). `fileLine` (`report.go:480-491`) never references `fr.Diff`. A repo-wide grep finds **zero** `.Diff` references anywhere in `internal/report/` or `cmd/`. - `cmd/license-tool/commands.go:newLicenseCmd:232-234` — the `license` command prints only `"%s: %s\n"` (path + action), discarding `fr.Diff`. - The data is present: `applier.go:ApplyFile:174` computes `diff = unifiedDiff(...)`, `Apply:127` sets `fr.Diff = diff`, and `report.Build:106` preserves it via `append([]model.FileResult(nil), files...)`. So the diff reaches the `model.Report` and is simply never emitted by any renderer. - The JSON renderer drops it too: `jsonFile` (`report.go` struct) has no `Diff`/`diff` field, so even `--format json` cannot surface it. **Reproduction** (built from HEAD, `go build ./cmd/license-tool`): a clean git repo with a headerless `main.go` + `.license-tool.yaml` (MIT). `license-tool apply` (no `--write`) prints the audit report and `main.go [no managed header]` — no `---`/`+++`/`@@` hunk and no `SPDX-License-Identifier` header text. Confirmed. **Suggested fix**: - Render `fr.Diff` for the dry-run path in `renderText` (gate on `!opts.Write && fr.Diff != ""`), and in `newLicenseCmd`'s result loop. The report must know it is a dry run — `model.Report` does not currently carry a `Write`/`DryRun` flag, so either thread one through `report.Build` or have the command decide whether to print diffs (the command already knows `f.write`). Threading a flag onto the report is cleaner and keeps `license` and `apply` consistent. - Add a `diff` field to `jsonFile` (omitempty) so JSON consumers get parity. - This is a render-layer fix; it does **not** touch the preserve-first boundaries. The v0.2.1 universal-shebang precedent ("touch both `render.preserveBoundary` and `detect.PreserveBoundary` + the filetype table") does **not** apply here — that pattern is for boundary/splice behavior, and this defect is purely about emitting already-correct data. Do not let the issue's framing pull a fixer into the boundary code. - Keep the 100% gate green: add command/report tests asserting unified-diff headers (`---`/`+++`/`@@`) and at least one line of header content appear in dry-run output, and assert they are **absent** under `--write` (so the new branch's both sides are covered). **Corrections to the issue**: - Line refs are accurate: `README.md:72` and `README.md:75` (not 80) carry the "prints a unified diff" promise; `SECURITY.md:39` carries it. `applier.go:120-127` (the `fr.Diff = diff` assignment is at line 127) and `commands.go:232-234` are correct. - Minor: `commands.go:233` prints `"%s: %s"` (path-colon-action), not "path and action" as separate columns — cosmetic, not load-bearing. - The impact is slightly **broader** than the body implies in one respect: `--format json` also omits the diff (no `Diff` field on `jsonFile`), so the gap is not text-renderer-only. **Open questions**: - Scope: should the diff print in **all** formats (text + markdown + json) or text-only? JSON parity argues for adding the field everywhere. - Should diffs print on every dry-run by default (potentially large output across a whole repo), or behind a `--diff`/`--no-diff` flag with a sensible default? This is a UX/behavior decision the README's blanket "prints a unified diff" doesn't settle for large trees. - Should `license` and `apply` share the exact same diff-rendering path, or does `license` (whole-file LICENSE/LICENSES rewrites) warrant different treatment? **Overlaps**: none found in the cited code; this is an output-rendering gap independent of the v0.2.1 shebang fix (#5). _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:10 +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#15