Skip to content

fix(llm): analyze oversized source lines without losing coverage - #992

Open
Patrick-Erichsen wants to merge 8 commits into
NVIDIA:mainfrom
Patrick-Erichsen:fix-oversized-source-lines
Open

Patrick-Erichsen wants to merge 8 commits into
NVIDIA:mainfrom
Patrick-Erichsen:fix-oversized-source-lines

Conversation

@Patrick-Erichsen

Copy link
Copy Markdown
Contributor

A single minified source line can exceed the serialized LLM request limit even after line-based chunking. The guard correctly refuses the request, but the analyzer cannot inspect that line. A retained 945 KB line reproduced this in three semantic analyzers.

Depends on #980; it must land first. The feature changes are isolated in commit 19d329b26f1a. This change splits only physical lines that still do not fit into bounded, overlapping source spans. It preserves every original byte, line ending, line number and Unicode-column range. An original line counts as complete only when successful spans cover every character. Failed or missing spans remain visible, and successful sibling findings survive.

The ledger emits one planned terminal result per original line. Additional distinct failure reasons/errors use existing scope-boundary records, preventing ordinary partial gaps from becoming fatal duplicate-work errors. Genuine operational failures remain fatal. Whole-file SC10 redaction and all existing request, runtime and security limits are preserved. No packet layer is added upstream.

Validation:

  • 22 source-span controls: untouched fix(llm): bound complete serialized inputs before provider calls #980 fails 21; candidate passes all 22.
  • 18 terminal-accounting controls: all fail before the correction and pass after, including actual generic/meta aggregation paths.
  • 593 relevant core, meta, finalizer, ledger and source tests pass.
  • Three retained-source planner replays reconstruct every byte and pass the unchanged serialized guard; no original code or models executed.
  • make lint, make format-check and git diff --check pass.

Dependency #980's unit job still fails 15 tests. All 15 also reproduce on exact upstream base e14c450b260958ed3ebd83c94c09c4b093e3e656 and CI merge 6139f3655b283b8a33844faf125933f7d20f0352, with identical failing static code/tests: 12 stale PowerShell preconditions, 1 stale other-languages expectation and 2 multiline-printf completeness cases. This paired evidence demonstrates no #980 regression for those failures; required CI remains unwaived.

Independent shared review of this exact six-file candidate passed at P0–P3 in two complete passes with no findings. Full CI and dependency #980 remain required before landing. More spans use the existing shared runtime budget; these local tests do not prove a real model scan will finish within every package's budget.

Generated-source regression controls can be run without API keys or models:

python -m pytest -q tests/nodes/test_llm_source_fragments.py tests/nodes/test_meta_source_fragments.py tests/nodes/test_fragment_terminal_ledger.py

Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
The redaction test bypassed constructor state required by serialized token accounting. Construct it with the existing inert provider fixture while retaining credential redaction assertions.

The formatting-limit test set the input budget before the initial request, so the new submission guard correctly stopped before formatting. Reduce the budget after the successful mocked response and prove formatting refuses a second request.

Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>

@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 Review]

Decision: Approved

Value and readiness

This addresses the remaining oversized-line gap after #980. It is ready for maintainer/PIC review: source spans retain original line/Unicode-column provenance, successful siblings keep their findings, and completion requires coverage of the whole original line. This is a stacked change and #980 must land first.

Material findings

No required changes identified. The complete change includes #980's prompt accounting plus bounded overlapping physical-line spans in src/skillspector/llm_analyzer_base.py:1582-1629. Progress is explicit, line endings are retained on the final span, irreducible metadata remains an unsubmitted failure, and fragment responses cannot invent a different line. _project_line_fragments reconciles character coverage before projecting line work. _consolidate_fragment_ledger_events emits one terminal planned row per original line and retains distinct failure facts as scope boundaries, preserving operational failures and avoiding duplicate planned-work errors. The meta path distinguishes span identities, accounts for missing/unknown returns, retains original findings for incomplete lines, and keeps whole-file SC10 redaction outside column provenance. New controls exercise Unicode/CRLF/no-final-newline reconstruction, overlap boundaries, failed/missing spans, sibling findings, normal generic/meta aggregation, mixed error reasons, final ledger status and SC10 credentials crossing prospective boundaries.

PIC tradeoffs

More spans consume the existing shared runtime/provider budget. Full source submission is not proof that every package will complete, or that an LLM can reconstruct arbitrary long-distance dependencies from overlapping spans. Redacted SC10 files deliberately keep oversized lines whole and fail closed to preserve credential safety and truthful provenance. No new packet layer, public ledger schema or runtime dependency is introduced.

Verification and gaps

Reviewed the complete ten-file stacked diff and exact-head planner, run loops, generic/meta aggregation, public ledger/finalizer contracts and regression assertions at 795eb28c30f2b26c253da5a4fb997ddd98995412, its #980 dependency and #977 context, and all available reviews/discussion/threads (none). Hosted test-unit reports 17,866 passed and 15 failed. All 15 failure identities match the independently fetched main e14c450 baseline run 41513800480. Five other checks succeeded. Red CI and the unmerged #980 dependency still block landing; they are not waived by this approval. No contributor code/tests/builds/imports/hooks or regexes were executed locally, and no real model or retained package replay was performed in this review.

Preserve main response recovery and failed-request diagnostics. Guard the
new raw recovery calls with the same actual-client serialized limits.
Add sync/async retained-evidence and diagnostic-wrapper regression cases.

Validation: independent static review and git diff --cached --check.
Contributor code and tests were not executed. Hosted CI is required.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Preserve newer response recovery, partial warnings and failed-request
diagnostics while projecting fragments to their original source lines.
Recovered fields retain evidence without proving complete line coverage.
Retain conservative meta filtering until fragmented source is complete.
Add warning/provider-failure regression cases for semantic and meta paths.

Validation: independent static review, AST parsing as data, Ruff lint and
format checks, and git diff --cached --check. Contributor code and tests
were not executed. Fresh hosted CI remains required.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

@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 Review]

Decision: Approved

Value and readiness: The resolved stack retains the oversized-line source spans and exact original-source accounting while preserving current main's recovered evidence, warnings and diagnostics. No required source correction remains after independent integration review. #980 must be confirmed merged before this PR.

Material findings: The branch now contains the resolved #980 commit and current main. Fragment projection retains recovered values alongside their warning/failure, while only successful spans establish full original-line coverage. Recovery warnings and distinct runtime/provider facts survive consolidation without duplicate original-line work items. Main's fatal SYSTEM accounting is preserved. Meta return reconciliation retains canonical recovered/warning records rather than fabricating MissingBatchResult. Ordinary nonfragment per-item recovery retains main's identity checks; incomplete fragmented source cannot qualify to discard an original finding until the complete successful source coverage is proven. Eight new integration cases cover generic retained HIGH recovery, warning/provider failures, mixed reason facts, canonical meta evidence, and the real planner/meta path refusing to discard original HIGH findings after partial recovery. The old terminal test now distinguishes its single original-line WORK_ITEM from preserved fatal SYSTEM accounting, retaining uniqueness and no-UNACCOUNTED assertions. No code blocker identified.

PIC tradeoffs: Additional spans still consume the shared runtime budget. Incomplete fragmented meta context stays conservative even when an individual recovered assessment claims a false positive; ordinary batch partial recovery remains available. This preserves the PR's honest-coverage goal without adding a new policy or output schema. Dependency #980 remains an explicit merge gate.

Verification and gaps: Read the complete feature delta against resolved #980 and the recovery integration, source-span planning/provenance, ledger projection, meta filtering, and relevant regression assertions. Independent review compared unchanged base methods and safe diagnostics/retry paths; static AST, Ruff lint/format and Git whitespace checks passed. No contributor application code, tests, imports, setup, hooks, scanner, model or regex probes executed. At the review inventory five hosted checks succeeded and test-unit was still running. New-head hosted CI is required; neither old local replay claims nor prior-head CI establish this integration's runtime result.

Reviewed head: 8412390d784b7fa80f4c2dea38e27523440b0552.

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