Repository navigation
Conversation
BREAKING CHANGE: remove automatic internal generate fallback for accounts without Provider API access. Preserve sessions and credentials; provide explicit entitlement migration guidance.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Memory benchmarkCompared base
Interpretation: No metric shows a clear Base-to-PR difference beyond its measured run-to-run variation. Extension overhead above pi baseline
Values are medians of 6 alternating, paired runs. The value after Environment: pi
|
patlux
left a comment
patlux
left a comment
There was a problem hiding this comment.
Direction is right given #5, and the transport/test changes are clean (one request, no fallback, same message for emitted and rejected errors, abort classification). Three things I'd want resolved before this ships as 0.8.0:
- The generate client is still in the package (see
index.tscomment). - README is written as a proposal, but README is what npm renders for the released version (see README comments).
- The "documented Provider API only" principle stops at inference; quota still uses
/alpha/*(see README comment).
Also: pre-1.0 breaking → 0.8.0, not a patch. Please note the intended version in the PR so the release PR doesn't ship it as 0.7.9.
| import { pickCommandCodeApiKey, withResolvedCommandCodeApiKey } from "./src/converters.ts" | ||
| import { createStreamCommandCode } from "./src/core.ts" | ||
| import { calculateCommandCodeCost, commandCodeCostRatesAt } from "./src/cost.ts" | ||
| import { commandCodeCostRatesAt } from "./src/cost.ts" |
There was a problem hiding this comment.
The generate client stays in the shipped package: src/core.ts (836 lines), src/transcript.ts, the converter paths and their tests remain, and files: ["src/"] publishes them. For a breaking change whose rationale is "do not touch the internal endpoint", shipping an unreachable /alpha/generate client is contradictory, and it leaves ~1.3k lines that are tested but never run in production.
This is already a feat!; remove it here (plus tests/test-core*, converter/transcript tests, calculateCommandCodeCost if it becomes unused), or open the follow-up and make it a blocker for the 0.8.0 release.
| const requestApiKey = options?.apiKey | ||
| if (transport === "generate") return deps.streamGenerate(model, context, options) | ||
|
|
||
| transport = "provider" |
There was a problem hiding this comment.
transport = "provider" is now set before the request, so /commandcode-status shows transport: provider for a Go account that was just denied — exactly the users who will read that status after hitting the new error. Either set it only after the first non-denied event (as before), or drop the transport state entirely now that it has one real value.
|
|
||
| > **Disclaimer:** This is an unofficial, community-maintained integration. It is not affiliated with, endorsed by, or supported by Command Code. You need your own Command Code account, API key, and a plan with Provider API access. Command Code's terms, availability, and pricing apply. | ||
|
|
||
| ## Breaking migration: Provider API access required |
There was a problem hiding this comment.
README is what npm renders for the released version. "The next breaking release removes…" will be wrong the moment this is released, and the section sits above Install. Write it as the state of the code ("This extension requires Provider API access. 403 upgrade_required stops with …") and keep the unreleased/proposal status in CHANGELOG and the PR.
| 2. After changing accounts, run `/login` again and reload the host. Existing sessions, credentials and model selections are not deleted or migrated by this change. | ||
| 3. Retry a small request and check `/commandcode-status`. There is no switch to re-enable internal generation. Do not use alternate endpoint overrides to bypass plan restrictions. | ||
|
|
||
| GOAT/Provider accounts with API entitlement continue to use the same documented inference endpoints. Quota reporting is unchanged. This is a breaking-change proposal, not a claim that the version on npm already includes it. |
There was a problem hiding this comment.
Two things in this paragraph:
- "This is a breaking-change proposal, not a claim that the version on npm already includes it." — same as above, this sentence must not land on
main. - "Quota reporting is unchanged":
/commandcode-quotaand the usage provider still call/alpha/whoami,/alpha/billing/*,/alpha/usage/summary— the same undocumented surface Use Command Code Provider API Instead #5 complains about. The PR's principle ("documented Provider API only") is not applied there. If that is deliberate (read-only, no plan bypass), say so explicitly; if not, it belongs in the same breaking release rather than a second one.
|
|
||
| ## Unreleased | ||
|
|
||
| ### Breaking changes |
There was a problem hiding this comment.
Pre-1.0 breaking change → minor bump (0.8.0). Consider naming the target version here or in the PR so the release PR cannot accidentally ship it as a patch. (#163's RELEASE.md text says the same.)
pierreraby
left a comment
pierreraby
left a comment
There was a problem hiding this comment.
Approved — on the implementation and, deliberately, on the product call. Verified locally at the exact head in an isolated worktree (npm ci): tsc --noEmit PASS, test-transport 17/17, full test:unit exit 0 (400 checks), test-pi-local + test-omp-compat PASS (incl. exactly-one-denied-provider-request / zero-generate-retries on both hosts), Prettier + git diff --check PASS. I also had this head independently reviewed in a fresh context (APPROVE): all seven checks verified — no path from the registered provider to /alpha/generate, 403 upgrade_required detection preserved for both emitted error events and rejected iterators, unrelated errors/429s/malformed bodies/aborts untouched, no denial caching across key changes, legacy fixtures unreachable, migration docs complete. The docs URL in the error (https://commandcode.ai/docs/provider) resolves (HTTP 200).
On the product decision: the ToS basis is solid (founder request in #5 with ban warning, official docs excluding the Go plan), and I agree with the hard cut over a bypass flag — a grace switch would prolong exactly the usage Command Code asked to stop. Two asks for the release: (1) say explicitly in the release notes that this supersedes #154, which 0.7.8 had just hardened — the whiplash deserves one honest sentence; (2) consider the four polish notes from review, chiefly wrapping the router's raw errorMessage in the shared redactor (small step back vs #155) and softening "then sign in again" to retry-first unless accounts changed.
Breaking change — separate review/release decision
Remove automatic
/alpha/generatefallback when the documented Provider API responds with403 upgrade_required. The official provider docs exclude Go API use; an internal CLI endpoint must not silently bypass that restriction. Related policy concern: #5.Verification
npm testwith Pi 1.1.0 and OMP 18.11.1 required: PASS.Based on current main including #154; this intentionally supersedes fallback behavior rather than duplicating its robustness fix. Keep this separate from a routine patch release: no version bump, tag, merge or npm publication is performed. Routing/diagnostics PRs #158/#159 remain separate and will need their small transport-adjacent overlap reconciled at integration.