From e560781c87e2b69bb52818a5932758a3d588610d Mon Sep 17 00:00:00 2001 From: Rodin Date: Tue, 19 May 2026 02:15:06 +0000 Subject: [PATCH] Removing intermediate files --- CYCLE_REPORT_2026-05-15.md | 116 ---------- CYCLE_STATUS_2026-05-15-1314.md | 48 ----- CYCLE_STATUS_2026-05-15-1354.md | 76 ------- CYCLE_STATUS_2026-05-15-1418.md | 65 ------ CYCLE_STATUS_2026-05-15-1427.md | 38 ---- CYCLE_STATUS_2026-05-15-1442.md | 38 ---- DEV_LOOP_CHECKPOINT_2026-05-15.md | 54 ----- DEV_LOOP_FINAL_2026-05-15.md | 83 ------- DEV_LOOP_HEALTH.md | 74 ------- DEV_LOOP_STATUS.md | 42 ---- DEV_LOOP_STATUS.txt | 25 --- DEV_LOOP_STATUS_2026-05-15-1342.md | 51 ----- DEV_LOOP_SUMMARY.md | 139 ------------ PLAN-125.md | 175 --------------- PLAN-141.md | 154 ------------- PLAN-143.md | 125 ----------- docs/DESIGN-137-doc-map.md | 82 ------- docs/DESIGN-51-personas.md | 334 ----------------------------- docs/DESIGN-57-yaml-persona.md | 87 -------- 19 files changed, 1806 deletions(-) delete mode 100644 CYCLE_REPORT_2026-05-15.md delete mode 100644 CYCLE_STATUS_2026-05-15-1314.md delete mode 100644 CYCLE_STATUS_2026-05-15-1354.md delete mode 100644 CYCLE_STATUS_2026-05-15-1418.md delete mode 100644 CYCLE_STATUS_2026-05-15-1427.md delete mode 100644 CYCLE_STATUS_2026-05-15-1442.md delete mode 100644 DEV_LOOP_CHECKPOINT_2026-05-15.md delete mode 100644 DEV_LOOP_FINAL_2026-05-15.md delete mode 100644 DEV_LOOP_HEALTH.md delete mode 100644 DEV_LOOP_STATUS.md delete mode 100644 DEV_LOOP_STATUS.txt delete mode 100644 DEV_LOOP_STATUS_2026-05-15-1342.md delete mode 100644 DEV_LOOP_SUMMARY.md delete mode 100644 PLAN-125.md delete mode 100644 PLAN-141.md delete mode 100644 PLAN-143.md delete mode 100644 docs/DESIGN-137-doc-map.md delete mode 100644 docs/DESIGN-51-personas.md delete mode 100644 docs/DESIGN-57-yaml-persona.md diff --git a/CYCLE_REPORT_2026-05-15.md b/CYCLE_REPORT_2026-05-15.md deleted file mode 100644 index b56b901..0000000 --- a/CYCLE_REPORT_2026-05-15.md +++ /dev/null @@ -1,116 +0,0 @@ -# Dev-Loop Cycle Report — 2026-05-15 12:16 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Schedule:** Every 4 hours -**Repository:** gitea.weiker.me/rodin/review-bot - -## Status Summary - -| Metric | Status | -|--------|--------| -| **Repository Health** | ✅ **EXCELLENT** | -| **Main Branch** | Current (1f58c65) | -| **Working Tree** | Clean (no uncommitted) | -| **Test Suite** | ✅ All 7 packages passing | -| **Code Coverage** | 76.7% (up from 70.4%) | -| **Open Issues** | 0 active work items | -| **Open PRs** | 0 pending review | -| **Stale Branches** | ✅ Cleaned | - -## Recent Accomplishments (This Cycle) - -All 4 approved PRs successfully merged to main: - -### 1. Issue #150 — Directory Symlink Bypass Security Fix -- **PR:** #152 -- **Commit:** 76b6493 -- **Status:** ✅ Merged -- **What:** Added `filepath.EvalSymlinks` to `validateDocmapPath` to close intermediate directory symlink bypass -- **Impact:** Security hardening for doc-map config path confinement - -### 2. Issue #154 — Main Test Refactor -- **PR:** #155 -- **Commit:** 77a7f66 -- **Status:** ✅ Merged -- **What:** Extracted `baseSubprocessArgs` helper in main_test.go -- **Impact:** Reduced test boilerplate, improved maintainability - -### 3. Issue #146 — Doc-Map Path Validation Tests -- **PR:** #151 -- **Commit:** 430e61f -- **Status:** ✅ Merged (rebased) -- **What:** Added `TestMainSubprocess_InvalidDocMapPath` and `TestMainSubprocess_InvalidDocMapFile` -- **Impact:** Better test coverage for doc-map error handling - -### 4. Issue #143 — Trusted VCS Ref for Doc-Map Config -- **PR:** #153 -- **Commit:** 02dfc12 -- **Status:** ✅ Merged (rebased) -- **What:** New `--doc-map-trusted-ref` flag to fetch doc-map YAML from trusted VCS ref instead of PR branch -- **Impact:** Prevents malicious PRs from modifying doc-map config to inject arbitrary docs - -## Code Coverage Analysis - -| Package | Coverage | Target | Status | -|---------|----------|--------|--------| -| `budget` | 91.8% | >80% | ✅ Excellent | -| `review` | 91.5% | >80% | ✅ Excellent | -| `llm` | 81.3% | >80% | ✅ Good | -| `gitea` | 83.8% | >80% | ✅ Good | -| `github` | 85.6% | >80% | ✅ Good | -| `internal/netutil` | 90.0% | >80% | ✅ Good | -| `cmd/review-bot` | 36.8% | >60% | ⚠️ Below target | -| **Total** | **76.7%** | >70% | ✅ Good | - -**Recommendation:** `cmd/review-bot` coverage remains challenging due to CLI integration nature. Priority: integration tests, not unit coverage expansion. - -## Repository Hygiene - -✅ **All stale branches cleaned:** -- issue-137, issue-141, issue-143, issue-146, issue-150 (dev branches) -- origin-main, pr-151-merge, pr-152-merge, pr-155-merge, test-146 (merge artifacts) - -✅ **Working tree:** Pristine (no uncommitted changes) -✅ **Remote sync:** On-time with origin/main (1f58c65) - -## Test Results (Complete) - -``` -ok gitea.weiker.me/rodin/review-bot/budget (cached) -ok gitea.weiker.me/rodin/review-bot/cmd/review-bot (cached) -ok gitea.weiker.me/rodin/review-bot/gitea (cached) -ok gitea.weiker.me/rodin/review-bot/github (cached) -ok gitea.weiker.me/rodin/review-bot/internal/netutil (cached) -ok gitea.weiker.me/rodin/review-bot/llm (cached) -ok gitea.weiker.me/rodin/review-bot/review (cached) -``` - -## What's Next? - -### Backlog Review -No open high-priority issues blocking the next development cycle. Backlog is ready for prioritization: -- Review Gitea issues for feature requests / bugs -- Consider doc-map integration tests (improve CLI coverage) -- Assess performance optimization opportunities - -### Recommended Next Sprint -1. **Integration test suite** for main CLI entrypoint (drive cmd/review-bot coverage up) -2. **Performance audit** of doc-map filtering on large PR diffs -3. **User documentation** review (e.g., composite action usage examples) - -## Files Updated This Cycle - -- ✅ `CHANGELOG.md` — Added issue #143, #150 entries -- ✅ `DEV_LOOP_STATUS.md` — 4 PRs merged, repo clean -- ✅ Branch cleanup — Removed 12 stale local branches - -## Cron Health - -- **Last run:** 2026-05-15 12:16 UTC -- **Runtime:** ~45 seconds -- **Status:** ✅ Nominal -- **Action:** Merge cycle complete → ready for next sprint - ---- - -_**Next cycle:** 2026-05-15 16:16 UTC (check for new backlog items, start next issue if available)_ diff --git a/CYCLE_STATUS_2026-05-15-1314.md b/CYCLE_STATUS_2026-05-15-1314.md deleted file mode 100644 index 3470763..0000000 --- a/CYCLE_STATUS_2026-05-15-1314.md +++ /dev/null @@ -1,48 +0,0 @@ -# Dev-Loop Cycle Status — 2026-05-15 13:14 UTC - -**Cycle ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Context:** Cron checkpoint after 1314 UTC - -## Status: ✅ GREEN - -**All systems nominal.** Previous cycle (12:16 UTC) completed successfully: -- 4 PRs merged (security, tests, feature, refactor) -- 76.7% test coverage (target: >70% ✅) -- Main branch clean and synced with origin -- No open issues or stale branches -- Test suite passing on all 7 packages - -## Current Metrics - -| Metric | Value | Target | Status | -|--------|-------|--------|--------| -| Test Coverage | 76.7% | >70% | ✅ Pass | -| Open PRs | 0 | 0 | ✅ Pass | -| Open Issues | 0 | 0 | ✅ Pass | -| Main Synced | ✅ | ✅ | ✅ Pass | -| Last Test Run | ✅ All pass | ✅ All pass | ✅ Pass | - -## What's Ready - -### For Next Work Item -1. Backlog assessment — any new issues from Gitea -2. Integration test suite for CLI entrypoint (if available) -3. Performance audit candidate: doc-map filtering on large diffs - -### Skills + Tools -- All PRs use `gitea-rodin` token (✅ correct) -- No stale worktrees (✅ cleaned) -- CHANGELOG updated (✅ automated) -- Dev-loop plan files available for reference - -## Cron Schedule - -| Time (UTC) | Action | Last | Next | -|------------|--------|------|------| -| Every 4h | Review cycle | 12:16 | 16:31 | - -**Next checkpoint:** 2026-05-15 16:31 UTC - ---- - -**Analyst Notes:** Repo is stable. Ready to begin next feature/issue work when assigned. diff --git a/CYCLE_STATUS_2026-05-15-1354.md b/CYCLE_STATUS_2026-05-15-1354.md deleted file mode 100644 index 912ee50..0000000 --- a/CYCLE_STATUS_2026-05-15-1354.md +++ /dev/null @@ -1,76 +0,0 @@ -# Dev-Loop Cycle Status — 2026-05-15 13:54 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Cycle:** review-bot-dev-loop (4-hour schedule) -**Status:** ✅ **STEADY STATE** — All work merged, repo healthy, ready for next sprint - -## Summary - -### Repository Health — ✅ EXCELLENT - -| Check | Status | Details | -|-------|--------|---------| -| Main branch | ✅ Current | fb899ab (2026-05-15 13:42 UTC) | -| Working tree | ✅ Clean | No uncommitted changes | -| Test suite | ✅ All pass | 7 packages, all pass | -| Code coverage | ✅ 76.7% | Above 70% target | -| Open issues | ✅ None | Backlog clean | -| Open PRs | ✅ None | All approved work merged | -| Stale branches | ✅ Clean | All cleaned up | - -### This Cycle — 2026-05-15 (0900-1400 UTC) - -**Work Completed:** -- ✅ All 4 approved PRs merged to main (#152, #155, #151, #153) -- ✅ Rebases completed cleanly (#151, #153) -- ✅ Code coverage improved to 76.7% -- ✅ All stale branches removed -- ✅ Repository now in steady state - -**Key Metrics:** -- **PRs merged:** 4 -- **Commits landed:** 6 -- **Test pass rate:** 100% (7/7 packages) -- **Coverage change:** +6.3% (from 70.4% to 76.7%) - -### Next Actions - -**Immediate (next cycle ~1400-1800 UTC):** -1. Review Gitea backlog for feature requests / bugs -2. Consider picking up integration test work or performance audit -3. Monitor for any production issues - -**Medium-term priorities** (from previous cycle report): -- Integration test suite for CLI (drive cmd/review-bot coverage up) -- Performance audit of doc-map filtering -- User documentation review - -## Notable Changes This Session - -1. **New Test Coverage** (issue #146, #143) - - Doc-map path validation tests added - - Trusted VCS ref feature now tested - -2. **Security Improvements** (issue #150) - - Symlink bypass closed via `filepath.EvalSymlinks` - - Path confinement hardened - -3. **Code Quality** (issue #154) - - Test boilerplate reduced via helper extraction - - Maintainability improved - -## Repository Snapshot - -``` -Status: Synced with origin/main -Main: fb899ab (latest commit checkpoint) -Tests: All passing ✅ -Cov: 76.7% (target: >70%) -Files: Clean working tree -PRs: None pending -``` - ---- -**Ready for next sprint. No blockers.** - -Generated: 2026-05-15 13:54 UTC | Cron: review-bot-dev-loop diff --git a/CYCLE_STATUS_2026-05-15-1418.md b/CYCLE_STATUS_2026-05-15-1418.md deleted file mode 100644 index fc5c5a0..0000000 --- a/CYCLE_STATUS_2026-05-15-1418.md +++ /dev/null @@ -1,65 +0,0 @@ -# Dev-Loop Cycle Status — 2026-05-15 14:18 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Cycle:** review-bot-dev-loop (4-hour schedule) -**Status:** ✅ **STEADY STATE** — All work merged, repo healthy, zero blockers - -## Health Check Summary - -| Check | Status | Details | -|-------|--------|---------| -| Main branch | ✅ Current | 4311ccf (2026-05-15 13:54 UTC) | -| Working tree | ✅ Clean | No uncommitted changes | -| Test suite | ✅ All pass | 7 packages, 100% pass rate | -| Code coverage | ✅ 76.7% | Above 70% baseline target | -| Open issues | ✅ None | Backlog empty | -| Open PRs | ✅ None | All approved work merged | -| Remote sync | ✅ On-time | Fetched from origin/main | - -## Metrics This Cycle - -- **Issues resolved:** 0 (steady state) -- **PRs merged:** 0 (all prior work landed) -- **Commits reviewed:** 5 (monitoring only) -- **Test pass rate:** 100% (7/7 packages) -- **Code coverage:** 76.7% (stable) - -## Next Actions - -### Immediate (Next 4-hour cycle) - -1. **Gitea backlog review** — Check for feature requests or bug reports -2. **Consider backlog work** from previous cycle report: - - Integration test suite for CLI (drive cmd/review-bot coverage up from 53.3%) - - Performance audit of doc-map filtering - - User documentation review - -3. **Monitor remote branches** — Consolidate stale branches if needed - -### Medium-term Opportunities - -- **cmd/review-bot coverage** (currently 53.3%) — integration tests needed -- **Performance profiling** — doc-map filtering on large diffs -- **Documentation** — composite action examples, CLI guide updates - -## Repository Snapshot - -``` -Branches: main (current) + 30+ stale remote branches (candidates for cleanup) -Tests: All passing ✅ -Coverage: 76.7% (stable) -Files: Clean working tree ✅ -Status: Ready for new work assignment -``` - -## Recommendation - -**No blockers. Ready to pick up next backlog item.** If no new issues assigned, recommend: -1. Pick integration test work (issue-like scope) to improve cmd/review-bot coverage -2. Run performance analysis on doc-map filtering -3. Plan v0.5.0 roadmap based on backlog priorities - ---- -**Cycle complete.** Repo healthy. Standing by for next assignment. - -Generated: 2026-05-15 14:18 UTC | Cron: review-bot-dev-loop diff --git a/CYCLE_STATUS_2026-05-15-1427.md b/CYCLE_STATUS_2026-05-15-1427.md deleted file mode 100644 index 89c4404..0000000 --- a/CYCLE_STATUS_2026-05-15-1427.md +++ /dev/null @@ -1,38 +0,0 @@ -# Dev-Loop Cycle Status — 2026-05-15 14:26 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Cycle:** review-bot-dev-loop (4-hour schedule) -**Status:** ✅ **STEADY STATE** — All systems nominal, repo healthy - -## Health Check Summary - -| Check | Status | Details | -|-------|--------|---------| -| Main branch | ✅ Current | HEAD at 8ab45be | -| Working tree | ✅ Clean | No uncommitted changes | -| Test suite | ✅ All pass | Go tests passing | -| Code coverage | ✅ 76.7% | Above baseline target | -| Open issues | ✅ None | No assigned work | -| Open PRs | ✅ None | All work merged | -| Remote sync | ✅ On-time | Up-to-date with origin | - -## Actions This Cycle - -- ✅ Verified main branch is current -- ✅ Confirmed all tests passing -- ✅ Checked for new issues/PRs — none found -- ✅ Confirmed remote sync status -- ✅ Repo in clean, mergeable state - -## Backlog Opportunities - -1. **Integration tests** — cmd/review-bot coverage (53.3% → target 80%) -2. **Performance profiling** — doc-map filtering optimization -3. **Documentation** — Composite action examples - -## Recommendation - -**No new assignments.** Repo ready for next feature work. Standing by. - ---- -Generated: 2026-05-15 14:26 UTC | Cron: review-bot-dev-loop diff --git a/CYCLE_STATUS_2026-05-15-1442.md b/CYCLE_STATUS_2026-05-15-1442.md deleted file mode 100644 index f0b8203..0000000 --- a/CYCLE_STATUS_2026-05-15-1442.md +++ /dev/null @@ -1,38 +0,0 @@ -# Dev-Loop Cycle Status — 2026-05-15 14:42 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Cycle:** review-bot-dev-loop (4-hour schedule) -**Status:** ✅ **STEADY STATE** — All systems nominal, repo healthy - -## Health Check Summary - -| Check | Status | Details | -|-------|--------|---------| -| Main branch | ✅ Current | HEAD at 8ab45be (synced) | -| Working tree | ✅ Clean | No uncommitted changes | -| Test suite | ✅ All pass | 100% pass rate (go test ./...) | -| Code coverage | ✅ 76.7% | Above baseline target | -| Open issues | ✅ None | No assigned work | -| Open PRs | ✅ None | All merged | -| Remote sync | ✅ On-time | Up-to-date with origin/main | - -## Actions This Cycle - -- ✅ Fetched origin/main — up-to-date -- ✅ Ran full test suite — all pass -- ✅ Calculated code coverage — 76.7% -- ✅ Checked for new issues/PRs — none found -- ✅ Verified working tree clean - -## Backlog Opportunities - -1. **Integration tests** — cmd/review-bot coverage (53.3% → target 80%) -2. **Performance profiling** — doc-map filtering optimization -3. **Documentation** — Composite action examples - -## Recommendation - -**No new assignments.** Repo ready for next feature work. Standing by. - ---- -Generated: 2026-05-15 14:42 UTC | Cron: review-bot-dev-loop diff --git a/DEV_LOOP_CHECKPOINT_2026-05-15.md b/DEV_LOOP_CHECKPOINT_2026-05-15.md deleted file mode 100644 index fc2820b..0000000 --- a/DEV_LOOP_CHECKPOINT_2026-05-15.md +++ /dev/null @@ -1,54 +0,0 @@ -# Dev-Loop: Checkpoint — 2026-05-15 13:14 UTC - -**Cycle ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c - -## Status Summary - -✅ **All systems nominal.** - -## Key Events (This Checkpoint) - -1. **v0.4.0 Release Prepared** (13:05 UTC) - - CHANGELOG marked as stable (Unreleased → v0.4.0) - - 4 PRs merged in previous cycle - - 76.7% test coverage - - Shipped: security hardening, test coverage, feature (doc-map trusted ref), refactor - -2. **Current Commit:** `80b04d1` (2026-05-15 13:14 UTC) - - All tests passing - - Main synced with origin - - No uncommitted changes - - Ready for next work assignment - -## Backlog for Next Cycle - -### High Priority -1. **Integration test suite** — CLI entrypoint tests (if available) -2. **Performance audit** — doc-map filtering on large diffs - -### Medium Priority -3. **User documentation** — doc-map usage guide, best practices -4. **Backlog triage** — Check Gitea for new issues - -## Metrics - -- **Coverage:** 76.7% (↑ up from 71.2% at cycle start) -- **Test Pass Rate:** 100% (7 packages) -- **Open Issues:** 0 -- **Open PRs:** 0 -- **Stale Branches:** 0 - -## What's Ready - -- ✅ Pre-code skill — use for next issue -- ✅ Dev-loop process — worktree setup, pre-push checklist validated -- ✅ gitea-rodin token — all PRs reviewed/merged with correct identity -- ✅ Test infrastructure — all passing, ready for new features - -## Next Checkpoint - -**Scheduled:** 2026-05-15 16:31 UTC (cron every 4 hours) - ---- - -**Status:** Ready for next sprint. All systems green. v0.4.0 release cycle complete. diff --git a/DEV_LOOP_FINAL_2026-05-15.md b/DEV_LOOP_FINAL_2026-05-15.md deleted file mode 100644 index 640c09e..0000000 --- a/DEV_LOOP_FINAL_2026-05-15.md +++ /dev/null @@ -1,83 +0,0 @@ -# Dev-Loop Final Status — 2026-05-15 12:31 UTC - -**Cycle ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Run:** Every 4 hours (last: 12:16 UTC, next: 16:31 UTC) - -## Executive Summary - -✅ **CYCLE COMPLETE** — All 4 approved PRs merged, full test suite passing, repo clean and ready. - -## Merge Status - -| # | Issue | Type | Commit | Status | -|---|-------|------|--------|--------| -| #152 | #150 | Security | 76b6493 | ✅ Merged | -| #155 | #154 | Refactor | 77a7f66 | ✅ Merged | -| #151 | #146 | Test | 430e61f | ✅ Merged | -| #153 | #143 | Feature | 02dfc12 | ✅ Merged | - -**All PRs:** Merged to main, branches cleaned, worktrees removed. - -## Current State - -``` -Main Branch: 1f58c65 (2026-05-15 12:09 UTC) -Working Tree: Clean (no uncommitted changes) -Remote Sync: ✅ On-time with origin/main -Last Test Run: ✅ All 7 packages pass -Coverage: 76.7% (target: >70%) -Open Issues: 0 active items -Open PRs: 0 pending review -Stale Branches: ✅ Cleaned -``` - -## Test Results - -``` -✅ budget — 92.0% coverage -✅ cmd/review-bot — 53.3% coverage (cli integration, expected lower) -✅ gitea — 85.2% coverage -✅ github — 86.3% coverage -✅ internal/net — 85.7% coverage -✅ llm — 81.3% coverage -✅ review — 92.2% coverage -``` - -## What Shipped This Cycle - -1. **Security Hardening (#150):** Directory symlink validation -2. **Test Coverage (#146):** Doc-map validation error tests -3. **Feature (#143):** Trusted VCS ref for doc-map config (prevents config injection) -4. **Refactor (#154):** Test helper extraction (reduced boilerplate) - -## Next Actions - -### Immediate (next cycle, 16:31 UTC) -- Assess backlog for new issues -- Continue integration test expansion if available -- Performance audit candidate: doc-map filtering on large diffs - -### Backlog Ready -- Integration test suite for CLI entrypoint -- Performance optimization opportunities -- User documentation review - -## Cron Health - -- **Last execution:** 2026-05-15 12:16 UTC (~45s runtime) -- **Status:** ✅ Nominal -- **Pattern:** Consistent 4-hour cycles -- **Alert threshold:** >2 min runtime or test failures - -## Files - -- ✅ CHANGELOG.md — Updated with issue entries -- ✅ DEV_LOOP_STATUS.md — 4 PRs merged -- ✅ Branch cleanup — 12 stale branches removed -- ✅ Test suite — All passing - ---- - -**Cycle Status:** ✅ READY FOR NEXT SPRINT - -Ready to start work on next high-priority backlog item when available. diff --git a/DEV_LOOP_HEALTH.md b/DEV_LOOP_HEALTH.md deleted file mode 100644 index bdb78fa..0000000 --- a/DEV_LOOP_HEALTH.md +++ /dev/null @@ -1,74 +0,0 @@ -# Dev Loop Health Check — 2026-05-15 09:24 UTC - -## Status: ✅ CLEAN & READY - -### Summary -- **Main branch:** current (6d82535) -- **Latest commit:** chore: dev-loop verification — issue-130 already in main, worktree stale -- **Active worktrees:** NONE (all cleaned) -- **Repository state:** ✅ HEALTHY - -### Cycle Completion -✅ Issue #130 (GitHub PR reviews): Verified complete in main via cherry-picks -✅ Issue #137 (doc-map validation): Verified complete in main -✅ Worktree cleanup: All stale worktrees removed -✅ Main branch: Fast-forward current with latest changes - -### What Was Accomplished - -**Issue #130 Self-Review Findings (ALL ADDRESSED):** -- ✅ f7008ab: refactor(#130): move IsBlockedIP to internal/netutil -- ✅ 1e50a22: refactor(#130): rename vcsReviewComment.NewPosition → NewLine -- ✅ 3e33e3d: fix(#130): pass VCS_TYPE env var from action.yml Run review step -- ✅ 3387456: docs(#130): fix README CLI example and env var table - -**Earlier Completed (Issue #141):** -- chore(#141): hardened validate-docmap subcommand -- security fixes addressing REQUEST_CHANGES -- path traversal protections - ---- - -## Repository Status - -| Metric | Status | -|--------|--------| -| Main branch SHA | 6d82535 (2026-05-15 09:24 UTC) | -| Working tree | ✅ Clean | -| Worktrees | ✅ None active | -| Remote tracking | ✅ Current | -| Last push | ✅ Successful (6d82535) | - ---- - -## Next Steps for Human/Maintainer - -### Priority Issues for Next Cycle -1. **Issue #143** — fetch doc-map config from trusted VCS ref -2. **Issue #146** — (review Gitea for issue details) -3. **Issue #150** — add EvalSymlinks to validateDocmapPath - -### Coverage Observations -- `cmd/review-bot`: 36.8% (target: >60%) -- `budget`: 91.8% ✅ -- `review`: 91.5% ✅ -- `llm`: 81.3% -- **Total:** 70.4% - -### Recommendations -- Increase cmd/review-bot coverage by adding integration/e2e tests -- Consider extracting main logic to testable functions -- Review SKILL.md and dev-loop-spec.md for documentation gaps - ---- - -## Cron Metadata - -- **Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -- **Schedule:** Every 4 hours -- **Runtime:** 2026-05-15 09:23 UTC -- **Repo:** gitea.weiker.me/rodin/review-bot - ---- - -_Dev-loop cycle complete. Repo is clean, ready for next development sprint._ diff --git a/DEV_LOOP_STATUS.md b/DEV_LOOP_STATUS.md deleted file mode 100644 index b423229..0000000 --- a/DEV_LOOP_STATUS.md +++ /dev/null @@ -1,42 +0,0 @@ -# Dev Loop Status — 2026-05-15 12:15 UTC - -**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Status:** ✅ HEALTHY — All 4 PRs merged, all tests passing, repo clean - -## Quick Status - -- **Main branch:** Synced with origin/main (1f58c65) -- **Tests:** All passing ✅ (7 packages, all pass) -- **Working tree:** Clean (no uncommitted changes) - -## PR Merge Summary — 2026-05-15 - -All 4 approved PRs have been merged to main: - -| PR | Issue | Type | Merged Commit | Status | -|----|-------|------|---------------|--------| -| #152 | #150 | Security | 76b6493 | ✅ Merged (closed) | -| #155 | #154 | Refactor | 77a7f66 | ✅ Merged (closed) | -| #151 | #146 | Test | 430e61f | ✅ Merged (rebased) | -| #153 | #143 | Feature | 02dfc12 | ✅ Merged (rebased) | - -### Notes - -- **PR #151 (issue-146):** Rebased to drop already-merged base commit (`40a16b7` → `98479c9` on main). Follow-up fix `9b64c60` was already incorporated by main. One clarification commit `430e61f` landed. -- **PR #153 (issue-143):** Rebased onto main, dropping 2 issue-146 base commits now on main. CHANGELOG merge conflict resolved (both security entries preserved). 2 clean commits landed. -- **PR #152 / #155:** Already on main via direct merge; PRs closed without re-merge. - -## Dev Loop Health - -| Metric | Status | Details | -|--------|--------|---------| -| Main branch | ✅ Current | 1f58c65 (2026-05-15 12:15 UTC) | -| Working tree | ✅ Clean | No uncommitted changes | -| Test suite | ✅ All pass | 7 packages, all pass | -| Open PRs | ✅ None | All approved PRs merged | -| Worktrees | ✅ Clean | rb-issue-143 and rb-issue-146 removed | - -## Next Actions - -- No open approved PRs remain -- Dev-loop can start on new issues from the backlog diff --git a/DEV_LOOP_STATUS.txt b/DEV_LOOP_STATUS.txt deleted file mode 100644 index e60840c..0000000 --- a/DEV_LOOP_STATUS.txt +++ /dev/null @@ -1,25 +0,0 @@ -Last updated: 2026-05-15 (dev-loop run) -Coverage (origin/main): 54.1% cmd/review-bot - -## Open Issues -- #143: bug: doc-map config loaded from PR branch (untrusted) → IN PR #153 -- #150: fix: validateDocmapPath — add EvalSymlinks → IN PR #152 -- #154: refactor: extract shared base-args helper in main_test.go (LOW PRIORITY, deferred NIT) - -## Closed This Run -- #144: bug: dev-loop merged PR autonomously → closed (fixed by #148 pure shell dispatch) -- #145: bug: merged despite REQUEST_CHANGES → closed (fixed by #148 pure shell dispatch) -- #146: missing subprocess tests → closed (fixed by PR #151 + comments) -- #147: coverage <50% → closed (54.1% on origin/main) - -## Open PRs (waiting for review/merge by Aaron) -- #151: test(#146): add InvalidDocMapPath/File tests (base: main) — labels: ai-review -- #152: fix(#150): EvalSymlinks dir-symlink bypass (base: main) — labels: needs-review -- #153: feat(#143): doc-map-trusted-ref (base: main, rebased on issue-146) — labels: needs-review - -## Merge Order -Recommended: #152 first (no deps), then #151, then #153 (rebased on issue-146, no conflict) - -## Notes -- PR #153 is rebased on issue-146 (which is the base for PR #151). Merge #151 before #153. -- PR #154 (refactor) is low priority — deferred NIT from PR #151 review. diff --git a/DEV_LOOP_STATUS_2026-05-15-1342.md b/DEV_LOOP_STATUS_2026-05-15-1342.md deleted file mode 100644 index 2c75f0b..0000000 --- a/DEV_LOOP_STATUS_2026-05-15-1342.md +++ /dev/null @@ -1,51 +0,0 @@ -# Dev-Loop: Status Report — 2026-05-15 13:42 UTC - -**Cycle ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c - -## Cycle Summary - -✅ **All systems operational. No action required.** - -### Current State -- **Commit:** Latest main synced with origin -- **Test Status:** 100% pass rate (all 7 packages) -- **Coverage:** 76.7% -- **Open Issues:** 0 -- **Open PRs:** 0 -- **Uncommitted Changes:** None - -### v0.4.0 Release Status -- Release CHANGELOG prepared -- 4 PRs merged in previous cycle -- Security hardening, test coverage, and doc-map trusted ref feature shipped -- Ready for tag and publish when Aaron approves - -## Recommended Next Steps - -### High Priority -1. **Integration test suite** — Expand CLI entrypoint tests for real-world scenarios -2. **Performance audit** — Profile doc-map filtering on large diffs (>1000 files) - -### Medium Priority -3. **User documentation** — Write doc-map usage guide with examples -4. **Backlog review** — Check for community feedback or feature requests - -## Metrics This Cycle - -| Metric | Value | Status | -|--------|-------|--------| -| Test Pass Rate | 100% | ✅ | -| Coverage | 76.7% | ✅ | -| Open Issues | 0 | ✅ | -| Open PRs | 0 | ✅ | - -## Ready For -- ✅ Next feature work -- ✅ Performance optimization -- ✅ Documentation expansion -- ✅ Release publishing - ---- - -**Next Automated Check:** 2026-05-15 17:42 UTC (4-hour interval) -**Status:** 🟢 READY FOR WORK diff --git a/DEV_LOOP_SUMMARY.md b/DEV_LOOP_SUMMARY.md deleted file mode 100644 index 68308e5..0000000 --- a/DEV_LOOP_SUMMARY.md +++ /dev/null @@ -1,139 +0,0 @@ -# Dev Loop Cycle Summary — 2026-05-15 09:37 UTC - -## Cycle Report - -**Cycle ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c -**Duration:** 4-hour scheduled run -**Runtime Status:** ✅ COMPLETE -**Overall Health:** ✅ EXCELLENT - ---- - -## Key Findings - -### 1. Repository Health -- ✅ Main branch is current with origin/main -- ✅ Working tree clean, no uncommitted changes -- ✅ All 77+ tests passing -- ✅ Coverage improved to **77.1%** (↑6.7% from previous cycle) -- ✅ No merge conflicts or stale branches in active development - -### 2. Recent Merges & Completions -- ✅ Issue #130 (GitHub PR reviews): Fully integrated into main - - 4 commits cherry-picked from review-bot-issue-130-work - - All self-review findings addressed - - Verified: main includes all fixes -- ✅ Issue #137 (doc-map features): Previously completed, now stable -- ✅ Issue #141 (validate-docmap): Completed, security hardened - -### 3. Active Ready Issues - -| Issue | Type | Commits | Status | Blocker? | -|-------|------|---------|--------|----------| -| #143 | Feature | 1 | Review-ready | None | -| #146 | Fix | 2 | Review-ready | None | -| #150 | Security | 1 | Review-ready | None | -| #154 | Refactor | 2 | Review-ready | None | - -**All issues are decoupled and can merge in any order.** - ---- - -## Metrics - -### Test Coverage -``` -Total Coverage: 77.1% (↑ from 70.4%) -Cmd/review-bot: TBD (tracking separately) -Budget: 91.8% (stable) -Review: 91.5% (stable) -LLM: 81.3% (stable) -Internal packages: ~85% (estimated) -``` - -### Test Results -``` -Total Tests: 77 -Passed: 77 ✅ -Failed: 0 -Skipped: 0 -Timeout: 0 -``` - -### Linting & Formatting -``` -go fmt: ✅ pass -go vet: ✅ pass (no blockers) -``` - ---- - -## Recommendations - -### For Aaron (Maintainer) - -**Merge Priority (suggested):** -1. **#150** (EvalSymlinks) — Security fix, should land first -2. **#143** (doc-map config) — Feature, complements #150 -3. **#146** (path resolution) — Optimization, no risk -4. **#154** (test refactor) — Low-risk cleanup - -**Pre-merge checklist:** -- [ ] Review each PR for design alignment -- [ ] Run `go test -v ./...` locally on each branch -- [ ] Check for dependency order (test separately if needed) -- [ ] Rebase each onto main before merge to avoid unclean history - -### For Dev-Loop (Automated) - -**Next cycle (4 hours from now):** -1. Re-verify main is still current -2. Re-run test suite (regression check) -3. Measure coverage again (track trend) -4. Check if any PRs merged (update local tracking) -5. Flag any coverage drops or new test failures - -**Long-term (next week):** -- Analyze cmd/review-bot coverage gaps (36.8% → target 60%+) -- Consider integration/e2e tests for main CLI logic -- Review SKILL.md documentation accuracy -- Suggest follow-up issues from current backlog - ---- - -## Backlog Overview - -### Completed (In Main) -- ✅ Issue #130 — GitHub PR review API + VCS routing -- ✅ Issue #137 — doc-map feature validation -- ✅ Issue #141 — validate-docmap subcommand (hardened) - -### Ready to Review (4 Issues) -- ⏳ Issue #143 — fetch doc-map config from trusted VCS ref -- ⏳ Issue #146 — reuse resolved doc-map path early (optimization) -- ⏳ Issue #150 — EvalSymlinks security fix -- ⏳ Issue #154 — test refactoring/cleanup - -### Queued for Triage -- 📋 Issue #139, #148, others from `origin/review-bot-issue-*` branches - ---- - -## Artifacts - -- **Coverage report:** `coverage.out` (77.1%) -- **Status:** This file + `DEV_LOOP_STATUS.md` -- **Latest commit:** ffbbdf5 (status update pushed to main) - ---- - -## Notes - -- Significant improvement in coverage (+6.7%) suggests good test additions in active branches -- All security-sensitive branches (143, 146, 150) are ready for human review -- No urgent issues blocking development pipeline -- Repo is in excellent shape for next phase of work - ---- - -_This cycle completed successfully at 2026-05-15 09:37 UTC._ diff --git a/PLAN-125.md b/PLAN-125.md deleted file mode 100644 index 29ac82c..0000000 --- a/PLAN-125.md +++ /dev/null @@ -1,175 +0,0 @@ -# Plan: Issue #125 — Rename GITEA_URL → VCS_URL - -## Problem - -The `GITEA_URL` environment variable (and `--gitea-url` flag) implies the binary only works with Gitea. -Now that review-bot supports both Gitea and GitHub/GHES, this name is misleading. -Renaming to `VCS_URL` makes the binary platform-agnostic in its interface. - -## Constraints - -- Must not break existing users who already use `GITEA_URL` — need a fallback -- The CLI flag `--gitea-url` should also be updated to `--vcs-url` for consistency -- `INTEGRATION_GITEA_URL` in integration tests is a test-only env var, not the binary's interface; but should be updated for clarity -- The action YAML uses `GITEA_URL` as an internal shell variable in bash scripts — distinct from the env var passed to the binary -- All changes must compile and pass existing tests - -## Files Affected - -### Binary / Go source -| File | Change | -|------|--------| -| `cmd/review-bot/main.go` | Rename `--gitea-url` → `--vcs-url`, add `VCS_URL` as primary, keep `GITEA_URL` fallback | -| `cmd/review-bot/integration_test.go` | Rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` (test-only, no external compat concern) | -| `integration_test.go` | Same — rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` | - -### Action YAML -| File | Change | -|------|--------| -| `.gitea/actions/review/action.yml` | Rename input `gitea-url` → `vcs-url`; update env var passed to binary: `VCS_URL` instead of `GITEA_URL`; keep internal bash var as `GITEA_URL` (only used for release download, not passed to binary) | -| `.gitea/workflows/ci.yml` | Rename `GITEA_URL` env var to `VCS_URL` in Run review step | - -### Documentation -| File | Change | -|------|--------| -| `README.md` | Update CLI example, env var table entry | - -## Proposed Approach - -### 1. Backward-compatible env var lookup in main.go - -Replace: -```go -giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL") -``` - -With: -```go -giteaURL := flag.String("vcs-url", envOrDefaultFallback("VCS_URL", "GITEA_URL", ""), "VCS server URL (e.g. https://gitea.example.com)") -``` - -Add a helper: -```go -// envOrDefaultFallback reads primary env var; if empty, falls back to deprecated env var. -func envOrDefaultFallback(primary, deprecated, defaultVal string) string { - if v := os.Getenv(primary); v != "" { - return v - } - if v := os.Getenv(deprecated); v != "" { - slog.Warn("deprecated env var in use; rename to " + primary, "old", deprecated, "new", primary) - return v - } - return defaultVal -} -``` - -**Note:** This must be called AFTER `setupLogger` conceptually, but the flag default is evaluated at flag registration time. Since `setupLogger` runs before `flag.Parse()`, the slog.Warn will print correctly at runtime. We use `log.Printf` as a fallback if this proves problematic. - -Actually — flag defaults are evaluated at registration (line 57), before `setupLogger`. The warning won't go through slog. Two options: -- Use `log.Printf` for the deprecation warning (always visible) -- Move the fallback lookup to after `flag.Parse()`, checking if the parsed value is still empty - -**Decision:** Move fallback to a post-parse check. This is cleaner: -```go -vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL") -flag.Parse() -// Backward compat: fall back to deprecated GITEA_URL -if *vcsURL == "" { - if v := os.Getenv("GITEA_URL"); v != "" { - slog.Warn("GITEA_URL is deprecated; use VCS_URL instead") - *vcsURL = v - } -} -``` - -This is clean, idiomatic, and the warning goes through slog correctly. - -### 2. Keep `--gitea-url` as deprecated alias - -Add a hidden flag for backward compat: -```go -giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url") -``` - -Post-parse: -```go -if *vcsURL == "" && *giteaURLAlias != "" { - slog.Warn("--gitea-url is deprecated; use --vcs-url instead") - *vcsURL = *giteaURLAlias -} -``` - -### 3. Internal variable rename - -Rename `giteaURL` local variable → `vcsURL` throughout `main.go` for consistency. - -### 4. Error message update - -```go -fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n") -``` - -### 5. Action YAML changes - -In `.gitea/actions/review/action.yml`: -- Input `gitea-url` → `vcs-url` (with same description, `required: false`, `default: ''`) -- Line 172: `GITEA_URL: ${{ inputs.gitea-url || github.server_url }}` → `VCS_URL: ${{ inputs.vcs-url || github.server_url }}` -- Lines 115, 140: internal bash vars `GITEA_URL=` are used for downloading binaries — NOT passed to the review-bot binary. Leave them as internal bash vars (they're scope-local in bash). These could be renamed to `SERVER_URL` or `BASE_URL` for local clarity, but renaming them isn't strictly required. - -In `.gitea/workflows/ci.yml`: -- Line 52: `GITEA_URL: ${{ github.server_url }}` → `VCS_URL: ${{ github.server_url }}` - -### 6. Integration test updates - -`INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` in both test files. - -### 7. README - -- CLI example: `--gitea-url` → `--vcs-url` -- Env var table: `GITEA_URL` → `VCS_URL`, add note about `GITEA_URL` fallback - -## Backward Compatibility Summary - -| Old | New | Fallback? | -|-----|-----|-----------| -| `GITEA_URL` env var | `VCS_URL` | ✅ with deprecation warning | -| `--gitea-url` flag | `--vcs-url` | ✅ with deprecation warning | -| `gitea-url` action input | `vcs-url` | ⚠️ No (action version bump handles this) | -| `INTEGRATION_GITEA_URL` | `INTEGRATION_VCS_URL` | N/A (test-only) | - -## Error Cases - -- Both `VCS_URL` and `GITEA_URL` set: `VCS_URL` wins (primary takes precedence) -- Both `--vcs-url` and `--gitea-url` provided: `--vcs-url` wins -- Neither set: existing "missing required flags" error unchanged - -## Edge Cases - -- `os.Getenv` returns "" for unset AND set-to-empty — consistent with existing behavior -- The `envOrDefault` helper is unchanged; we add `envOrDefaultFallback` for the one renamed var - -## Testing Strategy - -- Existing unit tests pass unchanged (they don't test env var parsing directly) -- Integration tests updated to use new env var name -- Manual: `GITEA_URL=https://example.com ./review-bot --repo x --pr 1 ...` should print deprecation warning and proceed -- Manual: `VCS_URL=https://example.com ./review-bot ...` should work silently - -## Completion Checklist - -1. `VCS_URL` is read first; `GITEA_URL` is fallback with deprecation warning -2. `--vcs-url` flag is primary; `--gitea-url` is deprecated alias with warning -3. Error message references `--vcs-url` not `--gitea-url` -4. `action.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary -5. `ci.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary -6. README updated in CLI example and env var table -7. Integration tests use `INTEGRATION_VCS_URL` -8. `go test ./...` passes -9. `go vet ./...` passes -10. `go build ./cmd/review-bot` succeeds - -## Open Questions - -- Should the CLI flag `--gitea-url` be completely hidden from `--help` or just deprecated with a note? The issue doesn't specify. Decision: keep it visible but add "(deprecated: use --vcs-url)" to the description. -- Should action.yml also add `gitea-url` as a deprecated input alias? The issue says "Update the action to pass the new env var name" — no mention of backward compat for the action input. Decision: rename only, no alias (action users pin a version anyway). -- The bash-internal `GITEA_URL` variable in action.yml scripts (used for release download, not passed to binary) — rename for clarity? Decision: yes, rename to `BASE_URL` to avoid confusion with the env var. diff --git a/PLAN-141.md b/PLAN-141.md deleted file mode 100644 index 1a2cf7c..0000000 --- a/PLAN-141.md +++ /dev/null @@ -1,154 +0,0 @@ -# Plan: validate-docmap subcommand (Issue #141) - -## Problem - -CI has no way to verify that `doc-map.yml` is kept up to date. When a developer adds a new -module/directory, they may forget to add a `paths:` entry. When a design doc is deleted or -moved, the `docs:` entry becomes stale. Both failures are silent — the AI reviewer just gets -no docs injected, and nobody notices. - -This is a **pure static check**: no AI, no VCS API. Just YAML parsing + glob matching + `os.Stat`. - -## Constraints - -- No external API calls or AI involvement -- Must compose with `git diff --name-only` output via stdin (standard CI pattern) -- Reuse existing `ParseDocMapConfig` from `review/docmap.go` -- Glob matching logic must also reuse (or expose) existing `globMatch`/`mappingMatches` -- Follow the `validate-url` subcommand pattern exactly -- Both checks must always run — report all failures, not just the first -- `outWriter`/`errWriter` vars must be respected for testability - -## Proposed Approach - -### 1. Export a glob-coverage helper from `review/docmap.go` - -Add one new exported function: - -```go -// FileCoveredByDocMap returns true if any paths: glob in cfg matches the given file. -func FileCoveredByDocMap(cfg *DocMapConfig, file string) bool -``` - -This is a thin wrapper over the existing unexported `mappingMatches`. It lets the `cmd/` layer -call into the review package without duplicating glob logic. - -**Alternative considered:** Duplicate the loop in `cmd/`. Rejected — duplication of non-trivial -glob matching is a maintenance hazard. Exporting one function is cleaner. - -### 2. New file: `cmd/review-bot/validatedocmap.go` - -Implements `runValidateDocmap(args []string) int` following the `validateurl.go` pattern. - -``` -Flag parsing (use flag.NewFlagSet — NOT global flag, to avoid polluting main.go's flag state): - --docmap (required) path to YAML file - --repo-root (optional, default ".") base for resolving docs: paths - -Step 1: Parse flags. Validate --docmap is set. Exit 2 on error. -Step 2: ParseDocMapConfig(docmapPath) → exit 2 on parse error -Step 3: Read stdin lines → changedFiles []string -Step 4: Coverage check — for each file in changedFiles: - if !FileCoveredByDocMap(cfg, file) → record as uncovered -Step 5: Stale-docs check — for each unique docs: entry across all mappings: - if os.Stat(filepath.Join(repoRoot, docPath)) fails → record as stale -Step 6: If any uncovered or stale entries → print ERROR sections → return 1 - Else → print "OK" → return 0 -``` - -Exit codes (parallel to `validate-url`): -- `0` — clean -- `1` — coverage or stale-doc failures -- `2` — usage error, missing flag, or YAML parse error - -### 3. Wire into `main.go` - -Add `case "validate-docmap":` to the existing `os.Args[1]` switch. - -### 4. Tests: `cmd/review-bot/validatedocmap_test.go` - -Test table covering: -| Case | stdin | docmap | repo-root | want exit | -|------|-------|--------|-----------|-----------| -| clean | covered file | valid docmap | docs exist | 0 | -| uncovered file | uncovered file | valid docmap | docs exist | 1 | -| stale doc | covered file | stale docs: | missing path | 1 | -| both failures | uncovered + stale | | | 1 | -| empty stdin | (empty) | valid docmap | docs exist | 0 | -| missing --docmap flag | | | | 2 | -| bad YAML | | invalid YAML | | 2 | - -Use `os.MkdirTemp` + `os.WriteFile` to create real temp directories for the stale-docs check. - -### 5. README update - -Add a subsection under the `validate-url` section showing the `validate-docmap` invocation. - -## State/Data Model - -No persistent state. All inputs are flags + stdin + local filesystem. - -## Error Cases - -| Scenario | Behavior | -|----------|----------| -| `--docmap` flag missing | Print usage, exit 2 | -| YAML parse fails | Print error message, exit 2 | -| stdin read error | Print error, exit 2 | -| `--repo-root` does not exist | Individual docs: entries will fail Stat; logged per-path, exit 1 | -| changed file is empty string (blank line) | Skip (trim + ignore empty) | - -## Edge Cases - -- Blank lines in stdin input (from git diff with trailing newline) → trim and skip -- Duplicate `docs:` entries across multiple mappings → deduplicate before checking existence -- `docs:` entry that is a directory (ends with `/`) → `os.Stat` the path; if it exists it's fine -- `--repo-root` with trailing slash → use `filepath.Join` which normalizes it -- Changed files with `../` or absolute paths → check only (no traversal needed here since we're just calling `FileCoveredByDocMap`, which is pure string matching) - -## Testing Strategy - -- Unit tests with real temp files for stale-doc check (no mocking needed for `os.Stat`) -- `outWriter`/`errWriter` capture pattern (same as `validateurl_test.go`) -- Table-driven tests - -## Open Questions - -- **stdin vs `--files` flag**: Using stdin matches the standard CI pipe idiom and avoids shell - quoting issues with many files. Confirmed by Aaron's clarification. -- **Empty stdin coverage**: Aaron said empty stdin = no coverage failures. This means - "no changed files, no problem" — vacuously true. Makes sense for `git diff` on unchanged branches. -- **Directory docs: entries**: `os.Stat` is sufficient — if the directory exists, it's valid. - We don't recursively verify it has `.md` files. Kept simple. -- **`--repo-root` vs always cwd**: Default to cwd but allow override. This makes the command - usable from CI scripts that `cd` to a different directory. - -## Completion Checklist (generated for this task) - -1. `FileCoveredByDocMap` exported and covers the all-mappings, any-glob-matches logic correctly? -2. `runValidateDocmap` follows `runValidateURL` exactly: flag parse → validate → work → exit code? -3. Both checks always run (no early exit after first failure section)? -4. Empty stdin treated as clean (exit 0, no coverage errors)? -5. All `docs:` entries deduplicated before stale check? -6. `outWriter`/`errWriter` used (not `fmt.Println` directly), so tests can capture output? -7. `case "validate-docmap":` added to `main.go` dispatch switch? -8. Tests cover all 7 cases in the table above? -9. README updated with usage example? -10. `go test ./...` passes with no new failures? - -## Implementation Phases - -### Phase 1: Export helper in `review/docmap.go` -- Add `FileCoveredByDocMap(cfg *DocMapConfig, file string) bool` -- Add test in `review/docmap_test.go` - -### Phase 2: `cmd/review-bot/validatedocmap.go` -- Full `runValidateDocmap` implementation - -### Phase 3: Wire into `main.go` + tests -- `case "validate-docmap":` dispatch -- `validatedocmap_test.go` with full table - -### Phase 4: README + final -- Update README -- `go test ./...` diff --git a/PLAN-143.md b/PLAN-143.md deleted file mode 100644 index 1d8e521..0000000 --- a/PLAN-143.md +++ /dev/null @@ -1,125 +0,0 @@ -# PLAN-143: Load doc-map config from trusted (default) branch - -**Issue:** #143 -**Status:** Planning -**Branch:** TBD (issue-143) - ---- - -## Problem Statement - -The `--doc-map` flag reads the doc-map YAML config from the local `GITHUB_WORKSPACE` checkout, which is the **PR branch** in CI. A malicious PR author can: - -1. Modify `.review-bot/doc-map.yml` in their branch to map any path glob to sensitive docs -2. review-bot reads the PR-branch doc-map config -3. Docs from the **default branch** are fetched and injected into the LLM prompt -4. Via prompt injection in those docs, the attacker could exfiltrate content - -The config is the trust boundary. The *data* fetched (design docs) already comes from the default branch via VCS API. The *config* is what needs to be pinned to the default branch. - -## Constraints - -- Must not break existing callers (backward compatibility) -- Should have a clearly named flag/env var -- Fall back to local workspace if no trusted ref configured (for users not yet migrated) -- The gargoyle workflow (.github/workflows/review.yml) will need updating - -## Proposed Approach - -### Option A: Fetch via VCS API from default branch (preferred) - -Add a new flag `--doc-map-trusted-ref` (default: `""` = use local workspace). - -When `--doc-map-trusted-ref` is set: -1. Use the VCS API to fetch the file at `--doc-map` path from the specified ref -2. Parse the fetched content as YAML -3. Use this config (not the local workspace copy) - -When `--doc-map-trusted-ref` is empty: -- Current behavior (local workspace) with a deprecation warning - -This follows the same pattern as `patterns-repo` which fetches from VCS. - -### Option B: Auto-detect and always use default branch - -Always fetch doc-map from the default branch via VCS API, ignoring local workspace. -Simpler API but breaks local testing (where there's no VCS to fetch from). - -### Recommendation - -Option A — explicit `--doc-map-trusted-ref` flag. The gargoyle workflow would set: -```yaml -doc-map-trusted-ref: "main" -``` - -This is explicit and allows local testing to continue using local workspace. - -## Implementation Plan - -### Phase 1: VCS API fetch for doc-map config - -**Files to change:** -- `cmd/review-bot/main.go` — add `--doc-map-trusted-ref` flag, conditional fetch logic -- `review/docmap.go` — add `FetchDocMapConfig(vcs, owner, repo, ref, path string) (*DocMapConfig, error)` -- `action.yml` — add `doc-map-trusted-ref` input -- `README.md` — document new flag - -**Logic:** -```go -if *docMapTrustedRef != "" { - // Fetch from VCS (trusted branch) — secure - content, err := vcs.GetFileContent(ctx, owner, repoName, *docMapTrustedRef, resolvedDocMap) - ... - docMapCfg, err = review.ParseDocMapConfigContent(content) -} else { - // Local workspace (backward compat with deprecation warning) - slog.Warn("doc-map loaded from local workspace (PR branch) — consider --doc-map-trusted-ref for security") - docMapCfg, err = review.ParseDocMapConfig(resolvedDocMap) -} -``` - -### Phase 2: Tests - -- `TestFetchDocMapConfig_Success`: mock VCS returns valid YAML → parses correctly -- `TestFetchDocMapConfig_NotFound`: VCS returns 404 → clear error -- `TestMainSubprocess_DocMapTrustedRef`: subprocess test for the new flag - -### Phase 3: Gargoyle workflow update - -Update `.github/workflows/review.yml` in gargoyle to add `doc-map-trusted-ref: main`. - -## State/Data Model - -New flag: `--doc-map-trusted-ref` / `DOC_MAP_TRUSTED_REF` env var -- Type: string -- Default: `""` (local workspace) -- Example value: `"main"`, `"master"`, `HEAD` - -## Error Cases - -- VCS returns 404 for doc-map path at trusted ref → error + exit (not silent) -- VCS returns 404 but local copy exists → do NOT fall back (could be attack path) -- Parse error on fetched content → error + exit - -## Edge Cases - -- What if the doc-map doesn't exist at the trusted ref? → log error, exit (don't silently continue) -- What if trusted-ref is a commit SHA? → should work via VCS GetFileContent -- What if the user sets trusted-ref to the PR branch? → Works, but defeats the purpose. Not our problem to prevent. - -## Open Questions - -- Should we warn when `--doc-map` is set without `--doc-map-trusted-ref`? → Yes, deprecation warning pointing to docs -- Should we add `--doc-map-trusted-ref` to the `validate-docmap` subcommand? → No, that subcommand operates on local files only; it's a developer tool - -## Acceptance Criteria - -- [ ] `--doc-map-trusted-ref` flag added to `action.yml` and `cmd/review-bot/main.go` -- [ ] When set, doc-map config fetched from VCS at the specified ref (not local workspace) -- [ ] When unset, local workspace used with deprecation warning in logs -- [ ] 404 from VCS is a hard error (no silent fallback to local copy) -- [ ] Tests cover: fetch success, fetch 404, parse error -- [ ] Gargoyle `.github/workflows/review.yml` updated to use `doc-map-trusted-ref: main` -- [ ] README updated -- [ ] CHANGELOG updated -- [ ] `make precommit` passes diff --git a/docs/DESIGN-137-doc-map.md b/docs/DESIGN-137-doc-map.md deleted file mode 100644 index 02f2d13..0000000 --- a/docs/DESIGN-137-doc-map.md +++ /dev/null @@ -1,82 +0,0 @@ -# Design: doc-map input for path-scoped design doc injection (Issue #137) - -## Problem - -review-bot can inject context via `patterns-repo` (external VCS repos) and `conventions-file` -(a single file from the reviewed repo). There is no mechanism to inject local repo documentation -files scoped to the paths changed in a PR. - -First consumer: `grgl/gargoyle#778` needs a "doc adherence" reviewer that checks code against the -module's governing design doc, without injecting every doc in the tree. - -## Approach - -### New: `doc-map` input - -A `.review-bot/doc-map.yml` config file in the reviewed repo maps source path globs to governing -design docs. review-bot reads the map, intersects it with changed PR paths, and injects only the -relevant docs into the system prompt. - -### Config format - -```yaml -mappings: - - paths: - - "lib/gargoyle/engine/signal_risk/**" - docs: - - docs/domain/contexts/risk/risk-controls.md - - paths: - - "lib/gargoyle/trading/**" - docs: - - docs/domain/contexts/trading/ -``` - -- `paths` — glob patterns (including `**`) matched against changed file paths in the PR -- `docs` — file paths or directory paths (all `.md` files under a directory) to inject -- Docs are deduplicated across mappings - -### Architecture - -| Component | Description | -|-----------|-------------| -| `review/docmap.go` | YAML parsing, glob matching with `**` support, doc loading via VCS | -| `cmd/review-bot/main.go` | Step 6c: parses config, intersects with changed files, calls LoadMatchingDocs | -| `budget/budget.go` | New `DesignDocs` section — injected after Conventions in system prompt | -| `action.yml` | `doc-map` and `doc-map-max-bytes` inputs, wired to `DOC_MAP_FILE`/`DOC_MAP_MAX_BYTES` | - -### Doc file loading - -- The `doc-map` YAML file is read from the local workspace (like `system-prompt-file`). -- Doc files listed in the config are fetched via VCS API (same as `conventions-file`), - enabling them to be loaded from any branch without a local checkout. -- `GetAllFilesInPath` is tried first; if it returns files, they are treated as a directory listing. - If it returns empty, `GetFileContent` is tried as a fallback (single file). - -### Glob matching - -`**` is implemented by splitting patterns and paths on `/`, then matching segment-by-segment. -A `**` segment consumes zero or more path segments (not just one level like `*`). - -### Budget integration - -`DesignDocs` is added to `budget.Sections` between `Conventions` and `FileContext`. -Trim order: Patterns → Conventions → DesignDocs → FileContext → Diff. -Design docs appear in the system prompt under `## Design Documents`. - -### Context size guard - -Default: 100 KB. Configurable via `--doc-map-max-bytes` / `DOC_MAP_MAX_BYTES`. -Truncation is noted inline with a `⚠️` message. - -## Error handling - -| Situation | Behavior | -|-----------|----------| -| `--doc-map` file not found | Fatal error (like `--system-prompt-file`) | -| `--doc-map` file invalid YAML | Fatal error with descriptive message | -| Unknown YAML keys | Log warning, continue | -| Doc file not found in VCS | Log warning, skip | -| Doc directory empty or no `.md` files | Log debug, skip | -| Total size exceeds limit | Truncate with notice, log warning | -| No changed paths match any mapping | No docs injected, review runs normally | -| `paths` or `docs` list empty in a mapping | Skip that mapping | diff --git a/docs/DESIGN-51-personas.md b/docs/DESIGN-51-personas.md deleted file mode 100644 index b865dcf..0000000 --- a/docs/DESIGN-51-personas.md +++ /dev/null @@ -1,334 +0,0 @@ -# Design: Role-based Review Personas (Issue #51) - -> **Note:** This design was revised during implementation to use JSON instead of YAML -> to maintain the repository's zero-external-dependencies convention. All persona -> files use JSON format. See "Design Revision" section at the end for details. - -## Problem - -Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to: - -1. **Redundancy** — Two reviewers (e.g., GPT + Claude twins) often flag identical issues -2. **Gaps** — Generic reviewers miss specialized concerns (security, domain logic, architecture) -3. **Noise** — NITs about style mixed with critical security findings -4. **No ownership** — Findings lack clear domain attribution - -## Constraints - -- Must work with existing CLI flags and CI workflow patterns -- Must not break backwards compatibility (existing configs still work) -- Must integrate cleanly with the budget system (personas add to context) -- Multiple personas running in parallel must not interfere with each other -- Each persona must have clear scope boundaries (no duplication) - -## Proposed Approach - -### 1. Persona Definition - -A persona is a named review role with: -- **Identity** — Who am I? What's my expertise? -- **Focus** — What do I look for? -- **Scope boundaries** — What do I explicitly NOT comment on? -- **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain? - -Personas are defined in JSON files that can live: -1. In the pattern repos (shared across projects) -2. In the target repo (project-specific personas) -3. Inline via a new `--persona-file` flag (JSON format) - -### 2. Persona File Format - -```json -# .review/personas/security.yaml -name: security -display_name: Security Specialist -model_preference: opus # optional hint for expensive analysis - -identity: | - You are a security specialist reviewing code for vulnerabilities. - Your expertise: OWASP Top 10, injection attacks, auth/authz, secrets management, - event sourcing security (replay attacks, event injection). - -focus: - - Injection attacks (SQL, command, path traversal, template) - - Authentication and authorization gaps - - Secrets exposure (hardcoded credentials, tokens in logs) - - Input validation (unsanitized input, unsafe deserialization) - - Race conditions with security implications - - Event sourcing attack vectors - -ignore: - - Code style and naming conventions - - Performance (unless security-related) - - Documentation - - General code quality - - Test coverage - -severity: - critical: "Remote code execution, auth bypass, data exfiltration" - major: "Privilege escalation, information disclosure, DoS" - minor: "Missing rate limiting, verbose errors" - nit: "Theoretical risk with low exploitability" - -output_format: | - For each finding: - - Severity: [CRITICAL|MAJOR|MINOR|NIT] - - Attack vector: How could this be exploited? - - Evidence: Code snippet showing the vulnerability - - Recommendation: Specific fix -``` - -### 3. New CLI Flags - -``` ---persona-file PATH Path to persona JSON file (local or in repo) ---persona NAME Built-in persona name (security, architect, domain) -``` - -Either flag sets the persona. If neither is provided, behavior is unchanged (generic review). - -### 4. Prompt Assembly - -Current flow: -``` -SystemBase → Patterns → Conventions → [LLM] -``` - -New flow with persona: -``` -PersonaPrompt (from YAML) → Patterns (filtered?) → Conventions → [LLM] -``` - -The persona's identity/focus/ignore/severity sections become the system prompt, replacing the generic "You are an expert code reviewer" base. - -### 5. Built-in Personas - -Ship with these built-in personas (loadable via `--persona NAME`): - -| Name | Focus | -|------|-------| -| `security` | Vulnerabilities, auth, secrets | -| `architect` | Patterns, consistency, design | -| `domain` | Business logic (requires repo-specific config) | -| `docs` | Documentation, API clarity | - -Built-in personas live in `review/personas/` as embedded Go assets or YAML shipped with the binary. - -### 6. CI Workflow Integration - -Single persona: -```yaml -- uses: rodin/review-bot/.gitea/actions/review@v1 - with: - reviewer-name: security - persona: security - ... -``` - -Multiple personas (parallel jobs): -```yaml -jobs: - review: - strategy: - matrix: - include: - - name: security - persona: security - - name: architect - persona: architect - steps: - - uses: rodin/review-bot/.gitea/actions/review@v1 - with: - reviewer-name: ${{ matrix.name }} - persona: ${{ matrix.persona }} -``` - -Custom persona from repo: -```yaml -- uses: rodin/review-bot/.gitea/actions/review@v1 - with: - reviewer-name: trading - persona-file: .review/personas/trading.yaml -``` - -### 7. Persona + Patterns Interaction - -Some personas benefit from filtered patterns: -- Security → only security-related patterns -- Architect → all patterns (structural focus) -- Domain → domain docs, not language patterns - -For v1, keep it simple: all patterns are included regardless of persona. Future enhancement could add `patterns_filter` to persona YAML. - -### 8. Output Format Changes - -Persona name appears in the review header: -```markdown -# Security Review - -## Summary -No critical vulnerabilities found in this change. - -## Findings -| # | Severity | File | Line | Finding | -... - -## Recommendation -**APPROVE** — No security-relevant issues detected. - ---- -*Review by security* - -``` - -## State/Data Model - -### Persona struct - -```go -// review/persona.go -type Persona struct { - Name string `yaml:"name"` - DisplayName string `yaml:"display_name"` - ModelPref string `yaml:"model_preference,omitempty"` - Identity string `yaml:"identity"` - Focus []string `yaml:"focus"` - Ignore []string `yaml:"ignore"` - Severity Severity `yaml:"severity"` - OutputFormat string `yaml:"output_format,omitempty"` -} - -type Severity struct { - Critical string `yaml:"critical"` - Major string `yaml:"major"` - Minor string `yaml:"minor"` - Nit string `yaml:"nit"` -} -``` - -### Loading precedence - -1. `--persona-file PATH` → load from local file system -2. `--persona NAME` → load from embedded built-ins -3. Neither → use generic system prompt (current behavior) - -## Error Cases - -| Error | Handling | -|-------|----------| -| Persona file not found | Fatal exit with clear message | -| Invalid YAML in persona file | Fatal exit with parse error | -| Both `--persona` and `--persona-file` specified | Fatal exit: mutually exclusive | -| Unknown built-in persona name | Fatal exit with list of valid names | -| Empty identity in persona | Warning, fall back to generic prompt | - -## Edge Cases - -- **Empty focus list**: Valid — persona relies on identity alone -- **Empty ignore list**: Valid — no explicit scope exclusions -- **No severity section**: Use default MAJOR/MINOR/NIT definitions -- **Model preference set but budget insufficient**: Ignore preference, log warning -- **Persona file in pattern repo**: Fetch like other pattern files - -## Testing Strategy - -### Unit tests -- `persona_test.go`: Parse valid/invalid YAML, validate required fields -- `prompt_test.go`: Verify persona prompt assembly -- Integration with budget: persona prompts count toward token limit - -### Integration tests -- End-to-end with `--persona security` (built-in) -- End-to-end with `--persona-file custom.yaml` -- Backwards compatibility: no flags = generic behavior - -### Manual verification -- Run security persona on a PR with obvious vulnerability -- Verify security persona ignores style issues -- Verify non-security persona doesn't flag security issues - -## Implementation Phases - -### Phase 1: Persona types and loading -- [ ] `review/persona.go`: Persona struct + YAML parsing -- [ ] `review/persona_test.go`: Unit tests -- [ ] Embed built-in personas in binary -- [ ] Compiles clean, tests pass - -### Phase 2: Prompt generation -- [ ] `review/prompt.go`: `BuildPersonaPrompt(p Persona) string` -- [ ] Modify `BuildSystemBase()` to accept optional persona -- [ ] Integrate persona prompt with budget system -- [ ] Tests for prompt assembly - -### Phase 3: CLI integration -- [ ] Add `--persona` and `--persona-file` flags -- [ ] Flag validation (mutually exclusive, valid names) -- [ ] Load persona based on flags -- [ ] Pass persona to prompt builder - -### Phase 4: Action integration -- [ ] Add `persona` and `persona-file` inputs to action.yml -- [ ] Update README with persona examples -- [ ] End-to-end CI test - -### Phase 5: Built-in personas -- [ ] `security.yaml` built-in -- [ ] `architect.yaml` built-in -- [ ] `docs.yaml` built-in -- [ ] Document each persona's focus - -## Open Questions - -1. **Persona file location in repo**: Should we support `--persona-file .review/security.yaml` where the file is fetched from the PR's repo (like conventions)? This adds complexity but enables project-specific personas without action changes. - -2. **Model preference enforcement**: If persona specifies `model_preference: opus` but the action uses a different model, should we warn? Override? Ignore? Current thinking: log warning, use the specified model (user controls model via action input). - -3. **Severity override output**: If persona defines custom severity levels (CRITICAL), should the JSON output include them, or map back to standard MAJOR/MINOR/NIT? Current thinking: keep standard output format, use severity calibration only for prompt guidance. - -## Completion Checklist - -1. Persona struct matches YAML schema exactly? -2. Built-in personas embedded in binary (not external files)? -3. `--persona` and `--persona-file` are mutually exclusive? -4. Unknown persona name produces clear error with valid options? -5. Empty persona file fields have sensible defaults? -6. Persona prompt integrates with budget system (token counting)? -7. Backwards compatibility: no flags = current behavior? -8. Review header shows persona display name? -9. Sentinel still uses reviewer-name (not persona name)? -10. Unit tests cover parse errors, missing fields, valid YAML? - -## Design Review Findings (Self-Review) - -### Finding 1: Severity Mapping -The persona YAML allows `critical` severity, but the LLM output parser (`review/parser.go`) only accepts MAJOR/MINOR/NIT. - -**Resolution:** Keep standard output format. Persona severity section is ONLY for calibrating the LLM's judgment (prompt guidance). Output must still use MAJOR/MINOR/NIT. Document this clearly in persona format docs. - -### Finding 2: Embedding Built-in Personas -Go doesn't natively embed YAML. Must use `//go:embed` directive (Go 1.16+). - -**Resolution:** Create `review/personas/` directory with YAML files and use: -```go -//go:embed personas/*.yaml -var embeddedPersonas embed.FS -``` - -### Finding 3: display_name vs reviewer-name -Design says header shows "persona display name" but sentinel uses "reviewer-name". This is correct - they serve different purposes: -- `display_name` → human-readable header ("Security Specialist Review") -- `reviewer-name` → machine sentinel for cleanup (``) - -When persona is used, `display_name` takes precedence for the header title, but `reviewer-name` (CLI flag) is still used for the sentinel. - -## Design Revision: YAML with gopkg.in/yaml.v3 - -**Decision:** Add `gopkg.in/yaml.v3` as a dependency. - -YAML is preferred over JSON for persona files because: -- Multi-line strings are cleaner (no escaping quotes in identity/focus text) -- Comments are supported for documentation -- More human-readable for complex persona definitions - -The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas. diff --git a/docs/DESIGN-57-yaml-persona.md b/docs/DESIGN-57-yaml-persona.md deleted file mode 100644 index 067a9de..0000000 --- a/docs/DESIGN-57-yaml-persona.md +++ /dev/null @@ -1,87 +0,0 @@ -# Design: YAML Support for Persona Files (#57) - -## Problem - -JSON is awkward for persona files that contain multi-line text (identity, severity descriptions). YAML supports cleaner multi-line strings and comments, improving readability and maintainability. - -## Constraints - -- Backwards compatibility: existing JSON personas must continue to work -- Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486) -- Consistency: use `.yaml` extension (not `.yml`) -- Library: use `github.com/goccy/go-yaml` v1.16.0+ (approved in CONVENTIONS.md); we implement custom AST-based depth/node-count checks for precise alias-aware validation - -## Proposed Approach - -1. **Update `parsePersona`** to detect format from file extension -2. **Add YAML parsing** with explicit depth limit (defense in depth) -3. **Keep JSON as fallback** for files without `.yaml`/`.yml` extension -4. **Convert built-in personas** to YAML format -5. **Update embed directive** to include both formats - -### File Extension Detection - -```go -func parsePersona(data []byte, source string) (*Persona, error) { - isYAML := strings.HasSuffix(source, ".yaml") || strings.HasSuffix(source, ".yml") - if isYAML { - return parseYAML(data, source) - } - return parseJSON(data, source) -} -``` - -### YAML Parsing with Depth Protection - -We implement a custom AST-based depth/node-count walk (`checkYAMLDepth` in -`review/persona.go`) rather than relying on library decoder options. Key design -decisions: - -- **Library:** `github.com/goccy/go-yaml` with `ast.Node`-based traversal -- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection) -- **Node-count limit:** Conservative overcounting bounds total validation work -- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths - -See `review/persona.go:checkYAMLDepth` for the authoritative implementation. - -## State/Data Model - -No new state. Same `Persona` struct, just different parsing. - -## Error Cases - -| Error | Handling | -|-------|----------| -| Invalid YAML syntax | Return parse error with source file | -| Deeply nested YAML | Custom AST walk (`checkYAMLDepth`) rejects before decode | -| Unknown extension | Fall back to JSON parsing | -| Missing required fields | Validation rejects after parse | - -## Edge Cases - -- File with `.json` extension but YAML content → JSON parse fails, user sees error -- File with no extension → defaults to JSON -- Embedded persona reference like `builtin:security` → detect by embed path (`personas/X.yaml`) - -## Testing Strategy - -1. Unit tests for YAML parsing (valid, invalid, deeply nested) -2. Unit tests for extension detection -3. Integration test for built-in personas (now YAML) -4. Backwards compat test: verify JSON still works for external files - -## Completion Checklist - -1. [ ] `go-yaml` dependency added at v1.16.0+ -2. [ ] Extension detection uses case-insensitive comparison -3. [ ] YAML parse errors include source file name -4. [ ] JSON parsing still works for `.json` files -5. [ ] Built-in personas converted to YAML with readable multi-line strings -6. [ ] Embed directive updated to include `*.yaml` -7. [ ] Test for deeply nested YAML rejection -8. [ ] All existing tests pass - -## Open Questions - -- Should we support both `.yaml` AND `.yml`? Issue says `.yaml` only for consistency, but some users expect `.yml`. **Decision:** Support both for reading, recommend `.yaml` in docs. -- Should we add a "format" field to detect mismatched extension/content? **Decision:** No, keep it simple. Extension determines format.