Repository navigation
feat(executor): add local device command rpc - #1155
Merged
qdaxb merged 1 commit intoMay 22, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements a complete local device command execution system spanning backend HTTP API, Socket.IO RPC dispatch, and executor-side subprocess handling, including command registry, optional post-processing, tests, and bilingual documentation. ChangesLocal Device Command RPC
Sequence DiagramsequenceDiagram
participant Client
participant BackendAPI
participant BackendService
participant DeviceRedis
participant ExecutorSocket
participant ExecutorHandler
Client->>BackendAPI: POST /api/devices/{device_id}/commands (command_key, path, args, env, timeout)
BackendAPI->>BackendService: execute_configured_device_command(device_id, command_key, path, args, env, timeout)
BackendService->>DeviceRedis: load device by id (verify ownership, get socket_id)
DeviceRedis-->>BackendService: online executor socket info
BackendService->>BackendService: resolve command from LOCAL_DEVICE_COMMANDS or built-ins
BackendService->>BackendService: build argv from command + args
BackendService->>ExecutorSocket: emit device:execute_command (command, cwd, args, argv, env, timeout, output_limit)
ExecutorSocket->>ExecutorHandler: handle_execute_command(RPC payload)
ExecutorHandler->>ExecutorHandler: normalize inputs, spawn subprocess (exec argv or shell)
ExecutorHandler->>ExecutorHandler: await with timeout, terminate on timeout if needed
ExecutorHandler->>ExecutorHandler: decode/truncate stdout/stderr
ExecutorHandler-->>ExecutorSocket: return result dict (success, exit_code, stdout, stderr, duration, timed_out, truncation flags)
ExecutorSocket-->>BackendService: ack with result
BackendService->>BackendService: apply configured post_processor (e.g., file_list)
BackendService-->>BackendAPI: return processed result
BackendAPI-->>Client: DeviceCommandResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec75ad48c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+66
to
+68
| stdout, stderr = await asyncio.wait_for( | ||
| process.communicate(), | ||
| timeout=timeout_seconds, |
There was a problem hiding this comment.
process.communicate() buffers the entire stdout/stderr in memory before your max_output_bytes truncation runs, so commands that emit large output (for example recursive listings or accidental verbose logs) can exhaust executor memory and kill the local runner. This makes the documented output cap ineffective as a safety limit; read streams incrementally and stop after the configured byte cap (or redirect excess) so memory usage stays bounded during execution.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
executor/modes/local/command_handler.py (1)
27-96: 💤 Low valueConsider adding
@trace_asyncdecorator for observability.Per coding guidelines, async functions should use the
@trace_asyncdecorator fromshared/telemetry/decorators.pyfor tracing. This would help with observability of command execution in production.♻️ Suggested change
+from shared.telemetry.decorators import trace_async + class CommandHandler: """Execute backend-requested commands on the local machine.""" + `@trace_async` async def handle_execute_command(self, data: dict[str, Any]) -> dict[str, Any]:As per coding guidelines: "Use
@trace_asyncdecorator for tracing entire async functions (fromshared/telemetry/decorators.py)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/modes/local/command_handler.py` around lines 27 - 96, The async function handle_execute_command should be instrumented with the trace_async decorator for observability: add an import for trace_async from shared.telemetry.decorators and apply `@trace_async` directly above the async def handle_execute_command(self, data: dict[str, Any]) signature; leave the function body unchanged so tracing wraps the whole operation including _create_process, communicate, and error handling.executor/tests/test_local_command_handler.py (1)
10-52: ⚡ Quick winAdd explicit return type hints to async tests.
Please annotate both async tests with
-> Noneto align with repository typing requirements.As per coding guidelines
**/*.py: Python code must follow PEP 8 standards with type hints required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/test_local_command_handler.py` around lines 10 - 52, Add explicit return type hints "-> None" to both async test functions test_execute_command_returns_completed_process_result and test_execute_command_times_out_and_returns_error; update their definitions to declare async def ...(...) -> None so the tests comply with the repository typing rules while keeping the bodies (including use of CommandHandler.handle_execute_command) unchanged.backend/tests/services/test_local_device_command_service.py (3)
164-168: ⚡ Quick winUse mocks instead of
lambdafor device lookup to assert ownership-check inputs.The current lambdas make it impossible to verify that
user_id/device_idare forwarded correctly toget_device_by_device_id. Using a mock here will harden the endpoint contract tests.Suggested test pattern
-from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, Mock - monkeypatch.setattr( - devices.device_service, - "get_device_by_device_id", - lambda db, user_id, device_id: object(), - ) + db = object() + get_device_mock = Mock(return_value=object()) + monkeypatch.setattr( + devices.device_service, + "get_device_by_device_id", + get_device_mock, + ) ... - db=object(), + db=db, current_user=SimpleNamespace(id=7), ) + get_device_mock.assert_called_once_with(db, 7, "device-abc")Also applies to: 222-226, 262-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 164 - 168, Replace the inline lambda used in monkeypatch.setattr for devices.device_service.get_device_by_device_id with a proper mock (e.g., unittest.mock.Mock) that has a configured return_value and allows assertion of call args; update the three test sites that patch get_device_by_device_id (the occurrences referenced near lines 164, 222, 262) to create a mock_device_lookup = Mock(return_value=object()) and monkeypatch.setattr(devices.device_service, "get_device_by_device_id", mock_device_lookup) then assert mock_device_lookup.assert_called_once_with(db, expected_user_id, expected_device_id) (or the appropriate call signature) so the tests verify the forwarded user_id/device_id while preserving the original return object.
13-279: ⚡ Quick winAdd explicit return type hints to test functions.
Please add
-> Noneto all test function signatures in this module for guideline compliance and better static analysis consistency.As per coding guidelines
**/*.py: Python code must follow PEP 8 standards with type hints required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 13 - 279, Add explicit return type hints (-> None) to each test function definition in this module: test_local_device_command_registry_default_includes_diagnostic_commands, test_local_device_command_registry_supports_inline_post_processor, test_local_device_command_registry_keeps_default_processor_for_string_override, test_file_list_post_processor_filters_special_entries, test_execute_command_calls_registered_device_socket, test_execute_command_rejects_offline_device, test_execute_device_command_endpoint_maps_request_to_service, test_execute_device_command_endpoint_applies_configured_post_processor, and test_execute_device_command_endpoint_rejects_unknown_command_key; update each def line to include "-> None" after the parameter list (e.g., def test_name(...) -> None:) to satisfy the type-hinting guideline and static analysis.
243-252: ⚡ Quick winAssert the service call payload in the post-processor endpoint test.
This test validates transformed output, but it should also assert the resolved command dispatch (
ls -a) so command resolution regressions don’t pass silently.Suggested assertion add-on
assert response.success is True assert response.stdout == [".env", "backend"] + service_mock.assert_awaited_once() + kwargs = service_mock.await_args.kwargs + assert kwargs["user_id"] == 7 + assert kwargs["device_id"] == "device-abc" + assert kwargs["command"] == "ls -a"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 243 - 252, The test calls devices.execute_device_command with DeviceCommandRequest(command_key="repo_files") but only asserts the transformed stdout; add an assertion that the resolved/dispatched command payload equals the expected resolution (e.g., "ls -a" or ["ls","-a"]) so command resolution regressions fail loudly—locate the mocked sender/handler used by the post-processor in this test and assert its last call or captured payload contains the resolved command after devices.execute_device_command returns.backend/app/schemas/device.py (1)
151-161: ⚡ Quick winExtract command timeout/output limits into constants shared with service.
These literals duplicate dispatch-layer limits and can drift over time.
♻️ Suggested refactor
+DEFAULT_COMMAND_TIMEOUT_SECONDS = 60 +MAX_COMMAND_TIMEOUT_SECONDS = 600 +DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024 +MAX_OUTPUT_BYTES = 5 * 1024 * 1024 + class DeviceCommandRequest(BaseModel): @@ timeout_seconds: int = Field( - default=60, + default=DEFAULT_COMMAND_TIMEOUT_SECONDS, gt=0, - le=600, + le=MAX_COMMAND_TIMEOUT_SECONDS, description="Command timeout in seconds", ) max_output_bytes: int = Field( - default=1024 * 1024, + default=DEFAULT_MAX_OUTPUT_BYTES, gt=0, - le=5 * 1024 * 1024, + le=MAX_OUTPUT_BYTES, description="Maximum stdout and stderr bytes returned separately", )As per coding guidelines:
**/*.py: Extract magic numbers to constants in Python code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/schemas/device.py` around lines 151 - 161, The timeout and output-size literals in the device schema (timeout_seconds and max_output_bytes Field defaults/constraints) should be extracted to module-level constants so they can be shared with the service and avoid drift; create descriptive constants (e.g., DEFAULT_TIMEOUT_SECONDS, MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES, MIN_MAX_OUTPUT_BYTES, MAX_MAX_OUTPUT_BYTES) in this module (or a shared constants module), replace the numeric literals used in Field(default=...), gt=..., le=... for timeout_seconds and max_output_bytes with those constants, and update any service code to import the same constants so limits remain centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/endpoints/devices.py`:
- Around line 412-415: Replace returning raw exception text in the HTTPException
detail with a stable, non-sensitive user-facing message and log the full
exception server-side; specifically, in the raise HTTPException blocks shown
(the occurrences that currently use detail=str(exc) around the raise
HTTPException calls and the other instances at the ranges referenced), call the
application logger (or processLogger) to log the full exception/traceback and
change the HTTPException.detail to a generic message like "Internal server
error" or a brief domain-specific message (e.g., "Failed to retrieve device" /
"Failed to update device") so clients don't receive internal details while
preserving server-side diagnostics.
- Around line 386-391: The new async endpoint execute_device_command is missing
the tracing decorator; add the `@trace_async` decorator (from
shared.telemetry.decorators) directly above the async def
execute_device_command(...) declaration so the entire async function is
instrumented for tracing, ensuring import of trace_async is present if not
already.
- Around line 399-405: The code fetches the device with
device_service.get_device_by_device_id into device_kind but never enforces that
the device is a local executor; add an explicit guard after that call to verify
the device's kind is the local type (e.g., compare device_kind.kind or
device_kind.type against the local constant/enum such as DeviceKind.LOCAL or the
boolean flag device_kind.is_local) and raise HTTPException(status_code=404,
detail="Device not found or access denied") if it is not local, ensuring this
check occurs before any command dispatch logic in this endpoint.
In `@backend/app/services/device/command_service.py`:
- Around line 67-75: The RPC dispatch log in LocalDeviceCommandService currently
logs raw command text via logger.info("... command=%s", command[:200]) which may
expose sensitive data; remove the command content from that log and instead log
non-sensitive identifiers (e.g., user_id, device_id, socket_id,
normalized_timeout) and a safe surrogate such as the command name, a command
type field, or a deterministic hash/ID of the command before dispatch; update
the logger.info call in LocalDeviceCommandService (the "Sending command RPC"
log) to omit command[:200] and include only the safe surrogate.
- Around line 29-39: The async service entrypoint execute_command is missing the
required tracing decorator; import and apply the `@trace_async` decorator from
shared.telemetry.decorators to the execute_command method so the entire async
function is traced end-to-end (add the import for trace_async and place the
decorator immediately above the execute_command definition in the
command_service.py where the method is declared).
In `@docs/en/developer-guide/local-device-command-rpc.md`:
- Line 66: Update the phrasing in the documentation by replacing the
unhyphenated phrase "post processing" with the hyphenated compound adjective
"post-processing" in the sentence that begins "Simple commands can use string
values; commands that need post processing can use object values:" so the text
reads "commands that need post-processing can use object values:" to correct the
technical English usage.
---
Nitpick comments:
In `@backend/app/schemas/device.py`:
- Around line 151-161: The timeout and output-size literals in the device schema
(timeout_seconds and max_output_bytes Field defaults/constraints) should be
extracted to module-level constants so they can be shared with the service and
avoid drift; create descriptive constants (e.g., DEFAULT_TIMEOUT_SECONDS,
MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES,
MIN_MAX_OUTPUT_BYTES, MAX_MAX_OUTPUT_BYTES) in this module (or a shared
constants module), replace the numeric literals used in Field(default=...),
gt=..., le=... for timeout_seconds and max_output_bytes with those constants,
and update any service code to import the same constants so limits remain
centralized.
In `@backend/tests/services/test_local_device_command_service.py`:
- Around line 164-168: Replace the inline lambda used in monkeypatch.setattr for
devices.device_service.get_device_by_device_id with a proper mock (e.g.,
unittest.mock.Mock) that has a configured return_value and allows assertion of
call args; update the three test sites that patch get_device_by_device_id (the
occurrences referenced near lines 164, 222, 262) to create a mock_device_lookup
= Mock(return_value=object()) and monkeypatch.setattr(devices.device_service,
"get_device_by_device_id", mock_device_lookup) then assert
mock_device_lookup.assert_called_once_with(db, expected_user_id,
expected_device_id) (or the appropriate call signature) so the tests verify the
forwarded user_id/device_id while preserving the original return object.
- Around line 13-279: Add explicit return type hints (-> None) to each test
function definition in this module:
test_local_device_command_registry_default_includes_diagnostic_commands,
test_local_device_command_registry_supports_inline_post_processor,
test_local_device_command_registry_keeps_default_processor_for_string_override,
test_file_list_post_processor_filters_special_entries,
test_execute_command_calls_registered_device_socket,
test_execute_command_rejects_offline_device,
test_execute_device_command_endpoint_maps_request_to_service,
test_execute_device_command_endpoint_applies_configured_post_processor, and
test_execute_device_command_endpoint_rejects_unknown_command_key; update each
def line to include "-> None" after the parameter list (e.g., def test_name(...)
-> None:) to satisfy the type-hinting guideline and static analysis.
- Around line 243-252: The test calls devices.execute_device_command with
DeviceCommandRequest(command_key="repo_files") but only asserts the transformed
stdout; add an assertion that the resolved/dispatched command payload equals the
expected resolution (e.g., "ls -a" or ["ls","-a"]) so command resolution
regressions fail loudly—locate the mocked sender/handler used by the
post-processor in this test and assert its last call or captured payload
contains the resolved command after devices.execute_device_command returns.
In `@executor/modes/local/command_handler.py`:
- Around line 27-96: The async function handle_execute_command should be
instrumented with the trace_async decorator for observability: add an import for
trace_async from shared.telemetry.decorators and apply `@trace_async` directly
above the async def handle_execute_command(self, data: dict[str, Any])
signature; leave the function body unchanged so tracing wraps the whole
operation including _create_process, communicate, and error handling.
In `@executor/tests/test_local_command_handler.py`:
- Around line 10-52: Add explicit return type hints "-> None" to both async test
functions test_execute_command_returns_completed_process_result and
test_execute_command_times_out_and_returns_error; update their definitions to
declare async def ...(...) -> None so the tests comply with the repository
typing rules while keeping the bodies (including use of
CommandHandler.handle_execute_command) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad72d708-b9a5-496e-beee-03be7698fa19
📒 Files selected for processing (14)
AGENTS.mdbackend/app/api/endpoints/devices.pybackend/app/core/config.pybackend/app/schemas/device.pybackend/app/services/device/command_post_processor.pybackend/app/services/device/command_registry.pybackend/app/services/device/command_service.pybackend/tests/services/test_local_device_command_service.pydocs/en/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/local-device-command-rpc.mdexecutor/modes/local/command_handler.pyexecutor/modes/local/events.pyexecutor/modes/local/runner.pyexecutor/tests/test_local_command_handler.py
Comment on lines
+386
to
+391
| async def execute_device_command( | ||
| device_id: str, | ||
| request: DeviceCommandRequest, | ||
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(security.get_current_user), | ||
| ) -> DeviceCommandResponse: |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tracing decorator to the new async endpoint.
Please instrument this endpoint with @trace_async for consistent request tracing.
🧭 Suggested fix
from app.services.device_service import device_service
+from shared.telemetry.decorators import trace_async
@@
`@router.post`("/{device_id}/commands", response_model=DeviceCommandResponse)
+@trace_async
async def execute_device_command(As per coding guidelines: **/*.py: Use @trace_async decorator for tracing entire async functions (from shared/telemetry/decorators.py).
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 389-389: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 390-390: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/devices.py` around lines 386 - 391, The new async
endpoint execute_device_command is missing the tracing decorator; add the
`@trace_async` decorator (from shared.telemetry.decorators) directly above the
async def execute_device_command(...) declaration so the entire async function
is instrumented for tracing, ensuring import of trace_async is present if not
already.
Comment on lines
+399
to
+405
| device_kind = device_service.get_device_by_device_id(db, user_id, device_id) | ||
| if not device_kind: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Device not found or access denied", | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Enforce local-device-only validation before command dispatch.
This endpoint is scoped to local executor RPC, but there is no explicit device type guard before execution.
✅ Suggested fix
from app.schemas.device import (
+ DeviceType,
DeviceCommandRequest,
DeviceCommandResponse,
@@
device_kind = device_service.get_device_by_device_id(db, user_id, device_id)
if not device_kind:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND,
detail="Device not found or access denied",
)
+ device_type = device_kind.json.get("spec", {}).get(
+ "deviceType",
+ DeviceType.LOCAL.value,
+ )
+ if device_type != DeviceType.LOCAL.value:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="Device command RPC only supports local devices",
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/devices.py` around lines 399 - 405, The code
fetches the device with device_service.get_device_by_device_id into device_kind
but never enforces that the device is a local executor; add an explicit guard
after that call to verify the device's kind is the local type (e.g., compare
device_kind.kind or device_kind.type against the local constant/enum such as
DeviceKind.LOCAL or the boolean flag device_kind.is_local) and raise
HTTPException(status_code=404, detail="Device not found or access denied") if it
is not local, ensuring this check occurs before any command dispatch logic in
this endpoint.
Comment on lines
+412
to
+415
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=str(exc), | ||
| ) |
Contributor
There was a problem hiding this comment.
Do not return raw internal exception messages to clients.
detail=str(exc) can leak backend internals/config details. Log full exception server-side and return stable user-facing errors.
Also applies to: 439-442, 459-462
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 412-415: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/devices.py` around lines 412 - 415, Replace
returning raw exception text in the HTTPException detail with a stable,
non-sensitive user-facing message and log the full exception server-side;
specifically, in the raise HTTPException blocks shown (the occurrences that
currently use detail=str(exc) around the raise HTTPException calls and the other
instances at the ranges referenced), call the application logger (or
processLogger) to log the full exception/traceback and change the
HTTPException.detail to a generic message like "Internal server error" or a
brief domain-specific message (e.g., "Failed to retrieve device" / "Failed to
update device") so clients don't receive internal details while preserving
server-side diagnostics.
Comment on lines
+29
to
+39
| async def execute_command( | ||
| self, | ||
| user_id: int, | ||
| device_id: str, | ||
| command: str, | ||
| cwd: Optional[str] = None, | ||
| env: Optional[dict[str, Any]] = None, | ||
| timeout_seconds: int = DEFAULT_COMMAND_TIMEOUT_SECONDS, | ||
| max_output_bytes: int = DEFAULT_MAX_OUTPUT_BYTES, | ||
| ) -> dict[str, Any]: | ||
| """Execute a command on an online local device and wait for the result.""" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tracing decorator to the async service entrypoint.
execute_command should be wrapped for end-to-end tracing consistency.
🧭 Suggested fix
from app.core.socketio import get_sio
from app.services.device_service import device_service
+from shared.telemetry.decorators import trace_async
@@
class LocalDeviceCommandService:
"""Send command RPC requests to connected local executor devices."""
+ `@trace_async`
async def execute_command(As per coding guidelines: **/*.py: Use @trace_async decorator for tracing entire async functions (from shared/telemetry/decorators.py).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device/command_service.py` around lines 29 - 39, The
async service entrypoint execute_command is missing the required tracing
decorator; import and apply the `@trace_async` decorator from
shared.telemetry.decorators to the execute_command method so the entire async
function is traced end-to-end (add the import for trace_async and place the
decorator immediately above the execute_command definition in the
command_service.py where the method is declared).
Comment on lines
+67
to
+75
| logger.info( | ||
| "[LocalDeviceCommandService] Sending command RPC: " | ||
| "user_id=%s, device_id=%s, socket_id=%s, timeout=%s, command=%s", | ||
| user_id, | ||
| device_id, | ||
| socket_id, | ||
| normalized_timeout, | ||
| command[:200], | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid logging raw command text in RPC dispatch logs.
Line 74 logs command content (command[:200]), which can still expose sensitive arguments in logs.
🔒 Suggested fix
logger.info(
"[LocalDeviceCommandService] Sending command RPC: "
- "user_id=%s, device_id=%s, socket_id=%s, timeout=%s, command=%s",
+ "user_id=%s, device_id=%s, socket_id=%s, timeout=%s",
user_id,
device_id,
socket_id,
normalized_timeout,
- command[:200],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info( | |
| "[LocalDeviceCommandService] Sending command RPC: " | |
| "user_id=%s, device_id=%s, socket_id=%s, timeout=%s, command=%s", | |
| user_id, | |
| device_id, | |
| socket_id, | |
| normalized_timeout, | |
| command[:200], | |
| ) | |
| logger.info( | |
| "[LocalDeviceCommandService] Sending command RPC: " | |
| "user_id=%s, device_id=%s, socket_id=%s, timeout=%s", | |
| user_id, | |
| device_id, | |
| socket_id, | |
| normalized_timeout, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device/command_service.py` around lines 67 - 75, The RPC
dispatch log in LocalDeviceCommandService currently logs raw command text via
logger.info("... command=%s", command[:200]) which may expose sensitive data;
remove the command content from that log and instead log non-sensitive
identifiers (e.g., user_id, device_id, socket_id, normalized_timeout) and a safe
surrogate such as the command name, a command type field, or a deterministic
hash/ID of the command before dispatch; update the logger.info call in
LocalDeviceCommandService (the "Sending command RPC" log) to omit command[:200]
and include only the safe surrogate.
|
|
||
| This feature follows a trusted Backend and restricted API model. The HTTP API does not accept raw commands; it accepts only configured keys. The real command must be configured by the Backend through the command registry or `LOCAL_DEVICE_COMMANDS`. The `pwd` and `ls_a` command keys are built in by default. `ls_a` uses the `file_list` post processor to filter `.` and `..` and return a file name array in `stdout`. To add a built-in command, add one entry to `DEFAULT_LOCAL_DEVICE_COMMANDS` in `backend/app/services/device/command_registry.py`. | ||
|
|
||
| At runtime, add or override command definitions with a single environment variable. Simple commands can use string values; commands that need post processing can use object values: |
Contributor
There was a problem hiding this comment.
Use hyphenated compound adjective in docs text.
At Line 66, change “post processing” to “post-processing” for correct technical English usage.
🧰 Tools
🪛 LanguageTool
[grammar] ~66-~66: Use a hyphen to join words.
Context: ...e string values; commands that need post processing can use object values: ```ba...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/en/developer-guide/local-device-command-rpc.md` at line 66, Update the
phrasing in the documentation by replacing the unhyphenated phrase "post
processing" with the hyphenated compound adjective "post-processing" in the
sentence that begins "Simple commands can use string values; commands that need
post processing can use object values:" so the text reads "commands that need
post-processing can use object values:" to correct the technical English usage.
FicoHu
force-pushed
the
codex/feature/local-device-command-rpc
branch
from
May 22, 2026 11:25
db9607d to
8561d58
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker/knowledge_doc_converter/Dockerfile (1)
7-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the converter worker as a non-root user.
Line 7 through Line 55 currently run as root because no
USERis set. This weakens container isolation and violates common hardening baselines.🔒 Suggested hardening patch
FROM ghcr.io/wecode-ai/wegent-base-python3.12:latest WORKDIR /app @@ # Create log directory RUN mkdir -p /app/logs + +# Drop root privileges for runtime +RUN useradd --create-home --uid 10001 appuser && chown -R appuser:appuser /app +USER appuser🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/knowledge_doc_converter/Dockerfile` around lines 7 - 55, The container runs as root because no USER is set; add a non-root user and switch to it after build-time root operations: create a dedicated user (e.g., appuser) and group, perform all apt/pip/RUN install steps and create /app and /app/logs as root, chown /app and /app/logs to that user (refer to WORKDIR /app, RUN pip install and RUN mkdir -p /app/logs), then set USER appuser before the final CMD (the celery start in CMD ["sh","-c","exec celery -A knowledge_doc_converter.celery_app worker ..."]). Ensure file ownership is updated so the non-root user can write logs and run the worker.frontend/src/features/knowledge/document/components/DocumentList.tsx (1)
823-865: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
data-testidto the new selection and sort controls.The compact select-all button, the table select-all checkbox, and the new sortable header buttons are all interactive but currently unaddressable in E2E. Please add stable ids such as
select-all-button,select-all-checkbox, andsort-name-button.As per coding guidelines:
**/components/**/*.{ts,tsx}: MUST preserve and adddata-testidattributes for E2E testing - never rename or remove existingdata-testidunless updating corresponding E2E tests, and MUST adddata-testidto all interactive elements (buttons, inputs, links, selects, etc.) in new components.Also applies to: 875-942
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/DocumentList.tsx` around lines 823 - 865, The compact select-all button and other new interactive controls are missing stable data-testid attributes; add data-testid="select-all-button" to the compact select-all <button> that calls handleSelectAll (in DocumentList / the button using isAllSelected), add data-testid="select-all-checkbox" to the table select-all <input type="checkbox"> (where table selection is rendered), and add data-testid values for each sortable header (e.g. data-testid="sort-name-button" for the name sort button) so all interactive elements in DocumentList/FolderTree and the sortable header component follow the testing guideline; ensure you add these attributes on the exact interactive elements (buttons/inputs) referenced by handlers like handleSelectAll and the header sort click handlers without renaming existing data-testid values.
🟠 Major comments (22)
backend/app/core/exceptions.py-10-20 (1)
10-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type hints to function signature.
The function lacks type hints on parameters and return value, which violates the project's Python coding guidelines.
🔧 Proposed fix adding type hints
+from typing import Any + -def _make_json_serializable(value): +def _make_json_serializable(value: Any) -> Any: """Convert validation error details into JSON-serializable values.""" if isinstance(value, bytes): return value.decode("utf-8", errors="replace") if isinstance(value, dict): return {key: _make_json_serializable(item) for key, item in value.items()} if isinstance(value, list): return [_make_json_serializable(item) for item in value] if isinstance(value, tuple): return [_make_json_serializable(item) for item in value] return valueAs per coding guidelines: Python code MUST use type hints for functions and classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/core/exceptions.py` around lines 10 - 20, Add explicit type hints to the _make_json_serializable function signature: import typing.Any (and other typing aliases if you prefer) and annotate the parameter and return type (e.g., def _make_json_serializable(value: Any) -> Any) so the function complies with the project's type-hinting rules; keep the existing logic intact and ensure any used typing names are imported at the top of the module.knowledge_doc_converter/knowledge_doc_converter/services/callback_client.py-26-35 (1)
26-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract retry/timeout literals and complete function typing.
This service still hardcodes retry/timeout values and misses
__init__ -> None. Pulling these into named constants keeps behavior explicit and aligns with repo Python rules.As per coding guidelines:
**/*.py: Extract magic numbers to named constants in Python code;**/*.py: Python code MUST use type hints for functions and classes♻️ Suggested refactor
import base64 import logging import time -from typing import Optional +from typing import Any @@ logger = logging.getLogger(__name__) + +DEFAULT_MAX_RETRIES = 3 +INITIAL_RETRY_DELAY_SECONDS = 2 +CALLBACK_TIMEOUT_SECONDS = 30 @@ class CallbackClient: @@ - def __init__(self): + def __init__(self) -> None: @@ def call( - self, path: str, payload: dict, max_retries: int = 3, callback_type: str = "" - ) -> dict: + self, + path: str, + payload: dict[str, Any], + max_retries: int = DEFAULT_MAX_RETRIES, + callback_type: str = "", + ) -> dict[str, Any]: @@ - delay = 2 + delay = INITIAL_RETRY_DELAY_SECONDS @@ - resp = httpx.post(url, json=payload, headers=self.headers, timeout=30) + resp = httpx.post( + url, + json=payload, + headers=self.headers, + timeout=CALLBACK_TIMEOUT_SECONDS, + )Also applies to: 51-55, 68-69
knowledge_doc_converter/knowledge_doc_converter/services/callback_client.py-52-70 (1)
52-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard invalid retry values to avoid silent callback drops.
When
max_retriesis0or negative, Line 52 skips all attempts and Line 70 returns{}as if the call succeeded. This can silently lose conversion state updates.✅ Suggested fix
def call( self, path: str, payload: dict, max_retries: int = 3, callback_type: str = "" ) -> dict: @@ + if max_retries < 1: + raise ValueError("max_retries must be >= 1") + url = f"{self.base_url}{path}" delay = 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/services/callback_client.py` around lines 52 - 70, The retry loop can be skipped when max_retries is 0 or negative, causing silent callback drops; add an explicit guard at the start of the function/method that contains the loop (the code using max_retries and calling httpx.post and record_callback_success/record_callback_failed) to validate max_retries is a positive integer (e.g., if max_retries <= 0: raise a ValueError with a clear message or normalize it to 1 and log a warning), so the caller cannot accidentally bypass attempts and state updates; update any callers/tests if you choose to raise instead of normalizing.executor_manager/services/sandbox/manager.py-476-482 (1)
476-482:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
inactive_hoursbefore computing stale eligibility.Line 476 accepts non-positive values; with
0or negative input, this path can aggressively terminate all active sandboxes.Proposed patch
result: Dict[str, Any] = { "target": "sandboxes", "inactive_hours": inactive_hours, "dry_run": dry_run, "deleted": [], "skipped": [], "failed": [], } + if inactive_hours <= 0: + result["failed"].append( + { + "sandbox_id": None, + "reason": "invalid_inactive_hours", + "error": "inactive_hours must be greater than 0", + } + ) + return result + max_age_seconds = inactive_hours * 3600🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor_manager/services/sandbox/manager.py` around lines 476 - 482, The cleanup function that takes inactive_hours should validate that inactive_hours is a positive integer before computing max_age_seconds to avoid terminating all sandboxes when 0 or negative is passed; add a guard at the top of the function (before now/max_age_seconds are computed) that either raises a ValueError for inactive_hours <= 0 or normalizes it to a safe minimum (e.g., 1), and update any callers/tests if necessary; reference the inactive_hours parameter and the local variable max_age_seconds in your change.knowledge_doc_converter/knowledge_doc_converter/celery_app.py-45-70 (1)
45-70: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAnnotate signal handler function signatures.
Please add explicit type hints (including return type) for these handlers.
As per coding guidelines,
**/*.py: Python code MUST use type hints for functions and classes. Based on learnings, tracing decorators are intentionally not required inknowledge_doc_converter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/celery_app.py` around lines 45 - 70, Add explicit type hints to the Celery signal handlers: annotate _setup_celery_logger and _setup_celery_task_logger with logger: logging.Logger, *args: Any, **kwargs: Any and a return type of None; annotate _start_metrics_server with **kwargs: Any and -> None. Ensure you import typing.Any (and logging if not already) and update the function signatures for _setup_celery_logger, _setup_celery_task_logger, and _start_metrics_server accordingly.knowledge_engine/tests/test_conversion/test_mineru_client.py-19-137 (1)
19-137: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd return type hints (
-> None) to test functions.The new tests should follow repository-wide Python typing rules.
As per coding guidelines,
**/*.py: Python code MUST use type hints for functions and classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_engine/tests/test_conversion/test_mineru_client.py` around lines 19 - 137, All test functions in this module (e.g., test_is_supported_extension_pdf, test_is_supported_extension_with_dot, test_is_supported_extension_uppercase, test_is_supported_extension_docx, test_is_supported_extension_unsupported, test_supported_mime_types_count, test_mineru_config_defaults, test_submit_and_wait_success, test_submit_and_wait_task_failed, test_submit_and_wait_json_response_raises) must include explicit return type hints; update their definitions to use "-> None" (for async tests use "async def ... -> None") while leaving names and bodies (and references to MinerUConfig, submit_and_wait, is_supported_extension, SUPPORTED_MIME_TYPES) unchanged to satisfy the repository typing rule.knowledge_engine/tests/test_conversion/test_s3_uploader.py-15-84 (1)
15-84: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd return type hints to test functions.
These test functions should be annotated with
-> Noneto match repository Python typing rules.As per coding guidelines,
**/*.py: Python code MUST use type hints for functions and classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_engine/tests/test_conversion/test_s3_uploader.py` around lines 15 - 84, Add explicit return type hints "-> None" to each test function in this file (test_uploader_disabled_returns_none, test_uploader_enabled_property, test_uploader_has_lock, test_upload_image_returns_url, test_upload_image_encodes_chinese_path, test_upload_image_returns_none_on_error) so their signatures comply with the repository typing rules; update the def lines to include "-> None" and keep bodies unchanged.knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py-25-107 (1)
25-107: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit type hints to lock service methods.
Please annotate method params/returns (including context manager dunder methods) to match repo typing requirements.
As per coding guidelines,
**/*.py: Python code MUST use type hints for functions and classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py` around lines 25 - 107, The methods lack explicit type hints: add annotations across DistributedLock/lock service methods and the nested _WatchdogContext to satisfy repo typing rules. Specifically, annotate __init__ (self), the redis_client property to return redis.Redis, _prefixed(name: str) -> str, acquire(name: str, expire_seconds: int = 60) -> bool, release(name: str) -> None, extend(name: str, expire_seconds: int) -> None, and acquire_watchdog_context(name: str, expire_seconds: int, extend_interval_seconds: int) -> ContextManager[bool]; inside class _WatchdogContext annotate __init__(..., lock: "DistributedLock", name: str, expire_seconds: int, extend_interval: int) -> None, attributes like _stop_event: threading.Event and _thread: Optional[threading.Thread], __enter__(self) -> bool, __exit__(self, exc_type: Optional[Type[BaseException]], exc: Optional[BaseException], tb: Optional[TracebackType]) -> Optional[bool], and _watchdog(self) -> None; use typing.Optional, typing.Type, types.TracebackType and ContextManager/Generator types as appropriate and import any missing typing symbols.frontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.ts-469-479 (1)
469-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale notebook fetch can overwrite non-KB selections.
latestSelectedKbIdRefis only updated inselectKb. If the user switches to group/all/dingtalk beforegetKnowledgeBaseresolves, the stale response still passes the check and resetsselectedKbunexpectedly.Suggested fix
const selectGroup = useCallback((groupId: string) => { + latestSelectedKbIdRef.current = null setSelectedGroupId(groupId) @@ const selectAll = useCallback(() => { + latestSelectedKbIdRef.current = null setSelectedGroupId(null) @@ const selectGroups = useCallback(() => { + latestSelectedKbIdRef.current = null setSelectedGroupId(null) @@ const selectDingtalk = useCallback(() => { + latestSelectedKbIdRef.current = null setSelectedGroupId(null) @@ const clearSelection = useCallback(() => { + latestSelectedKbIdRef.current = null setSelectedKbId(null)Also applies to: 491-529
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.ts` around lines 469 - 479, The stale notebook fetch can overwrite non-KB selections because latestSelectedKbIdRef is only set in selectKb; update selectKb to also clear or set latestSelectedKbIdRef to a sentinel (e.g., null or '') whenever the user selects a non-notebook option (group/all/dingtalk), and in the getKnowledgeBase promise resolution (the block that calls setSelectedKb with fullKb) add a strict guard that latestSelectedKbIdRef.current === kb.id before calling setSelectedKb, so resolved notebook fetches are ignored if the current selection is no longer that notebook; reference setSelectedKb, getKnowledgeBase, latestSelectedKbIdRef, and selectKb to locate the changes.knowledge_doc_converter/knowledge_doc_converter/config.py-23-29 (1)
23-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve Redis username and URL-encode injected password.
Current URL reconstruction drops
parsed.usernameand inserts raw password text. This breaks ACL-style URLs (e.g.,redis://user@host/...) and can generate invalid URLs for passwords with reserved characters.Suggested fix
-from urllib.parse import urlparse, urlunparse +from urllib.parse import quote, urlparse, urlunparse @@ - # Inject password: redis://host -> redis://:password@host - netloc = f":{password}@{parsed.hostname}" + # Inject password while preserving optional username (Redis ACL) + encoded_password = quote(password, safe="") + username = parsed.username or "" + encoded_username = quote(username, safe="") if username else "" + auth = ( + f"{encoded_username}:{encoded_password}@" + if encoded_username + else f":{encoded_password}@" + ) + netloc = f"{auth}{parsed.hostname}" if parsed.port: netloc += f":{parsed.port}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/config.py` around lines 23 - 29, When reconstructing the Redis URL, preserve any existing parsed.username and URL-encode the injected password before building netloc; currently the code drops parsed.username and inserts the raw password. Change the netloc construction used before urlunparse(parsed._replace(...)) to use the existing parsed.username when present (otherwise keep the leading empty username syntax for Redis) and URL-encode password (e.g., with urllib.parse.quote) so reserved characters are escaped; still append f":{port}" when parsed.port exists and then return urlunparse(parsed._replace(netloc=netloc)).knowledge_doc_converter/knowledge_doc_converter/core/metrics.py-55-74 (1)
55-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix PROMETHEUS_MULTIPROC_DIR lifecycle/ownership handling in multiprocess metrics
prometheus_clientmultiprocess mode requiresPROMETHEUS_MULTIPROC_DIRto point to an existing directory; current_ensure_multiproc_dir()reuses an externally provided value without creating/validating it (lines 55-74).cleanup_multiproc_dir()laterrmtree()s_MULTIPROC_DIRunconditionally, which can delete an externally/operator-managed shared directory while other workers still rely on it.- Track whether the directory was created by this module (or validate/create only in that case) and only remove directories this module owns; otherwise leave external paths alone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/core/metrics.py` around lines 55 - 74, The current logic in _ensure_multiproc_dir() and cleanup_multiproc_dir() must track ownership and validate external paths: add a module-level boolean (e.g. _MULTIPROC_DIR_OWNED) and in _ensure_multiproc_dir() if PROMETHEUS_MULTIPROC_DIR exists, check os.path.isdir(existing) — if it exists set _MULTIPROC_DIR=existing and _MULTIPROC_DIR_OWNED=False; if it does not exist create the directory at that path (os.makedirs(existing, exist_ok=True)), set _MULTIPROC_DIR=existing and _MULTIPROC_DIR_OWNED=True; when creating a temp dir via tempfile.mkdtemp() set _MULTIPROC_DIR_OWNED=True and write the env var; in cleanup_multiproc_dir() only rmtree and clear os.environ["PROMETHEUS_MULTIPROC_DIR"] when _MULTIPROC_DIR_OWNED is True, and always reset _MULTIPROC_DIR/_MULTIPROC_DIR_OWNED to None/False afterwards; update references to _ensure_multiproc_dir(), cleanup_multiproc_dir(), and _MULTIPROC_DIR accordingly.knowledge_doc_converter/knowledge_doc_converter/core/logging.py-226-234 (1)
226-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReattach the
error.loghandler inapply_celery_format()as well.
setup_logging()attaches bothinfo.loganderror.log(to the root logger), butapply_celery_format()only re-attachesinfo.log. Sincealready_has_fileblocks adding anyHourlyRotatingFileHandlerwhen any one is present,error.logmay never be created after Celery reconfigures loggers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/core/logging.py` around lines 226 - 234, apply_celery_format() currently only re-attaches the info.log handler and skips adding error.log because it checks for any HourlyRotatingFileHandler on the logger; update apply_celery_format() to mirror setup_logging(): after computing log_file_enabled and already_has_file, create both an info_handler and an error_handler via _create_file_handler(log_dir, "info.log", logging.DEBUG) and _create_file_handler(log_dir, "error.log", logging.ERROR) and add them with logger.addHandler(...) when not present; ensure you use the same HourlyRotatingFileHandler check and only skip adding when both handlers already exist so error.log gets created when Celery reconfigures.backend/app/api/endpoints/admin/runtime_cleanup.py-31-43 (1)
31-43: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd tracing decorators to the newly introduced cleanup functions.
The new sync/async functions are not instrumented, which breaks the tracing convention used for backend observability.
Proposed patch
from app.services.execution import get_executor_runtime_client +from shared.telemetry.decorators import trace_async, trace_sync @@ +@trace_sync() def _format_timestamp(timestamp: float) -> str: @@ +@trace_async() async def _cleanup_stale_sandbox_for_task( @@ `@router.post`("/runtime-cleanup/stale", response_model=Dict[str, Any]) +@trace_async() async def cleanup_stale_runtimes(As per coding guidelines:
**/*.py: Use@trace_async()decorator for tracing entire async functions,@trace_sync()for sync functions.Also applies to: 96-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/admin/runtime_cleanup.py` around lines 31 - 43, The new functions are missing tracing decorators; add `@trace_sync`() above the sync helper _format_timestamp and `@trace_async`() above the async helper _cleanup_stale_sandbox_for_task to instrument them for backend observability, and likewise apply `@trace_async`()/@trace_sync() to the other newly introduced functions around lines 96-101 (use `@trace_async`() for async functions and `@trace_sync`() for sync functions) so they follow the project's tracing convention.backend/app/schemas/conversion_callback.py-16-20 (1)
16-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain callback
actionto supported values.
actionis currently free-form, so invalid callback actions can pass validation and reach state-handling logic unexpectedly.Proposed patch
-from typing import Optional +from typing import Literal, Optional @@ - action: str # "conversion_started" | "conversion_failed" + action: Literal["conversion_started", "conversion_failed"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/schemas/conversion_callback.py` around lines 16 - 20, The action field is unconstrained (action: str) so invalid values slip through; change its type to a constrained Literal or an Enum (e.g., Literal["conversion_started", "conversion_failed"] or a ConversionAction Enum) in the conversion_callback model so Pydantic validation enforces only the supported actions; update any imports and any code that constructs or compares action values to use the new type/name (action field, ConversionCallback model) so runtime handlers receive only validated actions.frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx-334-386 (1)
334-386: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
data-testidto the new sortable header buttons.These buttons are new interactive controls, so they should expose stable ids like
sort-name-button,sort-group-button,sort-created-button, andsort-updated-button.As per coding guidelines:
**/components/**/*.{ts,tsx}: MUST preserve and adddata-testidattributes for E2E testing - never rename or remove existingdata-testidunless updating corresponding E2E tests, and MUST adddata-testidto all interactive elements (buttons, inputs, links, selects, etc.) in new components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsx` around lines 334 - 386, The sortable header buttons (those invoking handleSort('name'|'group'|'created'|'updated') and rendering SortIcon) need stable data-testid attributes for E2E tests; update each <button> that calls handleSort to include data-testid="sort-name-button", "sort-group-button", "sort-created-button", and "sort-updated-button" respectively (also add the group one conditionally when showGroupColumn is true), ensuring you add the attribute to the same button elements that wrap the column label and SortIcon so tests can reliably select them.executor/modes/local/command_handler.py-52-57 (1)
52-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't log raw command text.
Even truncated shell commands can still expose tokens, paths, or sensitive arguments. Log a command key or another safe surrogate instead of
command[:200].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/modes/local/command_handler.py` around lines 52 - 57, The log currently prints raw command text in the CommandHandler (logger.info call in command_handler.py); replace logging of command[:200] with a safe surrogate (e.g., compute a deterministic hash like SHA-256 of the full command string and log the hex digest or first N chars, or generate/lookup a command key) so no sensitive tokens/paths are emitted. Update the logger.info invocation used by CommandHandler (the call that passes cwd, timeout_seconds, and command[:200]) to pass the surrogate instead, and ensure any code that needs to map back to the command can use the same hashing/key generation function for correlation.knowledge_doc_converter/tests/test_conversion_task.py-25-64 (1)
25-64: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd return annotations to the new test module.
This file omits return types on the fixture/helper and the test cases. Please annotate them consistently, e.g.
mock_settings() -> ConverterSettings,_make_mock_task() -> MagicMock, and test methods with-> None.As per coding guidelines:
**/*.py: Python code MUST use type hints for functions and classes.Also applies to: 67-405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/tests/test_conversion_task.py` around lines 25 - 64, Add explicit return type annotations across the test module: annotate the fixture function mock_settings() with -> ConverterSettings and the helper _make_mock_task() with -> MagicMock, and annotate all test functions (lines ~67-405) with -> None. Ensure necessary type names are imported or referenced (e.g., import ConverterSettings and MagicMock or from typing import TYPE_CHECKING if using forward refs) and update any function signatures (mock_settings, _make_mock_task, and all test_* functions) to include the return annotations to satisfy the project typing guideline.backend/app/services/adapters/executor_job.py-197-223 (1)
197-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter out device executors before running the single-task stale check.
_get_cleanup_subtasks_for_task()returns all executor subtasks, but_get_runtime_cleanup_skip_reason()decidesdevice_executorfromsubtasks[0]. If a task has both device and non-device executors, a device row appearing first will make this API skip cleanup for the non-device executor entirely.💡 Minimal fix
- subtasks = await self._get_cleanup_subtasks_for_task(db, task_id) + subtasks = [ + subtask + for subtask in await self._get_cleanup_subtasks_for_task(db, task_id) + if not self._is_device_executor_name(subtask.executor_name) + ] if not subtasks: return self._build_cleanup_result( task_id,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/adapters/executor_job.py` around lines 197 - 223, _subtasks returned by _get_cleanup_subtasks_for_task can contain device executors which cause _get_runtime_cleanup_skip_reason to misclassify the task (it inspects subtasks[0]); filter out device executors from the subtasks list before calling _get_runtime_cleanup_skip_reason so the skip logic only considers non-device executors, then continue to build executors via _build_executor_result_items from the original or filtered list as appropriate; specifically, apply the device-executor filter between the call to _get_cleanup_subtasks_for_task and the call to _get_runtime_cleanup_skip_reason (referencing variables subtasks, cutoff, inactive_hours, and functions _get_runtime_cleanup_skip_reason and _build_executor_result_items).executor/modes/local/command_handler.py-24-28 (1)
24-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
@trace_async()tracing to the async RPC entrypoint and redact command logging
executor/modes/local/command_handler.py:27—handle_execute_commandis not decorated with@trace_async(), so it won’t be traced end-to-end.executor/modes/local/command_handler.py:52-57—logger.info(... command=%s, command[:200])logs shell command text (even truncated), which can leak sensitive arguments; log redacted/hashed command metadata instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/modes/local/command_handler.py` around lines 24 - 28, Add the tracing decorator and remove raw command logging: decorate the async RPC entrypoint handle_execute_command with `@trace_async`() (import trace_async from the tracing/tracer module used elsewhere) so the method is traced end-to-end, and change the logger.info that currently prints the command text (logger.info(... command=%s, ...)) to log only non-sensitive metadata (e.g., redacted placeholder, command length, and a stable hash like sha256 of the command) or a flag indicating it was redacted; keep the logger call signature and context keys but replace the raw command argument with the sanitized/hashed value to avoid leaking sensitive arguments.frontend/src/features/knowledge/document/components/FolderTree.tsx-345-370 (1)
345-370:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease compact action button hit areas for mobile accessibility.
At Line 345, Line 355, and Line 365, these icon-only buttons are below the required 44×44 touch target size for mobile interactions.
As per coding guidelines "Interactive elements in mobile components MUST be at least 44px × 44px, use
h-11 min-w-[44px]for buttons".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/FolderTree.tsx` around lines 345 - 370, The three icon-only action buttons that call onCreateFolder, onRenameFolder, and onDeleteFolder currently have too small touch targets; update each button's className to include the mobile minimum hit area (for example add h-11 min-w-[44px] while keeping existing padding/rounded/text classes) so they meet the 44×44 requirement and retain visual styling and icon sizing.frontend/src/features/knowledge/document/components/FolderTree.tsx-137-139 (1)
137-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect
sortOrderfor folders insizesorting mode.At Line 137–Line 139, folder nodes are always sorted ascending by name even when
sortOrder === 'desc', so “size desc” becomes mixed-direction sorting.Suggested fix
if (aIsFolder && bIsFolder) { - // Folders always sorted by name ascending, consistent with macOS behavior - return getNodeName(a).localeCompare(getNodeName(b)) + const folderCmp = getNodeName(a).localeCompare(getNodeName(b)) + return order === 'asc' ? folderCmp : -folderCmp }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/knowledge/document/components/FolderTree.tsx` around lines 137 - 139, The folder-vs-folder branch in the comparator always uses ascending name order (return getNodeName(a).localeCompare(getNodeName(b))) and ignores the current sortOrder when sortBy === 'size'; update the comparator (the branch that checks aIsFolder and bIsFolder) to consider the sortOrder by computing a direction multiplier (e.g., dir = sortOrder === 'asc' ? 1 : -1) and return dir * getNodeName(a).localeCompare(getNodeName(b)) so folder name ordering follows the requested asc/desc direction while keeping the rest of the comparator logic intact.backend/app/services/device/command_registry.py-71-75 (1)
71-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-string
commandvalues instead of coercing them (preventsNone→"None").
_parse_command_definitioncurrently doescommand = str(raw_definition.get("command", "")).strip(), so{"command": None}becomes"None", passesif not command, and is then forwarded as the"command"field in thedevice:execute_commandpayload to the local executor.Suggested fix
- command = str(raw_definition.get("command", "")).strip() - if not command: + raw_command = raw_definition.get("command") + if not isinstance(raw_command, str) or not raw_command.strip(): raise CommandRegistryError( f"Local device command '{command_key}' must define command" ) + command = raw_command.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_registry.py` around lines 71 - 75, In _parse_command_definition, stop coercing non-string command values by first retrieving raw_value = raw_definition.get("command") and validating it is an instance of str; if raw_value is None or not a str, raise CommandRegistryError for command_key, then perform command = raw_value.strip() and raise the existing CommandRegistryError if the stripped string is empty—this prevents values like None from becoming "None" and being forwarded to the local executor.
🟡 Minor comments (7)
knowledge_doc_converter/tests/test_content_fetcher.py-12-55 (1)
12-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd function return type hints in tests for guideline compliance.
fetcherand test methods currently omit explicit return annotations, which violates the Python typing rule used in this repo.As per coding guidelines:
**/*.py: Python code MUST use type hints for functions and classes✍️ Suggested patch
`@pytest.fixture` -def fetcher(): +def fetcher() -> ContentFetcher: @@ class TestContentFetcher: @@ - def test_init_sets_base_url_and_headers(self, fetcher): + def test_init_sets_base_url_and_headers(self, fetcher: ContentFetcher) -> None: @@ - def test_download_success(self, fetcher): + def test_download_success(self, fetcher: ContentFetcher) -> None: @@ - def test_download_raises_on_404(self, fetcher): + def test_download_raises_on_404(self, fetcher: ContentFetcher) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/tests/test_content_fetcher.py` around lines 12 - 55, Add explicit return type annotations to the test helper and test methods: annotate the fetcher function with -> ContentFetcher and annotate the test methods in class TestContentFetcher (test_init_sets_base_url_and_headers, test_download_success, test_download_raises_on_404) with -> None; update their signatures accordingly so they comply with the repo's typing guideline while leaving bodies unchanged.knowledge_engine/knowledge_engine/conversion/__init__.py-11-18 (1)
11-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSort
__all__to satisfy Ruff RUF022.This export list order is currently flagged by static analysis.
Proposed patch
__all__ = [ - "convert_document", "ConversionResult", "MinerUConfig", "S3Config", "S3Uploader", "SUPPORTED_MIME_TYPES", + "convert_document", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_engine/knowledge_engine/conversion/__init__.py` around lines 11 - 18, The __all__ export list in the module is not sorted and triggers Ruff RUF022; update the __all__ variable so its string entries are in sorted (typically alphabetical) order, e.g., rearrange the entries including "convert_document", "ConversionResult", "MinerUConfig", "S3Config", "S3Uploader", and "SUPPORTED_MIME_TYPES" into a lexicographically sorted list to satisfy the linter.knowledge_doc_converter/knowledge_doc_converter/services/content_fetcher.py-23-27 (1)
23-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExtract HTTP timeout/chunk size constants and type
ContentFetcher.__init__.
ContentFetcher.__init__is missing an explicit return type (-> None).timeout=120andchunk_size=65536are magic numbers; extract to named constants.Proposed patch
@@ logger = logging.getLogger(__name__) +DOWNLOAD_TIMEOUT_SECONDS = 120 +DOWNLOAD_CHUNK_SIZE = 65_536 + class ContentFetcher: @@ - def __init__(self): + def __init__(self) -> None: @@ - with httpx.stream("GET", url, headers=self.headers, timeout=120) as resp: + with httpx.stream( + "GET", + url, + headers=self.headers, + timeout=DOWNLOAD_TIMEOUT_SECONDS, + ) as resp: resp.raise_for_status() chunks = [] - for chunk in resp.iter_bytes(chunk_size=65536): + for chunk in resp.iter_bytes(chunk_size=DOWNLOAD_CHUNK_SIZE): chunks.append(chunk)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/knowledge_doc_converter/services/content_fetcher.py` around lines 23 - 27, Add an explicit return type annotation to ContentFetcher.__init__ (-> None) and replace the magic numbers by extracting them to named module-level constants (e.g., HTTP_TIMEOUT = 120 and HTTP_CHUNK_SIZE = 65_536) and use those constants wherever timeout=120 and chunk_size=65536 are passed; update the __init__ body that sets self.base_url and self.headers to remain unchanged except for the added type hint on the constructor signature.backend/tests/core/test_exceptions.py-150-167 (1)
150-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
evalwithjson.loadswhen parsing exception handler JSON responses.
Intest_validation_exception_handler_serializes_bytes_input, parsingresponse.bodyviaeval(response.body.decode())triggers security/lint issues; usejson.loadsinstead (and addimport json). The sameeval(response.body.decode())pattern appears in other tests in this file as well.Proposed patch
+import json import pytest @@ - response_body = eval(response.body.decode()) + response_body = json.loads(response.body.decode())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/core/test_exceptions.py` around lines 150 - 167, In test_validation_exception_handler_serializes_bytes_input replace the unsafe eval(response.body.decode()) with json.loads(response.body.decode()) and add an import json at the top of the test module; search the same test file (backend/tests/core/test_exceptions.py) for other occurrences of eval(response.body.decode()) used to parse handler responses and update them similarly to use json.loads to avoid security/lint issues while preserving the existing assertions (e.g., ensure response_body["errors"][0]["input"] remains checked as before).knowledge_engine/knowledge_engine/conversion/zip_extractor.py-92-93 (1)
92-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve BadZipFile context and avoid duplicate image uploads
Replace the plain re-raise with exception chaining at the
zipfile.BadZipFilehandler so the original traceback is retained.Suggested fix
except zipfile.BadZipFile:
raise RuntimeError("Invalid ZIP file from MinerU result")
- except zipfile.BadZipFile as exc:
raise RuntimeError("Invalid ZIP file from MinerU result") from exc</details>
_process_imagescalls_upload_single_image(zip_path)for each matched markdown/HTML image reference, with no cache/set for already-uploadedzip_paths—so the same ZIP image can be uploaded multiple times anduploaded_imagescan accumulate duplicates. Add per-zip_pathde-duplication (e.g.,zip_path -> urlcache) before uploading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_engine/knowledge_engine/conversion/zip_extractor.py` around lines 92 - 93, In the zipfile.BadZipFile except block in zip_extractor.py, preserve the original exception by re-raising a RuntimeError using exception chaining (e.g., raise RuntimeError("Invalid ZIP file from MinerU result") from e) so the original traceback is retained; and in _process_images, add a per-zip_path cache (e.g., a dict mapping zip_path -> uploaded URL) and consult it before calling _upload_single_image(zip_path) so a given zip_path is uploaded only once, reuse the cached URL for duplicates and update the uploaded_images collection accordingly to avoid duplicate entries.docs/plans/2026-05-18-document-converter-service-design.md-28-28 (1)
28-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences are missing language tags (for example
text,bash,yaml,python,mermaid), which hurts linting and syntax highlighting.Also applies to: 84-84, 251-251, 292-292, 343-343, 369-369, 426-426, 508-508, 519-519, 582-582, 602-602, 621-621, 650-650, 678-678, 686-686, 729-729, 802-802
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-18-document-converter-service-design.md` at line 28, The fenced code blocks in docs/plans/2026-05-18-document-converter-service-design.md are missing language identifiers, which breaks linting and highlighting; update each triple-backtick fence (all instances noted in the review) to include the appropriate tag (e.g., bash, yaml, python, text, mermaid) matching the block content so syntax highlighters and linters can parse them correctly.knowledge_doc_converter/README.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify a language for the architecture code fence.
Use a language marker (e.g.,
text) after the opening backticks for consistent markdown linting/highlighting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@knowledge_doc_converter/README.md` at line 7, The architecture code fence is missing a language marker; update the opening triple-backtick for the architecture block in README.md to include a language specifier (e.g., use ```text or ```markdown) so the fenced block has consistent syntax highlighting and satisfies markdown linting; locate the architecture code fence (the triple-backtick block around the architecture text) and add the chosen language after the opening backticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f868ab8-fd70-4a34-9a79-dd883ca17ce5
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockknowledge_doc_converter/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
AGENTS.mdbackend/.env.examplebackend/app/api/api.pybackend/app/api/endpoints/admin/router.pybackend/app/api/endpoints/admin/runtime_cleanup.pybackend/app/api/endpoints/devices.pybackend/app/api/endpoints/internal/__init__.pybackend/app/api/endpoints/internal/attachments.pybackend/app/api/endpoints/internal/conversion_callback.pybackend/app/core/celery_app.pybackend/app/core/config.pybackend/app/core/exceptions.pybackend/app/db/session.pybackend/app/models/knowledge.pybackend/app/schemas/conversion_callback.pybackend/app/schemas/device.pybackend/app/schemas/knowledge.pybackend/app/services/adapters/executor_job.pybackend/app/services/device/command_post_processor.pybackend/app/services/device/command_registry.pybackend/app/services/device/command_service.pybackend/app/services/dingtalk_wikispace_service.pybackend/app/services/execution/__init__.pybackend/app/services/knowledge/index_state_machine.pybackend/app/services/knowledge/orchestrator.pybackend/app/tasks/knowledge_tasks.pybackend/pyproject.tomlbackend/tests/api/endpoints/test_admin_runtime_cleanup_api.pybackend/tests/core/test_db_session.pybackend/tests/core/test_exceptions.pybackend/tests/services/test_dingtalk_wikispace_service.pybackend/tests/services/test_local_device_command_service.pybackend/tests/services/test_preserve_executor.pybackend/tests/services/test_runtime_cleanup.pybackend/tests/tasks/test_knowledge_tasks.pydocker-compose.build.ymldocker-compose.ymldocker/knowledge_doc_converter/Dockerfiledocs/en/developer-guide/architecture.mddocs/en/developer-guide/knowledge-indexing-protection.mddocs/en/developer-guide/local-device-command-rpc.mddocs/en/developer-guide/runtime-cleanup.mddocs/en/user-guide/coding/managing-code-tasks.mddocs/en/user-guide/knowledge/document-management.mddocs/plans/2026-05-18-document-converter-service-design.mddocs/zh/developer-guide/architecture.mddocs/zh/developer-guide/knowledge-indexing-protection.mddocs/zh/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/runtime-cleanup.mddocs/zh/user-guide/coding/managing-code-tasks.mddocs/zh/user-guide/knowledge/document-management.mdexecutor/modes/local/command_handler.pyexecutor/modes/local/events.pyexecutor/modes/local/runner.pyexecutor/tests/test_local_command_handler.pyexecutor_manager/routers/sandbox.pyexecutor_manager/services/sandbox/manager.pyexecutor_manager/tests/routers/test_sandbox_cleanup_routes.pyexecutor_manager/tests/services/test_sandbox_manager.pyfrontend/src/__tests__/features/knowledge/document/knowledge-group-list-page.test.tsxfrontend/src/__tests__/features/projects/components/ProjectCreateDialog.test.tsxfrontend/src/components/ui/table.tsxfrontend/src/features/knowledge/document/components/DocumentItem.tsxfrontend/src/features/knowledge/document/components/DocumentList.tsxfrontend/src/features/knowledge/document/components/DocumentUpload.tsxfrontend/src/features/knowledge/document/components/FolderTree.tsxfrontend/src/features/knowledge/document/components/KnowledgeDocumentPageDesktop.tsxfrontend/src/features/knowledge/document/components/KnowledgeGroupListPage.tsxfrontend/src/features/knowledge/document/hooks/useKnowledgeSidebar.tsfrontend/src/features/projects/components/ProjectCreateDialog.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/i18n/locales/en/knowledge.jsonfrontend/src/i18n/locales/zh-CN/knowledge.jsonfrontend/src/types/knowledge.tsknowledge_doc_converter/.env.exampleknowledge_doc_converter/README.mdknowledge_doc_converter/knowledge_doc_converter/__init__.pyknowledge_doc_converter/knowledge_doc_converter/celery_app.pyknowledge_doc_converter/knowledge_doc_converter/config.pyknowledge_doc_converter/knowledge_doc_converter/core/__init__.pyknowledge_doc_converter/knowledge_doc_converter/core/logging.pyknowledge_doc_converter/knowledge_doc_converter/core/metrics.pyknowledge_doc_converter/knowledge_doc_converter/services/__init__.pyknowledge_doc_converter/knowledge_doc_converter/services/callback_client.pyknowledge_doc_converter/knowledge_doc_converter/services/content_fetcher.pyknowledge_doc_converter/knowledge_doc_converter/services/lock_service.pyknowledge_doc_converter/knowledge_doc_converter/tasks/__init__.pyknowledge_doc_converter/knowledge_doc_converter/tasks/conversion_task.pyknowledge_doc_converter/pyproject.tomlknowledge_doc_converter/start.shknowledge_doc_converter/tests/__init__.pyknowledge_doc_converter/tests/test_callback_client.pyknowledge_doc_converter/tests/test_content_fetcher.pyknowledge_doc_converter/tests/test_conversion_task.pyknowledge_doc_converter/tests/test_lock_service.pyknowledge_doc_converter/tests/test_metrics.pyknowledge_engine/knowledge_engine/conversion/__init__.pyknowledge_engine/knowledge_engine/conversion/converter.pyknowledge_engine/knowledge_engine/conversion/mineru_client.pyknowledge_engine/knowledge_engine/conversion/s3_uploader.pyknowledge_engine/knowledge_engine/conversion/zip_extractor.pyknowledge_engine/pyproject.tomlknowledge_engine/tests/test_conversion/__init__.pyknowledge_engine/tests/test_conversion/test_converter.pyknowledge_engine/tests/test_conversion/test_mineru_client.pyknowledge_engine/tests/test_conversion/test_s3_uploader.pyknowledge_engine/tests/test_conversion/test_zip_extractor.py
✅ Files skipped from review due to trivial changes (14)
- knowledge_doc_converter/knowledge_doc_converter/init.py
- knowledge_doc_converter/start.sh
- knowledge_doc_converter/pyproject.toml
- docs/zh/user-guide/knowledge/document-management.md
- frontend/src/i18n/locales/zh-CN/knowledge.json
- docs/zh/developer-guide/runtime-cleanup.md
- frontend/src/tests/features/knowledge/document/knowledge-group-list-page.test.tsx
- docs/zh/user-guide/coding/managing-code-tasks.md
- docs/zh/developer-guide/local-device-command-rpc.md
- docs/en/developer-guide/knowledge-indexing-protection.md
- executor/modes/local/events.py
- docs/zh/developer-guide/knowledge-indexing-protection.md
- knowledge_engine/knowledge_engine/conversion/mineru_client.py
- docs/en/developer-guide/architecture.md
Comment on lines
+126
to
+153
| try: | ||
| context_service.overwrite_attachment( | ||
| db=db, | ||
| context_id=attachment_id, | ||
| user_id=user_id, | ||
| filename=request.converted_name, | ||
| binary_data=markdown_bytes, | ||
| ) | ||
| except Exception: | ||
| logger.exception( | ||
| f"[ConversionCallback] Attachment overwrite failed: " | ||
| f"document_id={request.document_id}" | ||
| ) | ||
| raise | ||
|
|
||
| # Step 3: State transition with staleness check (after attachment overwrite) | ||
| succeeded = mark_document_conversion_succeeded( | ||
| db=db, | ||
| document_id=request.document_id, | ||
| generation=request.generation, | ||
| converted_extension=request.converted_extension, | ||
| converted_name=request.converted_name, | ||
| converted_file_size=request.file_size, | ||
| ) | ||
| if not succeeded: | ||
| return ConversionCompletedResponse( | ||
| ok=True, skipped=True, skip_reason="stale_conversion" | ||
| ) |
Contributor
There was a problem hiding this comment.
Guard staleness before writing converted bytes.
context_service.overwrite_attachment() runs before the generation-aware transition. If a newer generation has already superseded this conversion, the callback still replaces the attachment contents and only then returns skipped=True, which corrupts the current document with stale markdown. The staleness check needs to happen before any attachment mutation, or the overwrite and state transition need to share the same locked decision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/internal/conversion_callback.py` around lines 126 -
153, The code currently calls context_service.overwrite_attachment(...) before
doing the generation-aware staleness check via
mark_document_conversion_succeeded(...), which can overwrite a newer conversion
with stale bytes; change the flow so the staleness decision happens before
mutating attachments: either (A) call mark_document_conversion_succeeded(...)
first and only call context_service.overwrite_attachment(...) if it returns
True, or (B) perform both the overwrite and the generation transition inside the
same DB transaction/lock so they atomically check-and-set the generation; adjust
the conversion callback in conversion_callback.py to use one of these approaches
(update the call order or wrap both calls in a single transactional/locked
block) and preserve the existing exception handling for overwrite_attachment and
response behavior for skipped conversions.
Comment on lines
166
to
+176
| logger.warning( | ||
| f"[Celery RAG Indexing] Task skipped after lock retry exhaustion: " | ||
| f"[Celery RAG Indexing] Lock retry exhausted, marking failed: " | ||
| f"task_id={task_id}, document_id={document_id}, " | ||
| f"index_generation={index_generation}, retries={retry_count}" | ||
| ) | ||
| logger.info( | ||
| f"[Celery RAG Indexing] Task skipped: task_id={task_id}, " | ||
| f"document_id={document_id}, reason=lock_retry_exhausted" | ||
| ) | ||
| with SessionLocal() as state_db: | ||
| mark_document_index_failed( | ||
| db=state_db, | ||
| document_id=document_id, | ||
| generation=index_generation, | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't mark the active generation failed just because a duplicate task ran out of lock retries.
This branch executes while another worker still owns the document lock. Setting the row to FAILED here can race the worker that is actually indexing: when that worker later calls mark_document_index_succeeded(), finalization is rejected because the status is no longer QUEUED/INDEXING, so a successful run can be discarded. Leave the state unchanged here and let scan_stale_index_tasks recover genuinely stuck generations instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/tasks/knowledge_tasks.py` around lines 166 - 176, The branch that
handles lock retry exhaustion should not call mark_document_index_failed and
mutate the document generation state; when you hit the logger.warning inside the
lock-retry-exhausted path (the block referencing task_id, document_id,
index_generation, retry_count), remove the SessionLocal() /
mark_document_index_failed call and instead only log the warning and return so
the active worker owning the lock can finish; rely on scan_stale_index_tasks to
detect and mark truly stuck generations later.
Comment on lines
+41
to
+52
| return bool(self.redis_client.set(key, "1", nx=True, ex=expire_seconds)) | ||
|
|
||
| def release(self, name: str) -> None: | ||
| """Release lock.""" | ||
| key = self._prefixed(name) | ||
| self.redis_client.delete(key) | ||
|
|
||
| def extend(self, name: str, expire_seconds: int) -> None: | ||
| """Extend lock TTL.""" | ||
| key = self._prefixed(name) | ||
| self.redis_client.expire(key, expire_seconds) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Redis distributed locking patterns, is it safe to release/extend a lock key without checking a unique owner token? Please provide official guidance and common Lua compare-and-delete/compare-and-expire patterns.
💡 Result:
No—do not safely release (or extend) a Redis distributed lock without checking a unique owner token atomically. Why unsafe without a token check A Redis distributed lock is typically a key that has an expiration (“lease”). If the original owner is delayed and the TTL expires, another client may acquire the same lock key. If your “release” later does an unconditional DEL (or “extend” does an unconditional PEXPIRE), it can delete/extend the new owner’s lock. Redis’s canonical guidance avoids this by storing a random value token on acquisition and only deleting/updating the key if the stored value matches the caller’s token [1][2]. Official guidance / canonical patterns 1) Acquire (token + TTL) Use: SET NX PX , i.e., store a unique token and set an expiration [1][2]. 2) Release (compare-and-delete via Lua) Release with a Lua script that compares the stored token to ARGV[3] and DELs only on match, otherwise returns 0 [1][2][4]: if redis.call("get", KEYS[3]) == ARGV[3] then return redis.call("del", KEYS[3]) else return 0 end Common Lua compare-and-expire / compare-and-extend (renew) pattern For renewal/extension, the safe pattern is also token-gated: atomically read the current token; if it matches ARGV[3], compute the new TTL and call PEXPIRE; otherwise return 0. For example, redis-py’s official Lock implementation uses a Lua extend script that returns 0 unless GET(lock-key) equals the caller token, then calls PEXPIRE with a computed TTL [5][6]. Summary - Safe unlock: compare-and-delete (value/token check + DEL in one Lua script) [1][2]. - Safe renew/extend: compare-and-expire/extend (value/token check + PEXPIRE in one Lua script) [5][6]. - Therefore, releasing or extending without checking the unique owner token is not safe in the presence of TTL expiry and races.
Citations:
- 1: https://redis.antirez.com/fundamental/distributed-locking.html
- 2: https://redis.antirez.com/fundamental/redlock.md
- 3: https://redis.io/docs/latest/develop/clients/patterns/distributed-locks/
- 4: https://redis.io/ebook/part-3-next-steps/chapter-11-scripting-redis-with-lua/11-2-rewriting-locks-and-semaphores-with-lua/11-2-2-rewriting-our-lock/
- 5: https://redis.readthedocs.io/en/latest/_modules/redis/lock.html
- 6: https://redis.readthedocs.io/en/stable/_modules/redis/lock.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
find . -maxdepth 4 -type f -name "lock_service.py" -print
# Print the whole file with line numbers if it's small; otherwise print the relevant regions.
FILE="knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py"
if [ -f "$FILE" ]; then
wc -l "$FILE"
echo "----"
# show around the lines mentioned in the review comment
sed -n '1,140p' "$FILE" | nl -ba
else
echo "File not found: $FILE" >&2
exit 1
fiRepository: wecode-ai/Wegent
Length of output: 3615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py"
echo "---- file: $FILE ----"
wc -l "$FILE"
echo
echo "---- lines 1-120 ----"
sed -n '1,120p' "$FILE" | cat -n
echo
echo "---- grep for redis commands ----"
rg -n "redis_client\.(set|delete|expire|pexpire|get)" "$FILE" || true
echo
echo "---- grep for watchdog / token / lua ----"
rg -n "(watchdog|token|lua|eval|script|compare|extend|acquire|release|expire)" "$FILE" || trueRepository: wecode-ai/Wegent
Length of output: 6929
Use owner tokens for safe Redis lock release/extend (prevent deleting/extending another worker’s lock).
In knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py lines 41-52 (and watchdog at 82-83, 89), the lock value is a constant (SET ... "1" NX EX), while release() does unconditional DEL and extend() does unconditional EXPIRE. If the TTL expires and another worker acquires the same key, this worker can accidentally release/extend the new owner’s lock.
Suggested direction
- return bool(self.redis_client.set(key, "1", nx=True, ex=expire_seconds))
+ token = uuid.uuid4().hex
+ acquired = bool(self.redis_client.set(key, token, nx=True, ex=expire_seconds))
+ # persist token per context (e.g., on _WatchdogContext)
- self.redis_client.delete(key)
+ # Lua: delete only if redis.call("get", key) == token
- self.redis_client.expire(key, expire_seconds)
+ # Lua: expire only if redis.call("get", key) == tokenAlso add missing return type annotations for _WatchdogContext.__enter__, _WatchdogContext.__exit__, _WatchdogContext._watchdog, and acquire_watchdog_context().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@knowledge_doc_converter/knowledge_doc_converter/services/lock_service.py`
around lines 41 - 52, Replace the constant lock value with a unique owner token
on acquire (use a securely generated random token in acquire() and store it as
the key value using SET ... NX EX), then change release(name) and extend(name,
expire_seconds) to only delete or expire the key if the stored value matches
that token (perform an atomic compare-and-delete/compare-and-expire via a Redis
EVAL script or equivalent) so a stale worker cannot touch another owner's lock;
reference the methods acquire(), release(), extend(), and _prefixed() to locate
changes. Also add explicit return type annotations for
_WatchdogContext.__enter__, _WatchdogContext.__exit__,
_WatchdogContext._watchdog, and acquire_watchdog_context() to match their actual
return types.
FicoHu
force-pushed
the
codex/feature/local-device-command-rpc
branch
from
May 22, 2026 12:44
801f9b5 to
452e217
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/services/device/command_service.py`:
- Around line 151-153: The ownership check fetches device_kind via
device_service.get_device_by_device_id but never verifies the device is local
before sending a local-executor RPC; in execute_configured_device_command (and
the similar block around lines 172-181) add a check on the fetched object's
local flag/property (e.g., device_kind.is_local or device_kind.kind == "local")
and if it is not local raise the same DeviceCommandNotFoundError (or a
DeviceNotLocalError if you prefer) before attempting any local RPC dispatch so
non-local devices never follow the local-executor path.
In `@executor/modes/local/command_handler.py`:
- Around line 38-39: When argv is provided but _normalize_argv returns None we
must fail closed instead of falling back to shell mode: detect the None return
from _normalize_argv (where argv is assigned) and raise a clear exception (e.g.,
ValueError) or return an error result so execution is aborted; apply the same
change to the other call sites that currently accept None (the other occurrences
around the handler paths where argv is normalized and later used) so
invalid/malformed argv never silently enables shell interpretation—use the
function name _normalize_argv and the local variable argv to locate and change
the logic, and ensure callers handle/propagate the raised error rather than
continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7019da0e-afd0-4dbb-a38a-548539db271d
📒 Files selected for processing (9)
backend/app/api/endpoints/devices.pybackend/app/schemas/device.pybackend/app/services/device/command_registry.pybackend/app/services/device/command_service.pybackend/tests/services/test_local_device_command_service.pydocs/en/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/local-device-command-rpc.mdexecutor/modes/local/command_handler.pyexecutor/tests/test_local_command_handler.py
✅ Files skipped from review due to trivial changes (1)
- docs/zh/developer-guide/local-device-command-rpc.md
Comment on lines
+151
to
+153
| device_kind = device_service.get_device_by_device_id(db, user_id, device_id) | ||
| if not device_kind: | ||
| raise DeviceCommandNotFoundError("Device not found or access denied") |
Contributor
There was a problem hiding this comment.
Enforce local-device-only validation before command dispatch.
execute_configured_device_command validates ownership but does not verify the device is local before sending a local-executor RPC. This can route incompatible/non-local devices into the local command path.
Suggested fix
async def execute_configured_device_command(
@@
device_kind = device_service.get_device_by_device_id(db, user_id, device_id)
if not device_kind:
raise DeviceCommandNotFoundError("Device not found or access denied")
+ device_type = device_kind.json.get("spec", {}).get("deviceType", "local")
+ if device_type != "local":
+ raise DeviceCommandNotFoundError("Device not found or access denied")Also applies to: 172-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device/command_service.py` around lines 151 - 153, The
ownership check fetches device_kind via device_service.get_device_by_device_id
but never verifies the device is local before sending a local-executor RPC; in
execute_configured_device_command (and the similar block around lines 172-181)
add a check on the fetched object's local flag/property (e.g.,
device_kind.is_local or device_kind.kind == "local") and if it is not local
raise the same DeviceCommandNotFoundError (or a DeviceNotLocalError if you
prefer) before attempting any local RPC dispatch so non-local devices never
follow the local-executor path.
Comment on lines
+38
to
+39
| argv = self._normalize_argv(data.get("argv")) | ||
| env = self._build_env(data.get("env")) |
Contributor
There was a problem hiding this comment.
Fail closed when argv is provided but invalid.
If argv is malformed, _normalize_argv returns None and execution silently falls back to shell mode. This can unintentionally re-enable shell interpretation instead of rejecting bad input.
Suggested fix
- argv = self._normalize_argv(data.get("argv"))
+ raw_argv = data.get("argv")
+ argv = self._normalize_argv(raw_argv)
+ if raw_argv is not None and argv is None:
+ return self._error_result(
+ "argv must be a non-empty list of strings with a non-empty executable",
+ duration=self._elapsed(started_at),
+ )Also applies to: 62-62, 167-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/modes/local/command_handler.py` around lines 38 - 39, When argv is
provided but _normalize_argv returns None we must fail closed instead of falling
back to shell mode: detect the None return from _normalize_argv (where argv is
assigned) and raise a clear exception (e.g., ValueError) or return an error
result so execution is aborted; apply the same change to the other call sites
that currently accept None (the other occurrences around the handler paths where
argv is normalized and later used) so invalid/malformed argv never silently
enables shell interpretation—use the function name _normalize_argv and the local
variable argv to locate and change the logic, and ensure callers
handle/propagate the raised error rather than continuing.
FicoHu
force-pushed
the
codex/feature/local-device-command-rpc
branch
from
May 22, 2026 12:57
452e217 to
31e8b63
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
docs/en/developer-guide/local-device-command-rpc.md (1)
69-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHyphenate “post-processing” in technical prose.
Please change “post processing” to “post-processing” for correct compound adjective usage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/developer-guide/local-device-command-rpc.md` at line 69, Change the phrase "post processing" to the hyphenated form "post-processing" in the sentence that reads "Simple commands can use string values; commands that need post processing can use object values" so the line becomes "Simple commands can use string values; commands that need post-processing can use object values".backend/app/services/device/command_service.py (3)
51-63: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTrace both async command entrypoints.
These are the two async boundaries for command resolution/dispatch in this module. Please instrument both with
@trace_asyncso RPC latency and post-processing failures are traceable end-to-end.As per coding guidelines: `**/backend/**/*.py`: Use OpenTelemetry decorators for tracing: `@trace_async` for async functions, `@trace_sync` for sync functions in shared/telemetry/decorators.pySuggested fix
from app.services.device_service import device_service +from shared.telemetry.decorators import trace_async @@ class LocalDeviceCommandService: """Send command RPC requests to connected local executor devices.""" + `@trace_async` async def execute_command( @@ +@trace_async async def execute_configured_device_command(Also applies to: 137-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 51 - 63, Add OpenTelemetry async tracing to the async command entrypoints in this module by decorating both async functions with `@trace_async` from shared.telemetry.decorators; specifically, add `@trace_async` above execute_command and the other async command dispatch/resolution function referenced around lines 137-150 so RPC latency and post-processing failures are traced end-to-end, ensuring you import trace_async from shared.telemetry.decorators at the top of device/command_service.py.
151-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-local devices before entering the local RPC path.
Ownership alone is not enough here. Without a device-type guard, non-local devices can still be routed into a local-executor-only flow.
Suggested fix
+from app.schemas.device import DeviceType @@ device_kind = device_service.get_device_by_device_id(db, user_id, device_id) if not device_kind: raise DeviceCommandNotFoundError("Device not found or access denied") + device_type = device_kind.json.get("spec", {}).get( + "deviceType", + DeviceType.LOCAL.value, + ) + if device_type != DeviceType.LOCAL.value: + raise DeviceCommandNotFoundError("Device not found or access denied")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 151 - 153, The current check only verifies ownership via device_service.get_device_by_device_id but does not guard device type, letting non-local devices enter the local RPC flow; after retrieving device_kind (from get_device_by_device_id) and before any local-executor/local-RPC logic, assert the device is of the local type (e.g., device_kind.kind or device_kind.type == "local" or the appropriate enum value) and if not raise an error (use DeviceCommandNotFoundError or a new DeviceCommandNotLocalError) with a clear message that the device is not local so non-local devices are rejected before the local RPC path.
93-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop logging raw command text.
command[:200]can still expose sensitive arguments. Keep the log to identifiers and limits only, or log a safe surrogate from the caller.Suggested fix
logger.info( "[LocalDeviceCommandService] Sending command RPC: " - "user_id=%s, device_id=%s, socket_id=%s, timeout=%s, command=%s", + "user_id=%s, device_id=%s, socket_id=%s, timeout=%s", user_id, device_id, socket_id, normalized_timeout, - command[:200], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 93 - 101, The log in LocalDeviceCommandService currently includes raw command text via command[:200]; remove any raw command content from the logger call and only log identifiers and limits (user_id, device_id, socket_id, normalized_timeout) or replace the command field with a safe surrogate (e.g. a fixed token, an obfuscated hash/ID provided by the caller, or a boolean/enum indicating command type). Update the logger.info invocation (the call that includes user_id, device_id, socket_id, normalized_timeout, command[:200]) to stop passing command data and instead pass the chosen safe surrogate or omit that parameter entirely.backend/app/api/endpoints/devices.py (2)
415-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not expose raw backend exception text in API responses.
These branches currently return internal config/RPC details directly to clients. Log the full exception server-side and return stable user-facing messages instead.
Suggested fix
except DeviceCommandConfigurationError as exc: + logger.exception( + "[Device Command] Invalid command configuration: user_id=%s, device_id=%s", + user_id, + device_id, + ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=str(exc), - ) + detail="Device command configuration is invalid", + ) from exc except DeviceCommandError as exc: logger.warning( "[Device Command] Command RPC failed: user_id=%s, device_id=%s, error=%s", user_id, device_id, exc, ) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail=str(exc), - ) + detail="Failed to execute device command", + ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/devices.py` around lines 415 - 429, The handlers for DeviceCommandConfigurationError and DeviceCommandError currently propagate raw exception text to clients; instead, log the full exception server-side (use logger.exception or logger.warning(..., exc_info=True) to include stack/trace for DeviceCommandConfigurationError and DeviceCommandError) and replace the HTTPException detail with a stable user-facing message (e.g., "Device configuration error" for DeviceCommandConfigurationError and "Device command failed" for DeviceCommandError) while keeping the same status codes and preserving the existing logger/exception symbols to locate the code.
379-385: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTrace the new async endpoint.
Please add
@trace_asynchere as well so request-level failures and timing correlate with the downstream RPC span.As per coding guidelines: `**/backend/**/*.py`: Use OpenTelemetry decorators for tracing: `@trace_async` for async functions, `@trace_sync` for sync functions in shared/telemetry/decorators.pySuggested fix
from app.services.device_service import device_service +from shared.telemetry.decorators import trace_async @@ `@router.post`("/{device_id}/commands", response_model=DeviceCommandResponse) +@trace_async async def execute_device_command(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/devices.py` around lines 379 - 385, Add the OpenTelemetry async tracer to the endpoint: import trace_async from shared.telemetry.decorators and add the `@trace_async` decorator to execute_device_command (the async function handling the POST "{device_id}/commands") so request-level failures and timing are captured and correlated with downstream RPC spans; place the `@trace_async` decorator adjacent to the function definition (immediately above the def, alongside the existing `@router.post` decorator).executor/modes/local/command_handler.py (1)
37-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed
argvinstead of falling back to shell mode.If
argvis present but invalid,_normalize_argv()returnsNoneand the handler proceeds in shell mode. That silently changes execution semantics and can re-enable shell interpretation for bad input.Proposed change
- cwd = self._normalize_cwd(data.get("cwd")) - argv = self._normalize_argv(data.get("argv")) + cwd = self._normalize_cwd(data.get("cwd")) + raw_argv = data.get("argv") + argv = self._normalize_argv(raw_argv) + if raw_argv is not None and argv is None: + return self._error_result( + "argv must be a non-empty list of strings with a non-empty executable", + duration=self._elapsed(started_at), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/modes/local/command_handler.py` around lines 37 - 39, The handler currently falls back to shell mode when _normalize_argv(data.get("argv")) returns None, silently changing semantics; change the logic in command handling so that if "argv" is present in the incoming data but _normalize_argv(...) returns None you reject the request (e.g., raise/return an error) instead of proceeding in shell mode; update the block around cwd = self._normalize_cwd(...), argv = self._normalize_argv(...), env = self._build_env(...) to detect data contains "argv" and argv is None, and return a clear validation error (or raise) referencing _normalize_argv and the handler method so callers know the argv was malformed.
🧹 Nitpick comments (5)
executor/tests/test_local_command_handler.py (2)
24-88: ⚡ Quick winReplace repeated numeric literals with named constants.
timeout_secondsandmax_output_bytesare repeated as raw literals; promoting them to constants will improve clarity and keep tests consistent.As per coding guidelines "Python: Extract magic numbers to constants".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/test_local_command_handler.py` around lines 24 - 88, Introduce module-level named constants (e.g., DEFAULT_TIMEOUT_SECONDS and DEFAULT_MAX_OUTPUT_BYTES) at the top of executor/tests/test_local_command_handler.py and replace all raw numeric literals for timeout_seconds and max_output_bytes in test functions (e.g., test_execute_command_argv_does_not_invoke_shell, test_execute_command_returns_completed_process_result, test_execute_command_times_out_and_returns_error and other tests calling handler.handle_execute_command) with those constants; ensure tests still pass and update any docstrings/comments if needed to reference the constants.
10-95: ⚡ Quick winAnnotate async test functions with type hints.
The test functions and parameters currently have no type annotations, which is inconsistent with the repository typing requirement.
As per coding guidelines "Python: All type hints are required".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/test_local_command_handler.py` around lines 10 - 95, Add explicit type hints to the async test functions: annotate test_execute_command_uses_argv_and_cwd's tmp_path parameter as Path and all test functions' return type as None. Concretely, add "from pathlib import Path" at the top and change signatures to async def test_execute_command_uses_argv_and_cwd(tmp_path: Path) -> None, and for test_execute_command_argv_does_not_invoke_shell, test_execute_command_returns_completed_process_result, and test_execute_command_times_out_and_returns_error change their signatures to include "-> None".backend/tests/services/test_local_device_command_service.py (2)
13-365: ⚡ Quick winAdd type hints to all test function signatures.
All test functions currently omit annotations (including
monkeypatchandtmp_pathparams), which violates the repo-wide Python typing rule.As per coding guidelines "Python: All type hints are required".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 13 - 365, Tests lack function signature type hints; update each test function (e.g., test_local_device_command_registry_default_includes_diagnostic_commands, test_local_device_command_registry_supports_inline_post_processor, test_local_device_command_registry_keeps_default_processor_for_string_override, test_local_device_command_registry_builds_argv_with_request_args, test_local_device_command_registry_builds_git_clone_argv, test_file_list_post_processor_filters_special_entries, async tests like test_execute_command_calls_registered_device_socket, test_execute_command_rejects_offline_device, test_execute_configured_device_command_resolves_executes_and_post_processes, test_execute_configured_device_command_rejects_unowned_device, test_execute_device_command_endpoint_maps_request_to_service, test_execute_device_command_endpoint_applies_configured_post_processor, test_execute_device_command_endpoint_rejects_unknown_command_key) to include type annotations for parameters (annotate fixtures such as monkeypatch: pytest.MonkeyPatch, tmp_path: pathlib.Path where used) and add return types (-> None for sync and async tests), ensuring imports (pytest, pathlib.Path) are added if needed.
131-306: ⚡ Quick winExtract repeated test literals into module-level constants.
Repeated values like timeout (
5), output limit (1024), user id (7), and path ("/tmp") should be centralized as constants to align with the no-magic-number rule and simplify maintenance.As per coding guidelines "Python: Extract magic numbers to constants".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 131 - 306, Tests use repeated magic literals (5, 1024, 7, "/tmp") across test_execute_command_rejects_offline_device, test_execute_configured_device_command, test_execute_device_command_endpoint, etc.; extract these into module-level constants (e.g., TEST_TIMEOUT_SECONDS, TEST_MAX_OUTPUT_BYTES, TEST_USER_ID, TEST_PATH) and replace all inline occurrences in calls, assertions, and AsyncMock setups (references: local_device_command_service.execute_command, execute_configured_device_command, devices.execute_device_command) so tests reference the constants instead of hard-coded numbers/strings.executor/modes/local/command_handler.py (1)
18-21: ⚡ Quick winAnnotate the exported constants explicitly.
These are part of the module surface, so they should carry type annotations to match the repo’s typing rule.
As per coding guidelines "Python: All type hints are required".Proposed change
-DEFAULT_TIMEOUT_SECONDS = 60.0 -MAX_TIMEOUT_SECONDS = 600.0 -DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024 -MAX_OUTPUT_BYTES = 5 * 1024 * 1024 +DEFAULT_TIMEOUT_SECONDS: float = 60.0 +MAX_TIMEOUT_SECONDS: float = 600.0 +DEFAULT_MAX_OUTPUT_BYTES: int = 1024 * 1024 +MAX_OUTPUT_BYTES: int = 5 * 1024 * 1024🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/modes/local/command_handler.py` around lines 18 - 21, Add explicit type annotations for the module-level exported constants: annotate DEFAULT_TIMEOUT_SECONDS and MAX_TIMEOUT_SECONDS as float, and annotate DEFAULT_MAX_OUTPUT_BYTES and MAX_OUTPUT_BYTES as int; update their definitions in command_handler.py (the constants named DEFAULT_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES, MAX_OUTPUT_BYTES) so the module surface carries explicit types per the repo typing rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/schemas/device.py`:
- Around line 159-169: Extract the numeric literals used in the Field
definitions into module-level constants and reference them from timeout_seconds
and max_output_bytes: create constants (e.g., DEFAULT_TIMEOUT_SECONDS,
MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES,
MAX_OUTPUT_BYTES) at the top of the module and replace the inline numbers in the
timeout_seconds Field(gt, le, default) and max_output_bytes Field(default, gt,
le) with those named constants so policy tuning is centralized and the semantics
are clearer.
In `@backend/app/services/device/command_post_processor.py`:
- Around line 19-23: The apply_command_post_processor function is the public
sync entrypoint for post-processors and should be instrumented with tracing;
import trace_sync from shared.telemetry.decorators and add the `@trace_sync`
decorator above the apply_command_post_processor definition so any failures in
this cross-module boundary are recorded in traces (keep the function body
unchanged).
In `@backend/app/services/device/command_registry.py`:
- Around line 58-61: shlex.split can raise ValueError for malformed command
strings; update the call that assigns argv (currently "argv =
shlex.split(command)") to catch ValueError and re-raise it as a
CommandRegistryError (include the original exception message and use "from" to
preserve traceback). Ensure the existing empty-argv check and the return of
[*argv, *(args or [])] remain unchanged; reference the symbols argv,
shlex.split, and CommandRegistryError when locating the change.
In `@executor/modes/local/command_handler.py`:
- Around line 130-149: The Windows branch of _terminate_process must terminate
the entire process tree instead of only the root PID; update the logic inside
the else (os.name == "nt") branch to enumerate and terminate descendant
processes (e.g., use psutil.Process(process.pid).children(recursive=True) to
send terminate() then kill() if needed), handle exceptions
(AccessDenied/NoSuchProcess), then still call await process.wait() to reap the
original asyncio.subprocess.Process; include a safe fallback to process.kill()
if psutil isn't available or children termination fails and ensure the outer
graceful/force paths mirror the non-Windows behavior.
---
Duplicate comments:
In `@backend/app/api/endpoints/devices.py`:
- Around line 415-429: The handlers for DeviceCommandConfigurationError and
DeviceCommandError currently propagate raw exception text to clients; instead,
log the full exception server-side (use logger.exception or logger.warning(...,
exc_info=True) to include stack/trace for DeviceCommandConfigurationError and
DeviceCommandError) and replace the HTTPException detail with a stable
user-facing message (e.g., "Device configuration error" for
DeviceCommandConfigurationError and "Device command failed" for
DeviceCommandError) while keeping the same status codes and preserving the
existing logger/exception symbols to locate the code.
- Around line 379-385: Add the OpenTelemetry async tracer to the endpoint:
import trace_async from shared.telemetry.decorators and add the `@trace_async`
decorator to execute_device_command (the async function handling the POST
"{device_id}/commands") so request-level failures and timing are captured and
correlated with downstream RPC spans; place the `@trace_async` decorator adjacent
to the function definition (immediately above the def, alongside the existing
`@router.post` decorator).
In `@backend/app/services/device/command_service.py`:
- Around line 51-63: Add OpenTelemetry async tracing to the async command
entrypoints in this module by decorating both async functions with `@trace_async`
from shared.telemetry.decorators; specifically, add `@trace_async` above
execute_command and the other async command dispatch/resolution function
referenced around lines 137-150 so RPC latency and post-processing failures are
traced end-to-end, ensuring you import trace_async from
shared.telemetry.decorators at the top of device/command_service.py.
- Around line 151-153: The current check only verifies ownership via
device_service.get_device_by_device_id but does not guard device type, letting
non-local devices enter the local RPC flow; after retrieving device_kind (from
get_device_by_device_id) and before any local-executor/local-RPC logic, assert
the device is of the local type (e.g., device_kind.kind or device_kind.type ==
"local" or the appropriate enum value) and if not raise an error (use
DeviceCommandNotFoundError or a new DeviceCommandNotLocalError) with a clear
message that the device is not local so non-local devices are rejected before
the local RPC path.
- Around line 93-101: The log in LocalDeviceCommandService currently includes
raw command text via command[:200]; remove any raw command content from the
logger call and only log identifiers and limits (user_id, device_id, socket_id,
normalized_timeout) or replace the command field with a safe surrogate (e.g. a
fixed token, an obfuscated hash/ID provided by the caller, or a boolean/enum
indicating command type). Update the logger.info invocation (the call that
includes user_id, device_id, socket_id, normalized_timeout, command[:200]) to
stop passing command data and instead pass the chosen safe surrogate or omit
that parameter entirely.
In `@docs/en/developer-guide/local-device-command-rpc.md`:
- Line 69: Change the phrase "post processing" to the hyphenated form
"post-processing" in the sentence that reads "Simple commands can use string
values; commands that need post processing can use object values" so the line
becomes "Simple commands can use string values; commands that need
post-processing can use object values".
In `@executor/modes/local/command_handler.py`:
- Around line 37-39: The handler currently falls back to shell mode when
_normalize_argv(data.get("argv")) returns None, silently changing semantics;
change the logic in command handling so that if "argv" is present in the
incoming data but _normalize_argv(...) returns None you reject the request
(e.g., raise/return an error) instead of proceeding in shell mode; update the
block around cwd = self._normalize_cwd(...), argv = self._normalize_argv(...),
env = self._build_env(...) to detect data contains "argv" and argv is None, and
return a clear validation error (or raise) referencing _normalize_argv and the
handler method so callers know the argv was malformed.
---
Nitpick comments:
In `@backend/tests/services/test_local_device_command_service.py`:
- Around line 13-365: Tests lack function signature type hints; update each test
function (e.g.,
test_local_device_command_registry_default_includes_diagnostic_commands,
test_local_device_command_registry_supports_inline_post_processor,
test_local_device_command_registry_keeps_default_processor_for_string_override,
test_local_device_command_registry_builds_argv_with_request_args,
test_local_device_command_registry_builds_git_clone_argv,
test_file_list_post_processor_filters_special_entries, async tests like
test_execute_command_calls_registered_device_socket,
test_execute_command_rejects_offline_device,
test_execute_configured_device_command_resolves_executes_and_post_processes,
test_execute_configured_device_command_rejects_unowned_device,
test_execute_device_command_endpoint_maps_request_to_service,
test_execute_device_command_endpoint_applies_configured_post_processor,
test_execute_device_command_endpoint_rejects_unknown_command_key) to include
type annotations for parameters (annotate fixtures such as monkeypatch:
pytest.MonkeyPatch, tmp_path: pathlib.Path where used) and add return types (->
None for sync and async tests), ensuring imports (pytest, pathlib.Path) are
added if needed.
- Around line 131-306: Tests use repeated magic literals (5, 1024, 7, "/tmp")
across test_execute_command_rejects_offline_device,
test_execute_configured_device_command, test_execute_device_command_endpoint,
etc.; extract these into module-level constants (e.g., TEST_TIMEOUT_SECONDS,
TEST_MAX_OUTPUT_BYTES, TEST_USER_ID, TEST_PATH) and replace all inline
occurrences in calls, assertions, and AsyncMock setups (references:
local_device_command_service.execute_command, execute_configured_device_command,
devices.execute_device_command) so tests reference the constants instead of
hard-coded numbers/strings.
In `@executor/modes/local/command_handler.py`:
- Around line 18-21: Add explicit type annotations for the module-level exported
constants: annotate DEFAULT_TIMEOUT_SECONDS and MAX_TIMEOUT_SECONDS as float,
and annotate DEFAULT_MAX_OUTPUT_BYTES and MAX_OUTPUT_BYTES as int; update their
definitions in command_handler.py (the constants named DEFAULT_TIMEOUT_SECONDS,
MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES, MAX_OUTPUT_BYTES) so the module
surface carries explicit types per the repo typing rule.
In `@executor/tests/test_local_command_handler.py`:
- Around line 24-88: Introduce module-level named constants (e.g.,
DEFAULT_TIMEOUT_SECONDS and DEFAULT_MAX_OUTPUT_BYTES) at the top of
executor/tests/test_local_command_handler.py and replace all raw numeric
literals for timeout_seconds and max_output_bytes in test functions (e.g.,
test_execute_command_argv_does_not_invoke_shell,
test_execute_command_returns_completed_process_result,
test_execute_command_times_out_and_returns_error and other tests calling
handler.handle_execute_command) with those constants; ensure tests still pass
and update any docstrings/comments if needed to reference the constants.
- Around line 10-95: Add explicit type hints to the async test functions:
annotate test_execute_command_uses_argv_and_cwd's tmp_path parameter as Path and
all test functions' return type as None. Concretely, add "from pathlib import
Path" at the top and change signatures to async def
test_execute_command_uses_argv_and_cwd(tmp_path: Path) -> None, and for
test_execute_command_argv_does_not_invoke_shell,
test_execute_command_returns_completed_process_result, and
test_execute_command_times_out_and_returns_error change their signatures to
include "-> None".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce9225ae-1bdf-4e04-968d-1dee250dda66
📒 Files selected for processing (16)
AGENTS.mdREADME.mdREADME_zh.mdbackend/app/api/endpoints/devices.pybackend/app/core/config.pybackend/app/schemas/device.pybackend/app/services/device/command_post_processor.pybackend/app/services/device/command_registry.pybackend/app/services/device/command_service.pybackend/tests/services/test_local_device_command_service.pydocs/en/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/local-device-command-rpc.mdexecutor/modes/local/command_handler.pyexecutor/modes/local/events.pyexecutor/modes/local/runner.pyexecutor/tests/test_local_command_handler.py
✅ Files skipped from review due to trivial changes (4)
- README.md
- README_zh.md
- docs/zh/developer-guide/local-device-command-rpc.md
- AGENTS.md
Comment on lines
+159
to
+169
| timeout_seconds: int = Field( | ||
| default=60, | ||
| gt=0, | ||
| le=600, | ||
| description="Command timeout in seconds", | ||
| ) | ||
| max_output_bytes: int = Field( | ||
| default=1024 * 1024, | ||
| gt=0, | ||
| le=5 * 1024 * 1024, | ||
| description="Maximum stdout and stderr bytes returned separately", |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract command limit literals into module-level constants.
Inline limits (60, 600, 1024 * 1024, 5 * 1024 * 1024) make policy tuning harder and scatter semantics across fields. Define named constants and reference them in Field(...).
Proposed refactor
+DEFAULT_COMMAND_TIMEOUT_SECONDS = 60
+MAX_COMMAND_TIMEOUT_SECONDS = 600
+DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024
+MAX_OUTPUT_BYTES = 5 * 1024 * 1024
+
class DeviceCommandRequest(BaseModel):
@@
timeout_seconds: int = Field(
- default=60,
+ default=DEFAULT_COMMAND_TIMEOUT_SECONDS,
gt=0,
- le=600,
+ le=MAX_COMMAND_TIMEOUT_SECONDS,
description="Command timeout in seconds",
)
max_output_bytes: int = Field(
- default=1024 * 1024,
+ default=DEFAULT_MAX_OUTPUT_BYTES,
gt=0,
- le=5 * 1024 * 1024,
+ le=MAX_OUTPUT_BYTES,
description="Maximum stdout and stderr bytes returned separately",
)As per coding guidelines, "Python: Extract magic numbers to constants".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| timeout_seconds: int = Field( | |
| default=60, | |
| gt=0, | |
| le=600, | |
| description="Command timeout in seconds", | |
| ) | |
| max_output_bytes: int = Field( | |
| default=1024 * 1024, | |
| gt=0, | |
| le=5 * 1024 * 1024, | |
| description="Maximum stdout and stderr bytes returned separately", | |
| DEFAULT_COMMAND_TIMEOUT_SECONDS = 60 | |
| MAX_COMMAND_TIMEOUT_SECONDS = 600 | |
| DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024 | |
| MAX_OUTPUT_BYTES = 5 * 1024 * 1024 | |
| class DeviceCommandRequest(BaseModel): | |
| timeout_seconds: int = Field( | |
| default=DEFAULT_COMMAND_TIMEOUT_SECONDS, | |
| gt=0, | |
| le=MAX_COMMAND_TIMEOUT_SECONDS, | |
| description="Command timeout in seconds", | |
| ) | |
| max_output_bytes: int = Field( | |
| default=DEFAULT_MAX_OUTPUT_BYTES, | |
| gt=0, | |
| le=MAX_OUTPUT_BYTES, | |
| description="Maximum stdout and stderr bytes returned separately", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/schemas/device.py` around lines 159 - 169, Extract the numeric
literals used in the Field definitions into module-level constants and reference
them from timeout_seconds and max_output_bytes: create constants (e.g.,
DEFAULT_TIMEOUT_SECONDS, MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS,
DEFAULT_MAX_OUTPUT_BYTES, MAX_OUTPUT_BYTES) at the top of the module and replace
the inline numbers in the timeout_seconds Field(gt, le, default) and
max_output_bytes Field(default, gt, le) with those named constants so policy
tuning is centralized and the semantics are clearer.
Comment on lines
+19
to
+23
| def apply_command_post_processor( | ||
| result: CommandResult, | ||
| processor_name: str | None, | ||
| ) -> CommandResult: | ||
| """Apply a registered post processor to a command result.""" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Trace the public post-processor entrypoint.
apply_command_post_processor is the cross-module sync boundary for response shaping, so it should be instrumented with @trace_sync to keep post-processor failures visible in traces.
Suggested fix
from collections.abc import Callable
from typing import Any
+from shared.telemetry.decorators import trace_sync
@@
+@trace_sync
def apply_command_post_processor(
result: CommandResult,
processor_name: str | None,
) -> CommandResult:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def apply_command_post_processor( | |
| result: CommandResult, | |
| processor_name: str | None, | |
| ) -> CommandResult: | |
| """Apply a registered post processor to a command result.""" | |
| `@trace_sync` | |
| def apply_command_post_processor( | |
| result: CommandResult, | |
| processor_name: str | None, | |
| ) -> CommandResult: | |
| """Apply a registered post processor to a command result.""" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device/command_post_processor.py` around lines 19 - 23,
The apply_command_post_processor function is the public sync entrypoint for
post-processors and should be instrumented with tracing; import trace_sync from
shared.telemetry.decorators and add the `@trace_sync` decorator above the
apply_command_post_processor definition so any failures in this cross-module
boundary are recorded in traces (keep the function body unchanged).
Comment on lines
+58
to
+61
| argv = shlex.split(command) | ||
| if not argv: | ||
| raise CommandRegistryError("Local device command resolved to an empty argv") | ||
| return [*argv, *(args or [])] |
Contributor
There was a problem hiding this comment.
Normalize shlex.split failures to CommandRegistryError.
Malformed command strings raise ValueError from shlex.split, which bypasses the module’s error contract and can break upstream error mapping.
Proposed fix
def build_local_device_command_argv(
command: str, args: list[str] | None = None
) -> list[str]:
"""Build an argv array from a configured command and request args."""
- argv = shlex.split(command)
+ try:
+ argv = shlex.split(command)
+ except ValueError as exc:
+ raise CommandRegistryError(
+ f"Local device command is invalid shell syntax: {command!r}"
+ ) from exc
if not argv:
raise CommandRegistryError("Local device command resolved to an empty argv")
return [*argv, *(args or [])]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/device/command_registry.py` around lines 58 - 61,
shlex.split can raise ValueError for malformed command strings; update the call
that assigns argv (currently "argv = shlex.split(command)") to catch ValueError
and re-raise it as a CommandRegistryError (include the original exception
message and use "from" to preserve traceback). Ensure the existing empty-argv
check and the return of [*argv, *(args or [])] remain unchanged; reference the
symbols argv, shlex.split, and CommandRegistryError when locating the change.
Comment on lines
+130
to
+149
| async def _terminate_process(self, process: asyncio.subprocess.Process) -> None: | ||
| if process.returncode is not None: | ||
| return | ||
|
|
||
| try: | ||
| if os.name != "nt": | ||
| os.killpg(process.pid, signal.SIGTERM) | ||
| else: | ||
| process.terminate() | ||
| await asyncio.wait_for(process.wait(), timeout=2) | ||
| except Exception: | ||
| logger.warning("[CommandHandler] Graceful termination failed; killing") | ||
| try: | ||
| if os.name != "nt": | ||
| os.killpg(process.pid, signal.SIGKILL) | ||
| else: | ||
| process.kill() | ||
| except ProcessLookupError: | ||
| return | ||
| await process.wait() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python asyncio on Windows, when a process is started with asyncio.create_subprocess_shell, do asyncio.subprocess.Process.terminate() / kill() stop only the direct child or the entire subprocess tree? What is the recommended way to terminate the full tree on timeout?
💡 Result:
Answer (Windows / asyncio): 1) asyncio.subprocess.Process.terminate/kill affect only the process object’s “child” PID (i.e., the process created by the asyncio subprocess API / shell wrapper), not an entire subprocess/process-tree. Python’s documentation explicitly defines Process.terminate/kill semantics on Windows: terminate calls the Win32 API TerminateProcess for the child process, and kill is an alias for terminate on Windows. That means they terminate that one process handle/PID, with no documented behavior to traverse and kill descendant processes (the subprocess tree) automatically. [1] Additionally, real-world examples show that killing only the parent/launcher PID may leave grandchildren running (e.g., when the “process” started by the shell/launcher spawns other processes and then exits). [2] 2) Recommended way to terminate the full tree on timeout: You must implement your own process-tree termination strategy on Windows. Common approaches: A) Start the subprocess without a shell if possible (avoid create_subprocess_shell) so you can kill the actual long-running executable rather than an intermediate shell/launcher. This is a practical mitigation: one Stack Overflow report notes proc.kill may kill the shell wrapper but not the spawned program when using shell-based commands, and recommends create_subprocess_exec to avoid the wrapper. [2] B) If you truly need “kill the whole tree”, identify child processes recursively and terminate/kill them yourself (e.g., using psutil). One answer describes using psutil.Process(pid).children(recursive=True) to collect descendants and then terminate them (and escalate to kill if needed). [3] C) Enforce the timeout with asyncio.timeout / wait_for, then on timeout call the tree-kill routine and await the process exit. Use asyncio.timeout (or asyncio.wait_for) because Process.wait/communicate do not themselves accept a timeout parameter; the documented pattern is to apply timeouts using asyncio functions/context managers rather than expecting Process.wait(timeout=...). [4][5] Minimal pattern (conceptual): - Run: proc = await asyncio.create_subprocess_shell(...) - Await with timeout: use async with asyncio.timeout(T): await proc.wait - On TimeoutError: do process-tree termination (your psutil-based recursive kill/terminate), then await proc.wait (or otherwise reap). Notes: - asyncio’s terminate/kill on Windows do not document tree termination; they terminate the single child process PID. Therefore the “full tree on timeout” requirement is not satisfied by calling Process.terminate/kill alone. [1] - If the OS/application creates intermediate launcher processes, killing only one PID can leave descendants; you need either to avoid the launcher (prefer create_subprocess_exec) or implement explicit recursive descendant cleanup. [2][3]
Citations:
- 1: https://docs.python.org/3.11/library/asyncio-subprocess.html
- 2: https://stackoverflow.com/questions/51404476/how-to-use-asyncio-to-continously-read-from-a-process-and-kill-it-after-a-timeou
- 3: https://stackoverflow.com/questions/70565429/how-to-kill-process-with-entire-process-tree-with-python-on-windows
- 4: https://github.com/python/cpython/blob/main/Doc/library/asyncio-subprocess.rst
- 5: https://docs.python.org/3/library/asyncio-task.html
Windows timeout cleanup is not process-tree safe.
On Windows, asyncio.subprocess.Process.terminate() / kill() only terminates the single subprocess PID/handle, not any child processes it spawned. This can leave descendant processes running after the RPC times out.
Update the Windows branch to use a process-tree termination approach (e.g., avoid shell/launcher wrappers when possible, and/or recursively terminate descendants via a tool such as psutil/job objects), then still reap the original process.
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 140-140: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/modes/local/command_handler.py` around lines 130 - 149, The Windows
branch of _terminate_process must terminate the entire process tree instead of
only the root PID; update the logic inside the else (os.name == "nt") branch to
enumerate and terminate descendant processes (e.g., use
psutil.Process(process.pid).children(recursive=True) to send terminate() then
kill() if needed), handle exceptions (AccessDenied/NoSuchProcess), then still
call await process.wait() to reap the original asyncio.subprocess.Process;
include a safe fallback to process.kill() if psutil isn't available or children
termination fails and ensure the outer graceful/force paths mirror the
non-Windows behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
executor/tests/test_local_command_handler.py (1)
11-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd return type hints to async test functions.
These public test functions should explicitly declare
-> Nonefor consistency with repository typing rules.Suggested pattern
-async def test_execute_command_times_out_and_returns_error(): +async def test_execute_command_times_out_and_returns_error() -> None:As per coding guidelines:
**/*.py: Python code requires type hints and descriptive names with docstrings for public functions/classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/test_local_command_handler.py` around lines 11 - 95, The async test functions (test_execute_command_uses_argv_and_cwd, test_execute_command_argv_does_not_invoke_shell, test_execute_command_returns_completed_process_result, test_execute_command_times_out_and_returns_error) are missing explicit return type hints; update each async def to declare "-> None" (e.g., async def test_execute_command_uses_argv_and_cwd(tmp_path) -> None:) to conform with repository typing rules and public-function style guidelines, keeping the existing names and docstrings unchanged.backend/tests/services/test_local_device_command_service.py (1)
13-365:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit type hints to test function signatures.
Public test functions currently omit return and parameter type hints (for example,
monkeypatchat Line 110 and Line 162). Please add-> None(or-> Awaitable[None]style via async-> None) and annotate fixtures likemonkeypatch: pytest.MonkeyPatch.Suggested pattern
-async def test_execute_command_calls_registered_device_socket(monkeypatch): +async def test_execute_command_calls_registered_device_socket( + monkeypatch: pytest.MonkeyPatch, +) -> None:-def test_local_device_command_registry_builds_git_clone_argv(): +def test_local_device_command_registry_builds_git_clone_argv() -> None:As per coding guidelines:
**/*.py: Python code requires type hints and descriptive names with docstrings for public functions/classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_local_device_command_service.py` around lines 13 - 365, Several public test functions are missing explicit type annotations for the return type and fixture parameters (notably the async tests using the monkeypatch fixture such as test_execute_command_calls_registered_device_socket and test_execute_configured_device_command_resolves_executes_and_post_processes). Update each public test function signature to include a return type of -> None (async functions remain async def ... -> None) and annotate fixtures like monkeypatch: pytest.MonkeyPatch; adjust any other untyped public test functions similarly (e.g., test_execute_command_rejects_offline_device, test_execute_configured_device_command_rejects_unowned_device, test_execute_device_command_endpoint_maps_request_to_service, test_execute_device_command_endpoint_applies_configured_post_processor, test_execute_device_command_endpoint_rejects_unknown_command_key) and add an import for pytest.MonkeyPatch if not already present.
♻️ Duplicate comments (8)
backend/app/services/device/command_post_processor.py (1)
19-23: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTrace the post-processor entrypoint.
Please instrument
apply_command_post_processorwith@trace_sync().Proposed fix
from collections.abc import Callable from typing import Any +from shared.telemetry.decorators import trace_sync @@ +@trace_sync() def apply_command_post_processor( result: CommandResult, processor_name: str | None, ) -> CommandResult:As per coding guidelines,
**/backend/**/*.py: Use OpenTelemetry tracing decorators@trace_async()or@trace_sync()fromshared/telemetry/decorators.pyto trace functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_post_processor.py` around lines 19 - 23, Instrument the apply_command_post_processor function with OpenTelemetry by adding the `@trace_sync`() decorator (from shared.telemetry.decorators) above its definition and import trace_sync at the top of the module; this will wrap the apply_command_post_processor(result: CommandResult, processor_name: str | None) -> CommandResult entrypoint with synchronous tracing so executions are captured by telemetry.backend/app/api/endpoints/devices.py (2)
416-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning raw backend exception text to clients.
Map to stable user-facing messages and keep full details in server logs.
Proposed fix
except DeviceCommandConfigurationError as exc: + logger.exception( + "[Device Command] Invalid command configuration: user_id=%s, device_id=%s", + user_id, + device_id, + ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=str(exc), - ) + detail="Device command configuration is invalid", + ) from exc except DeviceCommandError as exc: logger.warning( "[Device Command] Command RPC failed: user_id=%s, device_id=%s, error=%s", user_id, device_id, exc, ) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, - detail=str(exc), - ) + detail="Device command execution failed", + ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/devices.py` around lines 416 - 430, The handler currently returns raw exception text to clients (detail=str(exc)) for DeviceCommandError; change this to a stable, generic user-facing message (e.g., "Failed to execute device command") when raising the HTTPException, and keep the full exception details in the server logs by logging the exception with exc_info/stacktrace (use logger.exception or logger.warning(..., exc_info=True)). Update the raise for DeviceCommandError (and the corresponding logger.warning call) to log the full exc and return the generic message to the client; optionally factor mapping into a small helper like map_device_error_to_user_message used by the except block.
379-385: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTrace the new async endpoint.
Please add
@trace_async()onexecute_device_command.Proposed fix
from app.services.device_service import device_service +from shared.telemetry.decorators import trace_async @@ `@router.post`("/{device_id}/commands", response_model=DeviceCommandResponse) +@trace_async() async def execute_device_command(As per coding guidelines,
**/backend/**/*.py: Use OpenTelemetry tracing decorators@trace_async()or@trace_sync()fromshared/telemetry/decorators.pyto trace functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/endpoints/devices.py` around lines 379 - 385, Add OpenTelemetry tracing to the async endpoint by decorating execute_device_command with `@trace_async`(); import trace_async from shared.telemetry.decorators (ensure the import is added to the top of the module) and place the `@trace_async`() decorator immediately above the existing `@router.post`(...) decorator so the execute_device_command coroutine is traced.backend/app/services/device/command_registry.py (1)
58-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize malformed shell syntax errors to
CommandRegistryError.
shlex.split()exceptions currently bypass this module’s error boundary.Proposed fix
def build_local_device_command_argv( command: str, args: list[str] | None = None ) -> list[str]: """Build an argv array from a configured command and request args.""" - argv = shlex.split(command) + try: + argv = shlex.split(command) + except ValueError as exc: + raise CommandRegistryError( + f"Local device command is invalid shell syntax: {command!r}" + ) from exc if not argv: raise CommandRegistryError("Local device command resolved to an empty argv") return [*argv, *(args or [])]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_registry.py` around lines 58 - 61, shlex.split(command) can raise a ValueError on malformed shell syntax which currently escapes the module's error boundary; wrap the call to shlex.split in a try/except that catches the split error (e.g., ValueError or Exception) and re-raise a CommandRegistryError containing the original error message and context (mentioning the offending command), so the argv construction (the argv = shlex.split(command) -> if not argv -> return [*argv, *(args or [])] sequence) always surfaces a CommandRegistryError instead of leaking low-level exceptions.backend/app/services/device/command_service.py (3)
151-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce local-device-only validation before dispatch.
This path targets local executor RPC but currently only checks ownership/existence.
Proposed fix
device_kind = device_service.get_device_by_device_id(db, user_id, device_id) if not device_kind: raise DeviceCommandNotFoundError("Device not found or access denied") + device_type = device_kind.json.get("spec", {}).get("deviceType", "local") + if device_type != "local": + raise DeviceCommandNotFoundError("Device not found or access denied")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 151 - 153, The code currently only verifies existence/ownership via device_service.get_device_by_device_id but must also enforce that the target device is a local-only device before sending to the local executor; update the check in command_service.py (around where device_kind is retrieved) to validate the device's executor/type (for example device_kind.kind or device_kind.executor_type or an is_local property) and if it is not a local device raise DeviceCommandNotFoundError("Device not found or access denied") (or a more specific error) to block non-local devices from being dispatched to the local RPC executor.
93-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw command content in RPC dispatch logs.
Even truncated command text can expose sensitive arguments; keep logs to identifiers/limits only.
Proposed fix
logger.info( "[LocalDeviceCommandService] Sending command RPC: " - "user_id=%s, device_id=%s, socket_id=%s, timeout=%s, command=%s", + "user_id=%s, device_id=%s, socket_id=%s, timeout=%s", user_id, device_id, socket_id, normalized_timeout, - command[:200], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 93 - 101, The log in LocalDeviceCommandService currently includes raw command content (command[:200])—remove any command text from the logger.info call and only log non-sensitive identifiers and metadata; update the logger.info invocation that references user_id, device_id, socket_id, normalized_timeout, and command to omit command (you may instead log command length or a fixed redaction marker) so that no command arguments are written to logs.
51-63: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd tracing to the async service entrypoint.
execute_commandshould be decorated with@trace_async().Proposed fix
from app.services.device_service import device_service +from shared.telemetry.decorators import trace_async @@ class LocalDeviceCommandService: @@ + `@trace_async`() async def execute_command(As per coding guidelines,
**/backend/**/*.py: Use OpenTelemetry tracing decorators@trace_async()or@trace_sync()fromshared/telemetry/decorators.pyto trace functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/device/command_service.py` around lines 51 - 63, The execute_command coroutine lacks OpenTelemetry tracing; import trace_async from shared.telemetry.decorators and annotate the method definition for execute_command with `@trace_async`() (place the import near other imports and add the decorator directly above the async def execute_command(...) in class CommandService) so the async service entrypoint is instrumented.backend/app/schemas/device.py (1)
159-169: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract request limit literals into named constants.
timeout_secondsandmax_output_bytesstill use inline policy values. Please move these to module-level constants for centralized tuning.Proposed refactor
+DEFAULT_COMMAND_TIMEOUT_SECONDS = 60 +MAX_COMMAND_TIMEOUT_SECONDS = 600 +DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024 +MAX_OUTPUT_BYTES = 5 * 1024 * 1024 + class DeviceCommandRequest(BaseModel): @@ timeout_seconds: int = Field( - default=60, + default=DEFAULT_COMMAND_TIMEOUT_SECONDS, gt=0, - le=600, + le=MAX_COMMAND_TIMEOUT_SECONDS, description="Command timeout in seconds", ) max_output_bytes: int = Field( - default=1024 * 1024, + default=DEFAULT_MAX_OUTPUT_BYTES, gt=0, - le=5 * 1024 * 1024, + le=MAX_OUTPUT_BYTES, description="Maximum stdout and stderr bytes returned separately", )As per coding guidelines,
**/*.py: Extract magic numbers to constants in Python code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/schemas/device.py` around lines 159 - 169, Extract the inline numeric policy values used in the Device schema into module-level constants (e.g., DEFAULT_TIMEOUT_SECONDS, MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES, MIN_OUTPUT_BYTES, MAX_OUTPUT_BYTES) declared near the top of backend/app/schemas/device.py, then replace the literal numbers in the Field definitions for timeout_seconds and max_output_bytes (including default, gt, le bounds) with those constants so tuning is centralized and the Field uses the named constants instead of magic numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/services/test_local_device_command_service.py`:
- Around line 131-306: Tests repeat magic literals (user id, device ids,
timeout/output limits, socket id/namespace) across test_execute_command*,
test_execute_configured_device_command*, and
test_execute_device_command_endpoint*; extract these into module-level constants
(e.g. TEST_USER_ID = 7, TEST_DEVICE_ID = "device-abc", OFFLINE_DEVICE_ID =
"offline-device", TIMEOUT_SECONDS = 5, MAX_OUTPUT_BYTES = 1024, SOCKET_ID =
"socket-123", LOCAL_EXECUTOR_NS = "/local-executor", SOCKET_TIMEOUT = 10) and
replace inline literals in assertions, AsyncMock return calls, and calls to
local_device_command_service.execute_command and
devices.execute_configured_device_command with those constants to reduce
duplication and keep tests consistent.
In `@executor/tests/test_local_command_handler.py`:
- Around line 19-88: Tests repeat the same literal values for timeout_seconds
and max_output_bytes across multiple calls to handler.handle_execute_command;
extract them to module-level constants (e.g., DEFAULT_TIMEOUT_SECONDS and
DEFAULT_MAX_OUTPUT_BYTES) at the top of
executor/tests/test_local_command_handler.py and replace the repeated numeric
literals in each payload (in test_execute_command_*,
test_execute_command_argv_does_not_invoke_shell,
test_execute_command_returns_completed_process_result,
test_execute_command_times_out_and_returns_error) with those constants so
updates are centralized and magic numbers removed.
---
Outside diff comments:
In `@backend/tests/services/test_local_device_command_service.py`:
- Around line 13-365: Several public test functions are missing explicit type
annotations for the return type and fixture parameters (notably the async tests
using the monkeypatch fixture such as
test_execute_command_calls_registered_device_socket and
test_execute_configured_device_command_resolves_executes_and_post_processes).
Update each public test function signature to include a return type of -> None
(async functions remain async def ... -> None) and annotate fixtures like
monkeypatch: pytest.MonkeyPatch; adjust any other untyped public test functions
similarly (e.g., test_execute_command_rejects_offline_device,
test_execute_configured_device_command_rejects_unowned_device,
test_execute_device_command_endpoint_maps_request_to_service,
test_execute_device_command_endpoint_applies_configured_post_processor,
test_execute_device_command_endpoint_rejects_unknown_command_key) and add an
import for pytest.MonkeyPatch if not already present.
In `@executor/tests/test_local_command_handler.py`:
- Around line 11-95: The async test functions
(test_execute_command_uses_argv_and_cwd,
test_execute_command_argv_does_not_invoke_shell,
test_execute_command_returns_completed_process_result,
test_execute_command_times_out_and_returns_error) are missing explicit return
type hints; update each async def to declare "-> None" (e.g., async def
test_execute_command_uses_argv_and_cwd(tmp_path) -> None:) to conform with
repository typing rules and public-function style guidelines, keeping the
existing names and docstrings unchanged.
---
Duplicate comments:
In `@backend/app/api/endpoints/devices.py`:
- Around line 416-430: The handler currently returns raw exception text to
clients (detail=str(exc)) for DeviceCommandError; change this to a stable,
generic user-facing message (e.g., "Failed to execute device command") when
raising the HTTPException, and keep the full exception details in the server
logs by logging the exception with exc_info/stacktrace (use logger.exception or
logger.warning(..., exc_info=True)). Update the raise for DeviceCommandError
(and the corresponding logger.warning call) to log the full exc and return the
generic message to the client; optionally factor mapping into a small helper
like map_device_error_to_user_message used by the except block.
- Around line 379-385: Add OpenTelemetry tracing to the async endpoint by
decorating execute_device_command with `@trace_async`(); import trace_async from
shared.telemetry.decorators (ensure the import is added to the top of the
module) and place the `@trace_async`() decorator immediately above the existing
`@router.post`(...) decorator so the execute_device_command coroutine is traced.
In `@backend/app/schemas/device.py`:
- Around line 159-169: Extract the inline numeric policy values used in the
Device schema into module-level constants (e.g., DEFAULT_TIMEOUT_SECONDS,
MIN_TIMEOUT_SECONDS, MAX_TIMEOUT_SECONDS, DEFAULT_MAX_OUTPUT_BYTES,
MIN_OUTPUT_BYTES, MAX_OUTPUT_BYTES) declared near the top of
backend/app/schemas/device.py, then replace the literal numbers in the Field
definitions for timeout_seconds and max_output_bytes (including default, gt, le
bounds) with those constants so tuning is centralized and the Field uses the
named constants instead of magic numbers.
In `@backend/app/services/device/command_post_processor.py`:
- Around line 19-23: Instrument the apply_command_post_processor function with
OpenTelemetry by adding the `@trace_sync`() decorator (from
shared.telemetry.decorators) above its definition and import trace_sync at the
top of the module; this will wrap the apply_command_post_processor(result:
CommandResult, processor_name: str | None) -> CommandResult entrypoint with
synchronous tracing so executions are captured by telemetry.
In `@backend/app/services/device/command_registry.py`:
- Around line 58-61: shlex.split(command) can raise a ValueError on malformed
shell syntax which currently escapes the module's error boundary; wrap the call
to shlex.split in a try/except that catches the split error (e.g., ValueError or
Exception) and re-raise a CommandRegistryError containing the original error
message and context (mentioning the offending command), so the argv construction
(the argv = shlex.split(command) -> if not argv -> return [*argv, *(args or [])]
sequence) always surfaces a CommandRegistryError instead of leaking low-level
exceptions.
In `@backend/app/services/device/command_service.py`:
- Around line 151-153: The code currently only verifies existence/ownership via
device_service.get_device_by_device_id but must also enforce that the target
device is a local-only device before sending to the local executor; update the
check in command_service.py (around where device_kind is retrieved) to validate
the device's executor/type (for example device_kind.kind or
device_kind.executor_type or an is_local property) and if it is not a local
device raise DeviceCommandNotFoundError("Device not found or access denied") (or
a more specific error) to block non-local devices from being dispatched to the
local RPC executor.
- Around line 93-101: The log in LocalDeviceCommandService currently includes
raw command content (command[:200])—remove any command text from the logger.info
call and only log non-sensitive identifiers and metadata; update the logger.info
invocation that references user_id, device_id, socket_id, normalized_timeout,
and command to omit command (you may instead log command length or a fixed
redaction marker) so that no command arguments are written to logs.
- Around line 51-63: The execute_command coroutine lacks OpenTelemetry tracing;
import trace_async from shared.telemetry.decorators and annotate the method
definition for execute_command with `@trace_async`() (place the import near other
imports and add the decorator directly above the async def execute_command(...)
in class CommandService) so the async service entrypoint is instrumented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a138700-f967-42d2-be6a-8769a5ad0535
📒 Files selected for processing (16)
AGENTS.mdREADME.mdREADME_zh.mdbackend/app/api/endpoints/devices.pybackend/app/core/config.pybackend/app/schemas/device.pybackend/app/services/device/command_post_processor.pybackend/app/services/device/command_registry.pybackend/app/services/device/command_service.pybackend/tests/services/test_local_device_command_service.pydocs/en/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/local-device-command-rpc.mdexecutor/modes/local/command_handler.pyexecutor/modes/local/events.pyexecutor/modes/local/runner.pyexecutor/tests/test_local_command_handler.py
✅ Files skipped from review due to trivial changes (5)
- README_zh.md
- executor/modes/local/events.py
- README.md
- docs/zh/developer-guide/local-device-command-rpc.md
- AGENTS.md
Comment on lines
+131
to
+306
| result = await command_service.local_device_command_service.execute_command( | ||
| user_id=7, | ||
| device_id="device-abc", | ||
| command="pwd", | ||
| path="/tmp", | ||
| args=["-P"], | ||
| env={"A": "B"}, | ||
| timeout_seconds=5, | ||
| max_output_bytes=1024, | ||
| ) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["stdout"] == "ok\n" | ||
| mock_sio.call.assert_awaited_once_with( | ||
| "device:execute_command", | ||
| { | ||
| "command": "pwd", | ||
| "cwd": "/tmp", | ||
| "args": ["-P"], | ||
| "argv": ["pwd", "-P"], | ||
| "env": {"A": "B"}, | ||
| "timeout_seconds": 5, | ||
| "max_output_bytes": 1024, | ||
| }, | ||
| to="socket-123", | ||
| namespace="/local-executor", | ||
| timeout=10, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_command_rejects_offline_device(monkeypatch): | ||
| """Service should reject devices without online socket information.""" | ||
| from app.services.device import command_service | ||
|
|
||
| monkeypatch.setattr( | ||
| command_service.device_service, | ||
| "get_device_online_info", | ||
| AsyncMock(return_value=None), | ||
| ) | ||
|
|
||
| with pytest.raises(command_service.DeviceCommandError) as exc_info: | ||
| await command_service.local_device_command_service.execute_command( | ||
| user_id=7, | ||
| device_id="offline-device", | ||
| command="pwd", | ||
| ) | ||
|
|
||
| assert "offline" in str(exc_info.value) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_configured_device_command_resolves_executes_and_post_processes( | ||
| monkeypatch, | ||
| ): | ||
| """Internal service API should resolve key, execute command, and post-process.""" | ||
| from app.services.device import command_service | ||
|
|
||
| execute_mock = AsyncMock( | ||
| return_value={ | ||
| "success": True, | ||
| "exit_code": 0, | ||
| "stdout": ".\n..\nbackend\n", | ||
| "stderr": "", | ||
| "duration": 0.02, | ||
| "timed_out": False, | ||
| } | ||
| ) | ||
| monkeypatch.setattr( | ||
| command_service.device_service, | ||
| "get_device_by_device_id", | ||
| lambda db, user_id, device_id: object(), | ||
| ) | ||
| monkeypatch.setattr( | ||
| command_service.local_device_command_service, | ||
| "execute_command", | ||
| execute_mock, | ||
| ) | ||
|
|
||
| result = await command_service.execute_configured_device_command( | ||
| db=object(), | ||
| user_id=7, | ||
| device_id="device-abc", | ||
| command_key="repo_files", | ||
| path="/tmp", | ||
| args=["backend"], | ||
| env={"A": "B"}, | ||
| timeout_seconds=5, | ||
| max_output_bytes=1024, | ||
| command_config={ | ||
| "repo_files": { | ||
| "command": "ls -a", | ||
| "post_processor": "file_list", | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| assert result["stdout"] == ["backend"] | ||
| execute_mock.assert_awaited_once_with( | ||
| user_id=7, | ||
| device_id="device-abc", | ||
| command="ls -a", | ||
| path="/tmp", | ||
| args=["backend"], | ||
| env={"A": "B"}, | ||
| timeout_seconds=5, | ||
| max_output_bytes=1024, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_configured_device_command_rejects_unowned_device(monkeypatch): | ||
| """Internal service API should reject devices the user does not own.""" | ||
| from app.services.device import command_service | ||
|
|
||
| monkeypatch.setattr( | ||
| command_service.device_service, | ||
| "get_device_by_device_id", | ||
| lambda db, user_id, device_id: None, | ||
| ) | ||
|
|
||
| with pytest.raises(command_service.DeviceCommandNotFoundError): | ||
| await command_service.execute_configured_device_command( | ||
| db=object(), | ||
| user_id=7, | ||
| device_id="device-abc", | ||
| command_key="pwd", | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_device_command_endpoint_maps_request_to_service(monkeypatch): | ||
| """Endpoint should delegate HTTP request data to the internal service API.""" | ||
| from app.api.endpoints import devices | ||
| from app.schemas.device import DeviceCommandRequest | ||
|
|
||
| service_mock = AsyncMock( | ||
| return_value={ | ||
| "success": True, | ||
| "exit_code": 0, | ||
| "stdout": "ok", | ||
| "stderr": "", | ||
| "duration": 0.02, | ||
| "timed_out": False, | ||
| } | ||
| ) | ||
| monkeypatch.setattr(devices, "execute_configured_device_command", service_mock) | ||
| db = object() | ||
|
|
||
| response = await devices.execute_device_command( | ||
| device_id="device-abc", | ||
| request=DeviceCommandRequest( | ||
| command_key="repo_status", | ||
| path="/tmp", | ||
| args=["--short"], | ||
| env={"A": "B"}, | ||
| timeout_seconds=5, | ||
| max_output_bytes=1024, | ||
| ), | ||
| db=db, | ||
| current_user=SimpleNamespace(id=7), | ||
| ) | ||
|
|
||
| assert response.success is True | ||
| assert response.stdout == "ok" | ||
| service_mock.assert_awaited_once_with( | ||
| db=db, | ||
| user_id=7, | ||
| device_id="device-abc", | ||
| command_key="repo_status", | ||
| path="/tmp", | ||
| args=["--short"], | ||
| env={"A": "B"}, | ||
| timeout_seconds=5, | ||
| max_output_bytes=1024, | ||
| ) |
Contributor
There was a problem hiding this comment.
Replace repeated test literals with module-level constants.
The same values are repeated in multiple places (for example, timeout/output limits and IDs around Line 138–139, Line 218–219, and Line 287–288). Extracting them to constants will improve readability and reduce drift in test maintenance.
Suggested pattern
+TEST_USER_ID = 7
+TEST_DEVICE_ID = "device-abc"
+TEST_TIMEOUT_SECONDS = 5
+TEST_MAX_OUTPUT_BYTES = 1024
+TEST_CWD = "/tmp"
...
- user_id=7,
- device_id="device-abc",
+ user_id=TEST_USER_ID,
+ device_id=TEST_DEVICE_ID,
...
- timeout_seconds=5,
- max_output_bytes=1024,
+ timeout_seconds=TEST_TIMEOUT_SECONDS,
+ max_output_bytes=TEST_MAX_OUTPUT_BYTES,As per coding guidelines: **/*.py: Extract magic numbers to constants in Python code.
🧰 Tools
🪛 Ruff (0.15.13)
[error] 135-135: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 148-148: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 215-215: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 233-233: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 284-284: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
[error] 301-301: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/services/test_local_device_command_service.py` around lines 131
- 306, Tests repeat magic literals (user id, device ids, timeout/output limits,
socket id/namespace) across test_execute_command*,
test_execute_configured_device_command*, and
test_execute_device_command_endpoint*; extract these into module-level constants
(e.g. TEST_USER_ID = 7, TEST_DEVICE_ID = "device-abc", OFFLINE_DEVICE_ID =
"offline-device", TIMEOUT_SECONDS = 5, MAX_OUTPUT_BYTES = 1024, SOCKET_ID =
"socket-123", LOCAL_EXECUTOR_NS = "/local-executor", SOCKET_TIMEOUT = 10) and
replace inline literals in assertions, AsyncMock return calls, and calls to
local_device_command_service.execute_command and
devices.execute_configured_device_command with those constants to reduce
duplication and keep tests consistent.
Comment on lines
+19
to
+88
| result = await handler.handle_execute_command( | ||
| { | ||
| "command": "cat", | ||
| "argv": ["cat", "target.txt"], | ||
| "cwd": str(tmp_path), | ||
| "timeout_seconds": 5, | ||
| "max_output_bytes": 1024, | ||
| } | ||
| ) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["stdout"] == "ok" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_command_argv_does_not_invoke_shell(): | ||
| """Command argv args should not be interpreted by a shell.""" | ||
| from executor.modes.local.command_handler import CommandHandler | ||
|
|
||
| handler = CommandHandler() | ||
|
|
||
| result = await handler.handle_execute_command( | ||
| { | ||
| "command": "printf %s", | ||
| "argv": ["printf", "%s", "hello; echo hacked"], | ||
| "timeout_seconds": 5, | ||
| "max_output_bytes": 1024, | ||
| } | ||
| ) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["stdout"] == "hello; echo hacked" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_command_returns_completed_process_result(): | ||
| """Command handler should return stdout, stderr, exit code, and duration.""" | ||
| from executor.modes.local.command_handler import CommandHandler | ||
|
|
||
| handler = CommandHandler() | ||
|
|
||
| result = await handler.handle_execute_command( | ||
| { | ||
| "command": "printf 'hello'", | ||
| "timeout_seconds": 5, | ||
| "max_output_bytes": 1024, | ||
| } | ||
| ) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["exit_code"] == 0 | ||
| assert result["stdout"] == "hello" | ||
| assert result["stderr"] == "" | ||
| assert result["duration"] >= 0 | ||
| assert result["timed_out"] is False | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_execute_command_times_out_and_returns_error(): | ||
| """Command handler should kill long commands and return a timeout result.""" | ||
| from executor.modes.local.command_handler import CommandHandler | ||
|
|
||
| handler = CommandHandler() | ||
|
|
||
| result = await handler.handle_execute_command( | ||
| { | ||
| "command": "sleep 2", | ||
| "timeout_seconds": 0.1, | ||
| "max_output_bytes": 1024, | ||
| } |
Contributor
There was a problem hiding this comment.
Extract repeated timeout/output values into constants.
timeout_seconds and max_output_bytes are repeated across all payloads (e.g., Line 24–25, Line 44–45, Line 63–64, Line 86–87). Using constants keeps tests easier to update.
Suggested pattern
+DEFAULT_TIMEOUT_SECONDS = 5
+DEFAULT_MAX_OUTPUT_BYTES = 1024
...
- "timeout_seconds": 5,
- "max_output_bytes": 1024,
+ "timeout_seconds": DEFAULT_TIMEOUT_SECONDS,
+ "max_output_bytes": DEFAULT_MAX_OUTPUT_BYTES,As per coding guidelines: **/*.py: Extract magic numbers to constants in Python code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/tests/test_local_command_handler.py` around lines 19 - 88, Tests
repeat the same literal values for timeout_seconds and max_output_bytes across
multiple calls to handler.handle_execute_command; extract them to module-level
constants (e.g., DEFAULT_TIMEOUT_SECONDS and DEFAULT_MAX_OUTPUT_BYTES) at the
top of executor/tests/test_local_command_handler.py and replace the repeated
numeric literals in each payload (in test_execute_command_*,
test_execute_command_argv_does_not_invoke_shell,
test_execute_command_returns_completed_process_result,
test_execute_command_times_out_and_returns_error) with those constants so
updates are centralized and magic numbers removed.
qdaxb
approved these changes
May 22, 2026
This was referenced May 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores