diff --git a/README.md b/README.md index 46255dda..1e1eff0f 100644 --- a/README.md +++ b/README.md @@ -524,10 +524,18 @@ least 10 seconds for container cleanup, then sends SIGKILL on a later collection to `codex exec`; `--agent ` and repeatable `--agent-arg ` replace it for another CLI. It writes `/quest-land-result.json`: an array of `{number, action:"requeue", reason}`, `{number, action:"fixed", head, reason}`, or -`{number, action:"handback", diagnosis, recommendation}`, with no extra fields. +`{number, action:"handback", diagnosis, recommendation}`, with no extra fields +but an optional `comment`. The payload adds `agent` and `model` metadata to each result (`--model ` selects the default Codex model; unknown models fall back to `Codex`). Reasons and diagnoses are single lines. Fixed heads must be committed on `pr-`. +A `comment` is Markdown of at most 4000 characters that the fixer wants on its PR. +The fixer holds no GitHub credential, since PR code it builds can read the +container's environment, so the daemon posts the comment as the bot when it +collects the job, before its own outcome comment. It quotes the text under the +recorded agent, strips HTML comments so the text can never carry the daemon's +hidden markers, defuses @-mentions, and redacts GitHub tokens, as it does for +every fixer text and log it posts. Jobs and attempts persist across daemon restarts. A running fixer outlives a restart only where the host leaves its process group alive (under systemd, diff --git a/assets/land-fix.md b/assets/land-fix.md index 6b5c44b9..8b2f907d 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -51,6 +51,15 @@ one entry per supplied PR, no extra fields. Use one of: {"number":123,"action":"fixed","head":"full 40-character SHA","reason":"one-line mechanical reason"} {"number":123,"action":"handback","diagnosis":"one line","recommendation":"one line"} +You have no GitHub credential and must not seek one. To tell a PR's maintainer +something, such as what a fix changed and why, a flaky test, or the decision a +handback needs, add an optional `"comment"` field to that PR's entry: Markdown of +at most 4000 characters, one per PR. The daemon posts it on that PR, quoted under +your name, after it collects the result. Write plain prose with no @-mentions, +HTML comments, or secrets: the daemon strips mentions, HTML comments, and GitHub +tokens. A comment decides nothing: the action sets the PR's outcome, and the +daemon still posts its own outcome comment. + A fixed head must be the tip of pr-, descend from its pinned head, and contain only that PR's fix and base history. Leave all other PR branches intact. Keep the result file uncommitted. diff --git a/quest/m0/land-fixer/README.md b/quest/m0/land-fixer/README.md index b2c0bc5c..bd255df4 100644 --- a/quest/m0/land-fixer/README.md +++ b/quest/m0/land-fixer/README.md @@ -83,7 +83,13 @@ Decision prompts, selected options marked ✅: disable the flaky test in the base PR then." - The flaky commit: ✅ in this batch (on its base PR, per the answer above) · in the next batch - - GitHub comments: the daemon posts them · ✅ a comment-only token · both + - GitHub comments: ✅ the daemon posts them from the fixer's result · a + comment-only token in the container · both. The user first picked the + token, then reversed it on 2026-11-01 after an independent audit: PR code + the fixer builds can read the container's environment, so a token there + would let PR code post as moq-bot[bot]. `quest land apply`, the daemon's + own comment deduplication, and `quest status`'s ejection reason all trust + moq-bot comments. - Workflow pushes: ✅ grant moq-bot `workflows` · the fixer avoids workflows - The user, starting the work: "The fixer is meant to handle conflicts and @@ -100,7 +106,6 @@ resolves itself and what it hands back; that docs update finishes the epic. ## Required - [Workflow permission](/quest/m0/land-fixer/workflows-permission.md) - waits on the maintainer to grant moq-bot `workflows`, so pushes touching workflows are accepted -- [Fixer comments](/quest/m0/land-fixer/comments.md) - a per-job, comment-only GitHub token lets the fixer explain its work on the PRs - [Attempt budget](/quest/m0/land-fixer/attempts.md) - only a fix that fails again spends a pick's single attempt - [Batch triage](/quest/m0/land-fixer/triage.md) - the fixer blames the failing PR before ejection, and a red base halts the queue - [CI failures park](/quest/m0/land-fixer/ci-park.md) - a PR whose own required CI fails is parked for the fixer with the failing log, not ejected diff --git a/quest/m0/land-fixer/check-status.md b/quest/m0/land-fixer/check-status.md index 07944d33..81041b90 100644 --- a/quest/m0/land-fixer/check-status.md +++ b/quest/m0/land-fixer/check-status.md @@ -29,8 +29,8 @@ landing through its own push or the pinned squash merge, as now. - Needs the App's `checks: write`, an installation permission the maintainer accepts. Check it during this work; it may already be granted for the existing check. -- [Fixer comments](/quest/m0/land-fixer/comments.md) explain decisions on - the PR; the check run shows state only. +- The fixer's comments, which the daemon posts on the PR, explain + decisions; the check run shows state only. ## Related diff --git a/quest/m0/land-fixer/comments.md b/quest/m0/land-fixer/comments.md deleted file mode 100644 index 23740fc5..00000000 --- a/quest/m0/land-fixer/comments.md +++ /dev/null @@ -1,40 +0,0 @@ -# [S] The fixer comments on GitHub - -## Goal - -The fixer can post and edit comments on the batch's PRs, for example to -explain a fix, a disabled flaky test, or the decision it needs. It does this -with a GitHub installation token for that one repository with only -`issues: write`, which the daemon mints for each job and which expires with -it. The token cannot push code, merge, or reach any other repository. As the -maintainer accepted, it can also edit or close issues and change labels in -that repository. - -## Plan - -Decided 2026-11-01 (user, /quest-plan): "It's also allowed to comment on -Github." See the [epic](/quest/m0/land-fixer/README.md). - -- How the fixer gets to comment: - - The daemon posts the comments the fixer writes into its result, so no - GitHub credential enters the container (recommended, because PR code the - fixer builds can read the container's environment, as - [moq-dev/quest#356](https://github.com/moq-dev/quest/pull/356) found for - the proxy key) - - ✅ A comment-only token in the container - - Both - -- GitHub has no comment-only permission; `issues: write` also allows editing - and closing issues and changing labels, and PR code the fixer builds could - read the token (Codex review of this plan): - - A local comment command: the daemon keeps the credential and exposes - only comment operations inside the container (recommended) - - ✅ Accept `issues: write` in the container - - The daemon posts comments from the result - -Mint an installation token for the job's repository with -`permissions: { issues: "write" }`, which covers PR conversation comments, and -`repositories: []`. Pass it to the agent as `GH_TOKEN` only; the wrapper -keeps every other credential out, as today. GitHub caps a token at one hour, -so a longer job gets a fresh one or stops commenting. The daemon keeps posting -its own requeue and handback comments. diff --git a/src/cli/land-fix.ts b/src/cli/land-fix.ts index 072dce4e..def00ab9 100644 --- a/src/cli/land-fix.ts +++ b/src/cli/land-fix.ts @@ -1,4 +1,5 @@ -// Untrusted agent side. The host wrapper supplies only a model endpoint and its client key. +// Untrusted agent side. The host wrapper supplies only a model endpoint and its client key; the agent holds no +// GitHub credential, and the daemon posts the comments it writes into its result. import { spawn, spawnSync } from "node:child_process"; import { constants, closeSync, fstatSync, openSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { basename, join } from "node:path"; @@ -21,13 +22,15 @@ export function readFixResult(root: string): string { closeSync(fd); } } -export type FixResult = { agent?: string; model?: string } & ( +/** The longest Markdown comment a result may ask the daemon to post on its PR. */ +export const COMMENT_LIMIT = 4000; +export type FixResult = { agent?: string; model?: string; comment?: string } & ( | { number: number; action: "requeue"; reason: string } | { number: number; action: "fixed"; head: string; reason: string } | { number: number; action: "handback"; diagnosis: string; recommendation: string } ); -/** Reject extra fields, duplicate/missing PRs, multiline reasons, and oversized output. */ +/** Reject extra fields, duplicate/missing PRs, multiline reasons, oversized comments, and oversized output. */ export function parseFixResult(text: string, numbers: number[]): FixResult[] { if (text.length > 100_000) throw new Error("fixer result exceeds 100 KB"); let value: unknown; @@ -54,9 +57,19 @@ export function parseFixResult(text: string, numbers: number[]): FixResult[] { const metadata = ["agent", "model"].filter((key) => key in item); if (metadata.some((key) => typeof item[key] !== "string" || !/^[a-zA-Z0-9 ._/-]{1,100}$/.test(item[key]))) throw new Error("invalid fixer identity"); + // One optional Markdown comment per PR: multiline, but no other control characters. + if ( + "comment" in item && + (typeof item.comment !== "string" || + !item.comment.trim() || + item.comment.length > COMMENT_LIMIT || + /[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/.test(item.comment)) + ) + throw new Error("invalid fixer comment"); + const optional = metadata.length + ("comment" in item ? 1 : 0); if ( fields.length === 0 || - Object.keys(item).length !== fields.length + metadata.length || + Object.keys(item).length !== fields.length + optional || fields.some((key) => !(key in item)) ) throw new Error("invalid fixer action or fields"); diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index 0b55a3e3..3d53114f 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -95,14 +95,14 @@ function identity(pid: number): string | undefined { return undefined; } } -async function comment( +/** Post `content` once per pick and kind: a retry finds the bot's own marker and posts nothing. */ +async function post( github: GitHub, plan: Plan, entry: Pick, - text: string, + content: string, suffix: string, - attribution = "Codex", - log = "", + attribution: string, section?: (room: number) => string, ): Promise { const path = `/repos/${plan.repository}/issues/${entry.number}/comments`; @@ -114,10 +114,22 @@ async function comment( ) ) return; - const lead = `${attribution} land fixer: ${fixerText(text)}${log ? `\n\n
${excerpt(log)}
` : ""}`; const tail = `\n\n${marker}\n\n(written by ${attribution})`; - const extra = section?.(COMMENT_LIMIT - lead.length - tail.length) ?? ""; - await github.request(path, { body: { body: `${lead}${extra}${tail}` } }); + const extra = section?.(COMMENT_LIMIT - content.length - tail.length) ?? ""; + await github.request(path, { body: { body: `${content}${extra}${tail}` } }); +} +async function comment( + github: GitHub, + plan: Plan, + entry: Pick, + text: string, + suffix: string, + attribution = "Codex", + log = "", + section?: (room: number) => string, +): Promise { + const content = `${attribution} land fixer: ${fixerText(text)}${log ? `\n\n
${excerpt(redactTokens(log))}
` : ""}`; + await post(github, plan, entry, content, suffix, attribution, section); } /** Queue eligibility can eject a previously requeued PR before building another batch. */ @@ -286,9 +298,29 @@ export function validateFix(root: string, entry: Entry, head: string, base: stri throw new Error("off-limits fixer edit: protected set must equal pinned head or base merge result"); } +/** Anything the fixer writes may echo a token it read; GitHub tokens never reach a post. */ +export function redactTokens(text: string): string { + return text.replace(/\b(?:gh[oprsu]_|github_pat_)[A-Za-z0-9_]{20,}/g, "[redacted token]"); +} + +/** + * The fixer's own Markdown comment, quoted under its name. It cannot carry a hidden marker, so it never + * passes for the daemon's handback, ejection, or apply comments, and it mentions no one. + */ +export function fixerMarkdown(text: string): string { + let value = redactTokens(text); + // Removing one opener can join the text around it into another. + while (value.includes("\\n\\n@person saw ${token}\\n\`\`\`\\nunclosed"}] +EOF`); + await fixture.finished; + expect(await fixture.collect()).toEqual(["requeued 1"]); + const [note, outcome] = fixture.pull.comments!.slice(-2).map((comment) => comment.body); + expect(outcome).toContain("Requeued unchanged:"); + expect(note).toContain("land fixer note:\n\n> The check `retry` flaked.\n> \n"); + // The daemon's own marker is the only hidden line. + expect(note.match(/`); + expect(EJECTED.exec(note)).toBeNull(); + expect(note).toContain("[at]person saw [redacted token]"); + expect(note).not.toContain(token); + // An unclosed fence ends with the quote, before the daemon's marker and attribution. + expect(note).toMatch(/> unclosed\n\n\n\n\(written by /); + }); + test("never posts a GitHub token the fixer echoes in its text or log", async () => { + const token = `ghs_${"A1b2".repeat(9)}`; + const fixture = await fixerFixture( + `echo "token ${token}" +printf '[{"number":1,"action":"handback","diagnosis":"saw %s","recommendation":"inspect manually"}]' '${token}' > quest-land-result.json`, + ); + await fixture.finished; + await fixture.collect(); + const bodies = fixture.pull.comments!.map((comment) => comment.body).join("\n"); + expect(bodies).toContain("[redacted token]"); + expect(bodies).not.toContain(token); + expect(redactTokens(`a ${token} github_pat_${"x".repeat(30)} b`)).toBe("a [redacted token] [redacted token] b"); + }); + test("quotes the fixer's Markdown so no hidden comment survives", () => { + for (const text of ["", "<", "a