Repository navigation
test: cover saved severity assessment caching - #1116
mldangelo-oai merged 2 commits into
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. |
![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: 06e65dc32e
ℹ️ 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".
| ORDER BY (assessment.scan_id = ?) DESC, assessment.assessed_at DESC, assessment.scan_id) AS rank | ||
| FROM json_each(?) AS selected | ||
| JOIN scan_severity_assessments AS assessment | ||
| ON assessment.finding_id = selected.key | ||
| AND assessment.input_sha256 = selected.value | ||
| WHERE assessment.rubric_sha256 IS ? | ||
| AND assessment.knowledge_base_sha256 IS ? |
There was a problem hiding this comment.
When multiple scans contain matching finding, rubric, and knowledge-base digests, this new query determines whether an assessment is reused and whether the current scan or newest historical row wins, but the change adds no Bun test exercising that behavior. A reversal in the ranking or a missed digest predicate could silently reuse the wrong severity result; add focused tests covering reuse, digest mismatches, current-scan preference, and newest-match selection.
AGENTS.md reference: sdk/typescript/AGENTS.md:L32-L33
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The core cross-scan reuse, selection precedence, and non-overwrite behavior lacks focused automated coverage.
Review effort: Balanced
Findings: 1
What changed in this PR
Reuses matching severity assessments across scans while retaining per-scan history.
Changes:
- Matches assessments by finding, rubric, and knowledge-base digests.
- Prefers current-scan or newest matching assessments.
- Adds migration 44 with a lookup index.
| File | Description |
|---|---|
sdk/typescript/src/severity-store.ts |
Passes digests and reuse state to persistence. |
sdk/typescript/src/classify-severity.ts |
Loads and saves reused assessments. |
plugins/codex-security/scripts/workbench_severity.py |
Implements matching and history-preserving saves. |
plugins/codex-security/scripts/workbench_schema.py |
Adds the reuse lookup index. |
plugins/codex-security/tests/test_workbench_setup_and_migrations.py |
Updates migration expectations. |
plugins/codex-security/tests/test_workbench_deep_scan.py |
Updates schema-version expectation. |
plugins/codex-security/tests/test_workbench_db.py |
Updates migration count expectation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rows = connection.execute( | ||
| """SELECT * FROM ( | ||
| SELECT assessment.*, selected.key AS finding_order, | ||
| ROW_NUMBER() OVER (PARTITION BY assessment.finding_id | ||
| ORDER BY (assessment.scan_id = ?) DESC, assessment.assessed_at DESC, assessment.scan_id) AS rank |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 06e65dc32e1cb2109d62385e6d2621ec6bf84ce5 against mdangelo/codex/pr939-stack-10-child-membership at 8bb57c26be1961d475ec14a946f5d9a5e886b925, scoped to this PR's diff and touched files.
Reviewed cross-scan lookup, digest matching, ranking, and reused-row persistence. No additional distinct findings. Direct SQLite probes passed for current-scan preference, newest historical fallback, input/rubric mismatch rejection, and preserving an unchanged reused row. Existing test-coverage comments were not duplicated.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
| validateSeverityClassification({ ...result, assessments: [previous] }, [ | ||
| finding, | ||
| ]); | ||
| await checkpoint?.save(finding, previous, result, true); |
There was a problem hiding this comment.
[P2] Remove the unreachable cross-scan reuse path
The persisted classifier hashes the entire canonical finding, including occurrenceId, and loadContractWithScanDirectory requires that ID to derive from the scan ID plus finding fingerprint. Consequently, a valid finding from another scan cannot have the same inputSha256: the history-wide ranking query can only return the current scan's row. This new save then adds a Python workbench process and an attempted upsert for every unchanged cache hit, with the reused predicate suppressing the actual update. I verified that changing only the scan-derived occurrence changes the digest, while an unchanged cached finding still invokes save. With the current identity contract, can we retain the per-scan lookup and skip saving cache hits? That lets us remove the history-ranking query, reused-write branch, and dedicated reuse index without losing supported reuse behavior.
There was a problem hiding this comment.
Independently reproduced the input-key mismatch at part 23 (#1095, commit 3065926), using the real finalizer identity routine and checkpoint loader. Two copies of the same finding retain findingId but get different scan-derived occurrenceId values; the actual checkpoint requests then have different inputSha256 values, so the exact-digest cross-scan lookup cannot reuse that assessment. The same probe reproduces on #1122, whose severity-store.ts is byte-identical to this PR's version (blob a159ac1a1b56a03d30990c53078cb829c2d318f5). This is an input-key reproduction; no live database or model run was performed.
8bb57c2 to
be51f35
Compare
06e65dc to
8f30c86
Compare
mldangelo-oai
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f30c86054
ℹ️ 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".
| CREATE INDEX scan_severity_reuse ON scan_severity_assessments | ||
| (finding_id, input_sha256, rubric_sha256, knowledge_base_sha256, assessed_at DESC); |
There was a problem hiding this comment.
This migration adds an index for cross-scan reuse, but no runtime query uses its keys: checkpoint(..., action="begin") still calls assessments() with the current scan_id, and that query only returns rows already attached to that scan. Fresh evidence in this revision is the new test at lines 373–376, which explicitly expects both identical findings in a second scan to invoke the model and receive new decisions. Consequently, matching historical assessments are never reused, so the advertised optimization does not occur and every new scan still incurs the classification calls while maintaining an unused five-column index.
Useful? React with 👍 / 👎.
be51f35 to
6b4fe01
Compare
00b5b3b to
678d409
Compare
6b4fe01 to
4c46558
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed the assessment-cache regression coverage and append-only migration sequence. No issue found in this test layer. The final stack's migration suite passed locally, including the later migration that removes the unused index.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact base-to-head diff with three independent review passes and root verification. No actionable findings remain. The checkpoint SQL refactor is equivalent, migration 44 preserves existing data, and the added tests verify same-scan cache reuse without extra saves plus independent classification across scans. Earlier cross-scan lookup comments concern code that is absent from this head.
Validation: 15 severity tests and 65 migration tests passed. Independent SQLite probes also checked upgrade/idempotence, index ordering, data preservation, integrity and foreign keys.
678d409 to
6dfd3e6
Compare
4c46558 to
63e9f8f
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent re-review passes and root reconciliation are complete for this restacked head.
No serious outstanding finding was identified in this change. The scoped prior review and current diff remain consistent.
Approval is for this change. Integrated-tip validation passed 1,357 SDK tests (31 skipped), 105 MCP tests (2 skipped), portable checks, types/builds and installed-package smoke. Other stack findings and known CI fixture failures still prevent the stack from being mergeable.
6dfd3e6 to
8231160
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at 8231160cbeab2bbc9f8a2797708241d0b584c3ba. The patch is unchanged from the prior three-pass review. I reconciled the inherited changes in touched files and retained the prior verified findings and tests.
No serious outstanding finding identified in this patch; prior conclusions remain consistent with the unchanged change or the current re-review.
Validation across touched paths at branch tip 06e75cfe: 1,356 SDK tests, 867 Python tests plus 100 subtests, and 108 MCP tests passed; builds, types, portable checks and installed-package checks passed. These are integration checks at the branch tip, not a claim that every intermediate PR or its CI is clean.
8231160 to
ed14ebb
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the identical patch and reconciled inherited touched-file changes at ed14ebb7aa6d.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch, touched-file changes, and current integration checks.
Current tip 355ec321 passed 75 report-projection tests, portable source checks, SDK build:ci and plugin build. The immediately preceding tip fec36f1d passed 1,360 SDK tests, 873 Python tests plus 100 subtests, 109 MCP tests, builds/types and the clean 600-entry installed-package check. Only report_projection.py and its test changed between those tips; SDK/MCP runtime files are byte-identical. Both descend from main 008a8b4d. These are integration checks, separate from own-head probes. No native Windows local run.
No serious outstanding finding introduced by this PR. Code review approval does not establish CI or merge readiness.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at 4d95f490703c.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Branch tip 3e0083d9, based on 008a8b4d, passed 1,392 SDK tests, 881 Python tests plus 100 subtests, 109 MCP tests, builds, portable source checks and the 600-entry installed-package check. These are tip checks, separate from own-head probes. No local Windows run was performed. The tip excludes the dependency update merged separately in #1184; combined validation remains outstanding.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
4d95f49 to
5ecb427
Compare
4f4bcd4 to
0faab67
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed exactly three fresh independent full-diff reviews, including every test change, and root verification at 5ecb42731bc5a10de3531e5b465816b6aafa7e6e against 0faab67d29ed96295f6dd6a627a981fa3a5f41df. No serious introduced code findings remain. Focused local validation passed: 15 SDK tests, 198 Python tests, nine portable source-check tests, native/plugin/SDK builds, types, formatting and Ruff.
Approval is temporarily withheld for the current hosted failure in release-pr.test.ts (“requests review when the proposal changes back to previously reviewed content”), which throws “The open release PR has no branch head.” Both that test and release-pr.mjs are byte-identical between this PR's base and head and are outside its five changed files. The named test passed once at each exact revision using the CI seed3182792084, but local Bun1.4.2 differs from hosted Bun1.3.14. These isolated controls do not establish flakiness or explain the hosted failure. The Node22 rollup failures report the failed prerequisite job.
The three code-review passes are complete; unchanged contributions do not need duplicate reviews. No implementation/test-source edits, model execution or local Windows run.
0faab67 to
e4cefbb
Compare
5ecb427 to
5fe9b31
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review remains complete with no serious introduced findings. The contribution, all touched blobs and complete head tree match the previously reviewed version. Root verified identity and retained the exactly three independent full-diff reviews and validation: 15 SDK tests, 198 Python tests, builds, types, formatting and portable checks passed.
The CI-only hold remains while replacement run37157109092 attempt1 completes. It has no failed latest-attempt jobs at the prepublication check. The earlier release-PR test failure belongs to the previous head's hosted run. Its unchanged base/head source and one successful local control at each revision do not explain that hosted failure or prove flakiness; local and hosted Bun versions differ.
If unchanged-head checks clear, this review can be approved without repeating the full reviews or tests. No source edits, model execution, installed-package smoke or local Windows run.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
The CI-only hold is cleared at 5fe9b31. The latest node-ci run 37157109092, attempt 1, completed successfully with 44 passed and 2 skipped jobs.
The head, base, complete PR patch and discussions remain verified. The existing three independent full-diff reviews, including all test changes, and root verification apply unchanged; no serious introduced finding remains. Existing validation includes 15 focused SDK tests, 198 Python tests, native/plugin/SDK builds, types, formatting and portable source checks. No duplicate reviews or test runs were needed for this clearance.
This approval covers this exact PR head. It does not establish successful validation of the whole stack with current main. The prior hosted release-test failure remains preserved as a historical result; this successful replacement run clears the approval hold without establishing its cause.
e4cefbb to
f72816f
Compare
5fe9b31 to
73a82e8
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 73a82e83c318ea3a2fd3bee6bccd0a35ca85f7ac against f72816f251265ba121c2c7284a321d3b725cb09f.
Retained exactly three prior independent HIGH reasoning full-diff reviews, including every test diff, after verifying identical ordered contributions and reconciling inherited changes in touched files. No duplicate full review passes were run. The only inherited touched-file change is the reviewed schema migration ordering and main entrypoint simplification; the own contribution is unchanged. Fresh validation passed 15 SDK tests, 198 Python tests, native host/plugin/SDK builds, types, formatting, Ruff, compatibility and all 9 portable source tests. No serious introduced finding remains.
Hosted CI was still pending at the 23:28 UTC check without failed jobs on this head. This review does not establish merge readiness or whole-stack integration. Validation was local macOS arm64 without model execution, source edits, a local Windows run, or a full installed-package smoke test.
73a82e8 to
40da7cb
Compare
f72816f to
2c9ff3e
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Approved the source at 40da7cb against 2c9ff3e. The actual GitHub diff, all five ordered file contributions and every touched base/head blob are identical to the previously reviewed version. Root verified the three original HIGH full-diff/test reports and their recorded hashes, retaining exactly those three passes.
The only whole-tree change is inherited from #1115: its scan-backed artifact guard and two MCP test updates. The head delta exactly matches the base delta; those changes received their own three fresh reviews, MCP validation and root public-tool controls. Prior 15 SDK and 198 Python passes and applicable build/portable checks retain their original tested revision and execution times. No duplicate code pass or test suite was run for this unchanged contribution. No serious introduced finding remains.
Hosted CI is still running. This is a source approval, not completed CI, merge readiness or whole-stack/current-main validation.
40da7cb to
eef3ca2
Compare
2c9ff3e to
0dac8ac
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed all five files of the actual PR diff at eef3ca2 against 0dac8ac with exactly three fresh independent HIGH full-diff passes, including every test change, plus root verification. The changed migration-count assertions and inherited schema/lifecycle changes were checked; no actionable introduced finding remains. Same-scan saved severity reuse and independent classification for another scan remain distinct.
Fresh validation passed 198 Python tests, plugin/SDK builds, types, formatting, Ruff, compatibility and all 9 portable tests. All 836 tracked source bytes/modes match the reviewed head. The prior 15 SDK passes at 73a82e8 are retained after verifying the complete SDK subtree and dependency identity; they are historical execution, not a fresh SDK run against the inherited Python changes. Native sources and 14 artifact hashes match the prior Darwin arm64 build. Ambient Python sandbox diagnostics are retained; commands exited 0.
No source edits, model execution, local Windows run or installed-package smoke was performed.
Hosted CI is still running, without failed latest-attempt jobs at the latest recorded check. This source approval does not establish merge readiness or successful whole-stack/current-main validation.
eef3ca2 to
d7ddd91
Compare
0dac8ac to
73f7746
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for d7ddd91f3e4d7e02654af698bf38d1f1cf2f6162 against 73f7746528c427a83dcc6f81b04773b7f14dc442. Completed exactly three fresh independent HIGH full actual-PR-diff passes, including every changed test and fixture, followed by root verification. No supported introduced source finding remains in this contribution.
Fresh validation passed all 71 migration tests, builds, source type checks, formatting and 9 portable source tests. Ruff and source compatibility passed after correcting the validation invocation’s version/Git context. Ambient Python profiling/process-enumeration permission diagnostics are separate from the 71-pass/exit-zero result. Prior severity SDK validation remains historical; no unchanged SDK suite or live model classification was rerun.
CI remains pending at the 2026-10-04T06:25:55.396371+00:00 audit; this is source approval only. Source approval and individual CI do not establish whole-stack/current-main successful validation or merge readiness. No model execution, local Windows run, native rebuild or clean installed-package smoke is claimed.
73f7746 to
151bd56
Compare
d7ddd91 to
b841674
Compare
There was a problem hiding this comment.
Source approval for b8416740b1cfeb5b33159776f3018966c6fb8c82 against 151bd562de43bd2cb9f7b6fba795c7463c8d0f0d. The ordered actual GitHub contribution is unchanged. All three prior independent HIGH full-diff reviews, including tests and fixtures, are retained after touched own/base modes and bytes were reconciled; inherited touched-file changes match the previously reviewed main changes on both sides. Root synthesis found no new supported finding.
Prior validation retains its original head and execution time; no duplicate local test or build was run for this unchanged contribution.
The prefix contains main at 634532d, while main has separately advanced to 1fb0e5f. This source approval does not establish successful whole-stack integration. Exact-head CI was pending in the 2026-10-04 09:26 UTC receipt and is tracked separately. No clean install, local Windows run or model execution is claimed.
CI update, 2026-10-04 09:39 UTC: current-head Windows Node22 test shard2 failed; this remains source-approved but CI-held. The identical provider-test blob is inherited from #1111, where a temporary-home warning causes three empty-stderr assertions to fail. This PR's job log has not been independently downloaded or assigned that immediate cause. The source finding is #1111 (comment). Original execution times and source-review results remain separate; no local Windows/model run or CI retry was performed.
151bd56 to
9956803
Compare
b841674 to
26f1cf8
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head 26f1cf82c286b0bb452322ef80a15b145fa60dea against base 9956803e8d1f9e4b64754fff3e594791c685c074 using three retained independent HIGH full reviews of the actual GitHub PR diff, including every changed test and fixture, followed by root synthesis. Actual three-dot and own-base diffs match.
The ordered contribution, exact GitHub diff bytes, touched own/base file modes and bytes, and applicable instructions match the earlier reviewed contribution. No inherited touched residual remains; original review and validation timestamps are retained. No unchanged local tests were repeated.
Hosted CI is still pending at the latest saved observation, with no failed jobs observed for this head. Source approval is separate from CI and integration. The reviewed prefix contains main 1fb0e5f, but successful whole-stack integration remains unestablished. No product models, clean installation, local Windows execution or new native compilation were performed.
Consolidate approved Deep Scan changes into the parent topic branch.
Part 11 of 46. Previous: #1115 · Next: #1117 · Stack index
Summary
Verify that unchanged findings reuse their own scan's saved severity assessments while another scan with matching findings is classified independently.
Changes
Testing
The severity-cache regressions passed in the shared SDK validation, which completed 157 tests with three platform skips. Rebuilt migration and composition checks passed within a 209-test Python selection. SDK/MCP types, Ruff check/format, SDK CI build, and portable source checks passed. These are local author and integration checks; CI on the final restacked head is reported in Checks.
Risk and rollout
This adds regression tests and an index migration. It does not enable cross-scan assessment reuse; reprocessing still performs a fresh assessment for the selected scan.
Public disclosure review