fix: stop Shared Drive-scoped change entries from poisoning the Google content-sync queue

[krishagel]

Summary

Fixes the prod alarm aistudio-prod-google-content-sync-oldest-message. Two connectors (b1429b0b-…, 1f870c92-…) had synced nothing since 2026-08-06 ~12:00 UTC, and 904 messages had accumulated in the DLQ.

Root cause

Google's Drive Changes feed (GET /drive/v3/changes) is not exclusively file-scoped. Entries with changeType: "drive" describe the Shared Drive itself — rename, membership change, restriction change — and carry no fileId.

driveChangeSchema in lib/repositories/google-drive/drive-client.ts declared fileId as a required z.string(), so changesListSchema.parse() threw a ZodError inside listChanges the moment such an entry appeared in a page. The throw happened before reconcileChanges could persist the cursor, so every SQS redelivery re-fetched the identical page and failed identically. The message never drained, its age breached the 30-minute alarm threshold, and after maxReceiveCount it landed in the DLQ — repeatedly.

Changes

lib/repositories/google-drive/drive-client.ts

• driveChangeSchema.fileId is now z.string().optional().
• Added changeType: z.string().optional() — deliberately a plain string and not a z.enum. An enum would re-poison the queue the first time Google ships a changeType value we did not anticipate.
• listChanges now requests changeType in its fields projection, so the API actually returns the discriminator the guard reads. Without this the projection would silently strip it.

infra/lambdas/google-content-sync/changes.ts (new)

• isFileScopedDriveChange() — decides whether a change entry names a file the sync path can act on. Returns false for changeType === "drive" and for a missing/empty fileId; it is a type predicate, so the caller gets fileId narrowed to string.
• isDriveRemovalChange() — true when a non-file-scoped entry reports removed. Deliberately not keyed on changeType === "drive", so the legacy "teamDrive" value and any future scope take this path too.
• Extracted into its own module because index.ts is not importable from the root Jest gate.

infra/lambdas/google-content-sync/index.ts

• processGoogleDriveChange() now guards first, before markSourceMissing() or any other side effect can run:
  • Drive metadata noise (rename, membership, restriction) → logged and skipped with return false. requiresSelectionSnapshot stays untouched, so reconcileChanges still persists the cursor for that page.
  • Drive removal (removed: true) → return true, requesting a selection snapshot. See below.
• Drive deletion and access loss remain covered by the existing 403/404 → markConnectorAccessLost path; that behaviour is unchanged.
• Extracted the catch-block outcome recording into recordDriveChangeFailure() — behaviour-identical, and it brings the function back under the complexity-15 lint ceiling that the new guard pushed it over.
• The skip/removal log line carries connectorId, changeType, driveId and time, so a spike in either can be correlated against Drive audit logs.

Review follow-up: don't swallow drive removals (Codex P1)

The first commit skipped every non-file-scoped entry unconditionally. That is right for the metadata noise the outage was about, but it also silently consumed removed: true — the entry Google emits when a Shared Drive is deleted or the account loses access.

That would have been a data-correctness bug. Sources imported from that drive become unreachable, and:

• a connector scoped to a shared drive would eventually surface the loss, because its next listChanges/watch call against that driveId returns 403/404 and markConnectorAccessLost fires;
• a connector reading the global changes feed would not — its later calls keep succeeding, so the stale content would stay active indefinitely with nothing to retire it.

Returning true sets requiresSelectionSnapshot, so reconcileChanges runs reconcileSelectionSnapshot → markUnseenSourcesMissing and retires whatever is no longer reachable. This reuses the same mechanism the pre-existing folder-mimeType path already uses rather than inventing a new one. The cursor advances on both branches — skips take the per-page persistSyncCursor, removals take the post-snapshot one — so the invariant the outage broke stays intact either way.

Verification

• Lint (eslint . --max-warnings 0): clean, zero warnings
• Typecheck: no new errors. One pre-existing error in infra/lambdas/agent-router/index.ts reproduces identically on the clean tree (verified via git stash) — unrelated AWS SDK type duplication
• Unit suite (bun run test:ci): 573 suites / 5434 tests passing. One pre-existing failure, tests/unit/agent-workspace-identity.test.ts, fails identically on the clean tree: @aws-sdk/client-ecs is imported by agent-router/index.ts but is absent from package.json and bun.lock entirely. Not touched by this PR
• Playwright E2E gate (pre-push hook): 317 passed, 41 skipped, 2 flaky-then-green (decision-semantic-search, nexus-projects — both unrelated)
• CI: all checks green

New tests

tests/unit/lib/repositories/google-drive-changes-feed.test.ts

• A mixed page containing a changeType: "drive" entry with no fileId plus a normal file entry now parses and returns both values (the exact shape that caused the outage).
• An unknown future changeType parses rather than throwing.
• The fields projection is asserted to request changeType.

tests/unit/google-content-sync-change-scope.test.ts

• Truth table for isFileScopedDriveChange: drive-scope wins even when a fileId is present; empty/missing fileId skipped; unknown changeType with a fileId processed; fileId type narrowing.
• Truth table for isDriveRemovalChange: drive-scoped removal flagged; "teamDrive", an unknown future scope and a changeType-less removal all covered; rename/membership noise ignored; a file-scoped removal explicitly not claimed, since that is the file path's job via markSourceMissing.
• Wiring assertions that the guard precedes the first side effect, that the removal signal is returned rather than swallowed, and that reconcileChanges persists the cursor on both branches.

Evidence

N/A — no UI surface. This is a backend Lambda + API-client path, not reachable via Playwright; covered by the unit/contract tests above.

Operational notes

• No alarm thresholds changed.
• Existing DLQ messages are intentionally not redriven or purged — they expire under the 14-day retention (~Aug 20).
• The affected connectors self-heal on their next webhook or scheduled tick once the Lambda is deployed. Deploy is manual.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e159156357

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review

Reviewed the diff, the full surrounding context of processGoogleDriveChange/reconcileChanges in infra/lambdas/google-content-sync/index.ts, and lib/repositories/google-drive/drive-client.ts. This is a solid, well-scoped fix.

Strengths

• Root cause correctly identified and fixed at the right layer. Loosening driveChangeSchema.fileId to optional (lib/repositories/google-drive/drive-client.ts:71) stops the ZodError that was killing the cursor advance before reconcileChanges could persist it — that's the actual poison-message mechanism, not just a symptom patch.
• changeType typed as a plain string, not an enum — good defensive choice per the PR description; an enum would just re-introduce the same class of bug the next time Google ships a new changeType.
• Fields projection updated to request changeType (drive-client.ts listChanges) — easy to miss; without it the guard in isFileScopedDriveChange would silently always see undefined and misclassify entries.
• Guard placed first, before any side effect, in processGoogleDriveChange (index.ts:1372) — confirmed by reading the surrounding code: markSourceMissing and the try/catch import path are both after the early return, and reconcileChanges (index.ts:1437) only sets requiresSelectionSnapshot from the return value, so a skip correctly leaves the cursor-persist path (if (!requiresSelectionSnapshot)) intact.
• Type predicate (change is T & { fileId: string }) is a nice touch — it lets the rest of processGoogleDriveChange use change.fileId as a plain string without a cast or non-null assertion.
• Verified there are no other consumers of driveChangeSchema/GoogleDriveChange in the codebase that assume fileId is always present (grep -rn "listChanges|driveChangeSchema|GoogleDriveChange\b" only hits these two files) — the optional-ification is safe.
• recordDriveChangeFailure extraction is behavior-preserving (checked line-by-line against the original inline catch block) and reasonably brings the function back under a complexity ceiling.
• Test coverage is good: the drive-client test exercises the actual listChanges parse path with a mixed page (drive-scoped + file-scoped), and the google-content-sync-change-scope test covers the full truth table for isFileScopedDriveChange, including the important edge case of an unknown future changeType still being processed when a fileId is present.

Minor notes (non-blocking)

• Source-text-based tests in google-content-sync-change-scope.test.ts (stripComments + indexOf order checks) can't verify real runtime behavior, only wiring order in the file — e.g. they wouldn't catch a bug in isFileScopedDriveChange's actual logic, only that it's called before markSourceMissing. That said, this matches an existing convention already used in several other test files in this repo (agent-cron-job-promotion.test.ts, agent-broker-route-allowlists.test.ts, etc.), apparently because infra/lambdas/google-content-sync/index.ts isn't importable from the root Jest gate — so this looks like a deliberate, precedented workaround rather than a new pattern, but worth keeping in mind as inherently more brittle than a behavioral test if the file is ever made importable.
• log.info("Skipped non-file Google Drive change entry", { connectorId, changeType }) doesn't include driveId or time from the change entry, which could be handy for correlating with Drive audit logs when debugging a spike in skipped entries. Not necessary, just a possible follow-up.
• Worth double-checking in the Google API docs whether changeType can also be "teamDrive" (the older/legacy alias for Shared Drive events) in any tenant still on the legacy naming — if so it'd currently fall through to the file-scoped branch and (correctly, per the fallback design) only get skipped if fileId is also absent. Given the plain-string-not-enum design this isn't a real risk, just flagging for awareness.

