Control automatic handoff availability#3
Conversation
|
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 Reviewed groups sequentially: settings resolver/UI, handoff integration, then tests/docs. Business / Product AssessmentVerdict: REQUEST CHANGES Strengths
In Scope Issues
Out of Scope Issues
Technical AssessmentVerdict: REQUEST CHANGES Input Boundary Shape Risk AssessmentStatus: Triggered Strengths
In Scope Issues
Out of Scope Issues
Reusability
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 |
|
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 AssessmentVerdict: REQUEST CHANGES Strengths
In Scope Issues
Out of Scope Issues
Technical AssessmentVerdict: REQUEST CHANGES Input Boundary Shape Risk AssessmentStatus: Triggered Strengths
In Scope Issues
Out of Scope Issues
Reusability
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
|
Summary: This PR changes handoff to wait by default, adds opt-in auto-proceed plus Business / Product AssessmentVerdict: APPROVE Strengths
In Scope Issues
Out of Scope Issues
Technical AssessmentVerdict: APPROVE Input Boundary Shape Risk AssessmentStatus: Triggered Strengths
In Scope Issues
Out of Scope Issues
Reusability
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 |
|
First review LGTM. Waiting for merge with main. |
…me-control # Conflicts: # README.md # agenticoding.test.ts # handoff/tool.ts
|
Merged latest |
…me-control # Conflicts: # agenticoding.test.ts # handoff/command.ts
|
Merged current Conflict resolution:
Verification after merge:
|
|
Update: restored mainline post-handoff auto- |
278063f to
941d055
Compare
59d3dcc to
699484c
Compare
699484c to
ff0ab7f
Compare
ff0ab7f to
5a3ca23
Compare
|
@ofriw Added the tool nudge tweak mentioned on Discord. It seems to be working stable. Please review when you can 🙏🏾 |
ofriw
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
|
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 AssessmentVerdict: REQUEST CHANGES Strengths
In Scope Issues
Out of Scope Issues
Technical AssessmentVerdict: REQUEST CHANGES Input Boundary Shape Risk AssessmentStatus: Triggered Strengths
In Scope Issues
Out of Scope Issues
Reusability
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 |
Summary
This PR adds
handoff.automaticEnabledto control automatic agent-initiated handoff while preserving explicit operator/handoff <direction>as the manual path.The implementation uses guard-based semantics: the
handofftool 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/handoffgenerated user turn. Successful handoff compaction continues to auto-send Pi's fixed post-compactionProceed.continuation;handoff.resumeBehavioris ignored and no continuation mode is configurable.Latest update — FB-033..FB-037
Closes the latest requested-change follow-ups from 2026-06-06:
session_before_compactcleanup 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/handoffrequests.handoff; queued warnings/status continue to say not to call the tool until the generated turn starts.handoff.0600mode before settings content is written.Previous requested-change closures
handoffparents, active manual generated-turn guidance override while automatic handoff is disabled, and disabled topic-boundary watchdog coverage.Proceed.changelog coverage, and save-time project override warning coverage./handoff <direction>compactiononErrorretry preservation, disabled-mode prompt fail-closed coverage includingnotebook_index, and bounded/deduplicated unchanged bad-source diagnostics.Requirements
handoff.automaticEnabled=falsesuppresses agent handoff-call guidance during normal turns.handofftool execution while automatic handoff is disabled is blocked unless an explicit/handoff <direction>request is active in its generated user turn./handoff <direction>requests are pending delivery only; they do not authorize the current turn to callhandoff./handoffuser message actually starts, using request identity rather than prompt equality./handoffremains valid across intermediate notebook/tool provider turns before the model finally callshandoff.handoff, including automatic-enabled watchdog branches./agenticoding-settingspersists the global raw JSON boolean, preserves unrelated settings and existing file mode, refuses malformed present globalhandoffparents without clobbering them, and warns when project settings override the saved global value.Acceptance criteria
handoff.automaticEnabledfrom global~/.pi/agent/settings.jsonplus project<cwd>/.pi/settings.json, with project override and own nested keys only.automaticEnabled: true.handoffparents, and non-ENOENT read failures fail closed to disabled with warning diagnostics; repeated unchanged bad-source warnings are deduplicated./handoff, or mention handoff timing in primer/watchdog/topic/status/notebook guidance, includingnotebook_indexand disabled topic-boundary watchdog nudges./handoffgenerated turns override disabled fallback prompt/topic guidance and tell the model to draft the handoff brief and call the temporarily enabled tool./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./handoff <direction>queues the generated prompt as a follow-up instead of waiting inside the command handler.Proceed.continuation.Contract changes
handoff.automaticEnabledfor automatic agent-initiated handoff availability./agenticoding-settingsas the dedicated extension-owned settings surface./handoff <direction>focused on manual handoff initiation, not configuration.Proceed.fixed and non-configurable.Out of scope
/handoff <direction>./handoff --enable,/handoff --disable,/handoff --wait,/handoff --proceed, or direct tool-call override parameters.turn_startcleanup changes.How to verify
Run from this branch/worktree after preparing
/tmp/pi-agenticoding-test-loader.mjsfromtest-loader.mjswith the local Pi package path:Latest local verification passed on 2026-06-06 at commit
f2fad77:git diff --checkLive verification from earlier PR iteration:
handoff.automaticEnabled=false,/handoff to discuss the importance of saying hellosuccessfully compacted after the model used notebook tools before callinghandoff.