fix(skills): make Skill Workshop lifecycle approvals decidable and non-wedging#100498
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab67deca7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| requireApproval: { | ||
| ...text, | ||
| description: approvalDescription.description, |
There was a problem hiding this comment.
Bind approval to the described proposal revision
When a proposal is revised after this metadata is loaded but before the operator clicks allow, the approval still authorizes the original proposal_id only; the later applySkillProposal path re-reads and applies the current proposal content (src/skills/workshop/service.ts:504), while reviseSkillProposal can change the same proposal in place (src/skills/workshop/service.ts:381). In a parallel tool call or another session during the 70s approval window, the operator can approve the displayed description/body size for revision N but apply revision N+1. Include the draft hash/proposed version in the approved params and reject if it changed before executing the lifecycle action.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 10:06 PM ET / 02:06 UTC. Summary PR surface: Source +118, Tests +216, Docs +8. Total +342 across 17 files. Reproducibility: yes. at source level: current main has a 120s generic plugin approval wait against a 90s Codex dynamic-tool watchdog, and the PR-introduced stale-metadata path follows from revising a pending proposal before apply re-reads it by id. I did not run a live Gateway/Codex approval session in this read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land the timeout and metadata fix after binding lifecycle approval to the displayed proposal revision, aligning all approval-description caps and docs, and adding a redacted live approval-flow proof. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main has a 120s generic plugin approval wait against a 90s Codex dynamic-tool watchdog, and the PR-introduced stale-metadata path follows from revising a pending proposal before apply re-reads it by id. I did not run a live Gateway/Codex approval session in this read-only review. Is this the best way to solve the issue? No, not yet: the 70s bounded wait and richer approval metadata are the right layer, but the lifecycle action must be tied to the displayed proposal revision and the approval description cap must be consistent across adapters and docs. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d095f4755339. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +118, Tests +216, Docs +8. Total +342 across 17 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
…n-wedging (#91266, #94249, #93173) Approval wait now fits inside the Codex dynamic-tool watchdog (70s +10s gateway grace under the 90s kill), approval cards carry proposal id, skill name, description, file count, and body size (spoof-safe rendering), and timeouts return a structured pending-not-failed outcome instead of a bare error. Expired requests cannot execute late; no auto-apply; generic plugin approvals unchanged.
1ab67de to
e3bd9da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3bd9da226
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const MAX_PLUGIN_APPROVAL_TIMEOUT_MS = 600_000; | ||
| export const PLUGIN_APPROVAL_TITLE_MAX_LENGTH = 80; | ||
| export const PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH = 256; | ||
| export const PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH = 512; |
There was a problem hiding this comment.
Use the shared 512-char cap for node approvals
This raises the plugin approval description contract to 512 characters, but the node-invoke policy approval adapter still does input.description.slice(0, 256) in src/gateway/node-invoke-plugin-policy.ts:67 before storing and broadcasting the same PluginApprovalRequestPayload. For dangerous node commands whose policies use the newly allowed extra room to explain the target/risk, reviewers on that path will silently lose the second half of the approval context while other plugin approval paths preserve it; update the adapter to use the shared limit instead of the old hard-coded cap.
Useful? React with 👍 / 👎.
…n-wedging (openclaw#91266, openclaw#94249, openclaw#93173) (openclaw#100498) Approval wait now fits inside the Codex dynamic-tool watchdog (70s +10s gateway grace under the 90s kill), approval cards carry proposal id, skill name, description, file count, and body size (spoof-safe rendering), and timeouts return a structured pending-not-failed outcome instead of a bare error. Expired requests cannot execute late; no auto-apply; generic plugin approvals unchanged.
…n-wedging (openclaw#91266, openclaw#94249, openclaw#93173) (openclaw#100498) Approval wait now fits inside the Codex dynamic-tool watchdog (70s +10s gateway grace under the 90s kill), approval cards carry proposal id, skill name, description, file count, and body size (spoof-safe rendering), and timeouts return a structured pending-not-failed outcome instead of a bare error. Expired requests cannot execute late; no auto-apply; generic plugin approvals unchanged.
What Problem This Solves
Agent-initiated Skill Workshop lifecycle actions were effectively broken under the default approval policy, and the approval popup gave operators nothing to decide on:
skill_workshop(action="apply")always timed out: the approval wait defaulted to 120s while the Codex dynamic-tool watchdog kills the call at 90s, so the tool call died before any human could decide ([Bug]: Skill Workshop agent-tool apply times out because approval wait exceeds tool watchdog #91266, consistently reproduced on large proposals in skill_workshop apply times out ("Approval timed out") on large proposals (~28KB) #94249).openclaw skills workshop inspectjust to know what they are approving (Skill Workshop apply popup carries no proposal details + fixed 60s TTL #93173)."Approval timed out"failure, reading like an error and inviting retry loops even though the proposal was safely pending.Closes #91266. Closes #94249. Closes #93173.
Why This Change Was Made
Approval wait bounded inside the tool budget (
src/skills/workshop/policy.ts): workshop lifecycle approvals now settimeoutMs: 70_000. The Codex dynamic-tool watchdog is 90s (extensions/codex/src/app-server/dynamic-tool-execution.ts:23); the gateway approval RPC reserves +10s cleanup grace, leaving 10s headroom — the tool call now always returns before the watchdog fires. A static bound is required because the upstream Codex dynamic-tool contract carries no deadline: verified directly incodex-rs/app-server-protocol/src/protocol/v2/item.rs(DynamicToolCallParams= thread/turn/call ids + tool + arguments only) andcodex-rs/app-server/src/dynamic_tools.rs(no deadline plumbing) at codexbe33f80bc6.Approval requests carry proposal metadata: the policy resolver loads the pending proposal (by
proposal_id, ornamevia the same resolution the tool uses) and builds the description with proposal id, target skill name, the 160-byte proposal description, support-file count, and body size in KB. Control/format characters are rendered inert and long names bounded, so proposal-controlled text cannot spoof the approval card. Unresolvable proposals fall back to today's static text. This enriches every approval surface (Control UI popup, chat approval card) with zero UI-component changes.PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTHraised 256 → 512 (validation loosening, additive for protocol consumers) to fit the metadata; workspace scope is threaded through gateway invocation, the Codex approval bridge, dynamic tools, and the native-hook relay so the resolver can read the right store.Honest, non-wedging timeout outcome: a new optional
timeoutReasonon therequireApprovalshape (used only by the workshop policy) turns approval timeout into a structured blocked (veto) tool result instead of a failure: the request expired without a decision, the proposal is unchanged and still pending, decide via the Skill Workshop UI oropenclaw skills workshop apply|reject|quarantine <id>, do not retry in a loop. Generic plugin approval timeouts are untouched. There is deliberately no auto-apply on timeout, and expired requests cannot execute later: the gateway approval manager resolves expired requests to null and rejects late decisions (src/gateway/exec-approval-manager.ts), and the embedded broker deletes pending requests before resolving timeout (src/infra/embedded-plugin-approval-broker.ts).User Impact
Evidence
check:changed: core, core tests, extensions, extension tests, docs — run 28758816731.codexcheckout atbe33f80bc6(files cited above).src/agents/harness/native-hook-relay.ts:722) — follow-up territory.fix— Skill Workshop approval prompts now show proposal details, fit inside the tool watchdog, and expire into an actionable pending state instead of an error.