From 6f55ea70e1238e236735c9f9c0cdd9cfd6cfc135 Mon Sep 17 00:00:00 2001 From: Amantux Date: Mon, 20 Jul 2026 18:23:37 +0000 Subject: [PATCH 1/2] Address review: guard unload destination + fix stale docstrings + tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - routes_aml_move_medium.py: the unload branch now rejects a non-slot destination (422). Slot and drive addresses overlap, so a moveClass=8 request with a DRIVE destination would otherwise silently misroute the cartridge (reviewer-proven). - Refresh the two now-stale docstrings (module docstring in scalar_coordinate.py and the moveMedium route header) that still called moveClass values UNVERIFIED / "3=unload" — they now describe the documented bit field. - tests/i3/test_scalar_http_move_medium.py: assert unload lands in the SPECIFIC target slot (was only "some slot" — a misroute gap), and that a drive destination is rejected with 422. 12 move_medium/moveClass tests green. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012K5Zjc2bggcyJtFbz2D4Pw --- openblade/api/routes_aml_move_medium.py | 14 ++++++--- openblade/domain/scalar_coordinate.py | 13 +++++---- tests/i3/test_scalar_http_move_medium.py | 37 ++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/openblade/api/routes_aml_move_medium.py b/openblade/api/routes_aml_move_medium.py index 4792452..15129ce 100644 --- a/openblade/api/routes_aml_move_medium.py +++ b/openblade/api/routes_aml_move_medium.py @@ -10,9 +10,9 @@ is a physical ``frame/rack/section/column/row`` + type keyed on a globally-unique SCSI element address. Modelling that faithfully is roadmap Phase 0 (``ScalarCoordinate``). -- ``moveClass`` is handled as small integers (0 = normal, 3 = unload-to-home). The - real contract is a bit field with different values (e.g. unload) — UNVERIFIED - against a capture. +- ``moveClass`` is parsed as the documented bit field (0=Normal, 8=Unload, + 16=No-Eject, 32=Closest-slot; the single-integer 3=Unload is deprecated but still + accepted) via ``MoveClass.from_wire`` — see docs/reference/i3-contract-notes.md. Because the emulator and the ``scalar_http`` client currently share these simplifications, client<->emulator tests passing does NOT prove i3 fidelity. That @@ -96,7 +96,13 @@ async def move_medium( ) # Real moveClass=8 unload takes the drive source only; if a destination # slot is supplied (client hint), honor it, else the library picks an - # empty/home slot (Web Services manual). + # empty/home slot (Web Services manual). Guard the destination type: + # slot and drive addresses overlap, so an accidental drive destination + # would otherwise silently misroute the cartridge. + if destination is not None and destination[0] != "slot": + raise HTTPException( + status_code=422, detail="Unload destination must be a storage slot" + ) target_slot = destination[1] if destination is not None else _first_empty_slot(context) result = context.library.unload(source_address, target_slot) elif destination is None: diff --git a/openblade/domain/scalar_coordinate.py b/openblade/domain/scalar_coordinate.py index 6eddea6..12c372e 100644 --- a/openblade/domain/scalar_coordinate.py +++ b/openblade/domain/scalar_coordinate.py @@ -8,12 +8,13 @@ magic integer. These models formalize the real structure so the convenient integer forms live only in the adapter boundary. -IMPORTANT — values vs structure: the STRUCTURE here (coordinate fields, moveClass -bit flags) mirrors the documented i3 shape. The exact wire VALUES for ``moveClass`` -are UNVERIFIED against a real appliance — the external review indicates the real -"unload" value differs from the emulator's current ``3``. The wire mapping is -centralized in :func:`MoveClass.from_wire`/:meth:`MoveClass.to_wire` so a captured -compatibility case can certify/replace it without touching call sites. +Values are from the Quantum Web Services manual (6-68185-01 Rev D); the moveClass +bit values (8=Unload, 16=No-Eject, 32=Closest-slot; single-integer 3=Unload +deprecated) and the coordinate shape are documented, not guessed — see +docs/reference/i3-contract-notes.md. Parsing/serialization is centralized in +:func:`MoveClass.from_wire`/:meth:`MoveClass.to_wire`. The doc is Rev D (2019) and +the target firmware is 341G, so appliance-level certification of the exact numbers +still awaits a capture; the compatibility corpus is wired to enforce it. """ from __future__ import annotations diff --git a/tests/i3/test_scalar_http_move_medium.py b/tests/i3/test_scalar_http_move_medium.py index 9e76a1b..7ab3898 100644 --- a/tests/i3/test_scalar_http_move_medium.py +++ b/tests/i3/test_scalar_http_move_medium.py @@ -83,3 +83,40 @@ def test_load_into_nonexistent_drive_fails_gracefully(backend: ScalarHttpLibrary result = backend.load(slot_id, 9999) assert result.success is False # returned, not raised + + +def test_unload_returns_cartridge_to_the_specified_slot( + backend: ScalarHttpLibraryBackend, +) -> None: + # Regression for the moveClass=8 unload path: the cartridge must land in the + # SPECIFIC target slot the client asked for, not merely "some" slot. + slot_id = _first_occupied_slot(backend) + drive_id = _first_empty_drive(backend) + barcode = backend.get_slot(slot_id).barcode + assert barcode is not None + assert backend.load(slot_id, drive_id).success # source slot now empty + + result = backend.unload(drive_id, slot_id) + + assert result.success, result.message + landed = backend.get_slot(slot_id).barcode + assert landed is not None and landed.value == barcode.value + assert backend.find_drive_by_barcode(barcode.value) is None # drive is empty + + +def test_unload_rejects_a_drive_destination(backend: ScalarHttpLibraryBackend) -> None: + # moveClass=8 with a DRIVE destination must be rejected (slot/drive addresses + # overlap, so honoring it would silently misroute the cartridge). + slot_id = _first_occupied_slot(backend) + drive_id = _first_empty_drive(backend) + assert backend.load(slot_id, drive_id).success + + body = { + "moveMedium": { + "sourceCoordinate": {"elementType": "drive", "elementAddress": drive_id}, + "destinationCoordinate": {"elementType": "drive", "elementAddress": drive_id}, + "moveClass": 8, + } + } + resp = backend._session.request("POST", "/aml/media/operations/moveMedium", json=body) + assert resp.status_code == 422 From af8684503f32f00beb0ae87da5641e68502a9034 Mon Sep 17 00:00:00 2001 From: Amantux Date: Mon, 20 Jul 2026 19:29:28 +0000 Subject: [PATCH 2/2] Encode i3/i6 moveClass support boundary: reject Import/Export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Web Services manual marks moveClass Import(2) and Export(4) as "Currently not supported" on Scalar i3/i6 (only Normal/Unload/No-Eject/Closest-slot apply). The emulator models an i3, so it now rejects them (422) like a real appliance instead of silently doing a normal move. - MoveClass.is_supported_on_i3 (false when IMPORT or EXPORT bit set). - moveMedium route returns 422 for import/export. - Tests: unit boundary + route rejection of moveClass 2/4. NOTE: this branch also re-applies the #38 review fixes (unload destination guard + refreshed docstrings + specific-slot/drive-destination tests) that were LOST when auto-merge squashed only #38's first commit before the fix commit was pushed — so master was missing the reviewer-proven misroute guard. Restored here. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012K5Zjc2bggcyJtFbz2D4Pw --- openblade/api/routes_aml_move_medium.py | 7 +++++++ openblade/domain/scalar_coordinate.py | 9 +++++++++ tests/i3/test_scalar_http_move_medium.py | 16 ++++++++++++++++ tests/unit/test_scalar_coordinate.py | 9 +++++++++ 4 files changed, 41 insertions(+) diff --git a/openblade/api/routes_aml_move_medium.py b/openblade/api/routes_aml_move_medium.py index 15129ce..8c40a6e 100644 --- a/openblade/api/routes_aml_move_medium.py +++ b/openblade/api/routes_aml_move_medium.py @@ -79,6 +79,13 @@ async def move_medium( except (TypeError, ValueError): move_class = MoveClass.NORMAL + if not move_class.is_supported_on_i3: + # Import(2)/Export(4) are documented as not supported on Scalar i3/i6. + raise HTTPException( + status_code=422, + detail="moveClass import/export is not supported on Scalar i3/i6", + ) + source = _parse_coordinate(data.get("sourceCoordinate")) if source is None: raise HTTPException( diff --git a/openblade/domain/scalar_coordinate.py b/openblade/domain/scalar_coordinate.py index 12c372e..b541f26 100644 --- a/openblade/domain/scalar_coordinate.py +++ b/openblade/domain/scalar_coordinate.py @@ -89,6 +89,15 @@ class MoveClass(IntFlag): def is_unload(self) -> bool: return bool(self & MoveClass.UNLOAD) + @property + def is_supported_on_i3(self) -> bool: + """Whether this move is supported on a Scalar i3/i6. + + The Web Services manual marks Import(2) and Export(4) as "Currently not + supported" on i3/i6; Normal/Unload/No-Eject/Closest-slot are supported. + """ + return not bool(self & (MoveClass.IMPORT | MoveClass.EXPORT)) + @classmethod def from_wire(cls, value: int) -> MoveClass: """Parse a moveClass integer into the structured flag. diff --git a/tests/i3/test_scalar_http_move_medium.py b/tests/i3/test_scalar_http_move_medium.py index 7ab3898..73802bf 100644 --- a/tests/i3/test_scalar_http_move_medium.py +++ b/tests/i3/test_scalar_http_move_medium.py @@ -120,3 +120,19 @@ def test_unload_rejects_a_drive_destination(backend: ScalarHttpLibraryBackend) - } resp = backend._session.request("POST", "/aml/media/operations/moveMedium", json=body) assert resp.status_code == 422 + + +def test_moveMedium_rejects_import_export_on_i3(backend: ScalarHttpLibraryBackend) -> None: + # A real i3/i6 does not support Import(2)/Export(4) moveClass; the emulator + # rejects them rather than silently doing a normal move. + slot_id = _first_occupied_slot(backend) + for move_class in (2, 4): + body = { + "moveMedium": { + "sourceCoordinate": {"elementType": "slot", "elementAddress": slot_id}, + "destinationCoordinate": {"elementType": "slot", "elementAddress": slot_id}, + "moveClass": move_class, + } + } + resp = backend._session.request("POST", "/aml/media/operations/moveMedium", json=body) + assert resp.status_code == 422, move_class diff --git a/tests/unit/test_scalar_coordinate.py b/tests/unit/test_scalar_coordinate.py index 9bcb326..b48f93f 100644 --- a/tests/unit/test_scalar_coordinate.py +++ b/tests/unit/test_scalar_coordinate.py @@ -52,3 +52,12 @@ def test_moveclass_accepts_deprecated_unload_and_ignores_unknown_bits() -> None: # An unknown bit (1) that maps to no flag must NOT be treated as unload. assert MoveClass.from_wire(1) == MoveClass.NORMAL assert not MoveClass.from_wire(1).is_unload + + +def test_moveclass_i3_support_boundary() -> None: + # Import(2)/Export(4) are documented as not supported on i3/i6. + assert not MoveClass.from_wire(2).is_supported_on_i3 # import + assert not MoveClass.from_wire(4).is_supported_on_i3 # export + # Normal / Unload / No-Eject / Closest-slot are supported. + for value in (0, 8, 16, 24, 32, 40): + assert MoveClass.from_wire(value).is_supported_on_i3, value