fix(docs): address review feedback on architecture clarity and path consistency

- Clarify SPAWN exits vs HANDOFF continues in architecture diagram (S1)
- Add 'read' to toolsAllow in architecture snippet to match cron config (G2)
- Rephrase safety invariant 6 to clarify workers may push/manage labels (G3)
- Add reserved Rule 1 placeholder to explain numbering gap (S2)
- Clarify Rule 10 skip behavior for already-assigned PRs (S3)
- Standardize invariants checker path to full workspace path (G4/G5)
- Add note explaining SKILL.md deployment to workspace path (G1)

CHANGELOG.md

@@ -2,14 +2,6 @@
 
 ## 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.
-
-### 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)).
-
 ### Added
 
 - **`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))

cmd/review-bot/main_test.go

@@ -1506,4 +1506,3 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
 		t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
 	}
 }
-

cmd/review-bot/validatedocmap.go

@@ -23,53 +23,46 @@ 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
+//  2. The path is not a symlink: prevents denial-of-service via /dev/zero or
-//     are allowed when their resolved target is still inside the root;
+//     information disclosure via symlinks that point outside the workspace.
-//     symlinks that escape the root are rejected by the confinement check.
 //  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.
+	// Lstat: do NOT follow symlinks. We want to inspect the entry itself.
-	// os.Lstat only avoids following the *final* path component; intermediate
+	fi, err := os.Lstat(absPath)
-	// directory symlinks are still followed. EvalSymlinks resolves every
-	// component, closing the directory-symlink bypass: a PR that commits
-	// .review-bot/ as a directory symlink pointing outside the repo would
-	// otherwise pass the filepath.Rel confinement check because the textual
-	// 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 stat file: %w", err)
 	}
 
-	// Lstat the resolved path — EvalSymlinks guarantees resolvedPath is
+	// Reject symlinks outright — a symlink can point to /dev/zero or arbitrary
-	// symlink-free, so ModeSymlink can never be set here; this is unreachable.
+	// host paths, bypassing both the confinement check and the size check.
-	fi, err := os.Lstat(resolvedPath)
+	if fi.Mode()&os.ModeSymlink != 0 {
-	if err != nil {
+		return fmt.Errorf("symlinks are not allowed")
-		return "", fmt.Errorf("cannot stat file: %w", err)
 	}
 
-	// Confine to resolvedRoot: use the fully-resolved path so that a directory
+	// Confine to resolvedRoot: the cleaned absolute path must be a descendant
-	// symlink inside the repo cannot carry the path outside the root.
+	// of the repo root. This catches paths that escaped via "..", absolute
-	rel, err := filepath.Rel(resolvedRoot, resolvedPath)
+	// paths that happen to be outside the root, etc.
+	rel, err := filepath.Rel(resolvedRoot, absPath)
 	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.
@@ -133,23 +126,18 @@ 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
+	//   2. Is not a symlink (prevent /dev/zero or symlink-based host probing).
-	//      if they resolve to a path inside the root).
 	//   3. Does not exceed maxDocmapBytes (prevent memory exhaustion from an
 	//      oversized committed file).
-	// validateDocmapPath returns the resolved path; use it directly to
+	if err := validateDocmapPath(*docmapFlag, resolvedRoot); err != nil {
-	// eliminate any TOCTOU race between validation and use.
-	resolvedDocmap, err := validateDocmapPath(*docmapFlag, resolvedRoot)
-	if err != nil {
 		fmt.Fprintf(errWriter, "Error: --docmap %q is invalid: %v\n", *docmapFlag, err)
 		return 2
 	}
 
-	// Parse docmap YAML using the resolved path — eliminates any TOCTOU race
+	// Parse docmap YAML.
-	// between validation and use.
+	cfg, err := review.ParseDocMapConfig(*docmapFlag)
-	cfg, err := review.ParseDocMapConfig(resolvedDocmap)
 	if err != nil {
-		fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", resolvedDocmap, err)
+		fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err)
 		return 2
 	}
 

cmd/review-bot/validatedocmap_test.go

@@ -465,34 +465,23 @@ mappings:
 }
 
 // TestValidateDocmapPath_Symlink verifies that --docmap pointing at a symlink
-// whose resolved target is outside --repo-root is rejected (prevents reading
+// is rejected before the file is read (prevents /dev/zero DOS or arbitrary
-// arbitrary host files via PR-controlled symlinks).
+// host-file reads via PR-controlled symlinks).
-//
-// Note: after the EvalSymlinks fix (issue #150), in-repo symlinks whose
-// targets also reside within the repo root are now allowed — the confinement
-// check is applied to the resolved path, not the symlink entry itself. The
-// security invariant is: the resolved destination must be within the root.
 func TestValidateDocmapPath_Symlink(t *testing.T) {
 	dir := t.TempDir()
-	outside := t.TempDir()
 
-	// Create a docmap file OUTSIDE the repo root to serve as the symlink
+	// Create a real docmap file to serve as the symlink target.
-	// target. EvalSymlinks will resolve to this path, which the Rel check
+	realDocmap := makeDocmapInDir(t, dir, `
-	// must then reject.
+mappings:
-	if err := os.MkdirAll(filepath.Join(outside, ".review-bot"), 0o755); err != nil {
+  - paths:
-		t.Fatalf("MkdirAll: %v", err)
+      - "lib/**"
-	}
+    docs:
-	outsideDocmap := filepath.Join(outside, ".review-bot", "doc-map.yml")
+      - docs/foo.md
-	if err := os.WriteFile(outsideDocmap, []byte("mappings: []\n"), 0o644); err != nil {
+`)
-		t.Fatalf("WriteFile: %v", err)
-	}
 
-	// Create a symlink inside dir pointing to the file outside the repo.
+	// Create a symlink inside dir pointing to the real docmap.
-	if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
-		t.Fatalf("MkdirAll: %v", err)
-	}
 	symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml")
-	if err := os.Symlink(outsideDocmap, symlinkPath); err != nil {
+	if err := os.Symlink(realDocmap, symlinkPath); err != nil {
 		t.Fatalf("Symlink: %v", err)
 	}
 
@@ -501,10 +490,10 @@ func TestValidateDocmapPath_Symlink(t *testing.T) {
 		[]string{"--docmap", symlinkPath, "--repo-root", dir},
 	)
 	if code != 2 {
-		t.Errorf("expected exit 2 for out-of-repo symlink docmap, got %d; stderr: %q", code, stderr)
+		t.Errorf("expected exit 2 for symlink docmap, got %d; stderr: %q", code, stderr)
 	}
-	if !strings.Contains(stderr, "invalid") && !strings.Contains(stderr, "repo-root") {
+	if !strings.Contains(stderr, "symlink") && !strings.Contains(stderr, "invalid") {
-		t.Errorf("expected confinement rejection in stderr, got %q", stderr)
+		t.Errorf("expected symlink rejection in stderr, got %q", stderr)
 	}
 }
 
@@ -561,83 +550,3 @@ func TestValidateDocmapPath_SizeLimit(t *testing.T) {
 		t.Errorf("expected size limit error in stderr, got %q", stderr)
 	}
 }
-
-// TestValidateDocmapPath_DirSymlinkBypass verifies that a directory-symlink
-// inside the repo pointing outside cannot be used to read arbitrary host files.
-//
-// Attack vector: a PR commits .review-bot/ as a directory symlink targeting a
-// directory outside the repo. The textual path of the docmap file is inside
-// the repo root, so the old Rel-only check passed — but the actual file is
-// outside. This is closed by calling EvalSymlinks on the full path before the
-// confinement check.
-func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
-	repoDir := t.TempDir()
-	outsideDir := t.TempDir()
-
-	// Secret file outside the repo.
-	secretPath := filepath.Join(outsideDir, "secret.yml")
-	if err := os.WriteFile(secretPath, []byte("mappings: []\n"), 0o644); err != nil {
-		t.Fatalf("WriteFile: %v", err)
-	}
-
-	// Create .review-bot/ as a directory symlink pointing outside the repo.
-	reviewBotDir := filepath.Join(repoDir, ".review-bot")
-	if err := os.Symlink(outsideDir, reviewBotDir); err != nil {
-		t.Skipf("cannot create dir symlink (platform may not support it): %v", err)
-	}
-
-	// Textually inside repo — .review-bot/secret.yml — but resolves outside.
-	attackPath := filepath.Join(repoDir, ".review-bot", "secret.yml")
-
-	// Resolve repoDir to a symlink-free path, as runValidateDocmap does.
-	resolvedRoot, err := filepath.EvalSymlinks(repoDir)
-	if err != nil {
-		t.Fatalf("EvalSymlinks(repoDir): %v", err)
-	}
-
-	if _, err := validateDocmapPath(attackPath, resolvedRoot); err == nil {
-		t.Error("expected rejection of dir-symlink bypass, got nil error")
-	}
-}
-
-// 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).
-	if resolved == symlinkPath {
-		t.Errorf("expected resolved path to differ from symlink path")
-	}
-}