Spruce up and make useful for 2026 - #6
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces Travis CI with a GitHub Actions workflow, modernizes the Dockerfile to ChangesCI Infrastructure, ESP32 Substrate Fakes, and Regression Tests
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Docker as Docker Build
participant Conformance as substrate-conformance
participant Regression as wifimanager-regression
participant Fakes as SubstrateFakes
participant WM as WifiManager
CI->>Docker: build MICROPYTHON_VERSION=v1.28.0
CI->>Docker: run substrate-conformance.py
Docker->>Conformance: micropython substrate-conformance.py
Conformance->>Fakes: import network and machine
Fakes-->>Conformance: PASS or FAIL per case
CI->>Docker: run wifimanager-status-regression.py
Docker->>Regression: micropython wifimanager-status-regression.py
Regression->>Fakes: install substrate and reset_all
Regression->>WM: load WifiManager and reset state
Regression->>Fakes: program_scan and fail_connect
Regression->>WM: scan and connect_candidates and manage
WM->>Fakes: sta.connect and sta.status
Fakes-->>WM: scripted responses
WM-->>Regression: events and state transitions
Regression-->>CI: PASS or FAIL per case
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dockerfile to build MicroPython v1.28.0 on Debian Bookworm, replaces Travis CI with GitHub Actions, and introduces an ESP32 test substrate with fakes and conformance tests. Feedback on the changes includes addressing CPython compatibility issues by adding empty __init__ methods to the Pin and WLAN fakes, raising a descriptive ValueError instead of ValueError(None), ensuring the patched fast_sleep yields control to the event loop, and explicitly installing libffi8 in the Dockerfile to prevent runtime shared library errors after purging build dependencies.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def __new__(cls, id, *args, **kwargs): | ||
| inst = cls._instances.get(id) | ||
| if inst is None: | ||
| inst = object.__new__(cls) | ||
| inst.id = id | ||
| inst.mode = None | ||
| inst.pull = -1 | ||
| inst.calls = [] | ||
| cls._instances[id] = inst | ||
| cls._levels.setdefault(id, 0) | ||
| if args or kwargs: | ||
| inst._configure(args, kwargs) | ||
| return inst |
There was a problem hiding this comment.
In standard Python (CPython), if __new__ returns an instance of the class, Python automatically invokes __init__ on that instance with the same arguments. Since Pin does not define an __init__ method, it falls back to object.__init__. When arguments are passed (e.g., Pin(5, Pin.OUT)), object.__init__ raises a TypeError: object.__init__() takes no arguments.
To ensure compatibility with standard Python test runners, linters, and hybrid environments, please define an empty __init__ method that accepts any arguments.
| def __new__(cls, id, *args, **kwargs): | |
| inst = cls._instances.get(id) | |
| if inst is None: | |
| inst = object.__new__(cls) | |
| inst.id = id | |
| inst.mode = None | |
| inst.pull = -1 | |
| inst.calls = [] | |
| cls._instances[id] = inst | |
| cls._levels.setdefault(id, 0) | |
| if args or kwargs: | |
| inst._configure(args, kwargs) | |
| return inst | |
| def __new__(cls, id, *args, **kwargs): | |
| inst = cls._instances.get(id) | |
| if inst is None: | |
| inst = object.__new__(cls) | |
| inst.id = id | |
| inst.mode = None | |
| inst.pull = -1 | |
| inst.calls = [] | |
| cls._instances[id] = inst | |
| cls._levels.setdefault(id, 0) | |
| if args or kwargs: | |
| inst._configure(args, kwargs) | |
| return inst | |
| def __init__(self, id, *args, **kwargs): | |
| pass |
| def __new__(cls, interface=STA_IF): | ||
| if interface not in (STA_IF, AP_IF): | ||
| raise ValueError("invalid WLAN interface identifier") | ||
| inst = cls._instances.get(interface) | ||
| if inst is None: | ||
| inst = object.__new__(cls) | ||
| inst.interface = interface | ||
| inst.reset() | ||
| cls._instances[interface] = inst | ||
| return inst |
There was a problem hiding this comment.
Similar to Pin, WLAN uses __new__ to return a cached singleton instance but does not define __init__. When WLAN(interface) is called with an argument, standard Python (CPython) automatically calls object.__init__ with that argument, raising TypeError: object.__init__() takes no arguments.
Please define an empty __init__ method to prevent this TypeError and ensure compatibility with standard Python environments.
| def __new__(cls, interface=STA_IF): | |
| if interface not in (STA_IF, AP_IF): | |
| raise ValueError("invalid WLAN interface identifier") | |
| inst = cls._instances.get(interface) | |
| if inst is None: | |
| inst = object.__new__(cls) | |
| inst.interface = interface | |
| inst.reset() | |
| cls._instances[interface] = inst | |
| return inst | |
| def __new__(cls, interface=STA_IF): | |
| if interface not in (STA_IF, AP_IF): | |
| raise ValueError("invalid WLAN interface identifier") | |
| inst = cls._instances.get(interface) | |
| if inst is None: | |
| inst = object.__new__(cls) | |
| inst.interface = interface | |
| inst.reset() | |
| cls._instances[interface] = inst | |
| return inst | |
| def __init__(self, interface=STA_IF): | |
| pass |
| if bssid is not None and len(bssid) != 6: | ||
| raise ValueError(None) |
There was a problem hiding this comment.
Raising ValueError(None) is non-standard and can lead to confusing error messages (e.g., displaying ValueError: None). It is better to provide a descriptive error message or raise ValueError() without arguments.
| if bssid is not None and len(bssid) != 6: | |
| raise ValueError(None) | |
| if bssid is not None and len(bssid) != 6: | |
| raise ValueError("bad bssid len") |
| async def fast_sleep(*_args, **_kwargs): | ||
| counter["n"] += 1 | ||
| if counter["n"] >= iterations: | ||
| raise _StopLoop() |
There was a problem hiding this comment.
The patched fast_sleep coroutine does not contain any await expressions, making it a synchronous coroutine. In cooperative multitasking (like asyncio / uasyncio), sleep is expected to yield control to the event loop to allow other scheduled tasks to run. If fast_sleep returns immediately without yielding, other concurrent tasks (e.g., background tasks in the library under test) may never get a chance to execute, potentially causing deadlocks or test failures.
To preserve cooperative multitasking behavior without introducing wall-clock delays, you can yield control using the original, unpatched sleep or sleep_ms with a delay of 0.
| async def fast_sleep(*_args, **_kwargs): | |
| counter["n"] += 1 | |
| if counter["n"] >= iterations: | |
| raise _StopLoop() | |
| async def fast_sleep(*_args, **_kwargs): | |
| counter["n"] += 1 | |
| if counter["n"] >= iterations: | |
| raise _StopLoop() | |
| if "sleep" in saved: | |
| await saved["sleep"](0) | |
| elif "sleep_ms" in saved: | |
| await saved["sleep_ms"](0) |
| apt-get install -y --no-install-recommends \ | ||
| build-essential \ | ||
| ca-certificates \ | ||
| git \ | ||
| libffi-dev \ | ||
| pkg-config \ | ||
| python3 \ |
There was a problem hiding this comment.
The unix port of MicroPython dynamically links to libffi for its ffi module. When libffi-dev and python3 are purged with --auto-remove (lines 21-26), the runtime library libffi8 (which is installed as a dependency of libffi-dev) will be automatically removed if no other package depends on it. This will cause the micropython binary to fail at runtime with a shared library loading error (e.g., libffi.so.8: cannot open shared object file).
To prevent libffi8 from being auto-removed, explicitly install libffi8 in the apt-get install list.
apt-get install -y --no-install-recommends \
build-essential \
ca-certificates \
git \
libffi-dev \
libffi8 \
pkg-config \
python3 \
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
tests/substrate-conformance.py (1)
103-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand conformance cases for contract items that are currently unguarded.
The suite is missing checks for: invalid
Pinid rejection,Pin.irqpresence,connect()rejecting unknown kwargs, andifconfig()set/get round-trip. Adding these will prevent silent contract drift.Suggested coverage additions
def case_connect_contract_and_spy(): @@ # bssid must be exactly 6 bytes assert _raises(ValueError, lambda: sta.connect("Net", bssid=b"\x01\x02")) + # contract shape: only kw-only bssid is accepted + assert _raises(TypeError, lambda: sta.connect("Net", foo="bar")) @@ def case_ifconfig_and_config(): @@ sta.connect("Office", "pw") assert sta.ifconfig()[0] != "0.0.0.0" + custom = ("10.0.0.2", "255.255.255.0", "10.0.0.1", "1.1.1.1") + assert sta.ifconfig(custom) is None + assert sta.ifconfig() == custom @@ def case_pin_external_input_and_isolation(): @@ machine.reset_all() assert machine.Pin(0).value() == 0 # reset_all clears GPIO state + assert _raises(ValueError, lambda: machine.Pin(-1)) + assert hasattr(machine.Pin(0), "irq")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/substrate-conformance.py` around lines 103 - 191, Add four missing contract validation tests to the conformance suite: expand case_connect_contract_and_spy to test that the connect method rejects unknown keyword arguments, expand case_ifconfig_and_config to verify that ifconfig values can be set and retrieved in a round-trip, add a new test case to verify that creating a Pin with an invalid id raises a ValueError, and add a new test case to verify that the Pin class provides an irq method or attribute. These tests should follow the existing pattern of using assertions and the _raises helper function where applicable.tests/stubs/webrepl.py (1)
4-5: 🎯 Functional Correctness | 🔵 TrivialConsider making
webrepl.startaccept arbitrary arguments for API compatibility.The stub currently has a zero-arg-only signature. While no calls within this repository pass arguments, accepting
*args, **kwargswould allow the stub to remain compatible with external code that may invokewebrepl.startwith options.♻️ Proposed compatibility patch
-def start(): +def start(*_args, **_kwargs): return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stubs/webrepl.py` around lines 4 - 5, The `start` function in the webrepl stub currently has a zero-argument signature which limits API compatibility with external code that may pass arguments when calling `webrepl.start()`. Modify the function signature to accept arbitrary positional and keyword arguments by adding `*args, **kwargs` parameters to the `start()` function definition, while keeping the existing return statement unchanged to maintain the current behavior.tests/stubs/logging.py (1)
82-105: 🎯 Functional Correctness | 🔵 TrivialAdd module-level logging wrappers to improve API compatibility.
The stub's
Loggerclass already implementswarning(),error(),critical(), andexception(), but these aren't exposed at the module level. Code usinglogging.warning(),logging.error(), etc. will fail withAttributeError. Additionally,getLogger()currently requires an argument; the standard library accepts it as optional, defaulting to the root logger.♻️ Proposed changes
-def getLogger(name): - return get_logger(name) +def getLogger(name=None): + return get_logger(name or "root")def debug(msg, *args): get_logger("root").debug(msg, *args) + + +def warning(msg, *args): + get_logger("root").warning(msg, *args) + + +warn = warning + + +def error(msg, *args): + get_logger("root").error(msg, *args) + + +def critical(msg, *args): + get_logger("root").critical(msg, *args) + + +def exception(msg, *args): + get_logger("root").exception(msg, *args)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stubs/logging.py` around lines 82 - 105, The stub is missing module-level logging wrapper functions for warning, error, critical, and exception methods, which are present in the Logger class but not exposed at the module level like info and debug are. Additionally, the getLogger function requires a name argument, but the standard library accepts it as optional and defaults to the root logger. Add wrapper functions for warning, error, critical, and exception following the same pattern as the existing info and debug functions that delegate to get_logger("root"), and modify the getLogger function signature to make the name parameter optional with a default value of None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 17-18: Add the persist-credentials: false configuration to both
checkout@v4 actions in the workflow file. Locate both instances of the
"Checkout" step using the actions/checkout@v4 action and add
persist-credentials: false to each one to prevent GitHub token material from
persisting in the git configuration and being accessible to subsequent workflow
steps.
- Line 18: The GitHub Actions in the workflow are using floating version tags
(like `@v4`) which poses a supply chain security risk. Replace all floating tags
with their full commit SHAs for the actions/checkout action on line 18, and any
other GitHub Actions referenced on lines 31 and 56. For each action, determine
the specific commit SHA corresponding to the version tag and update the uses
field to pin to that immutable commit hash instead of the floating tag.
- Around line 12-15: The current job has `packages: write` permission granted
for all execution paths including untrusted pull_request runs, which
unnecessarily exposes package-write capabilities. Remove `packages: write` from
the permissions block in the main job, keeping only `contents: read`. Then
create a separate `publish` job that is triggered only on push events to the
default branch, and grant only that job the `packages: write` permission. This
ensures package-write capability is isolated to the trusted publish path only.
In `@Dockerfile`:
- Around line 21-27: Remove libffi-dev from the list of packages being purged in
the apt-get purge --auto-remove command to prevent the runtime libffi8 library
from being removed as an automatic dependency. Keep build-essential, git,
pkg-config, and python3 in the purge list, but exclude libffi-dev so that the
libffi.so.8 library remains available for the micropython binary at runtime.
In `@substrate/machine.py`:
- Around line 23-35: Add validation in the `__new__` method to verify that the
provided `id` parameter is a valid pin ID. If the ID is invalid (does not meet
the valid pin criteria), raise `ValueError("invalid pin")` before attempting to
use the ID in the _instances dictionary or elsewhere in the method. This ensures
the contract requirement is explicitly enforced rather than relying on
incidental failures from unhashable objects.
- Around line 75-77: The Pin class is missing the irq method implementation that
is declared as part of the substrate contract, causing AttributeError when
consumers try to access it. Add the irq method to the same class that contains
the init method shown in the diff to expose the missing irq functionality and
match the declared substrate interface. Ensure the method signature and behavior
align with the expected contract that consumers are relying on.
In `@substrate/network.py`:
- Around line 171-177: The ifconfig method in the network.py file stores a
custom ifconfig value in self._config when arguments are provided, but the read
operation (when no arguments are passed) ignores this stored value and always
returns hardcoded tuples based on self._connected. Fix this by modifying the
return statement to first check if "ifconfig" exists in self._config and return
that stored value, then fall back to returning the hardcoded _CONNECTED_IFCONFIG
or _DISCONNECTED_IFCONFIG tuples only if no custom value was previously set.
- Around line 129-133: The connect() method uses **kwargs to accept arbitrary
keyword arguments, which prevents it from catching invalid call shapes and
weakens call-boundary fidelity. Replace the **kwargs parameter with an explicit
bssid parameter (with a default value of None) in the method signature, and then
use bssid directly instead of extracting it from kwargs using
kwargs.get("bssid"). This will make the method contract-strict and ensure only
valid parameters are accepted.
---
Nitpick comments:
In `@tests/stubs/logging.py`:
- Around line 82-105: The stub is missing module-level logging wrapper functions
for warning, error, critical, and exception methods, which are present in the
Logger class but not exposed at the module level like info and debug are.
Additionally, the getLogger function requires a name argument, but the standard
library accepts it as optional and defaults to the root logger. Add wrapper
functions for warning, error, critical, and exception following the same pattern
as the existing info and debug functions that delegate to get_logger("root"),
and modify the getLogger function signature to make the name parameter optional
with a default value of None.
In `@tests/stubs/webrepl.py`:
- Around line 4-5: The `start` function in the webrepl stub currently has a
zero-argument signature which limits API compatibility with external code that
may pass arguments when calling `webrepl.start()`. Modify the function signature
to accept arbitrary positional and keyword arguments by adding `*args, **kwargs`
parameters to the `start()` function definition, while keeping the existing
return statement unchanged to maintain the current behavior.
In `@tests/substrate-conformance.py`:
- Around line 103-191: Add four missing contract validation tests to the
conformance suite: expand case_connect_contract_and_spy to test that the connect
method rejects unknown keyword arguments, expand case_ifconfig_and_config to
verify that ifconfig values can be set and retrieved in a round-trip, add a new
test case to verify that creating a Pin with an invalid id raises a ValueError,
and add a new test case to verify that the Pin class provides an irq method or
attribute. These tests should follow the existing pattern of using assertions
and the _raises helper function where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb0443cc-82ea-4e32-b721-9c35981ebeb4
📒 Files selected for processing (12)
.github/workflows/ci.yml.travis.ymlDockerfileREADME.mdsubstrate/CONTRACT-esp32-v1.28.mdsubstrate/aiotest.pysubstrate/machine.pysubstrate/network.pytests/stubs/logging.pytests/stubs/webrepl.pytests/substrate-conformance.pytests/wifimanager-status-regression.py
💤 Files with no reviewable changes (1)
- .travis.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@substrate/machine.py`:
- Line 39: The __init__ method in the machine class uses `id` as a parameter
name, which shadows the Python builtin function and triggers the Ruff A002
linting rule. Rename the `id` parameter to a non-builtin name such as
`machine_id` or similar, and update all references to this parameter throughout
the __init__ method body to use the new parameter name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 869535b4-ce4e-4fe5-b705-1b3b354b1dea
📒 Files selected for processing (8)
.github/workflows/ci.ymlDockerfilesubstrate/aiotest.pysubstrate/machine.pysubstrate/network.pytests/stubs/logging.pytests/stubs/webrepl.pytests/substrate-conformance.py
✅ Files skipped from review due to trivial changes (1)
- tests/stubs/webrepl.py
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci.yml
- substrate/aiotest.py
- tests/substrate-conformance.py
- tests/stubs/logging.py
- substrate/network.py
- Dockerfile
1 participant
Summary by CodeRabbit