Skip to content

[CTX-1340] fix(ui): charge retained strings including click-arg keys (#2227) - #2229

Merged
Xuepoo merged 1 commit into
mainfrom
ctx-1340/fix-ui-retained-strings
Oct 18, 2026
Merged

Xuepoo merged 1 commit into
mainfrom
ctx-1340/fix-ui-retained-strings

Conversation

@Xuepoo

@Xuepoo Xuepoo commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Priority: P2 | Area: area:security | Labels: fix,P2,area:security | Milestone: v0.1.0 | RFC: isolation/resource RC limits | Task: CTX-1340

Closes #2227

What

The plugin UI budget counted only Text leaf bytes, leaving command identifiers, ClickCommand.args keys, string values, and tokens bounded only by the 320 KiB envelope. This charges every retained string against the existing 2 MiB aggregate.

Changes

  • bitty-lua/src/ui.rs: UiNode::retained_string_bytes, ClickCommand::retained_string_bytes, argument-key cap (UI_CLICK_MAX_ARG_KEY_BYTES = UI_CLICK_MAX_COMMAND_BYTES, no new numeric ceiling), validation, tests.
  • plugin_runtime/services.rs: mount/update/remove charge retained bytes; rejected update leaves the previous block and aggregate intact; diagnostic names the dimension.
  • tests/plugin_ui.rs: bridge-level key-cap test.

Contracts unchanged

2 MiB aggregate, 320 KiB marshal envelope, E_UI_BLOCK_BUDGET code/class, SCN-3/SCN-5. Only the aggregate diagnostic text differs. Error-vocabulary stability remains a cross-repo item (bitty-plugin-sdk#154).

Verification

  • fmt, clippy -D warnings (bitty-lua/bitty-runtime, all targets), Windows GNU all-targets: pass
  • cargo nextest run -p bitty-lua -p bitty-runtime --locked: 2,787 passed, 1 skipped
  • Exactly 2 MiB accepted; one byte over rejected; 2,098,175-byte key-only payload rejected on mount and update with the seed block intact
  • Mutation: key.len() * 0 makes the large-key test fail; reverted and re-run green
  • Rebased on e1f6920d

Charge Text bytes, theme tokens, command identifiers, argument keys, and string argument values against the existing 2 MiB SCN-4 aggregate, and cap argument keys at the existing 64-byte command-identifier bound. The 320 KiB envelope, the 2 MiB number, and the E_UI_BLOCK_BUDGET code/class are unchanged.
@Xuepoo Xuepoo added this to the v0.1.0 milestone Oct 18, 2026
@Xuepoo Xuepoo added fix Bug fix P2 Priority: medium area:security Area: security corpus / audit labels Oct 18, 2026
@coderabbitai

coderabbitai Bot commented Oct 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI (base), Organization UI (inherited)
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6d0906de-45df-4f88-a9db-1d4fa46c0d55
📥 Commits

Reviewing files that changed from the base of the PR and between e1f6920 and 14f47f3.

📒 Files selected for processing: 3
  • crates/bitty-lua/src/ui.rs
  • crates/bitty-runtime/src/plugin_runtime/services.rs
  • crates/bitty-runtime/tests/plugin_ui.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Xuepoo
Xuepoo merged commit 0fea511 into main Oct 18, 2026
17 checks passed
@Xuepoo
Xuepoo deleted the ctx-1340/fix-ui-retained-strings branch October 18, 2026 20:23

@Xuepoo Xuepoo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Independent review verdict (separate agent, guard cron hermes-planner).

Verdict: APPROVE.

Checks performed:

  • git show 14f47f3e --stat + full diff; gh pr diff 2229
  • Confirmed UiNode::retained_string_bytes() recursion is bounded by UI_MAX_NODES; per-string by existing caps; saturating_add/sub throughout; no new numeric ceiling (UI_CLICK_MAX_ARG_KEY_BYTES = UI_CLICK_MAX_COMMAND_BYTES).
  • Field rename aggregated_text_bytes -> aggregated_retained_bytes consistent across clear/mount/update/drop; update atomic (subtract-then-add, compare before mutate).
  • Tests present: unit test in ui.rs (identifiers charged, text_bytes() stays text-only), service tests (aggregated_retained_budget_counts_every_string, key-only >2 MiB fails closed, rejected update/mount leave registry intact), bridge integration test for 64/65-byte key boundary.

Only a non-blocking doc nit. CI green on head 14f47f3. Merge gate satisfied (comment-LGTM + green CI).

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

Labels

area:security Area: security corpus / audit fix Bug fix P2 Priority: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ui): charge click-command keys and other retained strings against the 2 MiB UI aggregate

1 participant