webgrip/unfold
5
0
Fork 0
Code Issues 1 Pull requests 2 Projects Releases 54 Packages 8 Wiki Activity Actions

fix(ploeg): only the worker says whether a Run delivered a pull request #199

Closed
ryangr0 wants to merge 2 commits from ryangr0/ploeg-worker-owns-delivery into development AGit
pull from: ryangr0/ploeg-worker-owns-delivery
merge into: webgrip:development
Conversation 0 Commits 2 Files changed 23 +1033 -104
ryangr0 commented 2026-10-03 12:55:12 +00:00
Owner
Copy link

Stacked on #188. The first commit here is #188's commit, and this PR's own change is the second commit (abb2e2ea). Merge #188 first; after that, this PR shows only its own commit. First slice of VIK-1732 (handoff EXEC-02). Implements the worker side of ADR-0059, which is still proposed; see "Owner decisions" below.

Problem

An agent could decide whether its Run delivered, and where:

  • Drop box claims were kept. Claude Code and ACP merged the drop box's outcome, links, checkpoint and failureReason whenever the adapter concluded nothing. OpenHands and the exec adapter returned the file as the whole report. resolveOutcome then kept any valid structured outcome together with its own links. So a drop box with {"outcome":"pr_opened","links":[".../other/repo/pulls/123"]} reached ploegd as this Run's pull request; TestAWriterThatClaimsAPullRequestItNeverOpenedDeliversNothing reproduces this.
  • A failed forge read meant "no pull request". The error was logged and the Run carried on as if the forge had answered with nothing.
  • A fork's pull request counted as the Run's. A pull request from a fork whose branch had the same name was treated as the Run's own.
  • A replaced pull request read as an update. If the pull request was replaced during the Run, the new one counted as pr_updated.

Invariant

  • Delivery comes only from the worker. No adapter or agent report can set pr_opened, pr_updated, links, checkpoint, failureReason, verification or delivery. The worker derives them from its own forge reads before and after the harness.
  • Those reads are bound to this Run. The pull request's head must be the Run's branch, in the Work Item's own repository (not a fork), against the target's base branch.
  • A failed read is "unknown", never "no pull request".

Change

  • Drop box. harness.MergeDropBox keeps only what the agent may report: findings, verdict, problem, solution, proposed Work Items, and a non-delivery outcome with its summary and stuck reason. OpenHands, the exec adapter and Claude Code now all read the drop box through it; ACP already did. A new conformance property, DeliveryClaimsNeverSurviveTheAdapter, checks all four adapters.
  • resolveOutcome. Strips the same claims from whatever an adapter returns. A pr_* claim becomes "no structured outcome", so the forge decides.
  • Forge lookup.
    • It returns the pull request's number, head commit and base branch.
    • It skips pull requests whose head repository differs: Forgejo head.repo.full_name, GitLab source_project_id / target_project_id.
    • Each read is tried 3 times (0 s, 2 s, 5 s).
  • Failed reads.
    • Before the harness: a writer does not start. It ends failed/infra_node, before any key is minted or money spent. A reader keeps today's behaviour of warning and continuing.
    • After the harness: the delivery is unknown, and a writer ends stuck (through #188's guard).
  • Delivery record. A new optional delivery field on the outcome carries forge, repository, branch, number, URL, base, head, headBefore, and observed (opened | updated | none | unknown) with a reason. outcomereport.v1 gains it additively: ploegd's decoder is lenient, so older ploegd ignores it.
  • Docs. docs/concepts/inside-a-run.md and a dated note in ADR-0059 describe what is implemented.

Owner decisions this PR takes provisionally (ADR-0059 is proposed)

  • Failure reason before the harness. A failed read before the harness uses the existing infra_node, as the reviewer's fetch failure already does, not a new infra_forge. A failed read after the harness is stuck with no failure reason.
  • Agent links are always dropped. An agent cites URLs in its prose instead. This is the ADR's proposal.
  • Retry policy. 3 tries, 7 s in total.

Not in this slice (next slice of VIK-1732)

  • ploegd does not use delivery yet. It does not check delivery against the claimed Work Item, does not store it, and does not read it in pullRequest (publication), the review watch or readyForReview. A direct outcome API call with crafted links is therefore still trusted by ploegd.
  • pr_updated without a push. A clean exit on an already-open pull request is still pr_updated even when its head did not move. Fixing that needs ploegd to settle with delivery first, otherwise a uniform-plan item would settle done with its pull request still open.
  • Legacy link-only rows. Marking them, and the older-worker window.

Tests

  • Shown failing with the old claim handling. I restored the old MergeDropBox and resolveOutcome handling, ran these, then removed it again:
    • TestAWriterThatClaimsAPullRequestItNeverOpenedDeliversNothing, a real ACP agent writing the exploit drop box. The old handling reported pr_opened with .../other/repo/pulls/123 and failureReason: budget.
    • TestAnAgentCannotClaimDelivery, with 3 cases.
    • TestMergeDropBox_DeliveryClaimsNeverSurvive.
    • The conformance property, which failed for claudecode, openhands and execbin.
  • Also new:
    • fork pull and merge requests are excluded, for Forgejo and GitLab
    • the 3-try read
    • observedDelivery in all 6 states, including a pull request replaced during the Run
    • real pr_opened and pr_updated against a git-http-backend forge, with number, head and headBefore
    • a pre-harness read failure stops the writer, and the harness never runs
    • a post-harness read failure is unknown
    • schema accept and reject cases for delivery
  • Updated tests:
    • The verify tests no longer rely on the agent's pr_opened claim. Their fake agent now really pushes, and the fake forge lists the pull request.
    • "a structured report with no PR keeps its own links" now expects the links to be dropped.

Commands (at abb2e2ea, Go 1.27.1, Node 24.21.0)

  • mise exec -- go test ./pkg/worker/... ./pkg/harness/... -count=1 -v: 281 passed, 0 failed, 2 skipped (TestLiveCanaries, TestLiveClaudeCodeIgnoresTargetHooksAndMCPServers, both opt-in live tests)
  • mise exec -- go test ./... -count=1 in apps/ploeg: 34 packages ok
  • mise run verify: all gates passed (vloer 7, demo-replay 1, vloer-extension 4, ploeg 7, brand 1, site 5, site-demo 1, helm 15, release 3, integration 1, docs 1)

Refs VIK-1732

🤖 Generated with Claude Code

**Stacked on #188.** The first commit here is #188's commit, and this PR's own change is the second commit (`abb2e2ea`). Merge #188 first; after that, this PR shows only its own commit. First slice of VIK-1732 (handoff EXEC-02). Implements the worker side of [ADR-0059](apps/ploeg/docs/adrs/0059-delivery-facts-come-from-the-forge-never-from-the-agents-outcome.md), which is still **proposed**; see "Owner decisions" below. ## Problem An agent could decide whether its Run delivered, and where: - **Drop box claims were kept.** Claude Code and ACP merged the drop box's `outcome`, `links`, `checkpoint` and `failureReason` whenever the adapter concluded nothing. OpenHands and the exec adapter returned the file as the whole report. `resolveOutcome` then kept any valid structured outcome together with its own links. So a drop box with `{"outcome":"pr_opened","links":[".../other/repo/pulls/123"]}` reached ploegd as this Run's pull request; `TestAWriterThatClaimsAPullRequestItNeverOpenedDeliversNothing` reproduces this. - **A failed forge read meant "no pull request".** The error was logged and the Run carried on as if the forge had answered with nothing. - **A fork's pull request counted as the Run's.** A pull request from a fork whose branch had the same name was treated as the Run's own. - **A replaced pull request read as an update.** If the pull request was replaced during the Run, the new one counted as `pr_updated`. ## Invariant - **Delivery comes only from the worker.** No adapter or agent report can set `pr_opened`, `pr_updated`, `links`, `checkpoint`, `failureReason`, `verification` or `delivery`. The worker derives them from its own forge reads before and after the harness. - **Those reads are bound to this Run.** The pull request's head must be the Run's branch, in the Work Item's own repository (not a fork), against the target's base branch. - **A failed read is "unknown", never "no pull request".** ## Change - **Drop box.** `harness.MergeDropBox` keeps only what the agent may report: findings, verdict, problem, solution, proposed Work Items, and a non-delivery outcome with its summary and stuck reason. OpenHands, the exec adapter and Claude Code now all read the drop box through it; ACP already did. A new conformance property, `DeliveryClaimsNeverSurviveTheAdapter`, checks all four adapters. - **resolveOutcome.** Strips the same claims from whatever an adapter returns. A `pr_*` claim becomes "no structured outcome", so the forge decides. - **Forge lookup.** - It returns the pull request's number, head commit and base branch. - It skips pull requests whose head repository differs: Forgejo `head.repo.full_name`, GitLab `source_project_id` / `target_project_id`. - Each read is tried 3 times (0 s, 2 s, 5 s). - **Failed reads.** - Before the harness: a writer does not start. It ends `failed`/`infra_node`, before any key is minted or money spent. A reader keeps today's behaviour of warning and continuing. - After the harness: the delivery is `unknown`, and a writer ends `stuck` (through #188's guard). - **Delivery record.** A new optional `delivery` field on the outcome carries forge, repository, branch, number, URL, base, `head`, `headBefore`, and `observed` (`opened` | `updated` | `none` | `unknown`) with a reason. `outcomereport.v1` gains it additively: ploegd's decoder is lenient, so older ploegd ignores it. - **Docs.** `docs/concepts/inside-a-run.md` and a dated note in ADR-0059 describe what is implemented. ## Owner decisions this PR takes provisionally (ADR-0059 is proposed) - **Failure reason before the harness.** A failed read before the harness uses the existing `infra_node`, as the reviewer's fetch failure already does, not a new `infra_forge`. A failed read after the harness is `stuck` with no failure reason. - **Agent links are always dropped.** An agent cites URLs in its prose instead. This is the ADR's proposal. - **Retry policy.** 3 tries, 7 s in total. ## Not in this slice (next slice of VIK-1732) - **ploegd does not use `delivery` yet.** It does not check `delivery` against the claimed Work Item, does not store it, and does not read it in `pullRequest` (publication), the review watch or `readyForReview`. A direct outcome API call with crafted links is therefore still trusted by ploegd. - **`pr_updated` without a push.** A clean exit on an already-open pull request is still `pr_updated` even when its head did not move. Fixing that needs ploegd to settle with `delivery` first, otherwise a uniform-plan item would settle `done` with its pull request still open. - **Legacy link-only rows.** Marking them, and the older-worker window. ## Tests - **Shown failing with the old claim handling.** I restored the old `MergeDropBox` and `resolveOutcome` handling, ran these, then removed it again: - `TestAWriterThatClaimsAPullRequestItNeverOpenedDeliversNothing`, a real ACP agent writing the exploit drop box. The old handling reported `pr_opened` with `.../other/repo/pulls/123` and `failureReason: budget`. - `TestAnAgentCannotClaimDelivery`, with 3 cases. - `TestMergeDropBox_DeliveryClaimsNeverSurvive`. - The conformance property, which failed for claudecode, openhands and execbin. - **Also new:** - fork pull and merge requests are excluded, for Forgejo and GitLab - the 3-try read - `observedDelivery` in all 6 states, including a pull request replaced during the Run - real pr_opened and pr_updated against a git-http-backend forge, with number, head and headBefore - a pre-harness read failure stops the writer, and the harness never runs - a post-harness read failure is unknown - schema accept and reject cases for `delivery` - **Updated tests:** - The verify tests no longer rely on the agent's `pr_opened` claim. Their fake agent now really pushes, and the fake forge lists the pull request. - "a structured report with no PR keeps its own links" now expects the links to be dropped. ## Commands (at abb2e2ea, Go 1.27.1, Node 24.21.0) - `mise exec -- go test ./pkg/worker/... ./pkg/harness/... -count=1 -v`: 281 passed, 0 failed, 2 skipped (`TestLiveCanaries`, `TestLiveClaudeCodeIgnoresTargetHooksAndMCPServers`, both opt-in live tests) - `mise exec -- go test ./... -count=1` in apps/ploeg: 34 packages ok - `mise run verify`: all gates passed (vloer 7, demo-replay 1, vloer-extension 4, ploeg 7, brand 1, site 5, site-demo 1, helm 15, release 3, integration 1, docs 1) Refs VIK-1732 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
The unpublished-work guard from #177 compared a writer's checkout with
its starting commit, but only when the Run resolved to no_change_needed,
and it trusted an empty pull request lookup. Four cases still ended as
complete:

- a writer whose open pull request already existed was credited
  pr_updated while its edits or commits stayed in the clone;
- a failed pull request lookup read as "no pull request", so a clean
  checkout became no_change_needed and the Work Item done;
- a writer that pushed its branch but opened no pull request was
  reported with a wrong reason (its pushed commits counted as not on
  the forge, because the depth-limited clone tracks only the base);
- nothing distinguished "the branch moved during the Run" from "the
  branch was already there".

The worker now records where the Run's branch stands on the forge
(git ls-remote) next to the starting commit, before the harness runs.
When a writer would end no_change_needed or pr_updated it reads the
branch again. A failed pull request lookup or branch read is stuck
("unknown"); a branch that moved without a pull request is stuck; a
pr_updated whose branch moved keeps pr_updated; otherwise uncommitted
paths (anything git status lists, so the repository's .gitignore
decides what is build output) or local commits that neither the remote
branch nor the base contains are stuck, naming them. pr_opened,
failed and stuck outcomes, and readers, are untouched.

Tests drive the real worker with a fake ACP agent against a
git-http-backend forge: edit-tool, shell and base-branch commits, a
pushed branch without a pull request, local-only changes under an open
pull request, a real push to it, a failed forge read, untracked output
the repository does not ignore, a reader that leaves files, and a
harness failure or pod termination that also edited the checkout.

VIK-1737

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
fix(ploeg): only the worker says whether a Run delivered a pull request
Some checks failed
[Workflow] On Pull Request / checks (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / warnings (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / release-policy (pull_request) Has been cancelled
Details
abb2e2ea2d
 
An agent could decide that its Run delivered, and where. Claude Code and
ACP merged the drop box's outcome, links, checkpoint and failure reason
whenever the adapter concluded nothing, OpenHands and the exec adapter
returned the file as the whole report, and resolveOutcome kept any
valid structured outcome with its own links. A drop box saying
pr_opened with a link to /other/repo/pulls/123 therefore reached ploegd
as this Run's pull request. Forge reads that failed were logged and
read as "no pull request", and a pull request from a fork with the same
branch name counted as the Run's.

This is the worker side of ADR-0059 (still proposed):

- harness.MergeDropBox keeps the agent's own account (findings, verdict,
  problem, solution, proposed Work Items, a non-delivery outcome and
  its summary and stuck reason) and drops pr_opened, pr_updated, links,
  checkpoint, failure reason, verification and delivery. Every adapter
  reads its drop box through it, and a new conformance property holds
  all four adapters to it.
- resolveOutcome strips the same claims from whatever an adapter
  returns, so pr_opened and pr_updated come only from the worker's
  forge reads.
- The forge lookup returns the pull request's number, head commit and
  base, and skips one whose head repository is not the Work Item's
  repository (Forgejo head.repo, GitLab source/target project).
- Each read is tried three times. A writer whose read before the
  harness fails does not start: it ends failed/infra_node before a key
  is minted. A pull request that replaced the earlier one during the Run
  is pr_opened, not pr_updated.
- The report carries a new optional delivery record (forge, repository,
  branch, number, URL, base, head before and after, and
  opened/updated/none/unknown). outcomereport.v1 gains it additively.
  ploegd does not check, store or read it yet.

Not in this change: pr_updated still follows a clean exit on an open
pull request even when its head did not move; ploegd still trusts the
links it is sent. Both need ploegd to store delivery first.

VIK-1732

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ryangr0 closed this pull request 2026-10-04 08:41:37 +00:00
Some checks failed
[Workflow] On Pull Request / checks (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / warnings (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / release-policy (pull_request) Has been cancelled
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
webgrip/unfold!199
No description provided.