Skip to content

fix: execute native skill and post saves in API chats - #1129

Open
mezotv wants to merge 2 commits into
mainfrom
codex/fix-api-skill-save
Open

mezotv wants to merge 2 commits into
mainfrom
codex/fix-api-skill-save

Conversation

@mezotv

@mezotv mezotv commented Sep 18, 2026

Copy link
Copy Markdown
Member

API chats could stop at createSkill or post creation waiting for interactive approval, but the API has no approval-response endpoint. Follow-up messages never executed the save.

Derive native write permissions from the authenticated OAuth scopes or API-key permissions. Expose and execute skill creation only with skills.write, and post creation/updates only with posts.write; preserve legacy api.write and wildcard grants. Missing permissions default to no native writes. Interactive chat approvals and external MCP approval rules remain unchanged. Clarify that the agent must call createSkill directly and only claim a save after it returns created.

Validation: all 91 API tests pass, including 17 integration regressions exercising the real transport, orchestration, and skill tool for OAuth/API-key scope combinations and interactive approval. The 3 code-mode tests, API and AI type checks, targeted lint, and diff checks also pass.

Companion MCP fix: usenotra/notra-mcp#42 surfaces unresolved approvals, failed/denied tool outputs, and stream failures instead of reporting progress text as a successful reply.

@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

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

Project Deployment Actions Updated
notra Ready Ready Preview Sep 18, 2026 3:39am UTC
notra-agent Ready Ready Preview Sep 18, 2026 3:39am UTC
notra-onboarding-agent Ready Ready Preview Sep 18, 2026 3:39am UTC
2 Skipped Deployments
Project Deployment Actions Updated
notra-ui Skipped Skipped Sep 18, 2026 3:39am UTC
notra-web Skipped Skipped Sep 18, 2026 3:39am UTC

Request Review

@github-actions github-actions Bot added apps/api Changes files in apps/api packages/ai Changes files in packages/ai priority/high Suggested review urgency; human overrides take precedence type/bug PR change classification labels Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 826b502.

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 3/5

This PR is not safe to merge until native API-chat writes enforce the corresponding post or skill permissions.

Findings

  1. P1 Security Native writes bypass scopes
  2. P2 Test bypasses production wiring

Summary

This PR adds a transport-specific native-tool approval policy so authenticated API chats can execute skill and post writes while interactive dashboard chats retain approval gating. It also strengthens prompt guidance around direct skill creation and adds a focused AI SDK test.

  • Adds requireNativeToolApproval, defaulting to approval-required.
  • Disables native approval in the API direct-stream transport.
  • Leaves external MCP approval enforcement separate.
  • Updates skill-save prompt guidance and regression coverage.
  • The new bypass needs resource-level authorization enforcement before it is safe for scoped API credentials.

Reviews (1) · Last reviewed commit: "fix: execute native skill saves in API c..."

Comment thread apps/api/src/lib/chat/direct-stream.ts Outdated
// The authenticated API request authorizes native writes; there is no
// interactive approval response endpoint on this transport. External
// MCP tools retain their separate approval policy.
requireNativeToolApproval: false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Native writes bypass scopes

Disabling native approval for every authenticated API chat lets a credential with only chats.write request skill or post creation even though it lacks skills.write or posts.write. The native tools authorize these database mutations using only the trusted organization ID, so a chat-scoped credential can perform durable writes outside its resource permissions. Preserve the resource-scope checks before bypassing approval, or limit this behavior to credentials with the corresponding write scope.

How this was verified: POST chat requests accept the chat write scope, while the newly unblocked skill and post tools perform organization-scoped writes without checking the credential's skill or post permissions.

Knowledge Base Used:

Comment thread packages/ai/src/utils/code-mode.test.ts Outdated
Comment on lines +146 to +177
const approvalTools = getStandaloneApprovalToolNames(requireApproval);
const result = await generateText({
model: new MockLanguageModelV4({
doGenerate: {
content: [
{
type: "tool-call",
toolCallId: "save-skill",
toolName: "createSkill",
input: JSON.stringify({ name: "marketplace-review-voice" }),
},
],
finishReason: { unified: "tool-calls", raw: "tool-calls" },
usage: {
inputTokens: { total: 1, noCache: 1, cacheRead: 0, cacheWrite: 0 },
outputTokens: { total: 1, text: 1, reasoning: 0 },
},
warnings: [],
},
}),
prompt: "Save marketplace-review-voice",
tools: {
createSkill: tool({
inputSchema: z.object({ name: z.string() }),
execute: async ({ name }) => {
saved = true;
return { name, status: "created" };
},
}),
},
toolApproval: ({ toolCall }) =>
approvalTools.has(toolCall.toolName) ? "user-approval" : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Test bypasses production wiring

This test calls getStandaloneApprovalToolNames directly and recreates the approval callback instead of exercising either changed integration point. It would still pass if the API transport stopped forwarding false or orchestration ignored the option, leaving the intended API-versus-interactive behavior without regression coverage. Exercise createDirectStandaloneChatResponse or orchestrateStandaloneChat through the real approval wiring.

Knowledge Base Used: AI orchestration and model services

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apps/api Changes files in apps/api packages/ai Changes files in packages/ai priority/high Suggested review urgency; human overrides take precedence type/bug PR change classification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant