[jevgenijostin]

Summary

Follow-up to #959, which adds the global clean_then_mop flag from get_status but explicitly leaves the per-room layer alone:

There is also a per-room layer. get_customize_clean_mode returns seq_type and repeat per segment [...] This PR models only the global value from get_status; the per-room layer is left alone.

This PR models that per-room layer: a CustomizeCleanModeTrait that reads get_customize_clean_mode into a list of SegmentCleanMode (one per room with an override) and a set_clean_then_mop(segment, enabled, ...) setter that writes set_customize_clean_mode.

Verification

Tested against a Roborock QV 35S (roborock.vacuum.a168). get_customize_clean_mode starts empty ([]) until a room has actually had a custom mode set from the app or API. Round-tripped a write/read on one room:

set_customize_clean_mode([{"segment": 7, "fan_power": 104, "water_box_mode": 203,
                            "mop_mode": 300, "repeat": 1, "seq_type": 1}])
-> ["ok"]

get_customize_clean_mode()
-> [{"segment": 7, "fan_power": 104, "water_box_mode": 203,
     "mop_mode": 300, "repeat": 1, "seq_type": 1}]

then reset back to seq_type: 0 to leave the room's real setting unchanged.

Notes

• Reuses is_clean_then_mop_mode_supported as the feature gate, same as feat(v1): expose clean-then-mop status from the device #959's global trait. No device has been found that supports one layer but not the other - the vendor app presents a single "vacuum then mop" toggle that can be applied globally or per room, backed by the same underlying setting.
• Mirrors StatusTrait.clean_then_mop / set_clean_then_mop's shape and docstrings at room granularity on purpose, so the two read the same way.
• Single device verified (QV 35S), same caveat as feat(v1): expose clean-then-mop status from the device #959.
• uv run pytest passes locally; new tests in tests/devices/traits/v1/test_customize_clean_mode.py.

Motivation

Lets a consumer (e.g. Home Assistant) expose "vacuum then mop" as a per-room choice, matching the Roborock app's own room-settings screen, not just the global job-level toggle from #959.

[allenporter]

Review assisted by Gemini 4 Argon in Antigravity.

Note: it looks like your reply in #984 (comment) posted the literal file path @C:/Users/jevge/AppData/Local/Temp/claude/shots/review-reply.txt instead of the file contents!

Looking at the updated commit (515b9ba5), a few architectural and API contract thoughts:

1. Passing/updating SegmentCleanMode & avoiding silent repeat resets (CustomizeCleanModeTrait.set_clean_then_mop):
   • Since omitting repeat causes the device to reset the room's repeat back to 1 rather than preserving it, repeat: int | None = None is a footgun for callers. Either repeat should be required / defaulted to 1 (or read from the existing by_segment entry), or the setter on CustomizeCleanModeTrait could accept a SegmentCleanMode (or typed VacuumModes / WaterModes / CleanRoutes enums rather than raw wire ints).
   • Also watch out for the docstring suggestion to fall back to StatusTrait (fan_power, water_box_mode, mop_mode) when a room has no override yet: when custom room modes are active (CleaningMode.CUSTOM), StatusTrait reports the sentinel codes fan_power=106 (VacuumModes.CUSTOMIZED), water_box_mode=204 (WaterModes.CUSTOMIZED), and mop_mode=302 (CleanRoutes.CUSTOMIZED), which aren't valid per-room settings.
2. Feature gating on CustomizeCleanModeTrait (customize_clean_mode.py#L85):
   • GET_CUSTOMIZE_CLEAN_MODE / SET_CUSTOMIZE_CLEAN_MODE is the general per-room custom mode command (is_customized_clean_supported), whereas is_clean_then_mop_mode_supported specifically gates seq_type. Gating the entire CustomizeCleanModeTrait on is_clean_then_mop_mode_supported hides the trait on devices that support per-room custom modes (fan_power, water_box_mode, mop_mode, repeat) without bit 93 (CLEAN_THEN_MOP_MODE).
3. Silent filtering of malformed list items (CustomizeCleanModeConverter.convert):
   • if isinstance(entry, dict) silently drops non-dict list entries (e.g. ["ok"]) and returns modes=[]. It should validate all(isinstance(entry, dict) for entry in response) and raise ValueError otherwise so V1TraitMixin.refresh() raises RoborockParsingException.
4. Docstring consistency with feat(v1): expose clean-then-mop status from the device #959 (customize_clean_mode.py#L3-L8):
   • The module docstring says the global StatusTrait.clean_then_mop flag "is per-job and clears after every clean", which contradicts the updated docstring in feat(v1): expose clean-then-mop status from the device #959 stating that the global seq_type is also persisted on the device while docked.
5. Trait discovery in tests (tests/devices/traits/v1/test_customize_clean_mode.py):
   • discover_features_fixture in tests/devices/traits/v1/fixtures.py is autouse=True and reads device_info.new_feature_set, so you can parametrize device_info (with bit 93 set/unset on new_feature_set) to test trait discovery via device.v1_properties.customize_clean_mode rather than constructing CustomizeCleanModeTrait() directly and assigning trait._rpc_channel.

[jevgenijostin]

Thanks, this is a thorough review and all of it checked out. Pushed 4142f21 addressing each point:

1. repeat footgun: now defaults to 1 in the signature instead of being omittable, so the wire request always carries an explicit value rather than relying on the device's own (surprising) reset-to-1 behavior.

   Sentinel-code fallback: you're right, I verified VacuumModes.CUSTOMIZED=106 / WaterModes.CUSTOMIZED=204 / CleanRoutes.CUSTOMIZED=302 in v1_clean_modes.py and removed the "fall back to StatusTrait" suggestion from the docstring entirely, replacing it with an explicit warning not to do that while a per-room custom mode is active.

2. Feature gate: changed to is_customized_clean_supported, confirmed in device_features.py that this is the one actually tied to get/set_customize_clean_mode (via ProductFeatures.MOP_SHAKE_MODULE/MOP_SPIN_MODULE), while is_clean_then_mop_mode_supported only covers the seq_type sub-axis. On a device with the former but not the latter, seq_type/clean_then_mop just stay None per room, which the existing Optional modeling already handles.

3. Converter: now raises on any non-dict list entry instead of filtering it out, so a malformed response surfaces as RoborockParsingException via refresh() like it should.

4. Docstring: fixed, it now matches StatusTrait.clean_then_mop's own description (persisted, not per-job).

5. Test discovery: looked into this, and it doesn't translate directly - is_customized_clean_supported is gated by product_features (derived from product_nickname/feature_info), not the new_feature_info_str bit-93 mechanism the existing _create_cleaning_mode_status_trait-style helpers parametrize. I didn't find any existing test in the suite exercising that product_features discovery path to mirror, and reverse-engineering a product_nickname fixture known to lack MOP_SHAKE_MODULE/MOP_SPIN_MODULE felt like a bigger yak-shave than this PR should take on. Kept the direct-construction style instead, consistent with how child_lock/led_status/valley_electricity_timer are tested today. Happy to revisit if you think it's worth it.

Also, sorry again for the garbled first reply, that was a genuine tooling slip on my end posting a literal file path - fixed now with the actual content.

[allenporter]

Note: This review was generated using Google Antigravity with Gemini 4 Argon and the repository's code-review skill (AGENTS.md).

Thanks for addressing the previous round of feedback in 4142f219 (switching requires_feature to is_customized_clean_supported, defaulting repeat: int = 1, rejecting non-dict list items in CustomizeCleanModeConverter, and documenting the StatusTrait sentinel codes)! The PR rebases cleanly onto main and passes pytest and ruff.

Looking closely at the updated trait scope (is_customized_clean_supported), how SET_CUSTOMIZE_CLEAN_MODE behaves across multiple rooms/models, and how this coordinates with #959, I left inline comments covering:

1. Public Trait API & Consumer Contract (Priority 1): Generalizing CustomizeCleanModeTrait beyond a raw-int set_clean_then_mop setter (so devices without CTM and rooms without an existing override have a usable, typed API), decoupling from unmerged #959, and placing container models in roborock.data.
2. Feature Gating & Mixin Initialization (Priority 1/2): Gating clean-then-mop read/write behavior on is_clean_then_mop_mode_supported via DeviceFeaturesTrait, and ensuring V1TraitMixin.__init__ runs despite dataclass MRO.
3. Multi-Room Data Loss & Model-Specific Fields (Priority 2): Preserving other segments' overrides and extra per-segment fields (e.g. mop_power, custom_water_box_distance) instead of overwriting the table with a 1-element params=[entry].
4. Tests & Fixtures: Using existing mock_data.DEVICE_PRODUCT_PAIRS (home_data_device_s5e.json) with discover_features_fixture instead of mutating trait._rpc_channel, and fixing the enabled=False assertion in test_set_clean_then_mop.

[jevgenijostin]

Thanks for the deep second pass - all of this is addressed now in a1b3279, pushed just now. Going through each point:

1. Coupling to feat(v1): expose clean-then-mop status from the device #959 & model placement: Moved SegmentCleanMode/CustomizeCleanModes (plus the SEQ_TYPE_CLEAN_THEN_MOP constant) into roborock/data/v1/v1_containers.py, re-exported via roborock.data through the existing from .v1 import * chain - tests/conformance/test_model_conformance.py picks them up automatically now, confirmed green. Reworded the module/class docstrings in customize_clean_mode.py to stop referencing StatusTrait.clean_then_mop/set_clean_then_mop by name (those don't exist on main, they're from the still-draft feat(v1): expose clean-then-mop status from the device #959) - it now just says "the global settings" generically.

2. Feature gating & dataclass MRO: CustomizeCleanModeTrait.__init__ now takes device_features: DeviceFeaturesTrait explicitly and calls both super().__init__() and common.V1TraitMixin.__init__(self), so _rpc_channel is always set. discover_features() in traits/v1/__init__.py constructs it with the device's DeviceFeaturesTrait (mirroring the wash_towel_mode pattern you pointed at). set_clean_then_mop checks self._device_features.is_clean_then_mop_mode_supported up front and raises RoborockUnsupportedFeature if it's False - test_set_clean_then_mop_unsupported now exercises this on the default S7 MaxV fixture (genuinely has per-room modes but not clean-then-mop) instead of faking it.

   On your point 1 specifically (seq_type: 0 vs None on firmware without bit 93): I didn't add a check for that, since I don't have a way to verify whether such firmware actually reports seq_type at all vs omitting the key - SegmentCleanMode is a plain data container mirroring the raw wire response (same as StatusV2), and it has no device-feature context to gate on. The write path is what actually matters for safety and that's covered. Happy to add a guard if you can confirm the firmware behavior, but didn't want to guess at it.

3. Raw wire integers on the public API: set_clean_then_mop now accepts VacuumModes | int, WaterModes | int, CleanRoutes | int for fan_power/water_box_mode/mop_mode (unwraps .code when a typed enum is passed) - test_set_clean_then_mop_accepts_typed_modes covers this. I left the broader "general per-room setter beyond clean-then-mop, with v1_clean_modes helpers resolving per-device defaults for an uncustomized room" as out of scope for this PR - it's a real gap but a separate, larger piece of work than exposing clean-then-mop itself. Open to taking it on as a follow-up.

4. Multi-room data loss / unmodeled keys: This was the big one. refresh() now also caches each room's raw wire dict keyed by its canonical typed segment id (self._raw_by_segment), and set_clean_then_mop builds the full table from that cache (overlaying only the 6 keys it actually changes) instead of a bare 1-entry/6-key dict - params=list(raw_entries.values()) sends every known room, and untouched rooms (plus any field this library doesn't model, e.g. mop_power/custom_water_box_distance) round-trip byte-for-byte. Also found and fixed, while testing this: a duplicate-segment response now raises instead of silently collapsing, a "Segment"-cased key no longer desyncs the raw cache from the typed one, a stringified segment id ("7") no longer gets double-sent, and concurrent set_clean_then_mop calls are now serialized under a lock so they can't clobber each other's writes (all four have dedicated regression tests, each verified by reverting the fix and confirming the test fails).

5. Test fixtures: Rewrote the test module to use device.v1_properties.customize_clean_mode via the existing discover_features_fixture/device fixtures instead of constructing the trait directly - exactly the pattern you sketched. S5E (is_customized_clean_supported=False) for the not-discovered case, SAROS_10R (both features True) for everything that exercises set_clean_then_mop, default S7 MaxV fixture for the "supports per-room modes but not clean-then-mop" unsupported case.

6. enabled=False assertion mismatch: Fixed - the post-write refresh mock is now built from expected_entry (which varies with enabled) instead of a static all-seq_type=1 fixture, so test_set_clean_then_mop actually exercises and asserts the False case now.

Verification run just now: focused suite 108 passed (incl. conformance), full suite 1082 passed, 11 deselected, 53 xfailed, 1 error - the 1 error (tests/e2e/test_device_manager.py::test_v1_device, a setup-time ValueError) is pre-existing on main too (reproduced it there independently, same error, same spot) and passes in isolation - looks like an unrelated test-order flake, not something this PR touches. ruff check/ruff format --check clean on all 4 changed files. mypy on the 4 changed files: 0 errors (the 7 it reports are all in unrelated files - containers.py, common.py, protocol.py, two proto files - pre-existing, confirmed via git worktree diff against main).

[allenporter]

Note: This review was generated using Google Antigravity with Gemini 4 Argon and the repository's code-review skill (AGENTS.md).

Thanks for the thorough update in a1b32790! Moving SegmentCleanMode and CustomizeCleanModes to v1_containers.py (covered by test_model_conformance.py), decoupling from #959, injecting DeviceFeaturesTrait with V1TraitMixin.__init__, accepting typed mode enums, serializing the read-merge-write path under asyncio.Lock while preserving untouched segments and unmodeled fields, and reworking the tests around device.v1_properties.customize_clean_mode all look great.

The branch merges cleanly with main (v7.17.0) and passes pytest, ruff, and mypy. Just a few small items left before we get this merged:

1. CI Blocker (codespell): uv run pre-commit run --all-files flags a typo (unsupport) in test_customize_clean_mode.py.
2. Public Trait API & Consumer Contract (Priority 1): Clarifying the non-CTM docstring on CustomizeCleanModeTrait, prefixing _SEQ_TYPE_CLEAN_THEN_MOP so it isn't wildcard-exported onto roborock.*, and rejecting sentinel mode codes (CUSTOMIZED / SMART_MODE) at runtime in set_clean_then_mop.
3. Minor Cleanup (Priority 2): Removing unused _WRITE_KEYS, parameterizing dict[int, dict[str, Any]], and tightening the inline hardware-verification comment.

[allenporter]

CI Blocker (pre-commit / codespell):

uv run pre-commit run --all-files fails codespell (exit code 65) on unsupport:

Suggested change

	hardware, rather than faking unsupport on a device that does support it.
	hardware, rather than faking lack of support on a device that does support it.

[allenporter]

Expose is_clean_then_mop_supported & fix non-CTM docstring claim (AGENTS.md Priority 1):

1. CustomizeCleanModeTrait is discovered on all is_customized_clean_supported devices (e.g. S7 MaxV, S8, Qrevo), most of which have is_clean_then_mop_mode_supported == False. Because self._device_features is private, a consumer holding customize_clean_mode has no public property on the trait itself to check whether set_clean_then_mop is supported without reaching across to v1_properties.device_features.is_clean_then_mop_mode_supported or catching RoborockUnsupportedFeature.
2. Lines 44–46 still assert that on a non-CTM device "SegmentCleanMode.seq_type (and therefore .clean_then_mop) is simply None for every room", even though nothing in refresh() enforces that (and test_refresh runs on the S7 MaxV fixture with seq_type=0/1). If non-CTM V1 firmware still serializes "seq_type": 0, a consumer checking mode.clean_then_mop is not None would render a toggle that always raises RoborockUnsupportedFeature.

Please expose a public @property def is_clean_then_mop_supported(self) -> bool delegating to self._device_features.is_clean_then_mop_mode_supported (similar to how WashTowelModeTrait and StatusTrait expose capability-derived properties directly on the trait), and update the docstring to direct callers to is_clean_then_mop_supported rather than assuming seq_type is omitted.

[allenporter]

Small follow-up on (1): since v1_properties.device_features.is_clean_then_mop_mode_supported is already public on PropertiesApi, adding a separate is_clean_then_mop_supported property on CustomizeCleanModeTrait is optional—either exposing it on the trait for convenience or simply updating the docstring here to point callers to device_features.is_clean_then_mop_mode_supported (instead of assuming SegmentCleanMode.seq_type is guaranteed None on non-CTM firmware) works!

[allenporter]

Dead code (_WRITE_KEYS) & bare dict annotations (AGENTS.md §1):

• _WRITE_KEYS is unused anywhere in the module or tests and can be removed.
• Please parameterize self._raw_by_segment: dict[int, dict[str, Any]] = {} (line 59) and raw_by_segment: dict[int, dict[str, Any]] = {} (line 93) instead of bare dict.

[allenporter]

Sentinel mode validation & source comment (AGENTS.md Priority 1/2):

1. Reject sentinel mode codes at runtime: Lines 127–132 document not to pass StatusTrait's sentinel CUSTOMIZED codes (fan_power=106, water_box_mode=204, mop_mode=302), and SMART_MODE (110 / 209 / 306) is a sentinel as well—yet the VacuumModes | int / WaterModes | int / CleanRoutes | int annotations statically permit those enum members. Validating against those sentinel codes up front (raising ValueError before acquiring self._write_lock) turns the docstring warning into a safe runtime check.
2. Inline hardware note: Lines 194–198 ("This has not been re-verified against real hardware for THIS device since this fix was written...") read like a PR comment rather than permanent library documentation—condensing that comment to state the full-table wire behavior directly (or verifying a two-room round-trip if convenient) would keep the module clean.

[allenporter]

Hey @jevgenijostin — stepping back for a second: thank you for your patience through multiple rounds here, and sorry for putting you through repeated waves of noisy AI-assisted review comments and shifting nits. The core work in a1b3279 (the container models, converter, lock-serialized full-table preservation, and discovery tests) is in great shape.

Please don't feel obligated to spend another round chasing the comments above — I'd love to help accelerate getting this across the finish line (we can push a small co-authored follow-up commit on our end to handle the remaining debug statements / API polish and get @Lash-L to review, unless you're already mid-edit and prefer to push yourself). Just let me know if you're working on anything locally so we don't step on your branch!