Compare commits
1 Commits
9673a9d53c
..
pr-153
| Author | SHA1 | Date | |
|---|---|---|---|
 Rodin
|
eff5b83852 |
+5
-1
@@ -1,16 +1,20 @@
|
||||
# CHANGELOG
|
||||
|
||||
## v0.4.0
|
||||
## Unreleased
|
||||
|
||||
### Security
|
||||
|
||||
- **`validateDocmapPath`: add `EvalSymlinks` to close directory-symlink bypass** ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)): The previous implementation used `os.Lstat` which only avoids following the *final* path component. An intermediate directory symlink (e.g. `.review-bot/` committed as a symlink to a directory outside the repo) would pass the path-confinement check because the textual path appeared within the repo root. `filepath.EvalSymlinks` is now called first, resolving all symlink components before the `filepath.Rel` confinement check. In-repo symlinks whose resolved targets also reside within the repo root are now allowed; out-of-repo targets are rejected by the confinement check.
|
||||
|
||||
- **`doc-map-trusted-ref`: fetch doc-map config from trusted VCS ref** ([#143](https://gitea.weiker.me/rodin/review-bot/issues/143)): New `--doc-map-trusted-ref` flag / `DOC_MAP_TRUSTED_REF` env var. When set, the doc-map YAML config is fetched from the specified VCS ref (e.g. `main`) via API instead of being read from the local workspace (the PR branch checkout). This prevents a malicious PR from modifying `.review-bot/doc-map.yml` to inject arbitrary design docs into the LLM prompt. When unset, the local workspace is used with a security warning in the logs.
|
||||
|
||||
### Tests
|
||||
|
||||
- **`TestValidateDocmapPath_DirSymlinkBypass`**: verifies that a directory symlink inside the repo pointing outside cannot be used to bypass path confinement ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)).
|
||||
|
||||
- **`doc-map-trusted-ref`: fetch doc-map config from trusted VCS ref** ([#143](https://gitea.weiker.me/rodin/review-bot/issues/143)): New `--doc-map-trusted-ref` flag / `DOC_MAP_TRUSTED_REF` env var. When set, the doc-map YAML config is fetched from the specified VCS ref (e.g. `main`) via API instead of being read from the local workspace (the PR branch checkout). This prevents a malicious PR from modifying `.review-bot/doc-map.yml` to inject arbitrary design docs into the LLM prompt. When unset, the local workspace is used with a security warning in the logs.
|
||||
>>>>>>> 3222c76 (feat(#143): fetch doc-map config from trusted VCS ref)
|
||||
|
||||
### Added
|
||||
|
||||
- **`doc-map-trusted-ref` input** (`--doc-map-trusted-ref` flag / `DOC_MAP_TRUSTED_REF` env var): Git ref (branch, tag, or SHA) from which to fetch the doc-map config via VCS API. Recommended for all `doc-map` users. Example: `doc-map-trusted-ref: main`. ([#143](https://gitea.weiker.me/rodin/review-bot/issues/143))
|
||||
|
||||
@@ -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)_
|
||||
@@ -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.
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
+84
-30
@@ -1,42 +1,96 @@
|
||||
# Dev Loop Status — 2026-05-15 12:15 UTC
|
||||
# Dev Loop Status — 2026-05-15 09:37 UTC
|
||||
|
||||
**Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c
|
||||
**Status:** ✅ HEALTHY — All 4 PRs merged, all tests passing, repo clean
|
||||
## Summary
|
||||
|
||||
## Quick Status
|
||||
- **Review-bot status:** ✅ MAIN BRANCH CURRENT & HEALTHY
|
||||
- **Coverage:** 77.1% (↑ from 70.4%) — healthy trajectory
|
||||
- **Tests:** ✅ All passing
|
||||
- **Active development tracks:**
|
||||
- issue-143: fetch doc-map config from trusted VCS ref (ready for review)
|
||||
- issue-146: reuse resolved doc-map path early (ready for review)
|
||||
- issue-150: add EvalSymlinks to validateDocmapPath (ready for review)
|
||||
- issue-154: refactor subprocess test helpers (ready for review)
|
||||
|
||||
- **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
|
||||
## Current State
|
||||
|
||||
All 4 approved PRs have been merged to main:
|
||||
### Main Branch
|
||||
- **HEAD:** 1650343 (dev-loop cycle complete)
|
||||
- **Status:** Clean, all tests passing, 77.1% coverage
|
||||
- **Recent work:** Issue #130 fixes merged and verified complete
|
||||
|
||||
| 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) |
|
||||
### Active Issue Branches (Ready for Review)
|
||||
|
||||
### Notes
|
||||
| Issue | Branch | Latest Commit | Status | Recommendation |
|
||||
|-------|--------|---------------|--------|-----------------|
|
||||
| #143 | origin/issue-143 | 3222c76 | Ready | Review feature + tests, consider for merge |
|
||||
| #146 | origin/issue-146 | 9b64c60 | Ready | 2 new test cases + 1 fix, review completeness |
|
||||
| #150 | origin/issue-150 | 4dce8e4 | Ready | Symlink validation, security-sensitive |
|
||||
| #154 | origin/issue-154 | 2892dff | Ready | Refactor/cleanup, low-risk |
|
||||
|
||||
- **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.
|
||||
### Priority Assessment
|
||||
|
||||
## Dev Loop Health
|
||||
**High Priority (Security/Risk):**
|
||||
- **#150** — EvalSymlinks for dir-symlink bypass (security fix)
|
||||
- **#143** — Fetch doc-map from trusted VCS ref (trust boundary)
|
||||
|
||||
| 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 |
|
||||
**Medium Priority (Feature):**
|
||||
- **#146** — Path resolution optimization + tests
|
||||
|
||||
## Next Actions
|
||||
**Low Priority (Cleanup):**
|
||||
- **#154** — Test refactoring
|
||||
|
||||
- No open approved PRs remain
|
||||
- Dev-loop can start on new issues from the backlog
|
||||
---
|
||||
|
||||
## Coverage Trends
|
||||
|
||||
| Package | Current | Previous | Δ |
|
||||
|---------|---------|----------|---|
|
||||
| cmd/review-bot | TBD | 36.8% | ↑ |
|
||||
| budget | 91.8% | 91.8% | → |
|
||||
| review | 91.5% | 91.5% | → |
|
||||
| llm | 81.3% | 81.3% | → |
|
||||
| **Total** | **77.1%** | **70.4%** | **↑6.7%** |
|
||||
|
||||
---
|
||||
|
||||
## Recommendations for Next Cycle
|
||||
|
||||
### Immediate (This Dev-Loop)
|
||||
1. **Checkout #150** — Review symlink fix, run security tests
|
||||
2. **Checkout #143** — Review doc-map config fetching, validate error handling
|
||||
3. **Decide merge order** — #150 or #143 first (dependency check)
|
||||
4. **Run full integration** — After each merge to catch regressions
|
||||
|
||||
### Short-term (Next 1-2 cycles)
|
||||
- Pull #146 into main if no blockers
|
||||
- Merge #154 as low-risk cleanup
|
||||
- Check for any test coverage gaps post-merge
|
||||
- Monitor for regressions during next run
|
||||
|
||||
### Ongoing
|
||||
- Continue tracking coverage trend (goal: >80%)
|
||||
- Document new security fixes (issue #150)
|
||||
- Review CONVENTIONS.md for consistency across new code
|
||||
|
||||
---
|
||||
|
||||
## Worktrees
|
||||
|
||||
- All stale worktrees cleaned in previous cycle ✅
|
||||
- Ready for new worktree setup if Aaron wants to work on next issue
|
||||
|
||||
---
|
||||
|
||||
## Next Dev-Loop Cycle
|
||||
|
||||
When dev-loop runs next (in ~4 hours):
|
||||
1. ✅ Verify main still current
|
||||
2. ✅ Re-run tests & coverage
|
||||
3. ✅ Check if any PRs merged (update local branches)
|
||||
4. ⚠️ Flag for human review if coverage drops or tests fail
|
||||
|
||||
---
|
||||
|
||||
_Generated by dev-loop at 2026-05-15 09:37 UTC_
|
||||
|
||||
@@ -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
|
||||
@@ -174,12 +174,9 @@ func main() {
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// Early validation of filesystem-path flags (fail fast before network I/O).
|
||||
// Skip local-path validation when --doc-map-trusted-ref is set: the flag
|
||||
// value is used as a VCS API path, not a local filesystem path, and the
|
||||
// file may not exist in the local checkout (sparse, PR-deleted, etc.).
|
||||
// Early validation of filesystem-path flags (fail fast before network I/O)
|
||||
var resolvedDocMapFile string
|
||||
if *docMapFile != "" && *docMapTrustedRef == "" {
|
||||
if *docMapFile != "" {
|
||||
resolved, err := validateWorkspacePath(*docMapFile, "doc-map")
|
||||
if err != nil {
|
||||
slog.Error("invalid doc-map path", "error", err)
|
||||
|
||||
@@ -903,17 +903,12 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
|
||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||
args := baseSubprocessArgs()
|
||||
// Replace the canonical --repo value with an invalid one.
|
||||
found := false
|
||||
for i, a := range args {
|
||||
if a == "--repo" && i+1 < len(args) {
|
||||
args[i+1] = "invalidrepo"
|
||||
found = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Fatal("baseSubprocessArgs() does not contain --repo; test is broken")
|
||||
}
|
||||
os.Args = args
|
||||
main()
|
||||
return
|
||||
@@ -935,17 +930,12 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
|
||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||
args := baseSubprocessArgs()
|
||||
// Replace the canonical --pr value with a non-numeric string.
|
||||
found := false
|
||||
for i, a := range args {
|
||||
if a == "--pr" && i+1 < len(args) {
|
||||
args[i+1] = "notanumber"
|
||||
found = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Fatal("baseSubprocessArgs() does not contain --pr; test is broken")
|
||||
}
|
||||
os.Args = args
|
||||
main()
|
||||
return
|
||||
@@ -1531,8 +1521,6 @@ func TestMainSubprocess_InvalidDocMapPath(t *testing.T) {
|
||||
}
|
||||
|
||||
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapPath")
|
||||
// t.TempDir() is evaluated here in the outer process, producing a real directory
|
||||
// that is passed as the GITHUB_WORKSPACE env var string to the subprocess.
|
||||
cmd.Env = append(cleanEnv(),
|
||||
"TEST_SUBPROCESS_MAIN=1",
|
||||
"GITHUB_WORKSPACE="+t.TempDir(),
|
||||
@@ -1570,8 +1558,6 @@ func TestMainSubprocess_InvalidDocMapFile(t *testing.T) {
|
||||
}
|
||||
|
||||
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapFile")
|
||||
// t.TempDir() is evaluated here in the outer process, producing a real directory
|
||||
// that is passed as the GITHUB_WORKSPACE env var string to the subprocess.
|
||||
cmd.Env = append(cleanEnv(),
|
||||
"TEST_SUBPROCESS_MAIN=1",
|
||||
"GITHUB_WORKSPACE="+t.TempDir(),
|
||||
@@ -1588,47 +1574,3 @@ func TestMainSubprocess_InvalidDocMapFile(t *testing.T) {
|
||||
t.Errorf("expected error about failed resolution, got: %s", output)
|
||||
}
|
||||
}
|
||||
|
||||
// TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation confirms that
|
||||
// --doc-map-trusted-ref bypasses local filesystem validation for --doc-map.
|
||||
// When the trusted-ref flag is set, the doc-map value is used as a VCS API
|
||||
// path; a nonexistent local file must not cause an early exit before network I/O.
|
||||
func TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation(t *testing.T) {
|
||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||
os.Args = []string{"review-bot",
|
||||
"--vcs-url", "https://gitea.example.com",
|
||||
"--repo", "owner/repo",
|
||||
"--pr", "1",
|
||||
"--reviewer-token", "tok",
|
||||
"--llm-base-url", "https://api.example.com",
|
||||
"--llm-api-key", "key",
|
||||
"--llm-model", "gpt-4",
|
||||
"--doc-map", "nonexistent-local.yml",
|
||||
"--doc-map-trusted-ref", "main",
|
||||
}
|
||||
main()
|
||||
return
|
||||
}
|
||||
|
||||
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation")
|
||||
cmd.Env = append(cleanEnv(),
|
||||
"TEST_SUBPROCESS_MAIN=1",
|
||||
"GITHUB_WORKSPACE="+t.TempDir(),
|
||||
)
|
||||
out, err := cmd.CombinedOutput()
|
||||
output := string(out)
|
||||
|
||||
// The test must fail (network I/O or VCS API failure) but must NOT
|
||||
// fail with the local filesystem validation error.
|
||||
// "failed to resolve" would indicate the early validateWorkspacePath ran —
|
||||
// that would be the bug this test is catching.
|
||||
if strings.Contains(output, "failed to resolve") {
|
||||
t.Errorf("--doc-map-trusted-ref should skip local path validation, but got filesystem error: %s", output)
|
||||
}
|
||||
|
||||
// It must still exit non-zero (real VCS call to example.com will fail).
|
||||
if err == nil {
|
||||
t.Fatal("expected non-zero exit when VCS API is unreachable, got success")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -23,19 +23,18 @@ const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB
|
||||
// 1. The path resolves to a regular file within resolvedRoot (path
|
||||
// confinement): prevents a PR-controlled --docmap from reading arbitrary
|
||||
// host files via absolute paths or ".." traversal.
|
||||
// 2. The resolved path is within resolvedRoot: in-repo file-level symlinks
|
||||
// are allowed when their resolved target is still inside the root;
|
||||
// symlinks that escape the root are rejected by the confinement check.
|
||||
// 2. The path is not a symlink: prevents denial-of-service via /dev/zero or
|
||||
// information disclosure via symlinks that point outside the workspace.
|
||||
// 3. The file does not exceed maxDocmapBytes: prevents memory exhaustion
|
||||
// from an oversized but legitimately committed doc-map file.
|
||||
//
|
||||
// resolvedRoot must already be an absolute, symlink-free path (obtained from
|
||||
// filepath.Abs + filepath.EvalSymlinks).
|
||||
func validateDocmapPath(localPath, resolvedRoot string) (string, error) {
|
||||
func validateDocmapPath(localPath, resolvedRoot string) error {
|
||||
// Resolve the docmap path to an absolute path.
|
||||
absPath, err := filepath.Abs(localPath)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("cannot resolve path: %w", err)
|
||||
return fmt.Errorf("cannot resolve path: %w", err)
|
||||
}
|
||||
|
||||
// Resolve ALL symlink components, not just the final one.
|
||||
@@ -47,36 +46,34 @@ func validateDocmapPath(localPath, resolvedRoot string) (string, error) {
|
||||
// path is inside the root while the actual destination is not.
|
||||
resolvedPath, err := filepath.EvalSymlinks(absPath)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("cannot resolve path (symlink): %w", err)
|
||||
return fmt.Errorf("cannot resolve path (symlink): %w", err)
|
||||
}
|
||||
|
||||
// Lstat the resolved path for size and existence checks — EvalSymlinks
|
||||
// guarantees no symlink components remain, so ModeSymlink can never be set.
|
||||
// Lstat the resolved path — at this point resolvedPath is symlink-free, so
|
||||
// ModeSymlink will never be set. We keep the check as defense-in-depth.
|
||||
fi, err := os.Lstat(resolvedPath)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("cannot stat file: %w", err)
|
||||
return fmt.Errorf("cannot stat file: %w", err)
|
||||
}
|
||||
|
||||
// Reject anything that is not a regular file (directories, FIFOs, device
|
||||
// nodes, etc.) — ParseDocMapConfig expects a plain YAML file and would
|
||||
// produce a confusing error on non-regular entries.
|
||||
if !fi.Mode().IsRegular() {
|
||||
return "", fmt.Errorf("docmap must be a regular file")
|
||||
// Defense-in-depth: reject any remaining symlink indicator.
|
||||
if fi.Mode()&os.ModeSymlink != 0 {
|
||||
return fmt.Errorf("symlinks are not allowed")
|
||||
}
|
||||
|
||||
// Confine to resolvedRoot: use the fully-resolved path so that a directory
|
||||
// symlink inside the repo cannot carry the path outside the root.
|
||||
rel, err := filepath.Rel(resolvedRoot, resolvedPath)
|
||||
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
|
||||
return "", fmt.Errorf("path must be within --repo-root")
|
||||
return fmt.Errorf("path must be within --repo-root")
|
||||
}
|
||||
|
||||
// Enforce size cap before reading to prevent memory exhaustion.
|
||||
if fi.Size() > maxDocmapBytes {
|
||||
return "", fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes)
|
||||
return fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes)
|
||||
}
|
||||
|
||||
return resolvedPath, nil
|
||||
return nil
|
||||
}
|
||||
|
||||
// runValidateDocmap implements the `review-bot validate-docmap` subcommand.
|
||||
@@ -140,59 +137,16 @@ func runValidateDocmap(args []string) int {
|
||||
// may reference a PR-controlled file (e.g. .review-bot/doc-map.yml).
|
||||
// Validate that it:
|
||||
// 1. Resolves within resolvedRoot (prevent reading arbitrary host files).
|
||||
// 2. Resolved target stays within the root (in-repo symlinks are allowed
|
||||
// if they resolve to a path inside the root).
|
||||
// 2. Is not a symlink (prevent /dev/zero or symlink-based host probing).
|
||||
// 3. Does not exceed maxDocmapBytes (prevent memory exhaustion from an
|
||||
// oversized committed file).
|
||||
// validateDocmapPath returns the resolved path; use it directly to
|
||||
// eliminate any TOCTOU race between validation and use.
|
||||
resolvedDocmap, err := validateDocmapPath(*docmapFlag, resolvedRoot)
|
||||
if err != nil {
|
||||
if err := validateDocmapPath(*docmapFlag, resolvedRoot); err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: --docmap %q is invalid: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
|
||||
// Open and read the docmap with a LimitedReader — closes the residual TOCTOU
|
||||
// window between the Lstat size check in validateDocmapPath and the file open
|
||||
// here. The limit is maxDocmapBytes+1 so we can detect a file that grew past
|
||||
// the cap after the stat without reading unbounded bytes.
|
||||
//
|
||||
// Defense-in-depth: stat the path immediately before and after open so we can
|
||||
// detect a file swap between validateDocmapPath's validation and this open via
|
||||
// os.SameFile. An attacker with workspace write access could otherwise replace
|
||||
// the validated file with a symlink in the gap between validation and use.
|
||||
preStat, err := os.Lstat(resolvedDocmap)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to stat docmap before open %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
f, err := os.Open(resolvedDocmap)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to open docmap %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
defer func() { _ = f.Close() }()
|
||||
// Verify we opened the same file that was validated — rejects a swap between
|
||||
// the pre-open Lstat and the open call.
|
||||
postStat, err := f.Stat()
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to stat open docmap %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
if !os.SameFile(preStat, postStat) {
|
||||
fmt.Fprintf(errWriter, "Error: --docmap %q changed between validation and open\n", *docmapFlag)
|
||||
return 2
|
||||
}
|
||||
docmapData, err := io.ReadAll(io.LimitReader(f, maxDocmapBytes+1))
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to read docmap %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
if int64(len(docmapData)) > maxDocmapBytes {
|
||||
fmt.Fprintf(errWriter, "Error: --docmap %q exceeded %d-byte limit after open\n", *docmapFlag, maxDocmapBytes)
|
||||
return 2
|
||||
}
|
||||
cfg, err := review.ParseDocMapConfigContent(string(docmapData), *docmapFlag)
|
||||
// Parse docmap YAML.
|
||||
cfg, err := review.ParseDocMapConfig(*docmapFlag)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
@@ -217,9 +171,6 @@ func runValidateDocmap(args []string) int {
|
||||
// Normalize Windows-style backslashes to forward slashes so that
|
||||
// changed-file paths from git on Windows match doc-map globs.
|
||||
f = strings.ReplaceAll(f, "\\", "/")
|
||||
// Strip a leading "./" emitted by non-git tools (e.g. `find`) so that
|
||||
// paths like "./cmd/foo.go" match doc-map globs written as "cmd/**".
|
||||
f = strings.TrimPrefix(f, "./")
|
||||
if !review.FileCoveredByDocMap(cfg, f) {
|
||||
uncovered = append(uncovered, f)
|
||||
}
|
||||
@@ -238,7 +189,7 @@ func runValidateDocmap(args []string) int {
|
||||
staleDocs := checkStaleDocs(cfg, resolvedRoot)
|
||||
if len(staleDocs) > 0 {
|
||||
failed = true
|
||||
fmt.Fprintln(errWriter, "ERROR: stale docmap entries (paths do not exist):")
|
||||
fmt.Fprintln(errWriter, "ERROR: stale docmap docs: entries (paths do not exist):")
|
||||
for _, d := range staleDocs {
|
||||
fmt.Fprintf(errWriter, " %s\n", d)
|
||||
}
|
||||
|
||||
@@ -595,102 +595,7 @@ func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
|
||||
t.Fatalf("EvalSymlinks(repoDir): %v", err)
|
||||
}
|
||||
|
||||
if _, err := validateDocmapPath(attackPath, resolvedRoot); err == nil {
|
||||
if err := validateDocmapPath(attackPath, resolvedRoot); err == nil {
|
||||
t.Error("expected rejection of dir-symlink bypass, got nil error")
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_NonRegularFile verifies that --docmap pointing at a
|
||||
// non-regular file (e.g. a directory) is rejected with a clear error before
|
||||
// ParseDocMapConfig is called.
|
||||
func TestValidateDocmapPath_NonRegularFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Use the directory itself as the docmap path — directories pass Lstat but
|
||||
// are not regular files.
|
||||
reviewBotDir := filepath.Join(dir, ".review-bot")
|
||||
if err := os.MkdirAll(reviewBotDir, 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", reviewBotDir, "--repo-root", dir},
|
||||
)
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for directory docmap, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "regular file") && !strings.Contains(stderr, "invalid") {
|
||||
t.Errorf("expected regular-file rejection in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRunValidateDocmap_DotSlashPrefix verifies that paths emitted with a
|
||||
// leading "./" (e.g. from `find` or `ls`) match doc-map globs correctly.
|
||||
// Without TrimPrefix, "./cmd/foo.go" would not match the pattern "cmd/**".
|
||||
func TestRunValidateDocmap_DotSlashPrefix(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
makeDocFile(t, dir, "docs/foo.md")
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "cmd/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
// File with a leading "./" should be treated as covered.
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"./cmd/foo.go\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 0 {
|
||||
t.Errorf("expected exit 0 for './' prefixed covered file, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_InRepoSymlinkAllowed verifies that an in-repo
|
||||
// file-level symlink whose resolved target is still within the repo root is
|
||||
// accepted. This is the positive case for the issue #150 behavioral change:
|
||||
// only symlinks that escape the root are rejected; intra-repo symlinks are
|
||||
// allowed because EvalSymlinks resolves the target and the confinement check
|
||||
// is applied to the resolved path, not the symlink entry itself.
|
||||
func TestValidateDocmapPath_InRepoSymlinkAllowed(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Create the real docmap file inside the repo root.
|
||||
if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
realDocmap := filepath.Join(dir, ".review-bot", "doc-map-real.yml")
|
||||
if err := os.WriteFile(realDocmap, []byte("mappings: []\n"), 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
|
||||
// Create a symlink inside the repo root that points to the real file
|
||||
// (also inside the root).
|
||||
symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml")
|
||||
if err := os.Symlink(realDocmap, symlinkPath); err != nil {
|
||||
t.Skipf("cannot create symlink (platform may not support it): %v", err)
|
||||
}
|
||||
|
||||
// Resolve dir to a symlink-free root, as runValidateDocmap does.
|
||||
resolvedRoot, err := filepath.EvalSymlinks(dir)
|
||||
if err != nil {
|
||||
t.Fatalf("EvalSymlinks(dir): %v", err)
|
||||
}
|
||||
|
||||
// In-repo symlink whose target is within root: must be accepted.
|
||||
resolved, err := validateDocmapPath(symlinkPath, resolvedRoot)
|
||||
if err != nil {
|
||||
t.Fatalf("expected in-repo symlink to be accepted, got error: %v", err)
|
||||
}
|
||||
// The returned resolved path must be the real file (not the symlink entry).
|
||||
// validateDocmapPath calls filepath.EvalSymlinks internally, so the returned
|
||||
// path is always the fully-resolved real path — it can never equal the
|
||||
// symlink entry itself.
|
||||
if resolved == symlinkPath {
|
||||
t.Errorf("expected resolved path to differ from symlink path")
|
||||
}
|
||||
}
|
||||
|
||||
+1
-24
@@ -231,8 +231,6 @@ These are statically checked by `~/.openclaw/workspace/scripts/test/check-invari
|
||||
| S6 | Active WIP does not cause early exit (only sets ACTIVE_WIP flag) |
|
||||
| S7 | SPAWN:impl guarded by `ACTIVE_WIP == 0` check |
|
||||
| S8 | No merge calls in any worker template |
|
||||
| S9 | Zero close-PR API calls in dispatch script (`state=closed` does not appear) |
|
||||
| S10 | No close-PR API calls in any worker template; every worker template contains `NEVER close a PR` |
|
||||
|
||||
---
|
||||
|
||||
@@ -265,20 +263,9 @@ Each worker receives a precise task description with substituted values:
|
||||
|
||||
Workers **always** remove the WIP label on completion and reply `NO_REPLY`.
|
||||
|
||||
### Worker Absolute Constraints
|
||||
|
||||
Every worker template begins with an `⛔ ABSOLUTE CONSTRAINTS` section containing these rules:
|
||||
|
||||
- **NEVER close a PR.** Never call `PATCH /pulls/{id}` with `state=closed`. Closing a PR requires human action. "Duplicate", "superseded", or "already done" are never a worker's call.
|
||||
- **NEVER merge a PR.** Never call the merge API. Merging requires human approval.
|
||||
- **NEVER use the gitea-aweiker token.** All API calls use the gitea-rodin token only.
|
||||
- **NEVER act on a PR with active REQUEST_CHANGES.** Fix the findings first.
|
||||
|
||||
The first two constraints are statically enforced by `check-invariants.sh`: S1 and S9 cover the dispatch script (no merge, no close); S8 covers worker templates (no merge calls); S10 covers worker templates (no close calls, with NEVER-close text verified present in each). The remaining two constraints (token usage and REQUEST_CHANGES gate) are enforced by runtime logic.
|
||||
|
||||
---
|
||||
|
||||
## 9. Fixes for Issues #144, #145, and #157
|
||||
## 9. Fixes for Issues #144 and #145
|
||||
|
||||
**Issue #144** (autonomous merge):
|
||||
The dispatch script contains no merge API calls anywhere. The `~/.openclaw/workspace/scripts/test/check-invariants.sh`
|
||||
@@ -289,13 +276,3 @@ Rule 2 is the **first** rule evaluated per PR. It cannot be skipped, reasoned pa
|
||||
or bypassed. It is checked before CI, before self-review, before handoff. The check
|
||||
uses latest-per-reviewer state, so a reviewer who re-approved after REQUEST_CHANGES
|
||||
is correctly handled.
|
||||
|
||||
**Issue #157** (autonomous PR close):
|
||||
Worker templates were missing an explicit constraint against closing PRs. The dispatch
|
||||
script never had a close call, but workers could reason their way into calling
|
||||
`PATCH /pulls/{id}` with `state=closed`. All worker templates now include
|
||||
`NEVER close a PR` in their ABSOLUTE CONSTRAINTS section. Invariant S9 verifies
|
||||
the dispatch script contains no close calls. Invariant S10 verifies
|
||||
worker templates contain no close calls and each contains the NEVER-close text.
|
||||
|
||||
Regression tests in `dispatch.bats` statically verify all of these constraints.
|
||||
|
||||
+1
-1
@@ -57,7 +57,7 @@ func ParseDocMapConfig(localPath string) (*DocMapConfig, error) {
|
||||
|
||||
// ParseDocMapConfigContent parses a doc-map YAML config from an in-memory
|
||||
// string. The source parameter is used only for error messages and log entries
|
||||
// (e.g. "owner/repo@main:.review-bot/doc-map.yml").
|
||||
// (e.g. "main:main@<ref>").
|
||||
//
|
||||
// Use this when the config content has been fetched from a trusted VCS ref
|
||||
// rather than read from the local workspace.
|
||||
|
||||
Reference in New Issue
Block a user