Skip to content

Control automatic handoff availability#3

Open
grzegorznowak wants to merge 25 commits into
agenticoding:mainfrom
grzegorznowak:story-03-handoff-resume-control
Open

Control automatic handoff availability#3
grzegorznowak wants to merge 25 commits into
agenticoding:mainfrom
grzegorznowak:story-03-handoff-resume-control

Conversation

@grzegorznowak

@grzegorznowak grzegorznowak commented May 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR adds handoff.automaticEnabled to control automatic agent-initiated handoff while preserving explicit operator /handoff <direction> as the manual path.

The implementation uses guard-based semantics: the handoff tool remains registered, prompt/watchdog/status/notebook guidance is suppressed when automatic handoff is disabled, and direct tool calls are rejected unless they are satisfying an active manual /handoff generated user turn. Successful handoff compaction continues to auto-send Pi's fixed post-compaction Proceed. continuation; handoff.resumeBehavior is ignored and no continuation mode is configurable.

Latest update — FB-033..FB-037

Closes the latest requested-change follow-ups from 2026-06-06:

  • FB-033 — stale pending-compaction ownership: session_before_compact cleanup now clears visible manual request/status/prompt/retry state only when the consumed pending compaction owns the current manual request by generation and request id, preserving newer queued /handoff requests.
  • FB-034 — queued manual watchdog/status guidance: queued manual requests awaiting their generated turn now receive queued-request guidance instead of automatic-enabled generic watchdog text telling the unauthorized current turn to call handoff; queued warnings/status continue to say not to call the tool until the generated turn starts.
  • FB-035 — active manual disabled-mode primer/footer: active manual generated-turn primer/watchdog/footer copy no longer carries ordinary disabled clean-transition/operator fallback wording while still instructing the model to save notes, draft the brief, and call handoff.
  • FB-036 — atomic settings temp-file permissions: global settings atomic saves now create/write temp files with the preserved existing mode or restrictive 0600 mode before settings content is written.
  • FB-037 — diagnostics fix-then-rebreak visibility: availability-warning dedupe resets after valid/fixed settings states so a later same invalid/unsupported state emits a fresh warning without adding settings-read caching.

Previous requested-change closures

  • FB-030..FB-032: settings writer no-clobber for malformed present handoff parents, active manual generated-turn guidance override while automatic handoff is disabled, and disabled topic-boundary watchdog coverage.
  • FB-023..FB-029: UUID manual request identity activation, queued-vs-active handoff status/warning split, stale manual and automatic/direct compaction-error status preservation, malformed nested settings fail-closed handling, file-mode preservation for atomic global settings saves, JSON-escaped unsupported diagnostics/TUI display, and README/CHANGELOG queued-vs-active precision.
  • FB-019..FB-022: manual request generation isolation for compaction retry races, disabled-mode manual status alignment, fixed non-configurable Proceed. changelog coverage, and save-time project override warning coverage.
  • FB-015..FB-018: manual /handoff <direction> compaction onError retry preservation, disabled-mode prompt fail-closed coverage including notebook_index, and bounded/deduplicated unchanged bad-source diagnostics.

Requirements

  • Default automatic handoff remains enabled when the setting is absent.
  • handoff.automaticEnabled=false suppresses agent handoff-call guidance during normal turns.
  • Direct handoff tool execution while automatic handoff is disabled is blocked unless an explicit /handoff <direction> request is active in its generated user turn.
  • Queued manual /handoff <direction> requests are pending delivery only; they do not authorize the current turn to call handoff.
  • Manual requests are activated only when the generated /handoff user message actually starts, using request identity rather than prompt equality.
  • A requested /handoff remains valid across intermediate notebook/tool provider turns before the model finally calls handoff.
  • If compaction fails after an active manual request, the request remains retry-ready instead of being cleared as stale.
  • Older compaction-error callbacks cannot corrupt newer same-direction manual requests or clear newer queued status.
  • Stale pending-compaction cleanup cannot clear newer queued manual request/status/prompt/retry state unless generation and request id prove ownership.
  • Queued manual request status/watchdog guidance never tells the unauthorized current turn to call handoff, including automatic-enabled watchdog branches.
  • Atomic global settings temp files are written with preserved or restrictive permissions before settings content is written.
  • Availability warning dedupe re-emits after settings are fixed and later broken again with the same bad state.
  • /agenticoding-settings persists the global raw JSON boolean, preserves unrelated settings and existing file mode, refuses malformed present global handoff parents without clobbering them, and warns when project settings override the saved global value.

Acceptance criteria

  • Reads handoff.automaticEnabled from global ~/.pi/agent/settings.json plus project <cwd>/.pi/settings.json, with project override and own nested keys only.
  • Missing files or absent key default to automaticEnabled: true.
  • Unsupported non-boolean values, invalid JSON, non-object roots, present non-object handoff parents, and non-ENOENT read failures fail closed to disabled with warning diagnostics; repeated unchanged bad-source warnings are deduplicated.
  • Unsupported string diagnostics/TUI display values are JSON-escaped/stringified.
  • Automatic-disabled normal turns avoid telling the model to call handoff, recommend /handoff, or mention handoff timing in primer/watchdog/topic/status/notebook guidance, including notebook_index and disabled topic-boundary watchdog nudges.
  • Active manual /handoff generated turns override disabled fallback prompt/topic guidance and tell the model to draft the handoff brief and call the temporarily enabled tool.
  • Manual /handoff <direction> records a pending operator request, sends/queues the handoff prompt, routes through normal compaction, and clears state after success or stale agent completion.
  • Busy manual /handoff <direction> queues the generated prompt as a follow-up instead of waiting inside the command handler.
  • README and CHANGELOG document the new key, default, disabled/manual behavior, queued-vs-active semantics, guard-based enforcement, TUI/global-only/project override behavior, and fixed non-configurable Proceed. continuation.

Contract changes

  • Adds boolean handoff.automaticEnabled for automatic agent-initiated handoff availability.
  • Keeps /agenticoding-settings as the dedicated extension-owned settings surface.
  • Keeps /handoff <direction> focused on manual handoff initiation, not configuration.
  • Keeps post-compaction Proceed. fixed and non-configurable.

Out of scope

  • Changing Pi core compaction behavior.
  • Disabling explicit operator /handoff <direction>.
  • Adding /handoff --enable, /handoff --disable, /handoff --wait, /handoff --proceed, or direct tool-call override parameters.
  • Adding project-scope writes to the settings TUI.
  • Adding per-session caching for automatic handoff setting reads.
  • Adding no-op turn_start cleanup changes.

How to verify

Run from this branch/worktree after preparing /tmp/pi-agenticoding-test-loader.mjs from test-loader.mjs with the local Pi package path:

node --loader /tmp/pi-agenticoding-test-loader.mjs --test \
  --test-name-pattern 'stale pending compaction|automatic enabled watchdog distinguishes queued|active manual primer has no clean-transition|atomic save writes temp file|diagnostics re-emit after fixed settings break again' \
  agenticoding.test.ts

node --loader /tmp/pi-agenticoding-test-loader.mjs --test \
  --test-name-pattern 'handoff automatic|automatic handoff|handoff availability|agenticoding settings|handoff prompt|active tools|queued manual|manual slash handoff|watchdog distinguishes|primer has no clean-transition|stale pending compaction' \
  agenticoding.test.ts

node --loader /tmp/pi-agenticoding-test-loader.mjs --test agenticoding.test.ts

git diff --check

Latest local verification passed on 2026-06-06 at commit f2fad77:

  • Focused FB-033..FB-037 regressions: 5/5
  • Focused Story 03/manual/settings prompt pattern: 45/45
  • Full suite: 183/183
  • git diff --check
  • Fresh implementation review approved with no material findings

Live verification from earlier PR iteration:

  • With project-local handoff.automaticEnabled=false, /handoff to discuss the importance of saying hello successfully compacted after the model used notebook tools before calling handoff.
Captura de pantalla_2026-05-28_12-31-26 Captura de pantalla_2026-05-28_12-41-57

@grzegorznowak grzegorznowak marked this pull request as draft May 24, 2026 17:29
@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Summary: The PR implements the requested handoff wait/proceed behavior and settings UI, but the raw settings merge can treat prototype/meta JSON keys as real handoff.resumeBehavior, which can violate the new safe default.

Reviewed groups sequentially: settings resolver/UI, handoff integration, then tests/docs.

Business / Product Assessment

Verdict: REQUEST CHANGES

Strengths

  • The default-wait, explicit-proceed, invalid JSON fallback, unsupported value fallback, and project override cases are covered by focused tests. Sources: agenticoding.test.ts:394, agenticoding.test.ts:436, agenticoding.test.ts:446, agenticoding.test.ts:459
  • User-facing docs describe the new default, opt-in proceed, global-only TUI save behavior, and project override semantics. Sources: README.md:43, README.md:52, README.md:129

In Scope Issues

  • Settings JSON can omit an own handoff.resumeBehavior but still resolve inherited prototype data as proceed. Sources: settings.ts:62, settings.ts:75, settings.ts:138

    Medium severity · Low likelihood

    Why: The acceptance criteria require missing handoff.resumeBehavior to resolve to wait. Because the merge target is a normal object and extraction reads inherited properties, a raw settings file containing a __proto__ object can make the resolver see handoff.resumeBehavior: "proceed" even though the real nested setting is absent. That can unexpectedly auto-send Proceed. after compaction.

    Assumptions / Preconditions: A global or project settings file contains JSON like { "__proto__": { "handoff": { "resumeBehavior": "proceed" } } } and no own top-level handoff.resumeBehavior.

    Downgrade Factors: This requires malformed or unusual local settings JSON; normal documented settings are handled correctly.

    Code Trail: readSettingsSource parses arbitrary raw JSON into a plain object, mergeSettings writes every key into a normal object, including __proto__, and extractResumeBehavior then reads settings.handoff without requiring an own property. The handoff resolver trusts that extracted value and returns "proceed" when it matches a supported value.

    Reproduction: Put the JSON above in ~/.pi/agent/settings.json or <project>/.pi/settings.json; the file has no own handoff.resumeBehavior, but the merged object exposes inherited handoff.resumeBehavior and can opt into auto-resume.

Out of Scope Issues

  • None.

Technical Assessment

Verdict: REQUEST CHANGES

Input Boundary Shape Risk Assessment

Status: Triggered
Boundary: Raw global/project Pi settings JSON -> nested merge and handoff.resumeBehavior resolver
Evidence / mitigation: Invalid JSON and unsupported own values are handled safely, but the boundary does not constrain prototype/meta keys before merging and reading stricter application settings assumptions.

Strengths

  • /handoff initiation and direct tool use share the same runtime behavior because the setting is resolved inside the handoff tool before compaction. Sources: handoff/tool.ts:126, handoff/tool.ts:128, handoff/command.ts:39
  • The settings UI save path preserves unrelated global settings keys while writing only nested handoff.resumeBehavior. Sources: settings.ts:172, settings.ts:182, settings.ts:187

In Scope Issues

  • None.

Out of Scope Issues

  • None.

Reusability

  • The PR keeps settings reading, model construction, display lines, and global write behavior in a separate module that can be reused by both runtime handoff logic and UI tests. Sources: settings.ts:115, settings.ts:192, settings.ts:239, settings.ts:349
  • GitHub reported no checks for this PR branch; I did not run local tests per the review sandbox rules. Sources: agenticoding.test.ts:394

review generated with CURe v. 0.7.3 · single-stage · sha bf278cb · model gpt-5.5/high · tok 602k/8k/610k · session agenticoding-pi-agenticoding-pr3-20260524-190841-2c39 · 4m5s

@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Summary: The PR largely implements the requested wait-by-default handoff behavior and settings surface, but configuration failure handling still has fail-open paths that can produce unexpected auto-resume or silent save failures.

Business / Product Assessment

Verdict: REQUEST CHANGES

Strengths

  • The default is now wait when handoff.resumeBehavior is absent, and Proceed. is only sent for explicit proceed. Sources: settings.ts:170, handoff/tool.ts:134
  • The settings display includes the resolved value, supported values, default behavior, global/project state, and global-only save note. Sources: settings.ts:272
  • User-facing docs describe the new default, opt-in proceed, TUI command, global save behavior, and project override semantics. Sources: README.md:120

In Scope Issues

  • Unreadable settings sources fail open instead of failing safely to wait Sources: settings.ts:130, settings.ts:157, settings.ts:194

    Medium severity · Low likelihood

    Why: The PR promises configuration problems fall back safely with warnings. Today, any settings read error is treated as “file missing,” so a broken project settings source can be ignored and a global "proceed" value can still auto-resume after compaction.

    Assumptions / Preconditions: A settings path exists but cannot be read, such as EACCES, EISDIR, or a transient filesystem error.

    Downgrade Factors: If Pi guarantees these settings files are always readable when present, impact drops. I did not find that guarantee in the reviewed code.

    Code Trail: handoff/tool.ts resolves resume behavior before registering the compaction callback. readSettingsSource catches all readFile errors and returns exists: false, invalid: false. resolveHandoffResumeBehavior only warns and returns wait for invalid sources, so non-ENOENT read failures are skipped. The write path uses the same “unreadable means missing” policy, which can also lose preservation guarantees if a file is writable but unreadable.

    Reproduction: Configure global settings as { "handoff": { "resumeBehavior": "proceed" } }, then make <cwd>/.pi/settings.json unreadable or a directory. Trigger handoff; the project source is treated as absent, no warning is emitted, and global proceed can send Proceed..

Out of Scope Issues

  • None.

Technical Assessment

Verdict: REQUEST CHANGES

Input Boundary Shape Risk Assessment

Status: Triggered
Boundary: Raw persisted JSON from ~/.pi/agent/settings.json and <cwd>/.pi/settings.json -> strict handoff.resumeBehavior enum and TUI settings model
Evidence / mitigation: JSON is parsed, cloned through own enumerable keys, and enum-validated before use. Prototype-key handling is covered statically. The missing-proof gap is non-JSON read failures, which are currently treated as absent instead of invalid or diagnosable. Sources: settings.ts:138, settings.ts:83, settings.ts:111, agenticoding.test.ts:446

Strengths

  • The settings merge path uses null-prototype objects and own-property access, reducing prototype pollution risk from raw settings JSON. Sources: settings.ts:62, settings.ts:66, settings.ts:83
  • Static tests cover default wait, explicit proceed, project override precedence, invalid JSON, unsupported values, prototype/meta keys, UI fallback, and global-only save behavior. Sources: agenticoding.test.ts:394, agenticoding.test.ts:436, agenticoding.test.ts:596

In Scope Issues

  • Settings TUI save failures can become unhandled async rejections Sources: settings.ts:338, settings.ts:219

    Medium severity · Low likelihood

    Why: A settings save that fails at mkdir or writeFile should produce a controlled user diagnostic and leave the panel in a known state. The current fire-and-forget async handler has no catch path, so filesystem failures can surface as unhandled rejections with no useful TUI feedback.

    Assumptions / Preconditions: The global settings directory or file cannot be created or written.

    Downgrade Factors: If the host framework globally catches rejected promises spawned inside component callbacks and reports them to users, impact is lower. The local code does not show that protection.

    Code Trail: The SettingsList change callback starts a void async IIFE. Inside it, model.save reaches writeGlobalHandoffResumeBehavior, which awaits mkdir and writeFile without catching write failures. A rejection skips buildAgenticodingSettingsModel, refreshSummary, and requestRender.

    Reproduction: Point HOME at a location where .pi/agent/settings.json cannot be written, open /agenticoding-settings, and change the value. The save promise rejects outside the component’s control instead of notifying the user.

Out of Scope Issues

  • None.

Reusability

  • The read/merge/extract helpers create a reasonable foundation for future raw Pi settings keys, though the module is intentionally handoff-specific today. Sources: settings.ts:91, settings.ts:104
  • The model/display/component split keeps persistence separate from the TUI rendering path and should be reusable for additional agenticoding settings. Sources: settings.ts:225, settings.ts:272, settings.ts:301

review generated with CURe v. 0.7.3 · single-stage · sha 85879c3 · model gpt-5.5/high · tok 594k/9k/603k · session agenticoding-pi-agenticoding-pr3-20260524-193138-4af8 · 5m14s

…ave failures

FB-002: readSettingsSource now distinguishes ENOENT (file genuinely
missing -> exists:false) from other read errors like EACCES/EISDIR
(exists:true, invalid:true). The resolveHandoffResumeBehavior function
already handles invalid sources with warnings and fallback to wait.

