From cb601b3034a67ebbbacc05596c9f502369f09799 Mon Sep 17 00:00:00 2001 From: bgagent Date: Tue, 22 Sep 2026 18:45:32 -0400 Subject: [PATCH] fix(cli): print the URI Linear actually redirects to (#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 Linear Redirect URI. Linear never redirects there — it is AgentCore's `resourceOauth2ReturnUrl`, where AgentCore sends the browser AFTER Linear redirects, to show the session id. So an operator who registered exactly what the template printed still failed consent with "Invalid redirect_uri parameter for the application", and the template's own traps list blamed a trailing slash. Linear redirects to the AgentCore provider callback on the `setup` path and to the CLI's loopback listener on `add-workspace`. The field now holds only that, one line per state: - provider exists -> the AgentCore callback, alone - vault, first run -> "Leave empty — `setup ` prints the URI to add" - no vault -> the loopback listener The consent page is still printed, after the form and labelled "nothing to paste", because it is how an operator confirms the vault is deployed. `setup` now prints the callback on EVERY consent, not only the run that created the provider. `provider.created` was standing in for "this URI is registered on the Linear app": true once per provider lifetime, while the real condition stays false until a human pastes it in. Anyone who interrupted the first run, or lost the scrollback, got no remedy on any later run — just an authorize URL that dies in the browser. Four existing tests asserted the old model, one named "the hosted URI REPLACES the loopback rather than joining it". They are corrected rather than deleted, since they record which mistake was being prevented. A new test pins the deferred state at exactly two lines so the explanation cannot grow back. Verified against a live vault stack and a real Linear workspace: the field now renders the same two-URI list that had to be assembled by hand to get consent through. 995 CLI tests, 64 suites. Refs #914 --- cli/src/commands/linear.ts | 116 +++++++++++++++++-------- cli/test/commands/linear.test.ts | 140 ++++++++++++++++++++++++------- 2 files changed, 187 insertions(+), 69 deletions(-) diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts index 482aec1d0..41cbb916a 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -222,7 +222,11 @@ export async function resolveTemplateCallbackUrls(args: { region?: string; stackName?: string; slug?: string; -}): Promise<{ hostedConsentUrl?: string; vaultCallbackUrl?: string; webhookUrl?: string }> { +}): Promise<{ + hostedConsentUrl?: string; + vaultCallbackUrl?: string; + webhookUrl?: string; +}> { // Unconfigured CLI is an expected state here, not an error. let config: CliConfig | undefined; try { @@ -240,8 +244,10 @@ export async function resolveTemplateCallbackUrls(args: { 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. + // Needs a slug: the provider is per-workspace, so without one there is nothing specific to + // look up. Inferring it from the registry belongs with the workspace-enumeration helpers + // landing in #917 (`listOnboardedWorkspaceSlugs`, which also falls back to Secrets Manager + // when the registry is empty) rather than in a second copy here. const vaultCallbackUrl = args.slug ? await lookupLinearVaultCallbackUrl({ region, workspaceSlug: args.slug }) : null; @@ -283,34 +289,55 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st ` 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". + // separated with newlines". ONLY URIs Linear itself redirects to belong here: + // it validates the `redirect_uri` on the authorize request against this list, + // so an entry Linear never sends to is inert, and a missing one is an opaque + // "Invalid redirect_uri". + // + // The hosted consent page is NOT one of them, and printing it here cost a real + // onboarding: it is AgentCore's `resourceOauth2ReturnUrl` (where AgentCore sends + // the browser AFTER Linear redirects, to show the session id — see + // `beginVaultConsent`), never Linear's `redirect_uri`. On the vault path Linear + // redirects to the AgentCore provider callback; on the `add-workspace` path it + // redirects to the CLI's own loopback listener. It is described below the block + // instead, so it cannot be pasted into this field. ' 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 + // Exactly the URIs the CLI sends, and no variants of them. 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 - ? [] - : [ - '', - ' `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.', - ]), + // strictly safer here; each line below is a string some flow actually sends. + // + // Loopback stays listed even when the vault is deployed: it is the working + // `redirect_uri` for `add-workspace`, Linear requires at least one entry to + // create the app, and on a first run the vault callback does not exist yet — so + // dropping it would leave the field empty at exactly the moment the operator is + // filling the form. + // ONE URI, and only ever the one the operator's own path actually uses. Listing a + // second flow's URI alongside it reads as two required fields and was the confusion + // that made this command hard to follow: a vault operator was told to register the + // loopback (which setup never redirects to) and, on a first run, was given nothing + // for the URI Linear does need. + // + // Bare URI per line, never annotated: `annotate` appends "← note" past a pad column, + // and the AgentCore callback runs ~100 chars, so an inline note would wrap the line — + // producing exactly the two-malformed-entries failure the traps below warn about. + ...(opts.vaultCallbackUrl + // Provider exists: this is the only URI Linear redirects to. + ? [` ${opts.vaultCallbackUrl}`] + : opts.hostedConsentUrl + // First run: the callback id is minted by setup, so there is nothing to paste yet. + // Deliberately one line — the ordering is setup's to explain, at the moment it + // hands over the URI. Recovering a callback for an already-onboarded workspace is + // `--slug`, documented on the flag, not here: a first-time reader does not have a + // workspace yet and every extra sentence competes with the fields that ARE needed. + ? [' Leave empty — `bgagent linear setup ` prints the URI to add, then stops.'] + // No vault: the CLI's own loopback listener genuinely is the redirect_uri. + : [ + ` ${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 +350,16 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st 'Click Create, then come back with the Client ID, Client Secret and that', 'signing secret: bgagent linear setup ', '', + // Printed AFTER the form, never inside it. Linear does not redirect to the consent + // page, so listing it among the Redirect URIs registers a URI no flow uses while the + // one Linear needs stays missing — which is exactly the dead end #914 fixes. It is + // still worth showing: it is how an operator confirms the vault is deployed. + ...(opts.hostedConsentUrl + ? [ + `Identity vault consent page (nothing to paste): ${opts.hostedConsentUrl}`, + '', + ] + : []), // 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. @@ -341,14 +378,6 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st ` • The app name is not the trigger: that is always \`${COMMENT_TRIGGER_TOKEN} \`, however`, ' 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.', - ] - : []), bar, ].join('\n'); } @@ -1023,9 +1052,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(''); diff --git a/cli/test/commands/linear.test.ts b/cli/test/commands/linear.test.ts index 73ec9531d..1c83e90cd 100644 --- a/cli/test/commands/linear.test.ts +++ b/cli/test/commands/linear.test.ts @@ -199,6 +199,75 @@ 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('never lists the hosted consent page as a Redirect URI', () => { + // The whole bug in one assertion: Linear never redirects to the consent page, so + // registering it satisfies nothing while the URI Linear needs stays missing. + const block = redirectUriBlock(renderLinearAppTemplate({ + hostedConsentUrl: HOSTED_CONSENT, + vaultCallbackUrl: VAULT_CALLBACK, + })); + expect(block).not.toContain(HOSTED_CONSENT); + expect(block).toContain(VAULT_CALLBACK); + }); + + test('still shows the consent page, outside the block and marked not-a-Redirect-URI', () => { + // Worth printing — it is how an operator confirms the vault is live — but it must + // not read as something to paste into the form. + const out = renderLinearAppTemplate({ hostedConsentUrl: HOSTED_CONSENT }); + expect(out).toContain(HOSTED_CONSENT); + expect(out).toMatch(/Identity vault consent page \(nothing to paste\)/); + expect(redirectUriBlock(out)).not.toContain(HOSTED_CONSENT); + }); + + test('lists ONLY the AgentCore callback once the provider exists', () => { + // One flow, one URI. Listing the loopback beside it read as two required entries and + // sent vault operators to register a URI `setup` 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 +355,20 @@ 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. This test previously asserted the opposite: that the hosted + // consent page belonged in this field and the loopback did not. Linear validates + // `redirect_uri` against this list, and it redirects to the AgentCore callback on the + // vault path and to the CLI's loopback listener on `add-workspace` — so both belong, + // and the consent page (an AgentCore return URL) does not. 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 +376,51 @@ 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 swapped the loopback out for the consent page + // when the vault was deployed, leaving the field holding only a URI Linear never + // redirects to — so registering exactly what the template printed still failed consent. + // Neither belongs there: one is an AgentCore return URL, the other another flow's URI. 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\./); + // The label plus exactly 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'); });