diff --git a/packages/bot-runner/lib/command-runner.ts b/packages/bot-runner/lib/command-runner.ts index c43ec5a01a0..b52b2ef5e13 100644 --- a/packages/bot-runner/lib/command-runner.ts +++ b/packages/bot-runner/lib/command-runner.ts @@ -51,6 +51,9 @@ export function makeEnqueueRunCommand( runAs, command, commandInput, + // Interactive command: a command error is a normal result to hand back + // to the user, not a job failure. + alertOnError: false, }, queuePublisher, dbAdapter, diff --git a/packages/bot-runner/tests/bot-runner-test.ts b/packages/bot-runner/tests/bot-runner-test.ts index 164c327a5c0..5f2a1f2b0ec 100644 --- a/packages/bot-runner/tests/bot-runner-test.ts +++ b/packages/bot-runner/tests/bot-runner-test.ts @@ -229,6 +229,7 @@ module('timeline handler', () => { runAs: '@alice:localhost', command: '@cardstack/boxel-host/commands/show-card/default', commandInput: {}, + alertOnError: false, }, }, 'enqueues expected command payload', diff --git a/packages/bot-runner/tests/command-runner-test.ts b/packages/bot-runner/tests/command-runner-test.ts index cd7f55a310c..96304201fc7 100644 --- a/packages/bot-runner/tests/command-runner-test.ts +++ b/packages/bot-runner/tests/command-runner-test.ts @@ -133,6 +133,7 @@ module('command runner', () => { runAs: '@alice:localhost', command: '@cardstack/boxel-host/commands/show-card/default', commandInput: { cardId: 'http://localhost:4201/test/Person/1' }, + alertOnError: false, }, }, 'publishes expected run-command payload', @@ -337,6 +338,7 @@ module('command runner', () => { }, ], }, + alertOnError: false, }, 'enqueues PR card creation in submissions realm', ); @@ -359,6 +361,7 @@ module('command runner', () => { }, }, }, + alertOnError: false, }, 'persists prCard link on the workflow card immediately after create-pr-card succeeds (so retry on later failure can reuse the existing PrCard)', ); @@ -385,6 +388,7 @@ module('command runner', () => { }, }, }, + alertOnError: false, }, 'clears prior error attributes on the workflow card after the GitHub PR succeeds, re-asserting the prCard link to survive any stale-fetch race', ); diff --git a/packages/realm-server/handlers/handle-run-command.ts b/packages/realm-server/handlers/handle-run-command.ts index 6b9d594a94b..7badcc042b6 100644 --- a/packages/realm-server/handlers/handle-run-command.ts +++ b/packages/realm-server/handlers/handle-run-command.ts @@ -82,6 +82,9 @@ export default function handleRunCommand({ runAs: userId, command, commandInput: commandInput ?? null, + // Interactive endpoint: a command error is returned to the caller, + // not treated as a job failure. + alertOnError: false, }, queue, dbAdapter, diff --git a/packages/realm-server/handlers/handle-webhook-receiver.ts b/packages/realm-server/handlers/handle-webhook-receiver.ts index 7699836e7ff..8a7063d5189 100644 --- a/packages/realm-server/handlers/handle-webhook-receiver.ts +++ b/packages/realm-server/handlers/handle-webhook-receiver.ts @@ -157,6 +157,9 @@ export default function handleWebhookReceiverRequest({ runAs, command: commandURL, commandInput, + // Webhook-triggered command: an error is handled per-command by the + // caller, not surfaced as a job failure. + alertOnError: false, }, queue, dbAdapter, diff --git a/packages/realm-server/scripts/sync-openrouter-models.ts b/packages/realm-server/scripts/sync-openrouter-models.ts index c29b510b51e..dde72367f96 100644 --- a/packages/realm-server/scripts/sync-openrouter-models.ts +++ b/packages/realm-server/scripts/sync-openrouter-models.ts @@ -55,6 +55,10 @@ export async function enqueueSyncOpenRouterModels({ runAs: REALM_USERNAME, command: COMMAND_SPECIFIER, commandInput: { realmIdentifier: realmURL }, + // System-initiated cron job: a command error must reject the job (Sentry + + // rejected status) rather than resolve silently, so a broken sync surfaces + // instead of quietly leaving the model realm stale. + alertOnError: true, }; try { diff --git a/packages/realm-server/tests/run-command-task-test.ts b/packages/realm-server/tests/run-command-task-test.ts index 16c1117eb66..816273db587 100644 --- a/packages/realm-server/tests/run-command-task-test.ts +++ b/packages/realm-server/tests/run-command-task-test.ts @@ -21,5 +21,13 @@ module(basename(import.meta.filename), function () { test('passes scoped command through unchanged', async function (assert) { await runSharedTest(runCommandTaskTests, assert, {}); }); + + test('throws when alertOnError is set and the command returns an error', async function (assert) { + await runSharedTest(runCommandTaskTests, assert, {}); + }); + + test('throws when alertOnError is set and runAs lacks permissions', async function (assert) { + await runSharedTest(runCommandTaskTests, assert, {}); + }); }); }); diff --git a/packages/runtime-common/tasks/run-command.ts b/packages/runtime-common/tasks/run-command.ts index d664480c933..8d37243f7f6 100644 --- a/packages/runtime-common/tasks/run-command.ts +++ b/packages/runtime-common/tasks/run-command.ts @@ -17,6 +17,15 @@ export interface RunCommandArgs extends JSONTypes.Object { runAs: string; command: string; commandInput: JSONTypes.Object | null; + // When true, a command that finishes with an error status makes the job + // throw rather than resolving with the error in-band. The queue then marks + // the job rejected and reports it to Sentry. Interactive callers (bot-runner, + // the run-command HTTP endpoint, webhooks) pass false so a command error + // stays a normal result to hand back to the user. System-initiated jobs + // (cron syncs) pass true so a persistently failing job is never silent. + // Required (not optional) so the args object satisfies JSONTypes.Object's + // JSON-shape index signature, which excludes `undefined`. + alertOnError: boolean; } // No coalesce handler: each run-command enqueue is a distinct invocation @@ -35,8 +44,27 @@ const runCommand: Task = ({ matrixURL, }) => async function (args) { - let { jobInfo, realmURL, realmUsername, runAs, command, commandInput } = - args; + let { + jobInfo, + realmURL, + realmUsername, + runAs, + command, + commandInput, + alertOnError, + } = args; + // Turn an error outcome into either a thrown failure (so the queue rejects + // the job and reports it to Sentry) or an in-band error result, depending + // on whether the caller asked to be alerted. + let fail = (message: string): RunCommandResponse => { + if (alertOnError) { + throw new Error(message); + } + return { + status: 'error', + error: message, + }; + }; log.debug( `${jobIdentity(jobInfo)} starting run-command for job: ${JSON.stringify({ realmURL, @@ -58,10 +86,7 @@ const runCommand: Task = ({ let message = `${jobIdentity(jobInfo)} ${runAs} does not have permissions in ${normalizedRealmURL}`; log.error(message); reportStatus(jobInfo, 'finish'); - return { - status: 'error', - error: message, - }; + return fail(message); } // Include JWTs for all realms the user has access to @@ -82,10 +107,7 @@ const runCommand: Task = ({ let message = `${jobIdentity(jobInfo)} invalid command specifier`; log.error(message, { command, realmURL: normalizedRealmURL }); reportStatus(jobInfo, 'finish'); - return { - status: 'error', - error: message, - }; + return fail(message); } let augmentedCommandInput = commandInput @@ -101,6 +123,13 @@ const runCommand: Task = ({ }); reportStatus(jobInfo, 'finish'); + + if (alertOnError && result.status === 'error') { + let message = `${jobIdentity(jobInfo)} command ${command} failed in ${normalizedRealmURL}: ${result.error ?? 'unknown error'}`; + log.error(message); + throw new Error(message); + } + return result; }; diff --git a/packages/runtime-common/tests/run-command-task-shared-tests.ts b/packages/runtime-common/tests/run-command-task-shared-tests.ts index 455535921e5..7f2f7ed0099 100644 --- a/packages/runtime-common/tests/run-command-task-shared-tests.ts +++ b/packages/runtime-common/tests/run-command-task-shared-tests.ts @@ -109,6 +109,7 @@ const tests = Object.freeze({ runAs: '@alice:localhost', command: '@cardstack/boxel-host/commands/show-card/default', commandInput: {}, + alertOnError: false, jobInfo: { id: 1 } as any, }); @@ -150,6 +151,7 @@ const tests = Object.freeze({ runAs: '@alice:localhost', command: ' ', commandInput: {}, + alertOnError: false, jobInfo: { id: 2 } as any, }); @@ -201,6 +203,7 @@ const tests = Object.freeze({ runAs: '@alice', command: 'http://localhost:4200/commands/create-submission', commandInput: null, + alertOnError: false, jobInfo: { id: 3 } as any, }); @@ -255,6 +258,7 @@ const tests = Object.freeze({ runAs: '@alice:localhost', command: '@cardstack/catalog/commands/create-submission/default', commandInput: { listingId: 'http://localhost:4201/catalog/AppListing/1' }, + alertOnError: false, jobInfo: { id: 4 } as any, }); @@ -267,6 +271,64 @@ const tests = Object.freeze({ accessibleRealms: ['http://localhost:4201/experiments/'], }); }, + + 'throws when alertOnError is set and the command returns an error': async ( + assert, + ) => { + assert.expect(2); + let task = runCommand( + makeTaskArgs({ + dbRows: [ + { + username: '@alice:localhost', + realm_url: 'http://localhost:4201/experiments/', + read: true, + write: true, + realm_owner: false, + }, + ], + prerenderResult: { status: 'error', error: 'boom' }, + }), + ); + + // A thrown task lets the queue mark the job rejected and report it to + // Sentry, instead of resolving with the error swallowed in-band. + await assert.rejects( + task({ + realmURL: 'http://localhost:4201/experiments/', + realmUsername: '@alice:localhost', + runAs: '@alice:localhost', + command: '@cardstack/catalog/commands/sync-openrouter-models/default', + commandInput: {}, + alertOnError: true, + jobInfo: { id: 5 } as any, + }), + /boom/, + 'rejects with the command error when alertOnError is set', + ); + assert.true(true, 'did not resolve in-band'); + }, + + 'throws when alertOnError is set and runAs lacks permissions': async ( + assert, + ) => { + assert.expect(1); + let task = runCommand(makeTaskArgs({ dbRows: [] })); + + await assert.rejects( + task({ + realmURL: 'http://localhost:4201/experiments/', + realmUsername: '@alice:localhost', + runAs: '@alice:localhost', + command: '@cardstack/catalog/commands/sync-openrouter-models/default', + commandInput: {}, + alertOnError: true, + jobInfo: { id: 6 } as any, + }), + /does not have permissions in/, + 'a permission failure rejects the job when alertOnError is set', + ); + }, } as SharedTests<{}>); export default tests;