fix: allow bot completion event reads - #1684
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe session events and artifacts routes now permit actorless Slack and Linear bot grants. Router policy and service authorization integration tests verify the new access. ChangesActorless completion-read access
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to This PR lets Slack and Linear services read session events and artifacts without a user actor. Because access is scoped to the service and endpoint rather than the originating session, a valid service credential could read another session's data; merge should wait for resource scoping or explicit security-owner acceptance. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The title clearly identifies the main change: allowing bot services to read completion events. It is concise and related to the actorless completion-read authorization changes, although it does not mention artifact reads or the specific services. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. ✨ 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 |
There was a problem hiding this comment.
Deep maintainability review found no structural regressions or actionable code-quality blockers. The change stays within the existing declarative actorless-grant policy rather than adding handler-level special cases, keeps both completion reads under the canonical sessions.read service ceiling, and adds exact allowlist coverage at both the route-policy and integration boundaries. No changed file crosses or approaches the 1k-line decomposition threshold.
Validated locally with the focused router policy tests, service-auth integration tests, and control-plane typecheck.
There was a problem hiding this comment.
Summary
PR #1684, fix: allow bot completion event reads, by @ColeMurray updates 3 files (+22/-3) to permit actorless Slack and Linear service principals to read session events and artifacts for completion extraction. The change is narrowly scoped, remains constrained by each service permission ceiling, and has appropriate route-policy and integration coverage.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The actorless grants are limited to the two required read endpoints and the two intended services.
- The existing
sessions.readservice ceiling remains enforced after actorless grant matching. - Tests verify the exact policy allowlist and exercise both services against both completion-read routes.
Questions
None.
Verdict
Approve
Validation: all 100 focused route-policy tests and all 32 focused service-auth integration tests pass; control-plane typechecking also passes.
Summary
sessions.readpermission ceilingRoot cause
The RBAC HTTP-boundary change made
GET /sessions/:id/eventsandGET /sessions/:id/artifactsactor-required routes. Slack and Linear completion callbacks use the shared extractor after the original request has completed, and the queued callback does not carry an actor assertion. Production telemetry confirmed Slack authenticated successfully as a service principal but was rejected with HTTP 403 before any D1 query.The media endpoint already has the equivalent narrow Slack actorless grant; this change restores the prerequisite completion reads without weakening other session routes.
Validation
npm test -w @open-inspect/control-plane -- src/router.policy.test.tsnpm run test:integration -w @open-inspect/control-plane -- test/integration/service-auth.test.tsnpm run typecheck -w @open-inspect/control-planenpm run build -w @open-inspect/control-planeCreated with Open-Inspect
Summary by CodeRabbit
New Features
Bug Fixes