Skills: make coding-agent background-first with direct completion notifications#53585
Skills: make coding-agent background-first with direct completion notifications#53585fabianwilliams merged 1 commit intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea2e784566
ℹ️ 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".
Greptile SummaryThis PR refactors Key observations:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: skills/coding-agent/SKILL.md
Line: 4-7
Comment:
**`install` metadata entries silently removed**
The PR drops the `install` block from the skill metadata:
```json
"install": [
{ "id": "node-claude", "kind": "node", "package": "@anthropic-ai/claude-code", "bins": ["claude"], "label": "Install Claude Code CLI (npm)" },
{ "id": "node-codex", "kind": "node", "package": "@openai/codex", "bins": ["codex"], "label": "Install Codex CLI (npm)" }
]
```
If OpenClaw uses these entries to surface an in-app "install missing tool" flow, removing them means users who don't have `claude` or `codex` installed will no longer be offered an auto-install path when this skill is invoked — they'll just hit the `anyBins` gate silently.
The PR description frames the change as "only updating built-in skill instructions and examples" and does not call this out as an intentional behaviour change. If it is intentional, please confirm it in the PR description and verify that the app handles a missing `install` section gracefully for this skill. If it's unintentional, the block should be restored.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/coding-agent/SKILL.md
Line: 32-34
Comment:
**"Wrong" example omits `background:true`, which is also wrong now**
The example currently flags only two problems with this command: `pty:true` and `--dangerously-skip-permissions`. But per the new mandatory rule ("Always background immediately — use `background:true` for every coding-agent launch"), the missing `background:true` is a third defect in this example that goes unmentioned. A reader could copy the correct flags and add `background:true` to this wrong command and think they've fixed it, but they'd still be using `--dangerously-skip-permissions` instead of `--permission-mode bypassPermissions --print`.
Consider adding a brief inline comment to the wrong example listing all three issues, or restructuring it so the contrast with the correct example is unambiguous:
```suggestion
# Wrong for Claude Code (PTY, wrong flags, no background)
bash pty:true command:"claude --dangerously-skip-permissions 'task'"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/coding-agent/SKILL.md
Line: 182-207
Comment:
**Examples don't demonstrate the Mandatory Pattern's notify-route injection step**
The "Mandatory Pattern" section (step 3) states: *"Include the notification route in the worker prompt and require the worker to call `openclaw message send` on completion."* However, the simplified examples in the PR review, batch PR review, Claude Code, OpenCode, and Pi sections all launch workers without injecting any notify-route variables into the worker prompt, e.g.:
```bash
bash pty:true workdir:$REVIEW_DIR background:true command:"codex review --base origin/main"
```
An agent following only the examples (rather than reading the Mandatory Pattern section carefully) would spawn workers with no completion notification path.
The parallel worktree example does reference the notify route (`"...Send the completion message with openclaw message send using the provided notify route.'"`) but without showing how to inject the actual route values. Aligning at least the Quick Start or one end-to-end example with the Completion Prompt Snippet template would reduce the risk of agents silently skipping the notification step.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs: enforce background mode & direct c..." | Re-trigger Greptile |
77ff66a to
20fd56f
Compare
fabianwilliams
left a comment
There was a problem hiding this comment.
Good simplification — making background:true the only path removes ambiguity for skill authors.
A few observations:
-
Completion via
openclaw message sendinstead of system events is a cleaner contract. We've seen in our own cron deployments that relying on delivery:announce + agent-initiated messaging causes duplicates. Having one explicit path (the worker callsopenclaw message send) is the right pattern. -
The notification-route capture before spawn is smart — ensures the agent knows WHERE to send results before it starts working. We've hit issues where background jobs complete but the operator never gets notified because the delivery target wasn't set up.
-
Scope boundary is well-drawn — SKILL.md only, no runtime changes. This makes it safe to land and iterate.
One question: the description mentions "openclaw message send" for completion — is that a new CLI command, or does it map to an existing tool? If it's new, should there be a note about minimum OpenClaw version required?
Otherwise LGTM for the skill instruction changes.
|
Rebased onto current upstream/main (resolved 2 conflicts in skills/coding-agent/SKILL.md):
Also added Rule #10 to address @greptile-apps's note that the simplified examples don't visibly inject the notify-route block:
This makes the implicit "reuse the injection block" instruction at the top of Quick Start an explicit rule, so an agent reading mechanically can't miss it. Force-pushed (with-lease) to your branch — Per @pSteiny's directive: fix-and-merge velocity. |
|
Good conflict resolution — keeping the One issue in the current file though: there are now two separate "Rules" sections that partially overlap and disagree:
The new canonical section at the bottom is missing the injection requirement entirely. An agent reading it top-to-bottom will hit Also a minor issue: Rule 10 in Two options: Option A (simpler): Add injection as an explicit rule in Option B (cleaner): Consolidate by removing the old Either way, the canonical Tip: You can customize Greptile's behavior for this repo with |
fabianwilliams
left a comment
There was a problem hiding this comment.
Rebased + Rule #10 added to address @greptile-apps's notify-route concern. Docs-only change, relevant CI green. Approving per @pSteiny's fix-and-merge directive.
Summary
coding-agentskill mixed foreground and background execution patterns, and its completion guidance depended on system-event / heartbeat delivery.user.
skills/coding-agent/SKILL.mdto use immediatebackground:truelaunches, require explicit notification-route capture before spawn, and send completion/failure notifications withopenclaw message send.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
N/A
git blame, prior PR, issue, or refactor if known): N/ARegression Test Plan (if applicable)
N/A
User-visible / Behavior Changes
coding-agentskill now documents a background-first workflow.openclaw message send.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
<your OS>Steps
skills/coding-agent/SKILL.md.openclaw message send.Expected
Actual
skills/coding-agent/SKILL.mdnow follows that workflow.Evidence
Attach at least one:
Suggested attachment:
skills/coding-agent/SKILL.mdHuman Verification (required)
openclaw message sendcompletionguidance.
--print --permission-mode bypassPermissions, and kept scratchgit initguidance forCodex.
Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
skills/coding-agent/SKILL.mdRisks and Mitigations