fix(shiftengine): tell the board when a Shift finishes #31

Merged

ryangr0 merged 4 commits from fix/tracker-writeback-and-role-workloads into development

2026-07-30 14:00:45 +00:00

agent-builder commented

2026-07-30 13:54:44 +00:00

Member

Found while triaging a production end-to-end run on 2026-07-30. The factory worked: webhook → routing → Shift → KEDA → worker → OpenHands → PR #30 opened → outcome ingested. Then it told nobody.

The reported bug

Engine.close gated the tracker write-back on next == StateNeedsHuman. A plan-less team's pr_opened settles StateDone, so the successful path — the one that matters — skipped notifyHuman entirely. No error surfaced because no call was made. Ticket 580 sat at done=false with zero comments while its pull request was open.

Underneath it, notifyHuman called SetStatus(..., StateNeedsHuman) while the Vikunja provider early-returns on anything that is not StateDone. The status write has never done anything on any path.

What changed

work.Terminal() gates the write-back. queued is the only settle result that is not terminal; announcing a stopped Shift mid-retry would be a lie.
SettleItem returns the state it wrote. queued coerces to stale at the attempt cap and the caller could not tell "failed, retrying" from "failed, gave up". The audit row records the effective state now too — it used to claim work_item.queued for a row that went stale.
CloseShift reports whether it won the CAS. Both racers still settle the item (crash repair); only the winner comments, so the outcome fast-path and the sweeper stop double-posting.
notifyHuman → notifyTracker, with wording per terminal state extracted to a pure trackerMessage() that is table-testable without Postgres.
An assignee shared by two teams is rejected at boot. AssigneeTeams() ranged a Go map, so the same config routed the same person to a different team on different boots — measured 168/200 vs 32/200 in one process.
The worker stops claiming a per-run credential it does not have. With no admin token, forgebroker.Static returns the shared token in the same field; cred.ID was already the truth predicate, now surfaced as claimResponse.forgeTokenPerRun. Also fixes contract drift — run-api.v1.schema.json declares additionalProperties: false and never listed forgeToken at all.
resolveTarget logs success, not only failure. The routing decision used to appear a hop later in the worker, after a pod had been scheduled.
Chart: worker ServiceAccount + per-Role resources. Workers ran as default (Kyverno require-non-default-serviceaccount); they now get <fullname>-worker, deliberately not ploegd's account. Resources were executor-wide, so every reader in a fan-out Round booked a whole writer-sized pod — three of those do not fit a one-node worker pool.

Ploeg still never closes a task

This deployment's Definition of Done is "in production, monitored, first telemetry observed". Ploeg opens a pull request and stops, so it is never in a position to know an item is finished, and the comment it posts literally asks a person to merge. The status write stays needs_human — now a documented rule rather than an unreachable accident.

Verification

TestPublish_DoneOutcomeStillNotifiesTheTracker is the incident as a test and fails against v0.2.0-rc.12 — confirmed by reverting the gate and watching it go red, then green again. Same both-ways check on the duplicate-assignee rejection.

Chart change verified by rendering: builder keeps the executor default (1 / 1Gi), reviewer takes its override (500m / 384Mi), both on sa=ploeg-worker. Goldens are additions only and the privileged-dind label count is unchanged.

Local CI gates all green: gofmt, go vet, go build, go test ./..., helm lint, ./scripts/helm-golden.sh check.

Not in this PR

Per-ticket route selection by tracker label and PLOEG_TARGET_STRICT are held for rc.14. ADR-0015 requires a hint to select among pre-registered routes rather than construct a target — tracker text is untrusted input, and a hint that constructs a target is an arbitrary-repository write primitive. That needs the OpenSpec/ADR round trip rather than being smuggled into a fix PR.

No VIK- trailer on these commits — there is no board ticket for this work yet.

🤖 Generated with Claude Code

Found while triaging a production end-to-end run on 2026-07-30. The factory worked: webhook → routing → Shift → KEDA → worker → OpenHands → **PR #30 opened** → outcome ingested. Then it told nobody. ## The reported bug `Engine.close` gated the tracker write-back on `next == StateNeedsHuman`. A plan-less team's `pr_opened` settles `StateDone`, so the successful path — the one that matters — skipped `notifyHuman` entirely. No error surfaced because no call was made. Ticket 580 sat at `done=false` with zero comments while its pull request was open. Underneath it, `notifyHuman` called `SetStatus(..., StateNeedsHuman)` while the Vikunja provider early-returns on anything that is not `StateDone`. The status write has never done anything on any path. ## What changed - **`work.Terminal()` gates the write-back.** `queued` is the only settle result that is not terminal; announcing a stopped Shift mid-retry would be a lie. - **`SettleItem` returns the state it wrote.** `queued` coerces to `stale` at the attempt cap and the caller could not tell "failed, retrying" from "failed, gave up". The audit row records the effective state now too — it used to claim `work_item.queued` for a row that went stale. - **`CloseShift` reports whether it won the CAS.** Both racers still settle the item (crash repair); only the winner comments, so the outcome fast-path and the sweeper stop double-posting. - **`notifyHuman` → `notifyTracker`**, with wording per terminal state extracted to a pure `trackerMessage()` that is table-testable without Postgres. - **An assignee shared by two teams is rejected at boot.** `AssigneeTeams()` ranged a Go map, so the same config routed the same person to a different team on different boots — measured 168/200 vs 32/200 in one process. - **The worker stops claiming a per-run credential it does not have.** With no admin token, `forgebroker.Static` returns the shared token in the same field; `cred.ID` was already the truth predicate, now surfaced as `claimResponse.forgeTokenPerRun`. Also fixes contract drift — `run-api.v1.schema.json` declares `additionalProperties: false` and never listed `forgeToken` at all. - **`resolveTarget` logs success**, not only failure. The routing decision used to appear a hop later in the worker, after a pod had been scheduled. - **Chart: worker ServiceAccount + per-Role resources.** Workers ran as `default` (Kyverno `require-non-default-serviceaccount`); they now get `<fullname>-worker`, deliberately not ploegd's account. Resources were executor-wide, so every reader in a fan-out Round booked a whole writer-sized pod — three of those do not fit a one-node worker pool. ## Ploeg still never closes a task This deployment's Definition of Done is "in production, monitored, first telemetry observed". Ploeg opens a pull request and stops, so it is never in a position to know an item is finished, and the comment it posts literally asks a person to merge. The status write stays `needs_human` — now a documented rule rather than an unreachable accident. ## Verification `TestPublish_DoneOutcomeStillNotifiesTheTracker` is the incident as a test and **fails against v0.2.0-rc.12** — confirmed by reverting the gate and watching it go red, then green again. Same both-ways check on the duplicate-assignee rejection. Chart change verified by rendering: `builder` keeps the executor default (1 / 1Gi), `reviewer` takes its override (500m / 384Mi), both on `sa=ploeg-worker`. Goldens are additions only and the `privileged-dind` label count is unchanged. Local CI gates all green: `gofmt`, `go vet`, `go build`, `go test ./...`, `helm lint`, `./scripts/helm-golden.sh check`. ## Not in this PR Per-ticket route selection by tracker label and `PLOEG_TARGET_STRICT` are held for rc.14. ADR-0015 requires a hint to *select among pre-registered routes* rather than construct a target — tracker text is untrusted input, and a hint that constructs a target is an arbitrary-repository write primitive. That needs the OpenSpec/ADR round trip rather than being smuggled into a fix PR. No `VIK-` trailer on these commits — there is no board ticket for this work yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

agent-builder added 4 commits

2026-07-30 13:54:44 +00:00

fix(shiftengine): write back to the tracker on every terminal settle 30d9ce3090

A Shift that opened a pull request told the board nothing. close() gated the
write-back on `next == StateNeedsHuman`, but a plan-less team's pr_opened
settles StateDone, so the successful path — the one that matters — skipped
notifyHuman entirely. No error was logged because no call was made. Observed
in production 2026-07-30: PR #30 opened, ticket 580 left untouched.

- work.Terminal() gates the write-back instead. queued is the only settle
  result that is not terminal; announcing a stopped Shift mid-retry is a lie.
- SettleItem returns the state it actually wrote. queued coerces to stale at
  the attempt cap, and the caller could not tell "failed, retrying" from
  "failed, gave up". The audit row now records the effective state too — it
  used to claim work_item.queued for a row that went stale.
- CloseShift reports whether THIS call won the CAS. Both racers still settle
  the item (crash repair), but only the winner comments, so the outcome
  fast-path and the sweeper stop double-posting.
- notifyHuman -> notifyTracker, with per-terminal-state wording extracted to
  a pure trackerMessage() that is table-testable without Postgres.

Ploeg still never closes a task. This deployment's Definition of Done is
"in production, monitored, first telemetry observed" — Ploeg opens a PR and
stops, so it is never in a position to know. The status write stays
needs_human, now as a documented rule rather than an unreachable accident.

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

fix(config): reject an assignee shared by two teams 60c836bdc3

AssigneeTeams() ranged a Go map and let the last writer win, so a username
listed under two teams resolved to a different team on different boots of the
SAME config. Measured on rc.12 in one process: 168/200 vs 32/200. Validate()
had no check for it, so the config loaded clean and misrouted quietly.

Reject it at boot, naming both teams and the username. Cross-team only — the
same name twice inside one team is a harmless typo that resolves identically
either way, and failing on it would be a gratuitous outage.

AssigneeTeams() itself now walks teams in sorted order too. Validate() only
runs from Load(), so a File built anywhere else would keep the coin flip.

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

fix(worker,httpapi): tell the truth about the forge credential, log routing f58c26119b

Two lines that misled during the 2026-07-30 e2e audit.

The worker logged "using a per-run forge credential" whenever the claim
carried a token — but with PLOEG_FORGEJO_ADMIN_TOKEN unset, forgebroker.Static
returns the SHARED token in that same field, with no id. ploegd said per-run
credentials were disabled while the worker said it was using one, and both
were reading the same run. cred.ID was already the truth predicate; surface it
as claimResponse.forgeTokenPerRun so the worker can say which it holds.

resolveTarget logged only its two failure branches, so a CORRECT routing
decision was invisible in ploegd's log and first appeared a hop later in the
worker — after a pod was scheduled and an agent had started. That is the
wrong end of the pipeline to learn where the work is going.

Also fixes contract drift: run-api.v1.schema.json declares
additionalProperties:false on claimResponse and never listed forgeToken at
all, so the wire had been violating its own published contract.

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

feat(chart): worker ServiceAccount and per-Role resources

On Pull Request / checks (pull_request) Successful in 5m10s

Details

12896f5c6c

Two gaps found while sizing a multi-Role Shift against a real cluster.

Worker pods named no ServiceAccount, so the API server gave them 'default'.
That trips Kyverno's require-non-default-serviceaccount and makes every
workload in the namespace indistinguishable in an audit log. They now get
<fullname>-worker, created by the chart and overridable via
executor.serviceAccountName. Deliberately not ploegd's account: ploegd holds
the LiteLLM master key and the forge admin credential, and a future
RoleBinding on that name must not reach the agent pods. The token stays
unmounted — this is identity, not privilege.

Resources were executor-wide only, so every reader in a fan-out Round asked
for a whole writer-sized pod. Three readers at 1 CPU / 1Gi do not fit a
one-node worker pool, and readers do not build anything. roles[] now accept
workerResources / dindResources, same field-by-field override shape as the
existing per-Role harness.

Verified by rendering: builder keeps the executor default (1 / 1Gi), reviewer
takes its override (500m / 384Mi), both on sa=ploeg-worker. Goldens are
additions only and the privileged-dind label count is unchanged.

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