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

feat: add file-type coverage — Perl, PowerShell, R, Makefile, TOML, Batch #9

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

Summary

The builtin file-type table (internal/filetype/filetype.go, the builtin slice starting at line 50) ships comment-syntax + preserve-first rules for ~19 language groups. Several common, widely-used file types are missing, so license-tool cannot apply (or detect) a license header for them and silently treats them as "unknown type" (see Lookup returning false at internal/filetype/filetype.go:209-222).

Add builtin coverage for:

Language Match Comment style Notes
Perl .pl, .pm, .t line # shebang common (#!/usr/bin/perl)
PowerShell .ps1, .psm1 line #
R .r, .R line # shebang via Rscript possible
Makefile filenames Makefile, GNUmakefile + ext .mk line #
TOML .toml line #
Windows Batch .bat, .cmd line REM NOT # or // — see Root cause

The critical correctness trap here is Windows Batch: it does not use # or //. Its line-comment token is REM (an alternative :: label-comment exists but is more fragile inside parenthesized blocks, so REM is the safe choice). Emitting # ... or // ... into a .bat/.cmd file produces a syntax error that breaks the script — the exact class of "the tool corrupted my file" bug the preserve-first machinery exists to prevent.

Type / Severity

  • Type: Enhancement (feature: file-type coverage).
  • Severity: Low-to-medium. No regression for existing types. Missing types are currently inert (reported as unknown, never edited), so there is no data-loss risk today — but shipping Batch with the wrong comment token would introduce a file-corruption bug, so the Batch entry's comment style must be correct on the first commit.

Reproduction

Current behavior — an unknown type is not recognized, so no header is applied. From the repo root:

cd /path/to/license-tool

# A Perl module — should get a "# " header, currently unknown.
printf '#!/usr/bin/perl\npackage Foo;\n1;\n' > /tmp/Foo.pm

# A Windows batch file — the dangerous case.
printf '@echo off\necho hello\n' > /tmp/run.bat

A focused Go test demonstrates the gap directly (this is the cheapest reproduction and mirrors the existing TestLookupTable in internal/filetype/filetype_test.go):

ft, ok := filetype.Lookup("Foo.pm")   // ok == false today  (want: true, Name "Perl")
ft, ok = filetype.Lookup("run.bat")   // ok == false today  (want: true, Name "Batch", LinePrefix "REM ")
ft, ok = filetype.Lookup("Makefile")  // ok == false today  (want: true, Name "Makefile")
ft, ok = filetype.Lookup("cfg.toml")  // ok == false today  (want: true, Name "TOML")

Lookup matches an exact base name first via nameIndex, then the lowercased extension via extIndex (internal/filetype/filetype.go:209-222), so Makefile/GNUmakefile must be registered as Filenames, while .mk is an Extensions entry — both routes for the same FileType, exactly like the existing Dockerfile entry at lines 138-144.

Root cause

internal/filetype/filetype.go, var builtin []model.FileType (lines 50-187), is the single source of recognized types. It has no entries for .pl/.pm/.t, .ps1/.psm1, .r/.R, Makefile/GNUmakefile/.mk, .toml, or .bat/.cmd. Verified absent: a grep for those extensions / REM / perl / powershell / makefile against filetype.go returns nothing.

Consequences:

  • Lookup(path) falls through both nameIndex and extIndex and returns model.FileType{}, false (lines 215-221). Downstream apply/detect treat the file as unknown and skip it.
  • For Windows Batch specifically, there is an additional hazard if this is added carelessly: the table's hash-comment cluster (Python/Shell/Ruby/YAML/Nix/HCL/Dockerfile, lines 102-144) all use line("# "), and the C-family cluster uses block("/*","*/"). Copy-pasting either into a Batch entry yields an invalid comment delimiter for cmd.exe. Batch comments are REM (line comment) or :: (label-comment). REM is the robust, position-independent choice.

The two preserve-first boundary implementations already handle the "must-be-first" constructs these new types need, so no boundary-engine change is required:

  • Render insert path: preserveBoundary(content, ft) in internal/render/render.go:320-367.
  • Detect path: PreserveBoundary(content, ft) in internal/detect/detect.go:171-219.

Both consume a BOM first (gated on the type listing PreserveBOM), and — per the PR #5 / v0.2.1 fix (commit f4a111f, "preserve shebang universally so headers never precede #!") — both now treat a leading #! as universally preserved, independent of ft.PreserveFirst. See the universal-shebang block at render.go:335-337 and the allowShebang := true branch at detect.go:187,202. That means Perl/R/PowerShell scripts that begin with a shebang are already handled correctly by the boundary code as long as the new FileType entries exist in the table. The only missing piece is the table data.

Proposed fix

Add the following entries to the builtin slice in internal/filetype/filetype.go. Place the hash-comment languages (Perl, PowerShell, R, Makefile, TOML) in the "Hash-comment languages" section (currently lines 101-144, alongside Python/Shell/Ruby/YAML/Nix/HCL/Dockerfile). Give Batch its own short section since its comment token is unique.

Use the existing helpers line(prefix) (line 28), after(kind) (line 33), and before(kind) (line 38). Follow the established PreserveFirst convention: every text type lists after(model.PreserveBOM) first (the "all: bom" rule documented at lines 46-49); add after(model.PreserveShebang) for types whose scripts can carry a shebang. The universal-shebang precedent (PR #5) means the explicit PreserveShebang rule is belt-and-suspenders for documentation/ordering, mirroring how Shell (lines 108-113) and PHP (lines 174-179) already list it.

// Perl: scripts begin with a "#!/usr/bin/perl" shebang; modules (.pm) do not.
{
    Name:          "Perl",
    Extensions:    []string{".pl", ".pm", ".t"},
    CommentStyle:  line("# "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveShebang)},
},
{
    Name:          "PowerShell",
    Extensions:    []string{".ps1", ".psm1"},
    CommentStyle:  line("# "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)},
},
{
    // R: ".r" and ".R" both used; the extIndex lowercases keys (see init, line 198),
    // so listing ".r" is sufficient to match "script.R".
    Name:          "R",
    Extensions:    []string{".r"},
    CommentStyle:  line("# "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveShebang)},
},
{
    // Makefile: exact filenames win over extensions (Lookup checks nameIndex first),
    // mirroring the Dockerfile entry. ".mk" covers included fragments.
    Name:          "Makefile",
    Extensions:    []string{".mk"},
    Filenames:     []string{"Makefile", "GNUmakefile"},
    CommentStyle:  line("# "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)},
},
{
    Name:          "TOML",
    Extensions:    []string{".toml"},
    CommentStyle:  line("# "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)},
},

// Windows Batch: comment token is "REM " (NOT "#" or "//"). A "#" or "//" line
// would be a cmd.exe syntax error and break the script.
{
    Name:          "Batch",
    Extensions:    []string{".bat", ".cmd"},
    CommentStyle:  line("REM "),
    PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)},
},

