Skip to content

Conversation

@ignatovv
Copy link
Member

Summary

  • Add /polish command combining code simplification and unused code cleanup
  • Integrate polish step into /cpp workflow (runs before commit)

/polish does:

  • Simplify verbose code (guard let, shorthand closures, nil coalescing)
  • Remove unused code from refactoring
  • Check for stale references in tests/mocks

@ignatovv ignatovv requested a review from a team as a code owner January 16, 2026 16:34
@claude
Copy link
Contributor

claude bot commented Jan 16, 2026

Best Practices

polish.md:10, 19
The git command git diff --name-only HEAD~1 will fail on new branches or initial commits where HEAD~1 doesn't exist. Consider using a safer approach:

  • git diff --cached --name-only -- "*.swift" for staged changes, or
  • git diff origin/develop...HEAD --name-only -- "*.swift" to compare against base branch

polish.md:46
The example shows array.filter(\.isActive).map(\.name) with ✅ suggesting it's always preferred, but keypath shorthand isn't universally clearer. The "(if clearer)" comment is good, but the ✅ marker implies a strong preference. Consider weakening the recommendation or providing guidance on when to use each style.

cpp.md:37
The polish step says "See /polish command for full checklist" which creates an implicit dependency. Consider either:

  • Running /polish as an explicit separate command before committing, or
  • Inlining the key polish checks directly in the cpp workflow

⚠️ Minor Issues - Fix git command edge case handling and clarify workflow dependencies

@claude
Copy link
Contributor

claude bot commented Jan 16, 2026

Best Practices

polish.md:106 - Process section has inconsistent step numbering. Steps 1-3 are defined (Analyze, Present Findings, Wait for Approval, Clean Up), but then "Step 4: Final Check" appears at line 100. Either rename to Step 5 or consolidate the process steps to match.


⚠️ Minor Issues - Fix git diff command to show working tree changes instead of comparing to previous commit

@ignatovv ignatovv changed the base branch from ios-5681-update-claudemd-4 to develop January 16, 2026 16:56
@claude
Copy link
Contributor

claude bot commented Jan 16, 2026

Best Practices

cpp.md:32-38 - The polish step integration lacks clarity about workflow interruption. The current text says "REQUIRES APPROVAL" but doesn't make it clear this is a blocking step that interrupts the automated flow. Consider rewording to emphasize:

  • This is a BLOCKING step (not fire-and-forget)
  • User MUST respond before proceeding
  • Unlike Step 3 (review auto-proceeds on approval), Step 0.5 ALWAYS requires user input

polish.md:29 - The head -20 limit could silently truncate changed files, hiding issues from the user. Either:

  • Remove the limit entirely (safest)
  • Or document why 20 is sufficient (if there's a specific reason)
  • Or add a warning when truncation occurs

polish.md:97-99 - The rg search example doesn't specify the search path, which defaults to current directory. This could miss references if run from a subdirectory. Consider adding explicit path:

rg "oldSymbolName" --type swift .

Or document that it should be run from repo root.

codeReview.md:10 & impact_git.md:10-12 - Git context precomputation may fail silently if origin/develop doesn't exist (e.g., fresh clone without fetch). The 2>/dev/null suppresses errors, but there's no fallback guidance for when precomputed values are missing.

All command files - The frontmatter syntax allowed-tools: Bash(git diff:*) appears to be a custom constraint system, but there's no documentation explaining:

  • How these constraints are enforced
  • What happens if a disallowed tool is used
  • Why specific tools are restricted per command

⚠️ Minor Issues - Fix workflow clarity, file truncation, search paths, and error handling

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.

2 participants