From e8229d32b9b07571446e0c9335bc5d462864745a Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 26 Sep 2026 23:05:31 +0200 Subject: [PATCH] fix: read the answer from the message_end event pi 0.87.x emits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pi is installed unpinned in the agent-pi image and its event vocabulary has already changed once. A v0.4.0 agent-pi pod produced {"type":"message_end","message":{...,"role":"assistant",...}} followed by {"type":"agent_settled"}, and extractEventText's two-case switch matched neither — so a run that had answered correctly came back as "no result found in pi CLI output". The symptom points at the model; the defect is in the parser. Only a real prompt through the runner exercises the path at all, which is how it survived: the fleet's working agents run older images, and the recently-built ones had never been prompted. The message_end case carries a role guard, because that event fires for every role — without it a user or system message would be returned as the answer, and a run that failed would look like a successful echo of its own prompt. Both vocabularies are kept: agent_end/message_update still cover the older pi that pi-agent's pinned v0.1.7 image runs, which is exactly why that agent kept working while the freshly-built one did not. --- CHANGELOG.md | 4 +++ pi/pi-runner.go | 22 ++++++++++++-- pi/pi-runner_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0a5fd4..93ceb89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- fix: read the answer out of the `message_end` event the current pi CLI emits, not only the `agent_end`/`message_update` pair older builds used. pi is installed **unpinned** in the `agent-pi` image and its event vocabulary has already changed once: a `v0.4.0` agent-pi pod produced `{"type":"message_end","message":{...,"role":"assistant",...}}` followed by `{"type":"agent_settled"}`, and `extractEventText`'s two-case switch matched neither — so a run that had answered correctly came back as `no result found in pi CLI output`. **The symptom points at the model and the defect is in the parser**, and since only a real prompt through the runner exercises the path at all, it survived: the fleet's working agents run older images, and the recently-built ones had never been prompted. The `message_end` case carries a **role guard**, because that event fires for every role — without it a user or system message would be returned as the answer, and a run that failed would look like a successful echo of its own prompt. Specs cover both vocabularies and the role guard, using the file's existing `pi`-shim pattern. + ## v0.90.2 - fix: grant the executor's ServiceAccount `create`/`get`/`update` on `apps/statefulsets`, so the service-agent reconcile loop can actually create the StatefulSet it exists to create. `executor-rbac.yaml` granted `batch/jobs` and `""/pods` and nothing under `apps` — because until the `type: service` work the executor never created a StatefulSet — so the loop ran correctly against a live cluster (`event=service_reconcile done configs=17 services=1 failures=1`) and then failed on the one Config that mattered: `create statefulSet failed: statefulsets.apps is forbidden: User "system:serviceaccount:dev:agent-task-executor" cannot create resource "statefulsets" in API group "apps"`. No SC, DoD row or task line named RBAC, so nothing in the task could have caught it before the first live reconcile. `delete` is deliberately **not** granted. The loop's removal path deletes by name, and a StatefulSet's name here is just the Config's name, so it is only safe on an executor that checks the object's Config controller ownerRef first (v0.18.1+). The chart cannot see which executor version it is paired with, so granting `delete` would make safety depend on a comment — and the unsafe pairing is narrow but real, because v0.18.0 shipped the undeploy path *without* the guard and is what dev runs today. Withholding it removes the coupling entirely: `get` lets the guarded path find nothing and return quietly, and no executor version can delete by name through this Role. Grant `delete` only once the pairing is enforced rather than documented — `check-executor-compat` in `nuke/agent` runs before mirror+upgrade and is the platform's existing gate for exactly this class of coupling. diff --git a/pi/pi-runner.go b/pi/pi-runner.go index 77f1c68..8a4bec0 100644 --- a/pi/pi-runner.go +++ b/pi/pi-runner.go @@ -221,14 +221,32 @@ func scanOutput( } // extractEventText returns text from a piEvent, or empty string if none. -// Handles both agent_end (last assistant message's last text content) and -// message_update (any text content delta) event types. +// +// It has to know every vocabulary the pi CLI has shipped under this runner, +// because pi is installed **unpinned** and its event names have already changed +// once. `agent_end` + `message_update` are the older builds; pi 0.87.x ends a turn +// with `{"type":"message_end","message":{...,"role":"assistant"}}` and closes the +// stream with `agent_settled`. +// +// A vocabulary the runner does not recognise yields "no result found in pi CLI +// output" on a run that in fact succeeded — so the symptom points at the model +// while the defect is in the parser, and only a real prompt through the runner +// exercises the path at all. That is how it survived: the fleet's working agents +// run older images and the ones built recently had never been prompted. func extractEventText(event piEvent) string { switch event.Type { case "agent_end": return lastAssistantText(event.Messages) case "message_update": return lastTextContent(event.Message.Content) + case "message_end": + // The role guard is load-bearing. message_end is emitted for *every* role, + // so without it a user or system message would be returned as the answer — + // the run would look like a success and echo the prompt back. + if event.Message.Role != "assistant" { + return "" + } + return lastTextContent(event.Message.Content) } return "" } diff --git a/pi/pi-runner_test.go b/pi/pi-runner_test.go index 50b1dbc..8eb067f 100644 --- a/pi/pi-runner_test.go +++ b/pi/pi-runner_test.go @@ -97,3 +97,73 @@ printf '{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":" Expect(result.Result).NotTo(ContainSubstring("--no-session")) }) }) + +var _ = Describe("piRunner event vocabulary", func() { + var ( + ctx context.Context + shimDir string + originalPath string + ) + + // Each spec installs a shim emitting one vocabulary's worth of output. The + // runner has to read the answer out of all of them: pi is installed unpinned, + // so its event names have already changed once and can change again. + setShim := func(script string) { + shimPath := filepath.Join(shimDir, "pi") + Expect(os.WriteFile(shimPath, []byte(script), 0755)).To(Succeed()) //nolint:gosec + } + + BeforeEach(func() { + ctx = context.Background() + shimDir = GinkgoT().TempDir() + originalPath = os.Getenv("PATH") + Expect(os.Setenv("PATH", shimDir+":"+originalPath)).To(Succeed()) + DeferCleanup(func() { + Expect(os.Setenv("PATH", originalPath)).To(Succeed()) + }) + }) + + It("reads the answer from the older agent_end vocabulary", func() { + setShim(`#!/bin/sh +printf '{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"OLD"}]}]}\n' +`) + + result, err := pi.NewRunner(pi.PiRunnerConfig{}).Run(ctx, "test") + + Expect(err).NotTo(HaveOccurred()) + Expect(result.Result).To(Equal("OLD")) + }) + + It("reads the answer from the message_end vocabulary pi 0.87.x emits", func() { + // Observed live on 2026-09-26: a v0.4.0 agent-pi pod produced exactly this + // stream and the runner answered "no result found in pi CLI output" — on a + // run that had in fact answered correctly. The symptom points at the model; + // the defect is here. + setShim(`#!/bin/sh +printf '{"type":"session","version":3}\n' +printf '{"type":"message_end","message":{"role":"system","content":[{"type":"text","text":"SYSTEM"}]}}\n' +printf '{"type":"message_end","message":{"role":"assistant","content":[{"type":"thinking","thinking":"hmm"},{"type":"text","text":"NEW"}]}}\n' +printf '{"type":"agent_settled"}\n' +`) + + result, err := pi.NewRunner(pi.PiRunnerConfig{}).Run(ctx, "test") + + Expect(err).NotTo(HaveOccurred()) + Expect(result.Result).To(Equal("NEW")) + }) + + It("does not return a non-assistant message as the answer", func() { + // The role guard is the whole point: message_end fires for every role, so + // without it a run producing no assistant text would echo the system prompt + // back as a successful result — the worst kind of failure, since it looks + // like an answer. + setShim(`#!/bin/sh +printf '{"type":"message_end","message":{"role":"system","content":[{"type":"text","text":"SYSTEM PROMPT"}]}}\n' +`) + + _, err := pi.NewRunner(pi.PiRunnerConfig{}).Run(ctx, "test") + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("no result found")) + }) +})