gc CLI prints the always+fresh named_session advisory on every command's stderr — 7.3% of all tool-result text city-wide (gc-dqn8l) - #157
Merged
Conversation
added 3 commits
August 23, 2026 06:46
…(gc-dqn8l)
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.
…ter (gc-nmd11) 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 7525125 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.
…ust (gc-nmd11) 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 7525125 the test fails on the advisory, and passes again once the fix is back.
Author
|
Codex signoff (pre-open, comment-only — not an approval): VERDICT: COMMENT Scope checked: full diff for cmd/gc/cmd_agent.go, cmd/gc/cmd_agent_test.go, cmd/gc/cmd_github.go, cmd/gc/cmd_github_test.go, cmd/gc/strict_warnings_test.go, and internal/config/config.go; related warning emission call sites and direct prov.Warnings readers in cmd/gc. Findings: None blocking. No P0/P1 findings. Verification: PASS Not checked: full repository test suite; live GitHub backfill against production credentials. |
johnzook
approved these changes
Aug 24, 2026
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…d's stderr — 7.3% of all tool-result text city-wide (gc-dqn8l) (#157) * fix(cli): stop reprinting the always+fresh advisory on every command (gc-dqn8l) 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. * fix(cli): route gc github pr backfill warnings through the shared filter (gc-nmd11) 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 7525125 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. * test(cli): make the backfill advisory test non-vacuous and marker-robust (gc-nmd11) 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 7525125 the test fails on the advisory, and passes again once the fix is back. --------- Co-authored-by: refinery costing <refinery@local>
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…d's stderr — 7.3% of all tool-result text city-wide (gc-dqn8l) (#157) * fix(cli): stop reprinting the always+fresh advisory on every command (gc-dqn8l) 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. * fix(cli): route gc github pr backfill warnings through the shared filter (gc-nmd11) 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 7525125 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. * test(cli): make the backfill advisory test non-vacuous and marker-robust (gc-nmd11) 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 7525125 the test fails on the advisory, and passes again once the fix is back. --------- Co-authored-by: refinery costing <refinery@local>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Symptom
Every
gcinvocation that loads city config prints a 7-line block, one lineper always+fresh named session:
It is a CONFIG lint about a static property of city.toml. It does not change
between invocations, and it is emitted again on
gc bd show,gc bd list,gc session list,gc convoy status,gc hook --claim— every command.Measured cost
From specs/tk-yhwfv.1 (the gc-toolkit context-budget epic, leg 2), trailing
24h across the city: 3,925 of 34,677 tool results carry the block — 1.6M
tokens on first pass alone, 7.3% of ALL tool-result text city-wide. And it
does not cost once: each block then sits in the agent's context for the
remainder of that session, re-read on every subsequent request.
Where it comes from
alwaysFreshWakeModeMarker.and cmd/gc/strict_warnings.go:23.
emitLoadCityConfigWarnings, called fromloadCityConfigFS(cmd_agent.go:56) — i.e. the shared config-load pathevery command takes.
loadCityConfigDefaultWarningWriter= os.Stderr(cmd_agent.go:~168).
Why stderr is not a defence
Two reasons it still costs:
configWarnWriter(cmd_agent.go:~175) alreadyroutes to io.Discard in JSON mode, but only at the call sites that thread it
through — cmd_convoy.go:164 and cmd_sling.go:420. The shared
loadCityConfigFSpath does not, so--jsonon other commands still getsit on stderr.
Suggested shape
Fire it once at config load or under an explicit verbose/doctor surface, never
on the stdout/stderr of every command. Options worth weighing: emit only for
commands whose subject IS the config (
gc config,gc doctor); gate behind--verbose; or emit once per process and suppress repeats.Whichever is chosen, keep it discoverable — the advisory is legitimate, it is
the per-invocation repetition that is the defect.
Provenance
Filed from gc-toolkit bead tk-yhwfv.1, which owns the pack-side half of the
context-budget work. This half is a gc BINARY change and cannot land from a
gc-toolkit branch.
Implementation notes
Implemented: suppress the always+fresh named_session advisory on the shared per-command config-load print path (shouldEmitLoadCityConfigWarning), mirroring the IsLegacyWorkspaceFieldWarning precedent. Classification is unchanged: still non-fatal in strict mode, still produced into prov.Warnings, and still printed in full by gc start and gc config -- neither consults this filter. Measured against the live city: gc bd show / gc bd list go 7 advisory lines -> 0 on stderr, gc config show still prints all 7. Tests written first and observed failing; local gate make test-fast-parallel green 10/10 (rc=0); go vet + gofmt + row censuses clean.
Refinery handoff
gc-dqn8l(bug, P1)polecat/gc-dqn8lmain8eeb9688; PR opened codex-green.