fix(git-submodules): refuse a non-positive stale_after, and never report an unreadable history as neglect

README.md

@@ -75,6 +75,14 @@ currency:
 # red on a healthy tree, which trains every reader to ignore it. Leave it unset and
 # the verdict is the structural one alone.
 #
+# The bound must be a POSITIVE duration: zero (or less) is refused rather than
+# honoured, because it would mark EVERY not-current pin stale — the permanently-red
+# gate this knob exists to avoid.
+#
+# The age is read from this repo's OWN history, and an UNREADABLE history is never
+# reported as neglect: it is a structural failure that names the read error, because
+# an unknown age is not a claim that the owner neglected the pin.
+#
 #   git-submodules: {mode: currency, stale_after: "168h"}
 
 # Assert the harness config files are identical to charly's twins.

candy/plugin-task/currency_test.go

@@ -511,4 +511,73 @@ func TestGitSubmodules_CurrencyStalenessBoundsNeglect(t *testing.T) {
 	if st != spec.StatusFail || !strings.Contains(msg, "not a duration") {
 		t.Fatalf("a malformed bound must fail loud; got %s: %s", st, msg)
 	}
+
+	// Arm 6 — a NON-POSITIVE bound is refused. Zero (or less) is not a bound: it marks
+	// EVERY not-current pin stale, which is precisely the permanently-red gate this knob
+	// exists to avoid, so it is an operator error the verb refuses rather than honours.
+	// The validator's round-1 observation named it.
+	for _, bad := range []string{"0s", "0", "-1h"} {
+		st, msg = runMaintenanceVerbIn(oldDir, "git-submodules", map[string]any{
+			"mode": "currency", "stale_after": bad,
+		})
+		if st != spec.StatusFail || !strings.Contains(msg, "not a POSITIVE duration") {
+			t.Fatalf("a non-positive bound must be refused; got %s: %s", st, msg)
+		}
+		if strings.Contains(msg, "STALE pin(s)") {
+			t.Fatalf("a refused bound must not judge staleness; got:\n%s", msg)
+		}
+	}
+
+	// Arm 7 — a FAILED READ is not neglect. When the repo's history cannot be read, the
+	// age of the last advance is UNKNOWN, and an unknown is not a claim that the owner
+	// neglected the pin. It is reported as a structural failure that names the read
+	// error — the workflow being unable to JUDGE the pin — and never as a stale verdict.
+	//
+	// The corruption is real: the superproject's HEAD commit object is removed, so the
+	// history genuinely cannot be read. The tree is otherwise the same neglect fixture, so
+	// the ONLY difference from Arm 2 is the readability of the evidence.
+	brokenDir, _, _ := curStale(t, tmp, "broken", true, 240*time.Hour)
+	brokenSHA := curGit(t, "-C", brokenDir, "rev-parse", "HEAD")
+	brokenObj := filepath.Join(brokenDir, ".git", "objects", brokenSHA[:2], brokenSHA[2:])
+	if err := os.Remove(brokenObj); err != nil {
+		t.Fatalf("remove the HEAD commit object: %v", err)
+	}
+	st, msg = runMaintenanceVerbIn(brokenDir, "git-submodules", map[string]any{
+		"mode": "currency", "stale_after": bound,
+	})
+	if st != spec.StatusFail {
+		t.Fatalf("an unreadable history must fail, not pass; got %s: %s", st, msg)
+	}
+	if strings.Contains(msg, "STALE pin(s)") {
+		t.Fatalf("an unreadable history must never be reported as neglect; got:\n%s", msg)
+	}
+	if !strings.Contains(msg, "could not be read") {
+		t.Fatalf("an unreadable history must name the read failure; got:\n%s", msg)
+	}
+
+	// Arm 8 — a pin in NO commit of this repo's history. The path is in the index and in
+	// .gitmodules, but this repo has never committed an advance of it, so its age is
+	// unbounded and it IS neglected. This is the third outcome of the read, and it is
+	// the opposite of Arm 7: the history reads FINE here (the repo has commits), it simply
+	// holds no advance of THIS path. A read that returns nothing is a fact about the
+	// bookkeeping, not a failure to read it.
+	noHistDir, run, c1, c2, _ := curSuper(t, filepath.Join(tmp, "nohist"))
+	curRecord(t, noHistDir, "d", c2) // current: isolates the arm under test
+	curRecord(t, noHistDir, "src", c1)
+	// Commit ONLY .gitmodules: the gitlinks stay in the index, so the pins exist for the
+	// currency mode (which reads the INDEX) while no commit of this repo ever touched
+	// the path `src`.
+	run("commit", "-qm", "the modules only", "--", ".gitmodules")
+	st, msg = runMaintenanceVerbIn(noHistDir, "git-submodules", map[string]any{
+		"mode": "currency", "stale_after": bound,
+	})
+	if st != spec.StatusFail {
+		t.Fatalf("a pin this repo has never advanced must fail; got %s: %s", st, msg)
+	}
+	if !strings.Contains(msg, "has never been advanced here") {
+		t.Fatalf("a pin in no commit must be reported as never advanced here; got:\n%s", msg)
+	}
+	if strings.Contains(msg, "could not be read") {
+		t.Fatalf("a readable history must not be reported as unreadable; got:\n%s", msg)
+	}
 }

candy/plugin-task/verbs.go

@@ -878,6 +878,14 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status
 				return spec.StatusFail, fmt.Sprintf(
 					"git-submodules: stale_after %q is not a duration this verb can parse (e.g. \"168h\")", raw)
 			}
+			// A bound of zero or less is not a bound: it marks EVERY not-current pin
+			// stale, which is the permanently-red gate this knob exists to avoid. The
+			// validator's round-1 observation named it, and it is refused here rather
+			// than left to the operator's discretion.
+			if d <= 0 {
+				return spec.StatusFail, fmt.Sprintf(
+					"git-submodules: stale_after %q is not a POSITIVE duration — a bound of zero or less marks every not-current pin stale, which is the permanently-red gate this bound exists to avoid", raw)
+			}
 			staleAfter, staleBoundSet = d, true
 		}
 		type pin struct {
@@ -985,25 +993,50 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status
 		// lastAdvancedAt reads the AGE of this repo's OWN bookkeeping for a pin: the
 		// timestamp of the last commit that touched the pin's path. This is the owner's own
 		// record of when it last advanced the pin — never the pin's distance from the served
-		// ref, which is churn. An ABSENT record (the path is in no commit of this repo's
-		// history) is reported as "never advanced here" rather than as a fresh pin, because
-		// a path with no bookkeeping has not been advanced for as long as the repo has
-		// existed.
-		lastAdvancedAt := func(p string) (time.Time, bool) {
-			out, _, exit := hostCapture(context.Background(), projDir,
+		// ref, which is churn.
+		//
+		// The THREE outcomes are different and must not be conflated, because they mean
+		// different things and only ONE of them is a claim about the pin:
+		//   * a TIMESTAMP — the age, measured;
+		//   * an EMPTY result — the path is in no commit of this repo's history, so this
+		//     repo has never advanced it: its age is unbounded and it IS neglected;
+		//   * a FAILING read — the age could not be measured at all (a broken object
+		//     store, a transient git failure). That is the workflow being unable to JUDGE
+		//     the pin, and it is reported as such — never as neglect, which would be a
+		//     claim the evidence does not support.
+		// The validator's round-1 observation named the conflation; the outcomes are now
+		// separate.
+		//
+		// A repo with NO COMMIT AT ALL is the third case, and it is a FACT rather than a
+		// failure: `git log` refuses on an unborn HEAD, but the refusal means "there is
+		// no history here", which is exactly the "never advanced" claim. It is told apart
+		// from a genuine read failure by COUNTING the repo's commits: zero commits means
+		// there is nothing here to have advanced anything (a fact), while a count that
+		// cannot be produced, or a history that exists but whose age for this path cannot
+		// be read, is a failure to JUDGE.
+		lastAdvancedAt := func(p string) (at time.Time, how, detail string) {
+			out, errOut, exit := hostCapture(context.Background(), projDir,
 				fmt.Sprintf("git log -1 --format=%%ct -- %s", shellQuote(p)))
 			if exit != 0 {
-				return time.Time{}, false
+				cOut, cErr, cExit := hostCapture(context.Background(), projDir, "git rev-list --all --count")
+				if cExit != 0 {
+					return time.Time{}, "read-failed", firstLine(cErr)
+				}
+				if n, perr := strconv.Atoi(strings.TrimSpace(cOut)); perr == nil && n == 0 {
+					// This repo has no commit at all: nothing was ever advanced here.
+					return time.Time{}, "no-history", ""
+				}
+				return time.Time{}, "read-failed", firstLine(errOut)
 			}
 			ts := strings.TrimSpace(out)
 			if ts == "" {
-				return time.Time{}, false
+				return time.Time{}, "no-history", ""
 			}
 			secs, err := strconv.ParseInt(ts, 10, 64)
 			if err != nil {
-				return time.Time{}, false
+				return time.Time{}, "read-failed", "unparsable timestamp " + strconv.Quote(ts)
 			}
-			return time.Unix(secs, 0), true
+			return time.Unix(secs, 0), "ok", ""
 		}
 
 		// THE GRAPH, classified once for EVERY path, then the depth rule walked over EVERY
@@ -1219,8 +1252,18 @@ func runGitSubmodules(projDir string, in params.GitSubmodulesInput) (spec.Status
 				if !r.leaf || !staleEligible(r.state) {
 					continue
 				}
-				at, known := lastAdvancedAt(r.path)
-				if !known {
+				at, how, detail := lastAdvancedAt(r.path)
+				switch how {
+				case "read-failed":
+					// The age could not be measured. That is the workflow being unable to
+					// JUDGE the pin — a structural-class failure that names the read
+					// error — and NOT a claim that the pin is neglected. A transient git
+					// error must never manufacture a stale verdict.
+					structural = append(structural, fmt.Sprintf(
+						"%s: the age of this repo's own last advance of it could not be read (%s) — the staleness of this pin cannot be judged",
+						r.path, detail))
+					continue
+				case "no-history":
 					stale = append(stale, fmt.Sprintf(
 						"%s: %s, and this repo's own history records no advance of it — it has never been advanced here",
 						r.path, r.state))

scripts/gate-task.sh

@@ -524,7 +524,13 @@ echo "$exp_out" | grep -q 'which is ITSELF an edge' || { echo "FAIL: the refusal
 # ---------------------------------------------------------------------------
 . "$FIX/shas"
 stale_fixture() {
-  local dir="$1" stamp="$2"
+  # The task name and the bound are parameters because every arm's output must be
+  # identifiable by ITSELF. Two arms used to share one fixture — and therefore one
+  # recorded SHA — so an evidence paste keyed only by the SHA was ambiguous: the
+  # same key selected the healthy arm's verdict. Now each arm has its OWN fixture
+  # (so its own SHA) and its OWN task name (which appears in its own output), and
+  # an evidence paste is keyed by the arm, not by a SHA.
+  local dir="$1" stamp="$2" task="${3:-currency-stale}" bound="${4:-168h}"
   mkdir -p "$dir"
   (
     cd "$dir" && git init -q -b main
@@ -537,14 +543,14 @@ stale_fixture() {
     # not merely "differs (no checkout to judge ancestry)".
     git -c protocol.file.allow=always submodule update --init -q gap
   )
-  cat > "$dir/charly.yml" <<'YML'
-currency-stale:
+  cat > "$dir/charly.yml" <<YML
+$task:
   task:
     description: ask whether every pin is current, with a bound on this repo's own neglect
     dir: .
     plan:
       - check: no pin is neglected past the bound
-        git-submodules: {mode: currency, stale_after: "168h"}
+        git-submodules: {mode: currency, stale_after: "$bound"}
         context: [runtime]
 YML
 }
@@ -563,15 +569,63 @@ printf '%s\n' "$stale_out" | grep -q '(bound 168h)' || {
 printf '%s\n' "$stale_out" | sed 's/^/   | /'
 
 echo "== verb:git-submodules currency (stale_after: the SAME pin, advanced recently, must PASS) =="
-stale_fixture "$WORK/stale-fresh" "$(date -u +%Y-%m-%dT%H:%M:%SZ)"
-fresh_out="$(cd "$WORK/stale-fresh" && "$CH" task currency-stale 2>&1)" || {
+stale_fixture "$WORK/stale-fresh" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" currency-fresh
+fresh_out="$(cd "$WORK/stale-fresh" && "$CH" task currency-fresh 2>&1)" || {
   echo "FAIL: a pin advanced just now is churn, not neglect" >&2; printf '%s\n' "$fresh_out" >&2; exit 1; }
 printf '%s\n' "$fresh_out" | grep -q '0 failed' || {
   echo "FAIL: the run must pass; got: $fresh_out" >&2; exit 1; }
 printf '%s\n' "$fresh_out" | grep -q '1 of 1 pin(s) NOT current' || {
   echo "FAIL: the not-current count must still be reported; got: $fresh_out" >&2; exit 1; }
 echo "   the same behind pin, advanced recently, passed the bound"
 
+echo "== verb:git-submodules currency (stale_after: a NON-POSITIVE bound must be REFUSED) =="
+# Zero (or less) is not a bound: it would mark EVERY not-current pin stale, which is
+# the permanently-red gate the knob exists to avoid. The verb refuses it rather than
+# honouring an operator error that would silently disarm the control in the other
+# direction.
+#
+# This arm has its OWN fixture, so its recorded SHA is its own and cannot be confused
+# with the healthy arm's SHA; and its own task name, which appears in its own output.
+stale_fixture "$WORK/stale-nonpositive" "2020-01-01T00:00:00Z" currency-nonpositive 0s
+if nonpos_out="$(cd "$WORK/stale-nonpositive" && "$CH" task currency-nonpositive 2>&1)"; then
+  echo "FAIL: a non-positive bound must be refused" >&2; printf '%s\n' "$nonpos_out" >&2; exit 1
+fi
+printf '%s\n' "$nonpos_out" | sed 's/^/   | /'
+printf '%s\n' "$nonpos_out" | grep -q 'not a POSITIVE duration' || {
+  echo "FAIL: the refusal must name the reason; got: $nonpos_out" >&2; exit 1; }
+printf '%s\n' "$nonpos_out" | grep -q 'STALE pin(s)' && {
+  echo "FAIL: a refused bound must not judge staleness; got: $nonpos_out" >&2; exit 1; }
+echo "   a bound of zero was refused, and no staleness was judged"
+
+echo "== verb:git-submodules currency (an UNREADABLE history is not neglect) =="
+# The same neglect fixture, with its HEAD commit object removed, so the history
+# genuinely cannot be read. The age of the last advance is then UNKNOWN — and an
+# unknown is not a claim that the owner neglected the pin. The verdict names the read
+# failure and never says STALE.
+stale_fixture "$WORK/stale-unreadable" "2020-01-01T00:00:00Z" currency-unreadable
+unread_sha="$(cd "$WORK/stale-unreadable" && git rev-parse HEAD)"
+rm -f "$WORK/stale-unreadable/.git/objects/${unread_sha:0:2}/${unread_sha:2}"
+# DEMONSTRATE that the removal genuinely broke the read the verb performs, so this
+# arm cannot pass for the wrong reason (a no-op removal would leave a readable
+# history and the arm would prove nothing). The very same read must FAIL here and
+# SUCCEED on the healthy fixture.
+if (cd "$WORK/stale-unreadable" && git log -1 --format=%ct -- gap) >/dev/null 2>&1; then
+  echo "FAIL: the unreadable fixture's history read must FAIL, or this arm proves nothing" >&2; exit 1
+fi
+if ! (cd "$WORK/stale-old" && git log -1 --format=%ct -- gap) >/dev/null 2>&1; then
+  echo "FAIL: the healthy fixture's history read must SUCCEED, or the contrast proves nothing" >&2; exit 1
+fi
+echo "   the fixture's history read genuinely fails, and the healthy fixture's succeeds"
+if unread_out="$(cd "$WORK/stale-unreadable" && "$CH" task currency-unreadable 2>&1)"; then
+  echo "FAIL: an unreadable history must fail, not pass" >&2; printf '%s\n' "$unread_out" >&2; exit 1
+fi
+printf '%s\n' "$unread_out" | sed 's/^/   | /'
+printf '%s\n' "$unread_out" | grep -q 'could not be read' || {
+  echo "FAIL: the verdict must name the read failure; got: $unread_out" >&2; exit 1; }
+printf '%s\n' "$unread_out" | grep -q 'STALE pin(s)' && {
+  echo "FAIL: an unreadable history must never be reported as neglect; got: $unread_out" >&2; exit 1; }
+echo "   an unreadable history was reported as unreadable, not as neglect"
+
 echo "== verb:file-parity (identical must PASS, drift must FAIL) =="
 par_out="$("$CH" task parity)"; echo "$par_out" | grep -q '0 failed' || { echo "FAIL: identical pair must pass" >&2; exit 1; }
 echo drifted >> r.copy