Refactor: one GitHub client, one thread reader, shared escaping

Duplication that review rounds flagged and declined as out of scope:

One GitHub REST client with a timeout and a configurable host. Today githubReader, tokenApi, githubApi and the outcomes helper overlap (/review explain <finding>: answer whether a finding still holds #75, Acknowledge a review within seconds and always complete its check run #74, Record each finding's outcome when a pull request closes #81, Monthly report of precision per reviewer and repository, and cost per fixed finding #83).
One GraphQL thread reader shared by re-reviews (src/rereview.ts) and outcomes (src/outcomes.ts) (Record each finding's outcome when a pull request closes #81).
Shared escaping of mentions and links in every bot comment (/review explain <finding>: answer whether a finding still holds #75).
Move the C# member scan out of gather.ts (Selection floor: add api-compat for public API changes and data-model for serialized contracts #66), stoppedBy out of provider.ts (Acknowledge a review within seconds and always complete its check run #74), and comment parsing into threads.ts (Re-review only what changed since the last review, and say when it's done #77).
A YAML library for .github/code-review/config.yml and one shared path-item check (Read per-repository settings from .github/code-review/ on the default branch #69).
One shared type for the eval output file, used by its writer and its reader (Evals: measure precision and findings per pull request, and compare two runs #73).

Split from the 2026-10-31 autopilot run's follow-up checklist.

Autopilot: more refactor items from the review rounds:

Read the Node version from a .node-version file in the workflows instead of hard-coding it (Run a cost-capped smoke eval when a prompt, charter or model changes #80 F6, Monthly report of precision per reviewer and repository, and cost per fixed finding #83 F9).
An options object for review(), whose parameter list keeps growing (Shadow mode for a new reviewer or prompt #76 F6, Close the loop on the bot's own threads, and pause after three unanswered reviews #82 F8).
Split the light path into its own reviewLight() (Deep reviews for sensitive paths and a light review for documentation-only pull requests #78 F4).
One shared helper for the upstream-findings total (Leave findings on template-owned files to the template's repository #70 F4).

One more item for this refactor, a follow-up from #112 #113's plan (landed in #119):

Share gather's git work with the template check: templateCandidate repeats the rev-parse, merge-base, diff --numstat and templateOf work that gather runs, and main.ts and the evals wire them together by hand; fold it into one gather step. See Parse C# members and check template files #119 (comment)

Autopilot: slice A is underway on branch refactor-95a: one GitHub client with a time limit and the runner's host, one review-thread reader shared by re-reviews and outcomes, and shared mention/link escaping. The rest of this issue stays open.

One more item for this refactor, found while planning #124; it landed in #126.

One writer check for outcomes: collectOutcomes in src/outcomes.ts keeps its own permission lookup (writerPermissions, isWriter) beside writerCheck in src/writers.ts. Once outcomes move onto the shared GitHub client, let writerCheck take that client and use it for outcomes' reactors and repliers, so the two checks can't drift apart.

Slice A landed in #125: one GitHub transport in src/github.ts with a time limit and the runner's host, one review-thread reader shared by re-reviews and outcomes, and mention/link escaping from one module.

Two follow-ups from slice A's plan, both part of this issue's list:

Move the remaining GitHub reads onto the shared client: readOpenIssues in src/issues.ts, the reads in src/writers.ts and training/guidance-issues.ts still build https://api.github.com URLs with their own fetch (issues and writers without a time limit), so they ignore a GitHub Enterprise Server's address and can hang a run. Smoke eval baseline gate and writer checks #121, which changed them, has landed.
Neutralize mentions and links in model-written text of every bot comment: the summary comment, finding threads, /review explain answers (/review explain <finding>: answer whether a finding still holds #75 F3) and the outcomes table post model text without neutralizeMentions, so an injection-steered finding can ping people or post links. This needs a rule that leaves code spans such as @Override intact.

What's left:

The two items above.
Move the C# member scan out of gather.ts, stoppedBy out of provider.ts, and comment parsing into threads.ts (slice B, planned).
A YAML library for .github/code-review/config.yml and one shared path-item check.
One shared type for the eval output file.
Read the Node version from .node-version in the workflows.
An options object for review(); split the light path into reviewLight().
One shared helper for the upstream-findings total.
Share gather's git work with the template check.
One writer check for outcomes, using the shared client.

Slice B landed in #127, with no change in behavior: the C# contract checks moved out of gather.ts into src/contracts.ts, stoppedBy moved out of provider.ts into src/checks.ts, re-reviews' finding-comment parsing moved into threads.ts, review() takes an options object and the light review runs in its own reviewLight(), the eval output file has one shared type, the upstream-findings total has one helper, and the workflows read the Node version from .node-version.

One follow-up from slice B's plan, part of this issue's comment-parsing item:

Read the finding marker in one place: parseFindingComment in src/outcomes.ts parses the ncode-review:finding marker with its own regex and takes the first marker, while markerFields in src/threads.ts takes the last (the one threadBody wrote, after any suggestion code); move it into threads.ts on top of markerFields so outcomes and re-reviews read a finding comment the same way.

What's left:

The follow-up above.
Move the remaining GitHub reads (src/issues.ts, src/writers.ts, training/guidance-issues.ts) onto the shared client.
Neutralize mentions and links in model-written text of every bot comment.
A YAML library for .github/code-review/config.yml and one shared path-item check.
Share gather's git work with the template check.
One writer check for outcomes, using the shared client.

@OverRide

Plan snapshot. The working copy is PLAN.md in the plan's checkout; this comment is not updated as steps run.

Plan: Finish the engine refactor of #95 (20 steps)

Plan: Finish the engine refactor of #95

Issue: #95

Goal

Close out #95 with its last slice. The remaining GitHub reads (open issues, closed issues, permission lookups, the training's guidance issues) move onto the one client in src/github.ts, so each has a time limit and goes to the runner's GitHub. Outcomes and the publish step's dismissals use the engine's one writer check. The finding marker is read in one place. Gather and the template check share one git read. Model-written text in every bot comment can no longer mention anyone or post links, though code spans stay intact. config.yml is parsed with the yaml library, and one check covers path items. Behavior stays the same except where a Decision says otherwise.

Context for the executor

Build: npm run typecheck
Test: npm test (about 50 s; needs node 24+ and git on PATH)
Conventions: TypeScript run directly by Node (node file.ts), strict tsconfig.json, ES modules with .ts import suffixes, import type for types only. Tests use node:test and node:assert/strict, one behavior per test, hermetic: fake fetch functions, never the network. Comments are full sentences in the repository's plain style. Log lines go through the log parameter or writeLog from src/log.ts. Never console.log in src/.
The shared client: githubApi(token, fetchFn, options) in src/github.ts returns an Api, (method, url, body?) => Promise<any>. A url starting with / goes under GITHUB_API_URL, or https://api.github.com when that variable isn't set. Each request has a 30 s time limit. Any answer that isn't a success throws a GitHubError with .status. With { terseErrors: true } its message leaves out GitHub's answer: GitHub refused GET /x: HTTP 502. githubReader(token) is the ReadApi form: 404 gives undefined, and anything else that isn't a success throws.
Gotchas:
- src/acknowledge.ts and src/finish.ts must load no npm package and neither provider.ts, config.ts nor credentials.ts. test/acknowledge.test.ts follows their imports to check this. They load, among others, github.ts, comments.ts, threads.ts, results.ts and markdown.ts, so none of those may import yaml or writers.ts. settings.ts is not in that graph, so it may import yaml.
- A test that asserts a full https://api.github.com/... URL must call useGitHubDotCom() from test/github-env.ts at the top level, so a runner's own GITHUB_API_URL can't change it.
- gather() sets pullRequest.headSha and marks the ChangedFile objects it gets in place (templateOwned, the contract flags). Give each gather() call its own readChanges() result.
- U+200B (zero-width space) is written \u200b in source and tests.

Decisions

The shared client for the moved reads is the Api from githubApi. main.ts builds one, const github = githubApi(githubToken, fetch, { terseErrors: true }), and uses it for outcomes, the writer check, the closed issues and the open issues. githubApi accepts token: string | undefined, and no token sends no authorization header.
failureText(error) in writers.ts gives HTTP <status> for a GitHubError and errorText(error) for anything else, so the warning texts stay (HTTP 502) as they are today.
writerCheck(repository, api, log):
- It counts a writer by association first, and failing that by permission or role_name in {admin, maintain, write}. That is the union of the writer check's rule and the outcomes' rule. The legacy permission reads maintain as write, so the result is the same.
- A 404 means no write access, with no warning. Any other failure also means no write access, and warns ::warning::Couldn't read <login>'s permission (<reason>); their text counts as a non-writer's.
- Outcomes build it from their own api and look up reactors with no association. Repliers keep withLookupLimit(…, maxReplyAuthorLookups). The publish step builds one from its api for its dismissals and for planFollowUps.
- Accepted differences, all in log lines only: outcomes and the publish step no longer warn on a 404, and the outcomes warning uses the writer check's wording.
pause.ts needs no transport change: checkPause already reads through readComments, which uses githubFetch. Its postPausedNotice takes the Api type instead of an inline function type.
The training's fetchGuidanceIssues keeps its signature and uses githubApi(token, fetchApi) inside. A refusal's message is now GitHub refused GET /repos/…: HTTP 403 ….
parseFindingComment moves to threads.ts, on top of markerFields, which reads the last marker. That is the one threadBody writes, after any suggestion. Today outcomes read the first marker, so a suggestion whose code holds a marker-like line could make them record the wrong finding. This is the one behavior fix in the item, and it goes under Fixed.
readChanges(pullRequest, prPath, basePath) in gather.ts does the rev-parse, merge-base and diff --numstat once. It returns { headSha, mergeBase, changed, template?, candidate? }. gather() takes it as an optional last parameter, and templateCandidate is removed. gatherChecked(pullRequest, prPath, basePath, guidanceDir, read, log) in template-check.ts runs readChanges, then confirmTemplateFiles when there is a candidate, then gather. main.ts and evals/run.ts call it. It lives in template-check.ts because that file already imports gather.ts.
The neutralizing rule, neutralizeProse(text) in src/markdown.ts. Outside code, it inserts U+200B:
- after an @ that is followed by [A-Za-z0-9_-];
- between ] and a following (, [ or :;
- between http: or https: (any case) and a following //;
- between www (any case) and a following ..
It leaves code alone:
- An inline code span is a backtick run that no backslash escapes, up to the next run of exactly the same length on the same line. Backslashes don't escape anything inside a span.
- A fenced code block opens on a line other than the text's first. The line starts at column 0 with three or more backticks, whose info string holds no backtick, or with three or more tildes. The block runs to the next line that has at most three leading spaces, then a run of the same character at least as long, then only spaces or tabs.
When code could be read differently, it escapes the whole text, code included. That happens when a line outside a fence has a backtick run with no closer on that line, when a fence-like line (^ {0,3}(```|~~~)) is indented or is the first line, or when a fence is never closed. GitHub might pair those differently than this rule does, and a ZWSP in code is invisible, while a ping is not. Callers escape < before calling it, as they do today.
Apply neutralizeProse only to model-written text in comments:
- threadBody: title, problem, consequence, fix and evidence. reviewBody: title, problem and fix.
- summaryComment: the summary line, and the titles in the findings, tracked and upstream lists.
- answerBody: the answer and the reason.
- The outcomes table's cells.
- stillOpenReply's note, replacing neutralizeMentions there.
Check-run text and annotations are not comments and notify no one, so they stay as they are. neutralizeMentions stays for teach.ts and tools/report.ts.
YAML: yaml@2.9.1 (eemeli/yaml, no dependencies) is installed with npm install --save-exact.
- parseYaml keeps its signature, its YamlValue shapes and its YamlError with line N: … messages. It parses with parseDocument(text, { schema: 'failsafe', lineCounter, prettyErrors: false, uniqueKeys: true }), so every value is a string. Then it walks the document: a plain '', ~ or null becomes null, and it refuses aliases, anchors, tags, block and multi-line scalars, deeper nesting, a root that isn't a mapping, and a __proto__ key.
- Accepted differences, each pinned by a test: a file whose keys are all indented the same now parses; double-quoted strings take every YAML escape, such as \u0041; and the message after line N: is the library's for a syntax error, such as Map keys must be unique. The warning shape <file> is not valid: line N: …; using the defaults. is unchanged.
One path-item check, pathItemProblem(key, item), serves both validateConfig (sensitive, skip, policies) and charterPaths (paths), with today's two messages.
The plan has 20 steps, so every item of Refactor: one GitHub client, one thread reader, shared escaping #95 fits and the plan closes it.

Out of scope

Any behavior change beyond the Decisions above. No renames beyond the plan, and no formatting-only churn.
The signatures of readComments, readIssueComments, checkPause, githubReader, fetchPullRequest and readEarlierFindings.
Check-run text, annotations and tools/report.ts.
#123 and owner/repo#123 references in model text: they are recorded as a follow-up.
Any npm dependency besides yaml. No yaml import outside src/settings.ts.

Steps

[ ] 1. Read the open issues through the shared GitHub client

Files: src/github.ts (edit), src/issues.ts (edit), src/main.ts (edit), test/issues.test.ts (edit)
Change:
- In github.ts, change the first parameter of githubApi to token: string | undefined.
- In issues.ts, the signature becomes readOpenIssues(repository: string, api: Api, isWriter: WriterCheck, exclude: number[] = []), with import type { Api } from './github.ts'. In the page loop, replace the fetchPage(...)/response.ok/response.json() block with issues = (await api('GET', \/repos/${repository}/issues?state=open&sort=updated&direction=desc&per_page=${pageSize}&page=${page}`)) as Listed[];inside the existingtry, keeping catch { break; }`.
- In main.ts, right after const log = writeLog;, add the comment // One client for the run's reads with the read-only token: each request has a time limit and goes to the runner's GitHub. and then const github = githubApi(githubToken, fetch, { terseErrors: true });. Use github in place of githubApi(githubToken, fetch, { terseErrors: true }) in the closed-event branch, keeping its token check. Call readOpenIssues(repository, github, isWriter, closedIssues(pullRequest.body)).
- In issues.test.ts, import githubApi from ../src/github.ts and useGitHubDotCom from ./github-env.ts, and call useGitHubDotCom(); after the imports. Rewrite each readOpenIssues('o/r', T, W, E, F) as readOpenIssues('o/r', githubApi(T, F), W, E).
Pattern: how recordOutcomes takes an Api in src/outcomes.ts.
Done when: if (Select-String -Path src/issues.ts -Pattern 'api\.github\.com|fetchPage' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 2. Read the closed issues through the shared GitHub client

Files: src/writers.ts (edit), src/main.ts (edit), test/pull-request.test.ts (edit)
Change:
- In writers.ts, import GitHubError, type Api from ./github.ts. Add export function failureText(error: unknown): string, returning error instanceof GitHubError ? `HTTP ${error.status}` : errorText(error), with the comment // Why a read failed, for a warning: GitHub's status for a refusal, or the error's text.
- In readWritersIssues, use failureText(error) instead of errorText(error) in the catch.
- readClosedIssues becomes (repository: string, numbers: number[], api: Api, isWriter: WriterCheck, log = () => {}), and its readIssue becomes (number) => api('GET', \/repos/${repository}/issues/${number}`)`. Keep its warning text.
- In main.ts, call readClosedIssues(repository, closedNumbers, github, isWriter, log).
- In pull-request.test.ts, import githubApi and useGitHubDotCom and call useGitHubDotCom(); after the imports. Rewrite readClosedIssues('o/r', closedNumbers, 't', isWriter, fetchJson) as readClosedIssues('o/r', closedNumbers, githubApi('t', fetchJson), isWriter). Rewrite the call in the test "a closed issue that cannot be read…" the same way, with (line) => lines.push(line) last.
Pattern: writerCheck's warning in src/writers.ts.
Done when: if (Select-String -Path src/writers.ts -Pattern 'issues/\$\{number\}, { headers' -Quiet) { exit 1 }; npm run typecheck && npm test`

[ ] 3. Look up permissions through the shared GitHub client

Files: src/writers.ts (edit), src/main.ts (edit), src/publish.ts (edit), test/pull-request.test.ts (edit), test/follow-up.test.ts (edit), test/pause.test.ts (edit)
Change:
- In writers.ts, the signature becomes writerCheck(repository: string, api: Api, log: (line: string) => void = () => {}). Add const writerRoles = new Set(['admin', 'maintain', 'write']);, with a comment that the legacy permission reads maintain as write and triage as read, while role_name names the role itself.
- Rewrite lookUp as follows, keeping the cache and the association check as they are, and delete the now-unused headers() helper:
```
try {
  const answer = (await api('GET', `/repos/${repository}/collaborators/${encodeURIComponent(login)}/permission`)) as { permission?: string; role_name?: string } | undefined;
  return writerRoles.has(answer?.permission ?? '') || writerRoles.has(answer?.role_name ?? '');
} catch (error) {
  // A 404 is an answer: the login isn't a collaborator, or doesn't exist.
  if (error instanceof GitHubError && error.status === 404) return false;
  log(`::warning::Couldn't read ${login}'s permission (${failureText(error)}); their text counts as a non-writer's.`);
  return false;
}
```
- In main.ts, call writerCheck(repository, github, log).
- In publish.ts, call writerCheck(results.repository, api, writeLog).
- In the tests, rewrite writerCheck('o/r', 't', F) as writerCheck('o/r', githubApi('t', F)), and writerCheck('o/r', 't', F, L) as writerCheck('o/r', githubApi('t', F), L). That covers every call in pull-request.test.ts, follow-up.test.ts and pause.test.ts. Import githubApi from ../src/github.ts where it is missing.
Pattern: githubApi('t', fetchPage) in test/rereview.test.ts.
Done when: if (Select-String -Path src/writers.ts -Pattern 'api\.github\.com|fetchJson' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 4. Outcomes use the shared writer check

Files: src/outcomes.ts (edit), test/outcomes.test.ts (edit)
Change:
- In outcomes.ts, delete writerPermissions and its comment, and the local permissions map and isWriter in collectOutcomes. In their place:
```
// The engine's writer check: a reply's author by their association or else their permission, a reactor, whose reaction carries no association, by their permission.
const isWriter = writerCheck(repository, api, log);
const isReplyWriter = withLookupLimit(isWriter, maxReplyAuthorLookups);
```
- For reactions, use writer: await isWriter(login, undefined). Import writerCheck with withLookupLimit from ./writers.ts.
- In outcomes.test.ts, in the test "collectOutcomes counts a replier whose permission cannot be read as a non-writer, and warns", change the regex's ending from their reactions and replies count as a non-writer's\.$ to their text counts as a non-writer's\.$.
Pattern: withLookupLimit already wraps the check in collectOutcomes.
Done when: if (Select-String -Path src/outcomes.ts -Pattern 'writerPermissions|/permission' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 5. The publish step's dismissals use the shared writer check

Files: src/publish.ts (edit), test/follow-up.test.ts (edit)
Change:
- In publish.ts, delete the local async function isWriter(api, repository, login) and its comment. In main(), after const threads = …, add const isWriter = writerCheck(results.repository, api, writeLog);.
- Pass (login) => isWriter(login, undefined) to dismissedThreads, without the .catch, and pass isWriter to planFollowUps. Drop the imports that are now unused (errorText, type Api).
- In follow-up.test.ts, in the test "the publish step counts a resolution or a 👎 only from a login whose repository access is admin, maintain or write", replace the last assert.match(stdout, /::warning::Couldn't read gone's access …/) line with:
  - the comment // A 404 is an answer, as in every writer check: gone isn't a collaborator.
  - then assert.doesNotMatch(stdout, /Couldn't read gone's/);
Pattern: collectOutcomes after step 4.
Done when: if (Select-String -Path src/publish.ts -Pattern '/permission' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 6. The training reads the guidance issues through the shared GitHub client

Files: training/guidance-issues.ts (edit), test/teach.test.ts (edit)
Change:
- fetchGuidanceIssues(repository, token, fetchApi = fetch) keeps its signature. Start it with const api = githubApi(token, fetchApi);. Each page becomes const batch = (await api('GET', \/repos/${repository}/issues?state=closed&labels=${guidanceLabel}&per_page=${pageSize}&page=${page}`)) as GuidanceIssue[];, which replaces the URL, the headers, the timeout and the response.okcheck. ImportgithubApifrom../src/github.ts`.
- In teach.test.ts, import useGitHubDotCom from ./github-env.ts and call it at the top level after the imports.
Pattern: step 1's readOpenIssues.
Done when: if (Select-String -Path training/guidance-issues.ts -Pattern 'api\.github\.com' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 7. Type the paused notice's client as the shared Api

Files: src/pause.ts (edit)
Change: Add import type { Api } from './github.ts'; and change postPausedNotice's first parameter to api: Api. checkPause stays as it is: it already reads through readComments, which uses githubFetch.
Pattern: postAnswer in src/explain.ts.
Done when: if (-not (Select-String -Path src/pause.ts -Pattern 'postPausedNotice\(api: Api' -Quiet)) { exit 1 }; npm run typecheck && npm test

[ ] 8. Read the finding marker in one place

Files: src/threads.ts (edit), src/outcomes.ts (edit), test/outcomes.test.ts (edit), test/threads.test.ts (edit)
Change:
- Move parseFindingComment and its comment from outcomes.ts to threads.ts, right after markerFields. Replace its first three lines with const fields = markerFields(body);, and read id, severity and reviewer from fields?.get(…). Keep the reviewers' fallback to the visible <sub> line as it is.
- Add to its comment: It reads the last marker, as markerFields does: the one threadBody wrote, after any suggestion.
- In outcomes.ts, import parseFindingComment from ./threads.ts. In outcomes.test.ts, import it from ../src/threads.ts instead of ../src/outcomes.ts.
- In threads.test.ts, import parseFindingComment and add this test, parseFindingComment reads the marker threadBody wrote, not one in a suggestion before it:
```
const decoy = '';
const body = threadBody(finding({ id: 'F2', severity: 'blocker', reviewers: ['security'] }), { startLine: 12, endLine: 12, replacement: decoy });
assert.deepEqual(parseFindingComment(body), { id: 'F2', severity: 'blocker', reviewers: ['security'] });
```
Pattern: parsePosted in src/threads.ts.
Done when: if (Select-String -Path src/outcomes.ts -Pattern 'ncode-review:finding' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 9. Read the pull request's changes once for gather and the template check

Files: src/gather.ts (edit), src/main.ts (edit), evals/run.ts (edit), test/template.test.ts (edit)

Change:

In gather.ts, replace templateCandidate with:

// What git says the pull request changes, read once for gather and the template check: the head, the merge base, the
// changed files, the template it adopts or updates, and the files confirmTemplateFiles checks. gather marks `changed`
// in place, so each gather gets its own.
export interface Changes { headSha: string; mergeBase: string; changed: ChangedFile[]; template?: Template; candidate?: TemplateCandidate }
export function readChanges(pullRequest: PullRequest, prPath: string, basePath: string): Changes {
  const headSha = git(prPath, ['rev-parse', 'HEAD']).trim();
  const mergeBase = git(prPath, ['merge-base', pullRequest.baseSha, headSha]).trim();
  const changed = parseNumstat(git(prPath, ['diff', '--numstat', '-M', '--no-ext-diff', '--no-textconv', mergeBase, headSha]));
  const template = templateOf(pullRequest, changed, prPath, basePath);
  const commit = readTemplate(prPath)?.commit;
  const candidate = template && commit ? { template, commit, files: changed.map((f) => f.path).filter((p) => isTemplateOwned(p, template)) } : undefined;
  return { headSha, mergeBase, changed, ...(template ? { template } : {}), ...(candidate ? { candidate } : {}) };
}

Write the interface over several lines, as the file's other interfaces are.

gather gets the last parameter changes: Changes = readChanges(pullRequest, prPath, basePath). Its first lines become pullRequest.headSha = changes.headSha; and const { mergeBase, changed } = changes;, and it uses const candidate = changes.template; in place of its own rev-parse, merge-base, diff --numstat and templateOf calls. Keep the review comment above pullRequest.headSha. Add to the comment above gather: changes, when given, is the readChanges the template check used.
In main.ts and evals/run.ts, replace the templateCandidate line with const changes = readChanges(pullRequest, prPath, basePath);, using head, base in the evals. Use changes.candidate in the confirmTemplateFiles line, and pass changes as gather's last argument.
In template.test.ts, import readChanges instead of templateCandidate. Rename the test to readChanges names the head's _commit and the changed files the template would own, and use readChanges(…).candidate in both assertions.

Pattern: the templateCandidate body it replaces.
Done when: if (Select-String -CaseSensitive -Path src/*.ts,evals/*.ts,test/*.ts -Pattern 'templateCandidate' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 10. Fold the template check into one gather step

Files: src/template-check.ts (edit), src/main.ts (edit), evals/run.ts (edit), test/template.test.ts (edit)

Change:

In template-check.ts, add the function below. Import gather, readChanges, type Gathered and type PullRequest from ./gather.ts.

// gather with the template check in it: the pull request's changes read once, the template's files confirmed through
// `read`, then everything the reviewers see. The review step and the evals both gather this way.
export async function gatherChecked(pullRequest: PullRequest, prPath: string, basePath: string, guidanceDir: string | undefined, read: ReadApi, log: (line: string) => void = () => {}): Promise<Gathered> {
  const changes = readChanges(pullRequest, prPath, basePath);
  const confirmed = changes.candidate ? await confirmTemplateFiles(changes.candidate, prPath, read, log) : undefined;
  return gather(pullRequest, prPath, basePath, guidanceDir, confirmed, changes);
}

In main.ts, replace the three lines changes/confirmed/gathered with const gathered = await gatherChecked(pullRequest, prPath, basePath, path.join(root, 'guidance'), githubReader(githubToken), log); and keep the comment above them. Do the same in evals/run.ts with head, base, guidance, githubReader(process.env['GITHUB_TOKEN']), log. Drop the imports that are now unused.

In template.test.ts, add this test after the readChanges one,

gatherChecked reviews the template's files as the project's when the template can't be read, and reads it only for an adoption

:

const { repo, base, baseSha } = adoption();
const unread = await gatherChecked({ ...pullRequest(), baseSha }, repo, base, undefined, async () => { throw new Error('offline'); });
assert.deepEqual(unread.files.filter((f) => f.templateOwned), []);
assert.ok(unread.template?.projectOwned.includes('.template/Get-PlanState.ps1'));
const reads: string[] = [];
const plain = await gatherChecked({ ...pullRequest('CONTRIBUTOR'), baseSha }, repo, base, undefined, async (p) => { reads.push(p); return undefined; });
assert.equal(plain.template, undefined);
assert.deepEqual(reads, []);

Pattern: step 9's call sites.
Done when: if (Select-String -Path src/main.ts,evals/run.ts -Pattern 'confirmTemplateFiles|readChanges' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 11. Add neutralizeProse for model-written text

Files: src/markdown.ts (edit), test/markdown.test.ts (edit)
Change:
- Add export function neutralizeProse(text: string): string, applying the rule in Decisions, with a comment that states it. It is about 45 lines and has three parts.
- A private escapeProse(text) that chains these replacements:
  - .replace(/@(?=[A-Za-z0-9_-])/g, '@\u200b')
  - .replace(/\](?=[([:])/g, ']\u200b')
  - .replace(/(https?:)(?=\/\/)/gi, '$1\u200b')
  - .replace(/(www)(?=\.)/gi, '$1\u200b')
- A private withCodeSpans(line): string | undefined. It walks the line: a \ copies itself and the next character; a backtick run of length n looks for the next run of exactly n on the line. With a closer, it emits the prose before the run through escapeProse and the span verbatim. Without one, it returns undefined.
- neutralizeProse splits the text on \n and tracks an open fence (its character and length):
  - Inside a fence, a line is copied as it is, and the fence closes on /^ {0,3}({3,}|~{3,})[ \t]*\r?$/` with the same character and at least the same length.
  - Outside a fence, a line matching /^ {0,3}({3,}|~{3,})/opens a fence. That is only allowed on a line other than the first, at column 0, and for backticks with no backtick in the rest of the line. Otherwise it returnsescapeProse(text)`.
  - Any other line goes through withCodeSpans, and undefined means it returns escapeProse(text).
  - At the end, a fence that is still open means it returns escapeProse(text).
- In markdown.test.ts, add one test per behavior, each comparing exact strings:
  - mentions and links in prose get U+200B: 'Ask @someone: see [docs](https://x.y), http://a and www.b.c' → 'Ask @\u200bsomeone: see [docs]\u200b(https:\u200b//x.y), http:\u200b//a and www\u200b.b.c';
  - an inline code span stays intact: 'Use `@Override` or `[a](b)`, @x' → 'Use `@Override` or `[a](b)`, @\u200bx';
  - a fenced block stays intact: 'Fix:\n```java\n@Override\n```\nthen @team' → 'Fix:\n```java\n@Override\n```\nthen @\u200bteam';
  - a backslash-escaped backtick opens no span: '\\`@user\\`' → '\\`@\u200buser\\`';
  - a backtick with no closer on its line escapes everything: 'a ` b\n`@x` c' → 'a ` b\n`@\u200bx` c';
  - an indented, first-line or unclosed fence escapes everything: 'x\n ```\n@y\n ```', '```js\n@x\n```' and 'x\n```\n@y' each get @\u200b before x or y.
Pattern: neutralizeMentions in the same file.
Done when: if (-not (Select-String -Path test/markdown.test.ts -Pattern 'neutralizeProse' -Quiet)) { exit 1 }; npm run typecheck && npm test

[ ] 12. Neutralize model text in finding threads

Files: src/threads.ts (edit), test/threads.test.ts (edit)
Change:
- Import neutralizeProse from ./markdown.ts. Add function prose(value: string): string { return neutralizeProse(text(value)); } after text(), with the comment // Model-written text: escaped as text() does, then neutralized, so it can't mention anyone or post a link.
- In threadBody, use prose() for finding.title, problem, consequence, fix and evidence, keeping text() for the reviewers. In reviewBody's not-on-a-changed-line items, use prose() for f.title, f.problem and f.fix, keeping text() for f.file.
- Add a test to threads.test.ts, threadBody neutralizes mentions and links in the finding's text, but not in code spans. Build the body from finding({ title: 'Ping @team', problem: 'See [x](https://evil.example).', fix: 'Add @OverRide.' }), and assert that it includes 'Ping @\u200bteam', '[x]\u200b(https:\u200b//evil.example)' and 'Add @OverRide.'.
- Add a second test that does the same for reviewBody with one not-on-a-changed-line finding, { inline: [], inBody: [finding({ title: 'Ping @team' })], alreadyPosted: [], tracked: [] }.
Pattern: text() in the same file.
Done when: if (-not (Select-String -Path src/threads.ts -Pattern 'neutralizeProse' -Quiet)) { exit 1 }; npm run typecheck && npm test

[ ] 13. Neutralize model text in the summary comment

Files: src/results.ts (edit), test/engine.test.ts (edit)
Change:
- Import neutralizeProse from ./markdown.ts.
- In summaryComment, wrap the verdict line's text in neutralizeProse(…), as neutralizeProse(results.summary.replace(/</g, '<').replace(/\s*\n\s*/g, ' ')).
- In the findings list, trackedList and upstreamList, use neutralizeProse(cell(f.title)) for each title. Leave the check-run functions unchanged.
- In engine.test.ts, add a test after "summaryComment shows reviewers, models, counts and the worst severity", summaryComment neutralizes mentions and links in the summary and the titles. Call summaryComment with a copy of that test's argument whose summary is 'Thanks @someone, see https://evil.example' and whose finding title is 'Ping @team about `@Override`'. Use the same second and third arguments as that test.
- Assert that the body includes 'Thanks @\u200bsomeone, see https:\u200b//evil.example' and 'Ping @\u200bteam about `@Override`'.
Pattern: step 12.
Done when: if (-not (Select-String -Path src/results.ts -Pattern 'neutralizeProse' -Quiet)) { exit 1 }; npm run typecheck && npm test

[ ] 14. Neutralize model text in `/review explain` answers

Files: src/explain.ts (edit), test/explain.test.ts (edit)
Change:
- Import neutralizeProse from ./markdown.ts. In answerBody, the reason becomes neutralizeProse(text(explanation.reason).replace(/^because\b[\s,:]*/i, '')), and the answer becomes neutralizeProse(text(explanation.answer)). failedBody and unknownRefBody stay as they are.
- In explain.test.ts, add answerBody neutralizes mentions and links in the answer and the reason, but not in code spans. Use answer: 'Ask @owner, see [this](https://evil.example).', holds: true and reason: 'because `@Override` is missing for @team.'.
- Assert that the body includes 'Ask @\u200bowner, see [this]\u200b(https:\u200b//evil.example).' and 'Still holds because `@Override` is missing for @\u200bteam.'.
Pattern: step 12.
Done when: if (-not (Select-String -Path src/explain.ts -Pattern 'neutralizeProse' -Quiet)) { exit 1 }; npm run typecheck && npm test

[ ] 15. Neutralize the outcomes table and the still-open reply

Files: src/outcomes.ts (edit), src/follow-up.ts (edit), test/outcomes.test.ts (edit), test/follow-up.test.ts (edit)
Change:
- In outcomes.ts, cell() returns neutralizeProse(…) around its current chain.
- In follow-up.ts, stillOpenReply uses neutralizeProse in place of neutralizeMentions, and its import changes to match.
- In outcomes.test.ts, add the outcomes section neutralizes mentions and links in its cells, using outcomesSection({ state: 'merged', head: 'b'.repeat(40), findings: [{ ...findings[0]!, reviewers: ['@team [x](https://evil.example)'] }] }). Assert that it includes '@\u200bteam [x]\u200b(https:\u200b//evil.example)'.
- In follow-up.test.ts, add stillOpenReply neutralizes links in the note, but not code spans, using stillOpenReply(headSha, 'See https://evil.example and [x](y); `@Override` stays.', 'polewskm'). Assert that it includes 'See https:\u200b//evil.example and [x]\u200b(y); `@Override` stays.'.
Pattern: step 12.
Done when: if (Select-String -Path src/follow-up.ts -Pattern 'neutralizeMentions' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 16. Add the yaml library

Files: package.json (edit), package-lock.json (edit)
Change: Run npm install --save-exact yaml@2.9.1. It adds "yaml": "2.9.1" to dependencies. Change nothing else by hand.
Pattern: "@anthropic-ai/claude-agent-sdk": "0.3.312" in package.json.
Done when: if ((Get-Content package.json -Raw) -notmatch '"yaml": "2\.9\.1"') { exit 1 }; npm ls yaml && npm run typecheck && npm test

[ ] 17. Parse config.yml with the yaml library

Files: src/settings.ts (edit), test/settings.test.ts (edit)
Change:
- In settings.ts, import isAlias, isMap, isScalar, isSeq, LineCounter, parseDocument from 'yaml'. Replace parseYaml's body and delete checkKey, stripComment, splitKey, parseInline, splitFlow and parseScalar.
- The new body works like this:
  - const lines = new LineCounter(); const doc = parseDocument(text, { schema: 'failsafe', lineCounter: lines, prettyErrors: false, uniqueKeys: true }); and at(offset, message) = new YamlError(\line ${lines.linePos(offset ?? 0).line}: ${message}`)`.
  - The first of doc.errors, then doc.warnings, throws at(problem.pos[0], problem.message).
  - A null doc.contents gives {}. A root that isn't a map, or has an anchor or tag, throws 'expected "key: value"'.
  - A helper plain(node) throws as follows. Anything with isAlias, .anchor or .tag throws 'unsupported YAML: anchors, aliases and tags'. A node that isn't a scalar throws 'nested lists and mappings are not supported'. A scalar of type BLOCK_LITERAL or BLOCK_FOLDED, or whose source text.slice(range[0], range[1]) holds a \n, throws 'multi-line values are not supported'. Otherwise it returns String(node.value).
  - A value scalar is null when it is PLAIN and '', ~ or null. A key is plain(key) and throws `"${key}" is not allowed as a key` for __proto__.
  - Each top-level value becomes, by its node: a seq (no anchor or tag) a list of value scalars; a map (no anchor or tag) a Record of value scalars; anything else a value scalar.
- Rewrite the comment above parseYaml. It says that YAML is read with the yaml library's failsafe schema, so every value is a string or null; that only the settings' subset is accepted (top-level keys whose values are a single value, a list or a mapping of single values); and that anchors, aliases, tags, multi-line values, more than one document and deeper nesting are refused, so a file never means something other than what it appears to.
- In settings.test.ts, in "parseYaml refuses what the subset does not support", remove ' indented: first' and add 'skip: !!str a', '---\na: b\n---\nc: d' and 'skip: "a\n b"'. Add two tests:
  - parseYaml keeps every value a string or null: parseYaml('a: 1\nb: true\nc: ~\nd: null\ne: "null"\nf:') deep-equals { a: '1', b: 'true', c: null, d: null, e: 'null', f: null }.
  - parseYaml reads what the yaml library allows beyond the old parser: indented keys and every escape: parseYaml(' skip: [a]') deep-equals { skip: ['a'] }, and parseYaml('skip: ["\\u0041"]') deep-equals { skip: ['A'] }.
Pattern: the prototype in Decisions. It was checked against every case in settings.test.ts.
Done when: if (Select-String -Path src/settings.ts -Pattern 'function splitFlow|function stripComment' -Quiet) { exit 1 }; npm run typecheck && npm test

[ ] 18. Check path items in one place

Files: src/settings.ts (edit)

Change:

Add the function below after reviewerName, and use it in validateConfig's list loop and in charterPaths' loop. Each problem is pushed as before, and item.trim() is still pushed otherwise.

// Why one item of a list of paths or globs can't be used, or undefined when it can.
function pathItemProblem(key: string, item: unknown): string | undefined {
  if (typeof item !== 'string' || !item.trim()) return `"${key}" has an empty item`;
  if (item.length > maxPathChars || /[\u0000-\u001f]/.test(item)) return `"${key}" has an item that is too long or has control characters`;
  return undefined;
}

Narrow item to string after the check with typeof item === 'string' where TypeScript needs it.

Pattern: the two loops it replaces.
Done when: if ((Select-String -Path src/settings.ts -Pattern 'too long or has control characters').Count -ne 1) { exit 1 }; npm run typecheck && npm test

[ ] 19. Say in the README how config.yml is read

Files: README.md (edit)
Change: Replace the first sentence of the paragraph that starts `config.yml` is a small subset of YAML with: `config.yml` is read with the [yaml](https://eemeli.org/yaml/) library, and only a small subset is accepted: top-level keys whose values are lists or, for `tiers:`, a mapping, in block or `[a, b]` style, with comments and quoted strings; anchors, aliases, tags, multi-line values and deeper nesting are refused. Keep the rest of the paragraph.
Pattern: the paragraph's own style.
Done when: if (-not (Select-String -Path README.md -Pattern 'is read with the \[yaml\]' -Quiet)) { exit 1 }

[ ] 20. Write the changelog entries

Files: CHANGELOG.md (edit)
Change: Under ## [Unreleased], add these bullets at the end of each existing group:
- ### Changed:
  - - The rest of the engine's GitHub reads go through the one client in src/github.ts, with its time limit and the runner's GitHub address: the open issues synthesis reads, the issues a description closes, permission lookups and the training's guidance issues. Outcomes and the publish step's dismissals use the engine's writer check, so who counts as a writer can't drift between them; a lookup GitHub answers with 404 counts as no write access without a warning. The review step reads a pull request's changes with git once for gather and the template check, and the finding marker is read in one place ([#95](https://github.com/NCodeGroup/code-review/issues/95)).
  - - .github/code-review/config.ymland a project reviewer's frontmatter are read with theyaml library, pinned to an exact version, accepting the same subset as before. A file whose keys are all indented the same now reads, a double-quoted string takes every YAML escape, and an error names its line followed by the library's own words ([#95](https://github.com/NCodeGroup/code-review/issues/95)).
- ### Fixed:
  - - A finding's outcome is read from the marker that ends its thread's first comment, as everything else reads it, so a one-click suggestion whose code holds a marker-like line can't change which finding the outcome records ([#95](https://github.com/NCodeGroup/code-review/issues/95)).
- ### Security:
  - - Model-written text in the bot's comments (the summary, finding threads, /review explainanswers, the outcomes table and still-open replies) can no longer mention anyone or post a link: outside code spans and fenced code blocks, a zero-width space follows each@that starts a mention and breaks](, ][, ]:, http://, https://andwww., and text whose code GitHub might read differently is escaped throughout. Code such as @OverRide in backticks stays as written ([#95](https://github.com/NCodeGroup/code-review/issues/95)).
Pattern: the existing #95 entries in ### Changed.
Done when: if (-not ((Select-String -Path CHANGELOG.md -Pattern 'zero-width space follows' -Quiet) -and (Select-String -Path CHANGELOG.md -Pattern 'read with the yaml library' -Quiet) -and (Select-String -Path CHANGELOG.md -Pattern 'marker that ends its thread' -Quiet))) { exit 1 }

Review checklist

test/acknowledge.test.ts still passes: no yaml import, and no writers.ts, reachable from acknowledge.ts or finish.ts.
No https://api.github.com string is left in src/ or training/ outside githubUrls().
writerCheck still checks the association first, caches each login, never looks up a bot's own reactions or replies (outcomes skip them first), and fails closed. Repliers stay capped at maxReplyAuthorLookups, and open-issue authors at maxIssueAuthorLookups.
gather runs rev-parse, merge-base and diff --numstat once per review, and gatherChecked reads the template only for a candidate.
neutralizeProse never leaves prose unescaped when GitHub could read it as prose. Check the fallbacks in particular, and that suggestion blocks and the bot's own template text are not passed through it.
parseYaml refuses everything the old parser refused except the recorded differences, and readConfig's warning keeps its shape.
package-lock.json changes only by adding yaml.

Follow-ups

Neutralize issue references in model text: neutralizeProse in src/markdown.ts leaves #123 and owner/repo#123 in model-written comments live, so an injection-steered finding can still post cross-reference backlinks on unrelated issues and pull requests; break them as mentions are broken, outside code.

Merge pull request #129 from NCodeGroup/refactor-95c

Follow-ups filed from this plan: #132 Neutralize issue references in model text

Navigation Menu

Repository navigation

Refactor: one GitHub client, one thread reader, shared escaping #95

Plan: Finish the engine refactor of #95

Goal

Context for the executor

Decisions

Out of scope

Steps

[ ] 1. Read the open issues through the shared GitHub client

[ ] 2. Read the closed issues through the shared GitHub client

[ ] 3. Look up permissions through the shared GitHub client

[ ] 4. Outcomes use the shared writer check

[ ] 5. The publish step's dismissals use the shared writer check

[ ] 6. The training reads the guidance issues through the shared GitHub client

[ ] 7. Type the paused notice's client as the shared Api

[ ] 8. Read the finding marker in one place

[ ] 9. Read the pull request's changes once for gather and the template check

[ ] 10. Fold the template check into one gather step

[ ] 11. Add neutralizeProse for model-written text

[ ] 12. Neutralize model text in finding threads

[ ] 13. Neutralize model text in the summary comment

[ ] 14. Neutralize model text in `/review explain` answers

[ ] 15. Neutralize the outcomes table and the still-open reply

[ ] 16. Add the yaml library

[ ] 17. Parse config.yml with the yaml library

[ ] 18. Check path items in one place

[ ] 19. Say in the README how config.yml is read

[ ] 20. Write the changelog entries

Review checklist

Follow-ups

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Refactor: one GitHub client, one thread reader, shared escaping #95

Description

Activity

polewskm commented on Oct 31, 2026

polewskm commented on Oct 31, 2026

polewskm commented on Oct 31, 2026

polewskm commented on Nov 1, 2026

polewskm commented on Nov 1, 2026

polewskm commented on Nov 1, 2026

polewskm commented on Nov 1, 2026

Plan: Finish the engine refactor of #95

Goal

Context for the executor

Decisions

Out of scope

Steps

[ ] 1. Read the open issues through the shared GitHub client

[ ] 2. Read the closed issues through the shared GitHub client

[ ] 3. Look up permissions through the shared GitHub client

[ ] 4. Outcomes use the shared writer check

[ ] 5. The publish step's dismissals use the shared writer check

[ ] 6. The training reads the guidance issues through the shared GitHub client

[ ] 7. Type the paused notice's client as the shared Api

[ ] 8. Read the finding marker in one place

[ ] 9. Read the pull request's changes once for gather and the template check

[ ] 10. Fold the template check into one gather step

[ ] 11. Add neutralizeProse for model-written text

[ ] 12. Neutralize model text in finding threads

[ ] 13. Neutralize model text in the summary comment

[ ] 14. Neutralize model text in /review explain answers

[ ] 15. Neutralize the outcomes table and the still-open reply

[ ] 16. Add the yaml library

[ ] 17. Parse config.yml with the yaml library

[ ] 18. Check path items in one place

[ ] 19. Say in the README how config.yml is read

[ ] 20. Write the changelog entries

Review checklist

Follow-ups

polewskm commented on Nov 1, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

[ ] 14. Neutralize model text in `/review explain` answers