fix(config): reject invalid cache_ttl instead of silently defaulting - #67
Merged
Conversation
aaearon
force-pushed
the
fix/cache-config-semantics
branch
from
August 16, 2026 07:40
dd10a0d to
49c1ff8
Compare
…l guard Renames maxSessionAge to sessionTimestampRetention and corrects its comment: CleanupSessions filters on activeIDs membership and never reads the constant.
ParseCacheTTL now returns (time.Duration, error). Absent still means the 4h default; unparseable, zero and negative values are all rejected, consistently. Validation runs in config.Load so a bad value surfaces at startup rather than when a command happens to build a cache. buildCachedLister and its seven command call sites propagate the error. Adds config coverage for partial-YAML defaults, non-nil Favorites, invalid YAML, ConfigDir naming, Save's 0600/0700 modes, MkdirAll failure, and a portable read-error test that also runs on the Windows CI leg.
Both resolution steps discarded the config.LoadDefaultWithPath error and substituted DefaultConfig(). Since Load started rejecting an invalid cache_ttl, that made `request submit` the one command where a bad value was neither honored nor reported. Propagate the error, and load the config before authenticating so the failure does not require a working auth cycle first. Extract the on-demand cache construction into buildCachedRolesLister, mirroring buildCachedLister, so its bad-TTL arm is reachable from a test. Also pin that `grant configure` still works over a broken config: it never calls Load, which is what keeps it a recovery path.
"invalid cache_ttl" alone leaves a user with a non-default GRANT_CONFIG no indication of which file to edit. Also tighten TestLoad_InvalidYAMLErrors to assert the yaml parse text: Load now has a second error source (the cache_ttl validation) that a bare "did it error" check would accept.
The changelog entry was 203 chars against the documented ~120, carried
mechanism notes that belong in the PR body, and said "at startup" —
which is wrong in both directions: nothing fails before a command
reaches config.Load, and `configure` never reaches it at all.
The "ERROR_DIRECTORY on Windows" claim was also wrong. os.MkdirAll
(os/path.go) stats the parent itself and synthesizes
&PathError{Op: "mkdir", Err: syscall.ENOTDIR} in platform-independent
Go, so it is ENOTDIR on both platforms. Corrected in CLAUDE.md, the
test comment and the ledger.
Record in the ledger that the seven buildCachedLister call sites'
error propagation remains unpinned, and why it is not worth
restructuring production code to fix.
Name the expected duration syntax on the unparseable arm (still wrapping the time.ParseDuration error) and name --refresh on the non-positive arm, so someone who used 0s as a cache kill-switch has a replacement.
runConfigure rebuilds the config from scratch, dropping favorites and default_provider. Rename the test, pin that loss, and record the sharp edge in CLAUDE.md; the remedy is to edit the file the error names.
request submit bootstraps its service in RunE first, so an unauthenticated user hits the auth prompt before the config error.
aaearon
force-pushed
the
fix/cache-config-semantics
branch
from
August 16, 2026 13:29
49c1ff8 to
e9889e5
Compare
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
1 participant
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.
Part 6 of 8. Base:
test/isolation-harness(#63) — must merge first.ParseCacheTTLreturns(time.Duration, error)and now rejects any explicitly-supplied invalid value:cache_ttl: garbagecache_ttl: 0sThe original plan rejected only
<= 0. That was an anti-pattern — it would have validated the same field two opposite ways, since"garbage"already defaulted silently. Consistency was the correct fix.grant configureis deliberately unaffected: it never callsLoad, so it remains a working recovery path for rewriting a broken config.Also
grant request submitdiscarded theLoaderror and substitutedDefaultConfig(), so it was the one command where an invalid TTL was neither rejected nor noticed — and theParseCacheTTLerror handling below it was unreachable dead code. Fixed, and the config load moved ahead ofbootstrapSCAService()so the failure surfaces without an auth round-trip (which is also what makes it testable).maxSessionAge→sessionTimestampRetention(still 24h). Its old comment claimed entries are "removed on cleanup" — false;CleanupSessionsfilters on active IDs and never reads it.TestGet_CorruptJSONpassed via the TTL branch, not the unmarshal branch. Rewritten so only the unmarshal guard can produce the miss.Load's read-error branch (the existing test skips on Windows, leaving that leg uncovered).A trap worth recording
The planned
MkdirAllassertion did not kill its mutation: with the error swallowed, the subsequentWriteFilefails for the sameENOTDIRreason, soSavestill errored and a naive check passed. Tightened to assert thefs.PathErrorOpismkdir— set by platform-independent Go code, so it is portable.Also corrected: the claim that this construction yields
ERROR_DIRECTORYon Windows. It does not —os.MkdirAllsynthesisesENOTDIRitself on both platforms.Known limit
The seven
buildCachedListercall sites' error propagation is unpinned — they sit in productionRunEclosures unreachable from unit tests. Static reading confirms all are correct; the ledger says so rather than reading a baredone.22 mutations reverified. Adversarial review performed (Codex credits exhausted; review by a Claude agent). Both blocking findings fixed.