refactor: consolidate extractAgentResponse into shared#423
Conversation
Move the duplicated extractAgentResponse module and its helpers (summarizeToolCall, getArtifactLabel, toEventArtifactInfo, etc.) from slack-bot and linear-bot into the shared package. The shared version accepts an ExtractorDeps interface (fetcher, internalSecret, log) instead of a package-specific Env, so both consumers now delegate to the shared implementation via a thin adapter that maps their Env bindings.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAgent response extraction was moved into a new shared module. Bot-specific extractor files were replaced with thin wrappers that delegate pagination, token aggregation, tool-call summarization, artifact fetching, and logging to the shared Changes
Sequence Diagram(s)mermaid Bot->>Shared: extractAgentResponse(sessionId, messageId, traceId) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 moves the duplicated event/artifact extraction logic into @open-inspect/shared and leaves thin Slack/Linear adapters behind. I reviewed 4 changed files (+352/-421) and the refactor looks behavior-preserving overall, with the dependency injection boundary keeping the shared code appropriately decoupled from Worker-specific Env types.
Critical Issues
- None.
Suggestions
- [Testing]
packages/shared/src/completion/extractor.ts- Consider adding shared-package unit tests for the extracted helpers andextractAgentResponseitself. The existing Slack tests still cover core behavior, but now that this logic lives in@open-inspect/shared, package-local tests would better protect future refactors without relying on consumer test suites.
Nitpicks
- None.
Positive Feedback
- The
ExtractorDepsinterface is a clean abstraction that removes directEnvcoupling without over-engineering the API. - Preserving the Linear-specific formatter in
linear-botkeeps the shared module focused on extraction only. - Re-exporting Slack compatibility helpers minimizes downstream churn while still consolidating the implementation.
Questions
- None.
Verdict
Approve: Ready to merge, no blocking issues.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![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
🧹 Nitpick comments (1)
packages/shared/src/completion/extractor.ts (1)
42-45:ExtractorDeps.fetcherinterface overclaims support for generic HTTP clients.The interface documents
fetcheras accepting "any HTTP-capable client", but both event and artifact fetches use hard-codedhttps://internalURLs. This hostname works only with Cloudflare Workers service bindings; a generic fetch-compatible client would fail attempting to resolve "internal" as a real hostname. Either update the interface documentation to clarify it requires service bindings, or parameterize the base URL to support both service bindings and generic fetch clients.Appears at lines 94–95 and 189–190.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/completion/extractor.ts` around lines 42 - 45, The ExtractorDeps.fetcher doc overclaims generic HTTP-client support while code uses hard-coded "https://internal" host (used in event/artifact fetches), so either clarify that ControlPlaneFetcher must be a service-binding capable of resolving "internal" or make the base host configurable; update the ExtractorDeps interface by adding a baseUrl (or controlPlaneBase) string field and update all uses in the artifact/event fetching logic (references: ExtractorDeps.fetcher, ControlPlaneFetcher and the event/artifact fetch call sites) to build requests from that base instead of the hard-coded "https://internal", or alternatively change the JSDoc on ExtractorDeps.fetcher to explicitly state it requires a service binding that can resolve "internal".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/completion/extractor.ts`:
- Around line 181-203: fetchSessionArtifacts is returning all session artifacts
(from GET /sessions/:sessionId/artifacts) and can attach artifacts from prior
messages to the wrong update; modify the flow to include message scoping: either
update the server endpoint to accept a messageId query param and return only
artifacts for that message, or extend the ArtifactResponse/ArtifactInfo to
include messageId and add client-side filtering inside fetchSessionArtifacts
(use the messageId from base or an added parameter) so only artifacts matching
the current messageId are returned; locate fetchSessionArtifacts and the
ListArtifactsResponse/ArtifactResponse types to implement the chosen change and
update the call sites that build the request headers/base accordingly.
---
Nitpick comments:
In `@packages/shared/src/completion/extractor.ts`:
- Around line 42-45: The ExtractorDeps.fetcher doc overclaims generic
HTTP-client support while code uses hard-coded "https://internal" host (used in
event/artifact fetches), so either clarify that ControlPlaneFetcher must be a
service-binding capable of resolving "internal" or make the base host
configurable; update the ExtractorDeps interface by adding a baseUrl (or
controlPlaneBase) string field and update all uses in the artifact/event
fetching logic (references: ExtractorDeps.fetcher, ControlPlaneFetcher and the
event/artifact fetch call sites) to build requests from that base instead of the
hard-coded "https://internal", or alternatively change the JSDoc on
ExtractorDeps.fetcher to explicitly state it requires a service binding that can
resolve "internal".
🪄 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: c2ae6f62-713a-42b7-ab70-b08ad5a85406
📒 Files selected for processing (4)
packages/linear-bot/src/completion/extractor.tspackages/shared/src/completion/extractor.tspackages/shared/src/index.tspackages/slack-bot/src/completion/extractor.ts
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
Addressing the review feedback: JSDoc overclaims (nitpick): Fair point. Tightened the JSDoc to clarify Shared-package unit tests (suggestion from open-inspect bot): Good suggestion. The existing slack-bot extractor tests still exercise the shared logic end-to-end. Adding shared-package-local unit tests is worth doing as a follow-up but out of scope for this refactor PR. |
Take thin wrapper extractors from main (PR #423), update shared extractor to use buildInternalAuthHeaders instead of inline generateInternalToken.
1 participant
Summary
extractAgentResponsemodule and all its helpers (summarizeToolCall,getArtifactLabel,getArtifactLabelFromArtifact,toEventArtifactInfo,toArtifactType,SUMMARY_TOOL_NAMES) fromslack-botandlinear-botinto@open-inspect/sharedatpackages/shared/src/completion/extractor.tsExtractorDepsinterface (fetcher,internalSecret,log) instead of a package-specificEnvtype, decoupling the logic from Cloudflare Worker bindingsEnvbindings intoExtractorDepsand delegate to the shared implementationformatAgentResponseremains in the linear-bot packageTest plan
npm run build -w @open-inspect/sharedpassesnpm run typecheckpasses for all 7 packages (shared, control-plane, web, slack-bot, linear-bot, github-bot)npm test -w @open-inspect/slack-bot— 30/30 tests pass (including extractor tests)npm test -w @open-inspect/linear-bot— 90/90 tests passnpm test -w @open-inspect/shared— 59/59 tests passnpm run lintpassesSummary by CodeRabbit