fix(harness,worker): a reading Run's review must survive every harness #35

Merged

ryangr0 merged 3 commits from ryangr0/harness-outcome-dropbox into development

2026-08-08 13:19:51 +00:00

AGit

ryangr0 commented

2026-08-08 06:07:51 +00:00

Owner

ComposePrompt tells every reading Run, on every harness, to deliver its review by writing JSON to the file named by PLOEG_OUTCOME_FILE. Only openhands and exec ever set it. On claude-code it was never exported at all; acp had no notion of a drop box.

So a reviewer on either wrote its review to the empty string and Ploeg read nothing: agent_runs.findings and verdict stayed blank, shiftengine.requestsChanges was therefore always false, and ADR-0017's review loop was inert — every Shift closed review_approved regardless of what the reviewer found, with nothing published to the pull request for a human to read either. values.yaml documents harness: {name: claude-code} on a role as supported, so this was reachable configuration.

What changed

One drop box, in pkg/harness (DropBoxEnv, DropBoxPath, ReadDropBox, MergeDropBox) rather than a copy per adapter. openhands moves onto it too, which is what makes ADR-0018's claim true rather than aspirational.
MergeDropBox carries the precedence. Findings and verdict are the agent's and always survive — a run that reviewed and then failed its shutdown handshake still did the review. Outcome and summary fill only a gap the adapter left: an adapter that classified a launch failure, a lost lease or a watchdog timeout holds evidence the agent does not, and an agent must not overturn it by writing a cheerful file (R2).
resolveOutcome no longer drops the PR link. Its structured-report arm returned the report untouched, with no Links — so a reader that correctly wrote a drop box lost the URL while one that returned nothing kept it. publishRound finds the pull request by scanning reported links, so a review-only Shift published its findings nowhere.
harnesstest.ReadingRunFindingsSurviveTheAdapter, run by all four adapter packages already. It is what stops this recurring on the fifth adapter.

Regression tests, proven to fail first

The new property against the unfixed adapters:

--- FAIL: TestConformance/ReadingRunFindingsSurviveTheAdapter   [acp]
        findings did not survive the adapter: got "" want "## Review\n\n- `broker.go:88` inverted TTL comparison\n"
--- FAIL: TestConformance/ReadingRunFindingsSurviveTheAdapter   [claudecode]
        run returned exit status 3
ok  execbin   ok  openhands

Exit status 3 is the script's own guard: PLOEG_OUTCOME_FILE was unset, which is the defect stated directly.

The link fix, with the change reverted:

