From 75251254e0721c019738a7929918f459a49aa08f Mon Sep 17 00:00:00 2001 From: refinery costing Date: Sun, 23 Aug 2026 06:46:48 +0000 Subject: [PATCH 1/3] fix(cli): stop reprinting the always+fresh advisory on every command (gc-dqn8l) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The always+fresh named_session advisory is a config lint: it reports a static property of city.toml that cannot change between invocations. It was nonetheless emitted from shouldEmitLoadCityConfigWarning, which sits on the shared loadCityConfigFS path that nearly every gc command takes, so the same 7-line block was reprinted on the stderr of `gc bd show`, `gc bd list`, `gc hook --claim`, and the rest. That is not free. Agent harnesses merge stderr into the tool result, so the block was measured at 1.6M tokens across 3,925 of 34,677 tool results in a trailing 24h — 7.3% of ALL tool-result text city-wide — and each copy then sits in the agent's context for the remainder of the session, re-read on every subsequent request. stderr is no defence for the same reason, and the block is a known jq-breaker on the `--json` call sites that do not thread configWarnWriter through. Suppress it in shouldEmitLoadCityConfigWarning, exactly mirroring the IsLegacyWorkspaceFieldWarning precedent immediately above it. This is a print-site change only — nothing about classification moves: - strict mode still treats it as non-fatal via strictWarningIsNonFatal, so `gc start --foreground`/`--controller`/`--dry-run` still exits 0 on the shipped example city; - config.ValidateNamedSessions still produces it, so it remains in prov.Warnings; - `gc start` and `gc config` both print raw prov.Warnings without consulting this filter, so the advisory stays fully discoverable on the surfaces whose subject IS the config. Suppression rather than a per-process dedup because every gc invocation is a fresh process — the sync.Map dedup in emitSupervisorLoadCityConfigWarnings only helps the long-lived supervisor. Validation: TestAlwaysFreshWakeModeWarningIsNonFatalAndUnprinted (renamed from ...AndEmitted, its assertion inverted) pins both halves of the new contract; TestEmitLoadCityConfigWarningsFiltersNonMigrationWarnings gains the advisory as an input and asserts it is filtered. Both were written first and observed failing. Measured end-to-end against the live city with a patched binary: `gc bd show` and `gc bd list` go from 7 advisory lines on stderr to 0, while `gc config show` still prints all 7. go vet clean on cmd/gc and internal/config. --- cmd/gc/cmd_agent.go | 15 +++++++++++++++ cmd/gc/cmd_agent_test.go | 4 ++++ cmd/gc/strict_warnings_test.go | 26 +++++++++++++++++--------- 3 files changed, 36 insertions(+), 9 deletions(-) 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/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") } } From 93536e3cb629cf8562c6ee0835fff94607ef2256 Mon Sep 17 00:00:00 2001 From: refinery costing Date: Sun, 23 Aug 2026 08:07:51 +0000 Subject: [PATCH 2/3] fix(cli): route gc github pr backfill warnings through the shared filter (gc-nmd11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-open signoff on polecat/gc-dqn8l (review bead gc-315wv) found the branch left one reachable command path still emitting the advisory it set out to remove. gc-dqn8l suppressed the always+fresh named_session notice in shouldEmitLoadCityConfigWarning, which the shared emitLoadCityConfigWarnings path consults. But doGitHubPRBackfill loads config via loadConfigCommandCityConfig and then iterated prov.Warnings itself, printing every entry raw — so in any city with an always+fresh named session, `gc github pr backfill` still reprinted the block on stderr before it reached GitHub or token handling. Replace that raw loop with the same emitLoadCityConfigWarnings / configWarnWriter pair cmd_sling.go, cmd_convoy.go, and cmd_rig.go already use. This command's subject is GitHub PR readiness, not the config, so it belongs on the filtered side of the split: actionable migration guidance still prints, static city.toml lints stay quiet, and the emitter's dedup drops the repeated copies the raw loop printed (the added fixture trips the agent_defaults/agents ambiguity warning twice). configWarnWriter also subsumes the hand-rolled `if !opts.jsonOutput` guard, preserving JSON-mode silence. The explicit config surfaces are deliberately untouched: cmd_start.go:745 and cmd_config.go:902 still print prov.Warnings unfiltered, so the advisory stays discoverable exactly where the config IS the subject. cmd_supervisor.go already consults the same filter via emitSupervisorLoadCityConfigWarnings. Also corrects the IsAlwaysFreshWakeModeWarning doc comment, which still claimed CLI filters use the marker "to print the notice" — collateral of gc-dqn8l inverting that behavior. Validation: new TestGitHubPRBackfillSuppressesAlwaysFreshAdvisory drives the real command through run() against a city carrying both a suppressed warning (always+fresh) and a kept one (both [agent_defaults] and [agents] present), so it fails against a blanket mute as well as against the raw loop. Verified failing at the reviewed commit 75251254e with the advisory present, passing after. `go test ./cmd/gc -run 'TestGitHubPR|TestEmitLoadCityConfigWarnings| TestAlwaysFreshWakeMode'` and `go test ./internal/config` pass; `go vet ./cmd/gc/... ./internal/config/...` clean. --- cmd/gc/cmd_github.go | 12 +++--- cmd/gc/cmd_github_test.go | 79 +++++++++++++++++++++++++++++++++++++++ internal/config/config.go | 6 +-- 3 files changed, 89 insertions(+), 8 deletions(-) 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..12e86fb868 100644 --- a/cmd/gc/cmd_github_test.go +++ b/cmd/gc/cmd_github_test.go @@ -419,3 +419,82 @@ 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. +// +// Migration warnings the operator must act on are unaffected: this asserts only +// the always+fresh marker is gone, and the shared filter still passes +// everything shouldEmitLoadCityConfigWarning approves. +func TestGitHubPRBackfillSuppressesAlwaysFreshAdvisory(t *testing.T) { + cityPath := writeGitHubMonitorTestCity(t) + appendMixedWarningConfig(t, cityPath) + 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()) + } + if strings.Contains(stderr.String(), "starts a fresh provider session after every drain") { + 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/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) } From 8eeb96886d5dc4980439774edc8b35aaa54443ed Mon Sep 17 00:00:00 2001 From: refinery costing Date: Sun, 23 Aug 2026 08:14:44 +0000 Subject: [PATCH 3/3] test(cli): make the backfill advisory test non-vacuous and marker-robust (gc-nmd11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review hardening of the test added in the previous commit. As first written it asserted absence by searching stderr for a copy of the advisory's message text, which fails open in two ways: if the fixture ever stopped provoking the advisory, or if the validator reworded it, the assertion would still pass while proving nothing. strict_warnings_test.go already avoids the second trap by deriving the warning from config.ValidateNamedSessions instead of hardcoding it; this brings the new test to the same standard. Two guards. The test now loads the fixture city through loadConfigCommandCityConfig up front and fails unless prov.Warnings actually contains an always+fresh advisory, so a validator change surfaces as a failure here rather than as a silently empty test. And it classifies stderr lines with config.IsAlwaysFreshWakeModeWarning — the same exported predicate the fix consults — so the assertion tracks the marker rather than a copy of the prose. Verified by inverting the fix: with cmd_github.go restored to the raw prov.Warnings loop at 75251254e the test fails on the advisory, and passes again once the fix is back. --- cmd/gc/cmd_github_test.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/cmd/gc/cmd_github_test.go b/cmd/gc/cmd_github_test.go index 12e86fb868..ef6b24e4fc 100644 --- a/cmd/gc/cmd_github_test.go +++ b/cmd/gc/cmd_github_test.go @@ -467,12 +467,34 @@ model = "sonnet" // emit path, but this command iterated prov.Warnings directly and so kept // emitting it — a reachable command path with the bug the branch removes. // -// Migration warnings the operator must act on are unaffected: this asserts only -// the always+fresh marker is gone, and the shared filter still passes -// everything shouldEmitLoadCityConfigWarning approves. +// 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 } @@ -491,8 +513,10 @@ func TestGitHubPRBackfillSuppressesAlwaysFreshAdvisory(t *testing.T) { if code != 0 { t.Fatalf("run code = %d, stdout = %q, stderr = %q", code, stdout.String(), stderr.String()) } - if strings.Contains(stderr.String(), "starts a fresh provider session after every drain") { - t.Fatalf("always+fresh advisory must stay off non-config command stderr, got %q", 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())