Accept per-field service_account in BigQuery credentials - #94
Merged
Merged
Conversation
The BigQuery executor reads `service_account_path`, and the DSN parser already maps `service_account` / `credentials_path` to it — but the per-field INI loader passed the section through verbatim, so a `service_account = /path/key.json` line was silently ignored and the client fell back to ADC. The docs use the shorter `service_account` spelling, making this an easy footgun. Normalize the aliases to `service_account_path` in `_load_credentials` (explicit `service_account_path` still wins). Vendored lib re-synced. Tests: alias normalizes from the file; explicit path isn't clobbered.
There was a problem hiding this comment.
Pull request overview
This PR fixes an inconsistency in BigQuery credential loading: the per-field INI profile form now treats service_account and credentials_path as aliases for service_account_path, matching DSN parsing behavior and preventing silent fallback to Application Default Credentials.
Changes:
- Normalize per-field
service_account/credentials_pathtoservice_account_pathin_load_credentials. - Re-sync the vendored
plugins/agami/lib/execute_sql.pycopy to match core behavior. - Add tests covering per-field alias normalization and “explicit path wins” semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/agami-core/src/execute_sql.py | Adds per-field alias normalization so BigQuery credentials consistently use service_account_path. |
| plugins/agami/lib/execute_sql.py | Mirrors the same alias normalization in the vendored executor copy. |
| tests/test_env_credentials.py | Adds coverage for per-field BigQuery alias behavior and precedence rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+127
to
+145
|
|
||
|
|
||
| def test_bigquery_explicit_service_account_path_is_not_clobbered(monkeypatch, tmp_path): | ||
| """If both `service_account_path` and the alias appear, the explicit path wins.""" | ||
| creds = tmp_path / "credentials" | ||
| creds.write_text( | ||
| "[gcp]\ntype = bigquery\nproject = my-proj\n" | ||
| "service_account_path = /explicit/key.json\n" | ||
| "service_account = /alias/key.json\n", | ||
| encoding="utf-8", | ||
| ) | ||
| import os | ||
| if os.name == "posix": | ||
| creds.chmod(0o600) | ||
| monkeypatch.setattr(execute_sql, "CREDENTIALS_PATH", creds) | ||
|
|
||
| assert _load_credentials("gcp")["service_account_path"] == "/explicit/key.json" | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
Addressed in d0a9ddb (landed after this review): the test is now @pytest.mark.parametrize("alias", ["service_account", "credentials_path"]), so both aliases are covered in the per-field/INI path. Mutation-checked — dropping the credentials_path arm of the loop fails the parametrized case.
Address the SDLC review's test-quality findings: - Cover the `credentials_path` alias too (was untested) via parametrize. - Rename/clarify the precedence test so it pins the `not …get(...)` guard; mutation-checked that dropping the guard now fails it. - Add the empty-value edge (`service_account =`) → stays unset, ADC path. Test-only change; production logic unchanged.
Contributor
Author
Agami SDLC review — Review stage (panel)Ran the standard review skills (code-review / silent-failure / test-analyzer / security-review) + the Agami rubric. This assists; human code-owner approval still required. Standard skills
Agami rubric
Agami review complete — 0 outstanding must-fix (2 raised, 2 fixed), 1 nit. Human PR approval still required. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What
A per-field
service_account = /path/key.jsonline in a BigQuery credentials profile was silently ignored — the executor readsservice_account_path, so the client fell back to Application Default Credentials instead of using the key. The docs use the shorterservice_accountspelling, making this an easy footgun._load_credentialsnow normalizes theservice_account/credentials_pathaliases toservice_account_pathfor the per-field INI form — matching what_parse_dsnalready does for theurl = bigquery://...?service_account=...form. An explicitservice_account_pathstill wins if both are present.Why
The three spellings were already equivalent in the DSN path but not the per-field path — inconsistent, and the inconsistency failed closed (fell back to ADC) with no error, so users couldn't tell why their key wasn't used.
Changes
packages/agami-core/src/execute_sql.py— alias normalization in_load_credentials(source of truth).plugins/agami/lib/execute_sql.py— re-synced vendored copy (dev.py sync-lib); drift check passes.tests/test_env_credentials.py— two tests: alias normalizes from the file; explicitservice_account_pathisn't clobbered.Test plan
uv run dev.py check— full suite green (1326 passed), ruff + gitleaks + lib-drift all pass.Customer-safety
No real names/data/credentials — neutral
my-proj//abs/path/key.jsonplaceholders only.