Skip to content

fix(publish): a run that did not finish opens no pull request (AGT-4786) #844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
unohee merged 1 commit into from
Oct 21, 2026
Merged

fix(publish): a run that did not finish opens no pull request (AGT-4786) #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
111 changes: 12 additions & 99 deletions src/automation/publishOnPark.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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' };

Expand All @@ -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();
});
});
Expand Down
Loading
Loading