Repository navigation
feat(cli): handle plan-gate denials centrally - #7441
Conversation
pamelachia
changed the title
withCommandTelemetry captures cli_upgrade_suggested once, before cli_command_executed, when a failure carries the entitlement_required envelope's entitlement. Commands with no per-command wiring (branches unpause) now fire it too. suggestUpgrade keeps the event only on its entitlements-lookup fallback. vanity-subdomains check-availability keeps its no-event exception through a new trackUpgradeSuggested option. Errors carrying an entitlement classify as plan_limit, whether or not their class declares the field.
The text layer's fail prints the upgrade hint before the error message whenever the error carries the entitlement_required envelope's entitlement, so commands with no per-command wiring (branches unpause) print it too. suggestUpgrade prints only on its entitlements-lookup fallback. Handler tests assert the failure carries the entitlement.
The persistent-branch 402 carries the entitlement_required envelope, so the central hint, event and plan_limit cover it. A 4xx without the envelope no longer triggers the follow-up project and entitlements requests.
979e26a to
82f5088
Compare
pamelachia
changed the title
|
Manual check from source against a Free-plan project (
|
82f5088 to
33a4c0e
Compare
CLI benchmark (Lightweight Commands): no changes foundCommit
API calls: unchanged for all 7 commands. Binary size: 134.9 MB, down 4.0 KB from 134.9 MB. Details: timing ranges, API calls per command, slowest phasesTiming ranges
API calls per command
Slowest phasesNo command moved beyond noise, so there are no phase deltas. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Verified both Claude findings against the checked-out code. Confirmed the SSO 404 upgrade-hint and telemetry regression, plus a defensive validation inconsistency limited to malformed entitlement objects. Codex's independent review completed with no findings.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/upgrade-suggest.ts:83 |
error-handling |
claude | SSO 404 responses carrying an entitlement_required envelope lose the upgrade hint and cli_upgrade_suggested event. suggestUpgrade now returns without emitting either, but the list/add/update/remove 404 handlers discard mapped.entitlement when constructing their specialized errors. upgradeSuggested still classifies these failures as plan_limit. |
| ⚪ NIT | apps/cli/src/shared/telemetry/error-actionability.ts:1152 |
correctness |
claude | The classifier accepts any non-null object-valued entitlement as a plan gate, while error normalization requires non-empty feature and upgrade_url strings. A malformed entitlement can therefore produce plan_limit classification without an upgrade hint or cli_upgrade_suggested event. |
Stats
Claude findings: 2 · Codex findings: 0 · Confirmed: 2 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Coly010
left a comment
Coly010
left a comment
There was a problem hiding this comment.
Approving, thanks! One optional ask: the SSO handlers build SsoListSamlDisabledError on a 404 without forwarding entitlement. If a 404 ever carried an entitlement_required envelope, suggestUpgrade would return true and nothing would print or fire. Unlikely, so a one-line forward or a test pinning it would do, but feel free to skip it.
The plan_limit reclassification also shifts those fingerprint suffixes, so worth a heads up to whoever watches the dashboards.
|
@Coly010 thanks for the review! Skipping the SSO forward: the Management API's plan gate never responds 404, so those 404 branches can't carry the envelope (details in #7441 (comment)). On the fingerprint shift: I checked what reads these fields. Two dashboards group by them and neither filters on |
I moved plan-gate handling out of each command into shared code. Since #7271,
mapHttpErrorattachesentitlementfrom the Management API'sentitlement_requiredbody, so the upgrade hint, thecli_upgrade_suggestedevent and theplan_limitclassification now all come from that one field, with no per-command wiring and no extra requests.Changed:
withCommandTelemetrycapturescli_upgrade_suggested(same name, same{feature_key, org_slug}payload) after the command, just beforecli_command_executed. Commands that never had wiring, such asbranches unpause, now get both.vanity-subdomains check-availabilitynow fires the event too; its no-event exception only existed for parity with the removed Go CLI.gateMapError/suggestUpgradewrapping atbranches create/update, the fourdomainscommands and the threevanity-subdomainscommands.branches updateno longer fetches the project and org entitlements after a 4xx, since the persistent-branch 402 carries the envelope. Only the SSO commands keep the entitlements lookup.plan_limitfrom the envelope: an error carryingentitlementclassifies asplan_limitwhatever its class declares, so gated denials frombranches unpause,domains get/create/activate/reverifyandvanity-subdomains getmove fromapi_statustoplan_limit(fingerprint suffix:api_statusbecomes:plan_limit).Note: the event moved from mid-handler to post-run. Payload and identity stay the same; it uses the same capture context as
cli_command_executed.Linear