Skip to content

refactor: convert automations, autofix, and webhooks to Hono sub-apps #1728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ColeMurray merged 4 commits into from
Sep 3, 2026
Merged

refactor: convert automations, autofix, and webhooks to Hono sub-apps #1728

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e1f334
refactor: convert automations, autofix, and webhooks to Hono sub-apps
ColeMurray Sep 3, 2026
5659765
test: give the automation lookup mock a per-test default
ColeMurray Sep 3, 2026
59e0834
refactor: register the Slack event route directly and share the autho…
ColeMurray Sep 3, 2026
3625739
test: name the fixture user once and answer batches positionally
ColeMurray Sep 3, 2026
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
25 changes: 13 additions & 12 deletions packages/control-plane/src/routes/autofix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
import { Hono } from "hono";
import { PrAutofixFeedbackStore } from "../db/pr-autofix-feedback-store";
import { admit } from "../routing/admit";
import type { ControlPlaneHonoEnv } from "../routing/hono-env";
import {
defineRoutes,
error,
json,
NO_AUTHORIZATION,
SCM_AGNOSTIC_WEB_SERVICE_ROUTE,
type Route,
type RequestContext,
} from "./shared";

const handleActivity: Route["handler"] = async (request, _env, _match, ctx) => {
async function handleActivity(request: Request, ctx: RequestContext): Promise<Response> {
const url = new URL(request.url);
const rawLimit = url.searchParams.get("limit") ?? "50";
const limit = Number(rawLimit);
Expand All @@ -29,13 +31,12 @@ const handleActivity: Route["handler"] = async (request, _env, _match, ctx) => {
}
throw caught;
}
};
}

export const autofixRoutes: Route[] = defineRoutes(SCM_AGNOSTIC_WEB_SERVICE_ROUTE, [
{
method: "GET",
path: "/autofix/activity",
authorization: NO_AUTHORIZATION,
handler: handleActivity,
},
]);
export const autofixRoutes = new Hono<ControlPlaneHonoEnv>();

autofixRoutes.get(
"/autofix/activity",
admit({ ...SCM_AGNOSTIC_WEB_SERVICE_ROUTE, authorization: NO_AUTHORIZATION }),
(c) => handleActivity(c.var.admitted.request, c.var.admitted.ctx)
);
201 changes: 112 additions & 89 deletions packages/control-plane/src/routes/automations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,40 @@
/**
* Unit tests for automation CRUD route handlers.
* Unit tests for automation CRUD routes.
*
* Tests run in Node (not workerd) with mocked AutomationStore and source
* control. Handler functions are extracted from the exported automationRoutes
* array and invoked directly, bypassing the auth middleware.
* control. Requests dispatch through the production `automationRoutes`
* module, so admission (including the automation ownership requirement)
* runs; authentication is mocked to supply the principal.
*/

import { describe, it, expect, vi, beforeEach } from "vitest";
import type * as AuthenticateModule from "../auth/authenticate";
import { automationRoutes } from "./automations";
import { HttpError, resolveRepoOrError, type RequestContext } from "./shared";
import { HttpError, resolveRepoOrError } from "./shared";
import type { Principal } from "../auth/principal";
import type { SqlDatabase } from "../db/sql-database";
import type { SqlDatabase, SqlStatement } from "../db/sql-database";
import type { Env } from "../types";
import { routePathPattern, TEST_BACKGROUND_TASK_CONTEXT } from "../router.test-support";
import {
createTestRequestHandler,
TEST_BACKGROUND_TASK_CONTEXT,
TEST_SERVICE_SECRETS,
} from "../router.test-support";
import {
AutomationExecutionUnauthorizedError,
AutomationTriggerBlockedError,
} from "../scheduler/scheduler";
import { PERMISSION_IDS, type PermissionId } from "@open-inspect/shared/rbac";
import {
BUILT_IN_ROLE_REGISTRY,
PERMISSION_IDS,
type PermissionId,
} from "@open-inspect/shared/rbac";

const mocks = vi.hoisted(() => ({ authenticate: vi.fn() }));

vi.mock("../auth/authenticate", async (importOriginal) => ({
...(await importOriginal<typeof AuthenticateModule>()),
authenticate: mocks.authenticate,
}));

const mockProviderAdapterGet = vi.hoisted(() => vi.fn());
const mockResolveGitHubCredentialAuthority = vi.hoisted(() => vi.fn());
Expand All @@ -40,6 +57,7 @@ vi.mock("../session/identity", () => ({
const mockStore = {
list: vi.fn(),
getById: vi.fn(),
resolveCanonicalOwner: vi.fn(async (automation: unknown) => automation),
update: vi.fn(),
softDelete: vi.fn(),
pause: vi.fn(),
Expand Down Expand Up @@ -169,13 +187,55 @@ vi.mock("./shared", async (importOriginal) => {

// ─── Helpers ────────────────────────────────────────────────────────────────

function createEnv(): Env {
/**
* The workspace database as admission and the handlers see it: admission's
* role lookups are answered here, every other statement goes to a statement
* spy, and `batch` is the shared spy.
*/
function createDatabase(permissions: readonly PermissionId[]): SqlDatabase {
const custom = permissions.length !== PERMISSION_IDS.length;
const role = custom
? { role_id: "role-1", role_key: null, role_name: "Custom" }
: { role_id: BUILT_IN_ROLE_REGISTRY.owner.id, role_key: "owner", role_name: "Owner" };
const statement = {
bind: vi.fn(() => statement),
first: vi.fn(async () => ({ satisfied: 1 })),
all: vi.fn(async () => ({ results: [] })),
};
return {
DB: { batch: mockBatch } as unknown as D1Database,
prepare(sql: string) {

@open-inspect open-inspect Bot Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[deep review] This feature-local fixture now recognizes authorization by literal SQL fragments and reconstructs role/permission rows, duplicating the same query-sensitive knowledge already centralized in ownerAuthorizationDatabase in router.test-support.ts. That duplication adds another maintenance point to an already 1,895-line suite: any authorization-query change can silently break fixtures independently. Please extend the canonical helper with permissions/batch/fallback-statement options and reuse it here rather than embedding admission SQL internals in the automation tests.

@ColeMurray ColeMurray Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done in 59e0834. router.test-support.ts now owns authorizationDatabase({ userId, permissions, statement, batch }), which recognizes admission's two lookups in one place; ownerAuthorizationDatabase() is the owner shortcut over it. The automations suite passes its statement spy and batch mock through and carries no SQL knowledge of its own.

if (sql.includes("FROM users u") || sql.includes("FROM role_permissions")) {
const admission: SqlStatement = {
bind: () => admission,
first: async <T>() =>
(sql.includes("FROM users u")
? { user_id: "user-1", suspended_at: null, ...role }
: null) as T | null,
all: async <T>() => ({
results: (sql.includes("FROM role_permissions")
? permissions.map((permission_id) => ({ permission_id }))
: []) as T[],
meta: { changes: 0 },
}),
run: async <T>() => ({ results: [] as T[], meta: { changes: 0 } }),
};
return admission;
}
return statement as unknown as SqlStatement;
},
batch: mockBatch,
} as unknown as SqlDatabase;
}

function createEnv(permissions: readonly PermissionId[] = PERMISSION_IDS): Env {
return {
...TEST_SERVICE_SECRETS,
SCM_PROVIDER: "github",
DB: createDatabase(permissions),
SESSION: {} as DurableObjectNamespace,
DEPLOYMENT_NAME: "test",
TOKEN_ENCRYPTION_KEY: "test-key",
} as Env;
} as unknown as Env;
}

const USER_PRINCIPAL: Principal = {
Expand All @@ -194,39 +254,7 @@ const SLACK_BOT_PRINCIPAL: Principal = {
},
};

function createCtx(
principal: Principal = USER_PRINCIPAL,
permissions: readonly PermissionId[] = PERMISSION_IDS
): RequestContext {
const statement = {
bind: vi.fn(() => statement),
first: vi.fn(async () => ({ satisfied: 1 })),
all: vi.fn(async () => ({ results: [] })),
};
return {
trace_id: "trace-1",
request_id: "req-1",
principal,
...(principal.kind === "user"
? {
authorization: {
userId: principal.userId,
suspendedAt: null,
role: { id: "role_builtin_owner", key: "owner" as const, name: "Owner" },
permissions: [...permissions],
},
}
: {}),
db: { batch: mockBatch, prepare: vi.fn(() => statement) } as unknown as SqlDatabase,
executionCtx: TEST_BACKGROUND_TASK_CONTEXT,
metrics: {
d1Queries: [],
spans: {},
time: async <T>(_name: string, fn: () => Promise<T>) => fn(),
summarize: () => ({}),
},
};
}
const handleRequest = createTestRequestHandler([automationRoutes]);

async function callRoute(
method: string,
Expand All @@ -238,11 +266,6 @@ async function callRoute(
permissions?: readonly PermissionId[];
}
): Promise<Response> {
const route = automationRoutes.find(
(candidate) => candidate.method === method && routePathPattern(candidate.path).test(path)
);
if (!route) throw new Error(`No route found for ${method} ${path}`);
const match = path.match(routePathPattern(route.path))!;
const url = new URL(`https://test.local${path}`);
if (options?.query) {
for (const [k, v] of Object.entries(options.query)) {
Expand All @@ -256,20 +279,13 @@ async function callRoute(
init.headers = { "Content-Type": "application/json" };
init.body = JSON.stringify(options.body);
}
const ctx = createCtx(options?.principal, options?.permissions);
const automationRequirement =
route.authorization.kind === "active-user"
? route.authorization.allOf.find((requirement) => requirement.kind === "automation")
: undefined;
if (automationRequirement) {
const automation = await mockStore.getById(
match.groups?.[automationRequirement.automationIdParam]
);
if (!automation)
return new Response(JSON.stringify({ error: "Automation not found" }), { status: 404 });
ctx.automationAdmission = { automation };
}
return route.handler(new Request(url, init), createEnv(), match, ctx);
const principal = options?.principal ?? USER_PRINCIPAL;
mocks.authenticate.mockImplementation(async (request: Request) => ({ principal, request }));
return handleRequest(

@open-inspect open-inspect Bot Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This helper now runs requireAutomation admission, so mockStore.getById is part of the setup for manage routes. beforeEach uses vi.clearAllMocks(), which preserves mock implementations, but it does not restore getById. As a result, the full file passes because an earlier PUT test leaves sampleRow, while selecting soft-deletes automation alone fails with a 404. Please establish an explicit getById default in beforeEach (and override it in missing-automation cases), or reset and rebuild all mock defaults per test.

@ColeMurray ColeMurray Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed and fixed in 5659765: the isolated run answered 404 exactly as described. beforeEach now sets getById to the sample row and the missing-automation cases override it to null.

new Request(url, init),
createEnv(options?.permissions),
TEST_BACKGROUND_TASK_CONTEXT
);
}

// ─── Sample data ────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -301,6 +317,9 @@ describe("automation route handlers", () => {
vi.clearAllMocks();
// Defaults every test can override; re-set here so per-test overrides
// (mockClear keeps implementations) cannot leak across tests.
// Admission resolves the automation for every manage route, so the
// lookup must not depend on what an earlier test left behind.
mockStore.getById.mockResolvedValue(sampleRow);
mockStore.getRepositoriesForAutomation.mockResolvedValue([]);
mockStore.getRepositoriesForAutomationIds.mockResolvedValue(new Map());
mockStore.getEnvironmentsForAutomation.mockResolvedValue([]);
Expand Down Expand Up @@ -587,19 +606,19 @@ describe("automation route handlers", () => {
};
});

await expect(
callRoute("POST", "/automations", {
body: {
...validBody,
repositories: [
{ repoOwner: "acme", repoName: "web-app" },
{ repoOwner: "acme", repoName: "api" },
],
},
})
).rejects.toMatchObject({
status: 404,
message: "Repository is not installed for the GitHub App",
const res = await callRoute("POST", "/automations", {
body: {
...validBody,
repositories: [
{ repoOwner: "acme", repoName: "web-app" },
{ repoOwner: "acme", repoName: "api" },
],
},
});

expect(res.status).toBe(404);
await expect(res.json()).resolves.toEqual({
error: "Repository is not installed for the GitHub App",
});
expect(mockStore.bindAutomationInsert).not.toHaveBeenCalled();
expect(mockStore.bindRepositoryInserts).not.toHaveBeenCalled();
Expand All @@ -615,17 +634,18 @@ describe("automation route handlers", () => {
})
);

await expect(
callRoute("POST", "/automations", {
body: {
...validBody,
repositories: [
{ repoOwner: "acme", repoName: "first" },
{ repoOwner: "acme", repoName: "second" },
],
},
})
).rejects.toMatchObject({ message: "failed first" });
const res = await callRoute("POST", "/automations", {
body: {
...validBody,
repositories: [
{ repoOwner: "acme", repoName: "first" },
{ repoOwner: "acme", repoName: "second" },
],
},
});

expect(res.status).toBe(404);
await expect(res.json()).resolves.toEqual({ error: "failed first" });
expect(mockBatch).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -931,7 +951,7 @@ describe("automation route handlers", () => {
);
});

it("fails closed when a bot actor bypasses admission", async () => {
it("refuses a bot actor at admission before any identity is resolved", async () => {
mockStore.getById.mockResolvedValue(sampleRow);

const res = await callRoute("POST", "/automations", {
Expand All @@ -944,8 +964,11 @@ describe("automation route handlers", () => {
principal: SLACK_BOT_PRINCIPAL,
});

expect(res.status).toBe(500);
await expect(res.json()).resolves.toEqual({ error: "Failed to resolve session identity" });
expect(res.status).toBe(403);
await expect(res.json()).resolves.toEqual({
error: "Forbidden",
code: "service_capability_required",
});
expect(mockUserStore.resolveOrCreateUser).not.toHaveBeenCalled();
expect(mockStore.bindAutomationInsert).not.toHaveBeenCalled();
});
Expand Down
Loading
Loading