fix(antigravity): gate keyring on auth method, fix eval-masking (#109, #124) - #1327
Open
ptone wants to merge 3 commits into
Open
fix(antigravity): gate keyring on auth method, fix eval-masking (#109, #124)#1327ptone wants to merge 3 commits into
ptone wants to merge 3 commits into
Conversation
added 3 commits
August 28, 2026 20:52
…#124) The agy-wrapper.sh keyring block (dbus-launch, gnome-keyring-daemon, secret-tool) was gated on `is_adc` — True only for vertex-ai — so it ran for api-key, oauth-token, AND none. On images without the keyring packages (including the omni image), this caused exit 127 under `set -e`, killing the wrapper before agy could start — even with a working GEMINI_API_KEY sitting in the environment. Changes: Decision 1 — Gate on auth method, not binary presence: - api-key: no keyring block (reads GEMINI_API_KEY from env) - vertex-ai: no keyring block (uses ADC) - none: no keyring block (no credentials) - oauth-token: keyring block, as before (only method that uses it) Decision 2 — oauth-token without keyring binaries fails loudly: - Checks for dbus-launch, gnome-keyring-daemon, secret-tool up front - Names the missing binary in the error message - Does NOT fall back to writing token to disk — per #108, on single-node tiers a deleted agent's HOME is inherited by the next tenant; a plaintext refresh token on disk would be a credential leak. Rejected deliberately. Decision 3 — Fix eval-masking (#124): - No bare `eval $(command ...)` — captures output, checks exit code first - No `>/dev/null 2>&1` on fatal commands (gnome-keyring-daemon --start) - Every failure produces a named, specific error message Decision 4 — Remove dead `has_token` parameter: - `has_token` was passed to `_generate_wrapper_script` but never read inside the function body. Removed from signature and all call sites. - `is_adc` parameter replaced by `auth_method` (the resolved method string).
…phemeral The docstring incorrectly implied the keyring was ephemeral because the daemon dies with the process tree. In fact, secret-tool store writes to ~/.local/share/keyrings/login.keyring on disk, encrypted with the unlock password. The daemon is ephemeral; the keyring files are not. Verified empirically: after gnome-keyring-daemon is killed, the keyring files persist at ~/.local/share/keyrings/ with the stored secrets intact. Added a CREDENTIAL PERSISTENCE NOTE to the docstring documenting this exposure for images where the keyring binaries are present. This PR does not address that exposure — it only ensures the keyring block is not reached for auth methods that do not need it.
…t comment Finding 1 (sn-1327-rev): eval "$_unlock_output" ran unconditionally after the || block. If gnome-keyring-daemon --unlock failed, _unlock_output contained merged stdout+stderr (error messages), and eval'ing that under set -e would produce an undiagnosable fatal exit. Fixed: eval is now inside the if-success branch; the else branch only prints the WARNING. Finding 2 (sn-1327-rev): The bash comment inside the generated wrapper still said "the keyring lives only for this process tree's lifetime." Corrected to distinguish daemon lifetime (ephemeral) from keyring store lifetime (persists on disk at ~/.local/share/keyrings/).
3 tasks
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.
Summary
is_adcrather than the resolved auth method. On images without gnome-keyring packages (including the omni image), this killed the wrapper at exit 127 underset -e— even with a working GEMINI_API_KEY in the environment.eval $(command ...)which masks exit codes of missing binaries, and>/dev/null 2>&1on fatal commands, producing zero diagnostic output on failure.has_tokenparameter removed — passed to_generate_wrapper_scriptbut never read.Decisions (per sn-impl-arch review)
Decision 1: Gate on auth method
api-key— no keyring block (agy reads GEMINI_API_KEY from env)vertex-ai— no keyring block (ADC via GOOGLE_APPLICATION_CREDENTIALS)none— no keyring block (no credentials)oauth-token— keyring block (only method that uses it)Decision 2: oauth-token fails loudly if binaries absent
Checks for dbus-launch, gnome-keyring-daemon, secret-tool before calling them. Names the missing binary. Does NOT fall back to writing token to disk.
Deliberate rejection: Writing the OAuth refresh token to
~/.gemini/antigravity-cli/antigravity-oauth-tokenwas considered and rejected. Per #108, on single-node tiers a deleted agent HOME survives and is inherited by the next agent with the same slug. A plaintext refresh token on disk would be a credential leak to the next tenant. A loud failure is better than a quiet credential leak.Credential persistence note: When the keyring block DOES run (oauth-token with binaries present),
secret-tool storewrites the refresh token to~/.local/share/keyrings/login.keyringon disk, encrypted with the unlock password (currently the literal"test"). The gnome-keyring daemon is ephemeral (dies with the process tree), but the keyring files persist on disk. On any tier where #108 applies (agent HOME inherited by next tenant), this is a credential stored on disk protected only by a known password. This PR does not address that exposure — it only ensures the keyring block is not reached for auth methods that do not need it.Decision 3: Fix eval-masking (#124)
eval $(...)— output captured and checked before eval>/dev/null 2>&1on fatal gnome-keyring-daemon commandseval $(missing_command)returns 0 because eval of empty stdout succeeds). It is now correctly fatal. gnome-keyring-daemon --unlock remains non-fatal (WARNING). gnome-keyring-daemon --start was previously fatal-but-silent; it is now fatal-with-diagnostics.Decision 4: Remove dead parameter
has_tokenremoved from_generate_wrapper_scriptsignature and all call sitesis_adcreplaced byauth_method(the resolved method string)Not in this change
Test plan
test_api_key_wrapper_has_no_keyring_calls— the mutation guard: reverting the gate makes this failtest_none_wrapper_has_no_keyring_calls— method=none also skips keyringtest_oauth_token_wrapper_has_keyring_calls— oauth-token still gets keyringtest_oauth_token_no_bare_eval_of_command— Hub phase stuck at 'starting' after spurious session lifecycle events #124 eval-masking guardtest_oauth_token_no_dev_null_on_fatal_commands— Hub phase stuck at 'starting' after spurious session lifecycle events #124 stream-discard guardtest_has_token_not_in_signature— dead parameter guardgo test ./harnesses/...)make test-fastpasses