mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 21:26:39 +00:00
1cd3eb742b
* feat(authz): enforce skill authorization at assembly and slash-activation (#4063 Phase 3) Phase 3 / Skills — the second of three resource-type PRs (Models, Skills, Sandbox). The RBAC provider already maps "skill" → config key "skills" (rbac.py _RESOURCE_POLICY_KEYS), so no schema change is needed. Layer 1 (assembly-time, mirrors Phase 1B): - filter_available_skills_by_authorization() in skill_filter.py filters the skill-name allowlist (set[str] | None) by the provider's "skill" policy. In lead agent, runs after _available_skill_names() so the catalog (describe_skill) and SkillActivationMiddleware share one filtered set. In subagent executor _load_skills(), filters config.skills before disk load. - filter_resources_by_authorization() in enforcement.py — generic version of filter_tools_by_authorization for any resource with a name attribute. Layer 2 (slash-activation): - No new middleware code needed. SkillActivationMiddleware._resolve_activation already checks "reference.name not in self._available_skills". Since Layer 1 filters the allowlist, denied skills cannot be slash-activated. authorization.enabled: false is a complete no-op. available_skills=None (no agent allowlist) + enabled: resolves all skill names from config and filters. 12 new tests + 243 existing authz/skills tests pass. * test(subagents): wait on the chained future instead of polling done() in caller-loop teardown test * feat(authz): enforce action-scoped skill:activate at runtime and reuse the catalog loader for filter candidates * feat(authz): arm skill:activate on the subagent chain, unify the client skill-surface user, and record the Layer 2 reversal in the design notes * chore(authz): align the subagent filter's fallback user with the chain's bucket convention * feat(authz): gate autonomous skill loading on skill:activate, close the bootstrap assembly leak, and reuse one provider per subagent assembly Review round 4 (willem-bd, CHANGES_REQUESTED): - [P1] Autonomous load path: describe_skill filters its match set on the action-scoped skill:activate decision (all-denied is indistinguishable from "no match"), and ToolErrorHandlingMiddleware stamps skill_context entries only when the action allows it — a denied SKILL.md read gets a skill_context_denied marker instead, so durable context, allowed-tools policy, and autonomous secret bindings never activate a denied skill. File visibility itself stays Layer 1 (sandbox-projection semantics). - [P1] Bootstrap assembly: build_middlewares/apply_prompt_template now receive the authorization-filtered set instead of the raw _BOOTSTRAP_SKILL_NAMES, and an empty skill_names index is preserved when deferred discovery is on rather than round-tripping through None into the legacy full-metadata prompt that re-advertises a denied skill (deferred-off keeps the legacy rendering; the filtered empty allowlist blanks it on denial). - [P2] SubagentExecutor resolves ResolvedSkillAuthorization once per assembly (_resolve_skill_authorization, memoized per executor) and shares it across the Layer 1 filter (authorization=), the activation middleware, and both new gates — uncached provider factories can no longer hand the layers different instances/policy snapshots. All gates share one helper (skill_filter.skill_activation_allowed) so slash, describe, and skill-file-load cannot drift; provider errors keep the configured fail-closed/fail-open policy. skill_authorization is threaded through _build_runtime_middlewares into both runtime builders. Tests: describe gating (deny/fail-closed/fail-open/disabled), read stamping (denied marker vs entry) + extract_skills skip without warning, bootstrap three-way param (allow/deny x deferred on/off), lead + subagent chain wiring for the stamp gate, and the distinct-instance provider factory repro. Each fix is mutation-verified (reverting it fails its test). Full A/B suite run: no new failures vs HEAD. * feat(authz): re-authorize persisted skill_context before reuse and route async paths through aauthorize Review round 5 (willem-bd): - [P1] Persisted entries: skill_context survives across runs, so both consumers now re-check the action-scoped skill:activate decision before reuse. SkillToolPolicyMiddleware._active_skills_for_paths skips a denied skill (same skip as a disabled/unallowlisted one; all-denied keeps the existing fail-closed-to-builtins semantics) and _in_context_secret_sources binds no secrets for a denied entry. skill_authorization is threaded into SkillToolPolicyMiddleware on both chains. - [P2] Async provider API: skill_activation_allowed_async mirrors the sync helper through await provider.aauthorize() (same pattern as authorize_sandbox_execution_async). Async hooks precompute an activation_decisions map on the event loop (slash target + persisted entry names, one batch of awaits) and hand it to the worker-thread handler, which consults the map instead of the sync API: SkillActivationMiddleware.awrap_model_call (activation + secret binding), ToolErrorHandlingMiddleware.awrap_tool_call (_amaybe_stamp awaits the decision before stamping), and SkillToolPolicyMiddleware's awrap_model_call/awrap_tool_call (entry names + slash source name). describe_skill becomes a StructuredTool with both a sync function (invoke -> authorize) and a coroutine (ainvoke -> aauthorize), so async agent execution never routes a loop-affine provider through the sync API while sync callers keep their semantics. Tests: allow-read-then-deny-next-run regressions for secret binding and allowed-tools (partial deny does not poison, all-denied fails closed to builtins), and an _AsyncOnlyProvider (sync authorize always fails, aauthorize works) driving four async-path assertions (activation, stamp allow/deny, describe, tool policy). All six fixes are mutation-verified; full A/B suite run against HEAD shows no new failures. * fix(authz): authorize skill activation by declared Skill.name, not directory basename ShenAC-SAC's P2: bundled skills may declare a name that differs from their directory (skills/public/vercel-deploy-claimable declares name: vercel-deploy), but the runtime gates derived authorization targets from the container path. - New deerflow/skills/container_registry.py: build_container_path_registry() + canonical_skill_name() — the one path-to-Skill.name resolution shared by the activation, tool-policy, and read-stamping middlewares. - ToolErrorHandlingMiddleware resolves the read path through the (user-scoped, via new user_id kwarg threaded from both runtime builders) registry before the skill:activate decision, sync and async (registry lookup in a worker thread, aauthorize() on the loop). RBAC allow by the declared name now activates; allow by the directory name does not. - SkillToolPolicyMiddleware async hooks canonicalize the policy paths first (three-phase: thread registry -> loop aauthorize -> thread filter reusing the registry; the cached per-model-step decision still short-circuits), so the decision map is keyed by the names _active_skills_for_paths checks — no more map miss falling back to the sync provider API from the thread. _entry_names removed: persisted entry["name"] is path-derived by construction; entries are resolved by path instead. - SkillActivationMiddleware collects slash names (already canonical) plus persisted entry *paths*, canonicalized off-loop via _canonical_names_for_paths(). Regressions (test_skills_authorization.py 45->50): sync stamp target, RBAC allow declared-name/deny directory-name both directions, async stamp via _AsyncOnlyProvider, composed async slash -> tool-policy keeps the skill's tools with zero sync fallback, persisted-entry secret binding survives the name mismatch. All three mutations (stamp target, policy map key, entry canonicalization) fail their tests. * fix(authz): close the sync-fallback class on skill activation paths willem-bd's two P2s on the canonical-name fix, plus a fourth instance of the same class found while sweeping the membership: - SkillToolPolicyMiddleware: a transient prepass registry failure is now preserved (_REGISTRY_LOAD_FAILED marker) — the worker-side filter applies its own fail-closed treatment instead of silently retrying storage, which recovered skills absent from the decision map and fell back to the synchronous provider API from the thread (fail-open retaining a denied skill's allowed-tools). - SkillActivationMiddleware: an empty entry-path list no longer pays a full skill-tree scan on ordinary authorization-enabled async model steps (previously a passive path with zero registry I/O). - 4th instance (reproduced with the PR fixtures, then fixed): the entry prepass failure on the secret-binding path let the consumer-side _load_skill_registry_by_path silently recover and resolve entry names outside the decision map — the same sync fallback, fail-open binding the aauthorize-denied skill's declared secret. The fix goes one step further than failure propagation: the prepass hands down the registry *snapshot* the decision map was keyed by, and entry sources resolve against that snapshot, so a map miss is impossible by construction (this also closes the narrower window where both loads succeed but disagree because storage changed mid-step). The slash source is deliberately unaffected: it does not consult the decision map (validated once at activation) and keeps its fresh per-call load. - Class membership swept: skill_activation_allowed() sync call sites are exactly four (stamping sync path and describe's sync implementation are legitimately sync); the only two async-reachable fallback points (the _activation_allowed helpers) are both construction-guarded now. Structural hardening (typed async-batch decision object where a map miss is fail-closed by construction) recorded in the plan notes as a follow-up, anchored at those two helpers. - _RegistryArg type alias replaces the degenerate dict|object|None union in the policy middleware signatures. Tests (test_skills_authorization.py 50->54): transient prepass failure -> builtins-only with exactly one storage attempt and zero provider calls; ordinary async step -> zero registry scans; two-call scenario -> entry binding zeroed while the slash binding survives; entries resolve against the prepass snapshot under a mid-step rename. Mutations verified red: marker reverted to None, over-broad slash kill, snapshot bypass. * fix(authz): correct secret-binding reference points and gate the rendered skill reminder Review round 9 (willem-bd) plus construction-audit follow-ups on the same class — every async divergence between two loads of the same data now has an explicit, pinned reference point: - Slash secret source resolves from a FRESH post-activation registry: the prepass snapshot is taken before the aauthorize awaits, so binding the slash source from it would activate NEW_KEY content while injecting OLD_KEY when a skill's declared secrets change in that window. The 'one scan per step' bonus that motivated sharing the snapshot is withdrawn — consistency wins. The two secret sources resolve independently: a transient fresh-load failure zeroes only the slash binding; entries keep binding from their own snapshot. - Same-skill dominance: when a skill is both the slash source (fresh) and a persisted entry (snapshot), only the activation-era declaration binds — unioning both eras injected a secret the skill no longer declares. Aligns with the tool-policy rule (slash activation dominates for the run); different-skill entries still stack additively. - The rendered 'Active skills' reminder (DurableContextMiddleware) no longer advertises skills the provider denies — surface consistency with describe_skill. Publication/consumption split: the activation middleware publishes per-step path-keyed skill:activate decisions into the run context (async: reused from the prepass snapshot, zero extra I/O; sync: one scan + authorize() only when entries exist — passive steps stay scan-free), the renderer consumes them and filters its render copy (state untouched; unpublished = permissive, matching the other run-context carriers). New carrier key listed in REDACTED_CONTEXT_KEYS with a redaction regression. - Wiring regressions on both chains for everything added above (user_id to the stamp gate, skill_authorization + token pairing + activation- before-durable ordering to the durable renderer): each mutation verified red (M4-M13 across this change set). - Tests 54->65: slash post-activation lookup, source independence, dominance, sync-chain sync-API path, render filter (async/sync/ disabled with load-count assertions), chain wiring x2, redaction listing. Plan notes record rounds 9-2..6 including the reference-point post-mortem and the audit axes exhausted. * fix(authz): anchor same-skill dominance on the slash identity, not binding results Review round 10 (willem-bd) plus a self-audit extension the new checklist items caught in the first fix itself: - R10 (P2): the same-skill exclusion was derived from successfully BOUND slash sources. When the prepass snapshots required-secrets [OLD_KEY] and the operator strips the declaration during the await window, the slash source binds nothing (required_secrets gate) — the exclusion set stayed empty and the stale entry view kept injecting OLD_KEY for a skill that now declares no secrets. The exclusion now derives from the AUTHENTICATED slash activation identity (run-context source path, owner-token checked): the identity's declared name resolves from the fresh registry first, then the entry snapshot, and joins the exclusion set regardless of binding success. - Self-audit (value->gate->downstream on the fix itself): the declared name can also change mid-run (same path, foo->bar). A name-anchored exclusion then holds only the new name while the snapshot entry resolves the same path to the old one — both eras bound again. The PATH is the stable identity shared by the activation and every earlier read: entries at the slash identity's path are now excluded before registry resolution (exclude_paths), independent of what either version calls the skill; the name-based set remains for cross-path same-name shadowing. Tests 65->67: no-secrets-era dominance (ACTIVE secrets None under the OLD_KEY-to-empty interleaving), path-anchored dominance across a mid-run rename (only the activation-era key binds). Mutations verified red: identity-derived exclusion disabled (M14), path exclusion disabled (M16). * test(authz): pass explicit config to lead middleware wiring test --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>