Repository navigation
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. |
 [Medium impact] The PR appears safe to merge; no new actionable issues were found. SummaryThe PR compares retained script content without treating capture-local occurrence numbers as stable identities.
No new actionable issues were found. The earlier unnumbered finding about incomplete inventories is addressed: retained sources are not compared when the inventory is incomplete. Tests were inspected, not run. Whether another PR supersedes this one could not be checked because GitHub access required authentication. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Check both script inventories] --> B{Complete?}
B -->|No| C[Compare metadata and producer hashes only]
B -->|Yes| D[Match equal retained fingerprints]
D --> E[Match missing-source slots by metadata]
E --> F[Compare remaining occurrences]
C --> G[Return changed, unchanged, or unknown]
F --> G
Reviews (8) · Last reviewed commit: "fix(browser): preserve provable partial ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: d15d62edc8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| before.scripts.items | ||
| .filter( | ||
| (script) => | ||
| !incompleteSourceGroups.has(sourceGroup(script.script_key)) && |
There was a problem hiding this comment.
When either capture's scripts section is truncated, an omitted duplicate can renumber the retained _1, _2, … keys even when every retained entry has source.included = true. This filter only excludes groups with a missing retained source, so it treats those renumbered keys as comparable; for example, before sources [A,B,C] and a truncated after [B,C] compare A with B and B with C, and compareDimension reports modified even though omission is the only proven difference. Gate ordinal-based source comparison on complete inventory (or use omission-aware group matching) so these unresolved identities remain unknown.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9f7c93460
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (inventoryComplete || | ||
| script.script_key === sourceGroup(script.script_key)) && |
There was a problem hiding this comment.
When scripts is incomplete and each capture retained only one member of a duplicate metadata group, keyAndBuildInventory assigns both rows the unsuffixed base key because it counts only retained drafts, so those keys can denote different group members. The fresh evidence in the final code is this bare-key exception: it compares their source digests and reports modified even if the full inventories are identical and only truncation/admission order selected source A on one side and source B on the other. Do not treat an unsuffixed key as unique unless inventory completeness proves it.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf34bc758
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| source_sha256: | ||
| comparableSources.has(script.script_key) && script.source.included | ||
| ? script.source.artifact.sha256 | ||
| : null, |
There was a problem hiding this comment.
For complete inventories containing duplicate-metadata scripts, a real source replacement can reorder every source-sorted suffix: with digest order before [A, B] and after [B, C], keyAndBuildInventory moves unchanged B from _2 to _1, while this projection compares _1 as A versus B and _2 as B versus C, reporting two modified scripts although the source multiset changed by only one occurrence. Match retained digests within each base group before assigning residual modifications so unchanged duplicate evidence keeps its identity.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e819785e38
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const inventoryComplete = | ||
| sectionsComplete(before, ["scripts"]) && | ||
| sectionsComplete(after, ["scripts"]); |
There was a problem hiding this comment.
For schema-valid capture files where scripts.total does not equal scripts.items.length, these lines still mark the inventory complete as long as the completeness arrays omit scripts. The new source matcher then enables digest pairing: two total: 2 captures that retain A and B respectively from an otherwise identical duplicate group are reported as one modified, although the omitted row can make both full inventories [A,B]. Check retained coverage (or reject the cross-field mismatch) before enabling source comparison so this remains unknown.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90eec6a7cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sectionsComplete(inspection, ["scripts"]) && | ||
| inspection.scripts.total === inspection.scripts.items.length, |
There was a problem hiding this comment.
When a schema-valid capture has completeness.dropped_events.scripts > 0 but does not redundantly list scripts in truncated_sections, this still marks the inventory complete whenever the retained total matches the item count. Two captures can then retain different source members from the same duplicate-metadata group and be reported as modified, even though the dropped rows could make both full inventories identical. Require the per-kind dropped count to be zero before enabling source matching.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10533f576b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| inventoryComplete && | ||
| [...left, ...right].every((script) => script.source.included); |
There was a problem hiding this comment.
When a duplicate-metadata group has partially retained sources—for example, before contains two known A sources and after contains known B plus one unavailable source—this all-or-nothing check disables every digest in the group. The fallback then reports unknown for null hashes, or even unchanged for matching non-null producer hashes, although no value for the missing source can make {A,A} equal {B,?}. Match retained digest counts against the unknown slots so this provable replacement remains a change.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| eventInventoryComplete(inspection, ["scripts"]) && | ||
| inspection.scripts.total === inspection.scripts.items.length, |
There was a problem hiding this comment.
When a schema-valid capture records an omitted row only as a nonzero/null completeness.excluded entry for scripts, while its section arrays are clear, its dropped count is zero, and total === items.length, this predicate still marks the inventory complete. Two captures retaining A and B from the same duplicate-metadata group then report modified, even though the excluded rows could make both full inventories [A,B]; include explicit script exclusions in completeness (or reject the inconsistent capture) before source matching.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Summary
Fixes #3879. Compare retained script content without using capture-local duplicate ordinals as cross-capture identity.
Problem and expected behavior
Different retained sources with identical metadata and a null producer hash could report unchanged. Duplicate keys can be renumbered when scripts are omitted, so ordinal differences alone do not prove replacement. Conversely, missing source text must not hide a provable difference:
[A,A]versus[B,?]cannot describe identical content. Explicit script exclusions must limit completeness even when redundant section flags are absent.Change and scope
The shared domain comparator cancels equal known fingerprints with multiplicity, then pairs unknown sources with compatible residual metadata before comparing remaining occurrences. Original capture keys are preserved. Script inventory completeness includes nonzero or unknown script exclusions; zero counts and unrelated exclusions do not disable script comparison. Incomplete inventories retain the metadata/producer-hash path.
The existing boundary fixture helper now removes exclusion rows when it explicitly marks their sections complete. Assertions and unrelated omission evidence are preserved.
Contract and boundary impact
Expected effects:
Evidence
10533f576b, the new actual-CLI partial-source regression failed:[A,A]versus[B,?]returnedunknown / 0rather than one provable replacement; the previous 16 CLI regressions passed.changed / 1rather thanunknown / 0.1andnullprevent source matching; zero counts and unrelated exclusions retain known changes.Validation performed
Fresh on 2026-10-20, Windows x64, pinned Node 24.18.0 / npm 11.16.0:
npm run test:focused -- tests/acceptance/tools/scriptCaptureComparison.test.ts tests/boundary/browser/webCaptureDiff.test.ts— 51 passed: 22 actual-CLI tests and 29 boundary tests, zero skipped. Required runtime artifacts prepared by the repository runner; final preparation reused its valid cache, tests executed freshly.npm run test:local -- src/domain/webCaptureDiff.network.test.ts— 45 passed, zero skipped.npm run check— passed: typecheck, lint (0 errors, 1740 repository warnings), format, knip, metadata, module boundaries and architecture guards. Two of six Turbo tasks reused valid caches.git diff --check— passed.Compatibility, safety, and release
No migration, schema or dependency change. Full suite, live MCP transport, live Chromium and other operating systems were not newly verified. MCP shares the comparator; these tests establish CLI and boundary behavior. No performance improvement claimed.
Published-head CI
39251655449remains failed; the local candidate has not been published and has no new CI result. Upstream cleanup repair #5249 has merged, but this candidate does not include a new upstream merge or claim that its former CI failures are resolved.Merge Danger
Door: two-way — revert comparator changes and their regressions together.
Blast Radius: script comparison.
Author readiness
Human owner: @z2Ace0107
Before merge
type(scope): outcome.