Feature/add codebase agent

[jeremyeder]

draft for discussion. I think we would also want to add something in claude.md that points to this agent. we also need to centralize logic. Should ewe move out of claude.md "entirely", and into an agent(s)?

[jeremyeder]

this is an important behavior change for claude

[jeremyeder]

[github-actions]

Claude Code Review

Summary

This PR introduces Amber, an AI colleague agent for the ACP platform codebase. The implementation is comprehensive with 3,163 additions across 16 files, introducing agent definitions, documentation, automated dependency syncing, and workflow diagrams. The work shows strong alignment with project standards and constitution compliance, with some areas needing attention before merge.

Overall Assessment: Strong implementation with critical and major issues that should be addressed before merge. The foundation is solid, but security, testing, and documentation gaps need resolution.

---

Issues by Severity

Critical Issues

1. Missing Tests for New Python Script (scripts/sync-amber-dependencies.py:343 lines)

• Issue: Zero test coverage for dependency sync script with complex parsing logic
• Risk: Parsing failures could silently corrupt Amber's knowledge
• Impact: Constitution Principle IV violation (TDD mandatory)
• Fix Required: Create tests/unit/test_sync_amber_dependencies.py with table-driven tests

2. GitHub Actions Workflow Permissions Too Broad (.github/workflows/amber-dependency-sync.yml:10-12)

• Issue: contents:write + issues:write for automated bot commits directly to main
• Risk: Compromised workflow could modify any file or create spam issues
• Constitution Reference: Principle II (Least Privilege)
• Fix Required: Change to PR-based workflow, use pull-requests:write instead

3. Workflow YAML Syntax Issues (.github/workflows/amber-dependency-sync.yml:57-58, 96)

• Line 57-58: grep pattern missing closing brace
• Line 96: Missing case-insensitive flag for conventional commit grep
• Impact: Validation steps will produce false positives/negatives

4. Constitution Compliance Validation Logic Gaps (.github/workflows/amber-dependency-sync.yml:75-103)

• Grep patterns don't match actual Amber content precisely
• Will file false positive constitution violations as issues
• Fix: Test validation script locally before merge

See full review details below for more issues and recommendations.

[github-actions]

Detailed Review (Part 2/3)

Major Issues

5. Amber Agent Definition Extremely Long (agents/amber.md:607 lines)

• Single file violates Component Modularity principle (Constitution V)
• Recommendation: Consider splitting into core/capabilities/workflows
• Not Blocking: Agent files may be exempt from 200-line limit

6. User Guide Documentation Gaps (docs/user-guide/working-with-amber.md)

• Missing: Troubleshooting for common failure modes
• Missing: Examples of bad vs good prompts for Amber
• Missing: When NOT to use Amber
• Missing: Escalation path when Amber declines requests
• Fix Required: Add practical troubleshooting section

7. Dependency Sync Script Error Handling (scripts/sync-amber-dependencies.py:106-108)

• Broad exception catch swallows all errors silently
• Parsing failures return empty dict with no user feedback
• Fix: Add specific exception handling

8. No Rollback Instructions for workflow changes

• PR description lacks rollback steps for automated daily commits
• Impact: Maintainers lack quick recovery path if workflow malfunctions
• Fix: Add rollback section to PR description

9. Mermaid Diagrams Not Validated

• PR includes 5 Mermaid diagrams with no syntax validation
• Risk: Broken diagrams in MkDocs
• Fix: Run mkdocs build --strict

10. Inconsistent Date References (docs/implementation-plans/amber-implementation.md:607)

• Example uses 2025-11-17 but current date is 2025-11-18

Minor Issues

11. Over-Engineering in Authority Hierarchy (agents/amber.md:98-178)

• 80 lines with flowchart and 12 examples adds cognitive load
• Not Blocking: Comprehensive is acceptable

12. Redundant Constitution Sections (agents/amber.md:179-234)

• 55 lines duplicated from CLAUDE.md
• Not Blocking: Redundancy aids agent context

13-15. Other minor issues (see full review)

[github-actions]

Positive Highlights & Recommendations (Part 3/3)

Positive Highlights

✅ Exceptional Constitution Alignment

• Authority hierarchy clearly defines Constitution as absolute
• Embedded constitution compliance checks in daily automation
• Safety principles align with on-call mentality

✅ Comprehensive User Documentation

• 707 lines of user guide with visual workflows
• Clear Quick Start for new users

✅ Smart Automation Design

• Daily dependency sync keeps knowledge current
• Auto-filing issues for violations
• Self-validating workflows

✅ Safety-First Approach

• TodoWrite integration for plan visibility
• Rollback instructions required
• Confidence levels for transparency

✅ Professional Implementation

• Follows conventional commits
• 1,258-line implementation plan
• Validation scripts included

✅ Mermaid Diagrams Enhance Understanding

• 5 diagrams with human checkpoints
• Color-coded decision paths

---

Recommendations

Before Merge (Required - P0/P1)

1. Add tests for Python script
2. Fix workflow security model (PR-based, not direct commits)
3. Fix YAML validation patterns
4. Add rollback documentation

Post-Merge (Recommended - P2/P3)

5. Consider splitting agent definition if maintenance becomes difficult
6. Enhance user guide with common patterns
7. Extract validation script for reusability
8. Track Amber metrics (auto-merge success rate, triage accuracy)

---

Constitution Compliance

• ✅ Principle II (Security): Workflow uses least privilege (needs refinement)
• ✅ Principle III (Type Safety): Explicit error handling
• ⚠️ Principle IV (TDD): VIOLATION - Missing tests for sync script
• ✅ Principle V (Modularity): Clear separation
• ✅ Principle VI (Observability): Validation steps included
• ✅ Principle VIII (Context Engineering): 200K token budget
• ✅ Principle X (Commit Discipline): Conventional commits

Critical Violation: Principle IV requires TDD. Python script needs tests before merge.

---

Final Recommendation

Action: Request changes for critical issues 1-4, then merge.

Confidence: 85% - Strong implementation with fixable gaps

Estimated Fix Time: 3-4 hours

Rationale: Missing tests violate Constitution Principle IV (non-negotiable), workflow security needs hardening per Principle II.