Skip to content

feat(codex): support app-server SecretRefs#94324

Merged
kevinlin-openai merged 1 commit into
openclaw:mainfrom
kevinslin:dev/kevinlin/codex-app-server-secretrefs
Jun 19, 2026
Merged

feat(codex): support app-server SecretRefs#94324
kevinlin-openai merged 1 commit into
openclaw:mainfrom
kevinslin:dev/kevinlin/codex-app-server-secretrefs

Conversation

@kevinlin-openai

Copy link
Copy Markdown
Contributor

Summary

  • Allow Codex app-server authToken and headers.* to accept SecretInput values while preserving plaintext string support.
  • Resolve plugin SecretRefs before Codex runtime options are built, with fail-closed handling for unresolved refs.
  • Hash app-server header secret values in shared-client key material and update SecretRef docs/matrix coverage.

Verification

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-codex-app-server-runtime.config.ts extensions/codex/src/app-server/config.test.ts extensions/codex/src/app-server/transport-websocket.test.ts
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.secrets.config.ts src/secrets/runtime.test.ts src/secrets/runtime-config-collectors-plugins.bundled.test.ts src/secrets/target-registry.test.ts src/secrets/target-registry.docs.test.ts
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.plugins.config.ts src/plugins/config-contracts.test.ts
  • git diff --check

@kevinlin-openai
kevinlin-openai requested a review from a team as a code owner June 18, 2026 00:47
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation extensions: codex size: M maintainer Maintainer-authored PR labels Jun 18, 2026
@kevinslin
kevinslin force-pushed the dev/kevinlin/codex-app-server-secretrefs branch from 5389f68 to e0376a1 Compare June 18, 2026 23:57
@clawsweeper

clawsweeper Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 18, 2026, 9:42 PM ET / 01:42 UTC.

Summary
The PR expands Codex app-server authToken and headers.* to accept SecretInput values, resolves them through manifest-owned plugin config contracts, hashes header secrets in shared-client keys, and updates SecretRef docs/tests.

PR surface: Source +66, Tests +289, Docs +20. Total +375 across 12 files.

Reproducibility: yes. for the review finding: source inspection shows appServer.headers.* wildcard matches return a dotted path string and the assignment writer later splits that path on dots. The feature itself still lacks real app-server runtime proof.

Review metrics: 1 noteworthy metric.

  • SecretInput config surfaces: 2 expanded. appServer.authToken and appServer.headers.* now accept SecretInput values, so maintainers should review credential routing before merge.

Stored data model
Persistent data-model change detected: vector/embedding metadata: src/plugins/config-contracts.ts, vector/embedding metadata: src/secrets/target-registry-data.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Preserve concrete wildcard header keys and add dotted-header regression coverage.
  • [P1] Add redacted terminal output, logs, copied live output, or a short recording from a real SecretRef-backed Codex app-server connection.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments provide tests only; add redacted terminal output, logs, copied live output, or a short recording from a real SecretRef-backed Codex app-server connection, then update the PR body so ClawSweeper re-reviews or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] SecretRef headers with dotted names can fail closed because the wildcard match path is split on dots during assignment instead of preserving the literal header key.
  • [P1] The PR body provides unit and contract test commands only; it does not yet show a real SecretRef-backed Codex app-server WebSocket connection after the patch.
  • [P1] The change expands remote app-server credential handling, so maintainers should see plaintext upgrade behavior and SecretRef fresh-install behavior before merge.

Maintainer options:

  1. Fix header assignment and prove the auth path (recommended)
    Preserve wildcard header keys as concrete map keys, add dotted-header regression coverage, and require redacted real app-server SecretRef proof before merge.
  2. Narrow the header SecretRef contract
    If dotted header names are intentionally unsupported, reject and document them before SecretRef resolution instead of letting assignment target the wrong path.
  3. Pause until contributor proof exists
    Keep the PR open but blocked if no one can provide terminal output, logs, copied live output, or a short recording from a real SecretRef-backed app-server setup.

Next step before merge

  • [P1] Human review is needed because the PR has a protected maintainer label, changes auth-provider credential handling, has a source-reproducible header-key bug, and lacks contributor real behavior proof.

Security
Cleared: No concrete security or supply-chain regression was found; the diff changes credential handling but adds no dependencies, workflows, secret exfiltration path, or broader permissions.

Review findings

  • [P2] Preserve dotted header keys during SecretRef assignment — extensions/codex/openclaw.plugin.json:261
Review details

Best possible solution:

Land the manifest-owned SecretInput expansion only after wildcard header assignment preserves literal map keys, dotted-header regression coverage is added, and redacted real app-server proof shows a SecretRef-backed token or header connects.

Do we have a high-confidence way to reproduce the issue?

Yes for the review finding: source inspection shows appServer.headers.* wildcard matches return a dotted path string and the assignment writer later splits that path on dots. The feature itself still lacks real app-server runtime proof.

Is this the best way to solve the issue?

No. The manifest-owned SecretInput expansion is the right layer, but this implementation still needs segment-safe wildcard assignment and real Codex app-server proof before it is the best fix.

Full review comments:

  • [P2] Preserve dotted header keys during SecretRef assignment — extensions/codex/openclaw.plugin.json:261
    appServer.headers.* opts arbitrary header names into SecretRef resolution, but wildcard matches are serialized as dotted paths and the assignment helper later splits on dots. A valid header key like X.Api.Key is treated as nested object traversal instead of replacing headers["X.Api.Key"], leaving the SecretRef unresolved for the WebSocket auth path. Preserve concrete wildcard segments or escape map keys, and add a dotted-header regression test.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2ef0589b760d.

Label changes

Label justifications:

  • P2: This is a normal-priority Codex app-server credential feature with bounded blast radius and one source-reproducible correctness issue.
  • merge-risk: 🚨 auth-provider: The PR changes Codex app-server token/header credential parsing, SecretRef resolution, and shared-client keying before remote WebSocket auth.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and comments provide tests only; add redacted terminal output, logs, copied live output, or a short recording from a real SecretRef-backed Codex app-server connection, then update the PR body so ClawSweeper re-reviews or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +66, Tests +289, Docs +20. Total +375 across 12 files.

View PR surface stats
Area Files Added Removed Net
Source 4 79 13 +66
Tests 4 289 0 +289
Docs 4 29 9 +20
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 12 397 22 +375

What I checked:

Likely related people:

  • steipete: Introduced Codex app-server controls and then split/hardened the app-server lifecycle around the config and runtime surface this PR extends. (role: original app-server contributor; confidence: high; commits: 31a0b7bd42a5, 3b65e2302a55, 659bcc5e5b59; files: extensions/codex/src/app-server/config.ts, extensions/codex/openclaw.plugin.json, extensions/codex/src/app-server/transport-websocket.ts)
  • vincentkoc: Recent history touches Codex app-server auth/profile startup and the manifest config-contract collector paths used by this PR. (role: recent app-server and config-contract contributor; confidence: high; commits: 2c7fe6a39c0f, f1cc8f0cfc7c, e6117618095c; files: extensions/codex/src/app-server/config.ts, src/plugins/config-contracts.ts, src/secrets/runtime-config-collectors-plugins.ts)
  • joshavant: Prior SecretRef runtime, strict/inspect, and target-registry work established the generic credential machinery this PR builds on. (role: related SecretRef feature contributor; confidence: medium; commits: 806803b7efe2, 1769fb2aa1d6, 788f56f30fd9; files: src/config/types.secrets.ts, src/plugin-sdk/secret-input.ts, src/secrets/target-registry-data.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 19, 2026
@kevinslin
kevinslin force-pushed the dev/kevinlin/codex-app-server-secretrefs branch from e0376a1 to c4e3991 Compare June 19, 2026 01:17
@kevinslin
kevinslin force-pushed the dev/kevinlin/codex-app-server-secretrefs branch from c4e3991 to 8ed092e Compare June 19, 2026 01:29
@kevinlin-openai
kevinlin-openai merged commit 741f708 into openclaw:main Jun 19, 2026
166 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 19, 2026
cxbAsDev pushed a commit to cxbAsDev/openclaw that referenced this pull request Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to documentation extensions: codex maintainer Maintainer-authored PR merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants