feat(adt-mcp): add delegated assistant read scope - #148
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Deploy Preview for adt-cli canceled.
|
📝 WalkthroughWalkthroughThe PR classifies ChangesDelegated MCP read access
REST runtime security surface
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully adds the delegated-assistant read scope with proper security controls and comprehensive test coverage. The implementation correctly follows the established patterns for agent policy validation, includes all necessary security checks, and maintains the fail-closed approach. All 20 tests pass, and the code is ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
MergerReady to Merge MERGE: the delegated read-scope change is internally consistent, and all review threads are already resolved or rejected with no remaining blocker. CI didn’t run, but there’s no diff-confirmed defect or open disagreement that would justify holding this PR. Commit |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
PR Summary by Qodofeat(adt-mcp): add delegated-assistant signed read scope
AI Description
Diagram
High-Level Assessment
Files changed (28)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Classify ATC, AUnit, and coverage as non-mutating read analysis while preserving optional exact scoped execution policies.
4592063 to
a74fb02
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts (2)
49-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd runtime dispatch coverage for
run_unit_tests.This verifies only
isMcpToolAllowed; the suppliedscope-enforcement.test.tschange exercises actual wrapped dispatch foratc_runbut notrun_unit_tests. Since both tools changed classification, add a read-authority handler test that invokesrun_unit_testswith coverage arguments and verifies the handler runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts` around lines 49 - 65, Add a runtime read-authority handler test for run_unit_tests alongside the existing dispatch coverage, invoking it with objectType, objectName, withCoverage, and coverageFormat arguments and asserting the handler executes. Use the established wrapped-dispatch test pattern from scope-enforcement.test.ts, while preserving the existing isMcpToolAllowed assertions.
15-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the catalogue test prove the complete read-only boundary.
listed.length > 10plus onelock_objectexclusion will not catch another mutation tool being accidentally advertised. Assert an explicit expected read set, or enumerate all known write entries and assert that none are listed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts` around lines 15 - 32, Update the delegated read catalogue test around MCP_TOOL_SCOPE_CATALOGUE and isMcpToolListed to assert the complete read-only boundary: compare listed against an explicit expected read-tool set, or explicitly verify every known write entry is absent. Replace the weak length-only assertion while preserving the existing required read tools and lock_object exclusion checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adt-mcp/src/lib/tools/scope-catalogue.ts`:
- Around line 552-560: Update isMcpToolListed() so the atc_run/run_unit_tests
safe_execute exception also requires access.classes to include safe_execute,
matching isMcpToolAllowed() dispatch behavior and preventing uncallable tools
from being listed. Preserve the existing operationClass checks and update the
relevant listed-versus-allowed invariant test.
In `@packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts`:
- Around line 1-2: Replace the Node built-in test imports in
delegated-assistant-catalogue.test.ts with the project’s configured Vitest
imports, using vitest or `@abapify/nx-vitest` for test, describe, and it as
applicable. Keep the existing test behavior unchanged and remove reliance on
node:test.
In `@packages/adt-mcp/tests/delegated-assistant-policy.test.ts`:
- Around line 1-7: Update delegated-assistant-policy.test.ts to import describe
and it from vitest instead of node:test, while preserving the existing test
behavior and other imports.
---
Nitpick comments:
In `@packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts`:
- Around line 49-65: Add a runtime read-authority handler test for
run_unit_tests alongside the existing dispatch coverage, invoking it with
objectType, objectName, withCoverage, and coverageFormat arguments and asserting
the handler executes. Use the established wrapped-dispatch test pattern from
scope-enforcement.test.ts, while preserving the existing isMcpToolAllowed
assertions.
- Around line 15-32: Update the delegated read catalogue test around
MCP_TOOL_SCOPE_CATALOGUE and isMcpToolListed to assert the complete read-only
boundary: compare listed against an explicit expected read-tool set, or
explicitly verify every known write entry is absent. Replace the weak
length-only assertion while preserving the existing required read tools and
lock_object exclusion checks.
🪄 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 Plus
Run ID: 61efa746-1054-406e-bf34-85f9a7af3061
📒 Files selected for processing (38)
.agents/repo-guide.md.agents/rules/git/autonomous-delivery.md.agents/rules/git/no-auto-commit.mddocs/roadmap/README.mddocs/roadmap/epics/_template.mddocs/roadmap/epics/e01-include.mddocs/roadmap/epics/e02-function.mddocs/roadmap/epics/e03-badi.mddocs/roadmap/epics/e04-strust.mddocs/roadmap/epics/e05-format-plugin-api.mddocs/roadmap/epics/e06-gcts-format-plugin.mddocs/roadmap/epics/e07-gcts-command-plugin.mddocs/roadmap/epics/e08-checkin.mddocs/roadmap/epics/e09-acds-parser.mddocs/roadmap/epics/e10-rap-bdef.mddocs/roadmap/epics/e11-rap-srvd.mddocs/roadmap/epics/e12-rap-srvb.mddocs/roadmap/epics/e13-startrfc.mddocs/roadmap/epics/e14-flp.mddocs/roadmap/epics/e15-wb.mdopenspec/changes/add-delegated-assistant-read-scope/design.mdopenspec/changes/add-delegated-assistant-read-scope/proposal.mdopenspec/changes/add-delegated-assistant-read-scope/specs/adt-mcp/spec.mdopenspec/changes/add-delegated-assistant-read-scope/tasks.mdopenspec/changes/classify-atc-as-read-analysis/design.mdopenspec/changes/classify-atc-as-read-analysis/proposal.mdopenspec/changes/classify-atc-as-read-analysis/specs/adt-mcp/spec.mdopenspec/changes/classify-atc-as-read-analysis/tasks.mdpackages/adt-mcp/src/index.tspackages/adt-mcp/src/lib/http/invocation.tspackages/adt-mcp/src/lib/tools/scope-catalogue.tspackages/adt-mcp/tests/delegated-assistant-catalogue.test.tspackages/adt-mcp/tests/delegated-assistant-policy.test.tspackages/adt-mcp/tests/http-invocation.test.tspackages/adt-mcp/tests/scope-enforcement.test.tspackages/adt-server/src/index.tspackages/adt-server/src/rest-runtime.tspackages/adt-server/tests/rest-runtime.test.ts
💤 Files with no reviewable changes (1)
- .agents/rules/git/no-auto-commit.md
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
View your CI Pipeline Execution ↗ for commit ff614ec
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
|
1 participant
Summary
Verification
Technical Details
This change introduces a new
delegated-assistantsigned invocation policy that allows interactive MCP clients to discover and call read tools on behalf of an authenticated principal without duplicating the server's tool catalogue. The policy is product-neutral and does not include product-specific agent names or workflows.The change also updates the repository's git workflow rules to enable autonomous delivery of verified feature work to scoped branches, replacing the previous "no auto-commit" rule with a more flexible "autonomous delivery" rule that still gates destructive operations and protected-branch updates.
Summary by cubic
Adds a product-neutral delegated-assistant read scope to
adt-mcpso interactive clients can list and run server-owned read tools (incl. ATC and AUnit) without client allowlists. Also exposes a REST runtime composition seam inadt-serverand tightens scoped listing rules for ATC/AUnit.New Features
delegated-assistantread policy bound to principal,threadId,executionId,systemSid, and one Destination; exportedparseDelegatedAssistantReadPolicy.atc_runandrun_unit_tests(incl. coverage) asread; ordinary read authority can dispatch them, and when using an exact scoped policy their listing requires thesafe_executeclass (still supported for stricter runs).loadRestRuntimeSecurity,RestRuntimeSecurity, andRestRuntimeSecurityOptionsfromadt-serverto compose REST runtime.Refactors
.agents/rules/git/no-auto-commitwith.agents/rules/git/autonomous-delivery; updated guides and epic templates to allow committing and pushing authorized feature work without extra prompts.Written for commit ff614ec. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests