From 680bdbcd9767dc3abafe1dde759bbdbcc0301384 Mon Sep 17 00:00:00 2001 From: Olli-Pekka Heinisuo Date: Tue, 11 Aug 2026 10:36:17 +0300 Subject: [PATCH 1/2] feat: add canonical settings and terms commands Add the canonical account settings UX, terms acceptance and token refresh flows, and shared REST/GraphQL remediation handling. Preserve structured CLI and MCP errors across OAuth and opaque API-token authentication. --- README.md | 1 + docs/implementation/auth.md | 3 +- docs/implementation/cli-commands.md | 51 +++ docs/implementation/config.md | 7 +- .../services/code-navigation-service.test.ts | 37 ++ .../src/services/code-navigation-service.ts | 11 +- .../execute-with-token-refresh.test.ts | 24 ++ .../services/execute-with-token-refresh.ts | 4 +- .../src/services/githits-service.test.ts | 39 ++ .../src/services/githits-service.ts | 22 + .../package-intelligence-service.test.ts | 75 +++- .../services/package-intelligence-service.ts | 18 +- .../refreshing-githits-service.test.ts | 53 ++- .../services/refreshing-githits-service.ts | 4 +- .../src/shared/pkgseer-graphql.test.ts | 55 +++ .../src/shared/pkgseer-graphql.ts | 10 +- .../src/shared/terms-acceptance.test.ts | 39 ++ .../src/shared/terms-acceptance.ts | 86 ++++ .../shared/code-navigation-error-map.test.ts | 14 + .../src/shared/code-navigation-error-map.ts | 24 ++ .../shared/githits-service-error-map.test.ts | 16 + .../src/shared/githits-service-error-map.ts | 3 + .../package-intelligence-error-map.test.ts | 14 + .../shared/package-intelligence-error-map.ts | 7 +- scripts/cli-smoke.ts | 1 + src/cli.ts | 2 + src/commands/doctor.test.ts | 5 + src/commands/doctor.ts | 6 + src/commands/feedback.test.ts | 40 +- src/commands/feedback.ts | 34 +- src/commands/index.ts | 12 + src/commands/languages.test.ts | 43 +- src/commands/languages.ts | 34 +- src/commands/settings-schema.test.ts | 73 ++++ src/commands/settings-schema.ts | 139 +++++++ src/commands/settings.test.ts | 298 ++++++++++++++ src/commands/settings.ts | 385 ++++++++++++++++++ src/container.ts | 5 + src/services/settings-service.test.ts | 157 +++++++ src/services/settings-service.ts | 155 +++++++ src/shared/command-metadata.test.ts | 7 + src/shared/command-metadata.ts | 42 ++ 42 files changed, 1966 insertions(+), 89 deletions(-) create mode 100644 packages/core-internal/src/services/execute-with-token-refresh.test.ts create mode 100644 packages/core-internal/src/shared/terms-acceptance.test.ts create mode 100644 packages/core-internal/src/shared/terms-acceptance.ts create mode 100644 src/commands/settings-schema.test.ts create mode 100644 src/commands/settings-schema.ts create mode 100644 src/commands/settings.test.ts create mode 100644 src/commands/settings.ts create mode 100644 src/services/settings-service.test.ts create mode 100644 src/services/settings-service.ts diff --git a/README.md b/README.md index 27182a46..b901151a 100644 --- a/README.md +++ b/README.md @@ -350,6 +350,7 @@ githits example Find real-world implementations from open source githits languages List or filter supported programming languages githits feedback Submit feedback about GitHits results githits doctor Diagnose configuration and auth state +githits settings View and update preferences, privacy, and terms githits search Explore repository code, dependencies, docs, and symbols githits search-status Check the status of a previous indexed search githits code List, read, and grep indexed dependency source diff --git a/docs/implementation/auth.md b/docs/implementation/auth.md index 5c5ef8d3..e25bd705 100644 --- a/docs/implementation/auth.md +++ b/docs/implementation/auth.md @@ -177,7 +177,7 @@ CLI startup / MCP server start Per API call (via RefreshingGitHitsService): └─ TokenProvider.getToken() → get fresh token - └─ on AuthenticationError from API → forceRefresh() → retry once + └─ on AuthenticationError or TERMS_ACCEPTANCE_REQUIRED → forceRefresh() → retry once ``` The MCP server starts without a synchronous auth gate. Tool calls resolve tokens through the shared token provider and return per-tool auth errors when no valid token is available. @@ -185,6 +185,7 @@ The MCP server starts without a synchronous auth gate. Tool calls resolve tokens ## Troubleshooting - **"Authentication required" from a command or MCP tool** — No valid token found. Run `githits login` or set `GITHITS_API_TOKEN`. +- **"Terms acceptance required"** — Run `githits settings terms accept` or open the environment-specific `acceptance_url` returned by the backend. OAuth sessions refresh after acceptance; static `ghi-*` tokens remain unchanged and are re-evaluated server-side without token refresh. - **Different auth behavior across terminals or agents** — Run `githits doctor` or `githits doctor --json` to compare redacted runtime, environment, config, and auth-storage diagnostics without exposing token values. - **"Already logged in."** — Token is still valid. Use `githits login --force` to re-authenticate. - **Port conflicts on login** — The callback server uses the port from the stored client registration. On first login, a random port (8000–9999) is chosen and saved. Use `--port ` to change it (triggers re-registration). diff --git a/docs/implementation/cli-commands.md b/docs/implementation/cli-commands.md index 45e8a570..f5521aae 100644 --- a/docs/implementation/cli-commands.md +++ b/docs/implementation/cli-commands.md @@ -17,6 +17,13 @@ The CLI exposes setup/auth commands, `doctor`, `example`, `languages`, `feedback | `languages [query]` | — | `--json` | List or filter supported languages | | `feedback [solution_id]` | `--accept` or `--reject` | `-m, --message `, `--tool `, `--json` | Submit solution-tied or generic session feedback | | `doctor` | — | `--json` | Print redacted diagnostics for GitHits runtime, environment, service URLs, config, and auth storage | +| `settings` | — | `--json` | Show canonical preferences, privacy and terms, and account limits | +| `settings show` | — | `--json` | Explicit form of `settings` for showing all account settings | +| `settings get ` | setting key | `--json` | Read one writable setting using its public CLI name | +| `settings set ` | setting key and typed value(s) | `--json` | Selectively update one writable account setting | +| `settings clear ` | clearable setting key | `--json` | Clear the default language or replace blocked license IDs with an empty list | +| `settings terms` | — | `--json` | Show the current Terms of Service acceptance state | +| `settings terms accept` | — | `--yes`, `--json` | Confirm and accept the current Terms of Service | | `pkg info ` | package spec | `--verbose`, `--json` | Show a package overview (latest version, downloads, license, vulnerabilities) | | `pkg vulns ` | package spec (optional `@version`) | `--severity`, `--scope`, `--include-withdrawn`, `--verbose`, `--json` | List known vulnerabilities for a package (npm/pypi/hex/crates/nuget/maven/packagist/rubygems/go/swift) | | `pkg deps ` | package spec (optional `@version`) | `--lifecycle`, `--depth`, `--verbose`, `--json` | Analyse dependencies: direct runtime deps, structured groups, optional capped transitive graph (npm/pypi/hex/crates/vcpkg/zig/rubygems/go/swift) | @@ -94,6 +101,50 @@ For automation, `githits init uninstall --yes` is user-level only and never touc **File structure:** The init command uses a subdirectory (`src/commands/init/`) because it has distinct submodules (agent definitions, setup handlers, orchestrator). This is an accepted variation for commands with significant internal complexity. +### `githits settings` + +```sh +githits settings +githits settings --json +githits settings show +githits settings get license-mode +githits settings set license-mode safe +githits settings set marketing-emails disabled +githits settings set blocked-license-ids 0198a7d0-6750-7ace-a68c-418062117d95 0198a7d0-6750-7ace-a68c-418062117d96 +githits settings clear blocked-license-ids +githits settings terms +githits settings terms accept +githits settings terms accept --yes --json +``` + +Settings calls the self-scoped account API with the active credential. The root +command and `show` display all settings. `get`, `set`, and `clear` use a +whitelisted public key schema: `default-language-id`, `license-mode`, +`blocked-license-ids`, and `marketing-emails`. The schema validates each value +and maps it to the canonical API field, so the CLI does not expose the negative +`marketing_email_opted_out` storage name. `marketing-emails` accepts +`enabled`/`disabled`; `license-mode` accepts `safe`/`yolo`/`custom`; blocked +license IDs are an atomic list replacement. `clear blocked-license-ids` sends +an explicit empty list, while `clear default-language-id` sends null. + +JSON overview and update output remains the canonical settings object. +`settings get --json` returns `{key, value}` using the public key and +value. Every mutation sends exactly one selective PATCH. JSON batch input is +intentionally omitted until an atomic multi-setting workflow is required. + +Terms acceptance prompts unless `--yes` is supplied. OAuth sessions are +force-refreshed after the write so subsequent requests receive the updated JWT +claim. Static `GITHITS_API_TOKEN` credentials are not refreshed; their terms +state is re-evaluated server-side without token refresh. If acceptance succeeds but +OAuth refresh fails, output reports the saved acceptance and instructs the user +to run `githits login --force`. + +Downstream REST and GraphQL clients recognize the structured +`TERMS_ACCEPTANCE_REQUIRED` response, refresh OAuth at most once, and retry at +most once. A still-gated request returns the stable +`githits settings terms accept` remediation plus the authenticated web +acceptance URL; `ghi-*` credentials never enter a refresh loop. + ### `githits example` ``` diff --git a/docs/implementation/config.md b/docs/implementation/config.md index 65494df9..97526b29 100644 --- a/docs/implementation/config.md +++ b/docs/implementation/config.md @@ -2,11 +2,11 @@ ## Purpose -The CLI uses three separate service URLs and supports three authentication modes. Getting these wrong causes subtle failures — wrong URL means auth works but API calls fail, wrong auth mode means some tools work but others silently return errors. This document explains the configuration model so changes are made with full context. +The CLI uses four separate service URLs and supports three authentication modes. Getting these wrong causes subtle failures — wrong URL means auth works but API calls fail, wrong auth mode means some tools work but others silently return errors. This document explains the configuration model so changes are made with full context. ## Background -GitHits separates its MCP server (which handles OAuth discovery and the MCP protocol), REST API (which handles search, languages, and feedback), and package/source service. In production, they use independent endpoints. +GitHits separates its MCP server (which handles OAuth discovery and the MCP protocol), REST API (which handles search, languages, and feedback), account settings API, and package/source service. In production, they use independent endpoints. ## URL Configuration @@ -14,6 +14,7 @@ GitHits separates its MCP server (which handles OAuth discovery and the MCP prot |---|---|---|---| | **MCP URL** | `https://mcp.githits.com` | `GITHITS_MCP_URL` | OAuth discovery (`.well-known`), DCR registration, auth flow | | **API URL** | `https://api.githits.com` | `GITHITS_API_URL` | REST endpoints (`/search`, `/languages`, `/feedbacks`) | +| **Accounts URL** | `https://accounts.githits.com` | `GITHITS_ACCOUNTS_URL` | Self-scoped settings and Terms of Service acceptance | | **Package/source URL** | GitHits-managed package/source service | `GITHITS_CODE_NAV_URL` | Package/source service endpoint used by indexed `search` / `pkg` / `docs` / `code` tooling | > **These are different services.** Override every URL that differs from production when pointing to a non-production backend. @@ -41,6 +42,7 @@ The container (`src/container.ts`) resolves authentication in priority order: | `/search` | Full access | Full access | Blocked | | `/languages` | Full access | Full access | Blocked | | `/feedbacks` | Full access | Full access | Blocked | +| `/functions/v1/settings/me` | Full access | Full access | Blocked | Package/source access uses the package/source service URL from `GITHITS_CODE_NAV_URL`, defaulting to the GitHits-managed endpoint. MCP registration for `search`, `search_status`, `docs_*`, `pkg_*`, `code_files`, `code_read`, and `code_grep` is always on; CLI registration for top-level `search` / `search-status` plus the `githits code`, `githits pkg`, and `githits docs` groups is also always on. @@ -51,6 +53,7 @@ Package/source access uses the package/source service URL from `GITHITS_CODE_NAV | `GITHITS_MCP_URL` | Override MCP server URL | `http://localhost:7071/mcp` | | `GITHITS_API_URL` | Override REST API URL | `http://localhost:8000` | | `GITHITS_CODE_NAV_URL` | Override package/source service URL | `http://localhost:4000` | +| `GITHITS_ACCOUNTS_URL` | Override account settings origin | `https://zcwquvryvmjuwckxdevg.supabase.co` | | `GITHITS_API_TOKEN` | API token for authentication | `ghi-abc123...` | | `GITHITS_AUTH_STORAGE` | Override OAuth credential storage for the current process (`keychain` or `file`) | `file` | | `GITHITS_TELEMETRY` | Emit end-of-run timing spans to stderr for local profiling | `1` | diff --git a/packages/core-internal/src/services/code-navigation-service.test.ts b/packages/core-internal/src/services/code-navigation-service.test.ts index ebb3f127..cf289078 100644 --- a/packages/core-internal/src/services/code-navigation-service.test.ts +++ b/packages/core-internal/src/services/code-navigation-service.test.ts @@ -17,6 +17,7 @@ import { CodeNavigationTargetNotFoundError, CodeNavigationVersionNotFoundError, } from "./code-navigation-service.js"; +import { TermsAcceptanceRequiredError } from "./githits-service.js"; import { createMockTokenProvider } from "./test-helpers.js"; function mockFetch(impl: () => Promise) { @@ -34,6 +35,42 @@ describe("CodeNavigationServiceImpl", () => { originalFetch = globalThis.fetch; }); + it("recognises HTTP terms gating and does not loop without a refresh token", async () => { + const fetchFn = mockFetch(() => + Promise.resolve( + new Response( + JSON.stringify({ + errors: [ + { + message: "Terms acceptance required", + extensions: { + code: "TERMS_ACCEPTANCE_REQUIRED", + terms_url: "https://githits.com/legal/terms-of-service/", + acceptance_url: + "https://acceptance.example.test/settings/privacy", + }, + }, + ], + }), + { status: 403 }, + ), + ), + ); + const service = new CodeNavigationServiceImpl( + BASE_URL, + createMockTokenProvider({ + forceRefresh: mock(() => Promise.resolve(undefined)), + }), + ); + + await expect( + service.listFiles({ + target: { registry: "NPM", packageName: "express" }, + }), + ).rejects.toBeInstanceOf(TermsAcceptanceRequiredError); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + afterEach(() => { globalThis.fetch = originalFetch; if (originalDebug === undefined) delete process.env.GITHITS_DEBUG; diff --git a/packages/core-internal/src/services/code-navigation-service.ts b/packages/core-internal/src/services/code-navigation-service.ts index afadb17e..1ccd4ba6 100644 --- a/packages/core-internal/src/services/code-navigation-service.ts +++ b/packages/core-internal/src/services/code-navigation-service.ts @@ -16,6 +16,7 @@ import { import { executeWithTokenRefresh } from "./execute-with-token-refresh.js"; import { AuthenticationError, + isTokenRefreshableError, SERVER_AUTHENTICATION_REJECTED_MESSAGE, } from "./githits-service.js"; import type { TokenProvider } from "./token-provider.js"; @@ -1837,7 +1838,7 @@ export class CodeNavigationServiceImpl implements CodeNavigationService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeUnifiedSearch(token, params), }); } @@ -1849,7 +1850,7 @@ export class CodeNavigationServiceImpl implements CodeNavigationService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeUnifiedSearchStatus(token, searchRef, waitTimeoutMs), }); @@ -2449,7 +2450,7 @@ export class CodeNavigationServiceImpl implements CodeNavigationService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeListFiles(token, params), }); } @@ -2552,7 +2553,7 @@ export class CodeNavigationServiceImpl implements CodeNavigationService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeReadFile(token, params), }); } @@ -2632,7 +2633,7 @@ export class CodeNavigationServiceImpl implements CodeNavigationService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeGrepRepo(token, params), }); } diff --git a/packages/core-internal/src/services/execute-with-token-refresh.test.ts b/packages/core-internal/src/services/execute-with-token-refresh.test.ts new file mode 100644 index 00000000..ce440511 --- /dev/null +++ b/packages/core-internal/src/services/execute-with-token-refresh.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it, mock } from "bun:test"; +import { executeWithTokenRefresh } from "./execute-with-token-refresh.js"; +import { TermsAcceptanceRequiredError } from "./githits-service.js"; + +describe("executeWithTokenRefresh", () => { + it("never calls the refresh hook for an opaque ghi-* token", async () => { + const forceRefresh = mock(() => Promise.resolve("unexpected-token")); + const executeWithToken = mock(() => + Promise.reject(new TermsAcceptanceRequiredError()), + ); + + await expect( + executeWithTokenRefresh({ + getToken: mock(() => Promise.resolve("ghi-static-token")), + forceRefresh, + executeWithToken, + shouldRefresh: () => true, + }), + ).rejects.toBeInstanceOf(TermsAcceptanceRequiredError); + + expect(forceRefresh).not.toHaveBeenCalled(); + expect(executeWithToken).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/core-internal/src/services/execute-with-token-refresh.ts b/packages/core-internal/src/services/execute-with-token-refresh.ts index 7a79e574..9367b9a9 100644 --- a/packages/core-internal/src/services/execute-with-token-refresh.ts +++ b/packages/core-internal/src/services/execute-with-token-refresh.ts @@ -28,7 +28,9 @@ export async function executeWithTokenRefresh( try { return await options.executeWithToken(token); } catch (error) { - if (!options.shouldRefresh(error)) { + // Opaque ghi-* credentials are re-evaluated server-side and have no local + // refresh flow. Do not invoke the provider's refresh hook for them. + if (token.startsWith("ghi-") || !options.shouldRefresh(error)) { throw error; } diff --git a/packages/core-internal/src/services/githits-service.test.ts b/packages/core-internal/src/services/githits-service.test.ts index 410ded4e..bfe64d2a 100644 --- a/packages/core-internal/src/services/githits-service.test.ts +++ b/packages/core-internal/src/services/githits-service.test.ts @@ -16,6 +16,7 @@ import { ApiRateLimitError, AuthenticationError, GitHitsServiceImpl, + TermsAcceptanceRequiredError, } from "./githits-service.js"; // Helper to mock global fetch with proper typing @@ -79,6 +80,44 @@ describe("GitHitsServiceImpl", () => { }); describe("search", () => { + it("recognises the canonical terms-required 403 contract", async () => { + mockFetch(() => + Promise.resolve( + new Response( + JSON.stringify({ + reason: "Terms acceptance required", + code: "TERMS_ACCEPTANCE_REQUIRED", + terms_url: "https://githits.com/legal/terms-of-service/", + acceptance_url: + "https://acceptance.example.test/settings/privacy", + }), + { status: 403 }, + ), + ), + ); + + await expect(service.search({ query: "test" })).rejects.toMatchObject({ + name: TermsAcceptanceRequiredError.name, + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }); + }); + + it("uses the production acceptance page when a legacy response omits it", async () => { + mockFetch(() => + Promise.resolve( + new Response(JSON.stringify({ code: "TERMS_ACCEPTANCE_REQUIRED" }), { + status: 403, + }), + ), + ); + + await expect(service.search({ query: "test" })).rejects.toMatchObject({ + name: TermsAcceptanceRequiredError.name, + acceptanceUrl: "https://app.githits.com/settings/privacy", + }); + }); + it("sends correct request and returns markdown", async () => { const fn = mockFetch(() => Promise.resolve(new Response("# Result\nCode example here")), diff --git a/packages/core-internal/src/services/githits-service.ts b/packages/core-internal/src/services/githits-service.ts index fb71d81d..df438ff9 100644 --- a/packages/core-internal/src/services/githits-service.ts +++ b/packages/core-internal/src/services/githits-service.ts @@ -8,8 +8,21 @@ import { import { parseHttpErrorDetail } from "../shared/http-error-detail.js"; import type { ClientHeaderBuilder } from "../shared/request-headers.js"; import { withTelemetrySpan } from "../shared/telemetry.js"; +import { + TermsAcceptanceRequiredError, + throwIfTermsAcceptanceRequired, +} from "../shared/terms-acceptance.js"; import { validateServiceUrl } from "./config.js"; +export { + createTermsAcceptanceError, + TERMS_ACCEPTANCE_REQUIRED_CODE, + TERMS_ACCEPTANCE_URL, + TERMS_URL, + type TermsAcceptanceRemediation, + TermsAcceptanceRequiredError, +} from "../shared/terms-acceptance.js"; + const DEFAULT_EXAMPLE_REQUEST_TIMEOUT_MS = 240_000; /** @@ -41,6 +54,14 @@ export class AuthenticationError extends Error { } } +/** A stale OAuth JWT can be refreshed once for either auth failure signal. */ +export function isTokenRefreshableError(error: unknown): boolean { + return ( + error instanceof AuthenticationError || + error instanceof TermsAcceptanceRequiredError + ); +} + /** * Error returned when the REST API asks the client to retry later. * @@ -352,6 +373,7 @@ export class GitHitsServiceImpl implements GitHitsService { const status = response.status; const body = await response.text().catch(() => ""); const detail = parseHttpErrorDetail(body, ["detail"]); + throwIfTermsAcceptanceRequired(body); switch (status) { case 401: diff --git a/packages/core-internal/src/services/package-intelligence-service.test.ts b/packages/core-internal/src/services/package-intelligence-service.test.ts index 3f358877..fe61a3ae 100644 --- a/packages/core-internal/src/services/package-intelligence-service.test.ts +++ b/packages/core-internal/src/services/package-intelligence-service.test.ts @@ -8,7 +8,10 @@ import { spyOn, } from "bun:test"; import { FetchTimeoutError } from "../shared/fetch-timeout.js"; -import { AuthenticationError } from "./githits-service.js"; +import { + AuthenticationError, + TermsAcceptanceRequiredError, +} from "./githits-service.js"; import { MalformedPackageIntelligenceResponseError, PackageIntelligenceAccessError, @@ -375,6 +378,76 @@ describe("PackageIntelligenceServiceImpl", () => { expect(result.package.name).toBe("express"); }); + it("refreshes and retries once for GraphQL terms gating", async () => { + let callCount = 0; + const fetchFn = mock(() => { + callCount++; + if (callCount === 1) { + return Promise.resolve( + jsonResponse({ + errors: [ + { + message: "Terms acceptance required", + extensions: { code: "TERMS_ACCEPTANCE_REQUIRED" }, + }, + ], + }), + ); + } + return Promise.resolve(jsonResponse(HAPPY_BODY)); + }); + const refreshed = mock(() => Promise.resolve("new-token")); + const service = new PackageIntelligenceServiceImpl( + ENDPOINT, + createMockTokenProvider({ forceRefresh: refreshed }), + asFetchFn(fetchFn), + ); + + const result = await service.packageSummary({ + registry: "NPM", + packageName: "express", + }); + + expect(result.package.name).toBe("express"); + expect(refreshed).toHaveBeenCalledTimes(1); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it("recognises the canonical terms-required HTTP 403 contract", async () => { + const fetchFn = mock(() => + Promise.resolve( + jsonResponse( + { + errors: [ + { + message: "Terms acceptance required", + extensions: { + code: "TERMS_ACCEPTANCE_REQUIRED", + terms_url: "https://githits.com/legal/terms-of-service/", + acceptance_url: + "https://acceptance.example.test/settings/privacy", + }, + }, + ], + }, + 403, + ), + ), + ); + const service = new PackageIntelligenceServiceImpl( + ENDPOINT, + createMockTokenProvider({ + forceRefresh: mock(() => Promise.resolve(undefined)), + }), + asFetchFn(fetchFn), + ); + + await expect( + service.packageSummary({ registry: "NPM", packageName: "express" }), + ).rejects.toBeInstanceOf(TermsAcceptanceRequiredError); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + it("classifies 403 as PackageIntelligenceAccessError", async () => { const fetchFn = mock(() => Promise.resolve(jsonResponse({ detail: "no access" }, 403)), diff --git a/packages/core-internal/src/services/package-intelligence-service.ts b/packages/core-internal/src/services/package-intelligence-service.ts index 7c66dbc3..48ea5372 100644 --- a/packages/core-internal/src/services/package-intelligence-service.ts +++ b/packages/core-internal/src/services/package-intelligence-service.ts @@ -35,6 +35,7 @@ import { import { executeWithTokenRefresh } from "./execute-with-token-refresh.js"; import { AuthenticationError, + isTokenRefreshableError, SERVER_AUTHENTICATION_REJECTED_MESSAGE, } from "./githits-service.js"; import { promoteGenericVersionNotFound } from "./promote-version-not-found.js"; @@ -2292,7 +2293,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageSummary(token, params), }), ); @@ -2405,7 +2406,6 @@ export class PackageIntelligenceServiceImpl typeof extensions?.retryable === "boolean" ? extensions.retryable : undefined; - if (isClientUpdateRequiredGraphQLError({ message, code })) { return new ClientUpdateRequiredError( undefined, @@ -2565,7 +2565,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageVulnerabilities(token, params), }), @@ -2772,7 +2772,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageDependencies(token, params), }), @@ -2786,7 +2786,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageUpgradeDependencyProbe(token, params), }), @@ -2800,7 +2800,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageUpgradeReview(token, params), }), @@ -3244,7 +3244,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executePackageChangelog(token, params), }), @@ -3365,7 +3365,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeListPackageDocs(token, params), }), ); @@ -3469,7 +3469,7 @@ export class PackageIntelligenceServiceImpl executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: (token) => this.executeReadPackageDoc(token, params), }), ); diff --git a/packages/core-internal/src/services/refreshing-githits-service.test.ts b/packages/core-internal/src/services/refreshing-githits-service.test.ts index 3e942a25..32fbc431 100644 --- a/packages/core-internal/src/services/refreshing-githits-service.test.ts +++ b/packages/core-internal/src/services/refreshing-githits-service.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it, mock } from "bun:test"; -import { AuthenticationError } from "./githits-service.js"; +import { + AuthenticationError, + TermsAcceptanceRequiredError, +} from "./githits-service.js"; import { RefreshingGitHitsService } from "./refreshing-githits-service.js"; import { createMockGitHitsService, @@ -83,6 +86,54 @@ describe("RefreshingGitHitsService", () => { service.search({ query: "test", language: "js" }), ).rejects.toThrow(AuthenticationError); }); + + it("refreshes and retries once when terms are accepted but the JWT claim is stale", async () => { + const gatedService = createMockGitHitsService({ + search: mock(() => Promise.reject(new TermsAcceptanceRequiredError())), + }); + const successService = createMockGitHitsService({ + search: mock(() => Promise.resolve("result after terms refresh")), + }); + let callCount = 0; + const factory = mock(() => + callCount++ === 0 ? gatedService : successService, + ); + const tokenProvider = createMockTokenProvider(); + const service = new RefreshingGitHitsService( + API_URL, + tokenProvider, + factory, + ); + + expect(await service.search({ query: "test" })).toBe( + "result after terms refresh", + ); + expect(tokenProvider.forceRefresh).toHaveBeenCalledTimes(1); + expect(factory).toHaveBeenCalledTimes(2); + }); + + it("does not retry terms gating when a static token cannot refresh", async () => { + const gatedService = createMockGitHitsService({ + search: mock(() => Promise.reject(new TermsAcceptanceRequiredError())), + }); + const factory = mock(() => gatedService); + const forceRefresh = mock(() => Promise.resolve(undefined)); + const tokenProvider = createMockTokenProvider({ + getToken: mock(() => Promise.resolve("ghi-static-token")), + forceRefresh, + }); + const service = new RefreshingGitHitsService( + API_URL, + tokenProvider, + factory, + ); + + await expect(service.search({ query: "test" })).rejects.toBeInstanceOf( + TermsAcceptanceRequiredError, + ); + expect(factory).toHaveBeenCalledTimes(1); + expect(forceRefresh).not.toHaveBeenCalled(); + }); }); describe("getLanguages", () => { diff --git a/packages/core-internal/src/services/refreshing-githits-service.ts b/packages/core-internal/src/services/refreshing-githits-service.ts index d090e554..8e96d4f4 100644 --- a/packages/core-internal/src/services/refreshing-githits-service.ts +++ b/packages/core-internal/src/services/refreshing-githits-service.ts @@ -1,11 +1,11 @@ import { executeWithTokenRefresh } from "./execute-with-token-refresh.js"; import { - AuthenticationError, type FeedbackParams, type FeedbackResult, type GitHitsService, GitHitsServiceImpl, type GitHitsServiceRuntimeOptions, + isTokenRefreshableError, type Language, type SearchParams, } from "./githits-service.js"; @@ -57,7 +57,7 @@ export class RefreshingGitHitsService implements GitHitsService { return executeWithTokenRefresh({ getToken: () => this.tokenProvider.getToken(), forceRefresh: () => this.tokenProvider.forceRefresh(), - shouldRefresh: (error) => error instanceof AuthenticationError, + shouldRefresh: isTokenRefreshableError, executeWithToken: async (token) => { const service = this.serviceFactory ? this.serviceFactory(this.apiUrl, token) diff --git a/packages/core-internal/src/shared/pkgseer-graphql.test.ts b/packages/core-internal/src/shared/pkgseer-graphql.test.ts index e8a199d9..87e6f80d 100644 --- a/packages/core-internal/src/shared/pkgseer-graphql.test.ts +++ b/packages/core-internal/src/shared/pkgseer-graphql.test.ts @@ -5,6 +5,7 @@ import { postPkgseerGraphql, } from "./pkgseer-graphql.js"; import { createClientHeaderBuilder } from "./request-headers.js"; +import { TermsAcceptanceRequiredError } from "./terms-acceptance.js"; function makeResponse( body: string, @@ -66,6 +67,60 @@ describe("postPkgseerGraphql", () => { expect(result.parsedBody).toEqual(JSON.parse(VALID_JSON)); }); + it("centralizes HTTP terms-gate classification for all consumers", async () => { + const fetchFn = mock(() => + Promise.resolve( + makeResponse( + JSON.stringify({ + code: "TERMS_ACCEPTANCE_REQUIRED", + acceptance_url: "https://acceptance.example.test/settings/privacy", + }), + { status: 403 }, + ), + ), + ); + + await expect( + postPkgseerGraphql({ + endpointUrl: ENDPOINT, + token: TOKEN, + query: "query { x }", + variables: {}, + fetchFn: asFetchFn(fetchFn), + }), + ).rejects.toMatchObject({ + name: TermsAcceptanceRequiredError.name, + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }); + }); + + it("centralizes GraphQL terms-gate classification for all consumers", async () => { + const fetchFn = mock(() => + Promise.resolve( + makeResponse( + JSON.stringify({ + data: null, + errors: [ + { + extensions: { code: "TERMS_ACCEPTANCE_REQUIRED" }, + }, + ], + }), + ), + ), + ); + + await expect( + postPkgseerGraphql({ + endpointUrl: ENDPOINT, + token: TOKEN, + query: "query { x }", + variables: {}, + fetchFn: asFetchFn(fetchFn), + }), + ).rejects.toBeInstanceOf(TermsAcceptanceRequiredError); + }); + it("returns parsedBody: null for 200 + invalid JSON", async () => { const fetchFn = mock(() => Promise.resolve( diff --git a/packages/core-internal/src/shared/pkgseer-graphql.ts b/packages/core-internal/src/shared/pkgseer-graphql.ts index 1206a926..6b2ffb3b 100644 --- a/packages/core-internal/src/shared/pkgseer-graphql.ts +++ b/packages/core-internal/src/shared/pkgseer-graphql.ts @@ -6,8 +6,8 @@ * - Owns: URL trailing-slash normalisation, required headers * (`Authorization`, `Content-Type`, `User-Agent`), one POST attempt, * optional `fetchFn` injection, structured response shape, transport - * wrapping via {@link PkgseerTransportError}. - * - Does NOT own: token refresh, GraphQL-error classification, Zod + * wrapping via {@link PkgseerTransportError}, and the shared terms gate. + * - Does NOT own: token refresh, domain GraphQL-error classification, Zod * schema validation, HTTP status dispatch. Those live per-service * so that GraphQL-level `UNAUTHORIZED` (which we only learn *after* * the POST completes) can still trigger the service's @@ -16,8 +16,8 @@ * Return contract: * - On any HTTP response (2xx, 4xx, 5xx): returns * `{ status, responseBody, parsedBody }`. `parsedBody` is the - * JSON-parsed body when valid JSON, otherwise `null`. **Never - * throws for a completed response**, regardless of status. + * JSON-parsed body when valid JSON, otherwise `null`. Completed responses + * only throw when they carry the cross-service terms-acceptance contract. * - On fetch rejection (DNS, socket, abort): emits one * `pkg-graphql` debug line, then throws * {@link PkgseerTransportError} preserving the rejection `cause`. @@ -27,6 +27,7 @@ import { validateServiceUrl } from "../services/config.js"; import { debugLog } from "./debug-log.js"; import { DEFAULT_FETCH_TIMEOUT_MS, fetchWithTimeout } from "./fetch-timeout.js"; import type { ClientHeaderBuilder } from "./request-headers.js"; +import { throwIfTermsAcceptanceRequired } from "./terms-acceptance.js"; export interface PkgseerGraphqlRequest { /** Full base URL for the package/source service. Trailing slashes tolerated. */ @@ -128,6 +129,7 @@ export async function postPkgseerGraphql( const responseBody = await response.text().catch(() => ""); const parsedBody = parseJsonOrNull(responseBody); + throwIfTermsAcceptanceRequired(parsedBody); return { status: response.status, diff --git a/packages/core-internal/src/shared/terms-acceptance.test.ts b/packages/core-internal/src/shared/terms-acceptance.test.ts new file mode 100644 index 00000000..2af33a6b --- /dev/null +++ b/packages/core-internal/src/shared/terms-acceptance.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "bun:test"; +import { + createTermsAcceptanceError, + TermsAcceptanceRequiredError, + throwIfTermsAcceptanceRequired, +} from "./terms-acceptance.js"; + +describe("terms acceptance contract", () => { + it("parses a REST response with an environment-specific acceptance URL", () => { + expect( + createTermsAcceptanceError({ + code: "TERMS_ACCEPTANCE_REQUIRED", + terms_url: "https://githits.com/legal/terms-of-service/", + acceptance_url: "https://acceptance.example.test/settings/privacy", + }), + ).toMatchObject({ + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }); + }); + + it("parses the same contract from GraphQL extensions", () => { + expect(() => + throwIfTermsAcceptanceRequired({ + errors: [ + { + extensions: { code: "TERMS_ACCEPTANCE_REQUIRED" }, + }, + ], + }), + ).toThrow(TermsAcceptanceRequiredError); + }); + + it("ignores unrelated access errors", () => { + expect(() => + throwIfTermsAcceptanceRequired({ code: "FORBIDDEN" }), + ).not.toThrow(); + }); +}); diff --git a/packages/core-internal/src/shared/terms-acceptance.ts b/packages/core-internal/src/shared/terms-acceptance.ts new file mode 100644 index 00000000..ed8c19ae --- /dev/null +++ b/packages/core-internal/src/shared/terms-acceptance.ts @@ -0,0 +1,86 @@ +export const TERMS_ACCEPTANCE_REQUIRED_CODE = "TERMS_ACCEPTANCE_REQUIRED"; +export const TERMS_URL = "https://githits.com/legal/terms-of-service/"; +export const TERMS_ACCEPTANCE_URL = "https://app.githits.com/settings/privacy"; + +export interface TermsAcceptanceRemediation { + termsUrl?: string; + acceptanceUrl?: string; +} + +/** Stable cross-transport signal that the current terms must be accepted. */ +export class TermsAcceptanceRequiredError extends Error { + readonly code: typeof TERMS_ACCEPTANCE_REQUIRED_CODE = + TERMS_ACCEPTANCE_REQUIRED_CODE; + readonly termsUrl: string; + readonly acceptanceUrl: string; + + constructor(remediation: TermsAcceptanceRemediation = {}) { + super( + "Terms acceptance required. Run `githits settings terms accept`, then retry.", + ); + this.name = "TermsAcceptanceRequiredError"; + this.termsUrl = remediation.termsUrl ?? TERMS_URL; + this.acceptanceUrl = remediation.acceptanceUrl ?? TERMS_ACCEPTANCE_URL; + } +} + +/** Parse the canonical REST or GraphQL terms gate without trusting its message. */ +export function createTermsAcceptanceError( + payload: unknown, +): TermsAcceptanceRequiredError | undefined { + const record = parseErrorRecord(payload); + if (!record) return undefined; + const contract = + record.code === TERMS_ACCEPTANCE_REQUIRED_CODE + ? record + : firstGraphQLErrorExtensions(record); + if (contract?.code !== TERMS_ACCEPTANCE_REQUIRED_CODE) return undefined; + + return new TermsAcceptanceRequiredError({ + termsUrl: stringField(contract, "terms_url"), + acceptanceUrl: stringField(contract, "acceptance_url"), + }); +} + +/** Apply the shared terms gate once before transport-specific error mapping. */ +export function throwIfTermsAcceptanceRequired(payload: unknown): void { + const error = createTermsAcceptanceError(payload); + if (error) throw error; +} + +function parseErrorRecord( + payload: unknown, +): Record | undefined { + if (typeof payload === "string") { + try { + return parseErrorRecord(JSON.parse(payload)); + } catch { + return undefined; + } + } + return payload && typeof payload === "object" + ? (payload as Record) + : undefined; +} + +function firstGraphQLErrorExtensions( + record: Record, +): Record | undefined { + const firstError = Array.isArray(record.errors) + ? record.errors[0] + : undefined; + if (!firstError || typeof firstError !== "object") return undefined; + const extensions = (firstError as Record).extensions; + return extensions && typeof extensions === "object" + ? (extensions as Record) + : undefined; +} + +function stringField( + record: Record, + field: string, +): string | undefined { + return typeof record[field] === "string" + ? (record[field] as string) + : undefined; +} diff --git a/packages/mcp/src/shared/code-navigation-error-map.test.ts b/packages/mcp/src/shared/code-navigation-error-map.test.ts index 5d820c34..dd5c880e 100644 --- a/packages/mcp/src/shared/code-navigation-error-map.test.ts +++ b/packages/mcp/src/shared/code-navigation-error-map.test.ts @@ -15,6 +15,7 @@ import { CodeNavigationValidationError, CodeNavigationVersionNotFoundError, MalformedCodeNavigationResponseError, + TermsAcceptanceRequiredError, } from "@githits/core-internal"; import { mapCodeNavigationError } from "./code-navigation-error-map.js"; @@ -33,6 +34,19 @@ class UnsupportedRegistryError extends Error { } describe("mapCodeNavigationError", () => { + it("maps terms gating with stable command and URL remediation", () => { + expect( + mapCodeNavigationError(new TermsAcceptanceRequiredError()), + ).toMatchObject({ + code: "TERMS_ACCEPTANCE_REQUIRED", + retryable: false, + details: { + action: "githits settings terms accept", + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://app.githits.com/settings/privacy", + }, + }); + }); it("classifies ClientUpdateRequiredError as UPDATE_REQUIRED", () => { expect( mapCodeNavigationError( diff --git a/packages/mcp/src/shared/code-navigation-error-map.ts b/packages/mcp/src/shared/code-navigation-error-map.ts index 7f230c12..f66cb882 100644 --- a/packages/mcp/src/shared/code-navigation-error-map.ts +++ b/packages/mcp/src/shared/code-navigation-error-map.ts @@ -23,6 +23,7 @@ import { MalformedCodeNavigationResponseError, type SuggestedRef, type TargetResolution, + TermsAcceptanceRequiredError, } from "@githits/core-internal"; import { AuthRequiredError } from "./require-auth.js"; @@ -35,6 +36,7 @@ export type MappedErrorCode = | "UNRESOLVABLE" | "ACCESS_DENIED" | "AUTH_REQUIRED" + | "TERMS_ACCEPTANCE_REQUIRED" | "NETWORK" | "INVALID_ARGUMENT" | "BACKEND_ERROR" @@ -81,6 +83,10 @@ export interface MappedErrorDetails { reason?: string; /** Whether auth failed before making a request or after backend rejection. */ authSource?: AuthenticationErrorSource; + /** Canonical legal document URL for terms-acceptance remediation. */ + termsUrl?: string; + /** Authenticated web UI where the user can accept the current terms. */ + acceptanceUrl?: string; } export interface MappedError { @@ -123,7 +129,25 @@ export function mapCodeNavigationError(error: unknown): MappedError { return mapped; } +export function mapTermsAcceptanceError( + error: unknown, +): MappedError | undefined { + if (!(error instanceof TermsAcceptanceRequiredError)) return undefined; + return { + code: "TERMS_ACCEPTANCE_REQUIRED", + message: error.message, + retryable: false, + details: { + action: "githits settings terms accept", + termsUrl: error.termsUrl, + acceptanceUrl: error.acceptanceUrl, + }, + }; +} + function classify(error: unknown): MappedError { + const termsError = mapTermsAcceptanceError(error); + if (termsError) return termsError; if (error instanceof ClientUpdateRequiredError) { return buildUpdateRequiredError(error.reason, error.currentVersion); } diff --git a/packages/mcp/src/shared/githits-service-error-map.test.ts b/packages/mcp/src/shared/githits-service-error-map.test.ts index de55257e..b7dd96c3 100644 --- a/packages/mcp/src/shared/githits-service-error-map.test.ts +++ b/packages/mcp/src/shared/githits-service-error-map.test.ts @@ -3,10 +3,26 @@ import { ApiRateLimitError, AuthenticationError, FetchTimeoutError, + TermsAcceptanceRequiredError, } from "@githits/core-internal"; import { mapGitHitsServiceError } from "./githits-service-error-map.js"; describe("mapGitHitsServiceError", () => { + it("preserves stable terms acceptance remediation", () => { + expect( + mapGitHitsServiceError("search", new TermsAcceptanceRequiredError()), + ).toEqual({ + code: "TERMS_ACCEPTANCE_REQUIRED", + message: + "Terms acceptance required. Run `githits settings terms accept`, then retry.", + retryable: false, + details: { + action: "githits settings terms accept", + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://app.githits.com/settings/privacy", + }, + }); + }); it("maps authentication errors without changing their message or source", () => { const mapped = mapGitHitsServiceError( "perform request", diff --git a/packages/mcp/src/shared/githits-service-error-map.ts b/packages/mcp/src/shared/githits-service-error-map.ts index 44dad9f0..a9bcaa06 100644 --- a/packages/mcp/src/shared/githits-service-error-map.ts +++ b/packages/mcp/src/shared/githits-service-error-map.ts @@ -4,6 +4,7 @@ import { FetchTimeoutError, } from "@githits/core-internal"; import type { MappedError } from "./code-navigation-error-map.js"; +import { mapTermsAcceptanceError } from "./code-navigation-error-map.js"; /** * Classify errors from the GitHits API service into the shared envelope used @@ -13,6 +14,8 @@ export function mapGitHitsServiceError( operation: string, error: unknown, ): MappedError { + const termsError = mapTermsAcceptanceError(error); + if (termsError) return termsError; if (error instanceof AuthenticationError) { return { code: "AUTH_REQUIRED", diff --git a/packages/mcp/src/shared/package-intelligence-error-map.test.ts b/packages/mcp/src/shared/package-intelligence-error-map.test.ts index 33df9aef..0e896f5a 100644 --- a/packages/mcp/src/shared/package-intelligence-error-map.test.ts +++ b/packages/mcp/src/shared/package-intelligence-error-map.test.ts @@ -11,10 +11,24 @@ import { PackageIntelligenceTargetNotFoundError, PackageIntelligenceValidationError, PackageIntelligenceVersionNotFoundError, + TermsAcceptanceRequiredError, } from "@githits/core-internal"; import { mapPackageIntelligenceError } from "./package-intelligence-error-map.js"; describe("mapPackageIntelligenceError", () => { + it("maps terms gating with stable command and URL remediation", () => { + expect( + mapPackageIntelligenceError(new TermsAcceptanceRequiredError()), + ).toMatchObject({ + code: "TERMS_ACCEPTANCE_REQUIRED", + retryable: false, + details: { + action: "githits settings terms accept", + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://app.githits.com/settings/privacy", + }, + }); + }); it("maps ClientUpdateRequiredError to UPDATE_REQUIRED", () => { const mapped = mapPackageIntelligenceError( new ClientUpdateRequiredError(undefined, undefined, "0.2.0"), diff --git a/packages/mcp/src/shared/package-intelligence-error-map.ts b/packages/mcp/src/shared/package-intelligence-error-map.ts index abff3562..28d394f2 100644 --- a/packages/mcp/src/shared/package-intelligence-error-map.ts +++ b/packages/mcp/src/shared/package-intelligence-error-map.ts @@ -28,7 +28,10 @@ import type { MappedErrorCode, MappedErrorDetails, } from "./code-navigation-error-map.js"; -import { buildUpdateRequiredError } from "./code-navigation-error-map.js"; +import { + buildUpdateRequiredError, + mapTermsAcceptanceError, +} from "./code-navigation-error-map.js"; import { AuthRequiredError } from "./require-auth.js"; // Re-export for caller convenience — callers of @@ -54,6 +57,8 @@ export function mapPackageIntelligenceError(error: unknown): MappedError { } function classify(error: unknown): MappedError { + const termsError = mapTermsAcceptanceError(error); + if (termsError) return termsError; if (error instanceof ClientUpdateRequiredError) { return buildUpdateRequiredError(error.reason, error.currentVersion); } diff --git a/scripts/cli-smoke.ts b/scripts/cli-smoke.ts index 73888b00..6416d0e4 100644 --- a/scripts/cli-smoke.ts +++ b/scripts/cli-smoke.ts @@ -54,6 +54,7 @@ export const EXPECTED_TOP_LEVEL_COMMANDS = [ "languages", "feedback", "doctor", + "settings", "search", "search-status", "code", diff --git a/src/cli.ts b/src/cli.ts index 59702c88..6ce5441d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -29,6 +29,7 @@ import { registerLogoutCommand, registerMcpCommand, registerPkgCommandGroup, + registerSettingsCommand, registerUnifiedSearchCommands, } from "./commands/index.js"; import { loginFlow, stderrLoginOutput } from "./commands/login.js"; @@ -133,6 +134,7 @@ Support: support@githits.com`, registerLanguagesCommand(program); registerFeedbackCommand(program); registerDoctorCommand(program); + registerSettingsCommand(program); const registrationArgv = stripRootRegistrationOptions(argv); const updateCheckTask = startUpdateCheckTaskForInvocation({ args: argv, diff --git a/src/commands/doctor.test.ts b/src/commands/doctor.test.ts index 9289c244..64bbaedf 100644 --- a/src/commands/doctor.test.ts +++ b/src/commands/doctor.test.ts @@ -101,6 +101,7 @@ describe("doctor", () => { HOME: "/home/test", GITHITS_MCP_URL: "https://mcp.example.test", GITHITS_API_URL: "https://api.example.test", + GITHITS_ACCOUNTS_URL: "https://accounts.example.test", GITHITS_CODE_NAV_URL: "https://code.example.test", }, }), @@ -114,6 +115,10 @@ describe("doctor", () => { source: "env", value: "https://api.example.test", }); + expect(report.services.accountsUrl).toEqual({ + source: "env", + value: "https://accounts.example.test", + }); expect(report.services.codeNavigationUrl).toEqual({ source: "env", value: "https://code.example.test", diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 5a18a5d2..9eda519f 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -29,6 +29,7 @@ import { type FileSystemService, FileSystemServiceImpl, } from "../services/filesystem-service.js"; +import { DEFAULT_ACCOUNTS_URL } from "../services/settings-service.js"; type ProbeStatus = | "present" @@ -104,6 +105,7 @@ export interface DoctorReport { services: { mcpUrl: ServiceProbe; apiUrl: ServiceProbe; + accountsUrl: ServiceProbe; codeNavigationUrl: ServiceProbe; }; config: { @@ -283,6 +285,7 @@ function buildServicesReport(env: NodeJS.ProcessEnv): DoctorReport["services"] { return { mcpUrl: serviceProbe(env.GITHITS_MCP_URL, DEFAULT_MCP_URL), apiUrl: serviceProbe(env.GITHITS_API_URL, DEFAULT_API_URL), + accountsUrl: serviceProbe(env.GITHITS_ACCOUNTS_URL, DEFAULT_ACCOUNTS_URL), codeNavigationUrl: serviceProbe( env.GITHITS_CODE_NAV_URL ?? env.PKGSEER_URL, DEFAULT_CODE_NAV_URL, @@ -663,6 +666,9 @@ function formatDoctorReport(report: DoctorReport): string { lines.push("Services:"); lines.push(` MCP URL: ${formatServiceProbe(report.services.mcpUrl)}`); lines.push(` API URL: ${formatServiceProbe(report.services.apiUrl)}`); + lines.push( + ` Accounts URL: ${formatServiceProbe(report.services.accountsUrl)}`, + ); lines.push( ` Code navigation URL: ${formatServiceProbe(report.services.codeNavigationUrl)}`, "", diff --git a/src/commands/feedback.test.ts b/src/commands/feedback.test.ts index 8a879780..ad7d487e 100644 --- a/src/commands/feedback.test.ts +++ b/src/commands/feedback.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it, mock, spyOn } from "bun:test"; -import { AuthenticationError } from "@githits/core-internal"; +import { + AuthenticationError, + TermsAcceptanceRequiredError, +} from "@githits/core-internal"; import { AuthRequiredError } from "@githits/mcp/internal"; import { createMockGitHitsService } from "../services/test-helpers.js"; import { type FeedbackDependencies, feedbackAction } from "./feedback.js"; @@ -305,6 +308,41 @@ describe("feedbackAction", () => { exitSpy.mockRestore(); }); + it("preserves terms remediation in the JSON error envelope", async () => { + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + const deps = createDeps({ + githitsService: createMockGitHitsService({ + submitFeedback: mock(() => + Promise.reject( + new TermsAcceptanceRequiredError({ + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }), + ), + ), + }), + }); + + await expect( + feedbackAction("abc-123", { accept: true, json: true }, deps), + ).rejects.toThrow("process.exit"); + + expect(JSON.parse(String(errorSpy.mock.calls[0]?.[0]))).toMatchObject({ + code: "TERMS_ACCEPTANCE_REQUIRED", + details: { + action: "githits settings terms accept", + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }, + }); + expect(exitSpy).toHaveBeenCalledWith(1); + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); + it("emits JSON auth envelope when service returns 401 in JSON mode", async () => { const errorSpy = spyOn(console, "error").mockImplementation(() => {}); const exitSpy = spyOn(process, "exit").mockImplementation(() => { diff --git a/src/commands/feedback.ts b/src/commands/feedback.ts index 5aa89bc5..6f48cf06 100644 --- a/src/commands/feedback.ts +++ b/src/commands/feedback.ts @@ -1,17 +1,13 @@ import type { GitHitsService } from "@githits/core-internal"; -import { AuthenticationError } from "@githits/core-internal"; import { AuthRequiredError, buildAuthRequiredErrorPayload, + mapGitHitsServiceError, requireAuth, } from "@githits/mcp/internal"; import { type Command, Option } from "commander"; import { createContainer } from "../container.js"; -import { - buildCliMappedErrorPayload, - formatCliMappedError, - formatMappedErrorForTerminal, -} from "./format-mapped-error.js"; +import { formatCliMappedError } from "./format-mapped-error.js"; export interface FeedbackOptions { accept?: boolean; @@ -82,30 +78,8 @@ export async function feedbackAction( console.log(result.message); } } catch (error) { - if (error instanceof AuthenticationError) { - const mapped = { - code: "AUTH_REQUIRED" as const, - message: error.message, - retryable: false, - details: { authSource: error.source }, - }; - if (options.json) { - console.error(JSON.stringify(buildCliMappedErrorPayload(mapped))); - } else { - console.error(formatMappedErrorForTerminal(mapped)); - } - process.exit(1); - } - console.error( - formatCliMappedError( - { - code: "UNKNOWN", - message: `Failed to submit feedback: ${error instanceof Error ? error.message : "Unexpected error."}`, - retryable: false, - }, - options.json ?? false, - ), - ); + const mapped = mapGitHitsServiceError("submit feedback", error); + console.error(formatCliMappedError(mapped, options.json ?? false)); process.exit(1); } } diff --git a/src/commands/index.ts b/src/commands/index.ts index a56b7352..ddf27cd4 100644 --- a/src/commands/index.ts +++ b/src/commands/index.ts @@ -62,3 +62,15 @@ export { searchAction, searchStatusAction, } from "./search.js"; +export { + registerSettingsCommand, + type SettingsCommandDependencies, + type SettingsOptions, + type SettingsTermsAcceptOptions, + settingsAction, + settingsClearAction, + settingsGetAction, + settingsSetAction, + settingsTermsAcceptAction, + settingsTermsAction, +} from "./settings.js"; diff --git a/src/commands/languages.test.ts b/src/commands/languages.test.ts index a64dffc7..242d6450 100644 --- a/src/commands/languages.test.ts +++ b/src/commands/languages.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it, mock, spyOn } from "bun:test"; -import { AuthenticationError } from "@githits/core-internal"; +import { + AuthenticationError, + TermsAcceptanceRequiredError, +} from "@githits/core-internal"; import { AuthRequiredError } from "@githits/mcp/internal"; import { createMockGitHitsService } from "../services/test-helpers.js"; import { type LanguagesDependencies, languagesAction } from "./languages.js"; @@ -186,6 +189,44 @@ describe("languagesAction", () => { exitSpy.mockRestore(); }); + it("preserves terms remediation in the JSON error envelope", async () => { + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + const deps = createDeps({ + githitsService: createMockGitHitsService({ + getLanguages: mock(() => + Promise.reject( + new TermsAcceptanceRequiredError({ + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }), + ), + ), + }), + }); + + await expect( + languagesAction(undefined, { json: true }, deps), + ).rejects.toThrow("process.exit"); + + expect(JSON.parse(String(errorSpy.mock.calls[0]?.[0]))).toEqual({ + error: + "Terms acceptance required. Run `githits settings terms accept`, then retry.", + code: "TERMS_ACCEPTANCE_REQUIRED", + retryable: false, + details: { + action: "githits settings terms accept", + termsUrl: "https://githits.com/legal/terms-of-service/", + acceptanceUrl: "https://acceptance.example.test/settings/privacy", + }, + }); + expect(exitSpy).toHaveBeenCalledWith(1); + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); + it("emits JSON auth envelope when service returns 401 in JSON mode", async () => { const errorSpy = spyOn(console, "error").mockImplementation(() => {}); const exitSpy = spyOn(process, "exit").mockImplementation(() => { diff --git a/src/commands/languages.ts b/src/commands/languages.ts index 884f8737..249e83c8 100644 --- a/src/commands/languages.ts +++ b/src/commands/languages.ts @@ -1,21 +1,17 @@ import type { GitHitsService } from "@githits/core-internal"; -import { AuthenticationError } from "@githits/core-internal"; import { AuthRequiredError, buildAuthRequiredErrorPayload, colorize, dim, type LanguageMatch, + mapGitHitsServiceError, requireAuth, shouldUseColors, } from "@githits/mcp/internal"; import type { Command } from "commander"; import { createContainer } from "../container.js"; -import { - buildCliMappedErrorPayload, - formatCliMappedError, - formatMappedErrorForTerminal, -} from "./format-mapped-error.js"; +import { formatCliMappedError } from "./format-mapped-error.js"; export interface LanguagesOptions { json?: boolean; @@ -75,30 +71,8 @@ export async function languagesAction( } } } catch (error) { - if (error instanceof AuthenticationError) { - const mapped = { - code: "AUTH_REQUIRED" as const, - message: error.message, - retryable: false, - details: { authSource: error.source }, - }; - if (options.json) { - console.error(JSON.stringify(buildCliMappedErrorPayload(mapped))); - } else { - console.error(formatMappedErrorForTerminal(mapped)); - } - process.exit(1); - } - console.error( - formatCliMappedError( - { - code: "UNKNOWN", - message: `Failed to list languages: ${error instanceof Error ? error.message : "Unexpected error."}`, - retryable: false, - }, - options.json ?? false, - ), - ); + const mapped = mapGitHitsServiceError("list languages", error); + console.error(formatCliMappedError(mapped, options.json ?? false)); process.exit(1); } } diff --git a/src/commands/settings-schema.test.ts b/src/commands/settings-schema.test.ts new file mode 100644 index 00000000..e637da27 --- /dev/null +++ b/src/commands/settings-schema.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from "bun:test"; +import type { Settings } from "../services/settings-service.js"; +import { + buildSettingsClearPatch, + buildSettingsPatch, + getSettingValue, + parseClearableSettingsKey, + parseSettingsKey, +} from "./settings-schema.js"; + +const FIRST_UUID = "0198a7d0-6750-7ace-a68c-418062117d95"; +const SECOND_UUID = "0198a7d0-6750-7ace-a68c-418062117d96"; + +const SETTINGS: Settings = { + user_id: FIRST_UUID, + default_language_id: null, + license_mode: "safe", + blocked_license_ids: [SECOND_UUID], + marketing_email_opted_out: true, + example_generation_limit: 25, + terms_required: false, +}; + +describe("settings CLI schema", () => { + it("maps friendly scalar values to storage fields", () => { + expect(buildSettingsPatch("license-mode", ["custom"])).toEqual({ + license_mode: "custom", + }); + expect(buildSettingsPatch("marketing-emails", ["disabled"])).toEqual({ + marketing_email_opted_out: true, + }); + expect(buildSettingsPatch("marketing-emails", ["enabled"])).toEqual({ + marketing_email_opted_out: false, + }); + }); + + it("treats blocked license IDs as an atomic list replacement", () => { + expect( + buildSettingsPatch("blocked-license-ids", [FIRST_UUID, SECOND_UUID]), + ).toEqual({ blocked_license_ids: [FIRST_UUID, SECOND_UUID] }); + }); + + it("uses clear for nullable and empty-list values", () => { + expect(buildSettingsClearPatch("default-language-id")).toEqual({ + default_language_id: null, + }); + expect(buildSettingsClearPatch("blocked-license-ids")).toEqual({ + blocked_license_ids: [], + }); + }); + + it("rejects unknown keys and invalid values before an API call", () => { + expect(() => parseSettingsKey("marketing-email-opted-out")).toThrow( + "Unknown setting", + ); + expect(() => buildSettingsPatch("license-mode", ["unsafe"])).toThrow( + "Expected 'safe', 'yolo', or 'custom'", + ); + expect(() => buildSettingsPatch("blocked-license-ids", [])).toThrow( + "settings clear blocked-license-ids", + ); + expect(() => parseClearableSettingsKey("marketing-emails")).toThrow( + "cannot be cleared", + ); + }); + + it("presents negative storage booleans as positive domain values", () => { + expect(getSettingValue(SETTINGS, "marketing-emails")).toBe("disabled"); + expect(getSettingValue(SETTINGS, "blocked-license-ids")).toEqual([ + SECOND_UUID, + ]); + }); +}); diff --git a/src/commands/settings-schema.ts b/src/commands/settings-schema.ts new file mode 100644 index 00000000..7274f891 --- /dev/null +++ b/src/commands/settings-schema.ts @@ -0,0 +1,139 @@ +import { InvalidArgumentError } from "commander"; +import { z } from "zod"; +import type { Settings, SettingsPatch } from "../services/settings-service.js"; + +export const SETTINGS_KEYS = [ + "default-language-id", + "license-mode", + "blocked-license-ids", + "marketing-emails", +] as const; +export const CLEARABLE_SETTINGS_KEYS = [ + "default-language-id", + "blocked-license-ids", +] as const; + +export type SettingsKey = (typeof SETTINGS_KEYS)[number]; +export type ClearableSettingsKey = (typeof CLEARABLE_SETTINGS_KEYS)[number]; +export type SettingValue = string | string[] | null; + +const SETTINGS_KEY_SCHEMA = z.enum(SETTINGS_KEYS); +const CLEARABLE_SETTINGS_KEY_SCHEMA = z.enum(CLEARABLE_SETTINGS_KEYS); +const UUID_SCHEMA = z.uuid(); +const LICENSE_MODE_SCHEMA = z.enum(["safe", "yolo", "custom"]); +const MARKETING_EMAILS_SCHEMA = z.enum(["enabled", "disabled"]); + +/** Validate a public CLI setting name before constructing command dependencies. */ +export function parseSettingsKey(value: string): SettingsKey { + const parsed = SETTINGS_KEY_SCHEMA.safeParse(value); + if (!parsed.success) { + throw new InvalidArgumentError( + `Unknown setting '${value}'. Expected one of: ${SETTINGS_KEYS.join(", ")}.`, + ); + } + return parsed.data; +} + +/** Validate a setting name for the narrower clear operation. */ +export function parseClearableSettingsKey(value: string): ClearableSettingsKey { + const parsed = CLEARABLE_SETTINGS_KEY_SCHEMA.safeParse(value); + if (!parsed.success) { + throw new InvalidArgumentError( + `Setting '${value}' cannot be cleared. Expected one of: ${CLEARABLE_SETTINGS_KEYS.join(", ")}.`, + ); + } + return parsed.data; +} + +/** Convert one typed CLI setting update into the selective account API PATCH. */ +export function buildSettingsPatch( + key: SettingsKey, + values: string[], +): SettingsPatch { + switch (key) { + case "default-language-id": + return { default_language_id: parseUuid(singleValue(key, values)) }; + case "license-mode": + return { license_mode: parseLicenseMode(singleValue(key, values)) }; + case "blocked-license-ids": + return { blocked_license_ids: parseUuidList(key, values) }; + case "marketing-emails": + return { + marketing_email_opted_out: + parseMarketingEmails(singleValue(key, values)) === "disabled", + }; + } +} + +/** Build the explicit empty/default PATCH for settings that support clearing. */ +export function buildSettingsClearPatch( + key: ClearableSettingsKey, +): SettingsPatch { + switch (key) { + case "default-language-id": + return { default_language_id: null }; + case "blocked-license-ids": + return { blocked_license_ids: [] }; + } +} + +/** Read a storage-neutral value for the public CLI setting name. */ +export function getSettingValue( + settings: Settings, + key: SettingsKey, +): SettingValue { + switch (key) { + case "default-language-id": + return settings.default_language_id; + case "license-mode": + return settings.license_mode; + case "blocked-license-ids": + return settings.blocked_license_ids; + case "marketing-emails": + return settings.marketing_email_opted_out ? "disabled" : "enabled"; + } +} + +function singleValue(key: SettingsKey, values: string[]): string { + if (values.length !== 1) { + throw new InvalidArgumentError( + `Setting '${key}' expects exactly one value.`, + ); + } + return values[0] ?? ""; +} + +function parseUuid(value: string): string { + if (!UUID_SCHEMA.safeParse(value).success) { + throw new InvalidArgumentError("Expected a UUID."); + } + return value; +} + +function parseUuidList(key: SettingsKey, values: string[]): string[] { + if (values.length === 0) { + throw new InvalidArgumentError( + `Setting '${key}' expects at least one UUID; use \`githits settings clear ${key}\` for an empty list.`, + ); + } + if (values.some((value) => !UUID_SCHEMA.safeParse(value).success)) { + throw new InvalidArgumentError("Expected one or more UUIDs."); + } + return values; +} + +function parseLicenseMode(value: string): "safe" | "yolo" | "custom" { + const parsed = LICENSE_MODE_SCHEMA.safeParse(value); + if (!parsed.success) { + throw new InvalidArgumentError("Expected 'safe', 'yolo', or 'custom'."); + } + return parsed.data; +} + +function parseMarketingEmails(value: string): "enabled" | "disabled" { + const parsed = MARKETING_EMAILS_SCHEMA.safeParse(value); + if (!parsed.success) { + throw new InvalidArgumentError("Expected 'enabled' or 'disabled'."); + } + return parsed.data; +} diff --git a/src/commands/settings.test.ts b/src/commands/settings.test.ts new file mode 100644 index 00000000..3c6e3f79 --- /dev/null +++ b/src/commands/settings.test.ts @@ -0,0 +1,298 @@ +import { describe, expect, it, mock, spyOn } from "bun:test"; +import { Command } from "commander"; +import type { + Settings, + SettingsService, +} from "../services/settings-service.js"; +import { + createMockPromptService, + createMockTokenProvider, +} from "../services/test-helpers.js"; +import { + registerSettingsCommand, + type SettingsCommandDependencies, + settingsAction, + settingsClearAction, + settingsGetAction, + settingsSetAction, + settingsTermsAcceptAction, + settingsTermsAction, +} from "./settings.js"; + +const SETTINGS: Settings = { + user_id: "0198a7d0-6750-7ace-a68c-418062117d95", + default_language_id: null, + license_mode: "safe", + blocked_license_ids: [], + marketing_email_opted_out: false, + example_generation_limit: 25, + terms_required: true, +}; + +function createSettingsService( + overrides: Partial = {}, +): SettingsService { + return { + getSettings: mock(() => Promise.resolve(SETTINGS)), + updateSettings: mock(() => Promise.resolve(SETTINGS)), + acceptTerms: mock(() => + Promise.resolve({ ...SETTINGS, terms_required: false }), + ), + ...overrides, + }; +} + +function createDeps( + overrides: Partial = {}, +): SettingsCommandDependencies { + return { + settingsService: createSettingsService(), + tokenProvider: createMockTokenProvider(), + promptService: createMockPromptService(), + hasValidToken: true, + mcpUrl: "https://mcp.githits.com", + staticApiToken: false, + stdinIsTTY: true, + stdoutIsTTY: true, + ...overrides, + }; +} + +describe("settings commands", () => { + it("prints the canonical object in overview JSON mode", async () => { + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsAction({ json: true }, createDeps()); + + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual(SETTINGS); + output.mockRestore(); + }); + + it("groups overview text into preferences, privacy, and account limits", async () => { + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsAction({}, createDeps()); + + const text = String(output.mock.calls[0]?.[0]); + expect(text).toContain("Preferences"); + expect(text).toContain("Privacy and terms"); + expect(text).toContain("Account limits"); + expect(text).toContain("Acceptance required"); + output.mockRestore(); + }); + + it("PATCHes one typed setting without exposing its storage field", async () => { + const updateSettings = mock(() => Promise.resolve(SETTINGS)); + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsSetAction( + "marketing-emails", + ["enabled"], + { json: true }, + createDeps({ + settingsService: createSettingsService({ updateSettings }), + }), + ); + + expect(updateSettings).toHaveBeenCalledWith({ + marketing_email_opted_out: false, + }); + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual(SETTINGS); + output.mockRestore(); + }); + + it("gets one setting using its public value", async () => { + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsGetAction("marketing-emails", { json: true }, createDeps()); + + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual({ + key: "marketing-emails", + value: "enabled", + }); + output.mockRestore(); + }); + + it("clears a list with an explicit empty-list PATCH", async () => { + const updateSettings = mock(() => Promise.resolve(SETTINGS)); + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsClearAction( + "blocked-license-ids", + {}, + createDeps({ + settingsService: createSettingsService({ updateSettings }), + }), + ); + + expect(updateSettings).toHaveBeenCalledWith({ blocked_license_ids: [] }); + output.mockRestore(); + }); + + it("registers key/value commands without per-setting flags", () => { + const program = new Command(); + registerSettingsCommand(program); + + const settings = program.commands.find( + (command) => command.name() === "settings", + ); + expect(settings?.commands.map((command) => command.name())).toEqual([ + "show", + "get", + "set", + "clear", + "terms", + ]); + + const setHelp = settings?.commands + .find((command) => command.name() === "set") + ?.helpInformation(); + expect(setHelp).toContain("set [options] "); + expect(setHelp).not.toContain("--license-mode"); + expect(setHelp).not.toContain("--marketing-email-opted-out"); + }); + + it.each([ + ["settings show --json", ["settings", "show", "--json"], SETTINGS], + ["settings --json show", ["settings", "--json", "show"], SETTINGS], + [ + "settings terms --json", + ["settings", "terms", "--json"], + { terms_required: true }, + ], + ])("honors JSON mode for %s", async (_name, args, expected) => { + const program = new Command(); + const output = spyOn(console, "log").mockImplementation(() => {}); + registerSettingsCommand(program, async () => createDeps()); + + await program.parseAsync(["node", "githits", ...args]); + + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual(expected); + output.mockRestore(); + }); + + it("guards every settings operation before account API access", async () => { + const getSettings = mock(() => Promise.resolve(SETTINGS)); + const updateSettings = mock(() => Promise.resolve(SETTINGS)); + const acceptTerms = mock(() => Promise.resolve(SETTINGS)); + const deps = createDeps({ + hasValidToken: false, + settingsService: createSettingsService({ + getSettings, + updateSettings, + acceptTerms, + }), + }); + + const operations = [ + () => settingsAction({ json: true }, deps), + () => settingsGetAction("license-mode", { json: true }, deps), + () => settingsSetAction("license-mode", ["safe"], { json: true }, deps), + () => settingsClearAction("blocked-license-ids", { json: true }, deps), + () => settingsTermsAction({ json: true }, deps), + () => settingsTermsAcceptAction({ yes: true, json: true }, deps), + ]; + + for (const operation of operations) { + await expect(operation()).rejects.toThrow( + "No local GitHits authentication token found", + ); + } + expect(getSettings).not.toHaveBeenCalled(); + expect(updateSettings).not.toHaveBeenCalled(); + expect(acceptTerms).not.toHaveBeenCalled(); + }); + + it("prints only stable terms fields in terms JSON mode", async () => { + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsTermsAction({ json: true }, createDeps()); + + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual({ + terms_required: true, + }); + output.mockRestore(); + }); + + it("confirms acceptance and force-refreshes an OAuth JWT", async () => { + const acceptTerms = mock(() => + Promise.resolve({ ...SETTINGS, terms_required: false }), + ); + const forceRefresh = mock(() => Promise.resolve("refreshed-jwt")); + const confirm = mock(() => Promise.resolve(true)); + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsTermsAcceptAction( + { json: true }, + createDeps({ + settingsService: createSettingsService({ acceptTerms }), + tokenProvider: createMockTokenProvider({ forceRefresh }), + promptService: createMockPromptService({ confirm }), + }), + ); + + expect(confirm).toHaveBeenCalled(); + expect(acceptTerms).toHaveBeenCalledTimes(1); + expect(forceRefresh).toHaveBeenCalledTimes(1); + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toEqual({ + accepted: true, + token_refreshed: true, + settings: { ...SETTINGS, terms_required: false }, + }); + output.mockRestore(); + }); + + it("--yes skips confirmation", async () => { + const confirm = mock(() => Promise.resolve(false)); + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsTermsAcceptAction( + { yes: true }, + createDeps({ promptService: createMockPromptService({ confirm }) }), + ); + + expect(confirm).not.toHaveBeenCalled(); + output.mockRestore(); + }); + + it("never refreshes a static ghi-* API token", async () => { + const forceRefresh = mock(() => Promise.resolve(undefined)); + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsTermsAcceptAction( + { yes: true, json: true }, + createDeps({ + staticApiToken: true, + tokenProvider: createMockTokenProvider({ forceRefresh }), + }), + ); + + expect(forceRefresh).not.toHaveBeenCalled(); + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toMatchObject({ + accepted: true, + token_refreshed: null, + }); + output.mockRestore(); + }); + + it("reports saved acceptance when OAuth refresh fails", async () => { + const output = spyOn(console, "log").mockImplementation(() => {}); + + await settingsTermsAcceptAction( + { yes: true, json: true }, + createDeps({ + tokenProvider: createMockTokenProvider({ + forceRefresh: mock(() => Promise.reject(new Error("offline"))), + }), + }), + ); + + expect(JSON.parse(String(output.mock.calls[0]?.[0]))).toMatchObject({ + accepted: true, + token_refreshed: false, + warning: + "Terms acceptance was saved, but authentication refresh failed. Run `githits login --force` before retrying other commands.", + }); + output.mockRestore(); + }); +}); diff --git a/src/commands/settings.ts b/src/commands/settings.ts new file mode 100644 index 00000000..636f2348 --- /dev/null +++ b/src/commands/settings.ts @@ -0,0 +1,385 @@ +import { TERMS_URL, type TokenProvider } from "@githits/core-internal"; +import { requireAuth } from "@githits/mcp/internal"; +import type { Command } from "commander"; +import { createContainer } from "../container.js"; +import type { PromptService } from "../services/prompt-service.js"; +import { PromptServiceImpl } from "../services/prompt-service.js"; +import { createCliFetch } from "../services/proxy-fetch.js"; +import { + getAccountsUrl, + type Settings, + type SettingsService, + SettingsServiceImpl, +} from "../services/settings-service.js"; +import { + buildSettingsClearPatch, + buildSettingsPatch, + CLEARABLE_SETTINGS_KEYS, + type ClearableSettingsKey, + getSettingValue, + parseClearableSettingsKey, + parseSettingsKey, + SETTINGS_KEYS, + type SettingsKey, + type SettingValue, +} from "./settings-schema.js"; + +const REFRESH_FAILURE_WARNING = + "Terms acceptance was saved, but authentication refresh failed. Run `githits login --force` before retrying other commands."; + +export interface SettingsOptions { + json?: boolean; +} + +export interface SettingsTermsAcceptOptions extends SettingsOptions { + yes?: boolean; +} + +export interface SettingsCommandDependencies { + settingsService: SettingsService; + tokenProvider: TokenProvider; + promptService: PromptService; + hasValidToken: boolean; + mcpUrl: string; + staticApiToken: boolean; + stdinIsTTY: boolean; + stdoutIsTTY: boolean; +} + +export type SettingsDependenciesFactory = + () => Promise; + +export async function settingsAction( + options: SettingsOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + const settings = await deps.settingsService.getSettings(); + printSettings(settings, options.json ?? false); +} + +export async function settingsSetAction( + key: SettingsKey, + values: string[], + options: SettingsOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + const settings = await deps.settingsService.updateSettings( + buildSettingsPatch(key, values), + ); + printSettings(settings, options.json ?? false); +} + +export async function settingsGetAction( + key: SettingsKey, + options: SettingsOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + const settings = await deps.settingsService.getSettings(); + printSetting(key, getSettingValue(settings, key), options.json ?? false); +} + +export async function settingsClearAction( + key: ClearableSettingsKey, + options: SettingsOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + const settings = await deps.settingsService.updateSettings( + buildSettingsClearPatch(key), + ); + printSettings(settings, options.json ?? false); +} + +export async function settingsTermsAction( + options: SettingsOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + const settings = await deps.settingsService.getSettings(); + if (options.json) { + console.log(JSON.stringify({ terms_required: settings.terms_required })); + return; + } + console.log(formatTerms(settings)); +} + +export async function settingsTermsAcceptAction( + options: SettingsTermsAcceptOptions, + deps: SettingsCommandDependencies, +): Promise { + requireAuth(deps); + if (!options.yes) { + if (!deps.stdinIsTTY || !deps.stdoutIsTTY) { + throw new Error( + "Confirmation required. Review the Terms of Service, then run `githits settings terms accept --yes`.", + ); + } + const accepted = await deps.promptService.confirm( + `Accept the GitHits Terms of Service at ${TERMS_URL}?`, + false, + ); + if (!accepted) { + if (options.json) { + console.log(JSON.stringify({ accepted: false })); + } else { + console.log("Terms were not accepted."); + } + return; + } + } + + const settings = await deps.settingsService.acceptTerms(); + let tokenRefreshed: boolean | null = null; + let warning: string | undefined; + if (!deps.staticApiToken) { + try { + tokenRefreshed = (await deps.tokenProvider.forceRefresh()) !== undefined; + } catch { + tokenRefreshed = false; + } + if (!tokenRefreshed) warning = REFRESH_FAILURE_WARNING; + } + + if (options.json) { + console.log( + JSON.stringify({ + accepted: !settings.terms_required, + token_refreshed: tokenRefreshed, + settings, + ...(warning ? { warning } : {}), + }), + ); + return; + } + + console.log( + settings.terms_required + ? "The account still requires Terms of Service acceptance." + : "Terms of Service accepted.", + ); + if (warning) console.error(warning); +} + +export function registerSettingsCommand( + program: Command, + dependenciesFactory: SettingsDependenciesFactory = createSettingsDependencies, +): void { + const settings = program + .command("settings") + .summary("View and update account settings") + .description( + "View and update GitHits preferences, privacy, terms, and account limits.", + ) + .option("--json", "Output the canonical settings object as JSON") + .action(async (_options: SettingsOptions, command: Command) => { + await settingsAction( + settingsOptions(command), + await dependenciesFactory(), + ); + }); + + settings + .command("show") + .summary("Show all account settings") + .description("Show preferences, privacy, terms, and account limits.") + .option("--json", "Output the canonical settings object as JSON") + .action(async (_options: SettingsOptions, command: Command) => { + await settingsAction( + settingsOptions(command), + await dependenciesFactory(), + ); + }); + + settings + .command("get") + .summary("Get one account setting") + .description("Get one writable account setting by its public name.") + .argument( + "", + `Setting name: ${SETTINGS_KEYS.join(", ")}`, + parseSettingsKey, + ) + .option("--json", "Output the setting name and value as JSON") + .action( + async (key: SettingsKey, _options: SettingsOptions, command: Command) => { + await settingsGetAction( + key, + settingsOptions(command), + await dependenciesFactory(), + ); + }, + ); + + settings + .command("set") + .summary("Set one account setting") + .description("Set one writable account setting using its public name.") + .argument( + "", + `Setting name: ${SETTINGS_KEYS.join(", ")}`, + parseSettingsKey, + ) + .argument("", "Typed setting value or list of values") + .option("--json", "Output the canonical settings object as JSON") + .addHelpText( + "after", + [ + "", + "Values:", + " default-language-id ", + " license-mode ", + " blocked-license-ids [uuid...]", + " marketing-emails ", + "", + "Use `githits settings clear ` for an empty or unset value.", + ].join("\n"), + ) + .action( + async ( + key: SettingsKey, + values: string[], + _options: SettingsOptions, + command: Command, + ) => { + await settingsSetAction( + key, + values, + settingsOptions(command), + await dependenciesFactory(), + ); + }, + ); + + settings + .command("clear") + .summary("Clear one account setting") + .description( + "Clear default-language-id or replace blocked-license-ids with an empty list.", + ) + .argument( + "", + `Setting name: ${CLEARABLE_SETTINGS_KEYS.join(", ")}`, + parseClearableSettingsKey, + ) + .option("--json", "Output the canonical settings object as JSON") + .action( + async ( + key: ClearableSettingsKey, + _options: SettingsOptions, + command: Command, + ) => { + await settingsClearAction( + key, + settingsOptions(command), + await dependenciesFactory(), + ); + }, + ); + + const terms = settings + .command("terms") + .summary("Show Terms of Service status") + .description("Show the current Terms of Service acceptance requirement.") + .option("--json", "Output terms status as JSON") + .action(async (_options: SettingsOptions, command: Command) => { + await settingsTermsAction( + settingsOptions(command), + await dependenciesFactory(), + ); + }); + + terms + .command("accept") + .summary("Accept the current Terms of Service") + .description(`Accept the current Terms of Service at ${TERMS_URL}.`) + .option("--yes", "Accept without an interactive confirmation", false) + .option("--json", "Output the acceptance result as JSON") + .action(async (_options: SettingsTermsAcceptOptions, command: Command) => { + await settingsTermsAcceptAction( + settingsTermsAcceptOptions(command), + await dependenciesFactory(), + ); + }); +} + +function settingsOptions(command: Command): SettingsOptions { + const options = command.optsWithGlobals(); + return { json: options.json }; +} + +function settingsTermsAcceptOptions( + command: Command, +): SettingsTermsAcceptOptions { + const options = command.optsWithGlobals(); + return { json: options.json, yes: options.yes }; +} + +function printSettings(settings: Settings, json: boolean): void { + console.log(json ? JSON.stringify(settings) : formatSettings(settings)); +} + +function printSetting( + key: SettingsKey, + value: SettingValue, + json: boolean, +): void { + if (json) { + console.log(JSON.stringify({ key, value })); + return; + } + if (Array.isArray(value)) { + console.log(value.length > 0 ? value.join("\n") : "None"); + return; + } + console.log(value ?? "None"); +} + +function formatSettings(settings: Settings): string { + return [ + "Preferences", + ` Default language ID: ${settings.default_language_id ?? "Not set"}`, + ` License mode: ${settings.license_mode}`, + ` Blocked license IDs: ${formatList(settings.blocked_license_ids)}`, + "", + "Privacy and terms", + ` Marketing emails: ${settings.marketing_email_opted_out ? "Disabled" : "Enabled"}`, + ` Terms: ${settings.terms_required ? "Acceptance required" : "Accepted"}`, + ` Terms URL: ${TERMS_URL}`, + "", + "Account limits", + ` Example generation limit: ${settings.example_generation_limit ?? "Default"}`, + ].join("\n"); +} + +function formatTerms(settings: Settings): string { + return [ + "Terms of Service", + ` Status: ${settings.terms_required ? "Acceptance required" : "Accepted"}`, + ` URL: ${TERMS_URL}`, + ].join("\n"); +} + +function formatList(values: string[] | null): string { + return values?.length ? values.join(", ") : "None"; +} + +async function createSettingsDependencies(): Promise { + const container = await createContainer(); + return { + settingsService: new SettingsServiceImpl( + getAccountsUrl(), + container.tokenProvider, + createCliFetch(), + ), + tokenProvider: container.tokenProvider, + promptService: new PromptServiceImpl(), + hasValidToken: container.hasValidToken, + mcpUrl: container.mcpUrl, + staticApiToken: container.envApiToken !== undefined, + stdinIsTTY: process.stdin.isTTY === true, + stdoutIsTTY: process.stdout.isTTY === true, + }; +} diff --git a/src/container.ts b/src/container.ts index c97b2b16..33aca05c 100644 --- a/src/container.ts +++ b/src/container.ts @@ -16,6 +16,7 @@ import { PackageIntelligenceServiceImpl, RefreshingGitHitsService, startTelemetrySpan, + type TokenProvider, withTelemetrySpan, } from "@githits/core-internal"; import { version } from "../package.json"; @@ -247,6 +248,8 @@ export interface Dependencies { packageIntelligenceService: PackageIntelligenceService; /** GitHits REST API service */ githitsService: GitHitsService; + /** Active credential provider shared by CLI-only account services. */ + tokenProvider: TokenProvider; } export interface CreateContainerOptions { @@ -326,6 +329,7 @@ export async function createContainer( undefined, serviceRuntime, ), + tokenProvider, }; } @@ -384,6 +388,7 @@ export async function createContainer( ), serviceRuntime, ), + tokenProvider: tokenManager, }; }); } diff --git a/src/services/settings-service.test.ts b/src/services/settings-service.test.ts new file mode 100644 index 00000000..307ad443 --- /dev/null +++ b/src/services/settings-service.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, it, mock } from "bun:test"; +import { AuthenticationError } from "@githits/core-internal"; +import { + DEFAULT_ACCOUNTS_URL, + getAccountsUrl, + SettingsServiceImpl, +} from "./settings-service.js"; +import { createMockTokenProvider } from "./test-helpers.js"; + +const SETTINGS = { + user_id: "0198a7d0-6750-7ace-a68c-418062117d95", + default_language_id: null, + license_mode: "safe" as const, + blocked_license_ids: [], + marketing_email_opted_out: false, + example_generation_limit: null, + terms_required: true, +}; + +function response(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +describe("SettingsServiceImpl", () => { + it("gets canonical settings with the active bearer token", async () => { + const fetchFn = mock(() => Promise.resolve(response(SETTINGS))); + const service = new SettingsServiceImpl( + "https://accounts.githits.com", + createMockTokenProvider({ + getToken: mock(() => Promise.resolve("ghi-static")), + }), + fetchFn as unknown as typeof fetch, + ); + + expect(await service.getSettings()).toEqual(SETTINGS); + expect(fetchFn).toHaveBeenCalledTimes(1); + const [url, init] = fetchFn.mock.calls[0] as unknown as [ + string, + RequestInit, + ]; + expect(url).toBe("https://accounts.githits.com/functions/v1/settings/me"); + expect(init.method).toBe("GET"); + expect(init.headers).toEqual({ + Accept: "application/json", + Authorization: "Bearer ghi-static", + }); + }); + + it("sends only supplied fields in a PATCH", async () => { + const fetchFn = mock(() => Promise.resolve(response(SETTINGS))); + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider(), + fetchFn as unknown as typeof fetch, + ); + + await service.updateSettings({ + blocked_license_ids: [], + marketing_email_opted_out: false, + }); + + const [, init] = fetchFn.mock.calls[0] as unknown as [string, RequestInit]; + expect(init.method).toBe("PATCH"); + expect(JSON.parse(init.body as string)).toEqual({ + blocked_license_ids: [], + marketing_email_opted_out: false, + }); + }); + + it("posts an empty object to the terms acceptance route", async () => { + const fetchFn = mock(() => + Promise.resolve(response({ ...SETTINGS, terms_required: false })), + ); + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider(), + fetchFn as unknown as typeof fetch, + ); + + await service.acceptTerms(); + + const [url, init] = fetchFn.mock.calls[0] as unknown as [ + string, + RequestInit, + ]; + expect(url).toEndWith("/functions/v1/settings/me/terms/accept"); + expect(init.method).toBe("POST"); + expect(init.body).toBe("{}"); + }); + + it("fails closed on malformed settings responses", async () => { + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider(), + mock(() => + Promise.resolve(response({ ...SETTINGS, terms_required: "no" })), + ) as unknown as typeof fetch, + ); + + await expect(service.getSettings()).rejects.toThrow( + "invalid settings response", + ); + }); + + it("throws AuthenticationError when no active token exists", async () => { + const fetchFn = mock(() => Promise.resolve(response(SETTINGS))); + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider({ + getToken: mock(() => Promise.resolve(undefined)), + }), + fetchFn as unknown as typeof fetch, + ); + + await expect(service.getSettings()).rejects.toBeInstanceOf( + AuthenticationError, + ); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it("maps server-side 401 responses to AuthenticationError", async () => { + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider(), + mock(() => + Promise.resolve(response({ error: "unauthorized" }, 401)), + ) as unknown as typeof fetch, + ); + + await expect(service.getSettings()).rejects.toBeInstanceOf( + AuthenticationError, + ); + }); +}); + +describe("getAccountsUrl", () => { + it("defaults to the production accounts origin", () => { + expect(getAccountsUrl({})).toBe(DEFAULT_ACCOUNTS_URL); + }); + + it("supports a secure development override", () => { + expect( + getAccountsUrl({ + GITHITS_ACCOUNTS_URL: "https://zcwquvryvmjuwckxdevg.supabase.co", + }), + ).toBe("https://zcwquvryvmjuwckxdevg.supabase.co"); + }); + + it("rejects insecure non-loopback overrides", () => { + expect(() => + getAccountsUrl({ GITHITS_ACCOUNTS_URL: "http://attacker.test" }), + ).toThrow("GITHITS_ACCOUNTS_URL"); + }); +}); diff --git a/src/services/settings-service.ts b/src/services/settings-service.ts new file mode 100644 index 00000000..3a53f308 --- /dev/null +++ b/src/services/settings-service.ts @@ -0,0 +1,155 @@ +import { + AuthenticationError, + fetchWithTimeout, + isFetchTimeoutError, + LOCAL_AUTHENTICATION_MISSING_MESSAGE, + SERVER_AUTHENTICATION_REJECTED_MESSAGE, + type TokenProvider, + validateServiceUrl, +} from "@githits/core-internal"; +import { z } from "zod"; + +export const DEFAULT_ACCOUNTS_URL = "https://accounts.githits.com"; + +const SETTINGS_SCHEMA = z.strictObject({ + user_id: z.uuid(), + default_language_id: z.uuid().nullable(), + license_mode: z.enum(["safe", "yolo", "custom"]), + blocked_license_ids: z.array(z.uuid()).nullable(), + marketing_email_opted_out: z.boolean(), + example_generation_limit: z.number().int().nonnegative().nullable(), + terms_required: z.boolean(), +}); + +export type Settings = z.infer; +export type LicenseMode = Settings["license_mode"]; + +export interface SettingsPatch { + default_language_id?: string | null; + license_mode?: LicenseMode; + blocked_license_ids?: string[] | null; + marketing_email_opted_out?: boolean; +} + +export interface SettingsService { + getSettings(): Promise; + updateSettings(patch: SettingsPatch): Promise; + acceptTerms(): Promise; +} + +/** CLI-only client for the self-scoped canonical account settings API. */ +export class SettingsServiceImpl implements SettingsService { + constructor( + private readonly accountsUrl: string, + private readonly tokenProvider: TokenProvider, + private readonly fetchFn: typeof fetch = globalThis.fetch, + ) {} + + async getSettings(): Promise { + return this.request("", "GET"); + } + + async updateSettings(patch: SettingsPatch): Promise { + return this.request("", "PATCH", patch); + } + + async acceptTerms(): Promise { + return this.request("/terms/accept", "POST", {}); + } + + private async request( + suffix: string, + method: "GET" | "PATCH" | "POST", + body?: object, + ): Promise { + const token = await this.tokenProvider.getToken(); + if (!token) { + throw new AuthenticationError( + LOCAL_AUTHENTICATION_MISSING_MESSAGE, + "local", + ); + } + + const accountsUrl = validateServiceUrl( + this.accountsUrl, + "GITHITS_ACCOUNTS_URL", + ).replace(/\/+$/, ""); + let response: Response; + try { + response = await fetchWithTimeout( + `${accountsUrl}/functions/v1/settings/me${suffix}`, + { + method, + headers: { + Accept: "application/json", + Authorization: `Bearer ${token}`, + ...(body === undefined + ? {} + : { "Content-Type": "application/json" }), + }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }, + { fetchFn: this.fetchFn }, + ); + } catch (cause) { + if (isFetchTimeoutError(cause)) { + throw new Error("GitHits account settings request timed out.", { + cause, + }); + } + throw new Error( + "Could not reach GitHits account settings. Check your connection and GITHITS_ACCOUNTS_URL, then try again.", + { cause }, + ); + } + + if (response.status === 401) { + throw new AuthenticationError( + SERVER_AUTHENTICATION_REJECTED_MESSAGE, + "server", + ); + } + if (!response.ok) { + throw new Error(await parseSettingsError(response)); + } + + let payload: unknown; + try { + payload = await response.json(); + } catch (cause) { + throw new Error("GitHits returned an invalid settings response.", { + cause, + }); + } + const parsed = SETTINGS_SCHEMA.safeParse(payload); + if (!parsed.success) { + throw new Error("GitHits returned an invalid settings response.", { + cause: parsed.error, + }); + } + return parsed.data; + } +} + +export function getAccountsUrl( + env: Record = process.env, +): string { + return validateServiceUrl( + env.GITHITS_ACCOUNTS_URL ?? DEFAULT_ACCOUNTS_URL, + "GITHITS_ACCOUNTS_URL", + ); +} + +async function parseSettingsError(response: Response): Promise { + const fallback = `Account settings request failed with status ${response.status}.`; + let payload: unknown; + try { + payload = await response.json(); + } catch { + return fallback; + } + if (!payload || typeof payload !== "object") return fallback; + const record = payload as Record; + const detail = record.error ?? record.message ?? record.reason; + return typeof detail === "string" && detail.trim() ? detail : fallback; +} diff --git a/src/shared/command-metadata.test.ts b/src/shared/command-metadata.test.ts index 5a80a198..f9f38480 100644 --- a/src/shared/command-metadata.test.ts +++ b/src/shared/command-metadata.test.ts @@ -10,6 +10,13 @@ describe("authenticated command metadata", () => { "example", "languages", "feedback", + "settings", + "settings show", + "settings get", + "settings set", + "settings clear", + "settings terms", + "settings terms accept", "search", "search-status", "code files", diff --git a/src/shared/command-metadata.ts b/src/shared/command-metadata.ts index e96d569a..0aab23e9 100644 --- a/src/shared/command-metadata.ts +++ b/src/shared/command-metadata.ts @@ -24,6 +24,48 @@ export const AUTHENTICATED_COMMANDS = [ postLoginMessage: "Authentication complete. Submitting feedback...", jsonCapable: true, }, + { + path: "settings", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Loading account settings...", + jsonCapable: true, + }, + { + path: "settings show", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Loading account settings...", + jsonCapable: true, + }, + { + path: "settings get", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Loading account setting...", + jsonCapable: true, + }, + { + path: "settings set", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Updating account settings...", + jsonCapable: true, + }, + { + path: "settings clear", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Clearing account setting...", + jsonCapable: true, + }, + { + path: "settings terms", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Loading terms status...", + jsonCapable: true, + }, + { + path: "settings terms accept", + autoLoginEligible: true, + postLoginMessage: "Authentication complete. Accepting terms...", + jsonCapable: true, + }, "search", "search-status", "code files", From 3379a39d1b4ab560c744383b0366480c6a39e453 Mon Sep 17 00:00:00 2001 From: Olli-Pekka Heinisuo Date: Tue, 11 Aug 2026 13:53:04 +0300 Subject: [PATCH 2/2] fix: keep account settings forward compatible Remove the development account origin from public examples and allow newer account APIs to add settings fields without breaking older CLI versions. --- docs/implementation/config.md | 2 +- src/services/settings-service.test.ts | 16 ++++++++++++++-- src/services/settings-service.ts | 4 +++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/docs/implementation/config.md b/docs/implementation/config.md index 97526b29..d0a4d65f 100644 --- a/docs/implementation/config.md +++ b/docs/implementation/config.md @@ -53,7 +53,7 @@ Package/source access uses the package/source service URL from `GITHITS_CODE_NAV | `GITHITS_MCP_URL` | Override MCP server URL | `http://localhost:7071/mcp` | | `GITHITS_API_URL` | Override REST API URL | `http://localhost:8000` | | `GITHITS_CODE_NAV_URL` | Override package/source service URL | `http://localhost:4000` | -| `GITHITS_ACCOUNTS_URL` | Override account settings origin | `https://zcwquvryvmjuwckxdevg.supabase.co` | +| `GITHITS_ACCOUNTS_URL` | Override account settings origin | `https://accounts.example.test` | | `GITHITS_API_TOKEN` | API token for authentication | `ghi-abc123...` | | `GITHITS_AUTH_STORAGE` | Override OAuth credential storage for the current process (`keychain` or `file`) | `file` | | `GITHITS_TELEMETRY` | Emit end-of-run timing spans to stderr for local profiling | `1` | diff --git a/src/services/settings-service.test.ts b/src/services/settings-service.test.ts index 307ad443..95dff14b 100644 --- a/src/services/settings-service.test.ts +++ b/src/services/settings-service.test.ts @@ -105,6 +105,18 @@ describe("SettingsServiceImpl", () => { ); }); + it("ignores additive settings fields from newer account APIs", async () => { + const service = new SettingsServiceImpl( + DEFAULT_ACCOUNTS_URL, + createMockTokenProvider(), + mock(() => + Promise.resolve(response({ ...SETTINGS, future_setting: true })), + ) as unknown as typeof fetch, + ); + + await expect(service.getSettings()).resolves.toEqual(SETTINGS); + }); + it("throws AuthenticationError when no active token exists", async () => { const fetchFn = mock(() => Promise.resolve(response(SETTINGS))); const service = new SettingsServiceImpl( @@ -144,9 +156,9 @@ describe("getAccountsUrl", () => { it("supports a secure development override", () => { expect( getAccountsUrl({ - GITHITS_ACCOUNTS_URL: "https://zcwquvryvmjuwckxdevg.supabase.co", + GITHITS_ACCOUNTS_URL: "https://accounts.example.test", }), - ).toBe("https://zcwquvryvmjuwckxdevg.supabase.co"); + ).toBe("https://accounts.example.test"); }); it("rejects insecure non-loopback overrides", () => { diff --git a/src/services/settings-service.ts b/src/services/settings-service.ts index 3a53f308..dcfd2080 100644 --- a/src/services/settings-service.ts +++ b/src/services/settings-service.ts @@ -11,7 +11,9 @@ import { z } from "zod"; export const DEFAULT_ACCOUNTS_URL = "https://accounts.githits.com"; -const SETTINGS_SCHEMA = z.strictObject({ +// Strip unknown fields so additive account API changes remain compatible with +// older CLI versions while every field this version consumes stays validated. +const SETTINGS_SCHEMA = z.object({ user_id: z.uuid(), default_language_id: z.uuid().nullable(), license_mode: z.enum(["safe", "yolo", "custom"]),