Skip to content

fix(codex-runtime): preserve full-access permissions across re-migration (#27616) - #27764

Open
zccyman wants to merge 1 commit into
NousResearch:mainfrom
atyou2happy:fix/codex-runtime-preserve-permissions-27616
Open

zccyman wants to merge 1 commit into
NousResearch:mainfrom
atyou2happy:fix/codex-runtime-preserve-permissions-27616

Conversation

@zccyman

@zccyman zccyman commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #27616 — codex-runtime re-migration silently downgrades user's full-access Codex permissions back to :workspace.

Root Cause

migrate_codex_runtime() strips the entire old managed block (including the user's default_permissions) via _strip_existing_managed_block(), then rebuilds with the hardcoded default :workspace profile. Users who set :danger-no-sandbox / danger-full-access lose their setting on every migration pass.

Fix

  1. _extract_existing_permissions() — new function that reads top-level permission keys from existing config before stripping
  2. Preservation logic — before building the new managed block, check for full-access values and preserve them
  3. MigrationReport.preserved_permissions — new field surfaces preservation in summary output

Testing

75/75 tests pass, including 5 new unit tests + 3 new integration tests for permission preservation behavior.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 18, 2026
@BoardJames-Bot

Copy link
Copy Markdown

CI triage: the only current blocker is the test job, but it was cancelled rather than failed. The job log stops right after dependency installation (no pytest output / no failing test), while the sibling e2e job and all other checks passed.

I tried to rerun the failed/cancelled workflow job, but this bot does not have the required repository admin permission:

run 26014402595 cannot be rerun; Must have admin rights to Repository.

This looks like a transient/cancelled CI run, not a branch-local regression from the available logs. Maintainer action: rerun the Tests workflow for this SHA.

@zccyman
zccyman force-pushed the fix/codex-runtime-preserve-permissions-27616 branch from 74dd2df to d733c01 Compare May 18, 2026 06:14
…ion (NousResearch#27616)

When migrate_codex_runtime() re-runs, it strips the old managed block
(including the user's default_permissions) and rebuilds with the
hardcoded default :workspace profile.  Users who manually set
:danger-no-sandbox / danger-full-access lose their setting on every
migration pass — a footgun in gateway/iMessage contexts where there's
no interactive approval UI.

Changes:
- New _extract_existing_permissions() reads top-level permission keys
  from the existing config before the managed block is stripped
- migrate_codex_runtime() now checks for full-access values and
  preserves them instead of overwriting with :workspace
- MigrationReport.preserved_permissions field surfaces the preservation
  in summary output
- wrote_permissions_default tracking moved after the preservation check
  so it reflects the actual (possibly preserved) value

Fixes NousResearch#27616
@zccyman
zccyman force-pushed the fix/codex-runtime-preserve-permissions-27616 branch from d733c01 to 7e874af Compare May 21, 2026 07:37
@teknium1

teknium1 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for taking on #27616 — I verified the bug still exists on current main and this PR is aimed at the right code path.

Problems

  • The PR can duplicate a user-repaired top-level default_permissions. On the PR head, preservation is detected from existing text in hermes_cli/codex_runtime_plugin_migration.py:748-759, then the managed block re-emits default_permissions via :767-769. Because _strip_existing_managed_block() preserves root keys outside the managed block and _insert_managed_block_at_top_level() appends after a root-key prefix at :333-335, a manually repaired top-level key can remain and be emitted again.
  • _extract_existing_permissions() collects sandbox_mode and approval_policy at hermes_cli/codex_runtime_plugin_migration.py:456-483, but migrate() only uses existing_perms.get("default_permissions") at :756-759, and render_codex_toml_section() still only writes default_permissions at :276-288. If preserving those keys is part of the intended fix for codex-runtime migration should preserve full-access Codex permissions #27616, they are not actually preserved when they live inside the managed block.

Suggested changes

  • Add de-duplication for preserved top-level permission keys before inserting the regenerated managed block, or avoid re-emitting a key that remains in the user-owned root prefix.
  • Add a regression test for a manually repaired top-level full-access config (sandbox_mode, approval_policy, and default_permissions) that verifies the migrated TOML parses and has no duplicate root keys.

This is an automated hermes-sweeper review.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing the re-migration downgrade; the current-main path still replaces the managed permission profile, so the premise is valid.

Problems

  • PR head hermes_cli/codex_runtime_plugin_migration.py:748-770 reads a user-owned root default_permissions, then emits it in the new managed block. _strip_existing_managed_block() preserves content outside the markers, and _insert_managed_block_at_top_level() preserves root-key prefixes (hermes_cli/codex_runtime_plugin_migration.py:307-335 on current main). A manually repaired root key therefore survives and is duplicated, yielding invalid TOML.
  • _extract_existing_permissions() collects sandbox_mode and approval_policy (:456-483), but migrate() only consumes default_permissions (:756-759); the renderer only writes that one profile.
  • danger-full-access is accepted as a default profile at :751-755, although current main documents :danger-no-sandbox as the applicable built-in profile (hermes_cli/codex_runtime_plugin_migration.py:629-635).

Suggested changes

  • Establish one owner for root permission keys and test migrated TOML with tomllib for duplicate-free manually repaired configs.
  • Preserve all intended permission keys, or narrow extraction to the one supported key.
  • Validate default-profile values separately from sandbox_mode values.

This is an automated hermes-sweeper review.

dp = existing_perms.get("default_permissions", "")
if dp in _full_access_values:
default_permission_profile = dp
report.preserved_permissions = existing_perms

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This leaves any manually repaired root permission lines intact, but line 769 regenerates default_permissions inside the managed block. Please remove or avoid re-emitting user-owned root keys so migration cannot produce duplicate TOML keys.

# Preserve an explicit full-access or no-sandbox setting.
_full_access_values = {
":danger-no-sandbox",
"danger-full-access",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

danger-full-access is the reported sandbox_mode value, while current migration documentation identifies :danger-no-sandbox as the built-in default profile. Passing this through the renderer creates :danger-full-access; validate profile values separately.

dp = existing_perms.get("default_permissions", "")
if dp in _full_access_values:
default_permission_profile = dp
report.preserved_permissions = existing_perms

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The extracted sandbox_mode and approval_policy are only recorded in the report here; neither is supplied to the renderer. Either preserve those keys through migration or limit the extractor and report to default_permissions.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants