feat(codex): add opt-in native subagent defaults - #498
Conversation
📝 WalkthroughWalkthroughThe PR adds opt-in synchronization of selected subagent model and reasoning defaults into Codex configuration, with ownership-aware TOML editing, API and dashboard controls, localized documentation, and failure propagation through restore, sync, shutdown, and stop flows. ChangesNative subagent defaults
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant InjectionAPI
participant CodexSync
participant ConfigToml
Dashboard->>InjectionAPI: PUT model, effort, and syncCodexSubagentDefaults
InjectionAPI->>CodexSync: persist validated settings
CodexSync->>ConfigToml: apply marker-owned native defaults
ConfigToml-->>CodexSync: updated content or conflict warning
CodexSync-->>Dashboard: sync result and native-default warning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/index.ts (1)
213-243: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
restoreNativeCodex()before continuing cleanupsrc/cli/index.ts:489andsrc/cli/index.ts:705—restoreCodexCatalog()atsrc/codex/catalog/sync.ts:499-524can propagate file-system errors, so an exception here skips the laterrevertSystemEnv()/stripGrokConfig()steps. Wrap both calls in the same best-efforttry/catchused bysyncCleanup().🤖 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 `@src/cli/index.ts` around lines 213 - 243, Wrap both restoreCodexCatalog() calls in the CLI flows, including the call sites near the shutdown and stop handling paths, in best-effort try/catch blocks like syncCleanup(). Ensure any exception is handled without aborting subsequent revertSystemEnv() and stripGrokConfig() cleanup steps, while preserving the existing cleanup behavior.
🤖 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.
Inline comments:
In `@docs-site/src/content/docs/guides/web-dashboard.md`:
- Around line 62-66: Update the documentation around the Sub-agent delegation
picker and OpenCodex multi-agent guidance to limit the exact model and
reasoning-effort guarantee to the built-in guidance. State that custom
injectionPrompt content replaces the built-in guidance and must include the
{{model}} and/or {{effort}} placeholders when those overrides are required.
In `@gui/src/pages/use-dashboard-data.ts`:
- Around line 346-374: Update saveInjection to refresh the injection option
lists after a successful save: re-fetch /api/injection-model and apply its full
response, including injectionEfforts and injectionAvailable, rather than only
normalizing the PUT response. Preserve the existing committed UI state on
failure and always clear injectionSaving.
In `@gui/src/stop-proxy.ts`:
- Around line 12-16: Remove the hardcoded English fallback from failureMessage
and route the default stop-failure message through the existing i18n flow.
Update requestProxyStop or its App.tsx handleStop caller to supply a
t()-translated fallback, while preserving payload.message and payload.error
precedence and the existing HTTP status context.
- Around line 23-39: Update requestProxyStop to enforce a finite timeout around
the fetchFn call, using an AbortController or equivalent AbortSignal and
ensuring the timer is cleared after completion. Treat timeout-induced aborts
like other request errors by returning { accepted: true }, while preserving
existing response validation and cleanup behavior.
In `@gui/tests/app-stop.test.ts`:
- Around line 30-38: Guard both indexOf results in the test before slicing the
App.tsx source: assert that the "const handleStop" and "const brand" markers are
found and ordered correctly, then derive handler with slice. Keep the existing
handler-content assertions unchanged so the test fails loudly when either
boundary is missing or misplaced.
In `@src/cli/index.ts`:
- Around line 711-715: Update the restore/eject forward-switch handling around
stripGrokConfig so a failed result sets process.exitCode to a nonzero value
before logging, matching handleStop’s stopFailed behavior. Add a regression test
in the restore/eject CLI tests covering the !g.ok path and asserting the command
exits unsuccessfully.
In `@src/codex/subagent-defaults.ts`:
- Around line 461-474: Update the ownership check around TARGET_KEYS so
model/effort is evaluated as an atomic pair whenever defaults is non-null:
inspect both target definitions regardless of which keys are in desired, and add
conflicts when either existing key is user-owned. Preserve the existing no-op
return and conflict reporting so inject.ts warns whenever the pair cannot be
managed consistently.
In `@src/config.ts`:
- Around line 862-872: Update validFileConfigDiagnostics to avoid reassigning
its config parameter: compute the disabled reason from the original config
first, bind normalizeNativeSubagentSync(config, rawParsed) to a new local, and
use that local for subsequent configPlaceholderWarnings and related diagnostics.
Preserve the existing ordering so syncDisabledReason remains available for the
ignored-warning message.
In `@src/server/management/agent-settings-routes.ts`:
- Line 183: Extract the shared trim-aware predicate subagentDefaultSyncEffective
in src/config.ts using the syncCodexSubagentDefaults and injectionModel fields,
then replace the duplicated Boolean checks in this route’s GET/PUT response and
guard logic with it. Update configuredManagedSubagentDefaults in
src/codex/inject.ts to reuse the same predicate, preserving the intended
behavior that whitespace-only injectionModel values disable sync.
- Around line 247-257: Move the model-clearing reset block above the
supported-effort validation in the route handling update, so clearing nextModel
also clears nextEffort and nextSyncCodexSubagentDefaults before the guard
evaluates them. Preserve the existing validation for non-cleared models and add
a regression test covering PUT with model null, inherited
syncCodexSubagentDefaults true, and an unsupported injectionEffort.
In `@structure/05_gui-and-management-api.md`:
- Line 23: Update the Subagents row documenting PUT /api/injection-model to
state that contradictory cross-field combinations are rejected with HTTP 400:
enabling syncCodexSubagentDefaults requires an injection model and a supported
Codex reasoning injectionEffort. Also document that clearing the model
force-clears injectionEffort and syncCodexSubagentDefaults, matching the
enforcement in the agent-settings route.
In `@tests/codex-inject-integration.test.ts`:
- Around line 221-238: Extend the conflict scenarios in the tests around
“kept-user-base-url” and the external “model_provider” case to read back
config.toml after runInject. Assert that default_subagent_model and
default_subagent_reasoning_effort remain absent or unchanged when
nativeSubagentDefaultsWarning indicates routing ownership blocked injection,
while preserving the existing warning and success assertions. Confirm the inject
branch only writes these defaults when routing injection is allowed.
---
Outside diff comments:
In `@src/cli/index.ts`:
- Around line 213-243: Wrap both restoreCodexCatalog() calls in the CLI flows,
including the call sites near the shutdown and stop handling paths, in
best-effort try/catch blocks like syncCleanup(). Ensure any exception is handled
without aborting subsequent revertSystemEnv() and stripGrokConfig() cleanup
steps, while preserving the existing cleanup behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b2605ce-6a31-4382-ab9e-417e8da31f75
📒 Files selected for processing (55)
docs-site/src/content/docs/guides/codex-app-models.mddocs-site/src/content/docs/guides/sub-agent-surface.mddocs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/codex-app-models.mddocs-site/src/content/docs/ja/guides/sub-agent-surface.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/guides/codex-app-models.mddocs-site/src/content/docs/ko/guides/sub-agent-surface.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/guides/codex-app-models.mddocs-site/src/content/docs/ru/guides/sub-agent-surface.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/guides/codex-app-models.mddocs-site/src/content/docs/zh-cn/guides/sub-agent-surface.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdgui/src/App.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/dashboard-core-poll.tsgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/src/pages/use-dashboard-data.tsgui/src/stop-proxy.tsgui/tests/app-stop.test.tsgui/tests/dashboard-contracts.test.tsgui/tests/multi-agent-guidance.test.tsxsrc/cli/index.tssrc/codex/inject.tssrc/codex/subagent-defaults.tssrc/codex/sync.tssrc/config.tssrc/server/management/agent-settings-routes.tssrc/server/management/config-routes.tssrc/types.tsstructure/02_config-and-codex-home.mdstructure/03_catalog-and-subagents.mdstructure/05_gui-and-management-api.mdtests/cli-restore-back.test.tstests/codex-inject-integration.test.tstests/codex-inject.test.tstests/codex-journal.test.tstests/codex-sync-api.test.tstests/config.test.tstests/grok-lifecycle.test.tstests/injection-model-api.test.tstests/subagent-defaults.test.ts
| The Dashboard's **Sub-agent delegation** picker stores `injectionModel` and, optionally, | ||
| `injectionEffort`. On a v1 turn, opencodex injects guidance telling the parent agent which exact | ||
| model and reasoning effort to pass to `spawn_agent`. Choosing a model enables that guidance at any | ||
| parent reasoning effort; clearing the model also clears the stored effort. | ||
| `injectionEffort`. **OpenCodex multi-agent guidance** independently controls the delegation | ||
| instructions that use those values. On eligible v2 turns, that guidance tells the parent | ||
| agent which exact model and reasoning effort to pass to `spawn_agent`; clearing the model also clears | ||
| the stored effort. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the spawn-agent guarantee when custom prompts are used.
This wording implies every guidance variant passes the exact model and effort to spawn_agent, but injectionPrompt can replace the built-in guidance entirely. A custom prompt without {{model}} or {{effort}} will not necessarily provide those overrides. Clarify that this guarantee applies to built-in guidance, and document that custom prompts must include the placeholders when needed.
🤖 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 `@docs-site/src/content/docs/guides/web-dashboard.md` around lines 62 - 66,
Update the documentation around the Sub-agent delegation picker and OpenCodex
multi-agent guidance to limit the exact model and reasoning-effort guarantee to
the built-in guidance. State that custom injectionPrompt content replaces the
built-in guidance and must include the {{model}} and/or {{effort}} placeholders
when those overrides are required.
Source: Path instructions
| const saveInjection = async (patch: { | ||
| multiAgentGuidanceEnabled?: boolean; | ||
| syncCodexSubagentDefaults?: boolean; | ||
| model?: string | null; | ||
| effort?: string | null; | ||
| }) => { | ||
| if (injectionSaving) return; | ||
| setInjectionSaving(true); | ||
| try { | ||
| const res = await fetch(`${apiBase}/api/injection-model`, { | ||
| method: "PUT", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(patch), | ||
| }); | ||
| const data = await requireJson<{ | ||
| multiAgentGuidanceEnabled?: boolean; | ||
| syncCodexSubagentDefaults?: boolean; | ||
| model?: string | null; | ||
| effort?: string | null; | ||
| }>(res); | ||
| const normalized = normalizeInjectionSelection(data); | ||
| setMultiAgentGuidanceEnabled(normalized.multiAgentGuidanceEnabled); | ||
| setSyncCodexSubagentDefaults(normalized.syncCodexSubagentDefaults); | ||
| setInjectionModel(normalized.injectionModel); | ||
| setInjectionEffort(normalized.injectionEffort); | ||
| } catch { /* keep the last committed UI state */ } | ||
| finally { setInjectionSaving(false); } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether PUT /api/injection-model returns efforts/available in its response body.
fd -e ts agent-settings-routes.ts | xargs rg -n -A 30 "PUT|injection-model"Repository: lidge-jun/opencodex
Length of output: 17099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server PUT /api/injection-model =="
sed -n '191,290p' src/server/management/agent-settings-routes.ts | cat -n
echo
echo "== frontend saveInjection and nearby state =="
sed -n '330,390p' gui/src/pages/use-dashboard-data.ts | cat -n
echo
echo "== dashboard core poll / injection refresh =="
fd -e ts dashboard-core-poll.ts gui/src | while read -r f; do
echo "--- $f ---"
sed -n '200,260p' "$f" | cat -n
doneRepository: lidge-jun/opencodex
Length of output: 10771
Refresh the injection option lists after a save. saveInjection only applies multiAgentGuidanceEnabled/syncCodexSubagentDefaults/model/effort, but PUT /api/injection-model in src/server/management/agent-settings-routes.ts returns no efforts or available payload. That leaves injectionEfforts and injectionAvailable stale until the next 5s poll, so the effort dropdown can briefly show choices for the previous model after a model change. Re-fetch /api/injection-model after a successful save, or have the PUT handler return the full GET shape.
🤖 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 `@gui/src/pages/use-dashboard-data.ts` around lines 346 - 374, Update
saveInjection to refresh the injection option lists after a successful save:
re-fetch /api/injection-model and apply its full response, including
injectionEfforts and injectionAvailable, rather than only normalizing the PUT
response. Preserve the existing committed UI state on failure and always clear
injectionSaving.
| function failureMessage(payload: ProxyStopPayload | null, status: number): string { | ||
| if (typeof payload?.message === "string" && payload.message.trim()) return payload.message; | ||
| if (typeof payload?.error === "string" && payload.error.trim()) return payload.error; | ||
| return `Failed to stop proxy (HTTP ${status}).`; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded fallback string bypasses i18n.
Failed to stop proxy (HTTP ${status}). is a new user-visible string shown via alert() (see App.tsx handleStop), but it's a raw literal rather than routed through the app's t()/locale system used everywhere else in App.tsx (t("dash.stopConfirm"), t("dash.stop"), etc.).
♻️ Suggested direction
Pass a translated fallback into requestProxyStop/failureMessage (or have the caller substitute a t()-sourced default when outcome.message is absent) instead of hardcoding English text in this module.
As per path instructions, "Check that GUI state changes stay consistent with the management API responses and that user-visible strings go through the i18n locale files rather than hardcoded text."
🤖 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 `@gui/src/stop-proxy.ts` around lines 12 - 16, Remove the hardcoded English
fallback from failureMessage and route the default stop-failure message through
the existing i18n flow. Update requestProxyStop or its App.tsx handleStop caller
to supply a t()-translated fallback, while preserving payload.message and
payload.error precedence and the existing HTTP status context.
Source: Path instructions
| export async function requestProxyStop( | ||
| apiBase: string, | ||
| fetchFn: typeof fetch = fetch, | ||
| ): Promise<ProxyStopOutcome> { | ||
| let response: Response; | ||
| try { | ||
| response = await fetchFn(`${apiBase}/api/stop`, { method: "POST" }); | ||
| } catch { | ||
| return { accepted: true }; | ||
| } | ||
|
|
||
| const payload = await response.json().catch(() => null) as ProxyStopPayload | null; | ||
| if (!response.ok || payload?.success === false) { | ||
| return { accepted: false, message: failureMessage(payload, response.status) }; | ||
| } | ||
| return { accepted: true }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
No timeout guards the stop request.
fetchFn is awaited with no AbortSignal/timeout. The function only resolves {accepted:true} on a thrown error (e.g., connection reset) or {accepted:false} on a received response — if the network stalls without ever throwing or resolving, App.tsx's stopping state (and the disabled Stop button) never recovers until the browser's own long default socket timeout kicks in.
♻️ Suggested fix
export async function requestProxyStop(
apiBase: string,
fetchFn: typeof fetch = fetch,
): Promise<ProxyStopOutcome> {
let response: Response;
try {
- response = await fetchFn(`${apiBase}/api/stop`, { method: "POST" });
+ response = await fetchFn(`${apiBase}/api/stop`, { method: "POST", signal: AbortSignal.timeout(10_000) });
} catch {
return { accepted: true };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function requestProxyStop( | |
| apiBase: string, | |
| fetchFn: typeof fetch = fetch, | |
| ): Promise<ProxyStopOutcome> { | |
| let response: Response; | |
| try { | |
| response = await fetchFn(`${apiBase}/api/stop`, { method: "POST" }); | |
| } catch { | |
| return { accepted: true }; | |
| } | |
| const payload = await response.json().catch(() => null) as ProxyStopPayload | null; | |
| if (!response.ok || payload?.success === false) { | |
| return { accepted: false, message: failureMessage(payload, response.status) }; | |
| } | |
| return { accepted: true }; | |
| } | |
| export async function requestProxyStop( | |
| apiBase: string, | |
| fetchFn: typeof fetch = fetch, | |
| ): Promise<ProxyStopOutcome> { | |
| let response: Response; | |
| try { | |
| response = await fetchFn(`${apiBase}/api/stop`, { method: "POST", signal: AbortSignal.timeout(10_000) }); | |
| } catch { | |
| return { accepted: true }; | |
| } | |
| const payload = await response.json().catch(() => null) as ProxyStopPayload | null; | |
| if (!response.ok || payload?.success === false) { | |
| return { accepted: false, message: failureMessage(payload, response.status) }; | |
| } | |
| return { accepted: true }; | |
| } |
🤖 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 `@gui/src/stop-proxy.ts` around lines 23 - 39, Update requestProxyStop to
enforce a finite timeout around the fetchFn call, using an AbortController or
equivalent AbortSignal and ensuring the timer is cleared after completion. Treat
timeout-induced aborts like other request errors by returning { accepted: true
}, while preserving existing response validation and cleanup behavior.
| test("App clears stopping state and alerts for every rejected stop outcome", async () => { | ||
| const app = await Bun.file(new URL("../src/App.tsx", import.meta.url)).text(); | ||
| const handler = app.slice(app.indexOf("const handleStop"), app.indexOf("const brand")); | ||
|
|
||
| expect(handler).toContain("await requestProxyStop(API_BASE)"); | ||
| expect(handler).toContain("if (!outcome.accepted)"); | ||
| expect(handler).toContain("setStopping(false)"); | ||
| expect(handler).toContain("alert(outcome.message)"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the indexOf results before slicing.
app.indexOf("const handleStop") and app.indexOf("const brand") (Line 32) are used unguarded. If "const brand" disappears or moves before handleStop in App.tsx in a future refactor, indexOf returns -1 and .slice(start, -1) silently widens the extracted handler string to almost the entire file instead of failing loudly — the test would then keep passing without actually verifying that the assertions are scoped to handleStop, masking a real regression. gui/tests/dashboard-contracts.test.ts (Lines 18-19) already establishes the safer pattern of asserting toBeGreaterThan(-1) before slicing; this test should follow suit.
🧪 Proposed fix
- const handler = app.slice(app.indexOf("const handleStop"), app.indexOf("const brand"));
+ const handlerStart = app.indexOf("const handleStop");
+ const handlerEnd = app.indexOf("const brand");
+ expect(handlerStart).toBeGreaterThan(-1);
+ expect(handlerEnd).toBeGreaterThan(handlerStart);
+ const handler = app.slice(handlerStart, handlerEnd);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("App clears stopping state and alerts for every rejected stop outcome", async () => { | |
| const app = await Bun.file(new URL("../src/App.tsx", import.meta.url)).text(); | |
| const handler = app.slice(app.indexOf("const handleStop"), app.indexOf("const brand")); | |
| expect(handler).toContain("await requestProxyStop(API_BASE)"); | |
| expect(handler).toContain("if (!outcome.accepted)"); | |
| expect(handler).toContain("setStopping(false)"); | |
| expect(handler).toContain("alert(outcome.message)"); | |
| }); | |
| test("App clears stopping state and alerts for every rejected stop outcome", async () => { | |
| const app = await Bun.file(new URL("../src/App.tsx", import.meta.url)).text(); | |
| const handlerStart = app.indexOf("const handleStop"); | |
| const handlerEnd = app.indexOf("const brand"); | |
| expect(handlerStart).toBeGreaterThan(-1); | |
| expect(handlerEnd).toBeGreaterThan(handlerStart); | |
| const handler = app.slice(handlerStart, handlerEnd); | |
| expect(handler).toContain("await requestProxyStop(API_BASE)"); | |
| expect(handler).toContain("if (!outcome.accepted)"); | |
| expect(handler).toContain("setStopping(false)"); | |
| expect(handler).toContain("alert(outcome.message)"); | |
| }); |
🤖 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 `@gui/tests/app-stop.test.ts` around lines 30 - 38, Guard both indexOf results
in the test before slicing the App.tsx source: assert that the "const
handleStop" and "const brand" markers are found and ordered correctly, then
derive handler with slice. Keep the existing handler-content assertions
unchanged so the test fails loudly when either boundary is missing or misplaced.
| function validFileConfigDiagnostics(config: OcxConfig, rawParsed: unknown): ConfigDiagnostics { | ||
| // An unsafe hand-edited opt-in is disabled in memory instead of rejecting | ||
| // the entire config, which would hide unrelated providers/accounts. The next | ||
| // ordinary save persists the normalized absence. | ||
| const syncDisabledReason = nativeSubagentSyncDisabledReason(config, rawParsed); | ||
| config = normalizeNativeSubagentSync(config, rawParsed); | ||
| const warnings = configPlaceholderWarnings(config); | ||
| warnings.push(...malformedNativeSubagentFields(rawParsed).map(malformedNativeSubagentFieldWarning)); | ||
| if (syncDisabledReason) { | ||
| warnings.push(`syncCodexSubagentDefaults ignored: ${syncDisabledReason}`); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid reassigning the config parameter; the correctness here depends on statement order.
Line 866 must run before line 867: nativeSubagentSyncDisabledReason returns null as soon as config.syncCodexSubagentDefaults !== true, and normalizeNativeSubagentSync deletes exactly that key. Swap the two lines during any future refactor and syncDisabledReason becomes permanently null, silently dropping the syncCodexSubagentDefaults ignored: … warning from /api/* diagnostics — with no type error and no compile failure to catch it.
Binding the normalized value to a new local makes the dependency explicit and removes the ordering trap.
♻️ Proposed refactor
-function validFileConfigDiagnostics(config: OcxConfig, rawParsed: unknown): ConfigDiagnostics {
+function validFileConfigDiagnostics(parsedConfig: OcxConfig, rawParsed: unknown): ConfigDiagnostics {
// An unsafe hand-edited opt-in is disabled in memory instead of rejecting
// the entire config, which would hide unrelated providers/accounts. The next
// ordinary save persists the normalized absence.
- const syncDisabledReason = nativeSubagentSyncDisabledReason(config, rawParsed);
- config = normalizeNativeSubagentSync(config, rawParsed);
+ // Read the reason BEFORE normalizing: normalization deletes the very key the
+ // reason check keys on.
+ const syncDisabledReason = nativeSubagentSyncDisabledReason(parsedConfig, rawParsed);
+ const config = normalizeNativeSubagentSync(parsedConfig, rawParsed);
const warnings = configPlaceholderWarnings(config);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validFileConfigDiagnostics(config: OcxConfig, rawParsed: unknown): ConfigDiagnostics { | |
| // An unsafe hand-edited opt-in is disabled in memory instead of rejecting | |
| // the entire config, which would hide unrelated providers/accounts. The next | |
| // ordinary save persists the normalized absence. | |
| const syncDisabledReason = nativeSubagentSyncDisabledReason(config, rawParsed); | |
| config = normalizeNativeSubagentSync(config, rawParsed); | |
| const warnings = configPlaceholderWarnings(config); | |
| warnings.push(...malformedNativeSubagentFields(rawParsed).map(malformedNativeSubagentFieldWarning)); | |
| if (syncDisabledReason) { | |
| warnings.push(`syncCodexSubagentDefaults ignored: ${syncDisabledReason}`); | |
| } | |
| function validFileConfigDiagnostics(parsedConfig: OcxConfig, rawParsed: unknown): ConfigDiagnostics { | |
| // An unsafe hand-edited opt-in is disabled in memory instead of rejecting | |
| // the entire config, which would hide unrelated providers/accounts. The next | |
| // ordinary save persists the normalized absence. | |
| // Read the reason BEFORE normalizing: normalization deletes the very key the | |
| // reason check keys on. | |
| const syncDisabledReason = nativeSubagentSyncDisabledReason(parsedConfig, rawParsed); | |
| const config = normalizeNativeSubagentSync(parsedConfig, rawParsed); | |
| const warnings = configPlaceholderWarnings(config); | |
| warnings.push(...malformedNativeSubagentFields(rawParsed).map(malformedNativeSubagentFieldWarning)); | |
| if (syncDisabledReason) { | |
| warnings.push(`syncCodexSubagentDefaults ignored: ${syncDisabledReason}`); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/config.ts` around lines 862 - 872, Update validFileConfigDiagnostics to
avoid reassigning its config parameter: compute the disabled reason from the
original config first, bind normalizeNativeSubagentSync(config, rawParsed) to a
new local, and use that local for subsequent configPlaceholderWarnings and
related diagnostics. Preserve the existing ordering so syncDisabledReason
remains available for the ignored-warning message.
| ))); | ||
| return jsonResponse({ | ||
| multiAgentGuidanceEnabled: multiAgentGuidanceEnabled(config), | ||
| syncCodexSubagentDefaults: config.syncCodexSubagentDefaults === true && Boolean(config.injectionModel), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The "sync is effectively on" rule is now duplicated four times across three files — extract one predicate.
config.syncCodexSubagentDefaults === true && Boolean(config.injectionModel) appears at line 183, again at lines 211-212, and again at line 273 in this file; configuredManagedSubagentDefaults in src/codex/inject.ts (lines 78) encodes the same rule with !config.injectionModel, and nativeSubagentSyncDisabledReason in src/config.ts (lines 771-774) encodes a stricter variant using injectionModel?.trim().
That last one is already a divergence: this route treats a whitespace-only injectionModel as truthy (Boolean(" ") is true), while src/config.ts treats it as blank and disables sync. A hand-edited "injectionModel": " " therefore makes GET /api/injection-model report syncCodexSubagentDefaults: true while loadConfig() strips the flag and injection never writes the defaults. Note the PUT path at line 247 already uses nextModel?.trim(), so the trim rule is the intended one — GET/PUT responses are the outliers.
♻️ Proposed fix: one exported predicate, trim-consistent
Add next to the other config helpers (e.g. src/config.ts):
/** Single source of truth for "native sub-agent default sync is effectively on". */
export function subagentDefaultSyncEffective(
config: Pick<OcxConfig, "syncCodexSubagentDefaults" | "injectionModel">,
): boolean {
return config.syncCodexSubagentDefaults === true && Boolean(config.injectionModel?.trim());
}Then in this file:
- syncCodexSubagentDefaults: config.syncCodexSubagentDefaults === true && Boolean(config.injectionModel),
+ syncCodexSubagentDefaults: subagentDefaultSyncEffective(config),applied at lines 183, 211-212, and 273, and reuse it for the guard in configuredManagedSubagentDefaults.
🤖 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 `@src/server/management/agent-settings-routes.ts` at line 183, Extract the
shared trim-aware predicate subagentDefaultSyncEffective in src/config.ts using
the syncCodexSubagentDefaults and injectionModel fields, then replace the
duplicated Boolean checks in this route’s GET/PUT response and guard logic with
it. Update configuredManagedSubagentDefaults in src/codex/inject.ts to reuse the
same predicate, preserving the intended behavior that whitespace-only
injectionModel values disable sync.
| if (body.syncCodexSubagentDefaults === true && !nextModel?.trim()) { | ||
| return jsonResponse({ error: "syncCodexSubagentDefaults requires an injection model" }, 400); | ||
| } | ||
| if (nextSyncCodexSubagentDefaults && nextEffort !== undefined && !isCodexReasoningEffort(nextEffort)) { | ||
| return jsonResponse({ error: "syncCodexSubagentDefaults requires a supported Codex reasoning effort" }, 400); | ||
| } | ||
| // Clearing the model always clears model-dependent settings. | ||
| if (!nextModel) { | ||
| nextEffort = undefined; | ||
| nextSyncCodexSubagentDefaults = false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reorder the model-clearing reset above the effort guard, or clearing the model can be permanently rejected.
Line 250 gates on the effective nextSyncCodexSubagentDefaults (which inherits true from config), while the reset at lines 254-257 that clears nextEffort runs afterwards. With a persisted syncCodexSubagentDefaults: true and a stale unsupported config.injectionEffort (hand-edited, or written by an older build before the ladder changed), a PUT {"model": null} takes this path:
- line 230 →
nextModel = undefined - line 247 → skipped (
body.syncCodexSubagentDefaultsis absent, nottrue) - line 250 →
nextSyncCodexSubagentDefaultsis still inheritedtrue,nextEffortis the stale invalid value → 400 - lines 254-257 never run
The user cannot clear the injection model through the API at all, even though clearing it is exactly the action that would resolve the invalid state. loadConfig()'s normalization (src/config.ts lines 776-778) hides this on a fresh process, but the route mutates the shared in-memory config, so it persists for the life of the running proxy.
Applying the model-cleared reset first makes the guard consistent with the state actually about to be saved:
🐛 Proposed fix
if (body.syncCodexSubagentDefaults === true && !nextModel?.trim()) {
return jsonResponse({ error: "syncCodexSubagentDefaults requires an injection model" }, 400);
}
+ // Clearing the model always clears model-dependent settings — do this before
+ // the effort guard so a stale invalid effort cannot block the clear itself.
+ if (!nextModel) {
+ nextEffort = undefined;
+ nextSyncCodexSubagentDefaults = false;
+ }
if (nextSyncCodexSubagentDefaults && nextEffort !== undefined && !isCodexReasoningEffort(nextEffort)) {
return jsonResponse({ error: "syncCodexSubagentDefaults requires a supported Codex reasoning effort" }, 400);
}
- // Clearing the model always clears model-dependent settings.
- if (!nextModel) {
- nextEffort = undefined;
- nextSyncCodexSubagentDefaults = false;
- }A regression test for PUT {"model": null} against a config with syncCodexSubagentDefaults: true plus an unsupported injectionEffort would lock this in alongside the existing cases in tests/injection-model-api.test.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (body.syncCodexSubagentDefaults === true && !nextModel?.trim()) { | |
| return jsonResponse({ error: "syncCodexSubagentDefaults requires an injection model" }, 400); | |
| } | |
| if (nextSyncCodexSubagentDefaults && nextEffort !== undefined && !isCodexReasoningEffort(nextEffort)) { | |
| return jsonResponse({ error: "syncCodexSubagentDefaults requires a supported Codex reasoning effort" }, 400); | |
| } | |
| // Clearing the model always clears model-dependent settings. | |
| if (!nextModel) { | |
| nextEffort = undefined; | |
| nextSyncCodexSubagentDefaults = false; | |
| } | |
| if (body.syncCodexSubagentDefaults === true && !nextModel?.trim()) { | |
| return jsonResponse({ error: "syncCodexSubagentDefaults requires an injection model" }, 400); | |
| } | |
| // Clearing the model always clears model-dependent settings — do this before | |
| // the effort guard so a stale invalid effort cannot block the clear itself. | |
| if (!nextModel) { | |
| nextEffort = undefined; | |
| nextSyncCodexSubagentDefaults = false; | |
| } | |
| if (nextSyncCodexSubagentDefaults && nextEffort !== undefined && !isCodexReasoningEffort(nextEffort)) { | |
| return jsonResponse({ error: "syncCodexSubagentDefaults requires a supported Codex reasoning effort" }, 400); | |
| } |
🤖 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 `@src/server/management/agent-settings-routes.ts` around lines 247 - 257, Move
the model-clearing reset block above the supported-effort validation in the
route handling update, so clearing nextModel also clears nextEffort and
nextSyncCodexSubagentDefaults before the guard evaluates them. Preserve the
existing validation for non-cleared models and add a regression test covering
PUT with model null, inherited syncCodexSubagentDefaults true, and an
unsupported injectionEffort.
| | Key providers | Expose API-key provider presets for setup and dashboard flows. Multi-key pool per key-auth provider: `GET /api/providers/keys`, `POST /api/providers/keys`, `PUT /api/providers/keys/active`, `PUT /api/providers/keys/alias`, `DELETE /api/providers/keys` masked list, add (upsert + activate), switch, rename, and remove keys. `provider.apiKey` always mirrors the active pool entry so routing stays single-key. | | ||
| | OpenAI account mode | Report one OpenAI Codex card with Pool/Direct controls and one API-key card. Mode PATCH persists live without restart or catalog identity changes; Pool owns account/quota controls and Direct uses caller/main login only. Main-account DTOs report real credential presence and terminal `needsReauth` state instead of treating missing/invalid native auth as an unknown quota. | | ||
| | Subagents | Read/write the featured `subagentModels` list capped at five ids. | | ||
| | Subagents | Read/write the featured `subagentModels` list capped at five ids. `GET/PUT /api/injection-model` manages the shared delegation model/effort selection, the independent OpenCodex guidance switch, and the default-off `syncCodexSubagentDefaults` opt-in for native Codex subagent defaults. When OpenCodex owns the active Codex routing, native `[agents]` defaults apply to newly created Codex tasks after sync/restart; external user-managed provider configs remain untouched. The defaults do not cause delegation and preserve existing user-owned defaults rather than overwriting them. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the PUT rejection rules, matching how the /api/v2 row states its contract.
The row describes what the endpoint manages but not what it refuses. src/server/management/agent-settings-routes.ts enforces two hard 400s (lines 247-252): enabling syncCodexSubagentDefaults without an injection model, and enabling it while the effective injectionEffort is not a supported Codex reasoning effort. It also force-clears the effort and the sync flag when the model is cleared (lines 254-257). The neighbouring /api/v2 row already sets the precedent by stating "contradictory mode/flag pairs are rejected before writes", so a maintainer reading only this row would not expect the cross-field validation.
📝 Suggested addition
-| Subagents | Read/write the featured `subagentModels` list capped at five ids. `GET/PUT /api/injection-model` manages the shared delegation model/effort selection, the independent OpenCodex guidance switch, and the default-off `syncCodexSubagentDefaults` opt-in for native Codex subagent defaults. When OpenCodex owns the active Codex routing, native `[agents]` defaults apply to newly created Codex tasks after sync/restart; external user-managed provider configs remain untouched. The defaults do not cause delegation and preserve existing user-owned defaults rather than overwriting them. |
+| Subagents | Read/write the featured `subagentModels` list capped at five ids. `GET/PUT /api/injection-model` manages the shared delegation model/effort selection, the independent OpenCodex guidance switch, and the default-off `syncCodexSubagentDefaults` opt-in for native Codex subagent defaults. PUT rejects enabling the opt-in without an injection model or with an unsupported reasoning effort, and clearing the model clears both the effort and the opt-in. When OpenCodex owns the active Codex routing, native `[agents]` defaults apply to newly created Codex tasks after sync/restart; external user-managed provider configs remain untouched. The defaults do not cause delegation and preserve existing user-owned defaults rather than overwriting them. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Subagents | Read/write the featured `subagentModels` list capped at five ids. `GET/PUT /api/injection-model` manages the shared delegation model/effort selection, the independent OpenCodex guidance switch, and the default-off `syncCodexSubagentDefaults` opt-in for native Codex subagent defaults. When OpenCodex owns the active Codex routing, native `[agents]` defaults apply to newly created Codex tasks after sync/restart; external user-managed provider configs remain untouched. The defaults do not cause delegation and preserve existing user-owned defaults rather than overwriting them. | | |
| | Subagents | Read/write the featured `subagentModels` list capped at five ids. `GET/PUT /api/injection-model` manages the shared delegation model/effort selection, the independent OpenCodex guidance switch, and the default-off `syncCodexSubagentDefaults` opt-in for native Codex subagent defaults. PUT rejects enabling the opt-in without an injection model or with an unsupported reasoning effort, and clearing the model clears both the effort and the opt-in. When OpenCodex owns the active Codex routing, native `[agents]` defaults apply to newly created Codex tasks after sync/restart; external user-managed provider configs remain untouched. The defaults do not cause delegation and preserve existing user-owned defaults rather than overwriting them. | |
🤖 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 `@structure/05_gui-and-management-api.md` at line 23, Update the Subagents row
documenting PUT /api/injection-model to state that contradictory cross-field
combinations are rejected with HTTP 400: enabling syncCodexSubagentDefaults
requires an injection model and a supported Codex reasoning injectionEffort.
Also document that clearing the model force-clears injectionEffort and
syncCodexSubagentDefaults, matching the enforcement in the agent-settings route.
| test("kept-user-base-url: reports routing NOT injected and leaves the user's override alone", () => { | ||
| writeFileSync(join(codexHome, "config.toml"), [ | ||
| 'openai_base_url = "https://my-own-gateway.example/v1"', | ||
| 'model = "gpt-5.5"', | ||
| "", | ||
| ].join("\n"), "utf8"); | ||
|
|
||
| const r = runInject(codexHome, ocxHome); | ||
| const r = runInject(codexHome, ocxHome, JSON.stringify({ | ||
| syncCodexSubagentDefaults: true, | ||
| injectionModel: "gpt-5.6-sol", | ||
| injectionEffort: "high", | ||
| })); | ||
| expect(r.status).toBe(0); | ||
| const result = JSON.parse(r.stdout); | ||
| expect(result.success).toBe(true); | ||
| expect(result.message).toContain("routing NOT injected"); | ||
| expect(result.message).not.toContain("All models now route through opencodex proxy"); | ||
| expect(result.nativeSubagentDefaultsWarning).toContain("user-owned root openai_base_url"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Coverage gap: warning is asserted, but the actual config.toml write isn't verified in the same conflict scenarios.
Both extended tests confirm result.nativeSubagentDefaultsWarning fires when routing can't be injected (user-owned openai_base_url at Line 238, external model_provider at Line 300), but neither reads back config.toml to confirm default_subagent_model/default_subagent_reasoning_effort were not actually written in these branches. Since the whole point of this warning is "routing-ownership blocks injection" (per PR objectives), a regression where the native defaults get written anyway (pointing at a model that isn't actually reachable through the unrouted provider) would silently slip past this suite — only the warning text is checked, not the resulting file state.
🧪 Suggested additional assertions
expect(result.nativeSubagentDefaultsWarning).toContain("user-owned root openai_base_url");
+ const injected = readFileSync(join(codexHome, "config.toml"), "utf8");
+ expect(injected).not.toContain("default_subagent_model");
+ expect(injected).not.toContain("default_subagent_reasoning_effort");(equivalent addition for the external-provider test around Line 300)
Since this depends on how src/codex/inject.ts actually branches when nativeSubagentDefaultsWarning is set (not included in this review batch), please confirm the intended behavior before treating this purely as a test-only gap.
Also applies to: 288-300
🤖 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 `@tests/codex-inject-integration.test.ts` around lines 221 - 238, Extend the
conflict scenarios in the tests around “kept-user-base-url” and the external
“model_provider” case to read back config.toml after runInject. Assert that
default_subagent_model and default_subagent_reasoning_effort remain absent or
unchanged when nativeSubagentDefaultsWarning indicates routing ownership blocked
injection, while preserving the existing warning and success assertions. Confirm
the inject branch only writes these defaults when routing injection is allowed.
There was a problem hiding this comment.
Verdict
Request changes ÔÇö one API contract bug in model-clear ordering, plus required CI has not actually run (fork action_required). Feature direction and ownership/TOML design look solid; not merge-ready yet.
Summary
Opt-in syncCodexSubagentDefaults is well scoped: marker-owned TOML edits, journal scrub, structured warnings, docs/structure/GUI/API coverage, and focused tests. Base is correctly dev. The main code defect is that the documented "clearing model always clears effort + sync" path can 400 before the clear side effects run.
Blocking findings
PUT /api/injection-modelmodel-clear can 400 before clearing ÔÇösrc/server/management/agent-settings-routes.ts~L250ÔÇô257- Failure mode: With in-memory
syncCodexSubagentDefaults: true, a model set, and a non-CodexinjectionEffort,PUT { "model": null }hits the sync+effort gate at L250ÔÇô252 and returns400 syncCodexSubagentDefaults requires a supported Codex reasoning effort. The clear block at L254ÔÇô257 (which would drop effort and sync) never runs, so the documented contract ("clearingmodelalways clears both the effort and native-default opt-in") is violated. Load-time normalization usually strips sync for bad effort, so this is easiest to hit via hand-built/makeConfigstate or any path that skipsnormalizeNativeSubagentSyncÔÇö but the control-flow order is still wrong relative to the adjacent comment and docs. - Fix: Move the
if (!nextModel) { nextEffort = undefined; nextSyncCodexSubagentDefaults = false; }block above the sync/effort validation (or skip that gate when clearing the model). Add a regression test: sync on + unsupported inherited effort +PUT { model: null } 200 and all three cleared.
- Failure mode: With in-memory
Merge conflicts (required)
Prior review omitted this; current GitHub state is mergeable: CONFLICTING / DIRTY against dev. Author must rebase or merge dev and resolve the conflicts in this PR before merge (same bar as the other required quality items below).
Content conflicts in:
src/cli/index.tstests/cli-restore-back.test.ts
Additional required quality changes (fix in this PR before merge)
These are needed for merge quality (same spirit as soft-nits elevation on related work) ÔÇö not optional polish or follow-up PRs.
- Docs curl enables sync without a model ÔÇö
docs-site/src/content/docs/guides/sub-agent-surface.md~L94ÔÇô100 (and ja/ko/ru/zh-cn clones). Snippet is only{"syncCodexSubagentDefaults": true}, which alone is 400 (requires an injection model). Prose below is correct; fix the example payload (include model) or mark it as "after model is set". Sync locales. - Duplicated "sync effectively on" predicate ÔÇö
config.syncCodexSubagentDefaults === true && Boolean(config.injectionModel)appears in GET/PUT responses and inject paths. Extract one helper (whitespace-aware if you want GET/PUT and inject to agree on trim). Consolidate in this PR so response and inject paths cannot drift. - Lifecycle / stop-proxy blast radius ÔÇö exit-code and
stop-proxytyping changes are adjacent to restore reporting; keep them only if intentional for this feature, otherwise split out of this PR so review scope stays clear. (CodeRabbit's Grok-strip exit-code / atomic ownership / GUI timeout claims look overstated against this diff ÔÇö ownership early-return + tests cover the pair; scope correctly limits Grok best-effort.)
Standards / Spec / Security notes
- Standards: Base
devOK. Bun-native, privacy, and no auth/credential/workflow surface touched. Behavior changes have focused tests; docs + structure updated. Docs example vs API remains a required standards fix before merge. - Spec: PR-body requirements largely met (opt-in, ownership/TOML, journal scrub, API/Dashboard/i18n/docs, warnings, nonzero restore/sync reporting). Spec gap is the model-clear ordering above. Scope creep is small and justified.
- Security / privacy: No secret logging, no OAuth/token/workflow changes. Config.toml mutation remains marker-scoped and fail-closed on ambiguous ownership ÔÇö good. Not a security-boundary PR under MAINTAINERS.md, but config-file mutation still deserves careful review (this PR's ownership model looks sound).
CI / process gates
- Target branch:
devÔÇö correct. - Required checks:
Cross-platform CI,Service lifecycle, andReact Doctorare allaction_required(fork PR fromwonsh42/opencodex) ÔÇö they have not executed. Maintainer must approve workflows and wait for green before merge. Author-reported localbun run test/ typecheck / privacy:scan are not a substitute. - Maintainer approval still required per MAINTAINERS.md; this review is not an approve.
After fixes
Once all blocking + required quality changes above are addressed (and conflicts/rebase done), mark this PR Ready for review (leave draft only until then). Then ping for re-review / CI.
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
2 participants
Summary
syncCodexSubagentDefaultsoption that applies the selectedinjectionModeland optionalinjectionEffortto Codex's native[agents]defaults on sync or restart.GET/PUT /api/injection-model, the Dashboard, localized UI text, user documentation, and maintainer structure docs for the opt-in.nativeSubagentDefaultsWarning; failed sync and restore paths now report actionable errors instead of claiming success.Why
The Dashboard's subagent model and effort could inform OpenCodex-authored delegation guidance, but they did not populate Codex's native subagent defaults. This adds an explicit and reversible path for newly created Codex tasks while keeping existing user-authored configuration authoritative.
Safety and ownership
agents.default_subagent_modelandagents.default_subagent_reasoning_effortvalues.model_providerconfigurations and a user-owned rootopenai_base_urlblock native-default injection and return a structured warning.~/.codex/config.tomland~/.opencodex/config.jsonare runtime inputs or mutation targets, not committed artifacts.Scope
This PR intentionally excludes broad service-manager changes, provider work, v2 surface/routing changes, and general concurrency hardening. Lifecycle edits are limited to accurately reporting sync and native-restore failures.
Verification
bun run test— 4,667 pass, 0 fail across 356 files.bun run typecheck— passed.bun run lint:gui— passed.debug-put-install-order.test.tsx; the same assertion fails onupstream/dev(270 pass, 1 fail), while every impacted test passes in isolation.cd gui && bun run build— passed (164 modules).cd docs-site && bun run build— passed (131 pages).bun run privacy:scan— passed.git diff --check upstream/dev— passed.Checklist
Summary by CodeRabbit
New Features
Documentation