Skip to content

feat: Scope derived-surface lockstep into three classes and guard the prose one (#132) - #217

Merged
mbeacom merged 3 commits into
mainfrom
feat/lockstep-derived-surfaces
Sep 23, 2026
Merged

mbeacom merged 3 commits into
mainfrom
feat/lockstep-derived-surfaces

Conversation

@mbeacom

@mbeacom mbeacom commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

What and why

Closes the scoping question in #132. Two drift incidents in a single PR (#129) — stale ADR
action-item checkboxes, then a MANIFEST.md that had drifted six records — came from
treating hand-maintained mirrors of corpus state as one problem. They are three problems,
and conflating them produces either an unbuildable feature or a gate that reports green
while checking nothing.

ADR-0040 (proposed) records the split and what follows:

# Class Example Derivable? Mechanism Status
1 Derived inventory MANIFEST.md table + counts Yes Generate + git diff --exit-code Shipped (#131)
2 Referential integrity A doc citing ADR-0021 as live Checkable, not generatable Two lint rules, one successor definition Marker half shipped (#187); prose half here
3 Implementation claims ADR action-item checkboxes Neither Process only Written into CONTRIBUTING.md here

The split follows @davesheffer's refinement on the issue, including the decision to keep
the two class-2 rules separate rather than fold them into one scanner, and to start the
inventory formatter repo-local rather than as public CLI surface. His offered #116 slice
shipped as #187; this is the other half he scoped.

What the new guard does

scripts/check-stale-adr-references.ts (bun run check:stale-refs) fails when a document
that speaks in the present tense cites a superseded, rejected, or deprecated record
without saying so. Acknowledgement is per paragraph or list item; for a superseded
record it must name the successor, because the word "superseded" alone does not tell the
reader where to go next. Fenced blocks and frontmatter are dropped, for the reason ADR-0023
stopped reading markers inside fences.

Scope is the load-bearing decision, and it is explicit rather than derived. docs/adr/,
CHANGELOG.md, specs/, plan.md and source are excluded because they narrate history
correctly — rewriting them would falsify the past. At today's corpus that is the difference
between one finding and 149 mentions.

No public surface is added

adr graph --format json already emits every node's status and every supersedes edge.
The public CLI is a semver commitment (ADR-0031) and its write surface stays two commands,
so the Spec Kit adapter's tested hook invariant is untouched. Marker findings keep the
exit-code denial ADR-0022 gave them deliberately — this guard fails only this repository's
own clean-clone-builds step. No --strict, no adr lint source scan, no schema change.

Observed failing before it counted as coverage (ADR-0016)

Run against the tree before the defect was fixed and before the step was wired in:

$ bun run check:stale-refs
check-stale-adr-references: Prose cites a record that is no longer live without saying so:
  site/src/content/docs/commands.mdx:304  ADR-0021 is superseded — name ADR-0022 here, or move the sentence to the past tense
A citation is acknowledged when the same paragraph or list item names the successor (ADR-0040).
Narration of history belongs in docs/adr/, CHANGELOG.md or specs/, which are not scanned.
$ echo $?
1

That was a real defect: the published commands page cited ADR-0021 as the authority for the
"nothing enters the record" guarantee, which ADR-0022 superseded. It is fixed here.

The positive control is AGENTS.md, which names ADR-0021 on one line and links ADR-0022
two lines later in the same sentence. It does not fire — a per-line window would have
failed a correct document, which is why the window is the paragraph.

A second drift, found by touching the thing it describes

clean-clone-builds' network-denial step count was already wrong by one before this
change, and this PR adds a step. It is stated in five places:

Location Said Actual before Now
observed-failing-register.md §4.6 — "the canonical statement" 15 / 14 16 / 15 17 / 16
spec.md FR-050 15 / 14 16 / 15 17 / 16
tasks.md T093 15 / 14 16 / 15 17 / 16
ci.yml step comment 15 / 14 16 / 15 17 / 16
clean-clone-offline/README.md (twice) 15 / 14 16 / 15 17 / 16

§4.6 declares itself "the single source for the exemption" and says the other four "point
here rather than restate it. Three restatements would be three places to drift." None
pointed; all restated, and one restates it directly below the sentence saying it does not.
All five are corrected, and the drift is recorded rather than tidied away, following that
section's own convention. ADR-0040 action item 7 carries the question of whether that
number should be derived instead — it would be class 1 of this very record.

Checklist

  • Commits are DCO signed off
  • If this changes a recorded decision, an ADR is added — ADR-0040, proposed
  • Schema unchanged (no packages/core/src/schema/ edit, no schema:emit needed)
  • packages/ci/src and @adrkit/core unchanged, so no packages/ci/dist rebuild
  • New behavior is covered, and observed failing first — the guard's failure is
    captured above; 111 unit tests cover the pure core, 20 of which were written red
    across the two review rounds below
  • bun run typecheck && bun test && bun run lint green (2947 pass, 0 fail), plus
    every check:* guard, adr lint (40 records, 0 errors), and emit:manifest
    idempotent

Review round: six scanner defects, all reproduced first

@davesheffer reproduced a real Windows failure on 969d12b (Bun 1.3.14) — the guard
reported dozens of findings inside docs\adr\, the one directory it exists to skip,
because node:path.relative() returns backslash paths there while SCANNED excludes with
forward slashes. All 68 tests passed while the guard failed, which is the more useful
half of the report: the scope test asserted startsWith('docs/adr/'), false for every
docs\adr\… path. Copilot review caught five more. Fixed in cec52d2, each observed
failing as a test first (ADR-0016) — 15 red against the previous head:

# Defect Direction Fix
1 Windows separators defeated every exclusion False positives across the whole corpus normalizeRelative(path, separator), separator injected so a POSIX suite can reach it
2 statSync followed symlinks; readdirSync recursed through them CI reads outside the worktree, or cycles Any symlink at or under a scanned path throws
3 Unterminated --- dropped the entire document Silent pass Skip frontmatter only when it closes
4 A closing fence accepted an info string False positive after an early close Closing fence must carry nothing after it
5 Extensionless site routes invisible Both — undetectable citations and unrecognized successors Anchor the link form on an adr/ segment
6 Successor picked by adr graph's localeCompare edge order Machine-dependent output Re-sort with compareCodeUnits

Two notes on the fixes:

  • 2 refuses rather than skips. A skip is the silent-pass failure the rest of the file
    exists to avoid, and a documentation tree has no reason to contain a symlink — the answer
    is to check the file in. Core's marker reader already refuses every symlink component.
  • 5 made another project's corpus newly matchable. docs/DISTRIBUTION.md demonstrates
    the MADR repository, whose 0005 is a different decision from this corpus's superseded
    0005. It is inert only because it sits inside a fence, so the fence rule is now doing
    load-bearing work it was not doing before. A test pins that.

Two smaller items from the same review: the failure footer told every author to name a
successor while the per-finding line for a rejected record asks for the status word — it
now states only the rules that apply, and CONTRIBUTING.md draws the same distinction.
check:stale-refs is added to DOCUMENTED_UNPROTECTED_ROUTES in check-gate-integrity.ts.

Second round: three more, in the fixes themselves

Re-review of cec52d2 found that two of the fixes above did not hold. Fixed in 28dc62b,
each reproduced by running it first:

# Defect Evidence
7 The symlink check examined only a path's final entry, so a symlinked ancestor (site/src/content) still let the walk read outside the root NO REFUSAL — read: [ "site/src/content/docs/leak.md" ]
8 The check ran after the exclusion, so a symlinked docs/adr passed unexamined — and adr graph's loader does follow a symlinked root ln -s /tmp/outside docs/adr && adr graph → "title": "Outside the worktree"
9 \badr/ is not a path-segment boundary — a word boundary sits after the hyphen in not-adr referencedIds("[other](/not-adr/0005-old/)") → ["0005"]

Every path component is now lstated from the root down and the first symlink refused
(core's lstatWithoutSymlink shape), the check precedes the exclusion, and the anchor is
(?<![\w-])adr/. Re-running all three escape shapes against the fixed walk refuses each
while a clean tree still collects.

One boundary stated rather than claimed: the corpus loader following a symlinked
docs/adr is @adrkit/core's behavior and is unchanged here — clean-clone-builds
already runs adr lint against the same tree before this step. That exposure is
pre-existing, not introduced; what changed is that this guard no longer contributes to it.
Worth its own issue if the loader should refuse a symlinked corpus root.

Notes for reviewers

  • The acknowledgement grammar is deliberately mechanical, not clever. A doc naming a
    record and its successor three paragraphs apart will fire. That is a stated trade-off in
    ADR-0040 under "How we would know this was wrong": if contributors start splitting
    sentences to appease it, the grammar is wrong.
  • terminalSuccessor mirrors core's terminalLiveSuccessor exactly, including the
    clause that makes a chain ending at a rejected/deprecated record resolve to nothing.
    Without it the failure message would advise citing a rejected record, which the same
    guard rejects on the next run. It is reimplemented over the CLI's JSON rather than
    imported, because exporting core's version would be consumer SDK surface under ADR-0031;
    tests pin the two to the same behavior.
  • Out of scope, observed not fixed: packages/core/src/markers/{types,index}.ts and a
    few other source comments still cite ADR-0021 as their authority. Source is deliberately
    not scanned — a code comment narrating history is correct, and a source file that wants
    to declare governance has @adr, which rule 1 already covers. Worth a separate look if
    anyone disagrees.
  • ADR-0040 is proposed, not accepted. Action item 6 exists so its own
    proposed-state qualifiers get updated in the acceptance PR — ADR-0037 lacked that item
    and needed Update ADR-0037 references now that it is accepted, not proposed #216 to clean up afterwards.

Closes #132

… one

Closes the scoping question in #132. Two drift incidents in one PR (#129)
— stale ADR action-item checkboxes, then a MANIFEST.md that had drifted six
records — came from treating hand-maintained mirrors of corpus state as one
problem. They are three, and they need three answers.

ADR-0040 (proposed) records the split and what follows from it:

- Class 1, derived inventory: generate, then `git diff --exit-code`.
  Already shipped as `emit-manifest.ts` (#131).
- Class 2, referential integrity: two rules sharing one definition of
  successor and nothing else. The `@adr` marker half already ships as
  `stale-marker` (#116 part A, ADR-0022). The prose half is new here.
- Class 3, implementation claims: process only, stated as the honest limit
  rather than faked. Automating it would be the evaluator theater ADR-0005
  warns about.

The split, and the decision to keep both class-2 rules separate rather than
fold them into one scanner, follow @davesheffer's refinement on the issue.

`scripts/check-stale-adr-references.ts` fails when a document that speaks in
the present tense cites a superseded, rejected, or deprecated record without
saying so. Acknowledgement is per paragraph or list item; for a superseded
record it must name the successor, because the word "superseded" alone does
not tell the reader where to go next. Fenced blocks and frontmatter are
dropped, for the reason ADR-0023 stopped reading markers inside fences. Its
successor walk matches core's `terminalLiveSuccessor` exactly, including the
clause that makes a chain ending at a rejected record resolve to nothing —
otherwise the failure message would advise a citation the guard then rejects.

Scope is the load-bearing decision, and it is explicit rather than derived:
docs/adr/, CHANGELOG.md, specs/, plan.md and source are excluded because
they narrate history correctly. At today's corpus that is the difference
between one finding and 149 mentions.

No CLI surface is added. `adr graph --format json` already emits every
node's status and every supersedes edge, the public CLI is a semver
commitment (ADR-0031), and its write surface stays two commands so the Spec
Kit adapter's tested hook invariant is untouched. Marker findings keep the
exit-code denial ADR-0022 gave them deliberately; this guard fails only this
repository's own clean-clone-builds step.

Observed failing before being counted as coverage (ADR-0016): the guard
found site/src/content/docs/commands.mdx citing ADR-0021 as the live
authority for the marker contract, which ADR-0022 superseded, and exited 1.
That line is fixed here. AGENTS.md — which names ADR-0021 and links ADR-0022
two lines later — is the positive control and does not fire.

Also corrects the `clean-clone-builds` network-denial step count, which was
already wrong by one before this change and would have been wrong by two
after it. It is stated in five places: observed-failing-register.md §4.6,
which declares itself canonical and says the others point at it rather than
restate it; FR-050 in spec.md; tasks.md T093; the ci.yml comment; and
clean-clone-offline/README.md, twice — the second directly below the
sentence saying it is not restated there. None pointed; all five were wrong.
All are corrected, and the drift is recorded rather than tidied away,
following that section's own convention. It is ADR-0040's argument arriving
from outside its scope, and ADR-0040 action item 7 carries the question of
whether that number should be derived instead.

Signed-off-by: Mark Beacom <m@beacom.dev>
Copilot AI lite review requested due to automatic review settings September 23, 2026 02:42
@github-actions

Copy link
Copy Markdown

Decisions governing this change

  • 0001 — Record architecture decisions as versioned markdown in git
    • via path: docs/adr/**
  • 0006 — License Apache-2.0 with a DCO and develop in a single monorepo
    • via path: CONTRIBUTING.md
  • 0007 — Isolate integrations as optional adapters and build only against public surfaces
    • via path: .github/workflows/**
  • 0010 — Use Bun as the package manager and test runner while publishing Node-targeted artifacts
    • via path: .github/workflows/**
    • via path: package.json
  • 0011 — Host the canonical JSON Schema at its $id on adrkit.dev
    • via path: site/**
  • 0014 — Stage phase-landing evidence across a three-rung validation ladder
    • via path: specs/**/spec.md
    • via path: specs/**/tasks.md
  • 0017 — Keep dependency audit scope explicit and release-scoped
    • via path: .github/workflows/ci.yml
  • 0025 — Ship badges as recipes over existing output, not a new CLI surface
    • via path: .github/workflows/ci.yml
    • via path: package.json
  • 0030 — Keep extension surfaces that carry a dependency tree outside this repository
    • via path: package.json
  • 0032 — Publish one lockstep OCI image after the coordinated release succeeds
    • via path: .github/workflows/ci.yml
  • 0033 — Select interactive graph presentation at the CLI boundary while preserving piped DOT
    • via path: site/src/content/docs/**
  • 0035 — Execute the gates that certify a pull request from the default branch
    • via path: .github/workflows/**
    • via path: scripts/**
  • 0037 — Treat generated knowledge systems as downstream read models, not decision authorities
    • via path: site/src/content/docs/**
  • 0039 — Derive a valid-time window from date and supersession, and resolve a git ref at the CLI boundary
    • via path: site/src/content/docs/commands.mdx

Active proposals touching this change

These are not yet ratified and do not bind this change:

  • 0040 — Keep derived surfaces in lockstep with three mechanisms matched to three classes of drift (proposed)
    • via path: .github/workflows/ci.yml
    • via path: AGENTS.md
    • via path: CONTRIBUTING.md
    • via path: MANIFEST.md
    • via path: scripts/check-stale-adr-references.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved scanner safety and correctness issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)
What changed in this PR

Adds ADR-0040’s three-class drift model and a repository-local stale ADR prose-reference guard, with CI integration and related documentation/count updates.

Changes:

  • Adds and tests check:stale-refs.
  • Integrates the guard into CI and contributor guidance.
  • Adds ADR-0040 and updates the manifest and changelog.
  • Corrects ADR references and network-denial evidence counts.
File Summary Final review notes
specs/​010-catalog-backstage/​tasks.md Updates network-denial counts. —
specs/​010-catalog-backstage/​spec.md Updates FR-050 counts. —
specs/​010-catalog-backstage/​evidence/​observed-failing-register.md Records count drift. —
specs/​010-catalog-backstage/​evidence/​negative-cases/​clean-clone-offline/​README.md Updates denial counts. —
site/​src/​content/​docs/​commands.mdx Corrects the ADR-0021 reference. —
scripts/​check-stale-adr-references.ts Implements stale-reference scanning. Critical (1): prevent symlink traversal. Moderate (1): match core’s successor-edge behavior. Moderate (1): normalize Windows paths. Moderate (2): correct contradictory remediation text. Moderate (2): fail closed on malformed frontmatter. Moderate (1): validate closing fences. Moderate (1): recognize extensionless ADR routes.
scripts/​check-stale-adr-references.test.ts Adds scanner tests. —
package.json Registers the new script. —
MANIFEST.md Updates generated inventory data. —
docs/​adr/​0040-keep-derived-surfaces-in-lockstep-with-three-mechanisms-matched-to-three-classes.md Documents the three-class decision. —
CONTRIBUTING.md Adds drift-prevention guidance. Nit (1): distinguish successor naming for superseded records from status-word acknowledgement for rejected/deprecated records.
CHANGELOG.md Records the guard and fixes. —
AGENTS.md Adds repository guard guidance. —
.github/​workflows/​ci.yml Runs the new CI guard. Nit (2): update or derive the explicit route inventory.

Comment thread scripts/check-stale-adr-references.ts Outdated
Comment thread scripts/check-stale-adr-references.ts
Comment thread scripts/check-stale-adr-references.ts Outdated
Comment thread .github/workflows/ci.yml
@davesheffer

Copy link
Copy Markdown
Contributor

Windows reproduction on PR head 969d12b (Bun 1.3.14):

$ bun run check:stale-refs
docs\adr\0005-deterministic-first-evaluator-with-declarative-escalation.md:29  ADR-0005 is superseded — name ADR-0027 here, or move the sentence to the past tense
...dozens more findings under docs\adr\...
error: script "check:stale-refs" exited with code 1

collectDocs() uses node:path.relative(), which returns backslash-separated paths on Windows. Its exclude entries use forward slashes (docs/adr), so the corpus exclusion never matches. The test named excludes the corpus and its generated site mirror also misses this: it checks startsWith('docs/adr/'), which is false for every docs\adr\... path.

Please normalize repository-relative paths to forward slashes before exclusion matching and returning DocFile.path, and assert that collected files contain no ADR corpus files on Windows. I verified bun test scripts/check-stale-adr-references.test.ts (68 pass) and bun run typecheck (pass); the guard itself exits 1.

@davesheffer

Copy link
Copy Markdown
Contributor

Follow-up proposal after the path fix above: add a small Windows smoke job to ci.yml for this repo-local guard.

Suggested scope: windows-latest, the existing pinned checkout and Bun setup actions, bun install --frozen-lockfile, bun test scripts/check-stale-adr-references.test.ts, and bun run check:stale-refs. That exact guard failed on my Windows checkout while its 68 focused tests passed, so the job would exercise the gap the current tests missed. I would include an assertion that collectDocs() actually excludes docs/adr/ using paths normalized across platforms.

The existing Ubuntu clean-clone-builds job can keep its network-denial guarantee and full suite. This would be a separate, focused job with no claim about network denial. If that scope fits, I can take it as a follow-up PR after the path fix lands.

@mbeacom mbeacom changed the title Scope derived-surface lockstep into three classes and guard the prose one (#132) feat: Scope derived-surface lockstep into three classes and guard the prose one (#132) Sep 23, 2026
@davesheffer reproduced a genuine Windows failure on PR head `969d12b` with
Bun 1.3.14: `bun run check:stale-refs` reported dozens of findings inside
`docs\adr\`, the one directory the guard exists to skip. `node:path.relative()`
returns backslash-separated paths there and `SCANNED` writes its exclusions
with forward slashes, so no exclusion ever matched. The 68 tests that shipped
all passed while the guard itself failed — the scope test asserted
`startsWith('docs/adr/')`, which is false for every `docs\adr\…` path.

Copilot review on this PR caught five more, all reproduced before being fixed.

Each fix is observed failing first (ADR-0016); 15 tests were red against the
previous head before a line of the scanner changed.

1. **Windows separators.** `normalizeRelative(path, separator)` takes the
   separator as a parameter defaulting to `node:path.sep`, mirroring core's
   `normalizeMarkerPath` — including its reason for being conditional, that a
   backslash is an ordinary filename character on POSIX. Parameterizing it is
   what lets a POSIX-only suite reach the bug at all, which is the gap Dave
   identified rather than just the bug.

2. **Symlink traversal.** `statSync` followed links and `readdirSync` then
   recursed through whatever it reached, so a pull-request-authored tree could
   send the guard outside the worktree or into a cycle. Any symlink at or under
   a scanned path now throws, as core's marker reader already does. It refuses
   rather than skips: a skip is the silent pass the rest of the file exists to
   avoid, and a documentation tree has no reason to contain one.

3. **Unterminated frontmatter dropped the document.** The loop ran to EOF then
   stepped one past it, so the block loop never executed and every citation in
   that file was invisible while the command reported a clean run. Frontmatter
   is now skipped only when its closing delimiter is found; a lone `---` on
   line 1 is a legal thematic break, so the document is scanned, not failed.

4. **A closing fence accepted an info string**, so ```js … ```py closed the
   block early and the code after it was scanned as prose.

5. **Extensionless site routes were invisible.** `gen-adr-pages.ts` emits
   `/adr/<slug>/` with no extension and eight pages already link that way, so a
   citation written as a site route was undetectable and a successor cited that
   way never acknowledged one. The link alternative is now anchored on an `adr/`
   segment, which also removes a false positive the unanchored form had —
   `notes/2026-09-22-release.md` read as a citation of record `2026`.

6. **Successor selection rested on locale.** `adr graph` sorts edges with
   `localeCompare`; first-wins over that order would make the successor this
   guard names depend on the machine that ran it, which `emit-manifest.ts`
   refuses to do for the same reason. The index is re-sorted with
   `compareCodeUnits`. A lint-clean corpus has exactly one successor per record
   anyway — `supersession-consistent` is an error rule and this guard throws on
   a non-zero `adr graph` exit — so the sort is what makes the untested corpora
   deterministic.

Also: the failure footer told every author to name a successor while the
per-finding line for a `rejected` record asks for the status word, sending
contributors after a successor those states do not have; it now states only the
rules that apply to the findings reported, and `CONTRIBUTING.md` draws the same
distinction. `check:stale-refs` is added to `DOCUMENTED_UNPROTECTED_ROUTES` in
`check-gate-integrity.ts`, whose inventory of `bun run`-reachable gates was
left incomplete by the new step.

One behavior worth recording: anchoring on `adr/` made another project's corpus
newly matchable, and `docs/DISTRIBUTION.md` demonstrates the MADR repository,
whose `0005` is a different decision from this corpus's superseded `0005`. It
is inert only because it sits inside a fence, so the fence rule is now doing
load-bearing work it was not doing before. A test pins it.

Signed-off-by: Mark Beacom <m@beacom.dev>
@mbeacom

mbeacom commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner Author

Thanks @davesheffer.

Fixed in cec52d2.

The bug. Confirmed exactly as you described. node:path.relative() returns docs\adr\0005-….md on Windows, SCANNED writes its exclusions with forward slashes, and excludes therefore matched nothing — so the guard reported the corpus it exists to skip.

The test gap, which was the real finding. You are right that startsWith('docs/adr/') could never catch this, and "68 tests pass while the guard fails" is the part worth fixing structurally. Rather than assert the outcome on one platform, the normalization is now a pure function that takes the separator as a parameter, defaulting to node:path.sep:

export function normalizeRelative(path: string, separator: string = sep): string {
  return separator === '\\' ? path.replaceAll('\\', '/') : path;
}

That is core's normalizeMarkerPath shape, including its reason for staying conditional — on POSIX a backslash is an ordinary filename character, and rewriting it unconditionally would report a path that does not exist. With the separator injected, the POSIX suite asserts the Windows behavior directly:

  • normalizeRelative('docs\\adr\\0005-x.md', '\\') → 'docs/adr/0005-x.md'
  • excludes(normalizeRelative('docs\\adr\\0005-x.md', '\\'), ['docs/adr']) → true
  • the scope test now also asserts no collected path contains a backslash
  • plus segment-awareness, so docs/adr excludes docs/adr/x.md but not docs/adrs/x.md or docs/adr-notes.md

Copilot found five more in the same round — symlink traversal, an unterminated --- dropping a whole document, a closing fence accepting an info string, extensionless /adr/<slug>/ site routes being invisible in both directions, and successor selection resting on adr graph's localeCompare edge order. All six are in the PR body's review-round table, and all were observed failing as tests first (15 red against 969d12b).

On the Windows smoke job: yes, please, and thank you for offering. Your argument stands on its own — the parameterized test closes the bug, but not the class, and only a real windows-latest run proves the walk itself behaves. Your scope reads right to me: pinned checkout + Bun setup, bun install --frozen-lockfile, bun test scripts/check-stale-adr-references.test.ts, bun run check:stale-refs, no network-denial claim, clean-clone-builds unchanged.

Two things worth knowing before you start, neither a blocker:

  1. It is a second gate change, so it needs its own gate-change-acknowledged label under ADR-0035 — expected, just not a silent merge.
  2. collectDocs() now throws on any symlink at or under a scanned path. Worth a Windows assertion of its own: symlink semantics differ there, and I have only tested the refusal on POSIX.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity · 1 Low severity

Open (4)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify acknowledgement rules for superseded and rejected citations

CHANGELOG.md:19

This changelog entry describes one acknowledgement rule for all three statuses, but the implementation has two: superseded citations need the successor, while rejected/deprecated citations need the status word. As written, it tells readers to add a successor to rejected/deprecated citations, which does not satisfy findStaleReferences and contradicts the failure output. Distinguish the two rules here.

Comment thread scripts/check-stale-adr-references.ts
Comment thread scripts/check-stale-adr-references.ts Outdated
Comment thread scripts/check-stale-adr-references.ts Outdated
Copilot's re-review of `cec52d2` found three, all reproduced before being
fixed and all red as tests first.

1. **The symlink check examined only a path's final entry.** `lstatSync` on a
   constructed path looks at its last component; the OS still traverses every
   ancestor, following links. Replacing `site/src/content` with a symlink let
   the walk read `outside/docs/leak.md` and report it as
   `site/src/content/docs/leak.md` — outside the root entirely, which is the
   boundary the previous commit claimed to have established. Every component
   from the root down is now `lstat`ed, as core's `lstatWithoutSymlink` does.

2. **The check ran after the exclusion**, so a symlinked `docs/adr` was never
   examined. It is not read, but it is inside the boundary this guard claims,
   and `main()` hands the same tree to `adr graph` — whose corpus loader does
   follow a symlinked root, confirmed by running it. Checking before the
   exclusion is what lets this refuse before the CLI is spawned. The loader's
   own behavior is core's boundary and is unchanged; `adr lint` already loads
   the same corpus earlier in the same job, so that exposure is not new here.

3. **`\badr/` is not a path-segment boundary.** A word boundary exists between
   the hyphen and the `a` of `not-adr`, so `/not-adr/0005-old/` read as a
   citation of this corpus's superseded 0005 — an unrelated route failing a
   required check. The anchor is now a negative lookbehind rejecting a
   preceding word character or hyphen, which leaves the segment starts that
   actually occur: start of string, `/`, and `./`.

Verified by re-running each escape against the fixed walk: a symlinked
ancestor, a symlinked excluded path, and a symlinked leaf are all refused, and
a clean tree still collects.

Signed-off-by: Mark Beacom <m@beacom.dev>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Three moderate scanner findings and one documentation nit remain unresolved.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Resolved since last review (3)

@mbeacom mbeacom self-assigned this Sep 23, 2026
@mbeacom mbeacom added the gate-change-acknowledged A maintainer has seen and accepted this PR's change to the CI gate surface (ADR-0035) label Sep 23, 2026
@mbeacom
mbeacom merged commit e9d950e into main Sep 23, 2026
20 of 23 checks passed
@mbeacom
mbeacom deleted the feat/lockstep-derived-surfaces branch September 23, 2026 12:39
@mbeacom

mbeacom commented Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Tracking the Windows smoke job as #218 so it does not live only in this thread — scope as you proposed it, plus the two Windows-specific wrinkles I flagged above (the ADR-0035 label, and collectDocs() now refusing symlinks component-wise, whose semantics differ enough on Windows to need its own assertion or a visible skip).

@davesheffer it is noted there as yours. I could not assign you directly — you are not a collaborator on this repository — so the issue body is the record instead. Happy to add you if you would rather have the assignment.

mbeacom pushed a commit that referenced this pull request Sep 25, 2026
check:stale-refs shipped in #217 with 68 passing tests and failed on the
first Windows checkout that ran it: node:path.relative() returned
docs\adr\..., no exclusion matched, and the guard reported the corpus it
exists to skip. The path fix landed there; this covers the other half.

- New `windows-stale-refs-smoke` job on windows-latest: pinned checkout
  and Bun, `bun install --frozen-lockfile`, the focused test file, then
  `bun run check:stale-refs`. Unfiltered, named as covering this guard
  only, no network-denial claim; clean-clone-builds is unchanged.
- Walk-level tests. The existing assertions (`startsWith('docs/adr/')`,
  `not.toContain('docs/adr/0001-x.md')`) are both satisfied by a
  backslash path, so dropping normalizeRelative from the walk was caught
  by no test on any platform. Now: no collected path contains a
  backslash, and the excluded-directory fixture asserts the exact list.
- Symlink refusal tests skip only when the process cannot create a
  symlink, print that they skipped, and fail instead under
  ADRKIT_REQUIRE_SYMLINKS=1, which the job sets. A win32-only test pins
  that a directory junction is refused too.
- DOCUMENTED_UNPROTECTED_ROUTES names the new job, which also reaches
  check:stale-refs through `bun run`.

Observed failing first (ADR-0016), on Windows / Bun 1.3.14, with
normalizeRelative removed from the walk: the new suite fails 2 of 114
(both new walk-level tests); the previous suite passes 111 of 111; the
guard exits 1 with the original docs\adr\ findings.

Signed-off-by: davids <davesheffer@users.noreply.github.com>
Co-authored-by: davids <davesheffer@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate-change-acknowledged A maintainer has seen and accepted this PR's change to the CI gate surface (ADR-0035)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scope how derived surfaces stay in lockstep with the corpus (CLI, workflows, guards)

3 participants