Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review Please review the exact remote head |
|
@codex security review Please assess the exact remote head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
AutoReview/apps/api/src/controllers.ts
Lines 346 to 347 in 416ce93
Revalidate identity and MFA at the OAuth callback
The callback is public and treats the stored state record as sufficient authorization, so an owner can mint state without MFA because /start is a GET, and that state remains usable for ten minutes even if the account is subsequently disabled, its sessions are revoked, or its role is removed. In either case the callback can still replace the workspace's Google credentials; bind the nonce to the Identity UID/authentication context and revalidate the current account and required MFA immediately before storing tokens.
ℹ️ 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".
| const ownPublishedReply = | ||
| existing.value.status === "published" && | ||
| existing.value.publishedReply === snapshot.existingReply && | ||
| existing.value.snapshot.comment === snapshot.comment && | ||
| existing.value.snapshot.starRating === snapshot.starRating; |
There was a problem hiding this comment.
If Google's updated-review notification arrives after updateReply succeeds but before confirmPublished, the existing case is still publishing, so this condition is false and the webhook changes it to needs_attention, clears the draft, and advances its version. The in-flight confirmation then fails its version check, and later approval cannot enter the reconciliation branch because the case is no longer publishing, leaving a successfully posted reply permanently unconfirmed in local state. Treat a canonical reply matching the persisted publication intent as owned while publication is in progress.
AGENTS.md reference: AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
| const sources = await this.store.listKnowledge(principal.tenantId); | ||
| if ( | ||
| review.activeDraft.knowledgeSourceIds.some((sourceId) => { |
There was a problem hiding this comment.
Publication checks only source IDs that the model returned in knowledgeSourceIds, even though generation supplied every retrieved source and persisted all of their versions in review.knowledgeVersions. With two retrieved sources, a valid model response can cite one while its text was influenced by the other; retiring, expiring, or editing the omitted source then does not invalidate the draft, so it can be published using knowledge that is no longer approved. Iterate the persisted input-source versions rather than trusting the model's citation subset.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| await this.store.releaseEvent(principal.tenantId, envelope.message.messageId); | ||
| throw error; |
There was a problem hiding this comment.
When token refresh or the canonical Google getReview fails before ingestAndGenerate, this catch only releases the event lease and rethrows, so no audit record is produced for the failed asynchronous operation. Pub/Sub can retry and the review remains recoverable, but the required operational evidence is absent; append a metadata-only failure audit with a safe error classification before releasing the lease.
AGENTS.md reference: AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
| private async send(tenantId: string, review: ReviewCase) { | ||
| const registrations = await this.store.listDeviceRegistrations(tenantId); | ||
| const userIds = [...new Set(registrations.map((registration) => registration.userId))]; |
There was a problem hiding this comment.
Device registrations are selected solely by tenant and are never checked against the user's current Identity account or grants. Disabling an account, revoking its sessions, or removing its workspace role therefore leaves its registered phones receiving future review-arrival and scheduled-publication metadata until Expo declares the token invalid or the entire Google integration is disconnected; remove per-user registrations during revocation or validate recipients against current identities before sending.
Useful? React with 👍 / 👎.
| // Do not issue Set-Cookie from a background refresh: an older in-flight response | ||
| // must not recreate a browser session after DELETE /api/session cleared it. | ||
| // Identity refresh tokens remain valid until account/session revocation. | ||
| try { | ||
| return (await pending).idToken; |
There was a problem hiding this comment.
Once the cookie's original ID token reaches its one-hour expiry, this path returns the refreshed ID token but deliberately does not persist the new token, refresh token, or expiry. Consequently every subsequent proxied API request performs another Secure Token exchange, creating sustained Identity quota/latency pressure for every long-lived browser session and potentially logging the user out if the service rotates the refresh token. Preserve the logout generation/race protection while safely committing the refreshed session.
Useful? React with 👍 / 👎.
| push_endpoint = "${google_cloud_run_v2_service.worker.uri}/events/google-business" | ||
| oidc_token { | ||
| service_account_email = google_service_account.push.email | ||
| audience = google_cloud_run_v2_service.worker.uri | ||
| audience = var.worker_public_url |
There was a problem hiding this comment.
The Pub/Sub request targets the generated Cloud Run service URI but mints its ID token for the separately configured worker_public_url, which the supplied Terraform example sets to a custom origin. No Cloud Run custom audience is configured in this repository, so Cloud Run rejects the request before the worker can perform its own OIDC check; the Scheduler and notification-retry jobs repeat the same mismatch. Use the service URI as the audience or explicitly configure the custom audience for the service.
Useful? React with 👍 / 👎.
| await client.query( | ||
| "DELETE FROM runtime_knowledge_chunks WHERE tenant_id=$1 AND source_id=$2 AND source_version=$3", | ||
| [tenantId, sourceId, version], |
There was a problem hiding this comment.
Each approval writes a new source version, but this deletion removes only rows for that new version before reinserting them. Chunks from every earlier approved version therefore remain indefinitely, and editing or retiring the source has no cleanup path, preserving superseded uploaded document text and embeddings while causing unbounded storage growth. Delete obsolete versions when the new version commits and remove or expire all source chunks when the source is retired.
Useful? React with 👍 / 👎.
| globalKillSwitch: | ||
| settings.killSwitch || process.env.AUTOMATION_RELEASE_APPROVED !== "true", |
There was a problem hiding this comment.
An automatic task reads the kill switch and rule once, then reserves a slot, starts publication, fetches Google state, and performs the reply PUT without binding those authorization inputs to the publication transition. If an owner activates the kill switch or disables the rule after this read but before the PUT, the task still publishes, so the emergency stop is not deterministic for an operation that has not yet reached Google. Bind settings/rule versions into the atomic publication claim or revalidate them through a mechanism that invalidates the in-flight claim before writing.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| async setGoogleTokens(tenantId: string, tokens: GoogleTokens) { | ||
| const current = await this.getGoogleTokens(tenantId); | ||
| const value = { | ||
| ...tokens, | ||
| refreshToken: tokens.refreshToken ?? current?.refreshToken ?? null, | ||
| expiresAt: Date.now() + Math.max(60, tokens.expiresIn - 60) * 1_000, |
There was a problem hiding this comment.
During reconnection, an authorization-code exchange that returns no refresh token is merged with the previous connection's refresh token. If the owner authorized a different Google identity, the stored access token initially represents the new account, but after expiry currentAccessToken refreshes with the old account's credential and silently switches identities; stale locations from that account can then become operable again. Treat a new OAuth exchange as a complete credential boundary and fail closed when it lacks the required offline token rather than inheriting one from another authorization.
Useful? React with 👍 / 👎.
2 participants
What changes
The review workflow now persists tenant-scoped state rather than relying on a volatile demo store. Authenticated web and mobile screens expose Google onboarding, review operations, approved knowledge, rules, preferences and audit, with explicit loading/error/retry states and no silent demo fallback.
Publication uses an atomic state-and-intent commit, canonical Google revalidation and confirmation. A PUT accepted by Google with a lost response stays recoverable: reconciliation only performs GET and never blindly republishes. Stale or concurrent decisions cannot produce competing replies. Rejected unchanged reviews remain rejected on later duplicate events.
Knowledge approval indexes versioned, valid, location-scoped sources using PostgreSQL full-text and pgvector with a configurable Vertex embedding adapter. Sources are revalidated before sending. PDF/DOCX extraction is isolated in a bounded child process. Push submissions have a content-free persistent outbox, inspected Expo tickets, invalid-device removal and scheduled retry.
Identity Platform/TOTP, current account/revocation checks, KMS token encryption and forced RLS protect the controlled pilot. Schema migrations use a separate identity and database role; runtime startup refuses privileged or table-owner credentials. Terraform includes dashboard/worker/identity, bootstrap migrations, retention jobs and initial operational alerts. English README/setup/security documentation describes actual capabilities and external release gates.
Verification
Pilot boundaries
No approved Google Business Profile project or real location is available yet. Live Google/OAuth/PubSub, KMS/Cloud SQL/provider acceptance, physical push/device tests, signing/store release and privacy/Google operating-model approval remain external gates. Automatic publication is disabled by default and blocked by an operator release flag. This is a production-oriented controlled pilot, not a claim of production certification or a complete commercial SaaS.
Simulated onboarding also uses canonical Google resource names and the configured callback. Integration tests cover OAuth nonce replay rejection, authorized/consented import, no-send historical synchronization and immediate local disconnection.