@claude code review

Claude finished @RafaelPo's task in 1m 17s —— View job

Code Review

• Read repo guidelines (CLAUDE.md)
• Read all changed files and PR diff
• Analyze changes and provide review feedback

Overall: Looks good. Clean, well-tested changes that follow existing patterns consistently. Two minor observations below, neither blocking.

Artifact ID threading

The artifact_id is threaded correctly through all paths:

• TaskState.artifact_id computed field (tool_helpers.py:379)
• progress_message on completion (tool_helpers.py:455)
• _fetch_task_result return tuple (tool_helpers.py:592-595)
• everyrow_results_stdio (tools.py:847, 868)
• everyrow_results_http → try_store_result (tools.py:959)
• try_cached_result reads from Redis meta (result_store.py:274)
• _build_result_response adds to widget JSON + text summary (result_store.py:152-153, 188-189)

All paths handle the empty-string case correctly (no artifact_id key in widget, no text line). Test coverage is thorough for both present and absent cases.

No-auth mode

• CORS allow_origins=["*"] is gated behind cors=no_auth (http_config.py:110), which itself requires ALLOW_NO_AUTH=1 and defaults to 127.0.0.1 binding. Appropriate guards.
• Correctly does not set allow_credentials=True with the wildcard origin.
• Task ownership skip (tool_helpers.py:238, tools.py:80) is consistent — both _record_task_ownership and _check_task_ownership are bypassed.

Minor observations

1. Duplicated Unset guard — The pattern if aid is not None and not isinstance(aid, Unset): str(aid) appears identically in TaskState.artifact_id (line 381) and _fetch_task_result (line 594). If this pattern spreads further, a small _optional_str(value) helper could reduce repetition. Not blocking — the error field uses the same pattern at line 389 and this is a generated-client idiom.

2. Settings singleton mutation — settings.no_auth = input_args.no_auth (server.py:111) mutates the @lru_cache-backed singleton at startup. This follows the existing convention for settings.transport (line 80), so it's consistent, but it does mean the Settings model isn't truly immutable. Not a concern for this PR since it only runs once at startup before any concurrent access.

Tests

New test file test_tool_helpers.py provides solid coverage:

• TaskState.artifact_id with UUID, UNSET, and None
• Progress message includes artifact_id only on completion (not running/failed)
• HTTP mode variant also works

test_result_store.py additions cover:

• Widget JSON and text summary with/without artifact_id
• Cached result preserves artifact_id from Redis meta
• Store result persists artifact_id in meta and surfaces it in response

No issues found. Approving.