Merge pull request 'fix(#157): add never-close constraint to spec, S9 invariant, and regression test' (#158) from issue-157 into main

Reviewed-on: #158
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>

cmd/review-bot/validatedocmap.go

@@ -61,6 +61,13 @@ func validateDocmapPath(localPath, resolvedRoot string) error {
 		return fmt.Errorf("symlinks are not allowed")
 	}
 
+	// 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")
+	}
+
 	// 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)
@@ -171,6 +178,9 @@ 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)
 		}
@@ -189,7 +199,7 @@ func runValidateDocmap(args []string) int {
 	staleDocs := checkStaleDocs(cfg, resolvedRoot)
 	if len(staleDocs) > 0 {
 		failed = true
-		fmt.Fprintln(errWriter, "ERROR: stale docmap docs: entries (paths do not exist):")
+		fmt.Fprintln(errWriter, "ERROR: stale docmap entries (paths do not exist):")
 		for _, d := range staleDocs {
 			fmt.Fprintf(errWriter, "  %s\n", d)
 		}

cmd/review-bot/validatedocmap_test.go

@@ -599,3 +599,53 @@ func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
 		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)
+	}
+}

docs/dev-loop-spec.md

@@ -231,6 +231,8 @@ 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` |
 
 ---
 
@@ -263,9 +265,20 @@ 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 and #145
+## 9. Fixes for Issues #144, #145, and #157
 
 **Issue #144** (autonomous merge):
 The dispatch script contains no merge API calls anywhere. The `~/.openclaw/workspace/scripts/test/check-invariants.sh`
@@ -276,3 +289,13 @@ 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.