fix(incremental): prevent silent loss of existing symbols - #683
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: bbd318925d
ℹ️ 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".
|
@codex review The follow-up preserves current inbound semantic edges through targeted repair and adds regression assertions for surviving and removed targets. It also fixes the Windows CRLF/shebang import failure, reproduced locally before the fix. Unsupported-language deletion blocking remains the explicitly documented conservative behavior required by this change's scope; see the inline reply. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b24e3818c
ℹ️ 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".
|
@codex review Addressed both second-round findings: use Git-aware source cleanliness for CRLF/clean-filter checkouts, and reconcile current inbound retry edges through unique symbol ID mappings. Added clean/dirty CRLF and three-ID-spelling integration regressions, including an independent finalize check and stale-candidate cleanup on fresh prepare. Also added bounded cleanup retries for the pre-existing Git-for-Windows ENOBUFS/EBUSY test failure seen in two CI runs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6590c4b30
ℹ️ 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".
|
@codex review Addressed the third-round finding by collecting normalized dangling-target edge candidates from fresh batches before merge cleanup, excluding batch-existing. Retry now restores edges to symbols omitted entirely from the initial output, including baseline-to-replacement ID changes. Added integration coverage for that exact loss path and a Python regression for provenance, direction normalization, and highest-weight deduplication. Targeted retry regressions and all 89 Python merge-helper tests pass locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 562dd6eccf
ℹ️ 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".
|
@codex review Addressed the fourth-round finding: current dangling candidates are reconciled on every successful merge/finalize, even when no retry is needed. Both endpoint IDs are mapped through verified symbol replacements. Added a first-pass regression changing both source and target IDs without a retry manifest and checking merge plus independent finalization. Existing repair regressions and all 89 Python merge-helper tests pass locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50c4e8c05f
ℹ️ 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".
|
@codex review Addressed the fifth-round finding: unchanged IDs/names now require preserved class ownership or unique source identity, and source-parsing selection uses the same predicate. Added same-ID owner-substitution regressions at merge and finalize, ambiguity/unsupported-parser cases, and stable-owner coverage. The 68 incremental/matching/contract tests pass. Also fixed the unrelated Windows worker progress RPC starvation by yielding between cases in two long synchronous CLI suites; their 140 applicable local tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c136f0525
ℹ️ 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".
|
@codex review Addressed the sixth-round finding by preserving current endpoint meanings separately from baseline aliases. Saved repair edges use original HEAD descriptors for both endpoints, including unchanged source files, and are deferred outside batch-0 so a reused ID cannot attach them early to another symbol. Added regressions reusing both endpoint IDs and reassigning the target ID again during repair; only the intended current call edge survives. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 577e33f6ff
ℹ️ 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".
|
@codex review Fixed the overly broad Ruby attr evidence in 3e6af36. Static attr/attr_* arguments now contribute exact reader/writer names, while unrelated genuine deletion and no-argument obj.attr no longer taint the file. Dynamic/interpolated/escaped arguments and reflection retrieving an accessor for a later call remain unresolved. The new matrix covers all accessor forms, generated writers (run=), unrelated calls and dynamic aliases; a real-repository test publishes the genuine deletion, while the existing dynamic Ruby/Python cases preserve graph/fingerprints/meta. All 56 matching tests, four relevant publication regressions, core build, changed-file lint and diff checks passed locally. The previous Linux/Windows CI matrix passed; this head has fresh CI. Please review 3e6af36. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6af3636b
ℹ️ 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".
|
@codex review Fixed quoted Ruby writer argument evidence in c130fe0. Fully resolved accessor argument subtrees are excluded from generic leaves; exact installed names replace them, so attr_writer "run" / attr_writer :"run" contribute run= without falsely preserving a deleted reader. Dynamic/escaped/interpolated arguments remain unresolved. All 60 matching tests and three real-repository deletion/publication cases passed locally, plus core build/lint/diff checks. Both prior CI jobs passed; a fresh matrix is running for this head. Please review c130fe0. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c130fe054d
ℹ️ 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".
|
@codex review Please review the integrated symbol-loss validation contract and implementation at head 3f226fa, including preservation versus verified deletion, scoped evidence, ambiguous identities, one-retry publication protection and current-edge reconciliation. The audit replaces file-wide name flags with versioned kind/owner/name evidence and adds a 108-case positive/negative source matrix plus real publication regressions. See docs/incremental/symbol-loss-validation.md for the decision rules and interpretation limits. Local verification: core build/core tests, root suite (756 passed and 4 skipped before the final two C++ qualification regressions), final matching/evidence suites (167 passed), lint, diff checks, and Python merge helpers. Linux/Windows CI is running for this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f226fa424
ℹ️ 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".
|
@codex review Please review head 53d2e1f. The integrated scope model is unchanged; this follow-up adds missing declaration evidence for class-expression methods and Ruby alias AST nodes. Both review threads have reproduction details, fixes and positive/negative matrix coverage. The originally reported class-node examples already blocked via kind:null binding evidence; the related named-expression method omission was reproduced and fixed. Local verification: 117 evidence matrix tests, 59 source matching tests, 24 strict-parser tests, core build, lint and diff checks passed. The previous head passed Linux/Windows CI; CI is running again for this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d2e1f135
ℹ️ 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".
|
@codex review Please review head acc7950. This follow-up distinguishes unbound class-expression lexical scope from unresolved external binding. Its positive/negative matrix verifies genuine class/method deletion beside unrelated expression scopes while keeping wrapper/property assignments unknown. The interpretation is documented in docs/incremental/symbol-loss-validation.md. Local build, lint and 204 related tests passed. The preceding head passed Linux CI; its Windows run was still in progress at the last check. Both platform checks are running for this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc795050d
ℹ️ 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".
|
@codex review Please review the integrated scope/evidence model at head a84b3db against docs/incremental/source-evidence-audit.md and docs/incremental/symbol-loss-validation.md. This replaces the three independent ancestry heuristics with one lexical scope index and value-region traversal, uses tagged scopes instead of null/sentinel conflation, and separates declaration coverage gaps from runtime effects. The existing baseline, repair and publication boundaries are retained. Verification includes 20 scope-composition tests, the 133-case source-evidence matrix and actual merge/finalize/repair regressions. Full core: 1,013 passed; full root: 783 passed, 4 skipped; build/lint/diff checks passed. Linux/Windows CI is running for this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84b3db5ca
ℹ️ 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".
|
@codex review Please review head e9389cf. This follow-up accounts for the dual parameter/class-member role of TypeScript parameter properties in declaration coverage, using the existing indexed receiver scope. Positive and negative TS/TSX tests cover modifiers, optional/default forms, other names/classes, ordinary parameters and nested local classes. The related 194 tests, core build, lint and diff checks passed. The integrated scope/evidence architecture and its boundaries remain documented in docs/incremental/source-evidence-audit.md and docs/incremental/symbol-loss-validation.md. Latest-head Linux/Windows CI is running; the preceding head passed Linux while Windows was still running when superseded. |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
1 participant
Summary
Incremental analysis could silently drop existing functions, classes, or methods when an analyzer under-reported a changed file. Prepare now preserves the pre-update inventory, and merge and finalization independently validate it. A fixture keeping 20 methods in source but emitting one reports all 19 omissions and refuses publication without changing the graph, fingerprints, or metadata.
The decision contract and source-model audit describe the boundary and replacement of the earlier independent ancestry heuristics.
This protects partial and architecture updates. It does not reconstruct historical omissions or guarantee initial full-analysis completeness. Automatic deletion currently requires a declaration-coverage adapter for JavaScript/JSX, TypeScript/TSX, Ruby, Python, Go, Rust or C++; other grammars remain conservative even if parsing succeeds. Empty extraction, unsupported expansion/qualification and ambiguous old identities remain unknown. The gate compares structural source identities and recognized declaration/installer syntax; it does not execute arbitrary programs or perform compiler-wide type equivalence.
Linked issue(s)
Closes #680.
How I tested this
pnpm lintandgit diff --checke9389cf; Windows passed on rerun after an existing worktree path fixture exceeded its five-second timeoutCoverage includes the 20-to-1 and equal-count omissions, actual deletion, one-retry success/failure, byte-preserved durable baselines, ID/edge reconciliation, CRLF cleanliness, legacy data directories, stale-shard cleanup, and scope-composition invariants. These invariants exercise nested fields, local installers, expression-internal name changes, shadowing, loop/catch bindings, declaration order and missing/invalid coverage profiles. Analyzer outputs are simulated; live LLM omission rates were not measured.
Linux/Windows CI additionally builds the skill and viewer and runs all suites. Tests isolate graph-discovery fallback roots and use bounded asynchronous cleanup for transient Windows Git directory locks.
Versioning