fix: load Codex MCP credentials from devcontainer env - #758
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a global suppress flag and startup timeouts/env-var wiring to MCP server entries in ChangesEnvironment and Configuration Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Claude finished @keito4's task in 2m 0s —— View job コードレビュー
総評: ✅ APPROVE変更全体は意図が明確で安全性も適切に考慮されています。指摘事項はすべて minor / recommendation レベルです。 セキュリティ観点
|
| 観点 | 結果 |
|---|---|
| Security | ✅ ホワイトリスト検証済み、秘密情報の直書きなし |
| Performance | ✅ 問題なし |
| Quality | |
| Accessibility | N/A |
| AI Residuals | ✅ なし |
verdict: APPROVE — minor/recommendation のみのため変更をブロックしません。
![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: 296319a9b0
ℹ️ 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".
| text = '' | ||
| # Expose selected shared local secrets to CLI tools such as Codex MCP servers. | ||
| if [[ -r "$HOME/.devcontainer.env" ]]; then | ||
| while IFS='=' read -r _codex_env_key _codex_env_value; do |
There was a problem hiding this comment.
Update the read loop so it still handles the last record when ~/.devcontainer.env does not end with \n. In the current form, read returns non-zero at EOF and the loop body is skipped for that final line, so whichever selected credential is last (for example DOPPLER_TOKEN) is silently not exported, leading to intermittent MCP auth failures depending on file formatting.
Useful? React with 👍 / 👎.
|
(aside) |
|
Claude finished @keito4's task in 10m 45s —— View job CI修正完了 ✅
調査結果GitHub Actions上の全ワークフロー（CI Fallback、Dependabot Auto-merge、Claude Code Review）は全て成功状態でした。CIのpathフィルタが 修正内容Codexレビューで指摘されたP2バグを修正しました： 問題: 修正: - while IFS='=' read -r _codex_env_key _codex_env_value; do
+ while IFS='=' read -r _codex_env_key _codex_env_value || [[ -n $_codex_env_key ]]; do修正ファイル:
コミット: |
`while IFS='=' read` が改行なしで終わるファイルの最終行をスキップする 問題を修正。`|| [[ -n $_codex_env_key ]]` を追加して EOF 後も 最終レコードを処理するようにした。 Co-authored-by: keito4 <keito4@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nix/home/zsh.nix (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove secret file outside
pre/to prevent sourcing in non-login shells.
initContentruns for all interactive shells and calls_load_settings, which sources every file under$HOME/.zsh/configs/pre/. This causesdevcontainer-env.zshto load and export API tokens in every shell, not just login shells.loginExtrathen sources the same file again for login shells. Move this script to a login-specific directory and update both paths to prevent unnecessary token exposure and avoid double-sourcing.Suggested fix
loginExtra = '' - if [[ -r "$HOME/.zsh/configs/pre/devcontainer-env.zsh" ]]; then - source "$HOME/.zsh/configs/pre/devcontainer-env.zsh" + if [[ -r "$HOME/.zsh/login/devcontainer-env.zsh" ]]; then + source "$HOME/.zsh/login/devcontainer-env.zsh" fi ''; @@ - ".zsh/configs/pre/devcontainer-env.zsh" = { + ".zsh/login/devcontainer-env.zsh" = { text = '' # Expose selected shared local secrets to CLI tools such as Codex MCP servers. if [[ -r "$HOME/.devcontainer.env" ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nix/home/zsh.nix` around lines 19 - 23, The devcontainer-env.zsh secret is placed under $HOME/.zsh/configs/pre/ and gets sourced by initContent/_load_settings for all interactive shells and again by loginExtra, causing double-sourcing and token exposure; move devcontainer-env.zsh out of the pre/ directory into a login-only directory (e.g., $HOME/.zsh/configs/login/) and update the loginExtra source path to point to the new location, and ensure any references in initContent/_load_settings do not include the new login-only directory so non-login shells won't source it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nix/home/zsh.nix`:
- Around line 19-23: The devcontainer-env.zsh secret is placed under
$HOME/.zsh/configs/pre/ and gets sourced by initContent/_load_settings for all
interactive shells and again by loginExtra, causing double-sourcing and token
exposure; move devcontainer-env.zsh out of the pre/ directory into a login-only
directory (e.g., $HOME/.zsh/configs/login/) and update the loginExtra source
path to point to the new location, and ensure any references in
initContent/_load_settings do not include the new login-only directory so
non-login shells won't source it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27ad45dd-63b6-46ca-a434-4247173da4fe
📒 Files selected for processing (2)
.zsh/configs/pre/devcontainer-env.zshnix/home/zsh.nix
|
(aside) |
|
Claude finished @keito4's task in 12m 2s —— View job CI修正完了 ✅
根本原因前回セッション（
修正内容コードの改善をプッシュして CI Fallback を再トリガーしました。 コミット 変更内容:
品質ゲート: lint ✅ / format ✅ / test 114件 ✅ すべて通過 新しいコミットのプッシュにより CI Fallback が再実行され、Quality Gate チェックが復元される予定です。 |
- Strip trailing CR from values to handle Windows-style CRLF .devcontainer.env - Add comment noting the allowlist is duplicated between the two files - Add inline comment listing allowed keys for readability Co-authored-by: keito4 <keito4@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.zsh/configs/pre/devcontainer-env.zsh (1)
3-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnset removed tokens before reloading this file.
If one of these vars is inherited from the parent shell and later removed from
~/.devcontainer.env, this loader leaves the stale credential in place. That defeats the "no cached secrets" goal and can keep MCP auth pointed at the wrong account.💡 Suggested fix
+unset SUPABASE_ACCESS_TOKEN VERCEL_TOKEN LINEAR_API_KEY DOPPLER_TOKEN if [[ -r "$HOME/.devcontainer.env" ]]; then while IFS='=' read -r _codex_env_key _codex_env_value || [[ -n $_codex_env_key ]]; do # Strip trailing CR so Windows-style CRLF files work correctly. _codex_env_value="${_codex_env_value%$'\r'}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.zsh/configs/pre/devcontainer-env.zsh around lines 3 - 15, The loader currently only exports keys found in ~/.devcontainer.env but doesn't remove previously exported allowed tokens if they are absent; before reading the file (or at start of the block that processes it) explicitly unset the allowed variables SUPABASE_ACCESS_TOKEN, VERCEL_TOKEN, LINEAR_API_KEY, and DOPPLER_TOKEN (e.g., via unset of those names) so stale credentials exported earlier are cleared; ensure this happens prior to the while-read loop that uses _codex_env_key and _codex_env_value so the subsequent case/export only sets values present in the file.
🧹 Nitpick comments (1)
nix/home/zsh.nix (1)
112-129: ⚡ Quick winUse the tracked script as the Home Manager source.
This block is a second copy of
.zsh/configs/pre/devcontainer-env.zsh, so future auth changes can drift between the checked-in script and the generated one. Pointhome.fileat the tracked file instead of duplicating the body.💡 Suggested refactor
".zsh/configs/pre/devcontainer-env.zsh" = { - text = '' - # Expose selected shared local secrets to CLI tools such as Codex MCP servers. - # NOTE: This list is duplicated in .zsh/configs/pre/devcontainer-env.zsh. Keep both in sync. - if [[ -r "$HOME/.devcontainer.env" ]]; then - while IFS='=' read -r _codex_env_key _codex_env_value || [[ -n $_codex_env_key ]]; do - # Strip trailing CR so Windows-style CRLF files work correctly. - _codex_env_value="''${_codex_env_value%$'\r'}" - case "$_codex_env_key" in - # Allowed keys: SUPABASE_ACCESS_TOKEN | VERCEL_TOKEN | LINEAR_API_KEY | DOPPLER_TOKEN - SUPABASE_ACCESS_TOKEN|VERCEL_TOKEN|LINEAR_API_KEY|DOPPLER_TOKEN) - export "$_codex_env_key=$_codex_env_value" - ;; - esac - done < "$HOME/.devcontainer.env" - unset _codex_env_key _codex_env_value - fi - ''; + source = ../../.zsh/configs/pre/devcontainer-env.zsh; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nix/home/zsh.nix` around lines 112 - 129, The duplicated inline file body for ".zsh/configs/pre/devcontainer-env.zsh" should be replaced by pointing Home Manager at the tracked script instead of embedding its text; remove the large text = ''...'' block and set the file entry to reference the tracked script (use the home.file / home.file.<name> mechanism to source the existing ".zsh/configs/pre/devcontainer-env.zsh" file), ensuring only a single canonical source of truth for the devcontainer-env script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.zsh/configs/pre/devcontainer-env.zsh:
- Around line 3-15: The loader currently only exports keys found in
~/.devcontainer.env but doesn't remove previously exported allowed tokens if
they are absent; before reading the file (or at start of the block that
processes it) explicitly unset the allowed variables SUPABASE_ACCESS_TOKEN,
VERCEL_TOKEN, LINEAR_API_KEY, and DOPPLER_TOKEN (e.g., via unset of those names)
so stale credentials exported earlier are cleared; ensure this happens prior to
the while-read loop that uses _codex_env_key and _codex_env_value so the
subsequent case/export only sets values present in the file.
---
Nitpick comments:
In `@nix/home/zsh.nix`:
- Around line 112-129: The duplicated inline file body for
".zsh/configs/pre/devcontainer-env.zsh" should be replaced by pointing Home
Manager at the tracked script instead of embedding its text; remove the large
text = ''...'' block and set the file entry to reference the tracked script (use
the home.file / home.file.<name> mechanism to source the existing
".zsh/configs/pre/devcontainer-env.zsh" file), ensuring only a single canonical
source of truth for the devcontainer-env script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce30f7d-ddf4-477a-855e-87cf55948a19
📒 Files selected for processing (2)
.zsh/configs/pre/devcontainer-env.zshnix/home/zsh.nix
|
(aside) |
|
Claude finished @keito4's task in 11m 39s —— View job CI修正完了 ✅
根本原因PR がブロックされている理由は コードの問題ではなく構造的問題 です。
修正内容（コミット
|
Previously, if a key was removed from ~/.devcontainer.env the old value remained exported in the shell (inherited from parent). Adding an explicit `unset` before the read loop ensures removed tokens don't linger. Both the DevContainer script and the nix home-manager managed copy are updated in sync. Co-authored-by: keito4 <keito4@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
(aside) |
|
Claude finished @keito4's task in 6m 29s —— View job CI修正完了 ✅
実施内容根本原因: 対策: PR を close → reopen することで 現在のステータスチェック
|
|
Closing to trigger pull_request:reopened event so CI Fallback runs on current HEAD (0d3abc4). Reopening immediately. |
|
🎉 This PR is included in version 1.112.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
~/.devcontainer.envfor login shell startup.LINEAR_API_KEYbearer token env var.npx-based MCP servers and passDOPPLER_TOKENto Doppler MCP.Verification
git grep --cached -n -E 'sbp_|vcp_|lin_api_|dp\.pt\.|gho_|SUPABASE_ACCESS_TOKEN=|VERCEL_TOKEN=|LINEAR_API_KEY=|DOPPLER_TOKEN=|Bearer [A-Za-z0-9_-]+' -- .codex/config.toml nix/home/zsh.nix .zsh/configs/pre/devcontainer-env.zshreturned no matches.zsh -n .zsh/configs/pre/devcontainer-env.zshnix-instantiate --parse nix/home/zsh.nix >/dev/nullgit diff --cached --checkzsh -lc 'codex exec --ephemeral --sandbox read-only "Respond exactly: CLI_OK"'Summary by CodeRabbit