Skip to content

fix: [ENG-2941] tolerate missing body in auth:getState#709

Merged
bao-byterover merged 1 commit into
mainfrom
fix/ENG-2941
May 27, 2026
Merged

fix: [ENG-2941] tolerate missing body in auth:getState#709
bao-byterover merged 1 commit into
mainfrom
fix/ENG-2941

Conversation

@bao-byterover
Copy link
Copy Markdown
Collaborator

TUI sends no body for auth:getState, so the {projectPath} destructure added in #653 threw and the existing catch-all swallowed it into {isAuthorized: false}, forcing the TUI into the provider chooser and re-prompting byterover login on every startup. The valid token already proves the user is logged in; treat projectPath as optional and pull it via optional chaining so a missing body returns the correct auth state.

Summary

  • Problem: brv TUI always landed on the provider-chooser screen at startup and re-asked the user to log in when they selected byterover, even though brv status (CLI) and the local WebUI dashboard both reported the user as logged in with the right provider.
  • Why it matters: byterover users could not reach the REPL; reported as Urgent in ENG-2941.
  • What changed: the daemon's auth:getState handler now reads projectPath via optional chaining (data?.projectPath) instead of destructuring, and the shared request type marks projectPath as optional. Five regression tests added under a new setupGetState describe block (the path had zero coverage, which is why the original change slipped through).
  • What did NOT change (scope boundary): TUI client code is untouched. WebUI client code is untouched. The pre-existing broad catch that swallows userService.getCurrentUser failures into {isAuthorized: false} is also untouched (separate latent issue, predates Proj/rework llm billing #653, not part of ENG-2941's "always stuck" symptom).

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Test
  • Chore (build, dependencies, CI)

Scope (select all touched areas)

  • TUI / REPL
  • Agent / Tools
  • LLM Providers
  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)
  • Hub / Connectors
  • Cloud Sync
  • CI/CD / Infra

Linked issues

Root cause (bug fixes only, otherwise write N/A)

  • Root cause: PR Proj/rework llm billing #653 changed the daemon's auth:getState handler from const projectPath = this.resolveProjectPath(clientId) to const {projectPath} = data and updated the WebUI client to send {projectPath}. The TUI client (src/tui/features/auth/api/get-auth-state.ts:17) was missed and kept sending undefined as the body. On the daemon, destructuring undefined threw a TypeError, which the long-standing broad catch swallowed and converted into {isAuthorized: false}. The TUI's useAppViewMode then routed to ConfigProviderPage, and provider-flow.tsx's byterover branch re-prompted login.
  • Why this was not caught earlier: setupGetState had zero test coverage. The existing auth-handler.test.ts exercised broadcast, login, logout, and the external-auth-sync paths, but never the request/response path. TypeScript could not catch the gap either because the contract was changed in the type def (required) without updating the TUI caller.

Test plan

  • Coverage added:
    • Unit test
    • Integration test
    • Manual verification only
  • Test file(s): test/unit/infra/transport/handlers/auth-handler.test.ts (new describe('setupGetState') block with five cases).
  • Key scenario(s) covered:
    1. Valid token + body undefined returns {isAuthorized: true} with user, no projectConfigStore.read call. This is the ENG-2941 regression guard.
    2. Valid token + body {projectPath: '/foo'} returns full payload including brvConfig. This locks the WebUI happy path.
    3. Valid token + body {} returns authorized without brvConfig. Confirms the optional contract.
    4. Missing token + any body returns {isAuthorized: false}. Confirms the not-logged-in check still wins.
    5. Expired token + any body returns {isAuthorized: false}. Confirms token expiry still wins.

User-visible changes

  • TUI: brv (no args) now drops the user straight into the REPL when the user is logged in and byterover is the active provider, instead of forcing the provider-chooser screen.
  • CLI commands (brv status, brv providers list, brv login, brv logout): unchanged.
  • WebUI: unchanged.
  • No config, default, or flag changes.

Evidence

Before the fix, the new regression-guard test fails:

$ npx mocha --forbid-only "test/unit/infra/transport/handlers/auth-handler.test.ts"
  setupGetState
    1) returns isAuthorized=true and skips brvConfig when body is undefined (TUI sends no body)
    ✔ returns full state including brvConfig when body has projectPath (WebUI happy path)
    ✔ returns isAuthorized=true and skips brvConfig when body is empty object
    ✔ returns isAuthorized=false when token is missing, regardless of body
    ✔ returns isAuthorized=false when token is expired, regardless of body

  23 passing (236ms)
  1 failing

  1) AuthHandler — setupExternalAuthSync
       setupGetState
         returns isAuthorized=true and skips brvConfig when body is undefined (TUI sends no body):
      AssertionError: expected false to equal true

After the fix:

$ npx mocha --forbid-only "test/unit/infra/transport/handlers/auth-handler.test.ts"
  setupGetState
    ✔ returns isAuthorized=true and skips brvConfig when body is undefined (TUI sends no body)
    ✔ returns full state including brvConfig when body has projectPath (WebUI happy path)
    ✔ returns isAuthorized=true and skips brvConfig when body is empty object
    ✔ returns isAuthorized=false when token is missing, regardless of body
    ✔ returns isAuthorized=false when token is expired, regardless of body

  24 passing (228ms)

$ npm test
  8338 passing (30s)
  16 pending
  (0 failing)

$ npm run typecheck
  > tsc --noEmit && tsc --noEmit -p src/webui/tsconfig.json
  (clean exit)

Checklist

  • Tests added or updated and passing (npm test)
  • Lint passes (npm run lint) - pre-existing submodule config error on main (packages/byterover-packages/ui/... cannot resolve @workspace/typescript-config/react-library.json); reproduced on a clean main checkout, not introduced by this PR. The pre-commit lint:fix hook scoped to the three changed files passes.
  • Type check passes (npm run typecheck)
  • Build succeeds (npm run build)
  • Commits follow Conventional Commits format
  • Documentation updated (if applicable) - N/A
  • No breaking changes (or clearly documented above) - widening projectPath: string to projectPath?: string is strictly more permissive; all existing callers continue to type-check
  • Branch is up to date with main

Risks and mitigations

  • Risk: any future caller of auth:getState that omits the body will now receive {isAuthorized: true} with brvConfig: undefined instead of an error, so a caller that silently depends on brvConfig being populated could read undefined where it previously got a hard failure.
    • Mitigation: grep across src/ and test/ confirms only two callers exist today: WebUI (always sends {projectPath}, behavior unchanged) and TUI (never read brvConfig from this response). Five new tests pin the contract for both shapes.
  • Risk: the broad } catch { return {isAuthorized: false} } inside setupGetState still silently downgrades userService.getCurrentUser failures to "not logged in", so a flaky network can still flip the TUI to onboarding intermittently.
    • Mitigation: this is a pre-existing latent issue predating Proj/rework llm billing #653, separate from ENG-2941's "always stuck on every startup" symptom. Out of scope for this PR; can be filed as a follow-up if desired.

TUI sends no body for auth:getState, so the {projectPath} destructure
added in #653 threw and the existing catch-all swallowed it into
{isAuthorized: false} — forcing the TUI into the provider chooser and
re-prompting byterover login on every startup. The valid token already
proves the user is logged in; treat projectPath as optional and pull it
via optional chaining so a missing body returns the correct auth state.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

Claude finished @bao-byterover's task in 3m 14s —— View job


Review Summary

Score: 4.5 / 5 — clean, minimal, well-tested, properly scoped urgent fix. Two non-blocking observations below for follow-up.

  • Read CLAUDE.md and gather repo context
  • Read the three changed files
  • Inspect surrounding code (handler, callers, broad catch)
  • Cross-check claim that WebUI always sends {projectPath} and TUI sends none
  • Verify the broader contract / regression concerns
  • Post inline comments
  • Post overall summary + score

What works well

  • praise: Correct root-cause fix. The destructure-on-undefined trap from Proj/rework llm billing #653 is replaced with data?.projectPath and the existing projectPath ? ... : Promise.resolve() guard already handled the absent case downstream — so the change is genuinely one line of behavior with no new code paths.
  • praise: The type widening (projectPath: stringprojectPath?: string) is strictly more permissive. Verified both callers: WebUI (src/webui/features/auth/api/get-auth-state.ts:8) keeps a non-optional positional parameter and still always sends {projectPath}; TUI (src/tui/features/auth/api/get-auth-state.ts:17) sends undefined. No compile-time fallout.
  • praise: Test coverage is well-targeted — five focused cases pin the contract (undefined, {}, {projectPath}, missing token, expired token). The new setupGetState block fills a gap that, per the PR description, had zero coverage prior. ENG-2941 regression is explicitly guarded.
  • praise: PR description does the work — root cause, scope boundary, risks, before/after evidence. Nice.

