deduplicate header placement and sentinel logic across detect and render #27

New Issue

2026-06-04T16:19:33Z

KofTwentyTwo commented

2026-06-04 16:19:33 +00:00

(Migrated from github.com)

Finding

Header placement and sentinel logic are duplicated between detection and rendering. The two implementations must stay byte-compatible for idempotent replacement, but they currently drift in at least one edge case.

Evidence

internal/detect/detect.go:28 to internal/detect/detect.go:30 defines the detection sentinel value.
internal/render/render.go:41 to internal/render/render.go:50 duplicates the sentinel value.
internal/detect/detect.go:160 to internal/detect/detect.go:219 computes preserve-first boundaries for detection.
internal/render/render.go:310 to internal/render/render.go:367 separately computes preserve-first insertion boundaries.
The Python coding= bug is an example of predicate drift between these paths.

Why It Matters

This is a safety-critical part of a source-rewriting tool. If detection and rendering disagree, apply can insert headers in the wrong location, fail idempotency, or miss headers it previously wrote.

Suggested Fix

Move sentinel and preserve-boundary predicates into a small shared package with no import cycle. Add cross-package golden tests that assert inserted headers are detected at the same span on a second pass.

## Finding Header placement and sentinel logic are duplicated between detection and rendering. The two implementations must stay byte-compatible for idempotent replacement, but they currently drift in at least one edge case. ## Evidence - `internal/detect/detect.go:28` to `internal/detect/detect.go:30` defines the detection sentinel value. - `internal/render/render.go:41` to `internal/render/render.go:50` duplicates the sentinel value. - `internal/detect/detect.go:160` to `internal/detect/detect.go:219` computes preserve-first boundaries for detection. - `internal/render/render.go:310` to `internal/render/render.go:367` separately computes preserve-first insertion boundaries. - The Python `coding=` bug is an example of predicate drift between these paths. ## Why It Matters This is a safety-critical part of a source-rewriting tool. If detection and rendering disagree, apply can insert headers in the wrong location, fail idempotency, or miss headers it previously wrote. ## Suggested Fix Move sentinel and preserve-boundary predicates into a small shared package with no import cycle. Add cross-package golden tests that assert inserted headers are detected at the same span on a second pass.

KofTwentyTwo commented

2026-06-04 16:32:07 +00:00

(Migrated from github.com)

Status: confirmed — detect and render really do duplicate sentinel + preserve-boundary logic, and the two boundary predicates disagree on the coding= form, producing a reproducible mis-placement.

Severity: high (label is enhancement/dedup, but the drift is a live correctness bug, not just tech debt). The duplication itself is medium tech-debt; the concrete drift corrupts files, so I'd treat the underlying defect as high. See repro below.

Verified root cause

Sentinel duplication is real and intentional-but-fragile:
- internal/detect/detect.go:30 — const Sentinel = "license-tool:managed"
- internal/render/render.go:50 — const sentinel = "license-tool:managed" (the comment at render.go:41-49 documents the deliberate copy to avoid an import cycle). These two are currently byte-equal, so no live bug here — but note detect matches on the raw Sentinel substring while render writes it behind a SPDX-License-Tool: prefix (render.go:104); only the substring contract keeps them aligned.
Boundary duplication and the actual drift:
- internal/detect/detect.go:PreserveBoundary (lines 171-219), whose coding-pragma test is internal/detect/detect.go:isCodingPragma:519 → matches coding: OR coding=.
- internal/render/render.go:preserveBoundary (lines 320-367), whose coding-pragma arm at render.go:362 matches only coding: (strings.Contains(t, "coding:")).
- The two functions return different offsets for the same # coding=utf-8 input.

Reproduction (built from current tree, MIT/reuse on a Python file):

# -*- coding: utf-8 -*- → header inserted AFTER the pragma (correct).
# coding=utf-8 → header inserted at byte 0, BEFORE the pragma, demoting the encoding declaration to line 5.

Confirmed at the unit level too: on the original # coding=utf-8\nprint(...), detect.PreserveBoundary returns offset 15 (past the pragma) while render.preserveBoundary returns 0. Apply is still byte-idempotent on a second pass, so the corruption is silent — it never self-heals. Per PEP 263 a Python encoding declaration must be on line 1 or 2, so this is an actual file-semantics break, not a cosmetic one.

Suggested fix
This is the same defect class the v0.2.1 universal-shebang fix addressed (CHANGELOG: header written at byte 0 before a preserve-first prefix, corrupting the file) — there the boundary logic was the thing that had to change. Two parts:

Immediate correctness fix: widen render.go:362 to match coding= as well, ideally by having render call the same predicate detect uses (isCodingPragma) so they cannot diverge again. As with the shebang precedent, the fix must land in BOTH boundary functions' shared logic, and the file-type table (internal/filetype/filetype.go; Python/.py,.pyi carry after(PreserveCodingPragma) at lines ~103-118) does not itself need a new entry here — the rule is already wired; only render's predicate is too narrow.
The dedup the issue actually asks for: lift Sentinel and the preserve-boundary predicates into a small leaf package (e.g. internal/preserve) that both detect and render import, breaking the stated import-cycle concern (render's cycle worry is with the applier, not a leaf package). Add cross-package golden tests asserting that for every preserve-first fixture, detect.PreserveBoundary and render.preserveBoundary return the identical offset, and that a header inserted by render is detected at the same span on a second pass. Keep the 100% total gate green (.testcoverage.yml total: 100) — any new leaf package needs full coverage, and folding the two predicates into one removes a currently-duplicated (and currently-divergent) code path.

Corrections to the issue

Line refs are anchored on the doc-comment start, not the code: sentinel is at detect.go:30 (not 28-30) and render.go:50 (not 41-50); the boundary funcs are detect.go:171-219 and render.go:320-367 (the cited 160/310 starts are the doc comments). Minor.
"predicate drift between these paths" undersells it: the coding= case is not just a detect/render disagreement, it actively mis-places the header and breaks a PEP 263 encoding line — worth calling out as a standalone correctness bug, not only as evidence of duplication.
The sentinel constants are currently byte-identical, so there is no live sentinel drift today; the risk is future drift. The only confirmed live drift is the coding-pragma predicate.

Open questions

Should the coding= correctness fix ship as its own bugfix release ahead of the larger dedup refactor (recommended, given it corrupts files), or only as part of the shared-package extraction?
Scope of the shared package: just Sentinel + boundary predicates, or also the comment-region/EOL helpers? Pulling too much risks expanding the import surface.
Does detect's coding= recognition match the intended spec (PEP 263 allows coding: and coding=)? If yes, render is simply wrong; if the intent was coding:-only, then detect is the side to narrow — a maintainer should pick the canonical predicate before it's shared.

Overlaps

Depends on / shares root with the v0.2.1 universal-shebang fix (same boundary-corruption class; already shipped — not re-reporting it). No other open issue numbers identified; none.

Automated maintainer review.

**Status**: confirmed — detect and render really do duplicate sentinel + preserve-boundary logic, and the two boundary predicates disagree on the `coding=` form, producing a reproducible mis-placement. **Severity**: high (label is `enhancement`/dedup, but the drift is a live correctness bug, not just tech debt). The duplication itself is medium tech-debt; the concrete drift corrupts files, so I'd treat the underlying defect as high. See repro below. **Verified root cause** - Sentinel duplication is real and intentional-but-fragile: - `internal/detect/detect.go:30` — `const Sentinel = "license-tool:managed"` - `internal/render/render.go:50` — `const sentinel = "license-tool:managed"` (the comment at `render.go:41-49` documents the deliberate copy to avoid an import cycle). These two are currently byte-equal, so no live bug here — but note detect matches on the raw `Sentinel` substring while render writes it behind a `SPDX-License-Tool: ` prefix (`render.go:104`); only the substring contract keeps them aligned. - Boundary duplication and the actual drift: - `internal/detect/detect.go:PreserveBoundary` (lines 171-219), whose coding-pragma test is `internal/detect/detect.go:isCodingPragma:519` → matches `coding:` **OR** `coding=`. - `internal/render/render.go:preserveBoundary` (lines 320-367), whose coding-pragma arm at `render.go:362` matches **only** `coding:` (`strings.Contains(t, "coding:")`). - The two functions return different offsets for the same `# coding=utf-8` input. **Reproduction** (built from current tree, MIT/reuse on a Python file): - `# -*- coding: utf-8 -*-` → header inserted AFTER the pragma (correct). - `# coding=utf-8` → header inserted at byte 0, BEFORE the pragma, demoting the encoding declaration to line 5. Confirmed at the unit level too: on the original `# coding=utf-8\nprint(...)`, `detect.PreserveBoundary` returns offset 15 (past the pragma) while `render.preserveBoundary` returns 0. Apply is still byte-idempotent on a second pass, so the corruption is silent — it never self-heals. Per PEP 263 a Python encoding declaration must be on line 1 or 2, so this is an actual file-semantics break, not a cosmetic one. **Suggested fix** This is the same defect class the v0.2.1 universal-shebang fix addressed (CHANGELOG: header written at byte 0 before a preserve-first prefix, corrupting the file) — there the boundary logic was the thing that had to change. Two parts: 1. Immediate correctness fix: widen `render.go:362` to match `coding=` as well, ideally by having render call the same predicate detect uses (`isCodingPragma`) so they cannot diverge again. As with the shebang precedent, the fix must land in BOTH boundary functions' shared logic, and the file-type table (`internal/filetype/filetype.go`; Python/`.py`,`.pyi` carry `after(PreserveCodingPragma)` at lines ~103-118) does not itself need a new entry here — the rule is already wired; only render's predicate is too narrow. 2. The dedup the issue actually asks for: lift `Sentinel` and the preserve-boundary predicates into a small leaf package (e.g. `internal/preserve`) that both detect and render import, breaking the stated import-cycle concern (render's cycle worry is with the applier, not a leaf package). Add cross-package golden tests asserting that for every preserve-first fixture, `detect.PreserveBoundary` and `render.preserveBoundary` return the identical offset, and that a header inserted by render is detected at the same span on a second pass. Keep the 100% total gate green (`.testcoverage.yml` total: 100) — any new leaf package needs full coverage, and folding the two predicates into one removes a currently-duplicated (and currently-divergent) code path. **Corrections to the issue** - Line refs are anchored on the doc-comment start, not the code: sentinel is at `detect.go:30` (not 28-30) and `render.go:50` (not 41-50); the boundary funcs are `detect.go:171-219` and `render.go:320-367` (the cited 160/310 starts are the doc comments). Minor. - "predicate drift between these paths" undersells it: the `coding=` case is not just a detect/render disagreement, it actively mis-places the header and breaks a PEP 263 encoding line — worth calling out as a standalone correctness bug, not only as evidence of duplication. - The sentinel constants are currently byte-identical, so there is no live sentinel drift today; the risk is future drift. The only confirmed live drift is the coding-pragma predicate. **Open questions** - Should the `coding=` correctness fix ship as its own bugfix release ahead of the larger dedup refactor (recommended, given it corrupts files), or only as part of the shared-package extraction? - Scope of the shared package: just `Sentinel` + boundary predicates, or also the comment-region/EOL helpers? Pulling too much risks expanding the import surface. - Does detect's `coding=` recognition match the intended spec (PEP 263 allows `coding:` and `coding=`)? If yes, render is simply wrong; if the intent was `coding:`-only, then detect is the side to narrow — a maintainer should pick the canonical predicate before it's shared. **Overlaps** - Depends on / shares root with the v0.2.1 universal-shebang fix (same boundary-corruption class; already shipped — not re-reporting it). No other open issue numbers identified; none. _Automated maintainer review._

KofTwentyTwo commented

2026-06-04 20:28:25 +00:00

(Migrated from github.com)

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.

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.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#27