Merge pull request #957 from sortie-ai/fix/956-ci-failure-watch-window-ceiling

fix(config): bound reactions.ci_failure.watch_window_ms

Author: sergeyklay

CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- `reactions.ci_failure.watch_window_ms` now rejects a value above `9223372036854` (about 292 years) instead of converting it to a window of a fraction of a millisecond or to no bound at all. `0` still means no time limit. A deployment currently carrying a larger value is refused at startup and by `sortie validate` until the value is lowered.
+  ([#956](https://github.com/sortie-ai/sortie/issues/956))
+
 ### Changed
 
 - `reactions.review_comments`, `reactions.bot_review`, `reactions.merge_conflicts`, and `reactions.auto_merge` now bound a pending entry's age with a per-reaction `watch_window_ms` key instead of a hardcoded thirty-minute constant. The default stays `1800000` (thirty minutes), so a deployment that sets nothing behaves exactly as before; setting `0` removes the bound entirely. A workflow with no `auto_merge`, where a person reviews and merges, will normally want a larger value than the default. The four expiry log records changed their message text and renamed their `ttl_ms` attribute to `window_ms`.

cmd/sortie/validate_test.go

@@ -3401,6 +3401,103 @@ func TestValidateReviewAndMergeConflictReactionConfigs(t *testing.T) {
 	}
 }
 
+// TestValidateWatchWindowMSAcrossKinds covers every reaction kind that
+// carries watch_window_ms with a value above the shared ceiling. The
+// ci_failure kind is rejected during config.NewServiceConfig and surfaces
+// under a "config."-prefixed check produced by mapManagerError, while the
+// other four kinds pass config construction and are rejected by
+// ValidateReactionConfigs under a bare "reactions.<kind>" check, so each
+// row needs its own single-fault fixture: a ci_failure rejection would
+// otherwise fail NewServiceConfig and suppress every later diagnostic.
+func TestValidateWatchWindowMSAcrossKinds(t *testing.T) {
+	t.Parallel()
+
+	const overCeiling = 9223372036855
+
+	tests := []struct {
+		name      string
+		extraYAML string
+		wantCheck string
+	}{
+		{
+			name: "ci_failure",
+			extraYAML: `reactions:
+  ci_failure:
+    provider: github-actions
+    watch_window_ms: 9223372036855
+`,
+			wantCheck: "config.reactions.ci_failure.watch_window_ms",
+		},
+		{
+			name: "review_comments",
+			extraYAML: `reactions:
+  review_comments:
+    provider: gitea
+    watch_window_ms: 9223372036855
+`,
+			wantCheck: "reactions.review_comments",
+		},
+		{
+			name: "bot_review",
+			extraYAML: `reactions:
+  bot_review:
+    provider: gitea
+    watch_window_ms: 9223372036855
+`,
+			wantCheck: "reactions.bot_review",
+		},
+		{
+			name: "merge_conflicts",
+			extraYAML: `reactions:
+  merge_conflicts:
+    provider: gitea
+    watch_window_ms: 9223372036855
+`,
+			wantCheck: "reactions.merge_conflicts",
+		},
+		{
+			name: "auto_merge",
+			extraYAML: `reactions:
+  auto_merge:
+    provider: gitea
+    watch_window_ms: 9223372036855
+`,
+			wantCheck: "reactions.auto_merge",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			dir := t.TempDir()
+			wfPath := writeCustomWorkflowFile(t, dir, forgeFaultWorkflow(tt.extraYAML))
+
+			var stdout, stderr bytes.Buffer
+			code := run(context.Background(), []string{"validate", "--format", "json", wfPath}, &stdout, &stderr)
+			if code != 1 {
+				t.Fatalf("run(validate --format json) = %d, want 1; stderr: %s", code, stderr.String())
+			}
+
+			var out validateOutput
+			if err := json.Unmarshal(stdout.Bytes(), &out); err != nil {
+				t.Fatalf("json.Unmarshal(%q) error: %v", stdout.String(), err)
+			}
+			if out.Valid {
+				t.Errorf("validateOutput.Valid = true, want false")
+			}
+
+			d := diagWithCheck(out.Errors, tt.wantCheck)
+			if d == nil {
+				t.Fatalf("validateOutput.Errors = %v, want a diagnostic with check %q", out.Errors, tt.wantCheck)
+			}
+			if want := fmt.Sprintf("%d", overCeiling); !strings.Contains(d.Message, want) {
+				t.Errorf("diagnostic message = %q, want offending value %q", d.Message, want)
+			}
+		})
+	}
+}
+
 // TestValidateGiteaForge exercises the fold point end-to-end through
 // runValidate for a tracker.kind: gitea workflow, varying the reactions
 // and ci_feedback blocks to isolate one forge fault per case.

docs/architecture/05-workflow-specification.md

@@ -393,8 +393,8 @@ Extra fields:
 
 - `max_log_lines` (integer, via Extra): maximum CI log tail lines. Default: `50`.
 - `watch_window_ms` (integer, via Extra): bounds a pending CI entry's age, measured from the last
-  recorded head. Default: `86400000` (twenty-four hours). Must be non-negative. `0` removes the
-  clock bound.
+  recorded head. Default: `86400000` (twenty-four hours). MUST be non-negative and MUST NOT
+  exceed `9223372036854`. `0` removes the clock bound.
 
 **Reaction kind: `review_comments`**
 

docs/architecture/06-configuration-specification.md

@@ -278,6 +278,8 @@ This section is intentionally redundant so a coding agent can implement the conf
   `reactions.review_comments.provider`, ignores whether its `escalation` value is `label` or
   `comment`, and never falls through to another reaction kind's label. The primary path always
   parks by label; it borrows only this label name and no other review-reaction behavior
+- `reactions.ci_failure.watch_window_ms`: integer, default `86400000` (24 h); non-negative, not
+  above `9223372036854`; `0` removes the bound; re-read on every tick
 - `reactions.review_comments.poll_interval_ms`: integer, default `120000` (2 min); minimum `30000`
 - `reactions.review_comments.debounce_ms`: integer, default `60000` (60 sec); non-negative
 - `reactions.review_comments.max_continuation_turns`: integer, default `3`; positive

docs/architecture/12-ci-feedback-contract.md

@@ -302,7 +302,8 @@ the entry's lifetime.
 **Age and the watch window.** The entry's age is measured from `HeadRecordedAt`, the UTC time this
 process last recorded a head, falling back to the entry's creation time before any head has been
 recorded. `reactions.ci_failure.watch_window_ms` bounds that age (default `86400000`, twenty-four
-hours; `0` removes the clock bound). Past the window, the entry and its attempt counter are dropped
+hours; `0` removes the clock bound; a value above `9223372036854` is rejected when the typed
+configuration is built). Past the window, the entry and its attempt counter are dropped
 and a warning is logged; the fingerprint row is left intact. The watch also ends, whatever the clock
 says, on merge, on close, on a missing pull request (`ErrSCMNotFound`), and when the tracker issue
 reaches a `tracker.terminal_states` state.

docs/architecture/22-test-and-validation-matrix.md

@@ -34,6 +34,9 @@ Unless otherwise noted, Sections 17.1 through 17.7 are `Core Conformance`. Bulle
 - An absent `agent.max_consecutive_absences` key takes the default of `3`; `0` and negative
   values are rejected at config parse time; `SORTIE_AGENT_MAX_CONSECUTIVE_ABSENCES` overrides a
   file-supplied value and is rejected under the same rule
+- An absent `reactions.ci_failure.watch_window_ms` key takes the default; a negative value and a
+  value above `9223372036854` are rejected at config parse time, naming the field and the value;
+  `9223372036854` itself is accepted
 - Per-state concurrency override map normalizes state names and ignores invalid values
 - Prompt template renders `issue`, `attempt`, and `run`
 - Prompt rendering fails on unknown variables (strict mode)

docs/workflow-reference.md

@@ -986,7 +986,7 @@ Additional fields (via Extra):
 | Field             | Type    | Default      | Dynamic Reload | Description                                                                                                             |
 | ----------------- | ------- | ------------ | -------------- | ------------------------------------------------------------------------------------------------------------------------ |
 | `max_log_lines`   | integer | `50`         | Requires restart | Maximum CI log tail lines for prompt injection. `0` disables. Must be non-negative.                                   |
-| `watch_window_ms` | integer | `86400000`   | Every tick        | Bounds a pending entry's age, measured from the last recorded head. `0` removes the clock bound. Must be non-negative. |
+| `watch_window_ms` | integer | `86400000`   | Every tick        | Bounds a pending entry's age, measured from the last recorded head. `0` removes the clock bound. Must be non-negative and must not exceed `9223372036854`. |
 
 Example:
 
@@ -1294,8 +1294,7 @@ merged-or-closed drop branch, so setting `watch_window_ms: 0` leaves a `review_c
 until the tracker issue reaches a terminal state, and leaves an `auto_merge` entry polling
 until its own merge attempt returns the already-merged disposition. A quoted numeric value
 (for example `"1800000"`) is rejected for these four keys, although
-`reactions.ci_failure.watch_window_ms` accepts one. These four keys also reject a value
-above `9223372036854`, where `reactions.ci_failure.watch_window_ms` does not.
+`reactions.ci_failure.watch_window_ms` accepts one.
 
 #### Reaction kind: `label_commands`
 
@@ -1503,6 +1502,7 @@ reactions:
   configuration error.
 - `max_retries` must be non-negative for all kinds.
 - `escalation` must be `"label"` or `"comment"` for all kinds.
+- `watch_window_ms` must be non-negative and must not exceed `9223372036854` for `ci_failure`, `review_comments`, `bot_review`, `merge_conflicts`, and `auto_merge`.
 - `poll_interval_ms` must be >= `30000` for `review_comments`.
 - `debounce_ms` must be non-negative for `review_comments`.
 - `max_continuation_turns` must be positive for `review_comments`.
@@ -1523,8 +1523,9 @@ reactions:
 - When `provider` is absent or empty, all other fields in the kind sub-object are ignored.
 
 **Where each rule is enforced:** the rules that the config layer owns (reaction key shape,
-`max_retries`, `escalation`, `escalation_label`, and every `label_commands` rule) run during
-typed config construction, so `sortie validate` reports them offline. For the kind-specific
+`max_retries`, `escalation`, `escalation_label`, every `label_commands` rule, and
+`reactions.ci_failure`'s Extra keys) run during typed config construction, so `sortie validate`
+reports them offline. For the kind-specific
 rules, `sortie validate` runs the `review_comments`, `auto_merge`, `bot_review`,
 `merge_conflicts`, and `merge_completion` builders. These are the same builders used when the
 orchestrator constructs the reactions at startup, so both paths report the same invalid values.
@@ -3551,6 +3552,8 @@ Each error identifies the offending field path.
 | `config: ci_feedback.max_log_lines: invalid integer value: <val>`               | Non-integer value for `max_log_lines`.                                   | Use a plain integer (e.g., `50`).                                                                                                    |
 | `config: ci_feedback.max_log_lines: must be non-negative`                       | Negative value for `max_log_lines`.                                      | Use `0` (disable log fetching) or a positive integer.                                                                                |
 | `config: ci_feedback.escalation: must be "label" or "comment", got "<val>"`     | Invalid escalation strategy.                                             | Use `"label"` or `"comment"`.                                                                                                        |
+| `config: reactions.ci_failure.watch_window_ms: must not exceed 9223372036854 (about 292 years); use 0 for no time limit, got <val>` | `watch_window_ms` exceeds the ceiling. | Lower the value, or use `0` for no time limit. |
+| `config: reactions.ci_failure.watch_window_ms: must be non-negative, got <val>` | Negative value for `watch_window_ms`.                                    | Use `0` (no time limit) or a positive integer.                                                                                       |
 
 ### 9.3 Environment Variable Errors
 
@@ -3636,6 +3639,7 @@ lists the `SORTIE_*` variable that overrides the field, or "—" if not overrida
 | `reactions.<kind>.escalation`           | string           | `label`                      | —                                        | `"label"` or `"comment"`; restart required except for `ci_failure`                     |
 | `reactions.<kind>.escalation_label`     | string           | `needs-human`                | —                                        | Applied when `escalation` is `"label"`; restart required except for `ci_failure`       |
 | `reactions.ci_failure.max_log_lines`    | integer          | `50`                         | —                                        | CI log tail lines; `0` disables; non-negative; restart required                        |
+| `reactions.<kind>.watch_window_ms`      | integer          | `86400000` for `ci_failure`, `1800000` for the other four | — | Bounds a pending entry's age; non-negative; not above `9223372036854`; `0` removes the bound; restart required except for `ci_failure` |
 | `reactions.review_comments.poll_interval_ms` | integer     | `120000`                     | —                                        | Review poll interval; min `30000`; restart required                                    |
 | `reactions.review_comments.debounce_ms` | integer          | `60000`                      | —                                        | Debounce after last comment; non-negative; restart required                            |
 | `reactions.review_comments.max_continuation_turns` | integer | `3`                        | —                                        | Review-fix turns before escalation; positive; restart required                         |

internal/config/config.go

@@ -958,10 +958,10 @@ func populateCIFeedbackFromReactions(rc ReactionConfig) (CIFeedbackConfig, error
 		}
 		watchWindowMS = parsed
 	}
-	if watchWindowMS < 0 {
+	if err := ValidateWatchWindowMS(watchWindowMS); err != nil {
 		return CIFeedbackConfig{}, &ConfigError{
 			Field:   "reactions.ci_failure.watch_window_ms",
-			Message: "must be non-negative",
+			Message: err.Error(),
 		}
 	}
 
@@ -1442,7 +1442,8 @@ type CIFeedbackConfig struct {
 	// recorded head rather than from the entry's creation, so an
 	// actively worked pull request stays watched and only silence ages
 	// it out. Default 86400000 (twenty-four hours). Zero removes the
-	// clock bound. Must be non-negative.
+	// clock bound. Must be non-negative and must not exceed
+	// [MaxWatchWindowMS].
 	WatchWindowMS int
 }
 

internal/config/config_test.go

@@ -2128,11 +2128,12 @@ func TestPopulateCIFeedbackFromReactions(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		name      string
-		rc        ReactionConfig
-		want      CIFeedbackConfig
-		wantErr   bool
-		wantField string
+		name          string
+		rc            ReactionConfig
+		want          CIFeedbackConfig
+		wantErr       bool
+		wantField     string
+		wantMsgSubstr string
 	}{
 		{
 			name: "ProviderMapsToKind",
@@ -2252,6 +2253,55 @@ func TestPopulateCIFeedbackFromReactions(t *testing.T) {
 			wantErr:   true,
 			wantField: "reactions.ci_failure.watch_window_ms",
 		},
+		{
+			name: "WatchWindowMSCeilingAccepted",
+			rc: ReactionConfig{
+				Provider: "github-actions",
+				Extra:    map[string]any{"watch_window_ms": 9223372036854},
+			},
+			want: CIFeedbackConfig{
+				Kind:          "github-actions",
+				MaxLogLines:   50,
+				WatchWindowMS: 9223372036854,
+			},
+		},
+		{
+			name: "WatchWindowMSAboveCeiling",
+			rc: ReactionConfig{
+				Provider: "github-actions",
+				Extra:    map[string]any{"watch_window_ms": 9223372036855},
+			},
+			wantErr:       true,
+			wantField:     "reactions.ci_failure.watch_window_ms",
+			wantMsgSubstr: "must not exceed",
+		},
+		{
+			name: "WatchWindowMSWrapFixtureOne",
+			rc: ReactionConfig{
+				Provider: "github-actions",
+				Extra:    map[string]any{"watch_window_ms": 18446744073710},
+			},
+			wantErr:   true,
+			wantField: "reactions.ci_failure.watch_window_ms",
+		},
+		{
+			name: "WatchWindowMSWrapFixtureTwo",
+			rc: ReactionConfig{
+				Provider: "github-actions",
+				Extra:    map[string]any{"watch_window_ms": 18446744073709},
+			},
+			wantErr:   true,
+			wantField: "reactions.ci_failure.watch_window_ms",
+		},
+		{
+			name: "WatchWindowMSWrapFixtureThree",
+			rc: ReactionConfig{
+				Provider: "github-actions",
+				Extra:    map[string]any{"watch_window_ms": 99999999999999},
+			},
+			wantErr:   true,
+			wantField: "reactions.ci_failure.watch_window_ms",
+		},
 		{
 			name: "EscalationAndLabelPassThrough",
 			rc: ReactionConfig{
@@ -2279,6 +2329,12 @@ func TestPopulateCIFeedbackFromReactions(t *testing.T) {
 
 			if tt.wantErr {
 				assertConfigErrorField(t, err, tt.wantField)
+				if tt.wantMsgSubstr != "" {
+					var ce *ConfigError
+					if errors.As(err, &ce) && !strings.Contains(ce.Message, tt.wantMsgSubstr) {
+						t.Errorf("ConfigError.Message = %q, want substring %q", ce.Message, tt.wantMsgSubstr)
+					}
+				}
 				return
 			}
 			if err != nil {

internal/config/watchwindow.go

@@ -0,0 +1,29 @@
+package config
+
+import (
+	"fmt"
+	"math"
+	"time"
+)
+
+// MaxWatchWindowMS is the largest watch_window_ms value whose conversion to
+// a time.Duration stays positive.
+const MaxWatchWindowMS int64 = math.MaxInt64 / int64(time.Millisecond)
+
+// ValidateWatchWindowMS reports why ms is not a usable watch window in
+// milliseconds, and nil when it is.
+//
+// ms is an already-coerced millisecond count; ValidateWatchWindowMS performs
+// no type coercion of its own. The returned error carries no field name or
+// key name, so a caller composes it with whatever identifier its own
+// diagnostic uses. ValidateWatchWindowMS is a pure function of its argument
+// and is safe for concurrent use.
+func ValidateWatchWindowMS(ms int) error {
+	if ms < 0 {
+		return fmt.Errorf("must be non-negative, got %d", ms)
+	}
+	if int64(ms) > MaxWatchWindowMS {
+		return fmt.Errorf("must not exceed %d (about 292 years); use 0 for no time limit, got %d", MaxWatchWindowMS, ms)
+	}
+	return nil
+}