fix(workers): preserve explicit scanner model settings - #3802
Conversation
|
🦞👀 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 24, 2026, 4:02 AM ET / 08:02 UTC (Revision 3). ClawSweeper reviewWhat this changesThe branch forwards explicit model and reasoning settings to two security-worker scanner subprocesses and moves a skill-metadata browser check to a seeded local CI fixture. Merge readiness✅ Ready for maintainer review Keep this PR open. Current main still drops the explicit scanner settings, while the branch provides a bounded repair and retains the browser layout check in required CI. The related draft PR addresses a broader model rollout and is not a replacement. Priority: P2 Review scores
Verification
How this fits togetherClawHub security workers receive skill and package scan jobs, launch scanners with restricted environments, and use their reports in publication and moderation decisions. A separate browser CI runner starts a disposable app and Convex backend to check the rendered skill page. flowchart LR
A[Scan job] --> B[Security worker]
B --> C[Restricted child environment]
C --> D[ClawScan and SkillSpector]
D --> E[Scan result]
F[Local browser runner] --> G[Seeded skill catalog]
G --> H[Skill layout check]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the narrow worker allowlists and the required seed-backed browser assertion; handle any change to active scanner defaults in the separate dependency-gated rollout. Do we have a high-confidence way to reproduce the issue? Yes, from source: current main reconstructs scanner child environments from allowlists that omit the explicit settings. I did not execute a current-main scanner reproduction. Is this the best way to solve the issue? Yes—this is the narrowest fix for the pass-through gap. Inheriting the entire worker environment would weaken isolation, and removing the browser assertion would lose layout coverage. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9896c69728cd. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
This branch was successfully deployed
1 participant
What Problem This Solves
Security workers discard explicitly configured scanner model and reasoning settings when they create restricted subprocess environments. The required browser gate also depends on a live skills.sh entry that now renders the application's not-found page.
User Impact
Preserve optional scanner controls while leaving active scanner defaults, scanner versions, credentials, moderation decisions, and result reuse unchanged. Luna/high activation remains separate in #3800 until compatible scanner releases and credentialed verification are available.
Keep the existing skill metadata layout coverage in required CI using ClawHub's controlled local mirror fixture, rather than the availability of one live public entry.
Why This Change Was Made
aa945803: addREASONING_EFFORT,SKILLSPECTOR_MODEL, andSKILLSPECTOR_REASONING_EFFORTto both existing restricted child-environment lists. Tests cover both ClawScan workers and direct bundled-skill SkillSpector, including the existing worker-token and endpoint exclusions. Upstream scanner versions still determine supported settings.d86aef72: call the existingdevSeed:seedCanonicalSearchFixtureafter the disposable local backend is configured and functions are pushed; move the metadata layout test to the requiredprofile-contextshard. All typography, wrapping, no-overflow, and runtime-error assertions are preserved. The stress setup normalizes to exactly three category/topic nodes. The fixture leaves imports and scan admission disabled; no application or Convex source changes.744b8961: match the existing local-auth runner guard, so the broad public-backend browser command does not try to use a local-only fixture. The required local shard setsVITE_ENABLE_DEV_AUTH=1and executes every metadata assertion.Application production TypeScript: net +6 lines for supported provider controls. Test-runner infrastructure: net +3. Tests: net +35, including the intact moved layout test. Specs/changelog: net +8; CI routing: +1. Scanner workflow defaults and dependency locks are unchanged.
Evidence
Final head:
744b8961f49aee2480ff85b80eaae732923967e4.ci:static;ci:unit(7,195 passed, 3 skipped, coverage passed); schema and CLI TypeScript checks. The four worker/test files remain byte-identical to that proof and the earlier independent Sol/high review.ci:static; all fourprofile-contextbrowser specs, including the moved layout test, against a real disposable local Convex/backend and app with retries disabled; all 18 remaining public-read smoke tests. Command exit 0; lease stopped.ci:staticpassed; with dev auth disabled and an unreachable base URL the local-only spec skips without backend access; the canonical local-auth runner executes the metadata test with retries disabled: 1 passed, 0 skipped. Command exit 0; lease stopped.744b8961f49aee2480ff85b80eaae732923967e4; required results and current-head ClawSweeper findings are verified before merge.AI-assisted implementation; the changed boundaries and proof described above were independently reviewed.