[shaohuzhang1]

delete: knowledge export skills

[shaohuzhang1]

Based on the diff only, there are no syntax errors, but there are several risks and inconsistencies:

1. Commented-out code is a maintenance smell

   • //KNOWLEDGE_EXPORT_SKILLS..., //SHARED_KNOWLEDGE_EXPORT_SKILLS..., and //RESOURCE_KNOWLEDGE_EXPORT_SKILLS... leave dead code.
   • Prefer either:
     • Permanent removal + all dependent cleanup, or
     • Feature flag / deprecation marker if temporary.
   • Also, //KNOWLEDGE... lacks a space after //; some lint rules flag commented-out code.

2. Inconsistent removal strategy

   • APPLICATION_CLEAR_POLICY is deleted, while the three export-skills permissions are commented out.
   • Decide whether these permissions are temporarily disabled or permanently removed, and apply one consistent approach.

3. High risk of dangling references

   • Search the entire repo for:
     • KNOWLEDGE_EXPORT_SKILLS
     • SHARED_KNOWLEDGE_EXPORT_SKILLS
     • RESOURCE_KNOWLEDGE_EXPORT_SKILLS
     • APPLICATION_CLEAR_POLICY
     • EXPORT_SKILLS
     • CLEAR_POLICY
   • TypeScript will catch direct property access such as PermissionConstants.KNOWLEDGE_EXPORT_SKILLS, but will not catch string-based/dynamic references, API payloads, role definitions, tests, or frontend permission keys.
   • Check whether Operate.EXPORT_SKILLS or Operate.CLEAR_POLICY become unused enum members.

4. Persisted permissions are not disabled by this diff

   • If permissions are stored in a database, commenting/removing constants does not remove existing permission records or role mappings.
   • You likely need a migration/data cleanup to:
     • delete or deactivate the removed permissions,
     • remove role/permission associations,
     • invalidate permission caches.
   • Otherwise the backend may still honor existing stored permissions.

5. Check for fail-open authorization behavior

   • Verify how authorization behaves when a permission key is missing or unknown.
   • If lookup returns undefined and the caller treats that as “allowed” or “no restriction,” export-skill access may remain possible. Ensure missing permission means deny.

6. Numeric permission codes should not be reused

   • Codes 13, 42, 45, and 64 are now absent or commented out.
   • If these codes are persisted or exposed externally, do not reassign them to new permissions later.
   • Consider reserving them explicitly, e.g. a comment or a RESERVED_PERMISSION_CODES entry, to prevent accidental reuse.
   • Also verify that gaps in numeric codes do not break ordering, generated manifests, bitmasks, or external integrations.

7. Generated artifacts and permission consumers may need updates

   • Check for anything that iterates over Object.keys(PermissionConstants) or Object.values(PermissionConstants).
   • Update tests, snapshots, docs, i18n labels, OpenAPI/API permission manifests, frontend permission lists, and seeded role definitions.

8. Recommended approach

   • If temporary: keep the constants but mark them @deprecated, and gate authorization with a server-side feature flag. Add a ticket/reason comment.
   • If permanent: delete the constants and unused Operate values, remove all references, add a DB migration, and update tests/docs. Do not leave commented-out entries in committed code.

In short: the diff is syntactically fine, but it needs a repo-wide reference audit, a database/permission migration plan, consistent handling of removed permissions, and confirmation that missing permissions fail closed.

[shaohuzhang1]

Review of the diff

What the diff does: removes the exportSkills permission helper (and its JSDoc) from the system permission object.

Potential issues

1. Dangling references → runtime crash. Grep the whole repo for exportSkills and RESOURCE_KNOWLEDGE_EXPORT_SKILLS before merging. Any remaining system.exportSkills() call will throw TypeError: system.exportSkills is not a function, and any leftover import of the constant will fail to resolve.

2. Security / privilege regression. This is a permission gate. If the export feature still exists, deleting the check may leave it unguarded (or guarded only by a broader permission like RESOURCE_KNOWLEDGE_MCP_CONFIG), effectively granting export to users who shouldn't have it. Confirm the export route still has an authorization check, or that the feature itself was removed.

3. Persisted permission keys. If permission codes are stored in the DB (roles, tenant configs, audit logs), removing the mapping can:

   • leave orphan rows / stale grants,
   • produce "unknown permission" validation errors on save,
   • silently drop grants on the next write.
     Consider a migration, or keep the key resolvable (deprecated) for one release.

4. UI / API contract. Frontend or downstream consumers may read a permission list (e.g. GET /permissions) to show/hide the export button. Removing the entry changes that contract — check snapshots, OpenAPI/type definitions, i18n labels, and docs.

5. Dead constant. If P.RESOURCE_KNOWLEDGE_EXPORT_SKILLS is now unreferenced, remove it too (and its backend definition/enum entry); otherwise you keep dead code that suggests the permission still exists.

6. Tests. Update/remove unit tests, permission-table snapshots, and any fixtures asserting the full permission set.

Style / minor

7. Formatting artifact. The pasted hunk shows inconsistent leading whitespace (the removed /** block is indented differently from mcpConfig). Verify the real file uses the same 2‑space indentation and aligned JSDoc as its neighbours — the paste may just be mangled, but it's worth a glance.

8. Naming consistency. Other entries are named after the resource (mcpConfig); exportSkills mixes an action with the resource. If this permission is being renamed rather than deleted, prefer knowledgeExportSkills and keep the old name as an alias for a deprecation cycle.

Recommendation

If the export capability is gone: delete the constant + all call sites + tests in the same commit.
If it's only being renamed/consolidated: export a deprecated alias and migrate callers first, then remove in a follow-up. In both cases, run tsc --noEmit / lint with unused-export detection, and add a note to the changelog since this is an authorization-surface change.

[shaohuzhang1]

Based on the provided diff, the change simply removes the exportSkills permission helper:

/**
 * 工作空间「知识库」导出权限
 */
exportSkills: (id: string) => canWorkspaceKnowledge(P.KNOWLEDGE_EXPORT_SKILLS, id),

Here are the issues and suggestions to verify:

1. Check for remaining references

   • Search for workspace.exportSkills / exportSkills and P.KNOWLEDGE_EXPORT_SKILLS.
   • If any callers remain, this is a TypeScript compile error or a runtime undefined call. All usages must be removed or migrated.

2. Check whether P.KNOWLEDGE_EXPORT_SKILLS is now dead code

   • If no longer used anywhere, remove the constant to avoid dead permissions.
   • If the backend permission still exists and may be used later, consider deprecating it rather than silently leaving an unused constant.

3. Backend authorization must still be enforced

   • Frontend permission helpers are only for UX. Confirm the export API/endpoint independently verifies authorization. Do not rely on removing the frontend check as a security control.

4. Check related UI and routing

   • Confirm the export button/menu/action and any related route or feature flag are also removed or hidden.
   • Otherwise users may still trigger export through another path.

5. Verify tests, docs, and i18n

   • Remove/update tests referencing exportSkills.
   • Update permission documentation, API contracts, and translation keys if the export feature is fully removed.

6. Consider backward compatibility

   • If this module is consumed by other packages/apps, removing an exported member is a breaking change. Use a deprecated alias or a major version bump if needed.

7. Formatting/comment consistency

   • The deletion itself leaves the surrounding object and comments syntactically valid: mcpConfig keeps its trailing comma, and the next JSDoc block follows correctly.
   • No semicolon/style issue is introduced by this hunk.

8. Optimization

   • If several permission helpers follow the same pattern, consider generating them from a permission map to reduce repetition, but only if it improves readability and type safety.

Overall: the diff is syntactically clean, but the main risk is dangling references or an inconsistent removal of the export feature/permission. Search the entire repository for exportSkills and KNOWLEDGE_EXPORT_SKILLS before merging.