fix(send): only fail on non-benign skips

[omarkohl]

Private commits and up-to-date PRs are expected outcomes, not failures,
so 'jip send' should not exit non-zero for them. Generalize the
up-to-date exemption into a 'benign' flag on skipReason (cascades
inherit it from their ancestor); exit non-zero only for real problems —
conflicts, divergence, remote-ahead, missing description, push failures.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Generic skip error 🐞 Bug ◔ Observability

Description

executeSend now returns a generic "changes skipped" error for non-benign skips, removing the
previously included skip count in the error text. Since main.go prints only the error to stderr,
stderr-only CI/log views lose the quick summary even though detailed skip reasons are still printed
to stdout.

Code

cmd/send.go[R645-646]

+	if nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges) > 0 {
+		return errors.New("changes skipped")

Evidence

The diff shows executeSend now returns errors.New("changes skipped") when non-benign skips
exist, and the program’s main function prints only the error to stderr before exiting. This means
the detailed skip output on stdout may be missed in stderr-only views, and the error no longer
carries the skip count summary.

cmd/send.go[640-647]
main.go[10-14]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`executeSend` returns `errors.New("changes skipped")` when there are non-benign skips. This drops the count of non-benign skipped changes from the error message.

Because the CLI entrypoint prints only the returned error to stderr and exits non-zero, the stderr message should ideally include the count for quick diagnosis in stderr-only contexts.

### Issue Context
The command already prints detailed skipped-change information to stdout via `printAllSkipped(...)` / `printPreSkippedChanges(...)`, so this is not a functional bug; it’s an observability/detail regression for stderr-only consumers.

### Fix Focus Areas
- cmd/send.go[640-647]

### Suggested fix
Change the returned error to include the non-benign skip count, e.g.:
- compute `n := nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges)`
- `if n > 0 { return fmt.Errorf("%d change(s) skipped", n) }`

Apply similarly to the other non-benign skip return sites if you want consistent stderr messaging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit a64f8c0

Results up to commit ca726cd

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Generic skip error 🐞 Bug ◔ Observability

Description

executeSend now returns a generic "changes skipped" error for non-benign skips, removing the
previously included skip count in the error text. Since main.go prints only the error to stderr,
stderr-only CI/log views lose the quick summary even though detailed skip reasons are still printed
to stdout.

Code

cmd/send.go[R645-646]

+	if nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges) > 0 {
+		return errors.New("changes skipped")

Evidence

The diff shows executeSend now returns errors.New("changes skipped") when non-benign skips
exist, and the program’s main function prints only the error to stderr before exiting. This means
the detailed skip output on stdout may be missed in stderr-only views, and the error no longer
carries the skip count summary.

cmd/send.go[640-647]
main.go[10-14]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`executeSend` returns `errors.New("changes skipped")` when there are non-benign skips. This drops the count of non-benign skipped changes from the error message.

Because the CLI entrypoint prints only the returned error to stderr and exits non-zero, the stderr message should ideally include the count for quick diagnosis in stderr-only contexts.

### Issue Context
The command already prints detailed skipped-change information to stdout via `printAllSkipped(...)` / `printPreSkippedChanges(...)`, so this is not a functional bug; it’s an observability/detail regression for stderr-only consumers.

### Fix Focus Areas
- cmd/send.go[640-647]

### Suggested fix
Change the returned error to include the non-benign skip count, e.g.:
- compute `n := nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges)`
- `if n > 0 { return fmt.Errorf("%d change(s) skipped", n) }`

Apply similarly to the other non-benign skip return sites if you want consistent stderr messaging.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[omarkohl]

Changes since last push

cmd/send.go (+6, -7)

index 096f12fea2..ac467c545c 100644
--- a/cmd/send.go
+++ b/cmd/send.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -299,8 +298,8 @@
 		dags = filteredDAGs
 		if len(dags) == 0 && !opts.dryRun {
 			printPreSkippedChanges(w, preSkippedChanges)
-			if nonBenignSkips(nil, nil, preSkippedChanges) > 0 {
-				return errors.New("changes skipped — nothing to send")
+			if n := nonBenignSkips(nil, nil, preSkippedChanges); n > 0 {
+				return fmt.Errorf("%d change(s) skipped — nothing to send", n)
 			}
 			_, _ = fmt.Fprintf(w, "\nNothing to send.\n")
 			return nil
@@ -473,8 +472,8 @@
 		if len(skippedStates) > 0 || len(preSkippedChanges) > 0 {
 			printAllSkipped(w, skippedStates, skippedIDs, preSkippedChanges)
 		}
-		if nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges) > 0 {
-			return errors.New("changes skipped")
+		if n := nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges); n > 0 {
+			return fmt.Errorf("%d change(s) skipped", n)
 		}
 		return nil
 	}
@@ -642,8 +641,8 @@
 	}
 	// Only non-benign skips (conflicts, divergence, missing description, …)
 	// constitute a failure. Private commits and up-to-date PRs are expected.
-	if nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges) > 0 {
-		return errors.New("changes skipped")
+	if n := nonBenignSkips(skippedStates, skippedIDs, preSkippedChanges); n > 0 {
+		return fmt.Errorf("%d change(s) skipped", n)
 	}
 	return nil
 }

---

_{View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).
View the diff locally (will only work if you fetched the older commit at some point):
git range-diff main ca726cd a64f8c0
jj interdiff -f ca726cd -t a64f8c0}

[omarkohl]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit a64f8c0