Skip to content

fix: address full code review findings - #1

Merged
lukoplt merged 1 commit into
mainfrom
fix/review-findings
Jul 18, 2026
Merged

fix: address full code review findings#1
lukoplt merged 1 commit into
mainfrom
fix/review-findings

Conversation

@lukoplt

@lukoplt lukoplt commented Jul 18, 2026

Copy link
Copy Markdown
Owner

What & why

Zapracovává všechny nálezy z opakovaného code review a doplňuje regresní pokrytí rizikových cest.

Rename correctness

  • Zachovává YAML komentáře, scalar styles a nesouvisející formátování.
  • Přejmenovává definice named formulas a context variables i jejich skutečné reference.
  • Nechá beze změny string literals, record fields a dotted field access.
  • Po selhání obnoví původní soubory a po úspěchu validuje definice i staré reference.

Analysis and security

  • Rozlišuje stejně pojmenované controls a context variables podle scope v diffu, grafu, dead-code a impact analýze.
  • Zachovává správnou detekci přesunů unikátních controls mezi screens.
  • Odstraňuje false positives secrets na platných formulích a zachovává raw/entropy fallback pro neplatné formule a literal properties.
  • Kontroluje všechny secret matches a ignoruje prázdné allowlist položky.

Cancellation and release reliability

  • Ctrl+C se zachytává jen během operací, které cancellation token skutečně sledují, a vrací exit code 130.
  • pac detection, unpack a pack propagují cancellation; child process se po kill korektně drainuje.
  • Temp adresáře se uklidí i při chybě unpacku.
  • GitHub release zůstává draft, dokud neproběhne NuGet publish; finalize používá explicitní repository context.

Type

  • New rule (PAxxxx)
  • Rule/heuristic change
  • New command / flag
  • Bug fix
  • Docs / fixtures / CI
  • Other

Test coverage

  • Changed-path line coverage: 265/328 = 80.79% (ship gate passed)
  • Global coverage: 61.89% line / 59.98% branch
  • Tests: 123 passed, 0 failed
  • Added 21 focused regression scenarios covering rename, scope-aware analysis, secrets, cancellation, process timeout, pac detection and SourcePreparer cleanup.

Remaining integration gaps are the real external pac round-trip without EASEL_TEST_MSAPP, solution ZIP cleanup failure paths, and live GitHub release finalization.

Pre-landing review

Independent security, maintainability, testing, performance and adversarial re-review: no remaining findings.

Five findings discovered during the first ship review were fixed before commit and received dedicated regression coverage.

Plan completion

No relevant plan artifact was found; audit skipped.

Documentation

  • docs/rename.md now describes identifier-aware renaming, supported symbol kinds, preserved YAML formatting and intentionally unchanged string literals.
  • No additional documentation changes were required.

Checklist

  • dotnet build is warning-clean (0 warnings, 0 errors)
  • dotnet test passes (123/123)
  • dotnet format --verify-no-changes passes
  • New/changed behaviour has regression tests
  • NuGet pack succeeds
  • Self-contained osx-arm64 publish succeeds
  • Published binary smoke test reports 0 findings on CleanApp
  • release.yml parses as valid YAML
  • Docs updated where behaviour changed
  • Output stays deterministic
  • Read-only behaviour is preserved; rename operates on its temporary unpacked copy

Notes for reviewers

Please focus on exact YAML source-span handling in RenameEngine, scoped control identity in DiffEngine, and formula-vs-literal handling in the secrets pipeline.

Preserve YAML during symbol renames, make analysis scope-aware, harden secret detection and cancellation, and prevent partial public releases. Add regression coverage for the corrected edge cases.
@lukoplt
lukoplt merged commit 4b46b13 into main Jul 18, 2026
1 check passed
@lukoplt
lukoplt deleted the fix/review-findings branch July 18, 2026 22:39
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