mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-21 21:15:56 +08:00
Waleed
2223b63851
* refactor: consolidate local isRecord guards onto shared isRecordLike Nineteen files had re-declared a local `isRecord` guard rather than using the shared one from `@sim/utils/object`, drift that reappeared after #5061 first consolidated them. Two more imported the shared guard under an `isRecordLike as isRecord` alias. The copies were not interchangeable. Nine matched `isRecordLike` exactly. The rest omitted the array exclusion (`typeof x === 'object' && x !== null`, or `Boolean(x) && typeof x === 'object'`), so arrays passed the guard. Each of those call sites was reviewed individually: in every case the guard is followed by string/number field checks that an array fails anyway, so the outcome is unchanged. The one exception is `isOptionsTagData`, where `Object.values` on an array of option items really did make an array-form `<options>` tag render. It now accepts arrays explicitly rather than by accident. `executor/handlers/pi/search/extension-source.ts` keeps its own copy: it is source text written into an E2B/Daytona sandbox at runtime and cannot import. * refactor: replace inline record guards with isRecordLike 103 inline `typeof x === 'object' && x !== null && !Array.isArray(x)` guards (and the `x &&` / `Boolean(x)` spellings of the same conjunction) now call the shared guard. Inside a conjunction that already asserts `typeof x === 'object'`, `x &&` and `x !== null` are interchangeable, so all three orderings are the same predicate at runtime. Only exactly-equivalent conjunctions were converted. Matching required the three clauses to be one adjacent conjunction over the same operand, so a nearby but unrelated clause cannot be absorbed — generic-handler.ts, where the array case is handled inside the block rather than excluded by the guard, is correctly left alone. Two sites were reverted after type-check rejected them: instagram/server-utils.ts and workflows/[id]/log/route.ts both cast straight to a specific interface, which is legal from `object` but not from `Record<string, unknown>`. Their narrowing is genuinely not identical, so they keep the inline form rather than acquiring a double cast. Left as-is: `packages/ts-sdk` (published with no runtime dependencies) and the two sandbox sources written into E2B/Daytona as text, which cannot import. * refactor: consolidate duplicate record coercion helpers onto @sim/utils A second sweep found 28 more local record helpers hiding under names the previous `isRecord` grep never matched — `asRecord`, `toRecord`, `toRecordOrNull`, `asObject`, `isJsonObject`. rabbitmq defined the same `asRecord` twice within one service; dynatrace had three variants in one file. Fourteen of them were re-deriving the same two shapes, so those shapes now live in `@sim/utils/object` beside the guards they wrap: toRecord(value) // isRecordLike(value) ? value : {} toRecordOrNull(value) // isRecordLike(value) ? value : null Both preserve identity on a hit, so no call site starts copying. Eighteen local definitions are gone. `tools/instantly/utils.ts` keeps its exported `asRecord` because its return type is the local `JsonRecord` alias, but its body now delegates. `app/api/mcp/serve/[serverId]/route.ts` had an `isJsonObject` with zero call sites — deleted outright. Six helpers were deliberately left alone because they are NOT equivalent: `pagerduty`/`zendesk`/`gitlab` do `(value as Record) || {}`, which type-checks nothing at all, and `copilot/resources/extraction.ts`, `edit-workflow/validation.ts`, `pi/core/events.ts` omit the array exclusion. Those sit on webhook ingress and copilot paths where tightening is a behavior change, not a cleanup; they are audited separately. Two guards were removed rather than substituted, each proven dominated by an earlier check: the bedrock streaming `toolUse.input` guard was unreachable (`parseToolInput` already throws on non-objects before the loop builds `assembledToolUses`), and four `driver.ts` re-narrows follow an `if (!isRecordLike(x) || ...) throw` that dominates the later use. * fix(webhooks): guard non-string GitLab ref, and consolidate the last record helpers Two audits covered the six helpers held back from the previous commit for not being equivalent to isRecordLike. Five are now migrated; one is deliberately not. `gitlab.ts` carried a real crash path, independent of the guard work: const ref = (b.ref as string) || '' const branch = ref.replace('refs/heads/', '') The cast is unchecked and `|| ''` only catches falsy values, so a truthy non-string body field — `{"ref": 12345}` — reaches `.replace` and throws `TypeError: ref.replace is not a function` inside `formatInput`. That runs in the background worker after the webhook is already 200-ACKed, and GitLab does not auto-retry, so the delivery is lost silently. Now checks `typeof`. `pagerduty`/`zendesk`/`gitlab` each defined `asRecord` as `(value as Record<string, unknown>) || {}`, which type-checks nothing — a string or array passed through and was then spread into the workflow trigger payload (`gitlab.ts:114`) as character- or index-keyed garbage. All three now use the shared `toRecord`. These sit behind `verifyProviderAuth`, so reaching them requires the shared secret; this is robustness, not authorization. `copilot/resources/extraction.ts` and `pi/core/events.ts` were array-permissive but provably inert — extraction.ts has no key enumeration or spread anywhere, and events.ts only diverges by returning `null` instead of `{type:'other'}` for an array, which every consumer already no-ops on. Pinned with a test. `edit-workflow/validation.ts` is left permissive ON PURPOSE. Its `Object.entries` walk mirrors the unguarded walk in `operations.ts:86,191`, so an array-shaped `nestedNodes` from the model is currently visited by both. Tightening only the validation side would stop `collectHostedApiKeyInput` from stripping platform-managed API keys while the apply side still creates those child blocks. Both paths have to change together, with tests, in their own PR. * refactor: delete 31 unreachable module-local functions Removes 815 lines of provably dead code: module-local (non-exported) declarations whose identifier appears exactly once in their own file — the declaration itself. A non-exported symbol cannot be reached by an import, a barrel, a dynamic import, or a framework convention, so "unreferenced in its own file" is a complete proof of deadness rather than a heuristic. Notable removals include whole abandoned code paths: `findWebhookAndWorkflow` (78 lines), `calculateBillingProjection` and `initializeUserUsageLimit` (80 lines), `removeCredits` + `deductFromCredits` (54), `sendBatchSMS`, `executeToolBatch`, and four unused `async-runs/repository.ts` queries. Spans come from the TypeScript AST, not a regex. A regex cannot find a declaration's extent — a first-brace scan cuts inside a return-type annotation such as `Promise<{ canCreate: boolean }>` and silently corrupts the file. The AST pass also re-derives deadness from identifier nodes, which caught two wealthbox helpers that a regex export-check had wrongly reported as local. Note the runtime TypeScript API here is `@typescript/typescript6`; the bare `typescript` specifier resolves to the native compiler, which exposes no `createSourceFile`. * refactor: delete 434 stranded exported symbols Removes ~5,930 lines of unreachable code across 166 files: exported symbols that no other file in the repo mentions and that are unused inside their own file. Whole abandoned surfaces go with them — unused React Query hooks (useOrganizations, useOrganizationMembers, useUpgradeSubscription, ...), unused admin route contracts, dead executor constants and reference builders, and the landing-page StageWorkflow/LandingPreviewMount components. Three files left with no remaining code were removed outright. Candidates came from an AST pass; each was then verified individually against an UNFILTERED repo-wide search plus the reachability paths a name search misses: string-keyed tool/block registries, dynamic imports, `export *` barrels, and the docs generator's source-text parsing of tool files. 29 candidates were verified LIVE and kept. Those exposed a flaw in the candidate generator: it indexed only .ts/.tsx, while apps/docs/content/**/*.mdx imports React components directly — ActionImage appears in ~190 MDX pages and ActionVideo in ~115, and both scanned as dead. `app/global-error.tsx` was likewise kept, since Next.js reaches it by filename and its default export can never have a name reference. All 434 deleted names were afterwards cross-checked against every .mdx/.md/.json/.yaml in the repo: no hits. Verified with turbo type-check (23 workspaces), the full apps/sim suite (25,219 tests), all 26 audits, and a production `next build` — the last of these being what actually exercises route- and component-level reachability.