Implementation Plan: P3 Fleet Crash-Recovery and Resume Test Coverage#1563
Conversation
… (P3) Add 15 test methods across 3 files covering WP1-WP9: - test_state.py: TOCTOU sidecar OSError→INTERRUPTED, _write_pid exception swallow, illegal state transitions, REFUSED/RELEASED resume skip, corrupt state no-op, mixed success/failure captures - test_fleet_semaphore.py: constructor guard (max_concurrent≤0), CancelledError propagates and releases semaphore lock - test_sidecar.py: read_sidecar_from_path with missing dir and valid JSONL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: blocking issues found. See inline comments — changes are required before merging.
1 critical finding (NameError: bogus undefined in TestWritePidExceptionSwallow.test_nonexistent_state_logs_warning), 1 warning (cohesion: test placement), 1 warning (arch: private import in test).
Adds _write_pid to fleet/__init__.py so tests can import from the public package path (autoskillit.fleet) instead of the private sub-module (autoskillit.fleet._api). Consolidates the import in test_state.py.
…i.py TestWritePidExceptionSwallow tests _write_pid from fleet/_api.py. Moving it from test_state.py to a dedicated test_api.py keeps coverage for _api.py co-located with its source module.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: warning-only findings detected. See inline comments — no blocking changes required.
Verdict: approved_with_comments
4 findings posted as inline comments (all warnings, no blocking criticals):
src/autoskillit/fleet/__init__.py— private symbol_write_pidin__all__src/autoskillit/fleet/_api.py— latent circular import via package gatewaytests/fleet/test_fleet_semaphore.py—TestExecuteDispatchCancelledErrorLockReleasecohesion placement (requires human decision)tests/fleet/test_fleet_semaphore.py— CancelledError lock release assertion incompleteness
1 info finding (no inline comment): tests/fleet/test_sidecar.py — missing malformed JSONL edge case in TestReadSidecarFromPath.
…t_api.py with strengthened lock assertion Moved TestExecuteDispatchCancelledErrorLockRelease from test_fleet_semaphore.py to test_api.py for cohesion (it tests execute_dispatch, not FleetSemaphore). Added side-effect counter inside _raise_cancelled to prove the semaphore was actually held when cancellation fired, distinguishing lock-acquired-then-released from lock-never-acquired. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Add ~15 test methods across 3 existing test files (
test_state.py,test_fleet_semaphore.py,test_sidecar.py) covering fleet crash-recovery paths, lock correctness, state transition guards, resume algorithm edge cases, and sidecar I/O. Pure test additions — no source files modified.Closes #1541
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260430-081120-253210/.autoskillit/temp/make-plan/p3_fleet_crash_recovery_resume_test_coverage_plan_2026-04-30_081500.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary