diff --git a/.github/scripts/agents-guard.js b/.github/scripts/agents-guard.js index 4d67953ed..fddae9c0a 100644 --- a/.github/scripts/agents-guard.js +++ b/.github/scripts/agents-guard.js @@ -483,7 +483,6 @@ function evaluateGuard({ let instructions = []; const touchedFilesText = summarizeTouchedFiles(relevantFiles); if (blocked) { - instructions = []; if (fatalViolations.length > 0) { instructions.push('Restore the deleted or renamed workflows. These files cannot be moved or removed.'); } @@ -502,7 +501,7 @@ function evaluateGuard({ commentBody = [ marker, - '**Health 45 Agents Guard** stopped this pull request.', + '**Health 45 Agents Guard** stopped this pull request.', '', '**What we found**', ...failureReasons.map((reason) => `- ${reason}`), @@ -531,7 +530,7 @@ function evaluateGuard({ warnings, hasAllowLabel, hasCodeownerApproval, - authorIsCodeowner, + authorIsCodeowner, needsLabel, needsApproval, modifiedProtectedPaths: [...modifiedProtectedPaths], diff --git a/.github/scripts/keepalive_loop.js b/.github/scripts/keepalive_loop.js index 6a5d8ac9f..2e7c676e7 100644 --- a/.github/scripts/keepalive_loop.js +++ b/.github/scripts/keepalive_loop.js @@ -190,7 +190,7 @@ function normaliseChecklistSection(content) { let mutated = false; const updated = lines.map((line) => { - // Match bullet points (-, *, +) or numbered lists (1., 2), 3.) + // Match bullet points (-, *, +) or numbered lists (e.g. 1., 2., 3)) const match = line.match(/^(\s*)([-*+]|\d+[.)])\s+(.*)$/); if (!match) { return line; diff --git a/.github/workflows/agents-guard.yml b/.github/workflows/agents-guard.yml index d6e2f4aec..628c3121e 100644 --- a/.github/workflows/agents-guard.yml +++ b/.github/workflows/agents-guard.yml @@ -92,8 +92,8 @@ jobs: const headers = error?.response?.headers || {}; const remainingRaw = headers['x-ratelimit-remaining'] ?? headers['X-RateLimit-Remaining']; const resetRaw = headers['x-ratelimit-reset'] ?? headers['X-RateLimit-Reset']; - const remaining = typeof remainingRaw === 'string' ? Number(remainingRaw) : Number(remainingRaw); - const reset = typeof resetRaw === 'string' ? Number(resetRaw) : Number(resetRaw); + const remaining = Number(remainingRaw); + const reset = Number(resetRaw); const hasRemainingValue = Number.isFinite(remaining); const hasResetValue = Number.isFinite(reset); const isRateLimit = status === 403 && hasRemainingValue && remaining <= 0; diff --git a/docs/archive/CODE_QUALITY_ISSUES_2026-01-01.md b/docs/archive/CODE_QUALITY_ISSUES_2026-01-01.md new file mode 100644 index 000000000..ba367ec3a --- /dev/null +++ b/docs/archive/CODE_QUALITY_ISSUES_2026-01-01.md @@ -0,0 +1,184 @@ +# Code Quality Issues + +Tracked issues from Copilot code reviews and other sources. Fix all at once after sync PR merges complete. + +## From PR #163 (Travel-Plan-Permission sync) + +### 1. Redundant Ternary Operators in `agents-guard.yml` (3 instances) + +**File:** `.github/workflows/agents-guard.yml` +**Lines:** ~85-126, ~304-343, ~432-471 +**Priority:** Low + +```javascript +// Current (redundant): +const remaining = typeof remainingRaw === 'string' ? Number(remainingRaw) : Number(remainingRaw); +const reset = typeof resetRaw === 'string' ? Number(resetRaw) : Number(resetRaw); + +// Should be: +const remaining = Number(remainingRaw); +const reset = Number(resetRaw); +``` + +`Number()` handles both string and number inputs correctly. + +--- + +### 2. DRY Violation: `withApiRetry` Defined 3 Times + +**File:** `.github/workflows/agents-guard.yml` +**Priority:** Medium + +The `withApiRetry` function and `sleep` helper are copy-pasted in three separate workflow steps. Should be extracted to `.github/scripts/agents-guard.js` and imported once. + +--- + +### 3. tomlkit `isinstance(x, dict)` Check Fails + +**File:** `tools/resolve_mypy_pin.py` +**Priority:** High + +```python +# Current (broken with tomlkit): +tool = data.get("tool") +if not isinstance(tool, dict): + tool = {} +mypy = tool.get("mypy") +if not isinstance(mypy, dict): + mypy = {} + +# Should be (duck typing): +tool = data.get("tool") +if not hasattr(tool, "get"): + tool = {} +mypy = tool.get("mypy") +if not hasattr(mypy, "get"): + mypy = {} +``` + +tomlkit returns `tomlkit.items.Table` objects which support `.get()` but are not instances of `dict`. + +--- + +## From PR #164 (Travel-Plan-Permission sync) + +### 4. Missing Indentation in `authorIsCodeowner` Return + +**File:** `.github/scripts/agents-guard.js` +**Priority:** Low + +```javascript +// Current (2 spaces): + authorIsCodeowner, + +// Should be (4 spaces to match siblings): + authorIsCodeowner, +``` + +--- + +### 5. Inconsistent Indentation in Array Literal + +**File:** `.github/scripts/agents-guard.js` +**Priority:** Low + +Array element has 2 spaces instead of 6 spaces to align with other elements. + +--- + +### 6. Type Validation for `python_version` Before `str()` + +**File:** `tools/resolve_mypy_pin.py` +**Priority:** Medium + +```python +# Current: +return str(version) if version is not None else None + +# Should be (validate type first): +if isinstance(version, (str, int, float)): + return str(version) +return None +``` + +The `python_version` could be parsed as various types from TOML. Validate before converting. + +--- + +## Summary + +| Issue | File | Priority | Status | +|-------|------|----------|--------| +| Redundant ternary (×3) | agents-guard.yml | Low | ✅ Fixed | +| withApiRetry duplication | agents-guard.yml | Medium | ⬚ Open | +| tomlkit isinstance (×2) | resolve_mypy_pin.py | High | ✅ Fixed | +| Missing indentation | agents-guard.js | Low | ✅ Fixed | +| Inconsistent array indentation | agents-guard.js | Low | ✅ Fixed | +| Type validation python_version | resolve_mypy_pin.py | Medium | ✅ Fixed | +| Redundant instructions reassignment | agents-guard.js | Low | ✅ Fixed | +| Typo in numbered list comment | issue_scope_parser.js | Low | ⬚ N/A (not found) | +| Same typo in keepalive_loop.js | keepalive_loop.js | Low | ✅ Fixed | + +--- + +## From PR #100 (Manager-Database sync) + +### 7. Redundant `instructions = []` Reassignment + +**File:** `.github/scripts/issue_scope_parser.js` +**Line:** ~483 +**Priority:** Low + +```javascript +// Current (redundant): +let instructions = []; +if (condition) { + instructions = []; // This is redundant + ... +} + +// Should be: Remove the redundant reassignment +``` + +--- + +### 8. Typo in Numbered List Comment + +**File:** `.github/scripts/issue_scope_parser.js` +**Priority:** Low + +```javascript +// Current (inconsistent): +// Match bullet points (-, *, +) or numbered lists (1., 2), 3.)) + +// Should be: +// Match bullet points (-, *, +) or numbered lists (1., 2., 3.) +``` + +--- + +## From PR #37 (Template sync) + +### 9. Same Typo in keepalive_loop.js + +**File:** `.github/scripts/keepalive_loop.js` +**Line:** ~193 +**Priority:** Low + +```javascript +// Current (typo - "2]" should be "2."): +// Match bullet points (-, *, +) or numbered lists (1., 2), 3.) + +// Should be: +// Match bullet points (-, *, +) or numbered lists (e.g. 1., 2., 3)) +``` + +Same typo pattern as issue #8 - inconsistent parenthesis in numbered list examples. + +--- + +## Notes + +- All issues should be fixed in **stranske/Workflows** (source repo) +- After fixing, trigger sync to propagate to consumer repos +- Created: 2026-01-01 diff --git a/templates/consumer-repo/.github/scripts/agents-guard.js b/templates/consumer-repo/.github/scripts/agents-guard.js index 4d67953ed..fddae9c0a 100644 --- a/templates/consumer-repo/.github/scripts/agents-guard.js +++ b/templates/consumer-repo/.github/scripts/agents-guard.js @@ -483,7 +483,6 @@ function evaluateGuard({ let instructions = []; const touchedFilesText = summarizeTouchedFiles(relevantFiles); if (blocked) { - instructions = []; if (fatalViolations.length > 0) { instructions.push('Restore the deleted or renamed workflows. These files cannot be moved or removed.'); } @@ -502,7 +501,7 @@ function evaluateGuard({ commentBody = [ marker, - '**Health 45 Agents Guard** stopped this pull request.', + '**Health 45 Agents Guard** stopped this pull request.', '', '**What we found**', ...failureReasons.map((reason) => `- ${reason}`), @@ -531,7 +530,7 @@ function evaluateGuard({ warnings, hasAllowLabel, hasCodeownerApproval, - authorIsCodeowner, + authorIsCodeowner, needsLabel, needsApproval, modifiedProtectedPaths: [...modifiedProtectedPaths], diff --git a/templates/consumer-repo/.github/workflows/agents-guard.yml b/templates/consumer-repo/.github/workflows/agents-guard.yml index d6e2f4aec..628c3121e 100644 --- a/templates/consumer-repo/.github/workflows/agents-guard.yml +++ b/templates/consumer-repo/.github/workflows/agents-guard.yml @@ -92,8 +92,8 @@ jobs: const headers = error?.response?.headers || {}; const remainingRaw = headers['x-ratelimit-remaining'] ?? headers['X-RateLimit-Remaining']; const resetRaw = headers['x-ratelimit-reset'] ?? headers['X-RateLimit-Reset']; - const remaining = typeof remainingRaw === 'string' ? Number(remainingRaw) : Number(remainingRaw); - const reset = typeof resetRaw === 'string' ? Number(resetRaw) : Number(resetRaw); + const remaining = Number(remainingRaw); + const reset = Number(resetRaw); const hasRemainingValue = Number.isFinite(remaining); const hasResetValue = Number.isFinite(reset); const isRateLimit = status === 403 && hasRemainingValue && remaining <= 0; diff --git a/tools/resolve_mypy_pin.py b/tools/resolve_mypy_pin.py index df0c17cfb..8d53d7e4a 100755 --- a/tools/resolve_mypy_pin.py +++ b/tools/resolve_mypy_pin.py @@ -31,13 +31,16 @@ def get_mypy_python_version() -> str | None: content = pyproject_path.read_text() data = tomlkit.parse(content) tool = data.get("tool") - if not isinstance(tool, dict): + if not hasattr(tool, "get"): tool = {} mypy = tool.get("mypy") - if not isinstance(mypy, dict): + if not hasattr(mypy, "get"): mypy = {} version = mypy.get("python_version") - return str(version) if version is not None else None + # Validate type before conversion - TOML can parse various types + if isinstance(version, (str, int, float)): + return str(version) + return None except ImportError: pass