You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
--- 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.
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.
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.
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
Found from the outside while adding inbound-marker coverage to
mbeacom/adrkit-t018-dogfoodatc5dc677f(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.tsthat carries no explanatory comment.What happens
A source file declares
@adr 9999. The corpus has no9999.checkChangescorrectly produces adangling-markerfinding — andrenderCommentdrops it, so the pull request shows nothing at all.Minimal reproduction against
packages/*/srcatc5dc677f:The finding exists. The comment does not mention it.
Mechanism
packages/ci/src/comment.ts:48:Marker findings are excluded twice over: explicitly by
field, and again by the restriction to changed records — a source file is never inchangedRecords. Either clause alone is sufficient, so thefield !== '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.
absent/unreadable/out-of-treerender identically to "found nothing". Its proposed trigger iscounts.absent + counts.unreadable + counts['out-of-tree'] + counts.skipped > 0.0above. The scanner opened the file, read the marker, parsed it as well-formed, and could not bind it. Surface marker scan health in the rendered PR comment, not just the Action log #112's condition is false here, so its one-liner would stay silent.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 skippedand 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:
@adr 00012) produces a file that looks governed to its author and is not.In each case
adr checklocally reports it, and--jsoncarries 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:
warnin the body does not give it exit-code authority —CheckOutcome.okand the Action's failure condition read only validation errors on changed records, and would be untouched.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 thechangedRecordsfilter that no longer needs to be there?If the former, a one-line comment at
comment.ts:48saying 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