diff --git a/cmd/gc/cmd_agent.go b/cmd/gc/cmd_agent.go index 4dbd78c5ce..7593583ecf 100644 --- a/cmd/gc/cmd_agent.go +++ b/cmd/gc/cmd_agent.go @@ -163,10 +163,25 @@ func isNonFatalLoadConfigWarning(warning string) bool { return strings.Contains(warning, `"agent_defaults.`) || strings.Contains(warning, `"agents.`) } +// shouldEmitLoadCityConfigWarning reports whether a config-load warning is +// worth printing on the stderr of an arbitrary command. Migration guidance that +// a user must act on qualifies; static lints about a property of city.toml do +// not. The shared loadCityConfigFS path runs on nearly every gc invocation, so +// anything returning true here is reprinted for the life of the config — and +// agent harnesses merge stderr into the tool result, which turned the +// always+fresh advisory alone into 7.3% of all tool-result text city-wide +// (gc-dqn8l). Suppression here is not concealment: gc start and gc config both +// print raw prov.Warnings without consulting this filter, and strict mode +// classifies independently via strictWarningIsNonFatal. func shouldEmitLoadCityConfigWarning(warning string) bool { if config.IsLegacyWorkspaceFieldWarning(warning) { return false } + // A lint about a static property of city.toml — identical on every + // invocation, and unchanged by anything the current command did. + if config.IsAlwaysFreshWakeModeWarning(warning) { + return false + } if strings.Contains(warning, "both [agent_defaults] and [agents] are present") { return true } diff --git a/cmd/gc/cmd_agent_test.go b/cmd/gc/cmd_agent_test.go index 66b3c4bb38..6b9b42a89c 100644 --- a/cmd/gc/cmd_agent_test.go +++ b/cmd/gc/cmd_agent_test.go @@ -426,6 +426,7 @@ func TestEmitLoadCityConfigWarningsFiltersNonMigrationWarnings(t *testing.T) { `/city/pack.toml: both [agent_defaults] and [agents] are present; [agent_defaults] wins on overlapping keys and [agents] only fills gaps`, `/city/city.toml: workspace.global_fragments is deprecated: Use [agent_defaults] append_fragments or explicit template includes instead.`, `gc: warning: attachment-list fields (` + "`skills`, `mcp`, `skills_append`, `mcp_append`, `shared_skills`" + `) are deprecated as of v0.15.1 and ignored.`, + `named_session "gastown.mayor": mode "always" with wake_mode "fresh" on template "gastown.mayor" starts a fresh provider session after every drain; use only for a deliberate restart-per-cycle actor`, }, }) @@ -445,6 +446,9 @@ func TestEmitLoadCityConfigWarningsFiltersNonMigrationWarnings(t *testing.T) { if !strings.Contains(output, "attachment-list fields") { t.Fatalf("expected attachment deprecation warning, got %q", output) } + if strings.Contains(output, "starts a fresh provider session after every drain") { + t.Fatalf("always+fresh advisory should stay out of generic command stderr, got %q", output) + } } func TestDoAgentSuspendRootPackAgent(t *testing.T) { diff --git a/cmd/gc/cmd_github.go b/cmd/gc/cmd_github.go index 9f1e896006..965d2a459f 100644 --- a/cmd/gc/cmd_github.go +++ b/cmd/gc/cmd_github.go @@ -161,11 +161,13 @@ func doGitHubPRBackfill(opts githubPRBackfillOptions, stdout, stderr io.Writer) fmt.Fprintf(stderr, "gc github pr backfill: %v\n", err) //nolint:errcheck // best-effort stderr return 1 } - if !opts.jsonOutput { - for _, warning := range prov.Warnings { - fmt.Fprintf(stderr, "gc github pr backfill: warning: %s\n", warning) //nolint:errcheck // best-effort stderr - } - } + // Route through the shared filter rather than printing prov.Warnings raw. + // This command's subject is GitHub PR readiness, not the config, so it gets + // the same treatment as every other non-config command: actionable + // migration guidance prints, static city.toml lints stay quiet, and + // repeated warnings are deduped. The explicit config surfaces (gc start, + // gc config) still print prov.Warnings unfiltered (gc-dqn8l). + emitLoadCityConfigWarnings(configWarnWriter(opts.jsonOutput, stderr), prov) monitors, err := selectGitHubPRMonitors(cfg, opts.monitorName) if err != nil { diff --git a/cmd/gc/cmd_github_test.go b/cmd/gc/cmd_github_test.go index 5e82a6be38..ef6b24e4fc 100644 --- a/cmd/gc/cmd_github_test.go +++ b/cmd/gc/cmd_github_test.go @@ -419,3 +419,106 @@ func TestGitHubPRBackfillCommandPropagatesRepairStoreError(t *testing.T) { t.Fatalf("stderr = %q, want store error", stderr.String()) } } + +// appendMixedWarningConfig adds two config shapes to an existing test city, one +// per side of the shouldEmitLoadCityConfigWarning split: +// +// - a mode="always" named session on a wake_mode="fresh" template, the shape +// config.ValidateNamedSessions reports the always+fresh advisory for. The +// filter SUPPRESSES it — a static lint about city.toml, unchanged by +// whatever the current command did. +// - both [agent_defaults] and [agents] tables, whose ambiguity warning the +// filter KEEPS — the operator must act on it. +// +// A command wired to the filter therefore prints exactly one of the two, which +// is what distinguishes a real filter from a blanket mute. +func appendMixedWarningConfig(t *testing.T, cityPath string) { + t.Helper() + tomlPath := filepath.Join(cityPath, "city.toml") + existing, err := os.ReadFile(tomlPath) + if err != nil { + t.Fatalf("read city.toml: %v", err) + } + body := string(existing) + ` +[[agent]] +name = "watchdog" +wake_mode = "fresh" + +[[named_session]] +template = "watchdog" +mode = "always" + +[agent_defaults] +model = "sonnet" + +[agents] +model = "sonnet" +` + if err := os.WriteFile(tomlPath, []byte(body), 0o644); err != nil { + t.Fatalf("write city.toml: %v", err) + } +} + +// TestGitHubPRBackfillSuppressesAlwaysFreshAdvisory proves `gc github pr +// backfill` does not reprint the static always+fresh city.toml advisory on +// stderr. The command is not a config surface: its subject is GitHub PR +// readiness, so a lint about a property of city.toml that is identical on every +// invocation is pure noise there. gc-dqn8l removed that reprint from the shared +// emit path, but this command iterated prov.Warnings directly and so kept +// emitting it — a reachable command path with the bug the branch removes. +// +// The absence assertion is guarded two ways so it cannot pass vacuously. It +// first proves the fixture still provokes the advisory at config-load time, so +// a validator that stopped emitting it fails here instead of silently turning +// this into an empty test. And it classifies stderr with +// config.IsAlwaysFreshWakeModeWarning — the same exported predicate the fix +// consults — rather than a copy of the message text, so a reworded advisory +// cannot slip past. The paired positive assertion (a kept migration warning) +// distinguishes a filter from a blanket mute. +func TestGitHubPRBackfillSuppressesAlwaysFreshAdvisory(t *testing.T) { + cityPath := writeGitHubMonitorTestCity(t) + appendMixedWarningConfig(t, cityPath) + + // The fixture must actually provoke the advisory, or the absence assertion + // below proves nothing. + _, prov, err := loadConfigCommandCityConfig(cityPath) + if err != nil { + t.Fatalf("load fixture city: %v", err) + } + advisories := 0 + for _, w := range prov.Warnings { + if config.IsAlwaysFreshWakeModeWarning(w) { + advisories++ + } + } + if advisories == 0 { + t.Fatalf("fixture no longer provokes the always+fresh advisory; warnings = %q", prov.Warnings) + } + + oldToken := resolveGitHubTokenForBackfill + oldClient := newGitHubPRBackfillClient + resolveGitHubTokenForBackfill = func(context.Context) (string, error) { return "token", nil } + newGitHubPRBackfillClient = func(string) githubPRLister { + return fakeGitHubPRLister{prs: []githubmonitor.PullRequest{ + {Number: 1, BaseRefName: "main", HeadSHA: "abc", MergeStateStatus: "DIRTY"}, + }} + } + t.Cleanup(func() { + resolveGitHubTokenForBackfill = oldToken + newGitHubPRBackfillClient = oldClient + }) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", cityPath, "github", "pr", "backfill", "partcl-main"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("run code = %d, stdout = %q, stderr = %q", code, stdout.String(), stderr.String()) + } + for _, line := range strings.Split(stderr.String(), "\n") { + if config.IsAlwaysFreshWakeModeWarning(line) { + t.Fatalf("always+fresh advisory must stay off non-config command stderr, got %q", stderr.String()) + } + } + if !strings.Contains(stderr.String(), "both [agent_defaults] and [agents] are present") { + t.Fatalf("actionable migration warning must survive the filter, got %q", stderr.String()) + } +} diff --git a/cmd/gc/strict_warnings_test.go b/cmd/gc/strict_warnings_test.go index f236bb81ec..4249eb0f5a 100644 --- a/cmd/gc/strict_warnings_test.go +++ b/cmd/gc/strict_warnings_test.go @@ -6,18 +6,26 @@ import ( "github.com/gastownhall/gascity/internal/config" ) -// TestAlwaysFreshWakeModeWarningIsNonFatalAndEmitted proves the always+fresh -// advisory behaves like a warning on both downstream re-classifiers of config +// TestAlwaysFreshWakeModeWarningIsNonFatalAndUnprinted proves the always+fresh +// advisory behaves correctly on both downstream re-classifiers of config // warnings: strict mode — on by default for `gc start` — keeps it NON-FATAL, -// and the agent warning-emit path SURFACES it. The bundled gastown pack trips -// this warning, so without the config.IsAlwaysFreshWakeModeWarning wiring -// `gc start --foreground` / `--controller` / `--dry-run` exits 1 on the shipped -// example city, and `gc agent` drops the advisory silently. +// and the shared per-command warning-emit path SUPPRESSES it. The bundled +// gastown pack trips this warning, so without the +// config.IsAlwaysFreshWakeModeWarning wiring `gc start --foreground` / +// `--controller` / `--dry-run` exits 1 on the shipped example city. +// +// The advisory is a lint about a static property of city.toml: it never +// changes between invocations, so repeating it on the stderr of every `gc bd +// show` / `gc bd list` buys nothing. Agent harnesses merge +// stderr into the tool result, which made this one block 7.3% of all +// tool-result text city-wide (gc-dqn8l). It stays discoverable on the surfaces +// whose subject IS the config — `gc start` and `gc config` both print raw +// prov.Warnings, neither of which consults shouldEmitLoadCityConfigWarning. // // The warning text is derived from config.ValidateNamedSessions rather than // hardcoded so this test cannot pass against a string the validator no longer // emits. -func TestAlwaysFreshWakeModeWarningIsNonFatalAndEmitted(t *testing.T) { +func TestAlwaysFreshWakeModeWarningIsNonFatalAndUnprinted(t *testing.T) { warnings, err := config.ValidateNamedSessions(&config.City{ Workspace: config.Workspace{Name: "test-city"}, Agents: []config.Agent{{Name: "watchdog", WakeMode: "fresh"}}, @@ -41,8 +49,8 @@ func TestAlwaysFreshWakeModeWarningIsNonFatalAndEmitted(t *testing.T) { if len(fatal) != 0 || len(nonFatal) != 1 { t.Errorf("strict split: fatal=%v nonFatal=%v, want the always+fresh warning non-fatal", fatal, nonFatal) } - if !shouldEmitLoadCityConfigWarning(w) { - t.Error("an always+fresh warning must be emitted to the operator, not swallowed") + if shouldEmitLoadCityConfigWarning(w) { + t.Error("an always+fresh warning must not be repeated on every command's stderr") } } diff --git a/internal/config/config.go b/internal/config/config.go index 7e2ee58bc1..77e1df227f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4196,9 +4196,9 @@ func validateNamedSessions(cfg *City, requireBackingTemplate bool) (warnings []s const alwaysFreshWakeModeMarker = "starts a fresh provider session after every drain" // IsAlwaysFreshWakeModeWarning reports whether a load warning is the non-fatal -// always+fresh advisory. CLI warning filters use this to print the notice and -// keep it non-fatal in strict mode. Keep in sync with -// alwaysFreshWakeModeMarker. +// always+fresh advisory. CLI warning filters use this to keep the notice +// non-fatal in strict mode and to hold it off the stderr of commands whose +// subject is not the config. Keep in sync with alwaysFreshWakeModeMarker. func IsAlwaysFreshWakeModeWarning(warning string) bool { return strings.Contains(warning, alwaysFreshWakeModeMarker) }