fix(google-workspace): stop reporting connection_ok when nothing was checked - #1707
fix(google-workspace): stop reporting connection_ok when nothing was checked#1707kwit75 wants to merge 2 commits into
Conversation
…checked
Josh hit this on the Gmail node: one response contained both
connection_ok: true
Gmail API 403 — "Gmail API has not been used in project … or it is disabled"
Both lines are ours. He re-authenticated, which could never have helped, because
the tool had told him the connection was fine.
connection_ok was derived from three things, none of which touch the API: a
client object was constructed, the live probe is OPTIONAL and this service
supplied none, and the token's CLAIMED scopes look right. So it answered "do we
hold a plausible credential" while every reader takes it as "this node works".
A disabled API is invisible to all three — as are an exhausted quota, disabled
billing, and an org-policy block.
Changes:
* `checked: [...]` reports what was actually exercised, so a verified answer
is distinguishable from an assumed one.
* When no live call was made, connection_ok is now `'unknown'` with a note
rather than `true`. A green light nobody checked is worse than no light —
it redirects the debugging effort, which is exactly what happened here.
* `errorReason` surfaces Google's reason verbatim. It distinguishes
accessNotConfigured (API disabled) from forbidden (permission) from
rateLimitExceeded, and that one word names the fix.
* gmail -> probes users().getProfile(userId='me')
drive -> probes about().get(fields='user')
(calendar already probed calendarList().list)
sheets and docs deliberately get NO probe. Both would need a real
spreadsheet/document id, and probing with a fabricated one returns 404 on a
perfectly healthy API — a false negative, which is the same disease in the
other direction. They now honestly report `unknown` instead. Giving them a real
probe means a caller-supplied id or a create-then-delete, neither of which
belongs in a read-only diagnostic.
Verified: all five modules compile, ruff clean, and `execute` is confirmed
present in the `.client` import list of both gmail and drive (calendar's
existing probe was the pattern). NOT verified: a live call against Google — no
credentials here, so the probes' behaviour on a real 403 is unexercised.
Closes #1694.
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughGoogle Workspace connection checks now record which validations ran, expose API error reasons, report unverified reachability as unknown, and perform live Gmail or Drive API probes. ChangesGoogle Workspace connection diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionCheck
participant SharedDiagnostics
participant GoogleAPI
ConnectionCheck->>SharedDiagnostics: provide Gmail or Drive probe
SharedDiagnostics->>GoogleAPI: execute service-specific API request
GoogleAPI-->>SharedDiagnostics: response or error reason
SharedDiagnostics-->>ConnectionCheck: checked items and connection status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/tool_google_workspace/gmail/IInstance.py`:
- Line 149: Update the Gmail description associated with the connection check
near _check_connection_impl to document that true means the live probe
succeeded, false means the probe failed, and unknown means reachability was not
confirmed; apply the same status semantics to the Drive description. Modify the
Gmail site at nodes/src/nodes/tool_google_workspace/gmail/IInstance.py lines
149-149 and the Drive site at
nodes/src/nodes/tool_google_workspace/drive/IInstance.py lines 165-165, keeping
the probe implementations unchanged.
- Line 149: Update the check_connection implementation around
_check_connection_impl and its execute probe to use an endpoint authorized by
the configured read-only Gmail scopes, such as gmail.metadata or gmail.readonly,
while preserving successful diagnostics for send-only access by avoiding an
unsupported probe for that tier.
In `@nodes/src/nodes/tool_google_workspace/IInstance.py`:
- Around line 137-142: Update the diagnostic state used by the validation flow
in IInstance so the “Credential and scopes look correct” note is emitted only
when credential and scope checks actually completed successfully. Track
completion explicitly, including the non-empty user-token scope validation and
configuration checks, then use that state in the branch handling skipped API
diagnostics instead of claiming verification from connection_ok alone.
- Around line 106-114: Update the Google client execute error-wrapping path so
parsed Google error details from HttpError.content are preserved when raising
the replacement ValueError, allowing _check_connection_impl to populate
errorReason for Gmail/Drive probe failures. Attach the extracted reason to the
replacement exception or otherwise retain it through the raise, while preserving
the existing error and connection failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a07fb7da-10e5-486d-bf8a-84fa4b598e88
📒 Files selected for processing (3)
nodes/src/nodes/tool_google_workspace/IInstance.pynodes/src/nodes/tool_google_workspace/drive/IInstance.pynodes/src/nodes/tool_google_workspace/gmail/IInstance.py
| def check_connection(self, args: dict) -> dict: | ||
| """Check Gmail connection status and whether granted OAuth scopes cover the configured access tier. Read-only.""" | ||
| return self._check_connection_impl() | ||
| return self._check_connection_impl(probe=lambda s: execute(s.users().getProfile(userId='me'))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align both tool descriptions with the new diagnostic contract.
The shared implementation no longer represents a scope-only boolean; true requires successful validation, false can indicate probe failure, and unknown means reachability was not confirmed.
nodes/src/nodes/tool_google_workspace/gmail/IInstance.py#L149-L149: update the Gmail description to explain live probe success/failure andunknown.nodes/src/nodes/tool_google_workspace/drive/IInstance.py#L165-L165: update the Drive description with the same status semantics.
📍 Affects 2 files
nodes/src/nodes/tool_google_workspace/gmail/IInstance.py#L149-L149(this comment)nodes/src/nodes/tool_google_workspace/drive/IInstance.py#L165-L165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/tool_google_workspace/gmail/IInstance.py` at line 149, Update
the Gmail description associated with the connection check near
_check_connection_impl to document that true means the live probe succeeded,
false means the probe failed, and unknown means reachability was not confirmed;
apply the same status semantics to the Drive description. Modify the Gmail site
at nodes/src/nodes/tool_google_workspace/gmail/IInstance.py lines 149-149 and
the Drive site at nodes/src/nodes/tool_google_workspace/drive/IInstance.py lines
165-165, keeping the probe implementations unchanged.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -A5 -B5 '_GMAIL_(SEND|SETTINGS).*SCOPE|def check_connection|getProfile' \
nodes/src/nodes/tool_google_workspace/gmail/IInstance.pyRepository: rocketride-org/rocketride-server
Length of output: 3103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding Gmail instance class and the shared connection helper.
wc -l nodes/src/nodes/tool_google_workspace/gmail/IInstance.py
sed -n '1,190p' nodes/src/nodes/tool_google_workspace/gmail/IInstance.py | cat -n
# Locate shared helper definitions.
rg -n "_check_connection_impl|def _check_connection_impl|class .*Access|scopes" nodes/src/nodes/tool_google_workspace/gmail -S
# Read any referenced service/auth helper files.
git ls-files nodes/src/nodes/tool_google_workspace/gmailRepository: rocketride-org/rocketride-server
Length of output: 12773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate shared access/connection definitions =="
fd -a 'core/google_access|.*access.*|IInstance\.py' nodes nodes/src 2>/dev/null | sed 's#^\./##' | head -50
rg -n "_check_connection_impl|_require_send|GMAIL|superset_scopes|allowed_scopes|GmailAccess|GMAIL_ACCESS|access tiers|readonly|send" nodes/core nodes/src/nodes/tool_google_workspace/gmail nodes -g '*access*' -g '*Instance.py' -S
echo "== Read relevant shared files =="
while IFS= read -r f; do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,260p' "$f" | cat -n
done < <(git ls-files | rg '(^nodes/.*/core/google_access|(^|/)IInstance\.py|(^|/)__init__\.py$|(^|/)client\.py$)' | grep -E 'gmail|google_access|tool_google_workspace' || true)Repository: rocketride-org/rocketride-server
Length of output: 3588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== google_access.py =="
wc -l nodes/src/nodes/core/google_access.py
sed -n '1,260p' nodes/src/nodes/core/google_access.py | cat -n
echo "== Gmail client.py scope helpers/usages =="
wc -l nodes/src/nodes/tool_google_workspace/gmail/client.py
sed -n '1,260p' nodes/src/nodes/tool_google_workspace/gmail/client.py | cat -n
echo "== Gmail node read-only diagnostics/messages scope checks =="
sed -n '190,380p' nodes/src/nodes/tool_google_workspace/gmail/IInstance.py | cat -nRepository: rocketride-org/rocketride-server
Length of output: 31580
🌐 Web query:
Gmail API users.getProfile required OAuth scopes https://developers.google.com/workspace/gmail/api/reference/rest/v1/users/getProfile
💡 Result:
To call the Gmail API method users.getProfile, you must use one of the following OAuth 2.0 scopes [1][2]: - https://mail.google.com/ - https://www.googleapis.com/auth/gmail.modify - https://www.googleapis.com/auth/gmail.compose - https://www.googleapis.com/auth/gmail.readonly - https://www.googleapis.com/auth/gmail.metadata You should choose the most narrowly focused scope that provides the access your application requires [3]. For example, if your application only needs to read email metadata (which is what users.getProfile primarily provides), you may find that the gmail.metadata scope is the most appropriate, least-privileged choice [4].
Citations:
- 1: https://developers.google.com/workspace/gmail/api/reference/rest/v1/users/getProfile
- 2: https://developers.google.com/gmail/api/reference/rest/v1/users/getProfile
- 3: https://developers.google.cn/workspace/gmail/api/auth/scopes
- 4: https://developers.google.com/workspace/gmail/api/auth/scopes
Use a read-only probe for check_connection.
getProfile requires gmail.modify, gmail.compose, gmail.readonly, or gmail.metadata, but gmail.send alone does not. With only the send tier granted, this diagnostic makes check_connection() return connection_ok=False despite sending being authorized. Use a probe that only requires gmail.metadata/gmail.readonly, or avoid an API probe for that tier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/tool_google_workspace/gmail/IInstance.py` at line 149, Update
the check_connection implementation around _check_connection_impl and its
execute probe to use an endpoint authorized by the configured read-only Gmail
scopes, such as gmail.metadata or gmail.readonly, while preserving successful
diagnostics for send-only access by avoiding an unsupported probe for that tier.
Source: MCP tools
| except Exception as exc: | ||
| out['connection_ok'] = False | ||
| out['error'] = str(exc)[:200] | ||
| # Google distinguishes accessNotConfigured (API disabled) from | ||
| # forbidden (permission) from rateLimitExceeded, and that word | ||
| # alone names the fix. Losing it costs the reader the diagnosis. | ||
| reason = getattr(exc, 'reason', None) or getattr(exc, '_get_reason', lambda: None)() | ||
| if reason: | ||
| out['errorReason'] = str(reason).strip()[:120] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -A35 -B3 'def execute|errorReason|_get_reason|\.reason' \
nodes/src/nodes/tool_google_workspace/google_client.py \
nodes/src/nodes/tool_google_workspace/IInstance.pyRepository: rocketride-org/rocketride-server
Length of output: 11690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== IInstance relevant section =="
sed -n '1,155p' nodes/src/nodes/tool_google_workspace/IInstance.py | cat -n
echo
echo "== google_client imports and execute body =="
sed -n '1,45p' nodes/src/nodes/tool_google_workspace/google_client.py | cat -n
sed -n '402,430p' nodes/src/nodes/tool_google_workspace/google_client.py | cat -n
echo
echo "== google_client references to _check_connection_impl and execute =="
rg -n "_check_connection_impl|google_client\.execute|execute\(" nodes/src/nodes/tool_google_workspace -g '*.py'
echo
echo "== Python exception chaining probe =="
python3 - <<'PY'
from googleapiclient.errors import HttpError
import json
body = json.dumps({
"error": {"code": 403, "message": "Disabled API", "errors": [{"message": "API project not enabled", "domain": "global", "reason": "accessNotConfigured"}]}
}).encode()
exc = HttpError(resp=None, content=body, uri="https://example.iam.googleapis.com")
print("HttpError has reason:", hasattr(exc, 'reason'))
print("Google detail:", exc.content.decode())
print("first error detail reason:", (json.loads(exc.content.decode())["error"]["errors"][0])["reason"])
try:
try:
raise exc
except Exception as inner:
detail = getattr(getattr(inner, "resp", None), "status", None) or (getattr(inner, "reason", None) or str(inner))
raise ValueError(f"403: {detail}") from inner
except Exception as outer:
print("wrapper reason:", getattr(outer, "reason", None))
print("wrapper _get_reason:", getattr(outer, "_get_reason", None))
print("outer args:", outer.args)
print("outer cause:", repr(getattr(outer, "__cause__", None)))
PYRepository: rocketride-org/rocketride-server
Length of output: 33096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for dependency definitions mentioning google-api-client/googleapiclient =="
rg -n '"google-api|google-api-client|googleapiclient|google-api-python-client|google-auth' --glob 'pyproject.toml' --glob 'requirements*.txt' --glob 'Pipfile*' --glob 'setup.py' --glob 'setup.cfg' --glob 'poetry.lock' --glob 'requirements*.in' . | head -n 100
echo
echo "== Read-only structural check of ValueError construction from google_client.execute =="
python3 - <<'PY'
from pathlib import Path
p = Path('nodes/src/nodes/tool_google_workspace/google_client.py')
text = p.read_text()
start = text.index('def execute(')
end = text.index('\n\ndef ', start + 1) if '\n\ndef ' in text[start + 1:] else None
body = text[start:end] if end else text[start:]
print(body)
print("raises ValueError:", "raise ValueError(" in body)
print("includes cause from exception:", " from exc" in body)
# Extract only the reason detail and subsequent raise lines.
for i, line in enumerate(body.splitlines(), 1):
if 'detail = getattr(exc, "reason"' in line or 'raise ValueError(' in line:
print(f"{i}: {line}")
PYRepository: rocketride-org/rocketride-server
Length of output: 2493
🌐 Web query:
Google API Python client HttpError reason attribute source _get_reason
💡 Result:
In the Google API Python client (google-api-python-client), the _get_reason method is a private helper function defined within the HttpError class in googleapiclient/errors.py [1][2]. Purpose and Functionality: The _get_reason method is responsible for calculating and extracting the descriptive reason for an HTTP error from the response content [1][3][2]. Because Google APIs often return error information in a JSON-formatted body rather than just the standard HTTP status reason, _get_reason performs the following steps: 1. It attempts to parse the self.content attribute (the raw response body) as JSON [1][2]. 2. If successful, it navigates the JSON structure—typically looking under the error key—to extract the message field [1][2]. 3. It also attempts to identify and populate the self.error_details attribute by checking for common error detail keys such as detail, details, errors, or message [1][2]. 4. If the content is not valid JSON or the expected keys are missing, it falls back to the default self.resp.reason provided by the underlying HTTP response [1][2]. Usage Context: You should not typically call _get_reason directly in your application code, as it is intended for internal use by the HttpError class to populate its attributes [1][2]. Historically, developers found that fields like error_details were not always reliably populated unless _get_reason or the repr method had been triggered [4]. In modern versions of the library, the HttpError class is designed to handle this extraction internally, often during initialization [1][4]. If you need to access specific error information, such as the machine-readable reason field often found in Google API error responses (e.g., 'accessNotConfigured'), the library does not expose a dedicated reason attribute [5]. Instead, the recommended approach is to parse the content attribute of the HttpError object yourself using the json module [5]: import json try: #... call your API method... except HttpError as e: error_data = json.loads(e.content) # Access the specific reason field from the API error structure api_reason = error_data.get('error', {}).get('errors', [{}]).get('reason') print(f"API Error Reason: {api_reason}")
Citations:
- 1: https://github.com/googleapis/google-api-python-client/blob/main/googleapiclient/errors.py
- 2: https://github.com/googleapis/google-api-python-client/blob/25fba648ea647b62f2a6edc54ae927c1ed381b45/googleapiclient/errors.py
- 3: https://googleapis.github.io/google-api-python-client/docs/epy/googleapiclient.errors.HttpError-class.html
- 4: https://www.github.com/googleapis/google-api-python-client/issues/1185
- 5: Expose HttpError.reason when available googleapis/google-api-python-client#662
Preserve Google error details before wrapping execute failures.
google_client.execute converts HttpError into a ValueError before _check_connection_impl sees it, so getattr(exc, 'reason', None)/_get_reason() cannot populate errorReason for live Gmail/Drive probe failures. Preserve the parsed Google error reason from exc.content before raising, or attach it to the replacement exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/tool_google_workspace/IInstance.py` around lines 106 - 114,
Update the Google client execute error-wrapping path so parsed Google error
details from HttpError.content are preserved when raising the replacement
ValueError, allowing _check_connection_impl to populate errorReason for
Gmail/Drive probe failures. Attach the extracted reason to the replacement
exception or otherwise retain it through the raise, while preserving the
existing error and connection failure behavior.
| if 'api' not in out['checked'] and out['connection_ok']: | ||
| out['connection_ok'] = 'unknown' | ||
| out['note'] = ( | ||
| 'Credential and scopes look correct; no API call was made, so this ' | ||
| 'does not confirm the service is reachable or enabled.' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not claim scopes were verified after skipped diagnostics.
The configuration block can fail silently, and token scope validation only runs for a non-empty user token. This branch can still emit “Credential and scopes look correct”; track whether those checks actually completed before producing that note.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/tool_google_workspace/IInstance.py` around lines 137 - 142,
Update the diagnostic state used by the validation flow in IInstance so the
“Credential and scopes look correct” note is emitted only when credential and
scope checks actually completed successfully. Track completion explicitly,
including the non-empty user-token scope validation and configuration checks,
then use that state in the branch handling skipped API diagnostics instead of
claiming verification from connection_ok alone.
anandray
left a comment
anandray
left a comment
There was a problem hiding this comment.
Verdict: approve. Correct root-cause fix. One backward-compat note on connection_ok type worth surfacing.
What's good
- Root cause named precisely:
connection_okwas three local checks (client constructed, probe optional, claimed-scope inspection) presented as a single truth. Every reader (including Josh, who spent an hour re-authenticating) took it to mean "the API works." The gap between "we hold a plausible credential" and "the service is reachable" is a semantic mismatch, not a client bug — and this fix names both halves rather than picking a side. checked: [...]is the right primitive. Distinguishing assumed from measured is a shape that should exist in more of our diagnostic outputs. Reader can trustconnection_okmore when"api"is inchecked, and knows to treat it as an assumption when it isn't.unknown(nottrue, notfalse) when no live call was made. More honest than either alternative. A green light nobody checked is worse than no light because it redirects debugging effort — precisely what happened in #1694.errorReasonsurfacing Google's specific reason (accessNotConfigured,forbidden,rateLimitExceeded). That word alone names the fix; losing it costs the reader the diagnosis. Extraction handles both public.reasonand private._get_reason()shapes, which is a real thing google-api-python-client does.- Sheets/docs deliberately unprobed with the reasoning stated: a fabricated id returns 404 on a healthy API, and shipping a check that reports "broken" for a working service would be worse than the bug being fixed. That instinct — remove the probe rather than ship a false negative — is exactly right. False positive → false negative isn't a fix, it's the same disease pointing the other way.
- Length-caps (
[:200],[:120]) on error text prevent log-line explosion. - Honest disclosure that no live Google call was made in the author's environment. First real
check_connectionexercises it; alternative (leaving the bug because no live env) is worse.
Backward compat worth flagging
connection_ok was always bool; it can now be str ('unknown').
The narrow path: previously True becomes 'unknown' only when no probe was supplied and the credential-check passed. Callers doing if response['connection_ok']: still get truthy (non-empty string), so most existing code keeps working. But:
- Anyone doing
assert response['connection_ok'] is Trueorconnection_ok == Truefails. - Anyone deserialising into a strict-typed struct (dataclass, pydantic with
booltype) fails. - Any downstream logic that branches on
connection_ok == False(rather than falsy) treats'unknown'as "ok" — which is closer to correct than the old behaviour, but a semantic change nonetheless.
Worth grepping connection_ok across all downstream consumers (both server and saas) before merge to see if any callers do strict-bool checks. If nothing does, we're fine. If anything does, it either wants updating to in (True, 'unknown') or wants the new value as None/omitted instead of 'unknown'. Weakly prefer 'unknown' as-is because it's discoverable in JSON output.
Nits
- Type union
bool | 'unknown'onconnection_okisn't typed in the codebase (Python loose), but if there's a client library that types this response, worth updating in the same PR to catch consumer breakage at build time. getattr(exc, '_get_reason', lambda: None)()relying on a private method is fragile across google-api-python-client versions. Fine as a fallback after the public.reason, but worth a note if that library is ever version-pinned differently.
Approving.
… breaks
Anand asked for a grep for strict `connection_ok == True` comparisons before
merge. Ran the tests instead of grepping, which found more than the grep would
have: three failures, all caused by this PR.
sheets/test_sheets.py::test_check_connection_reports_ok
docs/test_docs.py::test_check_connection_reports_ok
AssertionError: assert 'unknown' is True
Both assert True where the new, intentional answer is 'unknown' — sheets and
docs make no API probe, so a client-and-scopes check cannot see a disabled API,
an exhausted quota, disabled billing or an org-policy block. Reporting True
there is precisely the false green this PR removes. Updated to assert
'unknown', that 'api' is absent from checked, and that the note is present.
Renamed to test_check_connection_reports_unknown_without_api_probe — a test
called "reports_ok" that asserts "unknown" is a trap for the next reader.
drive/test_drive.py::test_check_connection_reports_ok
assert False is True
This one is a real defect in the PR, not a stale expectation: drive gained an
about().get probe, FakeDrive has no about(), so the probe raised AttributeError
and the node reported connection_ok=False. Added the stub, and strengthened the
test to assert the probe actually ran — True is only honest if an API call
happened, so checking the boolean alone would have kept passing while the
diagnostic did nothing.
Answering the question Anand actually asked: outside IInstance.py, the only
consumers of connection_ok anywhere in the repo are these tests. No production
code, no SDK, no UI compares it. So the bool -> bool | 'unknown' widening is
contained.
Worth noting why this needed running rather than reading: PR CI here does not
execute unit tests — the os-matrix `./builder test` lane is a monthly cron. All
three failures would have merged green and surfaced weeks later.
345 passed, 20 skipped across test/tool_google_workspace.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP
2 participants
Josh hit this on the Gmail node. One response contained both:
Both lines are ours, and they contradict each other. He re-authenticated — which could never have helped — because the tool told him the connection was fine.
Why it passed
connection_okcame from three things, none of which touch the API:probeis optional and this service supplied none;token_scope_reportinspects the scopes the token claims — local.So it answered "do we hold a plausible credential" while every reader takes it as "this node works". A disabled API is invisible to all three — as are an exhausted quota, disabled billing, and an org-policy block.
Changes
checked: [...]— reports what was actually exercised, so a verified answer is distinguishable from an assumed one.connection_ok: unknown(with a note) when no live call was made, instead oftrue. A green light nobody checked is worse than no light, because it redirects the debugging effort — precisely what happened here.errorReasonsurfaces Googles reason verbatim:accessNotConfigured(API disabled) vsforbidden(permission) vsrateLimitExceeded. That one word names the fix.users().getProfile(userId=me); drive → probesabout().get(fields=user). (calendar already probedcalendarList().list.)sheets and docs deliberately get no probe
Both would need a real spreadsheet/document id. Probing with a fabricated one returns 404 on a perfectly healthy API — a false negative, which is the same disease pointing the other way. I wrote those probes first, then removed them: shipping a check that reports "broken" for a working service would have been worse than the bug being fixed.
They now honestly report
unknown. Giving them a genuine probe means a caller-supplied id or a create-then-delete, neither of which belongs in a read-only diagnostic.Verification
executeconfirmed present in the.clientimport list of both gmail and drive — checked rather than assumed, since a missing import wouldNameErroronly at call time. Calendars existing probe was the pattern.Not verified: a live call against Google. There are no credentials in my environment, so the probes behaviour on a real 403 is unexercised — the first real
check_connectionis the actual test.Closes #1694.
Summary by CodeRabbit
connection_okasunknownand includes a clarifying note; errors may include more specific reasons.