Skip to content

Fix skill config requirements, spawn allowlists, chat stop lifecycle, and systemd status#79538

Closed
HemantSudarshan wants to merge 1 commit into
openclaw:mainfrom
HemantSudarshan:hemant/fix-skills-spawn-stop-status
Closed

Fix skill config requirements, spawn allowlists, chat stop lifecycle, and systemd status#79538
HemantSudarshan wants to merge 1 commit into
openclaw:mainfrom
HemantSudarshan:hemant/fix-skills-spawn-stop-status

Conversation

@HemantSudarshan

@HemantSudarshan HemantSudarshan commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Real behavior proof

  • Behavior or issue addressed: Four user-facing regressions: skill config requirements rendered object metadata as [object Object], subagent spawn env allowlists were ignored, WebChat run state could stay active after backend error/context-overflow completion, and openclaw status could report a systemd Gateway service failed even when systemctl is-active reported active.
  • Real environment tested: Local Windows checkout at 705723b9d0 after rebasing onto upstream/main; Node v22.15.0 for repo test wrappers; WSL Ubuntu has systemd installed but no user bus and no Linux Node, so it could not provide honest live user-systemd proof on this host.
  • Exact steps or command run after this patch: Ran the touched skill/subagent/chat/frontmatter tests in one grouped repo invocation, ran the focused systemd stale-state regression, ran production dependency audit, and ran the changed gate from the rebased branch.
  • Evidence after fix: Copied terminal output summary from the final local run:
OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/agents/skills.buildworkspaceskillstatus.test.ts src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts src/agents/subagent-target-policy.test.ts src/agents/tools/agents-list-tool.test.ts src/shared/frontmatter.test.ts src/gateway/server-methods/chat.directive-tags.test.ts -- --reporter=verbose
[test] passed 3 Vitest shards in 84.38s
unit-fast: 17 passed
agents: 30 passed
gateway: 74 passed

OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/daemon/systemd.test.ts -t "prefers is-active" -- --reporter=verbose
✓ systemd runtime parsing > prefers is-active when show reports a stale failed state
Test Files 1 passed (1); Tests 1 passed | 60 skipped (61)

node scripts/pre-commit/pnpm-audit-prod.mjs --audit-level=high
No high or higher advisories found for production dependencies.
  • Observed result after fix: Object-shaped skill requirements normalize correctly, SPAWN_ALLOWLIST fallback allowlists are respected, chat final/error lifecycle maps the backend run id to the client run, and the systemd parser prefers active is-active output over stale failed show state. The dependency-audit failure from the old branch base is gone after rebase.
  • What was not tested: A live Linux user-systemd openclaw status run could not be executed on this Windows host because WSL reports systemctl --user bus unavailable and has no Linux Node/pnpm. The full src/daemon/systemd.test.ts file also has unrelated Windows-only failures around process.geteuid and quoted Windows EnvironmentFile paths; the new stale-state regression passed in isolation.

Verification

  • pnpm docs:list
  • pnpm exec oxfmt --check --threads=1 CHANGELOG.md docs/gateway/config-tools.md docs/tools/creating-skills.md docs/tools/skills.md docs/tools/subagents.md src/agents/acp-spawn.ts src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts src/agents/skills.buildworkspaceskillstatus.test.ts src/agents/subagent-spawn.ts src/agents/subagent-target-policy.test.ts src/agents/subagent-target-policy.ts src/agents/tools/agents-list-tool.test.ts src/agents/tools/agents-list-tool.ts src/daemon/systemd.test.ts src/daemon/systemd.ts src/gateway/server-methods/chat.directive-tags.test.ts src/gateway/server-methods/chat.ts src/shared/frontmatter.test.ts src/shared/frontmatter.ts
  • OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/agents/skills.buildworkspaceskillstatus.test.ts src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts src/agents/subagent-target-policy.test.ts src/agents/tools/agents-list-tool.test.ts src/shared/frontmatter.test.ts src/gateway/server-methods/chat.directive-tags.test.ts -- --reporter=verbose
  • OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test src/daemon/systemd.test.ts -t "prefers is-active" -- --reporter=verbose
  • node scripts/pre-commit/pnpm-audit-prod.mjs --audit-level=high
  • pnpm check:changed --base upstream/main
  • git diff --check upstream/main..HEAD

Note: this shell's default Node is v22.15.0, so pnpm emitted the repo engine warning (>=22.16.0 required); the commands above completed with that warning.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime agents Agent runtime and tooling size: M triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 8, 2026
@clawsweeper

clawsweeper Bot commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 11:46 PM ET / 03:46 UTC.

Summary
The PR changes skill requirement parsing, subagent spawn allowlist fallback behavior, WebChat run-id lifecycle mapping, systemd status parsing, and related docs/tests/changelog text.

PR surface: Source +79, Tests +158, Docs +7. Total +244 across 19 files.

Reproducibility: yes. at source level: current main still has the skill object-normalization gap, missing spawn allowlist env reader, and systemd ActiveState-only status path. I did not run live Docker/WebChat/Linux systemd reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Spawn Env Allowlist Surface: 2 env aliases added, 3 runtime call sites wired. These env names affect sessions_spawn target authorization, so precedence and registry filtering need maintainer attention before merge.

Stored data model
Persistent data-model change detected: serialized state: src/agents/subagent-target-policy.ts, vector/embedding metadata: CHANGELOG.md, vector/embedding metadata: docs/tools/creating-skills.md, vector/embedding metadata: docs/tools/skills.md, vector/embedding metadata: src/agents/skills.buildworkspaceskillstatus.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted real behavior proof for Docker/Coolify spawning, WebChat recovery, and Linux user-systemd status.
  • Rebase or split the branch while preserving configured-agent filtering and moving env fallback to config loading.
  • Remove release-owned changelog edits or move that context into the PR body/squash message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body contains test/check terminal output, but no redacted real Docker/Coolify spawn, WebChat recovery, or Linux user-systemd status proof; updating the PR body with redacted proof should trigger re-review. 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.

Mantis proof suggestion
A real WebChat visual proof would materially show whether backend error completion clears the active run and leaves /new unqueued. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify WebChat context-overflow or backend error completion clears the active run and /new is not queued.

Risk before merge

  • [P1] The branch is currently conflicting, so maintainers need a rebase or split before the actual merge result can be trusted.
  • [P2] The spawn env fallback changes a sessions_spawn authorization boundary and must preserve explicit config precedence plus current configured-agent registry filtering.
  • [P1] The WebChat run-id aliasing change modifies active-run/session state and needs paired cleanup proof for both client and backend run ids.
  • [P1] The PR body supplies focused test/check output, but not redacted real Docker/Coolify spawn, WebChat recovery, or Linux user-systemd status proof.

Maintainer options:

  1. Split And Rebase The Branch (recommended)
    Rebase on current main, preserve configured-agent filtering, and split unrelated fixes if the combined branch remains too hard to validate.
  2. Let The Focused Spawn PR Own Env Fallback
    Use fix(config): apply SPAWN_ALLOWLIST env for sessions_spawn (#79490) #79913 for the SPAWN_ALLOWLIST slice, then review this PR only for skill, systemd, and any still-unique WebChat changes.
  3. Replace With Narrow PRs
    If the conflict and proof gaps stay costly, preserve contributor credit while landing smaller replacement PRs for the open linked issues.

Next step before merge

  • [P1] Maintainers need to choose whether to split/rebase this broad conflicting branch or route the spawn slice through the focused sibling PR before any automated repair is safe.

Security
Needs attention: The diff changes sessions_spawn authorization and should not land until env fallback handling preserves canonical config semantics and configured-agent filtering.

Review findings

  • [P1] Preserve configured-agent filtering — src/agents/subagent-spawn.ts:808-815
  • [P1] Move env fallback into config loading — src/agents/subagent-target-policy.ts:42-45
  • [P2] Clean up both chat run aliases — src/gateway/server-methods/chat.ts:2508-2515
Review details

Best possible solution:

Split or rebase onto current main, keep the still-useful skill/systemd fixes, route spawn env handling through a fallback-only canonical config-load path, preserve configured-agent filtering, remove release-owned changelog edits, and add redacted real behavior proof.

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

Yes at source level: current main still has the skill object-normalization gap, missing spawn allowlist env reader, and systemd ActiveState-only status path. I did not run live Docker/WebChat/Linux systemd reproduction in this read-only review.

Is this the best way to solve the issue?

No as submitted. The useful fixes need a current-main rebase or split, canonical config-load handling for the env fallback, preserved configured-agent filtering, paired chat alias cleanup, and real behavior proof.

Full review comments:

  • [P1] Preserve configured-agent filtering — src/agents/subagent-spawn.ts:808-815
    Current main passes configured target ids into the spawn policy so allowAgents: ["*"] cannot launch arbitrary unconfigured agent ids. Rebase this env fallback so SPAWN_ALLOWLIST="*" still respects that registry boundary instead of reopening rogue agent-root creation.
    Confidence: 0.91
  • [P1] Move env fallback into config loading — src/agents/subagent-target-policy.ts:42-45
    Reading process.env from spawn and listing runtime paths creates a second authorization source outside config validation, doctor, and reload semantics. Apply the fallback at the loaded-config boundary, and only when explicit config is absent.
    Confidence: 0.87
  • [P2] Clean up both chat run aliases — src/gateway/server-methods/chat.ts:2508-2515
    The PR registers the backend run id as an alias for the client run, but the changed test only proves registration. Terminal, error, and abort cleanup should remove both registry keys so a backend-run alias cannot survive after the client run finishes.
    Confidence: 0.82
  • [P3] Remove release-owned changelog edits — CHANGELOG.md:170-173
    CHANGELOG.md is release-generated in this repository. Keep this release-note context in the PR body or squash message instead of editing the changelog directly.
    Confidence: 0.78

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 9d68f877ac3e.

Label changes

Label justifications:

  • P1: The PR targets real agent/WebChat regressions and touches a security-sensitive subagent authorization boundary.
  • merge-risk: 🚨 compatibility: The spawn env fallback can change deployment configuration behavior and must preserve canonical config loading and upgrade semantics.
  • merge-risk: 🚨 session-state: The WebChat change modifies active chat-run id aliasing and terminal cleanup behavior.
  • merge-risk: 🚨 security-boundary: The spawn allowlist controls which configured agents can be launched through the privileged sessions_spawn tool.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • 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 contains test/check terminal output, but no redacted real Docker/Coolify spawn, WebChat recovery, or Linux user-systemd status proof; updating the PR body with redacted proof should trigger re-review. 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

PR surface:

Source +79, Tests +158, Docs +7. Total +244 across 19 files.

View PR surface stats
Area Files Added Removed Net
Source 7 94 15 +79
Tests 7 160 2 +158
Docs 5 17 10 +7
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 19 271 27 +244

Security concerns:

  • [high] Runtime env fallback changes spawn authorization — src/agents/subagent-target-policy.ts:42
    Reading allowlist env vars directly in spawn policy/call sites creates an authorization source outside canonical config validation and reload behavior.
    Confidence: 0.87
  • [high] Wildcard env must preserve registry filtering — src/agents/subagent-spawn.ts:808
    The submitted branch predates current main's configuredAgentIds filtering, so the rebase must preserve that filter before wildcard SPAWN_ALLOWLIST can be safe.
    Confidence: 0.91

What I checked:

  • Repository policy read and applied: Root AGENTS.md and scoped agents, gateway, server-methods, and docs AGENTS.md files were read; config/env fallback, session-state, security-boundary, and release-owned changelog guidance affected the review. (AGENTS.md:20, 9d68f877ac3e)
  • Live PR state: GitHub reports this PR open, non-draft, head 705723b, and mergeable=CONFLICTING / mergeStateStatus=DIRTY. (705723b9d0e1)
  • Current main subagent hardening: Current main intersects wildcard and explicit subagent allowlists with configured agent ids; any env fallback must preserve this authorization boundary after rebase. (src/agents/subagent-target-policy.ts:51, 9d68f877ac3e)
  • PR env fallback location: The PR parses OPENCLAW_SPAWN_ALLOWLIST/SPAWN_ALLOWLIST inside the target-policy helper and wires process.env through runtime spawn/list call sites instead of canonical config loading. (src/agents/subagent-target-policy.ts:42, 705723b9d0e1)
  • Current main skill parser gap: Current main still normalizes requires.config with the generic string-list normalizer, so object entries are not read through their path field. (src/shared/frontmatter.ts:86, 9d68f877ac3e)
  • Current main systemd gap: Current main maps systemctl show ActiveState directly to running/stopped/unknown and does not cross-check systemctl is-active in readSystemdServiceRuntime. (src/daemon/systemd.ts:1334, 9d68f877ac3e)

Likely related people:

  • joshavant: Recent history shows Josh Avant authored the merged wildcard/configured-agent hardening that this PR's spawn fallback must preserve. (role: recent authorization-path contributor; confidence: high; commits: 00da318350e2, 5a3a448bc48d; files: src/agents/subagent-target-policy.ts, src/agents/subagent-spawn.ts, src/agents/acp-spawn.ts)
  • steipete: History shows Peter Steinberger introduced shared OpenClaw manifest parsing and the default subagent allowlist config surface touched by the PR. (role: shared parser and config-surface contributor; confidence: high; commits: ae1880acf6be, d92178471894; files: src/shared/frontmatter.ts, src/config/types.agent-defaults.ts, docs/tools/subagents.md)
  • vincentkoc: Recent merged work on systemd status and root-scope handling overlaps the daemon status path this PR changes. (role: recent systemd/status contributor; confidence: medium; commits: 64785823d093, 556a74d25914; files: src/daemon/systemd.ts, src/daemon/systemd.test.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.

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 8, 2026
@HemantSudarshan
HemantSudarshan force-pushed the hemant/fix-skills-spawn-stop-status branch from 054dbe6 to 705723b Compare May 9, 2026 17:24
This was referenced May 9, 2026
Open
Closed
Open
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jun 1, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 1, 2026
@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 1, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jun 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 14, 2026
Open
@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. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. 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. labels Jun 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 29, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution. I’m closing this branch because it remains dirty against current main, the spawn-authorization and WebChat lifecycle changes are not safe as submitted, and the requested real-behavior proof has not arrived. The underlying gaps can be revisited in focused PRs against current main.

@steipete steipete closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling app: web-ui App: web-ui docs Improvements or additions to documentation gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. 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

Milestone

No milestone

2 participants

@HemantSudarshan @steipete