--- FAIL: TestResolveOutcome_Precedence/a_structured_report_gains_the_PR_link_it_could_not_know
        worker_test.go:221: links = [], want [http://forge/pr/7]

Also here

docs/adrs/0018-* (proposed), backlog 109-115 for what this sweep found but did not fix, and architecture.md §9 divergence 18. Evidence: docs/research/2026-08-08-benchmarking-the-loop.md §8.

Backlog 115 is worth a look on its own: ADR-0017's close_reason already misreports at maxFixRounds: 1 — nextFixRound checks money -> cap -> verdict, so a fix round the reviewer approves closes as fix_round_cap_reached. That is one of ADR-0017's own re-evaluation triggers, hit before the loop has run in production.

Gates

.forgejo/workflows/on_pull_request.yml — full output

$ gofmt -l . && test -z "$(gofmt -l .)"
(no output — clean)

$ go vet ./...
(no output — clean)

$ go build ./...
(no output — clean)

$ go test ./...
?   	github.com/webgrip/ploeg/cmd/ploeg-worker	[no test files]
ok  	github.com/webgrip/ploeg/cmd/ploegd	(cached)
ok  	github.com/webgrip/ploeg/internal/ledger	(cached)
ok  	github.com/webgrip/ploeg/pkg/config	(cached)
ok  	github.com/webgrip/ploeg/pkg/forgebroker	(cached)
ok  	github.com/webgrip/ploeg/pkg/harness	(cached)
ok  	github.com/webgrip/ploeg/pkg/harness/adapters/acp	(cached)
ok  	github.com/webgrip/ploeg/pkg/harness/adapters/claudecode	(cached)
ok  	github.com/webgrip/ploeg/pkg/harness/adapters/execbin	(cached)
ok  	github.com/webgrip/ploeg/pkg/harness/adapters/openhands	(cached)
?   	github.com/webgrip/ploeg/pkg/harness/harnesstest	[no test files]
ok  	github.com/webgrip/ploeg/pkg/httpapi	(cached)
ok  	github.com/webgrip/ploeg/pkg/litellm	(cached)
ok  	github.com/webgrip/ploeg/pkg/llmbroker	(cached)
ok  	github.com/webgrip/ploeg/pkg/plan	(cached)
?   	github.com/webgrip/ploeg/pkg/provider	[no test files]
ok  	github.com/webgrip/ploeg/pkg/provider/forgejo	(cached)
ok  	github.com/webgrip/ploeg/pkg/provider/vikunja	(cached)
ok  	github.com/webgrip/ploeg/pkg/shiftengine	(cached)
ok  	github.com/webgrip/ploeg/pkg/store	(cached)
ok  	github.com/webgrip/ploeg/pkg/target	(cached)
ok  	github.com/webgrip/ploeg/pkg/work	(cached)
ok  	github.com/webgrip/ploeg/pkg/worker	(cached)

$ helm lint ops/helm/ploeg
==> Linting ops/helm/ploeg
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

$ helm template ploeg ops/helm/ploeg > /dev/null
ok
$ helm template ploeg ops/helm/ploeg -f ops/helm/ploeg/ci/executor-values.yaml > /dev/null
ok
$ helm template ploeg ops/helm/ploeg -f ops/helm/ploeg/ci/executor-cronjob-values.yaml > /dev/null
ok

$ ./scripts/helm-golden.sh check
chart render 'default' differs from its golden:
--- ops/helm/ploeg/ci/golden/default.yaml	2026-08-08 07:53:09.426936180 +0200
+++ -	2026-08-08 08:08:19.220297001 +0200
@@ -11,7 +11,6 @@
     helm.sh/chart: ploeg-CHART-VERSION
     app.kubernetes.io/managed-by: Helm
 automountServiceAccountToken: false
-
 ---
 # Source: ploeg/templates/service.yaml
 apiVersion: v1
@@ -33,7 +32,6 @@
     - name: http
       port: 8080
       targetPort: http
-
 ---
 # Source: ploeg/templates/deployment.yaml
 apiVersion: apps/v1
chart render 'executor' differs from its golden:

The golden check does not pass locally, and should not be "fixed". The diff is whitespace only — a blank line before each --- — on a branch that changes nothing under ops/helm. My helm is v3.18.4; CI pins v4.2.3, and the two disagree about the blank line before a document separator. Running helm-golden.sh update would commit that churn and break the CI check. The script's failure message advised exactly that, so this branch fixes the message instead (29bd394).

🤖 Generated with Claude Code

`ComposePrompt` tells every reading Run, on every harness, to deliver its review by writing JSON to the file named by `PLOEG_OUTCOME_FILE`. Only `openhands` and `exec` ever set it. On `claude-code` it was never exported at all; `acp` had no notion of a drop box. So a reviewer on either wrote its review to the empty string and Ploeg read nothing: `agent_runs.findings` and `verdict` stayed blank, `shiftengine.requestsChanges` was therefore always false, and **ADR-0017's review loop was inert — every Shift closed `review_approved` regardless of what the reviewer found**, with nothing published to the pull request for a human to read either. `values.yaml` documents `harness: {name: claude-code}` on a role as supported, so this was reachable configuration. ## What changed - **One drop box, in `pkg/harness`** (`DropBoxEnv`, `DropBoxPath`, `ReadDropBox`, `MergeDropBox`) rather than a copy per adapter. `openhands` moves onto it too, which is what makes ADR-0018's claim true rather than aspirational. - **`MergeDropBox` carries the precedence.** Findings and verdict are the agent's and always survive — a run that reviewed and then failed its shutdown handshake still did the review. Outcome and summary fill only a gap the adapter left: an adapter that classified a launch failure, a lost lease or a watchdog timeout holds evidence the agent does not, and an agent must not overturn it by writing a cheerful file (R2). - **`resolveOutcome` no longer drops the PR link.** Its structured-report arm returned the report untouched, with no `Links` — so a reader that correctly wrote a drop box lost the URL while one that returned nothing kept it. `publishRound` finds the pull request by scanning reported links, so a review-only Shift published its findings nowhere. - **`harnesstest.ReadingRunFindingsSurviveTheAdapter`**, run by all four adapter packages already. It is what stops this recurring on the fifth adapter. ## Regression tests, proven to fail first The new property against the unfixed adapters: ``` --- FAIL: TestConformance/ReadingRunFindingsSurviveTheAdapter [acp] findings did not survive the adapter: got "" want "## Review\n\n- `broker.go:88` inverted TTL comparison\n" --- FAIL: TestConformance/ReadingRunFindingsSurviveTheAdapter [claudecode] run returned exit status 3 ok execbin ok openhands ``` Exit status 3 is the script's own guard: `PLOEG_OUTCOME_FILE` was unset, which is the defect stated directly. The link fix, with the change reverted: ``` --- FAIL: TestResolveOutcome_Precedence/a_structured_report_gains_the_PR_link_it_could_not_know worker_test.go:221: links = [], want [http://forge/pr/7] ``` ## Also here `docs/adrs/0018-*` (proposed), backlog 109-115 for what this sweep found but did not fix, and `architecture.md` §9 divergence 18. Evidence: `docs/research/2026-08-08-benchmarking-the-loop.md` §8. Backlog 115 is worth a look on its own: **ADR-0017's `close_reason` already misreports at `maxFixRounds: 1`** — `nextFixRound` checks money -> cap -> verdict, so a fix round the reviewer *approves* closes as `fix_round_cap_reached`. That is one of ADR-0017's own re-evaluation triggers, hit before the loop has run in production. ## Gates <details><summary><code>.forgejo/workflows/on_pull_request.yml</code> — full output</summary> ``` $ gofmt -l . && test -z "$(gofmt -l .)" (no output — clean) $ go vet ./... (no output — clean) $ go build ./... (no output — clean) $ go test ./... ? github.com/webgrip/ploeg/cmd/ploeg-worker [no test files] ok github.com/webgrip/ploeg/cmd/ploegd (cached) ok github.com/webgrip/ploeg/internal/ledger (cached) ok github.com/webgrip/ploeg/pkg/config (cached) ok github.com/webgrip/ploeg/pkg/forgebroker (cached) ok github.com/webgrip/ploeg/pkg/harness (cached) ok github.com/webgrip/ploeg/pkg/harness/adapters/acp (cached) ok github.com/webgrip/ploeg/pkg/harness/adapters/claudecode (cached) ok github.com/webgrip/ploeg/pkg/harness/adapters/execbin (cached) ok github.com/webgrip/ploeg/pkg/harness/adapters/openhands (cached) ? github.com/webgrip/ploeg/pkg/harness/harnesstest [no test files] ok github.com/webgrip/ploeg/pkg/httpapi (cached) ok github.com/webgrip/ploeg/pkg/litellm (cached) ok github.com/webgrip/ploeg/pkg/llmbroker (cached) ok github.com/webgrip/ploeg/pkg/plan (cached) ? github.com/webgrip/ploeg/pkg/provider [no test files] ok github.com/webgrip/ploeg/pkg/provider/forgejo (cached) ok github.com/webgrip/ploeg/pkg/provider/vikunja (cached) ok github.com/webgrip/ploeg/pkg/shiftengine (cached) ok github.com/webgrip/ploeg/pkg/store (cached) ok github.com/webgrip/ploeg/pkg/target (cached) ok github.com/webgrip/ploeg/pkg/work (cached) ok github.com/webgrip/ploeg/pkg/worker (cached) $ helm lint ops/helm/ploeg ==> Linting ops/helm/ploeg [INFO] Chart.yaml: icon is recommended 1 chart(s) linted, 0 chart(s) failed $ helm template ploeg ops/helm/ploeg > /dev/null ok $ helm template ploeg ops/helm/ploeg -f ops/helm/ploeg/ci/executor-values.yaml > /dev/null ok $ helm template ploeg ops/helm/ploeg -f ops/helm/ploeg/ci/executor-cronjob-values.yaml > /dev/null ok $ ./scripts/helm-golden.sh check chart render 'default' differs from its golden: --- ops/helm/ploeg/ci/golden/default.yaml 2026-08-08 07:53:09.426936180 +0200 +++ - 2026-08-08 08:08:19.220297001 +0200 @@ -11,7 +11,6 @@ helm.sh/chart: ploeg-CHART-VERSION app.kubernetes.io/managed-by: Helm automountServiceAccountToken: false - --- # Source: ploeg/templates/service.yaml apiVersion: v1 @@ -33,7 +32,6 @@ - name: http port: 8080 targetPort: http - --- # Source: ploeg/templates/deployment.yaml apiVersion: apps/v1 chart render 'executor' differs from its golden: ``` </details> **The golden check does not pass locally, and should not be "fixed".** The diff is whitespace only — a blank line before each `---` — on a branch that changes nothing under `ops/helm`. My helm is v3.18.4; CI pins v4.2.3, and the two disagree about the blank line before a document separator. Running `helm-golden.sh update` would commit that churn and break the CI check. The script's failure message advised exactly that, so this branch fixes the message instead (`29bd394`). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

ryangr0 added 3 commits

2026-08-08 06:07:51 +00:00

fix(harness,worker): a reading Run's review must survive every harness 7e78a59e15

ComposePrompt tells every reading Run, on every harness, to deliver its
review by writing JSON to the file named by PLOEG_OUTCOME_FILE. Only
openhands and exec ever set that variable. On claude-code it was never
exported at all, and acp had no notion of a drop box, so a reviewer on
either wrote its findings to the empty string and Ploeg read nothing:
agent_runs.findings and verdict stayed blank, requestsChanges was
therefore always false, and ADR-0017's review loop was inert — every
Shift closed review_approved regardless of what the reviewer found, with
nothing published to the pull request for a human to read either.
values.yaml documents claude-code on a role as supported, so this was
reachable configuration, not a hypothetical.

The drop box is now one implementation in pkg/harness rather than a copy
per adapter, and openhands moves onto it. MergeDropBox fixes the
precedence the two shapes need: findings and verdict are the agent's and
always survive (a run that reviewed and then failed its shutdown
handshake still did the review), while outcome and summary only fill a
gap the adapter left — an adapter that classified a launch failure, a
lost lease or a watchdog timeout has evidence the agent does not, and an
agent must not overwrite it by writing a cheerful file.

resolveOutcome's structured-report arm returned the harness report with
no Links, so a reader that correctly wrote a drop box lost the PR URL
while one that returned nothing kept it. publishRound finds the pull
request by scanning reported links, so a review-only Shift published its
findings nowhere. Ploeg polled the forge and knows the URL; it now fills
the gap the agent could not.

harnesstest gains ReadingRunFindingsSurviveTheAdapter, which every
adapter package already runs. It fails against claude-code and acp before
these fixes and is what stops this recurring on the fifth adapter.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs(ci): name the helm-version trap in the golden check's own advice 29bd394618

The goldens are generated with the helm CI pins; helm 3 and helm 4
disagree about the blank line before a document separator, so a local
helm 3 shows a whitespace-only diff on a branch that changed nothing
under ops/helm. The failure message advised running 'update', which
would commit that churn and break the CI check it exists to serve.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs(adrs): ADR-0018 — the drop box is every harness's return path

On Pull Request / checks (pull_request) Successful in 1m1s

Details

0ee2890f9f

Records the verdict behind the harness fix: one drop box defined in
pkg/harness, honoured by every adapter, with a fixed merge precedence.
Findings and verdict are the agent's and always survive; outcome and
summary fill only a gap the adapter left, because an adapter that
classified a launch failure or a lost lease holds evidence the agent
does not and an agent must not overturn it (R2).

Also records what the sweep found but did not fix: settling spend for
swept runs, the PR head SHA nothing durable records, findPR's missing
pagination, the process-global ScratchDir, the unaudited lease on the
Shift path, TaskSpec's missing round/writes, and ADR-0017's own
close_reason misreporting at maxFixRounds 1 — backlog 109-115.

architecture.md gains divergence 18 and a correction to 11: "no metrics"
is a live-observability gap, not an absence of timing data — started_at
and finished_at are exact per Run after the fact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>