Skip to content

P/umadevis1/improve remediations - #1089

Open
umadevis1 wants to merge 21 commits into
dev/v3.xfrom
p/umadevis1/improve_remediations
Open

P/umadevis1/improve remediations#1089
umadevis1 wants to merge 21 commits into
dev/v3.xfrom
p/umadevis1/improve_remediations

Conversation

@umadevis1

@umadevis1 umadevis1 commented Sep 5, 2026

Copy link
Copy Markdown

PR updated with all the changes needed.

@umadevis1

umadevis1 commented Sep 10, 2026

Copy link
Copy Markdown
Author

This PR has a set of fixes that improve the remediation process. But this does not guarantee that remediation will always work yet due to multiple nuances to overcome. As a part of fixing this, the PR also rearchitects the RemediationProcessor to separate classes to pave way for more fixes.
This PR is tested with a set of FPRs and has seen significant improvement in number of remediations done.
The unit testcases have been updated to cover all the scenarios.
1 — Strip synthetic markers + blank lines (FileUtil.stripSyntheticLineMarkers, trimBlankLines). fcli injects // L37-style markers into every line before sending code to the LLM; these get stripped from the proposed fix before it's written to remediations.xml, along with leading/trailing blank lines. This is the doc's stated prerequisite for near-identical detection — it converts marker-driven near-duplicates into byte-identical ones.

P2 — Canonical hash, both sides (FileUtil.canonicalizeForHash, AuditProcessor.calculateHashBase64, RemediationProcessor.applyChange). Canonical form = normalize line endings to LF + strip trailing newline, applied identically on the audit side (writing the hash) and apply side (checking it). For transition compatibility (per the doc's explicit requirement — a corrected apply side must still work against already-generated remediations.xml), apply tries 4 hash forms per hunk: canonical/legacy × UTF-8/FVDL-declared-source-encoding, accepts any match, and logs which form matched.

3 — Identity dedup at hunk level, INCLUDING near-identical (whitespace/comment) matching. Before attempting to place a hunk, fcli checks whether its target content already matches what a prior remediation in this run wrote. A remediation is IDENTICAL if all its hunks are already satisfied this way; APPLIED if any hunk is genuinely new. This is what turns "skipped, no reason" into "identical to <instanceId>, applied once" — the doc's single largest unexplained-skip bucket (6/13, 6/14 on the two in-scope apps).

The identity key is built from createComparisonCode not the raw proposed text — so the comparison is already whitespace- and comment-insensitive, per language: it strips all whitespace (replaceAll("\\s+", "")) and strips comments using a per-language comment-symbol lookup (LanguageCommentMapperUtil/LanguagesCommentConfig//+/* */ for C-style, # for script languages, <!-- -->/<%-- --%> for markup/JSP), on top of the literal-alias normalization below. This means the doc's "near-identical fixes" case (whitespace/comment differences, which it calls language-specific R&D) is already covered by the same mechanism that handles exact identity — not a separate unimplemented feature.

4 — Patch-style application with an offset map. This is the core structural change, replacing "always text-search":

  • Map<Path, List<AppliedChange>> appliedByFile — per-file, per-run offset map. Each successful hunk records AppliedChange(instanceId, originalLineFrom, originalLineTo, deltaLines).
  • Remediations are processed widest-first (sorted by max hunk width descending), so a broad fix is recorded before narrower ones that might overlap it.
  • Each new hunk's declared line range is projected through the accumulated deltas already recorded for that file this run, instead of being searched for blind.
  • The projected range is then classified against the offset map: fully nested inside a prior AppliedChangeSUPERSEDED (no write needed, already covered); partially overlapping → CONFLICTS_WITH_ANOTHER_FIX (reported by instanceId + overlapping lines, not guessed); no overlap → verify the expected original code at the projected position and apply, or report ANCHOR_DOES_NOT_MATCH if it doesn't match.
  • Text search is now the fallback only — used solely for files genuinely untouched this run, where the hash mismatched.
  • Output model: applied / identical / superseded / skipped as four separate counters, with skippedByReason (Conflicts, Anchor does not match, ambiguous, source drift) explaining every remaining skip — nothing is silently dropped.

5 — Multi-file remediation atomicity (landed this session, on top of the above). One <Remediation> can bundle <FileChanges> for several files. Previously, if the first file's block threw (e.g. anchor mismatch), the whole remediation aborted — silently dropping a valid fix for a sibling file in the same bundle. Fixed to isolate per-file failure and let siblings still commit; a companion bug this exposed (a failed sibling's hunk getting wrongly marked "already applied," which would then cause a legitimate duplicate remediation for that same fix to be wrongly skipped) was fixed alongside it via a PreparedFileChanges record that returns the actual set of keys satisfied, not "all or nothing."

Literal-alias normalization ('\0'0, nullptrNULL) layered on top of the comment/whitespace-stripped comparison code, so functionally-identical C c

Boundary Token De dup Change: in applyChange(), right before NewCode's lines are spliced into the file, a new
dropDuplicatedBoundaryTokens(...) call compares the line immediately before the declared
lineFrom and immediately after lineTo (in the original file) against NewCode's first/last
line. On an exact, whitespace-normalized match, that duplicate boundary line is dropped from
NewCode before insertion — fixing the confirmed compile-breaking duplication bug (e.g. extra
closing brace in ProductController.java/APM0002320). Guarded so NewCode can never be trimmed
to zero lines, and blank-line matches are ignored (a blank line isn't a boundary token).

Exact Line disambiguation ChangedfuzzySearchContext() and fuzzySearchOriginalCode() now take the FPR's declared
LineFrom/LineTo, projected through this run's offset ledger (the shift calculation
previously computed only inside the offset-projection block is now computed unconditionally in
applyChange() and passed down). When either method's underlying fuzzy search finds more than one
candidate, it no longer throws SOURCE_CONTEXT_AMBIGUOUS/ORIGINAL_CODE_AMBIGUOUS immediately —
it first checks whether exactly one candidate's position exactly equals that projected/declared
position. If so, that candidate is used. If zero or more than one candidate coincide with it,
behavior is unchanged: still throws ambiguous, same as before. Exact-equality check against
data already trusted elsewhere in this class (the same ledger that powers hash-match/offset-
projection) consistent with the project's no-heuristic rule.

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.

1 participant