feat: add smart-commit skill and fix HTTP tests

[flowerornament]

Summary

• Add /smart-commit skill that intelligently decides between creating a PR or committing directly
• Add .claude-pr-policy.json to configure PR decision thresholds (files changed, lines changed, core files)
• Update AGENTS.md session completion workflow to use /smart-commit by default
• Add Claude Code project skills documentation (.claude/README.md)
• Fix HTTP integration test failures:
  • Added UDP socket connections in http_health_and_shutdown and http_eval_sends_udp tests
  • Fixed server shutdown hangs by sending proper HTTP requests instead of bare TCP connections
• All 10 integration tests now passing in 0.21s

Test Plan

• Run cargo test to verify all tests pass
• Test /smart-commit skill with small changes (should commit directly)
• Test /smart-commit skill with large changes (should create PR)
• Verify .claude-pr-policy.json thresholds are sensible for the project
• Review AGENTS.md workflow updates

🤖 Generated with Claude Code

[claude]

Code Review - PR #2: feat: add smart-commit skill and fix HTTP tests

Summary

This PR adds a valuable workflow automation feature (/smart-commit skill) and fixes critical HTTP integration test issues. The changes are well-structured and follow good patterns. Overall, this is solid work with a few minor areas for improvement.

✅ Strengths

1. Smart-Commit Skill Design

• Excellent separation of concerns: Configuration in JSON, documentation in README.md, implementation spec in SKILL.md
• Flexible thresholds: The configurable policy system (pr_required_if: "any"/"all") provides good flexibility
• Clear documentation: The SKILL.md provides comprehensive implementation steps and examples
• Safety-first approach: Conservative defaults (assume PR when in doubt)

2. Test Fixes

• Root cause addressed: The fixes properly address the server shutdown hang by sending real HTTP requests instead of bare TCP connections
• UDP connection handling: Adding connect_or_skip() calls in both tests (http_health_and_shutdown and http_eval_sends_udp) properly handles the UDP setup
• Consistent pattern: Both tests now follow the same cleanup pattern

3. Documentation Quality

• Comprehensive examples: Both .claude/README.md and SKILL.md provide clear usage examples
• AGENTS.md integration: Session completion workflow properly updated to reference the new skill

🔍 Issues & Suggestions

1. Test Fix - Potential Resource Leak (Minor)

Location: server/launcher/tests/launcher_tests.rs:142-144, 181-183

The tests now call connect_or_skip() but the receiver socket (_recv) is never used and may not be properly cleaned up in error paths. This is likely fine since Rust's RAII will clean up, but consider documenting why the receiver is unused in these specific tests.

2. JSON Schema Reference (Minor)

Location: .claude-pr-policy.json:2

The $schema field points to the generic JSON Schema spec, not a custom schema for this config file. Since there's no actual schema file in the repo, this is misleading.

Recommendation: Either remove the $schema field, create an actual JSON schema file, or change to a comment.

3. SKILL.md Implementation Detail (Minor - Documentation)

Location: .claude/skills/smart-commit/SKILL.md:151

The implementation mentions checking "Current branch is NOT main/master" as a condition, but this isn't reflected in the .claude-pr-policy.json configuration, decision criteria documentation, or safety rules.

Recommendation: Clarify whether branch checking is part of the implementation. If so, document it in the policy file. If not, remove it from the implementation steps.

4. Core Files List (Suggestion)

Location: .claude-pr-policy.json:7-15

The core files list is missing some potentially critical files like .claude-pr-policy.json itself, server/launcher/Cargo.toml, and test infrastructure files.

5. Error Handling in SKILL.md (Minor)

Location: .claude/skills/smart-commit/SKILL.md:177-180

The error handling section mentions "Use defaults (10 files, 100 lines, empty core files)" but these defaults aren't shown in the implementation steps. Consider adding a default configuration section.

6. Test Coverage Gap (Observation)

The PR description mentions test plan items but some are unchecked. Since this is a workflow automation skill that interacts with git, consider adding integration tests or at least manual verification results before merging.

🔒 Security Concerns

None identified. The changes are configuration and tooling-focused with no security implications.

⚡ Performance Considerations

No concerns. The skill runs git commands that are already fast, and the JSON config parsing is trivial overhead.

📊 Test Coverage

• ✅ HTTP test fixes properly address the shutdown hang issue
• ✅ All 10 integration tests now passing
• ⚠️ No automated tests for the /smart-commit skill itself (expected, since it's a Claude skill)

🎯 Recommendations Priority

High Priority:

• None - all issues are minor

Medium Priority:

1. Clarify branch checking behavior in SKILL.md (documentation consistency)
2. Add default configuration section to SKILL.md

Low Priority:

1. Fix or remove $schema field in policy JSON
2. Expand core files list
3. Document why receiver socket is unused in specific tests

✅ Approval Status

Approved with minor suggestions. The code is production-ready. The suggestions above are improvements that can be addressed in follow-up commits if desired.

---

Great work on this PR! The /smart-commit skill fills a real workflow need, and the test fixes resolve actual CI failures. The documentation is thorough and the implementation approach is sound.

[claude]

Code Review: PR #2 - Smart Commit Skill and HTTP Test Fixes

Summary

This PR introduces a valuable /smart-commit skill that intelligently decides between creating PRs and direct commits based on change significance. It also fixes critical HTTP integration test failures. Overall, this is a well-implemented and thoughtful contribution.

---

✅ Strengths

1. Excellent Documentation

• .claude/skills/smart-commit/SKILL.md: Comprehensive implementation guide (209 lines)
• .claude/README.md: Clear user-facing documentation with examples
• The documentation is thorough, with clear decision criteria, configuration options, and error handling

2. Smart Design Decisions

• Configurable thresholds: .claude-pr-policy.json allows customization without code changes
• Conservative defaults: 10 files, 100 lines are reasonable thresholds
• Core files list: Correctly identifies critical files (main.rs, Cargo.toml, AGENTS.md, etc.)
• "any" logic: Default OR logic is sensible for triggering PRs

3. Test Fixes Are Correct

• server/launcher/tests/launcher_tests.rs:139,180: Added proper UDP socket connections
• Lines 168,224: Fixed shutdown hangs by sending proper HTTP requests instead of bare TCP connections
• The comments explaining UDP socket usage patterns are helpful

4. Integration with Project Workflow

• AGENTS.md updates integrate /smart-commit into the session completion workflow
• The skill respects existing /pr and /commit skills (delegates to them)
• Manual override options are preserved

---

🔍 Code Quality Observations

JSON Configuration (.claude-pr-policy.json)

Good:

• Clean structure with comments explaining each field
• Reasonable default thresholds
• Includes self-reference (.claude-pr-policy.json) in core files

Minor consideration:

• Line 17: The glob pattern .claude/skills/*/SKILL.md is present in a JSON file. Ensure the skill implementation handles glob expansion properly (not just string matching). This should be documented in the implementation section.

Test Code (launcher_tests.rs)

Good:

• Lines 139-146, 180-186: Proper resource initialization with early returns
• Comments explain why unused variables exist (_recv)
• Lines 168, 224: Proper HTTP shutdown requests instead of raw TCP connections

Excellent:

• The test fixes address the root cause (missing UDP connections, improper shutdown signaling)
• No test coverage gaps introduced

Skill Documentation (SKILL.md)

Excellent:

• Implementation steps are detailed and sequential
• Error handling is well-specified
• Safety rules prevent dangerous operations
• Default configuration section addresses edge cases

Minor note:

• Line 20: Mentions "New feature - Commit message indicates feature work" as a criterion, but the implementation steps don't show how to analyze commit messages. If this isn't implemented yet, consider removing it or marking it as "future work."

---

🛡️ Security & Safety

No Security Concerns Found

• All git commands are read-only or delegated to existing skills
• No command injection vectors (uses proper git commands)
• No credential handling
• JSON parsing should be safe (standard JSON, no code execution)

Safety Rules Are Good

• Conservative approach: "When in doubt, create a PR"
• Verifies staged changes before acting
• Graceful degradation with defaults if policy file is missing

---

🚀 Performance Considerations

Optimal:

• Skill runs git commands in parallel where possible (SKILL.md:118-122)
• File operations are minimal (one JSON read)
• No expensive computations

No performance issues identified.

---

🧪 Test Coverage

Tests Fixed: ✅

• All 10 integration tests now passing (0.21s)
• http_health_and_shutdown: Fixed UDP connection and shutdown
• http_eval_sends_udp: Fixed UDP connection and shutdown

Test Coverage Assessment:

Good:

• The HTTP fixes directly address the test failures
• Integration tests verify end-to-end behavior

Suggestion:

• Consider adding a test for the .claude-pr-policy.json parsing and threshold logic (future work)
• Could add a test that verifies the skill correctly identifies core files from the config

---

📋 Recommendations

High Priority: None

All identified issues from the first review have been addressed in commit e0a99a0.

Low Priority Suggestions:

1. Glob Pattern in Core Files (line .claude-pr-policy.json:17)

   • Document in SKILL.md how .claude/skills/*/SKILL.md should be matched
   • Clarify if the skill should expand globs or match exact paths

2. Commit Message Analysis

   • SKILL.md:20 mentions analyzing commit messages for "new feature" detection
   • If not implemented, either remove this from the criteria or add to future work

3. Consider Adding Examples

   • Could add a .claude/skills/smart-commit/examples/ directory with sample outputs
   • Would help users understand the decision-making process

4. Error Message Improvements

   • When policy file is missing, could log a warning (not error) to inform users they're using defaults

---

🎯 Overall Assessment

Rating: ⭐⭐⭐⭐⭐ Excellent

This PR demonstrates:

• Strong understanding of the project architecture
• Thoughtful design with configurability and safety in mind
• Excellent documentation practices
• Proper test fixes addressing root causes

Merge Recommendation: ✅ APPROVED

This is a valuable addition to the project workflow. The smart-commit skill will streamline development by automatically making appropriate PR vs. commit decisions. The HTTP test fixes resolve critical integration test failures.

No blocking issues. The minor suggestions above are optional improvements for future iterations.

---

📝 Additional Notes

The author addressed all feedback from the initial review (commit e0a99a0):

1. ✅ Added test resource comments
2. ✅ Removed $schema field from JSON
3. ✅ Fixed branch checking inconsistency
4. ✅ Expanded core files list
5. ✅ Added default configuration documentation

Great work! 🎉