Merge pull request #53 from vitry/fix/zcode-0165-permission-turn

fix: retain managed turn through late permissions

Author: vitry

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

@@ -6,6 +6,8 @@
 
 **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.
 
+**Follow-up architecture:** Carry that lifecycle boundary through the managed broker. The broker forwards legacy completion without consuming its upstream turn, and the managed client acknowledges the executor's authoritative cleanup through an exact-owner broker control request. Also accept the observed bounded `requestedAt` field on 0.16.5 permission requests.
+
 **Tech Stack:** Node.js 22.13, ECMAScript modules, `node:test`, the existing JSON-RPC protocol client and job executor.
 
 ---
@@ -17,6 +19,10 @@
 - Modify `scripts/lib/review.mjs`: use non-destructive observation for `legacyWake` and release the local turn during teardown.
 - Modify `tests/process-zcode.test.mjs`: cover low-level observer, permission, timeout, release, and destructive-wait invariants.
 - Modify `tests/job-control.test.mjs`: cover captured 0.16.5 executor ordering and success/error cleanup.
+- Modify `scripts/zcode-broker.mjs`: retain the upstream route through legacy wake and release it only on exact managed-client acknowledgement or existing authoritative cleanup.
+- Modify `tests/zcode-client.test.mjs`: cover broker ordering, acknowledgement ownership, cleanup, and permission forwarding.
+- Modify `tests/fixtures/fake-zcode-cli.mjs`: emit the captured 0.16.5 permission shape after the false legacy completion.
+- Modify `tests/integration/companion.test.mjs`: cover fresh and resumed production managed paths.
 
 ### Task 1: Add non-destructive completion observation
 
@@ -216,3 +222,40 @@ git commit -m "build: refresh marketplace snapshot"
 ```
 
 If there are no generated tracked changes, skip this commit.
+
+### Task 4: Reproduce the managed-broker and 0.16.5 schema failures
+
+**Files:**
+- Test: `tests/zcode-client.test.mjs`
+- Test fixture: `tests/fixtures/fake-zcode-cli.mjs`
+- Test: `tests/process-zcode.test.mjs`
+
+- [ ] Add an opt-in captured fixture permission request after the false legacy completion and before the authoritative v4 terminal. Include the real numeric `requestedAt` field.
+- [ ] Add a direct protocol test proving the current validator rejects that captured field.
+- [ ] Add a managed broker test proving the early completion currently removes the route and the later permission is not allowed.
+- [ ] Run only these tests and record deterministic RED output before changing production code.
+
+### Task 5: Retain and explicitly acknowledge managed broker turns
+
+**Files:**
+- Modify: `scripts/lib/zcode-protocol.mjs`
+- Modify: `scripts/lib/zcode-client.mjs`
+- Modify: `scripts/zcode-broker.mjs`
+- Test: `tests/zcode-client.test.mjs`
+- Test: `tests/process-zcode.test.mjs`
+
+- [ ] Accept only bounded optional `requestedAt` permission timestamps.
+- [ ] Replace broker-side destructive terminal consumption with forwarding/non-destructive observation that retains the exact upstream turn.
+- [ ] Add an authenticated exact-session broker acknowledgement used by managed `releaseTurn()`. It locally releases the upstream turn, settles exact pending permissions, removes the route, and is idempotent for the same completed downstream cleanup without stopping the ZCode session or changing durable ownership.
+- [ ] Reject foreign, stale, malformed, and conflicting acknowledgements without touching a newer turn.
+- [ ] Run focused tests to GREEN, self-review, and commit.
+
+### Task 6: Lock the production fresh/resumed path and re-verify release readiness
+
+**Files:**
+- Modify: `tests/integration/companion.test.mjs`
+- Modify if required by generated parity checks: marketplace snapshot through the existing builder only
+
+- [ ] Extend the captured 0.16.5 managed integration to require an allowed permission response after the false legacy completion for both fresh and resumed sends.
+- [ ] Run focused protocol, broker, executor, and companion tests.
+- [ ] Run `npm run check`, review the complete follow-up diff, refresh the marketplace snapshot if required, and complete spec then quality review before pushing PR #52.

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

@@ -6,17 +6,25 @@ The executor already treats ZCode 0.16.5 `state.updated` notifications with reas
 
 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`.
 
+### Follow-up incident: managed broker boundary
+
+The first implementation preserved the executor-side broker client turn, but production uses two protocol layers. The managed broker still registered `consumeTerminalsWith()` on its upstream appserver connection. That callback consumed the same admission-time legacy notification, settled permission routes, and deleted the broker's active session before the executor could perform authoritative v4/snapshot reconciliation. The resumed incident therefore failed before `decidePermission()` was reached even though the persisted snapshot was `bypassPermissions` and the installed plugin matched the fix byte-for-byte.
+
+ZCode 0.16.5 permission requests also carry a numeric `requestedAt` field. The captured fixture omitted it and the strict protocol validator rejected it. Compatibility must accept this bounded transport metadata while continuing to reject unknown fields.
+
 ## Goals
 
 - Keep the exact turn armed after an admission-time legacy completion wake so later permission requests can be evaluated normally.
 - Preserve the current destructive behavior of `waitForCompletion()` for every existing caller.
 - Clear local turn state when `executeJob` reaches its real terminal or cleanup boundary.
+- Keep the managed broker's upstream turn and permission route alive until the executor confirms its authoritative terminal/cleanup boundary.
+- Accept the observed 0.16.5 `requestedAt` permission-request field.
 - Cover the captured 0.16.5 event ordering with deterministic regression tests.
 
 ## Non-goals
 
 - No change to `decidePermission`, permission snapshots, risk mapping, or offered-response validation.
-- No new appserver flag, permission field, broker authorization rule, or persisted schema.
+- No new appserver flag, permission policy, broker ownership rule, or persisted schema.
 - No reinterpretation of legacy completion as authoritative success or failure.
 - No broad refactor of the protocol or executor lifecycle.
 
@@ -38,10 +46,23 @@ Because the wake no longer consumes local state, `executeJob` must explicitly re
 
 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.
 
+### Managed broker terminal acknowledgement
+
+For a direct appserver client, `releaseTurn()` remains local. For an authenticated managed-broker client, release first sends a narrow broker-only acknowledgement for the exact session turn, then clears the downstream local turn. The broker validates session ownership and the exact active socket/token, locally releases the corresponding upstream protocol turn, settles only that turn's pending permission tasks, and removes its active route. It must not call `session/stop`, release durable session ownership, or accept a stale/foreign acknowledgement.
+
+The broker no longer treats a legacy `prompt_completed` notification as authority to delete its route. It forwards the validated notification to the active client as a wake and retains the upstream turn. Existing authoritative stop, owner-release, disconnect, protocol-close, and explicit terminal-acknowledgement paths remain responsible for cleanup.
+
+### Permission request compatibility
+
+The strict request validator accepts optional `requestedAt` only when it is a finite, non-negative safe integer timestamp. All existing required fields, risk levels, option validation, exact offered-response validation, and unknown-field rejection remain unchanged. The captured 0.16.5 fixture includes this field so broker and direct-client tests exercise the production request shape.
+
 ### Safety invariants
 
 - A validated early legacy completion leaves `turnState(sessionId) === 'armed'`.
 - A later permission request for that session reaches the configured handler and returns one offered response.
+- A managed broker retains the exact route after an early legacy completion and forwards a later permission request.
+- Only the exact owning client can acknowledge and release the broker turn.
+- A 0.16.5 request with bounded `requestedAt` is accepted; malformed timestamps and unknown fields remain rejected.
 - Authoritative executor teardown leaves `turnState(sessionId) === null`.
 - Ordinary `waitForCompletion()` still leaves `turnState(sessionId) === null` immediately after resolution.
 - Permission policy and durable job outcomes are unchanged.
@@ -53,6 +74,8 @@ The cleanup operation is local and idempotent. It must not send `session/stop`,
 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.
 4. Exercise cleanup on a non-success path so an observer cannot leave an armed turn behind.
 5. Run focused tests, then `npm run check` before review and PR creation.
+6. Add a broker-level captured-order regression where permission follows the false legacy completion, and assert explicit acknowledgement releases both protocol layers.
+7. Exercise fresh and resumed managed execution with the same captured ordering.
 
 ## Rollout and compatibility
 

marketplace/.agents/plugins/provenance.json

