Skip to content

fix(agents): repair SSH worker publication, starts and reads - #3307

Open
teemow wants to merge 9 commits into
mainfrom
beekeeper-execution/70a00f80-86c3-479f-b27a-7f2192f5fb1b/1
Open

teemow wants to merge 9 commits into
mainfrom
beekeeper-execution/70a00f80-86c3-479f-b27a-7f2192f5fb1b/1

Conversation

@teemow

@teemow teemow commented Nov 6, 2026

Copy link
Copy Markdown
Member

What this does

Fixes what still fails for workers on SSH execution hosts, in four commits that can be read one at a time.

Publication (fix(publish): settle creates and update pull requests)

  • A republication with --title or --body-file now updates the existing pull request and lists the changed fields in updated. A flag left out keeps the field, an identical retry writes nothing, and a failed update is an error rather than silence.
  • A failed gh pr create settles by the owned branch. A pull request found there is returned. If none is found, the error says GitHub created none and why, through a fixed diagnosis rather than raw output. Only an unreadable branch stays "uncertain".
  • Creating and that read have their own time. The 20-second attempt could run out on the transfer, right before the create, and an identical retry would then spend it the same way. This is the likely cause of the repeated "outcome uncertain", but it is inferred from the code, not observed.
  • Prepared workspaces carry a commit identity when the execution user has none, and --sync-base falls back to it for workspaces prepared earlier.
  • Publication Git starts no detached maintenance. That process outlived the command and is the likely cause of the TempDir cleanup flake, which never reproduced locally, so the fix is unproven against the flake itself.

Askpass (fix(secret): bind the askpass socket under a deep TMPDIR)

  • The test and production paths both fall back to /tmp when TMPDIR is too deep for a Unix socket.

Remote starts (fix(agents): reconcile remote starts that time out)

  • A failed activation reads the host's journal in time of its own and says what it left: a process that came up is a success, a prepared execution with no process is named, and an unanswered check stays unknown.
  • The same start (name, task, host) activates that execution instead of failing on the reserved name, ahead of admission because it already holds its slot.
  • The host sync stops an execution its host still holds prepared 15 minutes after its start was sent, so it no longer holds a slot or drains the host. The workspace stays.

Worker reads (fix(agents): give SSH workers reads the coordinator owns)

  • budget needs a live execution, not a scoped credential. Refusals name their reason, and a missing grant is no longer called "revoked".
  • agents hosts check answers for a worker's reads: without github.workerApp it follows the host's own gh login, and a logged-out host fails the check and every start. An older host, which does not report a login, keeps working until it updates.
  • beekeeper log works for a managed worker: its flags go to the coordinator, which answers with at most 256 KiB of the newest events. This gives a post-merge proof a channel.
  • note add no longer reads a log request as a review ask because it names a reviewer.
  • The execution guard passes a gh api graphql read whose query and variables are all literal and hold no mutation. Files, dynamic words, -X and repeated query fields stay refused.
  • A CI park gets a deadline, merge.ciTimeout or its per-repository value, so CI that never settles still wakes the worker.

For the reviewer

  • New authority: beekeeper log lets a managed worker read the coordinator's event log. It is read-only and bounded, like the existing roster relay, but it is a wider read than before. status, board and supervisor status stay refused.
  • Behaviour change: a lost activation reply whose process is running now counts as a successful start, not an error to recover by hand. Nothing activates twice. TestExecutionStartThroughHarnessRegistersBeforeSSHAndRecoversLostReply changed for this.
  • Behaviour change: the sync now stops an aged reservation whose host holds a prepared journal; it used to stay forever. TestExecutionSyncReleasesStaleUnacknowledgedReservation changed for this.
  • Lock time: a failing create can hold the state lock past the 20-second attempt, up to 50 seconds in the worst case.

Not covered

  • Read-only GraphQL through the worker App's gh wrapper stays refused, as documented. The guard change applies to hosts on their own login.
  • Running budget by hand over SSH outside a worker still reports gh executable not found.
  • The state lock is busy failures noted in Supervisor rework fails on remote starts: execution host deadline exceeded, activation outcome unconfirmed #3202 are a separate cause and are not changed here.
  • gh issue view --json projectItems still answers [] for another owner's board. That is GitHub's answer; the hint and the docs now say it proves nothing.

Testing

Focused runs on an execution host: ./internal/execution, ./internal/secret, ./internal/guard, and ./cmd filtered to executions, publication, starts, workers and notes. All pass. The full suite and lint are left to CI. None of this has been proven live on a rolled release.

Written from one execution, which owns one branch, so these fixes share one pull request rather than two batches. Overlap: #2339 also changes internal/execution/retire.go, internal/execution/server.go and docs/ssh-execution-hosts-operations.md. Here those are one identity constant, one capability field and separate doc paragraphs, so whichever merges second has at most a small conflict.

Closes #2242
Closes #3192
Closes #3194
Closes #3195
Closes #3196
Closes #3197
Closes #3198
Closes #3199
Closes #3202
Closes #3211
Closes #3275

🤖 Generated with Claude Code

beekeeper and others added 5 commits November 6, 2026 10:27
A republication with --title or --body-file updates the existing pull request and names what changed. A lost or failed gh pr create settles by the owned branch, with time of its own, so an identical retry no longer ends the same way. Prepared workspaces carry a commit identity, and publication Git starts no detached maintenance.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A TMPDIR too deep for a Unix socket falls back to /tmp, in production and in the test that failed under a build slot.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed activation reads the host's journal and says whether the execution exists. The same start activates it instead of failing on the reserved name, and the host sync stops one nobody retried.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The budget needs a live execution, not a scoped credential, and access refusals name their reason. hosts check answers for a worker's GitHub reads. Workers read the coordinator's event log, the guard passes literal GraphQL reads, CI parks get a deadline, and a log request naming a reviewer is no review ask.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@teemow
teemow requested a review from a team as a code owner November 6, 2026 09:28
@teemow

teemow commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Desk review of exact head 9cf13033f4a702fac7b054f55a1e2e0052ec190c: changes needed. This is an advisory review alongside CI, not a merge authorization. If already merged, these findings need follow-up fixes.

Findings:

  1. P1 — Preserve the execution lease deadline for PR writes. cmd/executionpublish.go:271 uses context.WithoutCancel(ctx) for gh pr edit. The incoming context was deliberately bounded by r.Lease.ExpiresAt in publishExecutionWithMode. If the lease expires during the push, the edit still starts with a fresh 15 seconds and no authority recheck. Even when authority is valid when gh starts, both this edit and the new create context at line 524 can continue writing after lease expiry. Give GitHub calls their own attempt budget while retaining the lease deadline; only the read-only settlement should outlive it. The existing lease-after-fetch test does not cover expiry during push/edit/create.

  2. P2 — An explicitly empty body is silently ignored. cmd/executionpublish.go:260 skips edits when TrimSpace(p.Body) is empty. Publishing with --body-file pointing to an empty or whitespace-only file therefore succeeds with no update or warning and retains the old body. The wire request cannot distinguish an omitted body flag from an explicitly empty body. Carry field presence separately, then apply the supplied body (or explicitly refuse it). This leaves a case of agents publish --body-file silently keeps the old PR body on republication #3195 unresolved.

  3. P2 — Successful activation reconciliation can still return the expired-context failure. cmd/executionstart.go:432 can successfully reconcile using activationOutcome's independent context, but discards that observation. At line 437 the acceptance loop derives its context from the original caller again. When that caller deadline expired during activation, the very next Inspect fails immediately despite reconciliation already confirming a ready running process. Use the reconciled observation, or an independently bounded acceptance context, to complete the reconciliation. The lost-reply test injects an error without expiring the caller context, so it misses this case. This leaves part of Supervisor rework fails on remote starts: execution host deadline exceeded, activation outcome unconfirmed #3202 unresolved.

Issue closure assessment (implementation at this head, not a claim of a rolled/live proof):

Closing issue Truly done?
#2242 Implemented: coordinator-owned CI park/wake already exists; this head adds the configured deadline. Existing tests cover success/red results and replay; the new test checks the deadline.
#3192 Implemented for managed workers: budget no longer needs a scoped grant; known logged-out hosts fail checks/starts; refusal reasons distinguish absent grants. The manual SSH command outside a worker remains excluded, as the PR states.
#3194 Implemented: prepared repositories get fallback identity, and base merge supports older workspaces.
#3195 Incomplete: normal body changes work, but explicitly empty bodies still silently retain old content (finding 2).
#3196 Mitigated, not proven resolved: detached maintenance is disabled in publication Git, including fixture operations, but the reported flake was never reproduced.
#3197 Reconciliation and sanitized diagnosis implemented; separate create/settlement time budgets address the inferred cause. The original repeated failure is not proven resolved live, and the new write contexts need finding 1 fixed.
#3198 Implemented: authenticated managed workers get coordinator log reads, and log requests bypass the review-word misclassification. The wider coordinator event-log read is explicit in the PR; no claim that it proves a deployment by itself.
#3199 Partial: literal read queries pass the execution guard on hosts using their own login; the scoped worker-App wrapper still refuses GraphQL. Empty projectItems is correctly documented as inconclusive.
#3202 Partial: same-start reconciliation and aged prepared-reservation cleanup are implemented. Finding 3 remains; the separately reported busy-state-lock cause is explicitly unchanged.
#3211 Implemented: both production and the peer test use short-path socket fallback, with a deep-TMPDIR regression test.
#3275 Implemented: supplied changed nonempty titles edit the existing PR and failures are surfaced.

Validation: exact-head worktree; source review of the diff and affected surrounding paths; read every closing issue and its comments. Focused existing tests passed: go test ./cmd -run 'TestExecution(StartThroughHarnessRegistersBeforeSSHAndRecoversLostReply|Log|CheckAnswersForWorkerGitHubReads)' -count=1 through beekeeper run. These tests do not reproduce findings 1–3. No full-suite or live deployment proof was performed, and no source changes, pushes, or merges were made.

beekeeper and others added 3 commits November 6, 2026 11:34
PR writes recheck authority and end with the lease; only the settling read outlives it. An empty --body-file or --title is refused. A start the host confirms completes even when the caller's deadline failed it. The worker App wrapper shares the guard's literal GraphQL read classifier, and goconst findings are fixed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gh pr edit reads review requests, which an App token may not reach, and fails the edit with them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@teemow

teemow commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

What this does

Fixes what still fails for workers on SSH execution hosts, in four commits that can be read one at a time.

Publication (fix(publish): settle creates and update pull requests)

  • A republication with --title or --body-file now updates the existing pull request and lists the changed fields in updated. A flag left out keeps the field, an identical retry writes nothing, and a failed update is an error rather than silence.
  • A failed gh pr create settles by the owned branch. A pull request found there is returned. If none is found, the error says GitHub created none and why, through a fixed diagnosis rather than raw output. Only an unreadable branch stays "uncertain".
  • Creating, updating and that read have their own time. The 20-second attempt could run out on the transfer, right before the create, and an identical retry would then spend it the same way. This is the likely cause of the repeated "outcome uncertain", but it is inferred from the code, not observed.
  • A write rechecks authority first and ends with the lease; only the read-only settlement outlives it.
  • An empty --body-file or --title is refused on the host, so it cannot look like a flag left out.
  • Prepared workspaces carry a commit identity when the execution user has none, and --sync-base falls back to it for workspaces prepared earlier.
  • Publication Git starts no detached maintenance. That process outlives the command and is the likely cause of the TempDir cleanup flake. The flake never reproduced, so this is a mitigation, and its issue stays open.

Askpass (fix(secret): bind the askpass socket under a deep TMPDIR)

  • The test and production paths both fall back to /tmp when TMPDIR is too deep for a Unix socket.

Remote starts (fix(agents): reconcile remote starts that time out)

  • A failed activation reads the host's journal in time of its own and says what it left: a process that came up is a success, a prepared execution with no process is named, and an unanswered check stays unknown. A start the host confirms completes even when the caller's own deadline is what failed.
  • The same start (name, task, host) activates that execution instead of failing on the reserved name, ahead of admission because it already holds its slot.
  • The host sync stops an execution its host still holds prepared 15 minutes after its start was sent, so it no longer holds a slot or drains the host. The workspace stays.

Worker reads (fix(agents): give SSH workers reads the coordinator owns)

  • budget needs a live execution, not a scoped credential. Refusals name their reason, and a missing grant is no longer called "revoked".
  • agents hosts check answers for a worker's reads: without github.workerApp it follows the host's own gh login, and a logged-out host fails the check and every start. An older host, which does not report a login, keeps working until it updates.
  • beekeeper log works for a managed worker: its flags go to the coordinator, which answers with at most 256 KiB of the newest events. This gives a post-merge proof a channel.
  • note add no longer reads a log request as a review ask because it names a reviewer.
  • The execution guard and the worker App's gh wrapper share one classifier (github.GraphQLRead): a gh api graphql read passes when its query and variables are all literal and hold no mutation. Files, dynamic words, -X, -H and repeated query fields stay refused.
  • A CI park gets a deadline, merge.ciTimeout or its per-repository value, so CI that never settles still wakes the worker.

For the reviewer

  • Policy change: the wrapper's rule that only GET and HEAD reach the shared token now has one exception, the literal GraphQL read above. A test used to assert the refusal; it now asserts the read passes and writes, files and method overrides stay refused.
  • New authority: beekeeper log lets a managed worker read the coordinator's event log. It is read-only and bounded, like the existing roster relay, but it is a wider read than before. status, board and supervisor status stay refused.
  • Behaviour change: a lost activation reply whose process is running now counts as a successful start, not an error to recover by hand. Nothing activates twice. TestExecutionStartThroughHarnessRegistersBeforeSSHAndRecoversLostReply changed for this.
  • Behaviour change: the sync now stops an aged reservation whose host holds a prepared journal; it used to stay forever. TestExecutionSyncReleasesStaleUnacknowledgedReservation changed for this.
  • Lock time: a failing create can hold the state lock past the 20-second attempt, up to 50 seconds in the worst case.
  • Refused input: a republication with an empty --body-file now fails rather than clearing the body. Clearing a body stays unsupported.

Not covered

Testing

Focused runs on an execution host: ./internal/execution, ./internal/secret, ./internal/guard, and ./cmd filtered to executions, publication, starts, workers and notes. All pass. The full suite and lint are left to CI. None of this has been proven live on a rolled release.

Written from one execution, which owns one branch, so these fixes share one pull request rather than two batches. Overlap: #2339 also changes internal/execution/retire.go, internal/execution/server.go and docs/ssh-execution-hosts-operations.md. Here those are one identity constant, one capability field and separate doc paragraphs, so whichever merges second has at most a small conflict.

Refs #3196
Refs #3197

Closes #2242
Closes #3192
Closes #3194
Closes #3195
Closes #3198
Closes #3199
Closes #3202
Closes #3211
Closes #3275

🤖 Generated with Claude Code

goconst counted the GraphQL read classifier's literals with the retry reader's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment