fix(reset): keep unknown credit counts distinct from zero in the confirmation - #134
Conversation
Preserve omitted WHAM applicability counts for account display and disclosure. Derive missing available counts from eligible credits, render unknown values consistently in command and TUI confirmations, and update coverage for omitted fields while removing the duplicate precondition test.
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/tests/commands.test.ts">
<violation number="1" location="packages/opencode/src/tests/commands.test.ts:2462">
P3: The `not.toContain('does not currently count')` assertion in this test is inert: `''` args render the account-list view (renderResetAccountList), and that disclosure string exists only in renderResetConfirm, so the assertion passes regardless of the implementation. Drop it or move it to a select/confirm-view test to actually guard the disclosure behavior.</violation>
</file>
<file name="packages/opencode/src/tui/command-dialogs.tsx">
<violation number="1" location="packages/opencode/src/tui/command-dialogs.tsx:145">
P3: When availableCount is undefined, the main account list renders 'credits unavailable', but the dialog account option renders '?/?'. These two surfaces describe the same unknown state differently, contradicting the PR's stated consistency goal. Use the same wording in resetAccountOptions (e.g. fall back to 'credits unavailable' when availableCount is undefined).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }), | ||
| ) | ||
| expect(payload.text).toContain('?/1 applicable/available') | ||
| expect(payload.text).not.toContain('does not currently count') |
There was a problem hiding this comment.
P3: The not.toContain('does not currently count') assertion in this test is inert: '' args render the account-list view (renderResetAccountList), and that disclosure string exists only in renderResetConfirm, so the assertion passes regardless of the implementation. Drop it or move it to a select/confirm-view test to actually guard the disclosure behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/commands.test.ts, line 2462:
<comment>The `not.toContain('does not currently count')` assertion in this test is inert: `''` args render the account-list view (renderResetAccountList), and that disclosure string exists only in renderResetConfirm, so the assertion passes regardless of the implementation. Drop it or move it to a select/confirm-view test to actually guard the disclosure behavior.</comment>
<file context>
@@ -2415,6 +2415,93 @@ describe('commands', () => {
+ }),
+ )
+ expect(payload.text).toContain('?/1 applicable/available')
+ expect(payload.text).not.toContain('does not currently count')
+ })
+
</file context>
| ? 'quota unavailable' | ||
| : `${account.usedPercent}%` | ||
| const counts = `${account.applicableAvailableCount ?? 0}/${account.availableCount ?? 0}` | ||
| const counts = `${account.applicableAvailableCount === undefined ? '?' : account.applicableAvailableCount}/${account.availableCount ?? '?'}` |
There was a problem hiding this comment.
P3: When availableCount is undefined, the main account list renders 'credits unavailable', but the dialog account option renders '?/?'. These two surfaces describe the same unknown state differently, contradicting the PR's stated consistency goal. Use the same wording in resetAccountOptions (e.g. fall back to 'credits unavailable' when availableCount is undefined).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tui/command-dialogs.tsx, line 145:
<comment>When availableCount is undefined, the main account list renders 'credits unavailable', but the dialog account option renders '?/?'. These two surfaces describe the same unknown state differently, contradicting the PR's stated consistency goal. Use the same wording in resetAccountOptions (e.g. fall back to 'credits unavailable' when availableCount is undefined).</comment>
<file context>
@@ -142,7 +142,7 @@ function resetAccountOptions(payload: OpenDialogPayload): ResetDialogOption[] {
? 'quota unavailable'
: `${account.usedPercent}%`
- const counts = `${account.applicableAvailableCount ?? 0}/${account.availableCount ?? 0}`
+ const counts = `${account.applicableAvailableCount === undefined ? '?' : account.applicableAvailableCount}/${account.availableCount ?? '?'}`
const status = account.eligible
? 'eligible'
</file context>
| const counts = `${account.applicableAvailableCount === undefined ? '?' : account.applicableAvailableCount}/${account.availableCount ?? '?'}` | |
| const counts = | |
| account.availableCount === undefined | |
| ? 'credits unavailable' | |
| : `${account.applicableAvailableCount === undefined ? '?' : account.applicableAvailableCount}/${account.availableCount}` |
|
Merged as
On the dropped test — you and cubic are right, and my #133 check was incomplete in an interesting way. I verified it was a rewrite rather than a deletion, and it was. What I did not check is whether the rewrite left it saying anything its neighbour did not. On Once the parameter was removed there was no longer any way for that test to express "the server reports no applicable credits" — the input that carried the condition was gone. So it kept a name describing a property its body could not test. That is worse than an ordinary duplicate: it reads like the pin for #131 while pinning nothing, and it would have absorbed exactly the attention a future reader should have spent looking for the real one. I confirmed the real pin holds before agreeing to the drop. Restoring the refusal reddens Generalising, since it will recur: after a signature change, a test's name and its body can drift apart silently. The name is not evidence. Checking that a rewritten test still fails for its stated reason is a different check from confirming it was rewritten rather than deleted, and I did the second one only. Gate on Worth noting the timing trap you hit — findings at 13:55, merge at 14:01, fix pushed to the closed branch at 14:24. I merged inside the review window. I will check for in-flight findings before merging rather than treating an approving read as final. |
Follow-up to #133. Cubic's three findings on that PR came in at 13:55 and the merge landed at 14:01; this commit was pushed to the branch at 14:24, so it sat on a closed PR. Same commit, new branch, one commit on top of
main.The findings were valid, all three:
Omitted
applicable_available_countwas treated as an explicit 0. The disclosure condition used=== 0, which is right, butcommands.ts:1240coerced the field with?? 0before it reached the row, so an absent field would have shown the "server does not count this credit as applicable" line falsely.undefinednow survives through to the row; the disclosure fires only on an explicit 0; the preview line renders?/N applicable/availablewhen the field is absent.Spend 1 of 0when both count fields are absent.availableCountnow falls back to the number of credits the eligibility filter accepts, and rendersunknownif that is also 0. The filter is factored intoisResetCreditEligible/countEligibleResetCreditssoselectCreditToSpendand the count share one definition. The TUIDialogConfirmreads the same row value, so both surfaces agree.Duplicate test.
accepts exhausted quota when the server reports no applicable creditsinreset-credits.test.tsbecame identical to the test above it once the parameter was removed, so I dropped it. You checked that rename on #133; it was a rewrite at the time, and cubic is right that after the signature change it no longer asserted anything its sibling did not. The inverted property is still pinned at the commands level byexhausted preview remains eligible when the server reports no applicable credits, which is the test that fails if the refusal comes back.Two new tests, each with its mutation confirmed applied (one-line diff) before the run:
?? 0on the applicable countomitted applicable count stays unknown in the account previewomitted credit counts fall back to the eligible credit totalbun run test: 1123 pass / 0 fail / 45 files. Typecheck and Biome clean. Order-dependence scan clean oncommands.test.ts,reset-credits.test.ts,command-dialogs.test.ts.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the reset flow treating missing credit counts as zero, which falsely showed the "server does not count this credit as applicable" line and
Spend 1 of 0. Missing counts now stay distinct from zero: the preview renders?/N applicable/availableand the confirmation rendersSpend 1 of unknown, falling back to the eligible credit total when available.selectCreditToSpendand the count fallback viaisResetCreditEligibleandcountEligibleResetCredits.DialogConfirmreads the same row value so both surfaces stay in sync.Written for commit 225c437. Summary will update on new commits.