@@ -1,15 +1,15 @@
 {
   "packageVersion": "0.1.0",
   "pluginVersion": "0.1.0",
-  "sourceRef": "ee1fa5c9778cf6dd02e094cf2bfe7f2835c02dab",
-  "sourceSha": "ee1fa5c9778cf6dd02e094cf2bfe7f2835c02dab",
+  "sourceRef": "9b1b65af0703dfeb885759ec174049a2f3d1f343",
+  "sourceSha": "9b1b65af0703dfeb885759ec174049a2f3d1f343",
   "dependencyLock": {
     "file": "npm-shrinkwrap.json",
     "sha256": "fa927194e6ca0b25c1d3f428859b2ab4798b8eabb4d31bc87188d73c630f9938"
   },
   "content": {
     "algorithm": "sha256",
-    "sha256": "40d1a65be6976f5f070b2784a35635f5aba4597c4846ec798b86d3f30dd77af7",
+    "sha256": "04443494d3f15c34e2d2f92acebea3a133d97cd9cab41f9034140e33e85d3020",
     "files": [
       {
         "path": ".agents/plugins/marketplace.json",
@@ -658,8 +658,8 @@
       },
       {
         "path": "plugins/zcode/scripts/lib/review.mjs",
-        "size": 43572,
-        "sha256": "0d945d223ce618ee28e2e81831ce855df1b18cc975f45dcc5538b8cc6e934d11"
+        "size": 44075,
+        "sha256": "8575a666fadd8022373e6c54ea4aabe7e3af708d1d64d002d9da85dc454378c1"
       },
       {
         "path": "plugins/zcode/scripts/lib/session-progress.mjs",
@@ -703,8 +703,8 @@
       },
       {
         "path": "plugins/zcode/scripts/lib/zcode-client.mjs",
-        "size": 52495,
-        "sha256": "0d4c854d4045260cc86d14d30e647db22efb08bd4f9ad30a2f795c3b5e37f901"
+        "size": 57159,
+        "sha256": "0f6542ca1d80f44745b1f1ff7d3f065db24b0e9f14060a99a174728b2cb7e604"
       },
       {
         "path": "plugins/zcode/scripts/lib/zcode-discovery.mjs",
@@ -713,8 +713,8 @@
       },
       {
         "path": "plugins/zcode/scripts/lib/zcode-protocol.mjs",
-        "size": 42018,
-        "sha256": "68e98ef8be8b670648e3add7966fb1619b3836be0ce6b2a29e678899f68f9426"
+        "size": 43295,
+        "sha256": "d01d37715518b956ed6ff6b2d0f1fafadc51c9c4998ce3617f14ef64230ba409"
       },
       {
         "path": "plugins/zcode/scripts/lib/zcode-runtime-config.mjs",
@@ -728,8 +728,8 @@
       },
       {
         "path": "plugins/zcode/scripts/zcode-broker.mjs",
-        "size": 103199,
-        "sha256": "3861c818720c569ac483c1b6488d0165e039f392d1ab9a5186e2175a71008a08"
+        "size": 107670,
+        "sha256": "c85c04fb48bacae4ca41d937ec9de0905d25fdb7f99bd76e4c122a876bd673ba"
       },
       {
         "path": "plugins/zcode/scripts/zcode-companion.mjs",

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

@@ -342,12 +342,21 @@ export async function executeJob(input) {
   }
   // Cleanup order is part of the progress lifecycle contract.
   await cleanupProgress();
-  try {
-    if (sessionId && typeof client.releaseTurn === 'function') client.releaseTurn(sessionId);
-  } catch (cleanupError) {
-    if (!primaryError && output?.job?.status !== 'succeeded') primaryError = cleanupError;
+  let releaseError;
+  if (sessionId && typeof client.releaseTurn === 'function') {
+    for (let attempt = 0; attempt < 2; attempt += 1) {
+      try { await client.releaseTurn(sessionId); releaseError = undefined; break; }
+      catch (cleanupError) { releaseError = cleanupError; }
+    }
+  }
+  if (releaseError) {
+    await jobLog?.appendBlock('Cleanup diagnostic', 'ZCode turn release cleanup was incomplete.', Date.now() + OPTIONAL_PROGRESS_FENCE_MS).catch(() => {});
+    if (!primaryError && output?.job?.status !== 'succeeded') primaryError = releaseError;
+  }
+  try { await client.close(); }
+  catch {
+    await jobLog?.appendBlock('Cleanup diagnostic', 'ZCode client close cleanup was incomplete.', Date.now() + OPTIONAL_PROGRESS_FENCE_MS).catch(() => {});
   }
-  await client.close().catch(() => {});
   if (!primaryError && appliedFinalization && output?.job?.status === 'succeeded' && typeof output.result === 'string') {
     await jobLog?.appendBlock('Final output', output.result, Date.now() + OPTIONAL_PROGRESS_FENCE_MS);
   }