fix: case-insensitive actor and label conditions#589
Conversation
The Durable Object already syncs status transitions to D1 via syncSessionIndexStatus in transitionSessionStatus. The router's synchronous D1 writes after calling the DO were redundant. - Remove D1 writes from handleArchiveSession, handleUnarchiveSession, handleUpdateSessionTitle, and handleCancelChild - Add syncSessionIndexTitle to the DO so title updates are synced to D1 from within the DO (matching the pattern used for status) - Wire the new sync through SessionLifecycleHandlerDeps
GitHub treats usernames and labels as case-insensitive, but our condition evaluators used exact string matching. This caused automations to silently skip when users entered e.g. "Colemurray" instead of "ColeMurray".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCase-insensitive matching is implemented for GitHub actor and label trigger conditions by lowercasing event data and configured values during comparison. Tests verify the new behavior across ChangesCase-Insensitive Actor and Label Matching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
This PR updates actor and label trigger conditions to compare case-insensitively, and it also moves session-title D1 index syncing into the session DO so title/status index updates live alongside the canonical lifecycle transitions.
- PR Title:
fix: case-insensitive actor and label conditions(#589) - Author:
@ColeMurray - Files changed: 6
- Additions/Deletions:
+79 / -38
Critical Issues
None.
Suggestions
- [Observability]
packages/control-plane/src/session/durable-object.ts:1474- The previous router path warned whenSessionIndexStore.updateTitle()returnedfalse. The new background sync logs thrown errors but no longer records the missing-row case. This looks non-blocking, but it may be worth preserving that warning if we still rely on it to detect D1/DO drift.
Nitpicks
None.
Positive Feedback
- The case-insensitive condition changes are minimal and preserve the existing operator semantics.
- The added tests cover both actor and label behavior across positive and negative cases.
- Centralizing index sync with the DO lifecycle flow reduces the chance of router/DO behavior diverging.
Questions
None.
Verdict
Approve: Ready to merge, no blocking issues.
Validation: npm test -w @open-inspect/shared passed (64 tests) and npm test -w @open-inspect/control-plane -- session-lifecycle.handler.test.ts passed (17 tests).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/triggers/conditions.test.ts (2)
62-74: ⚡ Quick winConsider adding the positive
none_ofpath.Similarly, the label tests cover
any_of→trueandnone_of→false. The passing case ofnone_of(label not present in event →true) is absent, so a regression makingnone_ofalways returnfalsewould slip through.✅ Suggested additional assertion
it("rejects labels with different casing (none_of)", () => { const event = buildMockEvent("github", { labels: ["Bug"] }); const conditions = [{ type: "label" as const, operator: "none_of" as const, value: ["BUG"] }]; expect(matchesConditions(conditions, event, conditionRegistry)).toBe(false); }); + + it("passes none_of when label is absent (case-insensitive)", () => { + const event = buildMockEvent("github", { labels: ["Enhancement"] }); + const conditions = [{ type: "label" as const, operator: "none_of" as const, value: ["bug"] }]; + expect(matchesConditions(conditions, event, conditionRegistry)).toBe(true); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/triggers/conditions.test.ts` around lines 62 - 74, Add a positive test for the label "none_of" path: create a mock GitHub event via buildMockEvent with labels that do NOT include the tested value (e.g., event labels ["Enhancement"]) then assert matchesConditions([{ type: "label", operator: "none_of", value: ["bug"] }], event, conditionRegistry) returns true; place this new it(...) alongside the existing label condition tests so the none_of behavior is validated in both false and true cases.
36-60: ⚡ Quick winConsider adding the positive
excludepath.The new actor tests cover
include→true,exclude→false, and exactinclude→true. There is no test asserting thatexcludereturnstruewhen the actor is not in the exclusion list — i.e., the passing side ofexclude. Without it, a regression that makes exclude always returnfalsewould not be caught by these new tests.✅ Suggested additional assertion
it("matches actor with exact casing", () => { const event = buildMockEvent("github", { actor: "octocat" }); const conditions = [ { type: "actor" as const, operator: "include" as const, value: ["octocat"] }, ]; expect(matchesConditions(conditions, event, conditionRegistry)).toBe(true); }); + + it("allows actor not in exclusion list (exclude)", () => { + const event = buildMockEvent("github", { actor: "octocat" }); + const conditions = [ + { type: "actor" as const, operator: "exclude" as const, value: ["COLEMURRAY"] }, + ]; + expect(matchesConditions(conditions, event, conditionRegistry)).toBe(true); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/triggers/conditions.test.ts` around lines 36 - 60, Add a positive test for the `exclude` path in the actor condition block: within the same describe("actor condition (case-insensitive)") add an `it` that uses `buildMockEvent("github", { actor: "ColeMurray" })`, a conditions array like `{ type: "actor", operator: "exclude", value: ["someone_else"] }`, and assert `matchesConditions(conditions, event, conditionRegistry)` is `true`; this ensures `matchesConditions` (and the actor condition logic) correctly returns true when the actor is not in the exclusion list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared/src/triggers/conditions.test.ts`:
- Around line 62-74: Add a positive test for the label "none_of" path: create a
mock GitHub event via buildMockEvent with labels that do NOT include the tested
value (e.g., event labels ["Enhancement"]) then assert matchesConditions([{
type: "label", operator: "none_of", value: ["bug"] }], event, conditionRegistry)
returns true; place this new it(...) alongside the existing label condition
tests so the none_of behavior is validated in both false and true cases.
- Around line 36-60: Add a positive test for the `exclude` path in the actor
condition block: within the same describe("actor condition (case-insensitive)")
add an `it` that uses `buildMockEvent("github", { actor: "ColeMurray" })`, a
conditions array like `{ type: "actor", operator: "exclude", value:
["someone_else"] }`, and assert `matchesConditions(conditions, event,
conditionRegistry)` is `true`; this ensures `matchesConditions` (and the actor
condition logic) correctly returns true when the actor is not in the exclusion
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ded8c145-b438-4249-996b-0f6d7d81e4a5
📒 Files selected for processing (6)
packages/control-plane/src/router.tspackages/control-plane/src/session/durable-object.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.test.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.tspackages/shared/src/triggers/conditions.test.tspackages/shared/src/triggers/registry.ts
💤 Files with no reviewable changes (1)
- packages/control-plane/src/router.ts
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
1 participant
Summary
"Colemurray"now matches"ColeMurray"— GitHub usernames are case-insensitive, so our condition matching should be too."bug"now matches a label named"Bug".Context
Discovered when configuring a
pull_request.openedautomation with an actor condition of"Colemurray"— it silently failed becauseArray.includes()is case-sensitive and GitHub returned"ColeMurray"insender.login.Test plan
npm test -w @open-inspect/shared— 64 tests passSummary by CodeRabbit
Bug Fixes
Tests