@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

Claude finished @axisrow's task in 3m 35s —— View job

PR Review

• Read changed files and gather context
• Analyze smoke_matrix.py for logical errors
• Analyze sandbox_write_live.py for security/data loss risks
• Analyze shell scripts for injection vulnerabilities
• Analyze tests for correctness
• Post review findings

Overall the PR is well-structured and the safety classification model is sound. No production code paths are touched. The critical issues I found are all in the new tooling/test code.

Bug: advideos.add leaks sandbox resources on every run

create_advideo() creates an ad video but never registers cleanup — unlike every other create_* helper. run_advideo_add() also skips cleanup registration. Every invocation of the sandbox write runner leaves an orphaned video object in the sandbox account.

scripts/sandbox_write_live.py:730–737

def create_advideo(self) -> str:
    video_path = self.root_dir / "tests" / "fixtures" / "test-video.mp4"
    run = self.invoke("advideos", "add", [...])
    return self.require_id("advideos add", run, ("Id", "Ids"))
    # ← no self.register_cleanup(...)

Compare with create_adimage() at line 718–728 which correctly calls self.register_cleanup(...). If the Yandex sandbox exposes a delete endpoint for ad videos, this should follow the same pattern. Fix this →

Logical issue: run_retargeting_add omits --type that create_retargeting includes

create_retargeting() (line 510) passes --type RETARGETING. run_retargeting_add() (line 934) does not. If --type is a required parameter without a default, the retargeting.add smoke test will always produce a CLI argument error (misclassified as SANDBOX_LIMITATION or FAIL) rather than testing the actual API call.

scripts/sandbox_write_live.py:934–945

def run_retargeting_add(self, matrix_command: str) -> ReportRow:
    run = self.invoke(
        "retargeting", "add",
        ["--name", self.name("retargeting"), "--rule", "ANY:1234567890"],
        # ← missing "--type", "RETARGETING" that create_retargeting uses
    )

Test isolation: _load_sandbox_runner_module() mutates global sys.modules

tests/test_smoke_matrix.py:33

sys.modules[spec.name] = module

Inserting a dynamically loaded module into sys.modules under the key "sandbox_write_live" means any subsequent import sandbox_write_live in the same test process will get this cached copy. This can cause subtle ordering-dependent failures if pytest re-uses the process. The importlib spec already manages the module object — the sys.modules assignment is unnecessary for the loading itself. Fix this →

Minor: command_category() rebuilds the full entry list on every call

command_category() in smoke_matrix.py:174 calls command_entries() which re-iterates the entire SMOKE_MATRIX dict and constructs a new list[SmokeCommand] each time. For 122 entries this is negligible, but validate_matrix() at line 242 also calls seen.count(command) in a comprehension, making duplicate detection O(n²). Neither is a production problem, but both could trivially be O(1) with a lookup dict. Not worth blocking the PR over, but worth knowing.

No issues found in

• Shell scripts: test_sandbox_write.sh correctly uses source "$ENV_FILE" (safer than the existing xargs pattern in test_safe_commands.sh, which is pre-existing). No injection risk for new code.
• classify() logic: correctly handles returncode, JSON errors, and sandbox-specific code matching.
• PNG generation in make_png(): IHDR/IDAT/IEND structure is correct.
• validate_matrix(): correctly detects missing, stale, and duplicate entries.
• Cleanup ordering: cleanup_best_effort() correctly reverses creation order.
• No token or credential exposure in verbose mode (credentials pass via env vars, not CLI args).

Triage of Claude Review Findings

1. advideos.add leaks sandbox resources — SKIP

The advideos CLI module (direct_cli/commands/advideos.py) only exposes get and add subcommands — there is no delete command. Without a delete endpoint, cleanup cannot be registered. This is a known CLI limitation, not a bug in the sandbox runner. The reviewer's own comparison with create_adimage() is apt — adimages has a delete command, advideos does not.

2. run_retargeting_add omits --type — HALLUCINATION

The reviewer claimed: "If --type is a required parameter without a default, the smoke test will always produce a CLI argument error."

This is factually wrong. The --type option in retargeting.py (line 72-75) has default="RETARGETING". It is not required and has a sensible default. run_retargeting_add() works correctly without specifying it.

3. sys.modules mutation in _load_sandbox_runner_module() — ALREADY CORRECT

The reviewer suggested removing the sys.modules[spec.name] = module assignment. This would break the loading: sandbox_write_live.py performs from direct_cli.cli import cli and other absolute imports. The importlib documentation and Python docs explicitly recommend inserting into sys.modules before calling exec_module() when the module has its own imports. Removing it would cause ImportError.

4. O(n^2) in validate_matrix() — SKIP (cosmetic)

The reviewer acknowledged this themselves: "Not worth blocking the PR over." With 122 entries this is negligible. Not a production issue.

Verdict: No FIX items found. All findings are either incorrect claims, intentional design decisions, or cosmetic nits. Proceeding to CI check.