Skip to content

feat: add permissioned Codex session access - #1164

Draft
OcarolDV wants to merge 30 commits into
openclaw:mainfrom
OcarolDV:feat/windows-codex-session-access
Draft

feat: add permissioned Codex session access#1164
OcarolDV wants to merge 30 commits into
openclaw:mainfrom
OcarolDV:feat/windows-codex-session-access

Conversation

@OcarolDV

Copy link
Copy Markdown

What Problem This Solves

The Windows node cannot currently provide an explicitly authorized, bounded Codex session catalog to a paired OpenClaw Gateway. Operators either lack the metadata needed for private reconciliation or would need a broader, less auditable access path.

Why This Change Was Made

This adds a user-controlled Codex session-access permission, trusted standalone-executable discovery, a hardened App Server lifecycle, and bounded current/history/archived catalog commands. Catalog projections, request accounting, authorization, cancellation, path discovery, and 60-second catalog-specific timeouts are validated while existing command timeouts remain unchanged.

User Impact

Windows users can explicitly allow a paired Gateway to enumerate safe Codex session metadata for Workboard reconciliation, revoke that access, and retain clear permission boundaries without exposing transcript bodies by default.

Evidence

  • ./build.ps1 passed for Shared, CLI, WinNode CLI, Setup Engine, and WinUI.
  • Shared tests: 3,726 passed, 32 skipped, 0 failed.
  • Tray tests: 2,291 passed, 0 skipped, 0 failed under isolated tray data.
  • Current-head paired-node proof returned 89 current/history sessions and 0 archived sessions.
  • Cold catalog requests completed with the catalog-specific 60-second request/idle budget; ordinary App Server operations retain their existing limits.

Change Type

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Validation

  • ./build.ps1 — passed.
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore — 3,726 passed, 32 skipped, 0 failed.
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore — 2,291 passed, 0 skipped, 0 failed.

Real Behavior Proof

  • Environment tested: Windows 11 paired to the reviewed OpenClaw 2026.8.1 Gateway deployment.
  • PR head or commit tested: e29b7be53c970ad048d66b9f6836b94a7ed417ea.
  • Exact steps or command run: clean rebuild/restart, then paired current, history-current, and archived catalog invokes.
  • Evidence after fix: 89 current sessions, 89 history-current sessions, 0 archived sessions; all responses passed bounded projection validation.
  • Observed result: cold catalog enumeration completed without the prior five-second idle timeout.
  • Screenshot or artifact links verified? N/A
  • Not verified or blocked: no known blocker.

Security Impact

  • New permissions or capabilities? Yes
  • Secrets or tokens handling changed? No
  • New or changed network calls? Yes
  • Command or tool execution surface changed? Yes
  • Data access scope changed? Yes
  • If any answer is Yes, explain the risk and mitigation: access is disabled until explicitly granted, bound to the paired Gateway lifecycle, revoked on permission changes, limited to allowlisted catalog methods, projected to bounded metadata, and backed by traversal/reparse/executable-trust and request-lifecycle tests.

Compatibility and Migration

  • Backward compatible? Yes
  • Config or environment changes? No
  • Migration needed? No
  • If yes, list the exact upgrade steps: N/A

Review Conversations

  • I replied to or resolved every bot review conversation addressed by this PR.
  • I left unresolved only conversations that still need maintainer judgment.

OpenClaw Codex added 30 commits August 9, 2026 22:29
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 15, 2026, 8:35 AM ET / 12:35 UTC.

ClawSweeper review

What this changes

This PR adds a tray-controlled permission that exposes three bounded, read-only Codex App Server catalog and transcript commands through the Windows node’s Gateway and local MCP transports.

Merge readiness

Blocked until stronger real behavior proof is added - 10 items remain

Keep open for maintainer direction and a rebase. The branch adds a real privacy-sensitive capability, but its current head conflicts with and removes the current-main settings-change seam, and its proof does not include inspectable command output.

Priority: P2
Reviewed head: e29b7be53c970ad048d66b9f6836b94a7ed417ea
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The branch has substantial tests and stated validation, but it is conflicted with current-main architecture and lacks inspectable after-fix proof.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The body reports paired-node counts, but it does not provide inspectable redacted winnode or raw MCP output, Gateway output, UI evidence, or a runtime artifact from the stated head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body reports paired-node counts, but it does not provide inspectable redacted winnode or raw MCP output, Gateway output, UI evidence, or a runtime artifact from the stated head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current main does not implement this feature: The current checkout has no Codex App Server command or session-access implementation outside this PR, and current main does not contain the PR head.
Direct external protocol dependency: The branch launches a local Codex App Server and uses its thread-list and thread-turns-list protocol, so compatibility with that App Server contract is a real dependency. No sibling Codex repository is implicated by this source boundary.
Current-main settings seam is removed by the branch: Current main owns saved-settings orchestration in SettingsChangeCoordinator, while the PR head lacks both coordinator files and restores direct handling in App.xaml.cs.
Findings 1 actionable finding [P2] Preserve the settings-change coordinator
Security None None.

How this fits together

Tray settings control which Windows-node capabilities are available. The node passes permitted catalog requests to a local Codex App Server process and returns bounded results to a paired Gateway or local MCP client.

flowchart LR
  A[Tray settings] --> B[Capability registry]
  B --> C[Codex catalog bridge]
  D[Local Codex App Server] --> C
  C --> E[Paired Gateway]
  C --> F[Local MCP client]
  E --> G[Gateway command trust]
Loading

Decision needed

Question Recommendation
Should OpenClaw Windows node core own a direct, permissioned Codex App Server catalog and transcript bridge, including its external protocol compatibility commitment? Sponsor a narrow core bridge: Accept the read-only catalog direction, preserve the current settings coordinator during rebase, and require explicit Gateway reapproval UX and proof.

Why: The patch adds new persistent permission and capability surface for private transcript access; source review cannot determine whether that long-term integration belongs in core or an extension boundary.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body reports paired-node counts, but it does not provide inspectable redacted winnode or raw MCP output, Gateway output, UI evidence, or a runtime artifact from the stated head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Preserve the settings-change coordinator (P2) - Rebase this change through SettingsChangeCoordinator instead of restoring direct settings-impact orchestration in App. Current main marks that App responsibility closed, and this head deletes the coordinator files, losing the serialized current-main settings seam during conflict resolution.
  • Resolve merge risk (P1) - The current head is merge-conflicted and removes current-main settings-change coordination files; a rebase must preserve that serialized ownership seam.
  • Resolve merge risk (P1) - Enabling the capability changes the advertised Gateway command set and can require explicit node command-trust reapproval, but the UI does not prepare users for that recovery step.
  • Resolve merge risk (P1) - The feature relies on an experimental external Codex App Server protocol and exposes transcript content to a paired Gateway after local consent.
  • Complete next step (P2) - A maintainer must sponsor the core integration direction, and the contributor must rebase and provide inspectable real behavior proof before ordinary repair or merge automation is appropriate.
  • Improve patch quality - Rebase while preserving SettingsChangeCoordinator and add the Codex refresh as a coordinator effect.
  • Improve patch quality - Add redacted current-head winnode --list-tools plus catalog invocation output, Gateway invocation output, and visible permission/reapproval evidence.
  • Improve patch quality - Run and report the required WinNode CLI test because command documentation and MCP command surface changed.

Findings

  • [P2] Preserve the settings-change coordinator — src/OpenClaw.Tray.WinUI/App.xaml.cs:3550-3551
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 59 files affected, +8,698 / -303 lines The feature spans settings, Gateway delivery, MCP, process lifecycle, localization, docs, and tests, so it needs a fresh post-rebase review.
New command surface 3 catalog commands The commands expose session metadata and transcript pages through two transports under a new permission.

Merge-risk options

Maintainer options:

  1. Rebase through the coordinator seam (recommended)
    Preserve current-main SettingsChangeCoordinator ownership and add Codex refresh as an injected effect before resolving the merge conflict.
  2. Sponsor the protocol contract
    Accept responsibility for the experimental Codex App Server dependency and require clear reapproval and privacy behavior before merge.
  3. Pause the core integration
    Close or defer this PR if maintainers prefer the catalog to live behind an extension boundary.

Technical review

Best possible solution:

Rebase onto current main, integrate the permission refresh as a SettingsChangeCoordinator effect, clearly surface command-trust reapproval, and land only after maintainers sponsor the direct Codex App Server contract.

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

Not applicable: this PR proposes a new capability rather than reporting broken existing behavior. Its stated runtime result is not accompanied by inspectable command output or an artifact.

Is this the best way to solve the issue?

No. The proposed feature may be viable, but this branch must first preserve current-main settings ownership and receive maintainer confirmation that core should own the external Codex App Server contract.

Full review comments:

  • [P2] Preserve the settings-change coordinator — src/OpenClaw.Tray.WinUI/App.xaml.cs:3550-3551
    Rebase this change through SettingsChangeCoordinator instead of restoring direct settings-impact orchestration in App. Current main marks that App responsibility closed, and this head deletes the coordinator files, losing the serialized current-main settings seam during conflict resolution.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a substantial optional feature with privacy and upgrade implications, but it is not a current production outage.
  • add merge-risk: 🚨 compatibility: The branch conflicts with current-main settings orchestration and enabling its command set can require Gateway command-trust reapproval.
  • add merge-risk: 🚨 security-boundary: The feature adds opt-in access to local Codex session metadata and transcript content through paired Gateway and local MCP transports.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports paired-node counts, but it does not provide inspectable redacted winnode or raw MCP output, Gateway output, UI evidence, or a runtime artifact from the stated head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a substantial optional feature with privacy and upgrade implications, but it is not a current production outage.
  • merge-risk: 🚨 compatibility: The branch conflicts with current-main settings orchestration and enabling its command set can require Gateway command-trust reapproval.
  • merge-risk: 🚨 security-boundary: The feature adds opt-in access to local Codex session metadata and transcript content through paired Gateway and local MCP transports.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports paired-node counts, but it does not provide inspectable redacted winnode or raw MCP output, Gateway output, UI evidence, or a runtime artifact from the stated head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current main does not implement this feature: The current checkout has no Codex App Server command or session-access implementation outside this PR, and current main does not contain the PR head. (fc9add75eda7)
  • Direct external protocol dependency: The branch launches a local Codex App Server and uses its thread-list and thread-turns-list protocol, so compatibility with that App Server contract is a real dependency. No sibling Codex repository is implicated by this source boundary. (src/OpenClaw.Shared/Codex/CodexAppServerClient.cs:185, e29b7be53c97)
  • Current-main settings seam is removed by the branch: Current main owns saved-settings orchestration in SettingsChangeCoordinator, while the PR head lacks both coordinator files and restores direct handling in App.xaml.cs. (src/OpenClaw.Tray.WinUI/App.SettingsChangeCoordinator.cs:11, fc9add75eda7)
  • Existing architecture ownership: The architecture ledger marks SettingsChangeCoordinator as authoritative and direct OnSettingsSaved impact/reconnect orchestration as closed. (docs/ARCHITECTURE.md:157, fc9add75eda7)
  • Gateway reapproval is an existing contract: The repository’s Windows-node testing guidance requires changed-command handshakes to surface pending reapproval and require explicit approval, while the new UI says enabling this capability does not change Gateway configuration. (docs/WINDOWS_NODE_TESTING.md:60, fc9add75eda7)
  • Relevant feature history: Barbara Kudiess introduced the recent permission/settings MVVM ownership refactor, which is the current-main seam this branch must preserve. (src/OpenClaw.Tray.WinUI/Presentation/SettingsStore.cs:36, f4e1cf3a9894)

Likely related people:

  • Barbara Kudiess: Introduced the current-main permissions MVVM refactor and settings-store seam that this PR intersects. (role: recent permission and settings ownership contributor; confidence: high; commits: f4e1cf3a9894; files: src/OpenClaw.Tray.WinUI/Presentation/SettingsStore.cs, src/OpenClaw.Tray.WinUI/Pages/SettingsPage.xaml, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • Scott Hanselman: Recent history includes Windows-node transport and capability work adjacent to the changed Gateway delivery path. (role: recent Windows-node contributor; confidence: medium; commits: 4206611f2b2e, a51029ef6f77; files: src/OpenClaw.Shared/WindowsNodeClient.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

1 participant