feat: netsim driver - #980
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Netsim driver package for virtual radio control through the Netsim HTTP API and Netsim CLI. The change includes driver and client APIs, CLI commands, tests, package integration, exporter configuration, and documentation. ChangesNetsim driver
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NetsimClient
participant Netsim
participant NetsimRESTAPI
participant NetsimCLI
NetsimClient->>Netsim: Request a Netsim operation
Netsim->>NetsimRESTAPI: Send validated HTTP request
NetsimRESTAPI-->>Netsim: Return response data
Netsim->>NetsimCLI: Toggle capture when required
NetsimCLI-->>Netsim: Return command result
Netsim-->>NetsimClient: Return the operation result
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`:
- Around line 75-79: Reject unsupported state values instead of treating them as
disabled: add shared validation for explicit enabled and disabled aliases,
raising NetsimError for anything else, and reuse it in set_radio and the
capture-setting method. Update driver_test.py at lines 58-64 and 150-154 to
cover invalid radio and capture states.
- Around line 66-72: Encode device_name and chip_id with safe="" before
constructing Netsim URL paths, applying the encoded values across patch_device
and the related device/chip request methods in driver.py (66-72, 75-79, 82-85,
88-91, and 103-105). Add regression coverage in driver_test.py (46-50) using a
device name requiring escaping, and verify the request target preserves it as
one path segment.
In `@python/packages/jumpstarter-driver-netsim/README.md`:
- Line 4: Update the first “Android netsim” link so its target references
Netsim-specific REST API documentation; if that documentation is not
appropriate, relabel the anchor to accurately describe the existing Cuttlefish
environment-control destination.
- Around line 19-20: Update the prerequisites in the README to document the
netsim radio flags required by the examples: include --netsim=true or explicitly
list the separate WiFi, Bluetooth, and UWB flags supported by the target
Cuttlefish version, while retaining the REST API requirement.
In `@python/pyproject.toml`:
- Line 27: Reorder the Netsim registration alphabetically in both manifests: in
python/pyproject.toml lines 27-27, place jumpstarter-driver-netsim before
jumpstarter-driver-network in [tool.uv.sources]; in
python/packages/jumpstarter-all/pyproject.toml lines 26-26, place
"jumpstarter-driver-netsim" before "jumpstarter-driver-network" in the
dependency list.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9eba86f-33d4-44f3-afc5-1a2b74608907
📒 Files selected for processing (12)
docs/source/reference/package-apis/drivers/netsim.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-netsim/.gitignorepython/packages/jumpstarter-driver-netsim/README.mdpython/packages/jumpstarter-driver-netsim/examples/exporter.yamlpython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/__init__.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client_test.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.pypython/packages/jumpstarter-driver-netsim/pyproject.tomlpython/pyproject.toml
| # Netsim Driver | ||
|
|
||
| `jumpstarter-driver-netsim` controls | ||
| [Android netsim](https://source.android.com/docs/devices/cuttlefish/control-environment) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## README excerpt"
sed -n '1,80p' python/packages/jumpstarter-driver-netsim/README.md
echo
echo "## Driver files"
git ls-files 'python/packages/jumpstarter-driver-netsim/**'
echo
echo "## Search for netsim/cuttlefish/API references"
rg -n "netsim|Cuttlefish|cuttlefish|REST|source.android.com/docs/devices" python/packages/jumpstarter-driver-netsim README.md python/packages -S || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 19645
🌐 Web query:
source.android.com Android netsim REST API documentation Netsim
💡 Result:
Netsim, the network simulation tool for the Android Emulator, does not expose a public-facing REST API for general user integration [1][2][3]. Instead, it primarily utilizes gRPC for its internal and external communication interfaces [4][5][6]. Key technical details regarding Netsim interfaces include: gRPC Architecture: The core communication for Netsim, including interaction with the Android Emulator and other clients (like Bumble), is handled via gRPC [5][6]. The source code repository includes a proto directory that defines these service interfaces [1][3]. Transport Interaction: External tools often interact with Netsim by connecting to its gRPC server [5][6]. For instance, when using the Bumble library, you can configure transports to connect to the Netsim gRPC server using specific monikers (e.g., android-netsim:localhost:8555) [5][6]. Web UI and Internal APIs: There is a Web UI component for Netsim [7]. This UI communicates with the running netsimd background process, typically by consuming the protocol buffers defined in the project [7]. Developers can explore the proto and ui directories in the official source repository to understand how these services are structured [3][7]. Configuration: Users typically control Netsim features (such as packet capture, Wi-Fi configuration, or logging) through command-line arguments passed to the emulator using the -netsim-args flag, rather than through a REST API [2]. For further technical exploration, you can examine the official source code repository, which contains the complete definitions for the protocols and services used by the tool [1][3].
Citations:
- 1: https://android.googlesource.com/platform/tools/netsim/
- 2: https://developer.android.com/studio/run/emulator-networking-advanced
- 3: https://android.googlesource.com/platform/tools/netsim/+/ebde0483ee39a8dc64f4b2abe3034d69c49b269a/
- 4: https://android.googlesource.com/platform/tools/netsim/+/refs/tags/aml_per_351310020/CMakeLists.txt
- 5: https://google.github.io/bumble/transports/android_emulator.html
- 6: https://google.github.io/bumble/platforms/android.html
- 7: https://android.googlesource.com/platform/tools/netsim/+show/refs/heads/android14-qpr1-s2-release/ui/README.md
🌐 Web query:
source.android.com devices cuttlefish Netsim driver netsim REST API
💡 Result:
Netsim (or netsimd) is an open-source network simulation tool used by both Cuttlefish and the Android Emulator to provide radio-level emulation for multi-device Android testing [1][2][3]. In the context of Cuttlefish, the netsim server is managed by the Cuttlefish host controller (run_cvd), which launches the netsimd process [4][5]. The Cuttlefish configuration establishes communication with the netsimd process using FIFO pipes for radio hardware abstraction layers (HALs) [4][5]. Regarding the Netsim REST API and driver connectivity: 1. Communication Interface: While older configurations might use specific file descriptors, modern integration often relies on gRPC interfaces [6][2]. The netsimd process exposes a gRPC server interface to interact with external tools and emulated radio chips [6][2]. 2. Service Discovery: External tools (such as Bumble) often discover the netsimd gRPC port automatically by parsing a netsim.ini file, which is created by the netsimd process upon startup [2][7]. This file's location is platform-dependent [7]. 3. REST/API usage: The netsim tool is fundamentally designed for radio-level control and HCI (Host Controller Interface) tracing [3][8]. While internal gRPC communication is the primary mechanism for interaction, command-line arguments (e.g., --hci-port, --pcap) are used to configure the netsimd daemon's behavior during launch [1]. Developers typically interact with netsim indirectly through the Android Emulator or Cuttlefish launch flags, rather than by manually invoking a standalone REST API [1][2]. For advanced debugging or custom controller implementation, documentation for the Bumble library provides the most practical examples of connecting to the netsim gRPC interface [6][2].
Citations:
- 1: https://developer.android.com/studio/run/emulator-networking-advanced
- 2: https://google.github.io/bumble/platforms/android.html
- 3: https://android.googlesource.com/platform/tools/netsim/
- 4: https://android.googlesource.com/device/google/cuttlefish/+/ea39f61a69dc9a0fe4bd7de4b6583d745a67c8a6/host/commands/run_cvd/launch/netsim_server.cpp
- 5: https://android.googlesource.com/device/google/cuttlefish/+/e147ea701b67a985b3292ecabb2c0c2abb9a6c14/host/commands/run_cvd/launch/netsim_server.cpp
- 6: https://google.github.io/bumble/transports/android_emulator.html
- 7: Android Emulator issue - google/bumble#401
- 8: https://android.googlesource.com/platform/tools/netsim/+/refs/tags/android-17.0.0_r1
Provide a Netsim-specific target for the first link.
The Android netsim anchor currently points to Cuttlefish environment-control docs, while the driver talks about the netsim REST API. Update the target to a Netsim-specific API doc, or relabel the anchor to Cuttlefish environment control.
🤖 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 `@python/packages/jumpstarter-driver-netsim/README.md` at line 4, Update the
first “Android netsim” link so its target references Netsim-specific REST API
documentation; if that documentation is not appropriate, relabel the anchor to
accurately describe the existing Cuttlefish environment-control destination.
bd231d1 to
5f18013
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`:
- Around line 93-106: Update test_ble_off_stops_rx so BLE is restored to "on" in
a finally block after it is disabled, covering assertions and polling timeouts
while preserving the existing test checks.
- Around line 3-4: Move pytest_addoption registration for --live-netsim-host and
--live-netsim-port from driver_live_test.py into the package conftest.py so
pytest recognizes the options during startup. Update the README’s live-test
command to use the supported make test-jumpstarter-driver-netsim entry point
with both options.
In
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`:
- Around line 125-132: Update the subprocess call in the capture toggle methods
to catch subprocess.TimeoutExpired and OSError, translating both into
NetsimError while preserving the existing nonzero-return handling. Add coverage
for CLI timeout and launch failures, including an invalid netsim_cli.
In `@python/packages/jumpstarter-driver-netsim/README.md`:
- Around line 96-108: Remove the undocumented move, create, and delete-chip
examples from the Netsim README unless the client implements them. If retaining
them, add and test matching NetsimClient methods, including move_device, and
corresponding NetsimClient.cli() handlers so each documented command works.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 34853e88-dfe8-46e4-baae-7a5f98fed0a2
📒 Files selected for processing (10)
docs/source/reference/package-apis/drivers/index.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-netsim/README.mdpython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client_test.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.pypython/packages/jumpstarter-driver-netsim/pyproject.tomlpython/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-all/pyproject.toml
- python/pyproject.toml
- python/packages/jumpstarter-driver-netsim/pyproject.toml
| Run with: pytest -m live --live-netsim-host=HOST --live-netsim-port=PORT | ||
| Skipped by default in CI. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -t f '^(conftest\.py|pyproject\.toml|pytest\.ini|tox\.ini|Makefile)$' . |
while IFS= read -r file; do
rg -n -C 3 'pytest_addoption|pytest_plugins|pkg-test-jumpstarter-driver-netsim|live-netsim|PYTEST' "$file" || true
doneRepository: jumpstarter-dev/jumpstarter
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files near Make/config/test driver:"
git ls-files | rg '(^Makefile$|^python/pyproject\.toml$|^python/packages/jumpstarter-driver-netsim/(README\.md|pyproject\.toml|pytest\.ini|tox\.ini|.*conftest\.py|.*driver_live_test\.py)$|(^|/)pytest\.ini$|(^|/)tox\.ini$)' || true
echo
echo "Relevant mentions:")
rg -n -C 4 'pkg-test-jumpstarter-driver-netsim|pytest|live-netsim|PYTEST|make .*test|pytest_addoption|pytest_plugins' Makefile python/pyproject.toml python/packages/jumpstarter-driver-netsim || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=$(git ls-files | rg '(^Makefile$|^python/pyproject\.toml$|^python/packages/jumpstarter-driver-netsim/(README\.md|pyproject\.toml|pytest\.ini|tox\.ini|.*conftest\.py|.*driver_live_test\.py)$|(^|/)pytest\.ini$|(^|/)tox\.ini$)' || true)
echo "Tracked files:"
printf '%s\n' "$files"
echo
echo "Relevant mentions:"
for f in $files; do
echo "--- $f"
rg -n -C 4 'pkg-test-jumpstarter-driver-netsim|pytest|live-netsim|PYTEST|make .*test|pytest_addoption|pytest_plugins' "$f" || true
doneRepository: jumpstarter-dev/jumpstarter
Length of output: 8143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "pyproject pytest config:"
cat -n python/packages/jumpstarter-driver-netsim/pyproject.toml | sed -n '26,29p'
echo
echo "conftest mentions:"
git ls-files | rg '(^|/)conftest\.py$' | while IFS= read -r f; do
echo "--- $f"
rg -n -C 2 'pytest_addoption|--live-netsim' "$f" || true
done
echo
echo "Programmatic behavior probe from source: does pytest_addoption appear only in test module and package config?"
python3 - <<'PY'
from pathlib import Path
p = Path("python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py")
src = p.read_text()
print("driver_live_test.py contains pytest_addoption:", "def pytest_addoption" in src)
print("driver_live_test.py registered as pytest plugin:", "pytest_plugins" in p.read_text())
cfg = Path("python/packages/jumpstarter-driver-netsim/pyproject.toml").read_text()
print("package pytest ini contains live exclusion:", "-m 'not live'" in cfg)
print("package pytest ini contains live option forwarding config:", "--live-netsim" in cfg or "PYTEST_ADDOPTS" in cfg)
PY
echo
echo "pytest help behavior for unknown option (if pytest is available in sandbox):"
python3 - <<'PY'
import subprocess, sys
result = subprocess.run([sys.executable, "-m", "pytest", "--live-netsim-host=localhost", "-h"], text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print("returncode", result.returncode)
print("stderr excerpt lines:")
for line in result.stderr.splitlines()[:30]:
print(line)
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 2382
🌐 Web query:
pytest plugin order pytest_addoption command line option parsing before collecting test modules
💡 Result:
In pytest, the pytest_addoption hook is executed during the initialization phase, which occurs well before the test collection phase [1][2]. Plugin load order and pytest_addoption execution follow this sequence at startup: 1. Builtin plugins are loaded [3][4]. 2. Third-party plugins (via setuptools entry points) and those specified via command-line -p or the PYTEST_PLUGINS environment variable are loaded [3][4]. 3. Initial conftest.py files are loaded (those found in the test paths or root directory) [3][4]. The pytest_addoption hook is called for each plugin immediately as it is loaded [4]. Consequently, pytest_addoption in installed plugins or via -p runs before any conftest.py file is discovered or processed [5][4]. Because conftest.py files are loaded later, their own pytest_addoption implementations are also executed after those of the plugins loaded in steps 1 and 2 [4]. Because pytest_addoption is part of the early configuration and CLI parsing stage, it is not possible for standard conftest.py files (which are discovered during later phases) to influence the command-line options defined by plugins that were already initialized [5][6][4]. For complex scenarios requiring cross-plugin option interaction, you must ensure the dependent plugin is loaded early enough or use advanced plugin manager hooks to intercept option registration [5][4]. Test collection only begins after all command-line arguments have been parsed using the options registered during this earlier pytest_addoption phase [1][7][2].
Citations:
- 1: https://docs.pytest.org/en/8.0.x/_modules/_pytest/hookspec.html
- 2: https://docs.pytest.org/en/7.2.x/_modules/_pytest/hookspec.html
- 3: https://docs.pytest.org/en/8.3.x/how-to/writing_plugins.html
- 4: https://docs.pytest.org/en/stable/how-to/writing_hook_functions.html
- 5: https://pytest.org/en/stable/how-to/writing%5Fhook%5Ffunctions.html
- 6: Doc on
conftest.pyhook interactions withpytest_addoption()possibly misleading pytest-dev/pytest#13304 - 7: pytest_addoption is called after CLI argparsing when conftest is in a subdirectory of
testsand the path is not specified pytest-dev/pytest#9337
🌐 Web query:
pytest discovered conftest.py pytest_addoption called during collection or config phase
💡 Result:
The message "pytest_addoption called during collection config phase" (or similar behavior where options added in a conftest.py are not recognized) occurs because pytest processes command-line arguments and initializes configuration early in its startup sequence [1][2][3]. For pytest_addoption to function correctly—specifically to register command-line options so that pytest can parse them—the hook must be executed before the command-line parsing phase [4]. Pytest only executes pytest_addoption from "initial" conftest.py files during this early initialization phase [1][4]. Key points regarding this behavior: 1. Initialization Order: Pytest loads plugins and "initial" conftest.py files first to discover all command-line options [1][4]. If a conftest.py file is located in a subdirectory and is not automatically discovered as an initial conftest, its pytest_addoption hook will not be called until later, during the collection phase [5][3]. By that time, the command-line arguments have already been parsed, causing pytest to report "unrecognized arguments" if you try to use an option defined in that late-loaded file [5][6][4]. 2. Why it fails: When pytest_addoption runs during the collection phase (or later), it is too late to influence the command-line parser [3]. Attempting to add options at this stage is ineffective because the parsing process has already concluded [3]. 3. How to fix it: - Move your conftest.py to the root directory of your project [2][4]. - If the file must reside in a subdirectory, ensure it is treated as an initial conftest by using the testpaths configuration in your pytest.ini file or by explicitly including it in the PYTEST_PLUGINS environment variable [1][4]. - If you need to access cross-plugin hooks during pytest_addoption, be aware that only plugins loaded earlier than the current one will be visible; conftest.py files generally cannot be relied upon to provide hooks to other plugins at this stage [1][7]. In summary, this behavior is a fundamental design aspect of how pytest manages plugin discovery and configuration [1][4]. To ensure options are available, ensure your conftest.py is identified as an initial conftest during startup [4].
Citations:
- 1: https://docs.pytest.org/en/stable/how-to/writing_hook_functions.html
- 2: https://docs.pytest.org/en/8.0.x/_modules/_pytest/hookspec.html
- 3: Fixes #9303, throw if addoption called after preparse with no default pytest-dev/pytest#9395
- 4: Confusing error messages when specifying a path that doesn't exist when specifying a custom argument pytest-dev/pytest#13607
- 5: pytest_addoption is called after CLI argparsing when conftest is in a subdirectory of
testsand the path is not specified pytest-dev/pytest#9337 - 6: Re-evaluation of commandline after(!) full import of all conftest.py files pytest-dev/pytest#9739
- 7: Doc on
conftest.pyhook interactions withpytest_addoption()possibly misleading pytest-dev/pytest#13304
🌐 Web query:
pytest initial conftest.py root conftest.py pytest_addoption test modules option parsing before collection
💡 Result:
In pytest, the timing of option parsing and conftest.py loading is a critical aspect of the test startup process, often leading to confusion regarding command-line argument availability. ### The Mechanism of pytest_addoption and Initial Conftests Pytest processes command-line arguments and configuration before collecting tests. To allow conftest.py files to define command-line options, pytest identifies "initial" conftest.py files [1][2][3]. 1. Initial Conftest Identification: Before performing general collection, pytest determines the test paths (based on command-line arguments, testpaths in configuration, or the current directory) [2][4][3]. 2. Loading: It loads any conftest.py files found in those test paths and their parent directories [2][4][3]. 3. Execution: When an initial conftest.py is loaded, its pytest_addoption(parser) hook is called immediately [1][5][6]. This allows these specific files to register command-line options that the user provides when invoking pytest [1][2]. ### Critical Limitations * Scope of Availability: Only hooks from plugins loaded before a given pytest_addoption execution are available to it [1][5]. Because conftest.py files are discovered and loaded after builtin, third-party, and command-line specified plugins, they cannot easily use hooks from other conftest.py files or late-loaded plugins to influence their own pytest_addoption logic [1][7][5]. * Discovery Timing: If a conftest.py is located in a subdirectory that is not part of the initial "test paths" discovery, it may not be loaded in time to influence the initial command-line parsing [8][9][3]. This can lead to "unrecognized arguments" errors [9]. If you encounter this, explicitly specifying the test path on the command line often ensures the necessary conftest.py is identified as an "initial" conftest [8][3]. * Not for All Plugins: pytest_addoption is specifically for registering options. While it runs "before collection" in the sense that it sets up the parser, it is not a mechanism for general code execution before collection; it is strictly limited to the argparse registration phase [6]. ### Summary of Best Practice To ensure your command-line options are parsed correctly: * Place pytest_addoption in a conftest.py located at the root of your test tree [2][10]. * If you have complex testing directory structures, use the testpaths configuration in your pytest.ini or pyproject.toml to ensure pytest correctly identifies the directories containing your critical conftest.py files during its initial scan [2][3]. * Understand that conftest.py files are not equivalent to installed plugins; they have restricted visibility during the early pytest_addoption phase [1][7][5].
Citations:
- 1: https://docs.pytest.org/en/stable/how-to/writing_hook_functions.html
- 2: https://docs.pytest.org/en/stable/how-to/writing_plugins.html
- 3: https://stackoverflow.com/questions/78836105/why-isnt-the-pytest-addoption-hook-run-with-the-configured-testpaths-usag
- 4: https://docs.pytest.org/en/8.3.x/how-to/writing_plugins.html
- 5: https://docs.pytest.org/en/stable/how-to/writing%5Fhook%5Ffunctions.html
- 6: https://docs.pytest.org/en/8.3.x/_modules/_pytest/hookspec.html
- 7: Doc on
conftest.pyhook interactions withpytest_addoption()possibly misleading pytest-dev/pytest#13304 - 8: pytest_addoption is called after CLI argparsing when conftest is in a subdirectory of
testsand the path is not specified pytest-dev/pytest#9337 - 9: Re-evaluation of commandline after(!) full import of all conftest.py files pytest-dev/pytest#9739
- 10: https://docs.pytest.org/en/8.1.x/how-to/writing%5Fplugins.html
Register the live-test options in the package pytest config, not the collected test module.
pytest_addoption in driver_live_test.py runs after pytest parses --live-netsim-* options, so these arguments are rejected as unrecognized. Move the option registration to python/packages/jumpstarter-driver-netsim/conftest.py and update the README to show the supported entry point, such as make test-jumpstarter-driver-netsim "--live-netsim-host=HOST --live-netsim-port=PORT".
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`
around lines 3 - 4, Move pytest_addoption registration for --live-netsim-host
and --live-netsim-port from driver_live_test.py into the package conftest.py so
pytest recognizes the options during startup. Update the README’s live-test
command to use the supported make test-jumpstarter-driver-netsim entry point
with both options.
Source: Coding guidelines
5f18013 to
6a760ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`:
- Line 3: Update the live-test command documentation in driver_live_test.py to
use the package target make pkg-test-jumpstarter-driver-netsim instead of
invoking pytest directly. Document the repository-supported argument-forwarding
syntax for selecting the live marker and passing --live-netsim-host and
--live-netsim-port options.
In
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`:
- Around line 224-225: Update start_capture to validate that netsim_cli is
configured before calling _resolve_device_id or _list_devices_raw, using the
existing _capture_toggle configuration-error path. Update
test_start_capture_no_cli to verify the configuration error is returned and no
HTTP request is made.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dec0e9e-9cf3-4897-b357-8f7242d6ce70
📒 Files selected for processing (5)
python/packages/jumpstarter-driver-netsim/README.mdpython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/conftest.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-netsim/README.md
| @@ -0,0 +1,181 @@ | |||
| """Live tests that run against a real netsim instance. | |||
|
|
|||
| Run with: pytest -m live --live-netsim-host=HOST --live-netsim-port=PORT | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the package test target for the live-test command.
Line 3 instructs contributors to run pytest directly. Replace it with make pkg-test-jumpstarter-driver-netsim and document the repository-supported mechanism to forward the live marker and --live-netsim-* options.
As per coding guidelines, “Run package tests with make pkg-test-<package_name> and run the complete test suite with make test.”
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`
at line 3, Update the live-test command documentation in driver_live_test.py to
use the package target make pkg-test-jumpstarter-driver-netsim instead of
invoking pytest directly. Document the repository-supported argument-forwarding
syntax for selecting the live marker and passing --live-netsim-host and
--live-netsim-port options.
Source: Coding guidelines
6a760ef to
61fb7dd
Compare
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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`:
- Around line 169-181: Validate local inputs before device discovery: in
patch_device, parse patch_json before calling _resolve_device_id; in set_radio,
parse enabled and build the chip patch before resolving the device. Update
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py
lines 133-181 to use cvd-1 for invalid-input cases and assert requests_mock
received no request.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23a2d8c0-06a5-4d13-a742-eaccfa1a36fb
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py
| device_id = self._resolve_device_id(name_or_id) | ||
| try: | ||
| data = json.loads(patch_json) | ||
| except json.JSONDecodeError as e: | ||
| raise NetsimError(f"invalid JSON: {e}") from e | ||
| return self._fmt(self._request("PATCH", f"/v1/devices/{device_id}", {"device": data})) | ||
|
|
||
| @export | ||
| def set_radio(self, name_or_id: str, radio: str, enabled: str) -> str: | ||
| """Toggle a radio on/off. radio: bt_classic, ble, wifi, uwb. enabled: on/off.""" | ||
| device_id = self._resolve_device_id(name_or_id) | ||
| state = _parse_bool(enabled, "enabled") | ||
| chips = self._build_chip_patch(radio, state=state) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate local arguments before device discovery.
patch_device resolves name_or_id before it parses patch_json. set_radio resolves name_or_id before it validates enabled and radio. If a named device is used while Netsim is unavailable, invalid input raises a connection error instead of the local validation error.
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py#L169-L181: Parsepatch_json, parseenabled, and build the chip patch before_resolve_device_id.python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py#L133-L181: Usecvd-1in invalid-input tests and assert thatrequests_mockreceived no request.
As per coding guidelines, “Provide comprehensive package test coverage.”
📍 Affects 2 files
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py#L169-L181(this comment)python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py#L133-L181
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`
around lines 169 - 181, Validate local inputs before device discovery: in
patch_device, parse patch_json before calling _resolve_device_id; in set_radio,
parse enabled and build the chip patch before resolving the device. Update
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_test.py
lines 133-181 to use cvd-1 for invalid-input cases and assert requests_mock
received no request.
Source: Coding guidelines
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
61fb7dd to
3d8e9ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.py`:
- Around line 43-45: Update get_capture and the DriverCall result handling
around call_async/decode_value to preserve binary data through the protobuf
Binary API instead of converting bytes with Latin-1 or allowing Base64 string
serialization. Ensure capture results remain represented as the expected Binary
field end to end.
In
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`:
- Around line 134-136: Update both PATCH response assertions in
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py
at lines 134-136 and 158-161 to use resp.ok, negated, alongside the existing
"Incorrect" text check. Ensure any successful 2xx response from either endpoint
causes the assertion to fail.
In
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`:
- Around line 202-204: Update get_capture to validate that capture_id is numeric
before constructing the request URL or calling _request_bytes, rejecting values
containing path or query delimiters with a local validation error. Add
regression coverage for non-numeric IDs, including values with “/” or “?”, while
preserving valid numeric capture requests.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d219482-9df0-40fc-ada4-146596d3bdb9
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-netsim/README.mdpython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.pypython/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-netsim/README.md
| def get_capture(self, capture_id: str) -> bytes: | ||
| result = self.call("get_capture", capture_id) | ||
| return result.encode("latin-1") if isinstance(result, str) else result |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^client\.py$' python | while IFS= read -r file; do
ast-grep outline "$file" --items all --match 'DriverClient|call' || true
done
rg -n -C 5 'class DriverClient|def call\(|bytes|base64|latin-1|serialize|deserialize' pythonRepository: jumpstarter-dev/jumpstarter
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files 'python/packages/jumpstarter-driver-netsim/**' 'python/packages/jumpstarter/**/client.py' 'python/packages/jumpstarter-protocol/**/*client*.py' | sed -n '1,200p'
echo "== netsim client lines =="
cat -n python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.py | sed -n '1,140p'
echo "== netsim driver/capture references =="
rg -n -C 4 'def get_capture|get_capture|Base64|base64|latin-1|pcap-data' python/packages/jumpstarter-driver-netsim
echo "== focused protocol/client definitions =="
fd -a 'client\.py$' python | xargs -r rg -n -C 8 'class DriverClient|def call\(' || true
echo "== focused encoding/read-write byte contracts =="
rg -n -C 4 'Base64|base64|latin-1|bytes|serde|pydantic|json|yaml|msgpack|pickle' python/packages/python packages 2>/dev/null | rg -n -C 4 'latin-1|base64|DriverClient|call\(' | sed -n '1,240p' || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 17427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DriverClient implementation =="
cat -n python/packages/jumpstarter/jumpstarter/client/client.py | sed -n '1,260p'
echo "== netsim driver implementation =="
cat -n python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py | sed -n '1,280p'
echo "== tests using bytes capture path =="
cat -n python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client_test.py | sed -n '1,230p'
echo "== all get_capture references =="
rg -n -C 3 'def get_capture|\.get_capture\(|get_capture\(' pythonRepository: jumpstarter-dev/jumpstarter
Length of output: 36370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find DriverClient definition =="
rg -n "class DriverClient|def call\(" python/packages/jumpstarter python/packages 2>/dev/null | sed -n '1,240p'
echo "== stub client definition =="
fd -a 'base\.py|.*client.*\.py' python/packages/jumpstarter | xargs -r rg -n -C 8 'class StubDriverClient|def call\(' || true
echo "== exported driver call path search =="
rg -n -C 5 'export.*DriverClient|rpc|methods|call\(' python/packages/jumpstarter/jumpstarter python/exporter python/packages -g '*.py' | sed -n '1,260p'Repository: jumpstarter-dev/jumpstarter
Length of output: 30671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local base.py =="
cat -n python/packages/jumpstarter/jumpstarter/client/base.py | sed -n '1,220p'
echo "== router drivercall helpers =="
rg -n -C 8 'encode_value|decode_value|encode_.*value|decode_.*value|bytes|str' python/packages/jumpstarter -g '*.py' | sed -n '1,260p'
echo "== decode_value definition =="
fd -a '.*driver.*\.py|.*router.*\.py|.*jumpstarter.*\.py' python/packages/jumpstarter python/packages/jumpstarter-protocol | xargs -r rg -n -C 8 'def decode_value|def encode_value' | sed -n '1,200p'Repository: jumpstarter-dev/jumpstarter
Length of output: 30806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== client core definitions =="
fd -a 'core\.py$' python/packages/jumpstarter/jumpstarter/client | xargs -r rg -n -C 12 'class AsyncDriverClient|def call_async|def streamingcall_async|async def.*DriverCall|encode|decode' | sed -n '1,260p'
echo "== encode/decode helpers all =="
rg -n -C 6 'def encode_value|def decode_value|value_to.*|.*to_value|bytes|latin-1|base64' python/packages/jumpstarter python/packages/jumpstarter-protocol -g '*.proto' -g '*.py' | sed -n '1,320p'Repository: jumpstarter-dev/jumpstarter
Length of output: 34155
🌐 Web query:
Google protobuf struct_pb2 Value bytes field encode decode base64
💡 Result:
The google.protobuf.Value message does not natively support a bytes field [1][2]. Because google.protobuf.Struct and google.protobuf.Value are designed to represent arbitrary JSON objects and values, they are limited to the types supported by the JSON specification (null, number, string, boolean, object, and array) [1][3][4]. Since bytes are not a standard JSON type, they cannot be directly assigned to a Value field [4]. To store binary data using google.protobuf.Value, you must encode the bytes into a string, typically using base64 encoding, and store that string in the string_value field [5][4]. When using protobuf libraries to convert messages to or from JSON (e.g., using MessageToDict or similar utility functions), bytes fields in standard protobuf messages are automatically base64-encoded because JSON itself does not support raw binary data [6]. However, when specifically working with google.protobuf.Value, you are responsible for this conversion manually before setting the string_value [5]. Summary of handling bytes with Value: 1. Encoding: Convert your bytes data to a base64-encoded string using your programming language's standard base64 library [5]. 2. Storage: Assign this base64 string to the string_value field of the Value message [5][3]. 3. Decoding: When retrieving the data, read the string_value and decode the base64 string back into bytes [5].
Citations:
- 1: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto
- 2: https://www.mintlify.com/protocolbuffers/protobuf/reference/well-known-types-ref
- 3: https://buf.build/protocolbuffers/wellknowntypes/raw/v25.0/-/google/protobuf/struct.proto
- 4: Add bytes_value (and possibly others) to the struct.Value well-known-type proto. protocolbuffers/protobuf#3078
- 5: https://go.googlesource.com/protobuf/+/refs/heads/master/types/known/structpb/struct.pb.go?autodive=0%2F%2F
- 6: https://stackoverflow.com/questions/63467082/why-protobuf-python-do-base64-encode-for-bytes-field-in-messagetodict-function
Keep the Binary field used for DriverCall results.
call_async serializes driver results via decode_value(response.result), and decode_value uses protobuf JSON Value; bytes must use the Binary API, not the Latin-1 fallback. If get_capture reaches this path, returning raw bytes will be encoded as a Base64 string or fail.
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/client.py`
around lines 43 - 45, Update get_capture and the DriverCall result handling
around call_async/decode_value to preserve binary data through the protobuf
Binary API instead of converting bytes with Latin-1 or allowing Base64 string
serialization. Ensure capture results remain represented as the expected Binary
field end to end.
| assert resp.status_code != 200 or "Incorrect" in resp.text, ( | ||
| "REST capture PATCH succeeded - netsim may have fixed the bug. Consider removing CLI fallback." | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep 'driver_live_test.py$' | head -20
echo "== relevant file outline =="
ast-grep outline python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py || true
echo "== relevant lines =="
sed -n '110,175p' python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py
echo "== inspect imports/requests usage context =="
sed -n '1,80p' python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py
echo "== exact matching assertions =="
rg -n "status_code != 200|resp\.ok|Correct|Incorrect|PATCH|Name-based|resolve_device_id" python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 7216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python requests 2xx status handling probe =="
python3 - <<'PY'
try:
import requests
except ImportError:
import subprocess
subprocess.check_call(["uv", "pip", "install", "requests"], stdout=subprocess.DEVNULL)
import requests
for s in [200, 201, 202, 204, 299, 300, 400, 500]:
res = requests.models.Response()
res.status_code = s
try:
res.raise_for_status()
raises = False
except requests.exceptions.HTTPError:
raises = True
print(f"status {s}: ok={res.ok}, raises_status_not_ok={raises} (2xx_ok={s < 300})")
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 1025
🌐 Web query:
Python requests library Response.ok property definition 2xx
💡 Result:
In the Python requests library, the Response.ok property is a boolean that returns True if the status code of the response is less than 400, and False otherwise [1][2][3]. While it correctly identifies successful requests—including all 2xx success codes—it is important to note that this property is not a specific check for a "200 OK" status code [1][2]. Because it returns True for any status code in the 2xx or 3xx range (as long as they are below 400), it should not be used when you specifically require a 200 status [2][3]. If you need to verify that a response is specifically 200, you should compare the response status code against the built-in lookup object, requests.codes.ok, or the integer 200 directly [4][5]: if response.status_code == requests.codes.ok: # Do something pass The Response.ok property essentially acts as a convenience check to determine if the request encountered a client error (4xx) or server error (5xx) [1][6].
Citations:
- 1: https://docs.python-requests.org/en/latest/_modules/requests/models/
- 2: https://docs.python-requests.org/en/latest/api/
- 3: https://requests.readthedocs.io/en/latest/api/
- 4: https://stackoverflow.com/questions/54087303/python-requests-how-to-check-for-200-ok
- 5: https://docs.python-requests.org/en/latest/user/quickstart/
- 6: https://github.com/psf/requests/blob/3e7d0a873f838e0001f7ac69b1987147128a7b5f/requests/models.py
🌐 Web query:
Python requests Response.raise_for_status implementation status code check < 400
💡 Result:
In the Python requests library, the raise_for_status method is implemented to raise an HTTPError exception only if the response status code indicates a client error (4xx) or a server error (5xx) [1][2]. Specifically, the internal implementation checks the status code with the following logic: 1. It checks if the status code is in the range [400, 500) (Client Error) [1][2]. 2. It checks if the status code is in the range [500, 600) (Server Error) [1][2]. 3. If the status code falls into either of these ranges, it constructs an error message and raises an HTTPError [1][2]. If the status code is less than 400 (which includes successful 2xx responses and informational 1xx responses), the method does not raise an exception, effectively treating these codes as successful [1][2]. For identifying if a response is successful (status code < 400), the requests library also provides the.ok property, which is implemented using this same logic [1][3]. It calls raise_for_status internally; if an HTTPError is raised, it returns False; otherwise, it returns True [1][3].
Citations:
- 1: https://docs.python-requests.org/en/stable/_modules/requests/models/
- 2: https://github.com/psf/requests/blob/v2.32.5/src/requests/models.py
- 3: https://github.com/psf/requests/blob/8149e9fe54c36951290f198e90d83c8a0498289c/requests/models.py
Detect all successful REST PATCH responses.
status_code != 200 already accepts successful 201, 202, and 204 responses. Use not resp.ok or "Incorrect" in resp.text in both tests so a Netsim update that makes either PATCH endpoint work fails the assertion.
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py#L134-L136python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py#L158-L161
📍 Affects 1 file
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py#L134-L136(this comment)python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py#L158-L161
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py`
around lines 134 - 136, Update both PATCH response assertions in
python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver_live_test.py
at lines 134-136 and 158-161 to use resp.ok, negated, alongside the existing
"Incorrect" text check. Ensure any successful 2xx response from either endpoint
causes the assertion to fail.
| def get_capture(self, capture_id: str) -> bytes: | ||
| """Download a packet capture as pcap bytes.""" | ||
| return self._request_bytes(f"/v1/captures/{capture_id}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate capture_id before constructing the capture URL.
get_capture accepts arbitrary strings although capture operations require numeric IDs. A value containing / or ? changes the requested path or query instead of returning a local validation error. Reject non-numeric IDs before _request_bytes, and add regression coverage.
Proposed fix
def get_capture(self, capture_id: str) -> bytes:
"""Download a packet capture as pcap bytes."""
+ if not capture_id.isdigit():
+ raise NetsimError(f"capture_id must be numeric, got {capture_id!r}")
return self._request_bytes(f"/v1/captures/{capture_id}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_capture(self, capture_id: str) -> bytes: | |
| """Download a packet capture as pcap bytes.""" | |
| return self._request_bytes(f"/v1/captures/{capture_id}") | |
| def get_capture(self, capture_id: str) -> bytes: | |
| """Download a packet capture as pcap bytes.""" | |
| if not capture_id.isdigit(): | |
| raise NetsimError(f"capture_id must be numeric, got {capture_id!r}") | |
| return self._request_bytes(f"/v1/captures/{capture_id}") |
🤖 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
`@python/packages/jumpstarter-driver-netsim/jumpstarter_driver_netsim/driver.py`
around lines 202 - 204, Update get_capture to validate that capture_id is
numeric before constructing the request URL or calling _request_bytes, rejecting
values containing path or query delimiters with a local validation error. Add
regression coverage for non-numeric IDs, including values with “/” or “?”, while
preserving valid numeric capture requests.
Source: Coding guidelines
1 participant
No description provided.