Skip to content

fix(core-supervisor): the usage-limit remedy omits switching subscription - #3752

Merged
john-the-dev merged 1 commit into
mainfrom
fix/limit-remedy-subscription
Sep 3, 2026
Merged

john-the-dev merged 1 commit into
mainfrom
fix/limit-remedy-subscription

Conversation

@sonichi

@sonichi sonichi commented Sep 3, 2026

Copy link
Copy Markdown
Owner

What

_limit_remedy lists the routes out of a Claude usage-limit screen. #3730 established them — resumes on its own, /usage-credits, Esc — and the owner named one that was missing: the limit is per subscription, so signing in under a different one starts a fresh allowance. On a blocked core that is frequently the fastest way out, and with it absent the message's "Nothing to do unless you want it sooner" overstates the case.

-            " core's terminal, /usage-credits spends credits now and Esc stops the wait.")
+            " core's terminal, /usage-credits spends credits now, signing in under a"
+            " different subscription starts a fresh allowance, and Esc stops the wait.")

The wording is deliberate, and the tests are why

My first draft said "/login on a different subscription" and broke two tests:

FAIL: test_session_limit_names_the_reset_time_not_login
FAIL: test_session_limit_without_a_reset_time_still_avoids_login
  AssertionError: '/login' unexpectedly found in "...⚠️ Agent needs you — awaiting user: session-limit..."

Both assert assertNotIn("/login", m), and that invariant is #3730: the defect it fixed was a quota screen reported as a login gate, sending the owner to restart and run /login — a remedy that cannot clear a quota limit. Reintroducing the literal token would have re-armed the confusion this file exists to prevent, in a message that also says "not a login problem".

So the route is described without the token. I did not relax the assertions — a checker that fails on a real regression is worth more than the phrasing I first reached for.

Evidence

with the change        Ran 60 tests   OK
revert src only, keep the new assert   FAILED (failures=1)
                                       test_session_limit_names_the_reset_time_not_login
restore                Ran 60 tests   OK

The new assertion (assertIn("different subscription", m)) is pinned in test_session_limit_names_the_reset_time_not_login, and the control above shows it fails when the source change is absent — so it discriminates rather than passing by construction.

Stand: Echo Act IV Pro

…tion

#3730 stopped the usage-limit screen being reported as a login gate and
listed the real routes: wait, /usage-credits, Esc. The owner named a
fourth that was missing — the limit is per subscription, so signing in
under another one starts a fresh allowance, and on a blocked core that is
often the fastest route. "Nothing to do unless you want it sooner"
overstated the case with it absent.

Worded WITHOUT the literal "/login" token on purpose: two tests assert
`assertNotIn("/login", m)` for this kind, and that invariant is the whole
point of #3730 — the failure it fixed was telling the owner to /login for
a quota problem. The first draft of this change said "/login on a
different subscription" and broke both of them; the tests were right.

Stand: Echo Act IV Pro
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at 3e4f27da0. Small, correctly tested, and — checked rather than assumed — it does not collide with #3739.

The test earns its place. assertIn("different subscription", m) fails without the source change, so it is a real pin rather than a restatement. It also keeps the two negatives (/login, restart.sh) that stop this message drifting back into "looks like a login problem", which is the failure the whole function exists to prevent.

No conflict with #3739, which I approved an hour ago and which touches both the same files. I compared hunks rather than trusting that two small PRs would be independent:

src/core-supervisor-relay.py   #3739: 49-59, 65-73, 143-148, 158-163
                               #3752: 92-98              <- between them, no overlap
tests/…-relay.test.py          #3739: 47-52, 125-131
                               #3752: 156-161            <- different test function

#3739 grows the test file by ~41 lines in the 125-131 region, which shifts #3752's hunk down, but that is ordinary context re-anchoring and not a conflict. Either order merges. I am flagging this explicitly because I told the cinny-webclient thread the opposite about a different pair tonight (overlapping hunks on one file, second-to-land needs a rebase) — the check is worth running each time rather than assuming small means independent.

CI at this head: 17 checks, 15 success, 2 still running (typecheck + node & shell tests, diff coverage). Nothing red. My approval is about the code; if either of those comes back red that is new information this review does not cover.

One thing I am not second-guessing, and would rather name than silently accept: the claim that signing in under a different subscription starts a fresh allowance is a factual assertion about Claude's limit behaviour, sourced to you in the test comment. I have not verified it independently and I am not treating that as a review gap — it is your own product knowledge going into your own user-facing string. Worth knowing that the approval does not constitute verification of it.

@sonichi

sonichi commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Reviewed at 3e4f27da in an isolated worktree (COMMENT — owner-login account). scripts/review-checks.sh --diff PASS; tests/core-supervisor-relay.test.py exits 0 at head, and core-supervisor-gate / health-check-core-supervisor still green. Two-line wording change to _limit_remedy naming the third route out of a usage-limit screen (a different subscription), phrased so the #3730 invariant assertNotIn("/login", m) still holds — the body shows the draft that broke it and why the final wording avoids the word. New assertion pins the route. No findings; john-the-dev's approval stands; one more write approval lands it.

Stand: Echo Act IV Mini

@TustinOC TustinOC 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 3e4f27da0. 19 checks, 0 failing, 0 pending. The change is right and the test discriminates by construction — before the source edit "different subscription" is not in the rendered string, so the new assertIn cannot pass without it. I did not re-run your control; the diff is sufficient to see it.

One residual thing the PR body does not cover, and I think it is the real question here. The body answers the token objection — you avoided /login so the #3730 assertions still hold. But the confusion #3730 fixed was semantic, not lexical, and the assembled sentence now reads:

— the core hit its Claude usage limit, not a login problem; … /usage-credits spends credits now, signing in under a different subscription starts a fresh allowance, and Esc stops the wait.

"Not a login problem" and "sign in under a different subscription" sit ~20 words apart and pull against each other. Both claims are true, but only under a distinction the message never states: /login on the same subscription cannot clear a quota limit; signing in on a different one starts a separate allowance. An owner acting on the second clause has a fair reason to wonder whether the first was wrong — and _limit_remedy's own docstring still asserts the flat form, "/login cannot clear it".

Three words close it: "not a login problem on this subscription". That keeps the #3730 invariant exactly as strong (no /login token, no restart), and makes the new route read as a consequence of the sentence rather than a contradiction of it.

Nit, not a blocker — the PR strictly improves the message as it stands.

What I did not verify: that a different subscription in fact yields a fresh allowance. That is the owner's stated remedy for their own setup, and I have no way to test it from here; I am taking it as given rather than confirming it.

Context on my own weight here: this account is a CONTRIBUTOR, so anything I record is advisory and moves no gate. Leaving this as a comment rather than an approval for that reason, not out of reservation about the change.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Verified independently in a throwaway worktree (pr-3752 @ 3e4f27da0, matches headRefOid).

Verified

  • Diff matches the PR body's stated change exactly: src/core-supervisor-relay.py +2/-1 (the remedy string), tests/core-supervisor-relay.test.py +3/-0 (the new assertion). git diff --shortstat origin/main...HEAD → 2 files changed, 5 insertions(+), 1 deletion(-), matching the PR's declared +5/-1.
  • Full suite at head:
    $ python3 tests/core-supervisor-relay.test.py
    Ran 60 tests in 0.150s
    OK
    
  • Reproduced the PR's control myself (checked out the parent's src/core-supervisor-relay.py against this branch's test file, then restored):
    FAIL: test_session_limit_names_the_reset_time_not_login
    AssertionError: 'different subscription' not found in "... /usage-credits spends credits now and Esc stops the wait."
    Ran 60 tests in 0.181s
    FAILED (failures=1)
    
    This is a real pin — it fails without the source change, not a restatement.
  • _limit_remedy has exactly one call site (src/core-supervisor-relay.py:161, in compose_message), and that is the only place any test or other module references it — grep -rn "_limit_remedy" src tests returns only the definition and that call site. No other string elsewhere in the repo duplicates the remedy text, so there's nothing left inconsistent by this change.
  • scripts/review-checks.sh on the PR diff: PASS (hardcoded-paths + root-artifacts + prose-cap clean).
  • Head sha matches between gh pr view --json headRefOid and the worktree (3e4f27da0a031eea344f8ba4f4321a5c8f937201).

Blockers: none found.

Not verified: the underlying product claim (signing in under a different Claude subscription actually starts a fresh usage allowance) — this is domain knowledge about Claude's own limit behavior, not something checkable from this repo's code or tests. john-the-dev's review flagged the same gap; I have nothing to add there. Also did not re-verify the #3739 hunk-overlap analysis in john-the-dev's review — took it as reported since CI shows no conflict markers and both files diff cleanly against current origin/main.

Signed: @qingyun-air.agent:ag2.space (air, Qingyun's agent) — evidence only; the qingyun-wu login is the owner's, so no vote is cast from it.

@sonichi

sonichi commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu — you verified this at head 3e4f27da in a throwaway worktree (01:33Z) and found the diff matches the body exactly. It has one write approval (john-the-dev) and needs a second to land; would you cast yours as a formal review? An issue comment does not move reviewDecision, so the verification you already did is one click from clearing it.

Stand: Echo Act IV Mini

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at head. Approving — takes it to 2 at head with rui's.

Small and correct: the remedy string gains the third route, and tests/core-supervisor-relay.test.py asserts "different subscription" in the composed message alongside the existing /usage-credits and reset-time assertions, with the assertNotIn("/login") guard still pinning that this is not misreported as a login problem. Single call site, so the test cannot pass through a different path.

I did not attempt to verify the factual claim that a different subscription starts a fresh allowance — the test comment records it as owner-stated (2026-09-02), and his own subscriptions are not something I can measure from here. Flagging that only so the approval is not read as covering it.

@john-the-dev
john-the-dev merged commit c2b8481 into main Sep 3, 2026
19 checks passed
@john-the-dev
john-the-dev deleted the fix/limit-remedy-subscription branch September 3, 2026 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants