Repository navigation
fix(risk-fork): enforce complete SkillSpector admission coverage - #453
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jmborden1112-beep
left a comment
jmborden1112-beep
left a comment
There was a problem hiding this comment.
Owner-controlled secondary-account approval, explicitly authorized by the solo maintainer. This is not independent human review. Exact head fa2d9f6 passed 23/23 checks; separate AI source/security/compatibility review found no remaining verified blocker. Default-off source only: no deployment, publication, provider call, spend or activation approval.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa2d9f6261
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || coverage.analyzer_status_count < REVIEWED_STATIC_ANALYZER_IDS.size | ||
| - coverage.missing_static_analyzer_ids.length | ||
| || coverage.analyzer_incomplete_count + coverage.applicable_static_analyzer_count | ||
| > coverage.analyzer_status_count | ||
| || coverage.applicable_static_analyzer_count > REVIEWED_STATIC_ANALYZER_IDS.size | ||
| - coverage.missing_static_analyzer_ids.length | ||
| || coverage.static_completed_work < coverage.applicable_static_analyzer_count | ||
| || coverage.static_completed_work > coverage.applicable_static_analyzer_count * MAX_REPORT_ITEMS |
There was a problem hiding this comment.
When an untrusted evidence producer can recompute the packet's self-hashes, these count-only checks do not prove which analyzer IDs appeared in the host-hashed raw report. For example, evidence produced from a report omitting one reviewed analyzer can be changed to missing_static_analyzer_ids: [], have analyzer_status_count incremented, and have its clear outcome plus normalized/evidence hashes recomputed; verifySkillSpectorAdmissionEvidence() then accepts it even when expected.report_hash still matches the incomplete raw report, because the host callback verifies only that hash and never re-derives coverage. Bind a trusted re-adaptation or expected normalized projection to the raw bytes, and add a coordinated-forgery test that retains the original raw-report hash.
Useful? React with 👍 / 👎.
| "applicable_static_analyzer_count": { "type": "integer", "minimum": 0, "maximum": 16 }, | ||
| "static_completed_work": { "type": "integer", "minimum": 0, "maximum": 320000 }, |
There was a problem hiding this comment.
For consumers that validate this public evidence contract through JSON Schema, the new global maximum permits impossible combinations such as applicable_static_analyzer_count: 1 with static_completed_work: 320000. The JavaScript verifier rejects that packet using the per-applicable-analyzer limit at skillspector-admission.mjs:1148-1149, but the schema—and therefore the risk-decision.v1.json schema that references it—accepts it, leaving schema-only consumers with different admission semantics. Encode the dependent maximum for each applicable count (or expose bounded per-analyzer work entries), and add schema-negative cases such as one applicable analyzer with 20,001 completed items.
Useful? React with 👍 / 👎.
| ]); | ||
| const MAX_REPORT_BYTES = 8 * 1024 * 1024; | ||
| const MAX_REPORT_ITEMS = 20_000; | ||
| const MAX_STATIC_COMPLETED_WORK = MAX_REPORT_ITEMS * REVIEWED_STATIC_ANALYZER_IDS.size; |
There was a problem hiding this comment.
The pinned v2.11.2 scanner defines a workflow-wide 10,000-event ceiling in its inspection ledger, and its reducer replaces overflow with an output-limit partial record rather than preserving every completed event. Because analyzer completed counts are derived from those retained ledger events, an authentic clear report cannot contain the 24,000 completed work items exercised by the new 1,500-component test. Raising the accepted aggregate to 320,000 therefore admits raw reports that the pinned scanner cannot produce as complete; keep the aggregate within the scanner's global ledger ceiling (and incomplete whenever its limit marker is present), then validate the boundary using actual pinned-scanner output rather than a hand-built report.
Useful? React with 👍 / 👎.
Summary
Repairs the three post-merge SkillSpector findings on #421:
A report containing only
static_yara, orscore: 100 / severity: LOW / recommendation: SAFE, could produce clear admission. A legitimate 1,500-component report inspected by all 16 reviewed analyzers instead failed when 24,000 completed work items exceeded a per-analyzer limit incorrectly applied to the aggregate.not_applicableis valid; unavailable, degraded, failed or reviewed-static disabled statuses cannot clear admission. Positive applicable completed static work remains required.Exact-head integration
Candidate:
fa2d9f6261c21cac46a6bc8ebecf68f6b9fa12b5, signed and GitHub-verified. Base:92cf2cc946ffe6319a4019dc78c97e530fb146a2.The original four source/schema/test/example paths are preserved. A normal signed merge retains merged stateless runtime #454 and authentication isolation #452 without rebasing or force-pushing. The exact current-main diff has nine paths: original four, hosted bundle/integrity pair, and three narrow documentation updates. Reviewed source integrity:
sha256:047ce6f45fe8d842d2896aba64a4225c30ee34167583acf63b749a29a18e7f38.Validation
Boundaries and compatibility
The
skillspector-admission-evidence.v1label is an unpublished source-only alpha contract tightened in place. Older normalized packets without the required coverage field fail closed and must be regenerated from the exact pinned report and independently verified current bindings. If the raw report omitted analyzer statuses, rerun the bounded network-isolated scan; do not fabricate coverage. No persisted production migration or automatic legacy acceptance is provided.SkillSpector remains advisory-only/default-off. This change does not invoke a scanner, grant trust, execute an effect, contact E2B, spend funds, publish a package, deploy/configure a service, or activate live protection. Hosted production, provider qualification and managed operations remain separate open gates.
Workflow Trace
Real multi-agent source/compatibility/security review; workers
gpt-5.6-lunaat medium. Coordinator retained the available runtime; requested Sol Ultra coordination was not available and is not claimed. No formal ECF receipt run. Main coordinator owned integration, generated refresh, tests, signing, push and GitHub state changes. Independent AI source reviews found no remaining verified blocker; owner-controlled secondary-account approval, if used, is not independent human review. Live deployment/provider behavior was not assessed.