fix: honor SMS notification storage

[Justinabox]

Summary

• add a safe AT+CPMS command builder and apply ModemConfig.sms_storage during SMS initialization
• preserve +CMTI storage/index on the internal raw SMS event and use reported storage before reading/deleting incoming SMS slots
• use +CDSI reported storage before reading/deleting delivery-report slots, with state handling for CPMS failures and default-storage restoration

Tests

• git diff --check
• PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_sms_service.py tests/test_delivery_reports.py tests/test_config.py tests/test_commands.py tests/test_urc.py tests/test_modem.py -q
• PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

Risks

• touches SMS storage command sequencing only; no real hardware commands beyond mocked AT+CPMS/CMGR/CMGD tests
• SMSService tracks selected storage and restores configured default for no-storage public APIs after non-default slot handling

Closes #14

[Justinabox]

Review/merge-manager classification: needs-fix.

I inspected the diff and ran git diff --check origin/main...origin/agent/sms-storage-selection-20260628 (passed), but I am not landing this yet because the current implementation appears to leave a modem-safety/data-loss race around multi-command storage operations.

Blocking findings:

1. AT+CPMS selection plus the dependent CMGR/CMGD/CMGL/bulk delete command is not protected by an SMS-level lock. Concurrent +CMTI / +CDSI handlers or public SMS APIs can interleave between “select storage” and “read/delete slot”, causing the later command to run against a different store than the URC reported.
2. Non-default +CMTI/+CDSI operations can leave the modem CPMS state on the non-default store. No-storage operations restore later in some paths, but the storage change is still externally visible and can affect receive/write behavior before restoration.

Requested changes before this can be safe-to-land:

• Add a lock/scope that covers storage selection and the dependent read/delete/list operation as one logical SMS operation.
• Restore the configured default storage after non-default notification handling, or otherwise make the state transition explicitly scoped and tested.
• Add regression tests proving interleaved notification/public operations cannot switch storage between select and read/delete, and that no-storage APIs use the configured default after a prior non-default notification.

No GitHub CI checks are configured; absence of CI is not treated as a pass.

[Justinabox]

Release-manager review classification: needs-fix before this can land.

The functional pieces look directionally right, and local gates passed in review, but the storage-selection critical section is not atomic enough for mixed inbound/storage activity:

• _read_raw_message() selects storage and then separately executes AT+CMGR.
• delete_message() selects storage and then separately executes AT+CMGD.
• ATCommandExecutor serializes individual AT commands only, while EventBus async subscribers/public SMS APIs can interleave between those calls.

A race such as select ME → concurrent task select SM → original task CMGR/CMGD would read/delete from the wrong storage despite the +CMTI/+CDSI URC reporting ME.

Requested fix:

1. Add a service-level lock (or equivalent) around each storage-select + CMGR/CMGD sequence so the selected modem storage cannot be changed by another SMS-service operation before the read/delete command.
2. Add a focused async regression test that fails before the fix by interleaving two storage operations (for example CMTI/CDSI or CMTI plus a public no-storage API), proving the first operation still reads/deletes from the URC-reported storage.
3. Rerun:
   • git diff --check
   • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

Holding as needs-fix; not merging until the select/read/delete critical section is covered.

[Justinabox]

Scheduled review/merge lane classification: needs-fix.

The storage-selection direction is good and the focused/full suites passed in a read-only review worktree, but I found a merge blocker around async atomicity:

• ATCommandExecutor serializes individual AT commands only.
• The PR currently awaits AT+CPMS=... selection and then later awaits AT+CMGR / AT+CMGD / AT+CMGL without a service-level lock covering the whole select+operation sequence.
• That leaves an interleaving window where another SMS coroutine or public API call can switch selected storage between selection and the dependent read/delete/list operation, causing a read/delete against the wrong modem store.
• A deterministic review harness reproduced the shape as AT+CPMS="ME","ME","ME" followed by a default AT+CMGR=2 before the intended non-default AT+CMGR=1, demonstrating the selected-storage operation is not atomic.

Requested fix:

1. Add a service-level asyncio.Lock (or equivalent) covering storage selection plus the immediately dependent command for read_message / raw read, delete_message, list_messages, delete_all, and CMTI/CDSI read/delete flows.
2. Add concurrency regression tests proving no interleaving between CPMS and CMGR/CMGD for non-default notification/report handling and concurrent default APIs.
3. Preserve the existing invariants: failed CPMS must not poison _selected_sms_storage, no-storage APIs restore the configured default, and logs remain privacy-safe.
4. Rerun:

git diff --check
PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

[Justinabox]

Review/merge-manager classification: needs-fix.

I am not marking this ready or merging yet. The branch is currently mergeable, but review found storage-selection safety gaps that need a fresh TDD fix before landing:

• Make notification storage selection + read/delete atomic at the SMS-service level. _read_raw_message() / delete_message() select storage with AT+CPMS and then issue AT+CMGR / AT+CMGD as separate executor operations, so another SMS/API operation can interleave between select and read/delete and reintroduce wrong-slot read/delete hazards.
• Restore the configured/default SMS storage after handling a notification from a non-default store, or otherwise prevent one ME/alternate-store +CMTI / +CDSI from drifting the modem's global CPMS state for later no-storage operations.
• Add regression tests that fail before the fix for the interleaving/drift cases, then rerun git diff --check and the full required pytest gate.

No hardware testing is required; fake transports are sufficient.

[Justinabox]

Release review: needs-fix.

This draft is currently DIRTY against main, so it is not mergeable as-is. Please rebase or repackage it on current main, resolve conflicts without broadening scope, rerun git diff --check plus the full required Python 3.11 test suite, push the refreshed head, and request a new release review. No behavior was approved or merged in this pass.

[Justinabox]

Release-captain classification: needs-fix. This PR is now DIRTY against the current default branch after later safe merges. Please rebase/repackage it from current origin/main, resolve only the genuine overlap, then rerun git diff --check and the required no-hardware full pytest gate on the new head. Do not attempt to merge the stale head directly.

[Justinabox]

Release lane update: needs-fix.

This branch was already stale/conflicting against the default branch before the safe #236 landing, and main has now advanced again. Please rebase or repackage it from current main in an isolated worktree, resolve conflicts without broadening scope, run git diff --check plus the required full pytest gate, push the refreshed head, and request a new review. Do not treat earlier green evidence as valid for the stale head.

[Justinabox]

Release review: needs-fix.

This PR is now DIRTY against the default branch after recent safe merges. Do not merge the stale head directly. Refresh/rebase it in an isolated worktree, resolve the concrete overlap against current main, run git diff --check plus the required full pytest gate on the refreshed head, then return it for review.

[Justinabox]

Release review: needs-fix. GitHub currently reports this branch DIRTY against current main, so it cannot be safely evaluated or landed as-is. Rebase/repackage it from current main, resolve only the intended scope, inspect the resulting diff for overlap with work already shipped, and run git diff --check plus PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q. Request a fresh review on the new head; do not force a merge of the stale head.

[Justinabox]

Release review classification: needs-fix. GitHub now reports this branch DIRTY against the advanced default branch, so it is not mergeable. Please create an isolated rebase/repackage branch from current origin/main, resolve and review the overlap, run git diff --check plus the required full test suite on the rebased head, push it, and request a fresh release review. Do not attempt to merge this stale head directly.

[Justinabox]

Release-lane classification: needs fix/revalidation before merge.

This draft was not independently reviewed or integration-tested in this pass and its GitHub mergeability is stale/unknown after current-main advances. Please rebase or otherwise validate it against current main, run the required full local gate, and request a fresh release review with exact head SHA and gate evidence. Do not treat absence of hosted checks as approval.

[Justinabox]

Release classification: needs-fix for this release pass.

The draft has not received a current exact-head review and prescribed local integration gate over the advancing main baseline. Rebase/repackage as needed against current main, resolve any conflicts, then provide a narrow behavioral review plus passing git diff --check and the prescribed full test suite before it can be reconsidered for landing.

[Justinabox]

Release review: needs-fix. GitHub reports this storage-selection PR DIRTY. Rebase/repackage against current CMTI/CDSI handling, then rerun storage-selection and required full-suite gates before re-review.