Notes:

  • R .r vs .R: init() (lines 195-204) lowercases every extension key into extIndex, and Lookup lowercases the queried extension (line 214), so a single ".r" entry matches both foo.r and foo.R. Do not add ".R" separately — duplicate keys would be redundant (last-write-wins in the map) and the table convention is one lowercase key.
  • No collisions: verified that .pl, .pm, .t, .ps1, .psm1, .r, .toml, .bat, .cmd, .mk, and filenames Makefile/GNUmakefile do not appear anywhere else in builtin today, so no existing mapping is displaced. (.t is Perl's test extension; it is unused elsewhere — keep it Perl.)
  • Boundary code: no change needed in internal/render/render.go or internal/detect/detect.go. The universal-shebang handling (PR #5) plus the per-type PreserveBOM/PreserveShebang rules already cover these types once the table entries exist.
  • 100% coverage gate must stay green: .testcoverage.yml enforces total: 100 over ./internal/... and ./cmd/... (the only excluded files are cmd/license-tool/main.go, scripts/gen_spdx.go, cmd/license-tool/wizard.go). Adding pure data rows to builtin introduces no new executable branches in filetype.go itself (the init/Lookup/Merge code paths are unchanged), but the new entries must each be exercised by a table-test assertion so the data is validated; see below.

Tests & acceptance criteria

Extend the existing tests in internal/filetype/filetype_test.go (no new test file needed):

  1. TestLookupTable (lines 11-71): add one case row per new mapping asserting wantName, wantOK: true, wantBlock: false (all six are line-comment types), wantSkip: false. Cover every extension and filename:

    • {"app.pl", "Perl", true, false, false}, {"Mod.pm", "Perl", true, false, false}, {"t/basic.t", "Perl", true, false, false}
    • {"deploy.ps1", "PowerShell", true, false, false}, {"mod.psm1", "PowerShell", true, false, false}
    • {"analysis.r", "R", true, false, false}, {"Analysis.R", "R", true, false, false} (proves the lowercase-key match)
    • {"Makefile", "Makefile", true, false, false}, {"GNUmakefile", "Makefile", true, false, false}, {"build/sub.mk", "Makefile", true, false, false}
    • {"pyproject.toml", "TOML", true, false, false}, {"Cargo.toml", "TOML", true, false, false}
    • {"run.bat", "Batch", true, false, false}, {"task.cmd", "Batch", true, false, false}

  2. Batch comment-style assertion (the load-bearing one): add an explicit check that Lookup("run.bat")'s CommentStyle.LinePrefix == "REM " and CommentStyle.Block == false. This is the regression guard against silently shipping #///. Example:

    bat, ok := Lookup("run.bat")
require.True(t, ok)
assert.False(t, bat.CommentStyle.Block)
assert.Equal(t, "REM ", bat.CommentStyle.LinePrefix)

  3. TestPreserveFirstOrdering (lines 73-113): add an assertion that Perl lists PreserveBOM then PreserveShebang (mirroring the Shell assertion at lines 83-87), confirming a shebang'd Perl script keeps #! on line 1.

  4. Round-trip (optional but recommended): if there is an apply/detect integration test elsewhere, add a Batch fixture proving a header is rendered as REM SPDX-License-Tool: license-tool:managed (via render.Header -> wrapComment line-prefix path, internal/render/render.go:191-201) and then re-detected (internal/detect/detect.go lineCommentRegion, lines 335-380, which derives its prefix from cs.LinePrefix so REM works without code changes), to confirm the new type is end-to-end safe and idempotent.

Acceptance criteria:

  • All six file-type groups resolve via filetype.Lookup with the names and # /REM comment styles above.
  • Batch resolves to LinePrefix: "REM " (asserted by a dedicated test), never # or //.
  • R resolves for both .r and .R.
  • Makefile resolves for the Makefile and GNUmakefile filenames and the .mk extension.
  • No existing TestLookupTable / TestPreserveFirstOrdering / Merge cases regress.
  • go test ./... passes and the go-test-coverage gate stays at 100% total (.testcoverage.yml).

References

  • File-type table to edit: internal/filetype/filetype.go — var builtin (lines 50-187), line/after/before helpers (lines 22-40), init/Lookup (lines 195-222).
  • Existing dual filename+extension precedent (Dockerfile): internal/filetype/filetype.go:138-144.
  • Existing shebang-listing precedents: Shell internal/filetype/filetype.go:108-113, PHP internal/filetype/filetype.go:174-179.
  • PreserveKind constants: internal/model/model.go:149-161.
  • Render insert boundary: internal/render/render.go preserveBoundary (lines 320-367); universal-shebang block (lines 335-337).
  • Detect boundary: internal/detect/detect.go PreserveBoundary (lines 171-219); universal-shebang branch (allowShebang := true, line 187; consume at line 202).
  • Universal-shebang precedent (the "must-be-first, universally preserved" pattern to follow): PR #5 / v0.2.1, commit f4a111f ("fix: preserve shebang universally so headers never precede #! (#5)").
  • Tests to extend: internal/filetype/filetype_test.go TestLookupTable (lines 11-71) and TestPreserveFirstOrdering (lines 73-113).
  • Coverage gate: .testcoverage.yml (threshold.total: 100).
## Summary The builtin file-type table (`internal/filetype/filetype.go`, the `builtin` slice starting at line 50) ships comment-syntax + preserve-first rules for ~19 language groups. Several common, widely-used file types are missing, so `license-tool` cannot apply (or detect) a license header for them and silently treats them as "unknown type" (see `Lookup` returning `false` at `internal/filetype/filetype.go:209-222`). Add builtin coverage for: | Language | Match | Comment style | Notes | |---|---|---|---| | Perl | `.pl`, `.pm`, `.t` | line `# ` | shebang common (`#!/usr/bin/perl`) | | PowerShell | `.ps1`, `.psm1` | line `# ` | | | R | `.r`, `.R` | line `# ` | shebang via `Rscript` possible | | Makefile | filenames `Makefile`, `GNUmakefile` + ext `.mk` | line `# ` | | | TOML | `.toml` | line `# ` | | | Windows Batch | `.bat`, `.cmd` | line `REM ` | **NOT** `#` or `//` — see Root cause | The critical correctness trap here is Windows Batch: it does not use `#` or `//`. Its line-comment token is `REM ` (an alternative `:: ` label-comment exists but is more fragile inside parenthesized blocks, so `REM ` is the safe choice). Emitting `# ...` or `// ...` into a `.bat`/`.cmd` file produces a syntax error that breaks the script — the exact class of "the tool corrupted my file" bug the preserve-first machinery exists to prevent. ## Type / Severity - **Type:** Enhancement (feature: file-type coverage). - **Severity:** Low-to-medium. No regression for existing types. Missing types are currently inert (reported as unknown, never edited), so there is no data-loss risk *today* — but shipping Batch with the wrong comment token would introduce a file-corruption bug, so the Batch entry's comment style must be correct on the first commit. ## Reproduction Current behavior — an unknown type is not recognized, so no header is applied. From the repo root: ```bash cd /path/to/license-tool # A Perl module — should get a "# " header, currently unknown. printf '#!/usr/bin/perl\npackage Foo;\n1;\n' > /tmp/Foo.pm # A Windows batch file — the dangerous case. printf '@echo off\necho hello\n' > /tmp/run.bat ``` A focused Go test demonstrates the gap directly (this is the cheapest reproduction and mirrors the existing `TestLookupTable` in `internal/filetype/filetype_test.go`): ```go ft, ok := filetype.Lookup("Foo.pm") // ok == false today (want: true, Name "Perl") ft, ok = filetype.Lookup("run.bat") // ok == false today (want: true, Name "Batch", LinePrefix "REM ") ft, ok = filetype.Lookup("Makefile") // ok == false today (want: true, Name "Makefile") ft, ok = filetype.Lookup("cfg.toml") // ok == false today (want: true, Name "TOML") ``` `Lookup` matches an exact base name first via `nameIndex`, then the lowercased extension via `extIndex` (`internal/filetype/filetype.go:209-222`), so `Makefile`/`GNUmakefile` must be registered as `Filenames`, while `.mk` is an `Extensions` entry — both routes for the same FileType, exactly like the existing `Dockerfile` entry at lines 138-144. ## Root cause `internal/filetype/filetype.go`, `var builtin []model.FileType` (lines 50-187), is the single source of recognized types. It has no entries for `.pl/.pm/.t`, `.ps1/.psm1`, `.r/.R`, `Makefile`/`GNUmakefile`/`.mk`, `.toml`, or `.bat/.cmd`. Verified absent: a grep for those extensions / `REM` / `perl` / `powershell` / `makefile` against `filetype.go` returns nothing. Consequences: - `Lookup(path)` falls through both `nameIndex` and `extIndex` and returns `model.FileType{}, false` (lines 215-221). Downstream apply/detect treat the file as unknown and skip it. - For Windows Batch specifically, there is an additional hazard if this is added carelessly: the table's hash-comment cluster (Python/Shell/Ruby/YAML/Nix/HCL/Dockerfile, lines 102-144) all use `line("# ")`, and the C-family cluster uses `block("/*","*/")`. Copy-pasting either into a Batch entry yields an **invalid** comment delimiter for `cmd.exe`. Batch comments are `REM ` (line comment) or `::` (label-comment). `REM ` is the robust, position-independent choice. The two preserve-first boundary implementations already handle the "must-be-first" constructs these new types need, so no boundary-engine change is required: - Render insert path: `preserveBoundary(content, ft)` in `internal/render/render.go:320-367`. - Detect path: `PreserveBoundary(content, ft)` in `internal/detect/detect.go:171-219`. Both consume a BOM first (gated on the type listing `PreserveBOM`), and — per the PR #5 / v0.2.1 fix (commit `f4a111f`, "preserve shebang universally so headers never precede #!") — both now treat a leading `#!` as **universally preserved**, independent of `ft.PreserveFirst`. See the universal-shebang block at `render.go:335-337` and the `allowShebang := true` branch at `detect.go:187,202`. That means Perl/R/PowerShell scripts that begin with a shebang are already handled correctly by the boundary code *as long as the new FileType entries exist in the table*. The only missing piece is the table data. ## Proposed fix Add the following entries to the `builtin` slice in `internal/filetype/filetype.go`. Place the hash-comment languages (Perl, PowerShell, R, Makefile, TOML) in the "Hash-comment languages" section (currently lines 101-144, alongside Python/Shell/Ruby/YAML/Nix/HCL/Dockerfile). Give Batch its own short section since its comment token is unique. Use the existing helpers `line(prefix)` (line 28), `after(kind)` (line 33), and `before(kind)` (line 38). Follow the established `PreserveFirst` convention: every text type lists `after(model.PreserveBOM)` first (the "all: bom" rule documented at lines 46-49); add `after(model.PreserveShebang)` for types whose scripts can carry a shebang. The universal-shebang precedent (PR #5) means the explicit `PreserveShebang` rule is belt-and-suspenders for documentation/ordering, mirroring how Shell (lines 108-113) and PHP (lines 174-179) already list it. ```go // Perl: scripts begin with a "#!/usr/bin/perl" shebang; modules (.pm) do not. { Name: "Perl", Extensions: []string{".pl", ".pm", ".t"}, CommentStyle: line("# "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveShebang)}, }, { Name: "PowerShell", Extensions: []string{".ps1", ".psm1"}, CommentStyle: line("# "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)}, }, { // R: ".r" and ".R" both used; the extIndex lowercases keys (see init, line 198), // so listing ".r" is sufficient to match "script.R". Name: "R", Extensions: []string{".r"}, CommentStyle: line("# "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM), after(model.PreserveShebang)}, }, { // Makefile: exact filenames win over extensions (Lookup checks nameIndex first), // mirroring the Dockerfile entry. ".mk" covers included fragments. Name: "Makefile", Extensions: []string{".mk"}, Filenames: []string{"Makefile", "GNUmakefile"}, CommentStyle: line("# "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)}, }, { Name: "TOML", Extensions: []string{".toml"}, CommentStyle: line("# "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)}, }, // Windows Batch: comment token is "REM " (NOT "#" or "//"). A "#" or "//" line // would be a cmd.exe syntax error and break the script. { Name: "Batch", Extensions: []string{".bat", ".cmd"}, CommentStyle: line("REM "), PreserveFirst: []model.PreserveRule{after(model.PreserveBOM)}, }, ``` Notes: - **R `.r` vs `.R`:** `init()` (lines 195-204) lowercases every extension key into `extIndex`, and `Lookup` lowercases the queried extension (line 214), so a single `".r"` entry matches both `foo.r` and `foo.R`. Do not add `".R"` separately — duplicate keys would be redundant (last-write-wins in the map) and the table convention is one lowercase key. - **No collisions:** verified that `.pl`, `.pm`, `.t`, `.ps1`, `.psm1`, `.r`, `.toml`, `.bat`, `.cmd`, `.mk`, and filenames `Makefile`/`GNUmakefile` do not appear anywhere else in `builtin` today, so no existing mapping is displaced. (`.t` is Perl's test extension; it is unused elsewhere — keep it Perl.) - **Boundary code:** no change needed in `internal/render/render.go` or `internal/detect/detect.go`. The universal-shebang handling (PR #5) plus the per-type `PreserveBOM`/`PreserveShebang` rules already cover these types once the table entries exist. - **100% coverage gate must stay green:** `.testcoverage.yml` enforces `total: 100` over `./internal/...` and `./cmd/...` (the only excluded files are `cmd/license-tool/main.go`, `scripts/gen_spdx.go`, `cmd/license-tool/wizard.go`). Adding pure data rows to `builtin` introduces no new executable branches in `filetype.go` itself (the `init`/`Lookup`/`Merge` code paths are unchanged), but the new entries must each be exercised by a table-test assertion so the data is validated; see below. ## Tests & acceptance criteria Extend the existing tests in `internal/filetype/filetype_test.go` (no new test file needed): 1. **`TestLookupTable`** (lines 11-71): add one case row per new mapping asserting `wantName`, `wantOK: true`, `wantBlock: false` (all six are line-comment types), `wantSkip: false`. Cover every extension and filename: - `{"app.pl", "Perl", true, false, false}`, `{"Mod.pm", "Perl", true, false, false}`, `{"t/basic.t", "Perl", true, false, false}` - `{"deploy.ps1", "PowerShell", true, false, false}`, `{"mod.psm1", "PowerShell", true, false, false}` - `{"analysis.r", "R", true, false, false}`, `{"Analysis.R", "R", true, false, false}` (proves the lowercase-key match) - `{"Makefile", "Makefile", true, false, false}`, `{"GNUmakefile", "Makefile", true, false, false}`, `{"build/sub.mk", "Makefile", true, false, false}` - `{"pyproject.toml", "TOML", true, false, false}`, `{"Cargo.toml", "TOML", true, false, false}` - `{"run.bat", "Batch", true, false, false}`, `{"task.cmd", "Batch", true, false, false}` 2. **Batch comment-style assertion (the load-bearing one):** add an explicit check that `Lookup("run.bat")`'s `CommentStyle.LinePrefix == "REM "` and `CommentStyle.Block == false`. This is the regression guard against silently shipping `#`/`//`. Example: ```go bat, ok := Lookup("run.bat") require.True(t, ok) assert.False(t, bat.CommentStyle.Block) assert.Equal(t, "REM ", bat.CommentStyle.LinePrefix) ``` 3. **`TestPreserveFirstOrdering`** (lines 73-113): add an assertion that Perl lists `PreserveBOM` then `PreserveShebang` (mirroring the Shell assertion at lines 83-87), confirming a shebang'd Perl script keeps `#!` on line 1. 4. **Round-trip (optional but recommended):** if there is an apply/detect integration test elsewhere, add a Batch fixture proving a header is rendered as `REM SPDX-License-Tool: license-tool:managed` (via `render.Header` -> `wrapComment` line-prefix path, `internal/render/render.go:191-201`) and then re-detected (`internal/detect/detect.go` `lineCommentRegion`, lines 335-380, which derives its prefix from `cs.LinePrefix` so `REM ` works without code changes), to confirm the new type is end-to-end safe and idempotent. Acceptance criteria: - [ ] All six file-type groups resolve via `filetype.Lookup` with the names and `# `/`REM ` comment styles above. - [ ] Batch resolves to `LinePrefix: "REM "` (asserted by a dedicated test), never `# ` or `//`. - [ ] R resolves for both `.r` and `.R`. - [ ] Makefile resolves for the `Makefile` and `GNUmakefile` filenames and the `.mk` extension. - [ ] No existing `TestLookupTable` / `TestPreserveFirstOrdering` / Merge cases regress. - [ ] `go test ./...` passes and the `go-test-coverage` gate stays at 100% total (`.testcoverage.yml`). ## References - File-type table to edit: `internal/filetype/filetype.go` — `var builtin` (lines 50-187), `line`/`after`/`before` helpers (lines 22-40), `init`/`Lookup` (lines 195-222). - Existing dual filename+extension precedent (Dockerfile): `internal/filetype/filetype.go:138-144`. - Existing shebang-listing precedents: Shell `internal/filetype/filetype.go:108-113`, PHP `internal/filetype/filetype.go:174-179`. - PreserveKind constants: `internal/model/model.go:149-161`. - Render insert boundary: `internal/render/render.go` `preserveBoundary` (lines 320-367); universal-shebang block (lines 335-337). - Detect boundary: `internal/detect/detect.go` `PreserveBoundary` (lines 171-219); universal-shebang branch (`allowShebang := true`, line 187; consume at line 202). - Universal-shebang precedent (the "must-be-first, universally preserved" pattern to follow): PR #5 / v0.2.1, commit `f4a111f` ("fix: preserve shebang universally so headers never precede #! (#5)"). - Tests to extend: `internal/filetype/filetype_test.go` `TestLookupTable` (lines 11-71) and `TestPreserveFirstOrdering` (lines 73-113). - Coverage gate: `.testcoverage.yml` (`threshold.total: 100`).
KofTwentyTwo commented 2026-06-04 16:30:23 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — all six file-type groups are genuinely absent from the builtin table and resolve as unknown today; I reproduced this against the actual code.

Severity: low (agree with the enhancement label; not the "low-to-medium" the body floats). This is purely additive coverage. There is no file-corruption risk today: when classify misses, the file is marked skipped, never edited (see impact below). The Batch comment-token caveat is a correctness requirement for the new code, not a description of a current bug — so it does not raise the severity of the present state.

Verified root cause: internal/filetype/filetype.go, var builtin = []model.FileType{...} (lines 50–187). No entry exists for .pl/.pm/.t, .ps1/.psm1, .r/.R, Makefile/GNUmakefile/.mk, .toml, or .bat/.cmd. A grep of filetype.go for those extensions / REM / perl / powershell / makefile / toml returns nothing, and .t does not appear anywhere in internal/, so the proposed .t → Perl mapping has no collision. Confirmed at runtime with a throwaway in-package test — every one of Foo.pm, app.pl, t/basic.t, deploy.ps1, mod.psm1, analysis.r, Analysis.R, Makefile, GNUmakefile, build/sub.mk, cfg.toml, run.bat, task.cmd returns ok=false, name="" from filetype.Lookup (Lookup falls through nameIndex then extIndex and returns model.FileType{}, false at lines 209–222).

Downstream impact (verified, slightly more precise than the body): an unknown type is not "silently dropped" — internal/enumerate/enumerate.go:153–157 sets Skip = true with SkipReason = reasonUnknownType ("unknown type"). So these files are enumerated and reported as skipped, never written. The "never edited / no data loss today" framing is correct.

Suggested fix: The proposed table rows are sound. Confirmed supporting claims:

  • The R .r-only entry is correct: init() lowercases extension keys (extIndex[strings.ToLower(e)], line 198) and Lookup lowercases the query (line 214), so a single ".r" matches both foo.r and foo.R. Do not add ".R".
  • Makefile dual Filenames + .mk Extensions mirrors the existing Dockerfile entry (lines 138–144); Lookup checks nameIndex before extIndex, so the exact filenames win.
  • Batch REM is the load-bearing correctness point and is justified. #/// would be a cmd.exe syntax error. The v0.2.1 universal-shebang precedent (PR #5, commit f4a111f, "preserve shebang universally so headers never precede #!") is the right model to cite: it touched both boundaries — render insert (preserveBoundary, universal #! consume at internal/render/render.go:335–337) and detect (PreserveBoundary, allowShebang := true at internal/detect/detect.go:187, matched at line 202). For this issue, however, no boundary-engine change is required — #! is already preserved universally, so shebang'd Perl/R/PowerShell are handled once the table rows exist. The only edits are (a) the builtin table rows and (b) the tests. Batch render/detect work unchanged: wrapComment line-prefix branch (internal/render/render.go:191–201) and lineCommentRegion (internal/detect/detect.go:335–380) both derive from cs.LinePrefix. Keep the 100% gate green by adding a TestLookupTable row per extension/filename plus a dedicated assertion that Lookup("run.bat").CommentStyle.LinePrefix == "REM " and .Block == false — the proposed rows add only data (no new branches in filetype.go), so 100% total over ./internal/... and ./cmd/... (.testcoverage.yml, total: 100) stays reachable.

Corrections to the issue:

  • Severity over-claim: the body's "low-to-medium" should be plain low for the current state. The Batch hazard is a property of the fix, not the present code; the present code only skips these files.
  • Minor impact wording: files are not just "treated as unknown" abstractly — they are concretely marked Skip/SkipReason="unknown type" at enumerate.go:153–157. The body's filetype.go:209–222 cite for why Lookup returns false is correct; the skip decision lives one layer up in enumerate. Not an error, just an incomplete trace.
  • Line refs spot-checked and accurate: builtin 50–187, hash-cluster 101–144, Dockerfile 138–144, Shell 108–113, PHP 174–179, Lookup 209–222, render universal-shebang 335–337, detect allowShebang 187. No corrections needed there.

Open questions:

  1. Batch :: vs REM: body picks REM (defensible — :: breaks inside parenthesized blocks). Confirm REM is the final decision; it is the safe choice.
  2. Prefix-matching edge for REM: lineCommentRegion trims the prefix to "REM" (drops the trailing space) before HasPrefix, so a body line such as REMOVE THIS would be mis-read as a comment line during detection. This is pre-existing prefix-model behavior (not introduced here), but Batch is the first keyword-style prefix (vs symbol prefixes like #, //, --), so a maintainer should decide whether that edge is acceptable or warrants a word-boundary check. Low likelihood in practice for a license-header region.
  3. .t is overwhelmingly Perl test files but is also used by other tools (e.g. Terraform test/tablegen); confirm claiming .t for Perl is acceptable, or drop .t and keep .pl/.pm only.
  4. Scope: ship all six in one PR, or split Batch (the only one with a correctness trap) into its own commit for a focused review/test?

Overlaps: none. Depends conceptually on the v0.2.1 universal-shebang work (PR #5 / f4a111f) but that is already shipped, so no open dependency. No other open issue covers the file-type table (verify against the issue list before closing).

Automated maintainer review.

**Status:** confirmed — all six file-type groups are genuinely absent from the `builtin` table and resolve as unknown today; I reproduced this against the actual code. **Severity:** low (agree with the `enhancement` label; not the "low-to-medium" the body floats). This is purely additive coverage. There is **no** file-corruption risk *today*: when `classify` misses, the file is marked skipped, never edited (see impact below). The Batch comment-token caveat is a correctness requirement *for the new code*, not a description of a current bug — so it does not raise the severity of the present state. **Verified root cause:** `internal/filetype/filetype.go`, `var builtin = []model.FileType{...}` (lines 50–187). No entry exists for `.pl/.pm/.t`, `.ps1/.psm1`, `.r/.R`, `Makefile`/`GNUmakefile`/`.mk`, `.toml`, or `.bat/.cmd`. A grep of `filetype.go` for those extensions / `REM` / `perl` / `powershell` / `makefile` / `toml` returns nothing, and `.t` does not appear anywhere in `internal/`, so the proposed `.t → Perl` mapping has no collision. Confirmed at runtime with a throwaway in-package test — every one of `Foo.pm`, `app.pl`, `t/basic.t`, `deploy.ps1`, `mod.psm1`, `analysis.r`, `Analysis.R`, `Makefile`, `GNUmakefile`, `build/sub.mk`, `cfg.toml`, `run.bat`, `task.cmd` returns `ok=false`, `name=""` from `filetype.Lookup` (`Lookup` falls through `nameIndex` then `extIndex` and returns `model.FileType{}, false` at lines 209–222). Downstream impact (verified, slightly more precise than the body): an unknown type is not "silently dropped" — `internal/enumerate/enumerate.go:153–157` sets `Skip = true` with `SkipReason = reasonUnknownType` ("unknown type"). So these files are enumerated and reported as skipped, never written. The "never edited / no data loss today" framing is correct. **Suggested fix:** The proposed table rows are sound. Confirmed supporting claims: - The R `.r`-only entry is correct: `init()` lowercases extension keys (`extIndex[strings.ToLower(e)]`, line 198) and `Lookup` lowercases the query (line 214), so a single `".r"` matches both `foo.r` and `foo.R`. Do not add `".R"`. - Makefile dual `Filenames` + `.mk Extensions` mirrors the existing Dockerfile entry (lines 138–144); `Lookup` checks `nameIndex` before `extIndex`, so the exact filenames win. - **Batch `REM ` is the load-bearing correctness point and is justified.** `#`/`//` would be a `cmd.exe` syntax error. The v0.2.1 universal-shebang precedent (PR #5, commit `f4a111f`, "preserve shebang universally so headers never precede #!") is the right model to cite: it touched **both** boundaries — render insert (`preserveBoundary`, universal `#!` consume at `internal/render/render.go:335–337`) and detect (`PreserveBoundary`, `allowShebang := true` at `internal/detect/detect.go:187`, matched at line 202). For *this* issue, however, **no boundary-engine change is required** — `#!` is already preserved universally, so shebang'd Perl/R/PowerShell are handled once the table rows exist. The only edits are (a) the `builtin` table rows and (b) the tests. Batch render/detect work unchanged: `wrapComment` line-prefix branch (`internal/render/render.go:191–201`) and `lineCommentRegion` (`internal/detect/detect.go:335–380`) both derive from `cs.LinePrefix`. Keep the 100% gate green by adding a `TestLookupTable` row per extension/filename plus a dedicated assertion that `Lookup("run.bat").CommentStyle.LinePrefix == "REM "` and `.Block == false` — the proposed rows add only data (no new branches in `filetype.go`), so 100% total over `./internal/...` and `./cmd/...` (`.testcoverage.yml`, `total: 100`) stays reachable. **Corrections to the issue:** - Severity over-claim: the body's "low-to-medium" should be plain **low** for the current state. The Batch hazard is a property of the *fix*, not the present code; the present code only skips these files. - Minor impact wording: files are not just "treated as unknown" abstractly — they are concretely marked `Skip`/`SkipReason="unknown type"` at `enumerate.go:153–157`. The body's `filetype.go:209–222` cite for *why* `Lookup` returns false is correct; the skip decision lives one layer up in enumerate. Not an error, just an incomplete trace. - Line refs spot-checked and accurate: `builtin` 50–187, hash-cluster 101–144, Dockerfile 138–144, Shell 108–113, PHP 174–179, `Lookup` 209–222, render universal-shebang 335–337, detect `allowShebang` 187. No corrections needed there. **Open questions:** 1. Batch `::` vs `REM`: body picks `REM ` (defensible — `::` breaks inside parenthesized blocks). Confirm `REM ` is the final decision; it is the safe choice. 2. Prefix-matching edge for `REM`: `lineCommentRegion` trims the prefix to `"REM"` (drops the trailing space) before `HasPrefix`, so a body line such as `REMOVE THIS` would be mis-read as a comment line during detection. This is pre-existing prefix-model behavior (not introduced here), but Batch is the first keyword-style prefix (vs symbol prefixes like `#`, `//`, `--`), so a maintainer should decide whether that edge is acceptable or warrants a word-boundary check. Low likelihood in practice for a license-header region. 3. `.t` is overwhelmingly Perl test files but is also used by other tools (e.g. Terraform test/`tablegen`); confirm claiming `.t` for Perl is acceptable, or drop `.t` and keep `.pl/.pm` only. 4. Scope: ship all six in one PR, or split Batch (the only one with a correctness trap) into its own commit for a focused review/test? **Overlaps:** none. Depends conceptually on the v0.2.1 universal-shebang work (PR #5 / `f4a111f`) but that is already shipped, so no open dependency. No other open issue covers the file-type table (verify against the issue list before closing). _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:01 +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 enhancement
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#9