ci: close twenty-six gate bypasses and test the exemption mechanism by Shepdesign · Pull Request #23 · Shepdesign-LLC/NetRelish

Shepdesign · 2026-09-24T18:45:56Z

What

PR #22 landed the non-negotiable-5 gate. Review caught two bypasses in it as it was merging, so they are on main, unfixed. Those are closed here — and so are twenty-three more, found by Copilot reviewing this very PR across eleven rounds, which is itself the argument for the test suite.

Sources/ is untouched.

File	What changed
`scripts/codeql_gate.py`	New. The gate, lifted out of the workflow heredoc, with twenty-six fail-open holes closed.
`scripts/tests/test_codeql_gate.py`	New. 110 tests. One per named hole, plus the scanner's edges.
`.github/workflows/codeql.yml`	The heredoc becomes one line: `python3 scripts/codeql_gate.py .codeql-results`.
`.github/workflows/ci.yml`	New `CodeQL gate self-test` job — Ubuntu, runs the suite on every push.
`docs/adr/0007-direct-build-updates.md`	Renamed from `0006-`. Two ADRs both claimed 0006.

Twenty-six ways past the gate, all closed

Each one approves a call that nobody approved. That is the direction that matters — a gate that wrongly blocks costs an explanation; a gate that wrongly approves costs the rule its meaning. Holes 4, 5 and 6 were reproduced live against this checkout before being fixed. Holes 4-26 were found by review of this branch, and 4 onward were each reproduced live before being fixed. Two are mine in a sharper sense: 9 I introduced while fixing 2 and defended on the thread before checking it, and 12 falsifies a claim I put in this description and called "a fact rather than a hope".

#	The hole	Found by
1	`/* // NETRELISH-ALLOW-ENDPOINT: adr-0006 */` — `line.partition("//")` scanned past an embedded `//`, so a commented-out block approved the line below it	review of #22
2	One marker approved every finding on its line, so a second call beside an approved one got in free	review of #22
3	A `//` inside a string literal ahead of the marker — the hole #22's own docstring called unclosable	closed as a consequence
4	`#/ // NETRELISH-ALLOW-ENDPOINT: adr-0007 /#` — extended regex literals were not a tracked context, and extended regex ignores whitespace, so that spelling is natural rather than contrived	Copilot, on this PR
5	A trailing marker also served as the comment "above" the next line — #2 wearing a newline	Copilot, on this PR
6	`let s = "\("// NETRELISH-ALLOW-ENDPOINT: adr-0007")"` — interpolation returns to code inside a string, and flat state read the nested quote as the outer closer	Copilot, on this PR
7	`docs/adr/NNNN-.md` was globbed without checking what matched, so a directory* named `0007-placeholder.md/` satisfied a citation with no decision written anywhere inside it	Copilot, on this PR
8	A missing SARIF `startLine` defaulted to 1, so a finding with no known location was handed line 1 and a marker sitting there approved it	Copilot, on this PR
9	The columnless dedup key included the message — and two calls on one line almost always carry the identical message, so they merged into one finding and one marker approved both	Copilot, on this PR
10	`is_file()` follows symlinks, so `docs/adr/0007-anything.md -> /etc/hosts` counted as the written decision	Copilot, on this PR
11	SARIF named the file whose comments decide approval, and nothing constrained it. An absolute `file:` URI or a `..` traversal escaped the checkout and was read — a location pointing at any file on the runner carrying a valid marker was approved	Copilot, on this PR
12	`let r = /\//` — a bare regex's escaped slash and its own closing delimiter are textually `//`, so the rest of the line was read as a comment	Copilot, on this PR
13	The extended-regex terminator used `find("/#")`, which matched the `/` of an escaped `\/` and ended the literal early, handing the rest of the pattern to the code path	Copilot, on this PR
14	SARIF picks the path spelling, and the raw string was the grouping key — so `Sources/A.swift` and `Sources/../Sources/A.swift` split one physical line into two groups of one, defeating the shared-line rule	Copilot, on this PR
15	ADR citations checked the matched file but never its parent, so `docs/adr -> /tmp/decisions` returned ordinary regular files from outside the checkout	Copilot, on this PR
16	The citation pattern was not token-bounded — `\d{4}` matched the first four digits of `adr-00070` and captured `adr-0007`, so a malformed citation rode in on a real ADR	Copilot, on this PR
17	Absolute SARIF paths were rebased against `Path.cwd()`, not the checkout being judged. With `--repo-root` elsewhere, a finding in one checkout was approved by a marker in another	Copilot, on this PR
18	`locatable` asked whether columns were present, never whether they were positions — two findings both at `startColumn: 0` collapsed into one, and a single marker approved both	Copilot, on this PR
19	Only `file:` was parsed as a URI, so `http://evil` fell through to `Path` and became the relative path `http:/evil` inside the checkout	Copilot, on this PR
20	Columns were validated individually but never as a range, so `startColumn: 10, endColumn: 5` counted as a position and two findings carrying it collided — hole 18 through a gap hole 18's fix did not cover	Copilot, on this PR
21	The finding identity omitted `endLine`, so two distinct multi-line regions sharing a start and an end column collapsed into one	Copilot, on this PR
22	A present-but-empty SARIF read as clean. `{"runs": []}` produced no gate results, and "no results" was indistinguishable from "no findings" — the gate printed `clean` and exited 0	Copilot, on this PR
23	A bare regex was still ordinary code, and its pattern text carries the punctuation the scanner steers by — in `"\(/[)]"/) // …"` the regex's `)` closed the interpolation and its `"` closed the string	Copilot, on this PR
24	Hole 23's fix read the previous character, so after `return` it saw a letter, called it a value, and left the regex's punctuation live — hole 23 one keyword away from where it was fixed	Copilot, on this PR
25	A present-but-malformed `endLine` was indistinguishable from an absent one, so a finding with broken coordinates shared an identity with a sound single-line one	Copilot, on this PR
26	`file://attacker/<checkout>/Sources/A.swift` names a file on another host, but the authority was discarded — the path resolved against this checkout and was approved by the local marker	Copilot, on this PR

// #5, before:
_ = try await URLSession.shared.data(from: a)  // NETRELISH-ALLOW-ENDPOINT: adr-0007
_ = try await URLSession.shared.data(from: b)  // ← also approved. by that marker.

How they are closed

Not with another string match — that is what produced holes 1, 3 and 4. A marker now only counts inside genuine line-comment text, decided by scanning the file once while tracking every lexical context that can hide or reveal a //: nested block comments (Swift's nest), string literals, raw strings and extended regex literals at any hash count, the multiline form of each, and string interpolation.

Hole 6 is why those contexts are a stack rather than a few flags. \( returns to code inside a literal, that code can open another string, and that one can interpolate again — nesting flat state cannot express. Each construct pushes, its terminator pops, and interpolations count their own parentheses so \(f(g(x))) ends at the right one. A // is recorded only when the stack is empty, which is the one place a genuine comment can be.

Unterminated constructs fail closed for free: an unclosed /*, string or #/ swallows the rest of the file, so nothing below it can be approved.

I previously wrote here that the bare /.../ form needed no handling — "a fact rather than a hope", because two unescaped slashes would end the literal at the first. True, and beside the point: the literal's own ending supplies the second slash. Hole 12. What actually makes it safe is that an escape in code is now opaque, so the closing delimiter has nothing left to pair with, and regex never has to be told apart from division.

Approval is now keyed to the call, not the line. A line carrying more than one gate finding fails closed, and the line above is offered as a home for a marker only when it is not itself flagged.

Deduplication is now split by what can be distinguished. Findings with columns are deduplicated by position — a position is a call site, the message is only description. Findings without columns are never deduplicated at all, because nothing about them tells one call from another; keeping every occurrence can only over-count, which blocks. Hole 9 was the first attempt at this getting it backwards.

A citation must resolve to a regular file that actually lives in the checkout — not a directory, not a symlink out of the repository. A finding with no usable startLine is a gate error rather than a guess at line 1, because an unlocatable finding cannot be exempted at all. And every source path is resolved and required to stay inside the checkout before it is read, so SARIF cannot point the gate at evidence that was never committed here.

The duplicate ADR — flagging this one for you

docs/adr/ held two ADRs both numbered 0006:

0006-direct-build-updates.md        accepted 2026-09-23
0006-identity-lives-in-keychain.md  accepted 2026-09-21

The gate resolves a citation by globbing docs/adr/NNNN-*.md, so adr-0006 named two unrelated decisions — it could not be cited at all. Two changes: the gate now fails closed on an ambiguous number and says which files clash, and the later of the two is renumbered to 0007 (the keychain ADR of 2026-09-21 keeps 0006). Both references in docs/RELEASING.md are updated; the ADR 0006 mentions in Sources/ and project.yml are all about identity and are correct as they stand.

Settled: Ryan's call is that the keychain ADR keeps 0006, which is what this branch already committed — so nothing changed on the back of it. The ADR 0006 references in Sources/App/NetRelishApp.swift, Sources/Vault/VaultStore.swift, Sources/Vault/Identity.swift and project.yml are all about identity and remain correct; docs/superpowers/specs/2026-09-21-vault-me-card-design.md cites the keychain ADR by filename and is unaffected. A test asserts the clash cannot come back.

Demo

This is CI infrastructure, so the demo is the gate itself, run exactly as the workflow runs it — python3 scripts/codeql_gate.py .codeql-results against a real file in this checkout:

Case	Expected	Got
Gate hit, no marker	blocks	`exit=1` ✅
Real `//` marker citing a real ADR	approves	`exit=0`, `::notice … approved endpoint (adr-0007)` ✅
Hole 1 — marker in `/* … */`	blocks	`exit=1` ✅
Hole 2 — two calls on one approved line	blocks	`exit=1`, "2 gate findings share this line" ✅
Hole 5 — trailing marker, second call below	approves line 1 only	`exit=1`, line 2 blocked ✅
Hole 6 — marker in an interpolated nested string	blocks	`exit=1` ✅

And the suite, which is what CI runs:

$ python3 -m unittest discover -s scripts/tests
Ran 110 tests in 0.051s
OK

They cover: all twenty-six holes end to end; string literals, raw strings (#"…"#, ##"…"##), extended regex literals (#/…/#, ##/…/##), interpolation (nested, \#( raw, doubly nested, multiline, nested parens, and \\( not opening one), the multiline and unterminated form of each, nested block comments, #if/#Preview not being mistaken for any of them, line numbering across multiline constructs; absent, ambiguous, directory-backed, symlinked and symlinked-parent ADR citations; source paths escaping the checkout by absolute URI, .. traversal and tracked symlink, and a traversing URI that lands back inside being canonicalised; two spellings of one path sharing a line; a bare regex's closing delimiter and a real comment after one; relative, file:/// and percent-encoded SARIF URIs; missing SARIF; missing and invalid start lines; unreadable sources; cross-file deduplication with matching and differing messages, with and without columns; and main()'s exit codes.

Why the tests run on Ubuntu, in `ci.yml`, and not in the CodeQL workflow

Deliberate. Analyze (swift) still cannot build this project — the swift-plugin-server EBADARCH failure from #22 is unchanged and untouched here, and still fails at Build both flavours, step 7. A gate whose only proof of correctness lives inside the job that is currently red is not proven at all. The suite is pure Python, touches no Swift, and runs in seconds on every push regardless.

What this still does not prove

The same limit as #22, undiminished: the queries have never been observed firing on real Swift. This PR proves the exemption mechanism behaves correctly given SARIF. It does not prove CodeQL produces that SARIF, because CodeQL still cannot build the project. Proving that needs the canary re-added, watched, and removed once the tracer is fixed upstream.

What has changed is that the enforcement half is now checked by something other than someone reading it carefully — which, on the evidence of twelve more holes surfacing across five review rounds — one introduced by a fix in this very branch, another falsifying a claim made in this description — was never going to be enough.

Twenty-eight enforcement defects across this mechanism's life, not one of them in its design. The module header lists them, because the ratio is the point and the number alone does not carry it.

Hole 22 deserves singling out: it is the exact failure this module's own docstring claims to prevent, sitting in the module. The code refuses to read a missing SARIF as clean and explains why. A present SARIF that says nothing is the same false assurance with an extra step, and I never closed the gap between them.

And the recurring shape is a fix that is correct about what it addresses and silent about the rest of its category — five of the twenty-three were produced by fixing another. Hole 20 is hole 18 through a gap hole 18's own fix left: I validated the two column values and never the relationship between them. Hole 23 is the same against hole 12 — I made a bare regex's delimiter safe and said nothing about its contents, then argued in this description that the form needed no tracking at all. Hole 24 is then the same against hole 23, one keyword away from where I had just fixed it. And hole 26 is the same against hole 19: I added a scheme check to guard against a URI that does not name a local file, and never looked at the authority field beside it — the gap was one line from the fix.

That is the number to weigh when deciding how much this mechanism can be trusted. It is not that the fixes were wrong; each was correct about the case in front of it. It is that "correct about this case" kept reading as "closed", to me and in these notes, five times.

Read the scanner in particular with that in mind. It is a heuristic that has now been wrong five times about what counts as a comment, and every known case is pinned by a test — but that is the only claim the evidence supports. It is not a Swift parser and does not become one by surviving another round.

Hole 23's fix has a second direction worth knowing about, because it is the one that could bite quietly. Treating every / as a regex opener would read a/b // NETRELISH-ALLOW-ENDPOINT: adr-0007 as a literal running up to the comment's own slashes, losing a real marker and blocking a legitimate exemption. So it follows Swift's rule — regex where an expression is expected, division after a value — and five of that fix's seven tests guard that direction rather than the hole.

One caveat that belongs in front of you, not in a thread

Hole 22's fix requires a SARIF to attest that the gate query ran, by naming the rule in tool metadata or in a result. That encodes an expectation about CodeQL's SARIF output which I cannot verify, for the same reason nothing else here can be: CodeQL cannot build this project. If CodeQL omits rules that produced no results, this will block every clean run. That is loud, immediate and fail-closed — the right direction to be wrong in, and the alternative is a silent pass — but it is an assumption, and it should be the first thing checked when the traced build works again.

The sharpest of the twenty is worth stating on its own. Hole 17 let a finding in one checkout be approved by a marker in a different checkout, and printed approved endpoint (adr-0007) against a plausible-looking path while doing it. Nothing about the output looked wrong. That is the failure mode this whole mechanism is supposed to make impossible, and it survived five rounds of review before anyone saw it.

I am not claiming round twelve is empty. I have made a version of that claim after each of the last eleven.

Rule check

Non-negotiable 5 — strengthens its enforcement. No app behaviour, no endpoint, Sources/ untouched. The contents: read scope on both workflows is unchanged.

Brand check

None. No UI, no assets, no tokens.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Generated by Claude Code

The non-negotiable-5 gate had two bypasses, both found by review as #22 was merging, both live on main: 1. `/* // NETRELISH-ALLOW-ENDPOINT: adr-0006 */` was accepted, because `line.partition("//")` scanned whatever followed an embedded `//`. A commented-out block containing a marker silently approved the line below it. 2. Approval was keyed to the SARIF startLine, so one marker approved every finding on that line. Adding a call beside an approved one got past the gate. Both are now closed, and a third hole the previous implementation documented as unclosable — a `//` inside a string literal ahead of the marker — is closed with them, because the fix is a real scanner rather than another string match. A marker now only counts inside genuine line-comment text, decided by scanning the file once while tracking the three contexts that can hide or reveal a `//`: nested block comments, string literals, and raw strings of any hash count, multiline forms included. Unterminated constructs fail closed for free. Shared lines fail closed: a marker names a line, not a call, so a line carrying more than one gate finding cannot be approved by one. Findings are deduplicated on position first, so the same result appearing in two SARIF files is still one finding rather than a false shared line. Every defect this mechanism has had — five now — has been in its enforcement rather than its design, and each was caught by a human reading it, or not caught at all, because a heredoc inside a workflow cannot be run. So the logic moves to scripts/codeql_gate.py with 44 tests beside it, and ci.yml gains a Ubuntu job that runs them on every push — deliberately outside the CodeQL workflow, which cannot build this project on the current xcode-27 image. A gate whose only proof of correctness lives inside the job that is currently red is not proven at all. Also renumbers one of two ADRs that both claimed 0006. The gate resolves a citation by globbing docs/adr/NNNN-*.md, so `adr-0006` named two unrelated decisions and could not be cited at all; it now fails closed on ambiguity, and a test asserts the repo never reintroduces the clash. The later of the two (direct-build updates, 2026-09-23) becomes 0007; the keychain ADR of 2026-09-21 keeps 0006. Sources/ is untouched. No app behaviour, no endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Critical gate-bypass findings and additional workflow/documentation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1 · 1

Open (4)

Skip markers inside Swift extended regex literals · New
Prevent inline markers from exempting the next line · New
Exclude message text from finding deduplication keys · New
Update stale ADR reference from 0006 to 0007 · New

What changed in this PR

Extracts and tests the CodeQL gate, closes known exemption bypasses, adds CI self-tests, and renumbers a duplicate ADR.

Changes:

Adds the standalone gate and 44-test suite.
Updates CodeQL/CI workflow wiring and Python artifact ignores.
Renumbers the ADR and updates release documentation.

File	Description
`scripts/tests/test_codeql_gate.py`	Gate and scanner test suite
`scripts/codeql_gate.py`	Standalone CodeQL enforcement gate
`docs/RELEASING.md`	ADR reference documentation
`docs/adr/0007-direct-build-updates.md`	Renumbered ADR
`.gitignore`	Ignores Python artifacts
`.github/workflows/codeql.yml`	Runs the extracted gate
`.github/workflows/ci.yml`	Adds CI self-tests

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot found three, all correct, all failing open — which is the direction that matters. Same pattern as the five before them: the design was right, the enforcement had a gap. 1. Extended regex literals. `#/ ... /#` at any hash count was not a tracked context, so `#/ // NETRELISH-ALLOW-ENDPOINT: adr-0007 /#` read as a comment and approved the line below. Extended regex ignores whitespace, so that spelling is natural rather than contrived. Now skipped to its matching delimiter, unterminated included. The bare `/.../` form needs no handling: two unescaped slashes would end the literal at the first, so `//` cannot occur inside one. 2. A trailing marker also served as the comment "above" the next line — the shared-line bypass wearing a newline: _ = try await URLSession.shared.data(from: a) // ...ALLOW...: adr-0007 _ = try await URLSession.shared.data(from: b) Both were approved by one exemption. The line above is now offered only when it is not itself flagged; the standalone-comment-above form is untouched, since a line holding only a comment is never a finding. 3. The message was part of positional identity, so the same call site reported with different wording in two SARIF files split into two findings and blocked a valid exemption. Columns now settle identity when present. When they are absent the message stays in the key, because nothing else can tell two calls on a line apart — splitting one finding blocks, merging two approves, and only one of those errs safely. Also fixes a reference this branch missed: the table in docs/RELEASING.md still pointed the direct-build decision at "ADR 0006", which after the renumber is the unrelated Keychain ADR. The remaining 0006 references in Sources/ and project.yml are all about identity and are correct as they stand. Eleven regression tests, one per hole and one per edge the fixes touch. 55 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

The scanner can misinterpret string interpolation and incorrectly approve a gate finding.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Model Swift string interpolation and nested lexical state · New
Exclude message text from finding deduplication keys

Resolved since last review (3)

Prevent inline markers from exempting the next line
Skip markers inside Swift extended regex literals
Update stale ADR reference from 0006 to 0007

Sixth hole, same class as the other five, found by review. It fails open. Swift interpolation returns to CODE inside a string literal, that code can open another string, and that one can interpolate again. The scanner held string state in three flat variables, so a nested opening quote was read as the outer string's closer and the scan fell out into "code" mid-literal: let s = "\("// NETRELISH-ALLOW-ENDPOINT: adr-0007")" That recorded a line comment and approved the call below it. Flags cannot express nesting, so the contexts are now a stack. Every construct pushes and its terminator pops: strings and raw strings at any hash count, their multiline forms, extended regex literals, and interpolations — which also count their own parentheses, so `\(f(g(x)))` ends at the right one rather than the first. `\(` is tested before the generic escape, since it is a prefix of it. A `//` is recorded only when the stack is empty. One inside an interpolation would comment out the closing paren and quote, so it cannot appear in code that compiles, and code that does not compile is code CodeQL never flagged. Skipping it there costs nothing and cannot approve anything. Newline resync now pops only the directly-enclosing single-line string. A newline inside an interpolation is legal within a multiline literal, and guessing wrong would resync out of a construct that is genuinely open; not resyncing merely swallows more of the file, which approves nothing. Nine regression tests: nested, raw-string (`\#(`), doubly nested and multiline interpolations; a comment genuinely following one; nested parens; `\\(` not opening one; a string opened inside an interpolation still hiding a marker; and the whole thing end to end through the gate. 64 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Four unresolved gate-enforcement findings remain, including three critical issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3

Open (3)

Ignore directory matches when locating ADR documents · New
Prevent columnless findings from overwriting each other · New
Missing SARIF start lines must fail closed · New

Resolved since last review (2)

Model Swift string interpolation and nested lexical state
Exclude message text from finding deduplication keys

All three from review, all high, all approving a call nobody approved. 1. `docs/adr/NNNN-*.md` was globbed without checking what matched. glob returns directories, so a directory named `0007-placeholder.md/` satisfied a citation with no written decision anywhere inside it — an approval backed by nothing. Matches are now filtered with is_file(). 2. A missing SARIF `startLine` defaulted to 1. A finding whose location is unknown was therefore handed line 1, and a marker that happened to sit there approved it. An unlocatable finding cannot be exempted at all: it is now a gate error, which blocks, and no location is guessed. Non-integer and non-positive values are rejected the same way. 3. The columnless deduplication key included the message, and I defended that on the review thread with an argument that does not hold. Two separate calls on one line almost always carry the IDENTICAL message, so the key merged them into one finding and a single marker approved both — the shared-line bypass coming back through the door I had just closed, in the commit that closed it. Findings with columns are still deduplicated by position, because a position is a call site and the message is only description. Findings without columns are now never deduplicated, since nothing about them can tell one call from another. That can only over-count, which blocks. Five regression tests: a directory matching an ADR citation, and a real ADR beside one; a missing startLine and an invalid one; and two columnless findings sharing a line with the same message. Verified the directory case is not vacuous — glob does return it. 69 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved gate bypass and path-validation findings remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Reject ADR symlinks outside the checkout · New
Constrain SARIF source paths to the repository · New

Resolved since last review (3)

Missing SARIF start lines must fail closed
Prevent columnless findings from overwriting each other
Ignore directory matches when locating ADR documents

Previously missed (2)

In code that hasn't changed since last review

Update obsolete defect count in workflow comment

.github/workflows/codeql.yml:164

This workflow comment repeats the obsolete claim that the gate has had five defects, while the interpolation bypass is also handled and tested in this PR. Update the count so the workflow's explanation does not under-report the mechanism's known failure modes.

Update module defect inventory with interpolation bypass

scripts/codeql_gate.py:24

This module documentation still says there have been only five defects and lists only five, but the implementation and this PR also close the interpolation bypass. Keeping the defect inventory stale makes the security mechanism's documented coverage incomplete; update the count and add the interpolation case.

Two more from review, both high, both reproduced here before fixing. Both approve a call using evidence that does not exist in this repository. 1. A symlink satisfied the ADR requirement. `is_file()` follows links, so `docs/adr/0007-anything.md -> /etc/hosts` was accepted as the written decision — reproduced, and the citation resolved cleanly. Matches must now be regular files that actually live here: symlinks are skipped alongside the directories the previous commit excluded. A symlink beside a real ADR of the same number is ignored rather than read as an ambiguity. 2. SARIF named the file whose comments decide approval, and nothing constrained it. An absolute `file:` URI, or a relative one containing `..`, escaped repo_root when joined and was read anyway, so a location pointing at any file on the runner carrying a valid marker was approved. Reproduced with a SARIF pointing at /tmp: the gate printed "approved endpoint (adr-0007)" and exited 0. Source paths are now resolved and required to stay inside the checkout. Resolution follows symlinks, so a tracked source file pointing outside is refused for the same reason. A path that does not stay inside is an exemption that could not be checked, which blocks. Five regression tests: a symlinked ADR alone and beside a real one; a SARIF location outside the checkout by absolute URI, by `..` traversal, and by a tracked symlink. 74 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Shepdesign · 2026-09-24T19:37:30Z

Standing note: `Analyze (swift)` is red, and it is not this PR's

Recording this once so it is not re-diagnosed on every push.

What is failing: the CodeQL workflow's Analyze (swift) job, at step 7, Build both flavours (unsigned — CI has no certificate). Steps 1-6 pass on every run, including Compile the custom queries, so the queries in this PR are valid against the runner's own CodeQL. analyze and the gate step are then skipped — they never execute.

Why it is not this PR's. Four things, none of them inference:

Evidence
Same step, every commit	`d79f1eb`, `fb54a28`, `31c8bec`, `d0a9808` — four completed runs, identical failing step
Red on the base branch too	`main` at `f2d3e78` fails the same way (run 36007064172)
The diff cannot reach it	This branch changes `.github/`, `scripts/` and two docs files. `Sources/`, `project.yml` and `NetRelish.xcodeproj` are untouched — and `Build AppStore`, `Build Direct` and `Test` are green on the same commits
Not a version problem	PR #22 tested CodeQL 2.27.0 and 2.27.1 against this failure; both died identically. The action SHA and bundle are pinned and did not move — the `xcode-27` image did

Root cause, from the banner at the top of .github/workflows/codeql.yml: CodeQL's tracer injects libtrace.dylib via DYLD_INSERT_LIBRARIES, and Xcode 27's swift-plugin-server then fails to spawn with EBADARCH. So @State and #Preview never expand inside the KeyboardShortcuts dependency and the traced build exits 65. The sandbox-exec variant of the same problem is worked around in the workflow; this one has no workaround.

No fix exists to port. It is not a change anyone can make in this repository — it clears when the xcode-27 image or CodeQL's tracer stops requiring the injection. Nothing in the workflow needs editing when it does: the job simply starts passing.

Not re-run. A re-run is for distinguishing a flake from a real failure. This has now reproduced identically on five commits across two branches and two CodeQL versions, which is well past what one re-run would establish.

What this costs the PR, honestly

The gate step never executes here, so none of the eleven bypasses this PR closes has been observed being closed by a real CodeQL run — they are closed against the test suite, which does run, on every push, in ci.yml on Ubuntu, deliberately outside this workflow. That split is why the suite is green while this job is red.

The standing limit from #22 is unchanged: proving the queries fire on real Swift needs the canary re-added and watched, and that waits on the same upstream fix.

Generated by Claude Code

Copilot

Copilot review overview

🟡 Changes recommended

Critical gate bypasses and a workflow-breaking URI resolution issue remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 2

Open (5)

Make regex scanning escape-aware to prevent comment bypasses · New
Reject ADR files reached through symlinked parent directories · New
Canonicalize paths before finding deduplication · New
Update stale defect count in workflow explanation · New
Update enforcement history from five to nine defects · New

Resolved since last review (2)

Constrain SARIF source paths to the repository
Reject ADR symlinks outside the checkout

… one path Round five. Three more fail-open holes, all reproduced before fixing, plus the two stale counts in my own documentation. 1. The bare `/.../` regex form. I wrote that it needed no handling because "two unescaped slashes would end the literal at the first, so the sequence `//` cannot occur inside one" — true, and beside the point. The literal's own ending supplies the second slash: in `let r = /\//` the escaped slash and the closing delimiter are textually `//`, and everything after was read as a comment. `let r = /\//; let s = "NETRELISH-ALLOW-ENDPOINT: adr-0007"` was an exemption. An escape in code is now opaque — skip it and whatever it escapes — which leaves the closing delimiter with nothing to pair with. Regex and division never have to be told apart, which is not something this scanner should attempt. 2. The extended-regex terminator used source.find("/#"), which matched the `/` of an escaped `\/` and ended the literal early, leaving the rest of the pattern to be read as code. The search is now escape-aware. 3. ADR citations checked the matched file but not its PARENT, so `docs/adr -> /tmp/decisions` returned perfectly ordinary regular files from outside the checkout. Resolution now goes through inside_checkout, which resolves the whole path instead of inspecting its last component. 4. SARIF picks the path spelling, and the raw string was the grouping key. So `Sources/A.swift` and `Sources/../Sources/A.swift` put two findings on one physical line into two groups of one, and a single marker approved both — the shared-line rule defeated by punctuation. Locations are canonicalised to checkout-relative paths as they are read, before anything groups on them. Also corrects the enforcement history in this module's header and in codeql.yml, both of which still said five. It is seventeen: five before the module was extracted, twelve found by review of the branch that extracted it, every one failing open, one of them introduced by the fix for another. The header now lists them, because the ratio is the point — reading this code carefully has never once been sufficient. Six regression tests: a bare regex's closing delimiter, a real comment after one, an escaped delimiter inside an extended regex, an ADR behind a symlinked parent, two spellings of one path sharing a line, and a traversing URI that lands back inside being canonicalised. 80 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Five unresolved findings remain, including three critical issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3

Open (3)

Citation matching accepts malformed suffixes · New
Rebasing absolute SARIF paths against the wrong checkout · New
Malformed SARIF columns collapse distinct findings · New

Resolved since last review (5)

Canonicalize paths before finding deduplication
Reject ADR files reached through symlinked parent directories
Make regex scanning escape-aware to prevent comment bypasses
Update enforcement history from five to nine defects
Update stale defect count in workflow explanation

Round seven. Two more fail-open holes, both reproduced. 1. Only `file:` was parsed as a URI; every other scheme fell through to Path as a relative filename. `http://evil` became `http:/evil`, and had the checkout contained that path, its markers would have been read and the finding approved — from a URI that names no source file at all. A scheme this does not understand is now a gate error. The guard is narrow on purpose. A relative reference whose FIRST segment contains a colon is invalid per RFC 3986 and must be written `./Odd:Name.swift`, so urlparse reading it as a scheme is correct. Ordinary paths are untouched, because a scheme cannot contain a slash — `Sources/A:B.swift` still resolves, and there is a test pinning that so this cannot start silently refusing real files. 2. positive_int validated each column alone, so `startColumn: 10, endColumn: 5` passed both checks and counted as a position. Two findings carrying that same impossible range collided in the identity map and one marker approved both — the same collapse as the previous round, through a gap the previous round's fix did not cover. A range that runs backwards carries no position, so it is columnless, and columnless findings are never merged. Twenty-two defects now, seventeen of them found by review of this branch. Three regression tests: an unsupported scheme whose path exists in the checkout and carries a marker, an ordinary path with a colon after a slash, and two findings sharing an impossible range. 88 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Critical findings remain around multi-line deduplication and empty or malformed SARIF being accepted as a clean run; the defect-history wording also needs correction.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Finding identity omits SARIF endLine · New
Malformed or empty SARIF incorrectly passes the gate · New

Resolved since last review (2)

Reject invalid ranges to prevent finding key collisions
Reject unsupported URI schemes instead of treating them as paths

Round eight. Two more, both reproduced, and the second is the worst kind this file can have: it is the exact failure the module's own docstring claims to prevent. 1. The finding identity omitted endLine, so two distinct multi-line regions sharing a path, startLine, startColumn and endColumn collapsed into one and a single marker approved both. endLine is now in the key; adding a component can only split a group, never merge one, so it errs toward blocking. Validity had to follow: endColumn < startColumn is NORMAL when a region spans lines, so the backwards-range check from the previous round now applies only within a single line. A multi-line region staying locatable has its own test, because getting that wrong would quietly push every multi-line finding into the columnless path. 2. A present-but-empty SARIF passed. `{"runs": []}` — or runs from an analysis that never loaded this pack — produced no gate results, and "no results" was indistinguishable from "no findings". The gate printed "clean" and exited 0. This file already refuses to read a MISSING SARIF as clean, and says why in its header. A report that says nothing is the same false assurance with an extra step, and a query that silently stops running is precisely how enforcement disappears without anyone noticing. A clean verdict now requires the rule to appear somewhere — in a run's tool metadata, driver or extension, since a query pack lands in the latter, or in a result. Either is proof the query was part of the analysis. CAVEAT, stated because it cannot be tested here: this expectation about CodeQL's SARIF metadata is unverified against real output, for the same reason everything else in this PR is — CodeQL cannot build this project. If CodeQL omits rules that produced no results, this blocks every clean run. That is loud, immediate and fail-closed, which is the correct direction to be wrong in; the alternative is the silent pass above. The test helper now builds SARIF that attests the query ran, because that is what real CodeQL emits and what a clean verdict now needs. Three of the existing tests failed against this change, correctly. Twenty-four defects now, nineteen found by review of this branch. Two of the nineteen were produced by fixing another. Seven regression tests. 95 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

A critical scanner bypass and additional review findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Handle regex literals when scanning interpolation · New

Resolved since last review (2)

Malformed or empty SARIF incorrectly passes the gate
Finding identity omits SARIF endLine

Round nine. One hole, reproduced. A bare `/.../` literal was still ordinary code to the scanner, and its pattern text can contain the exact characters the scanner steers by: let s = "\(/[)]"/) // NETRELISH-ALLOW-ENDPOINT: adr-0007" The regex's `)` closed the interpolation and its `"` closed the outer string, so the marker — which is string content, not a comment — was recorded as one and approved the call below. Two rounds ago I argued the bare form needed no tracking, because escape opacity left its closing delimiter with nothing to pair with. That answered the delimiter, and said nothing about the contents. The literal is now opaque as a whole. The interesting part is the other direction. Naively treating every `/` as a regex opener would read `a/b // NETRELISH-ALLOW-ENDPOINT: adr-0007` as a literal running up to the comment's own slashes, losing a REAL marker and blocking a legitimate exemption. So this follows Swift's own rule: `/.../` is a regex where an EXPRESSION is expected, division after a value — approximated by the previous significant character, since after an identifier, a number, `)`, `]` or a closing quote we are past a value. A `/` with no closing partner on the line is not a regex either, and falls through rather than swallowing the rest of the line. Seven regression tests, and most of them guard that second direction rather than the hole: division with a trailing marker, division twice on one line, a regex literal alone, a comment after one, and a lone slash. Getting this wrong the safe way would still be wrong. 102 total. Twenty-five defects now, twenty found by review of this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate gate-scanner issues can cause invalid deduplication or approve markers hidden in string content.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