Security / Performance

No concerns — this is a parsing/control-flow fix internal to the Lambda's Drive Changes handling, no new external input surface, no new IAM/network access, and no change to hot-path performance characteristics (the guard is O(1) per change entry).

Test coverage

Good — both the schema-level parsing behavior and the handler-level control flow are covered, plus the regression scenario (mixed page with a changeType: "drive" entry) is directly represented in the new drive-client test.

Nice fix — clear root-cause writeup, targeted change, and the cursor-persistence invariant (the actual thing that was broken) is explicitly tested.

[claude]

Review

Solid, well-scoped fix. The root cause diagnosis matches the code: driveChangeSchema.fileId was a required z.string(), so any changeType: "drive" entry in a Changes-feed page threw inside listChanges before reconcileChanges could persist the cursor — guaranteeing the same page (and DLQ message) retried forever. Verified the actual code paths, not just the description:

Correctness

• isFileScopedDriveChange is placed as the first statement in processGoogleDriveChange (index.ts:1363-1391), before any markSourceMissing/importFileIsolated call — confirmed the guard genuinely precedes the first side effect, not just the meta-test's claim.
• Confirmed change.fileId is safely used as string after the negated-predicate early return (same function scope, no closure boundary, so TS narrowing holds) — no unsound cast needed.
• Traced requiresSelectionSnapshot/persistSyncCursor in reconcileChanges (index.ts:1450-1492): a skipped drive-metadata entry keeps the flag false so the per-page cursor persist still runs; a drive-removal entry sets it true, deferring the persist until after reconcileSelectionSnapshot. This matches the pre-existing folder-mimeType path, so the new branch reuses an already-proven mechanism rather than inventing a new one.
• Checked for other consumers of GoogleDriveChange/listChanges (grepped across lib/, infra/, app/) — processGoogleDriveChange is the only caller, and the one existing test for listChanges (google-drive-client.test.ts) doesn't rely on fileId being required, so the schema relaxation doesn't silently break another path.
• changeType as a plain z.string() rather than an enum, plus requesting it in the fields projection, is the right call — an enum would have reintroduced exactly the poison-message failure mode this PR fixes the moment Google ships a new value.

Nit (non-blocking): brittle meta-tests
The "processGoogleDriveChange guard wiring" describe block in tests/unit/google-content-sync-change-scope.test.ts reads index.ts as text (via stripComments) and asserts on substring positions/counts (e.g. guardIndex < firstSideEffect, persistSyncCursor occurring exactly twice). This is a reasonable way to pin an invariant that can't otherwise be exercised without exporting internal helpers, but it's coupled to identifier names and source layout — an unrelated rename or reformat (not a behavior change) will break it. Given the extraction of changes.ts was explicitly done to make the pure logic testable, consider whether processGoogleDriveChange itself could eventually be exported/tested directly (e.g. via dependency injection for markSourceMissing/importFileIsolated) so this class of test isn't needed. Not blocking — just flagging for future hardening.

Security: none — purely internal Lambda parsing/control-flow logic, no new external input surface, no auth/IAM changes.

Performance: negligible — one extra string comparison per change entry; fields projection grows by one field.

Test coverage: thorough — truth table for isFileScopedDriveChange/isDriveRemovalChange (including the "drive-scoped wins even with a fileId present" and legacy teamDrive cases), a GoogleDriveClient.listChanges regression test reproducing the original mixed-page failure, and the cursor-cannot-strand invariant. Good use of expect.objectContaining to keep tests resilient to schema growth.

Nothing else stood out — looks correct and ready to merge.

[krishagel]

Round 2 — Codex P1 addressed

P1 — Handle removed Shared Drive entries before advancing the cursor

Agreed, and fixed in bf5378c. The first commit skipped every non-file-scoped entry unconditionally, which silently consumed the one drive-scoped entry that carries real meaning.

processGoogleDriveChange now splits the non-file-scoped branch:

• metadata noise (rename / membership / restriction, removed false or absent) → return false, skipped, per-page persistSyncCursor still runs;
• removed: true → return true, which sets requiresSelectionSnapshot so reconcileChanges runs reconcileSelectionSnapshot → markUnseenSourcesMissing and retires the now-unreachable sources. A hard access loss still surfaces through the existing 403/404 → markConnectorAccessLost path.

The predicate is isDriveRemovalChange in infra/lambdas/google-content-sync/changes.ts. It is deliberately not keyed on changeType === "drive" — the legacy "teamDrive" value and any future scope Google introduces take the same path, since what identifies the case is "removed, and not about a specific file". The cursor advances on both branches, which the tests assert by pinning reconcileChanges to exactly two persistSyncCursor call sites.

Other review notes, and what was deliberately left alone

Two further items came out of internal review. Both are pre-existing weaknesses that this PR does not introduce and does not make worse, and both would widen a prod hotfix well past its root cause, so they are flagged rather than fixed here:

1. changesListSchema.parse() is all-or-nothing over a 1000-entry page. driveChangeSchema.file is optional, but if present the nested googleDriveFileSchema still requires id/name/mimeType and validates webViewLink/iconLink as URLs. Any single malformed entry therefore still fails the whole page — the same class of outage via a different trigger. Making this per-entry resilient is the durable fix, but a naive .catch() would silently reclassify a real file as unresolvable and mark it missing, i.e. trade a stalled queue for data loss. That needs its own change with its own thought, not a rider on this one.

2. No cursor checkpointing during a selection snapshot. Once requiresSelectionSnapshot is set, persistSyncCursor does not run again until reconcileSelectionSnapshot completes, so a snapshot that hits the 900s Lambda timeout or the 10k-file GoogleDriveSnapshotBudget leaves the cursor unmoved and retries the same page. This already applies to the pre-existing folder-mimeType path (return true on GOOGLE_FOLDER_MIME_TYPE); the drive-removal branch reuses that same proven mechanism rather than inventing a new one. Note the direction of travel is still strictly better than before this PR, where these entries poisoned the queue immediately and unconditionally.

A third note — that isDriveRemovalChange does not compare change.driveId against the connector's own sharedDriveId — was reviewed and intentionally left as is. A drive-scoped connector's feed is already filtered to its own driveId, so the check would be a no-op there; and for a personal_oauth connector (sharedDriveId IS NULL) there is nothing to compare against, so filtering would reintroduce exactly the stranded-content bug above. The cost of the conservative behaviour is one extra snapshot on a rare event.

Verification at bf5378c

• eslint . --max-warnings 0 — clean
• bun run test:ci — 573 suites / 5434 tests passing
• Playwright E2E gate — 317 passed, 41 skipped
• All CI checks green

The single failing unit suite (tests/unit/agent-workspace-identity.test.ts) and the single typecheck error both reproduce identically on a clean tree, verified via git stash: @aws-sdk/client-ecs is imported by infra/lambdas/agent-router/index.ts but is absent from package.json and bun.lock entirely. Untouched by this PR and tracked separately.

[krishagel]

🤖 pr-fix routine round 1 complete — no code changes needed.

I re-read every piece of open feedback against the tree at bf5378cf rather than against the review text, and all of it is already satisfied:

Feedback	Source	State
P1 — handle removed Shared Drive entries before advancing the cursor	Codex (inline, on e159156)	Fixed in bf5378cf; verified in index.ts:1371-1391 + changes.ts. Thread replied to and resolved.
Log line should carry driveId / time for audit-log correlation	Claude review #1	Fixed — the log call now emits connectorId, changeType, driveId, time
Legacy "teamDrive" changeType should not fall through	Claude review #1	Fixed — isDriveRemovalChange keys on "removed, and not file-scoped", not on changeType === "drive"
Source-text meta-tests are brittle	Claude reviews #1 and #2	Non-blocking, deferred by design. Both reviewers noted this matches an existing repo convention forced by infra/lambdas/google-content-sync/index.ts not being importable from the root Jest gate. Making it a behavioral test needs DI on markSourceMissing/importFileIsolated — a refactor well outside a prod hotfix.

Two further items the author flagged in the round-2 comment — per-entry resilience in changesListSchema.parse(), and cursor checkpointing during a selection snapshot — are pre-existing weaknesses that this PR neither introduces nor worsens. I agree with leaving them out; the first in particular would trade a stalled queue for silent data loss if done naively.

• Review comments addressed: 0 new (1 pre-existing thread verified + resolved)
• CI failures fixed: 0 (none failing — mergeable_state: clean)
• Commits pushed: 0

This PR looks ready to merge. Nothing is blocking it but a human clicking merge. Labeling pr-fix-done; a new commit or comment clears that and re-queues it.

---

Generated by Claude Code