Fix A002: rename id parameter to request_id in Spec class#199
Fix A002: rename id parameter to request_id in Spec class#199
id parameter to request_id in Spec class#199Conversation
Agent-Logs-Url: https://github.com/GitHubSecurityLab/seclab-taskflow-agent/sessions/c5f089f2-6b83-41f3-97c7-a4d4417ef555 Co-authored-by: kevinbackhouse <4358136+kevinbackhouse@users.noreply.github.com>
id parameter to request_id in Spec class
kevinbackhouse
requested review from
JarLob,
Kwstubbs,
anticomputer and
sylwia-budzynska
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the internal JSON-RPC helper (jsonrpyc.Spec) to stop shadowing Python’s built-in id() by renaming the affected method parameters to request_id, and tightens linting by enabling Ruff’s A002 checks (argument shadows built-in).
Changes:
- Renamed
Specmethod parameters fromid→request_idacrosscheck_id,request,response, anderror, including internal usage and the in-file keyword call site. - Removed
A002from Ruff’s ignore list inpyproject.toml.
Show a summary per file
| File | Description |
|---|---|
| src/seclab_taskflow_agent/mcp_servers/codeql/jsonrpyc/init.py | Renames Spec API parameters to request_id and updates internal call sites/docstrings (partially). |
| pyproject.toml | Enables Ruff A002 enforcement by removing it from the ignore list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/seclab_taskflow_agent/mcp_servers/codeql/jsonrpyc/init.py:177
- The
response()docstring still refers to "respone" and to a request with*id*/":raises ... When id is invalid" even though the parameter is nowrequest_id. Please update the wording to match the renamed parameter and fix the spelling typo.
"""
Creates the string representation of a respone that was triggered by a request with *id*.
A *result* is required, even if it is *None*.
:param request_id: The id of the request that triggered this response.
:param result: The result of the request.
:return: The response string.
:raises RPCInvalidRequest: When *id* is invalid.
:raises RPCParseError: When *result* could not be encoded.
src/seclab_taskflow_agent/mcp_servers/codeql/jsonrpyc/init.py:214
- In
error(), the docstring body and:raises RPCInvalidRequest:line still refer to*id*, and "occured" is misspelled. Since the parameter was renamed torequest_id, please align the docstring wording (and fix the spelling) to avoid confusion.
Creates the string representation of an error that occured while processing a request with
*id*. *code* must lead to a registered :py:class:`RPCError`. *data* might contain
additional, detailed error information and is encoded by ``json.dumps`` when set.
:param request_id: The id of the request that triggered this error.
:param code: The error code.
:param data: Additional error data.
:return: The error string.
:raises RPCInvalidRequest: When *id* or *code* are invalid.
:raises RPCParseError: When *data* could not be encoded.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| if (id is not None or not allow_empty) and not isinstance(id, (int, str)): | ||
| raise TypeError(f"id must be an integer or string, got {id} ({type(id)})") | ||
| if (request_id is not None or not allow_empty) and not isinstance(request_id, (int, str)): | ||
| raise TypeError(f"id must be an integer or string, got {request_id} ({type(request_id)})") |
There was a problem hiding this comment.
check_id() now takes request_id, but the raised TypeError message still says "id must be...". This is inconsistent with the public API/docs and can confuse callers; consider updating the message to refer to request_id (or explicitly say "JSON-RPC id").
This issue also appears in the following locations of the same file:
- line 169
- line 205
| raise TypeError(f"id must be an integer or string, got {request_id} ({type(request_id)})") | |
| raise TypeError( | |
| f"request_id must be an integer or string, got {request_id} ({type(request_id)})" | |
| ) |
| def request( | ||
| cls, | ||
| method: str, | ||
| /, | ||
| id: str | int | None = None, | ||
| request_id: str | int | None = None, | ||
| *, | ||
| params: dict[str, Any] | None = None, | ||
| ) -> str: |
There was a problem hiding this comment.
Renaming the keyword argument from id to request_id in these Spec APIs is a breaking change for any external callers that were using id= as a keyword. If backwards-compatibility matters here, consider accepting id as a deprecated keyword alias via **kwargs (and mapping it to request_id) while keeping the public parameter named request_id to satisfy Ruff A002.
|
we should not modify this code, recommend we close and defer to #210 |
Ruff rule A002 flags function arguments that shadow Python builtins. The
Specclass injsonrpyc/__init__.pyusedidas a parameter name in four methods, shadowing the built-inid().Changes
jsonrpyc/__init__.py: Renamedid→request_idinSpec.check_id,Spec.request,Spec.response, andSpec.error, including all internal usages, the one keyword-argument call site (Spec.request(method, id=id, ...)→Spec.request(method, request_id=id, ...)), and docstrings.pyproject.toml: RemovedA002from the ruffignorelist.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
unsupported.example.com/usr/bin/python python -m pytest tests/ -x -q(dns block)If you need me to access, download, or install something from one of these locations, you can either: