fix(cli): surface parked /resume plans as informational notices, not errors - #3506
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis is right and the repro in the description is the useful kind. continuation_unavailable on a stock install really is the opt-in flag being off, and rendering that as a red error does read like session corruption. Carrying the protocol reason on a dedicated error class rather than re-parsing a message string is also the right shape: the driver keeps the fact, the surface decides the presentation.
Reviewed at exact head 07fe40a101a79e12abe6046f7bd227f678c3d6be against base 4acfa26934ce4b2b385b76f8a11048bb83fab861. One P1, inline. No checks have run on this head yet.
The P1 is that the fix does not do what the description says it does. The description commits to genuine failures (safety_check_failed, source_run_unreadable, …) keep the raw detail and stay red for diagnosis, but the catch block treats every SafeBoundaryResumeParkedError the same way. Five of the eight park reasons end up as informational notices. Details inline.
Everything else looks clean: the two new tests do fail without the change, the driver's two throw sites are both converted, and pointing the copy at MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1 in the second commit is a real improvement over just saying the feature is off.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
There was a problem hiding this comment.
Additional review pass at exact head 07fe40a101a79e12abe6046f7bd227f678c3d6be (MERGEABLE; test green). Confirming it here with the mechanism spelled out.
The existing [P1] is correct. SafeBoundaryResumeParkedError carries the protocol reason, but the catch in runMakaPiTui flattens every reason into a level: 'info' notice. The park-reason union (TURN_RESUME_PARK_REASONS, packages/runtime-host/src/protocol/turn.ts:96-106) has eight members; only three are genuinely informational:
continuation_unavailable— feature flag off (this PR's target case)resume_candidate_missing— nothing to resumesession_busy— a turn is already active
The other five are failure signals the description itself promised would stay red: safety_check_failed, source_run_unreadable, continuation_repair_required, continuation_started_indeterminate, and continuation_already_exists all currently render as a quiet info line. A safety-check failure reading like "feature is off" mutes exactly the signal a user (or a bug report) needs. The new tests pin only the two informational reasons, so nothing fails when a failure reason is softened — which is why the gate didn't catch it.
The fix is small: switch on error.reason — the three informational reasons get safeBoundaryResumeParkedCopy, the rest rethrow (or render as error with the raw reason preserved).
Executed evidence (real Windows x64, this head): the two new tests pass (/resume parked by the host is informational, Nothing to resume…); the full pi-tui-runner suite is 119/121 here — the 2 failures are the SIGTERM terminal-restoration tests, which fail identically on base 4acfa269 on this machine (pre-existing Windows-only flake, unrelated to this diff).
简体中文
既有 P1 独立确认:catch 把 8 种 park 原因全部降级成 info,但其中只有 3 种(continuation_unavailable / resume_candidate_missing / session_busy)是信息性的;safety_check_failed 等 5 种失败信号被一并静音,与 PR 描述"真失败保持红色"的承诺相悖。新增测试只钉了信息性原因,所以门禁没拦住。本机实测:两个新测试通过;套件里 2 个 SIGTERM 测试失败在 base 上同样复现(既有 Windows flake,与本 PR 无关)。
Astro-Han
left a comment
There was a problem hiding this comment.
I found a blocking issue in the parked-resume handling.
[P2] Parked error severity lost and continuation_unavailable copy is inaccurate
pi-tui-runner.ts:1986 maps every SafeBoundaryResumeParkedError reason to level: 'info', including genuine failures (source_run_unreadable, safety_check_failed, etc.) that should remain red errors with attention — contrary to the stated "genuine failures stay red".
Additionally, root-turn-coordinator.ts:2605-2610 collapses resume_feature_disabled, continuation_authority_unavailable, and safety_observation_unavailable into continuation_unavailable, but the UI copy unconditionally says "not enabled, set env=1" — false when the feature is enabled but authority/observation failed.
Fix: make classification exhaustive; only benign reasons as info, preserve others as errors, and distinguish continuation_unavailable causes before claiming the feature flag is off.
Head 07fe40a10. Checks test: success.
简体中文
该 parked 分类丢失严重性且误导文案。
Astro-Han
left a comment
There was a problem hiding this comment.
Update on 07fe40a101:
[P2] Resume error misclassification silences real failures
pi-tui-runner.ts:1986-1993 demotes 8 SafeBoundaryResumeParkedError kinds to info, including source_run_unreadable, safety_check_failed, continuation_repair_required, etc. Meanwhile root-turn-coordinator.ts:2605-2610 merges feature-disabled/authority-unavailable into continuation_unavailable and gives misguided flag hint. Real recovery failures will be silent/misrouted.
Fix: only resume_candidate_missing/session_busy/true feature-disabled use info; distinguish unavailable reasons; add tests per park reason.
Checks on 07fe40a101a79e12abe6046f7bd227f678c3d6be are test: PASS — code is NO-GO.
简体中文
恢复类错误降级导致静默与误导。0b5b9f6 to
7a27c2d
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found a blocking issue.
[P2] Parked error aggregation misguides users
root-turn-coordinator.ts:2810-2815 merges resume_feature_disabled, continuation_authority_unavailable, safety_observation_unavailable into continuation_unavailable, but pi-tui-runner.ts:293-296 always shows info "not enabled, set MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1" — users with authority/safety failures get wrong guidance and infrastructure faults never surface as error.
Fix: keep distinguishable causes; only true feature-disabled goes to info.
Prior P1 (parked reasons downgraded to info) is fixed and covered with regression test; hosted test/windows_recovery: SUCCESS.
简体中文
错误原因合并导致指引错误。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking code issues, with a conditional feature gate.
Code: ACCEPTABLE — no P0-P3. Prior continuation_unavailable aggregation P2 is closed: projectTurnResumePlan now keeps resume_feature_disabled / continuation_authority_unavailable / safety_observation_unavailable distinct, and TUI only treats feature-disabled etc. as info while other failures stay error.
Feature/Global-design: GO, CONDITIONAL. No new state/lifecycle/second authority, no split needed. Merge requires updating PR body and Chinese architecture doc to reflect removed continuation_unavailable, three new wire reasons, and epoch/Host/protocol changes.
Hosted windows_recovery: SUCCESS, test: PENDING — still not mergeable until checks green and docs updated.
简体中文
代码无阻断,设计条件通过,需补文档并等检查绿。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
This is a synthesis of the independent blind review by @Ox-Qronos at exact head What I checked myself:
Findings from Ox's freshness review: Overall — GO — the two prior merge conditions are now closed. The previous review required (1) PR body + bilingual docs to record P3 — Documented epoch number is stale — What I did not re-do: a full first-pass derivation — this was a freshness rebind plus condition-closure check, relying on the still-valid prior derivation; no Docker cells are involved. Gate: one P3 docs staleness remains; no P0–P2,
|
jackwener
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 078abdadb8c21414bad15a9e898866bf9f5d5cb3.
The two earlier correctness problems are closed. safeBoundaryResumeParkedCopy() now treats only the three benign outcomes as informational and leaves safety, recovery, and persistence failures at error severity. The protocol also separates resume_feature_disabled from authority and safety-observation failures, so the environment-variable guidance is now shown only when the feature is actually disabled. I found no remaining P0–P2 issue in this implementation slice; 69 focused CLI and Runtime Host tests passed locally.
I cannot approve this head because it is not mergeable into current main. The branch independently assigns Runtime Host compatibility epoch 54, while main is already at epoch 55, and a current-main synthetic merge conflicts in packages/runtime-host/src/protocol/index.ts. Please rebase, preserve the existing epoch ledger, assign this protocol change the next fresh epoch (56 with the current ledger), and update the protocol comments, tests, bilingual architecture documentation, and the PR body, which still describes the older epoch transition. This head also has no hosted check run or approval bound to it, so the new exact head will need fresh checks and review.
One non-blocking test gap remains: the TUI tests inject typed reasons directly rather than forcing an authority-read or safety-observation failure through RootTurnCoordinator and asserting the projected reason.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
…errors The TUI /resume command always failed with the raw protocol reason (e.g. 'Safe-boundary resume parked: continuation_unavailable') because the host's safeBoundaryResumeEnabled flag defaults to unset, so the resume plan is parked with 'resume_feature_disabled' on every stock install. Even with the feature enabled, a completed turn parks with 'resume_candidate_missing'. Both cases are informational — there is simply nothing safe to resume — but they rendered as red errors that read like session corruption. - runtime-host-session-driver: throw SafeBoundaryResumeParkedError carrying the protocol park reason instead of a plain Error - pi-tui-runner: catch it in /resume and print plain-language copy (feature disabled / nothing to resume / session busy) as an info notice; other reasons keep the raw detail for diagnosis - tests: pin the informational rendering for continuation_unavailable and resume_candidate_missing Refs apache#3505 Generated-by: Maka Agent (claude-opus-4-8)
The continuation_unavailable notice now names MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1 so a user who has not enabled safe-boundary resume learns how to turn it on instead of only learning that it is off. Generated-by: Maka Agent (claude-opus-4-8)
Keep feature-disabled, continuation-authority, and safety-observation outcomes distinct on the wire so only the true opt-in case receives informational CLI guidance. Generated-by: Codex
Document the current Host-to-CLI wire reasons, presentation severity, compatibility epoch, and unchanged ownership boundaries in both architecture counterparts. Generated-by: Codex
Both language versions of the resume architecture note said epoch 52, but the wire change bumped RUNTIME_HOST_COMPATIBILITY_EPOCH to 54; epoch 52 is the unrelated steering-echo entry.
078abda to
7701d9b
Compare
jackwener
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 7701d9bbd4bb60820d79cf98cd3dc9d1be50cee6. The earlier code and compatibility blockers are closed, but the PR description still records an obsolete protocol epoch.
[P2] Update the PR description from epoch 52 to the actual 56 → 57 transition
The implementation and both architecture documents correctly use compatibility epoch 57 on top of current main epoch 56. The PR description still says that this change advances the epoch to 52 and that verification covered 51 -> 52.
Please update the Summary and Verification sections to state 56 -> 57 and name the three new wire reasons: resume_feature_disabled, continuation_authority_unavailable, and safety_observation_unavailable. This matters because the PR description is the permanent compatibility record contributors will use when resolving later epoch conflicts.
The previous severity bug is fixed: only resume_feature_disabled, resume_candidate_missing, and session_busy produce informational notices, while authority, safety-observation, and other recovery failures remain errors. The former collapsed continuation_unavailable reason is removed from production code, and the protocol decoder rejects it. The current-main merge is clean, and exact-head test and windows_recovery checks succeeded.
One non-blocking test gap remains: the tests inject typed authority and safety-observation reasons at the CLI boundary instead of forcing those failures through RootTurnCoordinator and asserting the projected wire reason. A focused Host-level regression would protect that mapping, including the currently untested session_busy informational path.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
|
Updated the PR description to record the actual compatibility transition as |
Summary
/resumepreviously rendered every parked safe-boundary plan as a red error with a raw protocol reason. That was misleading for expected user states such as a disabled opt-in feature or a session with no interrupted run.Safe-boundary resume remains intentionally gated by
MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1. This PR does not change that gate or move continuation ownership. It fixes the Host-to-CLI reason boundary and the TUI presentation:continuation_unavailable:resume_feature_disabledcontinuation_authority_unavailablesafety_observation_unavailableruntime-host-session-drivercarries the exactTurnResumeParkReasoninSafeBoundaryResumeParkedError.pi-tui-runnerrenders only expected user states as informational notices:resume_feature_disabled→ explain the opt-in flag.resume_candidate_missing→ explain that no interrupted run exists.session_busy→ explain that a turn is already active.RUNTIME_HOST_COMPATIBILITY_EPOCHfrom 56 to 57 so mixed old/new peers fail during handshake instead of misclassifying a recovery failure.Fixes #3505
Verification
npm run build: pass.56 -> 57, pass.continuation_unavailable.MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1;AI use
Tool(s) and scope: Maka Agent and Codex — investigated the root cause, reproduced the behavior, implemented the Host/protocol/CLI fix and tests, updated architecture documentation, and prepared this description under human direction.
Checklist
Does this PR entail a change in behavior?