fix: apply editor settings to automations#642
Conversation
📝 WalkthroughWalkthroughSession initialization is refactored to be configurable with per-repository integration settings. A new shared resolver module extracts code-server and sandbox setting lookups from the router. Session initialization now tracks automation lineage via new ChangesSession Initialization and Integration Settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #642 fix: apply editor settings to automations by @ColeMurray updates automation-created sessions to use the shared initializeSession path and extracts editor/sandbox setting resolution into a reusable helper. Reviewed 6 changed files (+207/-109); the change looks correct and keeps the intended D1-before-DO initialization ordering.
Critical Issues
None found.
Suggestions
None blocking. Verification note: I attempted to run npm test -w @open-inspect/control-plane -- src/session/initialize.test.ts src/scheduler/durable-object.test.ts and npm run typecheck -w @open-inspect/control-plane, but this checkout does not have Node dev dependencies installed, so vitest and tsc were unavailable.
Nitpicks
None.
Positive Feedback
- Good reuse of
initializeSession, which removes a duplicated session creation path and preserves the D1-before-DO invariant for automations. - Automation lineage is now threaded into the shared D1 session index path and covered by focused tests.
- The new scheduler test covers resolved code-server and sandbox settings, including repo override merging.
Questions
None.
Verdict
Approve.
![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/session/integration-settings-resolution.ts`:
- Around line 17-24: Normalize the repo slug to a consistent case before any
store lookups and comparisons: compute a single normalized repo identifier
(e.g., use `${repoOwner}/${repoName}` and call .toLowerCase()) and pass that
normalized value into IntegrationSettingsStore.getResolvedConfig("code-server",
...) and into any allowlist checks (instead of the original mixed-case repo).
Apply the same change in the other similar block (the code around the second
getResolvedConfig usage referenced in the comment) so both the lookup and the
enabledRepos.includes check use the same normalized repo slug.
🪄 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: dda19adb-23f8-47a6-a072-c0b6a20faf13
📒 Files selected for processing (6)
packages/control-plane/src/router.tspackages/control-plane/src/scheduler/durable-object.test.tspackages/control-plane/src/scheduler/durable-object.tspackages/control-plane/src/session/initialize.test.tspackages/control-plane/src/session/initialize.tspackages/control-plane/src/session/integration-settings-resolution.ts
| const repo = `${repoOwner}/${repoName}`; | ||
| try { | ||
| const store = new IntegrationSettingsStore(db); | ||
| const { enabledRepos, settings } = await store.getResolvedConfig("code-server", repo); | ||
| const codeServerSettings = settings as CodeServerSettings; | ||
| if (codeServerSettings.enabled !== true) return false; | ||
| // enabledRepos: null -> all repos, [] -> none, [...] -> allowlist | ||
| if (enabledRepos !== null && !enabledRepos.includes(repo.toLowerCase())) return false; |
There was a problem hiding this comment.
Normalize repo slug before store lookups to avoid case-sensitive misses.
The allowlist logic uses lowercase comparison, but getResolvedConfig(...) currently receives potentially mixed-case repo. If callers pass non-lowercase values, repo overrides can be skipped while allowlist checks still run against lowercase, producing inconsistent resolution.
Suggested fix
- const repo = `${repoOwner}/${repoName}`;
+ const repo = `${repoOwner}/${repoName}`.toLowerCase();
try {
const store = new IntegrationSettingsStore(db);
const { enabledRepos, settings } = await store.getResolvedConfig("code-server", repo);
@@
- if (enabledRepos !== null && !enabledRepos.includes(repo.toLowerCase())) return false;
+ if (enabledRepos !== null && !enabledRepos.includes(repo)) return false;
@@
- const repo = `${repoOwner}/${repoName}`;
+ const repo = `${repoOwner}/${repoName}`.toLowerCase();
try {
const store = new IntegrationSettingsStore(db);
const { enabledRepos, settings } = await store.getResolvedConfig("sandbox", repo);
@@
- if (enabledRepos !== null && !enabledRepos.includes(repo.toLowerCase())) return {};
+ if (enabledRepos !== null && !enabledRepos.includes(repo)) return {};Also applies to: 43-49
🤖 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/control-plane/src/session/integration-settings-resolution.ts` around
lines 17 - 24, Normalize the repo slug to a consistent case before any store
lookups and comparisons: compute a single normalized repo identifier (e.g., use
`${repoOwner}/${repoName}` and call .toLowerCase()) and pass that normalized
value into IntegrationSettingsStore.getResolvedConfig("code-server", ...) and
into any allowlist checks (instead of the original mixed-case repo). Apply the
same change in the other similar block (the code around the second
getResolvedConfig usage referenced in the comment) so both the lookup and the
enabledRepos.includes check use the same normalized repo slug.
1 participant
Summary
initializeSessionso they preserve D1-before-DO init ordering, automation lineage, and resolved editor/sandbox settings.Verification
npm test -w @open-inspect/control-plane -- src/session/initialize.test.ts src/scheduler/durable-object.test.tsnpm run typecheck -w @open-inspect/control-planenpm run lint -w @open-inspect/control-planeCreated with Open-Inspect
Summary by CodeRabbit
New Features
Bug Fixes