Skip to content

Commit 3718b7d

Browse files
authored
Strengthen source-control provider foundation (#72)
* feat: add single-provider scm guardrails * refactor: apply scm provider review feedback
1 parent 975e333 commit 3718b7d

13 files changed

Lines changed: 278 additions & 35 deletions

File tree

CONTRIBUTING.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,13 @@ Use clear, descriptive commit messages:
6767
4. Update documentation if needed
6868
5. Provide a clear description of your changes
6969

70+
### Source Control Provider Contributions
71+
72+
For SCM/provider changes, follow:
73+
74+
- `docs/adr/0001-single-provider-scm-boundaries.md`
75+
- `docs/provider-contribution-checklist.md`
76+
7077
## Reporting Issues
7178

7279
When reporting issues, please include:
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# ADR 0001: Single-Provider SCM Deployment and Boundary Rules
2+
3+
## Status
4+
5+
Accepted
6+
7+
## Context
8+
9+
Open-Inspect currently runs with GitHub as the only production SCM integration, while external
10+
contributors have requested Bitbucket support. The codebase already has a `SourceControlProvider`
11+
abstraction, but GitHub-specific details can still leak into non-provider layers if not guarded.
12+
13+
The team decision is to keep deployments single-provider. We need a safe foundation that preserves
14+
existing GitHub behavior and prevents unsafe coupling during future provider contributions.
15+
16+
## Decision
17+
18+
1. **Single provider per deployment**
19+
- Deployment config (`SCM_PROVIDER`) selects the provider.
20+
- No per-session provider state is persisted.
21+
22+
2. **Fail fast for unimplemented providers**
23+
- If `SCM_PROVIDER` resolves to a provider without implementation (currently `bitbucket`),
24+
control-plane returns explicit `501 Not Implemented` responses for non-public routes.
25+
26+
3. **Provider/auth boundary rules**
27+
- Provider-specific PR URL and push-transport construction must live in provider implementations.
28+
- Direct GitHub API base URL usage is limited to approved auth/provider modules.
29+
30+
4. **Guardrails enforced by code review + focused tests**
31+
- Provider boundary expectations are documented and validated through provider/factory tests.
32+
33+
## Consequences
34+
35+
### Positive
36+
37+
- Minimizes migration risk by avoiding schema/API expansion.
38+
- Keeps GitHub paths stable and auditable.
39+
- Gives contributors a clear and safe insertion point for future providers.
40+
41+
### Negative
42+
43+
- Multi-provider-per-deployment use cases are intentionally unsupported.
44+
- A future shift to multi-provider would require a new ADR and migration plan.
45+
46+
## Follow-Up Rules for Provider Contributions
47+
48+
- Add new provider logic under `packages/control-plane/src/source-control/providers`.
49+
- Register provider in factory and env resolver.
50+
- Do not add provider-specific URL/token logic to router/session/slack layers.
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# Provider Contribution Checklist
2+
3+
Use this checklist before opening a pull request for a new source-control provider.
4+
5+
## Architecture Checklist
6+
7+
- [ ] Provider implementation lives under `packages/control-plane/src/source-control/providers`.
8+
- [ ] Provider factory (`createSourceControlProvider`) has an explicit case for the new provider.
9+
- [ ] Deployment resolver (`SCM_PROVIDER`) recognizes the new provider name.
10+
- [ ] No provider-specific URL/token construction was added to router/session/slack layers.
11+
12+
## Auth and API Checklist
13+
14+
- [ ] User-authenticated repository lookup and PR creation are implemented via
15+
`SourceControlProvider`.
16+
- [ ] Push auth/token generation is implemented via provider auth path (not session/router
17+
hardcoding).
18+
- [ ] Manual PR fallback URL is built via provider method (`buildManualPullRequestUrl`).
19+
- [ ] Push transport spec is built via provider method (`buildGitPushSpec`).
20+
21+
## Tests Checklist
22+
23+
- [ ] Provider factory tests cover selection and unsupported behavior.
24+
- [ ] Provider implementation tests cover:
25+
- [ ] manual PR URL building
26+
- [ ] push spec building
27+
- [ ] basic API mapping behavior
28+
- [ ] Existing create-PR branch consistency tests still pass.
29+
- [ ] Slack manual-PR button tests still pass.
30+
- [ ] No provider-specific URL/token logic is introduced outside provider/auth modules.
31+
32+
## Documentation Checklist
33+
34+
- [ ] Control-plane README documents any new provider-related env vars or constraints.
35+
- [ ] ADR updated or added when architecture assumptions change.

packages/control-plane/README.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,12 @@ All secrets are configured via Terraform. Required secrets include:
220220
- `GITHUB_APP_INSTALLATION_ID` - Single installation for all users
221221
- `REPO_SECRETS_ENCRYPTION_KEY` - AES-GCM key for encrypting repo secrets in D1
222222

223+
Optional variables:
224+
225+
- `SCM_PROVIDER` - Source control provider for this deployment (`github` or `bitbucket`, default:
226+
`github`). Current implementation supports `github` only; `bitbucket` returns explicit
227+
`501 Not Implemented` responses until implemented.
228+
223229
See
224230
[terraform/environments/production/terraform.tfvars.example](../../terraform/environments/production/terraform.tfvars.example)
225231
for the complete list.

packages/control-plane/src/router.ts

Lines changed: 106 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ import {
1010
getInstallationRepository,
1111
listInstallationRepositories,
1212
} from "./auth/github-app";
13+
import {
14+
resolveScmProviderFromEnv,
15+
SourceControlProviderError,
16+
type SourceControlProviderName,
17+
} from "./source-control";
1318
import { RepoSecretsStore, RepoSecretsValidationError } from "./db/repo-secrets";
1419
import { SessionIndexStore } from "./db/session-index";
1520

@@ -89,6 +94,18 @@ function error(message: string, status = 400): Response {
8994
return json({ error: message }, status);
9095
}
9196

97+
function withCorsAndTraceHeaders(response: Response, ctx: RequestContext): Response {
98+
const headers = new Headers(response.headers);
99+
headers.set("Access-Control-Allow-Origin", "*");
100+
headers.set("x-request-id", ctx.request_id);
101+
headers.set("x-trace-id", ctx.trace_id);
102+
return new Response(response.body, {
103+
status: response.status,
104+
statusText: response.statusText,
105+
headers,
106+
});
107+
}
108+
92109
/**
93110
* Get Durable Object stub for a session.
94111
* Returns the stub or null if session ID is missing.
@@ -115,6 +132,46 @@ const SANDBOX_AUTH_ROUTES: RegExp[] = [
115132
/^\/sessions\/[^/]+\/pr$/, // PR creation from sandbox
116133
];
117134

135+
type CachedScmProvider =
136+
| {
137+
envValue: string | undefined;
138+
provider: SourceControlProviderName;
139+
error?: never;
140+
}
141+
| {
142+
envValue: string | undefined;
143+
provider?: never;
144+
error: SourceControlProviderError;
145+
};
146+
147+
let cachedScmProvider: CachedScmProvider | null = null;
148+
149+
function resolveDeploymentScmProvider(env: Env): SourceControlProviderName {
150+
const envValue = env.SCM_PROVIDER;
151+
if (!cachedScmProvider || cachedScmProvider.envValue !== envValue) {
152+
try {
153+
cachedScmProvider = {
154+
envValue,
155+
provider: resolveScmProviderFromEnv(envValue),
156+
};
157+
} catch (errorValue) {
158+
cachedScmProvider = {
159+
envValue,
160+
error:
161+
errorValue instanceof SourceControlProviderError
162+
? errorValue
163+
: new SourceControlProviderError("Invalid SCM provider configuration", "permanent"),
164+
};
165+
}
166+
}
167+
168+
if (cachedScmProvider.error) {
169+
throw cachedScmProvider.error;
170+
}
171+
172+
return cachedScmProvider.provider;
173+
}
174+
118175
/**
119176
* Check if a path matches any public route pattern.
120177
*/
@@ -129,6 +186,47 @@ function isSandboxAuthRoute(path: string): boolean {
129186
return SANDBOX_AUTH_ROUTES.some((pattern) => pattern.test(path));
130187
}
131188

189+
function enforceImplementedScmProvider(
190+
path: string,
191+
env: Env,
192+
ctx: RequestContext
193+
): Response | null {
194+
try {
195+
const provider = resolveDeploymentScmProvider(env);
196+
if (provider !== "github" && !isPublicRoute(path)) {
197+
logger.warn("SCM provider not implemented", {
198+
event: "scm.provider_not_implemented",
199+
scm_provider: provider,
200+
http_path: path,
201+
request_id: ctx.request_id,
202+
trace_id: ctx.trace_id,
203+
});
204+
const response = error(
205+
`SCM provider '${provider}' is not implemented in this deployment.`,
206+
501
207+
);
208+
return withCorsAndTraceHeaders(response, ctx);
209+
}
210+
211+
return null;
212+
} catch (errorValue) {
213+
const errorMessage =
214+
errorValue instanceof SourceControlProviderError
215+
? errorValue.message
216+
: "Invalid SCM provider configuration";
217+
218+
logger.error("Invalid SCM provider configuration", {
219+
event: "scm.provider_invalid",
220+
error: errorValue instanceof Error ? errorValue : String(errorValue),
221+
request_id: ctx.request_id,
222+
trace_id: ctx.trace_id,
223+
});
224+
225+
const response = error(errorMessage, 500);
226+
return withCorsAndTraceHeaders(response, ctx);
227+
}
228+
}
229+
132230
/**
133231
* Validate sandbox authentication by checking with the Durable Object.
134232
* The DO stores the expected sandbox auth token.
@@ -406,32 +504,21 @@ export async function handleRequest(
406504
// Sandbox auth passed, continue to route handler
407505
} else {
408506
// Both HMAC and sandbox auth failed
409-
const corsHeaders = new Headers(sandboxAuthError.headers);
410-
corsHeaders.set("Access-Control-Allow-Origin", "*");
411-
corsHeaders.set("x-request-id", ctx.request_id);
412-
corsHeaders.set("x-trace-id", ctx.trace_id);
413-
return new Response(sandboxAuthError.body, {
414-
status: sandboxAuthError.status,
415-
statusText: sandboxAuthError.statusText,
416-
headers: corsHeaders,
417-
});
507+
return withCorsAndTraceHeaders(sandboxAuthError, ctx);
418508
}
419509
}
420510
} else {
421511
// Not a sandbox auth route, return HMAC auth error
422-
const corsHeaders = new Headers(hmacAuthError.headers);
423-
corsHeaders.set("Access-Control-Allow-Origin", "*");
424-
corsHeaders.set("x-request-id", ctx.request_id);
425-
corsHeaders.set("x-trace-id", ctx.trace_id);
426-
return new Response(hmacAuthError.body, {
427-
status: hmacAuthError.status,
428-
statusText: hmacAuthError.statusText,
429-
headers: corsHeaders,
430-
});
512+
return withCorsAndTraceHeaders(hmacAuthError, ctx);
431513
}
432514
}
433515
}
434516

517+
const providerCheck = enforceImplementedScmProvider(path, env, ctx);
518+
if (providerCheck) {
519+
return providerCheck;
520+
}
521+
435522
// Find matching route
436523
for (const route of routes) {
437524
if (route.method !== method) continue;
@@ -460,12 +547,6 @@ export async function handleRequest(
460547
return error("Internal server error", 500);
461548
}
462549

463-
// Create new response with CORS + correlation headers
464-
const corsHeaders = new Headers(response.headers);
465-
corsHeaders.set("Access-Control-Allow-Origin", "*");
466-
corsHeaders.set("x-request-id", ctx.request_id);
467-
corsHeaders.set("x-trace-id", ctx.trace_id);
468-
469550
const durationMs = Date.now() - startTime;
470551
logger.info("http.request", {
471552
event: "http.request",
@@ -479,11 +560,7 @@ export async function handleRequest(
479560
...ctx.metrics.summarize(),
480561
});
481562

482-
return new Response(response.body, {
483-
status: response.status,
484-
statusText: response.statusText,
485-
headers: corsHeaders,
486-
});
563+
return withCorsAndTraceHeaders(response, ctx);
487564
}
488565
}
489566

packages/control-plane/src/session/durable-object.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
} from "../sandbox/lifecycle/manager";
2828
import {
2929
createSourceControlProvider as createSourceControlProviderImpl,
30+
resolveScmProviderFromEnv,
3031
SourceControlProviderError,
3132
type SourceControlProvider,
3233
type SourceControlAuthContext,
@@ -188,9 +189,10 @@ export class SessionDO extends DurableObject<Env> {
188189
*/
189190
private createSourceControlProvider(): SourceControlProvider {
190191
const appConfig = getGitHubAppConfig(this.env);
192+
const provider = resolveScmProviderFromEnv(this.env.SCM_PROVIDER);
191193

192194
return createSourceControlProviderImpl({
193-
provider: "github",
195+
provider,
194196
github: {
195197
appConfig: appConfig ?? undefined,
196198
},
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { describe, expect, it } from "vitest";
2+
import { SourceControlProviderError } from "./errors";
3+
import { DEFAULT_SCM_PROVIDER, resolveScmProviderFromEnv } from "./config";
4+
5+
describe("resolveScmProviderFromEnv", () => {
6+
it("defaults to github when SCM_PROVIDER is unset", () => {
7+
expect(resolveScmProviderFromEnv(undefined)).toBe(DEFAULT_SCM_PROVIDER);
8+
});
9+
10+
it("normalizes case and whitespace", () => {
11+
expect(resolveScmProviderFromEnv(" GITHUB ")).toBe("github");
12+
expect(resolveScmProviderFromEnv(" bitbucket ")).toBe("bitbucket");
13+
});
14+
15+
it("throws for unknown provider values", () => {
16+
expect(() => resolveScmProviderFromEnv("gitlab")).toThrow(SourceControlProviderError);
17+
expect(() => resolveScmProviderFromEnv("gitlab")).toThrow(
18+
"Invalid SCM_PROVIDER value 'gitlab'"
19+
);
20+
});
21+
});
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { SourceControlProviderError } from "./errors";
2+
import type { SourceControlProviderName } from "./types";
3+
4+
export const DEFAULT_SCM_PROVIDER: SourceControlProviderName = "github";
5+
6+
export function resolveScmProviderFromEnv(value: string | undefined): SourceControlProviderName {
7+
const normalized = value?.trim().toLowerCase() ?? "";
8+
if (!normalized) {
9+
return DEFAULT_SCM_PROVIDER;
10+
}
11+
12+
if (normalized === "github" || normalized === "bitbucket") {
13+
return normalized;
14+
}
15+
16+
throw new SourceControlProviderError(
17+
`Invalid SCM_PROVIDER value '${normalized}'. Supported values: github, bitbucket.`,
18+
"permanent"
19+
);
20+
}

packages/control-plane/src/source-control/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export type {
2323
// Errors
2424
export type { SourceControlErrorType } from "./errors";
2525
export { SourceControlProviderError } from "./errors";
26+
export { DEFAULT_SCM_PROVIDER, resolveScmProviderFromEnv } from "./config";
2627

2728
// Providers
2829
export {

0 commit comments

Comments
 (0)