fix: address review findings from the v0.8.21 release PR - #7454
fix: address review findings from the v0.8.21 release PR#7454waleedlatif1 wants to merge 3 commits into
Conversation
Five findings from the release review that held up on inspection: - The member-connector listing is workspace-scoped but used the concealing knowledge-base error policy, so a caller without workspace access was told a knowledge base they never named could not be found. Give it a workspace-scoped policy, the way the bulk routes already have one. - Restoring a queued Build message left the search query live, and a live query puts the composer straight back into Search, so the edit was stranded. Clear it on restore through the existing guarded helper. - The self-hosting Redis page said losing the instance cannot affect completed work. High-volume webhook triggers claim idempotency keys in Redis by default, so a restart can let a provider's retry run a finished webhook a second time. Document that, and the database-backed escape hatch. - /ship gained a docs-manifest check without a matching regenerate step, so a PR that adds, removes, or renames a docs page aborted instead of self-healing. - The CLI updater prints a Yarn upgrade command, but the manual upgrade instructions offered only npm, pnpm, and bun. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013e5sXbYST2R4qNzM996GFg
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis follow-up addresses the held-up findings from the v0.8.21 release review.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/knowledge/member-connectors/route.ts | Switches the workspace-scoped endpoint from the knowledge-base-concealing connector policy to the dedicated member-connectors policy. |
| apps/sim/lib/knowledge/api/route-policies.ts | Adds a non-concealing internal error policy for workspace member-connector listing. |
| apps/sim/lib/knowledge/api/route-policies.test.ts | Verifies that knowledge-base connector errors remain concealed while workspace member-connector errors do not. |
| apps/sim/app/workspace/[workspaceId]/home/home.tsx | Clears search state before restoring the queued message’s Assistant or Build composer mode. |
| apps/docs/content/docs/platform/self-hosting/redis.mdx | Clarifies webhook idempotency replay behavior after Redis loss and documents stronger persistence options. |
Reviews (3): Last reviewed commit: "fix(docs): separate durable idempotency ..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…-findings-0903 # Conflicts: # .agents/skills/ship/SKILL.md # apps/docs/content/docs/cli/troubleshooting.mdx
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…upling The Redis page sent a reader whose side effect must not replay to `forceStorage: 'database'`. That keeps the claim in PostgreSQL so a restart cannot drop it, but the service still claims, runs, and records the result in separate steps, so a side effect that commits before the result is recorded can still replay. `executeTransactionallyIdempotent` is the one that commits the claim, the mutation, and the result together. Name both and say which solves which problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013e5sXbYST2R4qNzM996GFg
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Summary
Findings from the review on the v0.8.21 release PR (#7446) that held up when I checked them against the code. Each was verified before fixing — the ones that did not hold up are listed at the bottom rather than silently patched.
member-connectorsused a concealing error policy on a workspace-scoped route. The route'smapInputforwards onlyworkspaceIdand never names a knowledge base, but it usedinternalKnowledgeErrorPolicies.connectors, which is wrapped inconcealKnowledgeBase(...). A caller without workspace access got404 Knowledge base not foundfor a resource they never asked about.route-policies.tsalready documents this exact rule forlist/bulkMove/bulkDelete; this adds the matching workspace-scoped policy.home.tsx:193forces the composer into Search whenever the query is non-empty and the mode isbuild.restoreQueuedModesetbuildwithout clearing the query, so the mode flipped straight back and the edit was stranded. It now clears through the existing guardedclearSearchhelper (hoisted above its new caller), which also avoids a redundant URL write when no search is open.forceStorage: 'database'.Two further findings this branch originally carried — the missing
docs-manifest:generatein/shipPhase A, and the missing Yarn tab in the CLI upgrade docs — landed independently as #7450 and #7448 while this was in review. Both are resolved in favour of those, so they no longer appear in this diff.Type of Change
Testing
Added two contract tests on the error policies: the knowledge-base-scoped connector policy still conceals, and the workspace-scoped member-connector policy does not. Verified the second fails against the concealing policy and passes with the fix.
bun run lint,bun run check:audits(45 audits),docs-manifest:check, andtype-check(0 errors) all pass on the merged tree, along with 174 tests across the touched areas. Ran/simplifyand/cleanupover the diff — findings from both applied.Not changed, and why
Three findings did not hold up and are deliberately left alone:
chat-sanitize.tsbacktick scan is a trade-off the file documents explicitly, and the suggested JSON-aware boundary is what the current constraint deliberately replaced for a stated cost reason.custom-blocks.mdxFAQ addresses publishers, not consumers — "you" is already correct and consistent with the rest of the page.One further finding is real but out of scope here: the connector Access field bails for Slack because Slack declares
permissionScopedListingyet resolves through the standard-OAuth credential-group path it is not part of. Fixing it means changing which resolver that field uses while preserving Slack's custom-bot validation, which belongs with the Slack per-member work rather than in this PR.Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_013e5sXbYST2R4qNzM996GFg