feat: add scheduled sessions, export, workflow, and repo management tools#42
Conversation
WalkthroughAdds scheduled sessions (cron) management, session export, workflow and repository management across MCP client/server, formatters, and tests; updates documentation tool count from 26 to 41. No API surface removals or runtime schema/dispatch removals beyond added tools. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp_acp/client.py`:
- Around line 681-720: The update_scheduled_session method builds a sparse
payload for a partial update but currently uses PUT; change the HTTP verb in the
self._request call inside update_scheduled_session from "PUT" to "PATCH"
(keeping the same path and payload) so semantics match the docstring and other
partial-update endpoints, and update related tests to assert that the client
issues a PATCH request (add an assertion checking the HTTP method) to prevent
regressions.
- Around line 1064-1078: The remove_repo() method is incorrectly validating
repo_name with the DNS-1123 validator used for session names; update remove_repo
to only check repo_name is non-empty (use a simple non-empty string check
instead of self._validate_input(..., "repo_name")), and when building the DELETE
request path call self._request use a URL-encoded path segment for repo_name
(e.g., encode repo_name before interpolating into
f"/v1/sessions/{session}/repos/{repo_name}") so names like "foo_bar" or
"foo.bar" can be removed; keep existing validations for project and session and
continue to return the same dict response after the await self._request call.
In `@src/mcp_acp/server.py`:
- Around line 73-87: The _SESSION_TEMPLATE currently requires "task", but
acp_update_scheduled_session reuses it which prevents partial updates; change
the schema used by the update tool to a non-required version (e.g., create a new
_SESSION_UPDATE_TEMPLATE or clone _SESSION_TEMPLATE and remove the "required"
key / any nested required fields) and update acp_update_scheduled_session to
reference the new schema instead of _SESSION_TEMPLATE; make sure
ACPClient.update_scheduled_session remains accepting partial session_template
values.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9265dc19-2eb0-4998-add7-b40cab931d20
📒 Files selected for processing (8)
CLAUDE.mdsrc/mcp_acp/client.pysrc/mcp_acp/formatters.pysrc/mcp_acp/server.pytests/test_client.pytests/test_formatters.pytests/test_server.pytests/test_server_e2e.py
| async def update_scheduled_session( | ||
| self, | ||
| project: str, | ||
| name: str, | ||
| schedule: str | None = None, | ||
| display_name: str | None = None, | ||
| session_template: dict[str, Any] | None = None, | ||
| suspend: bool | None = None, | ||
| dry_run: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Update a scheduled session (partial update).""" | ||
| self._validate_input(project, "project") | ||
| self._validate_input(name, "name") | ||
|
|
||
| payload: dict[str, Any] = {} | ||
| if schedule is not None: | ||
| payload["schedule"] = schedule | ||
| if display_name is not None: | ||
| payload["displayName"] = display_name | ||
| if session_template is not None: | ||
| payload["sessionTemplate"] = session_template | ||
| if suspend is not None: | ||
| payload["suspend"] = suspend | ||
|
|
||
| if not payload: | ||
| raise ValueError("No fields to update. Provide schedule, display_name, session_template, or suspend.") | ||
|
|
||
| if dry_run: | ||
| return { | ||
| "dry_run": True, | ||
| "success": True, | ||
| "message": f"Would update scheduled session '{name}'", | ||
| "patch": payload, | ||
| } | ||
|
|
||
| try: | ||
| await self._request("PUT", f"/v1/scheduled-sessions/{name}", project, json_data=payload) | ||
| return {"updated": True, "message": f"Successfully updated scheduled session '{name}'"} | ||
| except ValueError as e: | ||
| return {"updated": False, "message": f"Failed to update: {str(e)}"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the exact code at the specified lines to confirm the HTTP method
sed -n '681,720p' src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 1599
🏁 Script executed:
# Search for any API documentation or contract definitions for scheduled-sessions
rg "scheduled-session|scheduled_session" -A 2 -B 2 --type py | head -100Repository: ambient-code/mcp
Length of output: 6199
🏁 Script executed:
# Look for tests related to scheduled-session updates
fd test -type f -name "*.py" | xargs rg "update_scheduled_session|scheduled.session" 2>/dev/null | head -50Repository: ambient-code/mcp
Length of output: 4342
🏁 Script executed:
# Search for any backend API specification or documentation files
fd -e md -e txt -e json | xargs rg "scheduled.session.*PUT|scheduled.session.*PATCH" 2>/dev/nullRepository: ambient-code/mcp
Length of output: 44
🏁 Script executed:
# Check how _request method is defined and what verbs it supports
rg "def _request\(" -A 10 src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 462
🏁 Script executed:
# Find and read the test_update_scheduled_session test
rg "test_update_scheduled_session" -A 20 tests/test_client.pyRepository: ambient-code/mcp
Length of output: 953
🏁 Script executed:
# Search for PATCH usage in the client
rg "PATCH" src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 596
🏁 Script executed:
# Check the full _request method implementation
sed -n '100,200p' src/mcp_acp/client.py | head -100Repository: ambient-code/mcp
Length of output: 3718
🏁 Script executed:
# Search for any API documentation or backend contract info
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs rg "scheduled.session.*PUT\|scheduled.session.*PATCH\|scheduled-sessions.*verb" 2>/dev/null || echo "No docs found with those patterns"Repository: ambient-code/mcp
Length of output: 93
🏁 Script executed:
# Check if there are other partial update methods in the client that use PUT or PATCH
rg "partial update\|sparse.*payload" -B 5 src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 42
Use PATCH instead of PUT for this partial update.
The method docstring states "partial update" and builds a sparse payload, but line 717 sends it with PUT. This creates a semantic mismatch: PUT typically means full replacement (omitted fields could be cleared), while PATCH means partial update. The codebase already uses PATCH for other partial updates on sessions. Either switch to PATCH or fetch and resend the full resource document. Additionally, the existing test does not validate the HTTP method, so add an assertion to prevent regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp_acp/client.py` around lines 681 - 720, The update_scheduled_session
method builds a sparse payload for a partial update but currently uses PUT;
change the HTTP verb in the self._request call inside update_scheduled_session
from "PUT" to "PATCH" (keeping the same path and payload) so semantics match the
docstring and other partial-update endpoints, and update related tests to assert
that the client issues a PATCH request (add an assertion checking the HTTP
method) to prevent regressions.
| async def export_session(self, project: str, session: str) -> dict[str, Any]: | ||
| """Export session chat as markdown.""" | ||
| self._validate_input(project, "project") | ||
| self._validate_input(session, "session") | ||
|
|
||
| text = await self._request_text("GET", f"/v1/sessions/{session}/export", project) | ||
| return {"export": text, "session": session} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the _request_text() method implementation
cd src/mcp_acp && grep -n "_request_text" client.py | head -20Repository: ambient-code/mcp
Length of output: 297
🏁 Script executed:
# Search for _request_text method definition
rg -n "def _request_text" src/mcp_acp/client.py -A 15Repository: ambient-code/mcp
Length of output: 661
🏁 Script executed:
# Check for any Accept header specifications in the codebase
rg -n "Accept" src/mcp_acp/ -B 2 -A 2Repository: ambient-code/mcp
Length of output: 602
🏁 Script executed:
# Look for export endpoint tests or documentation
fd -type f \( -name "*.py" -o -name "*.md" \) | xargs rg -l "export.*session\|export_session" 2>/dev/nullRepository: ambient-code/mcp
Length of output: 232
🏁 Script executed:
# Search for API specification files
fd -type f \( -name "*.json" -o -name "*.yaml" -o -name "*.yml" \) | head -20Repository: ambient-code/mcp
Length of output: 230
🏁 Script executed:
# Get the complete _request_text() method
sed -n '205,240p' src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 1418
🏁 Script executed:
# Look for test files related to export
fd -name "*test*.py" -o -name "test_*.py" | xargs rg -l "export_session\|export" 2>/dev/nullRepository: ambient-code/mcp
Length of output: 292
🏁 Script executed:
# Search for API specs or documentation
find . -name "*.md" -o -name "*.txt" -o -name "README*" | xargs rg -l "export\|markdown" 2>/dev/null | head -10Repository: ambient-code/mcp
Length of output: 42
🏁 Script executed:
# Check if there are any comments about markdown in the export context
rg -n "markdown\|export" src/mcp_acp/client.py -i -B 2 -A 2Repository: ambient-code/mcp
Length of output: 42
🏁 Script executed:
# Check if there are other methods that override Accept header
rg -n "Accept.*:" src/mcp_acp/ -B 3 -A 1Repository: ambient-code/mcp
Length of output: 668
🏁 Script executed:
# Look for backend or spec files
find . -type f -name "*.py" | xargs rg -l "export.*markdown\|markdown.*export" 2>/dev/nullRepository: ambient-code/mcp
Length of output: 44
🏁 Script executed:
# Check all usages of Accept header in the entire codebase
rg -r "Accept" . --type py 2>/dev/null | head -30Repository: ambient-code/mcp
Length of output: 4475
🏁 Script executed:
# Look for the export endpoint handler or documentation in comments
rg -n "export" src/mcp_acp/client.py -B 3 -A 3Repository: ambient-code/mcp
Length of output: 618
🏁 Script executed:
# Check if there's a different method that accepts custom headers
rg -n "def _request" src/mcp_acp/client.py -A 8Repository: ambient-code/mcp
Length of output: 674
🏁 Script executed:
# Look for any markdown-related Accept headers or content types
rg -i "markdown\|text/markdown\|application/vnd" src/mcp_acp/Repository: ambient-code/mcp
Length of output: 42
🏁 Script executed:
# Check if there are integration tests or docs that specify the expected format
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "export\|session" {} \;Repository: ambient-code/mcp
Length of output: 175
🏁 Script executed:
# Check API_REFERENCE.md for export endpoint specification
grep -n -i "export\|/v1/sessions" ./API_REFERENCE.md -A 5 -B 2Repository: ambient-code/mcp
Length of output: 3533
🏁 Script executed:
# Check README for export session information
grep -n -i "export\|markdown" ./README.md -A 3 -B 2Repository: ambient-code/mcp
Length of output: 1178
🏁 Script executed:
# Check CLAUDE.md for any context about export format
grep -n -i "export\|markdown" ./CLAUDE.md -A 3 -B 2Repository: ambient-code/mcp
Length of output: 155
🏁 Script executed:
# Look at the complete _request method to understand the pattern
sed -n '142,203p' src/mcp_acp/client.pyRepository: ambient-code/mcp
Length of output: 2454
Ensure export endpoint explicitly requests markdown format.
The export_session() method is documented to export as markdown, but _request_text() hardcodes Accept: text/plain. A strict backend requiring explicit markdown content negotiation could return 406 Not Acceptable. Either:
- Add an optional
accept_headerparameter to_request_text()and passtext/markdownfromexport_session(), or - Follow the pattern used by
get_session_transcript()(which accepts aformatquery parameter per API_REFERENCE.md) and add format negotiation to the export call.
Verify against the backend export endpoint spec to confirm expected content type or query parameter convention.
| async def add_repo(self, project: str, session: str, repo_url: str) -> dict[str, Any]: | ||
| """Add a repository to a running session.""" | ||
| self._validate_input(project, "project") | ||
| self._validate_input(session, "session") | ||
|
|
||
| return await self._request("POST", f"/v1/sessions/{session}/repos", project, json_data={"url": repo_url}) | ||
|
|
||
| async def remove_repo(self, project: str, session: str, repo_name: str) -> dict[str, Any]: | ||
| """Remove a repository from a session.""" | ||
| self._validate_input(project, "project") | ||
| self._validate_input(session, "session") | ||
| self._validate_input(repo_name, "repo_name") | ||
|
|
||
| await self._request("DELETE", f"/v1/sessions/{session}/repos/{repo_name}", project) | ||
| return {"removed": True, "message": f"Repo '{repo_name}' removed from session '{session}'"} |
There was a problem hiding this comment.
repo_name is validated with the wrong contract.
Line 1075 reuses the DNS-1123 validator, but repository names derived from URLs are not guaranteed to fit that shape. add_repo() accepts arbitrary repo URLs, so a repo with a common name like foo_bar or foo.bar can be added and then become impossible to remove through this tool. Validate only that the value is non-empty and URL-encode the path segment instead of treating it like a session name.
Proposed fix
+from urllib.parse import quote
+
async def remove_repo(self, project: str, session: str, repo_name: str) -> dict[str, Any]:
"""Remove a repository from a session."""
self._validate_input(project, "project")
self._validate_input(session, "session")
- self._validate_input(repo_name, "repo_name")
+ if not repo_name:
+ raise ValueError("repo_name must not be empty")
- await self._request("DELETE", f"/v1/sessions/{session}/repos/{repo_name}", project)
+ encoded_repo_name = quote(repo_name, safe="")
+ await self._request("DELETE", f"/v1/sessions/{session}/repos/{encoded_repo_name}", project)
return {"removed": True, "message": f"Repo '{repo_name}' removed from session '{session}'"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp_acp/client.py` around lines 1064 - 1078, The remove_repo() method is
incorrectly validating repo_name with the DNS-1123 validator used for session
names; update remove_repo to only check repo_name is non-empty (use a simple
non-empty string check instead of self._validate_input(..., "repo_name")), and
when building the DELETE request path call self._request use a URL-encoded path
segment for repo_name (e.g., encode repo_name before interpolating into
f"/v1/sessions/{session}/repos/{repo_name}") so names like "foo_bar" or
"foo.bar" can be removed; keep existing validations for project and session and
continue to return the same dict response after the await self._request call.
| _SESSION_TEMPLATE = { | ||
| "type": "object", | ||
| "description": "Session template (same as create_session: task, model, repos, displayName, etc.)", | ||
| "properties": { | ||
| "task": {"type": "string", "description": "The prompt/instructions"}, | ||
| "model": {"type": "string", "description": "LLM model", "default": "claude-sonnet-4"}, | ||
| "repos": { | ||
| "type": "array", | ||
| "items": {"type": "object", "properties": {"url": {"type": "string"}}}, | ||
| "description": "Repositories to clone", | ||
| }, | ||
| "displayName": {"type": "string", "description": "Display name for created sessions"}, | ||
| }, | ||
| "required": ["task"], | ||
| } |
There was a problem hiding this comment.
Use a different session_template schema for updates.
_SESSION_TEMPLATE makes task required, and acp_update_scheduled_session reuses it unchanged. That blocks valid partial updates like changing only model, repos, or suspend, even though ACPClient.update_scheduled_session() accepts a partial session_template. Split the create/update schemas, or drop the nested required fields for the update tool.
Also applies to: 470-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp_acp/server.py` around lines 73 - 87, The _SESSION_TEMPLATE currently
requires "task", but acp_update_scheduled_session reuses it which prevents
partial updates; change the schema used by the update tool to a non-required
version (e.g., create a new _SESSION_UPDATE_TEMPLATE or clone _SESSION_TEMPLATE
and remove the "required" key / any nested required fields) and update
acp_update_scheduled_session to reference the new schema instead of
_SESSION_TEMPLATE; make sure ACPClient.update_scheduled_session remains
accepting partial session_template values.
…ools Add 15 new MCP tools expanding the server from 26 to 41 tools: - Scheduled sessions (9): list, get, create, update, delete, suspend, resume, trigger, list_runs - Session export (1): export_session - Workflow management (2): set_workflow, get_workflow_metadata - Repo management (3): add_repo, remove_repo, get_repos_status All tools follow the 4-step pattern: client method, formatter, tool definition + dispatch, and unit tests. Closes ambient-code#28 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeremyeder
force-pushed
the
feat/scheduled-sessions-and-new-tools
branch
from
488edb4 to
b7f56e9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/mcp_acp/client.py (3)
681-718:⚠️ Potential issue | 🟠 MajorUse
PATCHfor scheduled-session partial updates.This method builds a sparse payload, but Line 717 still sends it via
PUT. On a full-replacement endpoint, omitted fields can be cleared, so a partial update can clobber existing config instead of patching it. Switch this call toPATCH; if the backend only supportsPUT, fetch and resend the full document instead. Also add a verb assertion in the client test so this cannot regress.Suggested fix
- await self._request("PUT", f"/v1/scheduled-sessions/{name}", project, json_data=payload) + await self._request("PATCH", f"/v1/scheduled-sessions/{name}", project, json_data=payload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_acp/client.py` around lines 681 - 718, The update_scheduled_session method currently builds a sparse payload but calls self._request with "PUT" which risks clobbering omitted fields; change the HTTP verb to "PATCH" in the call to self._request (in update_scheduled_session) so the server applies a partial update, and if your backend does not support PATCH implement the fallback: fetch the full scheduled-session via GET, merge the sparse payload into the full document, then send the merged document with PUT using the same _request helper. Also add/update a unit test to assert the client issues "PATCH" for partial updates (or the merge+PUT behavior) to prevent regressions.
836-842:⚠️ Potential issue | 🟠 MajorRequest markdown explicitly when exporting a session.
export_session()promises markdown, but it still uses_request_text(), which hardcodesAccept: text/plainat Line 222. A strict backend can return the wrong representation or reject the request. Let this path override the accept header to the export endpoint’s markdown media type, or use whatever format parameter the export spec defines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_acp/client.py` around lines 836 - 842, The export_session function currently relies on _request_text which forces Accept: text/plain; change export_session to request the markdown media type explicitly (e.g., Accept: text/markdown or the API's defined export media type) by either calling a lower-level request method that accepts custom headers or adding an optional accept/header override to _request_text, then pass the markdown accept header when calling export_session so the export endpoint returns markdown as promised; reference export_session and _request_text to locate and update the call site and/or signature.
1071-1078:⚠️ Potential issue | 🟠 Major
repo_nameneeds path-safe handling, not DNS-1123 validation.Line 1075 rejects valid repository names like
foo_barandfoo.bar, and Line 1077 interpolates the raw value into the URL path. That makes some repos addable but impossible to remove. Validate only thatrepo_nameis non-empty, then URL-encode the path segment before callingDELETE.Suggested fix
+from urllib.parse import quote + async def remove_repo(self, project: str, session: str, repo_name: str) -> dict[str, Any]: """Remove a repository from a session.""" self._validate_input(project, "project") self._validate_input(session, "session") - self._validate_input(repo_name, "repo_name") + if not repo_name: + raise ValueError("repo_name must not be empty") - await self._request("DELETE", f"/v1/sessions/{session}/repos/{repo_name}", project) + encoded_repo_name = quote(repo_name, safe="") + await self._request("DELETE", f"/v1/sessions/{session}/repos/{encoded_repo_name}", project) return {"removed": True, "message": f"Repo '{repo_name}' removed from session '{session}'"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_acp/client.py` around lines 1071 - 1078, The remove_repo method currently applies DNS-1123 style validation and interpolates repo_name raw into the URL, rejecting valid names like "foo_bar" and breaking deletes; change validation in remove_repo to only ensure repo_name is non-empty (use the existing _validate_input semantics or a simple truthy check instead of DNS-1123 rules) and URL-encode repo_name when constructing the path passed to self._request (i.e., encode the path segment used in f"/v1/sessions/{session}/repos/{repo_name}" before calling _request) so arbitrary valid repo names can be removed safely.src/mcp_acp/server.py (1)
73-87:⚠️ Potential issue | 🟠 MajorDon’t reuse the create-only template schema for scheduled-session updates.
_SESSION_TEMPLATErequirestask, andacp_update_scheduled_sessionreuses it unchanged. That blocks valid partial updates like changing onlymodelorrepos, even thoughACPClient.update_scheduled_session()accepts a partialsession_template. Define a separate update schema without required fields and use that for the update tool.Suggested fix
_SESSION_TEMPLATE = { "type": "object", "description": "Session template (same as create_session: task, model, repos, displayName, etc.)", "properties": { "task": {"type": "string", "description": "The prompt/instructions"}, "model": {"type": "string", "description": "LLM model", "default": "claude-sonnet-4"}, "repos": { "type": "array", "items": {"type": "object", "properties": {"url": {"type": "string"}}}, "description": "Repositories to clone", }, "displayName": {"type": "string", "description": "Display name for created sessions"}, }, "required": ["task"], } + +_SESSION_TEMPLATE_UPDATE = { + **_SESSION_TEMPLATE, + "required": [], +} @@ - "session_template": _SESSION_TEMPLATE, + "session_template": _SESSION_TEMPLATE_UPDATE,Also applies to: 470-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp_acp/server.py` around lines 73 - 87, The _SESSION_TEMPLATE schema currently requires "task" and is being reused for acp_update_scheduled_session, preventing partial updates; create a new schema (e.g., _SESSION_UPDATE_TEMPLATE) that mirrors _SESSION_TEMPLATE properties but has no "required" fields, then update the acp_update_scheduled_session tool to use this new schema instead of _SESSION_TEMPLATE so ACPClient.update_scheduled_session() can accept partial session_template updates (refer to symbols _SESSION_TEMPLATE, acp_update_scheduled_session, and ACPClient.update_scheduled_session).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/mcp_acp/client.py`:
- Around line 681-718: The update_scheduled_session method currently builds a
sparse payload but calls self._request with "PUT" which risks clobbering omitted
fields; change the HTTP verb to "PATCH" in the call to self._request (in
update_scheduled_session) so the server applies a partial update, and if your
backend does not support PATCH implement the fallback: fetch the full
scheduled-session via GET, merge the sparse payload into the full document, then
send the merged document with PUT using the same _request helper. Also
add/update a unit test to assert the client issues "PATCH" for partial updates
(or the merge+PUT behavior) to prevent regressions.
- Around line 836-842: The export_session function currently relies on
_request_text which forces Accept: text/plain; change export_session to request
the markdown media type explicitly (e.g., Accept: text/markdown or the API's
defined export media type) by either calling a lower-level request method that
accepts custom headers or adding an optional accept/header override to
_request_text, then pass the markdown accept header when calling export_session
so the export endpoint returns markdown as promised; reference export_session
and _request_text to locate and update the call site and/or signature.
- Around line 1071-1078: The remove_repo method currently applies DNS-1123 style
validation and interpolates repo_name raw into the URL, rejecting valid names
like "foo_bar" and breaking deletes; change validation in remove_repo to only
ensure repo_name is non-empty (use the existing _validate_input semantics or a
simple truthy check instead of DNS-1123 rules) and URL-encode repo_name when
constructing the path passed to self._request (i.e., encode the path segment
used in f"/v1/sessions/{session}/repos/{repo_name}" before calling _request) so
arbitrary valid repo names can be removed safely.
In `@src/mcp_acp/server.py`:
- Around line 73-87: The _SESSION_TEMPLATE schema currently requires "task" and
is being reused for acp_update_scheduled_session, preventing partial updates;
create a new schema (e.g., _SESSION_UPDATE_TEMPLATE) that mirrors
_SESSION_TEMPLATE properties but has no "required" fields, then update the
acp_update_scheduled_session tool to use this new schema instead of
_SESSION_TEMPLATE so ACPClient.update_scheduled_session() can accept partial
session_template updates (refer to symbols _SESSION_TEMPLATE,
acp_update_scheduled_session, and ACPClient.update_scheduled_session).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f06f40c6-d417-468b-94a4-f38c269b00c0
📒 Files selected for processing (8)
CLAUDE.mdsrc/mcp_acp/client.pysrc/mcp_acp/formatters.pysrc/mcp_acp/server.pytests/test_client.pytests/test_formatters.pytests/test_server.pytests/test_server_e2e.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Add 15 new MCP tools expanding the server from 26 to 41 tools:
All 15 tools follow the established 4-step pattern: client method → formatter → tool definition + dispatch → unit tests.
Tool Capabilities (41 total)
New in this PR
acp_list_scheduled_sessionsacp_get_scheduled_sessionacp_create_scheduled_sessionacp_update_scheduled_sessionacp_delete_scheduled_sessionacp_suspend_scheduled_sessionacp_resume_scheduled_sessionacp_trigger_scheduled_sessionacp_list_scheduled_session_runsacp_export_sessionacp_set_workflowacp_get_workflow_metadataacp_add_repoacp_remove_repoacp_get_repos_statusExisting (on main)
acp_list_sessionsacp_get_sessionacp_create_sessionacp_create_session_from_templateacp_delete_sessionacp_restart_sessionacp_clone_sessionacp_update_sessionacp_get_session_logsacp_get_session_transcriptacp_get_session_metricsacp_label_resourceacp_unlabel_resourceacp_list_sessions_by_labelacp_bulk_label_resourcesacp_bulk_unlabel_resourcesacp_bulk_delete_sessionsacp_bulk_stop_sessionsacp_bulk_restart_sessionsacp_bulk_delete_sessions_by_labelacp_bulk_stop_sessions_by_labelacp_bulk_restart_sessions_by_labelacp_list_clustersacp_whoamiacp_switch_clusteracp_loginTest plan
uv run pytest tests/— 232 tests passuv run ruff check .— no lint issuesuv run ruff format .— properly formattedCloses #28
🤖 Generated with Claude Code