fix: herdr target source, changelog-owner check; test: clean-room tolerant reader; chore: dependabot labels

.github/dependabot.yml

@@ -4,3 +4,7 @@ updates:
     directory: /
     schedule:
       interval: weekly
+    # Replaces Dependabot's default dependencies/github_actions labels, which
+    # the estate label rollout folds into kind:chore (cadence-ecosystem#553).
+    labels:
+      - kind:chore

.github/workflows/ci.yml

@@ -138,6 +138,7 @@ jobs:
         if: github.event_name == 'pull_request'
         env:
           BASE_SHA: ${{ github.event.pull_request.base.sha }}
+          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
           PR_AUTHOR: ${{ github.event.pull_request.user.login }}
           HEAD_REF: ${{ github.event.pull_request.head.ref }}
           HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }}

changelog_ownership_test.go

@@ -110,12 +110,120 @@ func TestChangelogOwnershipGuard(t *testing.T) {
 	}
 }
 
+// TestChangelogOwnershipGuardIgnoresMainsOwnReleaseEdit pins #458. In CI the
+// checkout is the pull-request merge commit, which also carries any release
+// that landed on main after the branch point. That release's CHANGELOG.md
+// edit belongs to main, not to the PR, whichever base SHA the event reports.
+func TestChangelogOwnershipGuardIgnoresMainsOwnReleaseEdit(t *testing.T) {
+	repo := t.TempDir()
+	runGit(t, repo, "init", "-b", "main")
+	runGit(t, repo, "config", "user.name", "Forgectl Test")
+	runGit(t, repo, "config", "user.email", "forgectl-test@example.invalid")
+	writeTestChangelog(t, repo, "# Changelog\n")
+	runGit(t, repo, "add", "CHANGELOG.md")
+	runGit(t, repo, "commit", "-m", "initial changelog")
+	branchPoint := strings.TrimSpace(runGit(t, repo, "rev-parse", "HEAD"))
+
+	runGit(t, repo, "checkout", "-b", "feature")
+	if err := os.WriteFile(filepath.Join(repo, "code.txt"), []byte("change\n"), 0o600); err != nil {
+		t.Fatalf("write code: %v", err)
+	}
+	runGit(t, repo, "add", "code.txt")
+	runGit(t, repo, "commit", "-m", "feature change")
+
+	runGit(t, repo, "checkout", "main")
+	writeTestChangelog(t, repo, "# Changelog\n\n## 1.0.0\n")
+	runGit(t, repo, "add", "CHANGELOG.md")
+	runGit(t, repo, "commit", "-m", "release")
+	mainTip := strings.TrimSpace(runGit(t, repo, "rev-parse", "HEAD"))
+
+	prHead := strings.TrimSpace(runGit(t, repo, "rev-parse", "feature"))
+
+	// Recreate refs/pull/N/merge: main's tip merged with the PR head.
+	runGit(t, repo, "checkout", "--detach", mainTip)
+	runGit(t, repo, "merge", "--no-ff", "-m", "merge pr", "feature")
+
+	guard, err := filepath.Abs("scripts/check-changelog-owner.sh")
+	if err != nil {
+		t.Fatalf("resolve changelog guard: %v", err)
+	}
+	for _, base := range []string{branchPoint, mainTip} {
+		if output, err := runChangelogGuardWithHead(t, repo, guard, base, prHead, "contributor", "feature", "cameronsjo/forgectl"); err != nil {
+			t.Fatalf("base %s: a release on main was blamed on the PR: %v\n%s", base, err, output)
+		}
+	}
+}
+
+// TestChangelogOwnershipGuardCatchesAnEditBehindAMergeFromMain is the negative
+// control for #458: a PR that edits CHANGELOG.md and then merges main into
+// itself must still be rejected, on the CI path (HEAD_SHA names the PR head
+// inside GitHub's merge commit) and on the fallback (HEAD is the PR tip itself,
+// whose second parent is main, not the PR).
+func TestChangelogOwnershipGuardCatchesAnEditBehindAMergeFromMain(t *testing.T) {
+	repo := t.TempDir()
+	runGit(t, repo, "init", "-b", "main")
+	runGit(t, repo, "config", "user.name", "Forgectl Test")
+	runGit(t, repo, "config", "user.email", "forgectl-test@example.invalid")
+	writeTestChangelog(t, repo, "# Changelog\n")
+	runGit(t, repo, "add", "CHANGELOG.md")
+	runGit(t, repo, "commit", "-m", "initial changelog")
+	branchPoint := strings.TrimSpace(runGit(t, repo, "rev-parse", "HEAD"))
+
+	runGit(t, repo, "checkout", "-b", "feature")
+	writeTestChangelog(t, repo, "# Changelog\n\nhand edit\n")
+	runGit(t, repo, "add", "CHANGELOG.md")
+	runGit(t, repo, "commit", "-m", "hand edit")
+
+	runGit(t, repo, "checkout", "main")
+	if err := os.WriteFile(filepath.Join(repo, "other.txt"), []byte("main\n"), 0o600); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+	runGit(t, repo, "add", "other.txt")
+	runGit(t, repo, "commit", "-m", "main moves on")
+	mainTip := strings.TrimSpace(runGit(t, repo, "rev-parse", "HEAD"))
+
+	// The PR merges main in, so its own tip is a merge whose ^2 is main.
+	runGit(t, repo, "checkout", "feature")
+	runGit(t, repo, "merge", "--no-ff", "-m", "merge main", "main")
+	prHead := strings.TrimSpace(runGit(t, repo, "rev-parse", "HEAD"))
+
+	guard, err := filepath.Abs("scripts/check-changelog-owner.sh")
+	if err != nil {
+		t.Fatalf("resolve changelog guard: %v", err)
+	}
+
+	// Fallback path: HEAD is the PR tip, HEAD_SHA unset.
+	for _, base := range []string{branchPoint, mainTip} {
+		if output, err := runChangelogGuard(t, repo, guard, base, "contributor", "feature", "cameronsjo/forgectl"); err == nil {
+			t.Fatalf("fallback, base %s: a CHANGELOG edit behind a merge from main passed\n%s", base, output)
+		}
+	}
+
+	// CI path: GitHub's merge commit of main's tip and the PR head.
+	runGit(t, repo, "checkout", "--detach", mainTip)
+	runGit(t, repo, "merge", "--no-ff", "-m", "merge pr", prHead)
+	for _, base := range []string{branchPoint, mainTip} {
+		if output, err := runChangelogGuardWithHead(t, repo, guard, base, prHead, "contributor", "feature", "cameronsjo/forgectl"); err == nil {
+			t.Fatalf("CI path, base %s: a CHANGELOG edit behind a merge from main passed\n%s", base, output)
+		}
+	}
+}
+
 func runChangelogGuard(t *testing.T, repo, guard, base, author, headRef, headRepo string) (string, error) {
+	t.Helper()
+	return runChangelogGuardWithHead(t, repo, guard, base, "", author, headRef, headRepo)
+}
+
+// runChangelogGuardWithHead pins HEAD_SHA explicitly (empty means unset for
+// the script), so an exported HEAD_SHA in the developer's shell cannot change
+// which path a test exercises.
+func runChangelogGuardWithHead(t *testing.T, repo, guard, base, headSHA, author, headRef, headRepo string) (string, error) {
 	t.Helper()
 	cmd := exec.CommandContext(t.Context(), guard)
 	cmd.Dir = repo
 	cmd.Env = append(os.Environ(),
 		"BASE_SHA="+base,
+		"HEAD_SHA="+headSHA,
 		"PR_AUTHOR="+author,
 		"HEAD_REF="+headRef,
 		"HEAD_REPO="+headRepo,

internal/cli/recipe.go

@@ -109,12 +109,14 @@ func newRecipeAfkCmd(deps module.Deps) *cobra.Command {
 }
 
 func runRecipeAfk(ctx context.Context, runner exec.Runner, opts recipeAfkOptions) error {
-	target, ok := resolveRecipeHerdrTarget(opts.Target)
+	target, source, ok := resolveRecipeHerdrTarget(opts.Target)
 	if !ok {
 		return WithExitCode(fmt.Errorf("no Herdr target found; pass --target or run inside a Herdr pane with %s or %s set", herdrPaneIDEnv, herdrActivePaneIDEnv), 2)
 	}
 	if err := validateRecipeHerdrTarget(target); err != nil {
-		return WithExitCode(err, 2)
+		// Name the source: a bad value inherited from HERDR_PANE_ID gives the
+		// operator no other path back to the variable that set it (#464).
+		return WithExitCode(fmt.Errorf("%w (from %s)", err, source), 2)
 	}
 	if opts.Rename != "" {
 		if err := validateRecipeRename(opts.Rename); err != nil {
@@ -398,14 +400,14 @@ func validateRecipePrompt(prompt string) error {
 	return nil
 }
 
-func resolveRecipeHerdrTarget(explicit string) (string, bool) {
+func resolveRecipeHerdrTarget(explicit string) (target, source string, ok bool) {
 	if explicit != "" {
-		return explicit, true
+		return explicit, "--target", true
 	}
 	for _, key := range herdrTargetEnvKeys {
 		if value, ok := lookupRecipeEnv(key); ok && value != "" {
-			return value, true
+			return value, key, true
 		}
 	}
-	return "", false
+	return "", "", false
 }

internal/cli/recipe_test.go

@@ -79,26 +79,30 @@ func TestResolveRecipeHerdrTarget(t *testing.T) {
 		explicit string
 		env      map[string]string
 		want     string
+		wantSrc  string
 		wantErr  bool
 	}{
 		{
 			name:     "explicit target wins",
 			explicit: "reviewer",
 			env:      map[string]string{herdrPaneIDEnv: "w1:p2"},
 			want:     "reviewer",
+			wantSrc:  "--target",
 		},
 		{
 			name: "pane id env wins over active pane env",
 			env: map[string]string{
 				herdrPaneIDEnv:       "w1:p2",
 				herdrActivePaneIDEnv: "w1:p3",
 			},
-			want: "w1:p2",
+			want:    "w1:p2",
+			wantSrc: herdrPaneIDEnv,
 		},
 		{
-			name: "active pane fallback",
-			env:  map[string]string{herdrActivePaneIDEnv: "w1:p3"},
-			want: "w1:p3",
+			name:    "active pane fallback",
+			env:     map[string]string{herdrActivePaneIDEnv: "w1:p3"},
+			want:    "w1:p3",
+			wantSrc: herdrActivePaneIDEnv,
 		},
 		{
 			name:    "missing target fails",
@@ -110,7 +114,7 @@ func TestResolveRecipeHerdrTarget(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			withRecipeEnv(t, tt.env)
-			got, ok := resolveRecipeHerdrTarget(tt.explicit)
+			got, src, ok := resolveRecipeHerdrTarget(tt.explicit)
 			if tt.wantErr {
 				if ok {
 					t.Fatal("resolveRecipeHerdrTarget() = ok, want missing target")
@@ -120,13 +124,27 @@ func TestResolveRecipeHerdrTarget(t *testing.T) {
 			if !ok {
 				t.Fatal("resolveRecipeHerdrTarget() = missing target, want ok")
 			}
-			if got != tt.want {
-				t.Fatalf("resolveRecipeHerdrTarget() = %q, want %q", got, tt.want)
+			if got != tt.want || src != tt.wantSrc {
+				t.Fatalf("resolveRecipeHerdrTarget() = %q from %q, want %q from %q", got, src, tt.want, tt.wantSrc)
 			}
 		})
 	}
 }
 
+// TestRecipeAfkBadTargetNamesItsSource pins #464: a malformed target inherited
+// from the environment names the variable that supplied it, and nothing runs.
+func TestRecipeAfkBadTargetNamesItsSource(t *testing.T) {
+	withRecipeEnv(t, map[string]string{herdrPaneIDEnv: "--config=/tmp/x"})
+	fake := &exec.FakeRunner{}
+	err := runRecipeAfk(context.Background(), fake, recipeAfkOptions{Prompt: "/go:afk"})
+	if err == nil || !strings.Contains(err.Error(), "from "+herdrPaneIDEnv) {
+		t.Fatalf("runRecipeAfk() error = %v, want it to name %s", err, herdrPaneIDEnv)
+	}
+	if len(fake.Calls) != 0 {
+		t.Fatalf("runRecipeAfk() ran %d commands on a rejected target", len(fake.Calls))
+	}
+}
+
 // TestRecipeAfkSteps pins the ordering decision, including the flag that stops
 // a self-targeted run from deadlocking against itself.
 func TestRecipeAfkSteps(t *testing.T) {

internal/pr/local.go

@@ -296,6 +296,12 @@ func (c *Client) rejectCleanRoomPath(absPath string) error {
 // for this use: the recorded string is only ever compared against a candidate
 // path in order to REFUSE. It never becomes an argv, and a broader view here
 // can only refuse more, never act on more.
+//
+// Do not route this through decodeBreadcrumbRecord: the strict decoder rejects
+// a record from a newer forgectl, and skipping it here would ALLOW its
+// workspace. TestPrepareLocal_RefusesCleanRoomFromRecordStrictDecoderRejects
+// pins that. repair.go's refFromRawRecord is the other tolerant reader, kept
+// tolerant for the same reason.
 func (c *Client) recordedWorkspaceFor(real string) (string, bool) {
 	entries, err := os.ReadDir(c.sessionsDir)
 	if err != nil {

internal/pr/local_test.go

@@ -483,3 +483,56 @@ func TestPrepareLocal_RefusesCleanRoomAcrossTMPDIRChange(t *testing.T) {
 		})
 	}
 }
+
+// TestPrepareLocal_RefusesCleanRoomFromRecordStrictDecoderRejects pins the
+// clean-room guard's TOLERANT reader (#504). recordedWorkspaceFor reads the
+// workspace string without the strict record decoder, so a breadcrumb from a
+// newer forgectl (a version this build does not read) still refuses its
+// workspace. Routing that reader through decodeBreadcrumbRecord would skip the
+// record and allow the path; this test goes red when that happens.
+func TestPrepareLocal_RefusesCleanRoomFromRecordStrictDecoderRejects(t *testing.T) {
+	elsewhere := t.TempDir()
+	workspace := filepath.Join(elsewhere, "forgectl-workflow-future1")
+	if err := os.MkdirAll(workspace, 0o750); err != nil {
+		t.Fatalf("mkdir workspace: %v", err)
+	}
+
+	sessionsDir := t.TempDir()
+	c := New(localGitRunner(), WithSessionsDir(sessionsDir), WithTmuxSession("forgectl"))
+
+	ref := Ref{Owner: "o", Repo: "r", Number: 1}
+	path, err := writeBreadcrumb(sessionsDir, ref, Breadcrumb{
+		Version:   breadcrumbVersion + 1,
+		Workspace: workspace,
+		Ref:       ref.String(),
+		Agent:     "claude",
+		CreatedAt: time.Now().UTC(),
+	})
+	if err != nil {
+		t.Fatalf("write breadcrumb: %v", err)
+	}
+	// The fixture must be one the strict decoder rejects, or this test proves
+	// nothing about the tolerant reader.
+	data, err := os.ReadFile(path) //nolint:gosec // path is the breadcrumb this test just wrote under t.TempDir()
+	if err != nil {
+		t.Fatalf("read breadcrumb: %v", err)
+	}
+	if _, err := decodeBreadcrumbRecord(data, path); err == nil {
+		t.Fatal("fixture precondition: the strict decoder must reject a newer-version record")
+	}
+
+	// The workspace sits outside the current $TMPDIR, so only the recorded
+	// breadcrumb can refuse it; the prefix scan cannot.
+	t.Setenv("TMPDIR", t.TempDir())
+
+	_, err = c.PrepareLocal(context.Background(), workspace, PrepareLocalOpts{
+		Agent:      "codex",
+		Provenance: ReviewProvenanceOperatorAuthored,
+	})
+	if err == nil {
+		t.Fatal("a clean-room workspace recorded by a newer forgectl must still be refused")
+	}
+	if !strings.Contains(err.Error(), "clean-room workspace") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}

internal/pr/repair.go

@@ -450,6 +450,9 @@ func cappedRecordBytes(raw []byte) string {
 // `--forget-if-absent` was built to clear, leaving `rm` as the only escape
 // again. The caller proceeds and SAYS SO instead, in the log and in the
 // confirmation prompt.
+//
+// local.go's recordedWorkspaceFor is the other tolerant reader: it skips the
+// strict decoder so a newer build's record still refuses its clean room.
 func refFromRawRecord(data []byte) (Ref, bool) {
 	var shallow struct {
 		Ref   string `json:"ref"`

internal/sops/driver_test.go

@@ -212,17 +212,21 @@ func TestIntegration_RoundTripAndDiffShape(t *testing.T) {
 		}
 	}
 
-	// The changed-line count: one content line plus sops' lastmodified and
-	// mac. Counted directly rather than through git, so the test needs no
-	// repository history.
-	changed := 0
-	for key, line := range afterLines {
-		if beforeLines[key] != line {
-			changed++
+	// Which lines changed: the value and sops' mac always; lastmodified too,
+	// except when the fixture's encryption and this write land in the same
+	// second, since sops stores it at one-second resolution. Counting to a
+	// fixed 3 made that a timing flake on fast runners. Compared directly
+	// rather than through git, so the test needs no repository history.
+	changed := changedKeys(beforeLines, afterLines)
+	must := map[string]bool{"llm_key_hermes": true, "mac": true}
+	for _, key := range changed {
+		if !must[key] && key != "lastmodified" {
+			t.Errorf("unexpected changed line %q (changed: %v)", key, changed)
 		}
+		delete(must, key)
 	}
-	if changed != 3 {
-		t.Errorf("%d lines changed, want 3 (the value plus sops' lastmodified and mac): %v", changed, changedKeys(beforeLines, afterLines))
+	for key := range must {
+		t.Errorf("expected the %q line to change (changed: %v)", key, changed)
 	}
 
 }