diff --git a/slop/plans/20260908_intercom-supervision.md b/slop/plans/20260908_intercom-supervision.md index b34e992..9bd8723 100644 --- a/slop/plans/20260908_intercom-supervision.md +++ b/slop/plans/20260908_intercom-supervision.md @@ -42,7 +42,10 @@ The earlier asynchronous subagent failure (missing pi-client/unix) prevented the - [ ] F3 deeper delivery confirmation: Pi's void adapter can ack before an asynchronous enqueue failure. Future UAT must inject that failure, avoid reporting confirmed model delivery, and keep the instruction recoverable. See the [SDK source-backed limitation](../reviews/20260908-review-fixes.md). Parent approved keeping this protocol expansion out of the current fix commit. - [x] Remove F5 general Intercom actuator, reject F6 nested placeholders, and correct F7 goal/log hashing boundary. - [x] Add focused regressions and inspect final full test/typecheck/lint/build output. [57-test evidence](../reviews/20260908-review-fixes-validation.txt); [initial child-environment failure and correction](../reviews/20260908-review-fixes-initial-validation.md). -- [ ] Parent independent post-change review before push. F8/F9 limitations and remaining native/UI/behavioral gaps are explicit in the [finding-by-finding disposition](../reviews/20260908-review-fixes.md). +- [x] Parent independent post-change review of `1668c94` completed and found P1–P5; [attributed review](../reviews/20260908-independent-post-fix-review.md). +- [x] Fix P1–P5: explicit hello request/reply, not-ready until worker-model success, peer-specific recovery guidance, one current-plan boundary and cancellation-safe Ready waits. [Disposition and provenance](../reviews/20260908-handshake-fixes.md); [67-test full validation](../reviews/20260908-handshake-validation.txt). Real two-adapter regressions replace reliance on automatic hello replies for handshake acceptance. +- [x] F8 usage observability: warn once for an unavailable usage result, not for normal post-compaction null tokens. No estimator or changed auto-compaction policy. +- [ ] Parent independent review of these handshake fixes before push. F3 durable enqueue confirmation stays open; F8 persistent unknown/null usage, F9 trusted-repo guardrails, and native/UI/behavioral gaps remain documented. Recovery operations were exercised only with isolated mocks/native test processes; existing user panes were not operated. Implementation worker commits locally only; parent owns review and push. diff --git a/slop/reviews/20260908-handshake-boundary-initial-validation.txt b/slop/reviews/20260908-handshake-boundary-initial-validation.txt new file mode 100644 index 0000000..a6b8212 --- /dev/null +++ b/slop/reviews/20260908-handshake-boundary-initial-validation.txt @@ -0,0 +1,122 @@ + +> @wassname2/pi-goals@0.2.2 test +> vitest run --reporter=verbose + + + RUN v4.1.9 /home/code/.pi/agent/git/github.com/wassname/pi-goals + + ✓ test/package-agent.test.ts > package manifest > includes the extension without registering a packaged subagent 3ms +stdout | test/role-models.test.ts > remembers each role without automatic switching overwriting another role +Role preferences restored: planning=planner, worker=small-worker, supervisor=astra; restore events did not overwrite the choice. + + ✓ test/intercom.test.ts > pi-intercom transport > receives exact advice once, acknowledges it and rejects unrelated peers 4ms + ✓ test/intercom.test.ts > pi-intercom transport > restores an unacknowledged steer on reconnect and stops replay after acknowledgment 2ms + ✓ test/intercom.test.ts > pi-intercom transport > advances the incremental overview only after acknowledgment 1ms + ✓ test/intercom.test.ts > pi-intercom transport > cancels a readiness wait on shutdown 1ms + ✓ test/intercom.test.ts > does not acknowledge a synchronous handoff failure, and retries the instruction 1ms + ✓ test/intercom.test.ts > detaches a completed binding and ignores its late advice without replay errors or false acceptance 1ms + ✓ test/role-models.test.ts > remembers each role without automatic switching overwriting another role 32ms + ✓ test/role-models.test.ts > fails on an unavailable remembered model without replacing the choice 5ms + ✓ test/intercom-handshake.test.ts > re-handshakes unchanged peers in either direction without hello ping-pong or lost advice 9ms + ✓ test/intercom-handshake.test.ts > replays pending advice and views across either role's own readiness transition 2ms + ✓ test/intercom-handshake.test.ts > cancels pending waits immediately on detach or reconfiguration 2ms + ✓ test/worker-view.test.ts > keeps human direction and source location while sending only new messages 3ms + ✓ test/worker-view.test.ts > restarts after compaction and does not report historical tool calls as active 1ms + ✓ test/worker-view.test.ts > bounds serialized Unicode and quoted logs while marking omissions 3ms + ✓ test/approval.test.ts > hashes only the current goal, excluding the log, interview, and their historical goal text 8ms + ✓ test/prompts.test.ts > planning prompt > requires fact finding or a focused question before a goal 2ms + ✓ test/prompts.test.ts > planning prompt > restores the same rule after compaction 2ms + ✓ test/prompts.test.ts > planning prompt > anchors work and sign-off to the user-visible result 0ms + ✓ test/herdr.test.ts > supervisor pane command > forks the planning session with pi-goals owning its Intercom dependency 3ms + ✓ test/herdr.test.ts > supervisor pane command > accepts Herdr's text version output and stale pane cleanup 51ms + ✓ test/background.test.ts > reports tracked running work, rather than equating idle agent with finished jobs 52ms + ✓ test/background.test.ts > distinguishes missing providers from an unavailable installed tracker 5ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > keeps the title, user voice and goals 2ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > drops the log, the learnings and the unlimited appendix 1ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > returns the whole plan when there is no ## Log yet (a fresh draft) 0ms + ✓ test/fold.test.ts > openSubtasks (the widget shows the next action, so the plan IS the task list) > lists the active goal's open and in-progress subtasks, stopping at the next goal 1ms + ✓ test/fold.test.ts > openSubtasks (the widget shows the next action, so the plan IS the task list) > does not leak subtasks from the goal below 1ms + ✓ test/fold.test.ts > does not show historical Log subtasks under the last active goal 0ms + ✓ test/append-log.test.ts > appendLog (the extension's only plan-file write) > creates ## Log at EOF when absent 2ms + ✓ test/append-log.test.ts > appendLog (the extension's only plan-file write) > appends after the last existing log line, before any following header 0ms +stdout | test/intercom-broker.test.ts > exchanges readiness, views and exact advice over a real isolated pi-intercom broker +Intercom broker: readiness confirmed; exact worker view and supervisor advice received. + + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > ticks the exact-matching goal line, case-insensitive, leaving subtasks alone 2ms + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > returns null on wording drift (fuzzy matching is the judge's job, not TypeScript's) 0ms + × test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > returns null when the subject matches more than one goal line 5ms + → expected '# Plan\n\n## Goals\n\n1. [/] goal: Im…' to be null + ✓ test/intercom-broker.test.ts > exchanges readiness, views and exact advice over a real isolated pi-intercom broker 254ms + ✓ test/goals-flow.test.ts > /goals flow > reports actual idle state, invalidates stopped views on start, and stops completed plans 59ms + ✓ test/goals-flow.test.ts > /goals flow > preserves drafts, records the interview, and keeps planning read-only 33ms + ✓ test/goals-flow.test.ts > /goals flow > forks a visible supervisor on Ready and keeps the main session as worker 27ms + ✓ test/goals-flow.test.ts > /goals flow > starts work only after the supervisor launcher resolves 38ms + ✓ test/goals-flow.test.ts > /goals flow > delivers an Intercom instruction to the worker 41ms + ✓ test/goals-flow.test.ts > /goals flow > closes the supervisor on clear but keeps the plan file 27ms + ✓ test/goals-flow.test.ts > /goals flow > accepts only an approval for the exact clean commit and goal block 61ms + ✓ test/goals-flow.test.ts > process role > keeps subagent children and visible supervisors out of the worker extension 0ms + ✓ test/goals-flow.test.ts > restores working linkage even when its remembered model is unavailable, and supports explicit recovery 21ms + ✓ test/goals-flow.test.ts > restores planning linkage even when its remembered model is unavailable, and supports explicit recovery 23ms + ✓ test/goals-flow.test.ts > shows a missing resumed supervisor, pauses writes, and automatically unpauses when that peer returns 26ms + ✓ test/goals-flow.test.ts > times out stale Ready retries in five seconds, without replacing the pane automatically 29ms + ✓ test/goals-flow.test.ts > explicitly restarts only the tracked pane, keeps the plan, and invalidates old approval binding 24ms + ✓ test/goals-flow.test.ts > does not persist startup results or launch work after session shutdown 23ms + ✓ test/goals-flow.test.ts > keeps a failed Ready model not-ready and recovers the same real supervisor binding 31ms + ✓ test/goals-flow.test.ts > points a present-but-paused peer recovery at the supervisor pane 21ms + ✓ test/goals-flow.test.ts > clear during the initial Ready wait cancels immediately and cannot resurrect the plan 25ms + ✓ test/goals-flow.test.ts > clear before the launcher resolves rejects late pane callbacks without restoring the binding 21ms + ✓ test/supervisor-session.test.ts > visible supervisor session > restores monitoring and read-only tools without replaying persisted views 8ms + ✓ test/supervisor-session.test.ts > visible supervisor session > renders all advice in real Pi tool rows, including collapsed and restored rows 21ms + ✓ test/supervisor-session.test.ts > visible supervisor session > asks for judgment and useful recaps without inventing instructions 1ms + ✓ test/supervisor-session.test.ts > visible supervisor session > writes readiness only after removing writing tools 2ms + ✓ test/supervisor-session.test.ts > visible supervisor session > compacts a large planning fork before writing readiness 1ms + ✓ test/supervisor-session.test.ts > visible supervisor session > does not become ready when initial compaction fails 2ms + ✓ test/supervisor-session.test.ts > visible supervisor session > writes a durable worker instruction 1ms + ✓ test/supervisor-session.test.ts > visible supervisor session > records approval only from a stopped view with evidence and no active work 96ms + ✓ test/supervisor-session.test.ts > blocks the general intercom actuator even if enabled after startup 3ms + ✓ test/supervisor-session.test.ts > keeps a supervisor unready after model restoration failure, then recovers explicitly without substituting a model 2ms + ✓ test/supervisor-session.test.ts > warns once on unavailable usage but stays quiet for Pi's post-compaction null token sample 3ms + ✓ test/rpc-review.test.ts > RPC review flow > opens Refine's editor before it starts the revision turn 1541ms +stdout | test/native-intercom.test.ts > runs a forked Pi supervisor and receives its exact instruction in another Pi session +Native Pi pair: fork retained planning context; SteerWorker delivered exactly: Read the real outputs before declaring completion. + + ✓ test/native-intercom.test.ts > runs a forked Pi supervisor and receives its exact instruction in another Pi session 2503ms + +⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯ + + FAIL test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > returns null when the subject matches more than one goal line +AssertionError: expected '# Plan\n\n## Goals\n\n1. [/] goal: Im…' to be null + +- Expected: +null + ++ Received: +"# Plan + +## Goals + +1. [/] goal: Implement the cache layer + - tasks: + 1. [x] wire client +2. [x] goal: Ship the docs + +## Log +3. [ ] goal: Ship the docs +" + + ❯ test/tick-goal.test.ts:30:42 + 28| it("returns null when the subject matches more than one goal line", (… + 29| const dup = `${plan}3. [ ] goal: Ship the docs\n`; + 30| expect(tickGoal(dup, "Ship the docs")).toBeNull(); + | ^ + 31| }); + 32| }); + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯ + + + Test Files 1 failed | 16 passed (17) + Tests 1 failed | 65 passed (66) + Start at 19:22:13 + Duration 2.84s (transform 2.51s, setup 0ms, import 5.72s, tests 5.21s, environment 2ms) + diff --git a/slop/reviews/20260908-handshake-fixes.md b/slop/reviews/20260908-handshake-fixes.md new file mode 100644 index 0000000..3363eda --- /dev/null +++ b/slop/reviews/20260908-handshake-fixes.md @@ -0,0 +1,42 @@ +# Post-review P1–P5 fixes — Pi/OpenAI implementation worker + +Baseline: `1668c94`. Independent review: [attributed, preserved source](20260908-independent-post-fix-review.md), reviewer run `bdb93a2e-52f4-4a6d-bac2-7c9eb48118b5`. + +Read AGENTS.md and installed Pi extension docs for lifecycle teardown, commands, model selection and compaction before editing. No user pane/session was operated; tests use isolated mocks/brokers/native Pi processes. No nested delegation or push. + +## Dispositions + +- **P1:** Replaced change-gated hello replies with an explicit request/reply bit. Every request receives one reply even when the peer state is unchanged; a reply never elicits another hello. Each successful exchange retries only still-pending instructions/current view, including after either side's own readiness transition. Repeated wire frames are allowed and deduplicated at the receiving adapter; this is not a new durable-delivery claim. `test/intercom-handshake.test.ts` wires **two real GoalIntercom adapters**, not an auto-ready peer. It checks repeated worker and supervisor reconfiguration, concurrent reset (four hello frames), single-sided reset (two frames), exact advice handoff, own-ready pause/resume, pending advice/view replay, and reconnect deduplication. Both peers must load the updated transport; mixed-version reconnect is not claimed supported. +- **P2:** The worker configures not-ready during startup/recovery. Startup can await peer readiness without claiming implementation readiness. It announces ready only after worker model restoration and the phase transition to working. Model failures remain not-ready. The Ready/retry flow test uses the same two real adapters and proves unavailable worker model → not-ready → `/model` + reconnect → Ready → working on the **same** binding/pane, followed by successful advice after healthy reconnect. Planning reconnect alone does not authorize implementation. +- **P3:** A known peer with incomplete readiness is distinguished from an absent peer. Guidance points to the supervisor pane's compaction/model diagnostics and `/model` + `/goals reconnect`, rather than treating every pause as disconnection. Regression checks widget and prompt guidance. +- **P4:** `src/plan.ts` now owns the single goal-line and Log-fold definitions. Widget scanning, subtasks, goal ticking and approval use the same current-plan boundary. Tick still rejects duplicate active-region matches; historical Log copies remain byte-for-byte unchanged. The existing approval/sign-off flow now includes duplicate/historical goal lines in the Log, succeeds and ends the active plan instead of reopening historical goals. +- **P5:** Detach/reconfigure immediately reject old readiness waiters. A small Ready-attempt identity plus plan-version guard invalidates asynchronous startup results on clear/recovery/replacement; stale menu/editor responses are also ignored. Regressions clear while the five-minute initial wait is pending (no clock advance needed to settle), then advance five minutes and verify no resurrection; a second test clears before the launcher callback resolves and verifies no late binding/pane persistence or work launch. No automatic late-pane kill was added. +- **F8 observability:** One warning per runtime if the entire usage result is unavailable at a settled check. No warning for Pi's ordinary post-compaction `tokens: null` sample. No speculative token estimator, new compaction policy, or change to Pi auto-compaction. Persistent null usage still cannot trigger the custom 100k check. + +## Verification and changed old assertions + +[Full successful commands/output](20260908-handshake-validation.txt): **67 tests passed in 17 files**, followed by successful typecheck, lint, build and `git diff --check`. The exact main-session test command unsets `PI_SUBAGENT_CHILD` and `PI_GOALS_ROLE` and sets a **fresh explicit** `PI_GOALS_EVIDENCE_DIR` to `slop/reviews/handshake-native`. + +Two intermediate failures are preserved, not counted as passes: + +1. [Handshake-focused run](20260908-handshake-initial-validation.txt): 26 passed/1 failed. The old assertion demanded exactly one outbound wire retry after configure+markReady. These now generate separate request/reply exchanges, which can retry the same still-unacked id more than once before its ack. Updated assertion requires at least one retry, every retry's exact id/text, and no further retries after ack. Two-real-adapter tests independently require exactly one user handoff/view callback after duplicate wire delivery. +2. [First full boundary run](20260908-handshake-boundary-initial-validation.txt): 65 passed/1 failed. Its old duplicate-goal fixture appended the duplicate **below `## Log`**, precisely the P4 behavior being corrected. The duplicate-rejection test now inserts the duplicate above the fold and still requires null; an added test requires historical copies below the fold to be ignored and unchanged. + +An intermediate typecheck passed; lint initially flagged import order and a nested assignment. Those were corrected; the full final lint passed with no fixes applied. + +[Native log inspection](20260908-handshake-log-inspection.txt) reads the final worker/supervisor event files, verifies zero error records and empty stderr, and matches the outgoing instruction, worker incoming instruction and adapter ack id `22068654-6e7c-4328-890f-9382b25c8ea7`. The worker user message is exactly `[supervisor] Read the real outputs before declaring completion.` This deterministic native test proves context retention/routing/tool exposure, not autonomous judgment or durable enqueue guarantees. + +## Evidence provenance caveat + +At the first inspection, these two tracked files were **already dirty**: + +- `slop/reviews/review-fixes-native/supervisor-events.jsonl` +- `slop/reviews/review-fixes-native/worker-events.jsonl` + +The inherited `PI_GOALS_EVIDENCE_DIR` pointed there. The first full test run accidentally refreshed them again. Their pre-task uncommitted bytes were not captured, so it is not established that all differences from HEAD were produced by this worker. They are left **unstaged and uncommitted**, not restored over unknown prior edits. The refreshed copies were separately preserved under this worker's output directory, `scratch-refreshed-prior-evidence/`. Final evidence uses only the fresh `handshake-native/` directory and is not mixed with these prior paths. + +## Still open / acceptance limits + +**F3 async enqueue confirmation remains open.** Pi's void `sendUserMessage` wrapper can return before a later async enqueue rejection. An adapter ack is not durable enqueue, model receipt, or execution confirmation. No correlated-receipt protocol was added; the future forced-async-rejection UAT remains required. A synchronous handoff failure still remains unacked for retry. + +Parent/reviewer owns independent post-change review and push. Rendered Herdr acceptance, the complete two-native-session ApproveGoal → CompleteGoal chain, fresh-shell role recovery without launcher environment, useful independent judgment and cost savings remain unproven. Planning/pause shell gates remain trusted-repo guardrails, not a security sandbox. If a pane split finishes after cancellation, it can remain untracked for human inspection; this change prevents stale state resurrection rather than operating a late pane automatically. diff --git a/slop/reviews/20260908-handshake-initial-validation.txt b/slop/reviews/20260908-handshake-initial-validation.txt new file mode 100644 index 0000000..5e12c54 --- /dev/null +++ b/slop/reviews/20260908-handshake-initial-validation.txt @@ -0,0 +1,77 @@ + +> @wassname2/pi-goals@0.2.2 test +> vitest run --run test/intercom-handshake.test.ts test/intercom.test.ts test/goals-flow.test.ts --reporter=verbose + + + RUN v4.1.9 /home/code/.pi/agent/git/github.com/wassname/pi-goals + + ✓ test/intercom-handshake.test.ts > re-handshakes unchanged peers in either direction without hello ping-pong or lost advice 7ms + ✓ test/intercom-handshake.test.ts > replays pending advice and views across either role's own readiness transition 1ms + ✓ test/intercom-handshake.test.ts > cancels pending waits immediately on detach or reconfiguration 1ms + ✓ test/intercom.test.ts > pi-intercom transport > receives exact advice once, acknowledges it and rejects unrelated peers 4ms + × test/intercom.test.ts > pi-intercom transport > restores an unacknowledged steer on reconnect and stops replay after acknowledgment 7ms + → expected [ { binding: 'binding', …(4) }, …(1) ] to match object [ { …(2) } ] + ✓ test/intercom.test.ts > pi-intercom transport > advances the incremental overview only after acknowledgment 1ms + ✓ test/intercom.test.ts > pi-intercom transport > cancels a readiness wait on shutdown 1ms + ✓ test/intercom.test.ts > does not acknowledge a synchronous handoff failure, and retries the instruction 1ms + ✓ test/intercom.test.ts > detaches a completed binding and ignores its late advice without replay errors or false acceptance 3ms + ✓ test/goals-flow.test.ts > /goals flow > reports actual idle state, invalidates stopped views on start, and stops completed plans 34ms + ✓ test/goals-flow.test.ts > /goals flow > preserves drafts, records the interview, and keeps planning read-only 18ms + ✓ test/goals-flow.test.ts > /goals flow > forks a visible supervisor on Ready and keeps the main session as worker 24ms + ✓ test/goals-flow.test.ts > /goals flow > starts work only after the supervisor launcher resolves 22ms + ✓ test/goals-flow.test.ts > /goals flow > delivers an Intercom instruction to the worker 18ms + ✓ test/goals-flow.test.ts > /goals flow > closes the supervisor on clear but keeps the plan file 23ms + ✓ test/goals-flow.test.ts > /goals flow > accepts only an approval for the exact clean commit and goal block 47ms + ✓ test/goals-flow.test.ts > process role > keeps subagent children and visible supervisors out of the worker extension 0ms + ✓ test/goals-flow.test.ts > restores working linkage even when its remembered model is unavailable, and supports explicit recovery 18ms + ✓ test/goals-flow.test.ts > restores planning linkage even when its remembered model is unavailable, and supports explicit recovery 16ms + ✓ test/goals-flow.test.ts > shows a missing resumed supervisor, pauses writes, and automatically unpauses when that peer returns 17ms + ✓ test/goals-flow.test.ts > times out stale Ready retries in five seconds, without replacing the pane automatically 26ms + ✓ test/goals-flow.test.ts > explicitly restarts only the tracked pane, keeps the plan, and invalidates old approval binding 23ms + ✓ test/goals-flow.test.ts > does not persist startup results or launch work after session shutdown 23ms + ✓ test/goals-flow.test.ts > keeps a failed Ready model not-ready and recovers the same real supervisor binding 23ms + ✓ test/goals-flow.test.ts > points a present-but-paused peer recovery at the supervisor pane 18ms + ✓ test/goals-flow.test.ts > clear during the initial Ready wait cancels immediately and cannot resurrect the plan 21ms + ✓ test/goals-flow.test.ts > clear before the launcher resolves rejects late pane callbacks without restoring the binding 35ms + +⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯ + + FAIL test/intercom.test.ts > pi-intercom transport > restores an unacknowledged steer on reconnect and stops replay after acknowledgment +AssertionError: expected [ { binding: 'binding', …(4) }, …(1) ] to match object [ { …(2) } ] + +- Expected ++ Received + + [ + { ++ "binding": "binding", ++ "id": "7f4566ea-f2ef-4a14-ad75-e138fc3a26a0", ++ "kind": "steer", ++ "role": "supervisor", ++ "text": "Read the full output.", ++ }, ++ { ++ "binding": "binding", + "id": "7f4566ea-f2ef-4a14-ad75-e138fc3a26a0", ++ "kind": "steer", ++ "role": "supervisor", + "text": "Read the full output.", + }, + ] + + ❯ test/intercom.test.ts:45:76 + 43| resumed.link.markReady(); + 44| await resumed.link.waitReady(); + 45| expect(resumed.fixture.sent.filter(message => message.kind === "stee… + | ^ + 46| resumed.fixture.receive({ binding: "binding", role: "worker", kind: … + 47| resumed.fixture.connect(false); + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯ + + + Test Files 1 failed | 2 passed (3) + Tests 1 failed | 26 passed (27) + Start at 19:20:05 + Duration 861ms (transform 365ms, setup 0ms, import 583ms, tests 437ms, environment 0ms) + diff --git a/slop/reviews/20260908-handshake-log-inspection.txt b/slop/reviews/20260908-handshake-log-inspection.txt new file mode 100644 index 0000000..df15d8b --- /dev/null +++ b/slop/reviews/20260908-handshake-log-inspection.txt @@ -0,0 +1,8 @@ +slop/reviews/handshake-native/worker-events.jsonl: 22 events; 0 error records; stderr 0 bytes +worker: ack received cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc +worker: ack received cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc +worker: in steer 22068654-6e7c-4328-890f-9382b25c8ea7 +worker: [{"type":"text","text":"[supervisor] Read the real outputs before declaring completion."}] +slop/reviews/handshake-native/supervisor-events.jsonl: 21 events; 0 error records; stderr 0 bytes +supervisor: out steer 22068654-6e7c-4328-890f-9382b25c8ea7 +supervisor: ack received 22068654-6e7c-4328-890f-9382b25c8ea7 diff --git a/slop/reviews/20260908-handshake-validation.txt b/slop/reviews/20260908-handshake-validation.txt new file mode 100644 index 0000000..410dbf5 --- /dev/null +++ b/slop/reviews/20260908-handshake-validation.txt @@ -0,0 +1,107 @@ ++ env -u PI_SUBAGENT_CHILD -u PI_GOALS_ROLE PI_GOALS_EVIDENCE_DIR=/home/code/.pi/agent/git/github.com/wassname/pi-goals/slop/reviews/handshake-native npm test -- --reporter=verbose + +> @wassname2/pi-goals@0.2.2 test +> vitest run --reporter=verbose + + + RUN v4.1.9 /home/code/.pi/agent/git/github.com/wassname/pi-goals + + ✓ test/background.test.ts > reports tracked running work, rather than equating idle agent with finished jobs 5ms + ✓ test/background.test.ts > distinguishes missing providers from an unavailable installed tracker 47ms + ✓ test/package-agent.test.ts > package manifest > includes the extension without registering a packaged subagent 3ms + ✓ test/herdr.test.ts > supervisor pane command > forks the planning session with pi-goals owning its Intercom dependency 3ms + ✓ test/herdr.test.ts > supervisor pane command > accepts Herdr's text version output and stale pane cleanup 26ms + ✓ test/prompts.test.ts > planning prompt > requires fact finding or a focused question before a goal 2ms + ✓ test/prompts.test.ts > planning prompt > restores the same rule after compaction 0ms + ✓ test/prompts.test.ts > planning prompt > anchors work and sign-off to the user-visible result 0ms + ✓ test/worker-view.test.ts > keeps human direction and source location while sending only new messages 3ms + ✓ test/worker-view.test.ts > restarts after compaction and does not report historical tool calls as active 1ms + ✓ test/worker-view.test.ts > bounds serialized Unicode and quoted logs while marking omissions 2ms +stdout | test/role-models.test.ts > remembers each role without automatic switching overwriting another role +Role preferences restored: planning=planner, worker=small-worker, supervisor=astra; restore events did not overwrite the choice. + + ✓ test/role-models.test.ts > remembers each role without automatic switching overwriting another role 14ms + ✓ test/role-models.test.ts > fails on an unavailable remembered model without replacing the choice 5ms + ✓ test/approval.test.ts > hashes only the current goal, excluding the log, interview, and their historical goal text 5ms + ✓ test/intercom-handshake.test.ts > re-handshakes unchanged peers in either direction without hello ping-pong or lost advice 9ms + ✓ test/intercom-handshake.test.ts > replays pending advice and views across either role's own readiness transition 1ms + ✓ test/intercom-handshake.test.ts > cancels pending waits immediately on detach or reconfiguration 2ms + ✓ test/intercom.test.ts > pi-intercom transport > receives exact advice once, acknowledges it and rejects unrelated peers 7ms + ✓ test/intercom.test.ts > pi-intercom transport > restores an unacknowledged steer on reconnect and stops replay after acknowledgment 4ms + ✓ test/intercom.test.ts > pi-intercom transport > advances the incremental overview only after acknowledgment 1ms + ✓ test/intercom.test.ts > pi-intercom transport > cancels a readiness wait on shutdown 1ms + ✓ test/intercom.test.ts > does not acknowledge a synchronous handoff failure, and retries the instruction 2ms + ✓ test/intercom.test.ts > detaches a completed binding and ignores its late advice without replay errors or false acceptance 1ms + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > ticks the exact-matching goal line, case-insensitive, leaving subtasks alone 2ms + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > returns null on wording drift (fuzzy matching is the judge's job, not TypeScript's) 0ms + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > returns null when the subject matches more than one goal line 0ms + ✓ test/tick-goal.test.ts > tickGoal (sign-off ticks the goal; agent only ticks on wording drift) > ignores a historical duplicate below the Log and leaves it unchanged 0ms + ✓ test/append-log.test.ts > appendLog (the extension's only plan-file write) > creates ## Log at EOF when absent 4ms + ✓ test/append-log.test.ts > appendLog (the extension's only plan-file write) > appends after the last existing log line, before any following header 1ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > keeps the title, user voice and goals 2ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > drops the log, the learnings and the unlimited appendix 1ms + ✓ test/fold.test.ts > foldPlan (current goals are above ## Log; durable memory is below it) > returns the whole plan when there is no ## Log yet (a fresh draft) 0ms + ✓ test/fold.test.ts > openSubtasks (the widget shows the next action, so the plan IS the task list) > lists the active goal's open and in-progress subtasks, stopping at the next goal 1ms + ✓ test/fold.test.ts > openSubtasks (the widget shows the next action, so the plan IS the task list) > does not leak subtasks from the goal below 1ms + ✓ test/fold.test.ts > does not show historical Log subtasks under the last active goal 0ms +stdout | test/intercom-broker.test.ts > exchanges readiness, views and exact advice over a real isolated pi-intercom broker +Intercom broker: readiness confirmed; exact worker view and supervisor advice received. + + ✓ test/goals-flow.test.ts > /goals flow > reports actual idle state, invalidates stopped views on start, and stops completed plans 47ms + ✓ test/goals-flow.test.ts > /goals flow > preserves drafts, records the interview, and keeps planning read-only 24ms + ✓ test/intercom-broker.test.ts > exchanges readiness, views and exact advice over a real isolated pi-intercom broker 236ms + ✓ test/goals-flow.test.ts > /goals flow > forks a visible supervisor on Ready and keeps the main session as worker 32ms + ✓ test/goals-flow.test.ts > /goals flow > starts work only after the supervisor launcher resolves 29ms + ✓ test/goals-flow.test.ts > /goals flow > delivers an Intercom instruction to the worker 26ms + ✓ test/goals-flow.test.ts > /goals flow > closes the supervisor on clear but keeps the plan file 20ms + ✓ test/goals-flow.test.ts > /goals flow > accepts only an approval for the exact clean commit and goal block 62ms + ✓ test/goals-flow.test.ts > process role > keeps subagent children and visible supervisors out of the worker extension 0ms + ✓ test/goals-flow.test.ts > restores working linkage even when its remembered model is unavailable, and supports explicit recovery 33ms + ✓ test/goals-flow.test.ts > restores planning linkage even when its remembered model is unavailable, and supports explicit recovery 24ms + ✓ test/goals-flow.test.ts > shows a missing resumed supervisor, pauses writes, and automatically unpauses when that peer returns 24ms + ✓ test/goals-flow.test.ts > times out stale Ready retries in five seconds, without replacing the pane automatically 25ms + ✓ test/goals-flow.test.ts > explicitly restarts only the tracked pane, keeps the plan, and invalidates old approval binding 42ms + ✓ test/goals-flow.test.ts > does not persist startup results or launch work after session shutdown 36ms + ✓ test/goals-flow.test.ts > keeps a failed Ready model not-ready and recovers the same real supervisor binding 31ms + ✓ test/goals-flow.test.ts > points a present-but-paused peer recovery at the supervisor pane 17ms + ✓ test/goals-flow.test.ts > clear during the initial Ready wait cancels immediately and cannot resurrect the plan 25ms + ✓ test/goals-flow.test.ts > clear before the launcher resolves rejects late pane callbacks without restoring the binding 19ms + ✓ test/supervisor-session.test.ts > visible supervisor session > restores monitoring and read-only tools without replaying persisted views 7ms + ✓ test/supervisor-session.test.ts > visible supervisor session > renders all advice in real Pi tool rows, including collapsed and restored rows 22ms + ✓ test/supervisor-session.test.ts > visible supervisor session > asks for judgment and useful recaps without inventing instructions 1ms + ✓ test/supervisor-session.test.ts > visible supervisor session > writes readiness only after removing writing tools 2ms + ✓ test/supervisor-session.test.ts > visible supervisor session > compacts a large planning fork before writing readiness 1ms + ✓ test/supervisor-session.test.ts > visible supervisor session > does not become ready when initial compaction fails 2ms + ✓ test/supervisor-session.test.ts > visible supervisor session > writes a durable worker instruction 1ms + ✓ test/rpc-review.test.ts > RPC review flow > opens Refine's editor before it starts the revision turn 1350ms + ✓ test/supervisor-session.test.ts > visible supervisor session > records approval only from a stopped view with evidence and no active work 87ms + ✓ test/supervisor-session.test.ts > blocks the general intercom actuator even if enabled after startup 2ms + ✓ test/supervisor-session.test.ts > keeps a supervisor unready after model restoration failure, then recovers explicitly without substituting a model 2ms + ✓ test/supervisor-session.test.ts > warns once on unavailable usage but stays quiet for Pi's post-compaction null token sample 2ms +stdout | test/native-intercom.test.ts > runs a forked Pi supervisor and receives its exact instruction in another Pi session +Native Pi pair: fork retained planning context; SteerWorker delivered exactly: Read the real outputs before declaring completion. + + ✓ test/native-intercom.test.ts > runs a forked Pi supervisor and receives its exact instruction in another Pi session 2530ms + + Test Files 17 passed (17) + Tests 67 passed (67) + Start at 19:24:07 + Duration 2.84s (transform 2.36s, setup 0ms, import 6.39s, tests 4.95s, environment 3ms) + ++ npm run typecheck + +> @wassname2/pi-goals@0.2.2 typecheck +> tsc --noEmit + ++ npm run lint + +> @wassname2/pi-goals@0.2.2 lint +> biome check src/ test/ + +Checked 32 files in 91ms. No fixes applied. ++ npm run build + +> @wassname2/pi-goals@0.2.2 build +> tsc + ++ git diff --check diff --git a/slop/reviews/20260908-independent-post-fix-review.md b/slop/reviews/20260908-independent-post-fix-review.md new file mode 100644 index 0000000..6ec5186 --- /dev/null +++ b/slop/reviews/20260908-independent-post-fix-review.md @@ -0,0 +1,100 @@ +# Post-fix independent bug review — pi-goals supervision recovery + +Scope: changes `2824396..1668c94` (fix commit `325b939` + evidence commit `1668c94`). +Inputs: AGENTS.md, `slop/reviews/20260908-review-fixes.md` (dispositions), `slop/reviews/20260908-independent-supervision-bug-review.md` (original F1–F9), full current sources of `src/{index,intercom,supervisor-session,approval,role-models,herdr}.ts`, changed tests, native evidence logs. +Read-only: no repo edits, no live panes, no messaging. Repro artifacts lived in /tmp only. + +Labels: **[TESTED]** = executed/compiled and observed. **[CODE]** = unambiguous from source. **[INFERENCE]** = depends on runtime behavior I could not observe here. Test passage is treated as evidence of the asserted path only, not as design proof. + +Baseline check: `env -u PI_SUBAGENT_CHILD -u PI_GOALS_ROLE npx vitest run` → 57/57 pass in 16 files, matching the worker's validation claim. **[TESTED]** + +--- + +## Fix verification F1–F7 + +**F1 (unavailable remembered model aborts restore) — verified fixed. [CODE + repo tests]** +`session_start` now sets `modelError` first, configures the binding and timers independent of model selection, and only then attempts `restoreModel` in try/catch (`src/index.ts:557-580`). On failure: no `setModel`, no ready hello (`configure(..., false)` at :562, `markReady` gated on `!modelError` at :572-573), paused widget (`pauseReason`/`updateWidget` :149-153, 271-276), write/sign-off gating (:453-456, 592-597), human input and read-only diagnosis retained. `/model` saves over the failed role choice because `RoleModels.enter` sets `this.role` before throwing (`src/role-models.ts:29-30`), so `/goals reconnect` then picks up the user's replacement — a deliberate, workable recovery chain. No fallback model is substituted anywhere. + +**F2 (resumed worker silently unsupervised) — verified fixed, with a residual in P3 below. [CODE + repo tests]** +`connected` now requires own-ready + binding + channel + peerReady (`src/intercom.ts:87`); `onConnectionChange` refreshes the widget immediately (:139, :145, :164); a resumed working worker hellos not-ready until its model restores, then `waitReady(5000)` and warns on failure (`src/index.ts:572-577`). Returning peers clear the pause automatically via the hello/`changed` path. Explicit `/goals restart` preserves plan/version and replaces only the tracked pane with a fresh approval binding (test asserts old checkpoint removed, plan unchanged, exactly one close). + +**F3 (inactive-plan steer throws/replays) — lifecycle part fixed; delivery gap honestly open. [CODE + repo tests]** +`detach()` (`src/intercom.ts:72-81`) clears the binding on completion/clear/restart; late steers for a dead binding are dropped at the binding check (:151) without invoking `onSteer` or acking — covered by the new intercom tests. Ack ordering is still handoff-before-ack (:186-193), so a synchronous `sendUserMessage` throw leaves the steer unrecorded/unacked and it retries (test proves retry succeeds). The documented residual is real and correctly **not** claimed fixed: Pi 0.84.1's `sendUserMessage` is a void wrapper over an async enqueue, so an ack can precede an asynchronous enqueue failure, after which the instruction is gone from the supervisor's pending set and never reaches the model — silently. This is a genuine gap a future correlated-receipt protocol should close; the recorded UAT requirement (force an async enqueue rejection) is the right acceptance test. Acceptable as an explicitly-open item, not as a resolved one. + +**F4 (stale pane → repeated 5-minute Ready waits) — verified fixed for the timeout itself. [CODE + repo tests]** +Existing-pane reconnect waits are 5s (`src/index.ts:186`, `:323`); first launch keeps the 300s compaction allowance (:213); failed `pane run` retains and reports the pane ID (:204-208); no automatic pane kill. **However, see P1: in one important sub-scenario the 5s retry fails deterministically even when the supervisor is healthy, so the fix's "retry" guidance does not actually recover there.** + +**F5 (supervisor gets general `intercom` tool) — verified fixed. [CODE + native test]** +`intercom` is in `BLOCKED_TOOLS` (`src/supervisor-session.ts:13`), filtered from active tools at start and bootstrap (:108-110, :146), and blocked at tool-call time even if re-enabled (:170-173). The native pair test asserts the real supervisor model request's tool list excludes `intercom` and `bash`. Note the tool gate uses `terminate: true`, so a blocked call ends the whole supervisor turn — a deliberate-looking but behaviorally stronger choice than the planning gate's plain block. + +**F6 (nested placeholder evidence) — verified fixed as far as claimed. [CODE + repo tests]** +Both the inline and child-bullet paths reject exactly `(empty until sign-off)` case-insensitively (`src/supervisor-session.ts:46,52`); the string matches the template in `src/prompts.ts:97`. Residual (documented as a presence floor): the child scan still accepts *any* deeper-indented nonblank bullet as evidence, and quoted variants like `"(empty until sign-off)"` pass. That is a semantic-judging limit, accurately disclaimed; supervisor judgment remains the real gate. Acceptable. + +**F7 (last-goal hash includes Log/Interview) — verified fixed. [CODE + TESTED via unit tests]** +`goalBlock` now truncates the plan at `## Log` before scanning and ends the block at the next goal line or any `#`/`##` heading, with `trimEnd` (`src/approval.ts:43-58`). `approval.test.ts` covers duplicate goal text in the log, Interview section, next-goal boundary, and real block edits invalidating the hash; the flow test proves a manual log line between ApproveGoal and CompleteGoal no longer invalidates. Fail-closed on upgrade (old hashes need re-approval) is the right direction. One residual asymmetry (P4 below). + +--- + +## New findings + +### P1 — Reconnect/Ready-retry can wedge in a half-open state: peer hello is only re-sent on *perceived* state change (Medium) [TESTED] + +`GoalIntercom.configure()` resets `peer`/`peerReady` (`src/intercom.ts:45-46`) and sends a hello, but the peer replies to a hello only when *its own* view changed (`src/intercom.ts:154-164`: `changed = !this.peer || this.peerReady !== message.ready; if (changed) this.hello()`). There is no periodic hello. If the peer's stored state already matches the incoming hello (same session id, same ready flag), it stays silent — so the side that reconfigured never learns the peer and `connected` stays false forever (until some unrelated broker `session_joined` at :148 happens to trigger a hello). + +Reproduced in isolation (compiled real `src/intercom.ts` from HEAD, two links cross-wired as broker peers, script in /tmp, no repo changes): + +``` +after initial link: worker.connected = true supervisor.connected = true +after worker reconfigure: worker.connected = false supervisor.connected = true +waitReady REJECTED after 201 ms: Supervisor did not become ready through pi-intercom; inspect its pane. +supervisor steer() succeeded (reports sent) +worker delivered steer: null +``` + +Consequences, all reachable through new/changed code paths: + +1. **`/goals reconnect` on a healthy link breaks it.** `src/index.ts:322-323` reconfigures the same binding; if nothing about readiness actually changed, every retry times out after 5s with "Goal recovery failed", and the only in-app escape is `/goals restart` — which closes the *healthy* pane and invalidates the approval binding (`beginReview` deletes checkpoints). +2. **The F1-on-Ready retry fails deterministically.** First Ready: `startSupervisor` completes the hello exchange (worker announced `ready=true` at `beginReview`/`configure`), then `restoreModel("worker")` throws → back to planning. After the user fixes the model, the next Ready takes the existing-pane path (`src/index.ts:184-186`): `configure` resets the worker's peer, hellos `ready=true`, the supervisor sees no change, stays silent, and `waitReady(5000)` times out — even though the supervisor is up and healthy. Every Ready/reconnect retry repeats this. This undercuts the F1/F4 recovery story on exactly the path those fixes target. +3. **Silent steer loss during the window.** While half-open, the supervisor's `connected` is true, so `SteerWorker` "succeeds", but the worker drops the message at `src/intercom.ts:166` (`event.fromSessionId !== this.peer`) with no notification; the steer sits in pending awaiting a changed hello that may never come. The SteerWorker result text ("Receipt and execution are not confirmed") softens but does not surface this. + +Why the tests miss it: the fixture auto-replies to **every** hello with `ready: true` (`test/intercom-fixture.ts:21`), which is precisely the behavior the real peer logic does not have. The new peer-return and reconnect tests therefore cannot observe the change-gated reply. + +Suggested direction (not implemented): reply to a hello whenever the sender may have lost state — e.g. have `configure` retain `peer`/`peerReady` when the binding is unchanged (re-handshake is only needed after a real transport/binding change), or include a monotonic hello generation and reply to any newer/unknown generation. An unconditional reply to every hello also works without ping-pong, because the *response* hello is still change-gated on the receiver's side. + +### P2 — Failed Ready leaves the worker announcing `ready=true` while paused (Low) [CODE] + +In the Ready flow, `beginReview` → `configure(approvalId, "worker", ctx)` defaults to `ready=true` (`src/intercom.ts:43`, `src/index.ts:171`). If `restoreModel("worker")` then throws (`src/index.ts:525` → catch at :537-544), the code rolls back `phase` to planning but never rolls back intercom readiness or detaches. The worker thereafter hellos `ready=true` on any broker event while `modelError` is set; the supervisor sees a ready, connected worker, `SteerWorker` sends successfully, and the worker's `onSteer` throws (`src/index.ts:113-116`) — unacked, error notification per replay. Self-consistent recovery exists (next Ready after `/model` works, modulo P1), so impact is confusing error noise and a misleading readiness signal, not loss. A `markNotReady`/detach in that catch would align the announced state with the pause. + +### P3 — Worker pause message misattributes a *supervisor-side* pause (Low) [CODE] + +`pauseReason()` (`src/index.ts:149-154`) collapses "peer absent" and "peer present but not ready" into one message: "Supervisor disconnected. Run /goals reconnect, or /goals restart…". When the supervisor pane is alive but paused on its own model restoration (`src/supervisor-session.ts:150-158` sets supervisor `modelError`, readiness stays false), the worker-side `/goals reconnect` will hello, get a `ready=false` reply, and time out after 5s — the advised action cannot work; the actual fix is `/model` + `/goals reconnect` in the supervisor pane. The supervisor pane does display its own error notification, and the design keeps both panes visible, so the user has the information — but the worker-side guidance points the wrong way. `peer` set + `peerReady` false is distinguishable from no peer; the message could be too. + +### P4 — `goalBlock` and `tickGoal` now scan different regions (Low) [CODE] + +Post-F7, `goalBlock` only sees the pre-`## Log` region (`src/approval.ts:44`), while `tickGoal` and `scanGoals` still scan the whole file (`src/index.ts:670-677`, :53-60). A goal-shaped line quoted inside the Log (e.g. a pasted checklist) yields: `goalBlock` finds a unique match → approval proceeds and hashes, but `CompleteGoal` → `tickGoal` finds two hits → returns null → sign-off blocked after a successful approval. Fail-closed, requires unusual plan content, and the same class of confusion pre-dates the fix (both sides failed before); noting it because the fix changed the boundary of only one of the two scanners. + +### P5 — Ready catch can resurrect a cleared plan phase after a concurrent `/goals clear` (Low) [CODE/INFERENCE] + +The Ready path awaits up to 300s inside `startSupervisor` (`src/index.ts:213`), and neither `detach()` nor `configure()` wakes `waitReady` waiters (only hellos/shutdown do; `detach`'s clearing of `binding` means its own not-ready hello is ignored by waiters, and the peer's reply is dropped by the now-empty binding check). If the user runs `/goals clear` during that wait, the plan state is cleared; when the wait later times out, the catch at `src/index.ts:537-544` unconditionally sets `phase: "planning"` and persists — yielding `phase: "planning"` with `planVersion: null`, a "drafting goals" widget over no plan, and "No active plan to disconnect" from `/goals clear`. Recoverable via a fresh `/goals `, and the interleaving requires issuing a command while the Ready select-loop is mid-wait, hence Low. I did not execute this interleaving; it follows from the unconditional catch and the waiter semantics. Guarding the catch on "state still belongs to this Ready attempt" (e.g. approvalId/planVersion unchanged) would close it. + +--- + +## Assessment of documented residuals (not accepted on documentation alone) + +- **F3 durable-delivery gap**: genuine and correctly scoped as open. Concretely, after an acked-but-async-failed enqueue, the supervisor waits indefinitely for a response to an instruction the worker model never saw, with no signal on either side; the recorded future UAT (inject an async enqueue rejection, assert no confirmed-delivery claim and recoverability) is the right bar. Fine to defer; not fine to call resolved — and it isn't. +- **F8 (unknown usage disables 100k compaction)**: the `?? 0` fallback (`src/supervisor-session.ts:187`) silently disables the custom compaction wherever `getContextUsage()` is unavailable, contradicting the AGENTS.md cost design with no user-visible signal. Mitigating factor I verified: the supervisor system prompt (with `planPath`) is re-appended every `before_agent_start` (:175), and approvals/checkpoints live on disk, so a fallback default-compaction does not lose the plan pointer or approval state — the consequence is cost/context-rot drift, not correctness. Still, a one-time "usage unknown; custom compaction inactive" notification would close the observability gap cheaply. Acceptable as a documented limitation; the silence is the weakest part. +- **F9 (planning/pause bash gate holes)**: accurately disclaimed as guardrail-not-sandbox. The paused-diagnostic gate (`src/index.ts:453-456`) intentionally inherits the same heuristic, including the `git diff --ext-diff` external-command hole that requires a pre-existing hostile `.git/config`. Given the threat model (trusted repo, trusted extensions), the README statement is sufficient; a hardened policy remains correctly out of scope. +- **Test-quality caveat**: beyond the fixture issue in P1, the native pair test drives only `SteerWorker`; the two-real-session `ApproveGoal → CompleteGoal` chain is still unexecuted end to end, so the F7 hash boundary and the view-freshness gate are verified only per-side (unit/flow tests) plus one real supervisor tool-list inspection. The disposition states this; I confirm it remains true at 1668c94. + +## What is solid + +- Shutdown/late-startup guards (`src/index.ts:180, 199-203, 210`) and the supervisor's `bootstrapping` `finally` fix are correct; the test proves no late persistence after shutdown. +- Queued pending steers when *own* readiness changes are handled correctly on both roles: republish is gated on `peerReady && this.ready` (`src/intercom.ts:158-162`), pause suppresses republication, and recovery replays exactly the unacked set from session entries. The only hole in this chain is P1's missing trigger. +- Approval binding safety (per-approvalId, head/tree/clean-worktree/goal-block-hash equality, restart invalidating old approvals) is preserved and extended by the restart flow. +- Human recovery availability while paused is real on both roles: input, read-only tools, `/model`, `/goals reconnect|restart|clear` all remain reachable; sign-off and writes fail closed. + +## Summary + +F1–F7 are fixed as claimed, with tests and native evidence matching the dispositions. The significant new finding is **P1**: the change-gated hello reply combined with `configure()`'s peer reset makes `/goals reconnect` and Ready-retry wedge half-open precisely when readiness did not change — including the F1-on-Ready retry scenario the fixes were built for — and steers are silently dropped in that window while `steer()` reports success. P2–P5 are low-severity consistency/guard gaps. Documented residuals F3/F8/F9 are honest; F3's delivery gap and F8's silent compaction-disable remain open items, not fixes. + +Attribution: independent reviewer, run bdb93a2e-52f4-4a6d-bac2-7c9eb48118b5; preserved verbatim by Pi/OpenAI implementation worker. diff --git a/slop/reviews/handshake-native/supervisor-events.jsonl b/slop/reviews/handshake-native/supervisor-events.jsonl new file mode 100644 index 0000000..2f97afd --- /dev/null +++ b/slop/reviews/handshake-native/supervisor-events.jsonl @@ -0,0 +1,21 @@ +{"type":"extension_ui_request","id":"1b7b765d-82f4-4ac3-bd12-011f1f3227af","method":"notify","message":"supervisor model: offline/test","notifyType":"info"} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-visible-supervisor-v2","data":{"version":2,"workerSessionId":"01a080c2-dc16-705a-b544-e383580d9ac7","planPath":"/tmp/goals-native-pair-vfPu3e/plan.md"},"id":"e0a271f9","parentId":"d768c1fd","timestamp":"2026-09-08T11:24:10.287Z"}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"in","message":{"binding":"native-pair-test","role":"worker","kind":"view","id":"cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc","text":"The worker stopped.\n\nThe saved plan needs a check of the actual outputs.\n\nworker view id: cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc","reason":"settled","backgroundQuiet":true}},"id":"848cb24d","parentId":"e0a271f9","timestamp":"2026-09-08T11:24:10.295Z"}} +{"type":"agent_start"} +{"type":"turn_start"} +{"type":"message_start","message":{"role":"user","content":[{"type":"text","text":"The worker stopped.\n\nThe saved plan needs a check of the actual outputs.\n\nworker view id: cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc"}],"timestamp":1788866650296}} +{"type":"message_end","message":{"role":"user","content":[{"type":"text","text":"The worker stopped.\n\nThe saved plan needs a check of the actual outputs.\n\nworker view id: cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc"}],"timestamp":1788866650296}} +{"type":"message_start","message":{"role":"assistant","content":[{"type":"toolCall","id":"test-steer","name":"SteerWorker","arguments":{"instruction":"Read the real outputs before declaring completion."},"partialArgs":"{\"instruction\":\"Read the real outputs before declaring completion.\"}","streamIndex":0}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"totalTokens":0,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"pending","timestamp":1788866650324}} +{"type":"message_update","assistantMessageEvent":{"type":"toolcall_start","contentIndex":0}} +{"type":"message_update","assistantMessageEvent":{"type":"toolcall_delta","contentIndex":0,"delta":"{\"instruction\":\"Read the real outputs before declaring completion.\"}"}} +{"type":"message_update","assistantMessageEvent":{"type":"toolcall_end","contentIndex":0,"toolCall":{"type":"toolCall","id":"test-steer","name":"SteerWorker","arguments":{"instruction":"Read the real outputs before declaring completion."}}}} +{"type":"message_end","message":{"role":"assistant","content":[{"type":"toolCall","id":"test-steer","name":"SteerWorker","arguments":{"instruction":"Read the real outputs before declaring completion."}}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":10,"output":10,"cacheRead":0,"cacheWrite":0,"reasoning":0,"totalTokens":20,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"toolUse","timestamp":1788866650324,"rawStopReason":"tool_calls"}} +{"type":"tool_execution_start","toolCallId":"test-steer","toolName":"SteerWorker","args":{"instruction":"Read the real outputs before declaring completion."}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"out","message":{"binding":"native-pair-test","role":"supervisor","kind":"steer","id":"22068654-6e7c-4328-890f-9382b25c8ea7","text":"Read the real outputs before declaring completion."}},"id":"0c0166ea","parentId":"ed2e1ca2","timestamp":"2026-09-08T11:24:10.345Z"}} +{"type":"tool_execution_end","toolCallId":"test-steer","toolName":"SteerWorker","result":{"content":[{"type":"text","text":"Worker instruction 22068654-6e7c-4328-890f-9382b25c8ea7 sent through pi-intercom. Receipt and execution are not confirmed by this result."}],"details":{},"isError":false},"isError":false} +{"type":"message_start","message":{"role":"toolResult","toolCallId":"test-steer","toolName":"SteerWorker","content":[{"type":"text","text":"Worker instruction 22068654-6e7c-4328-890f-9382b25c8ea7 sent through pi-intercom. Receipt and execution are not confirmed by this result."}],"details":{},"isError":false,"timestamp":1788866650346}} +{"type":"message_end","message":{"role":"toolResult","toolCallId":"test-steer","toolName":"SteerWorker","content":[{"type":"text","text":"Worker instruction 22068654-6e7c-4328-890f-9382b25c8ea7 sent through pi-intercom. Receipt and execution are not confirmed by this result."}],"details":{},"isError":false,"timestamp":1788866650346}} +{"type":"turn_end","message":{"role":"assistant","content":[{"type":"toolCall","id":"test-steer","name":"SteerWorker","arguments":{"instruction":"Read the real outputs before declaring completion."}}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":10,"output":10,"cacheRead":0,"cacheWrite":0,"reasoning":0,"totalTokens":20,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"toolUse","timestamp":1788866650324,"rawStopReason":"tool_calls"},"toolResults":[{"role":"toolResult","toolCallId":"test-steer","toolName":"SteerWorker","content":[{"type":"text","text":"Worker instruction 22068654-6e7c-4328-890f-9382b25c8ea7 sent through pi-intercom. Receipt and execution are not confirmed by this result."}],"details":{},"isError":false,"timestamp":1788866650346}]} +{"type":"turn_start"} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"ack","message":{"binding":"native-pair-test","role":"worker","kind":"received","id":"22068654-6e7c-4328-890f-9382b25c8ea7"}},"id":"6b63b93d","parentId":"8ce5aa46","timestamp":"2026-09-08T11:24:10.348Z"}} +{"id":"supervisor-state","type":"response","command":"get_state","success":true,"data":{"model":{"id":"test","name":"Offline test model","reasoning":false,"input":["text"],"contextWindow":16000,"maxTokens":1000,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0},"api":"openai-completions","provider":"offline","baseUrl":"http://127.0.0.1:37773"},"thinkingLevel":"off","isStreaming":true,"isCompacting":false,"steeringMode":"one-at-a-time","followUpMode":"one-at-a-time","sessionFile":"/tmp/goals-native-pair-vfPu3e/agent/sessions/--tmp-goals-native-pair-vfPu3e--/2026-09-08T11-24-10-111Z_01a080c2-dfff-7290-9118-66b87ab29080.jsonl","sessionId":"01a080c2-dfff-7290-9118-66b87ab29080","autoCompactionEnabled":true,"messageCount":5,"pendingMessageCount":0}} \ No newline at end of file diff --git a/slop/reviews/handshake-native/supervisor-stderr.txt b/slop/reviews/handshake-native/supervisor-stderr.txt new file mode 100644 index 0000000..e69de29 diff --git a/slop/reviews/handshake-native/worker-events.jsonl b/slop/reviews/handshake-native/worker-events.jsonl new file mode 100644 index 0000000..7ce503f --- /dev/null +++ b/slop/reviews/handshake-native/worker-events.jsonl @@ -0,0 +1,22 @@ +{"id":"planning","type":"response","command":"prompt","success":true} +{"type":"agent_start"} +{"type":"turn_start"} +{"type":"message_start","message":{"role":"user","content":[{"type":"text","text":"Retain this planning context for the supervisor fork."}],"timestamp":1788866649278}} +{"type":"message_end","message":{"role":"user","content":[{"type":"text","text":"Retain this planning context for the supervisor fork."}],"timestamp":1788866649278}} +{"type":"message_start","message":{"role":"assistant","content":[{"type":"text","text":"Test context retained. Actual outputs still need inspection."}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"totalTokens":0,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"pending","timestamp":1788866649355}} +{"type":"message_update","assistantMessageEvent":{"type":"text_start","contentIndex":0}} +{"type":"message_update","assistantMessageEvent":{"type":"text_delta","contentIndex":0,"delta":"Test context retained. Actual outputs still need inspection."}} +{"type":"message_update","assistantMessageEvent":{"type":"text_end","contentIndex":0,"content":"Test context retained. Actual outputs still need inspection."}} +{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"Test context retained. Actual outputs still need inspection."}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":10,"output":10,"cacheRead":0,"cacheWrite":0,"reasoning":0,"totalTokens":20,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"stop","timestamp":1788866649355,"rawStopReason":"stop"}} +{"type":"turn_end","message":{"role":"assistant","content":[{"type":"text","text":"Test context retained. Actual outputs still need inspection."}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":10,"output":10,"cacheRead":0,"cacheWrite":0,"reasoning":0,"totalTokens":20,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"stop","timestamp":1788866649355,"rawStopReason":"stop"},"toolResults":[]} +{"type":"agent_end","messages":[{"role":"user","content":[{"type":"text","text":"Retain this planning context for the supervisor fork."}],"timestamp":1788866649278},{"role":"assistant","content":[{"type":"text","text":"Test context retained. Actual outputs still need inspection."}],"api":"openai-completions","provider":"offline","model":"test","usage":{"input":10,"output":10,"cacheRead":0,"cacheWrite":0,"reasoning":0,"totalTokens":20,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0,"total":0}},"stopReason":"stop","timestamp":1788866649355,"rawStopReason":"stop"}],"willRetry":false} +{"type":"agent_settled"} +{"id":"worker-state","type":"response","command":"get_state","success":true,"data":{"model":{"id":"test","name":"Offline test model","reasoning":false,"input":["text"],"contextWindow":16000,"maxTokens":1000,"cost":{"input":0,"output":0,"cacheRead":0,"cacheWrite":0},"api":"openai-completions","provider":"offline","baseUrl":"http://127.0.0.1:37773"},"thinkingLevel":"off","isStreaming":false,"isCompacting":false,"steeringMode":"one-at-a-time","followUpMode":"one-at-a-time","sessionFile":"/tmp/goals-native-pair-vfPu3e/agent/sessions/--tmp-goals-native-pair-vfPu3e--/2026-09-08T11-24-09-110Z_01a080c2-dc16-705a-b544-e383580d9ac7.jsonl","sessionId":"01a080c2-dc16-705a-b544-e383580d9ac7","autoCompactionEnabled":true,"messageCount":2,"pendingMessageCount":0}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"out","message":{"binding":"native-pair-test","role":"worker","kind":"view","id":"cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc","text":"The worker stopped.\n\nThe saved plan needs a check of the actual outputs.\n\nworker view id: cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc","reason":"settled","backgroundQuiet":true}},"id":"57ac2295","parentId":"21d6e68a","timestamp":"2026-09-08T11:24:10.294Z"}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"ack","message":{"binding":"native-pair-test","role":"supervisor","kind":"received","id":"cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc"}},"id":"7ffd87dc","parentId":"57ac2295","timestamp":"2026-09-08T11:24:10.295Z"}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"ack","message":{"binding":"native-pair-test","role":"supervisor","kind":"received","id":"cf656aef-1404-4d5c-9a11-4fbd4ed5b5dc"}},"id":"b2ffeed1","parentId":"7ffd87dc","timestamp":"2026-09-08T11:24:10.335Z"}} +{"type":"entry_appended","entry":{"type":"custom","customType":"pi-goals-intercom","data":{"direction":"in","message":{"binding":"native-pair-test","role":"supervisor","kind":"steer","id":"22068654-6e7c-4328-890f-9382b25c8ea7","text":"Read the real outputs before declaring completion."}},"id":"34521c62","parentId":"b2ffeed1","timestamp":"2026-09-08T11:24:10.346Z"}} +{"type":"agent_start"} +{"type":"turn_start"} +{"type":"message_start","message":{"role":"user","content":[{"type":"text","text":"[supervisor] Read the real outputs before declaring completion."}],"timestamp":1788866650346}} +{"type":"message_end","message":{"role":"user","content":[{"type":"text","text":"[supervisor] Read the real outputs before declaring completion."}],"timestamp":1788866650346}} \ No newline at end of file diff --git a/slop/reviews/handshake-native/worker-stderr.txt b/slop/reviews/handshake-native/worker-stderr.txt new file mode 100644 index 0000000..e69de29