[shaohuzhang1]

delete: knowledge export skills on the backend

[shaohuzhang1]

Based on the provided diff, the changes are syntactically plausible: three PermissionConstants enum members are removed.

However, I see several potential risks and missing follow-ups:

1. High: Dangling references may break startup or authorization

Search the entire codebase for these removed symbols:

• KNOWLEDGE_EXPORT_SKILLS
• SHARED_KNOWLEDGE_EXPORT_SKILLS
• RESOURCE_KNOWLEDGE_EXPORT_SKILLS
• Operate.EXPORT_SKILLS

If any decorator, authorization check, serializer, fixture, test, or config still references them, it will likely fail with AttributeError, KeyError, or import errors.

2. High: Persistent permissions and role grants need migration

If these permissions are stored in DB tables, role-permission mappings, cached bitmasks, or serialized configs, simply deleting the enum members is not enough. You likely need a data migration to:

• Remove or disable the permission records.
• Remove role mappings.
• Recalculate role bitmasks.
• Invalidate permission caches.
• Handle existing records that reference the deleted permission names.

Otherwise, stale grants may remain.

3. High: Do not reuse deleted bit_index values

The removed permissions use:

• bit_index=13
• bit_index=64
• bit_index=45

If those bit indices are later reused for new permissions, existing role bitmasks may unintentionally grant the new permission. This is a privilege-escalation risk. Retire these bit indices permanently, or implement an explicit migration before reuse.

4. High: Check for authorization regressions

If the corresponding export-skill endpoints still exist, removing their permission constants may leave those endpoints:

• Unprotected,
• Protected by a different permission unintentionally,
• Returning 500 instead of 403,
• Inconsistent between workspace/shared/resource scopes.

Confirm that removing these permissions is intentional and that the endpoints are either removed, feature-disabled, or still correctly authorized.

5. Medium: Breaking change for external consumers

Permission names are often part of API responses, configs, or client behavior. Removing them may be a breaking change. If they were released, consider deprecation/aliasing first unless this is coordinated with all consumers.

6. Medium: Tests and snapshots need updates

Check for tests that assert:

• The full permission list,
• Permission counts,
• Role bitmasks,
• OpenAPI/schema snapshots,
• EE permission matrix behavior.

The is_ee=is_ee field on SHARED_KNOWLEDGE_EXPORT_SKILLS suggests EE-specific behavior may be affected.

7. Low: Dead enum values and bit gaps

After removal, Operate.EXPORT_SKILLS may become unused. Also, permission bits are now non-contiguous. That is usually fine, but verify no code assumes contiguous bit ranges or maximum bit indices.

Recommendation

Before merging:

1. Grep for all removed permission names and Operate.EXPORT_SKILLS.
2. Add/verify DB migration and cache invalidation.
3. Confirm endpoints are removed or still properly authorized.
4. Update tests/snapshots/docs.
5. Ensure deleted bit_index values are never reused without migration.

The diff itself does not show an obvious syntax error, but without the full repository I cannot confirm that all references and persisted data are handled.

[shaohuzhang1]

Review of the diff

@@ -91,7 +91,6 @@ def test_new_permissions_appear_in_workspace_role_tree(self):
         ids = {p["id"] for p in children["KNOWLEDGE"]["permission"]}
         self.assertIn(str(P.KNOWLEDGE_MCP_CONFIG.value), ids)
-        self.assertIn(str(P.KNOWLEDGE_EXPORT_SKILLS.value), ids)

Issues / irregularities

1. Test intent no longer matches its name.
   test_new_permissions_appear_in_workspace_role_tree is supposed to assert that new permissions appear. Deleting the assertion silently reduces coverage and lets a regression (permission disappearing from the role tree) pass. Prefer an explicit assertNotIn if removal is intentional, so the expected behavior is documented:

   self.assertNotIn(str(P.KNOWLEDGE_EXPORT_SKILLS.value), ids)

2. Possible count mismatch.
   The test uses count to validate the number of permissions per group (assertEqual(len(children[group]["permission"]), count)). If KNOWLEDGE_EXPORT_SKILLS was removed from the tree, the expected count likely needs to be decremented; otherwise the test may fail for an unrelated reason, or mask an off-by-one.

3. Dead code / orphaned enum.
   Check whether P.KNOWLEDGE_EXPORT_SKILLS is still referenced anywhere. If not, remove the enum member (and any serializers/views/migrations that reference it). Leaving it behind is dead code and can confuse future permission-tree work.

4. Data migration.
   If the permission was removed, ensure a migration cleans up existing Role/permission rows referencing KNOWLEDGE_EXPORT_SKILLS. Otherwise stale DB records may remain and the role tree can behave inconsistently across environments.

5. No replacement assertion.
   If EXPORT_SKILLS was replaced by a new permission (e.g. renamed), add an assertIn for the new permission so the test still verifies the "new permissions appear" contract.

Minor / optimization suggestions

• Use assertSet-style comparison or self.assertTrue(expected.issubset(ids)) for clearer intent when checking multiple permissions, or loop with subTest to get per-permission failure messages.
• The @patch(...) decorator injects serializer, which appears unused in this test. If it's not used, remove it (or assert it was used) to avoid misleading test setup.
• Given the current date and Python versions, consider assertIn over set membership is fine, but building ids once and using expected - ids in an error message would make failures easier to debug:

  missing = expected - ids
  self.assertFalse(missing, f"Missing permissions: {missing}")

• Verify the hunk line counts are right (-91,7 +91,6) — they are consistent with a single-line deletion.

Bottom line

The change is syntactically fine, but functionally it weakens the test without documenting the new expected behavior. Replace the deletion with an explicit negative assertion (or add the replacement permission), reconcile count, and clean up any now-unused enum/migration references.