fix(gentle): a corrupt Torque_Limit read must not raise from the cleanup

[OriNachum]

Found on hardware, mid-probe, while mapping joint limits.

The bug

CliError: Torque limit 2048 is out of range; valid range is 0–1000
  ... raised from gentle_move's finally

read_torque_limit returned 2048 on a healthy motor whose register actually held 500.
The read succeeded — it just returned nonsense, and 2048 is outside the register's own
[0, 1000] band, so it cannot be a torque limit at all.

That value was stashed as the pre-move Torque_Limit. The finally then tried to write it
back, the servo's range check rejected it, and a CliError was raised out of the cleanup —
masking whatever the move had actually been doing.

The finally suppressed only OverloadError, so a CliError sailed straight out of it.

Two things this is an instance of

A cleanup that can raise. Third time: safety._release_motor and profile._release were the
first two. The rule is now uniform — cleanup survives any BaseException (with SystemExit
re-raised), because a cleanup that throws replaces the failure the operator actually needs to see.

A read that succeeds and lies. Second time today: a read_position returned 0 immediately
after an EEPROM write while the servo genuinely held 3387. The bus layer retries reads that
fail; it has no way to know a successful one is garbage. A plausible-looking wrong value is
more dangerous than an error, because nothing downstream can tell it from a real reading.

The fix

• Validate at the point of READ, not the point of write. _sane_torque_limit() returns None
  for anything outside [0, 1000]. If we do not know the pre-move value, we say so rather than
  restoring a fiction.
• The finally cannot raise. With the pre-move value unknown, the conservative
  _CONTACT_TORQUE_LIMIT cap simply stays in place. A joint left slightly under-torqued is a
  nuisance; a cleanup that raises is a lie about why the move failed.

Tests

Two regression tests, both verified RED against the old code and GREEN against the fix:

• the move still completes and reports where it ended, rather than dying in its own finally
• 2048 never reaches the servo — only the cap is written

1244 tests pass (was 1242). black / isort / flake8 / bandit clean.

• arm101-cli (Claude)

[OriNachum]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix gentle_move: ignore corrupt Torque_Limit reads and never raise from cleanup

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Validate Torque_Limit at read-time and treat out-of-band values as unknown
• Make gentle_move’s restore path best-effort so finally never masks the real failure
• Add regression tests for corrupt reads and bump changelog/version to 0.22.1

Diagram

graph TD
  A["gentle_move()"] --> B["bus.read_torque_limit()"] --> C{Value in 0..1000?}
  C -- Yes --> D["original_torque_limit = value"] --> E["Write CONTACT cap"] --> F["Execute gentle motion"] --> G["finally: restore torque limit"]
  C -- No --> H["original_torque_limit = None"] --> E
  G --> I{original_torque_limit is None?}
  I -- No --> J["try write original; suppress BaseException"]
  I -- Yes --> K["Leave CONTACT cap in place"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Only suppress exceptions in finally (no read-time validation)

• ➕ Smallest change: make restore non-throwing and stop masking failures
• ➕ No new helper/logic for torque-limit plausibility
• ➖ Still treats corrupt reads as truth internally (until restore), making future logic riskier
• ➖ Doesn’t explicitly encode the invariant that Torque_Limit must be within 0..1000

2. Bus-level consistency reads (double-read / majority-read) for critical registers

• ➕ Addresses the deeper class of 'successful read that lies' more generally
• ➕ Could protect other reads (e.g., position) where plausibility checks are weaker
• ➖ Adds latency and complexity to the bus layer
• ➖ Hard to choose generic policies/thresholds; may still fail in correlated-error scenarios

3. Centralize cleanup safety via a shared helper/context manager

• ➕ Makes the 'cleanup must not raise (except SystemExit)' rule uniform and harder to regress
• ➕ Reduces repeated try/except patterns across safety/profile/gentle
• ➖ Requires a small refactor across modules; larger surface area for a hotfix release
• ➖ Must be carefully designed to avoid swallowing actionable operator signals

Recommendation: The PR’s approach is the best near-term fix: validate Torque_Limit at read-time (so corrupt values never influence restore decisions) and ensure finally cannot raise and mask the real failure. Consider a follow-on to add a reusable 'safe cleanup' helper and/or targeted multi-read strategies for registers where plausibility bounds aren’t as strict as Torque_Limit.

Files changed (5) +114 / -3

Bug fix (1) +46 / -2

gentle.pySanity-check torque-limit reads and make restore best-effort +46/-2

Sanity-check torque-limit reads and make restore best-effort

• Introduces _sane_torque_limit() with a hard bound (0..1000) to treat out-of-range reads as corrupt/unknown. Updates gentle_move to use this on the pre-move read and changes the finally restore to suppress all BaseException (re-raising SystemExit) so cleanup cannot mask the primary failure.

arm101/hardware/gentle.py

Tests (1) +50 / -0

test_gentle.pyAdd regressions for corrupt Torque_Limit reads in gentle_move cleanup +50/-0

Add regressions for corrupt Torque_Limit reads in gentle_move cleanup

• Adds a fake bus that returns a successful-but-garbage torque limit (2048) and two tests: the move must still return a result (no exception from finally), and the garbage value must never be written back (only the contact cap is written).

tests/test_gentle.py

Documentation (2) +17 / -0

arm101-cli__public.jsonlAdd hardware-run note about seam hiding due to factory offset +1/-0

Add hardware-run note about seam hiding due to factory offset

• Appends a new public memory entry documenting a hardware finding: the software bound can land on the seam for factory-offset joints, hiding additional travel across the wrap. This is contextual documentation for ongoing joint-limit mapping work.

.eidetic/memory/arm101-cli__public.jsonl

CHANGELOG.mdDocument 0.22.1 fix for corrupt Torque_Limit read masking failures +16/-0

Document 0.22.1 fix for corrupt Torque_Limit read masking failures

• Adds a 0.22.1 release entry describing the failure mode (garbage Torque_Limit read leading to CliError from finally) and the behavioral change (validate on read; restore cannot raise).

CHANGELOG.md

Other (1) +1 / -1

pyproject.tomlBump version to 0.22.1 +1/-1

Bump version to 0.22.1

• Updates project version from 0.22.0 to 0.22.1 to ship the hotfix.

pyproject.toml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 8 rules

✅ Skills: cicd

1. Silent torque restore failure 🐞 Bug ◔ Observability

Description

In gentle_move()'s finally, all exceptions from restoring Torque_Limit are swallowed, so a real
bus/comms failure can leave the motor capped (or in an unknown state) while gentle_move still
returns a normal result with no indication the restore failed or was skipped due to a corrupt read.

Code

arm101/hardware/gentle.py[R877-883]

        if original_torque_limit is not None:
-            with contextlib.suppress(OverloadError):
+            try:
                bus.write_torque_limit(motor, original_torque_limit)
+            except SystemExit:
+                raise
+            except BaseException:  # noqa: B036 - cleanup must outlive any bus failure
+                pass

Relevance

⭐ Low

Team prefers cleanup suppressing failures; best-effort BaseException suppression accepted to avoid
masking real errors (PRs #38,#39,#32).

PR-#38
PR-#39
PR-#32

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The restore path suppresses all failures, but the underlying bus API explicitly raises on write
failures; and the CLI surfaces the returned move dict verbatim, so without a new status field the
operator has no way to notice the restore did not occur.

arm101/hardware/gentle.py[792-907]
arm101/hardware/bus.py[1547-1588]
arm101/cli/_commands/arm.py[610-662]
arm101/hardware/safety.py[116-147]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`gentle_move()` intentionally prevents its cleanup from raising, but it currently provides **no signal** when torque-limit restoration was skipped (`original_torque_limit is None` due to a corrupt/out-of-range read) or when the restore write failed (any exception swallowed in the `finally`). This can leave the servo at `_CONTACT_TORQUE_LIMIT` while the CLI/operator sees an otherwise “successful” move result.

### Issue Context
- `MotorBus.write_torque_limit()` can fail with `CliError(EXIT_ENV_ERROR)` on comms/write failure.
- The CLI prints every key/value in the returned `gentle_move()` dict, so adding a status field is a straightforward way to make this visible without reintroducing cleanup-raised masking.

### Fix Focus Areas
- arm101/hardware/gentle.py[792-907]
- arm101/hardware/bus.py[1547-1588]
- arm101/cli/_commands/arm.py[610-662]

### Suggested approach
- Track and expose restore outcomes in the result dict, e.g.:
 - `"torque_limit_original": int | None`
 - `"torque_limit_restored": bool` (or `"torque_limit_restore_error": str | None`)
 - `"torque_limit_restore_skipped": bool` when the pre-move read was out-of-band
- Keep the “never raise from cleanup” rule, but capture the exception text (like safety does) for diagnostics instead of `pass`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud