fix(integrations): resolve trading accounts from OAuth connections#124
Conversation
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
…h accounts and streamline portfolio identity discovery
|
| Filename | Overview |
|---|---|
| apps/tradinggoose/lib/trading/order-detail.ts | Removed credentialId path, added tokenAccountId lookup — historical order records without tokenAccountId in request JSON will fail provider-detail retrieval |
| apps/tradinggoose/lib/trading/context.ts | Replaced credential-based authorization with direct OAuth connection account lookup; simplified PreflightContext by removing credentialId field |
| apps/tradinggoose/lib/credentials/oauth.ts | Split listOAuthConnectionsForUser; added listOAuthConnectionAccountsForUser and resolveOAuthConnectionAccountForUser; removed workspace-scoped listOAuthCredentialAccountsForUser |
| apps/tradinggoose/lib/trading/portfolio-identities.ts | Dropped workspaceId param; now resolves portfolio identities directly from user OAuth connection accounts |
| apps/tradinggoose/socket-server/trading/portfolio-manager.ts | Added stopped guard, stop() shutdown method, pollState early-exit on stopped, and switched from credential to connection account resolution |
| apps/tradinggoose/socket-server/index.ts | Added explicit tradingPortfolioStreamManager.stop() during shutdown and switched httpServer.close to io.close for proper Socket.IO drain |
| apps/tradinggoose/lib/trading/orders.ts | Dropped request/workspaceId/workflowId params; stores tokenAccountId instead of credentialId in order history request JSON going forward |
| apps/tradinggoose/app/api/providers/trading/portfolio-identities/route.ts | Removed workspaceId/workflowId query params and workspace/workflow access guards; authentication check retained |
| apps/tradinggoose/providers/trading/portfolio-identity.ts | Renamed credentialId → tokenAccountId throughout PortfolioIdentity type, value object parser, and identity key builder |
| apps/tradinggoose/widgets/widgets/components/trading-services.ts | Switched from workspace-scoped credential query to user-scoped OAuth connections query; dropped workspaceId param |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/tradinggoose/lib/trading/order-detail.ts:64-68
**Historical order records will fail provider-detail lookup**
`readOrderTokenAccountId` reads the `tokenAccountId` key from the stored `order.request` JSON, but orders submitted before this PR were persisted with the key `credentialId` (written by the old `orders.ts` path). For any such row, `readOrderTokenAccountId` returns `null` and the guard on line 66 throws `'Order history record is missing trading connection context'`, making every historical order's provider-detail endpoint return a 400/500. A fallback read (`readOrderRequestString(row, 'credentialId')`) would at least let old records surface a useful error, or the guard could skip the provider-lookup path gracefully for legacy rows.
Reviews (3): Last reviewed commit: "perf(trading): defer oauth connections q..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… and holdings functions
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
| const tokenAccountId = readOrderTokenAccountId(order) | ||
| const serviceId = readOrderServiceId(order) | ||
| if (!credentialId || !serviceId) { | ||
| throw new TradingServiceError('Order history record is missing trading credential context') | ||
| if (!tokenAccountId || !serviceId) { | ||
| throw new TradingServiceError('Order history record is missing trading connection context') | ||
| } |
There was a problem hiding this comment.
readOrderTokenAccountId reads the tokenAccountId key from the stored order.request JSON, but orders submitted before this PR were persisted with the key credentialId (written by the old orders.ts path). For any such row, readOrderTokenAccountId returns null and the guard on line 66 throws 'Order history record is missing trading connection context', making every historical order's provider-detail endpoint return a 400/500. A fallback read (readOrderRequestString(row, 'credentialId')) would at least let old records surface a useful error, or the guard could skip the provider-lookup path gracefully for legacy rows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/tradinggoose/lib/trading/order-detail.ts
Line: 64-68
Comment:
**Historical order records will fail provider-detail lookup**
`readOrderTokenAccountId` reads the `tokenAccountId` key from the stored `order.request` JSON, but orders submitted before this PR were persisted with the key `credentialId` (written by the old `orders.ts` path). For any such row, `readOrderTokenAccountId` returns `null` and the guard on line 66 throws `'Order history record is missing trading connection context'`, making every historical order's provider-detail endpoint return a 400/500. A fallback read (`readOrderRequestString(row, 'credentialId')`) would at least let old records surface a useful error, or the guard could skip the provider-lookup path gracefully for legacy rows.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
The report is real only for old local/dev rows. It is not a current canonical-flow bug.
Current code is consistent:
- orders.ts writes
request.tokenAccountId - order-records.ts reads
tokenAccountId - order-detail.ts authorizes by
tokenAccountId - portfolio-identity.ts now models
tokenAccountId
I would not add a credentialId fallback. That is legacy compatibility and violates AGENTS.md.
Concise resolution: clear or normalize old dev orderHistoryTable rows. If the old request.credentialId value is actually an OAuth account.id, this one-time data cleanup works:
UPDATE "orderHistoryTable"
SET request = (request - 'credentialId')
|| jsonb_build_object('tokenAccountId', request->>'credentialId')
WHERE request ? 'credentialId'
AND NOT request ? 'tokenAccountId';If old credentialId values are workspace credential IDs, there is no canonical runtime fallback. Delete/recreate those dev orders or do a one-time manual data repair by joining through the credential table outside app code.
Validation passed:
45 tests passed:
app/api/orders/[orderId]/provider-detail/route.test.ts
app/api/providers/trading/order/route.test.ts
lib/trading/order-records.test.ts
There was a problem hiding this comment.
Fair point — AGENTS.md is explicit: "no legacy backfill path," full stop. The suggestion to add a credentialId fallback read conflicts directly with that constraint, and I should have flagged that tension rather than recommending it.
Your analysis is correct:
- The write/read/authorize chain is internally consistent on
tokenAccountIdpost-PR - Any breakage is isolated to pre-PR local dev rows, not canonical production data
- The SQL repair script you provided is the right remediation path (assuming old
credentialIdvalues are OAuthaccount.ids; manual repair otherwise)
The original comment is resolved. No code change needed here.
Tip: You can give me more context with .greptile/rules.md so I get this right next time.
2 participants
Summary
Why
Trading portfolio selection is based on connected broker OAuth accounts, but several paths still resolved those selections through workspace-scoped credential rows. That made account discovery and selected-account authorization drift across widgets, API routes, tools, and socket streams.
This change keeps broker account discovery tied to the signed-in user's connected OAuth accounts while preserving workspace checks around workspace-owned resources such as order history and order submission.
Affected Areas
apps/tradinggooseapps/docspackages/*Issue Links( if any )
None.
Validation
Results:
tsc --noEmit.Risk / Rollout Notes
workspaceIdorworkflowId; it now requires session auth and resolves the signed-in user's connected broker accounts.Config / Data Changes
Screenshots / Video
Not applicable. No visible UI layout changes.
Checklist