Skip to content

fix(cli): surface parked /resume plans as informational notices, not errors - #3506

Merged
likun666661 merged 7 commits into
apache:mainfrom
me2seeks:fix/tui-resume-disabled-flag-feedback
Aug 28, 2026
Merged

fix(cli): surface parked /resume plans as informational notices, not errors#3506
likun666661 merged 7 commits into
apache:mainfrom
me2seeks:fix/tui-resume-disabled-flag-feedback

Conversation

@me2seeks

@me2seeks me2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

/resume previously 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:

  • Runtime Host preserves three distinct wire reasons instead of collapsing them into continuation_unavailable:
    • resume_feature_disabled
    • continuation_authority_unavailable
    • safety_observation_unavailable
  • runtime-host-session-driver carries the exact TurnResumeParkReason in SafeBoundaryResumeParkedError.
  • pi-tui-runner renders 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.
  • Authority, safety-observation, and other recovery failures keep the raw reason and remain red errors.
  • The closed wire union change advances RUNTIME_HOST_COMPATIBILITY_EPOCH from 56 to 57 so mixed old/new peers fail during handshake instead of misclassifying a recovery failure.
  • The Chinese and English Runtime Resume architecture documents now record the reason projection, severity boundary, epoch change, and unchanged ownership.

Fixes #3505

Verification

  • Full repository npm run build: pass.
  • Focused Runtime Host protocol, continuation, and CLI TUI suites: 204/204 pass.
  • Protocol epoch guard against current main: 56 -> 57, pass.
  • Protocol coverage accepts the three distinct unavailable reasons and rejects removed continuation_unavailable.
  • Live behavior:
    • feature disabled: informational guidance for MAKA_RUNTIME_SAFE_BOUNDARY_RESUME=1;
    • missing candidate/session busy: informational notice;
    • authority, safety-observation, and other recovery failures: red error with raw reason.

AI use

  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Build, focused tests, formatting, and protocol epoch guard pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above

@Astro-Han Astro-Han 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.

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.

Comment thread packages/cli/src/pi-tui-runner.ts

@zhiiw zhiiw 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.

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 resume
  • session_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 Astro-Han 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.

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 Astro-Han 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.

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.

简体中文恢复类错误降级导致静默与误导。

@M4n5ter
M4n5ter force-pushed the fix/tui-resume-disabled-flag-feedback branch 2 times, most recently from 0b5b9f6 to 7a27c2d Compare August 26, 2026 10:03

@Astro-Han Astro-Han 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.

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 Astro-Han 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.

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.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the independent blind review by @Ox-Qronos at exact head 27038ddd56361882e85a21235f1e8d6aeabae75f (base 38f0a275, freshness rebased from 250dde69). I verified the drift and the exact-head CI myself; the file:line finding below is from Ox's sealed review.

What I checked myself:

  • Compared 250dde69 → 27038ddd and confirmed the only PR-owned change is 49f8338e5 docs (bilingual) plus merges from main; the functional patch is semantically line-for-line identical, with only context-line epoch numbers shifting 51→52 to 53→54.
  • Checked exact-head CI: test run 32994326664 SUCCESS, windows_recovery run 32994326712 SUCCESS, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.

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 removed continuation_unavailable and the three wire reasons plus epoch change, and (2) hosted checks green. Both are now met: 49f8338e5 updated both language docs with the three reasons (turn.ts:122-124 + pi-tui-runner.ts:294-318 three info cases + default error), body is updated, and hosted checks are green. Local re-check 58/58 runtime-host + 146/146 cli green.

P3 — Documented epoch number is staledocs/architecture/runtime-resume-architecture.md and the PR body cite epoch 52 (“51 → 52”), but the head carries RUNTIME_HOST_COMPATIBILITY_EPOCH = 54 at packages/runtime-host/src/protocol/index.ts:95 after rebasing over main (#3924/#3925). The semantic claim (closed-union epoch bump, mixed peers fail at handshake) is still correct — only the cited number needs updating to 54 to avoid confusion.

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, test/windows_recovery green, head 27038ddd is otherwise ready to be approved and merged once a write holder approves (epoch doc update can be fixed in a follow-up or before merge).


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.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

me2seeks and others added 7 commits August 28, 2026 09:02
…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.
@me2seeks
me2seeks force-pushed the fix/tui-resume-disabled-flag-feedback branch from 078abda to 7701d9b Compare August 28, 2026 01:32
@jackwener
jackwener requested a review from Astro-Han August 28, 2026 04:15

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

@me2seeks

Copy link
Copy Markdown
Contributor Author

Updated the PR description to record the actual compatibility transition as 56 -> 57. The Summary and Verification now match the implementation and bilingual architecture docs; the three new wire reasons were already listed. No code or head change was needed.

@likun666661
likun666661 merged commit e0aedcc into apache:main Aug 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): TUI /resume can never succeed — safe-boundary resume disabled by unset env flag

5 participants