Skip to content

feat(adt-mcp): add delegated assistant read scope#148

Open
ThePlenkov wants to merge 4 commits into
mainfrom
feat/delegated-assistant-read-scope
Open

feat(adt-mcp): add delegated assistant read scope#148
ThePlenkov wants to merge 4 commits into
mainfrom
feat/delegated-assistant-read-scope

Conversation

@ThePlenkov

@ThePlenkov ThePlenkov commented Jul 25, 2026

Copy link
Copy Markdown
Member

Summary

  • Add a product-neutral delegated-assistant read invocation envelope
  • Keep tool catalogue and dispatch policy server-owned
  • Reject tool allowlists, limits, write, and safe-execute authority
  • Replace the obsolete repeated commit/push confirmation rule with autonomous scoped feature delivery

Verification

  • 20 focused policy, catalogue, and signed-JWT tests pass
  • Scoped ESLint and Prettier pass
  • Strict OpenSpec validation passes

Technical Details

This change introduces a new delegated-assistant signed 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.


Open in Devin Review

Summary by cubic

Adds a product‑neutral delegated-assistant read scope to adt-mcp so interactive clients can list and call server-owned read tools without client allowlists. Also classifies ATC and AUnit as non‑mutating read checks and exposes a REST runtime composition seam in adt-server.

  • New Features

    • Added a signed delegated-assistant read policy bound to principal, threadId, executionId, systemSid, and one Destination; exported parseDelegatedAssistantReadPolicy.
    • Classified atc_run and run_unit_tests (incl. coverage) as read; the optional exact safe_execute policy remains supported for stricter runs.
    • Exposed loadRestRuntimeSecurity and RestRuntimeSecurity from adt-server to compose REST runtime.
  • Refactors

    • Replaced .agents/rules/git/no-auto-commit with .agents/rules/git/autonomous-delivery; updated guides and epic templates to allow committing and pushing authorized feature work without extra prompts.

Written for commit a74fb02. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added delegated assistant read-only access for discovering and using server-provided read tools.
    • Delegated access supports object inspection, ATC checks, unit tests, and coverage while excluding mutation operations.
    • Preserved stricter scoped execution controls for workflows requiring them.
    • Expanded REST runtime security utilities through the public package interface.
  • Documentation

    • Updated roadmap and delivery guidance to support automatic commits and pushes of verified changes after implementation authorization.
    • Added specifications and implementation checklists for delegated read access and read-only analysis operations.
  • Tests

    • Added coverage for policy validation, tool visibility, authorization boundaries, and delegated invocation handling.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@netlify

netlify Bot commented Jul 25, 2026

Copy link
Copy Markdown

Deploy Preview for adt-cli canceled.

Name Link
🔨 Latest commit a74fb02
🔍 Latest deploy log https://app.netlify.com/projects/adt-cli/deploys/6a654c0ae59d09000786a06b

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR replaces approval-gated delivery guidance, adds strict delegated-assistant read access to ADT MCP, classifies ATC and unit-test analysis as read operations while retaining scoped execution, and exports REST runtime security types and loading utilities.

Changes

Autonomous delivery guidance

Layer / File(s) Summary
Autonomous commit and push policy
.agents/*, docs/roadmap/README.md, docs/roadmap/epics/*
Always-on Git rules and roadmap prompts now direct agents to commit and push verified scoped changes without a separate confirmation prompt, while retaining gates for destructive or out-of-scope actions.

Delegated MCP read access

Layer / File(s) Summary
Delegated read policy contract
openspec/changes/add-delegated-assistant-read-scope/*
Adds the delegated-assistant claim shape, read-only authority boundaries, validation scenarios, and implementation checklist.
Delegated invocation verification
packages/adt-mcp/src/lib/http/invocation.ts, packages/adt-mcp/src/index.ts, packages/adt-mcp/tests/*policy*, packages/adt-mcp/tests/http-invocation.test.ts
Adds strict delegated read-policy parsing, dispatch support, public exports, signed-credential coverage, and rejection tests for malformed or over-broad claims.
Read catalogue and analysis authorization
packages/adt-mcp/src/lib/tools/scope-catalogue.ts, packages/adt-mcp/tests/*catalogue*, packages/adt-mcp/tests/scope-enforcement.test.ts, openspec/changes/classify-atc-as-read-analysis/*
Moves ATC and unit-test analysis into the read catalogue, preserves scoped safe_execute handling, and validates delegated read listings, mutation denial, and analysis execution.

REST runtime security surface

Layer / File(s) Summary
REST runtime security exports
packages/adt-server/src/index.ts, packages/adt-server/src/rest-runtime.ts, packages/adt-server/tests/rest-runtime.test.ts
Names and exports the REST runtime security type and loader, with tests importing the loader through the package entry point.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant InvocationVerifier
  participant PolicyParser
  participant ScopeCatalogue
  participant Destination
  Client->>InvocationVerifier: submit signed delegated-assistant credential
  InvocationVerifier->>PolicyParser: parse delegated read policy
  PolicyParser->>ScopeCatalogue: request server/read tools
  ScopeCatalogue->>Destination: validate Destination and operation class
  Destination-->>ScopeCatalogue: authorize read-scoped execution
  ScopeCatalogue-->>Client: advertise and dispatch permitted tools
Loading

Possibly related PRs

  • abapify/adt-cli#147: Updates the same autonomous-delivery rules and delegated-assistant MCP read-scope implementation.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely captures the main change: adding a delegated assistant read scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/delegated-assistant-read-scope
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/delegated-assistant-read-scope

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed: dependency version conflict. Check your lock file or package.json.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

Gitar is working

Gitar

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@codacy-production

codacy-production Bot commented Jul 25, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 19 complexity · 2 duplication

Metric Results
Complexity 19
Duplication 2

View in Codacy

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.

@baz-reviewer

baz-reviewer Bot commented Jul 25, 2026

Copy link
Copy Markdown

Merger

Pending

PR has merge conflicts.

Commit a74fb02 · Updated 2026-07-25 23:51 UTC


Review this PR on Baz | Customize your next review

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment on lines +199 to +224
export function parseDelegatedAssistantReadPolicy(
claims: TrustedMcpInvocationClaims,
): DelegatedAssistantReadPolicy | undefined {
if (
claims.agentId !== 'delegated-assistant' ||
claims.destinationKeys.length !== 1 ||
!hasServerReadClasses(claims) ||
Object.keys(claims.limits).length !== 0 ||
!hasExactSortedKeys(claims.constraint, [
'kind',
'threadId',
'executionId',
'systemSid',
]) ||
claims.constraint.kind !== 'delegated-assistant-read-v1'
) {
return undefined;
}

const threadId = requiredUuid(claims.constraint.threadId);
const executionId = requiredUuid(claims.constraint.executionId);
const systemSid = requiredSystemSid(claims.constraint.systemSid);
if (!threadId || !executionId || !systemSid) return undefined;

return Object.freeze({ threadId, executionId, systemSid });
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Delegated-assistant policy correctly projects the full read catalogue via generic access construction

invocationRequestAccess at packages/adt-mcp/src/lib/http/server.ts:421-444 builds McpRequestAccess generically from invocation.classes/invocation.destinationKeys, only attaching frozenSource/scoped when the corresponding parsers match. For a delegated-assistant credential, parseAiReviewFrozenSourcePolicy and parseScopedAdtInvocationPolicy both return undefined (agentId mismatch), so the resulting access is a plain {classes:['server','read'], destinationKeys:[one]}. This yields the full server-owned read catalogue via isMcpToolListed, matching the stated intent. The gate isMcpInvocationDispatchPolicySupported now includes the delegated-assistant branch (invocation.ts:242-244), so unsupported/malformed delegated policies still fail closed and expose no tools. The wiring is consistent with the existing autonomous-review-agent path.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread .agents/repo-guide.md
| [`verification/after-changes`](rules/verification/after-changes.md) | Build, typecheck, test, lint, format checklist |
| Rule | Description |
| ------------------------------------------------------------------------------- | ------------------------------------------------------------ |
| [`git/autonomous-delivery`](rules/git/autonomous-delivery.md) | Commit and push authorized feature work without re-prompting |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Roadmap/agent-rule wording change is consistent across all sites

The PR replaces the git/no-auto-commit rule with git/autonomous-delivery and rewrites every 'Do NOT commit without approval' line across the epic docs and _template.md. I verified there are no dangling references to no-auto-commit remaining anywhere in .md, .cursor, .windsurf, .claude, or .agents (grep returned none), and the new rules/git/autonomous-delivery.md file that .agents/repo-guide.md:115 links to exists. The mechanical transformation appears applied completely and consistently.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

feat(adt-mcp): add delegated-assistant signed read scope

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add delegated-assistant signed read envelope for server-owned tool discovery and dispatch.
• Enforce exact claim shape; reject allowlists, limits, write, safe_execute, and extra Destinations.
• Replace no-auto-commit guidance with autonomous, verified feature-branch delivery rules.
Diagram

graph TD
  A["Interactive MCP client"] --> B["ADT credential issuer"] --> C["Delegated read JWT"] --> D["Invocation verifier"] --> E["Scope catalogue"]
  E --> F["tools/list"]
  E --> G["Dispatch checks"] --> H[("SAP via Destination")]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Reuse system-assistant envelope
  • ➕ No new agentId/policy parser to maintain
  • ➕ Potentially fewer moving parts for clients already using it
  • ➖ No explicit thread/execution binding for interactive delegation
  • ➖ Blurs compatibility behavior with a tighter delegated contract
2. Embed client tool allowlist/limits in the claim
  • ➕ Client can further constrain tool availability per session
  • ➖ Drifts from server catalogue; requires client updates as tools evolve
  • ➖ Increases claim surface area and verification complexity; widening risk
3. Add safe_execute to delegated envelope
  • ➕ Enables ATC/unit test style flows from interactive clients
  • ➖ Higher risk; requires separate, strict single-use grants and stronger checks
  • ➖ Conflicts with stated read-only/non-goals of delegated envelope

Recommendation: Keep the PR’s approach: introduce a product-neutral delegated-assistant read-only envelope with an exact claim shape and server-owned catalogue authority. It minimizes client coupling, prevents catalogue drift, and fails closed by rejecting any widened authority (limits, allowlists, extra classes/destinations). If safe_execute is needed later, add it via a separate explicit grant rather than expanding this envelope.

Files changed (28) +380 / -25

Enhancement (2) +41 / -1
index.tsExport delegated-assistant policy parser and types +2/-0

Export delegated-assistant policy parser and types

• Re-exports parseDelegatedAssistantReadPolicy and DelegatedAssistantReadPolicy from the package entrypoint.

packages/adt-mcp/src/index.ts

invocation.tsImplement delegated-assistant-read-v1 parsing and dispatch support gating +39/-1

Implement delegated-assistant-read-v1 parsing and dispatch support gating

• Adds delegated-assistant as a trusted agentId and introduces a strict parser requiring exactly server+read classes, one destination, empty limits, exact constraint keys, and validated UUID/system SID; dispatch admission now depends on this parser.

packages/adt-mcp/src/lib/http/invocation.ts

Tests (3) +162 / -0
delegated-assistant-catalogue.test.tsTest delegated read catalogue projection and dispatch denial for write/safe_execute +56/-0

Test delegated read catalogue projection and dispatch denial for write/safe_execute

• Verifies tools/list admits many read tools from the server-owned catalogue and excludes write and safe-execute tools; asserts dispatch denies write/safe_execute/unknown tools.

packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts

delegated-assistant-policy.test.tsAdd strict delegated-assistant policy unit tests +84/-0

Add strict delegated-assistant policy unit tests

• Covers accept-path for the exact policy and rejects widened authority (tool lists, limits, extra classes, extra destinations) plus malformed thread/execution/system bindings.

packages/adt-mcp/tests/delegated-assistant-policy.test.ts

http-invocation.test.tsAdd signed JWT verification test for delegated-assistant envelope +22/-0

Add signed JWT verification test for delegated-assistant envelope

• Extends invocation verifier tests to sign a delegated-assistant-read-v1 ES256 JWT and assert it verifies and is dispatch-admissible.

packages/adt-mcp/tests/http-invocation.test.ts

Documentation (23) +177 / -24
repo-guide.mdSwap always-on git rule reference to autonomous delivery +7/-7

Swap always-on git rule reference to autonomous delivery

• Updates the rules index to replace git/no-auto-commit with git/autonomous-delivery.

.agents/repo-guide.md

autonomous-delivery.mdAdd autonomous commit/push rule with gated high-risk actions +26/-0

Add autonomous commit/push rule with gated high-risk actions

• Introduces an always-on rule allowing verified atomic commits/pushes to scoped feature branches without re-prompting, while still gating force-pushes, protected branches, releases, and scope expansion.

.agents/rules/git/autonomous-delivery.md

README.mdUpdate epic workflow guidance to autonomous delivery +3/-1

Update epic workflow guidance to autonomous delivery

• Replaces the old “no commits without approval” guidance with autonomous verified feature delivery and links to the new rule.

docs/roadmap/README.md

_template.mdUpdate epic template commit instructions +1/-1

Update epic template commit instructions

• Changes the template’s instruction to commit and push verified changes to the scoped feature branch without a separate confirmation prompt.

docs/roadmap/epics/_template.md

e01-include.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e01-include.md

e02-function.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e02-function.md

e03-badi.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e03-badi.md

e04-strust.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e04-strust.md

e05-format-plugin-api.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e05-format-plugin-api.md

e06-gcts-format-plugin.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e06-gcts-format-plugin.md

e07-gcts-command-plugin.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e07-gcts-command-plugin.md

e08-checkin.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e08-checkin.md

e09-acds-parser.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e09-acds-parser.md

e10-rap-bdef.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e10-rap-bdef.md

e11-rap-srvd.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e11-rap-srvd.md

e12-rap-srvb.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e12-rap-srvb.md

e13-startrfc.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e13-startrfc.md

e14-flp.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e14-flp.md

e15-wb.mdAlign epic instructions with autonomous delivery +1/-1

Align epic instructions with autonomous delivery

• Updates the epic header guidance from explicit approval to autonomous scoped delivery wording.

docs/roadmap/epics/e15-wb.md

design.mdDocument delegated-assistant read design and rejected alternatives +44/-0

Document delegated-assistant read design and rejected alternatives

• Defines the delegated-assistant-read-v1 constraint shape and enforcement model, and records rejected approaches (product IDs, client allowlists, reuse system-assistant, add safe_execute).

openspec/changes/add-delegated-assistant-read-scope/design.md

proposal.mdAdd proposal for delegated-assistant read envelope and non-goals +28/-0

Add proposal for delegated-assistant read envelope and non-goals

• Explains motivation, intended behavior, explicit non-goals (no write/safe_execute/allowlists), and validation expectations.

openspec/changes/add-delegated-assistant-read-scope/proposal.md

spec.mdAdd OpenSpec requirements for delegated read catalogue + fail-closed widening +40/-0

Add OpenSpec requirements for delegated read catalogue + fail-closed widening

• Specifies that delegated assistants get the server-owned server/read catalogue and that malformed/widened authority is denied at discovery and dispatch.

openspec/changes/add-delegated-assistant-read-scope/specs/adt-mcp/spec.md

tasks.mdAdd implementation checklist and verification notes +13/-0

Add implementation checklist and verification notes

• Tracks completed tasks for policy/tests/implementation and documents known full-tree build blockers due to dependency mismatch.

openspec/changes/add-delegated-assistant-read-scope/tasks.md

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 29 files

Re-trigger cubic

@ThePlenkov
ThePlenkov force-pushed the feat/delegated-assistant-read-scope branch from 4592063 to a74fb02 Compare July 25, 2026 23:51
@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts (2)

49-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add runtime dispatch coverage for run_unit_tests.

This verifies only isMcpToolAllowed; the supplied scope-enforcement.test.ts change exercises actual wrapped dispatch for atc_run but not run_unit_tests. Since both tools changed classification, add a read-authority handler test that invokes run_unit_tests with 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 win

Make the catalogue test prove the complete read-only boundary.

listed.length > 10 plus one lock_object exclusion 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

📥 Commits

Reviewing files that changed from the base of the PR and between 41a624b and a74fb02.

📒 Files selected for processing (38)
  • .agents/repo-guide.md
  • .agents/rules/git/autonomous-delivery.md
  • .agents/rules/git/no-auto-commit.md
  • docs/roadmap/README.md
  • docs/roadmap/epics/_template.md
  • docs/roadmap/epics/e01-include.md
  • docs/roadmap/epics/e02-function.md
  • docs/roadmap/epics/e03-badi.md
  • docs/roadmap/epics/e04-strust.md
  • docs/roadmap/epics/e05-format-plugin-api.md
  • docs/roadmap/epics/e06-gcts-format-plugin.md
  • docs/roadmap/epics/e07-gcts-command-plugin.md
  • docs/roadmap/epics/e08-checkin.md
  • docs/roadmap/epics/e09-acds-parser.md
  • docs/roadmap/epics/e10-rap-bdef.md
  • docs/roadmap/epics/e11-rap-srvd.md
  • docs/roadmap/epics/e12-rap-srvb.md
  • docs/roadmap/epics/e13-startrfc.md
  • docs/roadmap/epics/e14-flp.md
  • docs/roadmap/epics/e15-wb.md
  • openspec/changes/add-delegated-assistant-read-scope/design.md
  • openspec/changes/add-delegated-assistant-read-scope/proposal.md
  • openspec/changes/add-delegated-assistant-read-scope/specs/adt-mcp/spec.md
  • openspec/changes/add-delegated-assistant-read-scope/tasks.md
  • openspec/changes/classify-atc-as-read-analysis/design.md
  • openspec/changes/classify-atc-as-read-analysis/proposal.md
  • openspec/changes/classify-atc-as-read-analysis/specs/adt-mcp/spec.md
  • openspec/changes/classify-atc-as-read-analysis/tasks.md
  • packages/adt-mcp/src/index.ts
  • packages/adt-mcp/src/lib/http/invocation.ts
  • packages/adt-mcp/src/lib/tools/scope-catalogue.ts
  • packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts
  • packages/adt-mcp/tests/delegated-assistant-policy.test.ts
  • packages/adt-mcp/tests/http-invocation.test.ts
  • packages/adt-mcp/tests/scope-enforcement.test.ts
  • packages/adt-server/src/index.ts
  • packages/adt-server/src/rest-runtime.ts
  • packages/adt-server/tests/rest-runtime.test.ts
💤 Files with no reviewable changes (1)
  • .agents/rules/git/no-auto-commit.md

Comment on lines +552 to +560
if (
operationClass !== access.scoped.operationClass &&
!(
(name === 'atc_run' || name === 'run_unit_tests') &&
access.scoped.operationClass === 'safe_execute'
)
) {
return false;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file exists and outline =="
fd 'scope-catalogue\.ts$' . || true
ast-grep outline packages/adt-mcp/src/lib/tools/scope-catalogue.ts --view expanded || true

echo "== relevant lines =="
sed -n '230,290p' packages/adt-mcp/src/lib/tools/scope-catalogue.ts
sed -n '520,570p' packages/adt-mcp/src/lib/tools/scope-catalogue.ts

echo "== search access and safe_execute =="
rg -n "McpRequestAccess|isMcpToolAllowed|isAllowed|safe_execute|atc_run|run_unit_tests|scoped" packages/adt-mcp -S

Repository: abapify/adt-cli

Length of output: 34455


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== catalogue row for tool classes =="
sed -n '90,215p' packages/adt-mcp/src/lib/tools/scope-catalogue.ts

echo "== safe-execute tests around listed/list vs dispatch =="
sed -n '210,260p' packages/adt-mcp/tests/safe-execute-enforcement.test.ts
sed -n '360,410p' packages/adt-mcp/tests/safe-execute-enforcement.test.ts

echo "== standalone behavioral probe for listed vs allowed invariant =="
node - <<'JS'
const ops = {
  'atc_run': 'safe_execute',
  'run_unit_tests': 'safe_execute',
  'list_destinations': 'read',
};
function isMcpToolAllowed(access, name) {
  const classes = access?.classes;
  const scopedAnalysis =
    access?.scoped?.operationClass === 'safe_execute' &&
    (name === 'atc_run' || name === 'run_unit_tests');
  const classAllowed = Boolean(
    Array.isArray(classes) &&
    classes.every(c => ['server','read','safe_execute','write'].includes(String(c))) &&
    (classes.includes(ops[name]) ||
      (scopedAnalysis && classes.includes('safe_execute'))),
  );
  if (!classAllowed) return false;
  const scoped = access?.scoped;
  return (
    !scoped ||
    ((scoped.operationClass === ops[name] ||
      ((name === 'atc_run' || name === 'run_unit_tests') &&
        scoped.operationClass === 'safe_execute')) &&
      scoped.toolNames.includes(name))
  );
}
function isMcpToolListed(access, name) {
  const operationClass = ops[name];
  if (!access) return false;
  if (!Array.isArray(access.classes) || !access.classes.every(c => ['server','read','safe_execute','write'].includes(String(c)))) return false;
  if (access.scoped) {
    if (
      operationClass !== access.scoped.operationClass &&
      !(
        (name === 'atc_run' || name === 'run_unit_tests') &&
        access.scoped.operationClass === 'safe_execute'
      )
    ) {
      return false;
    }
    if (!access.scoped.toolNames.includes(name)) return false;
    return true;
  }
  return false;
}
const access = {
  classes: ['read'],
  destinations: ['dest'],
  destinationKeys: ['dest:ZFOO'],
  scoped: {
    tokenId: 'jti',
    principal: 'P',
    correlationId: 'corr',
    scopeId: 'scope',
    executionId: 'exec',
    systemSid: 'sid',
    resourceKeys: [],
    toolNames: ['atc_run','run_unit_tests'],
    operationClass: 'safe_execute',
    maxToolCalls: 1,
  },
};
for (const name of ['atc_run','run_unit_tests','list_destinations']) {
  console.log(`${name}: listed=${isMcpToolListed(access,name)} allowed=${isMcpToolAllowed(access,name)}`);
}
JS

Repository: abapify/adt-cli

Length of output: 6909


Keep scoped tool listing backed by a matching safe-execution class.

isMcpToolListed() currently allows atc_run/run_unit_tests when scoped.operationClass === 'safe_execute', even if classes lacks safe_execute. Dispatch rejects such calls through isMcpToolAllowed(), so the envelope can advertise tools that cannot be called. Require access.classes.includes('safe_execute') here as well, or reject these scoped grants during McpRequestAccess construction and update the test covering the listed vs allowed invariant.

🤖 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/src/lib/tools/scope-catalogue.ts` around lines 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.

Comment on lines +1 to +2
import assert from 'node:assert/strict';
import test from 'node:test';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate target and package/config files =="
git ls-files | rg '(^|/)(package\.json|vitest\.config|vite\.config|jest\.config|tsdown\.config|nx\.json*|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb)$|packages/adt-mcp/tests/delegated-assistant-catalogue\.test\.ts$' || true

echo
echo "== target file top =="
if [ -f packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts ]; then
  sed -n '1,120p' packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts
fi

echo
echo "== package manifests with devDependencies/scripts =="
git ls-files '*/package.json' | while read -r f; do
  if grep -E '"(vitest|jest|node:test|`@vitest`|tsdown|nx)"' "$f" >/dev/null 2>&1; then
    echo "--- $f"
    node -e "const p=require('./$f'); console.log(JSON.stringify({scripts:p.scripts,devDependencies:p.devDependencies||{},dependencies:p.dependencies||{}}, null, 2))"
  fi
done

echo
echo "== test scripts/configs reference =="
rg -n '"(test|vitest|jest|tsdown)"|vitest|jest|tsdown|node:test' package.json packages package-lock.json bun.lock 2>/dev/null | head -200 || true

Repository: abapify/adt-cli

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== packages/adt-mcp/package.json =="
if [ -f packages/adt-mcp/package.json ]; then
  node -e "const p=require('./packages/adt-mcp/package.json'); console.log(JSON.stringify({name:p.name,scripts:p.scripts,devDependencies:p.devDependencies||{},dependencies:p.dependencies||{}}, null, 2))"
fi

echo
echo "== tool runner manifests =="
for f in tools/nx-vitest/package.json tools/nx-tsdown/package.json package.json packages/adt-mcp/package.json; do
  if [ -f "$f" ]; then
    echo "--- $f"
    node -e "const fs=require('fs'); const p=JSON.parse(fs.readFileSync('$f','utf8')); console.log(JSON.stringify({name:p.name,scripts:p.scripts,devDependencies:p.devDependencies||{},dependencies:p.dependencies||{}}, null, 2))"
  fi
done

echo
echo "== related test files under packages/adt-mcp/tests =="
git ls-files packages/adt-mcp/tests | while read -r f; do
  echo "--- $f"
  sed -n '1,40p' "$f"
done

echo
echo "== node:test imports in repo =="
rg -n "from 'node:test'|from \"node:test\"|import test from 'node:test'|import test from \"node:test\"" . --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' || true

Repository: abapify/adt-cli

Length of output: 37192


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== repo test configs =="
git ls-files | rg '(^|/)(vitest|vite|jest|tsdown|nx)\.(json|config\..*)$|(^|/)jest\.config\..*$|(^|/)vitest\.config\..*$|(^|/)tsdown\.config\..*$' || true

echo
echo "== adt-mcp project references =="
rg -n '"adt-mcp"|`@abapify/adt-mcp`|adt-mcp:' nx.json package.json tools packages/adt-mcp --glob '*.json' --glob '*.ts' --glob '*.js' --glob '*.yml' --glob '*.yaml' | head -200 || true

echo
echo "== tests pattern in adt-mcp package config if any =="
if [ -f packages/adt-mcp/project.json ] || [ -w packages/adt-mcp/package.json ]; then
  for f in packages/adt-mcp/project.json ; do
    [ -f "$f" ] && { echo "--- $f"; cat "$f"; }
  done
fi
rg -n 'test|vitest|jest|node:test|scripts' packages/adt-mcp nx.json package.json 2>/dev/null || true

Repository: abapify/adt-cli

Length of output: 32593


Use Vitest-based test imports in adt-mcp.

This file still imports Node’s built-in test runner, and packages/adt-mcp/project.json uses the @nx/vitest plugin, so these tests won’t use the configured Vitest runner. Replace test/describe/it imports from node:test with vitest/@abapify/nx-vitest.

🤖 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 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.

Source: Coding guidelines

Comment on lines +1 to +7
import assert from 'node:assert/strict';
import { describe, it } from 'node:test';
import {
parseDelegatedAssistantReadPolicy,
type TrustedMcpInvocationClaims,
} from '../src/index.js';
import { isMcpInvocationDispatchPolicySupported } from '../src/lib/http/invocation.js';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^|/)package\.json$|vitest|jest|test\.config|packages/adt-mcp' | sed -n '1,160p'

echo
echo "== target file imports =="
cat -n packages/adt-mcp/tests/delegated-assistant-policy.test.ts | sed -n '1,80p'

echo
echo "== adtmcp package scripts/config =="
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('packages/adt-mcp/package.json')
if p.exists():
    j=json.loads(p.read_text())
    print(p)
    for k in ['scripts','devDependencies','dependencies']:
        print(k, j.get(k))
PY

echo
echo "== vitest references =="
rg -n "'vitest'|\"vitest\"|vitest|node:test|bun test|bunx|test:" -S -g 'package.json' -g 'vitest.*' -g 'tsdown*' -g '!node_modules' .

Repository: abapify/adt-cli

Length of output: 15665


Use Vitest for this new suite.

node:test bypasses the repository-required Vitest runner here. Import describe/it from vitest so this test participates in the repository test configuration.

Proposed fix
-import { describe, it } from 'node:test';
+import { describe, it } from 'vitest';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import assert from 'node:assert/strict';
import { describe, it } from 'node:test';
import {
parseDelegatedAssistantReadPolicy,
type TrustedMcpInvocationClaims,
} from '../src/index.js';
import { isMcpInvocationDispatchPolicySupported } from '../src/lib/http/invocation.js';
import assert from 'node:assert/strict';
import { describe, it } from 'vitest';
import {
parseDelegatedAssistantReadPolicy,
type TrustedMcpInvocationClaims,
} from '../src/index.js';
import { isMcpInvocationDispatchPolicySupported } from '../src/lib/http/invocation.js';
🤖 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-policy.test.ts` around lines 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.

Source: Coding guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant