diff --git a/.claude/skills/recipes/pr-factory/SKILL.md b/.claude/skills/recipes/pr-factory/SKILL.md index 998f68120..4b2f0d444 100644 --- a/.claude/skills/recipes/pr-factory/SKILL.md +++ b/.claude/skills/recipes/pr-factory/SKILL.md @@ -89,7 +89,7 @@ All suites green. `src/recipe-pr-factory-stack.test.ts` is the composed-stack le - **Per-PR worker flow** — webhook → Slack thread (status reactions 🟢⚪🔴🟣) → per-thread agent session seeded with the diff → triage report → review → test plan. The default triage/review/test-plan workflow is seeded into `groups/pr-factory-worker/CLAUDE.local.md` (edit it to tune trusted contributors, merge policy, review depth) or replaced wholesale via `PR_FACTORY_REVIEW_SKILL` — see "Tailoring the bots" below. - **Approval cards for every consequential action** — send-to-testing, retry, skill edits, and every `gh` write (merge, close, comment), executed with the approving human's gh identity. -- **Supervisor bot** — its own Slack identity; takes feedback in an admin channel or @-mentioned in PR threads, proposes worker skill/instruction edits behind a diff + approval card, clears and retriggers PR sessions. +- **Supervisor bot** — its own Slack identity; takes feedback in an admin channel or @-mentioned in PR threads, proposes worker skill/instruction edits behind a diff + approval card. Approved edits apply to the next PR the worker triages. - **VM test runs** — approved plans clone an ephemeral VM from a template, check out the PR, build, boot, and hand the VM to the tester agent; PASS wakes the worker to propose a merge, FAIL to analyze. - **Canvases** — test plans, results, and review writeups render as Slack Canvases instead of file uploads (paid Slack plan; falls back to `.md` uploads otherwise). @@ -100,7 +100,7 @@ The shipped triage/review/test-plan workflow is deliberately generic. The PR Fac - **Container skills** live at `container/skills//SKILL.md` (read-only at `/app/skills` in every agent container). Each group's container config has a `skills` selection (default `'all'`) that controls which ones are symlinked into that group's `~/.claude/skills` at spawn — so a new skill folder reaches the worker on its next container start, no config change needed. - **Group-private skills**: a directory at `groups//.claude/skills//` is discovered as a project-level skill (the agent's cwd is `/workspace/agent`, the group folder). Use this for a skill only one group should ever see — but note it sits outside the supervisor's edit loop below. - **Precedence**: with `PR_FACTORY_REVIEW_SKILL=` set, every PR trigger opens with `Use the / skill to triage this pull request.` and the generic defaults seeded into `groups/pr-factory-worker/CLAUDE.local.md` are ignored. Without it, the seeded instructions run — editing them in place is the lighter path when the defaults are close. -- **Iteration loop**: `container/skills/` is what the supervisor bot edits — `propose_skill_edit` writes `container/skills//` behind a diff + approval card, then clears + retriggers affected PRs. Routing repo-specific workflow into a container skill (rather than CLAUDE.local.md) is what makes the feedback loop reviewable. +- **Iteration loop**: `container/skills/` is what the supervisor bot edits — `propose_skill_edit` writes `container/skills//` behind a diff + approval card; the edit applies to the next PR the worker triages (running sessions keep their old read-only skill view until they next spawn). Routing repo-specific workflow into a container skill (rather than CLAUDE.local.md) is what makes the feedback loop reviewable. ### Interview the operator, then generate diff --git a/.claude/skills/recipes/pr-factory/files/docs/pr-factory.md b/.claude/skills/recipes/pr-factory/files/docs/pr-factory.md index 2ce37ff5d..5e6b69c8b 100644 --- a/.claude/skills/recipes/pr-factory/files/docs/pr-factory.md +++ b/.claude/skills/recipes/pr-factory/files/docs/pr-factory.md @@ -174,11 +174,11 @@ CREATE INDEX idx_pr_threads_session ON pr_threads (session_id); | Function | Key | Used by | |----------|-----|---------| -| `getPrThread(channelId, threadTs)` | PK | session-ops (clear) | -| `getPrThreadByRepoPr(repo, prNumber)` | `idx_pr_threads_repo_pr` | handler (dedup, close, synchronize), session-ops, orchestrator | +| `getPrThread(channelId, threadTs)` | PK | CRUD primitive (covered by `pr-threads.test.ts`) | +| `getPrThreadByRepoPr(repo, prNumber)` | `idx_pr_threads_repo_pr` | handler (dedup, close, synchronize), orchestrator | | `getPrThreadBySession(sessionId)` | `idx_pr_threads_session` | testing-approval, gh-action-approval, reactions | | `updatePrThreadSession(channelId, threadTs, sessionId)` | PK | handler (synchronize repoints the row) | -| `deletePrThread(channelId, threadTs)` | PK | session-ops (clear) | +| `deletePrThread(channelId, threadTs)` | PK | CRUD primitive (covered by `pr-threads.test.ts`) | --- @@ -262,7 +262,7 @@ A separate agent group (`pr-factory-supervisor`) speaking as its own Slack bot. - **Admin channel** — shared session, engages on every message. - **PR threads** — engages when @-mentioned, sees accumulated thread history. -Its MCP tools: `clear_session`, `retrigger`, `propose_skill_edit` (see below). The instructions tell it to always clear + retrigger affected PRs after an edit lands. +Its MCP tool: `propose_skill_edit` (see below). A proposed edit is posted as a diff for human approval and, on accept, written under `container/skills/`. Skill edits apply to the **next** PR each affected worker session triages — running sessions keep their old read-only skill view until they next spawn; there is no force-rerun of an in-flight session. --- @@ -295,12 +295,10 @@ The template VM is operator-prepared: project at `~/nanoclaw` with an `origin` t **File:** `container/agent-runner/src/mcp-tools/pr-factory.ts` -Six tools, registered in every container via the mcp-tools barrel. Each writes a `kind: 'system'` row to `messages_out`; the host's delivery loop dispatches the `action` string to the matching registered handler. In a non-PR-factory install the actions are unregistered and dropped with "Unknown system action". +Four tools, registered in every container via the mcp-tools barrel. Each writes a `kind: 'system'` row to `messages_out`; the host's delivery loop dispatches the `action` string to the matching registered handler. In a non-PR-factory install the actions are unregistered and dropped with "Unknown system action". | Tool | Action emitted | Args | |------|----------------|------| -| `clear_session` | `pr_clear_session` | `{ pr_number, repo? }` | -| `retrigger` | `pr_retrigger` | `{ pr_number, repo? }` | | `propose_skill_edit` | `pr_propose_skill_edit` | `{ skill_name, file_name, content }` | | `send_to_testing` | `pr_send_to_testing` | `{}` (plan located via session → pr_threads → file naming) | | `credentialed_gh` | `pr_gh` | `{ command?, commands?, description }` — `command` is normalized into `commands` | @@ -316,8 +314,6 @@ When the agent omits `repo`, the field is omitted from the payload too — the h | Delivery action | Handler | Effect | |--------|-------------|--------| -| `pr_clear_session` | `session-ops.clearWorkerSession(repo, pr)` | Kill container, delete session, delete pr_threads row | -| `pr_retrigger` | `session-ops.retriggerWorker(repo, pr)` | Kill container, re-fetch diff, re-trigger in the same thread/session | | `pr_send_to_testing` | `testing-approval.handleSendToTesting` | Post plan canvas + approval card | | `pr_propose_skill_edit` | `skill-edit-approval.handleProposeSkillEdit` | Post diff + approval card | | `pr_gh` | gh-action seam → `gh-action-approval.handleGh` | Post command preview + approval card | @@ -431,7 +427,6 @@ tail -f data/pr-activity///*.log # all PRs | `webhook.ts` | core | GitHub webhook: HMAC, event/action filter, `PREvent` parsing | | `handler.ts` | core | Per-PR lifecycle: opener, session, pr_threads, trigger, wake; synchronize/close/draft handling; OneCLI proxy GitHub fetches | | `supervisor.ts` | core | `SUPERVISOR_FOLDER` + `SUPERVISOR_INSTRUCTIONS` | -| `session-ops.ts` | core | `clearWorkerSession` / `retriggerWorker`, keyed by `(repo, pr_number)` | | `testing-approval.ts` | core | Testing gate + retry card | | `skill-edit-approval.ts` | core | Skill-edit gate (traversal-guarded writes into `container/skills/`) | | `orchestrator.ts` | core | Tester wake, 30-min timeout, results → worker | @@ -472,9 +467,9 @@ tail -f data/pr-activity///*.log # all PRs ## Manual Operations -### Clear or retrigger a PR session +### Re-run a PR from scratch -Ask the supervisor bot (its `clear_session` / `retrigger` MCP tools), or from the host REPL the module exports `clearWorkerSession(repo, prNumber)` and `retriggerWorker(repo, prNumber)` from `src/modules/pr-factory/index.ts`. +There is no in-place "clear and retrigger" — a skill edit applies to the next PR the worker triages, and a fresh push (a `synchronize` event) re-triages an open PR in the same thread against the updated diff. To force a clean re-run, close and reopen the PR on GitHub, or push a no-op commit; the webhook drives a fresh session either way. ### Inspect pr_threads diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/SKILL.md b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/SKILL.md index 954ec64d3..ddf4cb25c 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/SKILL.md +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-factory-core -description: PR Factory component — the engine. GitHub pull_request webhook → per-PR Slack thread + worker agent session (triage/review/test-plan via default group instructions), supervisor bootstrap with skill-edit approval flow, send-to-testing approval gate, test-result coordination, session clear/retrigger ops, pr_threads index, and six container MCP tools. Ships seams for the optional gh-action-approval, vm-test-orchestrator, and slack-canvas components. +description: PR Factory component — the engine. GitHub pull_request webhook → per-PR Slack thread + worker agent session (triage/review/test-plan via default group instructions), supervisor bootstrap with skill-edit approval flow, send-to-testing approval gate, test-result coordination, pr_threads index, and four container MCP tools. Ships seams for the optional gh-action-approval, vm-test-orchestrator, and slack-canvas components. --- # pr-factory-core (PR Factory component) @@ -12,8 +12,7 @@ The PR Factory engine, as a host module (`src/modules/pr-factory/`) plus a conta - **Bootstrap** (`bootstrap.ts`) — idempotent, self-correcting setup: worker agent group (default triage/review/test-plan instructions seeded into `groups/pr-factory-worker/CLAUDE.local.md`), default-instance worker messaging group, optional supervisor agent group with `slack-supervisor`-instance messaging groups (admin channel + PR channel), and a `slack-tester`-instance messaging group when the operator-created `pr-tester` agent group exists. - **Approval flows** — send-to-testing (`testing-approval.ts`, plan file → card → human gate), retry-after-failure, and supervisor skill edits (`skill-edit-approval.ts`, diff preview → card → write on approve). One active card per thread (`dismiss-approvals.ts`); a 👀 reaction marks awaiting-approval threads and clears on resolution — the reject path is observed through core's `registerApprovalResolvedHandler` hook. - **Coordination** (`orchestrator.ts`) — wakes the tester agent when a VM is ready, enforces a 30-minute run timeout, posts results, wakes the worker to propose merge (PASS) or analyze (FAIL/PARTIAL). -- **Session ops** (`session-ops.ts`) — supervisor-driven `clear_session` / `retrigger` keyed by (repo, pr_number). -- **Container MCP tools** (`container/agent-runner/src/mcp-tools/pr-factory.ts`) — `clear_session`, `retrigger`, `propose_skill_edit`, `send_to_testing`, `credentialed_gh`, `submit_test_results`. Each emits a `pr_*` system action via messages_out; the host module registers the six matching delivery actions. +- **Container MCP tools** (`container/agent-runner/src/mcp-tools/pr-factory.ts`) — `propose_skill_edit`, `send_to_testing`, `credentialed_gh`, `submit_test_results`. Each emits a `pr_*` system action via messages_out; the host module registers the four matching delivery actions. Inert without `GITHUB_WEBHOOK_SECRET`: the module loads (approval handlers bind) but registers no delivery actions and mounts no webhook. @@ -99,7 +98,7 @@ SKILL=.claude/skills/recipes/pr-factory/skills/pr-factory-core ```bash mkdir -p src/modules/pr-factory for f in index bootstrap handler webhook orchestrator testing-approval skill-edit-approval \ - dismiss-approvals session-ops reactions activity-log defaults supervisor \ + dismiss-approvals reactions activity-log defaults supervisor \ worker-instructions canvas test-orchestration gh-action; do cp $SKILL/files/src/modules/pr-factory/$f.ts src/modules/pr-factory/$f.ts done @@ -196,7 +195,7 @@ cp $SKILL/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts co | Test | Guards | |------|--------| -| `src/modules/pr-factory/registration.test.ts` | The modules-barrel line via the REAL barrel, the six `pr_*` delivery actions + three core approval handlers (read-side registries), the `GITHUB_WEBHOOK_SECRET` env gate, the host-side `PR_FACTORY_DEFAULT_REPO` contract, and the gh-action seam's not-installed fallback | +| `src/modules/pr-factory/registration.test.ts` | The modules-barrel line via the REAL barrel, the four `pr_*` delivery actions + three core approval handlers (read-side registries), the `GITHUB_WEBHOOK_SECRET` env gate, the host-side `PR_FACTORY_DEFAULT_REPO` contract, and the gh-action seam's not-installed fallback | | `src/modules/pr-factory/bootstrap.test.ts` | Bootstrap's consumption of the entity-model writers on the real migrated schema: instance-keyed messaging groups (worker default, supervisor/tester named), wiring modes, instruction seeding, idempotence, foreign-wiring drop, drift self-correction | | `src/modules/pr-factory/webhook.test.ts` | `registerGitHubWebhook`'s mount on core's raw webhook registry over real HTTP: HMAC accept/reject, event filtering, 405, throwing-handler → 500 | | `src/modules/pr-factory/handler.test.ts` | handler's consumption of resolveSession / writeSessionMessage / pr_threads on real session DBs: the PR_CONTEXT trigger contract, the default triage directive, synchronize kill/re-create, draft deferral, redelivery no-op | @@ -205,7 +204,7 @@ cp $SKILL/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts co | `src/db/pr-threads.test.ts` | Migration barrel presence (real `runMigrations`), v2 schema shape (no bot column), the legacy-upgrade recreate arm, idempotence, CRUD | | `src/db/sessions-approval-helpers.test.ts` | The four appended sessions.ts helpers | | `container/.../pr-factory-registration.test.ts` | The container mcp-tools barrel line (AST: side-effect import before `startMcpServer()`) | -| `container/.../pr-factory-tools.test.ts` | The six tools' messages_out contract: exact `pr_*` action strings (paired with the host registrations), odd-seq, repo-omission, command normalization | +| `container/.../pr-factory-tools.test.ts` | The four tools' messages_out contract: exact `pr_*` action strings (paired with the host registrations), odd-seq, repo-omission, command normalization | ## Configuration diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files.txt b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files.txt index d7bdd426c..fbca195bd 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files.txt +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files.txt @@ -12,7 +12,6 @@ src/modules/pr-factory/orchestrator.ts src/modules/pr-factory/testing-approval.ts src/modules/pr-factory/skill-edit-approval.ts src/modules/pr-factory/dismiss-approvals.ts -src/modules/pr-factory/session-ops.ts src/modules/pr-factory/reactions.ts src/modules/pr-factory/activity-log.ts src/modules/pr-factory/defaults.ts diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts index 5e46837a8..85684fa82 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory-tools.test.ts @@ -16,14 +16,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; import { initTestSessionDb, closeSessionDb } from '../db/connection.js'; import { getUndeliveredMessages } from '../db/messages-out.js'; -import { - clearSession, - ghCommand, - proposeSkillEdit, - retrigger, - sendToTesting, - submitTestResults, -} from './pr-factory.js'; +import { ghCommand, proposeSkillEdit, sendToTesting, submitTestResults } from './pr-factory.js'; beforeEach(() => { initTestSessionDb(); @@ -41,39 +34,6 @@ function systemRows(): Array<{ seq: number; kind: string; action: string; conten } describe('pr-factory MCP tools → messages_out contract', () => { - it('clear_session emits pr_clear_session with pr_number, OMITTING repo so the host applies its default', async () => { - const res = await clearSession.handler({ pr_number: 42 }); - expect(res.isError).toBeUndefined(); - - const [row] = systemRows(); - expect(row.kind).toBe('system'); - expect(row.seq % 2).toBe(1); // container writes odd seq - expect(row.action).toBe('pr_clear_session'); - expect(row.content.pr_number).toBe(42); - // Repo absent in the payload → host-side PR_FACTORY_DEFAULT_REPO applies. - expect('repo' in row.content).toBe(false); - }); - - it('clear_session passes an explicit repo through unchanged', async () => { - await clearSession.handler({ pr_number: 42, repo: 'acme/widgets' }); - const [row] = systemRows(); - expect(row.content.repo).toBe('acme/widgets'); - }); - - it('retrigger emits pr_retrigger and honors an explicit repo', async () => { - await retrigger.handler({ pr_number: 7, repo: 'acme/widgets' }); - const [row] = systemRows(); - expect(row.action).toBe('pr_retrigger'); - expect(row.content).toMatchObject({ pr_number: 7, repo: 'acme/widgets' }); - }); - - it('retrigger omits repo from the payload when the agent does not pass one', async () => { - await retrigger.handler({ pr_number: 7 }); - const [row] = systemRows(); - expect(row.action).toBe('pr_retrigger'); - expect('repo' in row.content).toBe(false); - }); - it('propose_skill_edit emits pr_propose_skill_edit with the full file payload', async () => { await proposeSkillEdit.handler({ skill_name: 'my-review-skill', file_name: 'SKILL.md', content: '# v2' }); const [row] = systemRows(); @@ -85,6 +45,7 @@ describe('pr-factory MCP tools → messages_out contract', () => { await sendToTesting.handler({}); const [row] = systemRows(); expect(row.kind).toBe('system'); + expect(row.seq % 2).toBe(1); // container writes odd seq expect(row.action).toBe('pr_send_to_testing'); }); diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory.ts index 117717f94..56110a976 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/container/agent-runner/src/mcp-tools/pr-factory.ts @@ -1,6 +1,6 @@ /** - * PR Factory MCP tools — supervisor session ops, skill-edit proposals, - * testing gate, credentialed GitHub commands, and test-result submission. + * PR Factory MCP tools — supervisor skill-edit proposals, testing gate, + * credentialed GitHub commands, and test-result submission. * * The container can't write to inbound.db (host-owned), so each tool emits * a system action via messages_out and the host's delivery loop dispatches @@ -31,82 +31,6 @@ function genId(prefix: string): string { return `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; } -export const clearSession: McpToolDefinition = { - tool: { - name: 'clear_session', - description: - "PR Factory supervisor only. Wipe the worker's per-thread session for a PR (kills the container, deletes the session row). Use after editing a skill so the next triage starts from clean state. Always pair with `retrigger` immediately after.", - inputSchema: { - type: 'object' as const, - properties: { - pr_number: { - type: 'number', - description: 'Pull request number, e.g. 2318.', - }, - repo: { - type: 'string', - description: - 'Full repository name, e.g. "acme/widgets". Omit to use the host\'s configured default repo (PR_FACTORY_DEFAULT_REPO).', - }, - }, - required: ['pr_number'], - }, - }, - async handler(args) { - const pr_number = args.pr_number as number; - // When the agent omits repo, the field is omitted from the payload too — - // the HOST action handler applies PR_FACTORY_DEFAULT_REPO. The container - // never sees that env var, so it must not bake in a default of its own. - const repo = args.repo as string | undefined; - if (!pr_number) return err('pr_number is required'); - - writeMessageOut({ - id: genId('sys'), - kind: 'system', - content: JSON.stringify({ action: 'pr_clear_session', pr_number, ...(repo ? { repo } : {}) }), - }); - log(`pr_clear_session: ${repo ?? ''}#${pr_number}`); - return ok(`Clear requested for ${repo ?? 'the default repo'}#${pr_number}`); - }, -}; - -export const retrigger: McpToolDefinition = { - tool: { - name: 'retrigger', - description: - 'PR Factory supervisor only. Re-fetch the PR diff fresh from GitHub and re-bootstrap the worker session for that PR. Use immediately after `clear_session` on every affected PR.', - inputSchema: { - type: 'object' as const, - properties: { - pr_number: { - type: 'number', - description: 'Pull request number, e.g. 2318.', - }, - repo: { - type: 'string', - description: - 'Full repository name, e.g. "acme/widgets". Omit to use the host\'s configured default repo (PR_FACTORY_DEFAULT_REPO).', - }, - }, - required: ['pr_number'], - }, - }, - async handler(args) { - const pr_number = args.pr_number as number; - // Omitted repo stays omitted — the host applies PR_FACTORY_DEFAULT_REPO. - const repo = args.repo as string | undefined; - if (!pr_number) return err('pr_number is required'); - - writeMessageOut({ - id: genId('sys'), - kind: 'system', - content: JSON.stringify({ action: 'pr_retrigger', pr_number, ...(repo ? { repo } : {}) }), - }); - log(`pr_retrigger: ${repo ?? ''}#${pr_number}`); - return ok(`Retrigger requested for ${repo ?? 'the default repo'}#${pr_number}`); - }, -}; - export const proposeSkillEdit: McpToolDefinition = { tool: { name: 'propose_skill_edit', @@ -276,4 +200,4 @@ export const submitTestResults: McpToolDefinition = { }, }; -registerTools([clearSession, retrigger, proposeSkillEdit, sendToTesting, ghCommand, submitTestResults]); +registerTools([proposeSkillEdit, sendToTesting, ghCommand, submitTestResults]); diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/db/pr-threads.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/db/pr-threads.ts index 8aa510544..2e83498ac 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/db/pr-threads.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/db/pr-threads.ts @@ -7,9 +7,9 @@ * is resolved per messaging group via `messaging_groups.instance`. * * Used by: - * - pr-factory handler: insert on PR opened - * - pr-factory session-ops (clear / retrigger): lookup by (channel_id, thread_ts) + * - pr-factory handler: insert on PR opened; lookup + repoint on synchronize/close * - pr-factory orchestrator: lookup by (repo_full_name, pr_number) when test results land + * - pr-factory testing/gh/skill-edit gates + reactions: lookup by (session_id) */ import { getDb } from './connection.js'; diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/handler.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/handler.ts index cfc220f17..d2ff33387 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/handler.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/handler.ts @@ -480,62 +480,3 @@ async function handleSynchronize(pr: PREvent, existing: PrThread, cfg: HandlerCo await wakeContainer(fresh); } } - -/** - * Re-bootstrap an existing PR session: re-fetch the diff fresh from GitHub - * and write a new trigger message into the same session. Used by retrigger. - * - * Looks up repo/pr#/session via the pr_threads row keyed by (channel, thread). - * Caller (session-ops.retrigger) has already killed the container if needed. - */ -export async function rebootstrapPrSession( - channelId: string, - threadTs: string, - agentGroupId: string, - repoFullName: string, - prNumber: number, -): Promise { - const platformId = channelId; // already in `slack:CXXXX` form on disk - const messagingGroup = getMessagingGroupByPlatform('slack', platformId, 'slack'); - if (!messagingGroup) { - throw new Error(`No worker messaging group for ${platformId}`); - } - const sessionThreadId = `${platformId}:${threadTs}`; - - const { session } = resolveSession(agentGroupId, messagingGroup.id, sessionThreadId, 'per-thread'); - - prLog(prNumber, repoFullName, 'retrigger_bootstrapping'); - await refreshRepoMirror(); - const diff = await fetchDiff(repoFullName, prNumber); - const now = new Date().toISOString(); - - const content = [ - `Re-triage PR #${prNumber} — supervisor requested a rerun after a skill update. ${triageDirective()}`, - '', - `## Pull Request #${prNumber}`, - `**Repository:** ${repoFullName}`, - '', - '### Diff (re-fetched)', - '```diff', - diff, - '```', - '', - `[PR_CONTEXT: channel=${platformId} thread=${threadTs} repo=${repoFullName} pr=${prNumber}]`, - ].join('\n'); - - writeSessionMessage(agentGroupId, session.id, { - id: generateId('msg-retrigger'), - kind: 'chat', - timestamp: now, - platformId, - channelType: 'slack', - threadId: sessionThreadId, - content: JSON.stringify({ text: content, sender: 'Supervisor', senderId: 'pr-factory-supervisor' }), - }); - - const fresh = getSession(session.id); - if (fresh) { - prLog(prNumber, repoFullName, 'retrigger_container_waking'); - await wakeContainer(fresh); - } -} diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/index.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/index.ts index 16744dd2a..1a76d4990 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/index.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/index.ts @@ -37,11 +37,9 @@ import { clearAwaitingApproval } from './reactions.js'; import { registerGitHubWebhook } from './webhook.js'; import { handlePullRequest } from './handler.js'; import { bootstrapPrFactory, TESTER_FOLDER } from './bootstrap.js'; -import { DEFAULT_REPO } from './defaults.js'; import { getTestOrchestrator } from './test-orchestration.js'; import { dispatchGhAction } from './gh-action.js'; import { initOrchestrator, shutdownOrchestrator, handleTestResults } from './orchestrator.js'; -import { clearWorkerSession, retriggerWorker } from './session-ops.js'; import { handleSendToTesting } from './testing-approval.js'; import { handleProposeSkillEdit } from './skill-edit-approval.js'; import { getAgentGroupByFolder } from '../../db/agent-groups.js'; @@ -85,12 +83,6 @@ if (!GITHUB_WEBHOOK_SECRET) { // same default inside handleTestResults. pr_gh dispatches through the // gh-action seam so the agent gets feedback even when the // gh-action-approval component isn't installed. - registerDeliveryAction('pr_clear_session', async (content) => { - clearWorkerSession((content.repo as string) || DEFAULT_REPO, content.pr_number as number); - }); - registerDeliveryAction('pr_retrigger', async (content) => { - await retriggerWorker((content.repo as string) || DEFAULT_REPO, content.pr_number as number); - }); registerDeliveryAction('pr_send_to_testing', async (content, session) => { await handleSendToTesting(content, session); }); @@ -154,5 +146,3 @@ if (!GITHUB_WEBHOOK_SECRET) { }); }); } - -export { clearWorkerSession, retriggerWorker } from './session-ops.js'; diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/registration.test.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/registration.test.ts index 3fa70386a..6e269ac9f 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/registration.test.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/registration.test.ts @@ -1,6 +1,6 @@ /** * pr-factory-core guard — the modules-barrel line (`import - * './pr-factory/index.js'` in src/modules/index.ts), the six pr_* delivery + * './pr-factory/index.js'` in src/modules/index.ts), the four pr_* delivery * actions, the three core-owned pr_* approval handlers, the gh-action seam's * not-installed fallback, and the GITHUB_WEBHOOK_SECRET env gate. * @@ -43,8 +43,6 @@ const TEST_DIR = '/tmp/nanoclaw-test-prf-registration'; const ORIGINAL_CWD = process.cwd(); const DELIVERY_ACTIONS = [ - 'pr_clear_session', - 'pr_retrigger', 'pr_send_to_testing', 'pr_propose_skill_edit', 'pr_gh', @@ -89,7 +87,7 @@ describe('pr-factory module registration via the real modules barrel', () => { } }); - it('with the env trio primed before import, all six pr_* delivery actions register', async () => { + it('with the env trio primed before import, all four pr_* delivery actions register', async () => { vi.resetModules(); process.env.GITHUB_WEBHOOK_SECRET = 'test-secret'; process.env.PR_FACTORY_SLACK_CHANNEL_ID = 'C0TEST'; @@ -122,7 +120,8 @@ describe('pr-factory module registration via the real modules barrel', () => { const now = new Date().toISOString(); const { createAgentGroup } = await import('../../db/agent-groups.js'); const { createSession } = await import('../../db/sessions.js'); - const { createPrThread, getPrThreadByRepoPr } = await import('../../db/pr-threads.js'); + const { createPrThread } = await import('../../db/pr-threads.js'); + const { initSessionFolder, outboundDbPath } = await import('../../session-manager.js'); createAgentGroup({ id: 'ag-prf', name: 'Worker', @@ -141,6 +140,7 @@ describe('pr-factory module registration via the real modules barrel', () => { last_active: null, created_at: now, }); + initSessionFolder('ag-prf', 'sess-prf'); // The pr_threads row is keyed by the DEFAULT repo — only a handler that // fills the omitted `repo` with PR_FACTORY_DEFAULT_REPO can find it. createPrThread({ @@ -155,15 +155,19 @@ describe('pr-factory module registration via the real modules barrel', () => { await import('../index.js'); const { getDeliveryAction } = await import('../../delivery.js'); - const { killContainer } = await import('../../container-runner.js'); - const handler = getDeliveryAction('pr_clear_session'); + const handler = getDeliveryAction('pr_submit_test_results'); expect(handler).toBeDefined(); const session = { id: 'sess-prf', agent_group_id: 'ag-prf' }; - await handler!({ pr_number: 42 }, session as never, undefined as never); + // No `repo` in the payload — the handler must default it to acme/defaulted + // to find the pr_threads row keyed by that repo and post the verdict. + await handler!({ pr_number: 42, verdict: 'PASS', content: '## results' }, session as never, undefined as never); - expect(killContainer).toHaveBeenCalledWith('sess-prf', 'cleared by supervisor'); - expect(getPrThreadByRepoPr('acme/defaulted', 42), 'pr_threads row should be cleared').toBeUndefined(); + const outDb = new Database(outboundDbPath('ag-prf', 'sess-prf'), { readonly: true }); + const rows = outDb.prepare('SELECT content FROM messages_out').all() as Array<{ content: string }>; + outDb.close(); + expect(rows.length, 'verdict summary should reach the worker session resolved via the default repo').toBe(1); + expect((JSON.parse(rows[0].content) as { text: string }).text).toContain('PASS'); } finally { closeDb(); } diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/session-ops.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/session-ops.ts deleted file mode 100644 index 52516dda6..000000000 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/session-ops.ts +++ /dev/null @@ -1,60 +0,0 @@ -/** - * Session operations exposed to the supervisor via MCP tools. - * - * Both functions are keyed by (repo, pr_number) — the natural identifiers - * visible in every worker message. Internally they look up the PR via the - * pr_threads table and operate on the worker's per-thread session. - */ -import { getPrThreadByRepoPr, deletePrThread } from '../../db/pr-threads.js'; -import { getSession, deleteSession } from '../../db/sessions.js'; -import { killContainer } from '../../container-runner.js'; -import { log } from '../../log.js'; -import { prLog } from './activity-log.js'; - -import { rebootstrapPrSession } from './handler.js'; - -export interface SessionOpResult { - ok: boolean; - message: string; -} - -/** - * Wipe the worker's per-thread session for a PR. Kills the running - * container, deletes the session row, and removes the pr_threads index - * entry so a future retrigger re-bootstraps cleanly. - */ -export function clearWorkerSession(repo: string, prNumber: number): SessionOpResult { - const pr = getPrThreadByRepoPr(repo, prNumber); - if (!pr) return { ok: false, message: `No PR thread for ${repo}#${prNumber}` }; - - const session = getSession(pr.session_id); - if (session) { - killContainer(session.id, 'cleared by supervisor'); - deleteSession(session.id); - } - deletePrThread(pr.channel_id, pr.thread_ts); - prLog(prNumber, repo, 'session_cleared', { sessionId: pr.session_id }); - log.info('Worker session cleared', { repo, prNumber, sessionId: pr.session_id }); - return { ok: true, message: `Cleared session for PR #${prNumber} (${repo})` }; -} - -/** - * Re-trigger PR triage with a freshly fetched diff. Keeps the same Slack - * thread + session id; the worker re-runs its triage workflow against the - * latest GitHub state. - */ -export async function retriggerWorker(repo: string, prNumber: number): Promise { - const pr = getPrThreadByRepoPr(repo, prNumber); - if (!pr) return { ok: false, message: `No PR thread for ${repo}#${prNumber}` }; - - const session = getSession(pr.session_id); - if (!session) { - return { ok: false, message: `Session ${pr.session_id} for PR #${prNumber} not found` }; - } - - killContainer(session.id, 'retriggered by supervisor'); - await rebootstrapPrSession(pr.channel_id, pr.thread_ts, session.agent_group_id, repo, prNumber); - prLog(prNumber, repo, 'session_retriggered', { sessionId: pr.session_id }); - log.info('Worker session retriggered', { repo, prNumber, sessionId: pr.session_id }); - return { ok: true, message: `Retriggered PR #${prNumber} (${repo})` }; -} diff --git a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/supervisor.ts b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/supervisor.ts index 59672566b..69091976d 100644 --- a/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/supervisor.ts +++ b/.claude/skills/recipes/pr-factory/skills/pr-factory-core/files/src/modules/pr-factory/supervisor.ts @@ -26,17 +26,17 @@ The PR number is visible in every worker message (e.g. "PR #2318" in the triage ## MCP tools you own -- \`mcp__nanoclaw__clear_session({ pr_number, repo? })\` — wipes the worker's session for that PR (kills container, deletes session row). \`repo\` defaults to the host's configured default repo (PR_FACTORY_DEFAULT_REPO). -- \`mcp__nanoclaw__retrigger({ pr_number, repo? })\` — re-fetches the PR diff fresh from GitHub, re-bootstraps the session, wakes the worker. After any skill edit, **always** clear + retrigger affected PRs in one step. - \`mcp__nanoclaw__propose_skill_edit({ skill_name, file_name, content })\` — propose a skill file edit. Read the current file from \`/app/skills/\` first, then pass the full new content. The host posts the diff for human approval — the file is only written if approved. **Always use this tool to edit skills — never write to the filesystem directly.** +Skill edits apply to the **next** PR each affected worker session triages — running sessions keep their old read-only skill view until they next spawn. Tell the human the edit lands going forward; there is no force-rerun of an in-flight session. + ## Two workflows ### A — Quick fix in a PR thread 1. Read the thread (already accumulated). Identify what went wrong. 2. Propose the change. Use \`propose_skill_edit\` — the host posts the diff and the human approves or rejects. -3. On approval, clear + retrigger the PR. Tell the human what changed. +3. On approval, tell the human what changed and that it applies to the next PR the worker triages. ### B — Batch review in admin channel @@ -47,7 +47,7 @@ The PR number is visible in every worker message (e.g. "PR #2318" in the triage **Suggested fix:** \`\`\` 2. **Review**: when the human asks you in admin channel, walk them through the collected feedback, propose skill diffs (don't apply yet), iterate. -3. **Implement**: on approval — use \`propose_skill_edit\` for each file, then clear + retrigger every affected PR. +3. **Implement**: on approval — use \`propose_skill_edit\` for each file. The edits apply to subsequent PRs going forward. ## Where things are @@ -62,5 +62,5 @@ The PR number is visible in every worker message (e.g. "PR #2318" in the triage - **Patterns over one-offs** — fix the skill, not the individual PR. - **Evidence first** — quote the worker's actual output before proposing a fix. - **Human approves** — propose, don't apply. -- **Always finish the loop** — after editing a skill, immediately clear + retrigger every affected PR. Never leave it to the human to remember. +- **Edits apply going forward** — a skill edit changes how the worker triages the next PR; it does not re-run PRs already in flight. `;