From 2e16ee1034c937c3a354627cbeaf0c35b3c641c2 Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Tue, 28 Jul 2026 07:46:24 -0700 Subject: [PATCH 1/2] fix(google-workspace): stop reporting connection_ok when nothing was checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../nodes/tool_google_workspace/IInstance.py | 24 +++++++++++++++++++ .../tool_google_workspace/drive/IInstance.py | 2 +- .../tool_google_workspace/gmail/IInstance.py | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/nodes/src/nodes/tool_google_workspace/IInstance.py b/nodes/src/nodes/tool_google_workspace/IInstance.py index 5a1ebbfe7..070f720f0 100644 --- a/nodes/src/nodes/tool_google_workspace/IInstance.py +++ b/nodes/src/nodes/tool_google_workspace/IInstance.py @@ -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] try: cfg = Config.getNodeConfig(self.IGlobal.glb.logicalType, self.IGlobal.glb.connConfig) auth_type = (cfg.get('authType') or 'service').strip() @@ -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.' + ) return out diff --git a/nodes/src/nodes/tool_google_workspace/drive/IInstance.py b/nodes/src/nodes/tool_google_workspace/drive/IInstance.py index 5267e4933..112d7a178 100644 --- a/nodes/src/nodes/tool_google_workspace/drive/IInstance.py +++ b/nodes/src/nodes/tool_google_workspace/drive/IInstance.py @@ -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 diff --git a/nodes/src/nodes/tool_google_workspace/gmail/IInstance.py b/nodes/src/nodes/tool_google_workspace/gmail/IInstance.py index 2b7e27534..2b3bbbb25 100644 --- a/nodes/src/nodes/tool_google_workspace/gmail/IInstance.py +++ b/nodes/src/nodes/tool_google_workspace/gmail/IInstance.py @@ -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'))) # ======================================================================= # MESSAGES — read From df5b02c41578bbc29ba372904ca30758645645fe Mon Sep 17 00:00:00 2001 From: Dmitrii Kataraev Date: Tue, 28 Jul 2026 10:30:15 -0700 Subject: [PATCH 2/2] test(tool_google_workspace): fix the three tests this PR's own change breaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP --- nodes/test/tool_google_workspace/docs/test_docs.py | 9 +++++++-- nodes/test/tool_google_workspace/drive/test_drive.py | 11 +++++++++++ .../test/tool_google_workspace/sheets/test_sheets.py | 8 ++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/nodes/test/tool_google_workspace/docs/test_docs.py b/nodes/test/tool_google_workspace/docs/test_docs.py index c05860e76..f9e4433d1 100644 --- a/nodes/test/tool_google_workspace/docs/test_docs.py +++ b/nodes/test/tool_google_workspace/docs/test_docs.py @@ -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']) diff --git a/nodes/test/tool_google_workspace/drive/test_drive.py b/nodes/test/tool_google_workspace/drive/test_drive.py index 9a5dc5f64..d79398608 100644 --- a/nodes/test/tool_google_workspace/drive/test_drive.py +++ b/nodes/test/tool_google_workspace/drive/test_drive.py @@ -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) @@ -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']) diff --git a/nodes/test/tool_google_workspace/sheets/test_sheets.py b/nodes/test/tool_google_workspace/sheets/test_sheets.py index d42ecf531..d69ac7f38 100644 --- a/nodes/test/tool_google_workspace/sheets/test_sheets.py +++ b/nodes/test/tool_google_workspace/sheets/test_sheets.py @@ -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'])