Skip to content

feat(desktop): add host-scoped pricing override editor - #5557

Open
liuxiaocs7 wants to merge 9 commits into
apache:mainfrom
liuxiaocs7:feat/pricing-overrides-only
Open

liuxiaocs7 wants to merge 9 commits into
apache:mainfrom
liuxiaocs7:feat/pricing-overrides-only

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

Summary

Settings → Usage → Pricing currently shows a disconnected read-only table. This replaces it with a Host-scoped editor that lists user overrides and offers a searchable built-in catalog picker or exact-key manual entry. Users can edit input, output, cache-read, and cache-write rates, reset a built-in override, or delete a custom-only price. Blank optional rates remain distinct from zero.

Writes use the snapshot the user reviewed. Host changes preserve raw drafts while requiring fresh review, and conflicts or lost responses reconcile without replaying mutations. The Pricing tab has its own refresh and hides time-range summaries.

Merged main at bacb1caed961ba5d685fc4c86bd3bc0d4ac23b40. This PR supersedes the implementation in #4164; the old PR remains open. Usage pagination and the wire schema are preserved.

When a user changes model A to B while A is saving, successful completion now refreshes B's revision and duplicate checks while retaining B's inputs. Six historical compatibility declarations match main again; the staged protocol guard checks merges against the incoming parent so it no longer asks contributors to rewrite historical declarations. Experimental runners and large result data were removed from the current tree; durable regressions and a concise historical ablation report remain.

CI also exposed clipped Usage timestamps in Linux system fonts. The activity time column now accommodates localized dates and AM/PM markers, with the existing geometry stories exercised in Simplified Chinese, Traditional Chinese, and English.

Refs #2015 #2218 #4164

Verification

Latest sync: 7d5dd7adc0cc95c814506164c1fd9f0732d2dfa7. Regenerated the conflicting Astryx inventory from the combined tree (334 files). The WorkHub dock test now matches the fix that landed concurrently on main.

  • Clean dependency install, workspace builds, all workspace typechecks, lint/format, strict renderer architecture, and inventory coverage: passed.
  • Pricing/Usage controller, view model, protocol, client, IPC, and Settings resource suites: 152 passed. The eight mid-save model-change cases still cover saved and recovered outcomes; all eight were independently shown to fail without the authority refresh.
  • WorkHub dock and enablement: 16 passed, rerun after taking main's fixture.
  • Protocol epoch guard: 18 passed; Astryx inventory contracts: 23 passed.
  • Fresh Storybook build and 32 Pricing/Usage Chromium scenarios: passed, covering Pricing at 1280px/480px, Usage at wide/narrow widths in all three locales, selected-Host draft restoration, validation, recovery, and keyboard submission.
  • Real Electron Pricing write → renderer reload → authoritative read → delete journey: passed.
  • CI for this commit: passed, including workspace and Runtime Host tests, Desktop E2E, complete Storybook smoke, transcript geometry, and installed CLI release validation.

The historical 33-variant ablation conclusions are in the report; those historical counts are separate from current validation. A live authenticated multi-Host session was not run.

Before (main) and after:

Before After
Read-only Pricing Editable Pricing

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code authored the initial Pricing editor; Codex rebuilt it on main, addressed review findings, and verified behavior. The commits include Generated-by: trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XXL Over 2500 readable lines label Sep 21, 2026
Replace the disconnected Pricing table with an overrides-only editor and searchable built-in catalog picker. Preserve reviewed CAS snapshots, drafts across Host changes, and explicit reconciliation without replaying uncertain writes.

Rebuild the Pricing changes from apache#4164 on current main, keeping Usage pagination and wire compatibility. Localize catalog validation semantics to the editor and include focused, browser, IPC and Electron coverage.

Generated-by: Claude Code
Generated-by: Codex
@liuxiaocs7
liuxiaocs7 force-pushed the feat/pricing-overrides-only branch from add5d0d to b5d83e2 Compare September 21, 2026 05:45
Preserve pricing drafts and Host fencing alongside Usage pagination and capacity feedback. Regenerate the surface inventory, align compatible protocol declarations with epoch 183, and normalize imported plugin license headers.

Generated-by: Codex

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

This adds a Host-scoped pricing override editor backed by a complete effective-pricing snapshot, CAS mutation, and reconnect reconciliation. I checked the selected-Host bridge and Main IPC path, input validation, controller fencing around Host changes and in-flight reloads, conflict/reset behavior, and the new regression cases. I found no substantiated P0–P3 issue in those paths. I also inspected the two before/after screenshots; the Pricing tab visibly removes the usage-range summary and adds the override editor entry point.

The current-head test check passed, but I did not independently run the full Desktop E2E suite or an authenticated multi-Host desktop session. This PR currently conflicts with main in docs/astryx-surface-file-inventory.md; resolve that conflict and re-run the checks before a human merge decision. No database schema migration is included.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Preserve Pricing services and smoke coverage alongside the new Plan and Storage composition. Update compatibility declarations to epoch 199 and the Pricing DOM fixture for Astryx 0.6.3.

Generated-by: Codex
Add regressions for same-Host disposal, refresh ordering, malformed pagination, reset reconciliation and native Host-review focus. Remove three locally redundant bookkeeping steps validated independently and together; archive the reproducible 33-variant experiment.

Generated-by: Codex

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

Reviewed at 17e0d3db. The pricing override editor looks solid: rate parsing rejects malformed/negative/underflow input and keeps an unset cache rate distinct from 0, main strictly validates renderer snapshots and mutations, stale-Host snapshots are rejected before sending, and the reconcile path only re-reads rather than resending writes. The protocol change is helper-only and correctly declared compatible at the current epoch (the epoch guard passes against main).

No P0–P2 found. Three P3s:

  1. Merge resolution rewrote unrelated compatibility declarations (P3). Besides the new pricing-reconciliation-helpers.json, six existing files under packages/runtime-host/protocol-compatible-changes/ change their epoch: recall-query-operation.json 169→183, session-model-limit-authority.json 175→183, executor-catalog-browser-safe.json and issue-4032-composition-identity.json 189→199 (reason text now says "through epoch 199"), mechanical-candidate-sweep.json and turn-snapshot-optional-fields.json 198→199. These appear to come from the merge commits and the epoch guard only checks newly added declarations, so CI won't flag it. Please restore those six files to main.
  2. False conflict after changing the model key mid-save (P3) — inline below.
  3. Experiment scaffolding in the tree (P3). scripts/experiments/pricing-ablation*.mjs, the *.probes.ts files, and docs/archive/pricing-ablation-2026-09-30.json (+2.7k lines, pinned to an earlier tree) aren't wired into CI. Worth confirming with maintainers whether they belong in the repo or in the PR description.

Not verified: tests were not run locally; whether every Host reconnect bumps the Settings lifecycle epoch (if not, the editor could keep showing pricing_snapshot_stale until refresh).

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

switch (outcome.kind) {
case 'saved':
setWriteState({ kind: 'idle' });
if (ownsDialog) finishReconciledIntent(attempt, outcome.snapshot);

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.

P3: Fields stay editable while a save is pending, and changing the model key bumps dialogGenerationRef (line 200). So when this save settles as saved/synchronized, ownsDialog is false and the open dialog's base snapshot stays at the pre-save revision. The next save then fails the revision check and shows a "changed elsewhere" conflict whose only other writer was the user; the duplicate-key check also reads that stale base.

Repro: Add → manual → key A → Save → change key to B while pending → Save. The existing test around pricing-editor.test.ts:696 covers only review_required. Suggest either locking the key field while saving, or rebasing the open dialog onto outcome.snapshot even when it no longer owns the attempt.

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

Reviewed exact head 17e0d3db35aa01ee3f66bcbc5d8c03ae37668dd1. The PR replaces the disconnected Usage pricing table with a Host-scoped override editor. The UI keeps a reviewed snapshot as its write base, preserves drafts across Host changes, and fences older reloads (apps/desktop/src/renderer/features/usage/controller/pricing-controller.ts:127-179,211-259,497-530). Main IPC validates the renderer-supplied snapshot and mutation (apps/desktop/src/main/runtime-host-pricing-ipc-main.ts:82-124); the client checks Host identity and uses revision-CAS plus read-only reconciliation rather than replaying an uncertain write (apps/desktop/src/main/runtime-host-client.ts:635-709). This head also adds focused regression and ablation material.

I found no additional substantiated P0-P3 issue in the reviewed paths. The three P3 observations already published in the current-head review by Astro-Han remain for the author/maintainers; I am not duplicating those inline comments. In particular, the unrelated protocol-compatible-change declaration edits and experiment files are still present in this head. This is not a merge approval.

Node 24 clean npm ci and build:test passed; focused Pricing/Usage Desktop/Host tests passed (115/115). This head's hosted test passed. Diff check and static merge against current main ed38ccbb were clean; the branch is 2 commits behind main. I inspected the checked-in Pricing screenshot, but did not run a fresh native Electron write journey or a real multi-Host interactive session. Those behaviors remain outside my local verification.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Merge main at 2463d86, preserve historical protocol declarations, and keep the staged epoch guard aligned with incoming-parent compatibility checks. Refresh a changed pricing draft from successful writes without discarding its input; cover normal and recovered outcomes. Trim archived experiment scaffolding and register the Pricing service at the composition root.

Generated-by: Codex
Reserve room for full dates and day-period markers in native system fonts. Exercise the existing wide and narrow Usage geometry assertions in all three UI locales. The old 168px column clips localized timestamps; all 38 focused Pricing/Usage browser scenarios pass with the wider column.

Generated-by: Codex

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

I found no new P0–P3 issues in this update at e4ca6516744834ae64fe870afe64e98af84c9034.

The three concerns from 17e0d3db35aa01ee3f66bcbc5d8c03ae37668dd1 are addressed:

  • The six historical protocol-compatible declarations now match the current base. The new pricing declaration only adds pure reconciliation helpers; the protocol epoch guard accepts it without changing wire payloads.
  • A successful or reconciled save now advances the authority of a still-open draft whose model key changed during the write, without replacing its inputs or replaying the previous mutation. All eight new regression cases pass; removing that authority update makes all eight fail, and restoring it makes them pass again.
  • The standalone experiment runners and large archived JSON are removed. The retained short archive explicitly identifies its historical commit and does not present its counts as validation of this head.

I independently passed 172 focused Pricing/Host/IPC tests, 18 protocol-guard tests, the seven-package build, all four Desktop type checks, the production Desktop build, 32 Chromium Pricing/Usage scenarios, and the real Electron pricing save → renderer reload → readback → delete test. The browser scenarios include the selected-Host draft restoration and localized request timestamps.

The head's CI is green, but the PR currently has a merge conflict in docs/astryx-surface-file-inventory.md against the latest main. These results apply to this exact head, not a future conflict resolution. I did not test an authenticated remote Host or live provider billing.

Merge main at 031bca5 and regenerate the Astryx inventory from the combined tree. Preserve Pricing services alongside the new project restoration boundary. Update the incoming WorkHub dock test to use its current enablement provider instead of the retired enabled prop.

Generated-by: Codex
Merge bacb1ca and keep the already-landed WorkHub enablement fixture from main. Pricing behavior and the regenerated inventory remain unchanged.

Generated-by: Codex

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

This is a limited incremental check of e4ca6516 → 7d5dd7adc0cc95c814506164c1fd9f0732d2dfa7, not a new full review of the Pricing editor. I found no new P0–P2 issue in the increment, but the branch is not merge-ready against current main.

The increment consists of two merges from main. I compared each merge with its automatic merge result to separate imported main changes from manual resolutions. The first resolution regenerates the surface inventory and adapts the incoming WorkHub dock test to the enablement provider; the second takes main's already-fixed dock fixture. The Pricing controller/editor, IPC/client paths, pricing protocol helpers and regression tests are unchanged from the previous reviewed head. The settings-story composition retains both Pricing services and main's new TaskEntry provider.

A fresh trial merge with main 105302c63f998a2ce7c90f2cdebe0a594a083477 reproduces two content conflicts:

  • apps/desktop/src/renderer/features/usage/ui/usage-settings-view.tsx: the localized timestamp column is 224px here versus main's 176px. Resolve this with the localized wide/narrow Usage geometry checks, rather than mechanically discarding either protection.
  • docs/astryx-surface-file-inventory.md: the generated totals differ. Regenerate the inventory from the resolved combined tree; do not just choose one parent count.

There is also a P3 merge-preparation requirement in packages/runtime-host/protocol-compatible-changes/pricing-reconciliation-helpers.json. The current branch declares epoch 212 and passes its original-base protocol check. The candidate merged tree uses main's epoch 217, so the current guard rejects that unchanged declaration. After resolving/rebasing, recheck its compatibility rationale and pin it to the resulting epoch. This is not a new wire-incompatibility finding or a failure of the current head's CI.

CI run 38049322804 succeeded on this exact head, but does not validate a future conflict resolution. I did not rerun builds, tests, native Electron or the previous Pricing browser journeys for this narrow increment. The resolved exact head needs fresh checks, including the inventory, protocol guard and localized timestamp layout.

Preserve localized request timestamp spacing, regenerate the Astryx surface inventory, and re-pin the pricing reconciliation compatibility declaration to epoch 222.

Generated-by: Codex

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

I found no new P0–P2 issue in 7d5dd7ad → e2f2d7f231f2ccbc58459e532a873b87a63c556d. The previous merge-conflict and protocol-declaration concerns are resolved. This is an incremental check, not a new full review of everything imported from main.

The update is one merge of main 1751c399. The three non-merge feature commits are unchanged in range-diff. Comparing the actual merge with Git's reconstructed merge result identifies only the timestamp-column resolution, regenerated inventory and compatibility-declaration update. Pricing production code, its regression tests and its editor stories are unchanged from the previous head; the additional settings providers retain the Pricing services.

  • apps/desktop/src/renderer/features/usage/ui/usage-settings-view.tsx:611 retains the 224px timestamp column. All six English/Simplified Chinese/Traditional Chinese scenarios pass at 1280px and 720px. Measured text plus padding needs about 185px in Traditional Chinese and 197px in English, so the conflicting parent's 176px allocation would not fit those strings.
  • docs/astryx-surface-file-inventory.md:9 matches a fresh generation from the combined tree: 342 files, not either parent's stale total. Both committed inventory artifacts pass the coverage check.
  • packages/runtime-host/protocol-compatible-changes/pricing-reconciliation-helpers.json:2 now declares epoch 222. The protocol guard accepts it against current main. The added helpers still change no codec, operation payload or wire shape, so they do not require another epoch bump; historical declarations remain untouched.

The earlier mid-save model-change fix at apps/desktop/src/renderer/features/usage/controller/pricing-controller.ts:461 is preserved: all eight regression cases pass, and removing only that authority-refresh block at module load makes all eight fail. I also passed 184 focused Core/Host/Desktop tests, 18 protocol-guard tests, 23 inventory tests, 168 architecture tests plus the current-base architecture check, the E2E budget check, a clean workspace build, all four Desktop type checks, the production renderer build, and 32 Pricing/Usage Chromium scenarios. These browser runs include selected-Host draft restoration, validation/recovery, keyboard submission and the localized timestamp checks.

P3 documentation follow-up: the PR description's Verification section still names 7d5dd7ad, 334 files and the old CI run. Please update that evidence for this head. CI run 38751077483 is still queued, not green; GitHub reports the branch as mergeable but blocked. Latest fetched main 1751c399 is already included in this head. I did not rerun native Electron, an authenticated remote Host or live provider billing, and these results do not establish those boundaries or a future merge result.

This branch has not been deployed

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

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants