feat(connectors): Google Sheet connector - #18
Merged
Conversation
Second connector per MAGIC_MONTH1_TASK_LIST.md (magic-business): reads
and writes Order/Customer data in a Google Sheet, for shops without a
dedicated order-management system yet.
- Auth via Google service account (JWT bearer through google-auth);
the actual Sheets API v4 calls go through httpx like the Zalo
connector, so credential refresh uses a small httpx-based adapter
for google.auth.transport.Request instead of pulling in the separate
`requests` library just for that one call.
- Operations: read_range, update_range, append_row, batch_update
- Common Schema mapping is header-name-based (Customer/Order), with
Order.items round-tripped as a JSON string in an items_json column
since sheets have no nested types
- Error mapping follows Google's standard {error: {code, status,
message}} shape: RESOURCE_EXHAUSTED -> RateLimitError, 5xx/UNAVAILABLE/
INTERNAL -> TransientError (retried), everything else -> PermanentError
- 13 tests (auth mocked, no real RSA key needed; HTTP mocked via respx)
connectors/README.md updated to reflect google_sheet is implemented.
📝 WalkthroughWalkthroughChangesGoogle Sheet connector
Zalo error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GoogleSheetConnector
participant GoogleAuth
participant GoogleSheetsAPI
GoogleSheetConnector->>GoogleAuth: refresh service-account token when needed
GoogleSheetConnector->>GoogleSheetsAPI: execute authenticated spreadsheet operation
GoogleSheetsAPI-->>GoogleSheetConnector: return spreadsheet data or error
GoogleSheetConnector-->>GoogleSheetConnector: classify errors and retry transient failures
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Google Sheet Connector to read and write orders and customers via the Google Sheets API v4, complete with documentation, configuration examples, and comprehensive unit tests. The feedback focuses on improving the robustness and error handling of the connector. Key recommendations include replacing runtime assertions with explicit checks, handling Google authentication RefreshError and 401 Unauthorized statuses to allow proper retries, preventing resource leaks during connection failures, adding defensive type and value parsing checks, and supporting Pydantic models in the schema mapping methods.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+110
to
+116
| assert self._client is not None, "call connect() first" | ||
| token = await self._ensure_token() | ||
| headers = {"Authorization": f"Bearer {token}"} | ||
| try: | ||
| resp = await self._client.request(method, url, headers=headers, **kwargs) | ||
| except httpx.RequestError as e: | ||
| raise TransientError(f"network error calling Google Sheets API: {e}") from e |
There was a problem hiding this comment.
Using assert for runtime validation can be problematic because assertions are stripped when Python is run with optimization flags (-O). Replace the assertion with an explicit check and raise PermanentError. Additionally, network errors during token refresh will raise RefreshError (from google.auth.exceptions), which is not caught by httpx.RequestError. Catching RefreshError and wrapping it in TransientError ensures that temporary network blips during credential refresh are properly retried.
Suggested change
| assert self._client is not None, "call connect() first" | |
| token = await self._ensure_token() | |
| headers = {"Authorization": f"Bearer {token}"} | |
| try: | |
| resp = await self._client.request(method, url, headers=headers, **kwargs) | |
| except httpx.RequestError as e: | |
| raise TransientError(f"network error calling Google Sheets API: {e}") from e | |
| if self._client is None: | |
| raise PermanentError("Connector not connected. Call connect() first.") | |
| from google.auth.exceptions import RefreshError | |
| try: | |
| token = await self._ensure_token() | |
| headers = {"Authorization": f"Bearer {token}"} | |
| resp = await self._client.request(method, url, headers=headers, **kwargs) | |
| except RefreshError as e: | |
| raise TransientError(f"Google auth refresh failed: {e}") from e | |
| except httpx.RequestError as e: | |
| raise TransientError(f"network error calling Google Sheets API: {e}") from e |
Comment on lines
+121
to
+132
| try: | ||
| error = resp.json().get("error", {}) | ||
| except (json.JSONDecodeError, ValueError): | ||
| error = {} | ||
| status = error.get("status", "") | ||
| message = error.get("message", f"HTTP {resp.status_code}") | ||
|
|
||
| if resp.status_code == 429 or status in _RATE_LIMIT_STATUSES: | ||
| raise RateLimitError(f"Google Sheets API rate limited: {message}") | ||
| if resp.status_code >= 500 or status in _TRANSIENT_STATUSES: | ||
| raise TransientError(f"Google Sheets API transient error: {message}") | ||
| raise PermanentError(f"Google Sheets API error ({status or resp.status_code}): {message}") |
There was a problem hiding this comment.
If the access token expires or is revoked, the Google Sheets API will return a 401 Unauthorized status code. Currently, this is treated as a PermanentError, which prevents retries. Handling 401 by invalidating the token (self._credentials.token = None) and raising a TransientError allows the retry decorator to refresh the token and successfully retry the request.
Suggested change
| try: | |
| error = resp.json().get("error", {}) | |
| except (json.JSONDecodeError, ValueError): | |
| error = {} | |
| status = error.get("status", "") | |
| message = error.get("message", f"HTTP {resp.status_code}") | |
| if resp.status_code == 429 or status in _RATE_LIMIT_STATUSES: | |
| raise RateLimitError(f"Google Sheets API rate limited: {message}") | |
| if resp.status_code >= 500 or status in _TRANSIENT_STATUSES: | |
| raise TransientError(f"Google Sheets API transient error: {message}") | |
| raise PermanentError(f"Google Sheets API error ({status or resp.status_code}): {message}") | |
| try: | |
| error = resp.json().get("error", {}) | |
| except (json.JSONDecodeError, ValueError): | |
| error = {} | |
| status = error.get("status", "") | |
| message = error.get("message", f"HTTP {resp.status_code}") | |
| if resp.status_code == 401: | |
| self._credentials.token = None | |
| raise TransientError(f"Google Sheets API token expired or invalid: {message}") | |
| if resp.status_code == 429 or status in _RATE_LIMIT_STATUSES: | |
| raise RateLimitError(f"Google Sheets API rate limited: {message}") | |
| if resp.status_code >= 500 or status in _TRANSIENT_STATUSES: | |
| raise TransientError(f"Google Sheets API transient error: {message}") | |
| raise PermanentError(f"Google Sheets API error ({status or resp.status_code}): {message}") |
Comment on lines
+91
to
+94
| async def connect(self) -> bool: | ||
| self._client = httpx.AsyncClient(timeout=15.0) | ||
| await self._ensure_token() | ||
| return True |
There was a problem hiding this comment.
If _ensure_token() raises an exception during connect(), the newly created httpx.AsyncClient will not be closed, leading to a resource leak. Wrap the token initialization in a try...except block to ensure the client is closed if connection fails.
Suggested change
| async def connect(self) -> bool: | |
| self._client = httpx.AsyncClient(timeout=15.0) | |
| await self._ensure_token() | |
| return True | |
| async def connect(self) -> bool: | |
| self._client = httpx.AsyncClient(timeout=15.0) | |
| try: | |
| await self._ensure_token() | |
| except Exception: | |
| await self._client.aclose() | |
| self._client = None | |
| raise | |
| return True |
Comment on lines
+170
to
+172
| header: list[str] = raw_data["header"] | ||
| row: list[str] = raw_data["row"] | ||
| by_col = dict(zip(header, row + [""] * (len(header) - len(row)))) |
There was a problem hiding this comment.
Add defensive checks to ensure raw_data is a dictionary and contains the expected keys. This prevents unhandled TypeError or KeyError exceptions if the raw data format is malformed.
if not isinstance(raw_data, dict):
raise PermanentError("raw_data must be a dictionary")
header: list[str] = raw_data.get("header") or []
row: list[str] = raw_data.get("row") or []
by_col = dict(zip(header, row + [""] * (len(header) - len(row))))
Comment on lines
+187
to
+207
| if entity_type == "order": | ||
| items = [] | ||
| items_raw = by_col.pop("items_json", "") | ||
| if items_raw: | ||
| try: | ||
| items = json.loads(items_raw) | ||
| except (json.JSONDecodeError, ValueError): | ||
| items = [] | ||
| known = {f: by_col.pop(f, "") for f in _ORDER_FIELDS} | ||
| return { | ||
| "id": known["id"] or "", | ||
| "source_connector": self.name, | ||
| "external_id": known["id"] or "", | ||
| "customer_id": known["customer_id"] or None, | ||
| "status": known["status"] or "pending", | ||
| "items": items, | ||
| "total_amount": float(known["total_amount"] or 0), | ||
| "shipping_fee": float(known["shipping_fee"] or 0), | ||
| "created_at": known["created_at"] or None, | ||
| "metadata": by_col, | ||
| } |
There was a problem hiding this comment.
Ensure that items_json parses into a list of items rather than a dictionary or other JSON types, which would fail Pydantic validation later. Also, defensively handle ValueError when casting total_amount and shipping_fee to floats to prevent mapping failures due to malformed cell values.
if entity_type == "order":
items = []
items_raw = by_col.pop("items_json", "")
if items_raw:
try:
parsed = json.loads(items_raw)
if isinstance(parsed, list):
items = parsed
except (json.JSONDecodeError, ValueError):
pass
known = {f: by_col.pop(f, "") for f in _ORDER_FIELDS}
def _to_float(val: Any) -> float:
try:
return float(val) if val else 0.0
except ValueError:
return 0.0
return {
"id": known["id"] or "",
"source_connector": self.name,
"external_id": known["id"] or "",
"customer_id": known["customer_id"] or None,
"status": known["status"] or "pending",
"items": items,
"total_amount": _to_float(known["total_amount"]),
"shipping_fee": _to_float(known["shipping_fee"]),
"created_at": known["created_at"] or None,
"metadata": by_col,
}
Comment on lines
+211
to
+226
| def map_from_common_schema(self, common_data: dict[str, Any], entity_type: str) -> list[str]: | ||
| if entity_type == "customer": | ||
| fields = _CUSTOMER_FIELDS | ||
| elif entity_type == "order": | ||
| fields = [*_ORDER_FIELDS, "items_json"] | ||
| else: | ||
| raise PermanentError(f"GoogleSheetConnector cannot map entity_type={entity_type!r}") | ||
|
|
||
| row = [] | ||
| for field in fields: | ||
| if field == "items_json": | ||
| row.append(json.dumps(common_data.get("items", []))) | ||
| else: | ||
| value = common_data.get(field, "") | ||
| row.append("" if value is None else str(value)) | ||
| return row |
There was a problem hiding this comment.
Workflows often pass Pydantic models (like Customer or Order) instead of raw dictionaries to map_from_common_schema. Since Pydantic models do not have a .get() method, this will raise an AttributeError. Adding a check to convert Pydantic models to dictionaries makes the mapping robust. Additionally, ensure that items defaults to [] if it is None to avoid serializing it as "null".
def map_from_common_schema(self, common_data: dict[str, Any], entity_type: str) -> list[str]:
if not isinstance(common_data, dict):
if hasattr(common_data, "model_dump"):
common_data = common_data.model_dump()
elif hasattr(common_data, "dict"):
common_data = common_data.dict()
else:
raise PermanentError("common_data must be a dictionary or a Pydantic model")
if entity_type == "customer":
fields = _CUSTOMER_FIELDS
elif entity_type == "order":
fields = [*_ORDER_FIELDS, "items_json"]
else:
raise PermanentError(f"GoogleSheetConnector cannot map entity_type={entity_type!r}")
row = []
for field in fields:
if field == "items_json":
row.append(json.dumps(common_data.get("items") or []))
else:
value = common_data.get(field, "")
row.append("" if value is None else str(value))
return row![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
🧹 Nitpick comments (2)
connectors/google_sheet/connector.py (2)
123-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant exception class.
In Python,
json.JSONDecodeErrorinherits fromValueError. CatchingValueErroris sufficient to handle both json parsing errors and arbitrary invalid strings (like parsing floats).
connectors/google_sheet/connector.py#L123-L124: Simplify theexceptblock to justValueError.connectors/google_sheet/connector.py#L193-L194: Simplify theexceptblock to justValueError.♻️ Proposed refactor
- except (json.JSONDecodeError, ValueError): + except ValueError:🤖 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 `@connectors/google_sheet/connector.py` around lines 123 - 124, In connectors/google_sheet/connector.py, update both exception handlers at lines 123-124 and 193-194 to catch only ValueError, removing the redundant json.JSONDecodeError entry while preserving the existing error handling behavior.Source: Linters/SAST tools
118-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer checking
is_successfor HTTP responses.While Google Sheets API heavily uses
200 OK, it is safer and more idiomatic to check for any successful 2xx status code when working with REST APIs.♻️ Proposed refactor
- if resp.status_code == 200: + if resp.is_success: return resp.json() if resp.content else {}🤖 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 `@connectors/google_sheet/connector.py` around lines 118 - 119, Update the HTTP response status check in the Google Sheets request flow to use the response’s is_success property instead of comparing status_code to 200, while preserving the existing JSON-or-empty-dictionary return behavior.
🤖 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 `@connectors/google_sheet/connector.py`:
- Around line 111-117: Update _request so self._ensure_token() executes inside
the existing try block, allowing token-refresh httpx.RequestError exceptions to
be mapped to TransientError and handled by with_retry. Import and catch
GoogleAuthError in the same method, mapping authentication failures according to
the connector’s established auth-error handling instead of allowing them to
escape unhandled.
In `@connectors/google_sheet/README.md`:
- Around line 8-13: Update the setup instructions in the README around the
config copy and service-account JSON steps to explicitly warn that config.yaml
and the JSON key contain secrets and must not be committed or exposed. Recommend
using service_account_file or a secret manager instead of embedding the key in
config.yaml where possible.
- Around line 17-19: Update the setup and test command examples in the
google_sheet README so they use one consistent working directory. Add an
explicit change into connectors/google_sheet before relative commands, or
convert every affected install, test, and import-related path to be valid from
the repository root; apply the same correction to the sections referenced by the
comment.
- Around line 24-28: Update the Google Sheets connector documentation around the
header example and append_row usage to state that write columns must remain in
the canonical field order emitted by map_from_common_schema, or revise
serialization to use the sheet’s header order. Ensure the documented behavior
prevents reordered columns from receiving mismatched values, including the
additional affected examples.
---
Nitpick comments:
In `@connectors/google_sheet/connector.py`:
- Around line 123-124: In connectors/google_sheet/connector.py, update both
exception handlers at lines 123-124 and 193-194 to catch only ValueError,
removing the redundant json.JSONDecodeError entry while preserving the existing
error handling behavior.
- Around line 118-119: Update the HTTP response status check in the Google
Sheets request flow to use the response’s is_success property instead of
comparing status_code to 200, while preserving the existing
JSON-or-empty-dictionary return 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a941c6f-b02a-416b-8513-dad5eff64753
📒 Files selected for processing (9)
connectors/README.mdconnectors/google_sheet/README.mdconnectors/google_sheet/__init__.pyconnectors/google_sheet/config.example.yamlconnectors/google_sheet/connector.pyconnectors/google_sheet/pytest.iniconnectors/google_sheet/requirements.txtconnectors/google_sheet/tests/__init__.pyconnectors/google_sheet/tests/test_connector.py
Fixes the SonarCloud "C Reliability Rating" gate failure and Gemini/ CodeRabbit review comments on PR #18: - Replace `assert self._client is not None` with an explicit check + PermanentError — assertions are stripped under `python -O`, so this was flagged as a real reliability bug, not just style - connect(): close and clear the httpx client if token init fails during connect(), instead of leaking it - _request(): catch GoogleAuthError (covers RefreshError and friends) and raise AuthError — a revoked/invalid service account key isn't fixed by retrying, so it shouldn't go through with_retry; genuine network errors during token refresh still flow through the existing httpx.RequestError -> TransientError path since token fetch now runs inside the same try block as the request - _request(): treat HTTP 401 as TransientError and clear the cached token so the retry is forced to refresh, instead of failing permanently on a token that expired mid-flight - map_to_common_schema: reject non-dict raw_data, only accept a JSON list (not object/other) for items_json, and parse total_amount/ shipping_fee defensively instead of letting a malformed cell crash the whole mapping - map_from_common_schema: accept Pydantic models (via model_dump/dict) in addition to plain dicts, default items to [] instead of "null" - Simplify redundant `except (json.JSONDecodeError, ValueError)` to just ValueError (JSONDecodeError is already a ValueError subclass); use resp.is_success instead of `== 200` - README: warn that config.yaml/service-account key are secrets and shouldn't be committed, use repo-root-relative paths consistently, and document that map_from_common_schema writes in a fixed column order (reordering sheet columns will misalign writes) 9 new/updated tests covering the above; all passing (22 total in this connector, 36 in sdk/python, 22 in zalo — none affected).
Root cause of the lingering "C Reliability Rating on New Code" gate on PR #18, which persisted even after the Google Sheet connector's own redundant excepts were fixed in 89d2db5. UnicodeDecodeError and json.JSONDecodeError are both subclasses of ValueError, so listing them alongside ValueError in the same except tuple is dead weight — SonarCloud rule python:S5713, which it classifies as a Bug (Reliability), not a code smell. It's the same rule SonarCloud flagged on PR #17 (connector.py:163) before that PR merged. Because the Zalo connector landed on main today (PR #17), it still falls inside SonarCloud's New Code window, so its issues counted against PR #18's quality gate even though PR #18 doesn't touch those files. Behavior is unchanged: the same exceptions are still caught, just via their common base class. Left sdk/python/magic_ai_sdk/worker.py:131 (same pattern) untouched on purpose — it's long-standing code outside this PR's scope, and editing it would pull the whole file into Sonar's new-code window. Tests: zalo 22, google_sheet 22, sdk/python 36 — all passing.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
connectors/google_sheet/tests/test_connector.py (2)
225-235: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify that connection failure awaits
aclose().
_client is Nonedoes not prove the HTTP client was closed. Instrument theAsyncClientwith anAsyncMockand assertaclose()was awaited exactly once.🤖 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 `@connectors/google_sheet/tests/test_connector.py` around lines 225 - 235, Update test_connect_failure_closes_client_and_clears_it to replace the connector’s HTTP client with an instrumented AsyncClient using an AsyncMock for aclose(). After the expected connect failure, assert that aclose() was awaited exactly once, while retaining the existing assertion that conn._client is None.
214-223: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert the connector’s retry boundaries, not only final exceptions.
Both tests can pass with incorrect retry behavior:
connectors/google_sheet/tests/test_connector.py#L214-L223: verify a 401 triggers another request after token refresh.connectors/google_sheet/tests/test_connector.py#L243-L251: verifyRefreshErrorcauses exactly one refresh attempt and no retry.🤖 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 `@connectors/google_sheet/tests/test_connector.py` around lines 214 - 223, Strengthen the retry-boundary tests in connectors/google_sheet/tests/test_connector.py:214-223 and connectors/google_sheet/tests/test_connector.py:243-251. In test_401_response_clears_token_and_raises_transient, assert that token refresh is followed by a second request before the final TransientError; in the RefreshError test, assert exactly one refresh attempt and no retry request. Use the existing request mocks and connector symbols without changing production behavior.
🤖 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.
Nitpick comments:
In `@connectors/google_sheet/tests/test_connector.py`:
- Around line 225-235: Update test_connect_failure_closes_client_and_clears_it
to replace the connector’s HTTP client with an instrumented AsyncClient using an
AsyncMock for aclose(). After the expected connect failure, assert that aclose()
was awaited exactly once, while retaining the existing assertion that
conn._client is None.
- Around line 214-223: Strengthen the retry-boundary tests in
connectors/google_sheet/tests/test_connector.py:214-223 and
connectors/google_sheet/tests/test_connector.py:243-251. In
test_401_response_clears_token_and_raises_transient, assert that token refresh
is followed by a second request before the final TransientError; in the
RefreshError test, assert exactly one refresh attempt and no retry request. Use
the existing request mocks and connector symbols without changing production
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 244130b0-807b-422c-b2f0-b145729e8c9c
📒 Files selected for processing (5)
connectors/google_sheet/README.mdconnectors/google_sheet/connector.pyconnectors/google_sheet/tests/test_connector.pyconnectors/zalo/connector.pyconnectors/zalo/webhook.py
🚧 Files skipped from review as they are similar to previous changes (2)
- connectors/google_sheet/README.md
- connectors/google_sheet/connector.py
kienbui1995
pushed a commit
that referenced
this pull request
Jul 25, 2026
Tuần 2 of the Month-1 roadmap: the customer-service pipeline for Vietnamese
online shops.
Nhận tin -> Classify Intent -> Extract Order Info -> Fetch Order Status
-> Draft Response -> Guardrail Check -> Send hoặc Handoff
packs/ecomops/ (new):
- text.py — Vietnamese normalization. Customers routinely type without
diacritics, so both the message and the keyword lists are folded to a
diacritics-free form; "đơn hàng" and "don hang" match the same rule. đ/Đ
are handled explicitly since they don't decompose under NFD.
- intents.py — 7 EcomOps intents. RuleBasedIntentClassifier matches keyword
tokens in order allowing one filler word between them, because Vietnamese
slots pronouns inside fixed phrases ("khi nào EM nhận được hàng" must still
match "khi nao nhan"). A greeting only wins when it's the whole message, so
"chào shop, đơn em đâu" classifies as order_status. LLMIntentClassifier goes
through MagiC's own /api/v1/llm/chat gateway (cheapest routing) rather than a
provider SDK, so spend lands in the Cost Controller. HybridIntentClassifier
only pays for the LLM when the rules aren't confident, and keeps the rule
guess if the gateway is down.
- extraction.py — order code + phone. Phones are masked out before the bare
numeric order-code pattern runs, otherwise "sdt 0901234567" returns the phone
as the order id and every lookup silently misses.
- guardrails.py — an LLM draft is not automatically safe to send from a shop's
official account. Blocks leaked third-party contacts, hard delivery
guarantees, unapproved refunds/vouchers, and system-prompt/credential leaks.
Runs on normalized text so it can't be evaded by dropping diacritics.
- prompts.py — Vietnamese prompt library, kept as data so shops can retune tone
without touching pipeline code.
- workflow.py — the pipeline. Every dependency is injected, so it makes no
network calls and knows nothing about Zalo or Sheets. A blocked draft is
never returned to the caller: reply falls back to a safe message and action
becomes "handoff". Complaints skip drafting entirely.
- example_worker.py — end-to-end demo (Zalo webhook -> Sheet lookup -> guarded
reply), which is the stated Tuần 2 deliverable.
CI (this is the part that was quietly broken): the python job only ever ran
sdk/python. The 44 connector tests added in #17/#18 have never executed on CI,
and neither would these 76. Running `pytest connectors packs` from the root
failed outright because asyncio_mode lived only in per-directory pytest.ini
files — hence the new root pytest.ini. ruff was also weaker outside sdk/python
(E501/I aren't in ruff's default rule set), so a root ruff.toml now mirrors
sdk/python's settings; the 4 import-order issues that surfaced are fixed.
Tests: 76 new (packs) + 44 (connectors) + 36 (sdk) = 156, all passing. Go core
untouched, builds clean.
3 tasks
kienbui1995
added a commit
that referenced
this pull request
Jul 25, 2026
* feat(packs): EcomOps workflow + run connector/pack tests in CI
Tuần 2 of the Month-1 roadmap: the customer-service pipeline for Vietnamese
online shops.
Nhận tin -> Classify Intent -> Extract Order Info -> Fetch Order Status
-> Draft Response -> Guardrail Check -> Send hoặc Handoff
packs/ecomops/ (new):
- text.py — Vietnamese normalization. Customers routinely type without
diacritics, so both the message and the keyword lists are folded to a
diacritics-free form; "đơn hàng" and "don hang" match the same rule. đ/Đ
are handled explicitly since they don't decompose under NFD.
- intents.py — 7 EcomOps intents. RuleBasedIntentClassifier matches keyword
tokens in order allowing one filler word between them, because Vietnamese
slots pronouns inside fixed phrases ("khi nào EM nhận được hàng" must still
match "khi nao nhan"). A greeting only wins when it's the whole message, so
"chào shop, đơn em đâu" classifies as order_status. LLMIntentClassifier goes
through MagiC's own /api/v1/llm/chat gateway (cheapest routing) rather than a
provider SDK, so spend lands in the Cost Controller. HybridIntentClassifier
only pays for the LLM when the rules aren't confident, and keeps the rule
guess if the gateway is down.
- extraction.py — order code + phone. Phones are masked out before the bare
numeric order-code pattern runs, otherwise "sdt 0901234567" returns the phone
as the order id and every lookup silently misses.
- guardrails.py — an LLM draft is not automatically safe to send from a shop's
official account. Blocks leaked third-party contacts, hard delivery
guarantees, unapproved refunds/vouchers, and system-prompt/credential leaks.
Runs on normalized text so it can't be evaded by dropping diacritics.
- prompts.py — Vietnamese prompt library, kept as data so shops can retune tone
without touching pipeline code.
- workflow.py — the pipeline. Every dependency is injected, so it makes no
network calls and knows nothing about Zalo or Sheets. A blocked draft is
never returned to the caller: reply falls back to a safe message and action
becomes "handoff". Complaints skip drafting entirely.
- example_worker.py — end-to-end demo (Zalo webhook -> Sheet lookup -> guarded
reply), which is the stated Tuần 2 deliverable.
CI (this is the part that was quietly broken): the python job only ever ran
sdk/python. The 44 connector tests added in #17/#18 have never executed on CI,
and neither would these 76. Running `pytest connectors packs` from the root
failed outright because asyncio_mode lived only in per-directory pytest.ini
files — hence the new root pytest.ini. ruff was also weaker outside sdk/python
(E501/I aren't in ruff's default rule set), so a root ruff.toml now mirrors
sdk/python's settings; the 4 import-order issues that surfaced are fixed.
Tests: 76 new (packs) + 44 (connectors) + 36 (sdk) = 156, all passing. Go core
untouched, builds clean.
* fix(ecomops): canonicalize VN phone forms in the leaked-contact guardrail
Found while PR #19's CI was running: a shop that configures its hotline as
"+84987654321" (or "84987654321") had its OWN number flagged as a leaked
third-party contact, so any reply quoting the hotline was blocked and pushed to
a human. The rule compared raw digit strings, so the +84 and 0-prefixed
spellings of the same number never matched.
Adds `canonical_phone()` — folds "+84 987-654-321", "84987654321",
"987654321" and "0987-654-321" all to "0987654321" — and compares canonical
forms on both sides. Detection is unaffected: a genuinely foreign number is
still blocked (covered by a new test that allowlists the hotline and checks a
different number is still caught).
Also NOSONAR the `pip install ruff` lint line. Editing that line to widen the
lint scope made SonarCloud treat it as new code and re-apply the same two
supply-chain hotspots (S4830/S5445) already suppressed on the install lines
above it — that's what dropped this PR's Security Rating to C.
Tests: 14 new (canonical_phone table + hotline spellings + allowlist scoping),
170 total, all passing.
---------
Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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 does this PR do?
Second connector per the Month-1 roadmap (
MAGIC_MONTH1_TASK_LIST.md/ magic-business technical-planning, Tuần 2): reads and writesOrder/Customerdata in a Google Sheet, for shops that don't have a dedicated order-management system yet.connectors/google_sheet/—GoogleSheetConnectorimplementingBaseConnector. Auth via Google service account (JWT bearer throughgoogle-auth); the actual Sheets API v4 calls go throughhttpx(same as the Zalo connector), so credential refresh uses a small httpx-based adapter forgoogle.auth.transport.Requestinstead of pulling in the separaterequestslibrary just for that one call.read_range,update_range,append_row,batch_update.Customer/Order;Order.itemsround-trips as a JSON string in anitems_jsoncolumn since sheets have no nested types.{error: {code, status, message}}shape:RESOURCE_EXHAUSTED→RateLimitError, 5xx/UNAVAILABLE/INTERNAL→TransientError(retried viawith_retry), everything else →PermanentError.connectors/README.mdupdated to markgoogle_sheetas implemented (was "sắp có").No changes to Go core or the previously-merged connector framework (
sdk/python/magic_ai_sdk/connectors/).Related issue
Tracks Month 1, Tuần 2 (Google Sheet Connector) of the technical roadmap in magic-business/technical-planning.
Checklist
cd core && go build ./...(Go core untouched, builds clean)connectors/google_sheet/tests/test_connector.py), auth mocked (no real RSA key needed), HTTP mocked via respx, all passingconnectors/google_sheet/README.md(Vietnamese setup guide) +connectors/README.mdGenerated by Claude Code
Summary by CodeRabbit