Non-blocking observations (follow-ups, not for this PR)

  • note: The broad } catch { return {isAuthorized: false} } at src/server/infra/transport/handlers/auth-handler.ts:249–251 still flattens userService.getCurrentUser failures into "not logged in", which can re-create the exact ENG-2941 symptom (TUI stuck on provider chooser) on flaky networks instead of every startup. PR description already calls this out as out-of-scope. The sibling broadcastAuthStateChanged (lines 117–122) already shows the right pattern: broadcast {isAuthorized: true} without user details on getCurrentUser failure. See inline comment on line 226.
  • note: Stale comment in src/tui/features/auth/components/auth-initializer.tsx:75–76 claims brvConfig is resolved "via clientId", which has not been true since Proj/rework llm billing #653 changed resolution to the request body. After this fix, the TUI re-fetch on line 78 still won't populate brvConfig because the TUI sends no projectPath. Confirmed via grep that no TUI feature outside auth/ reads useAuthStore().brvConfig (the consumers in format-status.ts and vc-clone-flow.tsx read it from different APIs), so this is harmless today — but the comment is misleading and worth updating in a follow-up, or have the TUI client pass its own projectPath if brvConfig is ever needed there.

Verification I performed

  • Confirmed the only two callers of auth:getState are src/tui/features/auth/api/get-auth-state.ts:17 (sends undefined) and src/webui/features/auth/api/get-auth-state.ts:14 (sends {projectPath}). PR's risk-section claim holds.
  • Confirmed runtime guard projectPath ? ... : Promise.resolve() correctly skips the projectConfigStore.read call for both undefined and empty-string projectPath.
  • Confirmed nothing outside src/tui/features/auth/ reads useAuthStore().brvConfig in the TUI, so the broadcast-omits-brvConfig path the TUI hits today is functionally fine.
  • Did not run the test suite locally (this is a code-review task); PR shows green test/typecheck runs.

Recommendation

Ship it. The two notes above are genuine follow-ups, not gates on this PR — ENG-2941 is Urgent and this fix is the right minimal change.
· branch fix/ENG-2941

}

const {projectPath} = data
const projectPath = data?.projectPath
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.

note (non-blocking): The fix here is exactly right for ENG-2941. Flagging for a follow-up (already acknowledged in the PR description): the broad } catch { return {isAuthorized: false} } on lines 249–251 still converts any unexpected throw from userService.getCurrentUser (e.g. transient network error) into {isAuthorized: false}, which re-creates the exact "stuck on provider chooser" symptom this PR fixes — just on flaky networks instead of every startup. Worth narrowing the catch in a follow-up so token validity (the actual auth signal) is decoupled from user-info fetch failures. The new broadcastAuthStateChanged (lines 117–122) already does this — broadcasts {isAuthorized: true} without user details on getCurrentUser failure — so the pattern is already in this file.

export interface AuthGetStateRequest {
projectPath: string
projectPath?: string
}
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.

suggestion (non-blocking, if-minor): Widening projectPath: stringprojectPath?: string is the right call, but the field now has subtle semantics that aren't obvious from the type alone: omitting it returns a valid AuthGetStateResponse without brvConfig. Consider a one-line JSDoc so future callers don't get caught the way the TUI did in #653:

Suggested change
}
export interface AuthGetStateRequest {
/** Project-scoped lookup for `brvConfig` (space/team). Omit to skip the lookup and return auth-only state. */
projectPath?: string
}

expect(result.authToken).to.have.property('accessToken', 'test-access-token')
expect(projectConfigStore.read.called, 'projectConfigStore.read should not be called without projectPath').to.be
.false
})
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.

praise: Good ENG-2941 regression guard. The expect(projectConfigStore.read.called, '...').to.be.false assertion (line 573–574) is the right thing to pin — it locks in both the new contract (projectPath is optional) and the runtime behavior (skip the project read when absent). The empty-object case below (line 588) and the projectPath-present case (line 577) cover the other branches cleanly. The five tests together are a solid spec for the handler.

Copy link
Copy Markdown
Collaborator

@cuongdo-byterover cuongdo-byterover left a comment

Choose a reason for hiding this comment

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

good on my side

@bao-byterover bao-byterover merged commit 9ffe89a into main May 27, 2026
6 checks passed
@bao-byterover bao-byterover deleted the fix/ENG-2941 branch May 27, 2026 02:45
@github-actions github-actions Bot mentioned this pull request May 27, 2026
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.

2 participants