fix(codex): auto-enable harness plugin for forced runtime#67474
Conversation
Greptile SummaryThis PR wires up the previously-inert The remaining findings are P2 style notes: near-identical harness runtime collection functions in two modules, and a missing test for the Confidence Score: 5/5Safe to merge — the logic is correct end-to-end, tests cover the primary config-driven paths, and no P0/P1 issues were found. All remaining findings are P2 style/quality suggestions (duplicated helper function and a missing env-var test case). The core activation and startup logic is sound and well-tested. No files require special attention, though src/config/plugin-auto-enable.shared.ts and src/plugins/channel-plugin-ids.ts share near-identical harness-runtime collection logic that is worth consolidating in a follow-up. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/plugin-auto-enable.shared.ts
Line: 102-127
Comment:
**Duplicated harness runtime collection logic**
`collectEmbeddedHarnessRuntimes` here and `collectRequestedAgentHarnessRuntimes` in `channel-plugin-ids.ts` (lines 53–78) are near-identical implementations — same three sources (`agents.defaults`, `agents.list[*]`, `OPENCLAW_AGENT_RUNTIME`), same "auto"/"pi" exclusion, same deduplication. They differ only in the agent-list entry guard (`isRecord` vs. an explicit object check). If the filtering logic ever changes (e.g., a new excluded runtime name), both copies need to be updated in sync. A shared utility at a neutral boundary (e.g., `src/agents/harness-runtimes.ts`) would keep the two call sites consistent.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/plugin-auto-enable.core.test.ts
Line: 220-248
Comment:
**Missing test for `OPENCLAW_AGENT_RUNTIME` auto-enable path**
The new test covers the config-driven path (`agents.defaults.embeddedHarness.runtime`), but the third source in `collectEmbeddedHarnessRuntimes` — `env.OPENCLAW_AGENT_RUNTIME` — has no test in this file. Since `makeIsolatedEnv()` doesn't set that variable, a user who relies on the env-var path to force the codex runtime would not be covered. A companion case passing `makeIsolatedEnv({ OPENCLAW_AGENT_RUNTIME: "codex" })` would close the gap.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(codex): activate harness plugin for ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function collectEmbeddedHarnessRuntimes(cfg: OpenClawConfig, env: NodeJS.ProcessEnv): string[] { | ||
| const runtimes = new Set<string>(); | ||
| const pushRuntime = (value: unknown) => { | ||
| if (typeof value !== "string") { | ||
| return; | ||
| } | ||
| const normalized = normalizeOptionalLowercaseString(value); | ||
| if (!normalized || normalized === "auto" || normalized === "pi") { | ||
| return; | ||
| } | ||
| runtimes.add(normalized); | ||
| }; | ||
|
|
||
| pushRuntime(cfg.agents?.defaults?.embeddedHarness?.runtime); | ||
| if (Array.isArray(cfg.agents?.list)) { | ||
| for (const agent of cfg.agents.list) { | ||
| if (!isRecord(agent)) { | ||
| continue; | ||
| } | ||
| pushRuntime((agent.embeddedHarness as Record<string, unknown> | undefined)?.runtime); | ||
| } | ||
| } | ||
| pushRuntime(env.OPENCLAW_AGENT_RUNTIME); | ||
|
|
||
| return [...runtimes].toSorted((left, right) => left.localeCompare(right)); | ||
| } |
There was a problem hiding this comment.
collectEmbeddedHarnessRuntimes here and collectRequestedAgentHarnessRuntimes in channel-plugin-ids.ts (lines 53–78) are near-identical implementations — same three sources (agents.defaults, agents.list[*], OPENCLAW_AGENT_RUNTIME), same "auto"/"pi" exclusion, same deduplication. They differ only in the agent-list entry guard (isRecord vs. an explicit object check). If the filtering logic ever changes (e.g., a new excluded runtime name), both copies need to be updated in sync. A shared utility at a neutral boundary (e.g., src/agents/harness-runtimes.ts) would keep the two call sites consistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/plugin-auto-enable.shared.ts
Line: 102-127
Comment:
**Duplicated harness runtime collection logic**
`collectEmbeddedHarnessRuntimes` here and `collectRequestedAgentHarnessRuntimes` in `channel-plugin-ids.ts` (lines 53–78) are near-identical implementations — same three sources (`agents.defaults`, `agents.list[*]`, `OPENCLAW_AGENT_RUNTIME`), same "auto"/"pi" exclusion, same deduplication. They differ only in the agent-list entry guard (`isRecord` vs. an explicit object check). If the filtering logic ever changes (e.g., a new excluded runtime name), both copies need to be updated in sync. A shared utility at a neutral boundary (e.g., `src/agents/harness-runtimes.ts`) would keep the two call sites consistent.
How can I resolve this? If you propose a fix, please make it concise.| it("auto-enables an opt-in plugin when an embedded agent harness runtime is configured", () => { | ||
| const result = applyPluginAutoEnable({ | ||
| config: { | ||
| agents: { | ||
| defaults: { | ||
| embeddedHarness: { | ||
| runtime: "codex", | ||
| fallback: "none", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| env: makeIsolatedEnv(), | ||
| manifestRegistry: makeRegistry([ | ||
| { | ||
| id: "codex", | ||
| channels: [], | ||
| activation: { | ||
| onAgentHarnesses: ["codex"], | ||
| }, | ||
| }, | ||
| ]), | ||
| }); | ||
|
|
||
| expect(result.config.plugins?.entries?.codex?.enabled).toBe(true); | ||
| expect(result.changes).toContain( | ||
| "codex agent harness runtime configured, enabled automatically.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
OPENCLAW_AGENT_RUNTIME auto-enable path
The new test covers the config-driven path (agents.defaults.embeddedHarness.runtime), but the third source in collectEmbeddedHarnessRuntimes — env.OPENCLAW_AGENT_RUNTIME — has no test in this file. Since makeIsolatedEnv() doesn't set that variable, a user who relies on the env-var path to force the codex runtime would not be covered. A companion case passing makeIsolatedEnv({ OPENCLAW_AGENT_RUNTIME: "codex" }) would close the gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/plugin-auto-enable.core.test.ts
Line: 220-248
Comment:
**Missing test for `OPENCLAW_AGENT_RUNTIME` auto-enable path**
The new test covers the config-driven path (`agents.defaults.embeddedHarness.runtime`), but the third source in `collectEmbeddedHarnessRuntimes` — `env.OPENCLAW_AGENT_RUNTIME` — has no test in this file. Since `makeIsolatedEnv()` doesn't set that variable, a user who relies on the env-var path to force the codex runtime would not be covered. A companion case passing `makeIsolatedEnv({ OPENCLAW_AGENT_RUNTIME: "codex" })` would close the gap.
How can I resolve this? If you propose a fix, please make it concise.![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: 4299fe4580
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (requiredAgentHarnessPluginIds.has(plugin.id)) { | ||
| const activationState = resolveEffectivePluginActivationState({ | ||
| id: plugin.id, | ||
| origin: plugin.origin, | ||
| config: pluginsConfig, |
There was a problem hiding this comment.
This new early-return path loads any plugin matched by onAgentHarnesses as soon as activationState.enabled is true, which bypasses the non-bundled startup gate later in this function (plugin.origin !== "bundled" requiring explicit enablement). In practice, a workspace/global plugin can now be loaded at startup by just declaring activation.onAgentHarnesses: ["codex"] and having agents.*.embeddedHarness.runtime=codex, even when the plugin was never explicitly enabled. That is a behavior and trust-model regression versus the prior startup policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3b170fe54
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .filter((plugin) => | ||
| (plugin.activation?.onAgentHarnesses ?? []).some( | ||
| (entry) => normalizeOptionalLowercaseString(entry) === normalizedRuntime, | ||
| ), |
There was a problem hiding this comment.
resolveAgentHarnessOwnerPluginIds matches any plugin manifest that declares activation.onAgentHarnesses for a runtime, but it does not filter by origin or trust policy. That candidate set is then auto-materialized into plugins.entries.<id>.enabled=true, so forcing a runtime like codex can auto-enable a workspace/global plugin that only advertises onAgentHarnesses: ["codex"] even when the user never explicitly enabled it. This creates a startup/runtime trust regression for non-bundled plugins and should be constrained similarly to other bundled-only auto-enable paths.
Useful? React with 👍 / 👎.
f3b170f to
2aaaa64
Compare
|
Landed via rebase merge. Source/head: Review follow-up included:
Gates:
|
3 participants
Summary
When
agents.defaults.embeddedHarness.runtimeis forced tocodex, OpenClaw can start the Codex harness without activating the Codex plugin first. That leaves the runtime selected but its harness-owned plugin functionality unavailable at startup.This PR closes that gap by treating configured embedded harness runtimes as plugin auto-enable signals and by letting plugin manifests declare ownership of specific agent harness runtimes.
What changed
codexharness runtimeOPENCLAW_AGENT_RUNTIMEactivation.onAgentHarnessesFiles in scope
extensions/codex/openclaw.plugin.jsonsrc/config/plugin-auto-enable.core.test.tssrc/config/plugin-auto-enable.shared.tssrc/config/plugin-auto-enable.test-helpers.tssrc/config/plugin-auto-enable.types.tssrc/plugins/activation-planner.test.tssrc/plugins/activation-planner.tssrc/plugins/channel-plugin-ids.test.tssrc/plugins/channel-plugin-ids.tssrc/plugins/manifest.tsNon-goals
Validation