fix(backend): hold every expert credential path to the grant list - #14443
fix(backend): hold every expert credential path to the grant list#14443Pwuts wants to merge 1 commit into
Conversation
The approved continuation of a reviewed block re-resolved credentials from the whole account, so a block held to one granted account at run_block time could execute with another after approval, and a grant revoked in between had no effect; it now resolves under the session's expert. An unseeded expert could widen its own grants by installing a workflow: the allow-list is seeded from installed workflows on first read, so an install made from the expert's own session fed it. Both install paths settle the seed first, and grant/revoke refuse an expert session outright. Adds the failing tests three guards did not have: the credential filter behind every block run, the MCP grant check, and the flag-off tool-group assertion this PR's own tool group broke. Co-authored-by: Claude Opus 5 (Claude Code) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Mutation evidenceEvery guard in this PR was broken one at a time and the test that should catch it was run. Each mutation asserted exactly one textual match before applying, and the tree was restored from a saved copy afterwards.
Suites executedBefore this PR, Not run here: the DB-backed neighbours of |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## abhi/expert-workflow-credential-scope #14443 +/- ##
=======================================================================
Coverage 81.28% 81.28%
=======================================================================
Files 3507 3507
Lines 262838 262942 +104
Branches 24370 24373 +3
=======================================================================
+ Hits 213641 213744 +103
- Misses 43783 43792 +9
+ Partials 5414 5406 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Changes 🏗️
Makes #14415 correct and green on its own head. The stack merges bottom-up, so
devwill hold this PR's state as a real commit; three of the fixes it needs currently live only onabhi/expert-grant-cardat the top of the stack, and one test it needs to pass is fixed there too.The approved continuation of a reviewed block resolved credentials against the whole account.
continue_run_blockcalledresolve_block_credentialswithout the expert, so a block held to one granted account atrun_blocktime could execute with a different one after approval, and a grant revoked between the review and the approval had no effect. It now passessession.expert_id, like every other block path.An unseeded expert could widen its own grants by installing a workflow. The allow-list is seeded from the expert's installed workflows the first time it is read; an expert whose list was not yet stamped could install a library workflow and have that workflow's credentials derived into its own grants on the next read.
install_expert_workflowand the auto-install of an agent the expert just built now settle the seed first, andgrant_expert_credential/revoke_expert_credentialrefuse an expert session outright rather than relying on the tool-group gate alone.Three guards had no test that fails when they are removed — the point of the whole ownership boundary is that a prohibition is only real if something breaks when it goes:
scope_credentials_to_expert, the single filter behind block runs, MCP tools and webhook setup. Removing it entirely left every suite green; the test that appeared to cover it patched the matcher out and asserted only that the expert id was forwarded. The new tests run two account credentials for the same host throughresolve_block_credentialsand assert the ungranted one is neither matched nor silently preferred.handoff_to_expert_test::TestExpertToolGate::test_flag_off_disables_every_team_groupasserts the old disabled-group list, so this PR is red ontest (3.11),(3.12)and(3.13)on its own head. The one-line assertion fix is moved down fromc0fc8ff82e.Moved rather than rewritten. The
continue_run_blockfix, the settle-before-install fix and the test-assertion fix already exist onabhi/expert-grant-card(03168cca3e,f235c7757c,c0fc8ff82e); the code here is those hunks verbatim, so the version that lands is the one already written and reviewed rather than a third variant. The rest of03168cca3e— picker credentials (_credentials_id) onacquire_auto_credentials, the MCP and validation-error card annotations — is left where it is: it is 460 lines across 17 files and moving it down would be a larger change than the gap it closes on this head.Verified
I ran
copilot/tools/—expert_scope_test,expert_resources_test,utils_test,test_run_mcp_tool,run_mcp_tool_test,continue_run_block_test,handoff_to_expert_test,http_credentials_test,block_display_test,helpers_test,list_team_test— plusapi/features/experts/credentials_test.py,util/architecture_test.pyandblocks/test/test_block.py.Every guard was mutated back out and the right test failed each time; the mutation table is in a comment below.
credentials_test.py's DB-backed neighbours and the frontend suites were not run here.Checklist 📋
For code changes:
Agents and large language models used