fix(acp): forward hosted credentials - #118
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe ACP hosted WebSocket resolver now uses ChangesACP hosted authentication tests
ACP test runner and CI execution
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ 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.
Pull request overview
This PR updates the Theorem ACP hosted WebSocket URL resolution so server-side environments can still authenticate when there is no incoming Authorization header by falling back to a configured Theorem credential, and adds a targeted transport contract test for that behavior.
Changes:
- Extend
appendToken()to fall back toprocess.env.THEOREM_API_KEYwhen no request-derived bearer token is available. - Add a focused test asserting
THEOREM_API_KEYis used to append?token=...when no request token is present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/theorem-acp/src/hosted-client.ts | Adds server-credential fallback for hosted ACP WS tokenization. |
| packages/theorem-acp/src/hosted-client.test.ts | Adds a contract test for server credential fallback behavior. |
Comments suppressed due to low confidence (2)
packages/theorem-acp/src/hosted-client.test.ts:37
- These tests can become environment-dependent:
resolveHostedAcpWsUrl()derives a token viaforwardAuthHeaders()from several env vars (THEOREM_API_TOKEN / THEOREM_AGENT_* / RUSTYRED_AGENT_BEARER). If any of those are set in the runner, the resolved URL will include?token=..., making this assertion flaky. Clear the full set of token env vars in this test setup in addition to THEOREM_API_KEY.
This issue also appears on line 44 of the same file.
packages/theorem-acp/src/hosted-client.test.ts:46
- This test assumes THEOREM_API_KEY is the only available credential. If any of THEOREM_API_TOKEN / THEOREM_AGENT_* / RUSTYRED_AGENT_BEARER are set in the environment,
forwardAuthHeaders()will supply an Authorization header andappendToken()will prefer that over THEOREM_API_KEY, causing a nondeterministic failure. Clear the other token env vars in this test before setting THEOREM_API_KEY.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5937562bf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| }); | ||
|
|
||
| it('uses the server Theorem credential when no request token is present', () => { |
There was a problem hiding this comment.
After moving this file out of apps/web/src, none of the configured test jobs execute it: the web and console Vitest configurations only include their respective src/** trees, while @commonplace/theorem-acp exposes only a TypeScript check script and CI invokes only that check. Consequently, this new credential contract test and the four existing routing tests moved with it can regress while every unit-test job remains green; add a package test configuration/script and CI invocation, or retain the tests in a consuming application's suite.
Useful? React with 👍 / 👎.
| delete process.env.THEOREM_AGENT_API_TOKEN; | ||
| delete process.env.THEOREM_AGENT_API_BEARER; | ||
| delete process.env.RUSTYRED_AGENT_BEARER; | ||
| delete process.env.THEOREM_API_KEY; |
There was a problem hiding this comment.
When the test process has any supported hosted credential such as THEOREM_API_TOKEN, THEOREM_AGENT_API_TOKEN, THEOREM_AGENT_API_BEARER, or RUSTYRED_AGENT_BEARER, forwardAuthHeaders still supplies it and resolveHostedAcpWsUrl() appends it as ?token=..., contradicting the bare-URL expectation below. The test previously deleted all four variables, but this replacement deletes only THEOREM_API_KEY, making the result depend on the runner's environment once the relocated suite is wired back into Vitest.
Useful? React with 👍 / 👎.
Clear all auth credential env vars so the transport contract stays deterministic, and wire theorem-acp vitest into console CI. Co-authored-by: Travis Gilbert <Travis-Gilbert@users.noreply.github.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Routes CommonPlace server-side hosted ACP connections through the configured Theorem credential when an incoming Authorization header is unavailable. Includes a focused transport contract test.
Summary by CodeRabbit
Bug Fixes
Tests