feat(approval): add tenant-aware guidance when approval scope is missing#241
Conversation
When a user-identity approval API call fails due to missing `approval:*` scopes, the agent now checks `tenant_key` before guiding the user: - For tenant `736588c9260f175d`: shows a friendly "not yet supported" message - For other tenants: follows the standard permission-fix flow in lark-shared
|
|
github-actions
bot
added
the
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDocumentation update to the approval skill guidance that adds a specialized error-handling flow for user identity permission failures, branching remediation instructions based on tenant key to either indicate unsupported functionality or proceed with standard scope remediation. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a tenant-aware branching section to Key changes:
Confidence Score: 5/5Safe to merge; the one previously open P1 (missing fallback) is now addressed, and no new blocking issues are introduced. Both concerns raised in earlier review rounds have been resolved or are already tracked: the missing fallback for a failed No files require special attention beyond previously filed comments. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Approval API call fails] --> B{User identity?}
B -- No / Bot identity --> Z[Unaffected — existing flow]
B -- Yes --> C{permission_violations contains approval:* scope?}
C -- No --> Z
C -- Yes --> D[Call lark-cli contact +get-user]
D --> E{Call succeeded & tenant_key present?}
E -- No / error --> F[Standard lark-shared permission-fix flow]
E -- Yes --> G{tenant_key == 736588c9260f175d?}
G -- Yes --> H["⚠️ 当前租户暂不支持使用审批 skill，敬请期待"]
G -- No --> F
Reviews (2): Last reviewed commit: "fix(approval): address review feedback o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| 根据返回结果中 `data.user.tenant_key` 的值进行分流： | ||
|
|
||
| - **`tenant_key` 为 `736588c9260f175d`** → 直接告知用户： |
There was a problem hiding this comment.
The tenant key 736588c9260f175d is hardcoded directly in a versioned, (presumably) public skill file. This exposes a specific customer/tenant identifier to anyone who can read the repository, which is a privacy and security concern.
Beyond the sensitivity risk, this design is also fragile:
- When more tenants need the same "not yet supported" treatment, each will require a new commit to this file.
- When the feature is eventually enabled for this tenant, there will be no automated way to remove the exemption — it will silently keep showing the "not yet supported" message forever unless someone manually removes it.
Consider an alternative approach: have the lark-cli tool (or a dedicated endpoint) return a feature-flags/capability field in its response (e.g. data.user.features.approval_enabled: false) so the skill can branch on a boolean flag rather than a raw tenant ID. That way, tenant-specific rollout state lives in the platform, not in this file.
There was a problem hiding this comment.
Thanks for the review. A few points:
-
Privacy:
tenant_keyis not a secret — it's returned by a public API to any authenticated user within that tenant. -
Feature-flag approach: Agreed that a server-side feature flag would be ideal long-term. But that requires platform changes out of scope for this PR. This is a pragmatic stopgap that can be trivially removed once approval is rolled out to this tenant.
-
Fragility: For a single known tenant, the cost of one hardcoded value is minimal. If more tenants need it, that would be the signal to invest in the flag approach.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-approval/SKILL.md`:
- Line 17: Update the guidance to specify how to detect the missing approval
scope by inspecting the API error object's permission_violations field and
checking whether any reported scope string starts with the prefix "approval:"
(e.g., "approval:task:read", "approval:instance:write"); if such a scope is
present treat it as the "缺少 `approval:` 开头的 scope" case, then follow the
existing flow that first checks the current user's tenant_key before deciding
how to prompt for permissions.
- Around line 19-23: The SKILL.md guidance incorrectly references
data.user.tenant_key which does not exist in the lark-cli contact +get-user
response; update the document to remove or replace that instruction and instead
explain a correct way to detect tenant context (for example, use the tenant
information from the app installation/authorization event or call the
appropriate tenant-info API), and reference the actual fields returned by
contact +get-user (open_id, union_id, email, mobile, enterprise_email) so users
won’t rely on a non-existent data.user.tenant_key; check cmd/auth/auth.go and
shortcuts/contact/contact_get_user.go to verify the real response fields and
link to or mention the correct endpoint/method to obtain tenant details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2783d256-b1f7-4d7e-9954-a971e3c09167
📒 Files selected for processing (1)
skills/lark-approval/SKILL.md
- Reference `permission_violations` field explicitly instead of vague "错误原因" (coderabbit suggestion) - Add fallback when `contact +get-user` fails: fall through to standard permission-fix flow (greptile suggestion)
3 participants
Summary
approval:*scope, the agent now checks the user'stenant_keyvialark-cli contact +get-userbefore deciding how to guide the user.736588c9260f175d): displays a friendly "not yet supported" message instead of the standard permission-fix flow.lark-sharedpermission-handling logic as before.Motivation
Certain tenants do not yet have the approval feature enabled on the platform side. Without this change, the agent would misleadingly guide users through the standard scope-authorization flow, which would still fail. This provides a clear, actionable message instead.
Test Plan
approval:task:readpermission error as a user withtenant_key=736588c9260f175d→ should see the "not yet supported" messagetenant_key→ should see the standard permission-fix guidanceSummary by CodeRabbit