fix(auth): resolve seat via /v1/auth/status instead of JWT roles - #88
Conversation
The CLI gated 'berget code init' on hasBergetCodeSeat(JWT) — a Keycloak realm-role check. That broke Pro/Summit subscribers twice over: the config deliberately omits keycloakRole for the newer tiers (berget.seat- only), so those users never got ANY seat role, and roles drift anyway (a Summit subscriber can carry a stale berget_code_seat role — verified live). The API already authorizes inference against berget.seat (Odoo), not roles. configureAuth now asks GET /v1/auth/status (same canonical source the API authorizes against) and routes on tier: - seat → 'You have a <Tier> subscription' (tier-aware message) - no seat → API-key path (unchanged) - status unverifiable (network/5xx) → warn + sync OAuth anyway (same behavior as the old undecodable-JWT path) hasBergetCodeSeat and its role list are removed — roles are no longer read anywhere in the CLI. New SeatStatusPort keeps the ports/adapters pattern; production impl never throws (null on any failure). No backend changes needed: /v1/auth/status already exposes seatId/tier resolved from berget.seat for every tier.
![berget-ai[bot]](https://avatars.githubusercontent.com/u/160869282?s=60&v=4){kind=small}
| export function createSeatStatusService(): SeatStatusPort { | ||
| return { | ||
| async fetchSeatStatus(accessToken: string) { | ||
| const base = process.env.BERGET_API_URL || 'https://api.berget.ai'; |
There was a problem hiding this comment.
🟠 warning — Duplicates getAuthConfig() API-URL resolution and diverges in --local mode (localhost:3000 vs prod); reuse getAuthConfig().apiBaseUrl.
| async fetchSeatStatus(accessToken: string) { | ||
| const base = process.env.BERGET_API_URL || 'https://api.berget.ai'; | ||
| try { | ||
| const res = await fetch(`${base}/v1/auth/status`, { |
There was a problem hiding this comment.
🟠 warning — fetch has no timeout/AbortSignal — a hung request blocks the interactive init flow indefinitely; use AbortSignal.timeout(10_000).
| return handleUnverifiedAuth(prompter, files, homeDir, tool, cliAuth); | ||
| } | ||
|
|
||
| if (seatStatus.tier) { |
There was a problem hiding this comment.
🟠 warning — Gating on tier only: {seatId: 168, tier: null} sends a real seat-holder to "You do not have a Berget subscription"; check seatId too.
| * Never throws: any failure (network, non-OK, bad payload) returns null so | ||
| * the caller can fall back to a warn-and-continue path. | ||
| */ | ||
| export function createSeatStatusService(): SeatStatusPort { |
There was a problem hiding this comment.
🟡 nit — Implementation of a commands/code port lives in src/auth (inverted auth→commands type dependency); existing adapters live in commands/code/adapters or src/services.
ReviewSummaryThis PR replaces offline JWT-role-based seat detection ( RiskMEDIUM — The switch from offline to an online seat check is the right call for eliminating role drift, but the PR as written does not typecheck ( Issues
Suggestions
ArchitectureGood port/adapter decomposition (net-offline auth module, injectable seat-status), though the placement of the real adapter in CodeSense score: 7.5/10 — clean structure and well-faked tests, but it ships a signature/type mismatch that breaks the build and re-implements existing config resolution. Inline findingsBerget AI (berget/zai-org/GLM-5.3-Flash) | PR #88 |
- configureAuth Pick now includes seatStatusService (was TS2339 build
break, hidden by vitest's type-stripping)
- reuse getAuthConfig().apiBaseUrl instead of duplicating env resolution
(diverged in --local mode: localhost:3000 vs prod)
- AbortSignal.timeout(10s) so a hung connection can't block init
- seat gate keys on seatId OR tier: {seatId, tier: null} no longer sends
a real seat-holder to the 'no subscription' path (test pinned)
- move adapter to commands/code/adapters/ (matches clack-prompter etc.,
removes inverted auth→commands dependency)
- vi.stubEnv in tests (matches config.test.ts pattern)
| homeDir: os.homedir(), | ||
| isTty: process.stdin.isTTY, | ||
| prompter: new ClackPrompter(), | ||
| seatStatusService: createSeatStatusService(), |
There was a problem hiding this comment.
🟠 warning — WizardDeps has no seatStatusService property (TS2353) — npm run build fails; declaring the field on the interface (same fix as line 99) clears this.
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| vi.unstubAllEnvs(); |
There was a problem hiding this comment.
🟡 nit — vi.unstubAllEnvs() restores stubs but no longer deletes a real BERGET_API_URL (the old afterEach did), so the default-URL assertions fail wherever that env var is set — delete it in beforeEach.
ReviewFollow-up reviewNew changes: 1 commit — Previous findings:
New findings:
All four substantive code fixes were verified directly and are correct in isolation — the follow-up commit simply swapped one type error for a type and runtime error at the wiring site, so the PR remains unmergeable. CodeSense unchanged at 7.5. Suggestions
Inline findingsBerget AI (berget/zai-org/GLM-5.3-Flash) | PR #88 |
tsc caught what local type-stripping hid: WizardDeps lacked the field and the configureAuth call site in runInit didn't pass it. init.test.ts makeDeps now defaults to a no-seat fake; seat-path tests override.
There was a problem hiding this comment.
Review
Follow-up review
New changes: 1 commit (21061ae) — adds seatStatusService to WizardDeps, destructures it in runInit, and passes it through to configureAuth; test makeDeps now defaults a null-seat FakeSeatStatusService.
Previous findings:
- ✅ critical (init.ts
configureAuthmissingseatStatusService→ TS2345 + runtimeTypeError+ 3 crashed tests) — fixed: declared atinit.ts:40, destructured:67, passed:110,runInitCommandwires it:144; verified locally:tsc --noEmitexit 0,vitest run265/265 passed (was 3 failed). - ✅ warning (
seatStatusServicepassed toexecuteInitCommandbut absent fromWizardDeps→ TS2353) — fixed by the same interface change. - ✅ critical #1 from first review (TS2339 in
configureAuthPick) — now fully fixed (typecheck clean). - ✅ warnings (config reuse, 10s timeout,
seatId||tiergate) — fixed earlier, still correct in this wiring. - ⏳ nit (
seat-status.test.ts:9—vi.unstubAllEnvs()doesn't delete a realBERGET_API_URL, so default-URL asserts break in envs where it's set) — still present, file untouched by this commit. - ⏳ nit (explicit 401 fall-through test) — still open,
auth-sync.test.tsuntouched.
New findings: None. The test-helper spread exclusion list omits seatStatusService, but the double-set is with the identical value (overrides.seatStatusService ?? default on both sides) so it's inert; the two explicit seated fakes at init.test.ts:581/737 are redundant with makeDeps overrides but intentional for the seat-path tests.
Done right: the breaking wiring is now exercised by the repo's own runInit suite via the makeDeps default instead of crashing.
Inline findings
Berget AI (berget/zai-org/GLM-5.3-Flash) | PR #88
| homeDir: string; | ||
| isTty?: boolean; | ||
| prompter: Prompter; | ||
| seatStatusService: SeatStatusPort; |
There was a problem hiding this comment.
✅ good — Fixes both prior findings: seatStatusService is now a required WizardDeps field and is threaded to configureAuth (line 110); verified tsc clean and 265/265 tests green.
| homeDir: '/home/user', | ||
| isTty: overrides.isTty ?? true, | ||
| prompter: overrides.prompter ?? new FakePrompter([]), | ||
| seatStatusService: |
There was a problem hiding this comment.
✅ good — Defaulting a null-seat FakeSeatStatusService in makeDeps repairs the three previously crashing runInit tests without weakening their assertions.
There was a problem hiding this comment.
Review
Follow-up review
New changes: 1 commit (2820d89) — pure eslint --fix style pass: prettier line-wrapping and perfectionist reorderings (imports, object keys, union member order) across 7 files; zero semantic delta.
Previous findings:
- ✅ blocker
configureAuthmissingseatStatusService(TS2345 + TypeError + 3 crashed tests) — fixed. - ✅ warning
seatStatusServiceabsent fromWizardDeps(TS2353) — fixed. - ✅ blocker TS2339 in
configureAuthPick— fixed. - ✅ warnings (config reuse, 10s
AbortSignal.timeout,seatId||tiergate) — fixed, unchanged and still correct. - ⏳ nit
seat-status.test.tsvi.unstubAllEnvs()doesn't remove a real inheritedBERGET_API_URL, so default-URL asserts break in envs where it's set — still present; this commit touched the file but only reordered mock-object keys, leavingafterEachunhydrated. - ⏳ nit explicit 401 fall-through test for the
auth-syncseat path — still open;auth-sync.test.tstouched only by import reorder.
New findings: None — union reordering (null | SeatStatus), object-key ordering, and import sorting are inert in TS/JS. Verified: tsc --noEmit exit 0 and vitest run 23 files / 265/265 passed.
Done right: the style pass wasted no chance to sneak behavioral edits — every hunk is provably mechanical, confirmed by the green suite.
Inline findings
Berget AI (berget/zai-org/GLM-5.3-Flash) | PR #88
1 participant
Why the first fix wasn't enough (support case follow-up)
PR #84 taught the CLI to accept all seat roles — but a Pro subscriber still got 'no subscription'. Root cause found in prod data: Pro/Summit seats never get a Keycloak role at all (seat-product.config deliberately omits
keycloakRolefor the newer tiers — 'berget.seat-only'), and roles drift anyway (a Summit subscriber carried a staleberget_code_seatrole — verified live via/v1/auth/status).The API already authorizes inference against berget.seat (Odoo), not roles. The CLI was the last consumer reading roles.
Change
configureAuthroutes onGET /v1/auth/status(Bearer token) →seatId/tier— the same canonical source the API authorizes againsthasBergetCodeSeat+ role list removed — the CLI no longer reads JWT roles anywhereSeatStatusPort(ports/adapters pattern), production impl never throwsNo backend changes needed
/v1/auth/statusalready exposesseatId/tierfor every tier. No Keycloak role creation, no backfill — aligned with moving away from seat roles entirely.Tests (TDD)
New seat-status service tests (seat/no-seat/401/network/env-override); auth-sync cases re-routed through a
FakeSeatStatusService(seat / no-seat / unverifiable); dead-code tests removed. 54/54 in touched suites; remaining suite failures are pre-existing on main (missingopenid-clientin node_modules, middleware test — verified via stash).