fix(grok): preserve unknown usage for malformed billing tags - #3357
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af4494ccc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| while index < bytes.count { | ||
| let fieldStart = index | ||
| guard let key = Self.readVarint(bytes, index: &index), key != 0 else { | ||
| guard let key = Self.readVarint(bytes, index: &index), key >> 3 > 0, key >> 3 <= 536_870_911 else { |
There was a problem hiding this comment.
When a valid response contains an unknown length-delimited string or bytes field, the unconditional recursion at lines 501–507 applies this new field-number validation to its opaque contents. For example, an unknown bytes field containing 0x01 followed by eight zero bytes is valid protobuf wire data, but this guard interprets that payload as nested field zero, marks the entire scan incomplete, and prevents an otherwise validated active period from supplying its implicit 0% usage. Restrict completeness validation to paths known to contain submessages, or avoid propagating nested malformedness from opaque unknown fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c983271. The scanner now recurses only into the message paths declared by the official billing schema. Unknown length-delimited payloads are skipped entirely after checking their outer length; their bytes contribute neither completeness failures nor invented percentages/reset dates.
The six root/config opaque-payload regressions fail with 14 assertions before the repair and pass afterward. Tests also verify that malformed messages still block implicit zero at all 16 known message paths, and historical nested timestamps remain readable. The broader Grok/architecture run passes 191 tests; full-suite and exact-head CI verification are still running before merge.
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 1, 2026, 7:23 AM ET / 11:23 UTC. ClawSweeper reviewWhat this changesThe PR validates malformed protobuf tags and varints in Grok billing fallback responses while treating undeclared length-delimited fields as opaque bytes. Merge readinessKeep open for normal maintainer review: this owner-authored parser hardening is not on fetched main, the earlier opaque-byte concern is resolved, and captured exact-head macOS CI was still running. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads Grok billing gRPC-web fallback data when proxy usage is unavailable, then resolves it into the menu bar usage snapshot. The parser’s completeness result determines whether an omitted usage scalar may safely appear as 0% or must remain unavailable. flowchart LR
A[Grok billing response] --> B[Protobuf scanner]
B --> C[Validate tags and varints]
C --> D{Declared billing message?}
D -->|Yes| E[Decode nested fields]
D -->|No| F[Skip opaque bytes]
E --> G[Usage snapshot resolver]
F --> G
G --> H[Menu bar usage display]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge after the exact-head macOS test shards succeed, retaining schema-bounded recursion and the malformed/opaque-frame regression coverage. Do we have a high-confidence way to reproduce the issue? Yes: the added synthetic response fixtures exercise the parser-to-resolver path where malformed data could otherwise qualify for implicit 0% usage. Is this the best way to solve the issue? Yes: bounds validation plus descriptor-bounded recursive decoding is the narrowest repair that preserves valid unknown fields and existing implicit-zero semantics. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d637a5a5635a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
Landed as c3a25fd. The merged main tree exactly matches the tested head c983271, and the main checkout is clean after a fast-forward pull. The original malformed-tag cases reproduced eight failing assertions before repair. Review then exposed a real opaque-byte boundary issue: six root/config cases reproduced 14 failing assertions before schema-bounded recursion was added. Both sets now pass, along with valid boundary scalars, malformed messages at all 16 known nested paths, historical timestamps, and existing flat/captured billing fixtures. Verification on the final candidate:
Tests used the same local Swift toolchain via the native SwiftPM backend, with credential/session-file isolation and Keychain access suppressed. An earlier candidate had two synthetic RPC-fixture initialization timeouts; its unchanged rerun and the final candidate both pass without relaxed deadlines or assertions. No live-account probe or production occurrence is claimed. Request bytes and authentication are unchanged; #3336 remains independent. Changelog and provider documentation are updated. No release was published. |
1 participant
Summary
Prevent a Grok fallback response with invalid protobuf field numbers or overflowing varints from qualifying as a complete implicit-zero reading. This keeps an omitted percentage from incorrectly replacing unknown proxy usage with 0%. It follows up the completeness check in #3325 without changing valid zero-usage behavior, wire-published percentages, request bytes, authentication, or authoritative proxy plan/reset metadata.
The scanner previously checked only whether the encoded key was nonzero. Field zero with a fixed64 or empty length-delimited value passed that check and left the response marked complete. Oversized field numbers did the same; an overflowing varint could also truncate into a valid-looking key.
The repair follows the existing Antigravity reader's validation pattern: enforce the protobuf field-number range and reject overflow in the tenth varint byte. Review caught an important boundary: length-delimited fields can contain opaque bytes, not just nested protobuf messages. The scanner now descends only into the 16 message paths declared by Grok's billing descriptor and charger types, freshly checked September 1. Unknown byte fields are skipped entirely after validating their outer length, so they cannot invalidate a valid response or invent percentages/reset dates. Errors in known nested messages still propagate. Changelog and provider documentation are updated.
Verification
UInt64.maxscalar pass as positive controls.make checkpasses with zero violations across 2,082 Swift files and all 95 process-cleanup checks (one expected Linux-only skip on macOS).make testpasses on the repaired commit: 988 selections across 83 groups, all first-pass successful, zero failures/retries/timeouts (937.1 seconds).c9832713f4901ed551722665efbc6326b8723c5d: both macOS shards, all three Linux jobs, lint, the aggregate check, and GitGuardian are green.All verification is offline with synthetic responses, credential/session-file isolation, and Keychain access suppressed. No real-account probe or production occurrence is claimed. This is independent of the live-request proof still pending on #3336.