diff --git a/cloudflare/README.md b/cloudflare/README.md index 0461bfeb..89493b4c 100644 --- a/cloudflare/README.md +++ b/cloudflare/README.md @@ -167,8 +167,8 @@ and deletes its own object. ### Decision inbox After signing in, a maintainer lands on `/inbox` (`src/inbox/`): one active -decision thread across the projects they enrolled, then what is up next, what -is in progress, and their projects. Each item is one job target (`issues/1`, +decision thread across their projects, then what is up next, what is in +progress, and their projects. Each item is one job target (`issues/1`, `pulls/2`, a quest path), so an issue, its planning job, and that job's questions stay one item, and webhook updates and reconnects never add another prompt. Agents waiting on an answer come first, then failed jobs, new pull @@ -186,6 +186,15 @@ awaits a decision, and any other whose issue, pull request, or quest is still observed, so finished work stays set aside. Retrying a failed planning interview keeps its `plans/` target. +A user's projects are those in `github_project_members`. Enrolling a project +adds its enroller, and any other user joins by passing the maintainer check on +one of its pages, such as its setup page; no extra GitHub calls list +maintainers. Membership only decides what the inbox lists: every action still +rechecks authority. A check GitHub denies drops the user's membership along with +their read and skip state for the project, while one that cannot complete, such +as a rate limit, fails the request and leaves membership as it is. +Disconnecting a project removes its memberships. + Each maintainer's read state lives in `inbox_items`, keyed by a version of what the item showed (a question, a GitHub change, a quest's text). A newer version is unread again, and skipping an item lasts until it changes. A finished or diff --git a/cloudflare/migrations/0012_project_members.sql b/cloudflare/migrations/0012_project_members.sql new file mode 100644 index 00000000..0faf6440 --- /dev/null +++ b/cloudflare/migrations/0012_project_members.sql @@ -0,0 +1,12 @@ +-- Whose decision inbox lists a project: each user who passed a current +-- maintainer check on it, the enrolling user included. Membership is +-- navigation, never authority; every action still rechecks GitHub. A check +-- GitHub denies removes it, and disconnecting the project cascades. +-- `github_projects.enrolledBy` stays a record of who enrolled it. +CREATE TABLE github_project_members ( + repository INTEGER NOT NULL REFERENCES github_projects(id) ON DELETE CASCADE, + user TEXT NOT NULL REFERENCES user(id) ON DELETE CASCADE, + PRIMARY KEY(repository, user) +); +CREATE INDEX github_project_members_user ON github_project_members(user); +INSERT INTO github_project_members(repository, user) SELECT id, enrolledBy FROM github_projects; diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index f01e5142..ff490fc3 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -56,6 +56,8 @@ export class GitHubError extends Error { } /** A publication the bot cannot make, which needs a maintainer's decision rather than a retry. */ export class PublicationBlocked extends GitHubError {} +/** A maintainer check that was answered with a denial, rather than one that could not complete. */ +export class NotMaintainer extends GitHubError {} /** One file a publication sets or, with null content, deletes. */ /** One path's new content, or null to delete it, with its Git mode: a file, an executable, or a symlink. */ @@ -273,27 +275,61 @@ export class GitHubService { }); } + /** + * Verify current maintainer authority, and keep the user's inbox membership + * of an enrolled project in step with the answer. Membership lists the + * project; it never stands in for this check. A check that cannot complete, + * such as an outage or a rate limit, fails without changing membership. + */ async #maintainer(token: string, repo: GitHubRepository, userId: string) { + const db = this.#env.DB; + try { + await this.#authority(token, repo, userId); + } catch (error) { + if (error instanceof NotMaintainer) + await db.batch([ + db + .prepare("DELETE FROM github_project_members WHERE repository=? AND user=?") + .bind(repo.id, userId), + db.prepare("DELETE FROM inbox_items WHERE repository=? AND user=?").bind(repo.id, userId), + ]); + throw error; + } + await db + .prepare( + "INSERT INTO github_project_members(repository,user) SELECT id,? FROM github_projects WHERE id=? ON CONFLICT DO NOTHING", + ) + .bind(userId, repo.id) + .run(); + } + + async #authority(token: string, repo: GitHubRepository, userId: string) { const account = await this.#env.DB.prepare( "SELECT accountId FROM account WHERE userId=? AND providerId='github' LIMIT 1", ) .bind(userId) .first<{ accountId: string }>(); - if (!account || !/^\d+$/.test(account.accountId)) throw new GitHubError("Linked GitHub identity required"); + if (!account || !/^\d+$/.test(account.accountId)) throw new NotMaintainer("Linked GitHub identity required"); const user = await this.#api<{ id: number; login: string }>(token, `/user/${account.accountId}`); if (String(user.id) !== account.accountId || !name(user.login)) throw new GitHubError("GitHub identity changed"); + // Every caller has already read the repository with this token, and the + // user resolved above, so a 404 here means the user is no collaborator. const permission = await this.#api<{ permission: string; role_name: string; user: { id: number } }>( token, `${repositoryPath(repo)}/collaborators/${user.login}/permission`, - ); + ).catch((error: unknown) => { + if (error instanceof GitHubError && error.status === 404) + throw new NotMaintainer("Current GitHub maintainer authority required"); + throw error; + }); // GitHub maps maintain to legacy permission=write. Do not accidentally // grant maintainer authority to ordinary writers or custom roles. if ( String(permission.user.id) !== account.accountId || (permission.permission !== "admin" && permission.role_name !== "maintain") ) - throw new GitHubError("Current GitHub maintainer authority required"); + throw new NotMaintainer("Current GitHub maintainer authority required"); } async #authorized( @@ -349,11 +385,12 @@ export class GitHubService { return row?.state ?? null; } + /** The enrolled projects whose inbox the user belongs to. */ async projects(userId: string): Promise { - // Remembered selections are navigation only, never an authority grant. + // Membership is navigation only, never an authority grant. return ( await this.#env.DB.prepare( - "SELECT id,owner,name,state FROM github_projects WHERE enrolledBy=? ORDER BY owner,name", + "SELECT p.id,p.owner,p.name,p.state FROM github_projects p JOIN github_project_members m ON m.repository=p.id WHERE m.user=? ORDER BY p.owner,p.name", ) .bind(userId) .all() @@ -665,6 +702,10 @@ export class GitHubService { `INSERT INTO github_projects(id,owner,name,installation,state,enrolledBy,updatedAt) VALUES(?,?,?,?,'enabled',?,?) ON CONFLICT(id) DO UPDATE SET owner=excluded.owner,name=excluded.name,installation=excluded.installation,state=excluded.state,updatedAt=excluded.updatedAt`, ).bind(repo.id, repo.owner, repo.name, installation, userId, Date.now()), + // The project did not exist when the check above ran. + this.#env.DB.prepare( + "INSERT INTO github_project_members(repository,user) VALUES(?,?) ON CONFLICT DO NOTHING", + ).bind(repo.id, userId), ]); } return []; diff --git a/cloudflare/src/inbox/model.ts b/cloudflare/src/inbox/model.ts index 6d100ae1..c19b9ef4 100644 --- a/cloudflare/src/inbox/model.ts +++ b/cloudflare/src/inbox/model.ts @@ -5,7 +5,7 @@ import { parse } from "quest/core"; import { encodePath } from "../board/model"; -import { GitHubError, type Enrollment, GitHubService } from "../github/service"; +import { GitHubError, type Enrollment, GitHubService, NotMaintainer } from "../github/service"; import type { Activity, ProjectState } from "../github/state"; import { type Prompt, parsePrompt } from "../jobs/prompt"; import { hub } from "../jobs/routes"; @@ -196,21 +196,24 @@ export function arrange(items: Item[]) { export type Runners = Awaited>; /** - * Load the inbox across the projects a maintainer enrolled. Every project - * rechecks current GitHub authority; one that fails is reported, not hidden. + * Load the inbox across the projects the user is a member of. Every project + * rechecks current GitHub authority. A denied check ends the membership and + * hides the project; one that cannot complete is reported, not hidden. */ export async function load(env: Env, user: string) { const github = new GitHubService(env); - const projects: ProjectView[] = await Promise.all( - (await github.projects(user)).map(async (project) => { + const views = await Promise.all( + (await github.projects(user)).map(async (project): Promise => { try { return { project, state: await github.projectState(project, user), error: null }; } catch (error) { + if (error instanceof NotMaintainer) return null; if (!(error instanceof GitHubError)) throw error; return { project, state: null, error: error.message }; } }), ); + const projects = views.filter((view) => view !== null); const usable = projects.filter((view) => !view.error); const found = usable.map((view) => entities(view.project, view.state)); const [reads, jobs] = await Promise.all([ diff --git a/cloudflare/src/inbox/page.tsx b/cloudflare/src/inbox/page.tsx index 36cb42be..9c4633be 100644 --- a/cloudflare/src/inbox/page.tsx +++ b/cloudflare/src/inbox/page.tsx @@ -529,12 +529,13 @@ export function InboxPage(props: InboxProps) { ) : (

- No enrolled projects yet. Enroll a project to get its issues, pull - requests, and ready quests here. + No enrolled projects yet. Enroll a project, or open one a co-maintainer + enrolled, to get its issues, pull requests, and ready quests here.

)}

- Lists projects you enrolled. Runners and accounts are set up with quest runner. + Lists enrolled projects you maintain, once you open each one's setup page. Runners and accounts are + set up with quest runner.

{props.skipped.length > 0 && ( diff --git a/cloudflare/test/inbox.test.ts b/cloudflare/test/inbox.test.ts index 6ac73aab..32f82e68 100644 --- a/cloudflare/test/inbox.test.ts +++ b/cloudflare/test/inbox.test.ts @@ -4,7 +4,7 @@ import { app } from "../src/index"; import { questHref } from "../src/inbox/model"; import * as store from "../src/jobs/store"; import { privateKey } from "./github-key"; -import { origin } from "./login-fixtures"; +import { identity, oauth, origin } from "./login-fixtures"; import { commit, delivery, head, maintainerOf, project, pull } from "./project-fixtures"; const testEnv = { ...env, GITHUB_APP_PRIVATE_KEY: privateKey }; @@ -14,6 +14,7 @@ const jobs = "/projects/github/upstream/project/jobs"; let user = ""; let cookie = ""; let github: Awaited>["github"]; +let mock: Awaited>["mock"]; beforeEach(async () => { await db.batch([ @@ -22,19 +23,49 @@ beforeEach(async () => { db.prepare("DELETE FROM github_projects"), db.prepare("DELETE FROM github_installations"), ]); - ({ user, cookie, github } = await maintainerOf()); + ({ user, cookie, github, mock } = await maintainerOf()); }); afterEach(() => vi.restoreAllMocks()); -async function inbox(query: Record = {}) { +async function inbox(query: Record = {}, session = cookie) { const response = await app.request( `${origin}/inbox?${new URLSearchParams(query)}`, - { headers: { Cookie: cookie } }, + { headers: { Cookie: session } }, testEnv, ); expect(response.status).toBe(200); return response.text(); } +/** Sign in another GitHub user, who holds the maintain role when `maintains`. */ +async function other(maintains: boolean) { + const who = identity("github"); + const session = (await oauth(who)).session!; + vi.spyOn(globalThis, "fetch").mockImplementation(mock); + if (maintains) github.maintainers.push(who.id); + const { userId } = (await db + .prepare("SELECT userId FROM account WHERE providerId='github' AND accountId=?") + .bind(who.id) + .first<{ userId: string }>())!; + return { session, id: userId }; +} +/** Open the project's activity page, which checks maintainer authority. */ +function visit(session = cookie) { + return app.request(`${origin}/projects/github/upstream/project/state`, { headers: { Cookie: session } }, testEnv); +} +async function members() { + const { results } = await db + .prepare("SELECT user FROM github_project_members WHERE repository=10") + .all<{ user: string }>(); + return results.map((row) => row.user); +} +/** How many items the user has read or skipped on the project. */ +async function reads(who = user) { + const row = await db + .prepare("SELECT count(*) AS n FROM inbox_items WHERE repository=10 AND user=?") + .bind(who) + .first<{ n: number }>(); + return row!.n; +} /** The active decision thread's markup. */ function thread(html: string) { return html.match(/
/)?.[0] ?? ""; @@ -504,15 +535,83 @@ describe("decision inbox", () => { expect(html).not.toContain("Skipped and finished"); }); - it("reports a project without current authority and hides its items", async () => { + it("shows a co-maintainer the project once they open it", async () => { + const co = await other(true); + let html = await inbox({}, co.session); + expect(html).toContain("No enrolled projects yet"); + expect(html).not.toContain("Idea"); + expect((await visit(co.session)).status).toBe(200); + expect(await members()).toEqual(expect.arrayContaining([user, co.id])); + html = await inbox({}, co.session); + expect(html).toContain("Idea"); + expect(html).not.toContain("Unavailable"); + }); + + it("never lists a project for a non-maintainer", async () => { + const outsider = await other(false); + const refused = await visit(outsider.session); + expect(refused.status).toBe(403); + expect(await refused.text()).toContain("Current GitHub maintainer authority required"); + expect(await members()).toEqual([user]); + const html = await inbox({}, outsider.session); + expect(html).toContain("No enrolled projects yet"); + expect(html).not.toContain("Idea"); + }); + + it("drops a revoked maintainer along with their read state", async () => { + await submit("/projects/github/upstream/project/skip", [ + ["item", "issues/1"], + ["version", "0"], + ]); + expect(await reads()).toBe(1); github.role = "write"; - const html = await inbox({ done: "answer" }); - expect(html).toContain("Unavailable: Current GitHub maintainer authority required"); + let html = await inbox(); + expect(html).not.toContain("Idea"); + expect(html).not.toContain("Unavailable"); + expect(html).toContain("No enrolled projects yet"); + expect(await members()).toEqual([]); + expect(await reads()).toBe(0); + // Regaining the role does not restore membership until the maintainer opens the project. + github.role = "maintain"; + expect(await inbox()).not.toContain("Idea"); + expect((await visit()).status).toBe(200); + html = await inbox(); + expect(html.match(/

Up next<\/h2>[\s\S]*?<\/ol>/)![0]).toContain("Idea"); + expect(html).not.toContain("Skipped and finished"); + }); + + it("drops a maintainer removed from the repository", async () => { + await submit("/projects/github/upstream/project/skip", [ + ["item", "issues/1"], + ["version", "0"], + ]); + github.role = null; + const html = await inbox(); + expect(html).not.toContain("Unavailable"); + expect(html).toContain("No enrolled projects yet"); + expect(await members()).toEqual([]); + expect(await reads()).toBe(0); + }); + + it("keeps membership when a maintainer check cannot complete", async () => { + await submit("/projects/github/upstream/project/skip", [ + ["item", "issues/1"], + ["version", "0"], + ]); + github.limited = true; + let html = await inbox({ done: "answer" }); + expect(html).toContain("Unavailable: GitHub request failed (429)"); expect(html).not.toContain("Idea"); // With no decision left, the notice shows on the empty state. expect(html).toContain("Answer recorded."); expect(html).toContain("Nothing needs a decision right now."); expect(await inbox({ done: "constructor" })).not.toContain('

'); + expect((await visit()).status).toBe(403); + expect(await members()).toEqual([user]); + expect(await reads()).toBe(1); + github.limited = false; + html = await inbox(); + expect(html).toMatch(/Skipped and finished \(1\)[\s\S]*Idea/); }); it("keeps paused projects' questions but offers no new work", async () => { diff --git a/cloudflare/test/project-fixtures.ts b/cloudflare/test/project-fixtures.ts index 1eb36300..af644c1f 100644 --- a/cloudflare/test/project-fixtures.ts +++ b/cloudflare/test/project-fixtures.ts @@ -17,12 +17,16 @@ export const documents = [ ]; export async function project(id: number, owner: string, state: "enabled" | "paused" = "enabled") { - await db - .prepare( - "INSERT INTO github_projects(id,owner,name,installation,state,enrolledBy,updatedAt) VALUES(?,'upstream','project',5,?,?,0) ON CONFLICT(id) DO UPDATE SET state=excluded.state", - ) - .bind(id, state, owner) - .run(); + await db.batch([ + db + .prepare( + "INSERT INTO github_projects(id,owner,name,installation,state,enrolledBy,updatedAt) VALUES(?,'upstream','project',5,?,?,0) ON CONFLICT(id) DO UPDATE SET state=excluded.state", + ) + .bind(id, state, owner), + db + .prepare("INSERT INTO github_project_members(repository,user) VALUES(?,?) ON CONFLICT DO NOTHING") + .bind(id, owner), + ]); } export const pull = { @@ -55,19 +59,23 @@ export const issue = { * pull request #2, and mock GitHub. `github` changes what the mock reports, and * `mock` reinstalls it after another sign-in replaces it. Merges of #2 record * their request in `merged`, and land only at `head`; `refuse` makes GitHub - * refuse them with that message. + * refuse them with that message. Other GitHub IDs in `maintainers` hold the + * maintain role too, and `limited` rate-limits every role check. A `role` of + * null removes the signed-in user from the repository, so GitHub answers 404. */ export async function maintainerOf() { const github = { flake: true, pullFlake: true, - role: "maintain", + role: "maintain" as string | null, issue: "open", pull: "open", head, methods: ["squash", "merge", "rebase"], refuse: null as string | null, merged: [] as unknown[], + maintainers: [] as string[], + limited: false, }; const who = identity("github"); const cookie = (await oauth(who)).session!; @@ -130,13 +138,18 @@ export async function maintainerOf() { const id = path.split("/").at(-1)!; return Response.json({ id: Number(id), login: `person${id}` }); } - // Only the signed-in maintainer holds the maintain role; anyone else may read. + // Only the signed-in maintainer and `maintainers` hold the maintain role; anyone else may read. const permission = path.match(/\/collaborators\/person(\d+)\/permission$/); + if (permission && github.limited) return Response.json({ message: "API rate limit exceeded" }, { status: 429 }); + if (permission?.[1] === who.id && github.role === null) + return Response.json({ message: "Not Found" }, { status: 404 }); if (permission) return Response.json( permission[1] === who.id ? { user: { id: Number(who.id) }, permission: "write", role_name: github.role } - : { user: { id: Number(permission[1]) }, permission: "read", role_name: "read" }, + : github.maintainers.includes(permission[1]!) + ? { user: { id: Number(permission[1]) }, permission: "write", role_name: "maintain" } + : { user: { id: Number(permission[1]) }, permission: "read", role_name: "read" }, ); // The live issue and pull request that a job binds, which observations can lag. if (path === "/repos/upstream/project/issues/1") return Response.json({ state: github.issue }); diff --git a/cloudflare/test/projects.test.ts b/cloudflare/test/projects.test.ts index a9d5a08e..86690e68 100644 --- a/cloudflare/test/projects.test.ts +++ b/cloudflare/test/projects.test.ts @@ -495,6 +495,8 @@ describe("project setup routes", () => { expect(await env.DB.prepare("SELECT name FROM github_projects WHERE id=10").first()).toEqual({ name: options.repositoryName, }); + // Passing the maintainer check joins the project's inbox even when its preview fails. + expect(await github().service.projects(account!.userId)).toEqual([expect.objectContaining({ id: 10 })]); for (const next of ["paused", "disconnected"]) { const stopped = await app.request( route, @@ -508,6 +510,8 @@ describe("project setup routes", () => { expect(stopped.status).toBe(303); } expect(await state()).toBeNull(); + // Disconnecting removes every maintainer's membership with the project. + expect(await env.DB.prepare("SELECT count(*) AS n FROM github_project_members").first()).toEqual({ n: 0 }); }); it("keeps stop controls after transfer without inheriting opt-in for the new installation", async () => { diff --git a/quest/m0/kix/README.md b/quest/m0/kix/README.md index 03366547..ffcc3a68 100644 --- a/quest/m0/kix/README.md +++ b/quest/m0/kix/README.md @@ -103,7 +103,6 @@ operator-paid-hosting decisions: - [opencode adapter](/quest/m0/kix/opencode.md) - OpenCode Go, ChatGPT plans, and API-key providers through opencode - [Webhook reconcile](/quest/m0/kix/reconcile.md) - new issues and PRs arrive in the inbox ready to act on, without a manual refresh - [Merge jobs](/quest/m0/kix/merge-job.md) - a merge pick fixes conflicts, CI, and reviews on a runner, then lands the PR -- [Co-maintainer inbox](/quest/m0/kix/members.md) - every current maintainer sees enrolled projects in their inbox - [Decision paper trail](/quest/m0/kix/paper-trail.md) - planning PRs carry the recorded questions and answers, rendered by the service - [Agent activity](/quest/m0/kix/activity.md) - agent cards and map highlights built from checkpoint summaries - [Mobile web app](/quest/m0/kix/mobile.md) - an installable PWA with Web Push for decisions diff --git a/quest/m0/kix/members.md b/quest/m0/kix/members.md deleted file mode 100644 index a9850646..00000000 --- a/quest/m0/kix/members.md +++ /dev/null @@ -1,25 +0,0 @@ -# [S] Show co-maintainers' projects in their inbox - -## Goal - -Every current maintainer of an enrolled project sees it in their decision -inbox, not only the user who enrolled it. A maintainer whose access is revoked -stops seeing it. - -## Plan - -Decided 2026-10-18 as a follow-up of the decision inbox (#144), which lists -only projects the viewer enrolled (`enrolledBy`): - -- A maintainer joins a project's inbox membership when they pass the - per-request maintainer check on its project page. No extra GitHub calls to - list maintainers. -- Membership only decides which projects the inbox lists. It is not - authority: every action still rechecks the maintainer per request. -- A maintainer check that GitHub answers with a denial drops that user's - membership along with their read and skip state for the project. A check - that cannot complete (an outage or rate limit) fails the request and leaves - membership as it is. The enrolling user is a member like any other. -- Disconnecting a project removes its memberships with it. -- Test a co-maintainer joining, a revoked maintainer dropping out, a - rate-limited check keeping membership, and a non-maintainer never joining.