Compare commits
4 Commits
issue-148
...
3222c765c9
| Author | SHA1 | Date | |
|---|---|---|---|
| 3222c765c9 | |||
| 9b64c605f8 | |||
| 40a16b75e0 | |||
| 30fe48d265 |
@@ -141,6 +141,16 @@ inputs:
|
|||||||
description: 'Maximum bytes of injected doc content from doc-map (default 102400 = 100KB)'
|
description: 'Maximum bytes of injected doc content from doc-map (default 102400 = 100KB)'
|
||||||
required: false
|
required: false
|
||||||
default: '102400'
|
default: '102400'
|
||||||
|
doc-map-trusted-ref:
|
||||||
|
description: >-
|
||||||
|
Git ref (branch, tag, or SHA) from which to fetch the doc-map config file
|
||||||
|
via VCS API instead of reading it from the local workspace. Recommended
|
||||||
|
when using doc-map: set this to the default branch (e.g. 'main') so a
|
||||||
|
malicious PR cannot modify the doc-map config to inject arbitrary design
|
||||||
|
docs into the LLM prompt. When unset, the config is read from the local
|
||||||
|
workspace (the PR branch) with a security warning in the logs.
|
||||||
|
required: false
|
||||||
|
default: ''
|
||||||
|
|
||||||
runs:
|
runs:
|
||||||
using: 'composite'
|
using: 'composite'
|
||||||
@@ -506,6 +516,7 @@ runs:
|
|||||||
PERSONA_FILE: ${{ inputs.persona-file }}
|
PERSONA_FILE: ${{ inputs.persona-file }}
|
||||||
DOC_MAP_FILE: ${{ inputs.doc-map }}
|
DOC_MAP_FILE: ${{ inputs.doc-map }}
|
||||||
DOC_MAP_MAX_BYTES: ${{ inputs.doc-map-max-bytes }}
|
DOC_MAP_MAX_BYTES: ${{ inputs.doc-map-max-bytes }}
|
||||||
|
DOC_MAP_TRUSTED_REF: ${{ inputs.doc-map-trusted-ref }}
|
||||||
AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
|
AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
|
||||||
AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
|
AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
|
||||||
AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
|
AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
|
||||||
|
|||||||
@@ -2,8 +2,14 @@
|
|||||||
|
|
||||||
## Unreleased
|
## Unreleased
|
||||||
|
|
||||||
|
### Security
|
||||||
|
|
||||||
|
- **`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.
|
||||||
|
|
||||||
### Added
|
### 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))
|
||||||
|
|
||||||
- **`doc-map` input** (`--doc-map` flag / `DOC_MAP_FILE` env var): Path to a YAML file mapping source path globs to governing design docs. review-bot intersects the map with changed PR paths and injects matching docs into the system prompt under a `## Design Documents` heading. ([#137](https://gitea.weiker.me/rodin/review-bot/issues/137))
|
- **`doc-map` input** (`--doc-map` flag / `DOC_MAP_FILE` env var): Path to a YAML file mapping source path globs to governing design docs. review-bot intersects the map with changed PR paths and injects matching docs into the system prompt under a `## Design Documents` heading. ([#137](https://gitea.weiker.me/rodin/review-bot/issues/137))
|
||||||
- **`doc-map-max-bytes` input** (`--doc-map-max-bytes` flag / `DOC_MAP_MAX_BYTES` env var): Cap on total injected design doc content in bytes. Default: 102400 (100 KB). Prevents accidental context overflow when a PR touches many modules.
|
- **`doc-map-max-bytes` input** (`--doc-map-max-bytes` flag / `DOC_MAP_MAX_BYTES` env var): Cap on total injected design doc content in bytes. Default: 102400 (100 KB). Prevents accidental context overflow when a PR touches many modules.
|
||||||
- **`DesignDocs` budget section**: Design docs are included in the context budget and trimmed after conventions, before file context, if the total exceeds the model's context limit.
|
- **`DesignDocs` budget section**: Design docs are included in the context budget and trimmed after conventions, before file context, if the total exceeds the model's context limit.
|
||||||
|
|||||||
@@ -210,6 +210,7 @@ AI Core handles OAuth token management and deployment discovery automatically. M
|
|||||||
| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
|
| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
|
||||||
| `doc-map` | No | `""` | Path to a YAML file mapping source path globs to governing design docs |
|
| `doc-map` | No | `""` | Path to a YAML file mapping source path globs to governing design docs |
|
||||||
| `doc-map-max-bytes` | No | `102400` | Maximum bytes of injected doc content from doc-map (default 100KB) |
|
| `doc-map-max-bytes` | No | `102400` | Maximum bytes of injected doc content from doc-map (default 100KB) |
|
||||||
|
| `doc-map-trusted-ref` | No | `""` | Git ref (e.g. `main`) to fetch the doc-map config from via VCS API instead of local workspace. **Recommended for security** — prevents a PR from modifying the doc-map config to inject arbitrary docs. |
|
||||||
| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
|
| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
|
||||||
| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
|
| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
|
||||||
| `temperature` | No | `0` | LLM temperature (0 = server default) |
|
| `temperature` | No | `0` | LLM temperature (0 = server default) |
|
||||||
|
|||||||
@@ -0,0 +1,129 @@
|
|||||||
|
# Dev-Loop Skill: review-bot
|
||||||
|
|
||||||
|
This file documents the dev-loop architecture for the `review-bot` project.
|
||||||
|
It lives in the repo so changes are version-controlled alongside the code.
|
||||||
|
|
||||||
|
## Architecture
|
||||||
|
|
||||||
|
Dispatch is a **pure shell script** — no model reasoning.
|
||||||
|
|
||||||
|
```
|
||||||
|
Cron (agentTurn, toolsAllow: [exec, sessions_spawn, read])
|
||||||
|
→ runs dispatch script
|
||||||
|
→ reads output for SPAWN or HANDOFF lines
|
||||||
|
→ spawns worker if instructed
|
||||||
|
|
||||||
|
Dispatch script (~/.openclaw/workspace/scripts/dev-loop-dispatch.sh)
|
||||||
|
→ pure bash, all decisions are curl API calls + branches
|
||||||
|
→ exits after emitting one SPAWN line (at most one worker per run)
|
||||||
|
→ emits HANDOFF for each qualifying PR (does not exit after HANDOFF)
|
||||||
|
|
||||||
|
Workers (Opus, spawned by cron model)
|
||||||
|
→ receive precise task description
|
||||||
|
→ do one job: self-review, fix CI, address feedback, or implement
|
||||||
|
→ remove wip label when done, reply NO_REPLY
|
||||||
|
```
|
||||||
|
|
||||||
|
The cron model's **only** job: run script, read output, spawn worker if told to.
|
||||||
|
The model **never** assesses project state or makes dispatch decisions.
|
||||||
|
|
||||||
|
## Safety Invariants
|
||||||
|
|
||||||
|
1. **NEVER MERGE** — no merge API call exists anywhere in the script or worker templates
|
||||||
|
2. **REQUEST_CHANGES always blocks** — checked first, before CI, before self-review, before handoff
|
||||||
|
3. **WIP mutex** — one active worker per repo; WIP label gates new issue pickup
|
||||||
|
4. **One SPAWN per run** — script emits at most one SPAWN line per execution
|
||||||
|
5. **set -euo pipefail** — any curl failure aborts immediately, no partial actions
|
||||||
|
6. **Workers reply NO_REPLY** — no dispatch-level side effects (workers may push changes and manage labels as part of their task)
|
||||||
|
|
||||||
|
## Dispatch Rules (in order)
|
||||||
|
|
||||||
|
| Rule | Condition | Action |
|
||||||
|
|------|-----------|--------|
|
||||||
|
| 0 | WIP label > 1hr old | Remove stale WIP, continue |
|
||||||
|
| 0b | WIP label ≤ 1hr old | Mark ACTIVE_WIP=1, continue (only gates Rule 10) |
|
||||||
|
| _(1)_ | _(reserved — intentionally unused)_ | — |
|
||||||
|
| 2 | Any reviewer has REQUEST_CHANGES | SPAWN:findings |
|
||||||
|
| 3 | PR not mergeable | SPAWN:rebase |
|
||||||
|
| 4 | CI failure, no fix plan | SPAWN:ci-fix |
|
||||||
|
| 4b | CI failure, fix plan exists | Skip (worker in progress) |
|
||||||
|
| 5 | Bot review missing | Wait |
|
||||||
|
| 6 | CI pending/unknown | Wait |
|
||||||
|
| 7 | No clean self-review, no fix plan | SPAWN:self-review |
|
||||||
|
| 7b | Self-review needs attention, no fix plan | SPAWN:sr-fix |
|
||||||
|
| 8 | Unacknowledged bot review findings | SPAWN:address-feedback |
|
||||||
|
| 9 | Unresolved inline diff comments | SPAWN:address-feedback |
|
||||||
|
| 10 | All checks pass | HANDOFF |
|
||||||
|
| 11 | No open PRs + no ACTIVE_WIP | SPAWN:impl (next issue) |
|
||||||
|
|
||||||
|
## Files
|
||||||
|
|
||||||
|
| File | Description |
|
||||||
|
|------|-------------|
|
||||||
|
| `~/.openclaw/workspace/scripts/dev-loop-dispatch.sh` | Dispatch script — pure bash |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/self-review.md` | Self-review worker template |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/sr-fix.md` | Fix findings from self-review |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/ci-fix.md` | CI fix worker template |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/address-feedback.md` | Address feedback worker template |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/findings.md` | Address REQUEST_CHANGES findings |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/rebase.md` | Rebase worker template |
|
||||||
|
| `~/.openclaw/workspace/scripts/worker-tasks/impl.md` | Issue implementation worker template |
|
||||||
|
| `~/.openclaw/workspace/scripts/test/dispatch.bats` | Unit tests (bats) |
|
||||||
|
| `~/.openclaw/workspace/scripts/test/check-invariants.sh` | Static invariant checks |
|
||||||
|
| `~/.openclaw/workspace/memory/projects/review-bot.yaml` | Project config |
|
||||||
|
|
||||||
|
## Project Config
|
||||||
|
|
||||||
|
Config is at `~/.openclaw/workspace/memory/projects/review-bot.yaml`.
|
||||||
|
|
||||||
|
Key fields:
|
||||||
|
- `repo`: `rodin/review-bot`
|
||||||
|
- `api_base`: `https://gitea.weiker.me/api/v1`
|
||||||
|
- `user`: `rodin` (bot Gitea username)
|
||||||
|
- `labels.wip`: WIP label ID
|
||||||
|
- `labels.ready`: ready label ID
|
||||||
|
- `review_bots`: list of bot sentinel names
|
||||||
|
|
||||||
|
## Cron Config
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
- label: review-bot-dev-loop
|
||||||
|
schedule: "*/15 * * * *"
|
||||||
|
prompt: |
|
||||||
|
Run: bash ~/.openclaw/workspace/scripts/dev-loop-dispatch.sh review-bot
|
||||||
|
|
||||||
|
Read the output. If it contains a SPAWN line, load the matching template from
|
||||||
|
~/.openclaw/workspace/scripts/worker-tasks/<type>.md, substitute {{PROJECT}},
|
||||||
|
{{PR_NUM}}, and {{HEAD_SHA}}, then spawn with sessions_spawn(mode: "run",
|
||||||
|
model: "hai-anthropic/anthropic--claude-4.6-opus", thinking: "high").
|
||||||
|
|
||||||
|
If no SPAWN line in output, reply NO_REPLY.
|
||||||
|
|
||||||
|
See ~/.openclaw/workspace/skills/dev-loop/SKILL.md for full instructions.
|
||||||
|
(This repo's SKILL.md is deployed to that workspace path.)
|
||||||
|
model: hai-anthropic/anthropic--claude-4.5-haiku
|
||||||
|
toolsAllow: [exec, sessions_spawn, read]
|
||||||
|
```
|
||||||
|
|
||||||
|
## Tests
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# Unit tests (no real API calls):
|
||||||
|
bats ~/.openclaw/workspace/scripts/test/dispatch.bats
|
||||||
|
|
||||||
|
# Invariant checks (static analysis):
|
||||||
|
bash ~/.openclaw/workspace/scripts/test/check-invariants.sh
|
||||||
|
|
||||||
|
# Dry-run against real API:
|
||||||
|
DRY_RUN=1 bash ~/.openclaw/workspace/scripts/dev-loop-dispatch.sh review-bot
|
||||||
|
```
|
||||||
|
|
||||||
|
## Related Issues
|
||||||
|
|
||||||
|
- **#144** — autonomous merge: eliminated by removing all merge API calls from dispatch
|
||||||
|
- **#145** — merged despite REQUEST_CHANGES: eliminated by checking REQUEST_CHANGES first, unconditionally
|
||||||
|
- **#148** — this redesign
|
||||||
|
|
||||||
|
## Spec
|
||||||
|
|
||||||
|
Full design spec: `docs/dev-loop-spec.md`
|
||||||
+52
-9
@@ -101,6 +101,7 @@ func main() {
|
|||||||
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
|
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
|
||||||
docMapFile := flag.String("doc-map", envOrDefault("DOC_MAP_FILE", ""), "Path to YAML file mapping source path globs to governing design docs")
|
docMapFile := flag.String("doc-map", envOrDefault("DOC_MAP_FILE", ""), "Path to YAML file mapping source path globs to governing design docs")
|
||||||
docMapMaxBytes := flag.Int("doc-map-max-bytes", envOrDefaultInt("DOC_MAP_MAX_BYTES", review.DefaultDocMapMaxBytes), "Maximum bytes of injected doc content (default 102400)")
|
docMapMaxBytes := flag.Int("doc-map-max-bytes", envOrDefaultInt("DOC_MAP_MAX_BYTES", review.DefaultDocMapMaxBytes), "Maximum bytes of injected doc content (default 102400)")
|
||||||
|
docMapTrustedRef := flag.String("doc-map-trusted-ref", envOrDefault("DOC_MAP_TRUSTED_REF", ""), "Git ref (e.g. main) to fetch the doc-map config from via VCS API instead of local workspace. Recommended to prevent PR branch from controlling which docs are injected.")
|
||||||
|
|
||||||
flag.Parse()
|
flag.Parse()
|
||||||
|
|
||||||
@@ -173,6 +174,17 @@ func main() {
|
|||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Early validation of filesystem-path flags (fail fast before network I/O)
|
||||||
|
var resolvedDocMapFile string
|
||||||
|
if *docMapFile != "" {
|
||||||
|
resolved, err := validateWorkspacePath(*docMapFile, "doc-map")
|
||||||
|
if err != nil {
|
||||||
|
slog.Error("invalid doc-map path", "error", err)
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
|
resolvedDocMapFile = resolved
|
||||||
|
}
|
||||||
|
|
||||||
// Initialize clients
|
// Initialize clients
|
||||||
// Detect VCS type: explicit flag > env var > URL heuristic (default: gitea).
|
// Detect VCS type: explicit flag > env var > URL heuristic (default: gitea).
|
||||||
vcsType := envOrDefault("VCS_TYPE", "")
|
vcsType := envOrDefault("VCS_TYPE", "")
|
||||||
@@ -357,16 +369,46 @@ func main() {
|
|||||||
// Step 6c: Load path-scoped design docs if doc-map specified
|
// Step 6c: Load path-scoped design docs if doc-map specified
|
||||||
designDocs := ""
|
designDocs := ""
|
||||||
if *docMapFile != "" {
|
if *docMapFile != "" {
|
||||||
resolvedDocMap, err := validateWorkspacePath(*docMapFile, "doc-map")
|
var docMapCfg *review.DocMapConfig
|
||||||
if err != nil {
|
|
||||||
slog.Error("invalid doc-map path", "error", err)
|
if *docMapTrustedRef != "" {
|
||||||
|
// Fetch doc-map config from a trusted VCS ref (e.g. the default branch).
|
||||||
|
// This prevents a malicious PR from modifying the doc-map config to
|
||||||
|
// inject arbitrary docs into the LLM prompt.
|
||||||
|
slog.Info("doc-map: fetching config from trusted ref",
|
||||||
|
"path", *docMapFile,
|
||||||
|
"ref", *docMapTrustedRef)
|
||||||
|
content, fetchErr := vcs.GetFileContentRef(ctx, owner, repoName, *docMapFile, *docMapTrustedRef)
|
||||||
|
if fetchErr != nil {
|
||||||
|
slog.Error("doc-map: failed to fetch config from trusted ref",
|
||||||
|
"path", *docMapFile,
|
||||||
|
"ref", *docMapTrustedRef,
|
||||||
|
"error", fetchErr)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
docMapCfg, err := review.ParseDocMapConfig(resolvedDocMap)
|
source := fmt.Sprintf("%s/%s@%s:%s", owner, repoName, *docMapTrustedRef, *docMapFile)
|
||||||
if err != nil {
|
var parseErr error
|
||||||
slog.Error("failed to parse doc-map file", "file", *docMapFile, "error", err)
|
docMapCfg, parseErr = review.ParseDocMapConfigContent(content, source)
|
||||||
|
if parseErr != nil {
|
||||||
|
slog.Error("doc-map: failed to parse fetched config",
|
||||||
|
"source", source,
|
||||||
|
"error", parseErr)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
} else {
|
||||||
|
// Local workspace fallback — the doc-map is read from the PR branch checkout.
|
||||||
|
// SECURITY WARNING: a malicious PR can modify this file to inject arbitrary
|
||||||
|
// docs. Set --doc-map-trusted-ref (or DOC_MAP_TRUSTED_REF) to a trusted ref
|
||||||
|
// (e.g. "main") to fetch the config from the default branch instead.
|
||||||
|
slog.Warn("doc-map: loading config from local workspace (PR branch) — " +
|
||||||
|
"set --doc-map-trusted-ref to fetch from a trusted ref for security")
|
||||||
|
var parseErr error
|
||||||
|
docMapCfg, parseErr = review.ParseDocMapConfig(resolvedDocMapFile)
|
||||||
|
if parseErr != nil {
|
||||||
|
slog.Error("failed to parse doc-map file", "file", *docMapFile, "error", parseErr)
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Collect changed file paths from the PR for intersection.
|
// Collect changed file paths from the PR for intersection.
|
||||||
var changedPaths []string
|
var changedPaths []string
|
||||||
@@ -379,10 +421,11 @@ func main() {
|
|||||||
|
|
||||||
if len(matchedDocs) > 0 {
|
if len(matchedDocs) > 0 {
|
||||||
docMapOpts := review.DocMapOptions{MaxBytes: *docMapMaxBytes}
|
docMapOpts := review.DocMapOptions{MaxBytes: *docMapMaxBytes}
|
||||||
designDocs, err = review.LoadMatchingDocs(ctx, vcs, owner, repoName, matchedDocs, docMapOpts)
|
var loadErr error
|
||||||
if err != nil {
|
designDocs, loadErr = review.LoadMatchingDocs(ctx, vcs, owner, repoName, matchedDocs, docMapOpts)
|
||||||
|
if loadErr != nil {
|
||||||
// Non-fatal: individual missing files are already warned; log and continue.
|
// Non-fatal: individual missing files are already warned; log and continue.
|
||||||
slog.Warn("doc-map: partial failure loading docs", "error", err)
|
slog.Warn("doc-map: partial failure loading docs", "error", loadErr)
|
||||||
}
|
}
|
||||||
if designDocs != "" {
|
if designDocs != "" {
|
||||||
slog.Info("doc-map: injected design docs", "matched", len(matchedDocs), "bytes", len(designDocs))
|
slog.Info("doc-map: injected design docs", "matched", len(matchedDocs), "bytes", len(designDocs))
|
||||||
|
|||||||
@@ -1506,3 +1506,77 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
|
|||||||
t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
|
t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestMainSubprocess_InvalidDocMapPath confirms that --doc-map with a path traversal
|
||||||
|
// attempt is rejected before any network I/O.
|
||||||
|
func TestMainSubprocess_InvalidDocMapPath(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", "../../../etc/passwd",
|
||||||
|
}
|
||||||
|
main()
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapPath")
|
||||||
|
cmd.Env = append(cleanEnv(),
|
||||||
|
"TEST_SUBPROCESS_MAIN=1",
|
||||||
|
"GITHUB_WORKSPACE="+t.TempDir(),
|
||||||
|
)
|
||||||
|
out, err := cmd.CombinedOutput()
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected non-zero exit with path traversal doc-map, got success")
|
||||||
|
}
|
||||||
|
output := string(out)
|
||||||
|
if !strings.Contains(output, "doc-map") {
|
||||||
|
t.Errorf("expected error mentioning doc-map, got: %s", output)
|
||||||
|
}
|
||||||
|
if !strings.Contains(output, "resolves outside workspace") {
|
||||||
|
t.Errorf("expected error about path traversal, got: %s", output)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestMainSubprocess_InvalidDocMapFile confirms that --doc-map with a nonexistent file
|
||||||
|
// is rejected before any network I/O.
|
||||||
|
func TestMainSubprocess_InvalidDocMapFile(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.yml",
|
||||||
|
}
|
||||||
|
main()
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapFile")
|
||||||
|
cmd.Env = append(cleanEnv(),
|
||||||
|
"TEST_SUBPROCESS_MAIN=1",
|
||||||
|
"GITHUB_WORKSPACE="+t.TempDir(),
|
||||||
|
)
|
||||||
|
out, err := cmd.CombinedOutput()
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected non-zero exit with nonexistent doc-map file, got success")
|
||||||
|
}
|
||||||
|
output := string(out)
|
||||||
|
if !strings.Contains(output, "doc-map") {
|
||||||
|
t.Errorf("expected error mentioning doc-map, got: %s", output)
|
||||||
|
}
|
||||||
|
if !strings.Contains(output, "failed to resolve") {
|
||||||
|
t.Errorf("expected error about failed resolution, got: %s", output)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -0,0 +1,278 @@
|
|||||||
|
# Dev-Loop Dispatch Spec
|
||||||
|
|
||||||
|
**Version:** 1.0
|
||||||
|
**Status:** Implemented
|
||||||
|
**Implements:** Issue #148
|
||||||
|
|
||||||
|
This document is the authoritative spec for the review-bot dev-loop dispatch architecture.
|
||||||
|
The dispatch script (`~/.openclaw/workspace/scripts/dev-loop-dispatch.sh`) and its tests
|
||||||
|
are validated against the rules and invariants in this document.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Overview
|
||||||
|
|
||||||
|
The dev-loop is a 15-minute cron that advances the state of open pull requests and picks up
|
||||||
|
new issues when there is nothing in review. It is designed for **zero human intervention**
|
||||||
|
in the normal flow and **hard stops at key safety boundaries**.
|
||||||
|
|
||||||
|
### Architecture
|
||||||
|
|
||||||
|
```
|
||||||
|
Cron (15-min cadence)
|
||||||
|
→ exec: bash dev-loop-dispatch.sh <project>
|
||||||
|
→ read stdout for SPAWN/HANDOFF lines
|
||||||
|
→ if SPAWN: load worker template, spawn subagent
|
||||||
|
→ if HANDOFF: log, do nothing else
|
||||||
|
→ if neither: NO_REPLY
|
||||||
|
```
|
||||||
|
|
||||||
|
The cron model has **no ambient knowledge** of the project state. All state is derived
|
||||||
|
from the dispatch script's output, which in turn comes from live API calls.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Inputs
|
||||||
|
|
||||||
|
### Project Config
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
# memory/projects/<project>.yaml
|
||||||
|
repo: rodin/review-bot # <owner>/<repo>
|
||||||
|
api_base: https://gitea.../v1 # API base URL
|
||||||
|
token_path: ~/.openclaw/... # path to bearer token
|
||||||
|
user: rodin # bot Gitea username
|
||||||
|
labels:
|
||||||
|
wip: <id>
|
||||||
|
ready: <id>
|
||||||
|
review_bots: # sentinel names in review bodies
|
||||||
|
- sonnet
|
||||||
|
- gpt
|
||||||
|
- security
|
||||||
|
```
|
||||||
|
|
||||||
|
### Script Arguments
|
||||||
|
|
||||||
|
```bash
|
||||||
|
bash dev-loop-dispatch.sh <project> # normal run
|
||||||
|
DRY_RUN=1 bash dev-loop-dispatch.sh <project> # dry-run (no mutations)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. State
|
||||||
|
|
||||||
|
The dispatch script is **stateless per run**. All state lives in the Gitea API:
|
||||||
|
|
||||||
|
| State | API location |
|
||||||
|
|-------|-------------|
|
||||||
|
| Open PRs | `GET /repos/:repo/pulls?state=open` |
|
||||||
|
| PR labels | `GET /repos/:repo/issues/:n/labels` |
|
||||||
|
| PR reviews | `GET /repos/:repo/pulls/:n/reviews` |
|
||||||
|
| CI status | `GET /repos/:repo/commits/:sha/status` |
|
||||||
|
| Issue comments | `GET /repos/:repo/issues/:n/comments` |
|
||||||
|
| Inline diff comments | `GET /repos/:repo/pulls/:n/comments` |
|
||||||
|
| Issue timeline | `GET /repos/:repo/issues/:n/timeline` |
|
||||||
|
|
||||||
|
No file-based state. No cron-to-cron carry-over.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Output Protocol
|
||||||
|
|
||||||
|
The script emits structured lines to stdout. Stderr is diagnostic logging.
|
||||||
|
|
||||||
|
### `SPAWN:<type>:<number>:<sha>`
|
||||||
|
|
||||||
|
A worker is needed. The cron model reads this and spawns a subagent using the
|
||||||
|
template at `worker-tasks/<type>.md`.
|
||||||
|
|
||||||
|
| Field | Description |
|
||||||
|
|-------|-------------|
|
||||||
|
| `type` | Worker type: `self-review`, `ci-fix`, `address-feedback`, `findings`, `rebase`, `impl` |
|
||||||
|
| `number` | PR number (or issue number for `impl`) |
|
||||||
|
| `sha` | HEAD SHA of the PR (empty for `impl`) |
|
||||||
|
|
||||||
|
At most **one SPAWN** is emitted per script run.
|
||||||
|
|
||||||
|
### `HANDOFF:<pr_num>`
|
||||||
|
|
||||||
|
All checks passed for `pr_num`. The script applied the `ready` label and assigned
|
||||||
|
to the human reviewer. The cron model logs this and takes no further action.
|
||||||
|
|
||||||
|
Multiple HANDOFFs may be emitted in one run (one per qualifying PR).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Dispatch Rules
|
||||||
|
|
||||||
|
Rules are evaluated **in order** for each open PR. The first matching condition wins.
|
||||||
|
Only one SPAWN is emitted per full pass.
|
||||||
|
|
||||||
|
### Rule 0: WIP Cleanup
|
||||||
|
|
||||||
|
For each open PR with a `wip` label:
|
||||||
|
|
||||||
|
1. Find the timestamp when the label was most recently applied (via timeline events)
|
||||||
|
2. If age > 1hr: **remove the label** (stale lock — worker likely crashed)
|
||||||
|
3. If age ≤ 1hr: **set ACTIVE_WIP=1** (do not exit, only gates Rule 10)
|
||||||
|
|
||||||
|
### Rule 2: REQUEST_CHANGES Blocks
|
||||||
|
|
||||||
|
**ALWAYS evaluated before any other per-PR rule.**
|
||||||
|
|
||||||
|
For each reviewer, take their **latest** review state. If any reviewer's latest
|
||||||
|
state is `REQUEST_CHANGES`:
|
||||||
|
|
||||||
|
→ Acquire WIP label on this PR
|
||||||
|
→ Emit `SPAWN:findings:<pr_num>:<head_sha>`
|
||||||
|
→ Continue to next PR (but only one SPAWN total)
|
||||||
|
|
||||||
|
This rule cannot be bypassed by any condition. There is no waiver mechanism.
|
||||||
|
|
||||||
|
### Rule 3: Merge Conflicts
|
||||||
|
|
||||||
|
If `mergeable == false`:
|
||||||
|
|
||||||
|
→ Acquire WIP
|
||||||
|
→ Emit `SPAWN:rebase:<pr_num>:<head_sha>`
|
||||||
|
|
||||||
|
### Rule 4: CI Failure
|
||||||
|
|
||||||
|
If CI state is `failure` or `error`:
|
||||||
|
|
||||||
|
- If a fix plan comment exists for this HEAD SHA: **skip** (worker in progress)
|
||||||
|
- Otherwise:
|
||||||
|
|
||||||
|
→ Acquire WIP
|
||||||
|
→ Emit `SPAWN:ci-fix:<pr_num>:<head_sha>`
|
||||||
|
|
||||||
|
### Rule 5: Bot Reviews Missing
|
||||||
|
|
||||||
|
For each configured `review_bot`, check whether a review body contains the
|
||||||
|
sentinel `<!-- review-bot:<name> -->`.
|
||||||
|
|
||||||
|
If any sentinel is missing: **wait** (continue to next PR, no SPAWN).
|
||||||
|
|
||||||
|
### Rule 6: CI Pending/Unknown
|
||||||
|
|
||||||
|
If CI state is `pending` or `unknown`: **wait**.
|
||||||
|
|
||||||
|
### Rule 7: Self-Review
|
||||||
|
|
||||||
|
Check for a self-review comment from the bot user against the current HEAD SHA:
|
||||||
|
- Comment contains `Self-review against <head_sha>`
|
||||||
|
|
||||||
|
Sub-cases:
|
||||||
|
- **Missing**: No self-review comment →
|
||||||
|
→ Acquire WIP, emit `SPAWN:self-review:<pr_num>:<head_sha>`
|
||||||
|
- **Needs attention** (`Assessment: ⚠️`): Found, but has findings:
|
||||||
|
- Fix plan exists for HEAD SHA: skip
|
||||||
|
- No fix plan: → Acquire WIP, emit `SPAWN:sr-fix:<pr_num>:<head_sha>`
|
||||||
|
- **Clean** (`Assessment: ✅ Clean`): Continue to Rule 8
|
||||||
|
|
||||||
|
### Rule 8: Unacknowledged Bot Review Findings
|
||||||
|
|
||||||
|
For each **current** (contains `Evaluated against <head_short>`) APPROVED bot review
|
||||||
|
that has a findings table:
|
||||||
|
|
||||||
|
A finding is **unacknowledged** if it does not appear as `Finding #N` in a fix plan
|
||||||
|
comment from the bot user for this HEAD SHA.
|
||||||
|
|
||||||
|
If any unacknowledged findings exist:
|
||||||
|
- Fix plan exists: skip
|
||||||
|
- No fix plan: → Acquire WIP, emit `SPAWN:address-feedback:<pr_num>:<head_sha>`
|
||||||
|
|
||||||
|
### Rule 9: Unresolved Inline Diff Comments
|
||||||
|
|
||||||
|
An inline diff comment is **unresolved** if:
|
||||||
|
1. `in_reply_to_id` is null (top-level comment)
|
||||||
|
2. `resolver` is null (not formally resolved)
|
||||||
|
3. No other comment has `in_reply_to_id` pointing to this comment (no reply)
|
||||||
|
|
||||||
|
If unresolved comments exist:
|
||||||
|
- Fix plan exists: skip
|
||||||
|
- No fix plan: → Acquire WIP, emit `SPAWN:address-feedback:<pr_num>:<head_sha>`
|
||||||
|
|
||||||
|
### Rule 10: Handoff
|
||||||
|
|
||||||
|
All rules above passed. Verify all bot reviews are current (contain `Evaluated against <head_short>`).
|
||||||
|
|
||||||
|
If all current:
|
||||||
|
- Apply `ready` label
|
||||||
|
- Assign to `aweiker`
|
||||||
|
- Emit `HANDOFF:<pr_num>`
|
||||||
|
- Continue evaluating remaining PRs (do NOT exit)
|
||||||
|
|
||||||
|
If already assigned to `aweiker`: skip (assume handoff was already performed; continue to next PR without emitting another HANDOFF).
|
||||||
|
|
||||||
|
### Rule 11: New Issue Pickup
|
||||||
|
|
||||||
|
Only runs if: no open PRs exist AND `ACTIVE_WIP == 0`.
|
||||||
|
|
||||||
|
Fetch open, unassigned issues. Priority: bugs first, then by number ascending.
|
||||||
|
|
||||||
|
Claim the issue (assign to bot user to prevent double-pick), then:
|
||||||
|
→ Emit `SPAWN:impl:<issue_num>:`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 6. Safety Invariants
|
||||||
|
|
||||||
|
These are statically checked by `~/.openclaw/workspace/scripts/test/check-invariants.sh` and enforced in all changes:
|
||||||
|
|
||||||
|
| ID | Invariant |
|
||||||
|
|----|-----------|
|
||||||
|
| S1 | Zero merge API calls in dispatch script (`/merge` does not appear) |
|
||||||
|
| S2 | REQUEST_CHANGES check (Rule 2) appears before CI check (Rule 4) |
|
||||||
|
| S3 | REQUEST_CHANGES check (Rule 2) appears before ready label application (Rule 10) |
|
||||||
|
| S4 | No model/AI API references in dispatch script |
|
||||||
|
| S5 | `set -euo pipefail` present |
|
||||||
|
| 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 |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 7. Error Handling
|
||||||
|
|
||||||
|
| Error | Behavior |
|
||||||
|
|-------|----------|
|
||||||
|
| `curl` returns error | `set -euo pipefail` aborts script — no partial actions |
|
||||||
|
| `jq` parse error | Script aborts |
|
||||||
|
| Worker crashes | WIP label left on PR; stale WIP cleanup (Rule 0) removes it after 1hr |
|
||||||
|
| Race: two crons fire | WIP mutex prevents double-dispatch for same PR |
|
||||||
|
| `sessions_spawn` fails | Worker not spawned; WIP label orphaned → cleaned in 1hr |
|
||||||
|
| Config file missing | Exit code 2 with error message |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 8. Worker Templates
|
||||||
|
|
||||||
|
Each worker receives a precise task description with substituted values:
|
||||||
|
|
||||||
|
| Template | Trigger | Key job |
|
||||||
|
|----------|---------|---------|
|
||||||
|
| `self-review.md` | No clean self-review | Post self-review comment, remove WIP |
|
||||||
|
| `sr-fix.md` | Self-review needs attention | Address self-review findings, push, remove WIP |
|
||||||
|
| `ci-fix.md` | CI failing | Diagnose, fix, push, remove WIP |
|
||||||
|
| `address-feedback.md` | Unacknowledged findings or inline comments | Address feedback, push, remove WIP |
|
||||||
|
| `findings.md` | REQUEST_CHANGES present | Address REQUEST_CHANGES, push, remove WIP |
|
||||||
|
| `rebase.md` | Merge conflicts | Rebase on main, push, remove WIP |
|
||||||
|
| `impl.md` | New issue | Implement feature/fix, open PR |
|
||||||
|
|
||||||
|
Workers **always** remove the WIP label on completion and reply `NO_REPLY`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 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`
|
||||||
|
invariant `S1` verifies this. Workers do not receive merge instructions.
|
||||||
|
|
||||||
|
**Issue #145** (merged despite REQUEST_CHANGES):
|
||||||
|
Rule 2 is the **first** rule evaluated per PR. It cannot be skipped, reasoned past,
|
||||||
|
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.
|
||||||
+18
-2
@@ -52,15 +52,31 @@ func ParseDocMapConfig(localPath string) (*DocMapConfig, error) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("read doc-map file %q: %w", localPath, err)
|
return nil, fmt.Errorf("read doc-map file %q: %w", localPath, err)
|
||||||
}
|
}
|
||||||
|
return parseDocMapBytes(data, localPath)
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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. "main:main@<ref>").
|
||||||
|
//
|
||||||
|
// Use this when the config content has been fetched from a trusted VCS ref
|
||||||
|
// rather than read from the local workspace.
|
||||||
|
func ParseDocMapConfigContent(content, source string) (*DocMapConfig, error) {
|
||||||
|
data := []byte(content)
|
||||||
|
return parseDocMapBytes(data, source)
|
||||||
|
}
|
||||||
|
|
||||||
|
// parseDocMapBytes is the shared YAML parse implementation used by
|
||||||
|
// ParseDocMapConfig and ParseDocMapConfigContent.
|
||||||
|
func parseDocMapBytes(data []byte, source string) (*DocMapConfig, error) {
|
||||||
var cfg DocMapConfig
|
var cfg DocMapConfig
|
||||||
if err := yaml.UnmarshalWithOptions(data, &cfg, yaml.Strict()); err != nil {
|
if err := yaml.UnmarshalWithOptions(data, &cfg, yaml.Strict()); err != nil {
|
||||||
// Re-parse without strict mode to log which keys are unknown.
|
// Re-parse without strict mode to log which keys are unknown.
|
||||||
var relaxed DocMapConfig
|
var relaxed DocMapConfig
|
||||||
if err2 := yaml.Unmarshal(data, &relaxed); err2 != nil {
|
if err2 := yaml.Unmarshal(data, &relaxed); err2 != nil {
|
||||||
return nil, fmt.Errorf("parse doc-map YAML %q: %w", localPath, err)
|
return nil, fmt.Errorf("parse doc-map YAML %q: %w", source, err)
|
||||||
}
|
}
|
||||||
slog.Warn("doc-map YAML contains unknown keys (ignored)", "file", localPath, "error", err)
|
slog.Warn("doc-map YAML contains unknown keys (ignored)", "file", source, "error", err)
|
||||||
cfg = relaxed
|
cfg = relaxed
|
||||||
}
|
}
|
||||||
return &cfg, nil
|
return &cfg, nil
|
||||||
|
|||||||
@@ -510,3 +510,63 @@ func TestFileCoveredByDocMap_EmptyConfig(t *testing.T) {
|
|||||||
t.Error("expected false for empty config, got true")
|
t.Error("expected false for empty config, got true")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ============================================================
|
||||||
|
// ParseDocMapConfigContent
|
||||||
|
// ============================================================
|
||||||
|
|
||||||
|
func TestParseDocMapConfigContent_Valid(t *testing.T) {
|
||||||
|
content := `
|
||||||
|
mappings:
|
||||||
|
- paths:
|
||||||
|
- "lib/foo/**"
|
||||||
|
docs:
|
||||||
|
- docs/foo.md
|
||||||
|
`
|
||||||
|
cfg, err := ParseDocMapConfigContent(content, "owner/repo@main:.review-bot/doc-map.yml")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(cfg.Mappings) != 1 {
|
||||||
|
t.Fatalf("expected 1 mapping, got %d", len(cfg.Mappings))
|
||||||
|
}
|
||||||
|
if len(cfg.Mappings[0].Docs) != 1 || cfg.Mappings[0].Docs[0] != "docs/foo.md" {
|
||||||
|
t.Errorf("unexpected mapping: %+v", cfg.Mappings[0])
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseDocMapConfigContent_EmptyContent(t *testing.T) {
|
||||||
|
cfg, err := ParseDocMapConfigContent("", "test-source")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error for empty content: %v", err)
|
||||||
|
}
|
||||||
|
if len(cfg.Mappings) != 0 {
|
||||||
|
t.Errorf("expected 0 mappings for empty content, got %d", len(cfg.Mappings))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseDocMapConfigContent_InvalidYAML(t *testing.T) {
|
||||||
|
_, err := ParseDocMapConfigContent("mappings: [{{invalid", "test-source")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for invalid YAML, got nil")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseDocMapConfigContent_UnknownKeys(t *testing.T) {
|
||||||
|
content := `
|
||||||
|
mappings:
|
||||||
|
- paths:
|
||||||
|
- "lib/**"
|
||||||
|
docs:
|
||||||
|
- docs/foo.md
|
||||||
|
unknown_top_level_key: "should be warned but not fatal"
|
||||||
|
`
|
||||||
|
// Unknown top-level keys produce a warning but not an error.
|
||||||
|
cfg, err := ParseDocMapConfigContent(content, "test-source")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error for unknown keys: %v", err)
|
||||||
|
}
|
||||||
|
if len(cfg.Mappings) == 0 {
|
||||||
|
t.Error("expected mappings to be parsed despite unknown key")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user