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**.