Skip to content

Commit 15cb6bb

Browse files
vitryvitry
authored
Merge pull request #52 from vitry/fix/zcode-0165-permission-turn
fix: preserve active turn for late ZCode permissions
2 parents 4b4ea97 + 2e6abb0 commit 15cb6bb

11 files changed

Lines changed: 619 additions & 27 deletions

File tree

‎docs/superpowers/plans/2026-09-01-zcode-0165-permission-turn-lifecycle.md‎

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
# ZCode 0.16.5 Permission Turn Lifecycle Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Preserve the active protocol turn across an admission-time legacy completion wake so later ZCode permission requests succeed, then release the local turn at the executor's authoritative cleanup boundary.
6+
7+
**Architecture:** Add a non-destructive completion observer alongside the existing destructive waiter in the protocol/client layer. Migrate only `executeJob` to that observer and explicitly release its local turn during unconditional teardown; retain compatibility fallbacks for injected test clients that expose only the historical interface.
8+
9+
**Tech Stack:** Node.js 22.13, ECMAScript modules, `node:test`, the existing JSON-RPC protocol client and job executor.
10+
11+
---
12+
13+
## File map
14+
15+
- Modify `scripts/lib/zcode-protocol.mjs`: share completion validation/wait registration while distinguishing destructive consumption from observation; add local turn release behavior.
16+
- Modify `scripts/lib/zcode-client.mjs`: expose `observeCompletion()` and `releaseTurn()` without changing `waitForCompletion()`.
17+
- Modify `scripts/lib/review.mjs`: use non-destructive observation for `legacyWake` and release the local turn during teardown.
18+
- Modify `tests/process-zcode.test.mjs`: cover low-level observer, permission, timeout, release, and destructive-wait invariants.
19+
- Modify `tests/job-control.test.mjs`: cover captured 0.16.5 executor ordering and success/error cleanup.
20+
21+
### Task 1: Add non-destructive completion observation
22+
23+
**Files:**
24+
- Modify: `scripts/lib/zcode-protocol.mjs:95-145`
25+
- Modify: `scripts/lib/zcode-client.mjs:125-135`
26+
- Test: `tests/process-zcode.test.mjs`
27+
28+
- [ ] **Step 1: Write failing protocol tests**
29+
30+
Add tests that construct `ZCodeProtocolClient` with `PassThrough` streams, arm a turn, start `observeCompletion(sessionId)`, emit a matching `prompt_completed`, and assert:
31+
32+
```js
33+
const completion = protocol.observeCompletion(sessionId);
34+
protocol.handleLine(JSON.stringify({ method: 'state.updated', params: matchingCompletion }));
35+
assert.equal((await completion).reason, 'prompt_completed');
36+
assert.equal(protocol.turnState(sessionId), 'armed');
37+
```
38+
39+
Then emit `interaction/requestPermission` for the same session, return an offered allow response from the handler, and verify the protocol writes the allow result instead of `ZCODE_PERMISSION_SESSION_INVALID`. Add companion assertions that `waitForCompletion()` still clears the turn, observer timeout leaves it armed, and `releaseTurn()` clears it and rejects any still-pending observer.
40+
41+
- [ ] **Step 2: Run the focused tests and verify RED**
42+
43+
Run:
44+
45+
```bash
46+
node --test --test-name-pattern='non-destructive completion|completion observer|destructive completion' tests/process-zcode.test.mjs
47+
```
48+
49+
Expected: FAIL because `observeCompletion` and `releaseTurn` do not exist.
50+
51+
- [ ] **Step 3: Implement the minimal protocol behavior**
52+
53+
Refactor the current waiter registration into one internal path with an explicit consumption mode. Preserve the public destructive path exactly, and add:
54+
55+
```js
56+
observeCompletion(sessionId, timeoutMs) {
57+
return this.waitForCompletionMode(sessionId, timeoutMs, false);
58+
}
59+
60+
releaseTurn(sessionId) {
61+
if (!nonEmpty(sessionId)) throw protocolInputError();
62+
this.cancelTurn(sessionId, localTurnReleasedError(sessionId));
63+
}
64+
```
65+
66+
For observation mode, peek at an already queued completion rather than shifting it, do not call `abortTurn()` on resolution or timeout, and always unregister the observer. For destructive mode, keep the current shift, timeout cleanup, and `abortTurn()` behavior. Continue enforcing one waiter/observer per session with `waiterSessions`.
67+
68+
Expose the two operations from `ZCodeClient` with normal session-ID validation/documentation:
69+
70+
```js
71+
observeCompletion(sessionId, timeoutMs) {
72+
requireSessionId(sessionId);
73+
return this.protocol.observeCompletion(sessionId, timeoutMs);
74+
}
75+
76+
releaseTurn(sessionId) {
77+
requireSessionId(sessionId);
78+
this.protocol.releaseTurn(sessionId);
79+
}
80+
```
81+
82+
- [ ] **Step 4: Run focused and adjacent tests and verify GREEN**
83+
84+
Run:
85+
86+
```bash
87+
node --test tests/process-zcode.test.mjs tests/zcode-client.test.mjs
88+
```
89+
90+
Expected: PASS, including all existing destructive completion and permission tests.
91+
92+
- [ ] **Step 5: Self-review and commit**
93+
94+
Check that no existing call site changed and that observer cleanup cannot retain a timer, subscriber, or waiter-session entry. Then commit:
95+
96+
```bash
97+
git add scripts/lib/zcode-protocol.mjs scripts/lib/zcode-client.mjs tests/process-zcode.test.mjs
98+
git commit -m "fix: observe legacy completion without ending turn"
99+
```
100+
101+
### Task 2: Migrate executor wake and own local cleanup
102+
103+
**Files:**
104+
- Modify: `scripts/lib/review.mjs:220-360`
105+
- Test: `tests/job-control.test.mjs`
106+
107+
- [ ] **Step 1: Write the captured-ordering regression test**
108+
109+
Extend the existing `0.16.5 foreground execution treats legacy completion as admission` fixture client with `observeCompletion`, `releaseTurn`, and a permission handler capture. Make `observeCompletion` publish the legacy wake first, then invoke the captured permission handler with a medium-risk Write request offering allow/deny. Assert the handler returns allow while the executor remains running, then publish the v4 authoritative terminal and assert success plus one local release:
110+
111+
```js
112+
assert.deepEqual(permissionDecision, { decision: 'allow' });
113+
assert.equal(releaseTurnCalls, 1);
114+
assert.equal(releasedSessionId, sessionId);
115+
```
116+
117+
Add an error-path test where observation or authoritative reconciliation fails after admission; assert `releaseTurn(sessionId)` still runs once before `close()`.
118+
119+
- [ ] **Step 2: Run the executor regressions and verify RED**
120+
121+
Run:
122+
123+
```bash
124+
node --test --test-name-pattern='0.16.5 foreground execution|releases local turn' tests/job-control.test.mjs
125+
```
126+
127+
Expected: FAIL because `executeJob` still calls destructive `waitForCompletion()` and never releases the local turn explicitly.
128+
129+
- [ ] **Step 3: Implement the executor migration**
130+
131+
Construct `legacyWake` from `client.observeCompletion(activeSessionId)` when available. Keep a fallback to `client.waitForCompletion(activeSessionId)` only for existing injected test doubles that predate the internal interface:
132+
133+
```js
134+
const observeLegacyCompletion = typeof client.observeCompletion === 'function'
135+
? client.observeCompletion.bind(client)
136+
: client.waitForCompletion.bind(client);
137+
const legacyWake = waitForCompletionOrAbort(observeLegacyCompletion(activeSessionId), input.signal);
138+
```
139+
140+
In unconditional teardown, after all terminal/cancellation/error reconciliation and progress cleanup but before `client.close()`, release the known local session exactly once when supported:
141+
142+
```js
143+
try {
144+
if (sessionId && typeof client.releaseTurn === 'function') client.releaseTurn(sessionId);
145+
} catch (cleanupError) {
146+
if (!primaryError) primaryError = cleanupError;
147+
}
148+
```
149+
150+
Preserve the primary-error and cleanup-error conventions already used by the executor. Do not send an upstream stop from this release path and do not modify `decidePermission`.
151+
152+
- [ ] **Step 4: Run focused and executor-adjacent tests and verify GREEN**
153+
154+
Run:
155+
156+
```bash
157+
node --test tests/job-control.test.mjs tests/integration/companion.test.mjs
158+
```
159+
160+
Expected: PASS with the captured ordering, cleanup tests, and existing cancellation behavior unchanged.
161+
162+
- [ ] **Step 5: Self-review and commit**
163+
164+
Inspect the diff for exactly one production caller migration, one unconditional local cleanup, and no permission-policy changes. Then commit:
165+
166+
```bash
167+
git add scripts/lib/review.mjs tests/job-control.test.mjs
168+
git commit -m "fix: retain active turn through legacy wake"
169+
```
170+
171+
### Task 3: Verify contracts and release readiness
172+
173+
**Files:**
174+
- Modify if required by generated parity checks: checked-in marketplace mirrors only through the repository's existing builder
175+
- Test: repository-wide verification
176+
177+
- [ ] **Step 1: Run permission-policy and protocol contract tests**
178+
179+
Run:
180+
181+
```bash
182+
node --test --test-name-pattern='permission|completion' tests/process-zcode.test.mjs tests/zcode-client.test.mjs tests/job-control.test.mjs
183+
```
184+
185+
Expected: PASS; Rescue permission decisions remain unchanged and destructive completion callers retain their contract.
186+
187+
- [ ] **Step 2: Run the full repository check**
188+
189+
Run:
190+
191+
```bash
192+
npm run check
193+
```
194+
195+
Expected: PASS for line endings, lint, typecheck, all tests, qualification tests, and marketplace parity/build checks.
196+
197+
- [ ] **Step 3: Inspect final scope**
198+
199+
Run:
200+
201+
```bash
202+
git diff --check origin/main...HEAD
203+
git diff --stat origin/main...HEAD
204+
git diff origin/main...HEAD -- scripts/lib/review.mjs scripts/lib/zcode-client.mjs scripts/lib/zcode-protocol.mjs
205+
```
206+
207+
Expected: no whitespace errors; changes remain limited to the completion lifecycle, tests, and approved docs.
208+
209+
- [ ] **Step 4: Commit any verification-only generated parity update**
210+
211+
If and only if the repository's official check regenerates tracked marketplace parity files, review and commit those exact generated changes:
212+
213+
```bash
214+
git add marketplace
215+
git commit -m "build: refresh marketplace snapshot"
216+
```
217+
218+
If there are no generated tracked changes, skip this commit.

‎docs/superpowers/specs/2026-09-01-zcode-0165-permission-turn-lifecycle-design.md‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# ZCode 0.16.5 Permission Turn Lifecycle Design
2+
3+
## Context
4+
5+
The executor already treats ZCode 0.16.5 `state.updated` notifications with reason `prompt_completed` as legacy liveness signals. True completion is established by the v4 conversation observer or a coherent current-turn snapshot. However, `executeJob` obtains the liveness signal through `client.waitForCompletion()`, whose public contract consumes completion and deletes the protocol client's active-turn state.
6+
7+
ZCode 0.16.5 can emit that legacy notification immediately after admission, then request tool permission while the real runtime turn is still active. Once the legacy waiter deletes the turn, the permission request fails the exact-active-turn check and is returned as JSON-RPC `-32000` (`ZCODE_PERMISSION_SESSION_INVALID`). This is independent of the configured Codex permission mode: the incident job already carried `bypassPermissions`.
8+
9+
## Goals
10+
11+
- Keep the exact turn armed after an admission-time legacy completion wake so later permission requests can be evaluated normally.
12+
- Preserve the current destructive behavior of `waitForCompletion()` for every existing caller.
13+
- Clear local turn state when `executeJob` reaches its real terminal or cleanup boundary.
14+
- Cover the captured 0.16.5 event ordering with deterministic regression tests.
15+
16+
## Non-goals
17+
18+
- No change to `decidePermission`, permission snapshots, risk mapping, or offered-response validation.
19+
- No new appserver flag, permission field, broker authorization rule, or persisted schema.
20+
- No reinterpretation of legacy completion as authoritative success or failure.
21+
- No broad refactor of the protocol or executor lifecycle.
22+
23+
## Design
24+
25+
### Non-destructive legacy observation
26+
27+
Add a narrowly named protocol/client operation for observing the next validated completion notification without consuming the active turn. It must apply the same session, timeout, active-turn, duplicate-waiter, and `isCompletionFor` validation used by `waitForCompletion()`, but resolution must not call `abortTurn()` and must not consume turn ownership.
28+
29+
An observer timeout removes only that observer; it does not silently acquire authority to end the turn. Executor teardown remains responsible for local release. Add an idempotent client-level local release operation that cancels outstanding local completion observation and clears the protocol turn without sending an upstream stop request.
30+
31+
The existing `waitForCompletion()` remains unchanged in observable behavior: queued or live completion resolution consumes the turn, while timeout and cancellation retain their present destructive cleanup semantics.
32+
33+
Only `executeJob` switches its `legacyWake` construction to the non-destructive observer. The coordinator continues to use the wake solely to trigger authoritative v4/snapshot reconciliation.
34+
35+
### Explicit executor cleanup
36+
37+
Because the wake no longer consumes local state, `executeJob` must explicitly release the protocol turn after the authoritative lifecycle has finished. Cleanup belongs in the executor's existing unconditional teardown, after any terminal/cancellation reconciliation that may still need permission and turn identity, and before client close completes.
38+
39+
The cleanup operation is local and idempotent. It must not send `session/stop`, alter durable job state, or replace the existing cancellation paths. Successful terminal, provider failure, remote interruption, local abort, timeout, and error cleanup all converge on the same local turn release when a session was created or resumed.
40+
41+
### Safety invariants
42+
43+
- A validated early legacy completion leaves `turnState(sessionId) === 'armed'`.
44+
- A later permission request for that session reaches the configured handler and returns one offered response.
45+
- Authoritative executor teardown leaves `turnState(sessionId) === null`.
46+
- Ordinary `waitForCompletion()` still leaves `turnState(sessionId) === null` immediately after resolution.
47+
- Permission policy and durable job outcomes are unchanged.
48+
49+
## Testing
50+
51+
1. Add a protocol/client regression proving non-destructive observation preserves the armed turn and permits a subsequent permission request.
52+
2. Retain or strengthen the destructive waiter assertion so its compatibility contract is explicit.
53+
3. Add an executor regression with captured 0.16.5 ordering: admission, early legacy wake, later permission request, authoritative terminal, successful result, and final local turn cleanup.
54+
4. Exercise cleanup on a non-success path so an observer cannot leave an armed turn behind.
55+
5. Run focused tests, then `npm run check` before review and PR creation.
56+
57+
## Rollout and compatibility
58+
59+
This is an internal additive API and a one-call-site migration. No user configuration or data migration is required. Older ZCode versions continue to produce the same wake signal, and all callers outside `executeJob` retain existing semantics.

‎marketplace/.agents/plugins/provenance.json‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{
22
"packageVersion": "0.1.0",
33
"pluginVersion": "0.1.0",
4-
"sourceRef": "e7c986bcc8e743f6ba2ae35004cb51ad00a995ac",
5-
"sourceSha": "e7c986bcc8e743f6ba2ae35004cb51ad00a995ac",
4+
"sourceRef": "ee1fa5c9778cf6dd02e094cf2bfe7f2835c02dab",
5+
"sourceSha": "ee1fa5c9778cf6dd02e094cf2bfe7f2835c02dab",
66
"dependencyLock": {
77
"file": "npm-shrinkwrap.json",
88
"sha256": "fa927194e6ca0b25c1d3f428859b2ab4798b8eabb4d31bc87188d73c630f9938"
99
},
1010
"content": {
1111
"algorithm": "sha256",
12-
"sha256": "02b5640cd5cf5ad4c1dbae7dd9e5028710d8ac3f12077ea08ce2f670c4dd2543",
12+
"sha256": "40d1a65be6976f5f070b2784a35635f5aba4597c4846ec798b86d3f30dd77af7",
1313
"files": [
1414
{
1515
"path": ".agents/plugins/marketplace.json",
@@ -658,8 +658,8 @@
658658
},
659659
{
660660
"path": "plugins/zcode/scripts/lib/review.mjs",
661-
"size": 43083,
662-
"sha256": "5495fe99558e3d9595f9c016bdef9ed9aca4940b7f89438e544c5fd280c0f25f"
661+
"size": 43572,
662+
"sha256": "0d945d223ce618ee28e2e81831ce855df1b18cc975f45dcc5538b8cc6e934d11"
663663
},
664664
{
665665
"path": "plugins/zcode/scripts/lib/session-progress.mjs",
@@ -703,8 +703,8 @@
703703
},
704704
{
705705
"path": "plugins/zcode/scripts/lib/zcode-client.mjs",
706-
"size": 52052,
707-
"sha256": "2f38c2c6f0b1abbe094fbc7e67f359e6256ebfc0f05a602138f3c3a1e3559d77"
706+
"size": 52495,
707+
"sha256": "0d4c854d4045260cc86d14d30e647db22efb08bd4f9ad30a2f795c3b5e37f901"
708708
},
709709
{
710710
"path": "plugins/zcode/scripts/lib/zcode-discovery.mjs",
@@ -713,8 +713,8 @@
713713
},
714714
{
715715
"path": "plugins/zcode/scripts/lib/zcode-protocol.mjs",
716-
"size": 38990,
717-
"sha256": "a6531d8d112b7c605a26d8eee8c0af5f3e01f8e14ef34bfe3226df60a31f9db7"
716+
"size": 42018,
717+
"sha256": "68e98ef8be8b670648e3add7966fb1619b3836be0ce6b2a29e678899f68f9426"
718718
},
719719
{
720720
"path": "plugins/zcode/scripts/lib/zcode-runtime-config.mjs",

‎marketplace/plugins/zcode/scripts/lib/review.mjs‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,10 @@ export async function executeJob(input) {
225225
reporter.activateAcceptedBoundary({ readSnapshot: () => client.readSession(activeSessionId), describer: sessionDescriber });
226226
} catch { reporter.activateAcceptedBoundary({}); }
227227
reporter.activate({ method: 'state.updated', params: { scope: 'session', sessionId: activeSessionId, reason: 'prompt_started' } });
228-
const legacyWake = waitForCompletionOrAbort(client.waitForCompletion(activeSessionId), input.signal);
228+
const observeLegacyCompletion = typeof client.observeCompletion === 'function'
229+
? client.observeCompletion.bind(client)
230+
: client.waitForCompletion.bind(client);
231+
const legacyWake = waitForCompletionOrAbort(observeLegacyCompletion(activeSessionId), input.signal);
229232
const terminal = await awaitCurrentTurnTerminal({
230233
legacyWake, conversationObserver, readSnapshot: () => client.readSession(activeSessionId), turnBoundary, signal: input.signal,
231234
});
@@ -252,7 +255,7 @@ export async function executeJob(input) {
252255
}
253256
} catch (error) {
254257
primaryError = error instanceof SuccessfulResultFinalizationError ? error.cause : error;
255-
let current = initialBoundStopGuardComplete ? await input.store.readJob(workspace, job.id).catch(() => running) : null;
258+
try { let current = initialBoundStopGuardComplete ? await input.store.readJob(workspace, job.id).catch(() => running) : null;
256259
let resumeFailureSettlementRejected = false;
257260
if (input.resumeSessionId && current?.status === 'queued' && input.onResumeFailure) {
258261
try {
@@ -333,9 +336,17 @@ export async function executeJob(input) {
333336
}
334337
if (canFail) try { await input.store.finishJob(workspace, job.id, [current.status], 'failed', { error: safeError(error), exitCode: 1 }); } catch (finalizeError) { primaryError = finalizeError; }
335338
}
339+
} catch (reconciliationError) {
340+
primaryError = reconciliationError;
341+
}
336342
}
337343
// Cleanup order is part of the progress lifecycle contract.
338344
await cleanupProgress();
345+
try {
346+
if (sessionId && typeof client.releaseTurn === 'function') client.releaseTurn(sessionId);
347+
} catch (cleanupError) {
348+
if (!primaryError && output?.job?.status !== 'succeeded') primaryError = cleanupError;
349+
}
339350
await client.close().catch(() => {});
340351
if (!primaryError && appliedFinalization && output?.job?.status === 'succeeded' && typeof output.result === 'string') {
341352
await jobLog?.appendBlock('Final output', output.result, Date.now() + OPTIONAL_PROGRESS_FENCE_MS);

‎marketplace/plugins/zcode/scripts/lib/zcode-client.mjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ export class ZCodeClient {
129129
}
130130

131131
/** Wait for a validated terminal notification; no deadline applies unless configured on the client or supplied here. @param {string} sessionId @param {number} [timeoutMs] */ waitForCompletion(sessionId, timeoutMs) { return this.protocol.waitForCompletion(sessionId, timeoutMs); }
132+
/** Observe a validated terminal notification without consuming the active turn. @param {string} sessionId @param {number} [timeoutMs] */ observeCompletion(sessionId, timeoutMs) { return this.protocol.observeCompletion(sessionId, timeoutMs); }
133+
/** Locally release an active turn without sending an upstream request. @param {string} sessionId */ releaseTurn(sessionId) { requireSessionId(sessionId); this.protocol.releaseTurn(sessionId); }
132134
/** Exact local protocol invariant used to prove whether this client owns an active turn. @param {string} sessionId */ turnState(sessionId) { requireSessionId(sessionId); return this.protocol.turnState(sessionId); }
133135
/** @param {string} sessionId @param {{connectionId:string,clientMode:'desktop-continuous'|'web-remote-replayable'}} options */
134136
async subscribeConversation(sessionId, options) {

0 commit comments

Comments
 (0)