fix(worker,shiftengine,chart): make a reading Role able to review — and unable to write #32
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/rc14-refinement-enablers"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Everything here came out of running rc.13 in production. Nothing was found by reading.
The reader could not review, and was trusted on a false statement
A Shift's reading Roles dispatched correctly and then failed on arrival with
exec: "opencode": executable file not found in $PATH. That reads as "add the binary to the image".It isn't — shipping opencode would only have turned failed to start into started, reviewed, and
threw the review away. Three defects sat behind it:
A reader never received the code. The clone is
--depth 50 --branch <base>, and--depthimplies
--single-branch, so the writer's branch was not in the repository at all — while thereview contract told the agent "the repository checkout is your working directory, on branch
<shift branch>". Readers now fetch that branch and stand on it, with the base still present so
git diff base...branchis the change under review.A reader was never told the pull request existed.
priorPRwas interpolated only into thewriter's contract, which made "review the pull request" unsayable.
A reader held a full push credential.
AGENT_BUILDER_TOKENisrequireEnvon every workerpod,
BaseEnv: os.Environ()handed it to the agent, andauthURLbaked it intoorigin. Thecontract said "you hold no lease on this branch and no write credential, so a push will be
rejected by the forge" — untrue on every real deployment. The only control was a sentence the
model could disprove by trying.
A reading run now has the token scrubbed from its agent's environment and its
originreset to acredential-free URL. The scrub matches on value, not variable name, so a future variable
carrying the same token cannot quietly reopen the hole. The contract now describes the control
that exists instead of asserting one that didn't.
A clean reader was recorded as having pushed
Standing on the writer's branch, a reader finds the writer's PR, so the
runErr == nil && prURL != ""arm credited it with
pr_updated. On any plan whose last Round is a review, that is everysuccessful Shift. Readers now resolve to
no_change_neededand carry their findings.A successful review announced itself as a stoppage
trackerMessagekeyed its opening line on the settled state, and a configured plan settlesneeds_humanon success — the last word is "a person is asked to merge". So a successfulmulti-Round Shift opened its board comment with "Ploeg stopped working this item" directly above
the pull request it had just produced. A PR existing now outranks the state.
Also here, from the same rollout
create/namesplit. The old guard read a name as "an external accountexists", so naming the chart's own default suppressed the account the pods then referenced and
every Job died with
serviceaccount "ploeg-worker" not found.maxFixRoundsserialised from chart values, so ADR-0017's request-changes loop is reachable at all.Verification
Mutation-tested in both directions, because a failing test alone cannot tell a working gate from a
broken one:
writer-keeps-its-token case holds throughout.
trackerMessagecondition → the new regression case fails and the other four keeppassing.
gofmt,go vet,go build,go test ./...,helm lintand./scripts/helm-golden.sh checkallgreen locally.
Not in this PR
The ACP and claude-code adapters still cannot return findings at all — neither sets
PLOEG_OUTCOME_FILEnor populatesFindings, so onlyopenhandsandexeccan carry a review.That is what a second harness needs before ADR-0051 can be ratified, and it is tracked separately.
The reader path here is proven on
openhandswithdind: false, which needs no new image.🤖 Generated with Claude Code
All three broke, or hid, real dispatch on 2026-07-30. 1. The worker ServiceAccount guard inferred create from name. `if and .Values.executor.enabled (not .Values.executor.serviceAccountName)` read a name as "an external account exists", so naming the chart's OWN default suppressed the account the pod template then referenced and every worker Job died with `serviceaccount "ploeg-worker" not found`. Naming the default and saying nothing must be equivalent; they were opposite. Split into executor.serviceAccount.{create,name}, and route the object and the reference through one helper so they cannot disagree again. 2. The DinD hazard label never reached the Job. It was stamped on the pod TEMPLATE only. Kyverno autogens a Job rule for pod-security-baseline-enforce, and a Job selector matches the JOB's own labels — so the PolicyException could not admit the Job and every DinD team was rejected at admission. KEDA copies ScaledJob labels onto its Jobs, so the label now goes there too, gated by the same role->team->global dind resolution the pod template uses (extracted to ploeg.roleUsesDind). Consumers can then key their exception on the hazard instead of on a list of workload names. 3. maxFixRounds was never serialised. PLOEG_TEAM_PLANS was built as {pool, rounds}, dropping the field entirely, so ADR-0017's request_changes loop was unreachable from Helm values — a reviewer verdict could never re-open the writing Round. Verified by rendering: naming the default now both creates and references the account; create:false skips creation and keeps the external reference; the hazard label appears on a dind role's ScaledJob and pod template and on neither for a dind:false role; and maxFixRounds:2 reaches PLOEG_TEAM_PLANS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>fix(chart): three defects found by running rc.13 in productionto fix(worker,shiftengine,chart): make a reading Role able to review — and unable to write