Fix: gstack-config get returns "" with exit 0 for keys missing from the DEFAULTS table - #2368
Open
benjaminberes-bp wants to merge 1 commit into
Open
Conversation
…efault
Skill preambles read configuration with
VAR=$(gstack-config get <key> 2>/dev/null || echo "<default>")
and that fallback only fires on a non-zero exit. lookup_default ended in a
catch-all that echoed "" and returned 0, so for any key missing from the table
VAR came back empty and the default written right there in the preamble was
unreachable. The skill then branched on a value it never specified: "skip
entirely if QUESTION_TUNING is false", reached with QUESTION_TUNING="".
Four keys that skills actually read had no entry and took that path:
question_tuning -> callers assume "false"
repo_mode -> callers assume "unknown"
team_mode -> callers assume "false"
transcript_ingest_mode -> callers assume "off"
Each default above is the value the call sites already substitute in their own
`|| echo` fallback, so this only makes reachable what was already intended.
The catch-all now returns non-zero. That is deliberately scoped to the
unknown-key arm alone: keys whose default is intentionally empty still exit 0,
because "" is their real answer and their callers depend on it --
cross_project_learnings ("unset triggers the first-time prompt"),
redact_repo_visibility ("empty falls through to gh/glab detection"),
salience_allowlist, user_slug_at_*. Making every empty answer an error would
have broken those.
test/gstack-config-defaults.test.ts pins the class rather than the four
instances: it parses the case arms and asserts every `gstack-config get <key>`
site in the tree is covered, so adding a read without a default fails CI. It
also pins the exit-code contract in both directions. Verified failing against
the pre-fix script, where it names exactly those four keys.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
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.
Skill preambles read configuration with
VAR=$(gstack-config get <key> 2>/dev/null || echo "<default>")That fallback only fires on a non-zero exit.
lookup_defaultended in acatch-all that echoed
""and returned 0, so for any key absent from the tableVARcame back empty and the default written right there in the preamble wasunreachable. The skill then branched on a value it never specified — "skip
entirely if
QUESTION_TUNING: false", reached withQUESTION_TUNING="".Keys affected
Four keys that skills actually read had no entry:
question_tuningfalserepo_modeunknownteam_modefalsetranscript_ingest_modeoffEach default added here is taken from the call sites own
|| echofallback, sothis only makes reachable what was already intended.
Scope of the exit-code change
The catch-all now returns non-zero, and that is deliberately limited to the
unknown-key arm. Keys whose default is intentionally empty keep exit 0,
because
""is their real answer and their callers depend on receiving it:cross_project_learnings—# intentionally empty, unset triggers first-time promptredact_repo_visibility—# empty, falls through to gh/glab detectionsalience_allowlist,user_slug_at_*Making every empty answer an error would have broken those, which is why the
change is not "exit non-zero when the value is empty".
Test
test/gstack-config-defaults.test.tspins the class rather than the fourinstances. It parses the
casearms and asserts that everygstack-config get <key>site in the tree is covered by one, so adding a readwithout adding its default fails CI. It also pins the exit-code contract in
both directions.
Verified failing against the pre-fix script, where it names exactly those four
keys, and passing after.
Found on Windows 11 / git-bash / bun 1.3.14, but nothing here is
platform-specific.