FB-003: The async IIFE in createAgenticodingSettingsComponent's
SettingsList change callback now wraps the save/rebuild sequence in
try/catch. On failure it calls notify() with an error-level message
instead of silently dropping the rejection as an unhandled promise.

Regression tests:
- non-ENOENT read error test (FB-002): makes global settings file
  unreadable via chmod 000, asserts invalid:true + warning + wait
- write failure test (FB-003): blocks the .pi/agent directory with a
  file, asserts writeGlobalHandoffResumeBehavior rejects with EEXIST
@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Summary: This PR changes handoff to wait by default, adds opt-in auto-proceed plus /agenticoding-settings, and I found no blocking product or technical issues; gh pr checks reported no checks for the branch.

Business / Product Assessment

Verdict: APPROVE

Strengths

  • Implements the requested default change: absent or invalid handoff.resumeBehavior resolves to wait, while proceed remains available. Sources: settings.ts:164, settings.ts:177
  • The settings UI surfaces resolved value, supported values, default behavior, global/project state, and project override warnings. Sources: settings.ts:287, settings.ts:259
  • User-facing documentation covers the new default, opt-in migration path, TUI command, and override semantics. Sources: README.md:120, README.md:128

In Scope Issues

  • None.

Out of Scope Issues

  • None.

Technical Assessment

Verdict: APPROVE

Input Boundary Shape Risk Assessment

Status: Triggered
Boundary: Raw Pi settings JSON files (~/.pi/agent/settings.json, <cwd>/.pi/settings.json) -> stricter handoff.resumeBehavior enum and settings UI model.
Evidence / mitigation: The code reads and parses both JSON sources, rejects non-object or malformed roots, ignores inherited/prototype settings keys, validates only wait/proceed, falls back to wait, and blocks global writes when existing global JSON cannot be safely read or parsed. Sources: settings.ts:130, settings.ts:142, settings.ts:164, settings.ts:201

Strengths

  • Settings parsing is conservative: malformed JSON, unsupported values, and read errors fail safe to wait rather than auto-resuming. Sources: settings.ts:167, settings.ts:185
  • The handoff tool resolves behavior once before compaction and only sends Proceed. when the resolved value is explicitly proceed. Sources: handoff/tool.ts:128, handoff/tool.ts:133
  • Test coverage statically covers default wait, opt-in proceed, invalid JSON, unsupported values, project overrides, global-only saves, and fallback UI behavior. Sources: agenticoding.test.ts:398, agenticoding.test.ts:440, agenticoding.test.ts:663

In Scope Issues

  • None.

Out of Scope Issues

  • None.

Reusability

  • The settings read/merge/resolve helpers are separated from the TUI and handoff tool, so future agenticoding settings can likely reuse the same shape. Sources: settings.ts:154, settings.ts:236
  • The command registration follows the existing extension wiring pattern and keeps /handoff behavior separate from /agenticoding-settings. Sources: index.ts:53, settings.ts:401

review generated with CURe v. 0.7.3 · single-stage · sha a9a3959 · model gpt-5.5/high · tok 938k/8k/946k · session agenticoding-pi-agenticoding-pr3-20260525-082158-f9e7 · 8m11s

@grzegorznowak grzegorznowak marked this pull request as ready for review May 25, 2026 08:42
@ofriw

ofriw commented May 26, 2026

Copy link
Copy Markdown
Contributor

First review LGTM. Waiting for merge with main.

@grzegorznowak grzegorznowak marked this pull request as draft May 26, 2026 15:14
…me-control

# Conflicts:
#	README.md
#	agenticoding.test.ts
#	handoff/tool.ts
@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Merged latest origin/main into story-03-handoff-resume-control and resolved conflicts while keeping the PR in draft.\n\nResolution summary:\n- Kept PR #6 notebook/topic terminology and rehydration changes.\n- Preserved story-03 handoff resume control: default wait, optional handoff.resumeBehavior = \"proceed\", and /agenticoding-settings docs/tests.\n- Resolved handoff/tool.ts so the handoff brief uses notebook-on-demand semantics and only sends Proceed. when the resolved setting is proceed.\n\nValidation:\n- node --experimental-strip-types --loader /tmp/pi-agenticoding-test-loader.mjs --test agenticoding.test.ts — 143 pass, 0 fail.\n\nHead: 852a181be688603ae7e361d8b091769cf86adecd. PR remains draft.

@grzegorznowak grzegorznowak changed the title Control handoff auto-resume after compaction Control automatic handoff availability May 27, 2026
@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Merged current origin/main into story-03-handoff-resume-control and pushed 15afdb0.

Conflict resolution:

  • kept Story 03 manual /handoff flow (idle wait, temporary tool activation, no queued follow-up/deliverAs path);
  • incorporated the main-branch wording “that aligns with the direction above” into the manual handoff prompt and matching test expectation;
  • accepted the auto-merged handoff tool guideline wording “that the next context will need”.

Verification after merge:

  • Focused: 25/25
  • Full: 149/149

@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Update: restored mainline post-handoff auto-Proceed. behavior per operator direction. New head 140f8b7 changes handoff/tool.ts to call pi.sendUserMessage("Proceed.") on successful compaction, keeps handoff.automaticEnabled as the autonomous-tool availability control, and keeps handoff.resumeBehavior ignored/non-configuring.\n\nVerification from /workspaces/chunkhound_workspace/pi-agenticoding with /tmp/pi-agenticoding-test-loader.mjs:\n- Focused automatic/settings/prompt/active-tools/manual handoff/watchdog pattern: 25/25 pass\n- Full agenticoding.test.ts: 149/149 pass

@grzegorznowak grzegorznowak force-pushed the story-03-handoff-resume-control branch 2 times, most recently from 278063f to 941d055 Compare May 28, 2026 10:28
@grzegorznowak grzegorznowak force-pushed the story-03-handoff-resume-control branch 2 times, most recently from 59d3dcc to 699484c Compare May 28, 2026 12:12
@grzegorznowak grzegorznowak marked this pull request as ready for review May 28, 2026 12:49
@grzegorznowak grzegorznowak force-pushed the story-03-handoff-resume-control branch from 699484c to ff0ab7f Compare May 28, 2026 14:02
@grzegorznowak grzegorznowak force-pushed the story-03-handoff-resume-control branch from ff0ab7f to 5a3ca23 Compare May 28, 2026 15:26
@grzegorznowak grzegorznowak requested a review from ofriw May 28, 2026 15:56
@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

@ofriw Added the tool nudge tweak mentioned on Discord. It seems to be working stable. Please review when you can 🙏🏾

ofriw
ofriw previously requested changes Jun 3, 2026

@ofriw ofriw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DELETED

@ofriw ofriw dismissed their stale review June 3, 2026 12:31

Mistake

@ofriw ofriw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This review was generated by an AI agent. If you'd like to talk with other humans, drop by our Discord!


Hey Grzegorz — nice work on this PR. The settings infrastructure is well-thought-out, especially the prototype-safe JSON handling and atomic writes. The test coverage is thorough, and the withIsolatedSettings pattern makes the edge-case tests reliable. The async send-failure handling in the command handler is a nice touch.

Two things need fixing before merge:

1. Misleading error messages on read failures — When a settings file exists but can't be read (permissions, directory collision, transient FS error), the code reports "Invalid global settings JSON." That's actively misleading — the JSON is fine, the file's just unreachable. readSettingsSource catches all non-ENOENT errors in one bucket, then resolveHandoffAutomaticAvailability slaps the "invalid JSON" label on them. Should distinguish EACCES, EISDIR, etc. and report the actual problem. This also affects the TUI save-blocking logic in buildAgenticodingSettingsModel.

2. Compaction error handler nukes the pending manual handoff — On ctx.compact errors, clearPendingHandoffCompaction nullifies pendingRequestedHandoff entirely. The old code only set toolCalled = false, preserving the request so the watchdog could retry. Now a transient compaction failure (filesystem glitch, API hiccup) forces the operator to re-issue /handoff <direction>. The request should survive transient errors.

Would also flag: the tool no longer has promptSnippet or promptGuidelines, which means pi's tool-suggestion system won't recommend handoff. The guidance moved to the system prompt, which is better for conditional rendering but loses tool-level discoverability. Worth restoring a condensed version.


Attached is an agent optimized description of the changes in this PR - AGENT_REVIEW.md

@ofriw ofriw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This review was generated by an AI agent. If you'd like to talk with other humans, drop by our Discord!


Hey Grzegorz — the guard-based architecture here is solid work. The awaitingAgentTurn lifecycle for preventing stale-turn preemption, the prototype-safe settings handling, the atomic writes, and the withIsolatedSettings test pattern are all well done. Test coverage is thorough.

One thing needs fixing before merge: compaction errors now destroy the manual handoff request. The old onError callback only reset toolCalled = false, preserving the request so the watchdog could retry. The new clearPendingHandoffCompaction in handoff/cleanup.ts nullifies pendingRequestedHandoff and pendingRequestedHandoffPrompt entirely — so a transient compaction failure forces the operator to re-issue /handoff. The fix is straightforward: on onError, reset pendingHandoff = null and pendingRequestedHandoff.toolCalled = false but leave the request itself intact. The test asserting pendingRequestedHandoff === null after onError will need updating too.

A few minor items for a follow-up: (1) readSettingsSource reports all non-ENOENT errors as "Invalid JSON" — EACCES should say "unable to read" instead. (2) resolveHandoffAutomaticAvailability reads two files from disk on every call (~6 calls/turn) — a per-session cache would be a quick win. (3) The no-op turn_start hook in index.ts should be removed.


Attached is an agent optimized description of the changes in this PR - AGENT_REVIEW.md

@grzegorznowak

Copy link
Copy Markdown
Collaborator Author

Summary: pi-agenticoding/03 mostly implements configurable automatic handoff, but manual compaction-error retry and disabled-mode guidance gaps block the intended human-controlled handoff behavior.

Business / Product Assessment

Verdict: REQUEST CHANGES

Strengths

  • The user-facing docs describe the new default-enabled setting, disabled/manual behavior, guard-based enforcement, and global-only settings TUI with project override semantics. Sources: README.md:120, README.md:130, README.md:132
  • The changelog clearly records the new setting and preserves explicit /handoff <direction> as the manual path when automatic handoff is disabled. Sources: CHANGELOG.md:12, CHANGELOG.md:14

In Scope Issues

  • Manual /handoff compaction errors can still lose the retry-ready request during normal agent cleanup. Sources: work/pr_context.json:5, handoff/tool.ts:97, handoff/cleanup.ts:41, watchdog.ts:95

    High severity · Medium likelihood

    Why: The PR explicitly promises that a failed manual handoff compaction leaves the operator’s request available for retry. The error callback restores that request, but the restored state matches the stale-cleanup predicate used at agent_end, so the retry can be cleared before the operator gets the promised retry behavior.

    Assumptions / Preconditions: A manual /handoff request is active, the handoff tool starts compaction, compaction fails, and the normal agent_end handler runs after the error callback restores the request.

    Downgrade Factors: If Pi guarantees agent_end cannot run after compact.onError for a terminating tool call, the impact would be lower, but that ordering guarantee is not established in the PR evidence.

    Code Trail: handoff stores a retry copy before marking the active request as called, restores it in onError, and returns terminate: true; preserveManualHandoffRequestAfterCompactionError writes it back with toolCalled: false and awaitingAgentTurn: false; the watchdog clears any request with exactly that shape on agent_end.

    Reproduction: Configure handoff.automaticEnabled=false, issue /handoff implement auth, let the model call handoff, trigger compactOptions.onError, then run the registered agent_end handler. The pending request is eligible for stale cleanup instead of remaining retry-ready.

  • Disabled automatic mode still exposes a normal parent-session notebook hint that mentions “after handoff.” Sources: work/pr_context.json:5, notebook/tools.ts:203, notebook/tools.ts:245, agenticoding.test.ts:4410

    Low severity · High likelihood

    Why: The disabled-mode product contract is that normal turns should avoid handoff guidance. notebook_index is registered with parent prompt hints unconditionally, and one guideline still names handoff, while the suppression test only checks notebook_topic_set and notebook_write.

    Assumptions / Preconditions: Parent prompt hints are exposed to the model in disabled automatic mode.

    Downgrade Factors: The text says to scan the index “after handoff”; it does not directly tell the model to call handoff, so this is a lower-impact leakage than an explicit handoff recommendation.

    Code Trail: registerNotebookTools creates notebook tools with withPromptHints: true; notebook_index includes the “after handoff” guideline; the disabled-mode test checks topic and write guidelines but not index guidelines.

    Reproduction: Disable automatic handoff, register notebook tools, inspect pi.tools.get("notebook_index").promptGuidelines; the guideline still includes after handoff.

Out of Scope Issues

  • None.

Technical Assessment

Verdict: REQUEST CHANGES

Input Boundary Shape Risk Assessment

Status: Triggered
Boundary: Raw global/project settings JSON files -> stricter boolean handoff.automaticEnabled runtime assumption.
Evidence / mitigation: The PR parses raw settings files, treats missing files separately from read errors, rejects non-object roots, clones only own settings keys, extracts only own nested handoff.automaticEnabled, and fails unsupported values closed with warning diagnostics. Sources: settings.ts:98, settings.ts:115, settings.ts:139, settings.ts:210, settings.ts:223, settings.ts:280

Strengths

  • Settings parsing avoids inherited/prototype values and preserves the intended global/project layering for the new boolean setting. Sources: settings.ts:123, settings.ts:139, settings.ts:146, agenticoding.test.ts:536
  • The runtime guard keeps the handoff tool registered while refusing direct automatic-disabled tool calls unless an active manual request exists. Sources: handoff/tool.ts:63, handoff/tool.ts:73, handoff/tool.ts:77

In Scope Issues

  • Settings diagnostics are emitted every time availability is resolved, including high-frequency runtime paths. Sources: settings.ts:259, settings.ts:262, settings.ts:280, index.ts:209, index.ts:269, index.ts:302, index.ts:320, notebook/tools.ts:98

    Medium severity · High likelihood

    Why: A single malformed or unsupported setting can generate repeated warnings during normal operation, including prompt setup, context nudges, session start, turn end, and notebook writes. That makes diagnostics noisy and can obscure the actionable configuration problem.

    Assumptions / Preconditions: A global or project settings file remains invalid, unreadable, non-object, or contains an unsupported handoff.automaticEnabled value.

    Downgrade Factors: If the host UI already deduplicates identical notifications, the user-visible impact is reduced, but no such dedupe is present in the changed code.

    Code Trail: resolveHandoffAutomaticAvailability reads settings and calls notify for invalid sources or unsupported values. The PR then calls that resolver from multiple lifecycle hooks and from notebook_write, even where the caller only needs the effective boolean to refresh indicators.

    Reproduction: Leave <project>/.pi/settings.json malformed, then start a session, run a turn, and write a notebook page. Each resolver call can emit the same warning again.

  • The compaction-error tests miss the full lifecycle ordering that can clear restored retry state. Sources: agenticoding.test.ts:729, agenticoding.test.ts:734, agenticoding.test.ts:1376, agenticoding.test.ts:1387, handoff/compact.ts:25, watchdog.ts:95

    Medium severity · High likelihood

    Why: The tests assert onError restoration directly, but they do not follow it with the normal agent_end stale cleanup that currently matches the restored retry shape. That leaves the product-critical retry behavior under-tested.

    Assumptions / Preconditions: The host fires agent_end after a terminating tool call whose compaction fails.

    Downgrade Factors: A host-level ordering guarantee could reduce this gap, but the tests should encode that guarantee or cover the cleanup path.

    Code Trail: One test invokes compactOptions.onError({}) and immediately asserts the restored state; another does the same without registering the compaction hook. The actual compaction hook clears request state before compaction consumes it, and the watchdog later clears !toolCalled && !awaitingAgentTurn requests.

    Reproduction: Extend the existing compaction-error test to call the registered agent_end handler after compactOptions.onError({}); the restored pending request should remain available for retry.

Out of Scope Issues

  • None.

Reusability

  • The settings helpers are reusable for future raw settings keys because parsing, own-key cloning, layered lookup, model building, and write behavior are separated instead of embedded in one command path. Sources: settings.ts:115, settings.ts:123, settings.ts:146, settings.ts:210

review generated with CURe v. 0.8.0 · multi-stage - stages: 5 · sha 8010da8 · model gpt-5.5/high · tok 9m/97k/9m · session agenticoding-pi-agenticoding-pr3-20260605-162753-024f · 13m59s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants