diff --git a/CHANGELOG.md b/CHANGELOG.md index 40a27698..325cc7b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Changed +- **A run that did not finish opens no pull request (AGT-4786).** AGT-4664 published failed, rejected, `infra_error` and `rate_limited` runs as drafts so their work was visible. On 2026-10-20 that produced 25 PRs on cgf-portal in one night: 19 said "published because this run ended as `infra_error`", several were +3/-0 against multi-defect issues, every one was titled with the whole issue and closed it with `Closes`, and the owner closed all 25 unmerged. An infrastructure failure is not a deliverable. `publishFinishedRun` now takes only the reviewed path and the operator-park path; `publishUnfinishedWork` and `shouldPublishUnfinishedWork` are removed, and the call site no longer reads `opts.lifecycleFailed` / `opts.failureSummary`. **The work is not lost:** `preserveWorktree` WIP-commits dirty work onto the swarm branch and keeps the tree (INT-2729), `createWorktree` reuses that branch, and the failure itself is already commented on the issue by the worker audit path. **Not done yet:** a failed run's branch is not pushed to the remote, because the secret-leak guard that must precede any push (`assertNoSensitiveDataOnBranch`, AGT-4188) lives inside `commitAndCreatePRWithHead` (`worktreeManager.ts`, 1484 of 1500 lines, five callers that destructure `prUrl`) β€” pushing through a second code path would bypass it. `summarizeFailure` is kept, tested, for the comment that follow-up will write. + - **`idle_fill` no longer lifts a park or backoff on an issue nobody has touched (AGT-4787).** On 2026-10-20 one issue was dispatched 18 times in 14 hours and 52 were touched, mostly after an `infra_error` or a "parked for operator decision" that retrying cannot change. `src/automation/dispatchFingerprint.ts` stores, in its own table (`automation_parked_inputs`), a digest of the title and description a run started on (`recordRunInput`, called where the pipeline starts, so every later park or backoff β€” wherever in the ledger it happens β€” is measured against it). `filterAlreadyProcessed` skips the NEEDS_HUMAN resume and the RETRY_AT lift while the issue still matches that digest; an edit to either field lifts them as before. **The key is not `trackerUpdatedAt`, and not a comment count:** both bump on the daemon's own progress comments and state changes (AGT-4300 measured that key driving `attempt_no` to 40 on an unchanged issue), so a fingerprint on them would change after every comment the daemon posts and the block would never hold. Reads fail open (an unreadable store degrades to today's behaviour), the run-start write never stops a run. **Known gap:** tracker comments are not on a listed task, so an answer given *only* as a comment does not lift a held park; an explicit dispatch or a Todo transition does. The module already accepts comments and recognises the daemon's own signatures (`🧭 **[OpenSwarm] …`, `_via OpenSwarm_`), so wiring them in is the follow-up. - **The daemon refuses to start when unattended work is routed to a delegated CLI adapter (AGT-4444).** `claude`, `codex` and `cursor` hand the whole tool loop to another program, so OpenSwarm's publication fence, protected files, role tool limits and (since AGT-4783/4798) the credential-free, loopback-only shell never reach them; `base.ts` could only log that they were dropped, and `claude` runs with `--permission-mode bypassPermissions`. `src/adapters/unattendedPolicy.ts` now checks every place the daemon can pick an adapter β€” the top-level `adapter`, `autonomous.defaultRoles`, per-project `roles`, `adapterRouting` primary **and fallbacks** (whose schema default is `['cc-router', 'cursor']`, so a bare `adapterRouting:` block already routed to cursor), `orchestrator` and `loopAdvisor` β€” and startup throws naming each offending setting. A role switched off is not checked. Opt out with `autonomous.allowDelegatedAdapters: true`. Interactive CLI use is unaffected: the check runs only when an `autonomous` section exists. Periodic read-only reviews (`periodicReviews[].adapter`) are not covered. diff --git a/src/automation/publishOnPark.test.ts b/src/automation/publishOnPark.test.ts index 8d47f435..40d94d86 100644 --- a/src/automation/publishOnPark.test.ts +++ b/src/automation/publishOnPark.test.ts @@ -6,7 +6,7 @@ vi.mock('../support/worktreeManager.js', () => ({ commitAndCreatePRWithHead })); vi.mock('../core/eventHub.js', () => ({ broadcastEvent: vi.fn() })); import { PublicationScopeMismatchError } from '../support/publicationScopeFence.js'; -import { PUBLICATION_SCOPE_PARK_REASON, WORKER_NO_CHANGES_PARK_REASON, publishApprovedWork, publishFinishedRun, publishParkedIfNeeded, publishParkedWork, publishStuckWork, publishUnfinishedWork, shouldPublishParkedWork, shouldPublishUnfinishedWork, summarizeFailure } from './publishOnPark.js'; +import { PUBLICATION_SCOPE_PARK_REASON, WORKER_NO_CHANGES_PARK_REASON, publishApprovedWork, publishFinishedRun, publishParkedIfNeeded, publishParkedWork, publishStuckWork, shouldPublishParkedWork, summarizeFailure } from './publishOnPark.js'; beforeEach(() => { commitAndCreatePRWithHead.mockReset(); @@ -412,102 +412,25 @@ describe('publishStuckWork β€” terminal parks publish instead of holding', () => }); }); -describe('unfinished-run publication (AGT-4664)', () => { +describe('an unfinished run opens no pull request (AGT-4786)', () => { const info = { worktreePath: '/tmp/w', originalPath: '/tmp/r', branchName: 'swarm/AX-1774', issueId: 'AX-1774' }; const task = { id: 'task-1', issueIdentifier: 'AX-1774', title: 'Reconcile ledger', fileScope: ['src/'], fileScopeSource: 'drafted' as const }; const pr = { prUrl: 'https://github.com/o/r/pull/31', headSha: 'abc' }; - describe('shouldPublishUnfinishedWork', () => { - it.each(['failed', 'rejected', 'infra_error', 'rate_limited'])('publishes a %s run that has a worktree', (finalStatus) => { - expect(shouldPublishUnfinishedWork(true, { success: false, finalStatus })).toBe(true); - }); - - it.each(['cancelled', 'superseded', 'decomposed', 'deferred', 'approved', 'waiting_on_operator', undefined])( - 'does not publish a %s run', - (finalStatus) => { - expect(shouldPublishUnfinishedWork(true, { finalStatus })).toBe(false); - }, - ); - - it('does not publish without a worktree', () => { - expect(shouldPublishUnfinishedWork(false, { finalStatus: 'failed' })).toBe(false); - }); - - it('leaves an operator park to the parked path', () => { - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'failed', operatorPark: { code: 'worker_no_changes', reason: 'r' } })).toBe(false); - }); - - it('does not publish a sandbox outcome it cannot trust or a run that already has a PR', () => { - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'failed', workerResult: { executionOutcomeUnknown: true } })).toBe(false); - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'failed', testerResult: { executionOutcomeUnknown: true } })).toBe(false); - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'failed', reviewResult: { executionOutcomeUnknown: true } })).toBe(false); - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'failed', prUrl: 'https://x/1' })).toBe(false); - }); - - it('does not publish for a lifecycle-fence failure or a run that was approved going in', () => { - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'infra_error' }, { lifecycleFailed: true })).toBe(false); - expect(shouldPublishUnfinishedWork(true, { finalStatus: 'infra_error' }, { approvedAttempt: true })).toBe(false); - }); - }); - - describe('publishUnfinishedWork', () => { - it('opens a draft with the status and last failure, without attaching it to the ledger', async () => { - commitAndCreatePRWithHead.mockResolvedValue(pr); - const onPublication = vi.fn(async () => true); - const durability = { beforePublish: vi.fn(async () => true), onPublication } as unknown as ExecutionDurabilityHooks; - - const url = await publishUnfinishedWork(info, task, { finalStatus: 'rejected', failureDetail: 'reviewer: tests missing\nfor the new branch' }, durability); - - expect(url).toBe(pr.prUrl); - expect(commitAndCreatePRWithHead).toHaveBeenCalledWith( - info, task.title, 'AX-1774', - expect.stringMatching(/ended as `rejected`[\s\S]*Last failure: reviewer: tests missing for the new branch/), - { draft: true }, - ); - // No PR on the ledger row: a non-approved result with one becomes NEEDS_RECONCILE and stops the retry. - expect(onPublication).not.toHaveBeenCalled(); - }); - - it('bounds the failure text and survives a missing one', async () => { - commitAndCreatePRWithHead.mockResolvedValue(pr); - - await publishUnfinishedWork(info, task, { finalStatus: 'failed', failureDetail: 'x'.repeat(2_000) }, undefined); - await publishUnfinishedWork(info, task, { finalStatus: 'failed' }, undefined); - - const bodies = commitAndCreatePRWithHead.mock.calls.map((call) => String(call[3])); - expect(bodies[0].length).toBeLessThan(900); - expect(bodies[1]).toContain('no failure detail was recorded'); - }); - - it('publishes nothing when the lease fence refuses', async () => { - const durability = { beforePublish: vi.fn(async () => false), onPublication: vi.fn() } as unknown as ExecutionDurabilityHooks; - - expect(await publishUnfinishedWork(info, task, { finalStatus: 'failed' }, durability)).toBeUndefined(); - expect(commitAndCreatePRWithHead).not.toHaveBeenCalled(); - }); - - it('swallows a branch with nothing on it and any publication failure', async () => { - commitAndCreatePRWithHead.mockRejectedValueOnce(new Error('No commits to create PR from - branch has no changes compared to main')); - expect(await publishUnfinishedWork(info, task, { finalStatus: 'failed' }, undefined)).toBeUndefined(); - - commitAndCreatePRWithHead.mockRejectedValueOnce(new Error('gh: HTTP 502')); - expect(await publishUnfinishedWork(info, task, { finalStatus: 'failed' }, undefined)).toBeUndefined(); - }); - }); - describe('publishFinishedRun', () => { const noHook = () => undefined; - it('publishes a failed run once, as a draft, and leaves its result alone', async () => { + // 2026-10-20: 25 draft PRs in one night, 19 "published because this run ended as + // infra_error". Every unfinished status is covered, with and without a worktree + // change on it, and the result is left exactly as it was. + it.each(['failed', 'rejected', 'infra_error', 'rate_limited'])('opens no pull request for a %s run and leaves its result alone', async (finalStatus) => { commitAndCreatePRWithHead.mockResolvedValue(pr); - const result: { success: boolean; finalStatus: string; prUrl?: string; failureDetail?: string } = { success: false, finalStatus: 'failed', failureDetail: 'tests red' }; + const result: { success: boolean; finalStatus: string; prUrl?: string; failureDetail?: string } = { success: false, finalStatus, failureDetail: 'tests red' }; - await publishFinishedRun(info, task, result, undefined, noHook); + await publishFinishedRun(info, task, result, undefined, noHook, undefined, { failureSummary: 'reviewer: the new route has no test' }); - expect(commitAndCreatePRWithHead).toHaveBeenCalledTimes(1); - expect(commitAndCreatePRWithHead.mock.calls[0][4]).toEqual({ draft: true }); - // Setting prUrl here would turn the retry into NEEDS_RECONCILE. - expect(result).toEqual({ success: false, finalStatus: 'failed', failureDetail: 'tests red' }); + expect(commitAndCreatePRWithHead).not.toHaveBeenCalled(); + expect(result).toEqual({ success: false, finalStatus, failureDetail: 'tests red' }); }); it('publishes an approved run through the reviewed path only', async () => { @@ -557,16 +480,6 @@ describe('unfinished-run publication (AGT-4664)', () => { expect(commitAndCreatePRWithHead).toHaveBeenCalledTimes(1); }); - it('puts the ledger-style failure summary in the draft body when the result carries none', async () => { - commitAndCreatePRWithHead.mockResolvedValue(pr); - // A reviewer-rejection stall sets no failureDetail on the result. - const result = { success: false, finalStatus: 'failed' }; - - await publishFinishedRun(info, task, result, undefined, noHook, undefined, { failureSummary: 'reviewer: the new route has no test' }); - - expect(String(commitAndCreatePRWithHead.mock.calls[0][3])).toContain('Last failure: reviewer: the new route has no test'); - }); - it('publishes nothing for a lifecycle-fence failure', async () => { const result = { success: false, finalStatus: 'infra_error', failureDetail: 'lease lost' }; @@ -587,14 +500,14 @@ describe('unfinished-run publication (AGT-4664)', () => { expect(commitAndCreatePRWithHead).not.toHaveBeenCalled(); }); - it('runs no post-publication review for the unfinished draft', async () => { + it('runs no post-publication review for an unfinished run either', async () => { commitAndCreatePRWithHead.mockResolvedValue(pr); const hook = vi.fn(async () => {}); await publishFinishedRun(info, task, { success: false, finalStatus: 'failed' }, undefined, () => hook); // A review per failed attempt is one LLM call each, and on rate_limited it only hits the limit again. - expect(commitAndCreatePRWithHead).toHaveBeenCalledTimes(1); + expect(commitAndCreatePRWithHead).not.toHaveBeenCalled(); expect(hook).not.toHaveBeenCalled(); }); }); diff --git a/src/automation/publishOnPark.ts b/src/automation/publishOnPark.ts index 20b7118f..6040fca1 100644 --- a/src/automation/publishOnPark.ts +++ b/src/automation/publishOnPark.ts @@ -2,18 +2,23 @@ // OpenSwarm β€” publishing a finished run's branch (AGT-4076) // ============================================ // -// Both publication paths a worktree-mode run can take, kept together because +// The publication paths a worktree-mode run can take, kept together because // they share `commitAndCreatePR` and differ only in what the run earned: // // - `publishApprovedWork` β€” the reviewed path. A publication failure is fatal: // a run is not deliverable until its branch is reviewable. // - `publishParkedWork` β€” a run that stopped for an operator decision. Draft, // and a failure never changes the park. -// - `publishUnfinishedWork` β€” a run that failed, was rejected, hit a rate limit -// or an infrastructure fault but left changes behind. Draft, and a failure -// never changes the outcome (AGT-4664). +// A run that failed, was rejected, hit a rate limit or an infrastructure fault +// opens NO pull request (AGT-4786). AGT-4664 published those as drafts so their +// work was visible; on 2026-10-20 that produced 25 PRs in one night, 19 of them +// "published because this run ended as infra_error", several +3/-0 against +// multi-defect issues and all titled with the whole issue plus `Closes`. An +// infrastructure failure is not a deliverable. The work is not lost: +// `preserveWorktree` WIP-commits it onto the swarm branch and keeps the tree, +// and the retry resumes from that branch. // -// `publishFinishedRun` runs all three in the order a run can need them. +// `publishFinishedRun` runs the two that remain in the order a run can need them. // // Split out of runnerExecution.ts, which sits on the 1500-line pre-commit cap. @@ -424,42 +429,6 @@ export async function publishApprovedWork( } } -/** Terminal statuses of a run that stopped short of an approved review. */ -const UNFINISHED_STATUSES: ReadonlySet = new Set(['failed', 'rejected', 'infra_error', 'rate_limited']); - -/** - * Should a run that did not finish still publish what it built (AGT-4664)? - * - * True for the four outcomes where a worker may have left real changes behind: - * failed, rejected, infra_error and rate_limited. Measured on the deployed - * daemon on 2026-10-03: 48 worker stages, 32 failed, zero pull requests, and a - * tree per run piling up on disk β€” a failed run's work was only ever kept as a - * dirty directory. - * - * Not for anything else, and each exclusion is deliberate: - * - `cancelled`, `superseded`, `decomposed`, `deferred`: the run was told to - * stop or was replaced, so its tree is not a deliverable. - * - an `operatorPark` / `waiting_on_operator`: {@link shouldPublishParkedWork} - * already publishes those, with its own review hook. - * - a run that went into the approved publish as approved - * (`approvedAttempt`): a push or `gh` error there turns it into an - * `infra_error`, and republishing reviewed work as a draft would trade the - * retry that fixes it for a PR that hides it. The reviewed retry owns it. - * - a lifecycle-fence failure (`lifecycleFailed`): this executor no longer - * speaks for the run, and the runner's own comment says a failed fence must - * prevent publication. - * - a result that already carries a `prUrl`. - */ -export function shouldPublishUnfinishedWork( - hasWorktree: boolean, - result: PublishableResult, - opts: { approvedAttempt?: boolean; lifecycleFailed?: boolean } = {}, -): boolean { - if (!hasWorktree || opts.approvedAttempt || opts.lifecycleFailed) return false; - if (result.prUrl || result.operatorPark || pipelineExecutionBoundary(result)?.executionOutcomeUnknown === true) return false; - return UNFINISHED_STATUSES.has(result.finalStatus ?? ''); -} - // A pytest progress row is only result marks and a percentage. The first 500 // characters of a failing run were all of them, so a draft PR said nothing about // what failed (AGT-4672: #796, #798); the failing ids and totals come last. @@ -468,7 +437,7 @@ const PERCENT_MARK = /\s\[\s*\d{1,3}%\]/g; const SUMMARY_CAP = 500; const SUMMARY_HEAD = 150; -/** One line of a failure detail, bounded, for the draft PR's body. */ +/** One line of a failure detail, bounded. Built for a draft PR body (AGT-4672); no caller since AGT-4786. */ export function summarizeFailure(detail: string | undefined): string { const flat = (detail ?? '') .replace(/\s+/g, ' ') @@ -482,87 +451,27 @@ export function summarizeFailure(detail: string | undefined): string { return `${flat.slice(0, SUMMARY_HEAD)} … ${flat.slice(-(SUMMARY_CAP - SUMMARY_HEAD))}`; } -/** - * Publish the branch of a run that ended without an approved review, as a draft. - * - * The point is the same as {@link publishParkedWork}'s β€” work that exists - * should be visible β€” and the consequence is the one the operator asked for - * ("PR 올리면 μ›Œν¬νŠΈλ¦¬λ₯Ό μ—†μ• κ²Œ"): `commitAndCreatePRWithHead` commits whatever - * is in the tree, so afterwards the tree is clean and `preserveWorktree` removes - * it. The branch stays; a retry resumes from it (`createWorktree` reuses an - * existing branch), and a later approved publication finds this PR by branch - * name and promotes it out of draft. - * - * What it deliberately does NOT do: - * - It does not set `result.prUrl` or attach the PR to the ledger. - * `durableRunCoordinator.execute()` turns any non-approved result carrying a - * prUrl into `publication_reconcile` / NEEDS_RECONCILE, which would stop the - * retry this run is owed β€” the phantom-row shape that idled the loop on - * 2026-08-29. - * - It runs no review hook. A reviewer pass per failed attempt costs an LLM - * call each time, and on `rate_limited` it would only hit the limit again. - * - It does not enforce the write scope, for the reason {@link publishParkedWork} - * documents. - * - * Returns the PR URL, or undefined when nothing was published. Every failure is - * swallowed: the run's outcome and its retry budget are not this function's to - * change, and a tree it could not publish is committed locally either way. - */ -export async function publishUnfinishedWork( - worktreeInfo: WorktreeInfo, - task: PublishableTask, - result: Pick & Pick, - durability: ExecutionDurabilityHooks | undefined, -): Promise { - // The lease fence, as in the other two paths: an executor that lost its claim - // must not push for a run it no longer owns. - const publishAllowed = await durability?.beforePublish() ?? true; - if (!publishAllowed) { - console.warn(`[Runner] Unfinished-run publication fenced for ${task.issueIdentifier}; leaving the branch unpublished`); - return undefined; - } - try { - const { prUrl } = await commitAndCreatePRWithHead( - worktreeInfo, - task.title, - task.issueIdentifier || '', - `Published because this run ended as \`${result.finalStatus}\` before it was approved, so what it` - + ' built is visible instead of sitting in a worktree. It has not passed review and may be' - + ' incomplete β€” this PR is a draft on purpose.\n\n' - + `Last failure: ${summarizeFailure(result.failureDetail)}`, - { draft: true }, - ); - broadcastEvent({ - type: 'log', - data: { taskId: task.issueId || task.id, stage: 'pr', line: `Draft PR created for unfinished run (${result.finalStatus}): ${prUrl}` }, - }); - console.log(`[Runner] Unfinished run (${result.finalStatus}) published as draft for ${task.issueIdentifier}: ${prUrl}`); - return prUrl; - } catch (err) { - // "No commits to create PR from" is the common, correct outcome: the run - // failed before it edited anything. - const detail = err instanceof Error ? err.message : String(err); - if (!NO_COMMITS_TO_PUBLISH.test(detail)) { - console.warn(`[Runner] Could not publish unfinished work for ${task.issueIdentifier}: ${detail}`); - } - return undefined; - } -} - /** * Every publication a finished run can need, in the order it can need them. * * 1. parked before the approved publish (the pipeline set `operatorPark`); * 2. the approved publish itself; * 3. parked during it (the scope fence refused the push) β€” only if 1 did not - * already publish; - * 4. a run that simply did not finish ({@link shouldPublishUnfinishedWork}). + * already publish. + * + * A run that did not finish β€” failed, rejected, infra_error, rate_limited β€” + * takes none of these and opens no pull request (AGT-4786, see the file header). * - * A run takes exactly one of these. They are separate steps rather than one + * A run takes at most one of these. They are separate steps rather than one * decision because 1 and 3 depend on what 2 did to the result. * * `reviewHook` builds the post-publication review for 1 and 3; 2 gets the - * rolling-back variant and 4 gets none. + * rolling-back variant. + * + * `opts` is kept for the call site's sake and is no longer read: `lifecycleFailed` + * and `failureSummary` only fed the unfinished-run draft that this no longer opens. + * `summarizeFailure` stays as a tested pure function for the tracker comment the + * follow-up will write. */ export async function publishFinishedRun( worktreeInfo: WorktreeInfo | null | undefined, @@ -571,33 +480,13 @@ export async function publishFinishedRun( durability: ExecutionDurabilityHooks | undefined, reviewHook: (rollbackOnRejection: boolean) => ApprovedPublicationHook | undefined, verify?: VerifyConfig, - opts: { - lifecycleFailed?: boolean; - /** - * Why the run stopped, as the ledger would record it. `result.failureDetail` - * is set by only some failure paths β€” a reviewer-rejection stall leaves it - * empty β€” so a draft's body said "no failure detail was recorded" for a run - * the ledger could explain (cgf-portal AX-1635, 2026-10-03). - */ - failureSummary?: string; - } = {}, + // Not read (see above); typed so the call site keeps compiling. + _opts: { lifecycleFailed?: boolean; failureSummary?: string } = {}, ): Promise { const parkedPublished = await publishParkedIfNeeded(worktreeInfo, task, result, durability, reviewHook(false)); - // Read before the approved publish: a push or `gh` failure there rewrites - // `finalStatus` to `infra_error`, and by then this is no longer knowable. - const approvedAttempt = result.success === true && result.finalStatus === 'approved'; await publishApprovedWork(worktreeInfo, task, result, durability, reviewHook(true), verify); if (!parkedPublished) { await publishParkedIfNeeded(worktreeInfo, task, result, durability, reviewHook(false)); } - - if (worktreeInfo && shouldPublishUnfinishedWork(true, result, { approvedAttempt, lifecycleFailed: opts.lifecycleFailed })) { - await publishUnfinishedWork( - worktreeInfo, - task, - { finalStatus: result.finalStatus, failureDetail: opts.failureSummary ?? result.failureDetail }, - durability, - ); - } }