Skip to content

A dangling @adr marker is invisible in the rendered PR comment, even when the scan was completely healthy #126

Description

@mbeacom

Found from the outside while adding inbound-marker coverage to mbeacom/adrkit-t018-dogfood at c5dc677f (v0.6.0).

Filing this as a question about intent rather than a bug report: the behavior is explicit in the source, so it may well be deliberate. But the consequence is easy to hit and hard to notice, and the exclusion is the one decision in comment.ts that carries no explanatory comment.

What happens

A source file declares @adr 9999. The corpus has no 9999. checkChanges correctly produces a dangling-marker finding — and renderComment drops it, so the pull request shows nothing at all.

Minimal reproduction against packages/*/src at c5dc677f:

const lint = await lintCorpus({ dir, cwd });
const changedFiles = ['src/app.ts'];              // contains `// @adr 9999`
const markerScans = await readSourceMarkersBatch(changedFiles, cwd);
const outcome = checkChanges({ lint, changedFiles, dir, markerScans });

console.log(JSON.stringify(outcome.markerScan?.counts));
console.log(JSON.stringify(outcome.findings));
console.log(renderComment(outcome));
--- markerScan counts ---
{"scanned":1,"absent":0,"unreadable":0,"out-of-tree":0,"truncated":0,"skipped":0}

--- findings ---
[{"rule":"dangling-marker","severity":"warn",
  "message":"Source marker \"@adr 9999\" in src/app.ts:1 does not resolve to a record in the corpus",
  "path":"src/app.ts","field":"marker","pattern":"9999"}]

--- rendered comment ---
<!-- adrkit:ci -->

### Decisions governing this change

- **0001** — An example decision
  - via `path`: `src/**`

The finding exists. The comment does not mention it.

Mechanism

packages/ci/src/comment.ts:48:

finding.field !== 'marker' && finding.path !== undefined && changed.has(finding.path)

Marker findings are excluded twice over: explicitly by field, and again by the restriction to changed records — a source file is never in changedRecords. Either clause alone is sufficient, so the field !== 'marker' test reads as intentional rather than incidental.

Why this is not #112

I checked #112 first, and I think this is a genuinely different gap — worth saying because the fix proposed there would not cover this case.

Put together: #112 is "could not look"; this is "looked, saw it, could not bind it".

Live instance of the same shape, where the counters were 23 scanned, 0 absent, 0 unreadable, 0 out-of-tree, 6 truncated, 0 skipped and the comment still said nothing about a dangling marker in the changed set: run 31555185462.

Why it matters

The failure mode is a silent no-op on the surface most reviewers read:

  • A record is renamed or its id changes, and every marker pointing at the old id silently stops binding.
  • A typo (@adr 00012) produces a file that looks governed to its author and is not.
  • A file is copied between repositories with a marker naming a record the destination corpus does not have.

In each case adr check locally reports it, and --json carries it — so nothing is lost, and I want to be precise that this is a surfacing gap, not a detection one. But CI is where the cost lands, and CI is the one place that says nothing.

Constraints any fix would need to respect

Noting these because they are what make the current behavior defensible, and I do not think they force it:

  • ADR-0022 §2 makes marker findings advisory by construction, and §3 says a marker claim is rendered as a claim. Surfacing a warn in the body does not give it exit-code authority — CheckOutcome.ok and the Action's failure condition read only validation errors on changed records, and would be untouched.
  • The comment budget tuned in feat(core,ci): wire inbound @adr markers into check #106 means changed-record validation errors must keep priority under truncation. A dangling-marker line must not be able to displace them.
  • Volume is bounded in practice by MARKER_SCAN_FILE_CAP, but a per-marker list is still unbounded in principle; a count with the first N paths would sidestep that.

The question

Is the field !== 'marker' exclusion a considered trade-off (markers live in files the corpus does not own, so their findings are deliberately not the corpus's business to report), or belt-and-braces alongside the changedRecords filter that no longer needs to be there?

If the former, a one-line comment at comment.ts:48 saying so would stop the next person from reading it as an oversight — which is exactly what I did before checking the source. If the latter, this and #112 may be worth solving together as a single "marker health and marker findings both need a voice in the comment" change.


Recorded in the dogfood repository as an observation with its mechanism, deliberately not as a defect claim: mbeacom/adrkit-t018-dogfood#14

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions