Skip to content

test(table): pin the executor auth pairing on the table read route - #6603

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/table-schema-enrichment-auth
Aug 12, 2026
Merged

test(table): pin the executor auth pairing on the table read route#6603
waleedlatif1 merged 2 commits into
stagingfrom
fix/table-schema-enrichment-auth

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #6593. fetchTableSchema is the last enricher still calling the deprecated buildAuthHeaders, and after #6593 migrated its two siblings in the same file it now reads like a missed migration. It isn't — and "fixing" it today would break table tools.

  • GET /api/table/[tableId] authenticates through checkSessionOrInternalAuth → verifyInternalToken, which requires type: 'internal'. An executor delegation carries type: 'internal_delegation' and is rejected, then falls through to session auth the executor doesn't have.
  • The sibling table routes ([tableId]/groups, [tableId]/exports, imports/*) are already on internalTableSessionOrExecutorAuth, so this route is the straggler and a plausible next migration target.
  • tables.read also declares delegatedServices: ['copilot'], so an executor delegation would be refused at the operation layer even if the route accepted it.

So: assert the route still authenticates through the legacy path, and record on the caller why it is deliberately not migrated. The migration that flips this route now fails on a test that names the caller, instead of shipping and breaking table tools.

Why not migrate the route here

readTableUseCase exists, but this GET returns an 18-field payload including maxRows from getWorkspaceTableLimits, which the use case does not supply, and the response is consumed by the Tables UI. Doing it properly means a new contract matched field-by-field against the current output plus widening tables.read's principal policy — a scoped migration of its own, not a follow-up rider on something that isn't broken.

Correction to how this was previously described

I earlier called this a latent copy of the #6593 bug. The auth coupling is the same; the failure mode is the opposite. enrichTableToolSchema is wired as a tool-level toolEnrichment, and createLLMToolSchema wraps those in ToolSchemaEnrichmentError naming the tool — so a break here fails the Agent block loudly rather than silently degrading the schema the way the workflow and knowledge enrichers did. That is why this is a guard rather than a fix.

Type of Change

  • Test / maintainability

Testing

  • Verified the guard actually trips: stubbed out the route's checkSessionOrInternalAuth call and the new test failed while the existing PATCH tests stayed green, so it is targeted rather than incidental.
  • bunx vitest run app/api/table/ tools/ — 2,746 passing.
  • Repo-wide lint:check, type-check, and check:api-validation.
  • No runtime behavior change.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@waleedlatif1
test(table): pin the executor auth pairing on the table read route
5e7bf97 
fetchTableSchema reaches GET /api/table/[tableId] with a legacy type:'internal'
token, which only works while that route authenticates through
checkSessionOrInternalAuth. Its sibling table routes already moved to the
delegation policy, which rejects that token outright, so migrating this one
without moving the caller in the same change would break every table tool on an
Agent block.

Assert the route still authenticates through the legacy path so that migration
fails here first, and record on the caller why it is deliberately not on
buildExecutorDelegationHeaders yet.
@vercel

vercel Bot commented Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 12, 2026 5:04am

Request Review

@cursor

cursor Bot commented Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Low Risk
Test and documentation only; no runtime auth or route behavior changes.

Overview
Adds a regression guard so migrating GET /api/table/[tableId] off checkSessionOrInternalAuth without also updating fetchTableSchema fails loudly.

The new test asserts Bearer tokens still go through the legacy verifier that accepts type: 'internal' tokens from deprecated buildAuthHeaders. A matching comment on fetchTableSchema records why that enricher is deliberately not on buildExecutorDelegationHeaders yet — swapping either side alone would break every table tool on an Agent block.

Reviewed by Cursor Bugbot for commit 2c3fc06. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR documents the intentional legacy authentication pairing used by fetchTableSchema and adds focused regression coverage for the table GET route.

  • Adds a Bearer token to the GET-route test and verifies that the route forwards it unchanged to checkSessionOrInternalAuth.
  • Documents why fetchTableSchema must continue using buildAuthHeaders until the route and authorization policy are migrated together.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed
Filename Overview
apps/sim/app/api/table/[tableId]/route.test.ts Adds an effective regression test that pins the route’s legacy verifier choice and confirms the Bearer header reaches it unchanged.
apps/sim/tools/schema-enrichers.ts Adds documentation explaining the intentional dependency between fetchTableSchema and the table GET route’s existing authentication path.

Reviews (2): Last reviewed commit: "test(table): assert the Bearer header re..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Aug 12, 2026
View reviewed changes
Comment thread apps/sim/app/api/table/[tableId]/route.test.ts Outdated
@waleedlatif1
test(table): assert the Bearer header reaches the legacy verifier
2c3fc06 
Address review: the pairing test sent no Authorization header and its name
claimed to verify token acceptance, which is pinned separately in
lib/auth/internal.test.ts. Send a representative header, assert it reaches
checkSessionOrInternalAuth unmodified, and scope the name and docs to what this
guard actually covers — the route's choice of verifier.
@waleedlatif1

waleedlatif1 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

waleedlatif1 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

@cursor review

cursor[bot]
cursor Bot reviewed Aug 12, 2026
View reviewed changes

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2c3fc06. Configure here.

@waleedlatif1
waleedlatif1 merged commit e65345b into staging Aug 12, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/table-schema-enrichment-auth branch August 12, 2026 05:09
@waleedlatif1 waleedlatif1 mentioned this pull request Aug 12, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
@cursor cursor[bot] cursor[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@waleedlatif1