Repository navigation
Conversation
|
@codex review Merged main into this branch (cc84818). One conflict, in |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The hosts-file override pins both MCP replicas to one load balancer, preventing DNS failover and causing both to lose Fizzy connectivity if that balancer fails.
1 open issue
What changed in this PR
Enables Fizzy’s production OAuth server and hosted MCP delegation exchange while retaining the staff-only gate from #3246. The PR remains held pending deployment, HTTPS, and client-secret provisioning prerequisites.
Changes:
- Enables OAuth endpoints, allowlisted client metadata documents, and the hosted MCP resource.
- Configures MCP exchange credentials and pins outbound Fizzy traffic to an iad load balancer.
- Adds the exchange client secret to production deploy-secret fetching and extraction.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
saas/config/deploy.production.yml |
Enables OAuth and configures the MCP exchange, secret injection, and iad routing. |
saas/.kamal/secrets.production |
Fetches and extracts the MCP exchange client secret. |
Review effort: Balanced
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
State on cc84818: Codex code and security reviews found nothing. Copilot left one thread. The SaaS test jobs fail, and they failed the same way on eadbce0 before the merge. Both problems come from the
I haven't changed anything here. Ways to fix it: resolve at deploy time through something other than config-read ERB; have the test stub the lookup; or point the exchange and API at an iad-local name that covers both balancers, which would also answer Copilot. That last one is a networking change for Ops. The thread stays open until someone decides. |
The exchange and the API calls after it go to the app on the accessory's own host through Docker's host-gateway and that host's kamal-proxy, over the plain http the load balancer already uses to reach it. They stay in iad with no dependency on a single balancer, and reading the config no longer resolves a hostname, which CI's runners can't. The issuer's name maps to the host too, so an image without FIZZY_MCP_UPSTREAM fails its startup metadata check rather than reaching Fizzy outside iad.
|
@codex review d0df105 replaces the ERB This stays a held draft. The new prerequisite 4 in the description says why: the image pin has to move to a fizzy-mcp-server release that carries #19. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🔵 Needs a closer look
Production rollout requires operator confirmation of DNS/TLS, client and secret provisioning, and a pinned MCP release, while the unavailable upstream PR leaves routing and token exchange needing deployment validation.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
🔵 Needs a closer look
Production activation awaits operator confirmation of deployment, DNS/TLS, and client-secret provisioning, while the inaccessible MCP implementation leaves same-host routing and exchange requiring deployment validation.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 1ff574c443
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
State on 1ff574c, which merges main (#3240, and #3254's fizzy-mcp-server v0.2.1 pin) into this branch. The merge had no conflicts, and the diff against main is still exactly this PR's production env,
Still held as a draft. The remaining prerequisites are 1 to 3. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
* origin/main: Sweep dynamic registrations nobody authorized after a day, not thirty
There was a problem hiding this comment.
🔵 Needs a closer look
Production activation awaits operator confirmation of deployment, DNS/TLS, staff gating, and client-secret provisioning, while host-local MCP exchange and API calls require validation because the upstream implementation was unavailable for review.
No open issues 🎉
Review effort: Balanced
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Held: don't merge until the prerequisites below are done, on Jeremy's word.
Turns Fizzy's OAuth authorization server on in production for 37signals staff only, and lights the hosted MCP server's delegation exchange.
Change
OAUTH_ACCEPTANCE_ENABLED: discovery, consent and token endpoints answer.OAUTH_CIMD_ENABLED: Client ID Metadata Documents from the hosts already allowlisted indeploy.yml.FIZZY_OAUTH_MCP_RESOURCES: grants can be audienced tohttps://mcp.fizzy.do/mcp.fizzy_mcpaccessory:FIZZY_MCP_UPSTREAM=http://host.docker.internalwith--add-host host.docker.internal:host-gateway, which resolves the TODO that stood in its place. The exchange and the API calls after it go to the app on the accessory's own iad web host, through that host's kamal-proxy, over the plain http the balancers already use. They stay in iad without depending on any one balancer. The issuer, token endpoint and API base keep their https URLs; only the connection moves (fizzy-mcp-server#19).app.fizzy.do:host-gatewayas well, so an image withoutFIZZY_MCP_UPSTREAMfails its startup metadata check rather than reaching Fizzy outside iad.OAuth stays limited to staff by
OAUTH_STAFF_ONLY(#3246); this PR doesn't change that.Prerequisites, in order
mcp.fizzy.doresolves and serves HTTPS.fizzy-mcp-serverclient is provisioned withbin/rails oauth:provision_first_party_clients, run once withFIZZY_OAUTH_MCP_RESOURCES=https://mcp.fizzy.do/mcppassed explicitly, because production only gets that variable when this PR lands. Its secret is then minted withbin/rails "oauth:mint_client_secret[fizzy-mcp-server]"into the deploy secrets. Until that secret exists, any production deploy from this branch fails at secret fetch, which is why this PR is held.fizzy_mcp's image indeploy.ymlis pinned to that release's digest on main (Pin fizzy-mcp-server v0.2.1 #3254).cleanup_oauth_clientshourly.After merge
fizzy_mcpone host at a time (saas/bin/mcp-rolling-reboot).