Skip to content

fix: share bundled SkillSpector capacity across two roots - #4152

Open
Patrick-Erichsen wants to merge 1 commit into
mainfrom
parallelize-bundled-scans
Open

Patrick-Erichsen wants to merge 1 commit into
mainfrom
parallelize-bundled-scans

Conversation

@Patrick-Erichsen

Copy link
Copy Markdown
Collaborator

Large bundled plugins can exhaust the 20-minute SkillSpector stage while scanning roots serially: the protected Solaris replay completed only 52 of 62 roots before its deadline. Run up to two OpenAI roots together, with five process-wide model calls each, preserving the existing combined ceiling of ten and all child/stage deadlines. Other providers retain serial execution.

Keep aggregate ordering canonical and require every root before judge handoff. On failure, stop admission, cancel siblings and join process-group cleanup. Distinguish command failure kinds so exit-1 findings cannot conceal cancellation, timeout, output-limit, observer or cleanup failure; accepted findings require confirmed process-group absence.

Validation: 171 worker/shared checks; 8,391 full application tests with five configured skips and unchanged coverage thresholds; static, full TypeScript/build and package types; complete independent P0–P3 review with zero findings. The first overlapping broad/full run hit two subprocess startup timing assertions; unchanged sequential runs pass without timeout/assertion changes.

Paired controls use the real coordinator with 62 synthetic roots and a fixed-latency fake scanner: 5.34s serial → 3.34s candidate, identical aggregate SHA 5899cd25294d7ec3bdf1856a886ebb507dead862f5a3d985d53b67e70f451c25, all 62 findings and the existing 25-entry storage cap preserved. Peak children 1→2, combined configured model-call ceiling remains10. An installed scanner with mocked HTTP independently honors process caps1 and5. These controls make no real model calls and prove no production package recovery. Actual protected Solaris replay and normal package publication/download/install proof remain required; #4043 stays open.

@vercel

vercel Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
clawhub Ready Ready Preview Oct 30, 2026 3:29pm UTC

Request Review

@clawsweeper

clawsweeper Bot commented Oct 30, 2026 •

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.

@Patrick-Erichsen

Copy link
Copy Markdown
Collaborator Author

Paired coordinator proof (synthetic roots, no real models/network/publisher execution):

{
  "before": {
    "elapsedMs": 5336.251834,
    "completedRoots": 62,
    "totalFindings": 62,
    "retainedFindings": 25,
    "peakChildren": 1,
    "peakSumOfConfiguredModelLimits": 10,
    "perChildLimits": [
      10
    ],
    "aggregateSha256": "5899cd25294d7ec3bdf1856a886ebb507dead862f5a3d985d53b67e70f451c25",
    "modelsInvoked": false,
    "networkUsed": false,
    "publisherCodeExecuted": false,
    "limitation": "62 synthetic roots and fixed-latency fake scanner child exercise the real coordinator only; not actual model-enabled Solaris security or recovery proof.",
    "workerSourceSha256": "c1d01a28197c8afe4878851752c832251be101be09f1bd65bd0bc660e6251227"
  },
  "after": {
    "elapsedMs": 3342.644959,
    "completedRoots": 62,
    "totalFindings": 62,
    "retainedFindings": 25,
    "peakChildren": 2,
    "peakSumOfConfiguredModelLimits": 10,
    "perChildLimits": [
      5
    ],
    "aggregateSha256": "5899cd25294d7ec3bdf1856a886ebb507dead862f5a3d985d53b67e70f451c25",
    "modelsInvoked": false,
    "networkUsed": false,
    "publisherCodeExecuted": false,
    "limitation": "62 synthetic roots and fixed-latency fake scanner child exercise the real coordinator only; not actual model-enabled Solaris security or recovery proof."
  }
}

This executes the real TS coordinator. The same aggregate includes all62 findings, retaining25 under the existing storage cap. Native mocked-HTTP tests separately observe peak concurrency1 with cap1 and5 with cap5. Both retain the same unrelated partial/ValidationError outcomes; they are limiter compatibility evidence, not a completed security scan.

The initial independent P2 cleanup-acceptance finding is fixed: three failure-boundary regressions fail before and pass after, and real command cleanup-expiry classification is covered. The full nine-file fresh independent P0–P3 review has zero findings. All171 worker/shared and8,391 application tests pass (five configured skips), with static/types/build/package types green. Earlier overlapping-run startup timing failures are retained; unchanged sequential tests pass.

Actual protected Solaris replay and normal original-package scanner/judge/publication/download/install proof remain necessary. No issue closure or recovered-publisher claim follows from this control.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 30, 2026
@clawsweeper

clawsweeper Bot commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

Run two bundled OpenAI SkillSpector roots concurrently within the existing combined model-call ceiling, cancelling and joining siblings on failure.

Example: A plugin declares 62 bundled skill roots using OpenAI.

  • Before: Roots run serially, with up to ten model calls in the active scanner process.
  • After: Two roots can run together with five model calls each, while every root remains required before judge handoff.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch is focused and source-consistent, with meaningful subprocess coverage and quantified coordinator controls; real-provider capacity and the linked operational recovery remain unestablished.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored PR is outside the ordinary contributor-proof gate; its supplied coordinator output uses fake scanner children, and the separately reported installed-scanner check uses mocked HTTP, so neither establishes protected Solaris recovery or normal publication.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Performance · Worth it: Yes · Fix scope: Partial
User problem: Large bundled plugins can exhaust scanner preparation before all declared roots reach the security judge.
Reason: Maintainer authorship establishes direction, and the patch improves coordinator throughput without increasing the verified model-call ceiling or dropping required roots; it does not complete the linked recovery request.

Merge readiness

✅ Ready for maintainer review

Keep open: this maintainer-authored PR adds useful capacity sharing absent from current main, and inspection found no actionable introduced bug.

Priority: P2
Reviewed head: fb024951883fbf841979b020b116d9ee9bdb74fb

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied controls do not show the pinned model-enabled scanner completing the protected Solaris replay through the shipped worker.
Agent review details

How this fits together

The security worker resolves declared bundled skill roots, supervises SkillSpector subprocesses, and supplies their complete aggregate to the ClawScan judge.

flowchart TD
  A[Claimed package scan] --> B[Resolve declared roots]
  B --> C[Bounded root coordinator]
  C --> D[Supervised SkillSpector processes]
  D --> E[Complete canonical aggregate]
  E --> F[ClawScan judge]
  D --> G[Cancel siblings and join cleanup]
Loading

Technical review

Best possible solution:

Use the existing coordinator and subprocess supervisor for bounded parallel scanning, while keeping retained-publication recovery separately tracked.

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

Current main runs roots serially under a fixed twenty-minute stage deadline, so cumulative work can exhaust it; the supplied synthetic control exercises that coordinator, but no independent production reproduction was run.

Is this the best way to solve the issue?

Best-fix verdict: acceptable mitigation at the existing owner; extending the deadline would consume the lease envelope, while resumable scans would add a much broader state model, and real-provider throughput remains unproven.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Serial bundled-root scheduling changes intended behavior with a stated reason (b15bd52: Scan declared bundled skills instead of the entire package, preserve deterministic aggregation, and avoid multiplying scanner resources; the later per-root budget repair explicitly retained serial fan-out.)
  • Shared command cleanup and failure classification keeps the original intent (c1b7660: Confirm owned process-group absence on a bounded monotonic cleanup deadline before returning.)
  • Bounded bundled-scanner diagnostics keeps the original intent (8032540: Expose safe root indices, completion counts, budgets, and terminal classifications without leaking paths, findings, or scanner streams.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Patrick Erichsen: Raw commit c1b7660 adds scripts/lib/runWorkerCommand.ts:200 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: c1b7660fa33f; files: scripts/lib/runWorkerCommand.ts)
  • Vincent Koc: Raw commit b15bd52 adds scripts/security/run-codex-scan-worker.ts:1363 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b15bd52506c0; files: scripts/security/run-codex-scan-worker.ts)
  • Jason (Json): Raw commit eea13f2 adds scripts/security/run-codex-scan-worker.ts:1690 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: eea13f249ea5; files: scripts/security/run-codex-scan-worker.ts)

Review metrics

Metric Value Why it matters
Introduced change size Production +133/-17; tests +263/-12; spec +13/-0 The growth primarily covers distinct scheduling, cancellation, cleanup, failure-acceptance, and deadline contracts.

Labels

Label changes:

  • add P2: Bounded scanner capacity improvement for large bundled plugins, without evidence of an urgent regression introduced by this PR.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Bounded scanner capacity improvement for large bundled plugins, without evidence of an urgent regression introduced by this PR.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Use the existing protected diagnostic to measure completion and effective concurrency with the pinned scanner and real provider before claiming Solaris recovery.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 30, 2026, 11:40 AM ET / 15:40 UTC.

This branch was successfully deployed

1 active deployment
Preview – clawhub — fb024951 Deployed Oct 30, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant