Repository navigation
fix: share bundled SkillSpector capacity across two roots - #4152
Patrick-Erichsen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
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. |
|
Codex review: needs maintainer review before merge. What this changesRun 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.
Review scores
ProductKind: Performance · Worth it: Yes · Fix scope: Partial 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 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe 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]
Technical reviewBest 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
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/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. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 30, 2026, 11:40 AM ET / 15:40 UTC. |
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.