Skip to content

fix(auth): fail loudly on GitHub auth errors, naming the credential - #389

Draft
gblanc-1a wants to merge 4 commits into
AmadeusITGroup:mainfrom
gblanc-1a:fix/github-auth-diagnostics
Draft

gblanc-1a wants to merge 4 commits into
AmadeusITGroup:mainfrom
gblanc-1a:fix/github-auth-diagnostics

Conversation

@gblanc-1a

@gblanc-1a gblanc-1a commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

raw.githubusercontent.com answers 404 — never 401/403 — when the Authorization header carries a token GitHub rejects, even for content that is public and served fine anonymously. A stale VS Code session therefore makes every hub look non-existent, and the error blames the hub URL instead of the credential.

This makes every auth failure fail loudly, name the root cause, say which credential was used and where it came from, report scopes and SSO authorization, and offer Diagnose + Reset GitHub Token.

One deliberate exception: an account with no access to the default Amadeus-xDLC/genai.prompt-registry-config hub — an open-source contributor outside Amadeus — is an expected condition. Clear info logs, no notification, no error.

Important

Depends on #386 and #387, and must merge after both. This branch contains their commits, so the diff shown here includes them until they land. Review those two first; this PR's own change is the ~50 files outside packages/core/src/ports/log-sink.ts and the default-hubs consolidation.

Replaces #374. That PR diagnosed the problem correctly but then worked around it: hub-resolver.ts silently retried anonymously and served the config, and registry-manager.ts silently dropped a rejected promptregistry.githubToken. Both hide the fault the PR exists to expose.

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • ♻️ Code refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🧪 Test coverage improvement
  • 🔧 Configuration/build changes

Breaking at the source level only: core's TokenProvider.getToken now returns ResolvedToken { token, origin } instead of string. All ~19 in-repo call sites are updated. No user-visible or runtime behavior depends on it, but a downstream consumer of the published @ai-primitives-hub/core port would need the one-line change.

Related Issues

Relates to #374 — supersedes it. Close #374 in favour of this, or reuse its review thread.

Changes Made

Fail loudly (packages/infra/src/hub/hub-resolver.ts)

  • Exactly one authenticated fetch. No anonymous retry, ever.
  • On a non-200 with a credential attached: diagnose the token against api.github.com, emit a warn log event, then throw.
  • Throws RegistryError so callers classify on a code instead of matching message text: AUTH.TOKEN_REJECTED (401 on /user), AUTH.MISSING_SCOPE (no repo), AUTH.SSO_REQUIRED (x-github-sso present), AUTH.NO_REPO_ACCESS (valid token, repo invisible), HUB.FETCH_FAILED (anonymous, or api.github.com unreachable). Context carries { url, repoLocation, status, origin, scopes, sso, login }; hint is the verdict.

Token origin through the port (packages/core/src/ports/http.ts)

  • TokenProvider.getToken returns ResolvedToken { token, origin }; new TokenOrigin { kind, detail? } with kinds explicit | setting | vscode-session | env | gh-cli | unknown.
  • Every provider self-reports: EnvTokenProvider distinguishes GITHUB_TOKEN from GH_TOKEN (it used to collapse them), GhCliTokenProvider reports gh auth token, StaticTokenProvider takes an optional origin, VsCodeSessionTokenProvider reports the account label. CompositeTokenProvider passes the winner through untouched — that loop is where provenance died before.
  • New formatCredential renders origin=vscode-session(octocat) token=***<len=40,tail=9c1e>, threaded into the API client's auth context, the hub resolver's warn line and error context, https-bundle-downloader (which threw a bare HTTP <status> with no auth context at all), harvest logs, and ai-primitives-hub doctor.
  • Dropped the hand-rolled opts.explicitToken ? 'explicit' : 'env' in hub-harvester.ts, which mislabelled every gh-CLI and GH_TOKEN run. tokenSourceToOrigin maps harvest's existing TokenSource union onto TokenOrigin so logs share one vocabulary.

Global token applied, never dropped (registry-manager.ts)

  • promptregistry.githubToken is still applied, now marked origin=setting:promptregistry.githubToken. A rejected setting token produces a loud, origin-labelled failure instead of a silent fallback.
  • Deliberately not ported from fix(infra): credential diagnostics and anonymous fallback for hub resolution #374: the memoized startup probe (a network round-trip on activation), the rejection flag (which silently dropped the token), and its Clear Token action (which wrote ConfigurationTarget.Global — destructive and out of scope).

Expected vs real failure (packages/app/src/registry/hub-manager.ts, utils/first-run-hub-report.ts)

  • checkHubAvailability returns HubAvailability { available, reason?, credential?, detail? } mapped from the error code, alongside the existing boolean verifyHubAvailability.
  • First-run: reason === 'no-access' and isDefaultHub(reference)info, no notification, ending in "This is not an error." Anything else → warn plus the credential, the verdict, and a pointer to the two commands. When every default hub fails and all failures are expected, the user gets an information message; one real failure makes it a warning. The picker still offers Custom URL + Skip either way.

The user sees the failure (utils/show-auth-failure.ts)

  • showAuthFailure shows the original message plus the verdict, with Diagnose / Reset GitHub Token / Show Logs. showOperationFailure routes non-auth errors to a plain notification.
  • The marketplace provider uses it. fix(infra): credential diagnostics and anonymous fallback for hub resolution #374's variant fired the diagnostic and returned, so Failed to install bundle: … was never shown and a "healthy" verdict could appear for a failed install.

Targeted diagnosis (commands/diagnose-github-auth-command.ts)

  • Written fresh, targeted only: no source sweep, no GITHUB_SOURCE_TYPES set duplicating adapter knowledge, no RegistryManager dependency.
  • Detects a credential-level verdict from report.repoStatus === undefined — an explicit fact. fix(infra): credential diagnostics and anonymous fallback for hub resolution #374 inferred it from reports.length !== locations.length, which mislabels whenever there is exactly one location and a rejected credential.
  • Constant, unambiguously public control URL. getRecommendedHub() cannot serve here: the recommended hub is private, so "the control failed" would be indistinguishable from "no access".
  • Non-interactive (createIfNone: false) — read-only diagnostics must not pop a sign-in modal and then report on a session that was not the one that failed.

Testing

Test Coverage

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing completed
  • All existing tests pass

Added

  • packages/infra/test/hub/hub-resolver.test.ts — six cases: a stale token on public content must reject with AUTH.TOKEN_REJECTED, exactly one hub-config fetch happened (no anonymous retry), the onLog warn line carries origin + redacted token + verdict and never the token itself, plus one case per remaining code.
  • packages/infra/test/auth/{format-token-origin,static-token-provider}.test.ts — new files; composite-token-provider.test.ts gains a case asserting the winner's origin survives the chain.
  • apps/vscode-extension/test/utils/first-run-hub-report.test.ts (8), test/utils/show-auth-failure.test.ts (12), test/commands/diagnose-github-auth-command.test.ts (7), test/ui/marketplace-view-provider.installFailure.test.ts (2 — a regression guard that the message still starts with Failed to install bundle:).
  • Integration: test/suite/integration-scenarios.test.ts asserts the new command is both registered at activation and declared in the manifest's commands + commandPalette. A unit test can see neither — it mocks vscode and never loads package.json. Verified non-vacuous by breaking each half in turn (renaming the registerCommand id, then removing the palette entry); each produced the expected failure.

Deliberately not carried over: #374's expectAnonymousFallback helper, which asserts the opposite of the intended behavior.

Manual Testing Steps

Automated verification (all green):

  1. pnpm --filter "@ai-primitives-hub/*" build — clean.
  2. Vitest: core 228, infra 722, app 554, cli 286 passing.
  3. compile, compile-tests — clean.
  4. test:unit 2221 passing; test:integration 8 passing.
  5. eslint src test --fix in all five packages — clean, no files rewritten.
  6. node -e "require('@ai-primitives-hub/infra')" — the 12 new barrel exports resolve; formatCredential renders as documented.

Still outstanding — these need a real VS Code session and two GitHub accounts, and I could not run them:

  1. Set promptregistry.githubToken to garbage → every failure log contains origin=setting:promptregistry.githubToken, and a public hub fetch fails (no anonymous recovery). Notification offers Diagnose + Reset.
  2. Sign in with a GitHub account outside Amadeus, fresh install → no error notification; output channel shows the four ⓘ Hub not available to this account lines; picker offers Custom URL + Skip.
  3. Fail a marketplace install on a private repo → notification text starts with Failed to install bundle: and offers Diagnose + Reset.

Tested On

  • macOS

  • Windows

  • Linux

  • VS Code Stable

  • VS Code Insiders

VS Code Stable via @vscode/test-electron (integration suite), not a hand-driven session.

Screenshots

None. The three manual scenarios above are where the notification text would be worth capturing.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

The last box is the blocker: #386 and #387 must merge first.

Self-review went further than reading the diff — two independent review passes (functional completeness, technical debt) were run over the staged change. Both actionable findings were fixed: the anonymous path's error context now uses formatCredential(undefined) for consistency, and the diagnostics chain is a CompositeTokenProvider rather than an array iterated by hand.

Documentation

  • README.md updated
  • JSDoc comments added/updated
  • No documentation changes needed

docs/contributor-guide/architecture/authentication.md rewritten: keeps the 404-ambiguity explanation, adds origin= to the auth-context sample, states explicitly that the hub resolver has no anonymous fallback, adds a Token origin table and an Expected: no access to a default hub section with the verbatim log lines and the rule that it is info, never warn/error, and never a notification. Removed #374's two-shape (targeted vs sweep) table and its claim that a marketplace install launches the diagnosis automatically — it now shows the error with a Diagnose action.

Also corrects #374's description: it claimed describeToken logs gho_*** (length 40). The actual redactToken emits ***<len=N,tail=abcd> — length plus the last four characters. These strings now reach user-facing notification text via describeError, not only the output channel.

Additional Notes

Why a rebuild rather than a revert on bugfix/auth-issues: that branch's history would have become add-then-remove, and two of its three pieces (the log-sink move, the default-hubs consolidation) belong on their own branches regardless.

Behavior change reviewers should weigh: a rejected credential now fails public hub fetches that previously succeeded via the anonymous retry. That is the point — it surfaces a broken credential instead of masking it until the first private hub — but it does mean a user with a stale token who only uses public hubs will newly see an error. They now get an actionable one.

Reviewer Guidelines

Please pay special attention to:

  • The no-fallback decision. This is the crux. If you disagree, the rest of the PR does not follow.
  • isDefaultHub gating the "expected" path. Too broad a match would silence a genuine failure; the predicate ignores the git ref by design (see refactor: consolidate default hub configuration into infra #387).
  • TokenProvider becoming a breaking port change. The alternative was an optional side-channel for origin, which loses provenance the moment a caller forgets it. I took the compile-time-enforced route; it costs ~19 mechanical call-site updates.
  • catch blocks around resolver.resolve (app/src/registry/hub-manager.ts, load-hub-sources.ts, the extension's hub-manager.ts). RegistryError extends Error, so error.message consumers keep working — but please confirm none of them needed the old bare-Error shape.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0.

Guillaume BLANC added 4 commits August 6, 2026 09:45
infra needs the generic log-event callback but may not depend on app
(AGENTS.md dependency rule), so the shape moves to core/ports/log-sink.
packages/app/src/update re-exports it, keeping the app public surface
unchanged. No behavior change.
Two divergent copies existed: packages/infra/src/hub/default-hubs.ts
(Amadeus + community, config/default-hubs.json) and the extension's
src/config/default-hubs.ts (awesome-copilot + a non-existent community
hub, config/defaultHubs.json). The infra module is now the single source
of truth for both delivery layers:

- add isDefaultHub()/isRecommendedDefaultHub(), comparing type+location
  case-insensitively and ignoring the git ref
- split icon (plain text, CLI) from codicon (VS Code selector) so one
  config can serve both hosts
- move the JSON to packages/infra/config/default-hubs.json (the path the
  loader reads) and drop the camelCase extension copy
- give only the recommended hub recommended: true; two made
  getRecommendedHub() order-dependent
- replace the h.name === 'Amadeus' compare in cli init with the predicate

No behavior change beyond the recommended-flag fix.
raw.githubusercontent.com answers 404 — never 401/403 — when the
Authorization header carries a token GitHub rejects, even for public
content. A stale VS Code session therefore made every hub look
non-existent, and the error blamed the hub URL instead of the credential.

- hub-resolver: one authenticated fetch, no anonymous retry. On failure
  with a credential attached it diagnoses the token against
  api.github.com and throws a RegistryError whose code names the cause
  (AUTH.TOKEN_REJECTED / MISSING_SCOPE / SSO_REQUIRED / NO_REPO_ACCESS,
  or HUB.FETCH_FAILED), with the verdict as `hint`.
- TokenProvider now returns { token, origin }. Every provider
  self-reports (which env var, which VS Code account, which setting), so
  provenance can no longer be lost in the composite chain. formatCredential
  renders it into the API client's auth context, the hub resolver's warn
  line, bundle-download failures, harvest logs and `doctor`.
- The global promptregistry.githubToken setting is applied and marked
  origin=setting:promptregistry.githubToken — never silently dropped.
- verifyHubAvailability gains checkHubAvailability, which reports why. A
  default hub an account cannot see is logged at info as "not an error"
  with no notification; anything else warns and points at the commands.
- New showAuthFailure/showOperationFailure: the failure is always shown,
  with the verdict appended and Diagnose / Reset GitHub Token actions.
- New targeted-only promptregistry.diagnoseGitHubAuth: no source sweep, a
  constant public control URL, non-interactive session lookup, and
  repoStatus === undefined to detect a credential-level verdict. An
  integration test asserts it is both registered at activation and
  declared in the manifest's commands + commandPalette — a unit test can
  see neither, since it mocks `vscode` and never loads package.json.
- Rewrote docs/contributor-guide/architecture/authentication.md.

Carried over from AmadeusITGroup#374: github-token-diagnostics, github-api-client's
auth context, vscode-session-token-provider, github-auth-command's real
forceNewSession. Left behind: the anonymous fallback, the startup token
probe, and the silent global-token drop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant