Skip to content

Chore: resolve aislop findings across codebase - #342

Merged
tykeal merged 1 commit into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:fix/aislop-basedpyright-findings
Jul 27, 2026
Merged

Chore: resolve aislop findings across codebase#342
tykeal merged 1 commit into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:fix/aislop-basedpyright-findings

Conversation

@ModeSevenIndustrialSolutions

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Eliminates all aislop code-quality findings and adds the aislop quality
gate as a pre-commit hook (basedpyright was already configured and remains
clean). All changes are behaviour-preserving; the full test suite,
ruff, basedpyright and mypy all pass, and aislop ci reports score 100.

Substantive ai-slop findings

  • sitecustomize.py: convert three best-effort except Exception: pass
    swallows to log via the existing gated _dbg helper (observability), and
    suppress the gated stderr debug print (a startup-hook debug logger, not
    leftover debug) with an inline rationale.
  • gerrit_ssh.py: include the caught exception in the temp-dir cleanup debug
    log so the failure cause is no longer lost (silent-recovery).
  • gerrit_urls.py: file-level suppress hardcoded-url false positives — the
    module builds Gerrit URLs from a runtime host, so the https:// literals are
    URL schemes in f-strings, not hardcoded environment endpoints.

Tooling

  • Add the aislop local pre-commit hook and .aislop/config.yml
    (maxFileLoc: 900, failBelow: 100); skip aislop and gha-workflow-linter
    on pre-commit.ci (network sandbox).

Complexity refactors (behaviour-preserving)

Decompose every over-long function (>80 LOC) and flatten every deeply nested
block (>5 levels) into cohesive private helpers across core.py, cli.py,
gerrit_pr_closer.py, duplicate_detection.py, external_api.py,
gerrit_urls.py, reconcile_matcher.py, config.py, ssh_discovery.py,
ssh_agent_setup.py, commit_normalization.py and
orchestrator/reconciliation.py. Cohesive parameter groups were bundled into
small frozen dataclasses / NamedTuples to avoid trading the findings for
too-many-params.

Review feedback & CI fixes

  • gerrit_urls._location_to_path (Copilot): treat a relative redirect
    Location without a leading slash (e.g. r/dashboard/self) as absolute from
    root, so it is not folded into the netloc and the base-path segment survives.
  • ssh_discovery._ensure_keyscan_available (Copilot): let the
    "ssh-keyscan not found" SSHDiscoveryError propagate instead of being
    re-wrapped as a generic "check failed" error, preserving the actionable
    message.
  • gerrit_pr_closer._apply_pr_close_action (Copilot): describe the closed
    change using its real Gerrit status (merged / open / abandoned) rather than
    always logging "abandoned", which was inaccurate for MERGED/NEW/UNKNOWN.
  • Grype dependency audit (CI red): bump the transitive setuptools pin
    80.9.0 -> 83.0.0 to clear GHSA-h35f-9h28-mq5c (Medium).
  • Added regression tests for the three review fixes.

File size

maxFileLoc is calibrated to 900 to fit this codebase's cohesive
single-responsibility modules (19 files in the 450–900 range are normal for the
domain). The three orchestration god-files (core.py, cli.py,
gerrit_pr_closer.py) carry documented in-file file-level suppressions: their
long functions and deep nesting are decomposed here, but a full module/class
split is a large, dedicated follow-up refactor tracked separately.

Validation

  • uv run pytest — full suite passes (added regression tests for the fixes)
  • ruff check / ruff format --check (v0.15.21, matching the pinned hook) — clean
  • basedpyright — 0 errors, 0 warnings
  • mypy — clean
  • aislop ci — score 100, exit 0

Note: this is a large, mechanical refactor produced with careful
behaviour preservation and validated against the comprehensive test suite.
Opened as draft pending Copilot review, green CI, and manual review — the
per-file helper extractions are worth a careful eyeball, especially the
SSH/config parsing paths.

Copilot AI review requested due to automatic review settings July 24, 2026 15:40
@github-actions github-actions Bot added the chore Code chores (dependency updates, etc) label Jul 24, 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.

Pull request overview

This PR removes all reported aislop code-quality findings by performing behavior-preserving refactors across the Python codebase, and adds aislop as a local pre-commit quality gate with a repository-specific .aislop/config.yml.

Changes:

  • Add aislop configuration and a local pre-commit hook, and skip it on pre-commit.ci due to sandbox/network constraints.
  • Refactor multiple large functions into smaller helpers (SSH discovery/agent cleanup, reconciliation orchestration/matching, duplicate detection, config preprocessing, API retry framework) while preserving behavior.
  • Improve diagnostics in a few paths (e.g., temp-dir cleanup logging, startup-hook debug behavior).

Reviewed changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/github2gerrit/ssh_discovery.py Extracts keyscan/connectivity/output-validation helpers for host key discovery.
src/github2gerrit/ssh_agent_setup.py Factors secure temp-file overwrite and agent cleanup into helpers.
src/github2gerrit/reconcile_matcher.py Decomposes reconciliation into helper passes and Change-Id assignment steps.
src/github2gerrit/orchestrator/reconciliation.py Extracts similarity-threshold computation and reconciliation sub-paths into helpers.
src/github2gerrit/gerrit_urls.py Adds file-level aislop suppression and refactors base-path discovery into helpers/constants.
src/github2gerrit/gerrit_ssh.py Improves debug logging by including the caught exception during cleanup.
src/github2gerrit/gerrit_pr_closer.py Refactors PR-closing and Gerrit-abandon flows into cohesive helpers and small dataclasses.
src/github2gerrit/external_api.py Splits transient-error classification and retry handling into smaller composable helpers; adds default retry-policy resolver.
src/github2gerrit/duplicate_detection.py Extracts normalization, query building, candidate scoring, and reporting into helpers/NamedTuples.
src/github2gerrit/config.py Refactors INI preprocessing for multiline quoted values into dedicated helper functions.
src/github2gerrit/commit_normalization.py Extracts release-drafter parsing and rule application into helper methods.
src/github2gerrit/cli.py Large mechanical refactor: splits main()/processing paths into many helpers, including bulk and single-PR flows.
sitecustomize.py Converts silent exception swallows into gated debug logging and adds an inline aislop suppression for the gated print.
.pre-commit-config.yaml Adds a local aislop hook and updates pre-commit.ci skip list/documentation.
.aislop/config.yml Introduces the aislop quality-gate configuration (maxFileLoc/failBelow) and rationale.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/github2gerrit/gerrit_urls.py Outdated
Comment thread src/github2gerrit/ssh_discovery.py
Comment thread src/github2gerrit/gerrit_pr_closer.py
Copilot AI review requested due to automatic review settings July 26, 2026 15:01
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the fix/aislop-basedpyright-findings branch from 6ece011 to 531574d Compare July 26, 2026 15:01

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.

Pull request overview

Copilot reviewed 17 out of 20 changed files in this pull request and generated 2 comments.

Comment thread src/github2gerrit/ssh_discovery.py
Comment thread src/github2gerrit/ssh_agent_setup.py
Copilot AI review requested due to automatic review settings July 26, 2026 15:42
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the fix/aislop-basedpyright-findings branch from 531574d to 90355a9 Compare July 26, 2026 15:42

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.

Pull request overview

Copilot reviewed 17 out of 20 changed files in this pull request and generated 4 comments.

Comment thread src/github2gerrit/config.py Outdated
Comment thread src/github2gerrit/cli.py
Comment thread src/github2gerrit/cli.py
Comment thread src/github2gerrit/gerrit_urls.py
Eliminate all aislop code-quality findings and add the aislop quality
gate as a pre-commit hook (basedpyright was already configured and
remains clean). All changes are behaviour-preserving; the full test
suite, ruff, basedpyright and mypy all pass.

Substantive ai-slop findings:
- sitecustomize.py: convert three best-effort `except Exception: pass`
  swallows to log via the existing gated `_dbg` helper (observability),
  and suppress the gated stderr debug `print` (a startup-hook debug
  logger, not leftover debug) with an inline rationale.
- gerrit_ssh.py: include the caught exception in the temp-dir cleanup
  debug log so the failure cause is no longer lost.
- gerrit_urls.py: file-level suppress hardcoded-url false positives —
  the module builds Gerrit URLs from a runtime host, so the "https://"
  literals are URL schemes in f-strings, not hardcoded endpoints.

Tooling:
- Add the aislop local pre-commit hook and .aislop/config.yml
  (maxFileLoc 900, failBelow 100); skip aislop and gha-workflow-linter
  on pre-commit.ci (network sandbox).

Complexity refactors (behaviour-preserving):
- Decompose every over-long function (>80 LOC) and flatten every
  deeply nested block (>5 levels) into cohesive private helpers across
  core.py, cli.py, gerrit_pr_closer.py, duplicate_detection.py,
  external_api.py, gerrit_urls.py, reconcile_matcher.py, config.py,
  ssh_discovery.py, ssh_agent_setup.py, commit_normalization.py and
  orchestrator/reconciliation.py. Cohesive parameter groups were
  bundled into small frozen dataclasses/NamedTuples to avoid trading
  the findings for too-many-params.

File size:
- Calibrate maxFileLoc to 900 to fit this codebase's cohesive
  single-responsibility modules. The three orchestration god-files
  (core.py, cli.py, gerrit_pr_closer.py) carry documented in-file
  file-level suppressions: their long functions and deep nesting are
  decomposed here, but a full module/class split is a large, dedicated
  follow-up refactor tracked separately.

Review feedback and CI fixes:
- gerrit_urls._location_to_path: treat a relative redirect Location
  without a leading slash (e.g. "r/dashboard/self") as absolute from
  root, so it is not folded into the netloc and the base-path segment
  is no longer lost.
- gerrit_urls._discover_base_path_for_host: cache the discovered base
  path (including the empty string for the 200-OK "no base path" case)
  so subsequent lookups do not re-probe the network.
- ssh_discovery._ensure_keyscan_available: let the "ssh-keyscan not
  found" SSHDiscoveryError propagate instead of re-wrapping it in the
  broad handler as a generic "check failed" error, preserving the
  actionable message.
- ssh_discovery._raise_keyscan_command_error: log the return code with
  %s rather than %d; CommandError.returncode is int | None, and %d on
  None would raise TypeError and mask the original failure.
- ssh_agent_setup._overwrite_file_securely: flush the file's userspace
  buffers before os.fsync, so the secure-overwrite bytes actually reach
  disk rather than lingering in buffers.
- config._sanitize_ssh_key_content: strip embedded whitespace (not just
  leading/trailing) from base64 body lines so wrapped copy-paste keys
  are repaired, matching the documented intent.
- cli._apply_bool_env_overrides: take an explicit (value, env_var)
  mapping instead of assuming the env var is param_name.upper(), so
  prefixed variables can be handled without silent drift.
- gerrit_pr_closer._apply_pr_close_action: describe the closed change
  using its real Gerrit status (merged/open/abandoned) rather than
  always logging "abandoned", which was inaccurate for MERGED/NEW/
  UNKNOWN closures.
- Bump the transitive setuptools pin 80.9.0 -> 83.0.0 to clear the
  Grype dependency-audit finding (GHSA-h35f-9h28-mq5c, Medium).
- Add regression tests for the review fixes above.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
Copilot AI review requested due to automatic review settings July 26, 2026 16:17
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the fix/aislop-basedpyright-findings branch from 90355a9 to 5621bd3 Compare July 26, 2026 16:17

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.

Pull request overview

Copilot reviewed 18 out of 21 changed files in this pull request and generated no new comments.

@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions marked this pull request as ready for review July 27, 2026 11:51
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions requested a review from a team July 27, 2026 11:51
@tykeal
tykeal merged commit e06d6a2 into lfreleng-actions:main Jul 27, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Code chores (dependency updates, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants