Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/bot-runner/lib/command-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions packages/bot-runner/tests/bot-runner-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 4 additions & 0 deletions packages/bot-runner/tests/command-runner-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -337,6 +338,7 @@ module('command runner', () => {
},
],
},
alertOnError: false,
},
'enqueues PR card creation in submissions realm',
);
Expand All @@ -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)',
);
Expand All @@ -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',
);
Expand Down
3 changes: 3 additions & 0 deletions packages/realm-server/handlers/handle-run-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions packages/realm-server/handlers/handle-webhook-receiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions packages/realm-server/scripts/sync-openrouter-models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 8 additions & 0 deletions packages/realm-server/tests/run-command-task-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, {});
});
});
});
49 changes: 39 additions & 10 deletions packages/runtime-common/tasks/run-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -35,8 +44,27 @@ const runCommand: Task<RunCommandArgs, RunCommandResponse> = ({
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,
Expand All @@ -58,10 +86,7 @@ const runCommand: Task<RunCommandArgs, RunCommandResponse> = ({
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
Expand All @@ -82,10 +107,7 @@ const runCommand: Task<RunCommandArgs, RunCommandResponse> = ({
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
Expand All @@ -101,6 +123,13 @@ const runCommand: Task<RunCommandArgs, RunCommandResponse> = ({
});

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);
}
Comment on lines +127 to +131

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Good catch — fixed. Moved reportStatus(jobInfo, 'finish') to before the alert-on-error throw so every terminal path reports finish first, matching the permission-failure and invalid-specifier branches (which already report finish before fail() throws). Status/progress consumers now see the job finish regardless of whether it then rejects.


return result;
};

Expand Down
62 changes: 62 additions & 0 deletions packages/runtime-common/tests/run-command-task-shared-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});

Expand Down Expand Up @@ -150,6 +151,7 @@ const tests = Object.freeze({
runAs: '@alice:localhost',
command: ' ',
commandInput: {},
alertOnError: false,
jobInfo: { id: 2 } as any,
});

Expand Down Expand Up @@ -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,
});

Expand Down Expand Up @@ -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,
});

Expand All @@ -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;
Loading