diff --git a/docs/architecture.md b/docs/architecture.md index c4a494bb..996f8748 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -168,7 +168,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Review service | `runner/review.ts` | Builds the review view: reads history, links it to the plan, computes approval states and merge blockers. Applies review commands through the store. | In use | | Ask | `runner/questions.ts`, `runner/question-*.ts` | Answers a question about one plan item with a read-only Claude agent (Codex is refused in every phase, #93). Runs lane D's setup in a worker thread so the server stays responsive. Ask and planning share one read-only container runner (`runReadOnlyAgent`, #117) but keep separate workers, owners and leftovers. Labels its Docker objects with a per-database Ask owner and recovers only its own leftovers, so reviews sharing one Docker daemon do not block each other (#95). | In use | | Merge coordinator | `runner/merge.ts` | Runs the full merge gate twice, re-reads the local generation, then merges the exact reviewed head. Tracks merge-queue attempts. For a runner task it inspects and merges the task's own published pull request; a configured `github.pullRequest` that differs is refused (#121). | In use | -| Planning agent | `runner/planning.ts`, `web/planning.ts`, `runner/planning-provider.ts` | Runs plan suggestions and drafts with Claude in lane D's planning phase, on a read-only copy of the current head, in its own worker with its own leftovers ledger and owner token. The request and its timer share one 10-minute budget. Issue text is read with collaborators' comments only. | In use for a non-demo review with a `github` block | +| Planning agent | `runner/planning.ts`, `web/planning.ts`, `runner/planning-provider.ts` | Runs plan suggestions and drafts with Claude in lane D's planning phase, on a read-only copy of the current head, in its own worker with its own leftovers ledger and owner token. The request and its timer share one 10-minute budget. Issue text includes current collaborators' comments, or every comment under explicit author-bound trust. | In use for a non-demo review with a `github` block | | Runner coordinator | `runner/coordinator.ts`, `runner/lifecycle.ts` | Attempt admission, concurrency slots, compare-and-swap on results, retries, shutdown order. A stop made inside a user action takes effect only after that action's transaction commits (`Store.afterCommit`, #96). | In use with the opt-in `runner` block (#91) | | Execution | `runner/execution.ts` | Runs plan items in order: fresh workspace, prompt, agent, post-run audit, then the runner's own commit and ledger entry. Before launch, D's tree check (`checkTaskTree`) must pass. Start and resume require current approvals, own the review version across the run, recheck approvals before later items, and verify that a completed prefix still ends at the current head (#107). Pauses in needs amendment on an out-of-scope edit. A safety violation is saved before its terminal write and remains durably owed when a human gate delays escalation; a failed run is audited too. | In use with the opt-in `runner` block; started by `start` and `resume` on `/api/runner` (#91, #107, PRs #105 and #130) | | Workspace | `runner/workspace.ts`, `runner/runner-repository.ts` | The real lane D workspace. A runner-owned bare repository (owner-only directories) fetches base commits by ID and takes in each verified commit bundle under a per-attempt ref. Review and Ask read a task with runner commits from there, at the head the Store recorded. | In use with the opt-in `runner` block (#91) | diff --git a/docs/implementation/issue-prioritization.md b/docs/implementation/issue-prioritization.md index 99406702..596b2f09 100644 --- a/docs/implementation/issue-prioritization.md +++ b/docs/implementation/issue-prioritization.md @@ -108,8 +108,7 @@ The status line says one of: - "✕ Unavailable": no list has been retrieved yet, with the error; - "– Not configured": the review configuration has no `github.repository`. -A note says that trusting issues and queueing are not available yet. Demo mode -shows fixture issues and never contacts GitHub. +Demo mode shows fixture issues and never contacts GitHub. **State holders.** @@ -130,3 +129,42 @@ unconfigured state. `test/browser/issues.spec.ts` covers ranked order, reasons, trust marks, `aria-current` navigation, review input kept across navigation, review shortcuts ignored on the Issues screen, the unavailable → current → stale sequence, a late refresh after leaving the screen, and the 1280px layout. + +## H4b: Trust this issue + +**Decision and scope.** A trust decision is bound to the lower-cased repository identity, issue number and the issue's +current author login. It applies to planning and execute prompts. Revoking trust does not interrupt an invocation that +already started; every later plan-item admission, planning read and publish evaluates the new decision. Publishing is +guarded too, so a revoked decision cannot cross the next irreversible boundary. + +**Durable state.** Schema v17 adds one `issue_trust` row per repository and issue, retaining who decided, when, the +author that was observed, and a revocation time. Changing or deleting the GitHub author makes the row inapplicable. +Trust and untrust requests carry UUID v4 action IDs and use the ordinary durable action replay before GitHub is read. +Definite GitHub read failures are saved too and replay with their upstream-failure classification; shutdown remains +resendable. Concurrent callers with one action ID all observe the first durable outcome. +Each execute attempt also stores the SHA-256 digest and count of the exact comment strings put in its prompt. The +evidence is durably `prepared` after every pre-launch check and before the launcher receives the prompt, then becomes +`delivered` only after the launcher returns an owned handle; recovery can therefore distinguish either crash window. + +**Admission.** Start, resume, continuation approval and every plan item fetch the issue author and the complete current +collaborator list under a bounded GitHub read. A collaborator-authored issue passes without a local decision. Every +other issue needs a live, unrevoked row for that exact repository, number and author. A prompt text read revalidates the +admitted author and collaborator result against the issue and collaborator snapshot used for that text, closing the gap +between authorization and prompt construction. When explicit trust widened the comments, its author-bound Store row is +re-read immediately after the awaited text fetch so revocation cannot admit the stale all-comments result. The returned +execute source carries the same synchronous guard into `prepareExecution`, and the planning description carries it +into the recorded user action; each runs in the same turn immediately before its prompt is constructed. Malformed, +partial, failed and over-limit reads fail closed. Action-time +failures are saved under the action ID; per-item failures settle that attempt without admitting the next item. The task +and review versions are still checked in the same transaction that admits an attempt, after the external read. + +**Comments.** Without matching explicit trust, only current collaborators' comments enter planning and execute prompts. +With matching trust, every bounded comment enters the same untrusted-data block, including comments from deleted +accounts. The issue author is re-read with the comments, so a decision for an earlier author cannot widen the prompt. + +**Screen and demo.** The Issues table offers `Trust this issue` for outside authors, `Trust all comments` for current +collaborators, and `Remove trust` for either explicit decision. The collaborator's author-bound decision widens comment +access without changing its already-eligible status. While a request is in flight, the focused control remains +enabled for focus purposes, uses `aria-disabled`, and ignores repeat activation. Responses merge only their own row; +overlapping refreshes and trust requests cannot overwrite a committed decision or reset another control. Demo fixtures +use the same Store and API, but resolve author and collaborator state locally and never contact GitHub. diff --git a/github/issues.ts b/github/issues.ts index de528b28..174e70ab 100644 --- a/github/issues.ts +++ b/github/issues.ts @@ -15,6 +15,24 @@ export const ISSUE_PAGE_MAX_BYTES = 64 * 1024 * 1024; * to 0.75 s later: 12.75 s, below the 15-second serving request budget. */ export const ISSUE_KILL_GRACE_MS = 500, ISSUE_PIPE_GRACE_MS = 250; +/** One sequential access-and-text operation, including the subprocess settlement tail after its active-work abort. */ +export const ISSUE_READ_TIMEOUT_MS = 30_000; +export const ISSUE_READ_ACTIVE_MS = ISSUE_READ_TIMEOUT_MS - ISSUE_KILL_GRACE_MS - ISSUE_PIPE_GRACE_MS; + +/** Shares one active-work deadline across every sequential stage and awaits the caller's work through settlement. */ +export async function withIssueReadDeadline(signal: AbortSignal, + read: (sharedSignal: AbortSignal, activeTimeoutMs: number) => Promise): Promise { + signal.throwIfAborted(); + const deadline = new AbortController(); + const shared = AbortSignal.any([signal, deadline.signal]); + const timer = setTimeout(() => deadline.abort(new Error('Issue retrieval timed out.')), ISSUE_READ_ACTIVE_MS); + try { + const value = await read(shared, ISSUE_READ_ACTIVE_MS); + shared.throwIfAborted(); + return value; + } + finally { clearTimeout(timer); } +} export type IssueAuthorAssociation = | 'OWNER' | 'MEMBER' | 'COLLABORATOR' | 'CONTRIBUTOR' @@ -44,12 +62,21 @@ export interface IssueSnapshot { /** The issue text an execute prompt carries, as untrusted data. */ export interface IssueText { readonly number: number; readonly title: string; readonly body: string; readonly comments: readonly string[] } +export interface IssueAccess { readonly number: number; readonly authorLogin: string | null; readonly collaborator: boolean } export interface IssueGateway { readonly repository: string; fetch(options?: { signal?: AbortSignal; timeoutMs?: number }): Promise; } +/** The extra current-issue reads used by trust actions and runner admission. */ +export interface IssueTrustGateway extends IssueGateway { + issueAccess(number: number, options?: { signal?: AbortSignal; timeoutMs?: number }): Promise; + issueText(number: number, options?: { signal?: AbortSignal; timeoutMs?: number; trustedAuthor?: string | null; + /** Revalidate the admission read against the issue and collaborator snapshot used for this text read. */ + expectedAccess?: IssueAccess }): Promise; +} + type RunGh = (args: readonly string[], options?: { signal?: AbortSignal }) => Promise; const associations = new Set([ @@ -233,12 +260,39 @@ export class GhIssueGateway implements IssueGateway { })); } + async #loadIssueAuthor(number: number, signal: AbortSignal): Promise { + let decoded: unknown; + const output = await this.run(['api', '--method', 'GET', '-H', 'Accept: application/vnd.github+json', `repos/${this.repository}/issues/${number}`], { signal }); + try { decoded = JSON.parse(output); } + catch { throw new Error('GitHub returned invalid issue JSON.'); } + const issue = object(decoded, 'GitHub returned a malformed issue.'); + if (issue.number !== number) throw new Error('GitHub returned a different issue.'); + if (Object.hasOwn(issue, 'pull_request')) throw new Error(`#${number} is a pull request, not an issue.`); + return issue.user === null ? null : login(object(issue.user, 'GitHub returned an invalid issue author.').login, 'issue author'); + } + + async #loadIssueAccess(number: number, signal: AbortSignal): Promise<{ access: IssueAccess; collaborators: ReadonlySet }> { + const authorLogin = await this.#loadIssueAuthor(number, signal); + const collaborators = await this.#loadCollaborators(signal); + const currentAuthor = await this.#loadIssueAuthor(number, signal); + if (currentAuthor !== authorLogin) throw new Error(`Issue #${number}'s author changed during access verification.`); + return { access: { number, authorLogin: currentAuthor, + collaborator: currentAuthor !== null && collaborators.has(currentAuthor.toLocaleLowerCase('en-US')) }, collaborators }; + } + + async issueAccess(number: number, options: { signal?: AbortSignal; timeoutMs?: number } = {}): Promise { + if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid issue number.'); + return this.#bounded(options, async signal => (await this.#loadIssueAccess(number, signal)).access); + } + /** - * One issue's text for an execute prompt (#91): its title, body and the comments of repository collaborators only - * (design, "Which comments reach the agent"), oldest first. Everything stays untrusted data inside the prompt. + * One issue's text for an execute prompt (#91): its title, body and the comments permitted by current collaborator + * access or explicit author-bound trust (design, "Which comments reach the agent"), oldest first. + * Everything stays untrusted data inside the prompt. * Fails closed on anything malformed, on a pull request, and past the comment page limit. */ - async issueText(number: number, options: { signal?: AbortSignal; timeoutMs?: number } = {}): Promise { + async issueText(number: number, options: { signal?: AbortSignal; timeoutMs?: number; trustedAuthor?: string | null; + expectedAccess?: IssueAccess } = {}): Promise { if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid issue number.'); return this.#bounded(options, async signal => { let decoded: unknown; @@ -249,10 +303,12 @@ export class GhIssueGateway implements IssueGateway { if (issue.number !== number) throw new Error('GitHub returned a different issue.'); if (Object.hasOwn(issue, 'pull_request')) throw new Error(`#${number} is a pull request, not an issue.`); const title = boundedString(issue.title, 'title', 4096), body = boundedString(issue.body, 'body', MAX_BODY_LENGTH, true); + const authorLogin = issue.user === null ? null : login(object(issue.user, 'GitHub returned an invalid issue author.').login, 'issue author'); + const includeEveryComment = options.trustedAuthor !== undefined && options.trustedAuthor === authorLogin; // The execute prompt carries the issue as one JSON data block of at most MAX_PROMPT_BYTES (dataJSON). A running byte - // count stops reading early (a title and body already over it read no collaborator or comment page); the exact check + // count stops reading early (a title and body already over it read no included comment page); the exact check // below uses the prompt's own serializer, so an issue accepted here is one the prompt can carry. - const tooLong = () => new Error(`Issue #${number}'s title, body and collaborator comments are larger than the ${MAX_PROMPT_BYTES / 1024} KiB an execute prompt carries; codeboost does not cut an issue to fit.`); + const tooLong = () => new Error(`Issue #${number}'s title, body and included comments are larger than the ${MAX_PROMPT_BYTES / 1024} KiB an execute prompt carries; codeboost does not cut an issue to fit.`); // Only the size refusal is reworded; any other (text with a NUL, for one) keeps its own reason. const carried = (text: IssueText) => { try { dataJSON(text, 'Issue data'); } catch (error) { throw /exceeds/.test((error as Error).message) ? tooLong() : error; } @@ -260,8 +316,15 @@ export class GhIssueGateway implements IssueGateway { // The title and body alone, as the prompt serializes them (escaping included): an issue that cannot fit reads nothing more. carried({ number, title, body, comments: [] }); let total = Buffer.byteLength(title) + Buffer.byteLength(body); - const collaborators = await this.#loadCollaborators(signal); - const comments: string[] = []; + const collaborators = includeEveryComment && !options.expectedAccess ? null : await this.#loadCollaborators(signal); + if (options.expectedAccess) { + const collaborator = authorLogin !== null && collaborators!.has(authorLogin.toLocaleLowerCase('en-US')); + const currentAuthor = await this.#loadIssueAuthor(number, signal); + if (currentAuthor !== authorLogin || options.expectedAccess.number !== number || options.expectedAccess.authorLogin !== currentAuthor + || options.expectedAccess.collaborator !== collaborator) + throw new Error(`Issue #${number}'s author or collaborator access changed during admission.`); + } + const comments: string[] = [], includedCommentAuthors = new Set(); for (let page = 1; ; page++) { const listed = await this.run(['api', '--method', 'GET', '-H', 'Accept: application/vnd.github+json', `repos/${this.repository}/issues/${number}/comments`, '-f', `per_page=${PAGE_SIZE}`, '-f', `page=${page}`], { signal }); @@ -275,9 +338,13 @@ export class GhIssueGateway implements IssueGateway { const comment = object(value, 'GitHub returned a malformed comment.'); // A deleted ("ghost") author is nobody's collaborator. Other people's comments are dropped before their body is // checked, so none of theirs can make the issue unreadable. - if (comment.user === null) continue; - const author = login(object(comment.user, 'GitHub returned an invalid comment author.').login, 'issue author'); - if (!collaborators.has(author.toLocaleLowerCase('en-US'))) continue; + if (!includeEveryComment) { + if (comment.user === null) continue; + const author = login(object(comment.user, 'GitHub returned an invalid comment author.').login, 'issue author'); + const authorKey = author.toLocaleLowerCase('en-US'); + if (!collaborators!.has(authorKey)) continue; + includedCommentAuthors.add(authorKey); + } const text = boundedString(comment.body, 'comment', MAX_BODY_LENGTH, true); total += Buffer.byteLength(text); if (total > MAX_PROMPT_BYTES) throw tooLong(); @@ -287,6 +354,13 @@ export class GhIssueGateway implements IssueGateway { } const text = { number, title, body, comments }; carried(text); + if (options.expectedAccess) { + const current = await this.#loadIssueAccess(number, signal); + if (current.access.number !== options.expectedAccess.number || current.access.authorLogin !== options.expectedAccess.authorLogin + || current.access.collaborator !== options.expectedAccess.collaborator + || [...includedCommentAuthors].some(author => !current.collaborators.has(author))) + throw new Error(`Issue #${number}'s author or collaborator access changed during admission.`); + } return text; }); } diff --git a/github/pull-requests.ts b/github/pull-requests.ts index a2cb9196..7a7fa915 100644 --- a/github/pull-requests.ts +++ b/github/pull-requests.ts @@ -4,6 +4,7 @@ import { BRANCH, REPOSITORY, SHA } from './validate.ts'; /** A `gh` runner that can also write a request body to stdin (`gh api --input -`). */ export type RunGhWithInput = (args: readonly string[], options?: { signal?: AbortSignal; input?: string }) => Promise; +type MutationBoundary = () => void | Promise void)>; export interface OpenPullRequestInput { base: string; @@ -13,6 +14,8 @@ export interface OpenPullRequestInput { body: string; draft: boolean; marker: string; + /** Runs after validation and immediately before the irreversible open request. */ + beforeOpen?: MutationBoundary; } export interface OpenedPullRequest { number: number; url: string; headSha: string; draft: boolean } export interface PullRequestGateway { @@ -40,8 +43,8 @@ export interface PullRequestGateway { */ readPull(number: number, signal?: AbortSignal): Promise<{ open: boolean; headBranch: string; base: string; marker: string }>; /** Replaces the title and description of an open PR codeboost opened; marks it ready when `ready`, or a draft when `draft`. */ - /** `beforeReady` runs after the description update's await and before any ready or draft change; if it throws, no such change is made. */ - refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string; beforeReady?: () => void }, signal?: AbortSignal): Promise; + /** Boundary callbacks are awaited immediately before their named mutation; if one throws, that mutation is not made. */ + refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string; beforePatch?: MutationBoundary; beforeReady?: MutationBoundary }, signal?: AbortSignal): Promise; /** * Closes a PR codeboost opened (#111), read by its number (GitHub's list may lag behind it). An open PR must still be from * `headBranch` in this repository with `marker` on its first line, or it is refused (`PullRequestMisplaced`) and left @@ -50,7 +53,7 @@ export interface PullRequestGateway { */ close(number: number, input: { headBranch: string; marker: string; beforeClose?: () => void }, signal?: AbortSignal): Promise<{ number: number; url: string }>; /** Turns an open PR codeboost opened back into a draft; a no-op for a draft. */ - markDraft(number: number, input: { base: string; headBranch: string; marker: string }, signal?: AbortSignal): Promise; + markDraft(number: number, input: { base: string; headBranch: string; marker: string; beforeDraft?: MutationBoundary }, signal?: AbortSignal): Promise; } /** * GitHub refused a draft because the repository does not support draft PRs (for example a private repository on the @@ -164,6 +167,8 @@ export class GhPullRequestGateway implements PullRequestGateway { signal = this.#bounded(signal); this.#validate(input); if (markerOf(input.body) !== input.marker) throw new Error('The pull request description must start with its marker.'); + const finalize = await input.beforeOpen?.(); + finalize?.(); const post = () => this.#json(['api', '-X', 'POST', '-H', 'Accept: application/vnd.github+json', `repos/${this.repository}/pulls`], signal, { title: input.title, body: input.body, head: input.headBranch, base: input.base, draft: input.draft }); let response; @@ -248,16 +253,19 @@ export class GhPullRequestGateway implements PullRequestGateway { return { ...pr, marker: found[0]! }; } - async refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string; beforeReady?: () => void }, signal?: AbortSignal): Promise { + async refresh(number: number, input: OpenPullRequestInput & { ready: boolean; headSha?: string; beforePatch?: MutationBoundary; beforeReady?: MutationBoundary }, signal?: AbortSignal): Promise { signal = this.#bounded(signal); this.#validate(input); if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid pull request number.'); if (markerOf(input.body) !== input.marker) throw new Error('The pull request description must start with its marker.'); + const finalizePatch = await input.beforePatch?.(); + finalizePatch?.(); const patched = this.#pull(await this.#json(['api', '-X', 'PATCH', '-H', 'Accept: application/vnd.github+json', `repos/${this.repository}/pulls/${number}`], signal, { title: input.title, body: input.body }), input); if (patched.number !== number || markerOf(patched.body) !== input.marker) throw new Error('GitHub returned a different pull request.'); // The caller's re-check after the PATCH's await: a task change during it must not lead to a ready change. - input.beforeReady?.(); + const finalizeReady = await input.beforeReady?.(); + finalizeReady?.(); // A ready PR whose task went back to needs human becomes a draft again; a draft whose task is ready leaves draft. if (input.ready && patched.draft) await this.run(['pr', 'ready', String(number), '--repo', this.repository], { signal }); else if (input.draft && !patched.draft) await draftCall(() => this.run(['pr', 'ready', String(number), '--undo', '--repo', this.repository], { signal })); @@ -296,10 +304,12 @@ export class GhPullRequestGateway implements PullRequestGateway { } /** The caller has just read the PR as ready, so this changes it straight away and reads the result back once. */ - async markDraft(number: number, input: { base: string; headBranch: string; marker: string }, signal?: AbortSignal): Promise { + async markDraft(number: number, input: { base: string; headBranch: string; marker: string; beforeDraft?: MutationBoundary }, signal?: AbortSignal): Promise { signal = this.#bounded(signal); this.#validate(input); if (!Number.isSafeInteger(number) || number < 1) throw new Error('Invalid pull request number.'); + const finalize = await input.beforeDraft?.(); + finalize?.(); let refused: unknown = null; try { await draftCall(() => this.run(['pr', 'ready', String(number), '--undo', '--repo', this.repository], { signal })); } catch (error) { if (error instanceof DraftsUnsupported || signal?.aborted) throw error; refused = error; } diff --git a/runner/branch-push.ts b/runner/branch-push.ts index f878f993..d513bcea 100644 --- a/runner/branch-push.ts +++ b/runner/branch-push.ts @@ -2,7 +2,7 @@ import type { PlanIdentity } from '../core/identity.ts'; import { runInProcessGroup } from '../agents/process-group.ts'; import { GIT_OPTIONS, gitEnvironment } from '../git/clone.ts'; import { ghEnvironment } from '../github/gh-env.ts'; -import type { BranchPusher } from './publish.ts'; +import type { BranchPusher, MutationBoundary } from './publish.ts'; import type { GitCallOptions, RunnerRepository } from './runner-repository.ts'; /** @@ -106,7 +106,7 @@ export class GitBranchPusher implements BranchPusher { this.#config = config; this.#url = url; } - async push(identity: PlanIdentity, input: { head: string; branch: string }, signal?: AbortSignal): Promise { + async push(identity: PlanIdentity, input: { head: string; branch: string; beforePush?: MutationBoundary }, signal?: AbortSignal): Promise { signal?.throwIfAborted(); if (identity.repositoryId !== this.#config.repositoryId) throw new Error('The task belongs to another repository.'); if (!COMMIT_ID.test(input.head)) throw new Error('A full commit ID is required.'); @@ -123,6 +123,8 @@ export class GitBranchPusher implements BranchPusher { if (remote !== null && !owned.has(remote)) throw new BranchPushRefused(`The branch ${input.branch} holds commit ${remote}, which codeboost did not make. Nothing was pushed.`); signal?.throwIfAborted(); + const finalize = await input.beforePush?.(); + finalize?.(); // Leased to the value read above (pushArguments). try { await this.#git(pushArguments({ url: this.#url, ref, read: remote, head: input.head }), signal, true); diff --git a/runner/coordinator.ts b/runner/coordinator.ts index 5ed39532..84c44b7b 100644 --- a/runner/coordinator.ts +++ b/runner/coordinator.ts @@ -44,8 +44,15 @@ export interface RunnerDeps { * Task storage waits for the terminal write. */ cleanupPreparation(attempt: AttemptRecord): Promise; + /** Synchronous fail-closed evidence written after every preparation check and before D can receive the prompt. */ + beforeStart?(attempt: AttemptRecord, prepared: PreparedAttempt): void; /** D's start call: returns a handle at once, or throws with nothing left running. */ start(input: InvocationInput, prepared: PreparedAttempt): InvocationHandle; + /** + * Synchronous durable evidence for a launch that returned a handle. It runs after the coordinator owns that handle + * and before the running transition; a failure cancels and awaits D, then retains the slot as start-not-saved. + */ + onStarted?(attempt: AttemptRecord, prepared: PreparedAttempt): void; /** Validate a clean result; throw with an actionable reason if it is invalid. Returns the value to persist. */ validate(attempt: AttemptRecord, result: InvocationResult): unknown; /** @@ -318,11 +325,15 @@ export class RunnerCoordinator { try { const input = captureInvocation({ clone: prepared.clone, phase: ATTEMPT_PHASES[attempt.kind], vendor: prepared.vendor, approvedArgv: prepared.approvedArgv, deadline: attempt.deadline, attemptId: attempt.id, runnerOwner: this.#deps.runnerOwner, context: attempt.context }, now); + this.#deps.beforeStart?.(attempt, prepared); handle = this.#deps.start(input, prepared); } catch (error) { return await this.#endBeforeLaunch(job, attempt, { detail: `Launch failed: ${message(error)}` }, prepared); } job.handle = handle; let running: boolean | undefined; - try { running = this.#write(() => this.#store.markRunning(job.identity, attempt.id)); } catch { running = undefined; } + try { + this.#deps.onStarted?.(attempt, prepared); + running = this.#write(() => this.#store.markRunning(job.identity, attempt.id)); + } catch { running = undefined; } if (running === undefined) { // A storage error, not a stop: keep ownership until D settles, then hold the slot under a marker. handle.cancel('capture-failure'); diff --git a/runner/execution.ts b/runner/execution.ts index 3f2e2f77..80a21b05 100644 --- a/runner/execution.ts +++ b/runner/execution.ts @@ -1,4 +1,5 @@ import { identityKey, type PlanIdentity } from '../core/identity.ts'; +import { createHash } from 'node:crypto'; import type { PlanContext } from '../core/plan.ts'; import type { InvocationContext, InvocationHandle, InvocationInput, TaskClone } from '../agents/contract.ts'; import { prepareExecution } from '../core/execution-prompt.ts'; @@ -54,13 +55,15 @@ export interface TaskWorkspace { } /** D's start call for an execute/fix phase with this prompt and the tree check made for it; returns at once (see #51). */ export type AgentLauncher = (input: InvocationInput, prompt: string, workspace: WorkspaceRef, treeCheck: TaskTreeCheck) => InvocationHandle; +/** Issue text plus the synchronous authorization check that must run in the same turn as prompt construction. */ +export interface GuardedIssueText { readonly text: IssueText; validate(): void } /** Trusted runner-side sources for a task. Issue text and lessons are untrusted data inside the prompt. */ export interface ExecutionSources { planContext(identity: PlanIdentity): PlanContext; /** Entries of the audited runner commit, read from its immutable tree after export. */ checkpointContext(identity: PlanIdentity, head: string): PlanContext; - /** The issue text the prompt carries; fetched per attempt, so it may await (and must stop on abort). */ - issue(identity: PlanIdentity, signal: AbortSignal): IssueText | Promise; + /** Fetched per attempt; its guard is re-run synchronously at prompt construction after the await. */ + issue(identity: PlanIdentity, signal: AbortSignal): GuardedIssueText | Promise; lessons(identity: PlanIdentity): readonly string[]; vendor(identity: PlanIdentity): 'claude' | 'codex'; } @@ -120,7 +123,7 @@ export class SafetyFindings { } export interface ExecutionResult { head: string; unchanged: boolean; inScope: string[]; outOfScope: string[] } interface Private { workspace: WorkspaceRef; prompt: string; baseHead: string; linkSnapshot: DeclaredLinkSnapshot | undefined; - treeCheck: TaskTreeCheck | undefined } + treeCheck: TaskTreeCheck | undefined; promptComments: { count: number; digest: string } } /** * RunnerDeps for execute attempts: fresh workspace, prompt, agent, then audit and the runner's own commit. @@ -170,14 +173,20 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag const vendor = sources.vendor(identity); // D refuses Codex in phases it cannot work in (#93); refuse here too, before any GitHub call or task storage. if (vendor === 'codex') assertCodexPhase('execute'); - const issue = await sources.issue(identity, signal); + const guardedIssue = await sources.issue(identity, signal); signal.throwIfAborted(); + guardedIssue.validate(); + const issue = guardedIssue.text; const request = prepareExecution({ identity, attemptId: attempt.id, mode: 'execute', plan, itemId: item.id, issue, approvedLessons: sources.lessons(identity), allowedCommands: context.allowedCommands }); + const promptComments = { + count: issue.comments.length, + digest: createHash('sha256').update(JSON.stringify(issue.comments)).digest('hex'), + }; const declaredPaths = [...new Set(item.files.flatMap(file => [file.path, ...(file.renamed_from ? [file.renamed_from] : [])]))]; const ws = await workspace.materialize(attempt, baseHead, signal); // From here task storage exists: a failure hands it to the coordinator, which removes it after the terminal write. - const data: Private = { workspace: ws, prompt: request.prompt, baseHead, linkSnapshot: undefined, treeCheck: undefined }; + const data: Private = { workspace: ws, prompt: request.prompt, baseHead, linkSnapshot: undefined, treeCheck: undefined, promptComments }; const prepared = { clone: ws.clone, vendor, approvedArgv: request.approvedArgv, private: data }; try { data.linkSnapshot = await workspace.snapshotDeclaredLinks(ws, declaredPaths, signal); } catch (error) { throw new PreparationFailure(error, prepared); } @@ -206,7 +215,12 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag }, // Host-side files only (the staging clone); task storage waits for release. async cleanupPreparation(attempt) { await workspace.cleanupPreparation?.(attempt); }, + beforeStart(attempt, prepared) { + const data = prepared.private as Private; + store.recordAttemptComments(identityOf(attempt), attempt.id, data.promptComments); + }, start(input, prepared) { const data = prepared.private as Private; return launch(input, data.prompt, data.workspace, data.treeCheck!); }, + onStarted(attempt) { store.markAttemptCommentsDelivered(identityOf(attempt), attempt.id); }, validate() { throw new Error('Execute attempts publish through finish().'); }, async finish(attempt, _result, prepared, signal) { const data = prepared.private as Private, identity = identityOf(attempt); diff --git a/runner/lifecycle.ts b/runner/lifecycle.ts index e2029a17..cafcd7e6 100644 --- a/runner/lifecycle.ts +++ b/runner/lifecycle.ts @@ -39,6 +39,8 @@ export function assertUuidV4(value: unknown, name: string): asserts value is str /** A guard refused the action. Refusals are definite outcomes and are recorded for replay. */ export class GuardRefusal extends Error {} +/** A definite failure from a read-only external dependency. Saved action replays preserve its HTTP 502 classification. */ +export class UpstreamFailure extends Error {} /** A malformed request, refused before any transaction and never recorded. The server maps it to HTTP 400. */ export class BadRequest extends GuardRefusal {} /** Reusing an action ID for a different request. */ diff --git a/runner/production.ts b/runner/production.ts index 32a55ae5..7066078a 100644 --- a/runner/production.ts +++ b/runner/production.ts @@ -6,14 +6,14 @@ import { exportTaskDiff, removeTaskFilesystemsAsync, type TaskStorageLimits } fr import { recoverLeftovers } from '../agents/recovery.ts'; import { startClaudeInvocation } from '../agents/adapters/claude.ts'; import { GhAlreadyFixedGateway } from '../github/already-fixed.ts'; -import { GhIssueGateway, type IssueText } from '../github/issues.ts'; +import { GhIssueGateway, withIssueReadDeadline, type IssueAccess, type IssueText } from '../github/issues.ts'; import { GhPullRequestGateway } from '../github/pull-requests.ts'; import { baseBranch } from '../github/validate.ts'; import { identityKey } from '../core/identity.ts'; import type { RunnerDeps } from './coordinator.ts'; import { DEFAULT_DIAGNOSTICS_CAP_BYTES } from './diagnostics.ts'; -import { executionDeps, SafetyFindings, type AgentLauncher, type ExecutionSources } from './execution.ts'; -import { isUuidV4, quoteForTerminal, type ShutdownCapability } from './lifecycle.ts'; +import { executionDeps, SafetyFindings, type AgentLauncher, type ExecutionSources, type GuardedIssueText } from './execution.ts'; +import { GuardRefusal, isUuidV4, quoteForTerminal, type ShutdownCapability } from './lifecycle.ts'; import { recoverStartup, removalCommand, type RecoveryDeps, type RecoveryReport, type RunnerLock } from './recovery.ts'; import { GitBranchPusher, pushUrl } from './branch-push.ts'; import { PullRequestPublisher } from './publish.ts'; @@ -231,13 +231,29 @@ export async function setUpRunner(o: { service: ReviewService; capability: Shutd * read the issue, every collaborator page and every comment page again. Only a completed read is kept. The window is * measured on the monotonic clock, so a wall-clock step back cannot stretch it. */ - let lastRead: { number: number; at: number; text: IssueText } | null = null; - const issueText = async (number: number, signal: AbortSignal): Promise => { - if (lastRead && lastRead.number === number && performance.now() - lastRead.at < ISSUE_REUSE_MS) return lastRead.text; - const at = performance.now(), text = await issues.issueText(number, { signal, timeoutMs: 30_000 }); - lastRead = { number, at, text }; - return text; - }; + let lastRead: { access: IssueAccess; trustedAuthor: string | null | undefined; at: number; text: IssueText } | null = null; + const issueText = (number: number, signal: AbortSignal): Promise => withIssueReadDeadline(signal, async (readSignal, timeoutMs) => { + const access = await issues.issueAccess(number, { signal: readSignal, timeoutMs }); + const trust = service.store.issueTrust(review.github!.repository, number); + const explicitlyTrusted = trust?.revokedAt === null && trust.authorLogin === access.authorLogin; + if (!access.collaborator && !explicitlyTrusted) throw new GuardRefusal(`Issue #${number} is not trusted for its current author.`); + const trustedAuthor = explicitlyTrusted ? access.authorLogin : undefined; + const validate = () => { + if (trustedAuthor === undefined) return; + const current = service.store.issueTrust(review.github!.repository, number); + if (!current || current.revokedAt !== null || current.authorLogin !== trustedAuthor) + throw new GuardRefusal(`Issue #${number} is not trusted for its current author.`); + }; + // Explicit trust deliberately admits every comment and is fully represented by `trustedAuthor`. Collaborator-only + // reads depend on the complete current collaborator set, which IssueAccess does not carry, so never cache them. + if (trustedAuthor !== undefined && lastRead && lastRead.access.number === number && lastRead.access.authorLogin === access.authorLogin + && lastRead.access.collaborator === access.collaborator && lastRead.trustedAuthor === trustedAuthor + && performance.now() - lastRead.at < ISSUE_REUSE_MS) return { text: lastRead.text, validate }; + const at = performance.now(), text = await issues.issueText(number, { signal: readSignal, timeoutMs, trustedAuthor, expectedAccess: access }); + validate(); + lastRead = { access, trustedAuthor, at, text }; + return { text, validate }; + }); const only = (requested: typeof identity) => { if (identityKey(requested) !== identityKey(identity)) throw new Error('This server runs only its configured plan.'); }; diff --git a/runner/publish.ts b/runner/publish.ts index f22523bc..3190a43b 100644 --- a/runner/publish.ts +++ b/runner/publish.ts @@ -11,8 +11,8 @@ import type { Store, TaskPullRequest } from './store.ts'; * "Needs human"). The push of the task head to its branch is injected; `GitBranchPusher` (`branch-push.ts`) is the real one. */ export interface BranchPusher { - /** Makes `refs/heads/` on GitHub point at `head`. Settles only when the push finished or failed. */ - push(identity: PlanIdentity, input: { head: string; branch: string }, signal?: AbortSignal): Promise; + /** Makes the branch point at `head`. Implementations call `beforePush` after their final await, immediately before the write. */ + push(identity: PlanIdentity, input: { head: string; branch: string; beforePush?: MutationBoundary }, signal?: AbortSignal): Promise; } export interface PublishConfig { repository: string; baseBranch: string; @@ -54,6 +54,8 @@ const pullRequestList = (prs: readonly { number: number }[]) => `Pull requests $ * time, so an update is never cleared or overtaken while its GitHub calls run. The runner lock rules out a second process. */ const publishing = new WeakMap>(); +type MutationGuard = () => void | Promise; +export type MutationBoundary = () => void | Promise void)>; export class PullRequestPublisher { #store: Store; #checks: AlreadyFixedGateway; #pulls: PullRequestGateway; #pusher: BranchPusher; #config: PublishConfig; @@ -88,6 +90,15 @@ export class PullRequestPublisher { if (this.#closing || this.#coordinatorClosing()) throw new ShuttingDownError(); } + /** Fresh external authorization first, then every local precondition again with no await before the mutation. */ + #boundary(beforeMutation: MutationGuard | undefined, signal: AbortSignal | undefined, current: () => void): MutationBoundary { + return async () => { + if (beforeMutation) await beforeMutation(); + // The adapter invokes this returned finalizer after the await and immediately before its external write. + return () => { this.#assertOpen(); signal?.throwIfAborted(); current(); }; + }; + } + /** * The task's branch. The readable slug may collide (`Task_42` and `task-42`); the suffix, a hash of the exact task * identity, keeps branches of different tasks apart. @@ -103,7 +114,7 @@ export class PullRequestPublisher { * Opens the task's PR, or a draft PR with the open problems when `problems` is given (the task is in needs human). * Order: recover a lost opening; check; push; record the opening; open. A match or an unreadable check opens nothing. */ - async publish(identity: PlanIdentity, input: { problems?: readonly string[] } = {}, signal?: AbortSignal): Promise { + async publish(identity: PlanIdentity, input: { problems?: readonly string[]; beforeMutation?: MutationGuard } = {}, signal?: AbortSignal): Promise { signal?.throwIfAborted(); this.#assertOpen(); const key = identityKey(identity); @@ -146,7 +157,7 @@ export class PullRequestPublisher { const keep = (error: unknown) => { if (signal.aborted || error instanceof ShuttingDownError) throw error; problems.push(error); }; // Settles a lost opening (OpeningUnsettled while GitHub may still show it) and a lost update first. The task was // cancelled after they began, so neither moves its status. - try { await this.#recover(identity, false, signal); } catch (error) { keep(error); } + try { await this.#recover(identity, false, undefined, signal); } catch (error) { keep(error); } const close = async (number: number, headBranch: string, mark: string) => { try { // The shutdown flags right before the close, after the gateway's read: no await between them. @@ -202,11 +213,11 @@ export class PullRequestPublisher { return Math.max(0, Date.parse(lost.createdAt) + (this.#config.settleMs ?? DEFAULT_SETTLE_MS) - (this.#config.now ?? Date.now)()); } - async #publish(identity: PlanIdentity, input: { problems?: readonly string[] }, signal?: AbortSignal): Promise { + async #publish(identity: PlanIdentity, input: { problems?: readonly string[]; beforeMutation?: MutationGuard }, signal?: AbortSignal): Promise { const draft = input.problems !== undefined; - const recovered = await this.#recover(identity, draft, signal); + const recovered = await this.#recover(identity, draft, input.beforeMutation, signal); if (recovered) return recovered; - const notes = await this.#draftStranded(identity, signal); + const notes = await this.#draftStranded(identity, input.beforeMutation, signal); const task = this.#store.getTask(identity), snapshot = this.#store.getSnapshot(identity), plan = this.#store.getPlan(identity); const reviewVersion = this.#store.reviewVersion(identity); // The full publish guard (status, no attempt, merge, requeue or rebase) before any GitHub call. @@ -233,7 +244,7 @@ export class PullRequestPublisher { // The task's PRs are not where it can publish, and a person has to decide: none of them stays ready meanwhile, // although the task itself could otherwise be published as ready. // Only this run's notes: it looks every PR up again, so an earlier run's failure it has since fixed is not reported. - const latest = await this.#draftStranded(identity, signal, true); + const latest = await this.#draftStranded(identity, input.beforeMutation, signal, true); throw new PullRequestMisplaced(latest.length ? `${error.message} ${latest.join(' ')}` : error.message); } signal?.throwIfAborted(); @@ -256,7 +267,9 @@ export class PullRequestPublisher { if (this.#mayKeepReady(identity)) state = 'It is left as it is: the task is now in review, approved or merged.'; else if (pr.marker === marker(row.openingId)) { let drafted; - try { drafted = await this.#pulls.markDraft(row.number, { base: pr.base, headBranch: pr.headBranch, marker: pr.marker }, signal); } + try { drafted = await this.#pulls.markDraft(row.number, { base: pr.base, headBranch: pr.headBranch, marker: pr.marker, + beforeDraft: this.#boundary(input.beforeMutation, signal, () => this.#store.assertUnchangedSince(identity, + { stateVersion: task.stateVersion, reviewVersion, snapshotId: snapshot.id, draft })) }, signal); } catch (error) { if (signal?.aborted) throw error; state = error instanceof DraftsUnsupported ? 'It stays ready for review: this repository does not support draft pull requests.' @@ -294,7 +307,9 @@ export class PullRequestPublisher { let leftReady: number | undefined; if (earlier && live && !live.draft) { try { - const drafted = await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); + const drafted = await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId), + beforeDraft: this.#boundary(input.beforeMutation, signal, () => this.#store.assertUnchangedSince(identity, + { stateVersion, reviewVersion, snapshotId: snapshot.id, draft })) }, signal); stateVersion = this.#store.recordPullRequestDraft(identity, earlier.openingId, drafted.number, drafted.draft, { stateVersion, reviewVersion }); } catch (error) { if (!(error instanceof DraftsUnsupported)) throw error; @@ -325,7 +340,9 @@ export class PullRequestPublisher { if (needsDraft) this.#store.assertUnchangedSince(identity, { stateVersion, reviewVersion, snapshotId: snapshot.id, draft }); let drafted = null, leftReady: number | undefined; if (needsDraft && earlier && live) { - try { drafted = await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); } + try { drafted = await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId), + beforeDraft: this.#boundary(input.beforeMutation, signal, () => this.#store.assertUnchangedSince(identity, + { stateVersion, reviewVersion, snapshotId: snapshot.id, draft })) }, signal); } catch (error) { if (!(error instanceof DraftsUnsupported)) throw error; leftReady = live.number; } } signal?.throwIfAborted(); @@ -343,44 +360,59 @@ export class PullRequestPublisher { if (draft && !live.draft) { // Read before the call: nothing is written after it on the refusal path (#114). const seenVersion = this.#store.getTask(identity).stateVersion; - try { await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId) }, signal); } + try { await this.#pulls.markDraft(live.number, { base: this.#config.baseBranch, headBranch: branch, marker: marker(earlier.openingId), + beforeDraft: this.#boundary(input.beforeMutation, signal, () => this.#store.assertUnchangedSince(identity, + { stateVersion: check.stateVersion, reviewVersion: check.reviewVersion, snapshotId: snapshot.id, draft })) }, signal); } catch (error) { if (error instanceof DraftsUnsupported) return { kind: 'draft unsupported', number: live.number, seenVersion }; throw error; } signal?.throwIfAborted(); } // The push is a refresh's first content write (it moves the open PR's head), so the refresh is recorded before it; // beginRefresh re-reads the task after the draft change's await. A task change during the push cannot strand it. this.#assertOpen(); + await input.beforeMutation?.(); const stateVersion = this.#store.beginRefresh(identity, { checkId: check.id, openingId: earlier.openingId, headSha: snapshot.head, draft }); - await this.#pusher.push(identity, { head: snapshot.head, branch }, signal); + const refreshCurrent = () => this.#store.assertRefreshCurrent(identity, earlier.openingId, draft); + await this.#pusher.push(identity, { head: snapshot.head, branch, + beforePush: this.#boundary(input.beforeMutation, signal, refreshCurrent) }, signal); signal?.throwIfAborted(); // Re-read after the push's await, before anything else about the PR changes (description, ready or draft): a // cancel, reassignment or review during the push leaves the update in flight and the PR as it was. this.#assertOpen(); + await input.beforeMutation?.(); this.#store.assertRefreshCurrent(identity, earlier.openingId, draft); // Any failure here, including a draft refusal after the PR was made ready again meanwhile, leaves the update // recorded as in flight; the next publish settles it and starts again from the draft step above. const pr = await this.#pulls.refresh(live.number, { // The PR's base on GitHub: the lookup only returns a PR into the configured base. base: this.#config.baseBranch, headBranch: branch, draft, ready: !draft, headSha: snapshot.head, marker: marker(earlier.openingId), - beforeReady: () => { this.#assertOpen(); this.#store.assertRefreshCurrent(identity, earlier.openingId, draft); }, + beforePatch: this.#boundary(input.beforeMutation, signal, refreshCurrent), + beforeReady: this.#boundary(input.beforeMutation, signal, refreshCurrent), title: pullRequestTitle(plan), body: pullRequestBody({ plan, marker: marker(earlier.openingId), problems: input.problems }), }, signal); const status = this.#store.recordRefreshConfirmed(identity, earlier.openingId, pr, { head: snapshot.head, stateVersion }); - return this.#settleHead(identity, earlier.openingId, pr, snapshot.head, draft, status, branch, signal); + return this.#settleHead(identity, earlier.openingId, pr, snapshot.head, draft, status, branch, input.beforeMutation, signal); } // No PR exists yet, so moving the branch changes nothing a reviewer sees. this.#assertOpen(); - await this.#pusher.push(identity, { head: snapshot.head, branch }, signal); + await input.beforeMutation?.(); + const checkCurrent = () => this.#store.assertUnchangedSince(identity, + { stateVersion: check.stateVersion, reviewVersion: check.reviewVersion, snapshotId: snapshot.id, draft }); + await this.#pusher.push(identity, { head: snapshot.head, branch, + beforePush: this.#boundary(input.beforeMutation, signal, checkCurrent) }, signal); signal?.throwIfAborted(); // The last await before the irreversible call is behind us: beginPullRequest re-reads the task state in its transaction. this.#assertOpen(); + await input.beforeMutation?.(); const opening = this.#store.beginPullRequest(identity, { checkId: check.id, repository: this.#config.repository, base: this.#config.baseBranch, headBranch: branch, headSha: snapshot.head, draft, }); let pr; try { + await input.beforeMutation?.(); pr = await this.#pulls.open({ base: opening.base, headBranch: branch, draft, marker: marker(opening.openingId), + beforeOpen: this.#boundary(input.beforeMutation, signal, () => this.#store.assertUnchangedSince(identity, + { stateVersion: opening.ownerVersion, reviewVersion: opening.ownerReviewVersion, snapshotId: snapshot.id, draft })), title: pullRequestTitle(plan), body: pullRequestBody({ plan, marker: marker(opening.openingId), problems: input.problems }), }, signal); } catch (error) { @@ -391,7 +423,7 @@ export class PullRequestPublisher { return { kind: 'draft unsupported', number: null, seenVersion: this.#store.getTask(identity).stateVersion }; } const status = this.#store.recordPullRequestOpened(identity, opening.openingId, pr); - return this.#settleHead(identity, opening.openingId, pr, snapshot.head, draft, status, branch, signal); + return this.#settleHead(identity, opening.openingId, pr, snapshot.head, draft, status, branch, input.beforeMutation, signal); } /** @@ -401,7 +433,7 @@ export class PullRequestPublisher { * of the whole publish (AGENTS.md: a later step's failure must not turn a succeeded irreversible action into one). */ async #settleHead(identity: PlanIdentity, openingId: string, pr: { number: number; url: string; headSha: string; draft: boolean }, head: string, - draft: boolean, status: string, branch: string, signal?: AbortSignal): Promise { + draft: boolean, status: string, branch: string, beforeMutation?: MutationGuard, signal?: AbortSignal): Promise { const base = this.#config.baseBranch; // The version right after the open or refresh was recorded (no await since); on the leftReady path nothing is written // after the draft call, so a change made during it was not seen (#114). @@ -413,7 +445,10 @@ export class PullRequestPublisher { let drafted; // Only the GitHub call's failure becomes leftReady (the PR stays ready; the next publish of a task that can still be published reconciles it). A Store // failure after a draft change that landed propagates, so it is not misreported as a ready PR. - try { drafted = await this.#pulls.markDraft(pr.number, { base, headBranch: branch, marker: marker(openingId) }, signal); } + try { drafted = await this.#pulls.markDraft(pr.number, { base, headBranch: branch, marker: marker(openingId), + beforeDraft: this.#boundary(beforeMutation, signal, () => { + if (this.#mayKeepReady(identity)) throw new GuardRefusal('The task now keeps its pull request ready.'); + }) }, signal); } catch (error) { if (signal?.aborted) throw error; return { ...opened, leftReady: pr.number }; } // A fact about the PR, recorded against the versions read right now (no await since). this.#store.recordPullRequestDraft(identity, openingId, drafted.number, drafted.draft, @@ -446,7 +481,7 @@ export class PullRequestPublisher { * does not prove the request was refused while GitHub may still apply or show it, so the opening stays owned until * the settle time has passed; only then is it abandoned. The caller retries after OpeningUnsettled. */ - async #recover(identity: PlanIdentity, draft: boolean, signal?: AbortSignal): Promise { + async #recover(identity: PlanIdentity, draft: boolean, beforeMutation?: MutationGuard, signal?: AbortSignal): Promise { // An update whose confirmation was lost is repeated, not adopted: its description may or may not have landed. What // GitHub shows now (draft flag, head) is recorded first, so a change that did land is not forgotten. for (const refreshing of this.#store.taskPullRequests(identity).filter(pr => pr.refresh !== null)) { @@ -494,7 +529,10 @@ export class PullRequestPublisher { // Only when this publish is itself a draft publish; a ready publish's main path marks the PR ready anyway. // Re-read after the lookup's await, as before every other draft change: an approved task keeps its PR ready. if (lost.draft && draft && !pr.draft && !this.#mayKeepReady(identity)) { - try { found = await this.#pulls.markDraft(pr.number, { base: pr.base, headBranch: lost.headBranch, marker: marker(lost.openingId) }, signal); } + try { found = await this.#pulls.markDraft(pr.number, { base: pr.base, headBranch: lost.headBranch, marker: marker(lost.openingId), + beforeDraft: this.#boundary(beforeMutation, signal, () => { + if (this.#mayKeepReady(identity)) throw new GuardRefusal('The task now keeps its pull request ready.'); + }) }, signal); } catch (error) { if (!(error instanceof DraftsUnsupported)) throw error; // Definite: the PR is recorded as it is, and the main path reports it (`draft unsupported`, or the misplaced @@ -513,7 +551,7 @@ export class PullRequestPublisher { const status = this.#store.recordPullRequestOpened(identity, lost.openingId, found, mayReview); // It ends the publish only for a PR the main path would accept. Any other goes on to the main path, which makes all of // the task's PRs drafts and refuses with what a person has to do, as it does for every misplaced PR. - if (current && mayReview) return this.#settleHead(identity, lost.openingId, found, lost.headSha, draft, status, lost.headBranch, signal); + if (current && mayReview) return this.#settleHead(identity, lost.openingId, found, lost.headSha, draft, status, lost.headBranch, beforeMutation, signal); return null; } @@ -526,7 +564,7 @@ export class PullRequestPublisher { * which also adopts, may refuse first. A draft flag GitHub already shows is only recorded. A GitHub failure does not * replace the status refusal that follows: it is returned as a note for it, and the next publish tries again. */ - async #draftStranded(identity: PlanIdentity, signal?: AbortSignal, misplaced = false): Promise { + async #draftStranded(identity: PlanIdentity, beforeMutation?: MutationGuard, signal?: AbortSignal, misplaced = false): Promise { // `misplaced`: the main path found the task's PRs where it cannot publish, so being publishable keeps nothing ready. const keepsReady = () => this.#mayKeepReady(identity) || (!misplaced && this.#store.canPublish(identity, false)); if (keepsReady()) return []; @@ -561,7 +599,10 @@ export class PullRequestPublisher { let drafted: { number: number; draft: boolean } = live; // Re-read after the lookup's await: a task that was approved meanwhile keeps its PR ready. if (!live.draft && !keepsReady()) { - try { drafted = await this.#pulls.markDraft(live.number, { base: live.base, headBranch, marker: live.marker }, signal); } + try { drafted = await this.#pulls.markDraft(live.number, { base: live.base, headBranch, marker: live.marker, + beforeDraft: this.#boundary(beforeMutation, signal, () => { + if (keepsReady()) throw new GuardRefusal('The task now keeps its pull request ready.'); + }) }, signal); } catch (error) { if (signal?.aborted) throw error; notes.push(error instanceof DraftsUnsupported diff --git a/runner/publishing.ts b/runner/publishing.ts index 2bb3907a..b098169c 100644 --- a/runner/publishing.ts +++ b/runner/publishing.ts @@ -50,12 +50,15 @@ export class TaskPublishing { */ #retried = new Map(); #shortRetryMs: number; + /** Fresh admission check at every publish attempt, including automatic retries. Close jobs do not need issue trust. */ + #authorizePublish?: (identity: PlanIdentity, signal: AbortSignal) => Promise<() => Promise>; /** Jobs in progress, by task, from scheduling until the outcome is recorded; `abort` stops a publish on cancel. */ #running = new Map; abort: AbortController }>(); constructor(store: Store, publisher: PullRequestPublisher, runner: RunnerCoordinator, executor: ItemExecutor, capability?: ShutdownCapability, - env: NodeJS.ProcessEnv = process.env, options: { shortRetryMs?: number } = {}) { + env: NodeJS.ProcessEnv = process.env, options: { shortRetryMs?: number; authorizePublish?: (identity: PlanIdentity, signal: AbortSignal) => Promise<() => Promise> } = {}) { this.#store = store; this.#publisher = publisher; this.#runner = runner; this.#executor = executor; this.#write = settleWith(capability); this.#shortRetryMs = options.shortRetryMs ?? SHORT_RETRY_MS; + this.#authorizePublish = options.authorizePublish; this.#secrets = TOKEN_VARIABLES.flatMap(name => env[name] ?? []); } @@ -207,6 +210,7 @@ export class TaskPublishing { this.#closing = true; for (const timer of this.#retries.values()) clearTimeout(timer); this.#retries.clear(); + for (const running of this.#running.values()) running.abort.abort(new ShuttingDownError()); await this.#publisher.close(); while (this.#running.size) await Promise.all([...this.#running.values()].map(running => running.done)); } @@ -274,6 +278,18 @@ export class TaskPublishing { // Durable in-flight ownership before the first external write (AGENTS.md): if this process stops before the outcome // is recorded, startup finds the marker and publishes again or settles it as interrupted. No marker, no publish. const draft = job.kind === 'publish' && job.draft; + let assertAuthorized: (() => Promise) | undefined; + if (job.kind === 'publish' && this.#authorizePublish) { + try { assertAuthorized = await this.#authorizePublish(identity, abort.signal); } + catch (error) { + const stopped = error instanceof ShuttingDownError || error instanceof PublishCancelled || (this.#closing && (error as Error)?.name === 'AbortError'); + const record = { outcome: stopped ? 'stopped' as const : REFUSALS.some(type => error instanceof type) ? 'refused' as const : 'failed' as const, + draft, message: message(error, this.#secrets) }; + try { this.#write(() => this.#store.recordPublish(identity, record, actionId)); } + catch (writeError) { console.error(`Could not record the publish admission outcome: ${JSON.stringify(message(writeError, this.#secrets))}`); } + return; + } + } try { this.#write(() => this.#store.recordPublish(identity, job.kind === 'close' ? { outcome: 'closing', draft: false, action: 'close', message: 'The task\'s pull requests are being closed.' } : { outcome: 'publishing', draft, message: 'A pull request is being published.' })); @@ -288,7 +304,9 @@ export class TaskPublishing { : 'No open pull request of the task was left to close.' }; } else { // The problems are read when the publish starts: the task is in needs human, and its last attempt says why. - const outcome = await this.#publisher.publish(identity, draft ? { problems: this.#problems(identity) } : {}, abort.signal); + const outcome = await this.#publisher.publish(identity, { + ...(draft ? { problems: this.#problems(identity) } : {}), ...(assertAuthorized ? { beforeMutation: assertAuthorized } : {}), + }, abort.signal); record = describe(outcome, draft); seenVersion = outcome.seenVersion; } diff --git a/runner/store.ts b/runner/store.ts index 24258439..0bed520e 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -9,7 +9,7 @@ import type { Approval, SegmentChoice } from '../core/approvals.ts'; import type { InvocationContext, StopReason } from '../agents/contract.ts'; import type { AlreadyFixedResult } from '../github/already-fixed.ts'; import { - ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, HUMAN_GATES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES, + ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, HUMAN_GATES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, UpstreamFailure, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES, WRITABLE_KINDS, assertUuidV4, bounded, classifySettlement, requestHash, sameContext, type AttemptKind, type AttemptState, type Classification, type FirstReason, type Settlement, type TaskStatus, } from './lifecycle.ts'; @@ -106,6 +106,11 @@ export interface AttemptRecord { * with. The terminal write (or startup recovery) acted on it: the task went to needs human, unless it was closed. */ safetyFinding: string | null; + /** Digest and count of the exact issue comments carried by this attempt's prompt. */ + promptComments: { count: number; digest: string; state: 'prepared' | 'delivered' } | null; +} +export interface IssueTrustRecord { + repository: string; issue: number; authorLogin: string | null; trustedBy: string; trustedAt: string; revokedAt: string | null; } /** A non-terminal attempt at startup, with what recovery needs to stop its preparation and export its storage. */ export interface InterruptedAttempt extends AttemptRecord { @@ -169,8 +174,8 @@ export class Store { this.#db.exec('PRAGMA foreign_keys=ON; PRAGMA journal_mode=WAL; PRAGMA synchronous=FULL;'); this.#transaction(() => { const version = this.#get('PRAGMA user_version')!.user_version as number; - if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16].includes(version)) throw new Error('Unsupported store schema version.'); - if (version === 16) return; + if (![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17].includes(version)) throw new Error('Unsupported store schema version.'); + if (version === 17) return; if (version === 0) this.#db.exec(` CREATE TABLE plans (key TEXT PRIMARY KEY, issue INTEGER NOT NULL, revision INTEGER NOT NULL, snapshot_id TEXT); CREATE TABLE revisions (key TEXT NOT NULL REFERENCES plans(key), revision INTEGER NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,revision)); @@ -277,6 +282,15 @@ export class Store { this.#db.exec('ALTER TABLE attempts ADD COLUMN preparation_identity TEXT'); this.#db.exec('PRAGMA user_version=16'); } + // Repository-scoped issue trust and the exact comment-set evidence carried by execute prompts (#108). + if (version < 17) { + this.#db.exec(`CREATE TABLE IF NOT EXISTS issue_trust ( + repository TEXT NOT NULL, issue INTEGER NOT NULL, author_login TEXT, trusted_by TEXT NOT NULL, + trusted_at TEXT NOT NULL, revoked_at TEXT, PRIMARY KEY(repository,issue));`); + if (!this.#db.prepare('PRAGMA table_info(attempts)').all().some(column => column.name === 'prompt_comments')) + this.#db.exec('ALTER TABLE attempts ADD COLUMN prompt_comments TEXT'); + this.#db.exec('PRAGMA user_version=17'); + } }); } catch (error) { this.#db.close(); throw error; } } @@ -1307,8 +1321,70 @@ export class Store { diagnosticRef: row.diagnostic_ref as string | null, createdAt: row.created_at as string, startedAt: row.started_at as string | null, settledAt: row.settled_at as string | null, safetyFinding: row.safety_finding as string | null, + promptComments: row.prompt_comments === null || row.prompt_comments === undefined ? null : decode(row.prompt_comments), }; } + issueTrust(repository: string, issue: number): IssueTrustRecord | null { + const row = this.#get('SELECT * FROM issue_trust WHERE repository=? AND issue=?', repository.toLocaleLowerCase('en-US'), issue); + if (!row) return null; + return { repository: row.repository as string, issue: row.issue as number, authorLogin: row.author_login as string | null, + trustedBy: row.trusted_by as string, trustedAt: row.trusted_at as string, revokedAt: row.revoked_at as string | null }; + } + setIssueTrust(input: { repository: string; issue: number; authorLogin: string | null; trusted: boolean; trustedBy: string }): IssueTrustRecord { + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(input.repository) || !Number.isSafeInteger(input.issue) || input.issue < 1) + throw new GuardRefusal('Invalid issue trust target.'); + if (input.authorLogin !== null && (typeof input.authorLogin !== 'string' || !input.authorLogin || input.authorLogin.length > 100 || /[\s\u0000-\u001f\u007f]/u.test(input.authorLogin))) + throw new GuardRefusal('Invalid issue author.'); + if (typeof input.trustedBy !== 'string' || !input.trustedBy || input.trustedBy.length > 100) throw new GuardRefusal('Invalid trust decision owner.'); + const repository = input.repository.toLocaleLowerCase('en-US'); + return this.#transaction(() => { + const existing = this.issueTrust(repository, input.issue); + // Strictly order decisions even when two clients act in the same wall-clock millisecond. Browser responses use + // this timestamp as their CAS version, so an older response can never overwrite a newer same-author decision. + const previous = existing ? Date.parse(existing.revokedAt ?? existing.trustedAt) : -1; + const now = new Date(Math.max(Date.now(), previous + 1)).toISOString(); + if (input.trusted) { + if (existing?.revokedAt === null && existing.authorLogin === input.authorLogin) return existing; + this.#run(`INSERT INTO issue_trust(repository,issue,author_login,trusted_by,trusted_at,revoked_at) VALUES (?,?,?,?,?,NULL) + ON CONFLICT(repository,issue) DO UPDATE SET author_login=excluded.author_login,trusted_by=excluded.trusted_by,trusted_at=excluded.trusted_at,revoked_at=NULL`, + repository, input.issue, input.authorLogin, input.trustedBy, now); + } else { + if (existing && existing.revokedAt !== null && existing.authorLogin === input.authorLogin) return existing; + if (!existing || existing.authorLogin !== input.authorLogin) + throw new GuardRefusal('This issue is not trusted for its current author.'); + this.#run('UPDATE issue_trust SET revoked_at=? WHERE repository=? AND issue=?', now, repository, input.issue); + } + return this.issueTrust(repository, input.issue)!; + }); + } + recordAttemptComments(identity: PlanIdentity, id: string, evidence: { count: number; digest: string }): void { + if (!Number.isSafeInteger(evidence.count) || evidence.count < 0 || !/^[a-f0-9]{64}$/.test(evidence.digest)) + throw new GuardRefusal('Invalid prompt comment evidence.'); + const key = identityKey(identity); + this.#transaction(() => { + const row = this.#get('SELECT state,prompt_comments FROM attempts WHERE plan_key=? AND id=?', key, id); + if (!row || (row.state !== 'pending' && row.state !== 'running')) throw new GuardRefusal('The attempt is no longer active.'); + const value = encode({ ...evidence, state: 'prepared' }); + if (row.prompt_comments !== null && row.prompt_comments !== value) throw new GuardRefusal('The attempt already records different prompt comments.'); + if (row.prompt_comments === null) { this.#run('UPDATE attempts SET prompt_comments=? WHERE plan_key=? AND id=?', value, key, id); this.#touch(key); } + }); + } + markAttemptCommentsDelivered(identity: PlanIdentity, id: string): void { + const key = identityKey(identity); + this.#transaction(() => { + const row = this.#get('SELECT state,prompt_comments FROM attempts WHERE plan_key=? AND id=?', key, id); + if (!row || (row.state !== 'pending' && row.state !== 'running')) throw new GuardRefusal('The attempt is no longer active.'); + if (row.prompt_comments === null) throw new GuardRefusal('The attempt has no prepared prompt comment evidence.'); + const evidence = decode(row.prompt_comments) as { count?: unknown; digest?: unknown; state?: unknown }; + if (evidence.state === 'delivered') return; + if (evidence.state !== 'prepared' || !Number.isSafeInteger(evidence.count) || (evidence.count as number) < 0 + || typeof evidence.digest !== 'string' || !/^[a-f0-9]{64}$/.test(evidence.digest)) + throw new GuardRefusal('The attempt has invalid prompt comment evidence.'); + this.#run('UPDATE attempts SET prompt_comments=? WHERE plan_key=? AND id=?', + encode({ count: evidence.count, digest: evidence.digest, state: 'delivered' }), key, id); + this.#touch(key); + }); + } /** * A safety finding sends the task to needs human (plan-format.md, "After each run"), in the transaction that settles * its attempt. A task cannot change status while it has an active attempt, so it is still running here: no human gate @@ -1419,7 +1495,7 @@ export class Store { /** The latest attempts, oldest first, for status views: results are flagged, not read. */ recentAttempts(identity: PlanIdentity, limit: number): (Omit & { hasResult: boolean })[] { return this.#db.prepare(`SELECT * FROM (SELECT id, kind, phase, item, state, context, deadline, first_reason, stop_reason, exit_code, signal, - NULL AS result, result IS NOT NULL AS has_result, diagnostic, diagnostic_ref, created_at, started_at, settled_at, rowid AS row_order + NULL AS result, result IS NOT NULL AS has_result, diagnostic, diagnostic_ref, prompt_comments, created_at, started_at, settled_at, rowid AS row_order FROM attempts WHERE plan_key=? ORDER BY rowid DESC LIMIT ?) ORDER BY row_order`).all(identityKey(identity), limit) .map(row => { const { result: _result, ...attempt } = this.#attemptRecord(row); return { ...attempt, hasResult: row.has_result === 1 }; }); } @@ -1680,7 +1756,8 @@ export class Store { if (!replaying && !storage && !(error instanceof ActionIdReused) && !(error instanceof BadRequest) && this.#depth === 0) { const message = error instanceof Error ? bounded(error.message) : 'Refused.'; this.#transaction(() => { - if (!this.#get('SELECT 1 FROM user_actions WHERE plan_key=? AND action_id=?', key, action.actionId)) record({ ok: false, error: message }); + if (!this.#get('SELECT 1 FROM user_actions WHERE plan_key=? AND action_id=?', key, action.actionId)) + record({ ok: false, error: message, ...(error instanceof UpstreamFailure ? { kind: 'upstream' } : {}) }); if (error instanceof RefusalWithEffect) error.effect(); }); } @@ -1696,8 +1773,8 @@ export class Store { const row = this.#get('SELECT * FROM user_actions WHERE plan_key=? AND action_id=?', identityKey(identity), action.actionId); if (!row) return undefined; if (row.request_hash !== requestHash(action.kind, action.request)) throw new ActionIdReused('Action ID already used for a different request.'); - const outcome = decode<{ ok: boolean; value?: T; error?: string }>(row.response); - if (!outcome.ok) throw new GuardRefusal(outcome.error!); + const outcome = decode<{ ok: boolean; value?: T; error?: string; kind?: string }>(row.response); + if (!outcome.ok) throw outcome.kind === 'upstream' ? new UpstreamFailure(outcome.error!) : new GuardRefusal(outcome.error!); return { response: outcome.value as T, replayed: true }; } /** Append one feedback event. Call inside userAction so the event and its action share one transaction. */ diff --git a/scripts/demo-issues.ts b/scripts/demo-issues.ts index 1eb81b9f..f961983a 100644 --- a/scripts/demo-issues.ts +++ b/scripts/demo-issues.ts @@ -1,11 +1,11 @@ -import type { IssueGateway, IssueSnapshot, RepositoryIssue } from '../github/issues.ts'; +import type { IssueSnapshot, IssueTrustGateway, RepositoryIssue } from '../github/issues.ts'; const repository = 'codeboost-demo/retry-service'; const DAY = 86_400_000; /** Disposable fixture only. Demo issues never come from, or go to, GitHub. */ -export function demoIssueGateway(now: () => Date = () => new Date()): IssueGateway { - return { +export function demoIssueGateway(now: () => Date = () => new Date()): IssueTrustGateway { + const gateway: IssueTrustGateway = { repository, async fetch(options = {}): Promise { options.signal?.throwIfAborted(); @@ -31,5 +31,18 @@ export function demoIssueGateway(now: () => Date = () => new Date()): IssueGatew ], }; }, + async issueAccess(number, options = {}) { + options.signal?.throwIfAborted(); + const found = (await gateway.fetch(options)).issues.find(issue => issue.number === number); + if (!found) throw new Error(`Demo issue #${number} does not exist.`); + return { number, authorLogin: found.authorLogin, collaborator: found.trust === 'trusted' }; + }, + async issueText(number, options = {}) { + options.signal?.throwIfAborted(); + const found = (await gateway.fetch(options)).issues.find(issue => issue.number === number); + if (!found) throw new Error(`Demo issue #${number} does not exist.`); + return { number, title: found.title, body: found.body, comments: [] }; + }, }; + return gateway; } diff --git a/test/browser/issues.spec.ts b/test/browser/issues.spec.ts index 290b7579..5d9d033a 100644 --- a/test/browser/issues.spec.ts +++ b/test/browser/issues.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '@playwright/test'; +import { randomUUID } from 'node:crypto'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -57,6 +58,7 @@ test('ranks demo issues with visible reasons and trust, and keeps review input a ]); await expect(top.locator('td').nth(2)).toHaveText('160'); await expect(top.getByLabel('Trust: author is a repository collaborator')).toHaveText('✓ Collaborator'); + await expect(top.getByRole('button', { name: 'Trust all comments' })).toBeVisible(); await expect(issueRows(page).nth(2).getByLabel(/needs your trust before queueing/)).toHaveText('! Needs trust'); await expect(issueRows(page).nth(4).getByRole('listitem')).toHaveText(['1 point: 1 comment']); await expect(top.getByRole('link')).toHaveAttribute('rel', 'noopener noreferrer'); @@ -79,6 +81,397 @@ test('ranks demo issues with visible reasons and trust, and keeps review input a expect(errors).toEqual([]); }); +test('can explicitly trust a collaborator issue to include all comments, then remove that widening', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="17"]'); + await row.getByRole('button', { name: 'Trust all comments' }).click(); + await expect(row.getByRole('button', { name: 'Remove trust' })).toBeFocused(); + await expect(row.getByLabel(/trusted by you on/i)).toBeVisible(); + await row.getByRole('button', { name: 'Remove trust' }).click(); + await expect(row.getByRole('button', { name: 'Trust all comments' })).toBeFocused(); + await expect(row.getByLabel('Trust: author is a repository collaborator')).toBeVisible(); +}); + +test('trusts and untrusts a demo issue without GitHub and keeps keyboard focus on the action', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'); + const trust = row.getByRole('button', { name: 'Trust this issue' }); + await expect(trust).toBeVisible(); + await trust.focus(); + await page.keyboard.press('Enter'); + const remove = row.getByRole('button', { name: 'Remove trust' }); + await expect(remove).toBeFocused(); + await expect(row.getByLabel(/trusted by you on/i)).toContainText('✓ Trusted by you on'); + await page.reload(); + await expect(row.getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await row.getByRole('button', { name: 'Remove trust' }).focus(); + await page.keyboard.press('Enter'); + await expect(row.getByRole('button', { name: 'Trust this issue' })).toBeFocused(); + await expect(row.getByLabel(/needs your trust before queueing/)).toBeVisible(); +}); + +test('keeps keyboard focus on an issue link when a pending trust response rerenders the table', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let held: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const captured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action !== 'trust' || body.number !== 21 || held) { await route.continue(); return; } + held = { route, response: await route.fetch() }; captured.resolve(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await captured.promise; + const title = page.locator('tr[data-issue="23"] .issue-title a'); + await title.focus(); + await expect(title).toBeFocused(); + await held!.route.fulfill({ response: held!.response }); + await expect(title).toBeFocused(); +}); + +test('ignores an older trust response that returns after a newer action', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let first: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const captured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(); + const body = request.method() === 'POST' ? request.postDataJSON() as { action?: string } : {}; + if (body.action !== 'trust' || first) { await route.continue(); return; } + first = { route, response: await route.fetch() }; + captured.resolve(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'), button = row.locator('button.issue-trust'); + await button.click(); + await captured.promise; + // Simulate a second explicit activation while the first browser response is delayed. The generation guard owns it. + await button.evaluate(element => element.removeAttribute('aria-disabled')); + await button.click(); + await expect(row.getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await first!.route.fulfill({ response: first!.response }); + await expect(row.getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); +}); + +test('reuses the trust action ID after a lost response without overwriting a newer decision', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + type TrustBody = { action: string; actionId: string; number: number; authorLogin: string | null }; + const requests: TrustBody[] = []; + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as TrustBody : undefined; + if (body?.action !== 'trust' || body.number !== 21) { await route.continue(); return; } + requests.push(body); + if (requests.length === 1) { + await route.fetch(); // The server commits, but the browser never receives the response. + await route.abort('connectionreset'); + return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + const direct = await page.request.post(new URL('/api/issues', app.url).href, { + headers: { 'x-codeboost-token': app.token }, data: { action: 'untrust', actionId: randomUUID(), number: 21, + authorLogin: requests[0]!.authorLogin }, + }); + expect(direct.ok()).toBe(true); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await expect.poll(() => requests.length).toBe(2); + expect(requests[1]!.actionId).toBe(requests[0]!.actionId); + await expect(row.getByRole('button', { name: 'Trust this issue' })).toBeVisible(); +}); + +test('does not let an older same-author trust response overwrite a newer untrust', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + type TrustBody = { action: string; actionId: string; number: number; authorLogin: string | null }; + let held: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse; body: TrustBody } | undefined; + const captured = deferred(), refreshCompleted = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as TrustBody : undefined; + if (body?.action === 'refresh' && held) { + const response = await route.fetch(); await route.fulfill({ response }); refreshCompleted.resolve(); return; + } + if (body?.action !== 'trust' || body.number !== 21 || held) { await route.continue(); return; } + held = { route, response: await route.fetch(), body }; captured.resolve(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await captured.promise; + const untrust = await page.request.post(new URL('/api/issues', app.url).href, { + headers: { 'x-codeboost-token': app.token }, data: { action: 'untrust', actionId: randomUUID(), number: 21, + authorLogin: held!.body.authorLogin }, + }); + expect(untrust.ok()).toBe(true); + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await refreshCompleted.promise; + await held!.route.fulfill({ response: held!.response }); + await expect(row.getByRole('button', { name: 'Trust this issue' })).toBeVisible(); +}); + +test('does not let an equal-version trust response overwrite refreshed collaborator access', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let held: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse; + updated: { state: { issues: Record[] } } } | undefined; + let equalVersions: [unknown, unknown] | undefined; + const captured = deferred(), refreshed = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action === 'untrust' && body.number === 17 && !held) { + const response = await route.fetch(), updated = await response.json() as { state: { issues: Record[] } }; + held = { route, response, updated }; captured.resolve(); return; + } + if (body.action === 'refresh' && held) { + const response = await route.fetch(), updated = await response.json() as { state: { issues: Record[] } }; + const stale = held.updated.state.issues.find(issue => issue.number === 17)!; + updated.state.issues = updated.state.issues.map(issue => issue.number === 17 + ? { ...issue, trust: 'requires-approval', trustedAt: undefined, trustedBy: undefined } + : issue); + const current = updated.state.issues.find(issue => issue.number === 17)!; + equalVersions = [stale.trustChangedAt, current.trustChangedAt]; + await route.fulfill({ response, json: updated }); refreshed.resolve(); return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="17"]'); + await row.getByRole('button', { name: 'Trust all comments' }).click(); + await row.getByRole('button', { name: 'Remove trust' }).click(); + await captured.promise; + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await refreshed.promise; + expect(equalVersions?.[0]).toBeDefined(); + expect(equalVersions?.[0]).toBe(equalVersions?.[1]); + await expect(row.getByLabel(/needs your trust before queueing/)).toBeVisible(); + await held!.route.fulfill({ response: held!.response, json: held!.updated }); + await expect(row.getByRole('button', { name: 'Trust this issue' })).toBeVisible(); +}); + +test('reuses the trust action ID after a retryable 503', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + const actionIds: string[] = []; + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; actionId?: string; number?: number } : {}; + if (body.action !== 'trust' || body.number !== 21) { await route.continue(); return; } + actionIds.push(body.actionId!); + if (actionIds.length === 1) { + await route.fulfill({ status: 503, contentType: 'application/json', body: JSON.stringify({ error: 'The server is shutting down.' }) }); + return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await expect(row.getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); + expect(actionIds).toHaveLength(2); + expect(actionIds[1]).toBe(actionIds[0]); +}); + +test('keeps another issue disabled and focused when an overlapping trust request fails', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let held: import('@playwright/test').Route | undefined; + const captured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(); + const body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action !== 'trust' || body.number !== 21 || held) { await route.continue(); return; } + held = route; + captured.resolve(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await captured.promise; + await page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Trust this issue' }).click(); + const second = page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Remove trust' }); + await expect(second).toBeFocused(); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trusting…' })).toHaveAttribute('aria-disabled', 'true'); + await held!.fulfill({ status: 502, contentType: 'application/json', body: JSON.stringify({ error: 'GitHub unavailable.' }) }); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' })).toBeVisible(); + await expect(second).toBeFocused(); +}); + +test('keeps one issue trust failure visible when another overlapping request succeeds', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let first: import('@playwright/test').Route | undefined; + let second: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const firstCaptured = deferred(), secondCaptured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action !== 'trust') { await route.continue(); return; } + if (body.number === 21 && !first) { first = route; firstCaptured.resolve(); return; } + if (body.number === 23 && !second) { second = { route, response: await route.fetch() }; secondCaptured.resolve(); return; } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await firstCaptured.promise; + await page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Trust this issue' }).click(); + await secondCaptured.promise; + await first!.fulfill({ status: 502, contentType: 'application/json', body: JSON.stringify({ error: 'GitHub unavailable.' }) }); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + await second!.route.fulfill({ response: second!.response }); + await expect(page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); +}); + +test('keeps a refresh failure visible through trust rendering and clears it on the next refresh', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let failRefresh = false; + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string } : {}; + if (body.action === 'refresh' && failRefresh) { + failRefresh = false; + await route.fulfill({ status: 502, contentType: 'application/json', body: JSON.stringify({ error: 'Refresh unavailable.' }) }); + return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); + failRefresh = true; + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await expect(page.locator('#issues-status')).toContainText('Could not refresh issues. Refresh unavailable.'); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await expect(page.locator('#issues-status')).toContainText('Could not refresh issues. Refresh unavailable.'); + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); +}); + +test('does not reconcile a trust failure that occurs after refresh starts', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let holdRefresh = false; + let held: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const captured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(), body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action === 'refresh' && holdRefresh && !held) { + held = { route, response: await route.fetch() }; captured.resolve(); return; + } + if (body.action === 'trust' && body.number === 21) { + await route.fulfill({ status: 502, contentType: 'application/json', body: JSON.stringify({ error: 'GitHub unavailable.' }) }); return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await expect(page.locator('#issues-status')).toContainText('✓ Current'); + holdRefresh = true; + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await captured.promise; + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); + await held!.route.fulfill({ response: held!.response }); + await expect(page.locator('#issues-status')).toContainText('Could not trust issue #21'); +}); + +test('merges successful overlapping trust responses for different issues', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let held: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const captured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(); + const body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action !== 'trust' || body.number !== 21 || held) { await route.continue(); return; } + held = { route, response: await route.fetch() }; + captured.resolve(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await captured.promise; + await page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Trust this issue' }).click(); + await expect(page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await held!.route.fulfill({ response: held!.response }); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await expect(page.locator('tr[data-issue="23"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); +}); + +test('does not let a refresh started during trust overwrite the committed decision', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let trustRoute: import('@playwright/test').Route | undefined; + let refreshRoute: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse; + updated: { state: { issues: Record[] } } } | undefined; + const trustCaptured = deferred(), refreshCaptured = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(); + const body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action === 'trust' && body.number === 21 && !trustRoute) { trustRoute = route; trustCaptured.resolve(); return; } + if (body.action === 'refresh' && trustRoute && !refreshRoute) { + const response = await route.fetch(), updated = await response.json() as { state: { issues: Record[] } }; + updated.state.issues = updated.state.issues.map(issue => issue.number === 21 ? { ...issue, title: 'Refreshed issue metadata' } : issue); + refreshRoute = { route, response, updated }; refreshCaptured.resolve(); return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + await page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Trust this issue' }).click(); + await trustCaptured.promise; + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await refreshCaptured.promise; + const trusted = await trustRoute!.fetch(); + await trustRoute!.fulfill({ response: trusted }); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); + await refreshRoute!.route.fulfill({ response: refreshRoute!.response, json: refreshRoute!.updated }); + await expect(page.locator('tr[data-issue="21"]').getByRole('link', { name: 'Refreshed issue metadata' })).toBeVisible(); + await expect(page.locator('tr[data-issue="21"]').getByRole('button', { name: 'Remove trust' })).toBeVisible(); +}); + +test('does not let an older trust response overwrite a refresh or trust an obsolete author', async ({ page }) => { + app = await startServer(createDemo(join(root, 'demo')), 0); + let trustRoute: { route: import('@playwright/test').Route; response: import('@playwright/test').APIResponse } | undefined; + const trustCaptured = deferred(), refreshCompleted = deferred(); + await page.route('**/api/issues', async route => { + const request = route.request(); + const body = request.method() === 'POST' ? request.postDataJSON() as { action?: string; number?: number } : {}; + if (body.action === 'trust' && body.number === 21 && !trustRoute) { + trustRoute = { route, response: await route.fetch() }; trustCaptured.resolve(); return; + } + if (body.action === 'refresh' && trustRoute) { + const response = await route.fetch(), updated = await response.json() as { state: { issues: Record[] } }; + updated.state.issues = updated.state.issues.map(issue => issue.number === 21 + ? { ...issue, title: 'New issue metadata', authorLogin: 'replacement-author', trust: 'requires-approval' } + : issue); + await route.fulfill({ response, json: updated }); refreshCompleted.resolve(); return; + } + await route.continue(); + }); + await page.goto(app.url); + await page.getByRole('link', { name: 'Issues', exact: true }).click(); + const row = page.locator('tr[data-issue="21"]'); + await row.getByRole('button', { name: 'Trust this issue' }).click(); + await trustCaptured.promise; + await page.getByRole('button', { name: 'Refresh issues' }).click(); + await refreshCompleted.promise; + await expect(row.getByRole('link', { name: 'New issue metadata' })).toBeVisible(); + await trustRoute!.route.fulfill({ response: trustRoute!.response }); + await expect(row.getByRole('link', { name: 'New issue metadata' })).toBeVisible(); + await expect(row.getByRole('button', { name: 'Trust this issue' })).toBeVisible(); +}); + test('shows unavailable, then current, then stale issue data with the retrieval error', async ({ page }) => { const { gateway, pending } = scriptedGateway(); app = await startServer(createDemo(join(root, 'demo')), 0, undefined, undefined, undefined, gateway); diff --git a/test/cli.test.ts b/test/cli.test.ts index 3b0ac2c3..78ad8561 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -160,9 +160,16 @@ it('starts a publish the database owes once startup has verified the lock (#103) store.settleAttempt(identity, attempt.id, { firstReason: null, exitCode: 1, valid: false }); store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); } finally { store.close(); } - // A gh that records each call and fails, so nothing reaches GitHub. + // A gh that admits the collaborator-authored issue, then records and fails the publish so nothing reaches GitHub. const bin = docker.path.split(':')[0]!, calls = join(bin, 'gh-calls'); - writeFileSync(join(bin, 'gh'), `#!/bin/sh\necho "$@" >> '${calls}'\nexit 1\n`); chmodSync(join(bin, 'gh'), 0o755); + writeFileSync(join(bin, 'gh'), `#!/bin/sh +echo "$@" >> '${calls}' +case "$*" in + *repos/owner/repo/issues/3*) printf '%s' '{"number":3,"user":{"login":"member"}}'; exit 0 ;; + *repos/owner/repo/collaborators*) printf '%s' '[{"login":"member"}]'; exit 0 ;; +esac +exit 1 +`); chmodSync(join(bin, 'gh'), 0o755); docker.release(); const cli = start(docker); try { diff --git a/test/issue-board.test.ts b/test/issue-board.test.ts index 759c389b..43beb07f 100644 --- a/test/issue-board.test.ts +++ b/test/issue-board.test.ts @@ -2,6 +2,7 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { request } from 'node:http'; +import { randomUUID } from 'node:crypto'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { IssueBoard } from '../web/issues.ts'; import { startServer } from '../web/server.ts'; @@ -84,6 +85,44 @@ describe('issue board', () => { }); }); + it('exposes only fresh board trust as a control-admission hint', async () => { + const { gateway, calls } = heldGateway(); + const board = new IssueBoard(gateway); + expect(board.trustStatus(1)).toBe('unknown'); + const first = board.refresh(); + calls[0]!.result.resolve({ ...snapshot(), issues: [{ ...snapshot().issues[0]!, trust: 'requires-approval' }] }); + await first; + expect(board.trustStatus(1)).toBe('blocked'); + const second = board.refresh(); + calls[1]!.result.reject(new Error('gh: HTTP 502')); + await second; + expect(board.trustStatus(1)).toBe('unknown'); + }); + + it('checks trust only for the requested non-collaborator issue', async () => { + const { gateway, calls } = heldGateway(), trust = vi.fn(() => null); + const board = new IssueBoard(gateway, undefined, undefined, trust); + const issues = Array.from({ length: 1_000 }, (_, index) => ({ ...snapshot().issues[0]!, number: index + 1, + title: `Issue ${index + 1}`, trust: 'requires-approval' as const })); + const refresh = board.refresh(); + calls[0]!.result.resolve({ ...snapshot(), issues }); + await refresh; + trust.mockClear(); + expect(board.trustStatus(1_000)).toBe('blocked'); + expect(trust).toHaveBeenCalledTimes(1); + expect(trust).toHaveBeenCalledWith('owner/repo', 1_000); + }); + + it('shows a matching explicit decision before collaborator trust so its broader permission can be removed', async () => { + const { gateway, calls } = heldGateway(); + const board = new IssueBoard(gateway, undefined, undefined, () => ({ repository: 'owner/repo', issue: 1, + authorLogin: 'owner', trustedBy: 'local user', trustedAt: '2026-09-25T00:00:00.000Z', revokedAt: null })); + const refresh = board.refresh(); + calls[0]!.result.resolve(snapshot()); + await refresh; + expect(board.view()).toMatchObject({ state: { issues: [{ trust: 'approved', trustedBy: 'local user' }] } }); + }); + it('close aborts the refresh, awaits its settlement, and refuses new refreshes', async () => { const { gateway, calls } = heldGateway(); const board = new IssueBoard(gateway); @@ -140,6 +179,61 @@ describe('issue endpoints', { timeout: 30_000 }, () => { } finally { await app.close(); } }); + it('records repository-and-author-bound trust, replays it, and refuses a stale author precondition', async () => { + root = mkdtempSync(join(tmpdir(), 'codeboost-issues-trust-')); + let author: string | null = 'outside', accessReads = 0; + const base = snapshot(), external = { ...base, issues: [{ ...base.issues[0]!, authorLogin: author, trust: 'requires-approval' as const }] }; + const gateway = { + repository: 'owner/repo', + async fetch() { return { ...external, issues: [{ ...external.issues[0]!, authorLogin: author }] }; }, + async issueAccess(number: number) { accessReads++; return { number, authorLogin: author, collaborator: false }; }, + async issueText(number: number) { return { number, title: 'One', body: '', comments: [] }; }, + }; + const app = await startServer(createDemo(join(root, 'demo')), 0, undefined, undefined, undefined, gateway); + try { + await call(app.url, app.token, 'POST', { action: 'refresh' }); + const actionId = randomUUID(), requestBody = { action: 'trust', actionId, number: 1, authorLogin: 'outside' }; + const trusted = await call(app.url, app.token, 'POST', requestBody); + expect(trusted).toMatchObject({ status: 200, body: { state: { issues: [{ trust: 'approved', trustedBy: 'local user' }] } } }); + expect((await call(app.url, app.token, 'POST', requestBody)).body).toEqual(trusted.body); + expect(accessReads).toBe(1); + const untrustId = randomUUID(); + expect(await call(app.url, app.token, 'POST', { ...requestBody, actionId: untrustId, action: 'untrust' })) + .toMatchObject({ status: 200, body: { state: { issues: [{ trust: 'requires-approval' }] } } }); + expect(await call(app.url, app.token, 'POST', { ...requestBody, actionId: randomUUID() })) + .toMatchObject({ status: 200, body: { state: { issues: [{ trust: 'approved' }] } } }); + const replayedUntrust = await call(app.url, app.token, 'POST', { ...requestBody, actionId: untrustId, action: 'untrust' }); + expect(replayedUntrust).toMatchObject({ status: 200, body: { state: { issues: [{ trust: 'approved' }] } } }); + expect(accessReads).toBe(3); + author = 'renamed'; + const stale = await call(app.url, app.token, 'POST', { ...requestBody, actionId: randomUUID(), action: 'untrust' }); + expect(stale).toMatchObject({ status: 409, body: { error: expect.stringMatching(/author changed/i) } }); + } finally { await app.close(); } + }); + + it('records an issue-access failure under the trust action ID and replays the same 502 without another read', async () => { + root = mkdtempSync(join(tmpdir(), 'codeboost-issues-trust-failure-')); + let accessReads = 0, fail = true; + const gateway = { + repository: 'owner/repo', + async fetch() { return snapshot(); }, + async issueAccess(number: number) { accessReads++; if (fail) throw new Error('GitHub unavailable'); + return { number, authorLogin: 'outside', collaborator: false }; }, + async issueText(number: number) { return { number, title: 'One', body: '', comments: [] }; }, + }; + const app = await startServer(createDemo(join(root, 'demo')), 0, undefined, undefined, undefined, gateway); + try { + await call(app.url, app.token, 'POST', { action: 'refresh' }); + const requestBody = { action: 'trust', actionId: randomUUID(), number: 1, authorLogin: 'outside' }; + expect(await call(app.url, app.token, 'POST', requestBody)).toMatchObject({ status: 502, + body: { error: expect.stringMatching(/GitHub unavailable/) } }); + fail = false; + expect(await call(app.url, app.token, 'POST', requestBody)).toMatchObject({ status: 502, + body: { error: expect.stringMatching(/GitHub unavailable/) } }); + expect(accessReads).toBe(1); + } finally { await app.close(); } + }); + it('a disconnected browser stops waiting while the shared refresh continues for another caller', async () => { root = mkdtempSync(join(tmpdir(), 'codeboost-issues-')); const { gateway, calls } = heldGateway(); diff --git a/test/issues.test.ts b/test/issues.test.ts index 986bdad4..43aa60d6 100644 --- a/test/issues.test.ts +++ b/test/issues.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; -import { GhIssueGateway, ISSUE_PAGE_MAX_BYTES, type IssueText } from '../github/issues.ts'; +import { GhIssueGateway, ISSUE_PAGE_MAX_BYTES, ISSUE_PIPE_GRACE_MS, ISSUE_KILL_GRACE_MS, ISSUE_READ_ACTIVE_MS, + ISSUE_READ_TIMEOUT_MS, withIssueReadDeadline, type IssueText } from '../github/issues.ts'; import { prepareExecution } from '../core/execution-prompt.ts'; const rawIssue = (overrides: Record = {}) => ({ @@ -25,6 +26,71 @@ const responses = (issues: readonly unknown[], collaborators: readonly string[] ? collaborators.map(login => ({ login })) : issues); +it('shares one issue-read deadline across sequential stages and awaits subprocess settlement inside the wall budget', async () => { + vi.useFakeTimers(); + try { + const caller = new AbortController(), access = Promise.withResolvers(); + let shared: AbortSignal | undefined, settled = false; + const operation = withIssueReadDeadline(caller.signal, async (signal, timeoutMs) => { + shared = signal; + expect(timeoutMs).toBe(ISSUE_READ_ACTIVE_MS); + await access.promise; + return new Promise((_resolve, reject) => signal.addEventListener('abort', () => { + setTimeout(() => { settled = true; reject(signal.reason); }, ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS); + }, { once: true })); + }); + const outcome = operation.then(() => null, error => error); + await vi.advanceTimersByTimeAsync(20_000); + access.resolve(); + await vi.advanceTimersByTimeAsync(ISSUE_READ_ACTIVE_MS - 20_001); + expect(shared?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(shared?.aborted).toBe(true); + expect(settled).toBe(false); + await vi.advanceTimersByTimeAsync(ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS - 1); + expect(settled).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(await outcome).toBe(shared?.reason); + expect(settled).toBe(true); + expect(ISSUE_READ_ACTIVE_MS + ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS).toBe(ISSUE_READ_TIMEOUT_MS); + } finally { vi.useRealTimers(); } +}); + +it('preserves the caller abort reason and waits for the active issue stage to settle', async () => { + const caller = new AbortController(), aborted = Promise.withResolvers(), release = Promise.withResolvers(); + const reason = new Error('caller stopped'); + const operation = withIssueReadDeadline(caller.signal, async signal => { + signal.addEventListener('abort', () => aborted.resolve(), { once: true }); + await release.promise; + signal.throwIfAborted(); + }); + let settled = false; + void operation.finally(() => { settled = true; }).catch(() => undefined); + caller.abort(reason); + await aborted.promise; + expect(settled).toBe(false); + release.resolve(); + await expect(operation).rejects.toBe(reason); +}); + +it('refuses a successful issue stage that settles after the shared deadline', async () => { + vi.useFakeTimers(); + try { + let shared: AbortSignal | undefined; + const operation = withIssueReadDeadline(new AbortController().signal, signal => { + shared = signal; + return new Promise(resolve => signal.addEventListener('abort', () => { + setTimeout(() => resolve('late success'), ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS); + }, { once: true })); + }); + const outcome = operation.then(value => value, error => error); + await vi.advanceTimersByTimeAsync(ISSUE_READ_ACTIVE_MS); + expect(shared?.aborted).toBe(true); + await vi.advanceTimersByTimeAsync(ISSUE_KILL_GRACE_MS + ISSUE_PIPE_GRACE_MS); + expect(await outcome).toBe(shared?.reason); + } finally { vi.useRealTimers(); } +}); + describe('GitHub issue retrieval', () => { it.each([ 'OWNER', @@ -253,12 +319,129 @@ describe('issue text for an execute prompt (#91)', () => { expect(text.comments).toEqual([...full.filter((_, n) => n % 2).map(c => c.body), 'last']); expect(calls.filter(args => args[5]!.endsWith('/comments')).map(args => args.at(-1))).toEqual(['page=1', 'page=2']); }); + it('includes every comment only when trust matches the issue current author', async () => { + const comments = [comment('member', 'collaborator'), comment('outsider', 'outside'), comment(null, 'ghost')]; + const fixture = gateway([comments], rawIssue({ user: { login: 'outside-author' } })), g = fixture.gateway; + expect((await g.issueText(7)).comments).toEqual(['collaborator']); + expect((await g.issueText(7, { trustedAuthor: 'old-author' })).comments).toEqual(['collaborator']); + expect((await g.issueText(7, { trustedAuthor: 'outside-author' })).comments).toEqual(['collaborator', 'outside', 'ghost']); + expect(fixture.calls.filter(isCollaboratorRequest)).toHaveLength(2); + }); + it('reads the current author and collaborator list as one bounded admission decision', async () => { + await expect(gateway([], rawIssue({ user: { login: 'Member' } })).gateway.issueAccess(7)).resolves.toEqual({ + number: 7, authorLogin: 'Member', collaborator: true, + }); + await expect(gateway([], rawIssue({ user: null })).gateway.issueAccess(7)).resolves.toEqual({ + number: 7, authorLogin: null, collaborator: false, + }); + }); + it.each([ + ['old-author', 'new-author'], + ['old-author', null], + [null, 'new-author'], + ] as const)('refuses issue access when the author changes from %s to %s during collaborator retrieval', async (before, after) => { + const collaboratorsStarted = Promise.withResolvers(), release = Promise.withResolvers(); + const calls: string[] = []; + let current: string | null = before; + const g = new GhIssueGateway('owner/repo', async args => { + if (isCollaboratorRequest(args)) { + calls.push('collaborators'); collaboratorsStarted.resolve(); await release.promise; + return JSON.stringify(before === null ? [] : [{ login: before }]); + } + calls.push('issue'); + return JSON.stringify(rawIssue({ user: current === null ? null : { login: current } })); + }); + const checking = g.issueAccess(7); + await collaboratorsStarted.promise; + current = after; + release.resolve(); + await expect(checking).rejects.toThrow(/author changed during access verification/); + expect(calls).toEqual(['issue', 'collaborators', 'issue']); + }); + it('accepts a stable ghost author only after rereading it after collaborator retrieval', async () => { + const calls: string[] = []; + const g = new GhIssueGateway('owner/repo', async args => { + if (isCollaboratorRequest(args)) { calls.push('collaborators'); return '[]'; } + calls.push('issue'); return JSON.stringify(rawIssue({ user: null })); + }); + await expect(g.issueAccess(7)).resolves.toEqual({ number: 7, authorLogin: null, collaborator: false }); + expect(calls).toEqual(['issue', 'collaborators', 'issue']); + }); + it('refuses text when its current author or collaborator access differs from admission', async () => { + const changedAuthor = gateway([], rawIssue({ user: { login: 'other' } }), ['other']).gateway; + await expect(changedAuthor.issueText(7, { expectedAccess: { number: 7, authorLogin: 'member', collaborator: true } })) + .rejects.toThrow(/author or collaborator access changed during admission/); + const changedAccess = gateway([], rawIssue({ user: { login: 'member' } }), []).gateway; + await expect(changedAccess.issueText(7, { expectedAccess: { number: 7, authorLogin: 'member', collaborator: true } })) + .rejects.toThrow(/author or collaborator access changed during admission/); + }); + it.each([ + ['old-author', 'new-author'], + [null, 'new-author'], + ] as const)('refuses issue text when the author changes from %s to %s while comments load', async (before, after) => { + const commentsStarted = Promise.withResolvers(), release = Promise.withResolvers(); + const calls: string[] = []; + let current: string | null = before; + const g = new GhIssueGateway('owner/repo', async args => { + if (isCollaboratorRequest(args)) { calls.push('collaborators'); return '[]'; } + if (args[5]!.endsWith('/comments')) { + calls.push('comments'); commentsStarted.resolve(); await release.promise; return '[]'; + } + calls.push('issue'); + return JSON.stringify(rawIssue({ user: current === null ? null : { login: current } })); + }); + const checking = g.issueText(7, { trustedAuthor: before, + expectedAccess: { number: 7, authorLogin: before, collaborator: false } }); + await commentsStarted.promise; + current = after; + release.resolve(); + await expect(checking).rejects.toThrow(/author or collaborator access changed during admission/); + expect(calls).toEqual(['issue', 'collaborators', 'issue', 'comments', 'issue', 'collaborators', 'issue']); + }); + it('revalidates every included collaborator comment author after comment pagination', async () => { + const read = async (removeCommentAuthor: boolean, explicitlyTrusted: boolean) => { + const commentsStarted = Promise.withResolvers(), release = Promise.withResolvers(); + const calls: string[] = []; + let collaborators = ['issue-author', 'comment-author']; + const g = new GhIssueGateway('owner/repo', async args => { + if (isCollaboratorRequest(args)) { + calls.push('collaborators'); return JSON.stringify(collaborators.map(login => ({ login }))); + } + if (args[5]!.endsWith('/comments')) { + calls.push('comments'); commentsStarted.resolve(); await release.promise; + return JSON.stringify([comment('comment-author', 'prompt injection')]); + } + calls.push('issue'); return JSON.stringify(rawIssue({ user: { login: 'issue-author' } })); + }); + const result = g.issueText(7, { ...(explicitlyTrusted ? { trustedAuthor: 'issue-author' } : {}), + expectedAccess: { number: 7, authorLogin: 'issue-author', collaborator: true } }); + await commentsStarted.promise; + if (removeCommentAuthor) collaborators = ['issue-author']; + release.resolve(); + return { result, calls }; + }; + + const removed = await read(true, false); + await expect(removed.result).rejects.toThrow(/author or collaborator access changed during admission/); + expect(removed.calls).toEqual(['issue', 'collaborators', 'issue', 'comments', 'issue', 'collaborators', 'issue']); + + const stable = await read(false, false); + await expect(stable.result).resolves.toMatchObject({ comments: ['prompt injection'] }); + + const explicitlyTrusted = await read(true, true); + await expect(explicitlyTrusted.result).resolves.toMatchObject({ comments: ['prompt injection'] }); + }); it('refuses a pull request, a different issue, and text too long for a prompt', async () => { await expect(gateway([], rawIssue({ pull_request: { url: 'x' } })).gateway.issueText(7)).rejects.toThrow(/is a pull request/); await expect(gateway([], rawIssue({ number: 8 })).gateway.issueText(7)).rejects.toThrow(/different issue/); const long = Array.from({ length: 9 }, () => comment('member', 'x'.repeat(65_000))); await expect(gateway([long]).gateway.issueText(7)).rejects.toThrow(/larger than the 32 KiB an execute prompt carries/); }); + it('describes oversized explicitly trusted comments without calling them collaborator comments', async () => { + const outside = gateway([[comment('outsider', 'x'.repeat(40_000))]], rawIssue({ user: { login: 'outside-author' } }), []); + await expect(outside.gateway.issueText(7, { trustedAuthor: 'outside-author' })) + .rejects.toThrow(/title, body and included comments are larger than the 32 KiB/); + }); it('accepts exactly what an execute prompt can carry, end to end, and refuses the rest at the fetch (#91)', async () => { const promptOf = (issue: IssueText) => prepareExecution({ identity: { repositoryId: 'repo', taskId: 'task', planId: 'plan' }, attemptId: 'attempt-1', mode: 'execute', itemId: 'P1', approvedLessons: [], allowedCommands: [], issue, diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 9dce8961..7444365a 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -29,7 +29,7 @@ async function serve(answer: (view: View) => Answer, options: { describe?: Plann const provider: AuthorProvider = { invoke: (request, signal) => { requests.push(request); return answering(request, signal); } }; let issue = 0; const planning: PlanningDeps = { provider, describe: options.describe ?? (() => ({ issue: { number: issue, title: 'Retries', body: '', comments: [] }, - approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } })) }; + approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' }, validate: () => undefined })) }; const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, planning); let closed = false; const close = async () => { if (!closed) { closed = true; await app.close(); } }; @@ -195,7 +195,7 @@ it('reports a GitHub failure before a draft as 502, recording nothing', async () it('refuses a draft with 503 when shutdown begins while the issue is read', async () => { let resolve!: () => void; - const served = await serve(redraft, { describe: () => new Promise(done => { resolve = () => done({ issue: { number: 0, title: '', body: '', comments: [] }, approvedLessons: [], repo: { name: 'r', baseRef: 'main' } }); }) }); + const served = await serve(redraft, { describe: () => new Promise(done => { resolve = () => done({ issue: { number: 0, title: '', body: '', comments: [] }, approvedLessons: [], repo: { name: 'r', baseRef: 'main' }, validate: () => undefined }); }) }); const started = served.start('drafts'); await vi.waitFor(() => expect(resolve).toBeTypeOf('function')); const closing = served.close(); @@ -317,7 +317,7 @@ it('refuses a draft against a stale revision or snapshot, starting nothing', asy it('replays a draft start without reading GitHub again', async () => { let reads = 0, issue = 0; const served = await serve(redraft, { describe: () => { reads++; return { issue: { number: issue, title: 'Retries', body: '', comments: [] }, - approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' } }; } }); + approvedLessons: [], repo: { name: 'retry-service', baseRef: 'main' }, validate: () => undefined }; } }); issue = served.view.plan.issue; const body = { expectedRevision: served.view.plan.revision, snapshotId: served.view.snapshot.id, feedback: '', actionId: randomUUID() }; const first = await served.api('POST', '/api/plan/drafts', body); diff --git a/test/planning-drafts.test.ts b/test/planning-drafts.test.ts index 9ef94ef6..e7939170 100644 --- a/test/planning-drafts.test.ts +++ b/test/planning-drafts.test.ts @@ -170,7 +170,7 @@ it.each([9, 10])('migrates a v%i database to the current schema, reading its exi const migrated = open(); expect(migrated.getSuggestions(identity, id)).toMatchObject({ mode: 'suggest', state: 'pending' }); const db = new DatabaseSync(path); - try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); } finally { db.close(); } + try { expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); } finally { db.close(); } }); it('adds checkpoint bindings to v14 planning requests without losing their state', () => { const { store, path, open } = fixture(), id = store.beginSuggestions(identity, state(store), 'draft'); @@ -181,7 +181,7 @@ it('adds checkpoint bindings to v14 planning requests without losing their state expect(migrated.getDraft(identity, id).state).toBe('pending'); const db = new DatabaseSync(path); try { - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); expect(db.prepare('PRAGMA table_info(requests)').all().some(column => column.name === 'continuation')).toBe(true); } finally { db.close(); } }); diff --git a/test/planning-production.test.ts b/test/planning-production.test.ts index d6ede2a3..eec89019 100644 --- a/test/planning-production.test.ts +++ b/test/planning-production.test.ts @@ -4,14 +4,15 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, expect, it, vi } from 'vitest'; import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; -import type { IssueText } from '../github/issues.ts'; +import { ISSUE_READ_ACTIVE_MS, type IssueAccess, type IssueText } from '../github/issues.ts'; import { createDemo } from '../scripts/demo.ts'; import type { PlanningAgent } from '../runner/planning.ts'; import { ReviewService, type ReviewConfig } from '../runner/review.ts'; -import { ISSUE_READ_TIMEOUT_MS, productionPlanning } from '../web/planning.ts'; +import { productionPlanning } from '../web/planning.ts'; import { DatabaseSync } from 'node:sqlite'; import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; import { PLANNING_SHUTDOWN_GRACE_MS, startServer, type PlanningDeps } from '../web/server.ts'; +import { GuardRefusal } from '../runner/lifecycle.ts'; // Production planning wiring (#117): when it is on, what each request is told, and how the server awaits the issue. vi.setConfig({ testTimeout: 20_000 }); @@ -30,6 +31,9 @@ function production(config = demo()): ReviewConfig { return { ...config, demo: false, github: { repository: 'acme/retry-service', pullRequest: 7, issue } }; } const text = (number: number): IssueText => ({ number, title: 'Retries ignore the cap', body: 'Body with .', comments: ['Collaborator note.'] }); +const issues = (issueText: (number: number, options?: { trustedAuthor?: string | null; expectedAccess?: IssueAccess }) => Promise, collaborator = true) => ({ + issueAccess: async (number: number) => ({ number, authorLogin: 'outside', collaborator }), issueText, +}); const closable = (close = vi.fn(async () => undefined)) => ({ close, invoke: vi.fn() }) as unknown as PlanningAgent; const verified = { verifyLock: () => undefined }; @@ -42,11 +46,64 @@ it('is off in a demo and without a github block, so neither plans', () => { it('tells each request the GitHub issue, the repository and the base commit, read with a bound', async () => { const config = production(), service = new ReviewService(config); closers.push(() => service.close()); - const issueText = vi.fn(async (number: number) => text(number)), signal = new AbortController().signal; - const deps = productionPlanning(config, { ...verified, issues: { issueText }, agent: () => closable() })!(service); - expect(await deps.describe(signal)).toEqual({ issue: text(config.github!.issue), approvedLessons: [], - repo: { name: 'acme/retry-service', baseRef: service.store.getSnapshot(config.identity).base } }); - expect(issueText).toHaveBeenCalledWith(config.github!.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); + const issueText = vi.fn(async (number: number, options?: { signal?: AbortSignal; timeoutMs?: number; + trustedAuthor?: string | null; expectedAccess?: IssueAccess }) => text(number)); + const issueAccess = vi.fn(async (number: number, _options?: { signal?: AbortSignal; timeoutMs?: number }) => + ({ number, authorLogin: 'outside', collaborator: true })); + const signal = new AbortController().signal; + const deps = productionPlanning(config, { ...verified, issues: { issueAccess, issueText }, agent: () => closable() })!(service); + expect(await deps.describe(signal)).toMatchObject({ issue: text(config.github!.issue), approvedLessons: [], + repo: { name: 'acme/retry-service', baseRef: service.store.getSnapshot(config.identity).base }, validate: expect.any(Function) }); + expect(issueAccess.mock.calls[0]![1]!.signal).toBe(issueText.mock.calls[0]![1]!.signal); + expect(issueAccess.mock.calls[0]![1]!.signal).not.toBe(signal); + expect(issueAccess.mock.calls[0]![1]!.timeoutMs).toBe(ISSUE_READ_ACTIVE_MS); + expect(issueText).toHaveBeenCalledWith(config.github!.issue, { signal: issueAccess.mock.calls[0]![1]!.signal, + timeoutMs: ISSUE_READ_ACTIVE_MS, trustedAuthor: undefined, + expectedAccess: { number: config.github!.issue, authorLogin: 'outside', collaborator: true } }); +}); + +it('passes explicit trust into planning comments and refuses the next read after revocation', async () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + const issueText = vi.fn(async (number: number, options?: { trustedAuthor?: string | null }) => + ({ ...text(number), comments: options?.trustedAuthor === 'outside' ? ['Outside note.'] : [] })); + const deps = productionPlanning(config, { ...verified, issues: issues(issueText, false), agent: () => closable() })!(service); + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + await expect(deps.describe(new AbortController().signal)).resolves.toMatchObject({ issue: { comments: ['Outside note.'] } }); + expect(issueText).toHaveBeenLastCalledWith(config.github!.issue, expect.objectContaining({ trustedAuthor: 'outside' })); + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + await expect(deps.describe(new AbortController().signal)).rejects.toThrow(/not trusted/); +}); + +it('refuses when the issue author or collaborator access changes between admission and the text read', async () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + const issueText = vi.fn(async (_number: number, options?: { expectedAccess?: IssueAccess }) => { + expect(options?.expectedAccess).toEqual({ number: config.github!.issue, authorLogin: 'outside', collaborator: true }); + throw new Error(`Issue #${config.github!.issue}'s author or collaborator access changed during admission.`); + }); + const deps = productionPlanning(config, { ...verified, issues: issues(issueText), agent: () => closable() })!(service); + await expect(deps.describe(new AbortController().signal)).rejects.toThrow(/access changed during admission/); +}); + +it('refuses all-comments text when explicit trust is revoked during the awaited planning read', async () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + const started = Promise.withResolvers(), release = Promise.withResolvers(); + const issueText = async (number: number) => { started.resolve(); await release.promise; return text(number); }; + const deps = productionPlanning(config, { ...verified, issues: issues(issueText, false), agent: () => closable() })!(service); + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + const description = deps.describe(new AbortController().signal); + await started.promise; + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + release.resolve(); + await expect(description).rejects.toThrow(/not trusted/); +}); + +it('carries explicit trust to the planning prompt boundary', async () => { + const config = production(), service = new ReviewService(config); closers.push(() => service.close()); + const deps = productionPlanning(config, { ...verified, issues: issues(async number => text(number), false), agent: () => closable() })!(service); + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + const described = await deps.describe(new AbortController().signal); + service.store.setIssueTrust({ repository: config.github!.repository, issue: config.github!.issue, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + expect(() => described.validate()).toThrow(/not trusted/); }); it.each([['release', 'release'], ['', null]] as const)('names the configured base branch %j, or else the base commit, in the prompt', async (baseBranch, expected) => { @@ -55,7 +112,7 @@ it.each([['release', 'release'], ['', null]] as const)('names the configured bas const agent = () => ({ close: async () => undefined, invoke: async (request: AuthorRequest, signal: AbortSignal) => { requests.push(request); return new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true })); } }) as unknown as PlanningAgent; const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, - productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent })!); + productionPlanning(config, { ...verified, issues: issues(async number => text(number)), agent })!); closers.push(() => app.close()); const call = async (method: string, path: string, body?: unknown) => (await fetch(`${new URL(app.url).origin}${path}`, { method, headers: { 'x-codeboost-token': app.token, ...(body ? { 'content-type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined })).json() as Promise>; @@ -89,7 +146,7 @@ it('closes the Store and starts nothing when the planning setup refuses', async it('closes the planning agent when the server closes', async () => { const config = production(), close = vi.fn(async () => undefined); - const setup = productionPlanning(config, { ...verified, issues: { issueText: async number => text(number) }, agent: () => closable(close) })!; + const setup = productionPlanning(config, { ...verified, issues: issues(async number => text(number)), agent: () => closable(close) })!; const app = await startServer(config, 0, async () => 'answer', undefined, 2_000, undefined, undefined, setup); await app.close(); expect(close).toHaveBeenCalledTimes(1); @@ -123,7 +180,8 @@ function holding() { return new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true })); } }; return { provider, requests }; } -const issueOf = (number: number) => ({ issue: text(number), approvedLessons: [], repo: { name: 'acme/retry-service', baseRef: 'abc' } }); +const issueOf = (number: number) => ({ issue: text(number), approvedLessons: [], repo: { name: 'acme/retry-service', baseRef: 'abc' }, + validate: () => undefined }); it('awaits the issue before starting a suggestion, and sends it to the provider', async () => { const requests: AuthorRequest[] = []; @@ -136,7 +194,7 @@ it('awaits the issue before starting a suggestion, and sends it to the provider' const started = start(); await new Promise(done => setTimeout(done, 50)); expect(requests).toHaveLength(0); - resolve({ issue: text(view.plan.issue), approvedLessons: [], repo: { name: 'acme/retry-service', baseRef: 'abc' } }); + resolve({ issue: text(view.plan.issue), approvedLessons: [], repo: { name: 'acme/retry-service', baseRef: 'abc' }, validate: () => undefined }); const response = await started; expect(response).toMatchObject({ status: 200, body: { result: { requestId: expect.any(String) } } }); await vi.waitFor(() => expect(requests).toHaveLength(1)); @@ -144,11 +202,47 @@ it('awaits the issue before starting a suggestion, and sends it to the provider' expect(requests[0]!.prompt).toContain('acme/retry-service'); }); -it('starts no suggestion when the issue cannot be read, and reports GitHub\'s failure as 502', async () => { - const invoke = vi.fn(); - const { start, recorded } = await serve({ provider: { invoke }, describe: async () => { throw new Error('GitHub is unreachable.'); } }); - const response = await start(); +it('starts no suggestion when the issue cannot be read, and durably replays GitHub\'s 502', async () => { + let available = false; + const invoke = vi.fn(), describe = vi.fn(async () => { + if (!available) throw new Error('GitHub is unreachable.'); + return issueOf(1); + }); + const { start, recorded } = await serve({ provider: { invoke }, describe }); + const actionId = randomUUID(), response = await start(actionId); expect(response).toMatchObject({ status: 502, body: { error: 'The issue could not be read from GitHub: GitHub is unreachable.' } }); + available = true; + expect(await start(actionId)).toEqual(response); + expect(invoke).not.toHaveBeenCalled(); + expect(describe).toHaveBeenCalledTimes(1); + expect(recorded()).toBe(0); +}); + +it('preserves a planning trust refusal as 409 instead of labelling it a GitHub read failure', async () => { + let trusted = false; + const invoke = vi.fn(), describe = vi.fn(async () => { + if (!trusted) throw new GuardRefusal('Issue is not trusted.'); + return issueOf(1); + }); + const { start, recorded } = await serve({ provider: { invoke }, describe }); + const actionId = randomUUID(); + expect(await start(actionId)).toMatchObject({ status: 409, body: { error: 'Issue is not trusted.' } }); + trusted = true; + expect(await start(actionId)).toMatchObject({ status: 409, body: { error: 'Issue is not trusted.' } }); + expect(invoke).not.toHaveBeenCalled(); + expect(describe).toHaveBeenCalledTimes(1); + expect(recorded()).toBe(0); +}); + +it('revalidates planning trust after describe resolves and before constructing the prompt', async () => { + let trusted = true; + const invoke = vi.fn(), description = issueOf(1); + const describe = () => new Promise & { validate(): void }>(resolve => { + resolve({ ...description, validate: () => { if (!trusted) throw new GuardRefusal('Issue is not trusted.'); } }); + queueMicrotask(() => { trusted = false; }); + }); + const { start, recorded } = await serve({ provider: { invoke }, describe }); + expect(await start()).toMatchObject({ status: 409, body: { error: 'Issue is not trusted.' } }); expect(invoke).not.toHaveBeenCalled(); expect(recorded()).toBe(0); }); diff --git a/test/publish.test.ts b/test/publish.test.ts index c93e1a35..6d90569a 100644 --- a/test/publish.test.ts +++ b/test/publish.test.ts @@ -64,6 +64,7 @@ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: } }; const pulls: PullRequestGateway = { async open(input) { + const finalize = await input.beforeOpen?.(); finalize?.(); log.push(`open ${input.draft ? 'draft' : 'ready'}`); opened.push(input); if (options.open) return options.open(input); if (options.draftsUnsupported && input.draft) throw new DraftsUnsupported('no drafts'); @@ -111,6 +112,7 @@ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: .map(([m, pr]) => ({ ...pr, marker: m, base: bases.get(m) ?? publishConfig.baseBranch }))); }, async markDraft(number, input) { + const finalize = await input.beforeDraft?.(); finalize?.(); log.push(`draft ${number}`); // Like the adapter's read-back: the PR must be from the branch and into the base asked for. if ((bases.get(input.marker) ?? input.base) !== input.base) throw new Error('GitHub returned a pull request for a different branch.'); @@ -135,10 +137,11 @@ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: return { number, url: 'https://github.com/owner/repo/pull/1' }; }, async refresh(number, input) { + const finalizePatch = await input.beforePatch?.(); finalizePatch?.(); log.push(`refresh ${number} ${input.ready ? 'ready' : 'draft'}`); opened.push(input); if ((bases.get(input.marker) ?? input.base) !== input.base) throw new Error('GitHub returned a pull request for a different branch.'); options.onRefresh?.(); - input.beforeReady?.(); + const finalizeReady = await input.beforeReady?.(); finalizeReady?.(); if (options.refreshFails) throw new Error('timeout reading the PR back'); if (options.draftsUnsupported && input.draft) throw new DraftsUnsupported('no drafts'); if (closed.has(number)) throw new Error(`Pull request #${number} is not open.`); @@ -150,7 +153,10 @@ function harness(store: Store, options: { results?: AlreadyFixedResult[]; open?: return pr; }, }; - const pusher: BranchPusher = { async push(id, input, signal) { log.push(`push ${input.branch.replace(/-[0-9a-f]{16}$/, '')} ${input.head.slice(-3)}`); await options.push?.(id, input, signal); } }; + const pusher: BranchPusher = { async push(id, input, signal) { + const finalize = await input.beforePush?.(); finalize?.(); + log.push(`push ${input.branch.replace(/-[0-9a-f]{16}$/, '')} ${input.head.slice(-3)}`); await options.push?.(id, input, signal); + } }; const publisher = new PullRequestPublisher(store, { checks: gate, pulls, pusher, closing: options.closing }, publishConfig); publishBranch = publisher.branch(identity); return { log, checks, opened, pulls, publisher }; @@ -242,6 +248,21 @@ describe('opening the task PR', () => { expect(log).not.toContain('open ready'); expect(store.getTask(identity).status).toBe('running'); }); + it('rechecks local task currentness after awaited external authorization and immediately before pushing', async () => { + const store = runningTask(), entered = Promise.withResolvers(), release = Promise.withResolvers(); + let checks = 0; + const run = harness(store); + const publishing = run.publisher.publish(identity, { beforeMutation: async () => { + checks++; + if (checks === 2) { entered.resolve(); await release.promise; } + } }); + await entered.promise; + store.setAssignment(identity, store.getTask(identity).stateVersion, 'someone-else', 'code'); + release.resolve(); + await expect(publishing).rejects.toThrow(/Stale task state/); + expect(run.log.some(line => line.startsWith('push'))).toBe(false); + expect(run.log.some(line => line.startsWith('open'))).toBe(false); + }); it('refuses to open the PR when a review note is added during the push', async () => { const store = runningTask(); const { publisher, log } = harness(store, { push: async () => { @@ -337,7 +358,7 @@ describe('schema v7', () => { expect(upgraded.taskPullRequests(identity)).toEqual([]); expect(upgraded.latestAlreadyFixed(identity)).toBeNull(); const db = new DatabaseSync(path, { readOnly: true }); - expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); const names = db.prepare("SELECT name FROM sqlite_master WHERE type IN ('table','index') AND (name LIKE '%pull_requests%' OR name LIKE 'already_fixed_checks%') AND name NOT LIKE 'sqlite_autoindex%' ORDER BY name").all().map(row => row.name); expect(names).toEqual(['already_fixed_checks', 'already_fixed_checks_task', 'task_pull_requests', 'task_pull_requests_number', 'task_pull_requests_opening', 'task_pull_requests_task']); db.close(); @@ -969,6 +990,21 @@ describe('recovering a lost opening', () => { expect(again.log.some(line => line.startsWith('refresh'))).toBe(false); expect(store.taskPullRequests(identity)).toMatchObject([{ number: 100, draft: true, refresh: { head: oid(3) } }]); }); + it('rechecks authorization after an existing PR push and before refreshing it', async () => { + const store = runningTask(), live = new Map(), next = { value: 100 }; + store.transitionTask(identity, store.getTask(identity).stateVersion, 'needs human'); + await harness(store, { live, next }).publisher.publish(identity, { problems: ['x'] }); + rerun(store); + let checks = 0; + const again = harness(store, { live, next }); + await expect(again.publisher.publish(identity, { beforeMutation: () => { + checks++; + if (checks === 3) throw new GuardRefusal('Issue trust was revoked.'); + } })).rejects.toThrow('Issue trust was revoked'); + expect(checks).toBe(3); + expect(again.log.some(line => line.startsWith('push'))).toBe(true); + expect(again.log.some(line => line.startsWith('refresh'))).toBe(false); + }); it('runs one publish per task at a time, across publishers over the same Store', async () => { const store = runningTask(); let release!: () => void; diff --git a/test/runner-branch-push.test.ts b/test/runner-branch-push.test.ts index 44c05cf9..bdaeabec 100644 --- a/test/runner-branch-push.test.ts +++ b/test/runner-branch-push.test.ts @@ -8,6 +8,7 @@ import { fixtureGit as git } from './fixtures/git.ts'; import type { PlanIdentity } from '../core/identity.ts'; import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts'; import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushArguments, pushEnvironment, pushUrl, redact } from '../runner/branch-push.ts'; +import { GuardRefusal } from '../runner/lifecycle.ts'; import { ensureCommit, fetchTaskCommit, openRunnerRepository, type RunnerRepository } from '../runner/runner-repository.ts'; const OWNER = '0123456789abcdef0123456789abcdef'; @@ -68,6 +69,17 @@ describe('GitBranchPusher', () => { expect(git(s.source, 'for-each-ref')).toBe(sourceRefs); }); + it('runs the final caller guard after the remote read and immediately before the push', async () => { + const s = await setup(), head = await runnerCommit(s, 'guarded'), seen: number[] = []; + const guarded = pusher(s, { onProcessGroup: () => seen.push(seen.length + 1) }); + await expect(guarded.instance.push(IDENTITY, { head, branch: BRANCH, beforePush: () => { + expect(seen).toHaveLength(2); + throw new GuardRefusal('Issue trust was revoked.'); + } })).rejects.toThrow('Issue trust was revoked'); + expect(seen).toHaveLength(2); + expect(remoteRefs(s)).toBe(''); + }); + it('does nothing when the branch is already at the head, as after a push whose outcome was lost', async () => { const s = await setup(), head = await runnerCommit(s, 'one'); await pusher(s).instance.push(IDENTITY, { head, branch: BRANCH }); diff --git a/test/runner-coordinator.test.ts b/test/runner-coordinator.test.ts index f9bf6881..4aabd653 100644 --- a/test/runner-coordinator.test.ts +++ b/test/runner-coordinator.test.ts @@ -254,6 +254,32 @@ describe('storage failures', () => { expect(store.getAttempt(A, attempt.id).state).toBe('pending'); expect(() => runner.start(B, request(store, B))).toThrow(/No free runner slot/); }); + it('fails closed before launch when the synchronous pre-start hook throws', async () => { + const { store, runner, launches, preparations, deps, cleaned } = setup(); + deps.beforeStart = () => { throw new Error('evidence write failed'); }; + const attempt = runner.start(A, request(store, A)); + await until(() => preparations.length === 1, 'preparation'); preparations[0]!.resolve(); + await runner.settled(A); + expect(launches).toEqual([]); + expect(store.getAttempt(A, attempt.id)).toMatchObject({ state: 'failed', diagnostic: 'Launch failed: evidence write failed' }); + expect(cleaned()).toBe(1); + expect(runner.status(A).unresolved).toBeNull(); + }); + it('owns, cancels and settles the handle when the synchronous started hook throws', async () => { + const { store, runner, launches, preparations, deps } = setup(); + const markRunning = vi.spyOn(store, 'markRunning'); + deps.onStarted = () => { throw Object.assign(new Error('evidence write failed'), { code: 'ERR_SQLITE_ERROR' }); }; + const attempt = runner.start(A, request(store, A)); + await until(() => preparations.length === 1, 'preparation'); preparations[0]!.resolve(); + await until(() => launches.length === 1, 'launch'); + expect(launches[0]!.cancels).toEqual(['capture-failure']); + expect(runner.isActive(A)).toBe(true); + expect(markRunning).not.toHaveBeenCalled(); + launches[0]!.settle({ exitCode: null, stopReason: 'capture-failure' }); + await until(() => !runner.isActive(A), 'settlement'); + expect(runner.status(A).unresolved).toEqual({ attemptId: attempt.id, reason: 'start-not-saved' }); + expect(store.getAttempt(A, attempt.id).state).toBe('pending'); + }); it('still cancels and awaits the handle when reading the refused attempt fails', async () => { const { store, runner, launches, preparations } = setup(); vi.spyOn(store, 'markRunning').mockImplementation(() => { diff --git a/test/runner-execution.test.ts b/test/runner-execution.test.ts index 950c66ed..584fb78b 100644 --- a/test/runner-execution.test.ts +++ b/test/runner-execution.test.ts @@ -3,7 +3,7 @@ import { mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { DatabaseSync } from 'node:sqlite'; import { join } from 'node:path'; -import { randomUUID } from 'node:crypto'; +import { createHash, randomUUID } from 'node:crypto'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { Store } from '../runner/store.ts'; import { RunnerCoordinator } from '../runner/coordinator.ts'; @@ -12,7 +12,9 @@ import { MAX_REASON, ShuttingDownError, type ShutdownCapability } from '../runne import type { ChangeManifest, ManifestChange } from '../core/run-audit.ts'; import type { InvocationResult } from '../agents/contract.ts'; import type { Plan, PlanContext } from '../core/plan.ts'; +import type { PlanIdentity } from '../core/identity.ts'; import { TaskTreeRefused } from '../agents/container/changes.ts'; +import type { IssueText } from '../github/issues.ts'; const oid = (n: number) => n.toString(16).padStart(40, '0'); const RUNNER_OWNER = '0123456789abcdef0123456789abcdef'; @@ -39,7 +41,10 @@ function setup(options: { manifests?: Record void; /** The fake agent's result names this attempt instead. */ - foreignResult?: boolean; issue?: ExecutionSources['issue'] } = {}) { + foreignResult?: boolean; + issue?: (identity: PlanIdentity, signal: AbortSignal) => IssueText | Promise; + guardedIssue?: ExecutionSources['issue']; +} = {}) { const dir = mkdtempSync(join(tmpdir(), 'codeboost-exec-')); dirs.push(dir); const path = join(dir, 'state.sqlite'), store = new Store(path); store.createPlan(JSON.stringify(options.plan ?? plan), 'json', context, oid(1), oid(2)); @@ -84,7 +89,10 @@ function setup(options: { manifests?: Record ({ ...auditContext, baseEntries: [...auditContext.baseEntries, ...Object.values(options.manifests ?? {}).flatMap(value => value.changes.filter(change => change.kind === 'add' && change.path === 'extra.ts') .map(change => ({ path: change.path, kind: 'file' as const })))] }), - issue: options.issue ?? (() => ({ number: 1, title: 'Issue', body: 'Please fix', comments: [] })), lessons: () => [], vendor: () => options.vendor ?? 'claude' }; + issue: options.guardedIssue ?? (async (id, signal) => ({ + text: await (options.issue?.(id, signal) ?? { number: 1, title: 'Issue', body: 'Please fix', comments: [] }), + validate: () => undefined, + })), lessons: () => [], vendor: () => options.vendor ?? 'claude' }; const prompts: string[] = [], argv: (readonly (readonly string[])[])[] = [], owners: string[] = [], checks: unknown[] = []; const capability = options.capability?.(store), findings = new SafetyFindings(store, capability); if (options.settleError) store.settleAttempt = () => { throw Object.assign(new Error('disk full'), { code: 'ERR_SQLITE_ERROR' }); }; @@ -120,6 +128,14 @@ describe('item execution', () => { expect(owners[0]).toBe(RUNNER_OWNER); expect(argv[1]).toEqual([]); }); + it('records a digest and count of the exact comments carried by every execute attempt', async () => { + const comments = ['collaborator note', 'trusted outside note']; + const { store, executor } = setup({ issue: () => ({ number: 1, title: 'Issue', body: '', comments }) }); + await executor.runTask(identity); + const evidence = { count: 2, digest: createHash('sha256').update(JSON.stringify(comments)).digest('hex'), state: 'delivered' }; + expect(store.getAttempts(identity).map(attempt => attempt.promptComments)).toEqual([evidence, evidence]); + expect(store.recentAttempts(identity, 20).map(attempt => attempt.promptComments)).toEqual([evidence, evidence]); + }); it('reports a planned-but-unchanged item without committing', async () => { const { executor, commits } = setup({ manifests: { P1: manifest([]) } }); expect(await executor.runTask(identity)).toEqual({ kind: 'executed', items: ['P1', 'P2'], unchanged: ['P1'] }); @@ -230,9 +246,10 @@ describe('item execution', () => { expect(store.getSnapshot(identity).head).toBe(oid(2)); }); it('releases task storage after the terminal write when D\'s start call throws', async () => { - const { executor, log } = setup({ startError: new Error('docker refused') }); + const { store, executor, log } = setup({ startError: new Error('docker refused') }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'failed', reason: 'Launch failed: docker refused' }); expect(log).toContain('release P1 after failed'); + expect(store.getAttempts(identity)[0]!.promptComments).toMatchObject({ count: 0, state: 'prepared' }); }); it('does not take the agent\'s stderr for a safety violation', async () => { const { store, executor } = setup({ exit: { P1: { exitCode: 1, stderr: `${SAFETY_VIOLATION} fake` } } }); @@ -261,6 +278,7 @@ describe('item execution', () => { expect(log).toEqual(['materialize P1 @002', 'snapshot P1 [a.ts]', 'release P1 after failed']); expect(runner.status(identity).unresolved).toBeNull(); expect(store.getTask(identity).status).toBe('running'); + expect(store.getAttempts(identity)[0]!.promptComments).toBeNull(); }); it('stops before the next item when the plan gets a new revision during the run', async () => { let store!: Store; @@ -1063,6 +1081,30 @@ describe('item execution', () => { runner = h.runner; expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P2', state: 'not started', reason: 'The review server is shutting down.', completed: ['P1'] }); }); + it('stops before launching the next item when its per-item issue trust read is refused', async () => { + let reads = 0; + const h = setup({ issue: () => { + if (++reads === 2) throw new GuardRefusal('Issue #1 is not trusted for its current author.'); + return { number: 1, title: 'Issue', body: 'Please fix', comments: [] }; + } }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P2', state: 'failed', completed: ['P1'], + reason: expect.stringMatching(/not trusted for its current author/) }); + expect(h.store.getAttempts(identity)).toMatchObject([{ item: 'P1', state: 'completed' }, { item: 'P2', state: 'failed' }]); + expect(h.log.some(line => line.includes('start P2'))).toBe(false); + }); + it('revalidates issue trust synchronously when the execute prompt is constructed', async () => { + let trusted = true; + const h = setup({ guardedIssue: () => new Promise(resolve => { + resolve({ text: { number: 1, title: 'Issue', body: 'Please fix', comments: ['Outside note.'] }, + validate: () => { if (!trusted) throw new GuardRefusal('Issue #1 is not trusted for its current author.'); } }); + // The source has resolved, but the await continuation that constructs the prompt has not run yet. + trusted = false; + }) }); + expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'stopped', item: 'P1', state: 'failed', + reason: expect.stringMatching(/not trusted for its current author/) }); + expect(h.store.getAttempts(identity)).toMatchObject([{ item: 'P1', state: 'failed', promptComments: null }]); + expect(h.log.some(line => line.includes('start P1'))).toBe(false); + }); it('builds a pause\'s executed prefix from the plan the item ran against, even if a revision inserts an item before it', async () => { let store!: Store, imported: Error | undefined; const h = setup({ manifests: { P1: manifest([change('a.ts'), change('extra.ts', { kind: 'add', oldType: undefined })]) }, release: async () => { diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 173f1d03..f39c4d9f 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -105,7 +105,7 @@ describe('schema v12 review ordering', () => { migrated.saveReview(identity, { revision: 1, snapshotId: snapshot.id, reviewVersion: migratedVersion }, [{ item: 'P1', fingerprint: 'approved-after-upgrade' }], []); expect(migrated.unapprovedExecutionItems(identity, 1)).toEqual([]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); }); @@ -755,7 +755,7 @@ describe('runner commits and preparation groups (#87)', () => { db.exec('ALTER TABLE attempts DROP COLUMN preparation_identity; PRAGMA user_version=15;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, preparationIdentity: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); }); @@ -889,7 +889,7 @@ describe('durable safety findings (#87 item 3)', () => { db.exec('ALTER TABLE attempts DROP COLUMN safety_finding; PRAGMA user_version=7;'); db.close(); const reopened = open(path); expect(reopened.getAttempt(identity, attempt.id).safetyFinding).toBeNull(); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); it('adds the durable owed marker to a version 12 database', () => { @@ -899,7 +899,7 @@ describe('durable safety findings (#87 item 3)', () => { db.exec('ALTER TABLE attempts DROP COLUMN safety_owed; PRAGMA user_version=12;'); db.close(); const reopened = open(path); expect(reopened.owedSafetyFindings()).toEqual([]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); it('restores and clears an escalation owed behind a human gate across real store reopens', () => { @@ -939,7 +939,7 @@ describe('allocation baseline (#91)', () => { db.exec('ALTER TABLE attempts DROP COLUMN metadata_baseline; ALTER TABLE attempts DROP COLUMN storage_base; PRAGMA user_version=8;'); db.close(); const reopened = open(path); expect(reopened.interruptedAttempts()).toEqual([expect.objectContaining({ id: attempt.id, metadataBaseline: null, storageBase: null })]); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); }); describe('publish outcomes (#103)', () => { @@ -955,7 +955,7 @@ describe('publish outcomes (#103)', () => { reopened.recordPublish(identity, { outcome: 'opened', draft: false, message: 'Pull request #1 is open.', number: 1, url: 'https://github.com/o/r/pull/1' }); expect(reopened.lastPublish(identity)).toMatchObject({ outcome: 'opened', number: 1 }); expect(reopened.getTask(identity)).toEqual(before); - expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 16 }); + expect(new DatabaseSync(path).prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); }); it('stamps an outcome with the version the publish saw, and refuses one it cannot have seen (#114)', () => { const { store } = queued(), version = store.getTask(identity).stateVersion; diff --git a/test/runner-planning-feedback.test.ts b/test/runner-planning-feedback.test.ts index de9aeb00..d5be39c3 100644 --- a/test/runner-planning-feedback.test.ts +++ b/test/runner-planning-feedback.test.ts @@ -122,7 +122,7 @@ describe('planning API for lane G', () => { calls.push({ signal, resolve }); signal.addEventListener('abort', () => reject(signal.reason), { once: true }); }) }; - const deps: PlanningDeps = { provider, describe: () => ({ repo: { name: 'retry-service', baseRef: 'main' }, issue: { number: 3, title: 'Retries', body: '', comments: [] }, approvedLessons: [] }) }; + const deps: PlanningDeps = { provider, describe: () => ({ repo: { name: 'retry-service', baseRef: 'main' }, issue: { number: 3, title: 'Retries', body: '', comments: [] }, approvedLessons: [], validate: () => undefined }) }; return { deps, calls }; } const until = async (check: () => boolean) => { for (let i = 0; i < 100 && !check(); i++) await new Promise(r => setTimeout(r, 20)); }; diff --git a/test/runner-production.test.ts b/test/runner-production.test.ts index 529c694f..b5dfde1d 100644 --- a/test/runner-production.test.ts +++ b/test/runner-production.test.ts @@ -11,6 +11,7 @@ import type { RecoveryDeps } from '../runner/recovery.ts'; import type { AgentAdapterRequest } from '../agents/adapters/types.ts'; import { readCapturedFile } from '../agents/container/profile.ts'; import type { InvocationInput } from '../agents/contract.ts'; +import { ISSUE_READ_ACTIVE_MS, type IssueAccess } from '../github/issues.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; import { exportTaskDiff, removeTaskFilesystemsAsync } from '../agents/container/storage.ts'; import { RunnerCoordinator, type RunnerDeps } from '../runner/coordinator.ts'; @@ -19,10 +20,28 @@ import type { GitRebaser } from '../runner/rebase.ts'; vi.mock('../agents/recovery.ts', async original => ({ ...await original(), recoverLeftovers: vi.fn() })); const issueReads: number[] = []; +let issueAuthor = 'member', issueCollaborator = true; +let collaboratorComments: string[] = []; +let afterIssueAccess: (() => void) | undefined; +let beforeIssueTextReturn: (() => Promise | void) | undefined; +const issueStageOptions: { signal?: AbortSignal; timeoutMs?: number }[] = []; vi.mock('../github/issues.ts', async original => { const actual = await original(); return { ...actual, GhIssueGateway: class extends actual.GhIssueGateway { - override async issueText(number: number) { issueReads.push(number); return { number, title: 'T', body: 'B', comments: [] }; } + override async issueAccess(number: number, options: { signal?: AbortSignal; timeoutMs?: number } = {}) { + issueStageOptions.push(options); + const result = { number, authorLogin: issueAuthor, collaborator: issueCollaborator }; + afterIssueAccess?.(); + return result; + } + override async issueText(number: number, options: { signal?: AbortSignal; timeoutMs?: number; + trustedAuthor?: string | null; expectedAccess?: IssueAccess } = {}) { + issueStageOptions.push(options); + await beforeIssueTextReturn?.(); + if (options.expectedAccess && (options.expectedAccess.authorLogin !== issueAuthor || options.expectedAccess.collaborator !== issueCollaborator)) + throw new Error(`Issue #${number}'s author or collaborator access changed during admission.`); + issueReads.push(number); return { number, title: 'T', body: 'B', comments: options.trustedAuthor === issueAuthor ? ['Outside note.'] : [...collaboratorComments] }; + } } }; }); vi.mock('../agents/container/storage.ts', async original => ({ ...await original(), @@ -33,6 +52,11 @@ const roots: string[] = [], cleanups: (() => Promise | void)[] = []; afterEach(async () => { for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); + issueAuthor = 'member'; issueCollaborator = true; + collaboratorComments = []; + afterIssueAccess = undefined; + beforeIssueTextReturn = undefined; + issueStageOptions.length = 0; vi.restoreAllMocks(); }); const OWNER = 'c'.repeat(32), committer = { name: 'codeboost', email: 'runner@codeboost.invalid' }; @@ -93,12 +117,16 @@ describe('runner startup', () => { }); it('reads the issue once for a run of items, and again once the reuse window has passed', async () => { const { root, service } = fixture(); + service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, + authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, buildImage: () => 'x', recovery: () => recovery([]) }); issueReads.length = 0; const signal = new AbortController().signal, identity = service.config.identity; await sources.issue(identity, signal); await sources.issue(identity, signal); expect(issueReads).toHaveLength(1); + expect(issueStageOptions[0]!.signal).toBe(issueStageOptions[1]!.signal); + expect(issueStageOptions.slice(0, 2).map(options => options.timeoutMs)).toEqual([ISSUE_READ_ACTIVE_MS, ISSUE_READ_ACTIVE_MS]); // A wall-clock step back does not keep the old text. vi.spyOn(Date, 'now').mockReturnValue(0); await sources.issue(identity, signal); @@ -108,6 +136,69 @@ describe('runner startup', () => { await sources.issue(identity, signal); expect(issueReads).toHaveLength(2); }); + it('does not reuse collaborator-filtered comments without a complete collaborator snapshot', async () => { + const { root, service } = fixture(); + const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) }); + issueReads.length = 0; + collaboratorComments = ['Former collaborator note.']; + await expect(sources.issue(service.config.identity, new AbortController().signal)).resolves.toMatchObject({ + text: { comments: ['Former collaborator note.'] }, + }); + // The gateway's next filtered view represents that commenter losing collaborator access while the issue author's + // own access remains unchanged. Reusing the old text would keep untrusted instructions in the next prompt. + collaboratorComments = []; + await expect(sources.issue(service.config.identity, new AbortController().signal)).resolves.toMatchObject({ text: { comments: [] } }); + expect(issueReads).toEqual([service.store.getPlan(service.config.identity).issue, service.store.getPlan(service.config.identity).issue]); + }); + it('passes explicit trust into execute comments and refuses the next item read after revocation', async () => { + const { root, service } = fixture(); + issueAuthor = 'outside'; issueCollaborator = false; + service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, + authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); + const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) }); + await expect(sources.issue(service.config.identity, new AbortController().signal)).resolves.toMatchObject({ text: { comments: ['Outside note.'] } }); + service.store.setIssueTrust({ repository: 'owner/repo', issue: service.store.getPlan(service.config.identity).issue, + authorLogin: issueAuthor, trusted: false, trustedBy: 'local user' }); + await expect(sources.issue(service.config.identity, new AbortController().signal)).rejects.toThrow(/not trusted/); + }); + it('refuses when access changes between the execute admission and text read', async () => { + const { root, service } = fixture(); + const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) }); + // The gateway's first read admits the collaborator; its text read observes the changed current access and refuses. + afterIssueAccess = () => { issueCollaborator = false; }; + await expect(sources.issue(service.config.identity, new AbortController().signal)).rejects.toThrow(/access changed during admission/); + }); + it('refuses all-comments text when explicit trust is revoked during the awaited read', async () => { + const { root, service } = fixture(); + issueAuthor = 'outside'; issueCollaborator = false; + const issue = service.store.getPlan(service.config.identity).issue; + service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); + const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) }); + issueReads.length = 0; + beforeIssueTextReturn = () => { service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, + trusted: false, trustedBy: 'local user' }); }; + await expect(sources.issue(service.config.identity, new AbortController().signal)).rejects.toThrow(/not trusted/); + beforeIssueTextReturn = undefined; + service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); + await expect(sources.issue(service.config.identity, new AbortController().signal)).resolves.toMatchObject({ text: { comments: ['Outside note.'] } }); + expect(issueReads).toEqual([issue, issue]); + }); + it('returns a prompt-boundary guard that catches revocation after issue text resolves', async () => { + const { root, service } = fixture(); + issueAuthor = 'outside'; issueCollaborator = false; + const issue = service.store.getPlan(service.config.identity).issue; + service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: true, trustedBy: 'local user' }); + const { sources } = await setUpRunner({ service, capability, config: { root: join(root, 'runner'), committer }, lock: lock([]), env: { CLAUDE_CODE_OAUTH_TOKEN: 't' }, + buildImage: () => 'x', recovery: () => recovery([]) }); + const guarded = await sources.issue(service.config.identity, new AbortController().signal); + expect(guarded.text.comments).toEqual(['Outside note.']); + service.store.setIssueTrust({ repository: 'owner/repo', issue, authorLogin: issueAuthor, trusted: false, trustedBy: 'local user' }); + expect(() => guarded.validate()).toThrow(/not trusted/); + }); it('refuses before touching anything without a github block, a token, or with a demo', async () => { for (const [patch, env, message] of [[{ github: undefined }, { CLAUDE_CODE_OAUTH_TOKEN: 't' }, /github block/], [{}, {}, RUNNER_CREDENTIAL_MISSING], [{ demo: true }, { CLAUDE_CODE_OAUTH_TOKEN: 't' }, /Demos never/]] as const) { const { root, service, config } = fixture(), calls: string[] = []; @@ -152,7 +243,7 @@ describe('server with a runner setup', () => { const deps: RunnerDeps = { runnerOwner: OWNER, kinds: ['execute'], prepare: async () => { throw new Error('no agent here'); }, cleanupPreparation: async () => undefined, start: () => { throw new Error('no agent here'); }, validate: () => null }; const sources: ExecutionSources = { planContext: () => service.planContext(), checkpointContext: (_identity, head) => service.planContextAt(head), - issue: () => ({ number: 1, title: '', body: '', comments: [] }), lessons: () => [], vendor: () => 'claude' }; + issue: () => ({ text: { number: 1, title: '', body: '', comments: [] }, validate: () => undefined }), lessons: () => [], vendor: () => 'claude' }; return { deps, sources, findings: new SafetyFindings(service.store), recovery: { finalized: [], requeue: [], removedDirectories: [], unknownEntries: [], unmatchedStorage: [], repairedMerges: [] } }; }; it('closes the Store only after the plan runs in progress, and after a failing step', async () => { diff --git a/test/runner-publishing.test.ts b/test/runner-publishing.test.ts index 95b982df..d18ae8b2 100644 --- a/test/runner-publishing.test.ts +++ b/test/runner-publishing.test.ts @@ -22,6 +22,7 @@ import { PullRequestMisplaced, openingMarker, type OpenPullRequestInput, type Pu import type { AlreadyFixedGateway } from '../github/already-fixed.ts'; import type { PlanIdentity } from '../core/identity.ts'; import { approveItem } from '../core/approvals.ts'; +import { GhIssueGateway, type IssueTrustGateway } from '../github/issues.ts'; vi.setConfig({ testTimeout: 30_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -72,6 +73,7 @@ class FakeGitHub { pulls: PullRequestGateway = { repository: REPO, open: async (input: OpenPullRequestInput, signal?: AbortSignal) => { + const finalize = await input.beforeOpen?.(); finalize?.(); this.calls.push(`open ${input.draft ? 'draft' : 'ready'}`); if (this.head(input.headBranch) === null) throw new Error('No such branch on GitHub.'); const pr: Pr = { number: 100 + this.prs.length, url: `https://github.com/${REPO}/pull/${100 + this.prs.length}`, draft: input.draft, open: true, @@ -101,11 +103,12 @@ class FakeGitHub { pr.open = false; return { number, url: pr.url }; }, - markDraft: async number => { this.onDraft?.(); const pr = this.prs.find(candidate => candidate.number === number)!; pr.draft = true; return this.#view(pr); }, + markDraft: async (number, input) => { const finalize = await input.beforeDraft?.(); finalize?.(); this.onDraft?.(); const pr = this.prs.find(candidate => candidate.number === number)!; pr.draft = true; return this.#view(pr); }, refresh: async (number, input) => { + const finalizePatch = await input.beforePatch?.(); finalizePatch?.(); this.calls.push(`refresh ${input.draft ? 'draft' : 'ready'}`); const pr = this.prs.find(candidate => candidate.number === number)!; - input.beforeReady?.(); + const finalizeReady = await input.beforeReady?.(); finalizeReady?.(); pr.body = input.body; pr.draft = input.draft; return this.#view(pr); }, @@ -227,13 +230,14 @@ function world(): World { * of the three coordinator paths that can raise a finding. `before` shapes the Store before the runner exists, as an * earlier process would have left it. */ -async function serve(w: World, options: { before?: (service: ReviewService) => void; onPushSpawn?: (n: number, app: () => App, close: () => Promise) => void; demo?: boolean; startup?: boolean; settleMs?: number; env?: NodeJS.ProcessEnv; hold?: Promise; shortRetryMs?: number; findingSource?: FindingSource } = {}) { +async function serve(w: World, options: { before?: (service: ReviewService) => void; onPushSpawn?: (n: number, app: () => App, close: () => Promise) => void; demo?: boolean; startup?: boolean; settleMs?: number; env?: NodeJS.ProcessEnv; hold?: Promise; shortRetryMs?: number; findingSource?: FindingSource; + issueGateway?: IssueTrustGateway } = {}) { let app: App | undefined, spawns = 0, closing: Promise | undefined; const close = () => closing ??= app!.close(); let branchOf: (identity: PlanIdentity) => string = () => ''; let findings: SafetyFindings | undefined; - const config = { ...w.demo, demo: options.demo ?? false }; - app = await startServer(config, 0, undefined, undefined, 2_000, undefined, undefined, undefined, async service => { + const config = { ...w.demo, demo: options.demo ?? false, ...(options.issueGateway ? { github: { repository: options.issueGateway.repository, issue: 3, pullRequest: 1, baseBranch: 'main' } } : {}) }; + app = await startServer(config, 0, undefined, undefined, 2_000, options.issueGateway, undefined, undefined, async service => { const identity = service.config.identity, task = service.store.getTask(identity), plan = service.store.getPlan(identity); if (task.status !== 'merged' && task.status !== 'cancelled' && service.store.unapprovedExecutionItems(identity, plan.revision).length) { const review = service.load(); @@ -245,7 +249,7 @@ async function serve(w: World, options: { before?: (service: ReviewService) => v await ensureCommit(repository, service.store.getSnapshot(service.config.identity).head); const prepared: PreparedAttempt = { clone: { id: 'clone', taskId: 'task', directory: '/tmp/x', head: 'f'.repeat(40) }, vendor: 'claude', approvedArgv: [] }; const sources: ExecutionSources = { planContext: () => service.planContext(), checkpointContext: () => service.planContext(), - issue: () => ({ number: 3, title: '', body: '', comments: [] }), lessons: () => [], vendor: () => 'claude' }; + issue: () => ({ text: { number: 3, title: '', body: '', comments: [] }, validate: () => undefined }), lessons: () => [], vendor: () => 'claude' }; const pusher = new GitBranchPusher({ repository, repositoryId: service.config.identity.repositoryId, remote: REPO, url: w.remote, ownedCommits: identity => service.store.getLedger(identity).filter(entry => entry.origin === 'owned').map(entry => entry.sha), onProcessGroup: () => options.onPushSpawn?.(++spawns, () => app!, close) }); @@ -451,6 +455,158 @@ describe('publishing a finished task (#103)', () => { expect(w.github.calls).toEqual([]); }); + it('rechecks issue trust before a publish action crosses its external boundary', async () => { + const w = world(); + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { return { number, authorLogin: 'outside', collaborator: false }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + const { app, store, identity } = await serve(w, { before: completeAll, startup: false, issueGateway }); + expect(await act(app, 'publish')).toMatchObject({ status: 409, body: { error: expect.stringMatching(/not trusted for its current author/) } }); + expect(w.github.calls).toEqual([]); + store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + expect(await act(app, 'publish')).toMatchObject({ status: 200, body: { result: { outcome: 'publishing' } } }); + await app.publishing!.settled(identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'opened' }); + }); + + it('records an unavailable issue-access read as a failed publish, not a trust refusal', async () => { + const w = world(); + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess() { throw new Error('GitHub returned HTTP 502'); }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + const { app, store, identity } = await serve(w, { before: completeAll, startup: false, issueGateway }); + app.publishOwed(() => undefined); + await publishSettled(app, identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed', message: expect.stringMatching(/could not be verified.*502/) }); + expect(w.github.calls).toEqual([]); + }); + + it('rechecks issue trust before an automatic publish retry and stops after revocation', async () => { + const w = world(); + let accessReads = 0; + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { accessReads++; return { number, authorLogin: 'outside', collaborator: false }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + w.github.staleHeadOnce = 'a'.repeat(40); + const { app, store, identity } = await serve(w, { startup: false, shortRetryMs: 100, issueGateway, before: service => { + completeAll(service); + service.store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + } }); + app.publishOwed(() => undefined); + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject({ outcome: 'opened', reconcile: true }), { timeout: 10_000 }); + const calls = [...w.github.calls], readsBeforeRetry = accessReads; + store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + await vi.waitFor(() => expect(store.lastPublish(identity)).toMatchObject({ outcome: 'refused', message: expect.stringMatching(/not trusted/) }), + { timeout: 10_000, interval: 50 }); + expect(accessReads).toBe(readsBeforeRetry + 1); + expect(w.github.calls).toEqual(calls); + }); + + it('rechecks the bound trust decision after awaited safety reads and before pushing', async () => { + const w = world(), checking = Promise.withResolvers(), release = Promise.withResolvers(); + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { return { number, authorLogin: 'outside', collaborator: false }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + w.github.checks.check = async () => { w.github.calls.push('check'); checking.resolve(); await release.promise; + return { outcome: 'clear', baseHead: 'b'.repeat(40) }; }; + const { app, store, identity, branch } = await serve(w, { startup: false, issueGateway, before: service => { + completeAll(service); + service.store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + } }); + app.publishOwed(() => undefined); + await checking.promise; + store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + release.resolve(); + await publishSettled(app, identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'refused', message: expect.stringMatching(/not trusted/) }); + expect(w.github.head(branch)).toBeNull(); + expect(w.github.prs).toEqual([]); + }); + + it('re-reads current issue access after awaited safety reads and before pushing', async () => { + const w = world(), checking = Promise.withResolvers(), release = Promise.withResolvers(); + let access = { authorLogin: 'member', collaborator: true }, accessReads = 0; + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { accessReads++; return { number, ...access }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + w.github.checks.check = async () => { w.github.calls.push('check'); checking.resolve(); await release.promise; + return { outcome: 'clear', baseHead: 'b'.repeat(40) }; }; + const { app, store, identity, branch } = await serve(w, { startup: false, issueGateway, before: completeAll }); + app.publishOwed(() => undefined); + await checking.promise; + access = { authorLogin: 'outside', collaborator: false }; + release.resolve(); + await publishSettled(app, identity); + expect(accessReads).toBe(2); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'refused', message: expect.stringMatching(/not trusted.*current author/) }); + expect(w.github.head(branch)).toBeNull(); + expect(w.github.prs).toEqual([]); + }); + + it('does not publish when the issue author changes during the final collaborator read', async () => { + const w = world(), checking = Promise.withResolvers(), release = Promise.withResolvers(); + let author = 'outside', issueReads = 0, holdCollaborators = false; + const issueGateway = new GhIssueGateway(REPO, async args => { + if (args.some(argument => argument.endsWith('/collaborators'))) { + if (holdCollaborators) { holdCollaborators = false; checking.resolve(); await release.promise; } + return '[]'; + } + if (args[5] === `repos/${REPO}/issues`) return '[]'; + issueReads++; + if (issueReads === 3) holdCollaborators = true; + return JSON.stringify({ number: 3, user: { login: author } }); + }); + const { app, store, identity, branch } = await serve(w, { startup: false, issueGateway, before: service => { + completeAll(service); + service.store.setIssueTrust({ repository: REPO, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + } }); + app.publishOwed(() => undefined); + await checking.promise; + author = 'replacement'; + release.resolve(); + await publishSettled(app, identity); + expect(store.lastPublish(identity)).toMatchObject({ outcome: 'failed', message: expect.stringMatching(/author changed during access verification/) }); + expect(w.github.head(branch)).toBeNull(); + expect(w.github.prs).toEqual([]); + }); + + it('aborts and settles a publish whose issue authorization is still pending during shutdown', async () => { + const w = world(), reading = Promise.withResolvers(); + let accessSignal: AbortSignal | undefined; + const issueGateway: IssueTrustGateway = { + repository: REPO, + async fetch() { return { repository: REPO, retrievedAt: new Date().toISOString(), issues: [] }; }, + issueAccess(_number, options = {}) { + accessSignal = options.signal; reading.resolve(); + return new Promise((_resolve, reject) => options.signal?.addEventListener('abort', () => reject(options.signal!.reason), { once: true })); + }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + const { app, store, close } = await serve(w, { startup: false, issueGateway, before: completeAll }); + const records = vi.spyOn(store, 'recordPublish'); + app.publishOwed(() => undefined); + await reading.promise; + await close(); + expect(accessSignal?.aborted).toBe(true); + expect(records.mock.calls.some(call => call[1].outcome === 'stopped')).toBe(true); + expect(w.github.calls).toEqual([]); + }); + it('closes the Store only after a publish stopped during its push has settled, and the next start publishes once', async () => { const w = world(); let closedWhileBusy: boolean | undefined; diff --git a/test/runner-start.test.ts b/test/runner-start.test.ts index 08713c93..7796ddb0 100644 --- a/test/runner-start.test.ts +++ b/test/runner-start.test.ts @@ -12,6 +12,7 @@ import type { RunnerDeps } from '../runner/coordinator.ts'; import { SafetyFindings, type ExecutionSources } from '../runner/execution.ts'; import type { AttemptKind } from '../runner/lifecycle.ts'; import { approveItem } from '../core/approvals.ts'; +import type { IssueTrustGateway } from '../github/issues.ts'; vi.setConfig({ testTimeout: 20_000 }); const roots: string[] = [], cleanups: (() => Promise | void)[] = []; @@ -25,16 +26,18 @@ type App = Awaited>; * A server with a runner whose preparation always fails: an admitted item ends `failed` without an agent, which is * enough to see what start and resume admit. `before` shapes the Store before the runner exists, as recovery would. */ -async function serve(options: { kinds?: AttemptKind[]; approve?: boolean; before?: (service: ReviewService) => void; findings?: (findings: SafetyFindings, service: ReviewService) => void } = {}) { +async function serve(options: { kinds?: AttemptKind[]; approve?: boolean; before?: (service: ReviewService) => void; findings?: (findings: SafetyFindings, service: ReviewService) => void; + issueGateway?: IssueTrustGateway } = {}) { const root = mkdtempSync(join(tmpdir(), 'codeboost-start-')); roots.push(root); const demo = createDemo(join(root, 'demo')); - const app = await startServer({ ...demo }, 0, undefined, undefined, 2_000, undefined, undefined, undefined, async service => { + const config = options.issueGateway ? { ...demo, demo: false, github: { repository: options.issueGateway.repository, issue: 3, pullRequest: 1 } } : demo; + const app = await startServer(config, 0, undefined, undefined, 2_000, options.issueGateway, undefined, undefined, async service => { if (options.approve !== false) approvePlan(service); options.before?.(service); const deps: RunnerDeps = { runnerOwner: OWNER, kinds: options.kinds ?? ['execute'], prepare: async () => { throw new Error('no agent here'); }, cleanupPreparation: async () => undefined, start: () => { throw new Error('no agent here'); }, validate: () => null }; const sources: ExecutionSources = { planContext: () => service.planContext(), checkpointContext: () => service.planContext(), - issue: () => ({ number: 1, title: '', body: '', comments: [] }), lessons: () => [], vendor: () => 'claude' }; + issue: () => ({ text: { number: 1, title: '', body: '', comments: [] }, validate: () => undefined }), lessons: () => [], vendor: () => 'claude' }; const findings = new SafetyFindings(service.store); options.findings?.(findings, service); return { deps, sources, findings, recovery: { finalized: [], requeue: [], removedDirectories: [], unknownEntries: [], unmatchedStorage: [], repairedMerges: [] } }; @@ -58,6 +61,14 @@ function approvePlan(service: ReviewService) { service.store.saveReview(service.config.identity, view.expected, view.items.map(item => approveItem(view.plan, view.segments, item.id, service.config.identity, item.count === 0)), []); } +function trustGateway(read: () => { authorLogin: string | null; collaborator: boolean } | Error): IssueTrustGateway { + return { + repository: 'owner/repo', + async fetch() { return { repository: 'owner/repo', retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { const value = read(); if (value instanceof Error) throw value; return { number, ...value }; }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; +} /** Before the runner exists: queue the task and leave one execute attempt of its first item, failed. */ function failedFirstItem(service: ReviewService, budgetMs?: number) { @@ -94,6 +105,47 @@ function revise(service: ReviewService) { } describe('start (#91 part 2)', () => { + it('refuses an outside-authored issue until matching repository-scoped trust is recorded', async () => { + const gateway = trustGateway(() => ({ authorLogin: 'outside', collaborator: false })); + const { app, identity, store } = await serve({ issueGateway: gateway }); + expect((await act(app, 'start')).body.error).toMatch(/not trusted for its current author/i); + expect(store.getAttempts(identity)).toEqual([]); + store.setIssueTrust({ repository: 'owner/repo', issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + expect((await act(app, 'start')).body.result).toMatchObject({ outcome: 'started', item: 'P1' }); + }); + it('records and replays a failed or incomplete collaborator read as a definite upstream failure', async () => { + let result: ReturnType[0]> = new Error('incomplete collaborator page'); + const { app, identity, store } = await serve({ issueGateway: trustGateway(() => result) }); + const actionId = randomUUID(); + expect(await act(app, 'start', { actionId })).toMatchObject({ status: 502, + body: { error: expect.stringMatching(/could not be verified.*incomplete collaborator page/i) } }); + result = { authorLogin: 'member', collaborator: true }; + expect(await act(app, 'start', { actionId })).toMatchObject({ status: 502, + body: { error: expect.stringMatching(/could not be verified.*incomplete collaborator page/i) } }); + expect(store.getAttempts(identity)).toEqual([]); + }); + it('returns the first durable success when an identical concurrent access read fails later', async () => { + const secondStarted = Promise.withResolvers(), releaseFailure = Promise.withResolvers(); + let reads = 0; + const gateway: IssueTrustGateway = { + repository: 'owner/repo', + async fetch() { return { repository: 'owner/repo', retrievedAt: new Date().toISOString(), issues: [] }; }, + async issueAccess(number) { + if (++reads === 1) { await secondStarted.promise; return { number, authorLogin: 'member', collaborator: true }; } + secondStarted.resolve(); await releaseFailure.promise; throw new Error('later GitHub failure'); + }, + async issueText(number) { return { number, title: '', body: '', comments: [] }; }, + }; + const { app, identity, store } = await serve({ issueGateway: gateway }); + const actionId = randomUUID(), first = act(app, 'start', { actionId }); + await vi.waitFor(() => expect(reads).toBe(1)); + const second = act(app, 'start', { actionId }); + const accepted = await first; + expect(accepted).toMatchObject({ status: 200, body: { result: { outcome: 'started', item: 'P1' } } }); + releaseFailure.resolve(); + expect(await second).toMatchObject({ status: 200, body: { result: accepted.body.result } }); + expect(store.getAttempts(identity)).toHaveLength(1); + }); it('requires every item of the current plan revision to be approved before start', async () => { const { app, identity, store } = await serve({ approve: false }); expect(await view(app)).toMatchObject({ startable: false }); @@ -172,6 +224,12 @@ describe('status polling (#91 part 2)', () => { }); describe('resume (#91 part 2)', () => { + it('refuses an untrusted current author before resuming', async () => { + const { app, identity, store } = await serve({ issueGateway: trustGateway(() => ({ authorLogin: 'outside', collaborator: false })), + before: service => { failedFirstItem(service); } }); + expect((await act(app, 'resume')).body.error).toMatch(/not trusted for its current author/i); + expect(store.getAttempts(identity)).toHaveLength(1); + }); it('claims the requeue recovery left and continues from the first unfinished item', async () => { let interrupted = ''; const { app, identity, store, items } = await serve({ before: service => { @@ -279,7 +337,8 @@ describe('start and resume refusals and races (#91 part 2)', () => { expect(store.continuationProgress(identity)).toMatchObject({ completed: ['P1', 'P2'], next: 'P3' }); }); it('requires explicit continuation approval and resumes at the audited suffix through the API', async () => { - const { app, identity, store, items } = await serve({ before: service => { + const gateway = trustGateway(() => ({ authorLogin: 'outside', collaborator: false })); + const { app, identity, store, items } = await serve({ issueGateway: gateway, before: service => { committedFirstItem(service, false, ['other.ts']); const s = service.store, id = service.config.identity, snapshot = s.getSnapshot(id); const entries = [...service.planContext().baseEntries, { path: 'other.ts', kind: 'file' as const }]; @@ -294,9 +353,13 @@ describe('start and resume refusals and races (#91 part 2)', () => { next.items.map(item => approveItem(next, [], item.id, id, true)), []); const baseContext = service.planContext(); service.planContextAt = () => ({ ...baseContext, baseEntries: entries }); + s.setIssueTrust({ repository: gateway.repository, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); } }); expect(await view(app)).toMatchObject({ resumable: false, startable: false }); expect((await act(app, 'resume')).body.error).toMatch(/Approve the amended plan continuation/); + store.setIssueTrust({ repository: gateway.repository, issue: 3, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + expect((await act(app, 'approve-continuation')).body.error).toMatch(/not trusted/); + store.setIssueTrust({ repository: gateway.repository, issue: 3, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); const approved = await act(app, 'approve-continuation'); expect(approved).toMatchObject({ status: 200, body: { result: { outcome: 'approved', next: items[1] } } }); expect(store.getTask(identity).status).toBe('queued'); @@ -682,6 +745,47 @@ describe('start and resume refusals and races (#91 part 2)', () => { expect((await act(app, 'resume', { expectedStateVersion: stateVersion })).body.error).toMatch(/Stale task state/); expect(store.getAttempts(identity)).toHaveLength(2); }); + it('replays a saved action after shutdown stops new runner admission', async () => { + const { app, identity, store } = await serve({ issueGateway: trustGateway(() => ({ authorLogin: 'owner', collaborator: true })) }); + const { stateVersion, reviewVersion } = await view(app), actionId = randomUUID(); + const request = { expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion, actionId }; + const first = await act(app, 'start', request); + expect(first.status).toBe(200); + expect(store.savedAction(identity, { actionId, kind: 'start', request: { + attemptId: undefined, expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion, + } })).toBeDefined(); + + app.runner!.rejectAdmission(); + expect(await act(app, 'start', request)).toMatchObject({ status: 200, body: { result: first.body.result } }); + expect((await act(app, 'start', { expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion })).status).toBe(503); + }); + it('replays a saved action whose body finishes arriving after server shutdown begins', async () => { + const { app, close, database, identity, store } = await serve({ issueGateway: trustGateway(() => ({ authorLogin: 'owner', collaborator: true })) }); + const { stateVersion, reviewVersion } = await view(app), actionId = randomUUID(); + const first = await act(app, 'start', { expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion, actionId }); + const attemptCount = store.getAttempts(identity).length, url = new URL(app.url); + const text = JSON.stringify({ action: 'start', expectedStateVersion: stateVersion, expectedReviewVersion: reviewVersion, actionId }); + let finish!: () => void; + const replay = new Promise<{ status: number; body: Record }>((resolve, reject) => { + const req = httpRequest({ host: url.hostname, port: url.port, path: '/api/runner', method: 'POST', + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json', 'content-length': Buffer.byteLength(text) } }, res => { + const chunks: Buffer[] = []; + res.on('data', chunk => chunks.push(chunk)); + res.on('end', () => resolve({ status: res.statusCode!, body: JSON.parse(Buffer.concat(chunks).toString()) })); + }); + req.on('error', reject); + req.write(text.slice(0, 5)); + finish = () => req.end(text.slice(5)); + }); + await new Promise(resolve => setTimeout(resolve, 20)); + const closing = close(); + await new Promise(resolve => setTimeout(resolve, 20)); + finish(); + expect(await replay).toMatchObject({ status: 200, body: { result: first.body.result } }); + await closing; + const reopened = new Store(database); cleanups.push(() => reopened.close()); + expect(reopened.getAttempts(identity)).toHaveLength(attemptCount); + }); it('replays a pre-review-version action, but never creates a new action without the review version', async () => { const actionId = randomUUID(); let stateVersion = 0; const { app, identity, store } = await serve({ before: service => { diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index 1a898819..2eecd1cc 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -52,7 +52,7 @@ async function setup(agent: Record) { const review = new ReviewService({ database: join(root, 'state.sqlite'), repository: source, runnerRepository: repository.path, identity, pathIdentity: { caseSensitive: true, unicodeNormalization: 'none' } }); try { return review.planContextAt(commit); } finally { review.close(); } - }, issue: () => ({ number: 1, title: 'Issue', body: 'Fix', comments: [] }), + }, issue: () => ({ text: { number: 1, title: 'Issue', body: 'Fix', comments: [] }, validate: () => undefined }), lessons: () => [], vendor: () => 'claude' }; const findings = new SafetyFindings(store); // The agent: a container that edits the work volume, mounted as an agent container mounts it (metadata read-only). diff --git a/test/store.test.ts b/test/store.test.ts index 5ca267e4..67355f21 100644 --- a/test/store.test.ts +++ b/test/store.test.ts @@ -5,7 +5,7 @@ import { execFileSync, spawn } from 'node:child_process'; import { fixtureGit } from './fixtures/git.ts'; import { once } from 'node:events'; import { DatabaseSync } from 'node:sqlite'; -import { afterEach, expect, it } from 'vitest'; +import { afterEach, expect, it, vi } from 'vitest'; import { Store, requireSupportedNode } from '../runner/store.ts'; import type { Plan, PlanContext, EditReply } from '../core/plan.ts'; import { approveItem, approvalStates, choiceKeys, applyChoices, stable } from '../core/approvals.ts'; @@ -39,6 +39,38 @@ it('allocates revisions in SQLite, survives reopen, and keeps old revisions and expect(recovered.getPlan(identity)).toEqual(next); expect(recovered.getSnapshot(identity, first.id)).toEqual(first); expect(recovered.getSnapshot(identity).head).toBe(oid(4)); }); +it('scopes issue trust to repository and current author, supports revoke, and migrates schema v16', () => { + const { store, path } = fixture(); + expect(store.issueTrust('owner/a', 5)).toBeNull(); + const trusted = store.setIssueTrust({ repository: 'Owner/A', issue: 5, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + expect(trusted).toMatchObject({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trustedBy: 'local user', revokedAt: null }); + expect(store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: true, trustedBy: 'local user' })).toEqual(trusted); + expect(store.issueTrust('owner/b', 5)).toBeNull(); + const revoked = store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + expect(revoked.revokedAt).toBeTypeOf('string'); + expect(store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: false, trustedBy: 'local user' })).toEqual(revoked); + expect(() => store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'changed', trusted: false, trustedBy: 'local user' })).toThrow(/not trusted for its current author/); + close(store); + const legacy = new DatabaseSync(path); + legacy.exec('DROP TABLE issue_trust; ALTER TABLE attempts DROP COLUMN prompt_comments; PRAGMA user_version=16;'); legacy.close(); + const migrated = open(path), db = new DatabaseSync(path); + try { + expect(db.prepare('PRAGMA user_version').get()).toEqual({ user_version: 17 }); + expect(db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='issue_trust'").get()).toEqual({ name: 'issue_trust' }); + expect(db.prepare('PRAGMA table_info(attempts)').all().some(column => column.name === 'prompt_comments')).toBe(true); + expect(migrated.issueTrust('owner/a', 5)).toBeNull(); + } finally { db.close(); } +}); +it('strictly orders same-author trust decisions when the wall clock does not advance', () => { + const { store } = fixture(), clock = vi.spyOn(Date, 'now').mockReturnValue(1_000); + try { + const trusted = store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + const revoked = store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: false, trustedBy: 'local user' }); + const retrusted = store.setIssueTrust({ repository: 'owner/a', issue: 5, authorLogin: 'outside', trusted: true, trustedBy: 'local user' }); + expect(Date.parse(revoked.revokedAt!)).toBeGreaterThan(Date.parse(trusted.trustedAt)); + expect(Date.parse(retrusted.trustedAt)).toBeGreaterThan(Date.parse(revoked.revokedAt!)); + } finally { clock.mockRestore(); } +}); it('binds suggestion requests before the reply and rejects cross-plan, cancelled, delayed, replayed, and sibling applications', () => { const { store } = fixture(); const id = ready(store), sibling = ready(store); const other = { ...identity, planId: 'other' }; store.createPlan(JSON.stringify(plan()), 'json', { ...context, identity: other }, oid(1), oid(2)); diff --git a/web/cli.ts b/web/cli.ts index 2203dd3a..237382bc 100644 --- a/web/cli.ts +++ b/web/cli.ts @@ -83,7 +83,7 @@ if (values.help || (!values.demo && !values.config && values['release-preparatio for (const signal of ['SIGINT', 'SIGTERM'] as const) process.removeListener(signal, duringStartup); // A publish an earlier process owed (#103) starts only under a lock verified to name the database: publishOwed runs the // check itself, first, so no order of these lines can start one before it. - try { if (stopping) lock.verify(); else app.publishOwed(() => lock.verify()); } + try { if (stopping) lock.verify(); else await app.publishOwed(() => lock.verify()); } catch (error) { await app.close(); lock.release(); // The database path changed: a refusal the person acts on, so its message, not a stack. diff --git a/web/issues.ts b/web/issues.ts index a26c3f75..590e8078 100644 --- a/web/issues.ts +++ b/web/issues.ts @@ -1,8 +1,11 @@ import { IssuePrioritizer, type IssuePriorityState, type RankedIssue } from '../core/issue-ranking.ts'; import type { IssueGateway } from '../github/issues.ts'; +import type { IssueTrustRecord } from '../runner/store.ts'; /** Issue bodies stay on the server: the screen never shows them, and each may be up to 64 KiB. */ -export type IssueSummary = Omit; +export type IssueSummary = Omit & { + trust: RankedIssue['trust'] | 'approved'; trustedAt?: string; trustedBy?: string; trustChangedAt?: string; +}; type Summarized = State extends unknown ? Omit & { issues: IssueSummary[] } : never; export type IssueBoardState = Summarized; @@ -26,19 +29,41 @@ export const REFRESH_TIMEOUT_MS = 12_000; export class IssueBoard { readonly #prioritizer: IssuePrioritizer | null; readonly #reason: string; + readonly #trust: (repository: string, issue: number) => IssueTrustRecord | null; #state: IssueBoardState | null = null; #flight: Promise | null = null; #controller: AbortController | null = null; #closing = false; - constructor(gateway: IssueGateway | null, unavailableReason = 'Issue ranking is not configured.', now?: () => Date) { + constructor(gateway: IssueGateway | null, unavailableReason = 'Issue ranking is not configured.', now?: () => Date, + trust: (repository: string, issue: number) => IssueTrustRecord | null = () => null) { this.#prioritizer = gateway ? new IssuePrioritizer(gateway, now) : null; this.#reason = unavailableReason; + this.#trust = trust; } view(): IssueBoardView { if (!this.#prioritizer) return { configured: false, reason: this.#reason }; - return { configured: true, repository: this.#prioritizer.gateway.repository, refreshing: this.#flight !== null, state: this.#state }; + const state = this.#state && { ...this.#state, issues: this.#state.issues.map(issue => { + const decision = this.#trust(issue.repository, issue.number); + if (decision?.revokedAt === null && decision.authorLogin === issue.authorLogin) + return { ...issue, trust: 'approved' as const, trustedAt: decision.trustedAt, trustedBy: decision.trustedBy, + trustChangedAt: decision.trustedAt }; + if (decision?.revokedAt !== null && decision?.authorLogin === issue.authorLogin) + return { ...issue, trustChangedAt: decision.revokedAt }; + return issue; + }) } as IssueBoardState; + return { configured: true, repository: this.#prioritizer.gateway.repository, refreshing: this.#flight !== null, state }; + } + + /** Latest complete board knowledge for one issue. Unknown is deliberately not treated as trusted. */ + trustStatus(number: number): 'allowed' | 'blocked' | 'unknown' { + if (!this.#prioritizer || !this.#state || this.#state.state !== 'fresh') return 'unknown'; + const issue = this.#state.issues.find(candidate => candidate.number === number); + if (!issue) return 'unknown'; + if (issue.trust === 'trusted') return 'allowed'; + const decision = this.#trust(issue.repository, issue.number); + return decision?.revokedAt === null && decision.authorLogin === issue.authorLogin ? 'allowed' : 'blocked'; } async refresh(signal?: AbortSignal): Promise { diff --git a/web/planning.ts b/web/planning.ts index a862093d..3385eb3f 100644 --- a/web/planning.ts +++ b/web/planning.ts @@ -1,20 +1,21 @@ -import { GhIssueGateway, type IssueText } from '../github/issues.ts'; +import { GhIssueGateway, ISSUE_READ_TIMEOUT_MS, withIssueReadDeadline, type IssueTrustGateway } from '../github/issues.ts'; +import { GuardRefusal } from '../runner/lifecycle.ts'; import { PlanningAgent } from '../runner/planning.ts'; import type { ReviewConfig, ReviewService } from '../runner/review.ts'; import type { PlanningSetup } from './server.ts'; -/** Bounds the GitHub read of the issue before a suggestion request starts. */ -export const ISSUE_READ_TIMEOUT_MS = 30_000; +export { ISSUE_READ_TIMEOUT_MS } from '../github/issues.ts'; /** * Production planning (#117): on for a review with a github block, never in a demo. It needs no runner block, since - * planning only reads code, as Ask does. The issue text is read from GitHub for each request (collaborators' comments - * only, bounded like an execute prompt's); approved lessons stay empty until lessons exist (L1 to L4). + * planning only reads code, as Ask does. The issue text is read from GitHub for each request (current collaborators' + * comments, or every comment under explicit author-bound trust), bounded like an execute prompt; approved lessons stay + * empty until lessons exist (L1 to L4). */ export function productionPlanning(config: ReviewConfig, options: { /** The single-runner lock's check that the database path still names the locked file (runner/recovery.ts). */ verifyLock: () => void; - issues?: { issueText(number: number, options: { signal?: AbortSignal; timeoutMs?: number }): Promise }; + issues?: Pick; agent?: (service: ReviewService) => PlanningAgent; }): PlanningSetup | undefined { const github = config.github; @@ -29,13 +30,25 @@ export function productionPlanning(config: ReviewConfig, options: { const provider = options.agent?.(service) ?? new PlanningAgent(service); return { provider, - async describe(signal) { - const text = await issues.issueText(github.issue, { signal, timeoutMs: ISSUE_READ_TIMEOUT_MS }); + describe(signal) { return withIssueReadDeadline(signal, async (readSignal, timeoutMs) => { + const access = await issues.issueAccess(github.issue, { signal: readSignal, timeoutMs }); + const trust = service.store.issueTrust(github.repository, github.issue); + const explicitlyTrusted = trust?.revokedAt === null && trust.authorLogin === access.authorLogin; + if (!access.collaborator && !explicitlyTrusted) throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); + const validate = () => { + if (!explicitlyTrusted) return; + const current = service.store.issueTrust(github.repository, github.issue); + if (!current || current.revokedAt !== null || current.authorLogin !== access.authorLogin) + throw new GuardRefusal(`Issue #${github.issue} is not trusted for its current author.`); + }; + const text = await issues.issueText(github.issue, { signal: readSignal, timeoutMs, + trustedAuthor: explicitlyTrusted ? access.authorLogin : undefined, expectedAccess: access }); + validate(); // The configured base branch (#103), or the base commit when none (or an empty one) is configured. const baseRef = github.baseBranch || service.store.getSnapshot(config.identity).base; return { issue: { number: text.number, title: text.title, body: text.body, comments: [...text.comments] }, - approvedLessons: [], repo: { name: github.repository, baseRef } }; - }, + approvedLessons: [], repo: { name: github.repository, baseRef }, validate }; + }); }, close: () => provider.close(), }; }; diff --git a/web/public/app.js b/web/public/app.js index aeed68e6..0c499655 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -807,7 +807,9 @@ new ResizeObserver(() => { let issuesGeneration = 0, issuesView = null, issuesRequested = false, - issuesLoading = false; + issuesLoading = false, + issuesRefreshError = null, + issueErrorOrder = 0; function showView(next) { view = next; $("review-view").hidden = next !== "review"; @@ -833,14 +835,19 @@ function issueStatus() { return ["bad", `✕ Unavailable · ${esc(state.error)}`]; } function trustMark(issue) { - return issue.trust === "trusted" - ? '✓ Collaborator' - : '! Needs trust'; + if (issue.trust === "trusted") return `✓ Collaborator + `; + if (issue.trust === "approved") return `✓ Trusted by you on ${esc(new Date(issue.trustedAt).toLocaleDateString())} + `; + return `! Needs trust + `; } function renderIssues() { const [tone, text] = issueStatus(); - $("issues-status").className = tone; - $("issues-status").innerHTML = text; + const errors = [...trustErrors.values(), ...(issuesRefreshError ? [issuesRefreshError] : [])].sort((a, b) => a.order - b.order); + $("issues-status").className = errors.length ? "bad" : tone; + if (errors.length) $("issues-status").textContent = errors.map(error => error.message).join(" "); + else $("issues-status").innerHTML = text; $("issues-repository").textContent = issuesView?.configured ? issuesView.repository : ""; const state = issuesView?.configured ? issuesView.state : null; if (!state) { @@ -861,27 +868,117 @@ function renderIssues() { ) .join("")}`; } +const trustGenerations = new Map(), trustPending = new Map(), trustRetries = new Map(), committedTrustRows = new Map(), trustErrors = new Map(); +let trustCommitGeneration = 0; +function renderIssuesWithPending(focusIssue) { + const active = document.activeElement; + const activeRow = active?.closest?.("tr[data-issue]"); + const activeSelector = active?.matches?.("button.issue-trust") ? "button.issue-trust" + : active?.matches?.(".issue-title a") ? ".issue-title a" : null; + const focused = Number.isSafeInteger(focusIssue) ? { number: focusIssue, selector: "button.issue-trust" } + : activeSelector && activeRow ? { number: Number(activeRow.dataset.issue), selector: activeSelector } : null; + renderIssues(); + for (const [number, pending] of trustPending) { + const control = document.querySelector(`.issue-trust[data-issue="${number}"]`); + if (!control) continue; + control.setAttribute("aria-disabled", "true"); + control.textContent = pending.action === "trust" ? "Trusting…" : "Removing…"; + } + if (focused && Number.isSafeInteger(focused.number)) + document.querySelector(`tr[data-issue="${focused.number}"] ${focused.selector}`)?.focus(); +} +function withIssueTrust(issue, trust) { + const { trust: _trust, trustedAt: _trustedAt, trustedBy: _trustedBy, trustChangedAt: _trustChangedAt, ...metadata } = issue; + return { ...metadata, trust: trust.trust, + ...(trust.trustedAt === undefined ? {} : { trustedAt: trust.trustedAt }), + ...(trust.trustedBy === undefined ? {} : { trustedBy: trust.trustedBy }), + ...(trust.trustChangedAt === undefined ? {} : { trustChangedAt: trust.trustChangedAt }) }; +} +function trustCanReplace(replacement, current) { + return replacement.authorLogin === current.authorLogin + && (current.trustChangedAt === undefined + || (replacement.trustChangedAt !== undefined && replacement.trustChangedAt > current.trustChangedAt)); +} +function mergeIssueTrustView(updated, number) { + const currentState = issuesView?.configured ? issuesView.state : null; + const updatedState = updated?.configured ? updated.state : null; + const replacement = updatedState?.issues.find((issue) => issue.number === number); + const current = currentState?.issues.find((issue) => issue.number === number); + // A refresh may have completed while this trust request was in flight. Trust responses own only trust fields, and + // only for the same author; they must never restore an older title, rank, author, or a row the refresh removed. + if (!issuesView?.configured || !currentState || !updated?.configured || !updatedState || !replacement || !current + || !trustCanReplace(replacement, current)) return null; + const merged = currentState.issues.map((issue) => issue.number === number ? withIssueTrust(issue, replacement) : issue); + issuesView = { ...issuesView, state: { ...currentState, issues: merged } }; + return merged.find((issue) => issue.number === number) ?? null; +} +function mergeTrustCommittedDuring(updated, generation) { + if (!updated?.configured || !updated.state) return updated; + return { ...updated, state: { ...updated.state, issues: updated.state.issues.map((issue) => { + const committed = committedTrustRows.get(issue.number); + return committed && committed.generation > generation && trustCanReplace(committed, issue) + ? withIssueTrust(issue, committed) : issue; + }) } }; +} +async function changeIssueTrust(button) { + if (button.getAttribute("aria-disabled") === "true") return; + const number = Number(button.dataset.issue), action = button.dataset.action; + const issue = issuesView?.configured && issuesView.state?.issues.find((entry) => entry.number === number); + if (!issue || !["trust", "untrust"].includes(action)) return; + const retained = trustRetries.get(number); + const request = retained?.action === action && retained.authorLogin === issue.authorLogin ? retained + : { action, actionId: crypto.randomUUID(), number, authorLogin: issue.authorLogin }; + trustRetries.set(number, request); + const generation = (trustGenerations.get(number) ?? 0) + 1; + trustGenerations.set(number, generation); + trustErrors.delete(number); + trustPending.set(number, { action, generation }); + renderIssuesWithPending(number); + try { + const updated = await api("/api/issues", request); + if (trustRetries.get(number) === request) trustRetries.delete(number); + if (trustGenerations.get(number) !== generation) return; + trustPending.delete(number); + trustErrors.delete(number); + const committed = mergeIssueTrustView(updated, number); + if (committed) committedTrustRows.set(number, { generation: ++trustCommitGeneration, authorLogin: committed.authorLogin, + trust: committed.trust, trustedAt: committed.trustedAt, trustedBy: committed.trustedBy, trustChangedAt: committed.trustChangedAt }); + renderIssuesWithPending(); + } catch (error) { + // A transport failure or 503 proves nothing was returned. Keep the exact request and idempotency key for retry. + const ambiguous = !Number.isSafeInteger(error?.status) || error.status === 503 || error.outcomeUnknown === true; + if (!ambiguous && trustRetries.get(number) === request) trustRetries.delete(number); + if (trustGenerations.get(number) !== generation) return; + trustPending.delete(number); + trustErrors.set(number, { generation, order: ++issueErrorOrder, + message: `✕ Could not ${action === "trust" ? "trust" : "remove trust from"} issue #${number}. ${error.message}` }); + renderIssuesWithPending(); + } +} async function loadIssues() { if (issuesLoading) return; const generation = ++issuesGeneration; + const trustGeneration = trustCommitGeneration; + const trustErrorOrder = issueErrorOrder; issuesRequested = true; issuesLoading = true; + issuesRefreshError = null; // aria-disabled, not disabled: disabling the focused button would drop keyboard focus to the page. $("issues-refresh").setAttribute("aria-disabled", "true"); $("issues-refresh").textContent = "Refreshing…"; if (issuesView?.configured) issuesView = { ...issuesView, refreshing: true }; - renderIssues(); + renderIssuesWithPending(); try { const updated = await api("/api/issues", { action: "refresh" }); if (generation !== issuesGeneration) return; - issuesView = updated; - renderIssues(); + issuesView = mergeTrustCommittedDuring(updated, trustGeneration); + for (const [number, error] of trustErrors) if (error.order <= trustErrorOrder) trustErrors.delete(number); + renderIssuesWithPending(); } catch (error) { if (generation !== issuesGeneration) return; if (issuesView?.configured) issuesView = { ...issuesView, refreshing: false }; - renderIssues(); - $("issues-status").className = "bad"; - $("issues-status").textContent = `✕ Could not refresh issues. ${error.message}`; + issuesRefreshError = { generation, order: ++issueErrorOrder, message: `✕ Could not refresh issues. ${error.message}` }; + renderIssuesWithPending(); } finally { if (generation === issuesGeneration) { issuesLoading = false; @@ -895,6 +992,10 @@ $("issues-link").onclick = (event) => { showView("issues"); }; $("issues-refresh").onclick = () => loadIssues(); +$("issues-list").onclick = (event) => { + const button = event.target.closest("button.issue-trust"); + if (button) changeIssueTrust(button); +}; showView(new URLSearchParams(location.search).get("view") === "issues" ? "issues" : "review"); await refresh(); diff --git a/web/public/index.html b/web/public/index.html index 95a1214b..2e883cff 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -57,7 +57,6 @@

A little more room to review

Ranked by priority label, security and bug labels, reactions, comments and age. Issues from people who are not repository collaborators need your trust before they can be queued. - Trusting issues and queueing are not available yet.

diff --git a/web/public/style.css b/web/public/style.css index 700add0f..dc2363cf 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -775,3 +775,8 @@ body.resizing-conversation { cursor: col-resize; user-select: none; } font-size: 12px; color: var(--muted); } +.issue-trust { + display: block; + margin-top: 8px; + white-space: nowrap; +} diff --git a/web/server.ts b/web/server.ts index 49618f86..f844d054 100644 --- a/web/server.ts +++ b/web/server.ts @@ -2,6 +2,7 @@ import { createServer, type IncomingMessage } from 'node:http'; import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { randomBytes, timingSafeEqual } from 'node:crypto'; +import type { PlanIdentity } from '../core/identity.ts'; import { ReviewService, type ReviewConfig } from '../runner/review.ts'; import { Questions, type QuestionAgent } from '../runner/questions.ts'; import { GhMergeGateway, type MergeGateway } from '../github/merge.ts'; @@ -12,14 +13,18 @@ import { OWED_REFUSAL, TaskPublishing } from '../runner/publishing.ts'; import type { RunnerAssembly } from '../runner/production.ts'; import { baseBranch } from '../github/validate.ts'; import type { ShutdownCapability } from '../runner/lifecycle.ts'; -import { ActionIdReused, BadRequest, GuardRefusal, ShuttingDownError, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; -import { GhIssueGateway, type IssueGateway } from '../github/issues.ts'; +import { ActionIdReused, BadRequest, GuardRefusal, ShuttingDownError, UpstreamFailure, assertUuidV4, isUuidV4, sameContext } from '../runner/lifecycle.ts'; +import { GhIssueGateway, type IssueGateway, type IssueAccess, type IssueTrustGateway } from '../github/issues.ts'; import { demoIssueGateway } from '../scripts/demo-issues.ts'; import { IssueBoard } from './issues.ts'; import { SuggestionCoordinator, type PlanningMode, type SuggestionHandle, type SuggestionInput, type SuggestionStore } from '../core/planning-suggestions.ts'; import type { AuthorProvider } from '../core/planning-author.ts'; import { PLANNING_BUDGET_MS } from '../runner/planning-provider.ts'; -export type PlanningDescription = Pick & { repo: { name: string; baseRef: string } }; +export type PlanningDescription = Pick & { + repo: { name: string; baseRef: string }; + /** Revalidates any mutable authority carried by this description at the synchronous prompt-construction boundary. */ + validate(): void; +}; /** Live planning runs only through D (G4 after #51, #117). Until a provider is injected, starting a suggestion is refused. */ export interface PlanningDeps { provider: AuthorProvider; @@ -33,8 +38,6 @@ export interface PlanningDeps { } /** Start, cancel or apply a suggestion or a draft (#124): `/api/plan/` or `/api/plan///`. */ const PLANNING_REQUEST = /^\/api\/plan\/(suggestions|drafts)(?:\/([0-9a-f-]{36})\/(cancel|apply))?$/; -/** A dependency the server reads from (GitHub) failed: 502, not recorded. */ -class UpstreamFailure extends Error {} /** How long a planning request may take to settle after shutdown aborts it, before its worker is abandoned (Ask's grace). */ export const PLANNING_SHUTDOWN_GRACE_MS = 20_000; /** Production planning is built after the Store opens, from the review it serves (see web/cli.ts). */ @@ -58,19 +61,23 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (!Number.isSafeInteger(shutdownDrainMs) || shutdownDrainMs < 1 || shutdownDrainMs > MAX_SHUTDOWN_DRAIN_MS) throw new Error('Invalid shutdown drain deadline.'); const service = new ReviewService(config), token = randomBytes(32).toString('hex'); let questions: Questions, merges: MergeCoordinator | null, issues: IssueBoard, runner: RunnerCoordinator | null, suggestions: SuggestionCoordinator | null; - let planning: PlanningDeps | undefined; + let planning: PlanningDeps | undefined, issueSource: IssueGateway | null; /** Runs a task's plan items; one per Store, like the coordinator. Only the production runner has one. */ let executor: ItemExecutor | null = null; /** Publishes the task's pull request (#103); only a production runner whose setup built a publisher has one. */ let publishing: TaskPublishing | null = null; + /** Installed after the issue gateway and trust helpers exist; every publish attempt, including retries, calls it. */ + let authorizePublish: ((identity: PlanIdentity, signal: AbortSignal) => Promise<() => Promise>) | undefined; // Only coordinators' settlement and close code receive this; HTTP handlers never do. const capability = service.store.shutdownCapability(); try { if (!config.demo && config.github && config.github.issue !== service.store.getPlan(config.identity).issue) throw new Error('The GitHub merge issue must match the stored plan issue.'); questions=new Questions(service,questionAgent,capability); // Issue retrieval is read-only, so demos may show it; they use a local fixture and never contact GitHub. - issues = new IssueBoard(issueGateway ?? (config.demo ? demoIssueGateway() : config.github ? new GhIssueGateway(config.github.repository) : null), - 'Issue ranking needs a GitHub repository. Add a github block with a repository to the review configuration.'); + issueSource = issueGateway ?? (config.demo ? demoIssueGateway() : config.github ? new GhIssueGateway(config.github.repository) : null); + issues = new IssueBoard(issueSource, + 'Issue ranking needs a GitHub repository. Add a github block with a repository to the review configuration.', undefined, + (repository, issue) => service.store.issueTrust(repository, issue)); // With a runner block the merge targets the task's published PR (#121); without one, github.pullRequest is required. const published = !config.demo && config.runner !== undefined && config.github ? { repository: config.github.repository, baseBranch: baseBranch(config.github), ...(config.github.pullRequest !== undefined ? { configured: config.github.pullRequest } : {}) } : undefined; @@ -100,7 +107,13 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge runner = new RunnerCoordinator(service.store, assembly.deps, undefined, capability); executor = new ItemExecutor(service.store, runner, assembly.sources, assembly.findings, { capability }); // A demo never publishes, whatever its github block or an injected setup provides (setUpRunner refuses demos too). - if (assembly.publisher && !config.demo) { const coordinator = runner; publishing = new TaskPublishing(service.store, assembly.publisher(() => coordinator.closing), runner, executor, capability, assembly.env, { ...(assembly.shortRetryMs !== undefined ? { shortRetryMs: assembly.shortRetryMs } : {}) }); } + if (assembly.publisher && !config.demo) { const coordinator = runner; publishing = new TaskPublishing(service.store, assembly.publisher(() => coordinator.closing), runner, executor, capability, assembly.env, { + ...(assembly.shortRetryMs !== undefined ? { shortRetryMs: assembly.shortRetryMs } : {}), + ...(config.github ? { authorizePublish: (publishIdentity: PlanIdentity, signal: AbortSignal) => { + if (!authorizePublish) throw new GuardRefusal('Issue trust admission is not configured.'); + return authorizePublish(publishIdentity, signal); + } } : {}), + }); } } catch (error) { service.close(); throw error; } } const loadReview=()=>{const view=service.load();return {...view,notes:view.notes.map(note=>({...note,answerActive:questions.isRunning(note.id)}))};}; @@ -109,9 +122,32 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge .filter(note=>note.kind==='question') .map(note=>({id:note.id,answer:note.answer,answerActive:questions.isRunning(note.id)})); const identity = config.identity; + const trustGateway = issueSource && 'issueAccess' in issueSource ? issueSource as IssueTrustGateway : null; + const requireTrustedIssue = (access: IssueAccess): void => { + if (access.collaborator) return; + const repository = trustGateway!.repository; + const trust = service.store.issueTrust(repository, access.number); + if (!trust || trust.revokedAt !== null || trust.authorLogin !== access.authorLogin) + throw new GuardRefusal(`Issue #${access.number} is not trusted for its current author.`); + }; + const readIssueAccess = async (signal: AbortSignal): Promise => { + if (!trustGateway || !config.github) throw new GuardRefusal('Issue trust admission is not configured.'); + try { return await trustGateway.issueAccess(config.github.issue, { signal, timeoutMs: 12_000 }); } + catch (error) { + if (signal.aborted) throw signal.reason; + throw new UpstreamFailure(`Issue trust could not be verified: ${error instanceof Error ? error.message : String(error)}`); + } + }; + authorizePublish = async (_publishIdentity, signal) => { + const access = await readIssueAccess(signal); + requireTrustedIssue(access); + return async () => requireTrustedIssue(await readIssueAccess(signal)); + }; /** - * What `start` or `resume` would run (#91 part 2), or the refusal. It writes nothing, so the view asks it too and never - * offers what the action would refuse. `start` runs a task that is in review or queued and has attempted no item of its + * What `start` or `resume` would run (#91 part 2), or the local refusal. It writes nothing, so the view asks it too. + * The view also suppresses controls when the latest complete issue board says trust is blocked; every action still + * performs a fresh external admission check because collaborator status and authorship can change afterward. `start` + * runs a task that is in review or queued and has attempted no item of its * current plan revision; `resume` continues a running or queued task that attempted an item at any revision (or that * recovery left to requeue), from the first item the current revision has not completed, whether its last item * completed, failed or was stopped. A queued task with only earlier-revision attempts may take either; both run the @@ -219,25 +255,29 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge reason: error instanceof Error ? error.message : String(error) }; } } - return { available: !!runner, task, attempts, startable: !!progress && offered('start', progress), resumable: !!progress && offered('resume', progress), + const trustBlocked = !!config.github && issues.trustStatus(config.github.issue) === 'blocked'; + return { available: !!runner, task, attempts, startable: !trustBlocked && !!progress && offered('start', progress), resumable: !trustBlocked && !!progress && offered('resume', progress), stateVersion: task.stateVersion, reviewVersion: service.store.reviewVersion(identity), retryable, stopRequested: status.stopRequested, - unresolved: status.unresolved, continuation, publish: publishView(progress) }; + unresolved: status.unresolved, continuation, publish: publishView(progress, trustBlocked) }; }; /** The task's publishing (#103): in progress, offered (what the publish action would run), and the last outcome. */ - const publishView = (progress?: ReturnType) => { + const publishView = (progress: ReturnType | undefined, trustBlocked: boolean) => { if (!publishing) return { available: false, active: false, publishable: false, closable: false, draft: false, last: null }; let job: ReturnType | null = null; try { job = publishing.mode(identity, progress); } catch (error) { if (!(error instanceof GuardRefusal) && !(error instanceof ShuttingDownError)) throw error; } // `closable`: the task is cancelled and a close of its PRs is owed (#111): not after one that closed them. const last = publishing.lastOutcome(identity); - return { available: true, active: publishing.busy(identity), publishable: job?.kind === 'publish', closable: job?.kind === 'close' && last?.outcome !== 'closed', + return { available: true, active: publishing.busy(identity), publishable: !trustBlocked && job?.kind === 'publish', closable: job?.kind === 'close' && last?.outcome !== 'closed', draft: job?.kind === 'publish' && job.draft, last }; }; /** A run's outcome is in the task and attempt rows; once it ends, the task's status decides whether a publish is owed. */ const afterRun = (outcome: Promise) => void outcome .catch(error => console.error(`Runner run failed: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`)) - .finally(() => publishing?.actIfOwed(identity)); - const runnerAction = (input: Record) => { + .finally(() => { + if (!publishing || stopping) return; + publishing.actIfOwed(identity); + }); + const runnerAction = async (input: Record, signal: AbortSignal) => { const { action, attemptId, expectedStateVersion, expectedReviewVersion, actionId } = input; // Malformed requests are refused before userAction, so nothing is recorded under their action ID (HTTP 400). if (!['cancel-attempt', 'retry', 'cancel-task', 'start', 'resume', 'approve-continuation', 'publish', 'close-pull-requests'].includes(action as string)) throw new BadRequest('Unsupported runner action.'); @@ -252,6 +292,20 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge } if (action === 'cancel-attempt' || action === 'retry') assertUuidV4(attemptId, 'Attempt ID'); const request = { attemptId, expectedStateVersion, ...((action === 'start' || action === 'resume' || action === 'approve-continuation') ? { expectedReviewVersion } : {}) }; + let access: IssueAccess | undefined; + const trustGated = !!config.github && + (((action === 'start' || action === 'resume') && !!executor) || + (action === 'approve-continuation' && !!executor) || (action === 'publish' && !!publishing)); + if (trustGated) { + const replay = service.store.savedAction(identity, { actionId: actionId as string, kind: action as string, request }); + if (replay) return replay.response; + if (stopping || runner?.closing) throw new ShuttingDownError(); + try { access = await readIssueAccess(signal); } + catch (error) { + if (stopping || runner?.closing || signal.aborted) throw new ShuttingDownError(); + return service.store.userAction(identity, { actionId: actionId as string, kind: action as string, request }, () => { throw error; }).response; + } + } // A refused start or resume can still move the task to needs human (an expired budget is committed with the refusal), // and a task that moves there is owed a draft PR (#103). Only that move publishes: a person who pressed resume on a // task already in needs human asked for no publish. @@ -310,6 +364,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (action === 'cancel-task') return { outcome: runner ? runner.cancelTask(identity, expectedStateVersion as number, actionId as string) : service.store.cancelTask(identity, expectedStateVersion as number, actionId as string) }; if (!runner) throw new GuardRefusal(config.demo ? RUNNER_NOT_IN_DEMO : RUNNER_NOT_CONFIGURED); if (action === 'approve-continuation') { + if (access) requireTrustedIssue(access); if (!executor || runner.isActive(identity) || executor.busy(identity) || publishing?.busy(identity)) throw new GuardRefusal('The runner is busy or closing; continuation cannot be approved yet.'); const progress = service.store.continuationProgress(identity); @@ -327,6 +382,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge return { outcome: 'approved', checkpointId: progress.checkpoint.id, next: progress.next }; } if (action === 'start' || action === 'resume') { + if (access) requireTrustedIssue(access); const choice = runChoice(action); // In this transaction with the admission: a refused admission rolls the move to queued back with it. if (choice.queue) service.store.transitionTask(identity, expectedStateVersion as number, 'queued'); @@ -339,6 +395,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge return begun.attemptId ? { outcome: 'started', attemptId: begun.attemptId, item: choice.fromItem } : { outcome: 'settled' }; } if (action === 'publish') { + if (access) requireTrustedIssue(access); // Retries a publish that failed or was refused (#103); it runs after this action commits, in the background. if (!publishing) throw new GuardRefusal(config.demo ? 'Demos never publish pull requests.' : 'This runner does not publish pull requests.'); // Replaced by the publish's outcome once it settles (recordPublish), so a replay reports that. @@ -405,7 +462,9 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge try { described = await planning.describe(signal); } catch (error) { if (stopping) throw new ShuttingDownError(); - throw new UpstreamFailure(`The issue could not be read from GitHub: ${error instanceof Error ? error.message : String(error)}`); + const outcome = error instanceof GuardRefusal ? error + : new UpstreamFailure(`The issue could not be read from GitHub: ${error instanceof Error ? error.message : String(error)}`); + return service.store.userAction(identity, { actionId, kind, request }, () => { throw outcome; }).response; } } } @@ -424,6 +483,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Read above whenever the checks before this line pass; they cannot change across the synchronous action. if (!described) throw new GuardRefusal('Planning agent not available yet.'); const amendment = service.planningContextForAmendment(), context = amendment.context; + described.validate(); const handle = suggestions.start({ context, completedItems: amendment.completedItems, continuationBinding: amendment.continuation, continuationContext: amendment.continuationContext, revision: plan.revision, snapshotId: snapshot.id, issue: described.issue, approvedLessons: described.approvedLessons, feedback: input.feedback, @@ -491,6 +551,32 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge // Admitted requests drain normally (AGENTS.md); only the irreversible merge boundary rechecks the flag. if (stopping && input.action === 'merge') { json(503, { error: 'The review server is shutting down.' }); return; } if(path==='/api/issues') { + if (input?.action === 'trust' || input?.action === 'untrust') { + if (!trustGateway) throw new GuardRefusal('Issue trust actions are not configured.'); + if (!isUuidV4(input.actionId)) throw new BadRequest('An issue trust action needs a UUID v4 actionId.'); + if (!Number.isSafeInteger(input.number) || (input.number as number) < 1) throw new BadRequest('An issue trust action needs a positive issue number.'); + if (input.authorLogin !== null && (typeof input.authorLogin !== 'string' || !(input.authorLogin as string))) + throw new BadRequest('An issue trust action needs the current author login or null.'); + const kind = `issue-${input.action}`, request = { repository: trustGateway.repository, number: input.number, + authorLogin: input.authorLogin, action: input.action }; + const saved = service.store.savedAction(identity, { actionId: input.actionId, kind, request }); + if (saved) { json(200, issues.view()); return; } + let access: IssueAccess; + try { access = await trustGateway.issueAccess(input.number as number, { signal: requestAbort.signal, timeoutMs: 12_000 }); } + catch (error) { + if (stopping || requestAbort.signal.aborted) throw new ShuttingDownError(); + const failure = new UpstreamFailure(`The issue author could not be read from GitHub: ${error instanceof Error ? error.message : String(error)}`); + service.store.userAction(identity, { actionId: input.actionId, kind, request }, () => { throw failure; }); + json(200, issues.view()); return; + } + service.store.userAction(identity, { actionId: input.actionId, kind, request }, () => { + if (access.authorLogin !== input.authorLogin) throw new GuardRefusal('The issue author changed. Refresh before changing trust.'); + service.store.setIssueTrust({ repository: trustGateway.repository, issue: access.number, authorLogin: access.authorLogin, + trusted: input.action === 'trust', trustedBy: 'local user' }); + return { outcome: input.action === 'trust' ? 'trusted' : 'untrusted', number: access.number }; + }); + json(200, issues.view()); return; + } if(input?.action!=='refresh')throw new Error('Unsupported issue action.'); await testHooks.beforeIssueRefreshWait?.(); // A departing browser stops waiting; the board keeps the shared refresh for other callers. @@ -503,7 +589,7 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge finally { res.removeListener('close',depart); } return; } - if(path==='/api/runner') { json(200, { result: runnerAction(input), runner: runnerView() }); return; } + if(path==='/api/runner') { json(200, { result: await runnerAction(input, requestAbort.signal), runner: runnerView() }); return; } if(planningPath) { json(200, { result: await planningAction(path, input, requestAbort.signal) }); return; } if(path==='/api/settings') {service.store.setQuestionProvider(input.questionProvider);json(200,{questionProvider:service.store.questionProvider()});return;} if(input.action==='retry-question') { @@ -579,7 +665,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge * runs once, in the background; recovery finds a lost opening by its marker. A publish pushes, so `verifyLock` * (the runner lock still names the database) runs first, here: if it throws, nothing starts and its error is thrown. */ - publishOwed: (verifyLock: () => void) => { verifyLock(); publishing?.startup(identity); }, + publishOwed: (verifyLock: () => void): void | Promise => { + verifyLock(); + if (!publishing) return; + publishing.startup(identity); + }, url: `http://127.0.0.1:${address.port}/#${token}`, close: async () => { // Step 1, one synchronous turn: reject new API requests and new runner work. Admitted requests drain (step 2). stopping = true;