Skip to content

refactor(process): remove unused spawn fallback controls - #139326

Merged
steipete merged 1 commit into
mainfrom
steipete/cleanup-agents-process-neighborhood
Sep 5, 2026
Merged

refactor(process): remove unused spawn fallback controls#139326
steipete merged 1 commit into
mainfrom
steipete/cleanup-agents-process-neighborhood

Conversation

@steipete

@steipete steipete commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related: #135868

What Problem This Solves

Maintainer-requested cleanup companion to stage 3 of the #135868 split (decision brief concern C: process/agent authority and cleanup certification). The spawn fallback helper carries unused policy overrides, callbacks, and labels, while its tests still build process fixtures for a readiness implementation already removed on main.

Why This Change Was Made

Remove the private retryCodes, onFallback, and fallbackLabel surfaces and pass fallback spawn options directly. All three production calls were inspected: the child adapter uses only the child and usedFallback, and both Scheduled Task calls use only the child. None supplies a retry override or callback. There is no plugin SDK export of this helper.

Keep usedFallback because it prevents process-group signaling after a non-detached retry. Preserve the authority assertion outside retry handling, Node spawn readiness, error normalization, and fallback order. Remove the duplicate weaker revocation test and obsolete stream/PID/kill fixture setup; retain the EBADF-coded authority regression and real ENOENT coverage. No automatic-recovery or cleanup-certification changes are imported from #135868.

User Impact

No intended user-visible behavior change. The private process helper and its tests have less unused surface for subsequent ownership changes to maintain.

Evidence

Production: +7/-35, net -28 lines. Tests/support: +10/-64, net -54. Tooling/docs: unchanged.

  • node scripts/run-vitest.mjs src/process src/daemon/schtasks.startup-fallback.test.ts: 639 passed, 62 existing platform-specific skips across three shards. Native Windows integration is not claimed from this macOS run.
  • pnpm check:import-cycles: passed, zero runtime cycles.
  • Production, full-tree, and script dead-export scans: all passed with zero entries.
  • Full node scripts/check-changed.mjs: passed (exit 0), including all selected types, lint, dead-export scans, and repository guards.
  • Independent Codex branch autoreview: scoped-clean, no actionable P0–P2 findings.

The branch was rebased onto current main after local validation; none of the five owned files changed in that main drift. The patch remained identical. Exact-head hosted CI passed on b879c0e76f7c2ec23bc4b7cd863336fc299c1076: run 33988858000. The native review/prepare/merge flow landed this as befc0c24a243; remote merge state, main ancestry, and all five landed file contents were independently verified.

ClawSweeper review found no correctness or security findings and no pre-merge actions. No rank-up changes were requested. The review independently confirmed the three production callers and the retained authority and detachment coverage.

@steipete steipete self-assigned this Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Sep 5, 2026
@steipete
steipete marked this pull request as ready for review September 5, 2026 19:42
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 4:06 PM ET / 20:06 UTC.

ClawSweeper review

What this changes

Simplifies the internal process-launch helper by removing unused retry controls and labels, updating its caller, and trimming redundant test fixtures.

Merge readiness

✅ Ready for maintainer review

Keep open: this remains a useful, maintainer-requested cleanup absent from the inspected main and release sources. No actionable correctness or security findings were identified.

Priority: P3
Reviewed head: b879c0e76f7c2ec23bc4b7cd863336fc299c1076

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving cleanup with retained regression coverage and no identified blockers.
Proof confidence 🌊 off-meta tidepool Not applicable: The supplied MEMBER association exempts this internal cleanup from external-contributor proof requirements. It changes no authority boundary; reported process and Scheduled Task checks supplement source inspection without claiming native Windows proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The supplied MEMBER association exempts this internal cleanup from external-contributor proof requirements. It changes no authority boundary; reported process and Scheduled Task checks supplement source inspection without claiming native Windows proof.
Evidence reviewed 8 items Introduced patch verified: The merge-base-to-head diff matches the supplied introduction evidence: only the helper, child adapter, and corresponding tests/support change. The unrelated endpoint drift is excluded from this review.
Runtime invariants retained: The helper still checks caller authority outside retry handling before each spawn, waits for Node's spawn event, retries only EBADF, and reports usedFallback. The child adapter still derives actual detachment from that result before signaling the process tree.
Caller and API boundary: Inspection found three production calls: one child-adapter call and two Scheduled Task calls. The latter consume only child and supply no fallback controls; the adapter consumes child and usedFallback. Searches found no plugin SDK export or Codex dependency signal for this helper.
Findings None None.
Security None None.

How this fits together

The process-launch helper starts subprocesses for the supervisor and Windows Scheduled Task fallback. It returns the child process and whether fallback options were used, which the supervisor uses to choose safe termination behavior.

flowchart TD
  A[Supervisor command] --> C[Process launch helper]
  B[Windows task fallback] --> C
  C --> D[Check caller authority]
  D --> E[Spawn process]
  E -->|EBADF with fallback available| D
  E -->|Started| F[Child and fallback result]
  F --> G[Supervision and safe termination]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -28, Tests -54. Total -82 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 2 7 35 -28
Tests 3 10 64 -54
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 17 99 -82

Review metrics

None.

Technical review

Best possible solution:

Keep one small internal spawn helper with fixed EBADF retry behavior, direct fallback options, and explicit detachment reporting.

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

Not applicable: this PR removes unused internal surface rather than reporting a runtime defect; caller inspection supports that premise.

Is this the best way to solve the issue?

Yes: simplifying the existing helper and migrating its sole fallback-supplying caller is narrower than retaining unused compatibility controls or introducing another abstraction.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against df804fc6ff43.

Labels

Label justifications:

  • P3: This is bounded internal cleanup with no intended user-visible behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The supplied MEMBER association exempts this internal cleanup from external-contributor proof requirements. It changes no authority boundary; reported process and Scheduled Task checks supplement source inspection without claiming native Windows proof.

Evidence

What I checked:

  • Introduced patch verified: The merge-base-to-head diff matches the supplied introduction evidence: only the helper, child adapter, and corresponding tests/support change. The unrelated endpoint drift is excluded from this review. (src/process/spawn-utils.ts:12, b879c0e76f7c)
  • Runtime invariants retained: The helper still checks caller authority outside retry handling before each spawn, waits for Node's spawn event, retries only EBADF, and reports usedFallback. The child adapter still derives actual detachment from that result before signaling the process tree. (src/process/supervisor/adapters/child.ts:406, b879c0e76f7c)
  • Caller and API boundary: Inspection found three production calls: one child-adapter call and two Scheduled Task calls. The latter consume only child and supply no fallback controls; the adapter consumes child and usedFallback. Searches found no plugin SDK export or Codex dependency signal for this helper. (src/daemon/schtasks-runtime.ts:132, b879c0e76f7c)
  • Stronger regression coverage remains: The retained revocation test uses an EBADF-coded authority error and verifies that no second process starts. Real missing-executable coverage remains, as do SIGTERM and SIGKILL assertions preventing group signaling after a non-detached fallback. The removed stream/PID fixtures are unnecessary for spawn-event readiness. (src/process/spawn-utils.test.ts:60, b879c0e76f7c)
  • Cleanup still absent from main: Fetched main retains retryCodes, onFallback, fallbackLabel, and the wrapped fallback options. Comparing the relevant production files against the pinned merge base returned no changes. (src/process/spawn-utils.ts:19, df804fc6ff43)
  • Latest release check: The inspected v2026.9.2 helper also retains the removed controls; this cleanup is not already present in that release. (src/process/spawn-utils.ts:19, 3928bad9badf)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-05T19:47:34.062Z sha f07a51f3cbb5e2b62e19550b323390f392867ded :: needs maintainer review before merge. :: none
  • reviewed 2026-09-05T19:55:19.017Z sha f07a51f3cbb5e2b62e19550b323390f392867ded :: needs maintainer review before merge. :: none

@steipete
steipete force-pushed the steipete/cleanup-agents-process-neighborhood branch from f07a51f to b879c0e Compare September 5, 2026 20:01
@steipete
steipete merged commit befc0c2 into main Sep 5, 2026
176 checks passed
@steipete
steipete deleted the steipete/cleanup-agents-process-neighborhood branch September 5, 2026 20:26
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Open
@steipete steipete mentioned this pull request Sep 6, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
refactor(process): remove unused spawn fallback controls (openclaw#13…
7e7b3f1 
…9326)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete