feat: Github Setting for 'draft' mode#762
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional ChangesSCM Draft Mode Feature
Sequence DiagramsequenceDiagram
participant SandboxTool as inspect-plugin<br/>(create-pull-request)
participant Proxy as session-runtime-proxy<br/>(handleCreatePR)
participant Handler as pull-request.handler
participant Service as SessionPullRequestService
participant DOResolver as SessionDO<br/>.resolveAlwaysDraftDefault
participant ScmStore as ScmSettingsStore<br/>(D1)
participant GitHub as GitHub Provider
SandboxTool->>Proxy: POST /sessions/:id/pr<br/>{ title, body, draft? }
Proxy->>Proxy: validate draft is boolean if present
Proxy->>Handler: POST SessionInternalPaths.createPr<br/>{ ..., draft }
Handler->>Service: createPullRequest({ ..., draft })
Service->>DOResolver: resolveAlwaysDraftDefault()
DOResolver->>ScmStore: getResolvedSettings(owner/repo)
ScmStore-->>DOResolver: ScmSettings<br/>{ alwaysUseDraftMode }
DOResolver-->>Service: alwaysDraft boolean
Service->>GitHub: createPullRequest<br/>({ draft: alwaysDraft OR input.draft })
GitHub-->>Service: PR response
Service-->>Handler: created PR
Handler-->>Proxy: 200 response
Proxy-->>SandboxTool: PR result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/control-plane/src/routes/session-runtime-proxy.ts`:
- Around line 97-99: The validation check for the draft field in the if
statement starting at line 97 currently allows null values to pass through
because the condition uses != null, which evaluates to false when draft is null
(since null equals null). To reject null and only allow the field to be either
undefined or a boolean, change the validation condition to explicitly check that
draft is either undefined or a boolean type, treating null as invalid input.
This will ensure that line 109 never forwards a null value downstream for the
draft field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b44d3f64-8c9a-42de-a3f8-05f3d3e141f0
📒 Files selected for processing (13)
packages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/routes/integration-settings.tspackages/control-plane/src/routes/session-runtime-proxy.test.tspackages/control-plane/src/routes/session-runtime-proxy.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/http/handlers/pull-request.handler.tspackages/control-plane/src/session/pull-request-service.test.tspackages/control-plane/src/session/pull-request-service.tspackages/sandbox-runtime/src/sandbox_runtime/plugins/inspect-plugin.jspackages/shared/src/types/integrations.tspackages/web/src/app/(app)/settings/integrations/[id]/page.tsxpackages/web/src/components/settings/integrations/github-pr-integration-settings.tsx
ColeMurray
left a comment
ColeMurray
left a comment
There was a problem hiding this comment.
[Automated Review] Summary
Draft-mode threading is correct end-to-end (sandbox tool → proxy → handler → service → provider) and well covered by tests at the store/proxy/service layers. The provider boundary already honored draft (GitHub requestBody.draft, GitLab Draft: title prefix), so this is low-risk plumbing plus a settings surface. No blocking bugs. Verdict: Comment — the main thing worth resolving before merge is the "GitHub" / "GitHub Bot" naming collision (see inline).
Two findings touch unchanged files, so they can't be left inline:
- [Reliability]
source-control/providers/github-provider.ts(if (config.draft) requestBody.draft = true): GitHub rejects draft PRs on private repos under free plans (HTTP 422), with no fallback here. Enabling the global default could break PR creation for a subset of repos. Worth a note in the setting's help text and/or a graceful retry-without-draft on 422. - [Testing]
github-provider.test.tshas zero tests forcreatePullRequest, so the GitHub draft path this feature now depends on is unverified at the provider layer (service tests mock the provider). GitLab has three draft tests; GitHub should have at least one.
Inline comments below are prefixed [Automated Review].
| ); | ||
| } | ||
|
|
||
| function Section({ |
There was a problem hiding this comment.
[Automated Review] [Maintainability] This component is a near-verbatim copy of code-server-integration-settings.tsx — this Section helper is now duplicated across three integration components, along with the whole save/reset/dirty/toast loop. Not introduced solely by this PR, but each new integration deepens the duplication. A shared useIntegrationSettings hook + shared Section/RepoOverrideRow would shrink this file substantially and keep the integrations consistent.
| const [showResetDialog, setShowResetDialog] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (settings !== undefined && !initialized) { |
There was a problem hiding this comment.
[Automated Review] [Nit] The parent gates rendering on globalLoading/repoSettingsLoading and only mounts this section once data is resolved, so the useState initializer already captures the right value — this initialized/useEffect re-sync is redundant (and won't reflect later SWR revalidations). Harmless, and it mirrors the existing pattern, so fine to leave.
|
Some use-case questions here: In the original implementation, this still allows for the agent to override the draft PR setting via an input (this may be a bug). Is the intent of this change to have this ALWAYS be draft mode when selected, or if not explicitly provided. Related, I'm onboard with adding the optional parameter to the tool as this make sense. Would the intent of this functionality be better served by custom instructions to the agent, as it seems like it may create confusion to the agent, where the setting is hard-coded upstream to "always draft" while the agent itself called it with Related, I don't think putting this under Github PR makes sense, as the draft PR is an source control provider level concept, rather than specific to github. This is evident by the generic SCM provider types being updated. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Yes - this is a bug. My intention was a "force" option (but if you have a strong preference for a "default to draft mode" I could go that direction).
Yes my desire is to make the draft functionality generally exposed so tooling is flexible enough to enable guide particular workflows one direction or the other they could (i.e. open draft mode if tests have not been run, etc) - assuming not everyone will want to force it like I do. I think that has value but I've found limited success and at Bitly we prefer to always start as a draft. You can always flip to non-draft but it's not reversible - you can't undo automated review requests that are sent when you don't start in draft mode. There is a little bit of conflict in exposing a draft option in the tool if it's forced for a particular repo/setup - I think that's ok and won't cause problems - but happy to iterate if it does. I'll update the tool settings to imply that the draft option may be overridden by a remote policy.
I'm totally open on where to set this. I was thinking before i started it would be Github specific - but GitLab supports draft mode too so as you say a top level "SCM settings" or "[Github / GitLab] SCM" might be appropriate. I've been noodling on how to support repositories that use a fork workflow and that setting structure would be a good home for that too. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/control-plane/src/session/pull-request-service.test.ts`:
- Around line 291-298: The test assertion at line 298 expects draft to be true
when alwaysDraftDefault is enabled, but this contradicts the documented
draft-resolution contract where explicit draft parameter should take precedence.
In the test for "forces draft when always-draft is enabled, even if the request
sets draft=false", change the assertion in the expect.objectContaining call to
expect draft: false instead of draft: true, since the explicit draft: false
passed in createInput should override the alwaysDraftDefault setting according
to the documented behavior. Additionally, update the test name and any related
test logic to reflect that alwaysDraftDefault should only apply when draft is
omitted, not when it is explicitly set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a73d546d-c641-4157-8251-2121367679f7
📒 Files selected for processing (7)
packages/control-plane/src/routes/integration-settings.tspackages/control-plane/src/routes/session-runtime-proxy.tspackages/control-plane/src/session/pull-request-service.test.tspackages/control-plane/src/session/pull-request-service.tspackages/control-plane/src/source-control/providers/github-provider.test.tspackages/sandbox-runtime/src/sandbox_runtime/plugins/inspect-plugin.jspackages/web/src/components/settings/integrations/github-pr-integration-settings.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/control-plane/src/routes/session-runtime-proxy.ts
- packages/sandbox-runtime/src/sandbox_runtime/plugins/inspect-plugin.js
- packages/control-plane/src/session/pull-request-service.ts
- packages/web/src/components/settings/integrations/github-pr-integration-settings.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/shared/src/types/integrations.ts (1)
49-51: 🏗️ Heavy liftRename
alwaysUseDraftModeto match its default-only behavior.The name reads like an enforced policy, but runtime behavior treats it as a default that explicit tool input can override. Renaming now (for example,
defaultToDraftMode) would prevent semantics drift across API/UI/DB contracts.As per coding guidelines, "When threading existing fields through new code paths in TypeScript, evaluate whether the existing design (naming, types, units) is correct and fix bad names or units in the same change rather than propagating problems."
🤖 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 `@packages/shared/src/types/integrations.ts` around lines 49 - 51, The property `alwaysUseDraftMode` in the ScmSettings interface should be renamed to better reflect its actual behavior as a default setting rather than an enforced policy. Rename `alwaysUseDraftMode` to `defaultToDraftMode` (or similar alternative that clearly indicates it is a default rather than an always-enforced behavior) in the ScmSettings interface and update all references to this property throughout the codebase to use the new name consistently.Source: Coding guidelines
🤖 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 `@packages/control-plane/src/db/integration-settings.ts`:
- Around line 222-224: The validateScmSettings method currently validates
alwaysUseDraftMode but accepts enabledRepos even though it is ignored by
downstream SCM resolution, creating a misleading API contract. Modify the
validateScmSettings method to explicitly reject or throw an error when
enabledRepos is provided in the ScmSettings object, ensuring that only supported
configuration fields are accepted and preventing silent failures where accepted
config has no effect.
In `@packages/web/src/components/settings/scm-settings.tsx`:
- Line 238: The `split("/")` approach at lines 235, 304, and 330 in
scm-settings.tsx incorrectly assumes a single slash separator in the `fullName`,
causing it to fail for nested GitLab namespaces like `group/subgroup/repo`.
Change the parsing logic to extract the repository name from the rightmost
segment using `pop()` and construct the owner from all remaining segments by
joining them back together with "/". This approach will correctly handle
arbitrarily nested namespace structures and ensure the URL path matches the
expected API route pattern. Apply this same fix pattern at lines 308 and 333
where the same parsing logic appears.
---
Nitpick comments:
In `@packages/shared/src/types/integrations.ts`:
- Around line 49-51: The property `alwaysUseDraftMode` in the ScmSettings
interface should be renamed to better reflect its actual behavior as a default
setting rather than an enforced policy. Rename `alwaysUseDraftMode` to
`defaultToDraftMode` (or similar alternative that clearly indicates it is a
default rather than an always-enforced behavior) in the ScmSettings interface
and update all references to this property throughout the codebase to use the
new name consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab7d42b8-71bc-4acb-bec4-342bf95e503e
📒 Files selected for processing (9)
packages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/routes/integration-settings.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/pull-request-service.tspackages/shared/src/types/integrations.tspackages/web/src/app/(app)/settings/page.tsxpackages/web/src/components/settings/scm-settings.tsxpackages/web/src/components/settings/settings-nav.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/control-plane/src/session/pull-request-service.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/control-plane/src/db/scm-settings.ts`:
- Around line 22-29: The validateScmSettings function only validates the type of
the alwaysUseDraftMode field but does not enforce a strict object shape,
allowing extra keys and non-plain objects to be accepted from API payloads.
Enhance the validateScmSettings function to verify that the input is a plain
object, that only expected keys (like alwaysUseDraftMode) are present, and
reject any unexpected properties. Apply the same strict shape validation in the
setGlobal method referenced at lines 48-53 to ensure that global config keys are
restricted to the supported contract and prevent non-plain objects from being
persisted.
In `@packages/control-plane/src/routes/scm-settings.ts`:
- Around line 25-37: The handleGetGlobal function does not catch potential
database errors from store.getGlobal(), causing them to propagate as generic 500
errors. Wrap the store.getGlobal() call in a try-catch block and return a 503
status code response when a database error is caught. Apply the same error
handling pattern to handleListRepoSettings function which has the same
vulnerability with store.listRepoSettings().
- Around line 52-54: The validation check for body.settings only uses typeof to
verify it is an object, but this allows arrays to pass since typeof returns
"object" for arrays. Strengthen the validation condition in the if statement
(that currently checks typeof body.settings !== "object") by adding an
additional check to exclude arrays. Use Array.isArray() to ensure that if
body.settings is an array, the validation fails and returns the error response,
preventing invalid array payloads from being persisted.
In `@packages/web/src/app/api/scm-settings/repos/`[owner]/[name]/route.ts:
- Around line 18-32: The current error handling in the try-catch block returns a
500 status code for all errors, including malformed JSON bodies which should
return a 400 status code. Separate the JSON parsing from other operations by
wrapping the await request.json() call in its own try-catch block, so that JSON
parsing errors can be caught and handled separately to return a 400 status code
with an appropriate error message for bad requests, while keeping the 500 status
for other unexpected errors from the controlPlaneFetch call.
In `@packages/web/src/app/api/scm-settings/route.ts`:
- Around line 29-40: The catch block currently treats all errors the same and
returns a 500 status code, but JSON parsing errors from request.json() should
return a 400 status since they represent client input errors rather than server
failures. Modify the catch block to check if the error is a JSON parsing error
(typically a SyntaxError) and return a 400 status with an appropriate error
message for those cases, while keeping the 500 status for other unexpected
server errors. You can distinguish JSON parsing errors by checking the error
type or message when request.json() fails to parse the body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a83751ae-3672-48f5-a050-8e150665f5af
📒 Files selected for processing (11)
packages/control-plane/src/db/integration-settings.tspackages/control-plane/src/db/scm-settings.test.tspackages/control-plane/src/db/scm-settings.tspackages/control-plane/src/router.tspackages/control-plane/src/routes/scm-settings.tspackages/control-plane/src/session/durable-object.tspackages/shared/src/types/integrations.tspackages/web/src/app/api/scm-settings/repos/[owner]/[name]/route.tspackages/web/src/app/api/scm-settings/repos/route.tspackages/web/src/app/api/scm-settings/route.tspackages/web/src/components/settings/scm-settings.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/components/settings/scm-settings.tsx
jehiah
force-pushed
the
github_draft_mode_762
branch
from
6f2946b to
4dfecfb
Compare
|
RFRR @ColeMurray 
|
2 participants
I want to add support for 'draft' mode PRs. This is important for me because it allows iterating on a PR before review is requested from required reviewers based on github
CODEOWNERSfiles.Summary
Lets coding sessions open pull requests in draft mode, controlled two ways:
draftparameter on the agent'screate-pull-requesttool.per-repo overrides. This is intentionally separate from the existing "GitHub Bot" settings
(PR reviews / comment actions); it lives in its own integration card.
Draft creation already existed at the source-control provider layer (
github-provider.tshonorsconfig.draft; the GitLab provider uses theDraft:title prefix). This change threads the flagthrough the tool call and adds the setting that supplies the default.
Resolution order: an explicit
draftvalue from the tool call always wins; when omitted, theresolved "always use draft mode" setting for the session's repo decides (global default merged with
per-repo override); when D1 is unavailable it falls back to
false.Screenshots
Changes
New
drafttool parameterpackages/sandbox-runtime/.../plugins/inspect-plugin.js—create-pull-requestaccepts anoptional
draftboolean and forwards it.packages/control-plane/src/routes/session-runtime-proxy.ts— validatesdraftis a boolean andforwards it to the session Durable Object.
packages/control-plane/src/session/http/handlers/pull-request.handler.ts—CreatePrRequestcarries
draftthrough to the service.packages/control-plane/src/session/pull-request-service.ts— computes the effective draft value(
input.draft ?? resolveAlwaysDraftDefault()) and passes it to the provider.New "GitHub" integration setting (separate from "GitHub Bot")
packages/shared/src/types/integrations.ts— newIntegrationId"github-pr",GitHubPrSettings { alwaysUseDraftMode? }, settings-map entry,GitHubPrGlobalConfig, and a newINTEGRATION_DEFINITIONScard named "GitHub". The existing "GitHub Bot" card is unchanged.packages/control-plane/src/db/integration-settings.ts— validatesalwaysUseDraftModeis aboolean. Global-default + per-repo-override merge is inherited from the existing framework.
packages/control-plane/src/routes/integration-settings.ts— resolved-config branch forgithub-pr.packages/control-plane/src/session/durable-object.ts—resolveAlwaysDraftDefault()reads theresolved
github-prconfig for the session's repo; returnsfalsewhen D1 is unavailable.packages/web/src/components/settings/integrations/github-pr-integration-settings.tsx(new) +packages/web/src/app/(app)/settings/integrations/[id]/page.tsx— settings card with a global"Always use draft mode" toggle and per-repo overrides, mirroring the code-server pattern.
Scope notes
bot's @mentions — this is the intended scope, and the setting remains fully decoupled from the
GitHub Bot configuration.
integration_settings/integration_repo_settingstables already store arbitrary JSON keyed by integration id.Testing
pull-request-service.test.ts): defaultdraft=false, explicit flag forwarded,setting fallback when unspecified, and explicit
draft=falseoverriding an enabled setting.integration-settings.test.ts): round-trip global/repo, non-booleanrejection, and repo override flipping the global default.
session-runtime-proxy.test.ts):draftforwarding and non-boolean rejection.typecheckandlintare clean onall changed files.
🤖 Generated with Claude Code
Summary by CodeRabbit