Skip to content

Temp fix for chat to edit#1753

Merged
sarah-inkeep merged 1 commit intomainfrom
fix-chat-to-edit
Feb 6, 2026
Merged

Temp fix for chat to edit#1753
sarah-inkeep merged 1 commit intomainfrom
fix-chat-to-edit

Conversation

@sarah-inkeep
Copy link
Contributor

We need this bypass check in order for chat to edit to work

@vercel
Copy link

vercel bot commented Feb 6, 2026

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

Project Deployment Actions Updated (UTC)
agents-api Ready Ready Preview, Comment Feb 6, 2026 0:38am
agents-docs Ready Ready Preview, Comment Feb 6, 2026 0:38am
agents-manage-ui Ready Ready Preview, Comment Feb 6, 2026 0:38am

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2026

🦋 Changeset detected

Latest commit: 459745b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@inkeep/agents-core Patch
@inkeep/agents-manage-ui Patch
@inkeep/agents-api Patch
@inkeep/agents-cli Patch
@inkeep/agents-sdk Patch
@inkeep/agents-work-apps Patch
@inkeep/ai-sdk-provider Patch
@inkeep/create-agents Patch
@inkeep/agents-manage-mcp Patch
@inkeep/agents-mcp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link
Contributor

claude bot commented Feb 6, 2026

Claude finished @sarah-inkeep's task in 2m 54s —— View job


PR Review Summary

3 Key Findings | Risk: Medium

🟠🔶⚠️ Major (2) 🟠🔶⚠️

🟠 1) permissions.ts:77-80 Bypass contradicts "strict" function semantics

Issue: The function canUseProjectStrict was explicitly designed for scenarios where SpiceDB permission checks are always required (per the docstring: "always checks SpiceDB"). Adding a bypass undermines this semantic contract. The function name and docstring are now misleading.

Why: Code relying on "strict" behavior may now silently pass requests that should be validated. The docstring (lines 68-72) promises "always checks SpiceDB" but the implementation no longer does this for system/apikey users. This creates a misleading contract for future developers.

Fix: Either:

  1. Update the function name and docstring to reflect the new behavior (e.g., rename to canUseProjectInternal or update docs to explain bypass cases)
  2. Create a separate function for trusted internal callers that explicitly documents the bypass, keeping canUseProjectStrict truly strict

🟠 2) permissions.ts "Temp fix" without documented timeline or proper solution

Issue: The PR title and changeset explicitly call this a "temporary fix" but there's no documentation of what the proper solution should be, why this bypass is needed for "chat to edit", or when/how it will be replaced.

Why: Temporary fixes often become permanent. Without a clear understanding of the root cause (why doesn't the user have proper SpiceDB permissions for the target project?), this bypass may hide a deeper authorization model issue that will be harder to fix later.

Fix: Add a TODO comment explaining:

  • Why the proper authorization doesn't work for chat-to-edit
  • What the proper fix would look like (e.g., establishing proper SpiceDB relationships when initiating chat-to-edit)
  • Link to a tracking issue if one exists

Other Findings

Other Findings (3)

Potentially valid

Location Issue Reason Excluded
permissions.ts:77-80 Missing unit tests for new bypass logic MINOR severity - existing tests don't cover canUseProjectStrict, adding tests is good but not blocking
permissions.ts:68-72 Docstring says "always checks SpiceDB" which is now false Addressed in Major finding #1

Discarded as invalid or not applicable

Location Issue Reason Excluded
permissions.ts Security risk from bypass Pattern is consistent with established middleware (projectAccess.ts:53, signoz.ts:38). Tenant-level scoping enforced elsewhere. Not a new attack surface.

💡 APPROVE WITH SUGGESTIONS

Summary: This PR follows an established bypass pattern used elsewhere in the codebase for system and API key users. The security risk is mitigated by tenant-access middleware enforcing scope. However, the "strict" function's semantics are now misleading, and the "temp fix" nature warrants documentation of the proper solution. Consider updating the docstring at minimum, and adding a TODO or linked issue for the permanent fix.

@inkeep
Copy link
Contributor

inkeep bot commented Feb 6, 2026

No docs changes detected. This is an internal permissions bug fix that doesn't affect any documented APIs or user-facing features.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant