feat(core,cli): resolve inbound @adr source annotations without a schema change

[aballiet]

Closes #95.

Summary

ADR records already declare outbound governance through affects patterns. This adds the inverse, explicit edge: a source file can declare the decision it lives under with an @adr marker, and adr explain <path> reports both directions without changing the ADR schema.

// @adr 0012
export function syncOnce() { … }

Decisions governing src/services/sync/retry.ts:
  0009  [accepted] Resolve affects deterministically
    via path: src/services/sync/**
  0012  [accepted] Bind catalog entities to owned paths
    declared by src/services/sync/retry.ts:1 (@adr 0012)

via is the record reaching out; declared by is the file reaching in. The distinction remains explicit in human and JSON output.

The motivation is context precision. In the measured 1,357-file codebase from #95, subsystem globs matched 163 files (~56,000 tokens if all were touched), while defining-file patterns matched 28 (~9,100 tokens) but lost the surrounding files that still live under those decisions. Inbound markers let affects remain narrow without losing those explicit relationships.

This follows ADR-0012's principle: ownership is declared explicitly rather than inferred. Here the same principle points from a file to its governing decision.

Marker contract

• A declaration occupies a dedicated physical comment line. After optional spaces or tabs, one of //, /*, *, #, --, ;, %, <!--, """, or ''' begins the line, and @adr is the comment's first content. Trailing forms such as code(); // @adr 0012, inline strings, and prose before @adr do not declare the file.
• The reference is an AdrRef: a local id such as 0012 or a qualified ref such as payments:0012. A comma continues a list; a bare space ends it.
• Only the first 8192 bytes are considered. The pure scanner fills a fixed byte window rather than encoding the entire input.
• Filesystem reads fill an 8193-byte buffer (the window plus one sentinel), so short reads do not hide content and truncation is observed from bytes rather than re-derived from decoded text.
• A truncated final physical line is discarded, so severing @adr 00123 cannot invent the valid but different ref 0012.
• UTF-8 BOMs, invalid UTF-8 expansion, and CR, LF, and CRLF physical line endings preserve the same truncation and line-number contract.

The dedicated-line heuristic intentionally remains language-agnostic. A multiline string or fenced documentation example whose marker-looking comment begins the physical line can still match; avoiding that would require language-specific parsing. Scanning this branch yields four marker-looking lines: the real ADR-0021 dogfood declaration and three fenced documentation examples.

Malformed spellings that are not valid AdrRef tokens are currently ignored rather than normalized or diagnosed.

Resolution and output

• packages/core/src/markers/ contains the bounded scanner, the single filesystem boundary, marker resolution, and public contracts.
• A marker naming a missing local record produces dangling-marker at warn.
• A log-qualified marker targeting another corpus produces marker-unresolvable at info.
• Marker declarations are merged into an explain-only ExplainedDecision shape through optional declaredBy; the shared GoverningDecision used by checkChanges, adr check, and the Action remains marker-free.
• Pattern-only explain results omit declaredBy, preserving their existing JSON shape.
• adr explain --json always reports markers.state, markers.truncated, markers.windowBytes, and the declarations observed. Human output emits a Note: whenever the file was not scanned or the window was truncated, including when corpus errors stop resolution.

Filesystem boundary

Marker reads accept one repo-relative regular file beneath the working tree. Absolute paths, lexical escapes, and symlinks resolving outside the tree are not opened for marker scanning. Reads are bounded, read-only, and non-blocking; non-regular files such as FIFOs are rejected.

The boundary applies to the new content-derived marker edge. Existing affects resolution retains its behavior over the raw path argument, so a broad glob may still match an outside-path string.

The remaining limitations are recorded in ADR-0021: resolving an in-tree symlink can disclose whether an external target is absent, unreadable, or out of tree, and a concurrent process can race the realpath/open interval. This PR does not expose marker scanning through an untrusted CI surface.

Scope

• adr explain <path> scans and resolves inbound markers.
• adr check, checkChanges, @adrkit/ci, and the spec-kit agent context script do not scan markers. Extending enforcement to those surfaces is a separate decision because it changes CI semantics and the filesystem boundary.
• AdrFrontmatter, AffectsType, and schema/adr.schema.json are unchanged.
• No runtime dependency is added.
• packages/ci/dist remains byte-identical after rebuilding; marker runtime code is not reachable from the Action entry points.
• ADR-0021 records the design, trade-offs, boundary, and follow-up decisions.

Verification

• All commits are DCO signed off.
• bun install --frozen-lockfile
• bun run typecheck
• bun run build
• bun test
• bun run lint
• bun run check:deps
• bun run schema:emit produces no schema diff.
• The documentation site builds.
• Focused regressions cover dedicated-line false positives, bounded public scanning, BOM and invalid UTF-8 behavior, CR-only line endings, truncation safety, scan-state output, filesystem confinement, FIFO handling, resolution determinism, explain-only typing, and Action bundle scope.

[aballiet]

Follow-up commit 3fe022a: I had missed one of your conventions on the first pass.

markers/scan.ts and markers/resolve.ts claim in prose to be pure — text in, markers out, with markers/read.ts as the single deliberate filesystem boundary — but nothing checked it, while affects/ has had affects-purity.test.ts guarding the equivalent claim since feature 002 and CI names it ("Test (includes resolution-is-pure)"). CONTRIBUTING is explicit that a claim of the form "X is not there" needs a check.

Added the mirror test: determinism for both entry points with process.env unmutated, no fs/process/network/clock API in any marker source outside the boundary, and — the part that matters — the boundary asserted as the observed set of fs-importing files rather than a count, so a new one has to be declared deliberately instead of slipping past a green check.

[Copilot]

Pull request overview

Adds inbound @adr source annotations so adr explain can resolve file-declared decisions without changing the ADR schema.

Changes:

• Adds bounded marker scanning, resolution, and public core APIs.
• Integrates marker results into CLI human and JSON output.
• Adds ADR, documentation, and comprehensive tests.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
site/src/content/docs/commands.mdx	Documents marker syntax and output.
README.md	Summarizes inbound markers.
packages/core/test/surface.test.ts	Verifies public exports.
packages/core/test/markers-scan.test.ts	Tests scanning and bounded reads.
packages/core/test/markers-resolve.test.ts	Tests resolution and merging.
packages/core/test/markers-purity.test.ts	Tests purity boundaries.
packages/core/src/markers/types.ts	Defines marker types.
packages/core/src/markers/scan.ts	Implements marker scanning.
packages/core/src/markers/resolve.ts	Resolves and merges markers.
packages/core/src/markers/read.ts	Reads source headers.
packages/core/src/markers/index.ts	Exports marker APIs.
packages/core/src/index.ts	Exposes marker APIs publicly.
packages/core/src/check/index.ts	Adds optional declaration metadata.
packages/core/README.md	Documents core marker APIs.
packages/cli/test/lint.test.ts	Updates corpus count.
packages/cli/test/explain.test.ts	Updates explain contracts.
packages/cli/test/explain-markers.test.ts	Tests marker CLI behavior.
packages/cli/src/index.ts	Integrates markers into explain.
packages/cli/README.md	Documents CLI marker behavior.
docs/adr/0021-resolve-inbound-source-annotations-without-changing-the-schema.md	Records the design decision.

Suppressed comments (1)

packages/core/src/markers/read.ts:51

• Opening the path before establishing that it is a regular file can block forever on a FIFO (for example, mkfifo src/pipe; adr explain src/pipe). That never reaches the advertised unreadable scan state. Open nonblocking and verify the handle's file type before reading, or otherwise reject non-regular files without a blocking open.

    handle = await open(absolutePath, 'r');
    const buffer = new Uint8Array(MARKER_HEADER_WINDOW_BYTES + 1);
    const { bytesRead } = await handle.read(buffer, 0, buffer.length, 0);

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

packages/core/src/markers/read.ts:113

• A single FileHandle.read is allowed to return fewer bytes than requested before EOF. Treating that short read as the whole file can miss markers later in the 8192-byte window and incorrectly report truncated: false; fill the bounded buffer until EOF or capacity.

    const buffer = new Uint8Array(MARKER_HEADER_WINDOW_BYTES + 1);
    const { bytesRead } = await handle.read(buffer, 0, buffer.length, 0);
    const scan = scanSourceMarkers(new TextDecoder().decode(buffer.subarray(0, bytesRead)), path);
    return { path, state: 'scanned', markers: scan.markers, truncated: scan.truncated };

packages/core/src/markers/scan.ts:121

• Splitting only on LF treats a CR-only file as one physical line. A comment introducer on an earlier line can then make a later bare @adr look valid, and every reported line number is 1, contrary to the same-physical-line contract. Split CR, LF, and CRLF line endings.

  const lines = window.text.split('\n');

packages/core/src/markers/scan.ts:67

• This truncation boundary recognizes only LF. For a valid CR-only source larger than the window, lastNewline remains -1, so the entire header is discarded even when it contains complete marker lines. Treat CR as a physical line boundary too.

This issue also appears on line 121 of the same file.

  const text = new TextDecoder().decode(bytes.subarray(0, MARKER_HEADER_WINDOW_BYTES));
  const lastNewline = text.lastIndexOf('\n');
  return { text: lastNewline === -1 ? '' : text.slice(0, lastNewline + 1), truncated: true };

packages/cli/src/index.ts:520

• Marker scan state is preserved here only for JSON. On the same corpus-error path, human output discards absent/unreadable/out-of-tree/truncated state and emits no promised Note:. Render the scan note in the human branch as well.

            markers: markerScanJson(scan),
            findings: corpusFindings,

[mbeacom]

Thank you, @aballiet!

This is good work and I want it in. The ADR-0012 reading is the right one: it rejected inferring ownership and asked for an explicit annotation, and a file declaring its own decision is that principle pointed the other way. Nothing to supersede. Your three follow-ups also landed most of what I was going to raise, so I'll skip those.

I ran a panel of independent reviews over the branch before replying, which turned up more than I had on my own, including one thing I had told you that was wrong. Three things to sort before merging. The first two are the same problem wearing different clothes: the scanner reports a decision the file did not declare.

1. Try running the scanner across your whole branch. I did, and it finds 51 markers in 9 files, of which exactly one is a real declaration: your @adr 0021 dogfood. The rest are doc prose, test fixtures, and fenced examples. scan.ts is the one that made me wince:

$ adr explain packages/core/src/markers/scan.ts
Decisions governing packages/core/src/markers/scan.ts:
  0012  [accepted] Bind catalog entities to owned paths with an explicit annotation
    declared by packages/core/src/markers/scan.ts:31 (@adr 0012)
    declared by packages/core/src/markers/scan.ts:58 (@adr 0012)
    declared by packages/core/src/markers/scan.ts:78 (@adr 0012)
    declared by packages/core/src/markers/scan.ts:79 (@adr 0012)
  0013  [accepted] Reconcile adapter isolation and catalog binding with the offline snapshot generator
    declared by packages/core/src/markers/scan.ts:78 (@adr 0013)

Every one of those is the JSDoc where you explain the feature's trade-offs. Two catalog-ownership records with nothing to do with marker scanning land in the accepted bucket, the one an agent treats as binding. The PR calls out string literals as the accepted false positive; I think prose is the bigger one, because any file that discusses a decision now claims to live under it, and those are exactly the files worth explaining.

I tried two tightenings on your branch to see what would move it. Still a token rule, still no parser:

Rule	Markers
As shipped	51
Marker leads the comment content	35
+ introducer leads the line	4

The four left over are three fenced doc examples and your dogfood. The cost is trailing } // @adr 0012, which I'd argue is fine, since a file-level claim probably wants its own line anyway. Totally your call on the predicate; I'm more attached to the ratio than to my rule.

2. Truncation can still invent a reference, which is the invariant your own test protects. read.ts reads WINDOW + 1 bytes, so it knows whether the file continued, then discards that and returns a string. headerWindow re-derives truncation by re-encoding that string and comparing to 8192. Decode then encode is not length-preserving: TextDecoder strips a leading BOM, three bytes vanish, the re-encode lands under the window, and the severed-line cut-back never runs.

$ adr explain src/bomtest.ts      # BOM-prefixed, 8200 bytes, text says "@adr 00123"
Decisions governing src/bomtest.ts:
  0012  [accepted] Bind catalog entities to owned paths with an explicit annotation
    declared by src/bomtest.ts:2 (@adr 0012)

markers: {"state":"scanned","windowBytes":8192,"truncated":false,"declared":[{"ref":"0012","line":2}]}

The file never says 0012. Truncation invented it, and truncated: false claims the whole file was scanned. BOMs are ordinary in Windows-authored sources. It runs the other way too: invalid UTF-8 expands to U+FFFD, so a Latin-1 header can report truncated: true and clip text back below what was actually read. Cheapest fix is to stop re-deriving the fact you already have: thread the observed bytesRead > WINDOW from read.ts into the scanner and cut back on that flag, keeping the byte derivation only for the pure string-in entry point.

3. declaredBy on GoverningDecision. Small one. That type is shared by explain, check --json, and the Action's comment, but only explain calls mergeSourceDeclarations, so adr explain and adr check disagree about packages/core/src/check/index.ts and check --json never carries the key at all. Same shape you invoke ADR-0016 for with markers.state, just on the other surface. Moving it to an explain-local type also un-inverts check/index.ts importing from markers/types.ts while markers/resolve.ts imports from check/index.ts. Easier now than later: adding an optional field is additive, removing one isn't.

One more I'd like but won't gate on. A ref token that fails validation disappears with no finding and no note:

// @adr 0012.      ->  ["0012"]
// @adr 0012:      ->  []
// @adr ADR-0012   ->  []
// @adr #0012      ->  []

A trailing colon deletes the declaration, a trailing period keeps it, and ADR-0012 is a thing people will type. It under-attributes rather than mis-attributes, so nothing false reaches a reader, which is why it isn't in the three above. But the output is identical to a file that declares nothing, which is the property the rest of this PR works hard to avoid. Either stop the token scan before the character that can't continue a ref, or emit a warn naming what was rejected.

In the ADR. Worth saying in Scope that the asymmetry is deliberate: markers reach explain and nothing else. Two corrections to what I said earlier, both mine:

I told you the MCP sandbox would deny a marker read. It would not. The read guard is inert unless ADRKIT_MCP_TEST_READ_ROOTS is set, the four-tool trap test spawns the server with no env at all, and where it is armed the root is the whole working tree. So "the server never opens a caller-supplied path" is an unenforced invariant rather than a guarded one. That is my problem to fix, not yours, and it does not change your scope. It does mean the record shouldn't claim an enforcement that isn't there.

I also said adr explain gets markers to agents on day one. Mostly true, but this repo's own agent surface contradicts it: packages/adapters/spec-kit/scripts/context.sh shells out to adr check, not explain. Worth a clause so the scope reads honestly.

On the bot threads. The first is stale, you already fixed it in 5e739aa7.

On the TOCTOU, I think your conclusion is right and your argument can be stronger. "Anyone who can win the race can edit docs/adr/* directly" holds when you own the tree; it weakens when CI is the reader, because a committed edit is visible in a diff and a won race isn't. The version that survives: winning the race needs a concurrent process, and a job that only checks out and runs adr check has none, while a job that also builds the PR's code has already given the attacker far more.

More useful than the race, and I only found this via the panel: the same redirect is available with no race at all. An in-tree symlink is refused after realpath, and the failure mode is reported, so the three states discriminate an out-of-tree target:

ln -s /etc/hosts         a.ts   ->  out-of-tree   (target exists)
ln -s /etc/no-such-file  b.ts   ->  absent        (target does not)
ln -s /var/root/x        c.ts   ->  unreadable    (permission denied)

Harmless while the caller owns the tree. A capability delta once a fork PR's tree is scanned in public CI. O_NOFOLLOW doesn't touch it, because realpath has already answered. Nothing to change in this PR, but it belongs in the same clause: "The working tree is the boundary, and it is enforced" currently says confinement is checked twice and then stops, and the honest version is that the pre-I/O refusal covers lexical escapes only.

One question. Do you want to take the check/Action wiring as a second PR, or should I plan on it? Genuinely either is fine and it doesn't affect this merge; I just want to know who's holding it. Happy to hand over notes if you want it, since there are a couple of sharp edges in there worth flagging before you start.

[aballiet]

Thank you for the thorough review, @mbeacom. The three merge blockers are addressed in 0989f0b:

1. Markers now require a dedicated comment line: the comment introducer leads the physical line and @adr leads the comment content. Scanning the branch now finds four marker-looking lines—the real dogfood declaration and three fenced documentation examples—down from 51.
2. Filesystem truncation now uses the observed byte count rather than decoding and re-encoding. BOM, invalid UTF-8, short reads, CR-only line endings, and severed-reference cases are covered.
3. declaredBy now belongs to an explain-only ExplainedDecision; the shared GoverningDecision and checkChanges remain marker-free, removing the dependency inversion.

The ADR now states the explain-only scope explicitly, including the current context.sh behavior, MCP boundary, symlink-state disclosure, broad affects behavior for raw paths, and the remaining realpath/open race.

I also fixed the human-output scan note on corpus-error paths and kept the public scanner's work bounded to the 8192-byte window.

I have left malformed-marker diagnostics as a non-gating follow-up; the current behavior is documented in the PR description.

I'd be happy to take the check/Action integration as a separate PR. Please send over the sharp-edge notes you mentioned.

Full build, typecheck, lint, schema parity, dependency checks, site build, and all 1,840 tests pass. When convenient, could you take another look?

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/core/src/markers/read.ts:89

• The new filesystem path is tested only through bun:test; the Node smoke job imports core but never calls readSourceMarkers or adr explain. Because published core/CLI artifacts are required to run under Node, a Bun-only regression here would pass CI. Please exercise a marker scan through the built Node artifact (ideally the built CLI) in scripts/smoke-node.mjs.

export async function readSourceMarkers(path: string, cwd = process.cwd()): Promise<SourceMarkerScan> {

packages/core/test/markers-scan.test.ts:248

• This rationale contradicts the behavior documented in ADR-0021 and read.ts: resolveAffects still evaluates the raw argument, so a sufficiently broad matcher may govern a traversing path. Only marker-derived governance is confined to the working tree. Please avoid claiming that the pattern half can never match.

 * The contract is the repo-relative path `resolveAffects` matches its globs against.
 * An argument that leaves the tree is not a stricter version of that contract but a
 * different one: the pattern half of `explain` can never match it, so the marker half
 * must not answer for it either.

[mbeacom]

TLDR; LGTM

I re-ran the three blockers against 0989f0b rather than reading the diff, and all three are genuinely closed.

Scanning the branch now finds four markers: your ADR-0021 dogfood on check/index.ts:1, and three fenced documentation examples. adr explain packages/core/src/markers/scan.ts reports no markers at all now, which was the case that bothered me most, and the real declaration still resolves and renders correctly.

On truncation I ran a matrix rather than the single case I sent you: the severed-reference cut swept across the boundary, files at exactly 8192 and 8193 bytes, a multi-byte character straddling the cut, CR-only and CRLF endings, a BOM, and a 9000-byte file with no newline anywhere in the window. No partial reference survives in any of them, and truncated tracks the observed byte count in every one. My BOM case reported truncated: false on the old head and reports truncated: true with the severed line dropped on this one. Threading bytesRead was the right call, and separating the internal bounded entry point from the public string-in one is cleaner than what I suggested.

declaredBy on an explain-only ExplainedDecision is better than what I asked for. check --json carries no marker keys, packages/ci/dist is byte-identical, and the default type parameter on bucketDecisions keeps the shared shape source-compatible instead of breaking it. The import direction is un-inverted.

The record reads honestly now. Writing the symlink-state disclosure and the realpath/open race into it, rather than describing them as closed, is the version I wanted and did not ask for well.

One residual, which is neither a gate nor a request. The three fenced examples do still attribute a decision: adr explain packages/cli/README.md reports ADR-0012 as an accepted governing decision on the strength of a documentation snippet. Same class as the original blocker, at three instances instead of fifty-one, all of them in our own docs, and excluding them needs fence tracking. I named four as the good outcome in my review, so I am not moving that line after the fact. I will open it separately.

And yes, please take the check/Action integration. Notes are in #100. The first two will shape the design rather than just the implementation, so they are worth reading before you start.

Thanks for the care on this one, particularly for correcting my own errors in the record instead of just copying them in. 🙌