Skip to content

#234: Fix code review findings — 5 HIGH, 8 MEDIUM, 6 LOW severity#247

Merged
JeremyDev87 merged 15 commits intomainfrom
fix/234-code-review-findings
Feb 16, 2026
Merged

#234: Fix code review findings — 5 HIGH, 8 MEDIUM, 6 LOW severity#247
JeremyDev87 merged 15 commits intomainfrom
fix/234-code-review-findings

Conversation

@JeremyDev87
Copy link
Owner

Summary

Addresses all actionable findings from the 2026-02-12 comprehensive code review (issue #234). Supersedes closed draft PR #239.

HIGH severity (5)

  • H1: Remove untrusted plan comment fallback in findPlanComment() — only trusted bot authors accepted
  • H2: Escape labels/author in execute prompt shell templates via escapeArrayForShellArg()
  • H3: Reject NONE in authorized_approvers validation
  • H4: Replace unsafe as LeonidasMode cast with isValidLeonidasMode() type guard
  • H5: Replace unsafe as Command cast with isValidCommand() type guard

MEDIUM severity (8)

  • M1: Register API key and GitHub token with core.setSecret() in main.ts
  • M2: Add newline/carriage-return escaping to escapeForShellArg()
  • M5: Cache Octokit instances by token (Map-based singleton)
  • M6: Use path.join() instead of string interpolation for file paths
  • M12: Add coverage/** to ESLint ignores
  • E1: Validate MODE env var in post_process_cli.ts (remaining as LeonidasMode)
  • E2: Register GH_TOKEN with core.setSecret() in all post_process_cli.ts functions
  • E3: Escape single-quotes and strip control characters in escapeForShellArg()

LOW severity (6)

  • L1: Extract magic number 5 to named constant RESERVED_TURNS
  • L4: Strict parseRepo() validation — reject empty, missing separator, and extra path segments
  • L5: Restrict temp prompt file permissions to 0o600
  • E4: Reject extra path segments in parseRepo() (e.g., owner/repo/extra)
  • E5: Remove unnecessary as Command in isValidCommand()
  • E6: Add cross-platform comment for mode: 0o600

Skipped (false positives / already resolved)

  • M3: allowed_tools default — documented design decision
  • M4: Plan comment wrapping — already uses wrapUserContent()
  • M7: i18n test coverage — all 8 languages confirmed tested
  • M10: run() refactoring — already done
  • L3: Silent catch in linkSubIssues — already fixed

Closes #234

Test plan

  • All 427 tests pass (npx vitest run)
  • npm run lint passes (ESLint + Prettier)
  • npm run typecheck passes (tsc --noEmit)
  • npm run build succeeds (ncc bundle)
  • Code review: APPROVE (0 Critical, 0 High, 0 Medium remaining)
  • Security review: APPROVE (all injection vectors mitigated)

@JeremyDev87 JeremyDev87 self-assigned this Feb 16, 2026
@JeremyDev87 JeremyDev87 merged commit 6d9a834 into main Feb 16, 2026
13 checks passed
@JeremyDev87 JeremyDev87 deleted the fix/234-code-review-findings branch February 16, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix code review findings: 5 HIGH, 12 MEDIUM severity issues

1 participant