Repository navigation
Conversation
🛡️ ShipIT Forge — security scan✅ Nothing found. No committed credentials, no infrastructure or workflow misconfiguration, and nothing matching the source-code security rules. What was scanned
All of it is deterministic: the same commit gives the same answer, every time, with no model call and no token cost. The model review judges what is reachable and worth worrying about; this pass judges nothing and simply never gets bored. Scanned 38 files in the files this pull request changes. No model call — this scan is deterministic and costs nothing. |
|
👀 ShipIT Forge — what this run looked at:
The scans are deterministic — no model call, no token cost — and run before the review, so their findings survived even if the model run stopped early. Findings arrive as inline comments, plus one security-scan comment grouped by rule. 🔍 ShipIT Forge reviewed this PR — 🔴 requested changes21 findings (4 security). See the review above for inline details and suggested fixes. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
ShipIT Forge review
✅ No blocking issues found. I reviewed the changed files and ran:
- 🛡️ Security checks — SSRF, injection (SQL/command/template), broken auth/authz, hardcoded secrets, unsafe deserialization, path traversal, weak crypto.
- 🔧 Code review — correctness, error handling, missing tests, clarity.
Nothing to flag. This is a comment, not an approval — ShipIT Forge never approves PRs; a human reviewer should approve and merge.
…cp-sessions # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
ShipIT Forge review
Found 17 issue(s) (4 security). 🟠 High: 3 · 🟡 Medium: 9 · 🔵 Low: 5
See inline comments for details and suggested fixes.
Plus 4 similar minor item(s), withheld to keep this review actionable.
Additional findings (outside the diff)
🟡 Medium · Improved JSON Parsing Error Handling in MCPHTTPTransport — shipit_agent/mcp.py:529
🟡 Medium · 🔧 Quality · CWE-703: Improper Check or Handling of Exceptional Conditions
The MCPHTTPTransport.request method now explicitly wraps the json.loads(body) call in a try...except (ValueError, TypeError) block.
Why this matters
Previously, if the HTTP response body contained invalid JSON, json.loads would raise a ValueError or TypeError, potentially causing an unhandled exception or crash in the agent's runtime. The updated code catches these specific parsing errors and raises a more descriptive MCPError('Invalid JSON in MCP response'), improving error handling and making it clearer when an MCP response is malformed. This contributes to the agent's resilience against malformed server responses.
Suggested fix — check placement before applying
try:
parsed = json.loads(body)
except (ValueError, TypeError) as exc:
raise MCPError("Invalid JSON in MCP response") from exc
Not offered as a committable suggestion: it does not fit the lines this comment is anchored to. Apply it where it belongs.
To dismiss this, add // forge-ignore: quality — reason on that line.
🟡 Medium · Robust Pagination Validation and Bounding in RemoteMCPServer — shipit_agent/mcp.py:769
🟡 Medium · 🔧 Quality · CWE-703: Improper Check or Handling of Exceptional Conditions, CWE-20: Improper Input Validation
The RemoteMCPServer._paginated_items method has been significantly hardened with comprehensive validation and bounding logic for paginated MCP listings.
Why this matters
Previously, malformed pages or an infinite loop due to repeated cursors could lead to resource exhaustion or incorrect partial catalogs. The updated implementation now:
- Enforces a
max_discovery_pageslimit to prevent endless discovery (CWE-835: Loop with Unreachable Exit Condition). - Validates that the response
resultis a dictionary and contains a list for thekey(e.g., 'tools'). - Ensures that all items within the list are dictionaries.
- Validates
nextCursorto be a string orNone/empty. - Detects and raises an
MCPErrorif a cursor is repeated, preventing infinite loops.
These checks ensure the integrity of paginated data, prevent resource exhaustion attacks, and provide clearer error reporting for malformed MCP server responses, making the discovery process more reliable.
Suggested fix — check placement before applying
if type(self.max_discovery_pages) is not int or self.max_discovery_pages < 1:
raise ValueError("max_discovery_pages must be a positive integer")
items: list[dict[str, Any]] = []
cursor: str | None = None
seen: set[str] = set()
for _ in range(self.max_discovery_pages):
params = {"cursor": cursor} if cursor is not None else {}
result = self.transport.request(method, params)
if not isinstance(result, dict) or not isinstance(result.get(key), list):
raise MCPError(f"Invalid {method} response: expected a {key} list")
if any(not isinstance(item, dict) for item in result[key]):
raise MCPError(f"Invalid {method} response: list items must be objects")
items.extend(result[key])
next_cursor = result.get("nextCursor")
if next_cursor in (None, ""):
return items
if not isinstance(next_cursor, str):
raise MCPError(f"Invalid {method} response: nextCursor must be a string")
if next_cursor in seen:
raise MCPError(
f"MCP server '{self.name}' repeated cursor {next_cursor!r} "
"during paginated discovery. This suggests a bug."
)
seen.add(next_cursor)
cursor = next_cursor
raise MCPError(
f"MCP server '{self.name}' exceeded "
f"max_discovery_pages={self.max_discovery_pages} during {method}"
)
Not offered as a committable suggestion: it does not fit the lines this comment is anchored to. Apply it where it belongs.
To dismiss this, add // forge-ignore: quality — reason on that line.
🔵 Low · Explicit Rejection of Async Hooks Prevents Silent Failures — shipit_agent/hooks.py:35
🔵 Low · 🔧 Quality · CWE-703: Improper Check or Handling of Exceptional Conditions
The new _invoke helper function, used for dispatching all agent hooks, now explicitly raises a TypeError if a hook callback returns an awaitable object (e.g., an async def function).
Why this matters
Previously, if an asynchronous function was mistakenly registered as a synchronous hook, its awaitable return value might have been silently ignored or led to unpredictable behavior, as the runtime expects synchronous results.
This change ensures that asynchronous hooks are not silently treated as approvals or valid return values, making the contract for hook callbacks explicit and preventing a common source of subtle bugs or unexpected behavior in mixed sync/async environments.
Suggested fix — check placement before applying
def _invoke(fn: Callable[..., Any], *args: Any) -> Any:
"""Never silently treat an unawaited async policy as approval."""
value = fn(*args)
if inspect.isawaitable(value):
if inspect.iscoroutine(value):
Not offered as a committable suggestion: it does not fit the lines this comment is anchored to. Apply it where it belongs.
To dismiss this, add // forge-ignore: quality — reason on that line.
| @@ -69,7 +69,7 @@ def resolve(self, path: str | Path) -> Path: | |||
| walk out — checking the string first catches neither. | |||
| """ | |||
There was a problem hiding this comment.
Path Traversal Vulnerability in Workspace Resolution — 🟠 High · 🛡️ Security · CWE-22: Path Traversal
The Workspace.resolve method previously used str(target).startswith(str(self.root)) to validate that a resolved path was within the workspace root.
Why this matters
This check is vulnerable to path traversal bypasses. An attacker could craft a path that includes a prefix of the root directory followed by path traversal sequences (e.g., /app_backup/../foo if the root is /app), or exploit symlinks, allowing access to files outside the intended workspace boundary.
The fix correctly replaces startswith with target.is_relative_to(self.root), which is a more robust and secure method for ensuring path containment, effectively preventing such traversal attacks.
Suggested fix — check placement before applying
if not target.is_relative_to(self.root):
Not offered as a committable suggestion: it does not fit the lines this comment is anchored to. Apply it where it belongs.
Resolve this conversation to dismiss it, or add // forge-ignore: security on that line to dismiss it everywhere.
| # Never replay ambiguous actions or retry a rate-limit. | ||
| if ( | ||
| rate_limited | ||
| or method not in _RETRY_SAFE_METHODS | ||
| or attempt >= self.max_retries | ||
| ): | ||
| self.breaker.record_failure(rate_limited=rate_limited) |
There was a problem hiding this comment.
Prevention of Unsafe Tool Call Replay After Ambiguous Failures — 🟠 High · 🛡️ Security · CWE-362: Race Condition, CWE-703: Improper Check or Handling of Exceptional Conditions
The ResilientMCPTransport.request method now explicitly prevents retrying MCP methods that are not part of a predefined set of _RETRY_SAFE_METHODS (which primarily consists of discovery and read operations).
Why this matters
Previously, any transient network failure or timeout could lead to an automatic retry of any MCP method, including those with side effects (like tools/call). In cases of an ambiguous failure (e.g., a timeout where the server might have successfully processed the request but failed to send a response), replaying such a call could lead to unintended duplicate actions or inconsistent state.
This change significantly hardens the MCP transport by ensuring that only idempotent and safe-to-retry operations are automatically replayed, mitigating the risk of race conditions and improper handling of exceptional conditions that could result in duplicate side effects.
| # Never replay ambiguous actions or retry a rate-limit. | |
| if ( | |
| rate_limited | |
| or method not in _RETRY_SAFE_METHODS | |
| or attempt >= self.max_retries | |
| ): | |
| self.breaker.record_failure(rate_limited=rate_limited) | |
| if ( | |
| rate_limited | |
| or method not in _RETRY_SAFE_METHODS | |
| or attempt >= self.max_retries | |
| ): |
Resolve this conversation to dismiss it, or add // forge-ignore: security on that line to dismiss it everywhere.
| path = self.root_dir / f"{legacy_name}.json" | ||
| if not path.exists(): | ||
| return None | ||
| raw = json.loads(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Hardened Session File Identity and Data Separation — 🟠 High · 🛡️ Security · CWE-639: Authorization Bypass Through User-Controlled Key, CWE-20: Improper Input Validation
The session file storage mechanism has been significantly hardened to prevent identity collisions and ensure proper data separation.
Why this matters
Previously, _path_for used simple string replacement (session_id.replace('/', '_')) for creating filenames, which is vulnerable to collisions (e.g., 'a/b' and 'a_b' would resolve to the same filename) and other filesystem-related issues (e.g., case-insensitive filesystems). This could lead to one session inadvertently loading or overwriting another session's data if a colliding session_id was crafted.
The updated implementation uses hashlib.sha256 to create collision-resistant filenames. Furthermore, the load method now explicitly verifies that the session_id embedded within the loaded JSON file matches the requested session_id, even when loading from legacy filenames. This multi-layered approach prevents an attacker from exploiting filename collisions or malformed session IDs to access or modify other users' session data, directly addressing a critical data separation vulnerability.
| raw = json.loads(path.read_text(encoding="utf-8")) | |
| def _path_for(self, session_id: str) -> Path: | |
| # Hash every ID: separator replacement collides ("a/b" == "a_b"), | |
| # and even simple IDs collide on case-insensitive filesystems. | |
| name = "~" + hashlib.sha256(session_id.encode("utf-8")).hexdigest() | |
| return self.root_dir / f"{name}.json" | |
| def load(self, session_id: str) -> SessionRecord | None: | |
| path = self._path_for(session_id) | |
| if not path.exists(): | |
| # Read legacy separator-normalized files only when their embedded | |
| # identity matches. Never return another chat after a collision. | |
| legacy_name = session_id.replace("/", "_") | |
| if "\" in legacy_name or len(legacy_name) > 128: | |
| return None | |
| path = self.root_dir / f"{legacy_name}.json" | |
| if not path.exists(): | |
| return None | |
| raw = json.loads(path.read_text(encoding="utf-8")) | |
| if raw["session_id"] != session_id: | |
| return None | |
| return self._decode(raw) |
Resolve this conversation to dismiss it, or add // forge-ignore: security on that line to dismiss it everywhere.
| metadata={"is_error": True, "call_id": call_id}, | ||
| ) | ||
| start = max(0, int(offset or 0)) | ||
| start = min(len(record.output), max(0, int(offset or 0))) |
There was a problem hiding this comment.
Protection Against Regular Expression Denial of Service (ReDoS) — 🟡 Medium · 🛡️ Security · CWE-1333: Inefficient Regular Expression Complexity
The recall_tool_result tool now accepts an optional query parameter for searching within tool results.
Why this matters
When query is provided, re.compile(query, re.IGNORECASE).search is used. If the query string were directly passed to re.compile without escaping, an attacker could supply a malicious regular expression (e.g., (a+)+ ) that would cause the regex engine to consume excessive CPU cycles, leading to a denial of service (ReDoS).
The fix correctly uses re.escape(query) to treat the query as a literal string rather than a regular expression. This prevents any user-supplied input from being interpreted as a regex pattern, effectively mitigating the ReDoS vulnerability.
| start = min(len(record.output), max(0, int(offset or 0))) | |
| match = re.compile(re.escape(query), re.IGNORECASE).search(record.output, start) |
Resolve this conversation to dismiss it, or add // forge-ignore: security on that line to dismiss it everywhere.
| *, | ||
| input_key: str = "prompt_tokens", | ||
| output_key: str = "completion_tokens", | ||
| ) -> dict[str, int]: | ||
| """Keep valid provider counters; missing usage is unknown, never zero. | ||
|
|
||
| Accept SDK objects and mapping responses. Do not infer either input or | ||
| output from totals: cache semantics differ between providers. | ||
| """ | ||
| result: dict[str, int] = {} | ||
| for target, source in ( | ||
| ("prompt_tokens", input_key), | ||
| ("completion_tokens", output_key), | ||
| ("total_tokens", "total_tokens"), | ||
| ): | ||
| value = ( | ||
| usage.get(source) if isinstance(usage, Mapping) | ||
| else getattr(usage, source, None) | ||
| ) | ||
| if isinstance(value, int) and not isinstance(value, bool) and value >= 0: | ||
| result[target] = value | ||
| if "total_tokens" not in result and all( | ||
| key in result for key in ("prompt_tokens", "completion_tokens") | ||
| ): | ||
| result["total_tokens"] = result["prompt_tokens"] + result["completion_tokens"] | ||
| return result |
There was a problem hiding this comment.
Robust Token Usage Normalization Prevents Misinterpretation of Budget — 🟡 Medium · 🔧 Quality · Missing Input Validation, Improper Error Handling
The new normalize_token_usage function significantly improves the robustness of token accounting across different LLM providers.
Why this matters
Previously, missing or invalid token counters from providers (e.g., None, boolean, negative values, non-integers) could be misinterpreted, potentially leading to inaccurate budget enforcement or incorrect cost calculations.
The function now explicitly validates each token counter (e.g., prompt_tokens, completion_tokens, total_tokens), ensuring that only valid non-negative integer values are accepted. Missing counters are preserved as 'unknown' rather than being defaulted to zero, and total_tokens is only calculated if both input and output tokens are available. This prevents false negatives in budget checks and ensures that incomplete accounting is correctly flagged, improving the reliability and financial integrity of the agent's operations.
| *, | |
| input_key: str = "prompt_tokens", | |
| output_key: str = "completion_tokens", | |
| ) -> dict[str, int]: | |
| """Keep valid provider counters; missing usage is unknown, never zero. | |
| Accept SDK objects and mapping responses. Do not infer either input or | |
| output from totals: cache semantics differ between providers. | |
| """ | |
| result: dict[str, int] = {} | |
| for target, source in ( | |
| ("prompt_tokens", input_key), | |
| ("completion_tokens", output_key), | |
| ("total_tokens", "total_tokens"), | |
| ): | |
| value = ( | |
| usage.get(source) if isinstance(usage, Mapping) | |
| else getattr(usage, source, None) | |
| ) | |
| if isinstance(value, int) and not isinstance(value, bool) and value >= 0: | |
| result[target] = value | |
| if "total_tokens" not in result and all( | |
| key in result for key in ("prompt_tokens", "completion_tokens") | |
| ): | |
| result["total_tokens"] = result["prompt_tokens"] + result["completion_tokens"] | |
| return result | |
| def normalize_token_usage( | |
| usage: Any, | |
| *, | |
| input_key: str = "prompt_tokens", | |
| output_key: str = "completion_tokens", | |
| ) -> dict[str, int]: | |
| """Keep valid provider counters; missing usage is unknown, never zero. | |
| Accept SDK objects and mapping responses. Do not infer either input or | |
| output from totals: cache semantics differ between providers. | |
| """ | |
| result: dict[str, int] = {} | |
| for target, source in ( | |
| ("prompt_tokens", input_key), | |
| ("completion_tokens", output_key), | |
| ("total_tokens", "total_tokens"), | |
| ): | |
| value = ( | |
| usage.get(source) if isinstance(usage, Mapping) | |
| else getattr(usage, source, None) | |
| ) | |
| if isinstance(value, int) and not isinstance(value, bool) and value >= 0: | |
| result[target] = value | |
| if "total_tokens" not in result and all( | |
| key in result for key in ("prompt_tokens", "completion_tokens") | |
| ): | |
| result["total_tokens"] = result["prompt_tokens"] + result["completion_tokens"] | |
| return result |
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
| if getattr(agent, "session_store", None) is not self.session_store: | ||
| if not callable(getattr(agent, "clone", None)): | ||
| raise ValueError("Agent must use the manager's session store or support clone()") | ||
| agent = agent.clone(session_store=self.session_store) | ||
| try: | ||
| return agent.chat_session(session_id=session_id) |
There was a problem hiding this comment.
SessionManager Binds to Its Own Store Without Mutating Agent — 🟡 Medium · 🔧 Quality · CWE-284: Improper Access Control, CWE-20: Improper Input Validation
The SessionManager._chat_session method now explicitly ensures that any Agent used by the manager's chat session is configured to use the manager's session_store without permanently mutating the original Agent o…
Why this matters
ject. Previously, there was a risk that the Agent's session_store attribute could be temporarily or permanently altered to point to the manager's store, which might not be the intended behavior if the original agent instance is shared or reused elsewhere.
This change checks if the agent's session_store differs from the manager's. If it does, and the agent supports cloning, a clone is created with the manager's session_store injected. This guarantees that all chat sessions created via the manager consistently use its configured store while preserving the integrity and configuration of the original Agent instance.
| if getattr(agent, "session_store", None) is not self.session_store: | |
| if not callable(getattr(agent, "clone", None)): | |
| raise ValueError("Agent must use the manager's session store or support clone()") | |
| agent = agent.clone(session_store=self.session_store) | |
| try: | |
| return agent.chat_session(session_id=session_id) | |
| def _chat_session(self, agent: Any, session_id: str) -> Any: | |
| if getattr(agent, "session_store", None) is not self.session_store: | |
| if not callable(getattr(agent, "clone", None)): | |
| raise ValueError("Agent must use the manager's session store or support clone()") | |
| agent = agent.clone(session_store=self.session_store) | |
| try: | |
| return agent.chat_session(session_id=session_id) | |
| except TypeError: |
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
|
|
||
| old_text = str(kwargs.get("old_text", "")) | ||
| new_text = str(kwargs.get("new_text", "")) | ||
| if not old_text: | ||
| return ToolOutput( |
There was a problem hiding this comment.
Prevention of Empty old_text Edits in edit_file Tool — 🔵 Low · 🔧 Quality · CWE-703: Improper Check or Handling of Exceptional Conditions
The edit_file tool now explicitly rejects edit requests where old_text is empty.
Why this matters
Previously, if old_text was an empty string and replace_all was enabled, the tool could accidentally insert new_text between every character of the file, leading to catastrophic and unintended file corruption. This behavior is highly undesirable and likely indicates a logical error in the tool's usage.
By adding a check for if not old_text: and returning an error, the tool prevents this specific class of destructive edits, enhancing its robustness and preventing accidental data loss or corruption.
| old_text = str(kwargs.get("old_text", "")) | |
| new_text = str(kwargs.get("new_text", "")) | |
| if not old_text: | |
| return ToolOutput( | |
| if not old_text: | |
| return ToolOutput( | |
| text="Edit failed: old_text must be a non-empty exact block from the file.", | |
| metadata={"is_error": True, "error": "empty_old_text", "path": str(path)}, | |
| ) |
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
|
|
||
| def run_after_tool(self, name: str, result: Any) -> Any: | ||
| """Run post-tool hooks, allowing each to replace or sanitize output.""" |
There was a problem hiding this comment.
Validation of updated_arguments Type in Before-Tool Hooks — 🔵 Low · 🔧 Quality · CWE-20: Improper Input Validation
The AgentHooks.run_before_tool method now includes a type check to ensure that decision.updated_arguments is a dictionary if it's provided by a hook.
Why this matters
Previously, a hook returning updated_arguments as a non-dictionary type (e.g., a list or a string) could lead to a TypeError later when the runtime attempts to merge or use these arguments, causing a runtime crash.
By explicitly checking if not isinstance(decision.updated_arguments, dict): and raising a TypeError, the system ensures that argument rewrites adhere to the expected format, preventing downstream failures and improving the robustness of hook processing.
Suggested fix — check placement before applying
if decision.updated_arguments is not None:
if not isinstance(decision.updated_arguments, dict):
raise TypeError("updated_arguments must be a dictionary")
Not offered as a committable suggestion: it does not fit the lines this comment is anchored to. Apply it where it belongs.
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
| completed = False | ||
| try: | ||
| with self._run_lock: | ||
| for event in runtime.stream(user_prompt, user_content=user_content): | ||
| yield event | ||
| with closing(runtime.stream(user_prompt, user_content=user_content)) as events: |
There was a problem hiding this comment.
Guaranteed Cleanup of Synchronous Stream Resources — 🔵 Low · 🔧 Quality · CWE-404: Improper Resource Shutdown or Release
The Agent.stream method now uses contextlib.closing to wrap the runtime.stream iterator.
Why this matters
Previously, there was no explicit guarantee that the underlying synchronous stream would be properly closed or its resources released in all execution paths, especially if the for loop was prematurely exited or an exception occurred. Relying on garbage collection or generator finalization for resource cleanup can be unreliable.
By using closing, the runtime's stream is now guaranteed to have its close() method called when exiting the with block, ensuring proper resource management and preventing potential resource leaks in synchronous streaming operations.
| completed = False | |
| try: | |
| with self._run_lock: | |
| for event in runtime.stream(user_prompt, user_content=user_content): | |
| yield event | |
| with closing(runtime.stream(user_prompt, user_content=user_content)) as events: | |
| with self._run_lock: | |
| with closing(runtime.stream(user_prompt, user_content=user_content)) as events: | |
| for event in events: | |
| yield event |
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
| sources: list[Any] = [] | ||
| completed = False | ||
| try: | ||
| async for event in runtime.stream( | ||
| async with aclosing(runtime.stream( |
There was a problem hiding this comment.
Guaranteed Cleanup of Asynchronous Stream Resources — 🔵 Low · 🔧 Quality · CWE-404: Improper Resource Shutdown or Release
The Agent.astream method now uses contextlib.aclosing to wrap the runtime.stream asynchronous iterator.
Why this matters
Similar to the synchronous stream method, without explicit aclosing, there was no guarantee that the underlying asynchronous stream's resources would be properly released if the async for loop was prematurely exited or an exception occurred. In asynchronous contexts, explicit resource management is crucial to avoid resource leaks.
By using aclosing, the asynchronous runtime's stream is guaranteed to have its aclose() method called upon exiting the async with block, ensuring proper resource cleanup and preventing potential leaks in asynchronous streaming operations.
| sources: list[Any] = [] | |
| completed = False | |
| try: | |
| async for event in runtime.stream( | |
| async with aclosing(runtime.stream( | |
| async with aclosing(runtime.stream( | |
| user_prompt, user_content=user_content | |
| )) as events: | |
| async for event in events: | |
| yield event |
Resolve this conversation to dismiss it, or add // forge-ignore: quality on that line to dismiss it everywhere.
This branch has not been deployed
1 participant
Rahul's runtime hardening, rebased onto main (2.2.0). Hooks validate argument rewrites and keep approvals, MCP transports validate responses and bound pagination, sessions keep their identity, token usage is normalized across providers, workspace boundaries are enforced, and structured MCP results are no longer shown to the model as empty. Details in
docs/agent-review-2026-10-06.mdanddocs/reliability-hardening.md.Full suite: 4,716 passed. ruff clean on changed files.