refactor: consolidate duplicated auth header construction into shared utility#424
refactor: consolidate duplicated auth header construction into shared utility#424ColeMurray merged 2 commits intomainfrom
Conversation
…rnalAuthHeaders Add buildInternalAuthHeaders(secret, traceId) to @open-inspect/shared and replace 9 duplicated auth header construction sites across slack-bot, github-bot, linear-bot, and web packages.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR Title and number: refactor: consolidate duplicated auth header construction into shared utility (#424)
Author: @ColeMurray
Files changed: 16 files, +67 / -70
This refactor cleanly centralizes the repeated internal auth header construction without changing the observable behavior at the updated call sites. I reviewed the diff and did not find any correctness, security, performance, or maintainability regressions.
Critical Issues
- None.
Suggestions
- [Testing]
packages/shared/src/auth.ts:58- Consider adding a small shared-unit test forbuildInternalAuthHeaders()over time so the new helper's optional-secret and optional-trace-id behavior is covered at the abstraction boundary, rather than only through downstream mocks.
Nitpicks
- None.
Positive Feedback
- Consolidating the auth-header assembly into
@open-inspect/sharedremoves a lot of low-value duplication while keepingContent-Type/Acceptdecisions at the call site, which preserves flexibility. - The helper keeps the previous fail-open/fail-closed behavior intact across the different packages instead of forcing a one-size-fits-all policy.
- The updated package tests and repo typecheck/build all pass, which gives good confidence that the refactor is behavior-preserving.
Questions
- None.
Verdict
- Approve: Ready to merge, no blocking issues.
Take thin wrapper extractors from main (PR #423), update shared extractor to use buildInternalAuthHeaders instead of inline generateInternalToken.
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/github-bot/src/utils/integration-config.ts (1)
29-36: Consider adding an early guard for missing secret.Unlike
packages/linear-bot/src/utils/integration-config.ts(which has an early guard at line 25), this implementation relies on the helper returning an empty headers object whenINTERNAL_CALLBACK_SECRETis undefined. The request will proceed without anAuthorizationheader, and the control plane will reject it with a non-ok response, triggering fail-closed behavior.This works correctly but is less efficient—it makes a network request that will fail. If consistency with linear-bot is desired, consider adding an early guard:
export async function getGitHubConfig( env: Env, repo: string, log?: Logger ): Promise<ResolvedGitHubConfig> { + if (!env.INTERNAL_CALLBACK_SECRET) { + return { ...FAIL_CLOSED, model: env.DEFAULT_MODEL }; + } const [owner, name] = repo.split("/");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/github-bot/src/utils/integration-config.ts` around lines 29 - 36, Add an early guard that checks env.INTERNAL_CALLBACK_SECRET before calling buildInternalAuthHeaders to avoid making a needless network request when the secret is missing: if env.INTERNAL_CALLBACK_SECRET is falsy, return early (e.g., throw or return a suitable empty/unauthorized result) instead of calling env.CONTROL_PLANE.fetch; update the code path that currently calls buildInternalAuthHeaders(repo) and env.CONTROL_PLANE.fetch(`https://internal/integration-settings/github/resolved/${owner}/${name}`, { headers }) to perform the pre-check and short-circuit before building headers or fetching.
🤖 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/github-bot/test/handlers.test.ts`:
- Around line 20-22: The mock for buildInternalAuthHeaders should mirror the
real helper by returning both Authorization and the x-trace-id header; update
the vi.fn().mockResolvedValue to include an "x-trace-id" entry (or, if the real
helper accepts a trace-id/ctx param, make the mock inspect its arg and return
that value for "x-trace-id") so tests exercise trace propagation failures;
locate and update the mock declaration for buildInternalAuthHeaders to return {
Authorization: "Bearer test-internal-token", "x-trace-id":
"<same-or-propagated-trace-id>" } (or dynamically propagate the input trace id)
instead of only Authorization.
---
Nitpick comments:
In `@packages/github-bot/src/utils/integration-config.ts`:
- Around line 29-36: Add an early guard that checks env.INTERNAL_CALLBACK_SECRET
before calling buildInternalAuthHeaders to avoid making a needless network
request when the secret is missing: if env.INTERNAL_CALLBACK_SECRET is falsy,
return early (e.g., throw or return a suitable empty/unauthorized result)
instead of calling env.CONTROL_PLANE.fetch; update the code path that currently
calls buildInternalAuthHeaders(repo) and
env.CONTROL_PLANE.fetch(`https://internal/integration-settings/github/resolved/${owner}/${name}`,
{ headers }) to perform the pre-check and short-circuit before building headers
or fetching.
🪄 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: 011720f9-62d4-4bdd-a978-141595e72ebd
📒 Files selected for processing (16)
packages/github-bot/src/handlers.tspackages/github-bot/src/utils/integration-config.tspackages/github-bot/src/utils/internal.tspackages/github-bot/test/handlers.test.tspackages/github-bot/test/integration-config.test.tspackages/linear-bot/src/classifier/repos.tspackages/linear-bot/src/completion/extractor.tspackages/linear-bot/src/utils/integration-config.tspackages/linear-bot/src/utils/internal.tspackages/linear-bot/src/webhook-handler.tspackages/shared/src/auth.tspackages/slack-bot/src/classifier/repos.tspackages/slack-bot/src/completion/extractor.tspackages/slack-bot/src/index.tspackages/slack-bot/src/utils/internal.tspackages/web/src/lib/control-plane.ts
1 participant
Summary
buildInternalAuthHeaders(secret, traceId)to@open-inspect/shared/src/auth.tsthat encapsulates the repeated pattern of generating an HMAC token and buildingAuthorization+x-trace-idheadersslack-bot,github-bot,linear-bot,web) to use the shared helperContent-Type/Acceptsince those vary by call site -- callers spread the auth headers alongside their own content-type headerSites consolidated
src/index.tsgetAuthHeaderswrapper delegates to sharedsrc/classifier/repos.tssrc/completion/extractor.tssrc/handlers.tsgetAuthHeaderswrapper delegates to sharedsrc/utils/integration-config.tssrc/webhook-handler.tsgetAuthHeaderswrapper delegates to sharedsrc/utils/integration-config.tssrc/classifier/repos.tssrc/completion/extractor.tssrc/lib/control-plane.tsgetControlPlaneHeadersdelegates to sharedTest plan
npm run build -w @open-inspect/sharedpassesnpm run typecheckpasses for shared, slack-bot, github-bot, linear-bot, webnpm run lintpassesnpm test -w @open-inspect/slack-bot-- 30 tests passnpm test -w @open-inspect/github-bot-- 100 tests passnpm test -w @open-inspect/linear-bot-- 90 tests passnpm test -w @open-inspect/web-- 118 tests passSummary by CodeRabbit
Refactor
Tests