Skip to content

feat(v0.9.2): durable update and receipt-safe output presentation - #4859

Merged
Hmbown merged 5 commits into
mainfrom
codex/platform-release-complete
Jul 26, 2026
Merged

feat(v0.9.2): durable update and receipt-safe output presentation#4859
Hmbown merged 5 commits into
mainfrom
codex/platform-release-complete

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • makes stable update prompts durable/actionable while preserving release guardrails
  • records Hotbar Alt-1..8 automated and truthful manual-terminal QA boundaries
  • resolves Feature request: Output filter hook for thinking/response blocks #4468 with a receipt-preserving presentation/export boundary
  • gives OpenHarmony a pinned real-SDK tier-2 check and gates unsupported sandbox/clipboard surfaces
  • diagnoses stale resumed Windows workspaces before shell spawn with the exact path and recovery action

Product decisions

  • canonical output and signed thinking are never rewritten by an arbitrary hook; show_thinking and stream-json are derived presentation surfaces
  • physical terminal/device passes stay explicitly UNRUN until performed
  • OpenHarmony CI fails closed if the real SDK/sysroot is unavailable; no stub success
  • foreground exec_shell is not described as ConPTY; invalid inherited and explicit cwd values are distinguished

Testing

  • cargo fmt --all -- --check
  • git diff --check
  • focused Rust tests are delegated to GitHub CI because concurrent v0.9.2 lanes saturated this macOS host linker; do not treat this as local PASS

Closes #2970
Closes #3961
Closes #4100
Closes #4468
Refs #3758

Credit: OHOS lineage @shenyongqing and @shenjackyuanjie; Windows report @redjade75723; #4468 proposal and measurements @eugenicum.

The startup release check previously surfaced a newer-version result only as a
short-lived status toast, which is easy to miss during startup, onboarding, or a
busy transcript. Users could keep running stale binaries even when a verified
release was available.

Thread a small `UpdateNotice` (current + latest) through the version-hint chain
instead of a bare String, and at the startup drain point emit both the existing
transient toast and a durable `HistoryCell::System` notice. The persistent notice
shows current and latest versions, the release-notes link, and the exact
`codewhale update` / `codewhale update --check` commands plus restart guidance,
mirroring the existing toast+System pattern in `surface_prompt_override_notices`.

All existing release-JSON guardrails are untouched: no notice for disabled
updates, current/newer versions, or draft/prerelease/incomplete-asset releases.

Adds `update_notice_block_is_persistent_and_actionable` covering the notice
formatting and the current-version no-op, and updates the existing guardrail
tests for the new return type.

Closes #3961
Copilot AI review requested due to automatic review settings July 26, 2026 06:17
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 56s —— View job


Review complete

Summary

This PR makes the update prompt durable (a new HistoryCell::System transcript entry alongside the existing toast), refactors the version-check return type from String to a new UpdateNotice{current, latest} struct, tightens Hotbar Alt-1..8 test coverage, and adds an RFC formally rejecting pre-persistence output mutation for #4468. Overall the design is sound: UpdateNotice cleanly separates the transient toast from the durable block, the RFC's reasoning about receipt/replay fidelity is well-argued, and the Hotbar test expansion (looping 1..=8 across a modifier matrix, including AltGr) meaningfully increases coverage over the four hardcoded assertions it replaced.

Findings

1. Correctness — mirror/release-notes URL is not mirror-aware (crates/tui/src/tui/ui.rs:16292, inline comment posted)
UpdateNotice::notice_block() hardcodes https://github.com/Hmbown/CodeWhale/releases/tag/v{latest}. version_hint_from_latest_tag (which builds this notice) is the shared path for the direct GitHub check and the mirror paths — version_hint_from_release_mirror_env (CNB mirror via CODEWHALE_USE_CNB_MIRROR/RELEASE_BASE_URL_ENV, see crates/release/src/lib.rs) and version_hint_from_configured_update_uri. A user on a mirror (plausibly because GitHub isn't reachable for them) will now get a durable transcript notice pointing at github.com. The old toast text never carried a URL, so this is a new regression, and it cuts directly against this PR's own stated goal of "preserving all release-asset, mirror, ... guardrails."

2. Documentation accuracy — stale test-name reference (docs/evidence/hotbar-qa-matrix.md:65-66, inline comment posted)
The QA matrix cites hotbar_slot_from_key_accepts_only_alt_one_through_eight as regression evidence twice, but no such test exists; the actual (and actually-modified-by-this-PR) test is hotbar_alt_digit_requires_plain_alt_one_through_eight. Given the doc's explicit purpose is to avoid "fabricating manual passes," a broken automated-evidence citation is worth fixing.

Notes

  • No provider/model routing code is touched by this PR, so the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 provider-prefix invariant doesn't apply here.
  • HistoryCell::System { content } and App::add_message are existing, correctly-used APIs (same pattern as shell_job_routing.rs/mcp_routing.rs); the version_check.take() guarantee means the notice fires at most once per session, so no duplicate-notice risk.
  • Could not run cargo check/cargo test in this sandbox (command execution required approval that wasn't grantable in this context) — logic was verified by reading call sites and the codewhale-release crate directly; CI is the authoritative build/test signal per the PR description.

Comment thread crates/tui/src/tui/ui.rs
fn notice_block(&self) -> String {
format!(
"Update available: v{current} -> v{latest}\n\
Release notes: https://github.com/Hmbown/CodeWhale/releases/tag/v{latest}\n\

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.

Bug: the "Release notes" link is hardcoded to github.com/Hmbown/CodeWhale, but version_hint_from_latest_tag is the shared path for both the direct GitHub check and the mirror paths (version_hint_from_release_mirror_env → CNB mirror via CODEWHALE_USE_CNB_MIRROR/RELEASE_BASE_URL_ENV, and version_hint_from_configured_update_uri). For a user running CodeWhale specifically because they're on a mirror (e.g. GitHub is unreachable for them), the new durable transcript notice will now advertise a github.com URL — which directly contradicts this PR's stated goal of "preserving all release-asset, mirror, ... guardrails" (the old toast never included a URL, so this is a new regression, not pre-existing behavior).

Consider omitting the release-notes line (or making it conditional/generic) when the notice originates from a mirror/custom-URI source, since UpdateNotice currently has no way to know which source produced it.

Fix this →

| --- | --- | --- |
| `Alt-1` through `Alt-8` | Dispatch the corresponding slot | `hotbar_alt_digit_fires_from_composer_and_sidebar_states` |
| Bare `1` through `8` | Insert/retain ordinary composer input | `hotbar_bare_digit_inserts_text_even_when_composer_empty` |
| `Ctrl-number`, `Super/Cmd-number`, `Alt-0`, `Alt-9`, and F-keys | Never dispatch a Hotbar slot; `F1` remains help | `hotbar_slot_from_key_accepts_only_alt_one_through_eight` and global keybinding tests |

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.

Minor: hotbar_slot_from_key_accepts_only_alt_one_through_eight (cited here and on line 66) doesn't exist in the codebase — I could only find hotbar_alt_digit_requires_plain_alt_one_through_eight in crates/tui/src/tui/ui/tests.rs, which is the test actually modified by this PR to cover this row. Since this doc's whole point is to be truthful regression evidence (not fabricated passes), a broken test-name reference undercuts that goal — worth fixing the name.

Fix this →

Expand Hotbar event regression coverage across all eight slots and reserved modifier/key shapes, and record a truthful terminal QA matrix without fabricating device passes.

Resolve #4468 with a receipt-preserving presentation/export boundary: canonical model output and signed thinking remain immutable, while show_thinking and stream-json provide safe accessible derived views.

Credit: #4468 proposal and measurements by @eugenicum.

Refs #3758

Closes #4468
@Hmbown
Hmbown force-pushed the codex/platform-release-complete branch from 95e0d05 to 6942042 Compare July 26, 2026 06:22
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 14s —— View job


Review complete

  • Gather context (PR diff, changed files, CLAUDE.md)
  • Check whether prior review findings (mirror URL, stale test name) were addressed
  • Review crates/tui/src/tui/ui.rs diff
  • Review crates/tui/src/tui/ui/tests.rs diff
  • Review docs changes (ACCESSIBILITY.md, hotbar-qa-matrix.md, RFC 4468)
  • Post final review summary

Summary

This is a re-review of the current PR head (6942042); the diff and commit count are unchanged since the prior pass. UpdateNotice{current, latest} cleanly separates the transient toast from the new durable transcript block via HistoryCell::System, the Hotbar Alt-1..8 test now loops across a real modifier matrix (including AltGr Ctrl+Alt+N) instead of four hardcoded cases, and the new update_notice_block_is_persistent_and_actionable test directly asserts the #3961 acceptance criteria (current/latest versions, release-notes link, exact command, restart guidance, and no-notice-when-current). The RFC (docs/rfcs/4468-output-presentation-filters.md) is well-reasoned on receipt/replay fidelity and correctly scopes show_thinking/stream-json as presentation-only.

Findings (both previously reported, still unresolved in this diff)

1. Correctness — mirror/release-notes URL is not mirror-aware (crates/tui/src/tui/ui.rs:16292)
UpdateNotice::notice_block() still hardcodes https://github.com/Hmbown/CodeWhale/releases/tag/v{latest}. version_hint_from_latest_tag is the shared constructor for the direct GitHub check and both mirror paths (version_hint_from_release_mirror_env via CODEWHALE_USE_CNB_MIRROR/RELEASE_BASE_URL_ENV, and version_hint_from_configured_update_uri). A user on a mirror — plausibly because GitHub isn't reachable for them — now gets a durable transcript notice advertising a github.com URL. The old toast never carried a URL, so this is a new regression introduced by making the notice durable, and it cuts against the PR's own stated goal of "preserving all release-asset, mirror, ... guardrails."
UpdateNotice currently has no field to distinguish its source; consider adding one (or omitting/genericizing the release-notes line for mirror/custom-URI-sourced notices) so the link is only shown when it's actually correct.

2. Documentation accuracy — stale test-name reference (docs/evidence/hotbar-qa-matrix.md:65-66)
Both rows still cite hotbar_slot_from_key_accepts_only_alt_one_through_eight as regression evidence. That test doesn't exist; the test actually covering this row (and the one modified by this PR) is hotbar_alt_digit_requires_plain_alt_one_through_eight in crates/tui/src/tui/ui/tests.rs. Since this doc's stated purpose is truthful, non-fabricated regression evidence, a broken citation undercuts its own goal.

Notes

  • No provider/model routing code is touched, so the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 provider-prefix invariant doesn't apply.
  • HistoryCell::System { content } / App::add_message are existing, correctly-used APIs; version_check.take() still guarantees the notice fires at most once per session, so no duplicate-notice risk.
  • Could not run cargo check/cargo test in this sandbox (command execution requires approval not grantable here); reviewed by reading call sites, the codewhale-release crate, and the test diff directly. CI remains the authoritative build/test signal per the PR description.

Compile-check codewhale-tui with a pinned real OpenHarmony SDK instead of a stub, explicitly gate Linux sandbox surfaces, and document clipboard degradation and distribution boundaries.

Contributor lineage: @shenyongqing and @shenjackyuanjie.

Closes #2970
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 56s —— View job


Review complete

  • Gather context (PR diff since prior review, new commit 1df323d)
  • Check whether prior findings (mirror URL, stale test name) were addressed
  • Review new OpenHarmony CI/doc additions (ohos.yml, sandbox/mod.rs, clipboard.rs, HarmonyOS.md)
  • Post final review summary

Summary

Since the last pass, the head commit changed from 6942042 to 1df323d, which adds a new OpenHarmony tier-2 CI job (.github/workflows/ohos.yml) plus supporting doc-comments (crates/tui/src/sandbox/mod.rs, crates/tui/src/tui/clipboard.rs, docs/HarmonyOS.md) and a one-line ACCESSIBILITY.md clarification. The #3961/#4468 core diff (durable UpdateNotice, Hotbar tests, RFC) is unchanged from the prior review.

Findings

1. New — CI check silently drops -Dwarnings for the OpenHarmony job (.github/workflows/ohos.yml:38,70-71, inline comment posted)
The job sets RUSTFLAGS: -Dwarnings at job scope, but scripts/ohos-env.sh (sourced right before cargo check) exports CARGO_ENCODED_RUSTFLAGS for the target linker flags. Cargo treats these as mutually-exclusive rustflags sources — CARGO_ENCODED_RUSTFLAGS wins outright, it does not merge with RUSTFLAGS. So this "real tier-2 check" (per the commit message) currently allows warnings through silently, which cuts against this same PR's stated ethos in docs/HarmonyOS.md of never substituting something weaker and reporting it as full success.

2. Still unresolved from prior review — mirror/release-notes URL is not mirror-aware (crates/tui/src/tui/ui.rs:16292)
UpdateNotice::notice_block() still hardcodes https://github.com/Hmbown/CodeWhale/releases/tag/v{latest}, even though version_hint_from_latest_tag is the shared constructor for the direct GitHub check and both mirror paths (version_hint_from_release_mirror_env, version_hint_from_configured_update_uri). A mirror user now gets a durable transcript notice pointing at github.com — a regression versus the old toast, which never carried a URL.

3. Still unresolved from prior review — stale test-name citation (docs/evidence/hotbar-qa-matrix.md:65-66)
Both rows still cite hotbar_slot_from_key_accepts_only_alt_one_through_eight, which doesn't exist. The actual test is hotbar_alt_digit_requires_plain_alt_one_through_eight in crates/tui/src/tui/ui/tests.rs.

Notes on the new OpenHarmony additions

  • The workflow pins the third-party SDK-setup action to a full commit SHA and hard-fails if clang/sysroot are missing rather than substituting host headers — good, matches the "no false success" principle it documents.
  • sandbox/mod.rs and clipboard.rs changes are doc-comments only, accurately describing existing cfg(target_env = "ohos") gating; no logic changed.
  • No provider/model routing code is touched anywhere in this diff, so the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 provider-prefix invariant doesn't apply.
  • Could not run cargo check/cargo test in this sandbox (command execution requires approval not grantable here); reviewed by reading the diff, scripts/ohos-env.sh, and cargo's documented rustflags precedence rules directly. CI is the authoritative build/test signal per the PR description.
    · Branch: codex/platform-release-complete

@Hmbown Hmbown added the v0.9.2 Targeting v0.9.2 label Jul 26, 2026
set -euo pipefail
test -x "${OHOS_NATIVE_SDK}/llvm/bin/clang"
test -d "${OHOS_NATIVE_SDK}/sysroot"
. ./scripts/ohos-env.sh

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.

Bug: RUSTFLAGS: -Dwarnings is set job-wide (line 38), but scripts/ohos-env.sh (sourced here) exports CARGO_ENCODED_RUSTFLAGS for the target-specific linker flags. Cargo's rustflags sources are mutually exclusive with CARGO_ENCODED_RUSTFLAGS taking priority over RUSTFLAGS — it does not merge them. So for this job's cargo check (line 71), -Dwarnings is silently dropped and warnings won't fail the build, even though the job is meant to be "a real tier-2 check."

This directly undercuts the PR's own stated principle for this doc/CI addition (docs/HarmonyOS.md: "fails if the SDK, Clang, or sysroot is unavailable rather than substituting host headers ... that could report false success") — the check can currently pass with warnings present.

Fix: append -Dwarnings into the CARGO_ENCODED_RUSTFLAGS value built in scripts/ohos-env.sh (joined with the \037 separator already used there) instead of relying on the job-level RUSTFLAGS env var.

[Fix this →](https://claude.ai/code?q=In%20scripts%2Fohos-env.sh%2C%20the%20script%20exports%20CARGO_ENCODED_RUSTFLAGS%20for%20the%20aarch64-unknown-linux-ohos%20target%2C%20which%20takes%20priority%20over%20and%20silently%20discards%20the%20job-level%20RUSTFLAGS%3D-Dwarnings%20set%20in%20.github%2Fworkflows%2Fohos.yml.%20This%20means%20the%20OpenHarmony%20CI%20check%20does%20not%20actually%20deny%20warnings.%20Fix%20by%20appending%20-Dwarnings%20(joined%20with%20the%20existing%20%5Cx1f%20separator)%20into%20the%20CARGO_ENCODED_RUSTFLAGS%20value%20constructed%20in%20ohos-env.sh%2C%20so%20warn-as-error%20is%20preserved%20for%20the%20OpenHarmony%20cross-check.&repo=Hmbown/CodeWhale

Validate both inherited session workspaces and explicit cwd values before sandbox preparation or process launch, preserving the offending path and giving an actionable resume/fork or cwd recovery.

This follows the post-#4491 diagnosis: foreground exec_shell does not allocate a ConPTY, and a deleted saved workspace is the session-scoped input that survives terminal restarts.

Credit: original Windows report by @redjade75723.

Closes #4100
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (new commit since last review, current diff state)
  • Check whether prior findings (mirror URL, stale test name, rustflags) were addressed
  • Review any new changes in this sync
  • Post final review summary

· Branch: codex/platform-release-complete

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 45s —— View job


Review complete

  • Gather context (new commits since last review: c0a02fd fix(shell), b91a94d rustfmt)
  • Check whether prior findings (mirror URL, stale test name, rustflags) were addressed
  • Review new fix(shell) commit (crates/tui/src/tools/shell.rs, shell/tests.rs)
  • Post final review summary

Summary

Since the last pass, two commits were added: c0a02fd (fix(shell): diagnose stale resumed workspaces before spawn) and b91a94d (rustfmt-only reformatting of a line in crates/tui/src/tui/ui/tests.rs, no logic change). The #3961/#4468/OpenHarmony core diff is unchanged from the prior review.

The new validate_shell_working_dir helper is a solid, well-scoped fix: it stats the resolved work_dir before either shell-spawn path (execute_with_options_env_for_owner_and_work and execute_interactive_with_policy_env) and returns a clear, actionable error — distinguishing "saved session workspace" (implicit, from default_workspace) vs. "requested working directory" (explicit working_dir arg) — instead of letting spawn fail with an opaque OS error. The two new tests (deleted_saved_workspace_reports_path_and_recovery_before_spawn, explicit_missing_working_dir_is_not_misreported_as_session_corruption) directly cover both branches and the message-content distinction between them. No correctness issues found in this addition.

Findings (all three previously reported, still unresolved)

1. Correctness — mirror/release-notes URL is not mirror-aware (crates/tui/src/tui/ui.rs:16292)
UpdateNotice::notice_block() still hardcodes https://github.com/Hmbown/CodeWhale/releases/tag/v{latest}, even though version_hint_from_latest_tag (line 16300) is the shared constructor for the direct GitHub check and both mirror paths (version_hint_from_release_mirror_env, version_hint_from_configured_update_uri). A mirror user gets a durable transcript notice pointing at github.com — a regression versus the old toast, which never carried a URL, and this cuts against the PR's own stated goal of "preserving all release-asset, mirror, ... guardrails."

2. Documentation accuracy — stale test-name citation (docs/evidence/hotbar-qa-matrix.md:65-66)
Both rows still cite hotbar_slot_from_key_accepts_only_alt_one_through_eight, which doesn't exist. The actual test is hotbar_alt_digit_requires_plain_alt_one_through_eight in crates/tui/src/tui/ui/tests.rs.

3. CI — -Dwarnings silently dropped for the OpenHarmony job (.github/workflows/ohos.yml:38,70-71)
RUSTFLAGS: -Dwarnings is set job-wide, but scripts/ohos-env.sh:57 (sourced right before cargo check) exports CARGO_ENCODED_RUSTFLAGS, which takes priority over RUSTFLAGS (mutually exclusive, not merged). Warnings won't fail this "real tier-2 check" build. Fix: append -Dwarnings into the CARGO_ENCODED_RUSTFLAGS construction in ohos-env.sh using its existing separator.

Notes

  • No provider/model routing code is touched anywhere in this diff, so the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 provider-prefix invariant doesn't apply.
  • Could not run cargo check/cargo test in this sandbox (command execution requires approval not grantable here); reviewed by reading the diff and call sites directly. CI is the authoritative build/test signal per the PR description.
    · Branch: codex/platform-release-complete

@Hmbown
Hmbown merged commit 7312311 into main Jul 26, 2026
23 checks passed
@Hmbown
Hmbown deleted the codex/platform-release-complete branch July 26, 2026 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.9.2 Targeting v0.9.2

Projects

None yet

2 participants