Skip to content

docs: SAST complexity refactors can drop uncovered branches#65

Merged
CybotTM merged 2 commits into
mainfrom
feat/retro-sast-refactor-branch-safety
Jun 27, 2026
Merged

docs: SAST complexity refactors can drop uncovered branches#65
CybotTM merged 2 commits into
mainfrom
feat/retro-sast-refactor-branch-safety

Conversation

@CybotTM

@CybotTM CybotTM commented Jun 27, 2026

Copy link
Copy Markdown
Member

Summary

Adds one learning from a cross-session retrospective (2026-06-27) to references/static-analysis-tools.md: SAST maintainability/complexity findings — SonarCloud S3776 (cognitive complexity) and S1142 (too many returns) — whose fix extracts or reorders methods are behavior-risky. An extraction can silently drop a branch (e.g. a fallback-to-default path on an empty/invalid response).

The key insight: PHP-CS-Fixer, PHPStan, and a passing unit suite will not catch a dropped branch if that path isn't unit-covered — in a real 2026-06-27 cleanup only a human code reviewer did.

Guidance added

  • Before trusting green gates on a complexity refactor, diff the control-flow branches of the refactored method against the original (is every return/else/early-exit preserved?).
  • Add a unit test for the previously-uncovered branch FIRST, then refactor.
  • Often it's better to accept subjective complexity findings on controller/service/command layers (mark won't-fix with a comment) than perform a risky extraction.

Placement

New section appended to the existing SAST reference skills/php-modernization/references/static-analysis-tools.md, which is already linked from SKILL.md (Reference routing → "Static analysis"). Matches the file's existing narrative "hazard" style with a seen-in-practice anecdote. SKILL.md is untouched (no bloat, stays under its ~500-word ceiling).

Version decision

No version bump. This is a references-only addition to an already-linked file; SKILL.md, plugin.json, and composer.json are unchanged, so the check-version-parity hook stays green (all three remain at 1.19.1).

Validation

Local pre-commit run on the changed file: markdownlint-cli2 Passed, validate-skill (all files) Passed, check-version-parity Passed, plus trailing-whitespace/end-of-file/merge-conflict/large-file checks Passed. Commit is signed (ED25519) with Signed-off-by.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Copilot AI review requested due to automatic review settings June 27, 2026 11:11
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot added documentation Improvements or additions to documentation skill labels Jun 27, 2026
github-actions[bot]
github-actions Bot previously approved these changes Jun 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new section to the static analysis tools documentation, warning about the risks of silently dropping branches during SAST maintainability refactors. The feedback suggests refining the advice on diffing control-flow branches to focus on tracing logical paths rather than counting return statements, as reducing return statements is often the goal of such refactors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread skills/php-modernization/references/static-analysis-tools.md Outdated
…actors

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
@CybotTM CybotTM merged commit 792693d into main Jun 27, 2026
20 checks passed
@CybotTM CybotTM deleted the feat/retro-sast-refactor-branch-safety branch June 27, 2026 12:24
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants