Repository navigation
chore(security): trace fixed Solaris model exceptions safely - #4147
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.
|
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: blocked before merge. What this changesAdds a fixed Solaris scan workflow that records bounded exception locations without exporting raw errors or changing publication decisions. Example: Solaris 0.69.0 fails scanning skills/fleet-dispatcher with TypeError.
Review scores
ProductKind: Feature · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Blocked before merge - 2 items remain Keep open: this maintainer-authored PR adds useful diagnostic capability absent from main; one required security-contract documentation change remains. Priority: P2 Before merge
Findings
Tests
Agent review detailsHow this fits togetherA protected manual GitHub workflow verifies immutable Solaris source, invokes the production bundled SkillSpector helper through an observer, deletes private scan material, and uploads a restricted summary. flowchart TD
A[Protected manual workflow] --> B[Verify fixed Solaris source]
B --> C[Production bundled scan helper]
C --> D[Observed SkillSpector CLI]
D --> E[Restricted metadata projection]
E --> F[Summary artifact]
D --> G[Private scratch cleanup]
Technical reviewBest possible solution: Keep the fixed observer beside the existing diagnostic and document its export, admission, cleanup and nonpublication invariants in specs. Do we have a high-confidence way to reproduce the issue? Current main projects bounded error classes but no originating frame locations; the hosted failure is confirmed by run metadata, while its underlying TypeError cause remains unverified. Is this the best way to solve the issue? Best-fix verdict: acceptable diagnostic enablement; reusing the production helper avoids a second scanner implementation, while changing normal workers or exporting stderr would unnecessarily widen scope and disclosure. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 248990acafa8. Provenance checked
TestingProof path: unit tests only. 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, 7:01 AM ET / 11:01 UTC. |
|
Review follow-up: I added the fixed diagnostic contract to the existing Inspectable native before/after output from the retained paired control (both run the installed CLI with the same injected model TypeError, network disabled): {
"before": {
"cliExitCode": 2,
"simulatedModelCalls": 8,
"executionSuccessful": false,
"status": "failed",
"TypeErrorCount": 5,
"all1335OriginalHashesPreserved": true,
"actualModelsInvoked": false,
"networkEnabled": false,
"publisherCodeExecuted": false
},
"after": {
"cliExitCode": 2,
"simulatedModelCalls": 8,
"executionSuccessful": false,
"status": "failed",
"TypeErrorCount": 5,
"all1335OriginalHashesPreserved": true,
"actualModelsInvoked": false,
"networkEnabled": false,
"publisherCodeExecuted": false
},
"sameCliOutcomeAndReportFacts": true,
"privateCanariesAbsent": true,
"observerLines": 10,
"limit": "Both cases inject the same fake model TypeError into the actual installed CLI. This proves observer behavior, not the production exception origin."
}Observed lifecycle and first bounded exception record: [
{
"rootIndex": 0,
"kind": "installed"
},
{
"rootIndex": 0,
"kind": "failure",
"chain": [
{
"class": "TypeError",
"statusCode": null,
"frames": [
{
"module": "skillspector_llm",
"line": 1498
},
{
"module": "skillspector_llm_utils",
"line": 664
},
{
"module": "langchain_runnables",
"line": 3500
},
{
"module": "langchain_runnables",
"line": 6031
},
{
"module": "langchain_chat_models",
"line": 552
},
{
"module": "langchain_chat_models",
"line": 1931
},
{
"module": "langchain_chat_models",
"line": 1889
},
{
"module": "langchain_chat_models",
"line": 2226
}
],
"unknownFrames": 2,
"framesTruncated": true
},
{
"class": "RuntimeError",
"statusCode": null,
"frames": [
{
"module": "skillspector_llm_utils",
"line": 758
}
],
"unknownFrames": 0,
"framesTruncated": false
}
]
}
]This control proves observer behavior. It does not identify the real hosted exception origin: the local published image has OpenAI SDK2.54.0/LangChain OpenAI1.7.3, while the failed hosted replay used its uv-lock versions2.44.0/1.3.3. The actual protected hosted diagnostic remains necessary. The secondary RuntimeError in the synthetic chain is incidental event-loop context and is not evidence of the production cause. No publication recovery is claimed. The SecOps team request is still pending. Patrick's existing full-incident independent-review exception is recorded; no team approval is claimed and enforced GitHub gates remain enabled. |
|
The P3 finding is addressed in SecOps team approval remains pending and is not claimed. Continuing within Patrick’s recorded full-incident independent-review exception while preserving enforced GitHub gates. No diagnostic dispatch, original retry or publication recovery has occurred. |
The protected Solaris 0.69 replay 41036730169 completes 28/62 roots, then
skills/fleet-dispatcherexits 2 with five semantic TypeErrors despite 523.8 seconds remaining. Existing safe metadata cannot locate the exception. Add a fixed nonpublishing diagnostic that scans this unchanged root first, then all 62 roots only after a complete successful first job, and exports allowlisted trusted module IDs/line numbers and bounded exception classes/status codes.The observer invokes the installed CLI with the same source/model/budgets and wraps only its failure classifier, preserving returns, exceptions, retries and scan decisions. Original bytes and inventory verify before/after. Lifecycle/count validation prevents missing observer evidence from being reported as success. No dispatch inputs, arbitrary target/source/provider selectors, queue/claim/publication writes, raw messages, provider bodies, findings or publisher code execution. Default diagnostic scope and normal workers are unchanged.
Paired native control: with the same simulated model TypeError and network disabled, both before/after exit 2, make eight simulated calls, retain five TypeErrors and identical report facts, and preserve all 1,335 original hashes. After adds ten safe observer records, with private canaries absent. This proves observer behavior, not the actual hosted TypeError origin. The actual observed hosted run and causal repair remain follow-up work; this PR does not recover or unblock Solaris.
Validation: 94 focused tests; all 8,377 full tests pass with five configured skips and four workers, coverage thresholds pass;
bun run ci:static;bun run ci:types-build; schema and CLI packagetsc --noEmit. Independent restricted Sol/high P0–P3 review: scoped-clean, zero findings. Privacy, provenance, admission, symlink, bounds and observer-preservation controls pass.The existing security-moderation spec records this diagnostic’s admission/export/provenance/bounds/lifecycle/cleanup and non-recovery contract. Refreshed seven-file independent P0–P3 review is scoped-clean with zero findings; documentation-only follow-up preserves all six implementation files. Inspectable paired native control output and limitations. First CI attempt’s generated-card browser capacity/fixture failure is retained; one bounded failed-job rerun passed on unchanged implementation.