fix(structdrift): announce the resolved allow-list path (BLO-35637)

[allyblockcast]

What

tools/structdrift now prints the resolved absolute path of the allow-list it actually read, plus its entry count, before comparing.

One fmt.Fprintf. No new dependency (path/filepath was already imported), no behaviour change: exit codes, the comparison and the allow-list semantics are untouched.

Why

-allow is CWD-relative and defaults to tools/structdrift/allowed-drift.txt — a path that only resolves correctly from the repository root. The way this tool actually runs is a split checkout (trafficcontrol's .github/workflows/go-tc-drift.yml): the tool pinned in tool/, the tree under comparison mutable in client/. From tool/, the default resolves to the tool's own frozen copy.

That file exists. So the run stays green and nothing says which baseline it used — a moving tree gets compared against a pinned allow-list, and omissions newly accepted upstream start reading as real drift.

The defect was never the default. It was that nothing announced the substitution. Today the only guard is a comment in the caller's workflow, and a comment is not a guardrail.

Measured

Split checkout, frozen baseline (1 entry) against mutable (23 entries):

invocation	before	after
no -allow (wrong baseline)	no struct drift	allow-list: /…/tool/…/allowed-drift.txt (1 entries)
-allow ../client/… (right baseline)	no struct drift	allow-list: /…/client/…/allowed-drift.txt (23 entries)

Pre-fix output was byte-identical for the right and the wrong baseline — the substitution was undetectable from the log. Verified by reverting the guard and re-running both cases (mutation check), not just by observing the fixed output.

go vet, go build and go test ./tools/structdrift/ all pass.

Blast radius

• go-tc-drift.yml already passes -allow explicitly, and runs the tool pinned at ba3d37d6 — so CI is unaffected until that pin is bumped (bump owner: PlatformSREEngineer).
• allowed_drift_test.go reads allowed-drift.txt by package-relative path and never uses the flag.
• UPSTREAM.md's documented repo-root invocation keeps working unchanged — this deliberately does not make -allow required, which would have broken it.
• Org-wide code search for structdrift: only this repo and that one workflow.

Context

Found while closing out BLO-35637 (branch protection on this repo's main, now applied). The workflow-side half was BLO-35767 / Blockcast/trafficcontrol#1941, which fixed the caller; this fixes it at the source so the next caller cannot reintroduce it silently.

Issue: https://paperclip.blockcast.net/BLO/issues/BLO-35637

[allyblockcast]

🔗 Paperclip issue: BLO-35637
🔗 Paperclip issue: BLO-35767

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The nested Codex CLI is not launched inside the isolated Job pod; that lens was applied directly over the diff and the exact changed path.
Reviewed head: 4887ee0

Looks good. One fmt.Fprintf to stderr, placed after the parse so the count is real, with a handled filepath.Abs fallback. No behaviour change: exit codes, the comparison, and allow-list semantics are untouched, and filepath was already imported.

I independently verified the PR's blast-radius claims rather than taking them from the description:

• Blockcast/trafficcontrol/.github/workflows/go-tc-drift.yml passes -allow ../client/tools/structdrift/allowed-drift.txt explicitly, from working-directory: tool.
• The tool is pinned at ba3d37d67bba7c3337515107d37d4f68a91beb11 — matches the ba3d37d6 in the description, so this is inert in that workflow until PlatformSREEngineer bumps the pin.
• No step captures or pattern-matches the tool's output, so an added stderr line breaks no consumer.
• allowed_drift_test.go reads allowed-drift.txt by package-relative path and never touches the flag.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

• [code] tools/structdrift/main.go:154 — a missing allow-list renders as (0 entries), which is indistinguishable from a present-but-empty one. allowed() returns (empty, nil) on os.IsNotExist (main.go:97-99), so a stale or typo'd -allow path silently yields zero exemptions. That is the strongest form of the substitution this PR targets, and it is the one case the new line still does not announce. It fails in the safe direction — red, with 23 field names and the resolved path on screen — which is why this is a suggestion rather than a blocker.
  • Cheapest fix is at the call site, since allowed() is the only thing that knows: if _, err := os.Stat(*allowFile); os.IsNotExist(err) { ... "(missing)" }, or have allowed return the distinction.
• [tests] tools/structdrift/main.go:154 — nothing asserts the line is emitted. Reverting the guard and re-running both baselines is exactly the right check and I'd rather see that than a fixture, but it isn't durable: a later refactor of main() can drop the line and every test still passes. The logic is a print in main(), so a durable check means either a small os/exec test or lifting the formatting into a testable func — reasonable to skip, worth a deliberate decision.
• [comments] tools/structdrift/main.go:142-151 — the rationale now states the caller's tool/ + client/ split-checkout layout in three places: this comment, the ~14-line comment above the workflow's Compare struct fields step, and the PR body. When that layout changes, two of the three rot, and the stale copies are the ones a future reader hits first. Comment density matches this file's house style, so the length is right; consider referencing BLO-35637 and the workflow for the layout specifics rather than restating them.

Strengths

• filepath.Abs error is handled with a fallback to the raw path instead of being ignored or made fatal — the observability line cannot itself introduce a failure mode.
• Printing the entry count alongside the path is the stronger of the two signals: 1 vs 23 separates the frozen and mutable baselines even when the paths are visually similar in a log. Emitting both is good design, not redundancy.
• Correctly sequenced after allowed() so the count reflects a real parse, and correctly on stderr — consistent with every other diagnostic here, while no struct drift stays on stdout.
• Deliberately does not make -allow required, which preserves the documented repo-root invocation in UPSTREAM.md. The lazier fix was the right one.
• Fixing this at the source rather than only at the caller is the correct altitude: the workflow comment is not a guardrail, and the next caller would have reintroduced this silently.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[kkroo]

Approving as the human review this repo requires. Ally authored the PR, and its own at-head review is clean (0 Critical / 0 Important).

The diff adds one stderr line, the resolved absolute -allow path plus its entry count, printed before the comparison runs. It has no effect on exit codes, the comparison, or allow-list semantics. If filepath.Abs fails, it falls back to the raw flag value, which is fine for a log line. build-and-test is green at this head.

Reviewed head: 4887ee0