Skip to content

[codex] improve Codex plan mode compatibility#538

Merged
tiann merged 3 commits into
tiann:mainfrom
dsus4wang:codex/improve-codex-plan-mode
May 15, 2026
Merged

[codex] improve Codex plan mode compatibility#538
tiann merged 3 commits into
tiann:mainfrom
dsus4wang:codex/improve-codex-plan-mode

Conversation

@dsus4wang
Copy link
Copy Markdown
Contributor

Summary

Improves Codex plan-mode compatibility when using the app-server runtime.

Changes

  • Probes collaborationMode/list after app-server initialization and keeps native plan-mode attempts enabled until the runtime rejects the field.
  • Retries plan turns once without collaborationMode when the runtime reports the field is unsupported or invalid, with a user-visible fallback message.
  • Adds generic item/tool/requestApproval handling so exit_plan_mode requests flow through Codex permissions.
  • Resets Codex collaboration mode to default after an approved exit_plan_mode / ExitPlanMode request and immediately emits updated keepalive state.

Validation

  • cd cli && PATH=$HOME/.bun/bin:$PATH bun run test codexRemoteLauncher.test.ts appServerConfig.test.ts appServerPermissionAdapter.test.ts permissionHandler.test.ts
  • PATH=$HOME/.bun/bin:$PATH bun typecheck

@dsus4wang dsus4wang marked this pull request as ready for review April 27, 2026 15:04
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Retry predicate catches every collaborationMode error — message.includes("mode") is always true after the guard accepts collaborationMode, so unrelated app-server failures that merely mention the field are retried without collaboration mode. That silently downgrades a plan turn into a normal turn instead of surfacing the real failure. Evidence cli/src/codex/codexRemoteLauncher.ts:52
    Suggested fix:
    return message.includes("experimentalapi")
        || message.includes("unsupported")
        || message.includes("unknown")
        || message.includes("unexpected")
        || message.includes("unrecognized")
        || message.includes("invalid field");

Summary

  • Review mode: initial
  • Found one issue in the fallback detection for Codex collaboration mode. Residual test gap: add a negative test where startTurn fails with a non-compatibility error mentioning collaborationMode, and assert HAPI does not retry as a normal turn.

Testing

  • Not run: bun not found on PATH in this runner.

HAPI Bot

Comment thread cli/src/codex/codexRemoteLauncher.ts Outdated
|| message.includes('unrecognized')
|| message.includes('invalid')
|| message.includes('field')
|| message.includes('mode');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Major] message.includes("mode") is always true once the guard has matched collaborationMode, so this retries any app-server error that mentions the field, including non-compatibility failures. That silently resends a plan turn as a normal turn instead of surfacing the actual error.

Suggested fix:

return message.includes("experimentalapi")
    || message.includes("unsupported")
    || message.includes("unknown")
    || message.includes("unexpected")
    || message.includes("unrecognized")
    || message.includes("invalid field");

tiann added 2 commits May 15, 2026 22:35
…lan-mode

# Conflicts:
#	cli/src/codex/appServerTypes.ts
#	cli/src/codex/codexAppServerClient.ts
#	cli/src/codex/codexRemoteLauncher.test.ts
#	cli/src/codex/codexRemoteLauncher.ts
#	cli/src/codex/session.ts
#	cli/src/codex/utils/appServerPermissionAdapter.test.ts
#	cli/src/codex/utils/appServerPermissionAdapter.ts
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • None.

Summary

  • Review mode: follow-up after new commits
  • No issues found in the latest diff. The prior retry-predicate finding was addressed, and the added tests cover the fixed negative path plus the new collaboration mode envelope/omitted-plan behavior. Residual risk: app-server protocol envelope variants are still inferred from observed shapes rather than a stable documented schema.

Testing

  • Not run (automation; PR code not executed during review)

HAPI Bot

@tiann tiann merged commit f1accab into tiann:main May 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants