Skip to content

fix(analyzers): retain findings with unavailable Python AST columns - #623

Merged
rng1995 merged 50 commits into
mainfrom
codex/fix-missing-python-ast-columns
Oct 5, 2026
Merged

rng1995 merged 50 commits into
mainfrom
codex/fix-missing-python-ast-columns

Conversation

@mohgupta-ship-it

@mohgupta-ship-it mohgupta-ship-it commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Powered by Codex, on behalf of the repository contributor.

When Python AST column metadata is null or absent, behavioral finding emission can raise TypeError. Separate syntax nodes on the same line can also lose occurrences during report compaction because their incomplete locations and fallback source text are identical. This is an instrumented library robustness case; ordinary ast.parse supplies columns.

Preserve unknown columns as None, retain available Unicode endpoints and source evidence, and suppress repeated taint flows by rule and sink node. When either column is unavailable, carry a deterministic AST walk index in existing evidence metadata so distinct nodes survive compaction and JSON/SARIF reporting. Normal-column evidence, semantic fingerprints, baseline fingerprints, and resource limits retain their existing behavior. This hardens the locations introduced in #409 and the reporting contract in #584.

Validation at c823a2bfdac4d0fd1bc9d16f3002585acef8a719:

  • 431 focused tests pass, with one MCP integration test deselected. The optional-column suite has 74 cases, covering both analyzers, all normal/null/absent endpoint pairs, Unicode/multiline spans, identical/different payloads, reparsing, compaction roundtrips, fingerprints, scores, JSON, and SARIF.
  • Independent reviewers passed 45 additional shape/endpoint probes and resource-limit probes. A separate integration check passed six fresh metadata/report cases comprising 17 variants, including intact-column controls.
  • Changed-file lint, formatting, diff checks, and signoff pass. Three specialists and a separate evidence-bounded judge found no source blocker; rating bugfix, source disposition READY for maintainer review.
  • Fresh hosted CI is running for this head. No completed full-suite result is claimed for this follow-up yet.

Historical validation at parent c46832f: the full local suite completed with 7,897 passing tests and all six hosted checks passed. An unchanged 48-case replay retained all 41 baseline passes, with seven known unrelated failures and no normalized CLI report changes; all eight security smoke and three full-size stress checks passed. These results belong to the parent, not the new head.

AST walk indices are deterministic for unchanged source and parsing runtime, not permanent identifiers across source/parser changes. Invalid non-integer columns and missing line metadata remain outside scope.

Signed-off-by: Mohit Gupta <mohgupta@nvidia.com>
@mohgupta-ship-it
mohgupta-ship-it marked this pull request as ready for review September 24, 2026 14:01
@mohgupta-ship-it

mohgupta-ship-it commented Sep 24, 2026 •

Copy link
Copy Markdown
Member Author

Powered by Codex, on behalf of the repository contributor: updated council review of c823a2bfdac4d0fd1bc9d16f3002585acef8a719 against c7958a3268d9498644b22edb75d0f051bbc8cbfc.

This supersedes the earlier review of c46832f. The previously disclosed same-line occurrence loss is now fixed: a deterministic AST node index in existing evidence distinguishes nodes with incomplete columns through compaction and JSON/SARIF output. No source coordinates are invented. Semantic and baseline fingerprints, ordinary known-column evidence, raw scoring, and analyzer resource bounds remain intact.

Three independent specialists and a separate evidence-bounded judge found no actionable source blocker. Rating: bugfix; scoped source disposition: READY for maintainer review. This is not maintainer approval. Fresh hosted checks remain in progress, so the CI disposition is BLOCKED_BY_CI_OR_CONFLICT pending completion; no conflict is reported.

Exact-head evidence: 431 focused tests pass, including 74 optional-column cases; one MCP integration test was deselected. Independent shape/endpoint probes, budget-accounting probes, and a separate six-case/17-variant integration check pass. All 17 emitter calls were checked after the final refactor. Lint, formatting, diff checks, and signoff pass. Two instrumented same-line taint calls retain two JSON/SARIF results after compaction; their score is 67, matching normal metadata, rather than the prior collapsed score of 45.

Historical correction: the parent head's full local suite subsequently completed with 7,897 passes, and all six hosted checks passed. Its 48-case replay retained 41 baseline passes plus seven existing failures, with unchanged normalized reports; eight smoke and three stress checks passed. Those results do not certify the new head. No new critical-gap escalation is supported, and no merge or release was performed.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 1813de8e18b9b733bdbcf8fde5a70ed5e01e607f. I found no actionable correctness issue in this change.

I checked nullable/absent endpoint handling, UTF-8-to-character conversion, reflective subscript findings, repeated taint flows into one sink, separate same-line AST nodes, deterministic occurrence evidence, report compaction, JSON/SARIF locations, and semantic/baseline fingerprint behavior. The evidence index is confined to incomplete spans, and node-based taint deduplication retains the intended per-rule/per-sink behavior.

Validation on this head: 324 targeted tests passed across the optional-column, behavioral AST, taint-tracking, shared-parser/cache, Python AST, and deduplication suites. Nine additional reflective-subscript probes covering all normal/null/absent start/end combinations also passed. No full-suite run was performed as part of this review.

At review time, CI for this exact head reports action_required; successful repository CI is still required before merge. This review does not constitute combined 2.12.1 release qualification.

Review performed with Codex on behalf of Narendran Raghavan.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Second-Pass Review]

Independently re-reviewed current head e5c736f557dc919b591a5f356398bd7c84468ec7. The PR-specific changes remain identical after the latest main merges. I found no actionable code issue in the nullable-column paths, per-node taint identity, Unicode source mapping, or occurrence/report compaction.

On this exact head, all 200 independent source/analyzer/endpoint probes passed, covering mixed endpoint availability within one tree, nested and separate same-line calls, reflective/deserialization/taint sink shapes, Unicode/CRLF, deterministic reparsing, and preserved compaction identity. Another 114 tests passed across optional columns, shared-AST/cache, Python parsing, and the newly merged prepared static-runner context. This supplements the earlier focused review; it does not claim a full-suite run.

Approved for the reviewed code. Hosted CI and any subsequent source changes still require validation on the resulting commit.

Review performed with Codex on behalf of Narendran Raghavan.

@rng1995
rng1995 merged commit e3e268a into main Oct 5, 2026
6 checks passed
@rng1995
rng1995 deleted the codex/fix-missing-python-ast-columns branch October 5, 2026 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants