Feat/add mcp server capacities for tasks and audits #2919
Conversation
WalkthroughAdds task-template CRUD support across server, tools, and resolvers; introduces an HTTP DELETE client helper; expands some read/update tool payloads and outputs; updates docs and .gitignore to support an env template workflow. Changes
Sequence DiagramsequenceDiagram
actor User
participant Server as MCP Server
participant Resolver as Resolvers
participant Client as HTTP Client
participant API as Remote API
rect rgb(230, 245, 255)
Note over Server,API: Task Template CRUD (high level)
User->>Server: invoke task-template operation
alt List / Detail
Server->>API: GET /task-templates/ or /task-templates/{id}/
API-->>Server: 200 with JSON
else Create
Server->>Resolver: resolve_folder_id(...) / resolve_applied_control_id(...)
Resolver-->>Server: folder/control UUIDs
Server->>Client: POST /task-templates/ (payload)
Client->>API: POST request
API-->>Client: 201 Created
else Update
Server->>Resolver: resolve_task_template_id(name_or_id)
Resolver->>API: GET /task-templates/?name=...
API-->>Resolver: results
Resolver-->>Server: resolved UUID
Server->>Client: PATCH /task-templates/{id}/
Client->>API: PATCH request
API-->>Client: 200 OK
else Delete
Server->>Resolver: resolve_task_template_id(name_or_id)
Resolver-->>Server: resolved UUID
Server->>Client: make_delete_request(/task-templates/{id}/)
Client->>API: DELETE request
API-->>Client: 204 No Content
end
Server-->>User: success / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
jledoze
force-pushed
the
feat/add_mcp_server_capacities_for_tasks_and_audits
branch
2 times, most recently
from
313f9cf to
ed5bd3b
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: 1
🧹 Nitpick comments (8)
cli/ca_mcp/tools/write_tools.py (1)
906-1003: create_task_template payload construction and validation look correctFolder resolution, optional-field handling, and status validation are all implemented sensibly and match surrounding patterns.
One small improvement to reduce drift risk: the
valid_statuseslist here is duplicated inupdate_task_template(cli/ca_mcp/tools/update_tools.py). Consider hoisting this list to a shared constant (e.g.VALID_TASK_TEMPLATE_STATUSES) in one module and reusing it in both places.cli/ca_mcp/resolvers.py (1)
326-353: Task template resolver aligns with existing resolver conventions
resolve_task_template_idcorrectly mirrors the existing resolve helpers: UUID short‑circuit, name lookup via/task-templates/, pagination handling, and clear error messages for not‑found and ambiguous cases. This is consistent and easy to reason about.If you want to reduce duplication later, you could reuse
resolve_id_or_name("/task-templates/")instead of a bespoke resolver, but it’s fine as-is.cli/ca_mcp/tools/read_tools.py (3)
111-129: Expanded applied-control table is correct; consider clearer UUID/cost formattingThe new columns (UUID, Category, CSF Function, Effort, Impact, Priority, Cost) are wired up correctly to the corresponding keys.
Two optional UX tweaks:
- Use a safe default for
uuidto avoidNonerendering in the table:- uuid = item.get("id") + uuid = item.get("id", "N/A")
- Since
costis a structured JSON object, printing it directly will show a raw Python dict. If you want a more readable output, consider serializing it, e.g.:- cost = item.get("cost", 0) + cost = item.get("cost") + if isinstance(cost, (dict, list)): + cost_display = json.dumps(cost) + else: + cost_display = cost if cost is not None else "N/A" ... - result += f"|{uuid}|{ref_id}|{name}|{status}|{eta}|{domain}|{category}|{csf_function}|{effort}|{impact}|{priority}|{cost}|\n" + result += f"|{uuid}|{ref_id}|{name}|{status}|{eta}|{domain}|{category}|{csf_function}|{effort}|{impact}|{priority}|{cost_display}|\n"
906-920: Description column may need truncation for readabilityAdding
Descriptionto the requirement assessments table is useful, butdescription = req.get("description", "N/A")is unbounded, whereasrequirementis truncated to 30 chars. For long descriptions this can produce very wide tables.If you want consistent formatting, consider truncating the description similarly, e.g.:
- description = req.get("description", "N/A") + raw_description = req.get("description", "N/A") or "N/A" + description = raw_description[:60](or whatever width matches your usual outputs).
1136-1176: get_task_template_details returns rich data; consider normalizing folder and relation fieldsThe detail row construction is functionally correct, but a few fields are likely complex objects/lists:
folderis printed viatask.get('folder', 'N/A'), which may be a dict (e.g., with"str"and"id") like in other endpoints, leading to a raw dict in the table.assets,applied_controls,compliance_assessments,risk_assessments, andassigned_toare printed as full lists, which may be noisy.For a cleaner, more consistent output (and to match patterns elsewhere in this module), you could, for example:
- result += f"|{task.get('folder', 'N/A')}" + folder_obj = task.get('folder') or {} + folder_str = folder_obj.get('str', folder_obj) if isinstance(folder_obj, dict) else folder_obj or "N/A" + result += f"|{folder_str}" ... - result += f"|{task.get('assets', [])}" + assets = task.get('assets', []) + result += f"|{len(assets)} asset(s)" if isinstance(assets, list) else f"|{assets}"(and similar count-or-summary formatting for the other relationship lists).
This keeps the API flexible but produces more readable tables.
cli/ca_mcp/tools/update_tools.py (3)
214-257: Align cost type hint with documented JSON object structureThe docstring for
update_applied_controldescribescostas a JSON object with nested keys (currency, amortization_period, build/run with fixed_cost and people_days), but the type hint iscost: list = None, and it’s passed through verbatim.To avoid confusion for callers and linters, it would be clearer to type this as a mapping/dict:
- effort: str = None, - cost: list = None, + effort: str = None, + cost: dict = None, ... - if cost is not None: - payload["cost"] = cost + if cost is not None: + payload["cost"] = cost(no behavioral change, but the hint now matches the expected JSON shape).
Also applies to: 279-280
308-320: Document the new applied_controls parameter in update_requirement_assessmentThe new
applied_controls: list = Noneparameter is correctly included in the payload when provided, but it’s not mentioned in the docstring. That may surprise users of this tool.Consider extending the docstring to describe it and clarify what’s expected (e.g., list of applied-control UUIDs):
async def update_requirement_assessment( @@ - """Update requirement assessment in audit. Use get_requirement_assessments() to find IDs + """Update requirement assessment in audit. Use get_requirement_assessments() to find IDs @@ - selected: Applicability flag + selected: Applicability flag + applied_controls: List of applied control UUIDs to associate with this requirementOptionally, if you want parity with other functions, you could later add an option to resolve names to IDs, but the current behavior is technically fine if only UUIDs are expected.
Also applies to: 322-332, 356-357
724-830: update_task_template works; consider resolving task_id and reusing status constantsThe update logic itself looks good: only provided fields are included, status is validated, folder names are resolved to IDs, and an empty payload is rejected.
Two optional consistency improvements:
- Allow name-or-ID for task_id, like delete_task_template
delete_task_templatealready usesresolve_task_template_idso callers can pass either a UUID or a name. To mirror that behavior here:- try: - # Build update payload with only provided fields - payload = {} + try: + # Resolve task name to ID if needed + resolved_task_id = resolve_task_template_id(task_id) + + # Build update payload with only provided fields + payload = {} @@ - res = make_patch_request(f"/task-templates/{task_id}/", payload) + res = make_patch_request(f"/task-templates/{resolved_task_id}/", payload)
- Share the valid status list with create_task_template
The samevalid_statuses = ["pending", "in_progress", "cancelled", "completed"]list exists here and increate_task_template. Extracting a shared constant (e.g. in this module and referencing it fromwrite_tools, or vice‑versa) would help avoid drift if statuses ever change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/ca_mcp/client.py(1 hunks)cli/ca_mcp/resolvers.py(1 hunks)cli/ca_mcp/server.py(6 hunks)cli/ca_mcp/tools/read_tools.py(3 hunks)cli/ca_mcp/tools/update_tools.py(7 hunks)cli/ca_mcp/tools/write_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cli/ca_mcp/server.py (3)
cli/ca_mcp/tools/read_tools.py (2)
get_task_templates(1083-1133)get_task_template_details(1136-1176)cli/ca_mcp/tools/write_tools.py (1)
create_task_template(906-1003)cli/ca_mcp/tools/update_tools.py (2)
update_task_template(724-830)delete_task_template(833-850)
cli/ca_mcp/tools/update_tools.py (2)
cli/ca_mcp/client.py (4)
make_get_request(24-42)make_patch_request(66-84)make_delete_request(87-103)get_paginated_results(123-137)cli/ca_mcp/resolvers.py (2)
resolve_folder_id(6-34)resolve_task_template_id(326-353)
cli/ca_mcp/tools/write_tools.py (2)
cli/ca_mcp/resolvers.py (1)
resolve_folder_id(6-34)cli/ca_mcp/client.py (1)
make_post_request(45-63)
cli/ca_mcp/resolvers.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
cli/ca_mcp/tools/read_tools.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
🔇 Additional comments (3)
cli/ca_mcp/client.py (1)
87-103: DELETE helper is consistent with existing client patterns
make_delete_requestmirrors the other helpers (GET/POST/PATCH) in how it builds the URL, applies headers, TLS verification, and timeout. Looks good and ready to use.cli/ca_mcp/server.py (1)
9-30: Task template tools are correctly wired into the MCP serverThe new task-template functions (
get_task_templates,get_task_template_details,create_task_template,update_task_template,delete_task_template) are:
- Properly imported from their respective tool modules, and
- Registered with
mcp.tool()so they’re exposed via the MCP server.The wiring is consistent with existing tools.
Also applies to: 43-58, 60-70, 91-93, 114-115, 123-124
cli/ca_mcp/tools/update_tools.py (1)
833-850: delete_task_template correctly resolves names and uses DELETE helper
delete_task_template:
- Accepts either name or UUID via
resolve_task_template_id, and- Uses
make_delete_requestand checks for HTTP 204 before reporting success.This is consistent with the rest of the client and resolver patterns. As long as the backend uses 204 for successful deletes (which seems to be the expectation), this looks good.
| async def get_task_templates(limit: int = None, offset: int = None, ordering: str = None, search: str = None): | ||
| """List task templates with IDs, names, and details | ||
| Args: | ||
| limit: Number of results to return per page | ||
| offset: The initial index from which to return the results | ||
| ordering: Which field to use when ordering the results | ||
| search: A search term | ||
| """ | ||
| try: | ||
| params = {} | ||
|
|
||
| if limit is not None: | ||
| params["limit"] = limit | ||
| if offset is not None: | ||
| params["offset"] = offset | ||
| if ordering: | ||
| params["ordering"] = ordering | ||
| if search: | ||
| params["search"] = search | ||
|
|
||
| res = make_get_request("/task-templates/", params=params) | ||
|
|
||
| if res.status_code != 200: | ||
| return f"Error: HTTP {res.status_code} - {res.text}" | ||
|
|
||
| data = res.json() | ||
| tasks = get_paginated_results(data) | ||
|
|
||
| if not tasks: | ||
| return "No task tasks found" | ||
|
|
||
| result = f"Found {len(tasks)} task templates\n\n" | ||
| result += "|ID|Name|Description|Ref ID|Status|Recurrent|Enabled|Task Date|\n" | ||
| result += "|---|---|---|---|---|---|---|---|\n" | ||
|
|
||
| for task in tasks: | ||
| task_id = task.get("id", "N/A") | ||
| name = task.get("name", "N/A") | ||
| description = (task.get("description", "N/A") or "N/A")[:40] # Truncate | ||
| ref_id = task.get("ref_id", "N/A") | ||
| status = task.get("status", "N/A") | ||
| is_recurrent = "Yes" if task.get("is_recurrent") else "No" | ||
| enabled = "Yes" if task.get("enabled") else "No" | ||
| task_date = task.get("task_date", "N/A") | ||
|
|
||
| result += f"|{task_id}|{name}|{description}|{ref_id}|{status}|{is_recurrent}|{enabled}|{task_date}|\n" | ||
|
|
||
| return result | ||
| except Exception as e: | ||
| return f"Error in get_task_templates: {str(e)}" |
There was a problem hiding this comment.
Fix typo in empty‑result message for get_task_templates
The function works correctly, but the empty‑result message has a small typo: "No task tasks found".
Recommend updating it to "No task templates found":
- if not tasks:
- return "No task tasks found"
+ if not tasks:
+ return "No task templates found"📝 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.
| async def get_task_templates(limit: int = None, offset: int = None, ordering: str = None, search: str = None): | |
| """List task templates with IDs, names, and details | |
| Args: | |
| limit: Number of results to return per page | |
| offset: The initial index from which to return the results | |
| ordering: Which field to use when ordering the results | |
| search: A search term | |
| """ | |
| try: | |
| params = {} | |
| if limit is not None: | |
| params["limit"] = limit | |
| if offset is not None: | |
| params["offset"] = offset | |
| if ordering: | |
| params["ordering"] = ordering | |
| if search: | |
| params["search"] = search | |
| res = make_get_request("/task-templates/", params=params) | |
| if res.status_code != 200: | |
| return f"Error: HTTP {res.status_code} - {res.text}" | |
| data = res.json() | |
| tasks = get_paginated_results(data) | |
| if not tasks: | |
| return "No task tasks found" | |
| result = f"Found {len(tasks)} task templates\n\n" | |
| result += "|ID|Name|Description|Ref ID|Status|Recurrent|Enabled|Task Date|\n" | |
| result += "|---|---|---|---|---|---|---|---|\n" | |
| for task in tasks: | |
| task_id = task.get("id", "N/A") | |
| name = task.get("name", "N/A") | |
| description = (task.get("description", "N/A") or "N/A")[:40] # Truncate | |
| ref_id = task.get("ref_id", "N/A") | |
| status = task.get("status", "N/A") | |
| is_recurrent = "Yes" if task.get("is_recurrent") else "No" | |
| enabled = "Yes" if task.get("enabled") else "No" | |
| task_date = task.get("task_date", "N/A") | |
| result += f"|{task_id}|{name}|{description}|{ref_id}|{status}|{is_recurrent}|{enabled}|{task_date}|\n" | |
| return result | |
| except Exception as e: | |
| return f"Error in get_task_templates: {str(e)}" | |
| async def get_task_templates(limit: int = None, offset: int = None, ordering: str = None, search: str = None): | |
| """List task templates with IDs, names, and details | |
| Args: | |
| limit: Number of results to return per page | |
| offset: The initial index from which to return the results | |
| ordering: Which field to use when ordering the results | |
| search: A search term | |
| """ | |
| try: | |
| params = {} | |
| if limit is not None: | |
| params["limit"] = limit | |
| if offset is not None: | |
| params["offset"] = offset | |
| if ordering: | |
| params["ordering"] = ordering | |
| if search: | |
| params["search"] = search | |
| res = make_get_request("/task-templates/", params=params) | |
| if res.status_code != 200: | |
| return f"Error: HTTP {res.status_code} - {res.text}" | |
| data = res.json() | |
| tasks = get_paginated_results(data) | |
| if not tasks: | |
| return "No task templates found" | |
| result = f"Found {len(tasks)} task templates\n\n" | |
| result += "|ID|Name|Description|Ref ID|Status|Recurrent|Enabled|Task Date|\n" | |
| result += "|---|---|---|---|---|---|---|---|\n" | |
| for task in tasks: | |
| task_id = task.get("id", "N/A") | |
| name = task.get("name", "N/A") | |
| description = (task.get("description", "N/A") or "N/A")[:40] # Truncate | |
| ref_id = task.get("ref_id", "N/A") | |
| status = task.get("status", "N/A") | |
| is_recurrent = "Yes" if task.get("is_recurrent") else "No" | |
| enabled = "Yes" if task.get("enabled") else "No" | |
| task_date = task.get("task_date", "N/A") | |
| result += f"|{task_id}|{name}|{description}|{ref_id}|{status}|{is_recurrent}|{enabled}|{task_date}|\n" | |
| return result | |
| except Exception as e: | |
| return f"Error in get_task_templates: {str(e)}" |
🤖 Prompt for AI Agents
In cli/ca_mcp/tools/read_tools.py around lines 1083 to 1133, the empty-result
message incorrectly reads "No task tasks found"; update that string to "No task
templates found" so the output is accurate; make the single-line change where
the function returns the empty message.
ed5bd3b to
a958b55
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cli/ca_mcp/tools/read_tools.py (1)
1113-1113: Fix typo in empty-result message.The message reads "No task tasks found" but should be "No task templates found".
Apply this diff:
- return "No task tasks found" + return "No task templates found"
🧹 Nitpick comments (2)
cli/ca_mcp/resolvers.py (1)
326-353: Consider showing sample task names in ambiguity error for consistency.The function correctly resolves task template names to UUIDs. However, for consistency with
resolve_folder_id(lines 27-31), consider including sample task names in the ambiguity error message.Apply this diff:
if len(tasks) > 1: + task_names = [t["name"] for t in tasks[:3]] raise ValueError( - f"Ambiguous task template name '{task_name_or_id}', found {len(tasks)}" + f"Ambiguous task template name '{task_name_or_id}', found {len(tasks)}: {task_names}" )cli/ca_mcp/tools/read_tools.py (1)
906-920: Consider truncating the description field for table readability.The description field is not truncated, which could result in very wide tables if descriptions are long. For consistency with the "Requirement" field (line 912, truncated to 30 chars), consider truncating the description as well.
Apply this diff:
- description = req.get("description", "N/A") + description = (req.get("description", "N/A") or "N/A")[:40] # Truncate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/ca_mcp/client.py(1 hunks)cli/ca_mcp/resolvers.py(1 hunks)cli/ca_mcp/server.py(6 hunks)cli/ca_mcp/tools/read_tools.py(3 hunks)cli/ca_mcp/tools/update_tools.py(7 hunks)cli/ca_mcp/tools/write_tools.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/ca_mcp/client.py
🧰 Additional context used
🧬 Code graph analysis (5)
cli/ca_mcp/server.py (3)
cli/ca_mcp/tools/read_tools.py (2)
get_task_templates(1083-1133)get_task_template_details(1136-1176)cli/ca_mcp/tools/write_tools.py (1)
create_task_template(906-1003)cli/ca_mcp/tools/update_tools.py (2)
update_task_template(724-830)delete_task_template(833-850)
cli/ca_mcp/resolvers.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
cli/ca_mcp/tools/read_tools.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
cli/ca_mcp/tools/write_tools.py (2)
cli/ca_mcp/resolvers.py (1)
resolve_folder_id(6-34)cli/ca_mcp/client.py (1)
make_post_request(45-63)
cli/ca_mcp/tools/update_tools.py (2)
cli/ca_mcp/client.py (4)
make_get_request(24-42)make_patch_request(66-84)make_delete_request(87-103)get_paginated_results(123-137)cli/ca_mcp/resolvers.py (2)
resolve_folder_id(6-34)resolve_task_template_id(326-353)
🔇 Additional comments (5)
cli/ca_mcp/tools/write_tools.py (1)
906-1003: LGTM! Task template creation function is well-structured.The function correctly resolves the folder ID, validates the status field, and conditionally builds the payload based on provided parameters. The implementation follows established patterns in this file.
cli/ca_mcp/server.py (1)
28-29: LGTM! Task template tooling correctly registered.The new task template operations (read, create, update, delete) are properly imported and registered with the MCP server, following the established pattern.
Also applies to: 57-57, 68-69, 91-92, 114-114, 123-124
cli/ca_mcp/tools/read_tools.py (2)
111-128: LGTM! Enhanced applied controls output with additional fields.The table correctly includes the new columns (UUID, Category, CSF Function, Effort, Impact, Priority, Cost) and properly extracts corresponding data from the API response.
1136-1176: LGTM! Task template details function correctly implemented.The function properly retrieves and formats detailed task template information. The single-row table format effectively displays all available fields.
cli/ca_mcp/tools/update_tools.py (1)
833-850: LGTM! Delete task template function correctly implemented.The function properly resolves the task template identifier and performs the DELETE operation with appropriate status code checking.
cli/ca_mcp/tools/update_tools.py
Outdated
| category: str = None, | ||
| csf_function: str = None, | ||
| effort: str = None, | ||
| cost: list = None, |
There was a problem hiding this comment.
Fix type hint for cost parameter.
The documentation (lines 242-250) describes cost as a JSON object with nested structure, but the type hint on line 223 indicates list = None. This should be dict = None to match the documented structure.
Apply this diff:
- cost: list = None,
+ cost: dict = None,Also applies to: 242-250, 279-280
🤖 Prompt for AI Agents
In cli/ca_mcp/tools/update_tools.py around line 223 (and similarly update the
related docs/usages at lines 242-250 and 279-280), the parameter `cost` is
incorrectly type-hinted as `list = None` while documentation describes it as a
JSON object with nested structure; change the type hint to `dict = None` (and
update any other occurrences/annotations and docstrings to match) so the
function signature, type hints, and documentation consistently reflect that
`cost` is an optional dict representing the nested JSON structure.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/ca_mcp/tools/update_tools.py (1)
318-333: Resolveapplied_controlsnames to IDs inupdate_requirement_assessment.The docstring now promises that
applied_controlscan be a list of IDs or names:applied_controls: List of applied control IDs/names ...but the implementation simply forwards the list:
if applied_controls is not None: payload["applied_controls"] = applied_controlsUnlike other update helpers (e.g.,
update_risk_scenario,update_quantitative_risk_hypothesis), this never callsresolve_applied_control_id, so passing names will not work as advertised.Since
resolve_applied_control_idis already imported at module level, you can mirror the existing pattern:- if applied_controls is not None: - payload["applied_controls"] = applied_controls + if applied_controls is not None: + resolved_controls = [] + for control in applied_controls: + resolved_control_id = resolve_applied_control_id(control) + resolved_controls.append(resolved_control_id) + payload["applied_controls"] = resolved_controlsThis preserves the semantics (
Noneleaves unchanged, empty list clears associations) while allowing both UUIDs and names.Also applies to: 357-358
♻️ Duplicate comments (2)
cli/ca_mcp/tools/update_tools.py (1)
214-250: Aligncosttype hint with its documented JSON object structure.The docstring (lines 242‑250) describes
costas a JSON object with nested keys, but the signature still hints it aslist:cost: list = None,This diverges from both the documentation and the actual payload usage (
payload["cost"] = cost), and echoes the earlier review feedback.Recommend updating the type hint to
dict:- effort: str = None, - cost: list = None, + effort: str = None, + cost: dict = None,This will keep the function signature, docstring, and expected payload shape consistent.
Also applies to: 279-280
cli/ca_mcp/tools/read_tools.py (1)
1083-1133:get_task_templatesis sound; consider refining the empty‑result message.The new listing function correctly:
- Supports pagination and search via
limit,offset,ordering, andsearch.- Uses
get_paginated_resultsto handle both paginated and non‑paginated responses.- Produces a concise, well‑structured table of task template metadata.
Minor wording nit:
if not tasks: return "No task found"Given the function name and header,
"No task templates found"would be clearer and consistent with other helpers (e.g., “No quantitative risk scenarios found”).- if not tasks: - return "No task found" + if not tasks: + return "No task templates found"
🧹 Nitpick comments (1)
cli/ca_mcp/tools/read_tools.py (1)
1136-1176:get_task_template_detailsis useful; improve folder display and optional name resolution.The detailed view is comprehensive and the column ordering matches the data appended. Two suggestions:
- Render folder consistently with other tables.
Elsewhere, folder fields come as objects with a
"str"key and are rendered via(item.get("folder") or {}).get("str", "N/A"). Here you currently have:result += f"|{task.get('folder', 'N/A')}"which will likely print the raw dict. Consider:
- task = res.json() - - # Create result + task = res.json() + + # Normalize folder display (string or "N/A") + folder_display = (task.get("folder") or {}).get("str", "N/A") + + # Create result result = f"|ID|Name|Description|Ref ID|Status|Task Date|Recurrent|Enabled|Published|Link|Folder|Path|Observation|Evidences|Created|Updated|Assets|Applied Controls|Compliance Assessment|Risk Assessment|Assign To|\n" result += f"|{task.get('id', 'N/A')}|{task.get('name', 'N/A')}" ... - result += f"|{task.get('link', 'N/A')}" - result += f"|{task.get('folder', 'N/A')}" + result += f"|{task.get('link', 'N/A')}" + result += f"|{folder_display}"
- (Optional) Support name‑or‑ID lookup like update/delete helpers.
Currently
task_idis treated strictly as an ID. If you want parity withupdate_task_template/delete_task_template, you could useresolve_task_template_id(task_id)before callingmake_get_request, and adjust the docstring to mention ID/name. This is optional but would make the read path more forgiving.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/.mcp.env(1 hunks)cli/ca_mcp/tools/read_tools.py(3 hunks)cli/ca_mcp/tools/update_tools.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/ca_mcp/tools/update_tools.py (2)
cli/ca_mcp/client.py (4)
make_get_request(24-42)make_patch_request(66-84)make_delete_request(87-103)get_paginated_results(123-137)cli/ca_mcp/resolvers.py (5)
resolve_asset_id(168-193)resolve_folder_id(6-34)resolve_applied_control_id(226-253)resolve_requirement_assessment_id(256-263)resolve_task_template_id(326-353)
cli/ca_mcp/tools/read_tools.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
🔇 Additional comments (3)
cli/ca_mcp/tools/update_tools.py (2)
3-12: New client and resolver imports look consistent with added task‑template operations.The additions of
make_delete_request,get_paginated_results, andresolve_task_template_idline up with the newupdate_task_template/delete_task_templatefunctions below and keep usage consistent with other tools.
837-854:delete_task_templatecorrectly resolves names to IDs and uses DELETE.The delete helper cleanly:
- Resolves
task_idviaresolve_task_template_id, so both names and UUIDs are supported.- Calls
make_delete_requestwith/task-templates/{resolved_task_id}/.- Treats HTTP 204 as success and surfaces other statuses in a readable error string.
- Wraps everything in a try/except that returns a clear error message.
This is consistent with the rest of the MCP tools.
cli/ca_mcp/tools/read_tools.py (1)
906-907: Description column addition for requirement assessments looks correct.Adding
Descriptionto the header and wiring it toreq.get("description", "N/A")in the row construction keeps the table aligned and provides more context without changing existing behavior.No functional issues spotted here.
Also applies to: 913-913, 920-920
cli/.mcp.env
Outdated
| TOKEN=8ef43fbaefbd84510552dfab02761003be8e99c02560486c21a078ff1a631761 | ||
| VERIFY_CERTIFICATE=true | ||
| API_URL=https://kantox.ciso-assistant.com//api |
There was a problem hiding this comment.
Do not commit real tokens or tenant‑specific API URLs in .mcp.env.
- Line 1 looks like a real bearer/API token checked into VCS. This is a serious secret‑management issue: any value that has ever been live should be considered compromised and revoked/rotated immediately.
- The repo‑tracked
.mcp.envshould use placeholders (e.g.TOKEN=<your_token_here>) and, ideally, be an example file (.mcp.env.example), with the real.mcp.envgit‑ignored. - Line 3 has a double slash before
/api(...com//api), which will generate URLs like...com//api/.... Most stacks tolerate this, but it’s unnecessary and can be normalized to a single slash.
Consider:
-TOKEN=8ef43fbaefbd84510552dfab02761003be8e99c02560486c21a078ff1a631761
-VERIFY_CERTIFICATE=true
-API_URL=https://kantox.ciso-assistant.com//api
+TOKEN=<your_token_here>
+VERIFY_CERTIFICATE=true
+API_URL=https://<your-instance-host>/apiAnd ensure any previously used token is revoked server‑side.
🤖 Prompt for AI Agents
In cli/.mcp.env around lines 1 to 3, the file currently contains a real token
and a tenant-specific API URL with a double slash; replace the token value with
a placeholder (e.g. TOKEN=<your_token_here>), change this tracked file into an
example file (rename to .mcp.env.example) and ensure the real .mcp.env is added
to .gitignore, and normalize the API_URL to remove the duplicate slash
(API_URL=https://<tenant-domain>/api). Also treat the exposed token as
compromised and ensure it is revoked/rotated server-side immediately.
| result += "|UUID|Ref|Name|Status|ETA|Domain|Category|CSF Function|Effort|Impact|Priority|Cost|\n" | ||
| result += "|---|---|---|---|---|---|---|---|---|---|---|---|\n" |
There was a problem hiding this comment.
Improve get_applied_controls UUID and cost display defaults.
The new columns are useful, but two small details could improve correctness/readability:
uuid = item.get("id")will renderNoneifidis ever missing; other tables typically default to"N/A".cost = item.get("cost", 0)will show0when the cost field is absent, which can be misleading given thatcostis modeled as a complex JSON object elsewhere.
Consider:
- for item in controls:
- uuid = item.get("id")
+ for item in controls:
+ uuid = item.get("id", "N/A")
...
- priority = item.get("priority", "N/A")
- cost = item.get("cost", 0)
+ priority = item.get("priority", "N/A")
+ cost = item.get("cost", "N/A")This keeps the table consistent with other read tools and avoids treating missing structured cost as a numeric zero.
Also applies to: 115-128
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/ca_mcp/tools/update_tools.py (1)
318-333: Resolveapplied_controlsnames to IDs inupdate_requirement_assessmentDocstring says
applied_controlsaccepts IDs or names, but the implementation sends the list through unchanged. If callers pass names, the API will likely reject the payload.To mirror
update_risk_scenarioandupdate_task_template, resolve each entry:- if selected is not None: - payload["selected"] = selected - if applied_controls is not None: - payload["applied_controls"] = applied_controls + if selected is not None: + payload["selected"] = selected + if applied_controls is not None: + resolved_controls = [] + for control in applied_controls: + resolved_control_id = resolve_applied_control_id(control) + resolved_controls.append(resolved_control_id) + payload["applied_controls"] = resolved_controlsThis preserves the existing semantics:
Noneleaves associations unchanged;[]clears them.Also applies to: 357-358
♻️ Duplicate comments (2)
cli/ca_mcp/tools/read_tools.py (2)
1083-1133: Tweak empty‑result message for task templatesFunction works as expected, but the empty message
"No task found"is a bit vague given this tool is task‑template–specific and earlier feedback requested a template‑oriented message.Consider:
- if not tasks: - return "No task found" + if not tasks: + return "No task templates found"
111-112: Align UUID and cost defaults with other read tools
uuid = item.get("id")will renderNonewhen missing, andcost = item.get("cost", 0)treats a missing structured cost as numeric zero. Other tables tend to default missing values to"N/A"for readability and to avoid implying a real zero.Consider:
- for item in controls: - uuid = item.get("id") + for item in controls: + uuid = item.get("id", "N/A") ... - priority = item.get("priority", "N/A") - cost = item.get("cost", 0) + priority = item.get("priority", "N/A") + cost = item.get("cost", "N/A")Also applies to: 115-126, 128-128
🧹 Nitpick comments (2)
cli/ca_mcp/tools/read_tools.py (1)
1136-1176: Improve formatting of nested fields inget_task_template_detailsThe table currently prints complex fields like
folder,assets,applied_controls,compliance_assessments, andrisk_assessmentsvia rawdict/listrepresentations, which can be noisy.Consider:
- For
folder, use(task.get("folder") or {}).get("str", "N/A").- For list fields, map to human‑friendly strings (e.g., join
obj["str"]or IDs) rather than dumping full JSON.This will make the single‑row table much easier for the model (and humans) to read.
cli/mcp.md (1)
15-17: MCP env setup steps match the new.mcp.envworkflow; minor macOS spelling nitThe updated instructions to copy
.mcp.env.exampleto.mcp.envand then edit credentials align with the new ignore rule and are clearer than editing in place.Tiny polish if you care about branding consistency:
- On MacOS it's under `~/Library/Application\ Support/Claude/claude_desktop_config.json`. + On macOS it's under `~/Library/Application\ Support/Claude/claude_desktop_config.json`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(2 hunks)cli/ca_mcp/tools/read_tools.py(3 hunks)cli/ca_mcp/tools/update_tools.py(8 hunks)cli/mcp.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/ca_mcp/tools/read_tools.py (1)
cli/ca_mcp/client.py (2)
make_get_request(24-42)get_paginated_results(123-137)
cli/ca_mcp/tools/update_tools.py (2)
cli/ca_mcp/client.py (4)
make_get_request(24-42)make_patch_request(66-84)make_delete_request(87-103)get_paginated_results(123-137)cli/ca_mcp/resolvers.py (3)
resolve_folder_id(6-34)resolve_applied_control_id(226-253)resolve_task_template_id(326-353)
🪛 LanguageTool
cli/mcp.md
[uncategorized] ~17-17: The operating system from Apple is written “macOS”./Library/Application\ Supp...
Context: ...path will vary depending on your OS. On MacOS it's under `
(MAC_OS)
🔇 Additional comments (5)
.gitignore (1)
6-6: Ignore.mcp.envin git to keep MCP credentials localAdding
cli/.mcp.envis appropriate for keeping secrets out of version control; moving__pycache__is behavior‑neutral.Also applies to: 23-23
cli/ca_mcp/tools/read_tools.py (1)
905-920: Description column for requirement assessments looks consistentHeader and row now include
Description, and the newdescription = req.get("description", "N/A")field lines up correctly with the table columns and existing truncation of theRequirementcolumn.cli/ca_mcp/tools/update_tools.py (3)
223-250:costparameter wiring for applied controls looks correctType hint, docstring, and payload handling for
costnow consistently treat it as a JSON object; assigning it directly intopayload["cost"]when provided matches the described nested structure.Also applies to: 279-280
725-838:update_task_templateflow and validations look soundPayload building only includes provided fields, status is validated against an explicit allowed set,
folder_idis resolved to a folder UUID,task_idis resolved viaresolve_task_template_id, and the PATCH result is surfaced with a clear success/error message. Overall implementation is consistent with other update helpers.
841-858: Confirm expected DELETE status code for task templates
delete_task_templatetreats only HTTP 204 as success. If the backend ever returns 200 with a body on DELETE, this will report a false error.Please confirm the API contract for
/task-templates/{id}/and adjust the success check if necessary (e.g., accept 200 or 204).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/ca_mcp/tools/update_tools.py (1)
729-842: Verify status values and consider resolving asset IDs for consistency.The implementation looks solid overall:
- Status validation (lines 784-787) provides good defensive coding
- Applied controls are correctly resolved via
resolve_applied_control_id(lines 810-815)- Folder ID resolution follows established patterns (lines 824-826)
- Task ID resolution happens after payload validation (line 832), which is efficient
Two follow-ups to consider:
Status validation list (line 784): The allowed statuses
["pending", "in_progress", "cancelled", "completed"]should be verified against the API specification to ensure they're current and complete.Asset ID resolution (lines 808-809): Assets are passed through unchanged, but
update_risk_scenario(lines 156-161) andupdate_quantitative_risk_scenario(lines 558-564) resolve asset IDs/names. For consistency and user convenience, consider resolving asset identifiers here as well:if assets is not None: - payload["assets"] = assets + resolved_assets = [] + for asset in assets: + resolved_asset_id = resolve_asset_id(asset) + resolved_assets.append(resolved_asset_id) + payload["assets"] = resolved_assetsIf the API design intentionally requires UUIDs for task template assets while allowing names elsewhere, this is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/ca_mcp/tools/update_tools.py(8 hunks)cli/ca_mcp/tools/write_tools.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/ca_mcp/tools/write_tools.py
🧰 Additional context used
🧬 Code graph analysis (1)
cli/ca_mcp/tools/update_tools.py (2)
cli/ca_mcp/client.py (4)
make_get_request(24-42)make_patch_request(66-84)make_delete_request(87-103)get_paginated_results(123-137)cli/ca_mcp/resolvers.py (3)
resolve_folder_id(6-34)resolve_applied_control_id(226-253)resolve_task_template_id(326-353)
🔇 Additional comments (4)
cli/ca_mcp/tools/update_tools.py (4)
3-3: LGTM! Import additions are appropriate.The new imports
make_delete_requestandresolve_task_template_idare correctly used in the new task template functions.Also applies to: 11-11
223-223: LGTM! Type hint correction applied.The
costparameter is now correctly typed asdictand the documentation clearly describes the expected JSON structure. This addresses the previous review feedback.Also applies to: 242-250, 279-280
318-318: LGTM! Applied controls parameter fully documented and resolved.The
applied_controlsparameter now has comprehensive documentation and correctly resolves IDs/names before sending to the API, consistent with other update functions. This addresses the previous review feedback.Also applies to: 332-332, 357-362
845-862: LGTM! Delete function is well-implemented.The
delete_task_templatefunction correctly:
- Resolves task IDs/names via
resolve_task_template_idfor user convenience- Uses the appropriate
make_delete_requesthelper- Checks for HTTP 204 (No Content), which is the standard success response for DELETE operations
- Provides clear success/error messaging
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.