Skip to content

fix(config): modernize Codex approval policy defaults#2

Merged
tianheil3 merged 4 commits into
mainfrom
codex/fix-github_token-and-approvalpolicy-issues
Apr 29, 2026
Merged

fix(config): modernize Codex approval policy defaults#2
tianheil3 merged 4 commits into
mainfrom
codex/fix-github_token-and-approvalpolicy-issues

Conversation

@tianheil3
Copy link
Copy Markdown
Owner

@tianheil3 tianheil3 commented Apr 29, 2026

Context

Newer Codex app-server versions reject the legacy reject approval policy object at startup, and some environments expose GitHub auth as GH_TOKEN instead of GITHUB_TOKEN.

TL;DR

Use "never" as the default Codex approval policy and accept GH_TOKEN as a GitHub token fallback.

Summary

  • Default codex.approval_policy now uses "never" instead of the legacy reject map.
  • Env-backed GITHUB_TOKEN resolution now falls back to GH_TOKEN when needed.
  • Tests and docs cover the new default, fallback precedence, and explicit map compatibility.

Alternatives

  • Keep the legacy reject map by default; rejected because current Codex app-server rejects it.
  • Require only GITHUB_TOKEN; rejected because gh-based environments commonly expose GH_TOKEN.

Test Plan

  • make -C elixir all
  • mix test test/symphony_elixir/core_test.exs:1589 test/symphony_elixir/workspace_and_config_test.exs:722 test/symphony_elixir/app_server_test.exs:265

Summary:
- Changed the default codex.approval_policy from legacy reject object
  form to the string value never.
- Added GITHUB_TOKEN resolution fallback to GH_TOKEN when WORKFLOW.md
  references $GITHUB_TOKEN and that variable is not set.
- Updated core test expectations and README docs to reflect the new
  default policy and GitHub token fallback behavior.

Rationale:
- Current Codex app-server versions reject the legacy reject approval
  policy object, which can break session startup.
- Some shells expose GitHub auth only via GH_TOKEN; allowing fallback
  avoids startup failures when WORKFLOW.md uses $GITHUB_TOKEN.

Tests:
- mix test test/symphony_elixir/core_test.exs (fails locally: Elixir
  1.18.3, project requires ~> 1.19)

Co-authored-by: Codex <codex@openai.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4c175f0-892a-4ee6-92b2-9dbae3d1f908

📥 Commits

Reviewing files that changed from the base of the PR and between 784edf4 and 7a2e4cf.

📒 Files selected for processing (3)
  • elixir/lib/symphony_elixir/config/schema.ex
  • elixir/test/symphony_elixir/core_test.exs
  • elixir/test/symphony_elixir/workspace_and_config_test.exs
🚧 Files skipped from review as they are similar to previous changes (2)
  • elixir/lib/symphony_elixir/config/schema.ex
  • elixir/test/symphony_elixir/core_test.exs

📝 Walkthrough

Walkthrough

Default Codex approval policy changed from a nested reject object to the string "never". Added granular to supported codex.approval_policy values. Environment variable resolution now falls back from GITHUB_TOKEN to GH_TOKEN for tracker API keys; docs, schema, and tests updated accordingly.

Changes

Cohort / File(s) Summary
Configuration Schema
elixir/lib/symphony_elixir/config/schema.ex
Default codex.approval_policy changed to the string "never". Introduced resolve_env_var/1 to centralize env lookup and added fallback so requests for GITHUB_TOKEN also check GH_TOKEN.
Tests
elixir/test/symphony_elixir/core_test.exs, elixir/test/symphony_elixir/workspace_and_config_test.exs
Updated expectations for default approval policy to be the string "never". Added tests verifying tracker API key resolution prefers GITHUB_TOKEN and falls back to GH_TOKEN. Adjusted workflow test setup accordingly.
Documentation
elixir/README.md
Updated YAML workflow defaults and accepted values: codex.approval_policy default is "never", added granular to supported values, and documented GH_TOKEN as an alternative to GITHUB_TOKEN for tracker.api_key.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through schema, docs, and test,
Swapped nested reject for "never" — neat and dressed.
Now tokens peek left, then right, for a GH key,
GITHUB first, GH_TOKEN next — that’s me!
A tiny tweak, and everything’s in jest.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: modernizing the Codex approval policy defaults from legacy reject map to 'never' string.
Description check ✅ Passed The description includes all required sections (Context, TL;DR, Summary, Alternatives, Test Plan) with substantive content addressing the PR objectives and changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-github_token-and-approvalpolicy-issues

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
elixir/test/symphony_elixir/core_test.exs (1)

1633-1633: Add regression coverage for GH_TOKEN fallback behavior

These approval-policy assertions are correct, but this PR also changes GitHub token resolution. Please add tests for:

  1. GH_TOKEN is used when GITHUB_TOKEN is unset, and
  2. GITHUB_TOKEN wins when both are present.

Also applies to: 1660-1660

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@elixir/test/symphony_elixir/core_test.exs` at line 1633, Add two regression
tests near the existing approval-policy assertions that verify token resolution:
one that unsets GITHUB_TOKEN, sets GH_TOKEN to a known value, and asserts the
token resolver (function get_github_token/0) returns the GH_TOKEN value; and a
second that sets both GITHUB_TOKEN and GH_TOKEN to different values and asserts
get_github_token/0 returns the GITHUB_TOKEN value. Ensure the tests manipulate
System.put_env/2 and System.delete_env/1 around the assertions and clean
up/reset env afterwards so other tests are unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@elixir/lib/symphony_elixir/config/schema.ex`:
- Around line 516-520: The tracker_env_value/2 path currently calls
System.get_env/1 directly which bypasses your GH_TOKEN fallback; change
tracker_env_value/2 to use the resolve_env_var/1 helper instead of
System.get_env/1 so GH_TOKEN is considered anywhere an env lookup for GitHub
token is needed (ensure any branches that handle unset or default token values
call resolve_env_var("GITHUB_TOKEN") or resolve_env_var(env_name) rather than
System.get_env(env_name)).

---

Nitpick comments:
In `@elixir/test/symphony_elixir/core_test.exs`:
- Line 1633: Add two regression tests near the existing approval-policy
assertions that verify token resolution: one that unsets GITHUB_TOKEN, sets
GH_TOKEN to a known value, and asserts the token resolver (function
get_github_token/0) returns the GH_TOKEN value; and a second that sets both
GITHUB_TOKEN and GH_TOKEN to different values and asserts get_github_token/0
returns the GITHUB_TOKEN value. Ensure the tests manipulate System.put_env/2 and
System.delete_env/1 around the assertions and clean up/reset env afterwards so
other tests are unaffected.
🪄 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: f3d9fa23-a222-49b8-9845-6515c7979759

📥 Commits

Reviewing files that changed from the base of the PR and between b1ec604 and a6711f6.

📒 Files selected for processing (3)
  • elixir/README.md
  • elixir/lib/symphony_elixir/config/schema.ex
  • elixir/test/symphony_elixir/core_test.exs

Comment thread elixir/lib/symphony_elixir/config/schema.ex
Summary:
- Updated tracker env lookup to share the same GITHUB_TOKEN/GH_TOKEN
  fallback path used by token and path resolution helpers.
- Added regression tests for GitHub token fallback behavior when
  GITHUB_TOKEN is missing and when both GITHUB_TOKEN and GH_TOKEN are set.

Rationale:
- This closes the consistency gap where tracker env fallback did not use
  the centralized resolver, which could bypass GH_TOKEN fallback.
- Added explicit tests to guard precedence and fallback behavior.

Tests:
- mix test test/symphony_elixir/core_test.exs (fails locally: Elixir
  1.18.3, project requires ~> 1.19)

Co-authored-by: Codex <codex@openai.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@elixir/lib/symphony_elixir/config/schema.ex`:
- Around line 163-165: The formatter failure is caused by the multi-line
declaration of the approval_policy Ecto schema field; collapse and reformat the
field declaration for the :approval_policy field to a single,
mix-format-compliant line so it reads as a single field call (i.e., update the
field(:approval_policy, StringOrMap, default: "never") expression) ensuring
commas and spacing follow Elixir formatter expectations.
🪄 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: 167ded73-f875-498f-8986-4bfd4e48e1a1

📥 Commits

Reviewing files that changed from the base of the PR and between a6711f6 and 784edf4.

📒 Files selected for processing (2)
  • elixir/lib/symphony_elixir/config/schema.ex
  • elixir/test/symphony_elixir/core_test.exs

Comment thread elixir/lib/symphony_elixir/config/schema.ex Outdated
tianheil3 and others added 2 commits April 29, 2026 22:54
Summary:
- Reformat the `approval_policy` field declaration to single-line form
  so mix format --check-formatted passes.

Rationale:
- CI failed at fmt-check due to this exact style mismatch in
  config/schema.ex.

Tests:
- mix format --check-formatted

Co-authored-by: Codex <codex@openai.com>
The branch changed the schema default to the app-server-compatible
`"never"` value, but two CI assertions still exercised either the old
fixture policy or the old default expectation. This narrows the startup
test to the schema default path and updates the config default assertion
without changing the shared safer-policy fixture used by other tests.

Constraint: Newer Codex app-server rejects the legacy reject object default
Rejected: Change the global test fixture default to `"never"` | it breaks tests that intentionally exercise safer explicit approval policies
Confidence: high
Scope-risk: narrow
Tested: make -C elixir all
Tested: mix test test/symphony_elixir/core_test.exs:1589 test/symphony_elixir/workspace_and_config_test.exs:722 test/symphony_elixir/app_server_test.exs:265
Co-authored-by: Codex <codex@openai.com>
@tianheil3
Copy link
Copy Markdown
Owner Author

[codex] Pushed 7a2e4cf to fix CI by aligning approval-policy default tests while preserving explicit reject-map coverage. Local verification: make -C elixir all, plus targeted approval-policy regression tests.

@tianheil3 tianheil3 merged commit 6ecf9a4 into main Apr 29, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant