Skip to content

fix(google-workspace): stop reporting connection_ok when nothing was checked #1707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kwit75 wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix(google-workspace): stop reporting connection_ok when nothing was checked #1707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e16ee1
fix(google-workspace): stop reporting connection_ok when nothing was …
Jul 28, 2026
df5b02c
test(tool_google_workspace): fix the three tests this PR's own change…
Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions nodes/src/nodes/tool_google_workspace/IInstance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,27 @@ def _check_connection_impl(self, probe: 'Callable[[Any], None] | None' = None) -
'connection_ok': self._svc() is not None,
'access': getattr(access, 'tier', None),
'requiredScopes': list(getattr(access, 'scopes', []) or []),
# What was actually exercised, so a caller can tell a verified
# answer from an assumed one. Without this, `connection_ok: true`
# meant "we hold a plausible credential" while everyone read it as
# "this node works" — see #1694, where it sat next to a 403 saying
# the API was disabled and sent the reporter off re-authenticating.
'checked': ['client', 'scopes'],
}
service = self._svc()
if probe is not None and service is not None:
try:
probe(service)
out['checked'].append('api')
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]
Comment on lines 106 to +114

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.py

Repository: 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)))
PY

Repository: 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}")
PY

Repository: 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:

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.

try:
cfg = Config.getNodeConfig(self.IGlobal.glb.logicalType, self.IGlobal.glb.connConfig)
auth_type = (cfg.get('authType') or 'service').strip()
Expand All @@ -116,4 +129,15 @@ def _check_connection_impl(self, probe: 'Callable[[Any], None] | None' = None) -
out['error'] = f'invalid user token data: {str(exc)[:160]}'
except Exception:
pass # config lookup diagnostics must never raise

# No live call was made, so `connection_ok` here only means the
# credential looks usable. Say so rather than letting it read as proof:
# a disabled API, an exhausted quota, disabled billing and an org-policy
# block are all invisible to a client-and-scopes check.
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.'
)
Comment on lines +137 to +142

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

return out
2 changes: 1 addition & 1 deletion nodes/src/nodes/tool_google_workspace/drive/IInstance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def _files(self):
)
def check_connection(self, args: dict) -> dict:
"""Check Drive connection status and whether granted OAuth scopes cover the access tier. Read-only."""
return self._check_connection_impl()
return self._check_connection_impl(probe=lambda s: execute(s.about().get(fields='user')))

# =======================================================================
# FILES — read
Expand Down
2 changes: 1 addition & 1 deletion nodes/src/nodes/tool_google_workspace/gmail/IInstance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def _id_list(args: dict, key: str, op: str) -> list[str]:
)
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')))

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 and unknown.
  • 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.py

Repository: 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/gmail

Repository: 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 -n

Repository: 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:

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


# =======================================================================
# MESSAGES — read
Expand Down
9 changes: 7 additions & 2 deletions nodes/test/tool_google_workspace/docs/test_docs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,16 @@ def test_default_tier_is_write():
assert access.can_write is True


def test_check_connection_reports_ok():
def test_check_connection_reports_unknown_without_api_probe():
inst = _make()
out = inst.check_connection({})
assert isinstance(out, dict)
assert out['connection_ok'] is True
# 'unknown', not True: docs/sheets make no API probe, and a client-and-scopes
# check cannot see a disabled API, an exhausted quota, disabled billing or an
# org-policy block. Reporting True there is the false green this change removes.
assert out['connection_ok'] == 'unknown'
assert 'api' not in out['checked']
assert 'note' in out
assert out['access'] == 'write'
assert any('documents' in s for s in out['requiredScopes'])

Expand Down
11 changes: 11 additions & 0 deletions nodes/test/tool_google_workspace/drive/test_drive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,13 @@ def drives(self):
def changes(self):
return _Node(self, 'changes')

def about(self):
# check_connection's liveness probe calls about().get(fields='user').
# Without this the probe raises AttributeError and the node reports
# connection_ok=False — which is correct behaviour for an unreachable
# API, and exactly why the diagnostic must be exercised, not assumed.
return _Node(self, 'about')

def call_for(self, op):
"""Return the kwargs of the FIRST recorded call to terminal method ``op``."""
return next((kw for n, kw in self.calls if n == op), None)
Expand Down Expand Up @@ -727,6 +734,10 @@ def test_check_connection_reports_ok():
inst = _make()
out = inst.check_connection({})
assert out['connection_ok'] is True
# True is only honest when an API call actually happened; assert the probe
# ran rather than trusting the boolean on its own.
assert 'api' in out['checked']
assert inst.IGlobal.service.call_for('get') is not None
assert out['access'] == 'write'
assert any('drive' in s for s in out['requiredScopes'])

Expand Down
8 changes: 6 additions & 2 deletions nodes/test/tool_google_workspace/sheets/test_sheets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,14 @@ def test_default_tier_is_write():
# ---------------------------------------------------------------------------


def test_check_connection_reports_ok():
def test_check_connection_reports_unknown_without_api_probe():
inst = _make()
out = inst.check_connection({})
assert out['connection_ok'] is True
# 'unknown', not True: sheets makes no API probe, so the credential looking
# correct is not evidence the service is reachable. See IInstance.check_connection.
assert out['connection_ok'] == 'unknown'
assert 'api' not in out['checked']
assert 'note' in out
assert out['access'] == 'write'
assert any('spreadsheets' in s for s in out['requiredScopes'])

Expand Down
Loading