From 3575efbaf1afbee8c2127c0c07934d756e802a56 Mon Sep 17 00:00:00 2001 From: Changsu Seong <110822847+scs0209@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:41:54 +0900 Subject: [PATCH 1/6] fix(scm): distinguish gh auth timeout from authentication failure Wrap Available() failures so context deadlines and a missing gh binary are not reported as "gh CLI is not authenticated". Co-authored-by: Cursor --- internal/scm/github/github.go | 20 ++++++++- internal/scm/github/github_test.go | 67 ++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index baee32394..18e403fef 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -2,6 +2,7 @@ package github import ( + "bytes" "context" "encoding/json" "errors" @@ -151,8 +152,23 @@ func (h *Host) Available(ctx context.Context) error { if h.host != "" { authArgs = append(authArgs, "--hostname", h.host) } - if err := h.cmd(ctx, "gh", authArgs...).Run(); err != nil { - return errors.New("gh CLI is not authenticated") + cmd := h.cmd(ctx, "gh", authArgs...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + // Keep timeout / missing-binary failures distinct from auth failure so a + // cancelled reconcile context is not reported as "log in again". + if ctx.Err() != nil { + return fmt.Errorf("gh auth status timed out: %w", ctx.Err()) + } + if errors.Is(err, exec.ErrNotFound) { + return fmt.Errorf("gh CLI is not on PATH: %w", err) + } + detail := strings.TrimSpace(stderr.String()) + if detail != "" { + return fmt.Errorf("gh CLI is not authenticated: %s: %w", detail, err) + } + return fmt.Errorf("gh CLI is not authenticated: %w", err) } return nil } diff --git a/internal/scm/github/github_test.go b/internal/scm/github/github_test.go index bf24e5976..13945c8dc 100644 --- a/internal/scm/github/github_test.go +++ b/internal/scm/github/github_test.go @@ -3,6 +3,7 @@ package github import ( "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -1513,6 +1514,72 @@ func TestAvailableFallsBackToUnscopedAuthWhenHostUnknown(t *testing.T) { } } +func TestAvailableReportsTimeoutInsteadOfAuthFailure(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh auth status": {}, + }), func() bool { return true }, "", "") + + err := host.Available(ctx) + if err == nil { + t.Fatal("Available() error = nil, want timeout error") + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("Available() error = %v, want context.Canceled", err) + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("Available() error = %v, want timed out message", err) + } + if strings.Contains(err.Error(), "not authenticated") { + t.Fatalf("Available() error = %v, must not report auth failure on timeout", err) + } +} + +func TestAvailableReportsMissingBinaryInsteadOfAuthFailure(t *testing.T) { + t.Parallel() + + host := New(func(ctx context.Context, name string, args ...string) *exec.Cmd { + return exec.CommandContext(ctx, "no-mistakes-missing-gh-binary") + }, func() bool { return true }, "", "") + + err := host.Available(context.Background()) + if err == nil { + t.Fatal("Available() error = nil, want missing-binary error") + } + if !errors.Is(err, exec.ErrNotFound) { + t.Fatalf("Available() error = %v, want exec.ErrNotFound", err) + } + if !strings.Contains(err.Error(), "not on PATH") { + t.Fatalf("Available() error = %v, want not on PATH message", err) + } + if strings.Contains(err.Error(), "not authenticated") { + t.Fatalf("Available() error = %v, must not report auth failure when gh is missing", err) + } +} + +func TestAvailableWrapsAuthFailureWithStderr(t *testing.T) { + t.Parallel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh auth status": {stderr: "github.com\n X Failed to log in\n", code: 1}, + }), func() bool { return true }, "", "") + + err := host.Available(context.Background()) + if err == nil { + t.Fatal("Available() error = nil, want auth failure") + } + if !strings.Contains(err.Error(), "not authenticated") { + t.Fatalf("Available() error = %v, want not authenticated", err) + } + if !strings.Contains(err.Error(), "Failed to log in") { + t.Fatalf("Available() error = %v, want stderr detail", err) + } +} + type githubTestResponse struct { stdout string stderr string From f5162b1dd7a588565cee769b30f2d730759a2ee5 Mon Sep 17 00:00:00 2001 From: scs0209 Date: Thu, 27 Aug 2026 19:48:10 +0900 Subject: [PATCH 2/6] no-mistakes: apply CI fixes --- internal/scm/github/github.go | 5 ++++- internal/scm/github/github_test.go | 36 ++++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index 18e403fef..88dc4f014 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -158,9 +158,12 @@ func (h *Host) Available(ctx context.Context) error { if err := cmd.Run(); err != nil { // Keep timeout / missing-binary failures distinct from auth failure so a // cancelled reconcile context is not reported as "log in again". - if ctx.Err() != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { return fmt.Errorf("gh auth status timed out: %w", ctx.Err()) } + if ctx.Err() != nil { + return fmt.Errorf("gh auth status interrupted: %w", ctx.Err()) + } if errors.Is(err, exec.ErrNotFound) { return fmt.Errorf("gh CLI is not on PATH: %w", err) } diff --git a/internal/scm/github/github_test.go b/internal/scm/github/github_test.go index 13945c8dc..e6648ed54 100644 --- a/internal/scm/github/github_test.go +++ b/internal/scm/github/github_test.go @@ -1514,7 +1514,32 @@ func TestAvailableFallsBackToUnscopedAuthWhenHostUnknown(t *testing.T) { } } -func TestAvailableReportsTimeoutInsteadOfAuthFailure(t *testing.T) { +func TestAvailableReportsDeadlineExceededInsteadOfAuthFailure(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + + host := New(githubTestCmdFactory(map[string]githubTestResponse{ + "gh auth status": {}, + }), func() bool { return true }, "", "") + + err := host.Available(ctx) + if err == nil { + t.Fatal("Available() error = nil, want timeout error") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Available() error = %v, want context.DeadlineExceeded", err) + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("Available() error = %v, want timed out message", err) + } + if strings.Contains(err.Error(), "not authenticated") { + t.Fatalf("Available() error = %v, must not report auth failure on timeout", err) + } +} + +func TestAvailableReportsCancellationInsteadOfAuthFailure(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) @@ -1531,11 +1556,14 @@ func TestAvailableReportsTimeoutInsteadOfAuthFailure(t *testing.T) { if !errors.Is(err, context.Canceled) { t.Fatalf("Available() error = %v, want context.Canceled", err) } - if !strings.Contains(err.Error(), "timed out") { - t.Fatalf("Available() error = %v, want timed out message", err) + if !strings.Contains(err.Error(), "interrupted") { + t.Fatalf("Available() error = %v, want interrupted message", err) + } + if strings.Contains(err.Error(), "timed out") { + t.Fatalf("Available() error = %v, must not report timeout on cancellation", err) } if strings.Contains(err.Error(), "not authenticated") { - t.Fatalf("Available() error = %v, must not report auth failure on timeout", err) + t.Fatalf("Available() error = %v, must not report auth failure on cancellation", err) } } From 243d3670dffbbe9e49f376e9b1962ec32d5d8e8f Mon Sep 17 00:00:00 2001 From: scs0209 Date: Thu, 27 Aug 2026 19:55:10 +0900 Subject: [PATCH 3/6] no-mistakes: apply CI fixes --- internal/scm/github/github.go | 14 +++++++++++++- internal/scm/github/github_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/internal/scm/github/github.go b/internal/scm/github/github.go index 88dc4f014..7eca3948f 100644 --- a/internal/scm/github/github.go +++ b/internal/scm/github/github.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "net/url" "os/exec" "strconv" @@ -164,7 +165,7 @@ func (h *Host) Available(ctx context.Context) error { if ctx.Err() != nil { return fmt.Errorf("gh auth status interrupted: %w", ctx.Err()) } - if errors.Is(err, exec.ErrNotFound) { + if isMissingExecutable(err) { return fmt.Errorf("gh CLI is not on PATH: %w", err) } detail := strings.TrimSpace(stderr.String()) @@ -176,6 +177,17 @@ func (h *Host) Available(ctx context.Context) error { return nil } +func isMissingExecutable(err error) bool { + if errors.Is(err, exec.ErrNotFound) || errors.Is(err, fs.ErrNotExist) { + return true + } + var execErr *exec.Error + if errors.As(err, &execErr) { + return errors.Is(execErr.Err, exec.ErrNotFound) || errors.Is(execErr.Err, fs.ErrNotExist) + } + return false +} + func parsePullRequestURL(raw, expectedHost, expectedRepo string) (int, error) { trimmed := strings.TrimSpace(raw) parsed, err := url.Parse(trimmed) diff --git a/internal/scm/github/github_test.go b/internal/scm/github/github_test.go index e6648ed54..cb1bb322c 100644 --- a/internal/scm/github/github_test.go +++ b/internal/scm/github/github_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "os/exec" "strings" @@ -1589,6 +1590,31 @@ func TestAvailableReportsMissingBinaryInsteadOfAuthFailure(t *testing.T) { } } +func TestAvailableReportsCommandFactoryMissingBinaryInsteadOfAuthFailure(t *testing.T) { + t.Parallel() + + host := New(func(ctx context.Context, name string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestGitHubHelperProcess", "--") + cmd.Env = append(os.Environ(), "GITHUB_TEST_HELPER=1") + cmd.Err = &exec.Error{Name: name, Err: fs.ErrNotExist} + return cmd + }, func() bool { return true }, "", "") + + err := host.Available(context.Background()) + if err == nil { + t.Fatal("Available() error = nil, want missing-binary error") + } + if !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("Available() error = %v, want fs.ErrNotExist", err) + } + if !strings.Contains(err.Error(), "not on PATH") { + t.Fatalf("Available() error = %v, want not on PATH message", err) + } + if strings.Contains(err.Error(), "not authenticated") { + t.Fatalf("Available() error = %v, must not report auth failure when command factory marks gh missing", err) + } +} + func TestAvailableWrapsAuthFailureWithStderr(t *testing.T) { t.Parallel() From bdd893042fc01a1551b0f55b26260b5209c6a9bd Mon Sep 17 00:00:00 2001 From: scs0209 Date: Thu, 27 Aug 2026 20:02:38 +0900 Subject: [PATCH 4/6] feat(config): make gate reconcile timings configurable Expose gate_reconcile_interval and gate_reconcile_timeout as global-only settings (defaults 2m / 30s) so operators can raise the parked-approval reconcile budget for slow host probes without changing the executor. Co-authored-by: Cursor --- docs/src/content/docs/guides/configuration.md | 2 +- .../content/docs/reference/global-config.md | 26 ++++++++ internal/config/config.go | 58 ++++++++++++++--- internal/config/config_global_test.go | 63 +++++++++++++++++++ internal/config/config_repo_test.go | 22 +++++++ internal/pipeline/executor.go | 12 +++- 6 files changed, 171 insertions(+), 12 deletions(-) diff --git a/docs/src/content/docs/guides/configuration.md b/docs/src/content/docs/guides/configuration.md index 0545b345f..eea31e119 100644 --- a/docs/src/content/docs/guides/configuration.md +++ b/docs/src/content/docs/guides/configuration.md @@ -55,7 +55,7 @@ The rest of this page covers only the cross-cutting rules that involve both file ## Precedence - Repo config overrides global config field by field: repo `agent` replaces the global `agent` (including a full ordered fallback list), while `auto_fix`, `ci`, `commit`, `intent`, and the repository-scoped `test.evidence` fields overlay individual fields and fall through to the global default for anything unset (`intent.disabled_readers` adds to the globally disabled readers instead of replacing them). Local evidence location and retention are machine-wide and remain global-only; the [Global Config Reference](/no-mistakes/reference/global-config/#testevidence) owns the exact boundary. -- `agent_path_override`, `agent_config`, `agent_args_override`, `acpx_path`, `acp_registry_overrides`, `ci_timeout`, `daemon_connect_timeout`, `branch_sync_remote_timeout`, `step_quiet_warning`, `agent_timeout`, `review_agent_timeout`, `test_agent_timeout`, `log_level`, and `session_reuse` are global-only fields. +- `agent_path_override`, `agent_config`, `agent_args_override`, `acpx_path`, `acp_registry_overrides`, `ci_timeout`, `daemon_connect_timeout`, `branch_sync_remote_timeout`, `gate_reconcile_interval`, `gate_reconcile_timeout`, `step_quiet_warning`, `agent_timeout`, `review_agent_timeout`, `test_agent_timeout`, `log_level`, and `session_reuse` are global-only fields. - `commands`, `ignore_patterns`, `document.instructions`, `review.path_instructions`, `allow_repo_commands`, and `disable_project_settings` are repo-only fields. By default, `commands` and `agent` are read from the trusted default branch; a trusted `allow_repo_commands: true` opt-in instead honors their pushed-branch values. The other gate-control fields, including `review.path_instructions` and the repo `ci` overlay, always come from the trusted default branch. See the [Repo Config Reference](/no-mistakes/reference/repo-config/) security note. - no-mistakes reloads global config while setting up each run, so edits made before starting a run apply to it. For repeatable profiles (for example fast versus deep Codex settings), use separately initialized `NM_HOME` roots; `NM_HOME` moves all no-mistakes state, not just config. diff --git a/docs/src/content/docs/reference/global-config.md b/docs/src/content/docs/reference/global-config.md index 0e687c9b3..e1934dbbc 100644 --- a/docs/src/content/docs/reference/global-config.md +++ b/docs/src/content/docs/reference/global-config.md @@ -50,6 +50,10 @@ daemon_connect_timeout: "3s" branch_sync_remote_timeout: "60s" +gate_reconcile_interval: "2m" + +gate_reconcile_timeout: "30s" + log_level: info session_reuse: true @@ -461,6 +465,28 @@ Accepts any positive Go `time.ParseDuration` string. Raise this if your environment's Git credential helper (for example `gh auth git-credential`, invoked by Git as a child process against a private remote) legitimately takes longer than the default - this is a real, non-outage latency characteristic that has been observed taking 19-22s in some environments, not a hang. It is a machine/environment setting, not a per-repository one: it is read only from global config and has no matching field in a repository's `.no-mistakes.yaml`, so a pushed branch cannot widen or narrow how long the local service waits before failing closed. It never changes the fail-closed guarantee itself - a timeout or unknown remote state still always refuses synchronization without changing files or refs, whatever this value is set to. +### gate_reconcile_interval + +How often the daemon retries a parked approval-gate reconcile (for example after an agent is not authenticated) before the next attempt. + +| | | +| ------- | ---------------------- | +| Type | `string` (Go duration) | +| Default | `2m` | + +Accepts any positive Go `time.ParseDuration` string. Global-only: there is no matching field in a repository's `.no-mistakes.yaml`. + +### gate_reconcile_timeout + +Maximum wall time each parked approval-gate reconcile attempt may spend before the attempt fails and the next interval wait begins. Covers host probes such as `gh auth status` that can hang without returning. + +| | | +| ------- | ---------------------- | +| Type | `string` (Go duration) | +| Default | `30s` | + +Accepts any positive Go `time.ParseDuration` string. Global-only: there is no matching field in a repository's `.no-mistakes.yaml`. Raise this if a legitimate credential helper or network path routinely needs longer than the default for auth probes; leave it alone if you only want clearer timeout errors (those still surface as timed-out gate failures regardless of this value). + ### log_level Daemon log verbosity. diff --git a/internal/config/config.go b/internal/config/config.go index cdce06ab2..4800c2784 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -58,6 +58,13 @@ const ( DefaultDaemonConnectTimeout = 3 * time.Second // DefaultBranchSyncRemoteTimeout bounds each remote Git operation (ls-remote, fetch) in internal/branchsync. Global-config-only; a pushed branch cannot change it. Timeout still fails closed. DefaultBranchSyncRemoteTimeout = 60 * time.Second + // DefaultGateReconcileInterval is how often a parked approval gate is + // rechecked. Global-config-only; a pushed branch cannot change it. + DefaultGateReconcileInterval = 2 * time.Minute + // DefaultGateReconcileTimeout is the deadline for one approval-gate + // reconciliation check (including host.Available / gh auth status). + // Global-config-only; a pushed branch cannot change it. + DefaultGateReconcileTimeout = 30 * time.Second // CITimeoutUnlimited is the sentinel meaning "monitor until the PR is // merged, closed, or the run is aborted - never self-terminate". // Any non-positive ci_timeout, or the keywords "unlimited", "none", @@ -133,7 +140,13 @@ type GlobalConfig struct { TestAgentTimeout time.Duration `yaml:"-"` DaemonConnectTimeout time.Duration `yaml:"-"` BranchSyncRemoteTimeout time.Duration `yaml:"-"` - LogLevel string `yaml:"log_level"` + // GateReconcileInterval / GateReconcileTimeout bound how often and how + // long a parked approval gate is rechecked. They are machine-local + // operator knobs (slow hosts, contended gh auth) and global-only so a + // pushed branch cannot widen or shrink the reconcile budget. + GateReconcileInterval time.Duration `yaml:"-"` + GateReconcileTimeout time.Duration `yaml:"-"` + LogLevel string `yaml:"log_level"` // SessionReuse controls per-run agent session reuse in the review loop: // one durable fixer session across review-fix turns. Review turns always // run session-free so the rereview never resumes the session whose @@ -170,6 +183,8 @@ type globalConfigRaw struct { CITimeout string `yaml:"ci_timeout"` DaemonConnectTimeout string `yaml:"daemon_connect_timeout"` BranchSyncRemoteTimeout string `yaml:"branch_sync_remote_timeout"` + GateReconcileInterval string `yaml:"gate_reconcile_interval"` + GateReconcileTimeout string `yaml:"gate_reconcile_timeout"` BabysitTimeout string `yaml:"babysit_timeout"` StepQuietWarning string `yaml:"step_quiet_warning"` AgentTimeout string `yaml:"agent_timeout"` @@ -496,6 +511,8 @@ type Config struct { AgentTimeout time.Duration ReviewAgentTimeout time.Duration TestAgentTimeout time.Duration + GateReconcileInterval time.Duration + GateReconcileTimeout time.Duration LogLevel string SessionReuse bool Eval Eval @@ -779,6 +796,13 @@ daemon_connect_timeout: "3s" # (ls-remote or fetch) before treating the target as offline. Global-only. branch_sync_remote_timeout: "60s" +# How often a parked approval gate is rechecked, and the deadline for each +# check (including gh auth status). Raise gate_reconcile_timeout on a slow or +# contended machine so a transient auth-status delay is not cancelled mid-call. +# Global-only. +gate_reconcile_interval: "2m" +gate_reconcile_timeout: "30s" + # Reuse one durable fixer session per run across review-fix turns. Review turns # always run session-free so a rereview never resumes the session that prescribed # its fixes. Supported for claude, codex, grok, and pi; other agents run cold. @@ -1566,6 +1590,8 @@ func DefaultGlobalConfig() *GlobalConfig { TestAgentTimeout: DefaultTestAgentTimeout, DaemonConnectTimeout: DefaultDaemonConnectTimeout, BranchSyncRemoteTimeout: DefaultBranchSyncRemoteTimeout, + GateReconcileInterval: DefaultGateReconcileInterval, + GateReconcileTimeout: DefaultGateReconcileTimeout, LogLevel: "info", SessionReuse: true, Eval: evalDefaults(), @@ -1829,6 +1855,20 @@ func LoadGlobalFromBytes(data []byte) (*GlobalConfig, error) { } cfg.BranchSyncRemoteTimeout = d } + if raw.GateReconcileInterval != "" { + d, err := parsePositiveDuration("gate_reconcile_interval", raw.GateReconcileInterval) + if err != nil { + return nil, err + } + cfg.GateReconcileInterval = d + } + if raw.GateReconcileTimeout != "" { + d, err := parsePositiveDuration("gate_reconcile_timeout", raw.GateReconcileTimeout) + if err != nil { + return nil, err + } + cfg.GateReconcileTimeout = d + } if raw.LogLevel != "" { cfg.LogLevel = raw.LogLevel } @@ -2489,13 +2529,15 @@ func Merge(global *GlobalConfig, repo *RepoConfig) *Config { AgentPathOverride: global.AgentPathOverride, AgentArgsOverride: global.AgentArgsOverride, AgentConfig: global.AgentConfig, - CITimeout: global.CITimeout, - StepQuietWarning: global.StepQuietWarning, - AgentTimeout: global.AgentTimeout, - ReviewAgentTimeout: global.ReviewAgentTimeout, - TestAgentTimeout: global.TestAgentTimeout, - LogLevel: global.LogLevel, - SessionReuse: global.SessionReuse, + CITimeout: global.CITimeout, + StepQuietWarning: global.StepQuietWarning, + AgentTimeout: global.AgentTimeout, + ReviewAgentTimeout: global.ReviewAgentTimeout, + TestAgentTimeout: global.TestAgentTimeout, + GateReconcileInterval: global.GateReconcileInterval, + GateReconcileTimeout: global.GateReconcileTimeout, + LogLevel: global.LogLevel, + SessionReuse: global.SessionReuse, // Eval is global-only by design (see GlobalConfig.Eval), so it is // copied straight through with no repository override step. Eval: global.Eval, diff --git a/internal/config/config_global_test.go b/internal/config/config_global_test.go index 395ce0baa..ebdd05d91 100644 --- a/internal/config/config_global_test.go +++ b/internal/config/config_global_test.go @@ -41,6 +41,12 @@ func TestLoadGlobal_Defaults(t *testing.T) { if cfg.BranchSyncRemoteTimeout != DefaultBranchSyncRemoteTimeout { t.Errorf("branch_sync_remote_timeout = %v, want %v", cfg.BranchSyncRemoteTimeout, DefaultBranchSyncRemoteTimeout) } + if cfg.GateReconcileInterval != DefaultGateReconcileInterval { + t.Errorf("gate_reconcile_interval = %v, want %v", cfg.GateReconcileInterval, DefaultGateReconcileInterval) + } + if cfg.GateReconcileTimeout != DefaultGateReconcileTimeout { + t.Errorf("gate_reconcile_timeout = %v, want %v", cfg.GateReconcileTimeout, DefaultGateReconcileTimeout) + } if cfg.LogLevel != "info" { t.Errorf("log_level = %q, want %q", cfg.LogLevel, "info") } @@ -117,6 +123,12 @@ func TestEnsureDefaultGlobalConfig_CreatedConfigIsLoadable(t *testing.T) { if cfg.BranchSyncRemoteTimeout != DefaultBranchSyncRemoteTimeout { t.Errorf("branch_sync_remote_timeout = %v, want %v", cfg.BranchSyncRemoteTimeout, DefaultBranchSyncRemoteTimeout) } + if cfg.GateReconcileInterval != DefaultGateReconcileInterval { + t.Errorf("gate_reconcile_interval = %v, want %v", cfg.GateReconcileInterval, DefaultGateReconcileInterval) + } + if cfg.GateReconcileTimeout != DefaultGateReconcileTimeout { + t.Errorf("gate_reconcile_timeout = %v, want %v", cfg.GateReconcileTimeout, DefaultGateReconcileTimeout) + } if cfg.LogLevel != "info" { t.Errorf("log_level = %q, want %q", cfg.LogLevel, "info") } @@ -209,6 +221,43 @@ func TestLoadGlobal_TestAgentTimeout(t *testing.T) { } } +func TestLoadGlobal_GateReconcileTimings(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte("gate_reconcile_interval: 45s\ngate_reconcile_timeout: 90s\n"), 0o644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadGlobal(path) + if err != nil { + t.Fatalf("LoadGlobal: %v", err) + } + if cfg.GateReconcileInterval != 45*time.Second { + t.Fatalf("gate_reconcile_interval = %v, want 45s", cfg.GateReconcileInterval) + } + if cfg.GateReconcileTimeout != 90*time.Second { + t.Fatalf("gate_reconcile_timeout = %v, want 90s", cfg.GateReconcileTimeout) + } +} + +func TestLoadGlobal_InvalidGateReconcileTimings(t *testing.T) { + dir := t.TempDir() + for _, body := range []string{ + `gate_reconcile_timeout: "not-a-duration"`, + `gate_reconcile_timeout: "0s"`, + `gate_reconcile_timeout: "-1s"`, + `gate_reconcile_interval: "0s"`, + } { + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(body+"\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := LoadGlobal(path); err == nil { + t.Fatalf("LoadGlobal(%q) error = nil, want error", body) + } + } +} + func TestLoadGlobal_InvalidAgentTimeout(t *testing.T) { cases := []string{ `agent_timeout: "not-a-duration"`, @@ -604,6 +653,20 @@ func TestDefaultConfigYAML_MatchesGoDefaults(t *testing.T) { if d != DefaultBranchSyncRemoteTimeout { t.Errorf("YAML branch_sync_remote_timeout = %v, Go default = %v", d, DefaultBranchSyncRemoteTimeout) } + d, err = time.ParseDuration(raw.GateReconcileInterval) + if err != nil { + t.Fatalf("YAML gate_reconcile_interval %q is not a valid duration: %v", raw.GateReconcileInterval, err) + } + if d != DefaultGateReconcileInterval { + t.Errorf("YAML gate_reconcile_interval = %v, Go default = %v", d, DefaultGateReconcileInterval) + } + d, err = time.ParseDuration(raw.GateReconcileTimeout) + if err != nil { + t.Fatalf("YAML gate_reconcile_timeout %q is not a valid duration: %v", raw.GateReconcileTimeout, err) + } + if d != DefaultGateReconcileTimeout { + t.Errorf("YAML gate_reconcile_timeout = %v, Go default = %v", d, DefaultGateReconcileTimeout) + } d, err = time.ParseDuration(raw.ReviewAgentTimeout) if err != nil { t.Fatalf("YAML review_agent_timeout %q is not a valid duration: %v", raw.ReviewAgentTimeout, err) diff --git a/internal/config/config_repo_test.go b/internal/config/config_repo_test.go index ebd62144e..c367245c5 100644 --- a/internal/config/config_repo_test.go +++ b/internal/config/config_repo_test.go @@ -142,6 +142,28 @@ func TestLoadRepo_TestAgentTimeoutIsNotARepoSetting(t *testing.T) { } } +// TestLoadRepo_GateReconcileTimingsAreNotRepoSettings proves +// gate_reconcile_timeout / gate_reconcile_interval are inert in +// .no-mistakes.yaml: RepoConfig has no matching fields, so a pushed branch +// cannot widen the approval-gate reconcile budget. They are global-only +// operator machine settings. +func TestLoadRepo_GateReconcileTimingsAreNotRepoSettings(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".no-mistakes.yaml") + data := "gate_reconcile_timeout: \"999s\"\ngate_reconcile_interval: \"999s\"\n" + if err := os.WriteFile(path, []byte(data), 0o644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadRepo(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Agent != "" || cfg.Commands.Test != "" || cfg.Commands.Lint != "" || cfg.Commands.Format != "" { + t.Fatalf("unrelated repo config fields changed: %#v", cfg) + } +} + func TestLoadRepo_AgentAcceptsList(t *testing.T) { dir := t.TempDir() data := `agent: [codex, claude] diff --git a/internal/pipeline/executor.go b/internal/pipeline/executor.go index f1bad9c79..43defef20 100644 --- a/internal/pipeline/executor.go +++ b/internal/pipeline/executor.go @@ -31,8 +31,8 @@ import ( type EventFunc func(ipc.Event) const ( - defaultGateReconcileInterval = 2 * time.Minute - defaultGateReconcileTimeout = 30 * time.Second + defaultGateReconcileInterval = config.DefaultGateReconcileInterval + defaultGateReconcileTimeout = config.DefaultGateReconcileTimeout ) type approvalResponse struct { @@ -102,7 +102,7 @@ func NewExecutor(database *db.DB, p *paths.Paths, cfg *config.Config, ag agent.A if onEvent == nil { onEvent = func(ipc.Event) {} } - return &Executor{ + exec := &Executor{ db: database, paths: p, config: cfg, @@ -113,6 +113,12 @@ func NewExecutor(database *db.DB, p *paths.Paths, cfg *config.Config, ag agent.A gateReconcileInterval: defaultGateReconcileInterval, gateReconcileTimeout: defaultGateReconcileTimeout, } + if cfg != nil { + // Global config is the production path for these timings; SetGate* + // remains for tests and specialized embeddings. + exec.SetGateReconcileTimings(cfg.GateReconcileInterval, cfg.GateReconcileTimeout) + } + return exec } // runEvidenceDir resolves where this run's test evidence is written. The From bccb9688dd933a9f730c1c83fb47bbe3fe421b70 Mon Sep 17 00:00:00 2001 From: scs0209 Date: Thu, 27 Aug 2026 20:20:26 +0900 Subject: [PATCH 5/6] no-mistakes(document): Correct gate reconcile and gh auth docs --- .../content/docs/guides/troubleshooting.md | 4 +- .../content/docs/reference/global-config.md | 6 +- .../content/docs/reference/pipeline-steps.md | 4 +- internal/config/config_global_test.go | 30 +++++ internal/pipeline/executor_reconcile_test.go | 105 ++++++++++++++++++ 5 files changed, 142 insertions(+), 7 deletions(-) diff --git a/docs/src/content/docs/guides/troubleshooting.md b/docs/src/content/docs/guides/troubleshooting.md index 16a55fefc..fd93ce164 100644 --- a/docs/src/content/docs/guides/troubleshooting.md +++ b/docs/src/content/docs/guides/troubleshooting.md @@ -227,8 +227,8 @@ Symptom: pipeline completes but the PR step shows `skipped`. Check the [Provider Integration](/no-mistakes/guides/provider-integration/) requirements. Most common causes: -- `gh`, `glab`, `forgejo-axi`, or `tea` not installed -- The provider CLI reports that it is not authenticated +- `gh`, `glab`, `forgejo-axi`, or `tea` not installed (or, for GitHub, not on `PATH`) +- The provider CLI reports that it is not authenticated; on GitHub, a timed-out or interrupted `gh auth status` is reported separately from auth failure - Bitbucket env vars not set in the daemon's environment - Upstream is not one of the hosts listed in Provider Integration - Self-hosted GitHub Enterprise on a hostname that is not `github.com` isn't detected because `gh` isn't configured for the host; run `gh auth login --hostname your-ghe.example.com` so detection finds it. Once detection succeeds, the availability check is host-scoped (`gh auth status --hostname your-ghe.example.com`), so a stale token on `github.com` or any other configured gh host can no longer falsely mark the GHE repo as unauthenticated. diff --git a/docs/src/content/docs/reference/global-config.md b/docs/src/content/docs/reference/global-config.md index e1934dbbc..8c8b9168c 100644 --- a/docs/src/content/docs/reference/global-config.md +++ b/docs/src/content/docs/reference/global-config.md @@ -467,7 +467,7 @@ Raise this if your environment's Git credential helper (for example `gh auth git ### gate_reconcile_interval -How often the daemon retries a parked approval-gate reconcile (for example after an agent is not authenticated) before the next attempt. +How often the daemon rechecks a parked approval gate while waiting for user approval. Today this applies to the CI step's parked gate, which re-probes provider availability (including `gh auth status`) and clears the gate when the PR was merged or closed. | | | | ------- | ---------------------- | @@ -478,14 +478,14 @@ Accepts any positive Go `time.ParseDuration` string. Global-only: there is no ma ### gate_reconcile_timeout -Maximum wall time each parked approval-gate reconcile attempt may spend before the attempt fails and the next interval wait begins. Covers host probes such as `gh auth status` that can hang without returning. +Maximum wall time one parked approval-gate reconcile attempt may spend before the attempt stops, the gate stays parked, and the next interval wait begins. Covers host probes such as `gh auth status` that can hang without returning. | | | | ------- | ---------------------- | | Type | `string` (Go duration) | | Default | `30s` | -Accepts any positive Go `time.ParseDuration` string. Global-only: there is no matching field in a repository's `.no-mistakes.yaml`. Raise this if a legitimate credential helper or network path routinely needs longer than the default for auth probes; leave it alone if you only want clearer timeout errors (those still surface as timed-out gate failures regardless of this value). +Accepts any positive Go `time.ParseDuration` string. Global-only: there is no matching field in a repository's `.no-mistakes.yaml`. Raise this if a legitimate credential helper or network path routinely needs longer than the default for auth probes during reconcile. Timeout and interruption are reported distinctly from authentication failure; that distinction does not require raising this value. ### log_level diff --git a/docs/src/content/docs/reference/pipeline-steps.md b/docs/src/content/docs/reference/pipeline-steps.md index e4864bdbc..fb8a42cf2 100644 --- a/docs/src/content/docs/reference/pipeline-steps.md +++ b/docs/src/content/docs/reference/pipeline-steps.md @@ -214,8 +214,8 @@ Creates or updates a pull request. **Skipped when:** - The branch is the [PR base branch](/no-mistakes/reference/repo-config/#prbase_branch) (the repository's forge default branch, or the trusted `pr.base_branch` when configured) - The upstream host is not GitHub, GitLab, Forgejo, Bitbucket Cloud (`bitbucket.org`), Azure DevOps (`dev.azure.com` / `*.visualstudio.com`), or Gitea -- The provider CLI (`gh`, `glab`, `forgejo-axi`, or `tea`) is not installed for GitHub, GitLab, Forgejo, or Gitea -- The provider CLI is not authenticated for GitHub, GitLab, Forgejo, or Gitea +- The provider CLI (`gh`, `glab`, `forgejo-axi`, or `tea`) is not installed for GitHub, GitLab, Forgejo, or Gitea (GitHub also skips when `gh` is missing from `PATH`) +- The provider CLI is not authenticated for GitHub, GitLab, Forgejo, or Gitea (GitHub reports a timed-out or interrupted `gh auth status` separately from auth failure; either still skips) - Bitbucket Cloud credentials are missing (`NO_MISTAKES_BITBUCKET_EMAIL` or `NO_MISTAKES_BITBUCKET_API_TOKEN`) - The `az` CLI with the `azure-devops` extension is not installed or not authenticated for Azure DevOps - A legacy or manually edited non-GitHub repo record has `fork_url` set, because fork MR/PR routing is currently GitHub-only diff --git a/internal/config/config_global_test.go b/internal/config/config_global_test.go index ebdd05d91..21c589b07 100644 --- a/internal/config/config_global_test.go +++ b/internal/config/config_global_test.go @@ -240,6 +240,36 @@ func TestLoadGlobal_GateReconcileTimings(t *testing.T) { } } +// TestLoadGlobal_GateReconcileTimings_OperatorSlowAuthBudget is the documented +// operator path: raise interval/timeout in global config.yaml so slow gh auth +// probes fit the parked-gate reconcile budget (defaults remain 2m / 30s). +func TestLoadGlobal_GateReconcileTimings_OperatorSlowAuthBudget(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + body := "gate_reconcile_interval: \"5m\"\ngate_reconcile_timeout: \"2m\"\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadGlobal(path) + if err != nil { + t.Fatalf("LoadGlobal: %v", err) + } + if cfg.GateReconcileInterval != 5*time.Minute { + t.Fatalf("gate_reconcile_interval = %v, want 5m", cfg.GateReconcileInterval) + } + if cfg.GateReconcileTimeout != 2*time.Minute { + t.Fatalf("gate_reconcile_timeout = %v, want 2m", cfg.GateReconcileTimeout) + } + merged := Merge(cfg, &RepoConfig{}) + if merged.GateReconcileInterval != 5*time.Minute || merged.GateReconcileTimeout != 2*time.Minute { + t.Fatalf("Merge did not preserve global timings: interval=%v timeout=%v", + merged.GateReconcileInterval, merged.GateReconcileTimeout) + } + t.Logf("operator config.yaml loaded: gate_reconcile_interval=%v gate_reconcile_timeout=%v", + merged.GateReconcileInterval, merged.GateReconcileTimeout) +} + func TestLoadGlobal_InvalidGateReconcileTimings(t *testing.T) { dir := t.TempDir() for _, body := range []string{ diff --git a/internal/pipeline/executor_reconcile_test.go b/internal/pipeline/executor_reconcile_test.go index ffb52bb5e..9edd63119 100644 --- a/internal/pipeline/executor_reconcile_test.go +++ b/internal/pipeline/executor_reconcile_test.go @@ -4,10 +4,13 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "sync/atomic" "testing" "time" + "github.com/kunchenguid/no-mistakes/internal/config" "github.com/kunchenguid/no-mistakes/internal/types" ) @@ -292,6 +295,108 @@ func TestExecutor_GateRecheckIsBoundedAndApprovalWinsAfterTimeout(t *testing.T) } } +// TestExecutor_AppliesGateReconcileTimingsFromGlobalConfig is the operator +// path for gate_reconcile_interval / gate_reconcile_timeout: write them in +// global config.yaml, load + merge, construct NewExecutor with that Config +// (no SetGateReconcileTimings), and prove a hanging reconcile is bounded by +// the configured timeout rather than the hardcoded 30s default. If wiring +// were missing, the blocking check would hold the gate for ~30s and this +// 1s deadline would fail. +func TestExecutor_AppliesGateReconcileTimingsFromGlobalConfig(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + // Operator raises the per-attempt budget for slow gh auth probes; the + // interval stays long so only the timeout bound is under test here. + body := "gate_reconcile_interval: \"1h\"\ngate_reconcile_timeout: \"25ms\"\n" + if err := os.WriteFile(cfgPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + global, err := config.LoadGlobal(cfgPath) + if err != nil { + t.Fatalf("LoadGlobal: %v", err) + } + if global.GateReconcileInterval != time.Hour || global.GateReconcileTimeout != 25*time.Millisecond { + t.Fatalf("loaded timings = interval %v timeout %v, want 1h / 25ms", + global.GateReconcileInterval, global.GateReconcileTimeout) + } + cfg := config.Merge(global, &config.RepoConfig{}) + if cfg.GateReconcileInterval != time.Hour || cfg.GateReconcileTimeout != 25*time.Millisecond { + t.Fatalf("merged timings = interval %v timeout %v, want 1h / 25ms", + cfg.GateReconcileInterval, cfg.GateReconcileTimeout) + } + + database, p, run, repo := setupTest(t) + step := &reconcilingApprovalStep{name: types.StepCI, block: true, started: make(chan struct{})} + exec := NewExecutor(database, p, cfg, nil, []Step{step}, nil) + + workDir := t.TempDir() + done := make(chan error, 1) + go func() { done <- exec.Execute(context.Background(), run, repo, workDir) }() + select { + case <-step.started: + case <-time.After(3 * time.Second): + t.Fatal("gate reconciliation did not start") + } + if err := exec.Respond(types.StepCI, types.ActionApprove, nil); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("Execute() error = %v", err) + } + case <-time.After(time.Second): + t.Fatal("configured gate_reconcile_timeout was not applied; blocking check still used the hardcoded default") + } +} + +// TestExecutor_AppliesGateReconcileIntervalFromGlobalConfig proves the +// operator-configured interval (not the hardcoded 2m) drives how often a +// still-parked gate is rechecked. With interval 10ms, a second reconcile must +// arrive well before the 2m default; Approve ends the park cleanly. +func TestExecutor_AppliesGateReconcileIntervalFromGlobalConfig(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + body := "gate_reconcile_interval: \"10ms\"\ngate_reconcile_timeout: \"50ms\"\n" + if err := os.WriteFile(cfgPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + global, err := config.LoadGlobal(cfgPath) + if err != nil { + t.Fatalf("LoadGlobal: %v", err) + } + cfg := config.Merge(global, &config.RepoConfig{}) + + database, p, run, repo := setupTest(t) + step := &reconcilingApprovalStep{name: types.StepCI} + exec := NewExecutor(database, p, cfg, nil, []Step{step}, nil) + + workDir := t.TempDir() + done := make(chan error, 1) + go func() { done <- exec.Execute(context.Background(), run, repo, workDir) }() + waitForStepStatus(t, database, run.ID, types.StepCI, types.StepStatusAwaitingApproval) + + deadline := time.Now().Add(500 * time.Millisecond) + for step.calls.Load() < 2 && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if step.calls.Load() < 2 { + t.Fatalf("reconcile calls = %d within 500ms, want >= 2 from configured 10ms interval (default 2m would not recheck yet)", step.calls.Load()) + } + + if err := exec.Respond(types.StepCI, types.ActionApprove, nil); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("Execute() error = %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("parked gate did not complete after approve") + } +} + func TestExecutor_GateRecheckStopsAfterApprovalCancelAndShutdown(t *testing.T) { tests := []struct { name string From 15d0e9d0386f1e6366a806034f3ce8615fb2734a Mon Sep 17 00:00:00 2001 From: scs0209 Date: Thu, 27 Aug 2026 20:20:29 +0900 Subject: [PATCH 6/6] no-mistakes: apply agent fixes --- internal/config/config.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 4800c2784..0cb0ec713 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2521,14 +2521,14 @@ func Merge(global *GlobalConfig, repo *RepoConfig) *Config { } cfg := &Config{ - Agent: global.Agent, - Agents: copyAgents(global.Agents), - ACPXPath: global.ACPXPath, - ForgejoAXIPath: global.ForgejoAXIPath, - ACPRegistryOverrides: global.ACPRegistryOverrides, - AgentPathOverride: global.AgentPathOverride, - AgentArgsOverride: global.AgentArgsOverride, - AgentConfig: global.AgentConfig, + Agent: global.Agent, + Agents: copyAgents(global.Agents), + ACPXPath: global.ACPXPath, + ForgejoAXIPath: global.ForgejoAXIPath, + ACPRegistryOverrides: global.ACPRegistryOverrides, + AgentPathOverride: global.AgentPathOverride, + AgentArgsOverride: global.AgentArgsOverride, + AgentConfig: global.AgentConfig, CITimeout: global.CITimeout, StepQuietWarning: global.StepQuietWarning, AgentTimeout: global.AgentTimeout,