From a30ffabed6a000ed4efe381036986941ab3cdbbb Mon Sep 17 00:00:00 2001 From: bgagent Date: Thu, 1 Oct 2026 12:28:54 -0400 Subject: [PATCH] fix(cli): list the redirect URI each setup path actually sends (#914) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bgagent linear app-template` printed the hosted consent page as the only Linear Redirect URI. On the vault path Linear redirects to the AgentCore provider callback instead, so registering exactly what the template printed failed consent with "Invalid redirect_uri parameter for the application". Which URI Linear checks depends on the setup path: - vault: the AgentCore provider callback, minted when setup first registers the app, so it cannot exist on a first run; - Secrets Manager on a vault stack (setup's fallback when AgentCore is unavailable, and `setup --code` resuming one): the hosted consent page; - no vault, or `add-workspace`: the CLI's loopback listener. The Redirect URIs field now holds the vault-path URI, one line per state: - provider exists -> the AgentCore callback - vault, first run -> "Leave empty — setup prints the URI to add, then stops" (Linear's create-app form accepts an empty field) - no vault -> the loopback listener The consent page is printed after the form as a conditional entry ("add it only if setup falls back to Secrets Manager — setup says so"), and add-workspace's loopback gets its own trap line when the field does not show it. `setup` now prints the vault callback on every consent, not only on the run that created the provider. `provider.created` was standing in for "this URI is registered on the Linear app", which is true once per provider lifetime while the real condition stays false until someone pastes the URI in. Lookup failures are no longer swallowed. A credentials or region error reading the stack used to render a vault stack as a no-vault one, and an unreadable provider rendered "leave it empty" for an onboarded workspace. The template now warns, via `probeLinearVaultCallbackUrl`, which tells "absent" from "unreadable". `chooseSetupRedirect` is extracted from setup so a test can check that every redirect_uri setup can send on a vault stack is one the template tells the operator to register. That invariant is what drifted. Refs #914 --- cli/src/commands/linear.ts | 164 ++++++++++------ cli/src/linear-vault.ts | 34 ++-- .../linear-app-template-lookup.test.ts | 37 +++- cli/test/commands/linear.test.ts | 180 ++++++++++++++---- cli/test/linear-vault.test.ts | 38 ++-- docs/guides/LINEAR_SETUP_GUIDE.md | 2 +- .../content/docs/using/Linear-setup-guide.md | 2 +- 7 files changed, 331 insertions(+), 126 deletions(-) diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts index 482aec1d0..eef58480d 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -61,7 +61,7 @@ import { linearVaultUserId, linearVaultUserIdForSlug, mintLinearTokenFromVault, - lookupLinearVaultCallbackUrl, + probeLinearVaultCallbackUrl, upsertLinearCredentialProvider, } from '../linear-vault'; import { awaitOauthCallback, CALLBACK_URL } from '../oauth-callback-server'; @@ -182,12 +182,18 @@ export interface LinearAppTemplateOptions { */ readonly vaultCallbackUrl?: string; /** - * The stack's hosted consent page (`LinearVaultConsentUrl`). When supplied it is - * listed as a callback URL, which is what lets an operator onboard with NO - * localhost listener — the browser can reach this page from anywhere, whereas a - * loopback redirect dead-ends on a cloud desktop / SSH box / container. + * The stack's hosted consent page (`LinearVaultConsentUrl`). Its presence means the + * Identity vault is deployed. It is Linear's `redirect_uri` ONLY when `setup` falls + * back to Secrets Manager on a vault stack, so it is printed as a conditional entry, + * never as the field's value. */ readonly hostedConsentUrl?: string; + /** + * Set when a stack or vault lookup FAILED (credentials, region, permissions), as + * opposed to finding nothing. Without it a vault stack whose outputs could not be read + * renders as a no-vault stack, and an onboarded workspace renders "leave it empty". + */ + readonly lookupWarning?: string; /** * ABCA's Linear webhook endpoint, derived from the configured API URL. * @@ -222,7 +228,12 @@ export async function resolveTemplateCallbackUrls(args: { region?: string; stackName?: string; slug?: string; -}): Promise<{ hostedConsentUrl?: string; vaultCallbackUrl?: string; webhookUrl?: string }> { +}): Promise<{ + hostedConsentUrl?: string; + vaultCallbackUrl?: string; + lookupWarning?: string; + webhookUrl?: string; +}> { // Unconfigured CLI is an expected state here, not an error. let config: CliConfig | undefined; try { @@ -237,22 +248,45 @@ export async function resolveTemplateCallbackUrls(args: { const region = args.region ?? config?.region; if (!region) return { webhookUrl }; + // `getStackOutput` already returns null for a stack that is not deployed and throws for + // anything else. Keep that distinction: a swallowed credentials or region error would + // render a vault stack's template as a no-vault one, with no sign anything went wrong. + const failures: string[] = []; const hostedConsentUrl = args.stackName - ? await getStackOutput(region, args.stackName, 'LinearVaultConsentUrl').catch(() => null) - : null; - // Needs a slug: the provider is per-workspace, so without one there is nothing - // specific to look up. - const vaultCallbackUrl = args.slug - ? await lookupLinearVaultCallbackUrl({ region, workspaceSlug: args.slug }) + ? await getStackOutput(region, args.stackName, 'LinearVaultConsentUrl').catch((err: unknown) => { + failures.push(`stack ${args.stackName} in ${region} (${(err as Error)?.name ?? 'Error'})`); + return null; + }) : null; + // Needs a slug: the provider is per-workspace. Inferring it from the registry belongs with + // #917's `listOnboardedWorkspaceSlugs` rather than in a second copy here. + const probe = args.slug ? await probeLinearVaultCallbackUrl({ region, workspaceSlug: args.slug }) : null; + if (probe?.kind === 'unreadable') failures.push(`vault provider for '${args.slug}' (${probe.errorName})`); return { hostedConsentUrl: hostedConsentUrl ?? undefined, - vaultCallbackUrl: vaultCallbackUrl ?? undefined, + vaultCallbackUrl: probe?.kind === 'found' ? probe.callbackUrl : undefined, + lookupWarning: failures.length ? `Could not read ${failures.join('; ')}.` : undefined, webhookUrl, }; } +/** + * The `redirect_uri` the Secrets-Manager leg of `setup` sends to Linear. + * + * Extracted so the template and `setup` can be checked against each other: on a vault + * stack whose `setup` fell back to Secrets Manager this is the hosted consent page, and + * `app-template` must tell the operator so. A template that called the consent page + * "nothing to paste" was the result of the two drifting (#914 review). + */ +export function chooseSetupRedirect(args: { + readonly vaultAccessToken?: string | null; + readonly consentPageUrl?: string | null; +}): { readonly hostedUrl?: string; readonly redirectUri: string } { + const hostedUrl = args.vaultAccessToken ? undefined : (args.consentPageUrl ?? undefined); + return { hostedUrl, redirectUri: hostedUrl ?? CALLBACK_URL }; +} + export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): string { // Defaults match the upstream sample so unmodified `bgagent linear app-template` // produces a usable config without forcing every operator to invent strings. @@ -282,35 +316,39 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st ` Developer URL: ${developerUrl}`, ` Description: ${description}`, '', - // Linear's field is literally "Redirect URIs — All OAuth redirect URIs, - // separated with newlines". Each entry is annotated with the command that - // redirects to it: the flows use DIFFERENT URIs, and registering one then - // running the other yields an opaque "Invalid redirect_uri". - ' Redirect URIs (one per line, copied EXACTLY — paste, do not retype):', - // Exactly the URI the CLI sends, and no variants of it. Listing a second - // slashless form was meant as insurance against a typo, but Linear validates the - // whole field on save, so one bad line loses the good ones too — and the error - // then reads as though it were about the line just added. Fewer entries is - // strictly safer here; the CLI sends one string and this prints that string. - ...(opts.hostedConsentUrl ? [` ${opts.hostedConsentUrl}`] : [` ${callbackUrl}`]), - ...(opts.vaultCallbackUrl ? [` ${opts.vaultCallbackUrl}`] : []), - ...(opts.hostedConsentUrl - ? [] - : [ - '', - ' (That is a loopback URL, so it only works when your browser runs on the', - ' same machine as the CLI. Deploy with', - ' `--context enableLinearIdentityVault=true` for a hosted consent page that', - ' works from anywhere, and this command will list it instead.)', - ]), - ...(opts.vaultCallbackUrl || !opts.hostedConsentUrl - ? [] - : [ + // Linear validates the authorize request's `redirect_uri` against this field, so it + // must hold the URI the operator's setup path sends. Which one depends on the path: + // - vault (`setup` with the Identity vault): the AgentCore provider callback. It is + // minted when setup first registers the app, so it cannot exist on a first run. + // - Secrets Manager on a vault stack (setup's fallback when AgentCore is unavailable, + // and `setup --code` resuming one): the hosted consent page. + // - no vault, or `add-workspace`: the CLI's loopback listener. + // The consent page is printed as a conditional entry after the form rather than here: + // it is needed only on the fallback, and setup names it when that happens. + // + // One bare URI per line: an inline "← note" would wrap a ~100-char callback, producing + // the two-malformed-entries failure the traps warn about. + ...(opts.lookupWarning + ? [ + ` ⚠ ${opts.lookupWarning}`, + ' The Redirect URIs below may be wrong — check credentials and --region.', '', - ' `bgagent linear setup ` prints one more URI the first time it runs —', - ' the vault\'s own callback, whose id does not exist until then. Add it and', - ' re-run.', - ]), + ] + : []), + ' Redirect URIs (one per line, copied EXACTLY — paste, do not retype):', + ...(opts.vaultCallbackUrl + ? [` ${opts.vaultCallbackUrl}`] + : opts.hostedConsentUrl + ? [ + // Linear's create-app form accepts an empty Redirect URIs field (verified on the + // live form, 2026-10), so a first vault run genuinely has nothing to enter here. + ' Leave empty — `bgagent linear setup ` prints the URI to add, then stops.', + ] + : [ + ` ${callbackUrl}`, + ' (loopback — needs the browser on this machine; deploy with', + ' `--context enableLinearIdentityVault=true` for a hosted consent page)', + ]), '', ' Public: OFF', ' Client credentials: OFF', @@ -323,6 +361,14 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st 'Click Create, then come back with the Client ID, Client Secret and that', 'signing secret: bgagent linear setup ', '', + ...(opts.hostedConsentUrl + ? [ + `Identity vault consent page: ${opts.hostedConsentUrl}`, + ' Add it as a Redirect URI only if setup falls back to Secrets Manager —', + ' setup says so when it does.', + '', + ] + : []), // Each trap below cost someone a failed onboarding. Kept short deliberately: // the list grew long enough that operators stopped reading it and missed real // fields, which is its own failure mode. @@ -342,12 +388,7 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st ' you name the app (Linear lowercases the name for the display handle).', ' • actor=app cannot also request the `admin` scope — Linear rejects the pair.', ...(opts.hostedConsentUrl - ? [ - ' • Using the older `add-workspace` command? It still redirects to', - ` ${'http://localhost:8080/oauth/callback'} and is Secrets-Manager only, so add`, - ' that URI too if you plan to use it. `bgagent linear setup ` needs no', - ' localhost and is the only path that supports the Identity vault.', - ] + ? [` • Using \`add-workspace\` instead of \`setup\`? It redirects to ${callbackUrl}.`] : []), bar, ].join('\n'); @@ -599,7 +640,7 @@ export function makeLinearCommand(): Command { // config, no credentials, and no deployed stack: every lookup is // best-effort and the template degrades to explaining what is missing. const looked = opts.offline - ? { hostedConsentUrl: undefined, vaultCallbackUrl: undefined, webhookUrl: undefined } + ? { hostedConsentUrl: undefined, vaultCallbackUrl: undefined, lookupWarning: undefined, webhookUrl: undefined } : await resolveTemplateCallbackUrls({ region: opts.region, stackName: opts.stackName, @@ -613,6 +654,7 @@ export function makeLinearCommand(): Command { awsCallbackUrl: opts.awsCallbackUrl, vaultCallbackUrl: opts.vaultCallbackUrl ?? looked.vaultCallbackUrl, hostedConsentUrl: opts.hostedConsentUrl ?? looked.hostedConsentUrl, + lookupWarning: looked.lookupWarning, webhookUrl: looked.webhookUrl, })); }), @@ -1023,9 +1065,22 @@ export function makeLinearCommand(): Command { }); if (consent.kind === 'consent-required') { + // The redirect URI is stated on EVERY consent, not just the run that minted + // the provider (#914). `provider.created` is true exactly once per provider + // lifetime, while the thing it stands in for — "this URI is registered on the + // Linear app" — stays false until a human pastes it in. So anyone who + // interrupted the first run, lost the scrollback, or never finished the paste + // used to get no remedy on any later run: just an authorize URL that fails in + // the browser with "Invalid redirect_uri", naming the URI but not the cause. + // One line costs nothing when it is already registered. + if (provider.callbackUrl) { + console.log('\n This consent redirects through the vault callback below, which MUST be'); + console.log(' one of the Linear app\'s Redirect URIs — otherwise Linear answers'); + console.log(' "Invalid redirect_uri parameter for the application":\n'); + console.log(provider.callbackUrl); + } // One URL, one action. The page the browser lands on shows the session - // id and names itself, so printing it here is noise; the redirect URIs - // were dealt with on the first run above. + // id and names itself, so printing it here is noise. console.log('\n → Open this URL and Authorize:\n'); console.log(consent.authorizationUrl); console.log(''); @@ -1048,8 +1103,8 @@ export function makeLinearCommand(): Command { if (!minted) { throw new CliError( 'The vault did not return a token after the consent completed. Re-run ' - + `\`bgagent linear setup ${slug}\`; if it recurs, confirm the Linear app lists both ` - + 'redirect URIs printed above.', + + `\`bgagent linear setup ${slug}\`; if it recurs, confirm the Linear app lists the ` + + 'vault callback printed above as a Redirect URI.', ); } vaultAccessToken = minted; @@ -1066,8 +1121,9 @@ export function makeLinearCommand(): Command { // whenever the stack has one, because it works from any browser; the // localhost loopback only works when the browser runs on this machine, and // dead-ends on a cloud desktop / SSH box / container. - const setupHostedUrl = vaultAccessToken ? undefined : consentPageUrl; - const setupRedirectUri = setupHostedUrl ?? CALLBACK_URL; + const { hostedUrl: setupHostedUrl, redirectUri: setupRedirectUri } = chooseSetupRedirect({ + vaultAccessToken, consentPageUrl, + }); const pkce = generatePkce(); const state = randomState(); diff --git a/cli/src/linear-vault.ts b/cli/src/linear-vault.ts index c7a978959..7e3c133ba 100644 --- a/cli/src/linear-vault.ts +++ b/cli/src/linear-vault.ts @@ -150,27 +150,37 @@ export async function upsertLinearCredentialProvider(args: { } /** - * Read the callback URL of an EXISTING provider, or null if there isn't one. + * Outcome of reading an EXISTING provider's callback URL — the vault path's Linear + * `redirect_uri`, which `linear app-template` prints so the operator need not find it. * - * Read-only and best-effort by design: this exists so `linear app-template` can - * print the real vault redirect_uri instead of asking the operator to go and find - * it. On a first run the provider does not exist yet, and the operator may have no - * credentials at all, so every failure is reported as "unknown" rather than - * raised — a template command must still work offline. + * Read-only and never throws: on a first run the provider does not exist yet and the + * operator may have no credentials at all, and a template command must still work. + * + * `absent` and `unreadable` used to be one `null`, which was harmless while every + * caller rendered the same text either way. They no longer do: `app-template` tells an + * operator to leave Linear's Redirect URIs field EMPTY when no provider exists, and that + * advice is wrong for a workspace whose provider exists but could not be read (expired + * credentials, wrong region). So the caller has to be able to tell them apart. */ -export async function lookupLinearVaultCallbackUrl(args: { +export type VaultCallbackProbe = + | { readonly kind: 'found'; readonly callbackUrl: string } + | { readonly kind: 'absent' } + | { readonly kind: 'unreadable'; readonly errorName: string }; + +export async function probeLinearVaultCallbackUrl(args: { region: string; workspaceSlug: string; -}): Promise { +}): Promise { try { const control = makeClient(BedrockAgentCoreControlClient, { region: args.region }); const existing = await control.send( new GetOauth2CredentialProviderCommand({ name: linearVaultProviderName(args.workspaceSlug) }), ); - return existing.callbackUrl ?? null; - } catch { - // nosemgrep: ts-silent-success-masking -- the only caller renders a template and prints a "NOT FOUND" line naming how to create the provider; "absent" and "unreadable" lead to identical output, and raising would make the command that explains onboarding unusable before onboarding - return null; + return existing.callbackUrl ? { kind: 'found', callbackUrl: existing.callbackUrl } : { kind: 'absent' }; + } catch (err) { + const errorName = (err as { name?: string } | undefined)?.name ?? 'Error'; + if (errorName === 'ResourceNotFoundException') return { kind: 'absent' }; + return { kind: 'unreadable', errorName }; } } diff --git a/cli/test/commands/linear-app-template-lookup.test.ts b/cli/test/commands/linear-app-template-lookup.test.ts index 39843d89b..0524e912b 100644 --- a/cli/test/commands/linear-app-template-lookup.test.ts +++ b/cli/test/commands/linear-app-template-lookup.test.ts @@ -44,7 +44,7 @@ jest.mock('@aws-sdk/client-cloudformation', () => { }); jest.mock('../../src/linear-vault'); -const lookupVaultCallback = jest.mocked(linearVault.lookupLinearVaultCallbackUrl); +const lookupVaultCallback = jest.mocked(linearVault.probeLinearVaultCallbackUrl); const HOSTED = 'https://d111111abcdef8.cloudfront.net/'; const VAULT = 'https://bedrock-agentcore.us-east-1.amazonaws.com/identities/oauth2/callback/f8804c1b'; @@ -67,14 +67,14 @@ beforeEach(() => { jest.restoreAllMocks(); cfnSend.mockReset(); lookupVaultCallback.mockReset(); - lookupVaultCallback.mockResolvedValue(null); + lookupVaultCallback.mockResolvedValue({ kind: 'absent' }); jest.spyOn(config, 'loadConfig').mockReturnValue({ region: 'us-east-1' } as never); }); describe('app-template callback discovery', () => { test('resolves BOTH URLs so the operator has to look up neither', async () => { stackServingConsentUrl(); - lookupVaultCallback.mockResolvedValue(VAULT); + lookupVaultCallback.mockResolvedValue({ kind: 'found', callbackUrl: VAULT }); await expect(resolveTemplateCallbackUrls({ stackName: 'backgroundagent-dev', slug: 'acme' })) .resolves.toEqual({ hostedConsentUrl: HOSTED, vaultCallbackUrl: VAULT }); }); @@ -138,14 +138,31 @@ describe('app-template callback discovery', () => { await expect(resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' })).resolves.toEqual({}); }); - test('an AUTH failure also degrades to no URL — a template must render without credentials', async () => { - // getStackOutput deliberately rethrows non-"does not exist" errors; this command - // is the one caller that must not propagate them, or `app-template` would be - // unusable before `aws configure`. + test('an AUTH failure renders, but SAYS it could not read the stack', async () => { + // getStackOutput rethrows non-"does not exist" errors. This command must not + // propagate them — `app-template` has to work before `aws configure` — but it used to + // swallow them silently, which rendered a VAULT stack as a no-vault one (#914 review). cfnSend.mockRejectedValue(Object.assign(new Error('Unable to locate credentials'), { name: 'CredentialsProviderError', })); - await expect(resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' })).resolves.toEqual({}); + const out = await resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' }); + expect(out.hostedConsentUrl).toBeUndefined(); + expect(out.lookupWarning).toMatch(/Could not read stack st in us-east-1 \(CredentialsProviderError\)/); + }); + + test('an UNREADABLE provider is a warning, not "no provider" — leave-empty would be wrong', async () => { + stackServingConsentUrl(); + lookupVaultCallback.mockResolvedValue({ kind: 'unreadable', errorName: 'AccessDeniedException' }); + const out = await resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' }); + expect(out.vaultCallbackUrl).toBeUndefined(); + expect(out.lookupWarning).toMatch(/vault provider for 'acme' \(AccessDeniedException\)/); + }); + + test('a genuinely absent provider raises no warning — that is the ordinary first run', async () => { + stackServingConsentUrl(); + lookupVaultCallback.mockResolvedValue({ kind: 'absent' }); + const out = await resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' }); + expect(out.lookupWarning).toBeUndefined(); }); test('a stack without the vault output yields no hosted URL (flag-off deploy)', async () => { @@ -163,14 +180,14 @@ describe('app-template callback discovery', () => { test('a not-yet-created provider yields no vault URL while still returning the hosted one', async () => { // The ordinary first run: the hosted page exists, the provider does not. stackServingConsentUrl(); - lookupVaultCallback.mockResolvedValue(null); + lookupVaultCallback.mockResolvedValue({ kind: 'absent' }); await expect(resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' })) .resolves.toEqual({ hostedConsentUrl: HOSTED }); }); test('looks the provider up by the slug it was given', async () => { stackWithoutConsentUrl(); - lookupVaultCallback.mockResolvedValue(VAULT); + lookupVaultCallback.mockResolvedValue({ kind: 'found', callbackUrl: VAULT }); await resolveTemplateCallbackUrls({ stackName: 'st', slug: 'acme' }); expect(lookupVaultCallback).toHaveBeenCalledWith({ region: 'us-east-1', workspaceSlug: 'acme' }); }); diff --git a/cli/test/commands/linear.test.ts b/cli/test/commands/linear.test.ts index 73ec9531d..8d2a72031 100644 --- a/cli/test/commands/linear.test.ts +++ b/cli/test/commands/linear.test.ts @@ -20,6 +20,7 @@ import { PutCommand, ScanCommand } from '@aws-sdk/lib-dynamodb'; import { autoLinkTokenOwner, + chooseSetupRedirect, findReusableOauthAppCredentials, findWorkspaceRowBySlug, isWebhookSecretConfigured, @@ -165,6 +166,10 @@ function redirectUriBlock(out: string): string { const lines = out.split('\n'); const start = lines.findIndex((l) => l.includes('Redirect URIs')); const end = lines.findIndex((l, i) => i > start && l.includes('Public:')); + // Without these a missing label makes slice(-1, end) return '', and every + // not.toContain assertion against the block passes vacuously. + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); return lines.slice(start, end).join('\n'); } @@ -199,6 +204,111 @@ describe('renderLinearAppTemplate', () => { expect(out).not.toMatch(/\[bot\]/); }); + /** + * The Redirect URIs field holds ONLY what Linear redirects to (#914). + * + * The live failure these pin: the template printed the hosted consent page as the + * sole Redirect URI, an operator registered exactly that, and `bgagent linear setup` + * still died on "Invalid redirect_uri parameter for the application" — because the + * consent page is AgentCore's `resourceOauth2ReturnUrl`, not Linear's `redirect_uri`. + * Linear redirects to the AgentCore provider callback on the vault path and to the + * CLI's loopback listener on the `add-workspace` path. + */ + const HOSTED_CONSENT = 'https://d111111abcdef8.cloudfront.net/'; + const VAULT_CALLBACK = + 'https://bedrock-agentcore.us-east-1.amazonaws.com/identities/oauth2/callback/f8804c1b'; + + test('on the vault path the field holds the AgentCore callback, not the consent page', () => { + // The bug in one assertion: on the vault path Linear redirects to the callback, so a + // field holding only the consent page left the URI Linear needs missing. (The consent + // page IS the redirect_uri on setup's Secrets-Manager fallback — see the next test.) + const block = redirectUriBlock(renderLinearAppTemplate({ + hostedConsentUrl: HOSTED_CONSENT, + vaultCallbackUrl: VAULT_CALLBACK, + })); + expect(block).not.toContain(HOSTED_CONSENT); + expect(block).toContain(VAULT_CALLBACK); + }); + + test('shows the consent page as a CONDITIONAL Redirect URI, outside the field', () => { + // #914 review: it was labelled "nothing to paste", but on setup's Secrets-Manager + // fallback it is exactly the redirect_uri Linear checks. It stays out of the field — + // the vault path does not use it — and says when it is needed. + const out = renderLinearAppTemplate({ hostedConsentUrl: HOSTED_CONSENT }); + expect(out).toContain(`Identity vault consent page: ${HOSTED_CONSENT}`); + expect(out).toMatch(/Add it as a Redirect URI only if setup falls back to Secrets Manager/); + expect(out).not.toMatch(/nothing to paste/); + expect(redirectUriBlock(out)).not.toContain(HOSTED_CONSENT); + }); + + test('every redirect_uri setup can send on a vault stack is one the template tells you to register', () => { + // The invariant blocker 1 broke: template and setup drifted apart in one file. Derive + // the fallback's redirect_uri from setup's own choice, not from a literal here. + const fallback = chooseSetupRedirect({ vaultAccessToken: undefined, consentPageUrl: HOSTED_CONSENT }); + expect(fallback.redirectUri).toBe(HOSTED_CONSENT); + const out = renderLinearAppTemplate({ hostedConsentUrl: HOSTED_CONSENT, vaultCallbackUrl: VAULT_CALLBACK }); + const lines = out.split('\n'); + const at = lines.findIndex((l) => l.includes(fallback.redirectUri)); + expect(at).toBeGreaterThanOrEqual(0); + expect(lines[at + 1]).toMatch(/Add it as a Redirect URI/); + // And the vault leg's URI is in the field itself. + expect(redirectUriBlock(out)).toContain(VAULT_CALLBACK); + }); + + test('the vault leg sends no consent-page redirect', () => { + const vault = chooseSetupRedirect({ vaultAccessToken: 'tok', consentPageUrl: HOSTED_CONSENT }); + expect(vault.hostedUrl).toBeUndefined(); + expect(vault.redirectUri).toBe('http://localhost:8080/oauth/callback'); + }); + + test('a failed stack lookup is SAID, rather than rendering a vault stack as no-vault', () => { + const out = renderLinearAppTemplate({ lookupWarning: 'Could not read stack st in us-east-1 (ExpiredToken).' }); + expect(out).toMatch(/⚠ Could not read stack st in us-east-1 \(ExpiredToken\)\./); + expect(out).toMatch(/may be wrong — check credentials and --region/); + }); + + test('names the loopback for add-workspace when the vault hides it from the field', () => { + const out = renderLinearAppTemplate({ hostedConsentUrl: HOSTED_CONSENT }); + expect(out).toMatch(/Using `add-workspace` instead of `setup`\? It redirects to http:\/\/localhost:8080\/oauth\/callback/); + // Not printed when the loopback is already the field's entry. + expect(renderLinearAppTemplate()).not.toMatch(/Using `add-workspace` instead/); + }); + + test('lists ONLY the AgentCore callback once the provider exists', () => { + // Listing the loopback beside it read as two required entries and sent vault + // operators to register a URI the vault path never redirects to. + const block = redirectUriBlock(renderLinearAppTemplate({ + hostedConsentUrl: HOSTED_CONSENT, + vaultCallbackUrl: VAULT_CALLBACK, + })); + expect(block).toContain(VAULT_CALLBACK); + expect(block).not.toContain('http://localhost:8080/oauth/callback'); + }); + + test('offers NO URI on a first vault run — the honest answer, not another flow\'s URI', () => { + // The callback id does not exist before setup registers the app, so there is nothing + // truthful to print. Printing the loopback instead gave a vault operator a URI that + // cannot work and no sign of the one that can. + const block = redirectUriBlock(renderLinearAppTemplate({ hostedConsentUrl: HOSTED_CONSENT })); + expect(block).not.toContain('http://localhost:8080/oauth/callback'); + expect(block).toMatch(/Leave empty/); + expect(block).toMatch(/setup ` prints the URI to add/); + }); + + test('every URI sits alone on its line, so no entry can wrap', () => { + // The traps list warns that a line-wrapped URI becomes two malformed entries, and + // `annotate` appends "← note" past a pad column — which on a ~100-char AgentCore + // callback would wrap it. So URI lines carry the URI and nothing else. + const block = redirectUriBlock(renderLinearAppTemplate({ + hostedConsentUrl: HOSTED_CONSENT, + vaultCallbackUrl: VAULT_CALLBACK, + })); + for (const line of block.split('\n').filter((l) => l.includes('://'))) { + expect(line.trim()).not.toMatch(/←/); + expect(line.trim().split(/\s+/)).toHaveLength(1); + } + }); + test('warns that redirect URIs match EXACTLY, including the trailing slash', () => { // The live cause of a real dead-end: the consent page URL ends in "/" and an // operator registered it without. Exact-string matching then rejects it. @@ -286,20 +396,18 @@ describe('renderLinearAppTemplate', () => { expect(renderLinearAppTemplate({ appName: ' ' })).toContain('Application name: bgagent'); }); - test('lists exactly the URIs the single setup command uses — no menu', () => { - // One command means one set of URIs. Listing per-command alternatives was the - // menu that got the wrong subset registered and produced an opaque - // "Invalid redirect_uri". + test('lists exactly the one URI Linear redirects to on the operator\'s own path', () => { + // Corrected with #914. On the vault path Linear redirects to the AgentCore callback, so + // that is the field's one entry. The loopback (add-workspace) and the consent page + // (setup's Secrets-Manager fallback) are named separately, each with when it applies. const out = renderLinearAppTemplate({ hostedConsentUrl: 'https://d111111abcdef8.cloudfront.net/', vaultCallbackUrl: 'https://bedrock-agentcore.us-east-1.amazonaws.com/identities/oauth2/callback/f88', }); - expect(out).toContain('https://d111111abcdef8.cloudfront.net/'); - expect(out).toContain('https://bedrock-agentcore.us-east-1.amazonaws.com/identities/oauth2/callback/f88'); - // The loopback is NOT one of the listed URIs when a hosted page exists — setup - // will not redirect to it. (A trap further down may still explain that the older - // add-workspace command does; that is guidance, not a field value.) - expect(redirectUriBlock(out)).not.toContain('http://localhost:8080/oauth/callback'); + const block = redirectUriBlock(out); + expect(block).toContain('https://bedrock-agentcore.us-east-1.amazonaws.com/identities/oauth2/callback/f88'); + expect(block).not.toContain('http://localhost:8080/oauth/callback'); + expect(block).not.toContain('https://d111111abcdef8.cloudfront.net/'); // And no vault-setup: that command no longer exists. expect(out).not.toContain('vault-setup'); }); @@ -307,44 +415,52 @@ describe('renderLinearAppTemplate', () => { test('falls back to the loopback URI only when there is no hosted page', () => { const out = renderLinearAppTemplate(); expect(out).toContain('http://localhost:8080/oauth/callback'); - expect(out).toMatch(/only works when your browser runs on the/); + expect(out).toMatch(/needs the browser on this machine/); expect(out).toContain('enableLinearIdentityVault=true'); }); - test('the hosted URI REPLACES the loopback rather than joining it', () => { - // Two URIs for two flows was the confusion; setup picks one substrate, so the - // template lists one. + test('neither the consent page nor the loopback appears in the field on a vault first run', () => { + // Inverted with #914. The old behaviour put the consent page in the field as its only + // entry, which the vault path never sends — so registering exactly what the template + // printed still failed consent. Both are now named outside the field with when they apply. const out = renderLinearAppTemplate({ hostedConsentUrl: 'https://d111111abcdef8.cloudfront.net/' }); - expect(out).toContain('https://d111111abcdef8.cloudfront.net/'); - expect(redirectUriBlock(out)).not.toContain('localhost:8080'); + const block = redirectUriBlock(out); + expect(block).not.toContain('https://d111111abcdef8.cloudfront.net/'); + expect(block).not.toContain('http://localhost:8080/oauth/callback'); }); - test('lists the hosted URI EXACTLY once, as the CLI sends it', () => { + test('prints the hosted consent URI EXACTLY once, with no slashless variant', () => { // An earlier version printed a second slashless variant as insurance against a // typo. Linear validates the whole Redirect URIs field on save, so an extra bad - // line loses the good ones with it — and the failure then reads as though it - // were about the line just added. One entry, matching what the CLI sends. + // line loses the good ones with it. Still worth pinning after #914 moved this URL + // out of the form: one occurrence, exactly as the stack exports it. const hosted = 'https://d111111abcdef8.cloudfront.net/'; const slashless = hosted.replace(/\/$/, ''); const out = renderLinearAppTemplate({ hostedConsentUrl: hosted }); - // Exact equality, not a prefix test: a prefix test on a URL reads as (incomplete) - // sanitization to static analysis, and equality is the stronger assertion anyway — - // it would also catch a THIRD variant being printed. - const lines = out.split('\n').map((l) => l.trim()); - expect(lines.filter((l) => l === hosted || l === slashless)).toEqual([hosted]); - }); - - test('with a hosted page but no vault callback, it says setup will print one', () => { - // The vault callback id does not exist until the provider is created, which - // `setup` does on its first run — so the template promises it rather than - // pretending it is unavailable. + // Count occurrences of each exact form rather than trimmed whole lines: the URL now + // sits inline after a label, so a line-equality test would find neither. + expect(out.split(hosted)).toHaveLength(2); + expect(out.split(`${slashless}\n`)).toHaveLength(1); + }); + + test('defers to setup in ONE line — the field is the only thing it talks about', () => { + // The callback id does not exist until setup creates the provider, so there is nothing + // to paste yet. Everything else an operator might want here (why the ordering is like + // that, how to recover the URI for a workspace already onboarded via `--slug`) belongs + // to setup and to the flag's own help: a first-time reader has no workspace yet, and + // every extra sentence competes with the fields that ARE needed to create the app. const out = renderLinearAppTemplate({ hostedConsentUrl: 'https://d111111abcdef8.cloudfront.net/' }); - expect(out).toMatch(/prints one more URI the first time it runs/); + expect(out).toMatch(/Leave empty — `bgagent linear setup ` prints the URI to add, then stops\./); + // Linear's form accepts an empty field (verified live), so no placeholder is needed. + expect(out).not.toMatch(/If Linear will not save an empty field/); + // The label plus one line of guidance, and no more. + const block = redirectUriBlock(out).split('\n').filter((l) => l.trim()); + expect(block).toHaveLength(2); }); test('without a hosted URL it explains the loopback limitation and the fix', () => { const out = renderLinearAppTemplate(); - expect(out).toMatch(/same machine as the CLI/); + expect(out).toMatch(/needs the browser on this machine/); expect(out).toContain('enableLinearIdentityVault=true'); }); diff --git a/cli/test/linear-vault.test.ts b/cli/test/linear-vault.test.ts index 150e44553..7721e0a36 100644 --- a/cli/test/linear-vault.test.ts +++ b/cli/test/linear-vault.test.ts @@ -57,7 +57,7 @@ import { beginVaultConsent, finalizeVaultConsent, isVaultUnavailableError, - lookupLinearVaultCallbackUrl, + probeLinearVaultCallbackUrl, mintLinearTokenFromVault, linearVaultProviderName, linearVaultUserId, @@ -327,32 +327,38 @@ describe('isVaultUnavailableError', () => { }); }); -describe('lookupLinearVaultCallbackUrl', () => { - // Read-only and best-effort by design: `app-template` calls it before anything - // exists, so every failure must read as "unknown" rather than raise. A throw here - // would make the command that explains onboarding the one that cannot run first. +describe('probeLinearVaultCallbackUrl', () => { + // Read-only and never throws: `app-template` calls it before anything exists, so a + // throw would make the command that explains onboarding the one that cannot run first. + // But "absent" and "unreadable" must stay distinct (#914 review): the template tells an + // operator to leave the Redirect URIs field EMPTY when no provider exists, which is the + // wrong advice for one that exists but could not be read. + const probe = (): ReturnType => + probeLinearVaultCallbackUrl({ region: 'us-east-1', workspaceSlug: 'acme' }); + test('returns the callback URL of an existing provider', async () => { controlSend.mockResolvedValueOnce({ callbackUrl: 'https://bedrock-agentcore.../callback/uuid' }); - await expect(lookupLinearVaultCallbackUrl({ region: 'us-east-1', workspaceSlug: 'acme' })) - .resolves.toBe('https://bedrock-agentcore.../callback/uuid'); + await expect(probe()).resolves.toEqual({ kind: 'found', callbackUrl: 'https://bedrock-agentcore.../callback/uuid' }); }); - test('a provider that does not exist yet yields null, not an error', async () => { + test('a provider that does not exist yet is absent, not an error', async () => { controlSend.mockRejectedValueOnce(Object.assign(new Error('nope'), { name: 'ResourceNotFoundException' })); - await expect(lookupLinearVaultCallbackUrl({ region: 'us-east-1', workspaceSlug: 'acme' })) - .resolves.toBeNull(); + await expect(probe()).resolves.toEqual({ kind: 'absent' }); }); - test('absent credentials or an unavailable service also yield null', async () => { + test('absent credentials are UNREADABLE, never absent — and still do not throw', async () => { controlSend.mockRejectedValueOnce(Object.assign(new Error('no creds'), { name: 'CredentialsProviderError' })); - await expect(lookupLinearVaultCallbackUrl({ region: 'us-east-1', workspaceSlug: 'acme' })) - .resolves.toBeNull(); + await expect(probe()).resolves.toEqual({ kind: 'unreadable', errorName: 'CredentialsProviderError' }); + }); + + test('a denied read is unreadable, not absent', async () => { + controlSend.mockRejectedValueOnce(Object.assign(new Error('denied'), { name: 'AccessDeniedException' })); + await expect(probe()).resolves.toEqual({ kind: 'unreadable', errorName: 'AccessDeniedException' }); }); - test('a provider without a callback URL yields null rather than an empty string', async () => { + test('a provider without a callback URL is absent rather than an empty string', async () => { controlSend.mockResolvedValueOnce({}); - await expect(lookupLinearVaultCallbackUrl({ region: 'us-east-1', workspaceSlug: 'acme' })) - .resolves.toBeNull(); + await expect(probe()).resolves.toEqual({ kind: 'absent' }); }); }); diff --git a/docs/guides/LINEAR_SETUP_GUIDE.md b/docs/guides/LINEAR_SETUP_GUIDE.md index e4b6685b9..a788fa297 100644 --- a/docs/guides/LINEAR_SETUP_GUIDE.md +++ b/docs/guides/LINEAR_SETUP_GUIDE.md @@ -70,7 +70,7 @@ This prints the exact field values to paste, with the URLs already resolved from Two fields deserve attention: -- **Redirect URIs** — paste exactly what the template prints, one per line. Linear compares these as exact strings, and reports any mismatch as a cryptic `Invalid redirect_uri parameter for the application`. Don't retype them, don't add variants, and don't let a line wrap into two entries. +- **Redirect URIs** — paste exactly what the template prints, one per line. Linear compares these as exact strings, and reports any mismatch as a cryptic `Invalid redirect_uri parameter for the application`. Don't retype them, don't add variants, and don't let a line wrap into two entries. On a stack with the Identity vault, a **first** run has nothing to paste yet: the URI Linear needs is the vault's callback, which `bgagent linear setup` creates, prints, and asks you to add before re-running. If setup instead falls back to Secrets Manager, it tells you to register the hosted consent page — the template lists that page separately for this reason. - **Webhooks** — turn this ON and fill in the URL the template prints, with **Issues** and **Comments** both ticked under *Data change events*. Leave every **App events** checkbox off (see the warning below). Then copy the **Webhook signing secret** (`lin_wh_…`); setup asks for it. Click **Create** and copy the **Client ID** and **Client Secret**. diff --git a/docs/src/content/docs/using/Linear-setup-guide.md b/docs/src/content/docs/using/Linear-setup-guide.md index 6460bf507..39d2ea5bf 100644 --- a/docs/src/content/docs/using/Linear-setup-guide.md +++ b/docs/src/content/docs/using/Linear-setup-guide.md @@ -74,7 +74,7 @@ This prints the exact field values to paste, with the URLs already resolved from Two fields deserve attention: -- **Redirect URIs** — paste exactly what the template prints, one per line. Linear compares these as exact strings, and reports any mismatch as a cryptic `Invalid redirect_uri parameter for the application`. Don't retype them, don't add variants, and don't let a line wrap into two entries. +- **Redirect URIs** — paste exactly what the template prints, one per line. Linear compares these as exact strings, and reports any mismatch as a cryptic `Invalid redirect_uri parameter for the application`. Don't retype them, don't add variants, and don't let a line wrap into two entries. On a stack with the Identity vault, a **first** run has nothing to paste yet: the URI Linear needs is the vault's callback, which `bgagent linear setup` creates, prints, and asks you to add before re-running. If setup instead falls back to Secrets Manager, it tells you to register the hosted consent page — the template lists that page separately for this reason. - **Webhooks** — turn this ON and fill in the URL the template prints, with **Issues** and **Comments** both ticked under *Data change events*. Leave every **App events** checkbox off (see the warning below). Then copy the **Webhook signing secret** (`lin_wh_…`); setup asks for it. Click **Create** and copy the **Client ID** and **Client Secret**.