Regex literals after expression keywords break interpolation parsing · New
Invalid endLine incorrectly defaults and enables positional deduplication · New

Resolved since last review (1)

Handle regex literals when scanning interpolation

Round ten. Two holes, both reproduced, and both are the previous round's fixes being correct about what they addressed and silent about the rest. 1. `_regex_may_start` read the previous CHARACTER, so after `return` it saw a letter, concluded "past a value", and refused to treat `/[)]"/` as a regex. Its punctuation was live again and steered the scanner out of the string containing it — hole 23 exactly, one keyword away from where I fixed it. It now reads the previous TOKEN. An identifier is only a value if it is not one of the keywords that end an expression context: return, throw, try, await, case, where, in, is, as and the rest. None of them can be a variable name, so reading them this way cannot misfire. `return a/b` still divides, because the token before the slash is `a`, not `return`. That has a test, along with division after `)`, since widening what counts as a regex risks swallowing a real marker. 2. A present-but-malformed `endLine` was indistinguishable from an absent one: positive_int returned None for both and the constructor defaulted both to startLine. A finding with broken coordinates could therefore share an identity with a sound single-line one, and a single marker approved both. Findings now carry whether their coordinates are trusted, and an untrusted one follows malformed columns into the columnless path, where nothing is ever merged. An ABSENT endLine still means a single-line region and still deduplicates — with its own test, because losing that would quietly stop deduplication working at all. Twenty-seven defects now, twenty-two found by review of this branch. Four of those twenty-two were produced by fixing another. Six regression tests. 108 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in path handling, lexical scanning, CI coverage, documentation, and test invariants.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Remote file URI authority is ignored · New

Resolved since last review (2)

Invalid endLine incorrectly defaults and enables positional deduplication
Regex literals after expression keywords break interpolation parsing

Round eleven. One hole, reproduced, and it stood in the same function as the guard that was supposed to have covered it. `file://attacker/<checkout>/Sources/A.swift` names a file on another host. source_path used only `parsed.path` and dropped `netloc`, so that URI resolved against THIS checkout and was approved by the local marker there. Two rounds ago I added a scheme check to that same function, for exactly this class of problem — a URI that does not name a local file being treated as one. I validated the scheme and never looked at the field beside it. Fifth time in this PR that a fix has been right about what it addressed and silent about the rest of its category, and the first where the gap was one line from the fix. Per RFC 8089 only an empty authority or `localhost` means this machine; anything else is refused, case-insensitively. `file:///path` and `file://localhost/path` both still resolve, the latter with its own test, because tightening this risks refusing legitimate locations and that failure is silent — the gate simply blocks and the call reads as unapproved. Twenty-eight defects now, twenty-three found by review of this branch. Five of those twenty-three were produced by fixing another. Two regression tests. 110 total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf

Copilot

Copilot review overview

🔵 Needs a closer look

Eight moderate review findings remain unresolved.

Review effort: Lite
Findings: None

Resolved since last review (1)

Remote file URI authority is ignored

Previously missed (1)

In code that hasn't changed since last review

Non-Swift keywords misclassify division as regex

scripts/codeql_gate.py:447

and, or, and not are not Swift expression-start keywords; they can be ordinary identifiers. With and / value // NETRELISH-ALLOW-ENDPOINT: adr-0007, this set misclassifies the division as a regex, consumes the first slash of the real // comment as the regex terminator, and drops a legitimate exemption, causing the gate to block valid code. Remove these non-Swift keywords and add a division regression for an identifier with one of these names.

Shepdesign · 2026-09-25T01:00:38Z

Decision from Ryan on the duplicate ADR number: keep 0006 on the keychain ADR.

That is the direction this branch already took, so no change is needed:

File	Number
`docs/adr/0006-identity-lives-in-keychain.md` (accepted 2026-09-21)	keeps 0006
`docs/adr/0007-direct-build-updates.md` (accepted 2026-09-23)	renumbered to 0007

The offer to flip the rename or revert it is withdrawn — the renumber stands as committed, and the ADR 0006 references in Sources/ and project.yml remain correct because they are all about identity.

Nothing else in this PR is affected. The two items still open for review are unchanged: the unverifiable hole-22 assumption (a clean run must still name the gate rule in SARIF metadata, untestable while the traced build fails), and the fact that the queries have still never been observed firing on real Swift.

Generated by Claude Code

Copilot AI lite review requested due to automatic review settings September 24, 2026 18:46

Copilot started reviewing on behalf of Shepdesign September 24, 2026 18:46 View session