Repository navigation
refactor: merge findings without rewriting their evidence - #1119
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. |
|
@codex review |
|
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". |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The substantial model-driven merge path warrants human review because no model evaluation or full runtime suite was run.
Review effort: Balanced
Findings: None
What changed in this PR
Adds host-controlled Deep Scan merging that preserves original findings, provenance, identities, contexts, and coverage.
Changes:
- Validates model-proposed source grouping and canonical selection.
- Adds deterministic merge fixtures, preservation tests, and an opt-in model evaluator.
- Documents evaluation scope and limitations.
| File | Description |
|---|---|
sdk/typescript/src/scan-merge.ts |
Implements merge validation, preservation, coverage combination, and prompting. |
sdk/typescript/tests-ts/scan-merge.test.ts |
Tests merge invariants and preservation behavior. |
sdk/typescript/tests-ts/scan-projection-fixtures.test.ts |
Extends projection fixture coverage for merging. |
sdk/typescript/tests-ts/merge-eval.test.ts |
Tests evaluation fixtures and negative controls. |
sdk/typescript/scripts/merge-eval/run.ts |
Adds the opt-in model evaluation runner. |
sdk/typescript/scripts/merge-eval/fixtures.ts |
Defines synthetic merge scenarios. |
sdk/typescript/scripts/merge-eval/grade.ts |
Grades grouping and canonical selection. |
sdk/typescript/scripts/merge-eval/README.md |
Documents evaluator usage and limitations. |
evals/README.md |
Links the completed-report merge evaluation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 6d5877a466fd2b0553b44e3546a87c3e006f0245 against mdangelo/codex/pr939-stack-13-checkpoint-publication at bd3bf330ea4c9a0ad8a5878c32e84d95eb79d39f, scoped to this PR's diff and touched files.
Reviewed original-finding selection, grouping/merge behavior, retained source evidence, and changed tests. No new actionable findings.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
bd3bf33 to
e65aed1
Compare
6d5877a to
c73e8a7
Compare
![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: c73e8a7e0c
ℹ️ 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".
| const scope = | ||
| previous?.scope ?? inputs.find((input) => input.draft.scope)?.draft.scope; |
There was a problem hiding this comment.
When an earlier batch contains a threat model but no scope, this function still creates previous.scope solely to hold sourceScans; on the next merge, the nullish-coalescing expression treats that synthetic container as the retained scope and ignores the first actual input.draft.scope. The new scope remains nested in sourceScans, but its summary, limitations, and other fields never reach the canonical top-level scope consumed by scan preparation. Select a prior scope only when it contains real scope metadata, or merge the first new scope into the existing sourceScans container.
Useful? React with 👍 / 👎.
| priorCoverage?: SemanticCoverage, | ||
| ): SemanticCoverage { | ||
| const completed = [...(priorCoverage ? [priorCoverage] : []), ...inputs]; | ||
| const coverage: SemanticCoverage = { |
There was a problem hiding this comment.
When a completed child contains any valid top-level coverage extension, combineScanCoverage reconstructs the result from only completeness and the four known arrays, so the extension is silently discarded even with a single input. The semantic coverage schema deliberately permits additional properties, and projection retains them, making this the point where completed artifact data is lost; preserve or namespace non-host-owned fields while recomputing the host-owned merge fields.
AGENTS.md reference: sdk/typescript/AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
c73e8a7 to
2b9c16b
Compare
1a14c3e to
03f4d6c
Compare
2b9c16b to
5dabbee
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed grouping validation, stable finding identities, retained source evidence, and prevention of splitting accepted groups. No issue found in this merge layer. Merge/reconciliation and semantic-preservation tests passed at the integrated stack tip; these checks did not evaluate live-model grouping quality.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact diff with three high-reasoning passes and root verification. Approval remains withheld for the two existing metadata-preservation issues: #1119 (comment) and #1119 (comment). I reproduced both: a threat-model-only first batch prevents the next real scope summary/limitations from reaching the aggregate top level, and combining a single coverage input removes its permitted extension metadata.
Validation: 25 merge/evaluation tests and 9 projection-fixture tests passed; the plugin was rebuilt. Direct probes confirmed the two losses. No model evaluation was run. Review-process caveat: one reviewer accidentally saw these existing comments before inspecting source; the other two passes and my direct reproductions supplied separate verification.
5dabbee to
b3aaa8d
Compare
03f4d6c to
6a0842d
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reconciled the unchanged patch and prior three-pass review against this exact restacked head.
The touched files are byte-identical to the previously reviewed own head, retaining the existing scope/coverage metadata findings. The later merger replacement preserves the first real scope, but that later repair does not change this PR head.
Approval remains withheld. This review distinguishes the PR's own head from fixes verified later in the stack; it does not duplicate existing inline findings.
6a0842d to
63f0fba
Compare
b3aaa8d to
5b0e7f5
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at 5b0e7f5fef033e77064b85c4b759797d67505776. 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.
The unchanged production patch retains the existing scope/coverage metadata findings. The later merger replacement preserves the first real scope, but that later repair does not change this PR head.
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. Latest workflow runs for this unchanged head were verified successful at 2026-10-02 01:26 UTC. Earlier failed or canceled job records are superseded; this CI status correction does not alter the review decision.
63f0fba to
f170e3a
Compare
5b0e7f5 to
b0689ae
Compare
f170e3a to
d24d201
Compare
b0689ae to
6b8c8b5
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 6b8c8b58a031.
The unchanged production patch retains the existing scope/coverage metadata findings. The later merger replacement preserves the first real scope, but that later repair does not change this PR head.
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.
Approval remains withheld for the findings above.
6b8c8b5 to
3d70a37
Compare
d24d201 to
1722ac3
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 verified identical patch and reconciled inherited touched-file changes at 3d70a3799139.
The identical patch and byte-identical touched files retain the existing scope/coverage metadata findings. The later merger replacement repairs these paths; those later changes do not repair this PR head.
Integrated tip 6ef8cbb8, based on 008a8b4d, passed 1,388 SDK tests, 879 Python tests plus 100 subtests, 109 MCP tests, portable checks, builds and the clean 600-entry installed-package check. These are branch-tip integration checks, separate from own-head probes; no local Windows execution was performed.
Approval remains withheld for the findings above.
1722ac3 to
d3aefa4
Compare
3d70a37 to
3433e80
Compare
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 3433e801f67a.
The identical patch and byte-identical touched files retain the existing scope/coverage metadata findings. The later merger replacement repairs these paths; those later changes do not repair this PR head.
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.
Approval remains withheld for the outstanding findings.
d3aefa4 to
17687fa
Compare
3433e80 to
2bc66a5
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Approved the source contribution at 2bc66a56a0c3 / base 17687faef300 after exactly three fresh independent HIGH full actual-diff reviews (all 9 files and every test/fixture diff), plus independent root verification.
The two prior metadata-loss holds are fixed at this own head. An independent control used the actual merge validation, coverage combiner, shared SDK publisher and a registered workbench parent for three draft publications. Disk readback preserves late real scope after an earlier context-only batch, all source contexts, the earlier threat model, opaque/conflicting/nested coverage metadata across a resumed batch, and partial coverage. Inputs remain unchanged. These are synthetic completed inputs through actual publication; no child-producing or model-driven Deep Scan is claimed.
Fresh validation passed: 40 SDK tests, plugin/SDK builds, types, format, Ruff 0.16.9, compatibility, 9 portable checks and the 603-entry production archive. All 858 tracked source bytes/modes match. Native artifacts retain their historical Darwin arm64 build receipt after byte/source/dependency identity checks. No repeated unchanged Python suite, fresh native build, full installed-package smoke, local Windows run or models.
CI is not clear: the latest observed run contains the inherited workbench_result_merge.py --help source-contract failure introduced in #1118. The aggregate was still pending at the 2026-10-04 02:47 UTC audit. This source approval does not establish merge readiness or successful whole-stack/current-main validation.
17687fa to
25d161b
Compare
2bc66a5 to
f9bdcd4
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the full actual PR diff at f9bdcd40f022430db91b20caf42d87b4796e1348 against 25d161b369ebfacf2b813fb592bf261e34f81eb7, including every changed test/fixture, with exactly three fresh independent HIGH passes plus root verification. No supported introduced finding remains in this contribution.
Checked all-source accounting, retained finding identity, prior scope and coverage preservation, and the merge fixtures/oracle. Fresh focused SDK validation: 40 passed. Builds, types, format and the 607-member production archive check passed. No Python rerun was needed for this contribution.
The exact-head hosted MCP job reproduces the eight inherited #1118 registration fixture setup failures. The workflow remained in progress at the 08:11:35 UTC observation. This is source approval with a CI hold. The reviewed chain includes the prior main commit dff0436; main has since advanced. Whole-stack/current-main successful validation is not established.
Native artifact identities were retained after unchanged native source/dependency checks; no fresh native build, clean installed-package smoke, local Windows run or model execution.
25d161b to
5480e23
Compare
f9bdcd4 to
8da5249
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8da5249846
ℹ️ 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".
| structuredClone(source[field] ?? []), | ||
| ), | ||
| ) as never; | ||
| for (const reason of unresolved) coverage.deferred.push({ reason }); |
There was a problem hiding this comment.
When priorCoverage.deferred already contains a reason from an earlier merge and a resumed call supplies the same still-unresolved reason, this unconditional push adds a duplicate after the prior rows were exact-unioned. prepareSemanticScanDraft subsequently assigns separate generated IDs to the identical rows, making one outstanding pass appear as multiple tasks and allowing duplication to grow on each resume; include the { reason } rows in the same exact union or otherwise preserve idempotence against prior coverage.
AGENTS.md reference: sdk/typescript/AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| for (const source of finding.provenance.sourceFindings ?? []) | ||
| sources.set(source.id, { original: source.finding, canonical: finding }); |
There was a problem hiding this comment.
When a saved finding contains multiple sourceFindings entries with the same ID but different payloads, this Map.set silently retains only the last payload. That state is deliberately supported by preserveFindingDetails, whose source union keeps differing evidence versions under one ID, so the next successful merge reconstructs sourceFindings from the map and permanently discards earlier exact evidence; retain all distinct payloads associated with each ID while still exposing one grouping reference.
AGENTS.md reference: sdk/typescript/AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| "large-field-and-nested-history", | ||
| [input("current", [historic])], | ||
| [group(["history:0", "current:0"])], |
There was a problem hiding this comment.
In the large-field-and-nested-history case, the retained tail explicitly requires tail-repair, while the current historic observation requires visible-repair; the production prompt says findings with distinct required repairs must remain separate unless either repair corrects every observation. Expecting one group therefore penalizes a model that reads the long nested tail and follows remediation-subsumption, while a model that truncates or ignores the intended signal passes; use separate expected groups or rewrite the fixture to establish actual repair subsumption.
Useful? React with 👍 / 👎.
| findings[order[position]!.index] = finding; | ||
| }); | ||
| const contexts = [ | ||
| ...((previous?.scope?.["sourceScans"] as JsonObject[]) ?? []), |
There was a problem hiding this comment.
When a resumed aggregate's scope contains a valid non-array extension named sourceScans—for example, in a checkpoint created before this host-owned convention—the TypeScript cast provides no runtime validation and this spread throws a non-iterable TypeError before merging. Because semantic scopes permit arbitrary extension values, an extension-name collision can make a completed checkpoint unresumable; spread only arrays and retain any opaque legacy value as source context instead of assuming the host representation.
AGENTS.md reference: sdk/typescript/AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head 8da52498460f558d4c3460bfc535cc771b4c56e4 against base 5480e23d50e7bbc5e8b8503cc30049b386074aaa using three retained historical HIGH full actual-diff reports, including all changed tests and fixtures, followed by root synthesis. Original reports and their review times are preserved.
Source review remains held on a narrow P2 confirmed from the existing discussion: duplicate unresolved work. Passing combineScanCoverage its own prior output and the same unresolved reason creates two identical deferred rows. One exact-source synthetic control confirmed counts 1, 2, 2; it does not establish unbounded growth or coordinator behavior. Include unresolved rows in deduplication.
The contribution, GitHub diff bytes, touched own/base modes and bytes, and instructions match the earlier reviewed version. No fresh full source review or unchanged test suite was repeated. Other source-payload and legacy-scope allegations require supported input shapes not established in the touched scope; the nested-history evaluation has a limited tail-reading oracle, without a proved wrong grouping.
Hosted CI at 2026-10-04T12:53:09.313276+00:00–2026-10-04T12:53:33.556119+00:00 was pending with no failed jobs observed for this head. Source approval, CI and successful integration are separate. This prefix contains main 1fb0e5f, but whole-stack successful integration remains unestablished.
No product model, clean installation or local Windows run was performed. Native binaries were retained after source/dependency/artifact identity checks; no fresh native compilation occurred. Individual full-diff reviews qualify selective unchanged long-file context.
8da5249 to
3359dec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3359dec424
ℹ️ 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[field] = exactUnion( | ||
| completed.flatMap<unknown>((source) => | ||
| structuredClone(source[field] ?? []), |
There was a problem hiding this comment.
When a prior call added an unresolved reason and a resumed call later passes [] after that work completes, completed includes priorCoverage, so this union copies the host-generated deferred row back into the result. The prior aggregate also remains partial, causing the completeness calculation above to keep every subsequent aggregate partial indefinitely. Even with the new exact-union deduplication, an incremental scan that ever had outstanding work therefore cannot become complete; host-generated unresolved rows must be tracked or removed when they are no longer present.
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 3359dec42477888d60b291d477484ae1b0dfa4e9 against base 5480e23d50e7bbc5e8b8503cc30049b386074aaa. Three fresh independent HIGH full actual-diff passes, including every changed test/fixture, plus root synthesis.
The previous deferred-row duplication is fixed: the bounded exact-source control returns 1, 1, 1 rows across repeated raw helper output. All 32 selected SDK tests passed, as did plugin/SDK builds, types, format and source compatibility. The nested-history fixture now checks the two distinct synthetic groups. Two further coverage candidates remain conditional: manually supplied publication IDs can duplicate, and manually retained host-partial state can remain partial, but the reviewed current caller saves raw state and normally recomputes from current child coverage and unresolved work. No supported current producer for those problematic inputs was established.
Hosted CI is recorded separately in the final review evidence; it is not inferred from these local or retained checks. Source approval, hosted CI and whole-stack/current-main integration remain separate. No product model, local Windows run, clean install, fresh native compilation or deployment validation was performed.
Hosted CI observation 2026-10-04T13:37:39.993325+00:00 to 2026-10-04T13:38:02.237036+00:00: this exact head was success. No failed jobs were reported for this head in that observation. Later source/metadata readbacks are not CI refreshes.
Consolidate approved Deep Scan changes into the parent topic branch.
1528d80
into
mdangelo/codex/pr939-stack-13-checkpoint-publication
Part 14 of 46. Previous: #1118 · Next: #1120 · Stack index
Summary
Add a semantic merge step that groups duplicate findings and selects an existing finding to represent each group. Application code preserves the selected finding, original sources, and earlier accepted versions.
Changes
Testing
After the unresolved-task correction, all 31 merge/evaluation tests passed with 127 assertions; SDK types and formatting passed. The regression exercises three saved-coverage roundtrips and canonical publication while retaining distinct owned tasks. A separate activated-scheduler regression reproduced duplicate saved work before correction and passed afterward (1 test, 5 assertions).
The nested-history evaluation-oracle correction separately passed 31 merge/evaluation tests with 118 assertions, plus SDK CI build, types and formatting. Its negative control rejects collapsing the additional retained repair.
In earlier validation, all 21 merge cases passed with 71 assertions after giving the Boolean matrix distinct JUnit names. The existing report-comparison script reproduced duplicate test identities before the correction and passed afterward. Types, formatting and diff checks also passed.
Earlier merge-evaluation validation passed nine tests, including a native CLI check that the runner disables inherited dotted-name MCP servers while preserving their transport settings. Types, formatting and the SDK CI build passed.
Earlier validation passed all 30 focused merge/evaluation tests. Regressions reproduced lost scope, coverage metadata, and threat-model contexts before correction; the evaluation-runner case verified dotted server names at the SDK child-argument boundary. SDK types and formatting passed. Final-head CI is reported in Checks.
Risk and rollout
The scheduler in part 15 uses these helpers. Public SDK and plugin scan entrypoints adopt them in part 39 and part 40. The representative retains one source's text; complementary details remain available in the retained sources.
Public disclosure review