Add mitmproxy driver for HTTP(S) interception and backend mocking#254
Open
kirkbrauer wants to merge 18 commits intomainfrom
Open
Add mitmproxy driver for HTTP(S) interception and backend mocking#254kirkbrauer wants to merge 18 commits intomainfrom
kirkbrauer wants to merge 18 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new mitmproxy driver package for Jumpstarter enables HTTP(S) interception, request/response mocking, traffic recording/replay, request capture, and WebSocket support. Includes comprehensive documentation, demo application with backend simulator, example addons for media streaming, mock scenario templates, test fixtures, and complete driver implementation with client and server components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Addon as MitmproxyMockAddon
participant Registry as AddonRegistry
participant Engine as TemplateEngine
participant CustomAddon as Custom Addon<br/>(if delegated)
participant Response as HTTP Response
Client->>Addon: request(flow)
activate Addon
Addon->>Addon: _load_config()<br/>_find_endpoint()
Addon->>Addon: _matches_conditions()?
alt Addon Delegation
Addon->>Registry: get_handler(addon_name)
activate Registry
Registry-->>Addon: Handler instance
deactivate Registry
Addon->>CustomAddon: handle(flow, config)
activate CustomAddon
CustomAddon->>Engine: TemplateEngine.render()
activate Engine
Engine-->>CustomAddon: rendered response
deactivate Engine
CustomAddon-->>Addon: True (handled)
deactivate CustomAddon
else Template/File Response
Addon->>Engine: TemplateEngine.render(template)
activate Engine
Engine-->>Addon: rendered body
deactivate Engine
end
Addon->>Addon: _send_response(status, body)
Addon->>Response: set response + headers
deactivate Addon
sequenceDiagram
participant DUT as DUT Client
participant Proxy as MitmproxyMockAddon
participant CaptureClient as CaptureClient
participant Socket as Unix Socket
participant Driver as MitmproxyDriver
DUT->>Proxy: HTTP Request
activate Proxy
Proxy->>Proxy: Process request<br/>Generate response
Proxy->>CaptureClient: send_event(capture_dict)
activate CaptureClient
CaptureClient->>CaptureClient: _connect()
CaptureClient->>Socket: emit event (JSON)
deactivate CaptureClient
Proxy-->>DUT: HTTP Response
deactivate Proxy
Driver->>Socket: receive event
activate Driver
Driver->>Driver: Store in capture buffer
deactivate Driver
Driver->>Driver: wait_for_request()<br/>assert_request_made()
activate Driver
Driver-->>Driver: Match against buffer
deactivate Driver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py-95-99 (1)
95-99:⚠️ Potential issue | 🟡 Minor
messages[-1]raisesIndexErrorwhen the messages list is empty.The guard on line 92 only checks for
None, butflow.websocket.messagescan be an empty list (e.g. on connection open before any message arrives), making the index access on line 95 blow up. Since this template is copied verbatim by users, the bug will propagate.🛡️ Proposed fix
- last_msg = flow.websocket.messages[-1] - if last_msg.from_client: + if not flow.websocket.messages: + return + last_msg = flow.websocket.messages[-1] + if last_msg.from_client:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py` around lines 95 - 99, The code indexes flow.websocket.messages[-1] without ensuring the list is non-empty, which raises IndexError when messages == []; update the guard around that block (in the area using last_msg and flow.websocket.messages) to check that flow.websocket.messages is not None and has at least one element (e.g. check truthiness or len(flow.websocket.messages) > 0) before assigning last_msg and calling ctx.log.info so you only access the last element when the list contains an item.python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py-1-33 (1)
1-33:⚠️ Potential issue | 🟡 MinorModule docstring and example snippet reference "JSON" but the driver uses a YAML-based scenario system.
Lines 6, 11–16, and 56–57 all say "scenario JSON" / "mock scenario JSON". The PR description explicitly states this is a "YAML-based scenario system", and the scenario examples use YAML. Users copying this template will be confused about which format to use for
addon/addon_config.📝 Proposed fix
-The filename (without .py) becomes the addon name you reference -in mock scenario JSON files. +The filename (without .py) becomes the addon name you reference +in mock scenario YAML files. -Example: Save as ``addons/my_custom_api.py``, then reference in -your scenario JSON as:: +Example: Save as ``addons/my_custom_api.py``, then reference in +your scenario YAML as:: - "GET /my/endpoint/*": { - "addon": "my_custom_api", - "addon_config": { - "any_key": "passed to your handler" - } - } + GET /my/endpoint/*: + addon: my_custom_api + addon_config: + any_key: passed to your handlerAlso update lines 56–57 in the
handledocstring:- config: The "addon_config" dict from the mock scenario - JSON. Empty dict if not specified. + config: The "addon_config" dict from the mock scenario + YAML. Empty dict if not specified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py` around lines 1 - 33, The module docstring and examples incorrectly refer to a "scenario JSON" system; update all occurrences that mention "JSON" or "mock scenario JSON" to "YAML" (or "scenario YAML") so users know the driver uses a YAML-based scenario system; update the example snippet that shows how to reference the addon (the section that references "addon" and "addon_config") and the handle(flow, config) docstring lines (currently mentioning JSON) to mention YAML instead, ensuring the Handler class docstring and example usage consistently reference YAML-based scenarios.python/packages/jumpstarter-driver-mitmproxy/examples/scenarios/backend-degraded.yaml-41-45 (1)
41-45:⚠️ Potential issue | 🟡 MinorMisleading comment — 503 is not a timeout.
The comment says "time out" but the mock returns a
503 Service Unavailableafter 8 s of latency. A timeout is a transport-layer event (no response); a 503 is an explicit server-side error. Consider updating the comment to accurately describe what is being simulated (slow 503) and whether it additionally covers timeout scenarios.✏️ Suggested fix
- # All telemetry uploads time out -- tests retry logic + # All telemetry uploads return 503 with high latency -- tests slow-failure retry logic POST /api/v1/telemetry/upload: status: 503 body: {error: Backend overloaded} latency_ms: 8000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/scenarios/backend-degraded.yaml` around lines 41 - 45, Update the misleading comment above the POST /api/v1/telemetry/upload mock: it currently says "time out" but the mock returns status: 503 with latency_ms: 8000 (a slow 503), so change the comment to indicate this simulates a slow server error (e.g., "All telemetry uploads return a slow 503 to exercise retry logic"); if you intended to simulate transport-layer timeouts as well, add a separate scenario that omits a response or configures a transport timeout instead of using status: 503.python/packages/jumpstarter-driver-mitmproxy/examples/exporter.yaml-1-35 (1)
1-35:⚠️ Potential issue | 🟡 MinorFile is missing
apiVersion,kind, andmetadata— invalid standalone ExporterConfig.The comment on Line 3 says to install this as
/etc/jumpstarter/exporters/my-bench.yaml, but unlikedemo/exporter.yamlthis file has no top-level Jumpstarter metadata. Without those fields the Jumpstarter CLI will likely reject or misparse it.🛠️ Proposed fix — add missing top-level headers
# Example Jumpstarter exporter configuration with mitmproxy integration. # # Install to: /etc/jumpstarter/exporters/my-bench.yaml # ... +apiVersion: jumpstarter.dev/v1alpha1 +kind: ExporterConfig +metadata: + namespace: default + name: my-bench + export:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/exporter.yaml` around lines 1 - 35, This YAML lacks the top-level Jumpstarter resource headers; add apiVersion, kind (ExporterConfig), and metadata.name at the top so the file becomes a valid standalone ExporterConfig that the CLI will accept; keep the existing top-level "export:" block and existing resources like the dutlink (type: jumpstarter_driver_dutlink.driver.Dutlink) unchanged and follow the same header values/structure used in demo/exporter.yaml for consistency.python/packages/jumpstarter-driver-mitmproxy/pyproject.toml-23-25 (1)
23-25:⚠️ Potential issue | 🟡 MinorReplace
repowithjumpstarterin thesource_archiveURL.The
source_archiveURL currently useshttps://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip, but the actual repository isjumpstarter-dev/jumpstarter. Update tohttps://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/pyproject.toml` around lines 23 - 25, Update the source_archive entry in pyproject.toml to point to the correct GitHub repository: change the value of the source_archive key from using "repo" to "jumpstarter" so it becomes "https://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip"; edit the source_archive line in the [tool.hatch.metadata.hooks.vcs.urls] section to perform this replacement.python/packages/jumpstarter-driver-mitmproxy/README.md-379-388 (1)
379-388:⚠️ Potential issue | 🟡 MinorMissing Dockerfile for documented container deployment.
The README's "Container Deployment" section (lines 379-388) instructs users to run
podman build -t jumpstarter-mitmproxy:latest ., but noDockerfileorContainerfileexists in the package directory. Users following these instructions will encounter an immediate build error. Either provide the container build artifact or remove this deployment section from the README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/README.md` around lines 379 - 388, The README's "Container Deployment" section references building a container with the command "podman build -t jumpstarter-mitmproxy:latest ." and running "jmp exporter start my-bench" but there is no Dockerfile/Containerfile in the package, causing build failures; either add a Dockerfile/Containerfile that reproduces the image referenced by the podman build command (ensuring it exposes ports 8080/8081, mounts /etc/jumpstarter as shown, and installs the jmp CLI and exporter entrypoint), or remove/update the "Container Deployment" snippet in README.md (the podman build/run lines and the "jmp exporter start my-bench" example) to avoid instructing users to build a non-existent image.python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py-51-67 (1)
51-67:⚠️ Potential issue | 🟡 MinorADTS frame header encodes
frame_length = 0— malformed frame that strict decoders will reject.The
aac_frame_lengthfield is a non-byte-aligned 13-bit field spanning bytes 3–5 of the ADTS header:
byte 3 [1:0]→frame_length[12:11]byte 4 [7:0]→frame_length[10:3]byte 5 [7:5]→frame_length[2:0]For a 31-byte frame (7-byte header + 24 bytes of silence data) the correct values are:
byte 4=0x03(bits 10–3 of 31 =0b00000011)byte 5=0xFF(bits 2–0 of 31 =0b111in positions 7–5; VBR buffer-fullness0x7FFfills bits 4–0 =0b11111)The current
\x00\x1fpacks 31 as a plain big-endian 16-bit integer, ignoring the bit offsets. As a result, theaac_frame_lengthfield decodes to 0, making the frame technically invalid per ISO 14496-3 ADTS. Strict decoders (and DUT audio stacks under test) may skip or error on every generated frame, defeating the stated goal of exercising the DUT's decode/buffer/playback path.🛠️ Proposed fix — correct byte 4 and byte 5
SILENT_AAC_FRAME = ( b"\xff\xf1" # ADTS sync word + MPEG-4, Layer 0 b"\x50" # AAC-LC, 44100 Hz (idx 4) b"\x80" # Channel config: 2 (stereo) - b"\x00\x1f" # Frame length (header + padding) - b"\xfc" # Buffer fullness (VBR) + b"\x03\xff" # frame_length=31: byte4=frame_len[10:3]=0x03, byte5=frame_len[2:0]=0b111<<5 | buf_full[10:6]=0b11111 + b"\xfc" # Buffer fullness (VBR): buf_full[5:0]=0b111111, num_blocks=0b00 + b"\x00" * 24 # Silent spectral data )Derivation: frame = 7 (header) + 24 (data) = 31 =
0b0000000011111
byte 3 [1:0]= bits[12:11] =00→0x80unchanged ✓byte 4= bits[10:3] =0b00000011=0x03byte 5 [7:5]= bits[2:0] =0b111;[4:0]= VBR (0b11111) →0b11111111=0xFFbyte 6= VBR bits[5:0] + num_blocks →0b11111100=0xFC✓🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py` around lines 51 - 67, SILENT_AAC_FRAME currently encodes the 13-bit ADTS aac_frame_length incorrectly (producing 0); update the header bytes in SILENT_AAC_FRAME so bytes 3–5 contain the properly bit-packed frame_length for a 31-byte frame (7-byte header + 24 bytes payload): change the sequence that is currently b"\x00\x1f" b"\xfc" to use b"\x03\xff" b"\xfc" (i.e., set byte 4 = 0x03 and byte 5 = 0xFF) inside SILENT_AAC_FRAME, and if you ever change the silent payload size update the aac_frame_length packing in SILENT_AAC_FRAME accordingly (or compute it programmatically in _generate_silent_segment).python/packages/jumpstarter-driver-mitmproxy/examples/scenarios/update-available.yaml-33-38 (1)
33-38:⚠️ Potential issue | 🟡 Minor
GET /api/v1/statusappears truncated — missing fields present in the demo variant.Compared to
demo/scenarios/update-available.yaml, this endpoint is missingfirmware_version,uptime_s, andsource. Since this lives underexamples/, users will reference it as a template. Either complete it or add a comment indicating it's intentionally minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/scenarios/update-available.yaml` around lines 33 - 38, The GET /api/v1/status example response is missing fields compared to the demo variant; update the example scenario for the GET /api/v1/status endpoint to include the missing keys (firmware_version, uptime_s, source) with representative values, or alternatively add a clear YAML comment above the response stating this is intentionally minimal and not a full template; target the GET /api/v1/status block in examples/scenarios/update-available.yaml (the endpoint name and body keys) when making the change.python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py-244-261 (1)
244-261:⚠️ Potential issue | 🟡 MinorFix lint failures: unused
frame_intervaland loop variablei.Pipeline is failing due to these lint issues.
frame_intervalis assigned but never used (F841), andiin the loop is unused (B007).Proposed fix
- frame_interval = 1.0 / fps burst_duration_s = 10 # Generate 10 seconds of frames num_frames = int(burst_duration_s * fps) parts = [] - for i in range(num_frames): + for _ in range(num_frames): frame = self._get_frame( camera_id, frames_dir, resolution, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py` around lines 244 - 261, Remove the unused variables to satisfy linters: delete the unused assignment to frame_interval and change the loop to use a throwaway variable; specifically, remove the line setting frame_interval = 1.0 / fps and update the loop "for i in range(num_frames):" to "for _ in range(num_frames):" while keeping the existing body that calls self._get_frame(camera_id, frames_dir, resolution) and appends to parts (boundary, num_frames, parts, and _get_frame are the relevant symbols).python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py-370-388 (1)
370-388:⚠️ Potential issue | 🟡 Minor
restart()cannot explicitly disableweb_ui.Line 381:
restart_web = web_ui or self._web_ui_enabled— passingweb_ui=Falseexplicitly will still pick upself._web_ui_enabledasTrueif the previous run had the web UI enabled. There's no way for a caller to turn it off on restart.Consider using a tri-state (e.g.,
web_ui: bool | None = None) whereNonemeans "keep previous" andFalsemeans "explicitly disable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 370 - 388, The restart method currently can't disable the web UI because restart_web = web_ui or self._web_ui_enabled always falls back to the previous state; change the restart signature to accept a tri-state for web_ui (e.g., web_ui: bool | None = None) and update logic so that if web_ui is None you keep self._web_ui_enabled, otherwise use the explicit boolean value; update the restart() body to compute restart_web = self._web_ui_enabled if web_ui is None else web_ui and ensure the call into start(restart_mode, restart_web, replay_file) and any type hints/use sites are adjusted accordingly.python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py-252-276 (1)
252-276:⚠️ Potential issue | 🟡 Minor
test_assert_request_made_with_wildcardalso relies on uncleaned capture state.Same issue as
test_wait_for_request— without clearing captured requests, theassert_request_made("GET", "/api/v1/*")wildcard could match stale requests from earlier tests (e.g.,GET /api/v1/statusfromtest_capture_context_manager).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py` around lines 252 - 276, The test leaves previous captured requests in proxy_client's state so the wildcard assertion can match stale entries; before running the wildcard assertion in test_assert_request_made_with_wildcard, clear the proxy client's captured request buffer (e.g. call the proxy_client.clear_captured_requests()/clear_requests() method or equivalent) after wait_for_request() and before proxy_client.assert_request_made("GET", "/api/v1/*") so only requests made in this test are considered; ensure you use the existing proxy_client API (methods seen: set_mock, wait_for_request, assert_request_made) to locate where to insert the clear call.python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.py-235-267 (1)
235-267:⚠️ Potential issue | 🟡 Minor
dtis hardcoded to 0.1 s, but the actual push interval is configurable.
dton Line 237 drives battery drain, counter accumulation, GPS drift, and temperature rise, yet it ignores the realinterval_s. Whenpush_interval_msis set to e.g. 500 ms, the physics simulation will run at 5× the intended rate per wall-clock second.Proposed fix
Pass the actual interval into
SensorState(ornext_frame) instead of hardcoding:class SensorState: - def __init__(self, scenario: dict): + def __init__(self, scenario: dict, dt: float = 0.1): self.scenario = scenario self.t0 = time.time() self.frame_num = 0 + self.dt = dt ... def next_frame(self) -> dict: - dt = 0.1 # ~100ms per frame + dt = self.dt elapsed = time.time() - self.t0And in
_push_telemetry:- state = SensorState(scenario) + state = SensorState(scenario, dt=interval_s)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.py` around lines 235 - 267, The physics timestep dt is hardcoded to 0.1s inside SensorState.next_frame(), causing state updates to ignore the configurable push_interval_ms; change next_frame to accept an interval_s parameter (or set an instance attribute on SensorState from _push_telemetry) and use that value instead of the hardcoded dt for battery drain, counter accumulation, GPS drift, temperature, and any other time-based updates. Update all callers (e.g., _push_telemetry) to compute interval_s = push_interval_ms / 1000.0 and pass it into SensorState.next_frame (or set SensorState.interval_s) so the simulation runs with the real push interval. Ensure fallback behavior if interval_s is missing (e.g., default to previous value) to avoid breaking tests.
🧹 Nitpick comments (16)
python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py (1)
30-33:cleanup()is documented in the module docstring but absent from the class.The docstring describes
cleanup()as a reserved lifecycle hook (lines 30–33), but theHandlerclass provides no stub for it. Users copying this template won't know where to add cleanup logic and may miss implementing it when the hook becomes active.✨ Proposed addition
def websocket_message(self, flow: http.HTTPFlow, config: dict): ... + def cleanup(self): + """Called when the addon is unloaded (reserved for future use).""" + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py` around lines 30 - 33, The module docstring mentions a lifecycle hook cleanup() but the Handler class lacks this stub; add a cleanup(self) method to the Handler class (matching the other lifecycle methods' signature/style) that contains a short docstring stating it's a reserved/no-op hook and a no-op body so users know where to put teardown logic when the hook becomes active; ensure the method name is exactly cleanup to match the docstring and follow the same visibility/formatting as the existing methods.python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py (1)
80-81:_segment_cacheis initialized but never read or written — dead code.♻️ Proposed fix
def __init__(self): self._sequence_counters: dict[str, int] = {} - self._segment_cache: bytes | None = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py` around lines 80 - 81, The field _segment_cache on the class is dead code (never read or written); remove the unused attribute declaration or implement proper caching where segments are processed. Locate the class that defines self._segment_cache (the __init__ or attribute block with self._sequence_counters and self._segment_cache) and either delete the self._segment_cache: bytes | None = None line, or if intended, add reads/writes to it in the segment handling methods (e.g., the HLS segment fetch/serve functions) and ensure tests cover the caching behavior.python/packages/jumpstarter-driver-mitmproxy/demo/dut_simulator.py (1)
85-85: Preferurllib.parse.urlsplitover string-splitting for URL path extraction.
url.split(backend)[-1]breaks if the backend host:port string appears anywhere in the path. Usingurllib.parse.urlsplit(url).pathis both more robust and idiomatic.♻️ Proposed refactor
+from urllib.parse import urlsplit + def run_cycle(session: requests.Session, backend: str): ... for method, url in endpoints: - path = url.split(backend)[-1] + path = urlsplit(url).path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/dut_simulator.py` at line 85, The current extraction uses path = url.split(backend)[-1], which fails if the backend host:port appears elsewhere in the URL; replace this with using urllib.parse.urlsplit(url).path to reliably get the path component (and add from urllib.parse import urlsplit if not present). Update the assignment in the same function/scope where path is set (referenced as path and url) to use urlsplit(url).path so path extraction is robust and idiomatic.python/packages/jumpstarter-driver-mitmproxy/pyproject.toml (1)
14-18:testpathsexcludesdemo/— demo tests won't run by default.
testpaths = ["jumpstarter_driver_mitmproxy"]only runs the module-level test suite. Tests underdemo/test_demo.pyandexamples/test_device.pyare silently skipped. If those tests are only intended to run in a HiL environment, this is intentional and a comment here would clarify it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/pyproject.toml` around lines 14 - 18, The pytest configuration's testpaths only includes "jumpstarter_driver_mitmproxy", which excludes demo/ and examples/ so demo/test_demo.py and examples/test_device.py are not run; either add those folders to the testpaths array (e.g., include "demo" and "examples") so they run by default, or keep the current testpaths but add a concise inline comment in the [tool.pytest.ini_options] section stating that demo/ and examples/ tests are intentionally omitted because they require HiL (hardware-in-the-loop) to avoid confusion; update the pyproject.toml entry for testpaths accordingly and ensure the change references the testpaths setting.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py (1)
93-93: Class-level mutable_countersis shared across allTemplateEngineuses.
_countersis a class attribute (defaultdict(int)), meaning all callers share the same counter state. If there's only ever oneTemplateEngineused as a classmethod-based singleton, this is fine — but if this module is ever loaded in multiple contexts (e.g., tests), counters will leak between them. If this is intentional, a brief comment would clarify the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` at line 93, The class-level mutable _counters (defaultdict(int)) on TemplateEngine leaks state across uses; change it to be instance-scoped or explicitly reset so each TemplateEngine gets its own counters (e.g., initialize self._counters = defaultdict(int) in TemplateEngine.__init__ or lazy-initialize per-instance), or if shared behavior is intentional add a clarifying comment above the _counters declaration; update references to use self._counters and remove reliance on the class attribute.python/packages/jumpstarter-driver-mitmproxy/demo/conftest.py (2)
81-86:http_sessiononly configures an HTTP proxy — HTTPS requests will bypass the proxy.If any test exercises HTTPS endpoints, traffic will go direct rather than through
mitmproxy. Consider adding the HTTPS proxy and disabling certificate verification (or trusting the mitmproxy CA) for full coverage.♻️ Proposed enhancement
def http_session(): """Requests session pre-configured to route through the proxy.""" session = requests.Session() - session.proxies = {"http": f"http://127.0.0.1:{PROXY_PORT}"} + session.proxies = { + "http": f"http://127.0.0.1:{PROXY_PORT}", + "https": f"http://127.0.0.1:{PROXY_PORT}", + } + session.verify = False # Trust mitmproxy's CA for demo tests return session🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/conftest.py` around lines 81 - 86, The http_session fixture currently only sets the "http" proxy so HTTPS traffic bypasses mitmproxy; update the http_session fixture (requests.Session in http_session) to also set "https": f"http://127.0.0.1:{PROXY_PORT}" in session.proxies and either disable cert verification for tests (session.verify = False) or configure the session to trust the mitmproxy CA, so HTTPS requests are routed through and accepted by mitmproxy.
55-68:proxyfixture doesn't declarebackend_serveras a dependency.If tests rely on the proxy forwarding to the backend in passthrough mode, the backend may not be started. If mock-mode tests never need the backend, this is fine — but it would be safer to add an explicit dependency or document the expectation.
♻️ Option: add backend_server dependency
`@pytest.fixture`(scope="session") -def proxy(client): +def proxy(client, backend_server):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/conftest.py` around lines 55 - 68, The proxy fixture currently starts the mitmproxy driver but does not declare backend_server as a pytest dependency, which can leave the backend unstarted for tests that use passthrough; update the proxy fixture signature (the pytest fixture named proxy) to accept backend_server as a parameter (or explicitly call/start backend_server inside proxy) so the backend is ensured to be running when proxy.start("mock") is invoked, or alternatively add a clear docstring note in the proxy fixture indicating tests run in mock mode must not rely on a backend; reference the proxy fixture and backend_server symbol to locate where to add the dependency or documentation.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py (1)
374-376: Redundant local import ofPath.
Pathis already imported at module level (line 42). The local import on line 375 is unnecessary.Proposed fix
def cert_cmd(output: str): ... - from pathlib import Path - pem = self.get_ca_cert() out = Path(output)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py` around lines 374 - 376, Remove the redundant local import "from pathlib import Path" inside the function/block and use the module-level Path that is already imported at the top of the file (module import at line 42); locate the local import statement in jumpstarter_driver_mitmproxy.client.py (the inner "from pathlib import Path") and delete it so code continues referring to Path directly without re-importing.python/packages/jumpstarter-driver-mitmproxy/examples/conftest.py (1)
113-121: Docstring/name mismatch:mock_slow_backendreturns 504 instantly.The fixture name suggests simulating slowness, and the docstring says "gateway timeout", but it returns a 504 response immediately without any latency. For a slow-backend simulation, consider using
set_mock_with_latency. As-is, this is functionally identical to a simple error mock. Since this is an example file, it may confuse users who adopt it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/conftest.py` around lines 113 - 121, The fixture mock_slow_backend and its docstring claim to simulate a slow backend but currently use proxy.mock_endpoint returning a 504 immediately; replace or update it to actually introduce latency (e.g., use proxy.set_mock_with_latency or the equivalent API) so the endpoint both delays the response and then returns a 504 (or update the name/docstring to reflect an immediate 504 if you intend to keep no latency). Locate the mock_slow_backend fixture and change the proxy.mock_endpoint usage to the latency-aware helper (set_mock_with_latency) with the same method/path and status/body, or rename the fixture/docstring to indicate an immediate 504 error.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver_integration_test.py (1)
134-142: Minor flakiness risk: no_wait_for_portbefore status check in passthrough test.
test_start_passthrough_modedoesn't call_wait_for_portafterclient.start("passthrough"), unlike other tests. Ifstatus()is called before mitmdump is fully up,runningcould still beTrue(based on PID), but the test style is inconsistent with the rest of the suite.Suggested fix
def test_start_passthrough_mode(self, client): result = client.start("passthrough") assert "passthrough" in result + # Wait for proxy to be ready (consistent with other tests) status = client.status()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver_integration_test.py` around lines 134 - 142, In test_start_passthrough_mode add the same readiness wait used elsewhere: after calling client.start("passthrough") call the helper _wait_for_port (or the test-suite's equivalent readiness helper) before invoking client.status(); this ensures mitmdump is fully accepting connections so status() reflects the running/ready state consistently—update test_start_passthrough_mode to call _wait_for_port (or the existing wait helper) between client.start and client.status and keep client.stop() as-is.python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py (1)
294-296: Acceptfiles_basefrom addon config instead of hardcoding the path.
/opt/jumpstarter/mitmproxy/mock-filesis hardcoded rather than being derived from addon config. Since thehandle()method already receives a config dict and extracts other parameters likeframes_dir,fps, andresolution, the base path should also be configurable. This would allow different deployments to override the mock-files directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py` around lines 294 - 296, The code currently hardcodes files_base = Path("/opt/jumpstarter/mitmproxy/mock-files") inside the addon's handle() method; change it to read files_base (or mock_files_base) from the addon config dictionary alongside frames_dir/fps/resolution, defaulting to the current path if not provided, and then use that variable when constructing frame_dir = files_base / frames_dir so deployments can override the mock-files directory via config.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py (3)
52-63: Default bind address0.0.0.0exposes the proxy and web UI to all network interfaces.Both
ListenConfigandWebConfigdefault to0.0.0.0, which exposes the mitmproxy listener and the mitmweb UI (with its default password) to the network. For a proxy that intercepts HTTPS traffic, this broadens the attack surface unnecessarily. Consider defaulting to127.0.0.1and letting users opt in to broader binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 52 - 63, Defaulting ListenConfig.host and WebConfig.host to "0.0.0.0" exposes mitmproxy and mitmweb to all network interfaces; change the defaults in the ListenConfig and WebConfig classes so host defaults to "127.0.0.1" instead, and update any related docs/tests that assume the old default to make it explicit that users can opt in to broader binding if needed (search for ListenConfig and WebConfig to locate usages).
1180-1194: String replacement to patch paths in bundled addon is brittle.
str.replace()will substitute every occurrence of the default path strings in the bundled addon source, not just the intended configuration constants. If those strings appear in comments, docstrings, or error messages, they'll be silently corrupted.A safer approach: use sentinel markers (e.g.
__MOCK_DIR_PLACEHOLDER__) in the bundled addon that are unlikely to appear elsewhere, or write a small JSON sidecar config that the addon reads at startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1180 - 1194, The current approach mutates bundled_addon.py by blind string replacement (see variables bundled, path, self.directories.mocks, and self._capture_socket_path) which can corrupt unintended text; instead modify the bundled addon to use distinct sentinel placeholders (e.g. __MOCK_DIR_PLACEHOLDER__ and __CAPTURE_SOCKET_PLACEHOLDER__) or load a small JSON sidecar config at runtime, then update this driver to only replace those sentinel tokens (or write the sidecar file next to the addon) so replacements are deterministic and cannot affect comments/docstrings or other literals.
1023-1037: Accept loop overwrites_capture_reader_thread— only the last connection is joinable on shutdown.If the addon reconnects (or multiple addons connect), each accepted connection spawns a new thread (Line 1028-1033), but only the latest reference is stored in
self._capture_reader_thread. Previous reader threads become unjoinable, leaking thread objects on_stop_capture_server.Consider storing reader threads in a list and joining them all on shutdown.
Proposed fix
- _capture_reader_thread: threading.Thread | None = field( - default=None, init=False, repr=False - ) + _capture_reader_threads: list = field( + default_factory=list, init=False, repr=False + )In
_capture_accept_loop:- self._capture_reader_thread = threading.Thread( + t = threading.Thread( target=self._capture_read_loop, args=(conn,), daemon=True, ) - self._capture_reader_thread.start() + t.start() + self._capture_reader_threads.append(t)In
_stop_capture_server:- if self._capture_reader_thread is not None: - self._capture_reader_thread.join(timeout=5) - self._capture_reader_thread = None + for t in self._capture_reader_threads: + t.join(timeout=5) + self._capture_reader_threads.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1023 - 1037, The accept loop in _capture_accept_loop stores each new reader thread only in self._capture_reader_thread, so earlier threads cannot be joined and leak; change this to append each new Thread to a list attribute (e.g., self._capture_reader_threads) when created, ensure the list is initialized where the server starts, and in _stop_capture_server iterate over and join all threads from self._capture_reader_threads (optionally with a timeout) and clear the list; if concurrent access is possible, protect append/iterate with the same lock used for server state.python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.py (1)
155-177: Cancelled tasks are not removed from_tasksonset_scenario.On
unsubscribe/stop(Line 158) the entry is deleted, butset_scenario(Line 165) cancels without deleting before overwriting. This is functionally fine since Line 176 overwrites the entry, but there's a subtler gap: if the old task'sCancelledErrorhandler (Line 202-203) runs after the new task is stored, there's no conflict — though for consistency you coulddelbefore reassigning.More importantly, if a flow disconnects without an unsubscribe, the
_tasksentry and the completedasyncio.Taskobject are never cleaned up (the entry lingers forever). For a long-lived exporter this is a slow memory leak. Consider cleaning up in_push_telemetry's finally block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.py` around lines 155 - 177, The problem: cancelled or completed asyncio.Tasks stored in self._tasks (set_scenario handler and on disconnect) are never removed, causing a memory leak; fix by ensuring tasks are deleted from self._tasks when they are cancelled or finish. Update the set_scenario branch in the message handler to delete the old task entry (del self._tasks[flow_id]) before creating/assigning the new task, and modify _push_telemetry to remove its own entry from self._tasks in a finally block (or on CancelledError/normal exit) using the flow's id so the task is cleaned up when the websocket disconnects or the task completes.python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py (1)
70-132: Pervasivetime.sleep(1)for mock hot-reload is fragile and slow.Every test that sets a mock follows with
time.sleep(1)to wait for the addon's file-mtime polling. This makes the suite slow (≥6 s of pure sleep) and racy on busy CI where file-system timestamps or scheduling can lag.Consider adding a polling helper that verifies the mock is active (e.g., a lightweight sentinel request) rather than relying on a fixed sleep. Even in demo code, this would make the tests more reliable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py` around lines 70 - 132, Replace the fragile time.sleep(1) calls with a polling helper that actively verifies mock activation/removal: implement a helper (e.g., wait_for_mock_state(method, path, http_session, *, expect_active=True, timeout=5, interval=0.1)) that repeatedly issues the sentinel request (http_session.get("http://example.com" + path)) until the response matches the expected state (status/body for active or a non-mock response for removed) or times out; call this helper after proxy_client.set_mock and inside/after proxy_client.mock_endpoint to wait for activation and after context exit to wait for removal, and remove all time.sleep calls in tests like test_mock_replaces_real_response, test_mock_error_status, and test_mock_endpoint_context_manager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py`:
- Around line 213-228: The code currently hardcodes files_dir =
Path("/opt/jumpstarter/mitmproxy/mock-files"); change it to read the base
directory from the addon's configuration (e.g. config or addon_config key
"files_dir") with a sensible fallback to the existing path, and use that Path
object wherever files_dir is referenced (including the segment lookup block that
builds seg_path using segments_dir, channel_id, and resource); ensure the
driver/scenario loader supplies "files_dir" when registering the addon
(alongside segments_dir) so the addon uses the configured data directory rather
than the hardcoded path.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 161-163: The current env(...) template branch in bundled_addon.py
(the branch that calls cls._parse_args and returns os.environ.get(...)) can leak
secrets; change it to enforce an allowlist of permitted variable names (e.g.,
define ALLOWED_ENV_VARS set and check args[0] against it) and only return the
environment value when the variable is in that allowlist; otherwise return an
empty string. Also add a log entry (using the module/class logger referenced in
this file) whenever env() is invoked: log a warning that env() was used and
whether the variable was allowed or blocked to aid auditing. Ensure you update
the branch that currently does: elif expr.startswith("env("): ... so it performs
the allowlist check and logging before returning any value.
- Around line 133-197: Refactor the large branching logic in _evaluate into a
dispatch dictionary mapping expression prefixes/keys to small handler callables
(e.g., handlers keyed by "now_iso", "random_int(", "request_header(", etc.)
where each handler has signature like handler(expr, flow, state) and lives as a
private `@classmethod`; replace the long if/elif chain with a lookup that falls
back to logging the unknown expression. For _matches_conditions, extract each
condition check into private methods named _match_headers,
_match_absent_headers, _match_query, _match_body_contains, and _match_body_json
that take (endpoint, flow) or (conditions, flow) as appropriate and return bool,
then have _matches_conditions call those helpers sequentially. For
_find_endpoint, pull the wildcard matching loop into a helper named
_wildcard_match(endpoint_pattern, path) (or similar) and call it from
_find_endpoint so the method delegating matching becomes simple. Update
references to class attributes like _counters and existing helper
_parse_args/_resolve_dotted_path accordingly.
- Around line 243-248: The try block that calls
importlib.util.spec_from_file_location may get a None spec or a spec with
loader==None, so before calling spec.loader.exec_module(module) (and before
using spec to create a module), add a guard: check that spec is not None and
spec.loader is not None; if either is None, raise or log a clear
ImportError/RuntimeError (including script_path/name) and skip loading that
addon. Update the code around the importlib.util.spec_from_file_location /
module = importlib.util.module_from_spec(spec) / spec.loader.exec_module(module)
sequence to perform these checks and handle the failure path safely.
- Around line 749-757: The current path traversal guard using
str(file_path).startswith(str(files_dir_resolved)) is unsafe; replace that
prefix check with Path.is_relative_to to ensure file_path is actually inside
self.files_dir (you can use file_path.is_relative_to(files_dir_resolved) since
the package targets Python 3.11+), and keep the existing behavior of logging via
ctx.log.error and returning None when the check fails; update the variables
referenced (file_path, files_dir_resolved, self.files_dir) accordingly.
- Around line 602-608: The request hook is synchronous and uses time.sleep,
which blocks the mitmproxy event loop; change the hook definition from def
request(self, flow) to async def request(self, flow) and replace
time.sleep(latency_ms / 1000.0) with await asyncio.sleep(latency_ms / 1000.0);
ensure you import asyncio at the top and preserve computing latency_ms from
endpoint/get and the surrounding logic so the addon manager will await the
coroutine without blocking other requests.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 260-269: The CLI currently resolves the scenario path client-side
in scenario_load_cmd by calling Path(scenario_file).resolve() before passing it
to load_mock_scenario, which breaks remote exporter usage; change
scenario_load_cmd to pass the raw scenario_file argument through unchanged (do
not call Path.resolve or convert to an absolute client path) so
load_mock_scenario receives the original filename the driver expects (relative
to mock_dir or an exporter-side absolute path).
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 189-319: The start() method is too complex; extract mode-specific
command construction and startup/validation/message logic into helpers to reduce
cyclomatic complexity. Create helper methods _build_mock_cmd(cmd: list) which
calls _load_startup_mocks(), _write_mock_config(), generates addon_path and
_generate_default_addon() and appends "-s <addon>", _build_record_cmd(cmd: list)
which computes timestamp, sets flow_file, appends "-w <flow_file>" and sets
self._current_flow_file, and _build_replay_cmd(cmd: list, replay_file: str)
which resolves replay_path against self.directories.flows, checks existence (and
stops capture server if missing) and appends "--server-replay" flags; also
extract startup validation (mode checks and replay_file requirement) and final
message construction into small helpers like _validate_start_params(mode,
replay_file) and _format_start_message(mode, web_ui) so start() only
orchestrates flow, calls _start_capture_server(), builds base cmd, delegates to
the _build_* helpers, launches subprocess, and handles startup failure/cleanup.
- Around line 286-299: The subprocess is created with stdout=PIPE and
stderr=PIPE but those pipes are never drained, risking deadlock; change the
Popen call in the code that sets self._process to either redirect output to
subprocess.DEVNULL (for stdout and/or stderr) or keep only stderr as PIPE for
the startup check and then spawn a daemon thread (e.g. assign to
self._stderr_thread) that continuously reads and discards self._process.stderr
after confirming startup; ensure any spawned reader thread also handles stdout
(or stdout is redirected to DEVNULL) and that _stop_capture_server still cleans
up/joins the thread when self._process is set to None.
- Around line 127-150: The fields listen, web, and directories are annotated as
dict but are replaced with ListenConfig/WebConfig/DirectoriesConfig instances in
__post_init__, causing static type errors; update their type annotations to
reflect the real types (either ListenConfig/WebConfig/DirectoriesConfig or
Union[dict, ListenConfig] etc.), then modify __post_init__ to only call
ListenConfig.model_validate(self.listen) (and equivalents) when the current
value is a dict (preserve instances if already the proper model), and keep the
existing default_factory=dict behavior to allow YAML dict input; reference the
listen/web/directories fields and the __post_init__ method and use
ListenConfig.model_validate, WebConfig.model_validate,
DirectoriesConfig.model_validate to perform the conversion when needed.
In `@python/packages/jumpstarter-driver-mitmproxy/README.md`:
- Line 1: Add the required docs symlink for the mitmproxy driver by creating a
symbolic link named mitmproxy.md inside
python/docs/source/reference/package-apis/drivers/ that points to the package
README; specifically create the symlink mitmproxy.md ->
../../../../../packages/jumpstarter-driver-mitmproxy/README.md so the docs
reference the README.md in the jumpstarter-driver-mitmproxy package (verify the
link targets README.md in the repository root package and commit the new
symlink).
---
Nitpick comments:
In `@python/packages/jumpstarter-driver-mitmproxy/demo/conftest.py`:
- Around line 81-86: The http_session fixture currently only sets the "http"
proxy so HTTPS traffic bypasses mitmproxy; update the http_session fixture
(requests.Session in http_session) to also set "https":
f"http://127.0.0.1:{PROXY_PORT}" in session.proxies and either disable cert
verification for tests (session.verify = False) or configure the session to
trust the mitmproxy CA, so HTTPS requests are routed through and accepted by
mitmproxy.
- Around line 55-68: The proxy fixture currently starts the mitmproxy driver but
does not declare backend_server as a pytest dependency, which can leave the
backend unstarted for tests that use passthrough; update the proxy fixture
signature (the pytest fixture named proxy) to accept backend_server as a
parameter (or explicitly call/start backend_server inside proxy) so the backend
is ensured to be running when proxy.start("mock") is invoked, or alternatively
add a clear docstring note in the proxy fixture indicating tests run in mock
mode must not rely on a backend; reference the proxy fixture and backend_server
symbol to locate where to add the dependency or documentation.
In `@python/packages/jumpstarter-driver-mitmproxy/demo/dut_simulator.py`:
- Line 85: The current extraction uses path = url.split(backend)[-1], which
fails if the backend host:port appears elsewhere in the URL; replace this with
using urllib.parse.urlsplit(url).path to reliably get the path component (and
add from urllib.parse import urlsplit if not present). Update the assignment in
the same function/scope where path is set (referenced as path and url) to use
urlsplit(url).path so path extraction is robust and idiomatic.
In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py`:
- Around line 70-132: Replace the fragile time.sleep(1) calls with a polling
helper that actively verifies mock activation/removal: implement a helper (e.g.,
wait_for_mock_state(method, path, http_session, *, expect_active=True,
timeout=5, interval=0.1)) that repeatedly issues the sentinel request
(http_session.get("http://example.com" + path)) until the response matches the
expected state (status/body for active or a non-mock response for removed) or
times out; call this helper after proxy_client.set_mock and inside/after
proxy_client.mock_endpoint to wait for activation and after context exit to wait
for removal, and remove all time.sleep calls in tests like
test_mock_replaces_real_response, test_mock_error_status, and
test_mock_endpoint_context_manager.
In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/_template.py`:
- Around line 30-33: The module docstring mentions a lifecycle hook cleanup()
but the Handler class lacks this stub; add a cleanup(self) method to the Handler
class (matching the other lifecycle methods' signature/style) that contains a
short docstring stating it's a reserved/no-op hook and a no-op body so users
know where to put teardown logic when the hook becomes active; ensure the method
name is exactly cleanup to match the docstring and follow the same
visibility/formatting as the existing methods.
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/data_stream_websocket.py`:
- Around line 155-177: The problem: cancelled or completed asyncio.Tasks stored
in self._tasks (set_scenario handler and on disconnect) are never removed,
causing a memory leak; fix by ensuring tasks are deleted from self._tasks when
they are cancelled or finish. Update the set_scenario branch in the message
handler to delete the old task entry (del self._tasks[flow_id]) before
creating/assigning the new task, and modify _push_telemetry to remove its own
entry from self._tasks in a finally block (or on CancelledError/normal exit)
using the flow's id so the task is cleaned up when the websocket disconnects or
the task completes.
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py`:
- Around line 80-81: The field _segment_cache on the class is dead code (never
read or written); remove the unused attribute declaration or implement proper
caching where segments are processed. Locate the class that defines
self._segment_cache (the __init__ or attribute block with
self._sequence_counters and self._segment_cache) and either delete the
self._segment_cache: bytes | None = None line, or if intended, add reads/writes
to it in the segment handling methods (e.g., the HLS segment fetch/serve
functions) and ensure tests cover the caching behavior.
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py`:
- Around line 294-296: The code currently hardcodes files_base =
Path("/opt/jumpstarter/mitmproxy/mock-files") inside the addon's handle()
method; change it to read files_base (or mock_files_base) from the addon config
dictionary alongside frames_dir/fps/resolution, defaulting to the current path
if not provided, and then use that variable when constructing frame_dir =
files_base / frames_dir so deployments can override the mock-files directory via
config.
In `@python/packages/jumpstarter-driver-mitmproxy/examples/conftest.py`:
- Around line 113-121: The fixture mock_slow_backend and its docstring claim to
simulate a slow backend but currently use proxy.mock_endpoint returning a 504
immediately; replace or update it to actually introduce latency (e.g., use
proxy.set_mock_with_latency or the equivalent API) so the endpoint both delays
the response and then returns a 504 (or update the name/docstring to reflect an
immediate 504 if you intend to keep no latency). Locate the mock_slow_backend
fixture and change the proxy.mock_endpoint usage to the latency-aware helper
(set_mock_with_latency) with the same method/path and status/body, or rename the
fixture/docstring to indicate an immediate 504 error.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Line 93: The class-level mutable _counters (defaultdict(int)) on
TemplateEngine leaks state across uses; change it to be instance-scoped or
explicitly reset so each TemplateEngine gets its own counters (e.g., initialize
self._counters = defaultdict(int) in TemplateEngine.__init__ or lazy-initialize
per-instance), or if shared behavior is intentional add a clarifying comment
above the _counters declaration; update references to use self._counters and
remove reliance on the class attribute.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 374-376: Remove the redundant local import "from pathlib import
Path" inside the function/block and use the module-level Path that is already
imported at the top of the file (module import at line 42); locate the local
import statement in jumpstarter_driver_mitmproxy.client.py (the inner "from
pathlib import Path") and delete it so code continues referring to Path directly
without re-importing.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver_integration_test.py`:
- Around line 134-142: In test_start_passthrough_mode add the same readiness
wait used elsewhere: after calling client.start("passthrough") call the helper
_wait_for_port (or the test-suite's equivalent readiness helper) before invoking
client.status(); this ensures mitmdump is fully accepting connections so
status() reflects the running/ready state consistently—update
test_start_passthrough_mode to call _wait_for_port (or the existing wait helper)
between client.start and client.status and keep client.stop() as-is.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 52-63: Defaulting ListenConfig.host and WebConfig.host to
"0.0.0.0" exposes mitmproxy and mitmweb to all network interfaces; change the
defaults in the ListenConfig and WebConfig classes so host defaults to
"127.0.0.1" instead, and update any related docs/tests that assume the old
default to make it explicit that users can opt in to broader binding if needed
(search for ListenConfig and WebConfig to locate usages).
- Around line 1180-1194: The current approach mutates bundled_addon.py by blind
string replacement (see variables bundled, path, self.directories.mocks, and
self._capture_socket_path) which can corrupt unintended text; instead modify the
bundled addon to use distinct sentinel placeholders (e.g.
__MOCK_DIR_PLACEHOLDER__ and __CAPTURE_SOCKET_PLACEHOLDER__) or load a small
JSON sidecar config at runtime, then update this driver to only replace those
sentinel tokens (or write the sidecar file next to the addon) so replacements
are deterministic and cannot affect comments/docstrings or other literals.
- Around line 1023-1037: The accept loop in _capture_accept_loop stores each new
reader thread only in self._capture_reader_thread, so earlier threads cannot be
joined and leak; change this to append each new Thread to a list attribute
(e.g., self._capture_reader_threads) when created, ensure the list is
initialized where the server starts, and in _stop_capture_server iterate over
and join all threads from self._capture_reader_threads (optionally with a
timeout) and clear the list; if concurrent access is possible, protect
append/iterate with the same lock used for server state.
In `@python/packages/jumpstarter-driver-mitmproxy/pyproject.toml`:
- Around line 14-18: The pytest configuration's testpaths only includes
"jumpstarter_driver_mitmproxy", which excludes demo/ and examples/ so
demo/test_demo.py and examples/test_device.py are not run; either add those
folders to the testpaths array (e.g., include "demo" and "examples") so they run
by default, or keep the current testpaths but add a concise inline comment in
the [tool.pytest.ini_options] section stating that demo/ and examples/ tests are
intentionally omitted because they require HiL (hardware-in-the-loop) to avoid
confusion; update the pyproject.toml entry for testpaths accordingly and ensure
the change references the testpaths setting.
python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Outdated
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,392 @@ | |||
| # jumpstarter-driver-mitmproxy | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for the symlink in the docs directory
echo "=== Checking for mitmproxy symlink in docs ==="
ls -la python/docs/source/reference/package-apis/drivers/ 2>/dev/null | grep -i mitmproxy || echo "No mitmproxy symlink found"
# Check the actual directory structure of the driver package
echo -e "\n=== Driver package structure ==="
find python/packages/jumpstarter-driver-mitmproxy -type f -name "*.md" -o -name "Dockerfile" -o -name "README*" | head -20
# Check for all symlinks in the drivers reference directory
echo -e "\n=== All symlinks in drivers reference directory ==="
ls -la python/docs/source/reference/package-apis/drivers/ 2>/dev/null || echo "Directory not found"
# Check pyproject.toml for entry points and other requirements
echo -e "\n=== Driver package pyproject.toml ==="
cat python/packages/jumpstarter-driver-mitmproxy/pyproject.toml 2>/dev/null | head -50Repository: jumpstarter-dev/jumpstarter
Length of output: 5292
Add required docs symlink for mitmproxy driver.
The coding guidelines require a symlink in python/docs/source/reference/package-apis/drivers/mitmproxy.md pointing to this README. Create the symlink following the pattern used by other drivers:
mitmproxy.md -> ../../../../../packages/jumpstarter-driver-mitmproxy/README.md
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-mitmproxy/README.md` at line 1, Add the
required docs symlink for the mitmproxy driver by creating a symbolic link named
mitmproxy.md inside python/docs/source/reference/package-apis/drivers/ that
points to the package README; specifically create the symlink mitmproxy.md ->
../../../../../packages/jumpstarter-driver-mitmproxy/README.md so the docs
reference the README.md in the jumpstarter-driver-mitmproxy package (verify the
link targets README.md in the repository root package and commit the new
symlink).
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py (4)
390-402:files_dirinitialized with..relative path — fragile default.Line 393 sets
self.files_dir = Path(self.MOCK_DIR) / "../mock-files", which produces an un-resolved path containing... This works at runtime because_load_configoverwrites it from the config file, and_read_filecalls.resolve(). However, if_load_configfails or config has nofiles_dir, the..stays in the path, and the path traversal check in_read_file(which compares resolved paths) may behave unexpectedly depending on whetherMOCK_DIRitself contains symlinks.Use
.parent / "mock-files"instead of/ "../mock-files"for clarity and to avoid resolution surprises.Proposed fix
- self.files_dir: Path = Path(self.MOCK_DIR) / "../mock-files" + self.files_dir: Path = Path(self.MOCK_DIR).parent / "mock-files"Also apply at Line 434:
- self.files_dir = Path(self.MOCK_DIR).parent / "mock-files" + self.files_dir = Path(self.MOCK_DIR).parent / "mock-files"(Line 434 already uses
.parent— only Line 393 needs fixing.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` around lines 390 - 402, The default files_dir in __init__ is set using a "../" relative path which can leave unresolved ".." components and cause the path-traversal check in _read_file to behave unpredictably; change the initialization of self.files_dir in __init__ to construct the directory from Path(self.MOCK_DIR).parent / "mock-files" (same style already used elsewhere) so the path has the correct parent-based form before _load_config or _read_file run, leaving _load_config and _read_file unchanged.
326-340:CaptureClient.send_eventsilently drops events on persistent socket failures.If the Unix socket is unavailable (driver not running or socket path mismatch), all capture events are silently lost. This is documented behavior ("events are silently dropped"), but for debugging test failures it can be confusing when
wait_for_requesttimes out with no indication that the capture pipeline is broken. Consider logging a warning on the first failed connection attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` around lines 326 - 340, send_event currently drops events silently when the Unix socket is unavailable; modify CaptureClient.send_event to log a warning on the first failed connection attempt so failures are visible during tests: when _connect() returns False or when the first OSError is caught, emit a single warning (using the instance logger or module logger) that includes the socket path and that events will be dropped, then proceed with the existing reconnect/return logic; reference the send_event method, the _connect() call, the self._sock check, and the close() call to locate where to insert the warning.
619-653:requesthook:_find_endpointcalls_load_config()on every request, plus_load_state()at Line 621.Both
_load_configand_load_stateperform astat()syscall per invocation to check file modification time. In a test environment this is acceptable, but for higher-traffic scenarios the two stat calls per request could become a bottleneck. A short cooldown (e.g., only check every N seconds) would reduce the overhead without meaningfully delaying config hot-reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` around lines 619 - 653, The request hook calls _find_endpoint (which calls _load_config()) and also calls _load_state() on every request causing two stat() syscalls per request; add a short cooldown to both _load_config and _load_state so they only perform their file mtime stat when enough seconds have passed since the last check. Implement per-instance fields like _last_config_check, _last_state_check and a configurable _config_check_interval (seconds); on first call always load, otherwise compare time.time() to last check and skip the stat/syscall if inside the cooldown window, updating the last check time and reloading only when the interval has elapsed so request(), _find_endpoint(), _load_config(), and _load_state() retain hot-reload behavior without calling stat() every request.
302-302:CAPTURE_SOCKETis a hardcoded module-level constant but the driver generates dynamic socket paths.The driver's
_start_capture_serverpicks a socket path dynamically (preferred path or temp file), then patches this constant via string replacement in_generate_default_addon. If the bundled addon is loaded outside the driver's copy mechanism (e.g., directly viamitmdump -s bundled_addon.py), it will use the hardcoded/opt/jumpstarter/mitmproxy/capture.sockpath which may not match the driver's actual socket. Consider reading the socket path from an environment variable as a fallback to decouple the addon from the driver's patching step.Proposed fix
-CAPTURE_SOCKET = "/opt/jumpstarter/mitmproxy/capture.sock" +CAPTURE_SOCKET = os.environ.get( + "JMP_CAPTURE_SOCKET", "/opt/jumpstarter/mitmproxy/capture.sock" +)Then in the driver's
_generate_default_addon, set the env var instead of (or in addition to) string-patching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` at line 302, The module-level CAPTURE_SOCKET constant is hardcoded which breaks when the addon is run standalone; change the addon to read the socket path from an environment variable fallback (e.g., os.environ.get("JUMPSTARTER_CAPTURE_SOCKET", "/opt/jumpstarter/mitmproxy/capture.sock")) instead of relying solely on the literal CAPTURE_SOCKET, and update the driver's _generate_default_addon to set that environment variable (or continue patching but prefer setting JUMPSTARTER_CAPTURE_SOCKET) so _start_capture_server and other functions use the env var-derived path when present.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py (3)
1149-1171: Non-atomic write toendpoints.json— addon may read a partial file.
_write_mock_configand_write_stateopen the target file and write directly. If the mitmproxy addon hot-reloads at the same moment, it could read truncated JSON. The addon's_load_configcatches the exception and skips the update, so this won't crash, but it silently drops a config change until the next trigger.Write to a temp file in the same directory and
os.replace()for an atomic swap.Proposed fix for _write_mock_config
def _write_mock_config(self): """Write mock endpoint definitions to disk in v2 format.""" mock_path = Path(self.directories.mocks) mock_path.mkdir(parents=True, exist_ok=True) config_file = mock_path / "endpoints.json" v2_config = { "config": { "files_dir": self.directories.files, "addons_dir": self.directories.addons, "default_latency_ms": 0, "default_content_type": "application/json", }, "endpoints": self._mock_endpoints, } - with open(config_file, "w") as f: - json.dump(v2_config, f, indent=2) + tmp_file = config_file.with_suffix(".json.tmp") + with open(tmp_file, "w") as f: + json.dump(v2_config, f, indent=2) + os.replace(tmp_file, config_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1149 - 1171, Replace the non-atomic direct write in _write_mock_config by writing the JSON to a temp file in the same directory (use tempfile.NamedTemporaryFile or mkstemp with dir=mock_path and delete=False), flush and fsync the temp file, close it, then atomically swap it into place with os.replace(temp_path, config_file); also apply the same pattern to _write_state so both writes are atomic; ensure you import os and tempfile and maintain the same file permissions/ownership if needed.
262-284: Hardcoded web UI password"jumpstarter"embedded in command args and status URLs.Line 278 sets
web_password=jumpstarterand Line 331 embeds?token=jumpstarterin the returned URL. While this is a dev/test tool, a fixed credential means anyone on the network can access the mitmweb UI. Consider making this configurable (e.g., a field onWebConfig) or generating a random token per session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 262 - 284, The code currently hardcodes the mitmweb UI password ("jumpstarter") in _build_base_cmd and embeds the same token in the returned status URL; change this to a configurable or per-session secret by adding a web_password (or token) field to WebConfig (or generate a secure random token at session start), then use that value when building the command in _build_base_cmd (replace the literal "jumpstarter" with the config/token) and when composing the status URL (replace the hardcoded ?token=jumpstarter); ensure the token is generated with a cryptographically secure RNG, stored only in memory for the session, and not logged.
1187-1297: Fallback addon generated via f-string interpolation of directory paths into Python source — fragile and injection-prone.
_generate_default_addonembedsself.directories.mocksdirectly into a Python source string. If the path contains characters like",\, or{, the generated script will have syntax errors or unexpected behavior. The bundled addon path (Lines 1198-1214) is the primary path and uses string replacement which is slightly safer but still susceptible to the same issue. Since these are typically controlled server-side paths, the practical risk is low, but consider writing a small JSON config file alongside the addon instead of string-patching paths into source code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1187 - 1297, The fallback and bundled addon installation in _generate_default_addon currently injects self.directories.mocks and self._capture_socket_path directly into generated Python source (and does string.replace on bundled_addon.py), which is fragile and injection-prone; instead, write a small JSON config (e.g., {"mocks": "...", "capture_socket": "..."}) next to the addon and change the addon to read that config at runtime (referenced symbols: _generate_default_addon, bundled_addon.py, MitmproxyMockAddon.MOCK_DIR, and self._capture_socket_path) — update the bundled-copy branch to drop a config JSON rather than doing string.replace, and change the inline addon template to load paths from that config file using json.load at startup.python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py (2)
294-294: Hardcoded base path/opt/jumpstarter/mitmproxy/mock-filesis not configurableThe base files path is a magic constant baked into the method. It silently produces no frames (falling back to generated test patterns) if the addon is deployed at a different path, with no indication of why. Consider reading this from
configor an environment variable so the example demonstrates the pattern correctly.♻️ Suggested approach — read from
config- files_base = Path("/opt/jumpstarter/mitmproxy/mock-files") + files_base = Path(config.get("mock_files_base", "/opt/jumpstarter/mitmproxy/mock-files"))Note:
configwould need to be threaded through to_get_frameas a parameter if this approach is taken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py` at line 294, The hardcoded files_base Path("/opt/jumpstarter/mitmproxy/mock-files") should be made configurable: read the base path from an injected config or an environment variable (e.g., os.environ["MJPEG_MOCK_FILES"] with a sensible default) and use that value instead of the literal; if _get_frame is a helper method, add a parameter or capture the config on the addon instance (e.g., self.files_base) so _get_frame can reliably locate mock files and return a clear warning/log when the directory is missing.
248-263:.encode()applied to adjacent f-string group — correct but visually misleadingAdjacent f-strings and regular strings are concatenated; regular strings at compile time and f-strings at run time. Each f-string is entirely evaluated before being concatenated to adjacent f-strings. Per Python's
STRING+grammar production, the four f-strings at lines 254-257 form one atom and.encode()is applied to the whole combined string — noTypeErrorat runtime. However, it appears visually as if only the finalf"\r\n"is encoded, making the code easy to misread during maintenance. Use explicit parentheses to make the intent unambiguous.♻️ Proposed fix — explicit grouping
parts.append( - f"--{boundary}\r\n" - f"Content-Type: image/jpeg\r\n" - f"Content-Length: {len(frame)}\r\n" - f"\r\n".encode() + ( + f"--{boundary}\r\n" + f"Content-Type: image/jpeg\r\n" + f"Content-Length: {len(frame)}\r\n" + f"\r\n" + ).encode() + frame + b"\r\n" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py` around lines 248 - 263, The header string build that is currently written as adjacent f-strings with .encode() applied at the end (used when assembling parts in the loop inside the mjpeg streamer) is visually misleading; update the header construction in the loop that appends to parts so the entire header f-string block is grouped explicitly (wrap the multi-line f-string sequence in parentheses) before calling .encode(), e.g., build the header as one parenthesized f-string (using boundary and len(frame)) and then .encode() + frame + b"\r\n", ensuring the intent around .encode() on the full header is unambiguous for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py`:
- Around line 293-303: The current guard uses frame_dir.exists() which permits
non-directory files and can make frame_dir.glob("*.jpg") raise
NotADirectoryError; change the check in the code that sets frame_dir (the block
using files_base = Path(...), frame_dir = files_base / frames_dir and the
subsequent if frame_dir.exists():) to use frame_dir.is_dir() instead so glob
only runs when frame_dir is a directory, keeping the rest of the logic (sorting
.jpg/.jpeg and selecting frame_path = frames[counter % len(frames)] then
returning frame_path.read_bytes()) unchanged.
- Around line 163-193: The handler is using flow.request.path and splitting on
"/" which includes querystrings and causes resource mismatch; change to use
flow.request.path_components (the query‑stripped tuple) instead of path, e.g.
replace the path/parts logic with components = flow.request.path_components,
keep the same length check (len(components) < 6) and then set camera_id =
components[4] and resource = components[5] so snapshot.jpg and stream.mjpeg
match correctly even when queries are present; leave the rest of the logic
(_serve_snapshot, _serve_mjpeg_stream, fps/resolution/frames_dir) unchanged.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 154-181: The _evaluate_builtin method may IndexError or raise on
bad user templates because it assumes args from cls._parse_args(expr) contain
required positions; update _evaluate_builtin (in class method _evaluate_builtin
of bundled_addon.py) to validate the parsed args length for each branch
(random_int, random_float, random_choice, counter, env) before indexing and
return a controlled error/value (e.g., None or a descriptive string) for
malformed expressions; additionally wrap numeric conversions (int()/float()) and
random.randint/random.uniform calls in try/except to catch ValueError/TypeError
and handle non-numeric inputs gracefully, and ensure counter() handles missing
name (e.g., treat empty string or raise a clear error) rather than causing an
IndexError.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 466-498: The set_mock function currently calls json.loads(body)
and json.loads(headers) without handling json.JSONDecodeError (same issue in
set_state), so wrap those json.loads calls in try/except blocks that catch
json.JSONDecodeError and return a clear user-facing error string (e.g., "Invalid
JSON for body" or "Invalid JSON for headers") consistent with
set_mock_sequence/set_mock_template/set_mock_conditional; only proceed to set
self._mock_endpoints[key] and call self._write_mock_config() if parsing
succeeds. Ensure you reference the same keys
("status","body","content_type","headers") when building the dict and mirror the
error messages/format used elsewhere in the file.
- Around line 1041-1055: The accept loop currently overwrites
self._capture_reader_thread so only the last reader is ever joined; change the
single-thread field into a list (e.g. self._capture_reader_threads initialized
in __init__) and in _capture_accept_loop append each newly created Thread to
that list instead of assigning to _capture_reader_thread, optionally protecting
the list with a lock if concurrent access is possible; then update
_stop_capture_server to iterate over self._capture_reader_threads and join each
thread (skipping or handling already-finished threads) to ensure all reader
threads are waited on during shutdown.
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 662-668: The request hook in bundled_addon.py currently uses
synchronous time.sleep which blocks mitmproxy's event loop; change the hook to
an async coroutine by converting the method handling requests (e.g., request in
the class handling flows) to async def request(self, flow): and replace
time.sleep(latency_ms / 1000.0) with await asyncio.sleep(latency_ms / 1000.0);
add an import for asyncio, remove or stop using time.sleep, and ensure any
references to the sync request signature are updated to the async form so
latency simulation no longer blocks other flows.
- Around line 272-277: The code assumes importlib.util.spec_from_file_location
returns a valid spec and loader; guard against spec or spec.loader being None in
the bundled_addon.py logic around importlib.util.spec_from_file_location,
importlib.util.module_from_spec and spec.loader.exec_module: check that spec is
not None and spec.loader is not None before calling module_from_spec and
exec_module, and if either is None raise or log a clear ImportError (or skip
loading) including the script_path and name so the failure is explicit and
handled instead of causing an AttributeError.
- Around line 809-819: The path-traversal check using
str(file_path).startswith(str(files_dir_resolved)) can be bypassed; update the
validation in the block handling file_path resolution (variables file_path,
files_dir_resolved, self.files_dir) to use
file_path.is_relative_to(files_dir_resolved) instead (Python 3.9+). Replace the
startswith check with that call and return None / log the same error when it
returns False; if you must support older Pythons, implement an equivalent safe
check using pathlib.Path.parts comparison as a fallback.
- Around line 178-180: The env() handler (the branch starting with
expr.startswith("env(") in bundled_addon.py, which calls cls._parse_args and
currently returns os.environ.get(args[0], "")) exposes arbitrary environment
variables; change it to only return values for an allowlist of safe variable
names (e.g., a constant ALLOWED_ENV_VARS or a class attribute allowed_env_vars)
and otherwise return an empty string (or a safe placeholder). Update the logic
in the same method: parse the var name via cls._parse_args, validate it against
ALLOWED_ENV_VARS (or allowed_env_vars) and only then read os.environ.get(var);
for non-allowed names return "" and optionally log the denied access. Ensure no
other code paths call os.environ directly for unvalidated names.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 127-150: The listen/web/directories fields are annotated as dict
but are converted to Pydantic models in __post_init__, causing static type
errors; update the annotations for listen to ListenConfig, web to WebConfig, and
directories to DirectoriesConfig (keep the existing default_factory behavior and
the validation in __post_init__), so static checkers know the attributes are
model instances and code like self.listen.host is valid while preserving
model_validate calls in __post_init__ (refer to ListenConfig, WebConfig,
DirectoriesConfig, and __post_init__ in your patch).
- Around line 242-246: The subprocess is opened with stdout=subprocess.PIPE and
stderr=subprocess.PIPE which risks deadlock unless those pipes are actively
drained; update the Popen usage in the code that creates self._process so it
either redirects output to subprocess.DEVNULL (stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL) when you don't need output, or funnels stderr into
stdout (stderr=subprocess.STDOUT) and starts a dedicated reader (or thread) that
calls self._process.communicate() / reads the pipe asynchronously to drain it;
modify the block that constructs self._process (the Popen call) and add the
corresponding background reader/communicate logic if you keep pipes open.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py`:
- Line 294: The hardcoded files_base
Path("/opt/jumpstarter/mitmproxy/mock-files") should be made configurable: read
the base path from an injected config or an environment variable (e.g.,
os.environ["MJPEG_MOCK_FILES"] with a sensible default) and use that value
instead of the literal; if _get_frame is a helper method, add a parameter or
capture the config on the addon instance (e.g., self.files_base) so _get_frame
can reliably locate mock files and return a clear warning/log when the directory
is missing.
- Around line 248-263: The header string build that is currently written as
adjacent f-strings with .encode() applied at the end (used when assembling parts
in the loop inside the mjpeg streamer) is visually misleading; update the header
construction in the loop that appends to parts so the entire header f-string
block is grouped explicitly (wrap the multi-line f-string sequence in
parentheses) before calling .encode(), e.g., build the header as one
parenthesized f-string (using boundary and len(frame)) and then .encode() +
frame + b"\r\n", ensuring the intent around .encode() on the full header is
unambiguous for future maintainers.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 390-402: The default files_dir in __init__ is set using a "../"
relative path which can leave unresolved ".." components and cause the
path-traversal check in _read_file to behave unpredictably; change the
initialization of self.files_dir in __init__ to construct the directory from
Path(self.MOCK_DIR).parent / "mock-files" (same style already used elsewhere) so
the path has the correct parent-based form before _load_config or _read_file
run, leaving _load_config and _read_file unchanged.
- Around line 326-340: send_event currently drops events silently when the Unix
socket is unavailable; modify CaptureClient.send_event to log a warning on the
first failed connection attempt so failures are visible during tests: when
_connect() returns False or when the first OSError is caught, emit a single
warning (using the instance logger or module logger) that includes the socket
path and that events will be dropped, then proceed with the existing
reconnect/return logic; reference the send_event method, the _connect() call,
the self._sock check, and the close() call to locate where to insert the
warning.
- Around line 619-653: The request hook calls _find_endpoint (which calls
_load_config()) and also calls _load_state() on every request causing two stat()
syscalls per request; add a short cooldown to both _load_config and _load_state
so they only perform their file mtime stat when enough seconds have passed since
the last check. Implement per-instance fields like _last_config_check,
_last_state_check and a configurable _config_check_interval (seconds); on first
call always load, otherwise compare time.time() to last check and skip the
stat/syscall if inside the cooldown window, updating the last check time and
reloading only when the interval has elapsed so request(), _find_endpoint(),
_load_config(), and _load_state() retain hot-reload behavior without calling
stat() every request.
- Line 302: The module-level CAPTURE_SOCKET constant is hardcoded which breaks
when the addon is run standalone; change the addon to read the socket path from
an environment variable fallback (e.g.,
os.environ.get("JUMPSTARTER_CAPTURE_SOCKET",
"/opt/jumpstarter/mitmproxy/capture.sock")) instead of relying solely on the
literal CAPTURE_SOCKET, and update the driver's _generate_default_addon to set
that environment variable (or continue patching but prefer setting
JUMPSTARTER_CAPTURE_SOCKET) so _start_capture_server and other functions use the
env var-derived path when present.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1149-1171: Replace the non-atomic direct write in
_write_mock_config by writing the JSON to a temp file in the same directory (use
tempfile.NamedTemporaryFile or mkstemp with dir=mock_path and delete=False),
flush and fsync the temp file, close it, then atomically swap it into place with
os.replace(temp_path, config_file); also apply the same pattern to _write_state
so both writes are atomic; ensure you import os and tempfile and maintain the
same file permissions/ownership if needed.
- Around line 262-284: The code currently hardcodes the mitmweb UI password
("jumpstarter") in _build_base_cmd and embeds the same token in the returned
status URL; change this to a configurable or per-session secret by adding a
web_password (or token) field to WebConfig (or generate a secure random token at
session start), then use that value when building the command in _build_base_cmd
(replace the literal "jumpstarter" with the config/token) and when composing the
status URL (replace the hardcoded ?token=jumpstarter); ensure the token is
generated with a cryptographically secure RNG, stored only in memory for the
session, and not logged.
- Around line 1187-1297: The fallback and bundled addon installation in
_generate_default_addon currently injects self.directories.mocks and
self._capture_socket_path directly into generated Python source (and does
string.replace on bundled_addon.py), which is fragile and injection-prone;
instead, write a small JSON config (e.g., {"mocks": "...", "capture_socket":
"..."}) next to the addon and change the addon to read that config at runtime
(referenced symbols: _generate_default_addon, bundled_addon.py,
MitmproxyMockAddon.MOCK_DIR, and self._capture_socket_path) — update the
bundled-copy branch to drop a config JSON rather than doing string.replace, and
change the inline addon template to load paths from that config file using
json.load at startup.
Comment on lines
+163
to
+193
| path = flow.request.path | ||
| parts = path.rstrip("/").split("/") | ||
|
|
||
| # Expected: /streaming/video/camera/{camera_id}/{resource} | ||
| if len(parts) < 6: | ||
| return False | ||
|
|
||
| camera_id = parts[4] | ||
| resource = parts[5] | ||
|
|
||
| cameras = config.get("cameras", {}) | ||
| camera_config = cameras.get(camera_id, {}) | ||
| fps = config.get("fps", 15) | ||
| resolution = config.get("default_resolution", [640, 480]) | ||
| frames_dir = camera_config.get( | ||
| "frames_dir", config.get("frames_dir", "video/frames"), | ||
| ) | ||
|
|
||
| if resource == "snapshot.jpg": | ||
| self._serve_snapshot( | ||
| flow, camera_id, frames_dir, resolution, | ||
| ) | ||
| return True | ||
|
|
||
| elif resource == "stream.mjpeg": | ||
| self._serve_mjpeg_stream( | ||
| flow, camera_id, frames_dir, resolution, fps, | ||
| ) | ||
| return True | ||
|
|
||
| return False |
Contributor
There was a problem hiding this comment.
flow.request.path includes the query string — resource matching silently fails for URLs with query parameters
flow.request.path is documented as including both the path and query parts of the target URI, e.g. "/index.html?a=b". This means for a request like GET /streaming/video/camera/rear/snapshot.jpg?cache=no, splitting on / produces parts[5] == "snapshot.jpg?cache=no", which matches neither "snapshot.jpg" nor "stream.mjpeg", so the handler silently returns False and the mock never fires.
mitmproxy already exposes flow.request.path_components — a tuple of decoded, query-stripped path segments — which is the correct property to use here.
🐛 Proposed fix using path_components
def handle(self, flow: http.HTTPFlow, config: dict) -> bool:
"""Route video requests to the appropriate handler."""
- path = flow.request.path
- parts = path.rstrip("/").split("/")
-
- # Expected: /streaming/video/camera/{camera_id}/{resource}
- if len(parts) < 6:
- return False
-
- camera_id = parts[4]
- resource = parts[5]
+ # path_components returns decoded path segments with no query string
+ # e.g. ("streaming", "video", "camera", "rear", "snapshot.jpg")
+ parts = flow.request.path_components
+
+ # Expected: /streaming/video/camera/{camera_id}/{resource}
+ if len(parts) < 5:
+ return False
+
+ camera_id = parts[3]
+ resource = parts[4]📝 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.
Suggested change
| path = flow.request.path | |
| parts = path.rstrip("/").split("/") | |
| # Expected: /streaming/video/camera/{camera_id}/{resource} | |
| if len(parts) < 6: | |
| return False | |
| camera_id = parts[4] | |
| resource = parts[5] | |
| cameras = config.get("cameras", {}) | |
| camera_config = cameras.get(camera_id, {}) | |
| fps = config.get("fps", 15) | |
| resolution = config.get("default_resolution", [640, 480]) | |
| frames_dir = camera_config.get( | |
| "frames_dir", config.get("frames_dir", "video/frames"), | |
| ) | |
| if resource == "snapshot.jpg": | |
| self._serve_snapshot( | |
| flow, camera_id, frames_dir, resolution, | |
| ) | |
| return True | |
| elif resource == "stream.mjpeg": | |
| self._serve_mjpeg_stream( | |
| flow, camera_id, frames_dir, resolution, fps, | |
| ) | |
| return True | |
| return False | |
| # path_components returns decoded path segments with no query string | |
| # e.g. ("streaming", "video", "camera", "rear", "snapshot.jpg") | |
| parts = flow.request.path_components | |
| # Expected: /streaming/video/camera/{camera_id}/{resource} | |
| if len(parts) < 5: | |
| return False | |
| camera_id = parts[3] | |
| resource = parts[4] | |
| cameras = config.get("cameras", {}) | |
| camera_config = cameras.get(camera_id, {}) | |
| fps = config.get("fps", 15) | |
| resolution = config.get("default_resolution", [640, 480]) | |
| frames_dir = camera_config.get( | |
| "frames_dir", config.get("frames_dir", "video/frames"), | |
| ) | |
| if resource == "snapshot.jpg": | |
| self._serve_snapshot( | |
| flow, camera_id, frames_dir, resolution, | |
| ) | |
| return True | |
| elif resource == "stream.mjpeg": | |
| self._serve_mjpeg_stream( | |
| flow, camera_id, frames_dir, resolution, fps, | |
| ) | |
| return True | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py`
around lines 163 - 193, The handler is using flow.request.path and splitting on
"/" which includes querystrings and causes resource mismatch; change to use
flow.request.path_components (the query‑stripped tuple) instead of path, e.g.
replace the path/parts logic with components = flow.request.path_components,
keep the same length check (len(components) < 6) and then set camera_id =
components[4] and resource = components[5] so snapshot.jpg and stream.mjpeg
match correctly even when queries are present; leave the rest of the logic
(_serve_snapshot, _serve_mjpeg_stream, fps/resolution/frames_dir) unchanged.
Contributor
There was a problem hiding this comment.
@mangelajo, yes! The addon system is quite feature-rich. It supports:
- Custom request handlers for complex behaviors (WebSocket, HLS streaming, MJPEG feeds)
- Hot-reload during tests to swap scenarios dynamically
- Per-addon configuration passed from the mock scenario YAML
- Template expressions for dynamic responses
- State management shared between tests and addon logic
The included example addons (data_stream_websocket.py, hls_audio_stream.py, mjpeg_stream.py) show how to simulate real-time telemetry streams, audio feeds, and video cameras — all without needing actual hardware in the loop. Pretty powerful for HIL testing! 🚀
(The technical issue I flagged above about query string handling is still worth fixing when you get a chance, but no rush — just wanted to point it out for robustness.)
🧠 Learnings used
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2026-02-04T18:13:10.193Z
Learning: Applies to python/packages/jumpstarter-driver-*/**/*test*.py : Driver package tests should focus on end-to-end testing with server and client, using mocks only when dealing with system dependencies, services, or macOS/Linux compatibility issues
Comment on lines
+293
to
+303
| # Try loading from files directory | ||
| files_base = Path("/opt/jumpstarter/mitmproxy/mock-files") | ||
| frame_dir = files_base / frames_dir | ||
|
|
||
| if frame_dir.exists(): | ||
| frames = sorted(frame_dir.glob("*.jpg")) | ||
| if not frames: | ||
| frames = sorted(frame_dir.glob("*.jpeg")) | ||
| if frames: | ||
| frame_path = frames[counter % len(frames)] | ||
| return frame_path.read_bytes() |
Contributor
There was a problem hiding this comment.
Use is_dir() instead of exists() to guard glob()
If the path at frame_dir happens to be a regular file rather than a directory, exists() returns True but frame_dir.glob("*.jpg") raises NotADirectoryError. is_dir() is the appropriate check here.
🐛 Proposed fix
- if frame_dir.exists():
+ if frame_dir.is_dir():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/mjpeg_stream.py`
around lines 293 - 303, The current guard uses frame_dir.exists() which permits
non-directory files and can make frame_dir.glob("*.jpg") raise
NotADirectoryError; change the check in the code that sets frame_dir (the block
using files_base = Path(...), frame_dir = files_base / frames_dir and the
subsequent if frame_dir.exists():) to use frame_dir.is_dir() instead so glob
only runs when frame_dir is a directory, keeping the rest of the logic (sorting
.jpg/.jpeg and selecting frame_path = frames[counter % len(frames)] then
returning frame_path.read_bytes()) unchanged.
Comment on lines
+154
to
+181
| @classmethod | ||
| def _evaluate_builtin(cls, expr: str) -> Any | None: | ||
| """Evaluate built-in expressions (no flow needed).""" | ||
| if expr == "now_iso": | ||
| return datetime.now(timezone.utc).isoformat() | ||
| if expr == "now_epoch": | ||
| return int(time.time()) | ||
| if expr == "uuid": | ||
| import uuid | ||
| return str(uuid.uuid4()) | ||
| if expr.startswith("random_int("): | ||
| args = cls._parse_args(expr) | ||
| return random.randint(int(args[0]), int(args[1])) | ||
| if expr.startswith("random_float("): | ||
| args = cls._parse_args(expr) | ||
| return round(random.uniform(float(args[0]), float(args[1])), 2) | ||
| if expr.startswith("random_choice("): | ||
| args = cls._parse_args(expr) | ||
| return random.choice(args) | ||
| if expr.startswith("counter("): | ||
| args = cls._parse_args(expr) | ||
| name = args[0] | ||
| cls._counters[name] += 1 | ||
| return cls._counters[name] | ||
| if expr.startswith("env("): | ||
| args = cls._parse_args(expr) | ||
| return os.environ.get(args[0], "") | ||
| return None |
Contributor
There was a problem hiding this comment.
Template expression argument validation missing — IndexError on malformed expressions.
_evaluate_builtin calls cls._parse_args(expr) and then indexes args[0], args[1] without checking the argument count. For example, {{random_int(5)}} (missing second arg) will raise IndexError, and {{counter()}} (no args) will return ['']. Since these expressions come from user-provided mock templates, an IndexError will propagate up and may cause the addon to fail the entire request instead of returning a helpful error.
Proposed fix — validate argument count per expression
`@classmethod`
def _evaluate_builtin(cls, expr: str) -> Any | None:
"""Evaluate built-in expressions (no flow needed)."""
if expr == "now_iso":
return datetime.now(timezone.utc).isoformat()
if expr == "now_epoch":
return int(time.time())
if expr == "uuid":
import uuid
return str(uuid.uuid4())
if expr.startswith("random_int("):
args = cls._parse_args(expr)
+ if len(args) < 2:
+ ctx.log.error(f"random_int requires 2 args, got {len(args)}")
+ return 0
return random.randint(int(args[0]), int(args[1]))
if expr.startswith("random_float("):
args = cls._parse_args(expr)
+ if len(args) < 2:
+ ctx.log.error(f"random_float requires 2 args, got {len(args)}")
+ return 0.0
return round(random.uniform(float(args[0]), float(args[1])), 2)Similarly, wrap the int() / float() conversions in try/except to handle non-numeric arguments gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`
around lines 154 - 181, The _evaluate_builtin method may IndexError or raise on
bad user templates because it assumes args from cls._parse_args(expr) contain
required positions; update _evaluate_builtin (in class method _evaluate_builtin
of bundled_addon.py) to validate the parsed args length for each branch
(random_int, random_float, random_choice, counter, env) before indexing and
return a controlled error/value (e.g., None or a descriptive string) for
malformed expressions; additionally wrap numeric conversions (int()/float()) and
random.randint/random.uniform calls in try/except to catch ValueError/TypeError
and handle non-numeric inputs gracefully, and ensure counter() handles missing
name (e.g., treat empty string or raise a clear error) rather than causing an
IndexError.
Comment on lines
+466
to
+498
| @export | ||
| def set_mock(self, method: str, path: str, status: int, | ||
| body: str, | ||
| content_type: str = "application/json", | ||
| headers: str = "{}") -> str: | ||
| """Add or update a mock endpoint. | ||
|
|
||
| The addon script reads mock definitions from a JSON file on | ||
| disk. This method updates that file and, if the proxy is | ||
| running in mock mode, the addon will pick up changes on the | ||
| next request (it watches the file modification time). | ||
|
|
||
| Args: | ||
| method: HTTP method (GET, POST, PUT, DELETE, etc.) | ||
| path: URL path to match (e.g., "/api/v1/status"). | ||
| Append ``*`` for prefix matching. | ||
| status: HTTP status code to return. | ||
| body: Response body as a JSON string. | ||
| content_type: Response Content-Type header. | ||
| headers: Additional response headers as a JSON string. | ||
|
|
||
| Returns: | ||
| Confirmation message. | ||
| """ | ||
| key = f"{method.upper()} {path}" | ||
| self._mock_endpoints[key] = { | ||
| "status": int(status), | ||
| "body": json.loads(body) if body else {}, | ||
| "content_type": content_type, | ||
| "headers": json.loads(headers) if headers else {}, | ||
| } | ||
| self._write_mock_config() | ||
| return f"Mock set: {key} → {int(status)}" |
Contributor
There was a problem hiding this comment.
Unhandled JSONDecodeError in set_mock (and set_state) — inconsistent with other methods.
set_mock calls json.loads(body) and json.loads(headers) at Lines 493/495 without try/except, so invalid JSON will propagate as an unhandled exception through the RPC layer. The same issue exists in set_state (Line 795). Other methods like set_mock_sequence, set_mock_template, and set_mock_conditional correctly catch json.JSONDecodeError and return a user-friendly error string.
Proposed fix for set_mock
def set_mock(self, method: str, path: str, status: int,
body: str,
content_type: str = "application/json",
headers: str = "{}") -> str:
key = f"{method.upper()} {path}"
+ try:
+ parsed_body = json.loads(body) if body else {}
+ except json.JSONDecodeError as e:
+ return f"Invalid body JSON: {e}"
+ try:
+ parsed_headers = json.loads(headers) if headers else {}
+ except json.JSONDecodeError as e:
+ return f"Invalid headers JSON: {e}"
self._mock_endpoints[key] = {
"status": int(status),
- "body": json.loads(body) if body else {},
+ "body": parsed_body,
"content_type": content_type,
- "headers": json.loads(headers) if headers else {},
+ "headers": parsed_headers,
}Proposed fix for set_state
def set_state(self, key: str, value_json: str) -> str:
- value = json.loads(value_json)
+ try:
+ value = json.loads(value_json)
+ except json.JSONDecodeError as e:
+ return f"Invalid JSON: {e}"
self._state_store[key] = value
self._write_state()
return f"State set: {key}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 466 - 498, The set_mock function currently calls json.loads(body)
and json.loads(headers) without handling json.JSONDecodeError (same issue in
set_state), so wrap those json.loads calls in try/except blocks that catch
json.JSONDecodeError and return a clear user-facing error string (e.g., "Invalid
JSON for body" or "Invalid JSON for headers") consistent with
set_mock_sequence/set_mock_template/set_mock_conditional; only proceed to set
self._mock_endpoints[key] and call self._write_mock_config() if parsing
succeeds. Ensure you reference the same keys
("status","body","content_type","headers") when building the dict and mirror the
error messages/format used elsewhere in the file.
Comment on lines
+1041
to
+1055
| def _capture_accept_loop(self): | ||
| """Accept connections on the capture socket.""" | ||
| while self._capture_running: | ||
| try: | ||
| conn, _ = self._capture_server_sock.accept() | ||
| self._capture_reader_thread = threading.Thread( | ||
| target=self._capture_read_loop, | ||
| args=(conn,), | ||
| daemon=True, | ||
| ) | ||
| self._capture_reader_thread.start() | ||
| except socket.timeout: | ||
| continue | ||
| except OSError: | ||
| break |
Contributor
There was a problem hiding this comment.
_capture_accept_loop overwrites _capture_reader_thread on each connection — only the last reader is joined on shutdown.
Each accepted connection spawns a new thread stored in self._capture_reader_thread, overwriting the previous reference. In _stop_capture_server (Line 1098), only the last reader thread is joined. If the addon disconnects and reconnects (or multiple connections arrive), prior reader threads become un-joinable orphans.
Since all threads are daemon=True, this won't block process exit, but it means _stop_capture_server gives an incomplete guarantee. Consider tracking readers in a list.
Proposed fix — track all reader threads
Replace the single _capture_reader_thread field with a list:
- _capture_reader_thread: threading.Thread | None = field(
- default=None, init=False, repr=False
- )
+ _capture_reader_threads: list = field(
+ default_factory=list, init=False, repr=False
+ )In _capture_accept_loop:
- self._capture_reader_thread = threading.Thread(
+ t = threading.Thread(
target=self._capture_read_loop,
args=(conn,),
daemon=True,
)
- self._capture_reader_thread.start()
+ t.start()
+ self._capture_reader_threads.append(t)In _stop_capture_server:
- if self._capture_reader_thread is not None:
- self._capture_reader_thread.join(timeout=5)
- self._capture_reader_thread = None
+ for t in self._capture_reader_threads:
+ t.join(timeout=5)
+ self._capture_reader_threads.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 1041 - 1055, The accept loop currently overwrites
self._capture_reader_thread so only the last reader is ever joined; change the
single-thread field into a list (e.g. self._capture_reader_threads initialized
in __init__) and in _capture_accept_loop append each newly created Thread to
that list instead of assigning to _capture_reader_thread, optionally protecting
the list with a lock if concurrent access is possible; then update
_stop_capture_server to iterate over self._capture_reader_threads and join each
thread (skipping or handling already-finished threads) to ensure all reader
threads are waited on during shutdown.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py (2)
226-230:hash(resource)gives non-reproducible rotation across process restarts.Python's string
hash()is randomized byPYTHONHASHSEED, so the segment selected for a givenresourcename will differ between test runs. For a deterministic fallback rotation, use a stable hash such ashashlib.md5or derive the index from the sequence number parsed out ofresource.♻️ Stable rotation using sequence number
+ # Parse the sequence number from "seg_{n}.aac" for stable rotation + try: + seq_num = int(resource.removeprefix("seg_").removesuffix(".aac")) + idx = seq_num % len(candidates) + except ValueError: + idx = 0 - idx = hash(resource) % len(candidates) seg_path = candidates[idx]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py` around lines 226 - 230, The code uses Python's built-in hash(resource) in the rotation logic (candidates, idx, seg_path), which is non-deterministic across process restarts; replace it with a stable deterministic method such as computing a stable hash (e.g., hashlib.md5(resource.encode()).hexdigest() converted to an int) or derive the index from the segment sequence parsed from the resource name, then compute idx = stable_int % len(candidates) and assign seg_path = candidates[idx] so selection is reproducible across runs.
79-81:_sequence_countersand_segment_cacheare never used — dead code.Both fields are initialized but not read or written anywhere in the class. Either remove them or wire them up (e.g., use
_segment_cacheto avoid re-reading the same file on repeated requests, or use_sequence_countersfor per-channel sequence tracking instead of the wall-clock derivation).🧹 Remove unused fields
def __init__(self): - self._sequence_counters: dict[str, int] = {} - self._segment_cache: bytes | None = None + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py` around lines 79 - 81, Remove the dead fields initialized in __init__: delete the attributes self._sequence_counters and self._segment_cache (and their type hints) since they are never read or written; if you prefer to keep functionality instead, implement usage by wiring _segment_cache into the request handler to cache and reuse a previously read audio segment and replace the wall-clock-based sequence logic by using _sequence_counters per channel when building segment URIs — otherwise simply remove these two attribute initializations from the __init__ method and any related unused imports or references.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py (2)
95-95:_countersis a class-level singleton — counter state leaks across test runs.
TemplateEngine._countersis a class attribute (not instance), so{{counter(name)}}template expressions accumulate globally within a process. In pytest, counter values from one test will carry over into subsequent tests that use the same counter name, causing non-deterministic failures.Consider adding a
reset_counters()classmethod so test fixtures can callTemplateEngine.reset_counters()during teardown:♻️ Proposed addition
_counters: dict[str, int] = defaultdict(int) + + `@classmethod` + def reset_counters(cls) -> None: + """Reset all counter state (call between tests for isolation).""" + cls._counters.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` at line 95, TemplateEngine currently uses a class-level _counters: dict[str,int] = defaultdict(int), causing counter state to persist across instances and tests; add a classmethod reset_counters(cls) that clears or reinitializes cls._counters (e.g., cls._counters.clear() or cls._counters = defaultdict(int)) and call that from test fixtures as needed so {{counter(name)}} does not leak state between test runs; update any places referring to TemplateEngine._counters to continue using the class attribute but rely on reset_counters for lifecycle control.
328-334: Spool infrastructure is defined but never used — large/binary bodies are inlined unconditionally.
CAPTURE_SPOOL_DIR,_INLINE_BODY_LIMIT,_spool_dir, and_spool_counterare all defined but_build_capture_event(lines 895-910) always inlines the full request body viaflow.request.get_text() or "", with no size check or spooling. For large uploads or binary content (e.g., firmware), this causes:
- High memory pressure in the capture buffer.
get_text()decoding failures for non-UTF-8 binary bodies.Either implement the spooling logic (checking against
_INLINE_BODY_LIMITand writing oversized/binary bodies to_spool_dir), or remove the dead constants and initialisation to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` around lines 328 - 334, The spool constants (CAPTURE_SPOOL_DIR, _INLINE_BODY_LIMIT, _spool_dir, _spool_counter) are unused while _build_capture_event always inlines flow.request.get_text(); update _build_capture_event to implement spooling: use flow.request.get_content() to obtain bytes, detect non-UTF8/binary or check len(bytes) > _INLINE_BODY_LIMIT, and when oversized/binary create _spool_dir if missing, write the bytes to a unique file using _spool_counter (increment it safely), store the file path in the capture event instead of inlining the body, and on normal small/UTF-8 bodies decode to text (flow.request.get_text()) as before; include exception handling to fall back to an empty inline body and log errors.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py (1)
383-387:stopEvent is never set — it's dead code.The
while not stop.wait(timeout=0.5)loop exits only viaKeyboardInterrupt;stopis never set toTrue. The periodic timeout approach (to keep the GIL responsive) is correct, butstopadds confusion. Either document thatstopis a placeholder for future external cancellation, or simplify:♻️ Suggested simplification
- stop = Event() - while not stop.wait(timeout=0.5): - pass + while True: + time.sleep(0.5)(and
from threading import Eventcan be removed if no longer needed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py` around lines 383 - 387, The local threading.Event named stop is never set and makes the loop dead code; remove the Event usage and its import and simplify the control flow by replacing the "stop" loop (while not stop.wait(timeout=0.5)) with an explicit loop that sleeps or waits for the timeout (e.g., while True: time.sleep(0.5)) and keep the existing except KeyboardInterrupt: pass/break behavior; update any surrounding comments to reflect that external cancellation is not currently supported if needed.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py (1)
1313-1326: Fragile string-replacement coupling betweendriver.pyandbundled_addon.py.The two
content.replace(...)calls depend on the replacement targets ('/opt/jumpstarter/mitmproxy/mock-responses'and'/opt/jumpstarter/mitmproxy/capture.sock') exactly matchingMOCK_DIR's default value andCAPTURE_SOCKETinbundled_addon.py. If either constant is updated in one file but not the other, the replacement silently fails and the addon uses stale/wrong paths — a bug with no error signal.Consider passing paths to the addon via environment variables or mitmproxy
--setflags instead of post-copy string patching.♻️ Sketch of env-var-based approach
In
_build_base_cmd:# Pass addon configuration as environment variables env = os.environ.copy() env["MITMPROXY_MOCK_DIR"] = self.directories.mocks env["MITMPROXY_CAPTURE_SOCK"] = self._capture_socket_path or "" self._subprocess_env = env # store for PopenIn
start():self._process = subprocess.Popen(cmd, ..., env=self._subprocess_env)Then
bundled_addon.pyreads these at startup, eliminating the string-replacement step entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1313 - 1326, The current post-copy content.replace calls in driver.py (replacing '/opt/jumpstarter/mitmproxy/mock-responses' and '/opt/jumpstarter/mitmproxy/capture.sock') are fragile and should be removed; instead pass the runtime paths into the addon via environment variables or mitmproxy --set flags from _build_base_cmd so bundled_addon.py reads them at startup. Modify _build_base_cmd to copy os.environ, set MITMPROXY_MOCK_DIR = self.directories.mocks and MITMPROXY_CAPTURE_SOCK = self._capture_socket_path or "", store that env on the instance (e.g. self._subprocess_env), and ensure start() uses subprocess.Popen(cmd, ..., env=self._subprocess_env). Remove the string-replacement block in the copy logic and update bundled_addon.py to read os.environ['MITMPROXY_MOCK_DIR'] / ['MITMPROXY_CAPTURE_SOCK'] (or accept --set values) so the addon no longer depends on brittle literal replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py`:
- Around line 137-143: The playlist construction embeds channel.get('name',
channel_id) directly into the `#EXT-X-STREAM-INF` NAME field which can break M3U8
if the name contains quotes, commas, or newlines; update the code around the
playlist assignment (the block building playlist using bitrate, base, channel_id
and channel.get('name', channel_id)) to sanitize the channel name first—e.g.,
trim or remove newline characters and either escape or replace double quotes and
commas (or wrap in a safe-quoted form) via a small helper like
sanitize_channel_name(name) or inline replacements—then interpolate the
sanitized value into the NAME field.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Line 26: Remove the unused hashlib import in
jumpstarter_driver_mitmproxy/bundled_addon.py: delete the line importing
"hashlib" (the unused symbol) so the module no longer raises Ruff F401; ensure
no other code references hashlib and run the linter/tests to confirm the
pipeline error is resolved.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 778-782: The current fragile check in the get_ca_cert path uses
string matching on result.startswith("Error:"), so change the protocol to use a
structured JSON envelope from self.call for operations that can fail (e.g.,
self.call("get_ca_cert")) and update the caller to parse that JSON, check an
"ok" boolean, and raise RuntimeError(result["error"]) when ok is false;
specifically, modify the code that calls self.call("get_ca_cert") to JSON-decode
the response, validate it has keys like "ok" and "error", handle JSON decode
errors by raising a clear RuntimeError, and return the actual certificate
payload when result["ok"] is true. Ensure you update references to self.call and
the get_ca_cert handling so error detection is structural rather than
string-based.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1238-1261: The startup can leave the capture server running if any
of the mode setup functions (_load_startup_mocks, _write_mock_config,
_generate_default_addon, or _apply_mode_flags) raise; modify start so that after
calling _start_capture_server() you wrap the subsequent mode setup/config calls
in a try/except (or try/finally) that calls _stop_capture_server() on any
exception and then re-raises the error; specifically ensure exceptions from
_load_startup_mocks are caught and the server is stopped before propagating,
mirroring the existing replay-path behavior that calls _stop_capture_server() on
error.
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 849-856: The path-traversal check uses a brittle string startswith
between file_path and files_dir_resolved; replace it with Path.is_relative_to to
robustly verify containment (i.e., after resolving, call
file_path.is_relative_to(files_dir_resolved)) so paths like /opt/mock-files-evil
will not bypass the guard; update the block that currently references file_path,
files_dir_resolved and the try/except to use is_relative_to and keep the same
error return behavior on failure or exceptions.
- Around line 178-200: The _evaluate_builtin method is indexing args without
validating counts, causing IndexError on malformed templates (e.g.,
_evaluate_builtin handling of "random_int(", "random_float(", "random_choice(",
"counter(", "env(" after parsing via _parse_args). Fix by validating len(args)
before accessing indexes: require exactly 2 args for random_int/random_float, at
least 1 for random_choice, and exactly 1 for counter/env; when validation fails
log a warning via ctx.log.warn (include the expression and expected arg count)
and return a safe fallback (e.g., None for non-string builtins, empty string for
env) instead of indexing into args.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1155-1165: The accept loop in _capture_accept_loop currently
overwrites self._capture_reader_thread for each new connection so only the last
thread is tracked; change this to maintain a collection (e.g.
self._capture_reader_threads = [] initialized in __init__) and append each
created Thread instead of assigning to self._capture_reader_thread, and update
shutdown/cleanup logic that currently joins self._capture_reader_thread to
instead iterate over and join every thread in self._capture_reader_threads
(handle daemon/timeout as appropriate) so no reader threads are orphaned.
- Around line 541-573: The set_mock method currently calls json.loads on body
and headers without handling json.JSONDecodeError, which can leak unhandled
exceptions; update set_mock to parse body and headers inside try/except blocks
that catch json.JSONDecodeError, raise a clear ValueError (or other RPC-friendly
exception) that includes which field failed ("body" or "headers") and the
original error message, and only assign to self._mock_endpoints and call
self._write_mock_config() after both parses succeed; reference the set_mock
function, the _mock_endpoints assignment, and the _write_mock_config() call when
making the changes.
- Around line 854-873: The set_state function calls json.loads(value_json)
without handling malformed JSON; wrap the json.loads call in a try/except that
catches json.JSONDecodeError (imported from json) and return a clear error
response (e.g. a string like "Invalid JSON for key {key}: {e}" or raise a
suitable exception) instead of proceeding to assign to self._state_store and
calling self._write_state; update set_state to only update _state_store and call
_write_state when JSON parsing succeeds and include references to set_state,
json.loads, json.JSONDecodeError, self._state_store, and self._write_state to
locate the change.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py`:
- Around line 226-230: The code uses Python's built-in hash(resource) in the
rotation logic (candidates, idx, seg_path), which is non-deterministic across
process restarts; replace it with a stable deterministic method such as
computing a stable hash (e.g., hashlib.md5(resource.encode()).hexdigest()
converted to an int) or derive the index from the segment sequence parsed from
the resource name, then compute idx = stable_int % len(candidates) and assign
seg_path = candidates[idx] so selection is reproducible across runs.
- Around line 79-81: Remove the dead fields initialized in __init__: delete the
attributes self._sequence_counters and self._segment_cache (and their type
hints) since they are never read or written; if you prefer to keep functionality
instead, implement usage by wiring _segment_cache into the request handler to
cache and reuse a previously read audio segment and replace the wall-clock-based
sequence logic by using _sequence_counters per channel when building segment
URIs — otherwise simply remove these two attribute initializations from the
__init__ method and any related unused imports or references.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Line 95: TemplateEngine currently uses a class-level _counters: dict[str,int]
= defaultdict(int), causing counter state to persist across instances and tests;
add a classmethod reset_counters(cls) that clears or reinitializes cls._counters
(e.g., cls._counters.clear() or cls._counters = defaultdict(int)) and call that
from test fixtures as needed so {{counter(name)}} does not leak state between
test runs; update any places referring to TemplateEngine._counters to continue
using the class attribute but rely on reset_counters for lifecycle control.
- Around line 328-334: The spool constants (CAPTURE_SPOOL_DIR,
_INLINE_BODY_LIMIT, _spool_dir, _spool_counter) are unused while
_build_capture_event always inlines flow.request.get_text(); update
_build_capture_event to implement spooling: use flow.request.get_content() to
obtain bytes, detect non-UTF8/binary or check len(bytes) > _INLINE_BODY_LIMIT,
and when oversized/binary create _spool_dir if missing, write the bytes to a
unique file using _spool_counter (increment it safely), store the file path in
the capture event instead of inlining the body, and on normal small/UTF-8 bodies
decode to text (flow.request.get_text()) as before; include exception handling
to fall back to an empty inline body and log errors.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 383-387: The local threading.Event named stop is never set and
makes the loop dead code; remove the Event usage and its import and simplify the
control flow by replacing the "stop" loop (while not stop.wait(timeout=0.5))
with an explicit loop that sleeps or waits for the timeout (e.g., while True:
time.sleep(0.5)) and keep the existing except KeyboardInterrupt: pass/break
behavior; update any surrounding comments to reflect that external cancellation
is not currently supported if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1313-1326: The current post-copy content.replace calls in
driver.py (replacing '/opt/jumpstarter/mitmproxy/mock-responses' and
'/opt/jumpstarter/mitmproxy/capture.sock') are fragile and should be removed;
instead pass the runtime paths into the addon via environment variables or
mitmproxy --set flags from _build_base_cmd so bundled_addon.py reads them at
startup. Modify _build_base_cmd to copy os.environ, set MITMPROXY_MOCK_DIR =
self.directories.mocks and MITMPROXY_CAPTURE_SOCK = self._capture_socket_path or
"", store that env on the instance (e.g. self._subprocess_env), and ensure
start() uses subprocess.Popen(cmd, ..., env=self._subprocess_env). Remove the
string-replacement block in the copy logic and update bundled_addon.py to read
os.environ['MITMPROXY_MOCK_DIR'] / ['MITMPROXY_CAPTURE_SOCK'] (or accept --set
values) so the addon no longer depends on brittle literal replacements.
Comment on lines
+137
to
+143
| playlist = ( | ||
| "#EXTM3U\n" | ||
| "#EXT-X-VERSION:3\n" | ||
| f"#EXT-X-STREAM-INF:BANDWIDTH={bitrate}," | ||
| f"CODECS=\"mp4a.40.2\",NAME=\"{channel.get('name', channel_id)}\"\n" | ||
| f"{base}/media.m3u8\n" | ||
| ) |
Contributor
There was a problem hiding this comment.
Unsanitized channel name may produce malformed M3U8.
channel.get('name', channel_id) is embedded directly into the #EXT-X-STREAM-INF line inside a quoted string (Line 141). A name containing ", ,, or \n will break most HLS parsers. At minimum, strip or escape those characters before interpolation.
🛡️ Proposed minimal sanitization
+ name = channel.get('name', channel_id).replace('"', "'").replace('\n', ' ')
playlist = (
"#EXTM3U\n"
"#EXT-X-VERSION:3\n"
f"#EXT-X-STREAM-INF:BANDWIDTH={bitrate},"
- f"CODECS=\"mp4a.40.2\",NAME=\"{channel.get('name', channel_id)}\"\n"
+ f"CODECS=\"mp4a.40.2\",NAME=\"{name}\"\n"
f"{base}/media.m3u8\n"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/examples/addons/hls_audio_stream.py`
around lines 137 - 143, The playlist construction embeds channel.get('name',
channel_id) directly into the `#EXT-X-STREAM-INF` NAME field which can break M3U8
if the name contains quotes, commas, or newlines; update the code around the
playlist assignment (the block building playlist using bitrate, base, channel_id
and channel.get('name', channel_id)) to sanitize the channel name first—e.g.,
trim or remove newline characters and either escape or replace double quotes and
commas (or wrap in a safe-quoted form) via a small helper like
sanitize_channel_name(name) or inline replacements—then interpolate the
sanitized value into the NAME field.
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py
Show resolved
Hide resolved
Comment on lines
+778
to
+782
| result = self.call("get_ca_cert") | ||
| if result.startswith("Error:"): | ||
| raise RuntimeError(result) | ||
| return result | ||
|
|
Contributor
There was a problem hiding this comment.
Error detection via startswith("Error:") is fragile.
Any change to the driver's error message format (casing, wording) silently breaks RuntimeError propagation. Consider using a JSON envelope ({"ok": false, "error": "..."}) for all driver responses that can fail, so error detection is structural rather than string-based.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`
around lines 778 - 782, The current fragile check in the get_ca_cert path uses
string matching on result.startswith("Error:"), so change the protocol to use a
structured JSON envelope from self.call for operations that can fail (e.g.,
self.call("get_ca_cert")) and update the caller to parse that JSON, check an
"ok" boolean, and raise RuntimeError(result["error"]) when ok is false;
specifically, modify the code that calls self.call("get_ca_cert") to JSON-decode
the response, validate it has keys like "ok" and "error", handle JSON decode
errors by raising a clear RuntimeError, and return the actual certificate
payload when result["ok"] is true. Ensure you update references to self.call and
the get_ca_cert handling so error detection is structural rather than
string-based.
Comment on lines
+1238
to
+1261
| def _load_startup_mocks(self): | ||
| """Load mock_scenario file and inline mocks at startup. | ||
|
|
||
| The scenario file is loaded first as a base layer, then inline | ||
| ``mocks`` from the exporter config are overlaid on top (higher | ||
| priority). | ||
| """ | ||
| if self.mock_scenario: | ||
| scenario_path = Path(self.mock_scenario) | ||
| if not scenario_path.is_absolute(): | ||
| scenario_path = Path(self.directories.mocks) / scenario_path | ||
| if scenario_path.exists(): | ||
| with open(scenario_path) as f: | ||
| if scenario_path.suffix in (".yaml", ".yml"): | ||
| raw = yaml.safe_load(f) | ||
| else: | ||
| raw = json.load(f) | ||
| if "endpoints" in raw: | ||
| self._mock_endpoints = raw["endpoints"] | ||
| else: | ||
| self._mock_endpoints = raw | ||
|
|
||
| if self.mocks: | ||
| self._mock_endpoints.update(self.mocks) |
Contributor
There was a problem hiding this comment.
Resource leak when startup fails in mock mode — capture server left orphaned.
_load_startup_mocks() calls yaml.safe_load/json.load without any try/except. If the startup scenario file is malformed, the exception propagates through _apply_mode_flags() → start() with no cleanup. Because _start_capture_server() is called before _apply_mode_flags() (line 249), the Unix socket and its background threads are left running. The same risk applies to _write_mock_config() (disk error) and _generate_default_addon() (file copy failure) in the same block.
The replay path already handles its error case correctly by calling _stop_capture_server() before returning; the mock path needs the same protection.
🛡️ Proposed fix — guard the entire mode-setup block
def _apply_mode_flags(
self, cmd: list[str], mode: str, replay_file: str,
) -> str | None:
"""Append mode-specific flags to cmd. Returns error string or None."""
if mode == "mock":
- self._load_startup_mocks()
- self._write_mock_config()
+ try:
+ self._load_startup_mocks()
+ self._write_mock_config()
+ except Exception as e:
+ self._stop_capture_server()
+ return f"Failed to initialize mock mode: {e}"
addon_path = Path(self.directories.addons) / "mock_addon.py"
if not addon_path.exists():
- self._generate_default_addon(addon_path)
+ try:
+ self._generate_default_addon(addon_path)
+ except Exception as e:
+ self._stop_capture_server()
+ return f"Failed to generate mock addon: {e}"
cmd.extend(["-s", str(addon_path)])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 1238 - 1261, The startup can leave the capture server running if
any of the mode setup functions (_load_startup_mocks, _write_mock_config,
_generate_default_addon, or _apply_mode_flags) raise; modify start so that after
calling _start_capture_server() you wrap the subsequent mode setup/config calls
in a try/except (or try/finally) that calls _stop_capture_server() on any
exception and then re-raises the error; specifically ensure exceptions from
_load_startup_mocks are caught and the server is stopped before propagating,
mirroring the existing replay-path behavior that calls _stop_capture_server() on
error.
mangelajo
reviewed
Feb 25, 2026
| body: | ||
| log_level: info | ||
| features: | ||
| ota_updates: true |
Member
There was a problem hiding this comment.
I would perhaps just use raw json in here in a : |
format...
then the result can be anything really. May be add the content type of the result too?
Member
Author
There was a problem hiding this comment.
@mangelajo I have replaced it with raw JSON or a JSON file for long bodies.
Member
There was a problem hiding this comment.
it still looks as yaml, but I am fine with it, we can always add body_raw or similar later in time if we need :-)
mangelajo
reviewed
Feb 25, 2026
Member
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
This is a super useful integration testing tool! 🧪
very cool
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py (1)
919-925: Path traversal guard is still bypassable (prefix check).Line 923 uses
str(...).startswith(...), which is not a safe containment check for filesystem paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py` around lines 919 - 925, The current path traversal guard in bundled_addon.py uses a string prefix check between file_path and files_dir_resolved which can be bypassed; replace that check with a proper path containment test such as using Path.is_relative_to(files_dir_resolved) (or call file_path.relative_to(files_dir_resolved) in a try/except ValueError) inside the same try block where file_path and files_dir_resolved are resolved, and keep the ctx.log.error/return None on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 1403-1410: The loop over _collect_file_entries currently accepts
arbitrary paths for file_ref and builds file_path = base_dir / file_ref; change
it to reject absolute paths and any path containing parent-traversal before
using it: ensure file_ref is a relative path (not starting with "/" or a drive
letter), normalize and resolve file_path (e.g., file_path.resolve()) and verify
that the resolved path is a strict descendant of base_dir.resolve() (or equal)
before reading and calling client.upload_mock_file; also ensure
file_path.is_file() and exists() and skip/log entries that fail validation.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1167-1175: The code assumes `endpoints` is a mapping before
calling `_convert_url_endpoints`, which can raise if `endpoints` is a non-object
YAML/JSON value; add a guard after computing `endpoints` to verify it is a dict
(mapping) and handle invalid shapes (e.g., log or raise a clear TypeError, or
fallback to an empty dict) before assigning `self._mock_endpoints =
_convert_url_endpoints(endpoints)` so `_convert_url_endpoints` always receives a
dict; reference the local variables `raw`, `endpoints`, `self._mock_endpoints`
and the function `_convert_url_endpoints` when implementing the check.
- Around line 1608-1611: The code trusts relative_path and writes to dest
without validation, allowing absolute paths or directory traversal; ensure the
upload destination stays inside self.directories.files by resolving both the
base directory (self.directories.files) and the requested path
(Path(relative_path)) to absolute paths and verifying the resolved destination
is a subpath of the resolved base before creating directories or writing
content_base64 to dest; reject or raise an error if relative_path is absolute or
the resolved dest is not within the base, then proceed to create parents and
write bytes as before (references: relative_path, dest, self.directories.files,
content_base64).
- Around line 207-215: The current sanitisation of url_path into clean still
allows '..' segments and can cause path traversal when creating dest; instead,
split the cleaned path into segments (e.g., via url_path.split("/") or
PurePosixPath.parts), filter out empty segments, '.' and '..', rejoin using safe
'/' separators (or use Path with only those filtered segments), then build rel
and dest from that safe segment list; after constructing dest, resolve both dest
and files_dir (files_dir.resolve()) and verify dest is inside files_dir (e.g.,
ensure dest.resolve().is_relative_to(files_dir.resolve()) or compare string
prefixes) and fail or fall back to a safe filename if it is not, so variables to
change are clean, rel, dest and the creation of dest.parent and
dest.write_bytes(data).
- Around line 1581-1583: The code joins a user-controlled relative_path into src
(Path(self.directories.files) / relative_path) without validation, allowing path
traversal; update the logic around the src construction (the block that sets src
and checks src.exists()) to first reject absolute paths and any paths containing
parent-traversal components, then resolve the final path and ensure it is inside
the export base directory (self.directories.files) before reading; use
Path.resolve() and either Path.is_relative_to(self.directories.files) or a
commonpath comparison to enforce the containment check and return/raise if
validation fails.
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.py`:
- Around line 919-925: The current path traversal guard in bundled_addon.py uses
a string prefix check between file_path and files_dir_resolved which can be
bypassed; replace that check with a proper path containment test such as using
Path.is_relative_to(files_dir_resolved) (or call
file_path.relative_to(files_dir_resolved) in a try/except ValueError) inside the
same try block where file_path and files_dir_resolved are resolved, and keep the
ctx.log.error/return None on failure.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/bundled_addon.pypython/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.pypython/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Comment on lines
+1403
to
+1410
| for entry in _collect_file_entries(endpoints): | ||
| if "file" not in entry: | ||
| continue | ||
| file_ref = entry["file"] | ||
| file_path = base_dir / file_ref | ||
| if file_path.exists(): | ||
| click.echo(f" uploading {file_ref}") | ||
| client.upload_mock_file(file_ref, file_path.read_bytes()) |
Contributor
There was a problem hiding this comment.
Restrict companion file references to the scenario directory.
Line 1407 allows absolute/parent-traversal paths from file: entries, which can read arbitrary local files and upload them.
🔒 Proposed fix
base_dir = scenario_path.parent
for entry in _collect_file_entries(endpoints):
if "file" not in entry:
continue
- file_ref = entry["file"]
- file_path = base_dir / file_ref
+ file_ref = entry["file"]
+ if not isinstance(file_ref, str):
+ continue
+ file_path = (base_dir / file_ref).resolve()
+ if not file_path.is_relative_to(base_dir.resolve()):
+ click.echo(f" skipped unsafe file path: {file_ref}")
+ continue
if file_path.exists():
click.echo(f" uploading {file_ref}")
client.upload_mock_file(file_ref, file_path.read_bytes())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`
around lines 1403 - 1410, The loop over _collect_file_entries currently accepts
arbitrary paths for file_ref and builds file_path = base_dir / file_ref; change
it to reject absolute paths and any path containing parent-traversal before
using it: ensure file_ref is a relative path (not starting with "/" or a drive
letter), normalize and resolve file_path (e.g., file_path.resolve()) and verify
that the resolved path is a strict descendant of base_dir.resolve() (or equal)
before reading and calling client.upload_mock_file; also ensure
file_path.is_file() and exists() and skip/log entries that fail validation.
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Show resolved
Hide resolved
Comment on lines
+1167
to
+1175
| # Handle v2 format (with "endpoints" wrapper) or v1 flat format | ||
| if "endpoints" in raw: | ||
| endpoints = raw["endpoints"] | ||
| else: | ||
| endpoints = raw | ||
|
|
||
| # Convert URL-keyed endpoints to METHOD /path format | ||
| self._mock_endpoints = _convert_url_endpoints(endpoints) | ||
|
|
Contributor
There was a problem hiding this comment.
Guard scenario shape before endpoint conversion.
Line 1174 assumes endpoints is a dict; non-object YAML/JSON can raise at runtime (.items() access in _convert_url_endpoints).
🛠️ Proposed fix
# Handle v2 format (with "endpoints" wrapper) or v1 flat format
if "endpoints" in raw:
endpoints = raw["endpoints"]
else:
endpoints = raw
+ if not isinstance(endpoints, dict):
+ return "Invalid scenario: expected an object of endpoints"
# Convert URL-keyed endpoints to METHOD /path format
self._mock_endpoints = _convert_url_endpoints(endpoints)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 1167 - 1175, The code assumes `endpoints` is a mapping before
calling `_convert_url_endpoints`, which can raise if `endpoints` is a non-object
YAML/JSON value; add a guard after computing `endpoints` to verify it is a dict
(mapping) and handle invalid shapes (e.g., log or raise a clear TypeError, or
fallback to an empty dict) before assigning `self._mock_endpoints =
_convert_url_endpoints(endpoints)` so `_convert_url_endpoints` always receives a
dict; reference the local variables `raw`, `endpoints`, `self._mock_endpoints`
and the function `_convert_url_endpoints` when implementing the check.
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
Show resolved
Hide resolved
Comment on lines
+1608
to
+1611
| dest = Path(self.directories.files) / relative_path | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_bytes(base64.b64decode(content_base64)) | ||
| return f"Uploaded: {relative_path} ({dest.stat().st_size} bytes)" |
Contributor
There was a problem hiding this comment.
Validate upload destination path to prevent arbitrary file writes.
Line 1608 trusts relative_path; absolute or parent-traversal paths can write outside files_dir.
🔒 Proposed fix
def upload_mock_file(self, relative_path: str, content_base64: str) -> str:
@@
- dest = Path(self.directories.files) / relative_path
+ base = Path(self.directories.files).resolve()
+ dest = (base / relative_path).resolve()
+ if not dest.is_relative_to(base):
+ return f"Invalid file path: {relative_path}"
dest.parent.mkdir(parents=True, exist_ok=True)
dest.write_bytes(base64.b64decode(content_base64))
return f"Uploaded: {relative_path} ({dest.stat().st_size} bytes)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 1608 - 1611, The code trusts relative_path and writes to dest
without validation, allowing absolute paths or directory traversal; ensure the
upload destination stays inside self.directories.files by resolving both the
base directory (self.directories.files) and the requested path
(Path(relative_path)) to absolute paths and verifying the resolved destination
is a subpath of the resolved base before creating directories or writing
content_base64 to dest; reject or raise an error if relative_path is absolute or
the resolved dest is not within the base, then proceed to create parents and
write bytes as before (references: relative_path, dest, self.directories.files,
content_base64).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py (8)
1087-1087:⚠️ Potential issue | 🟠 Major
json.loads(value_json)inset_stateis unguarded.An invalid JSON payload from the client propagates as an unhandled
JSONDecodeErrorthrough the RPC layer, unlikeset_mock_sequence/set_mock_conditionalwhich catch and return a friendly error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` at line 1087, In set_state, json.loads(value_json) is unguarded and can raise JSONDecodeError; wrap the parse in a try/except (catch json.JSONDecodeError) around the json.loads call in the set_state function, mirror the error handling used by set_mock_sequence/set_mock_conditional (return the same friendly RPC error/response with a descriptive message and the invalid payload info) so malformed JSON from the client is caught and a consistent error is returned instead of letting the exception propagate.
783-789:⚠️ Potential issue | 🟠 Major
json.loads(body)/json.loads(headers)inset_mockraise unhandledJSONDecodeError.Unlike
set_mock_sequence/set_mock_template/set_mock_conditional, this method has notry/exceptaround thejson.loadscalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 783 - 789, In set_mock, JSONDecodeError can be raised by json.loads(body) and json.loads(headers); wrap each json.loads call in a try/except that catches json.JSONDecodeError and raises a ValueError (or otherwise mirrors the error handling used in set_mock_sequence / set_mock_template / set_mock_conditional) with a clear message identifying the endpoint key and whether the failure was parsing "body" or "headers", then proceed to set self._mock_endpoints[key] and call self._write_mock_config() as before.
1731-1736:⚠️ Potential issue | 🟡 Minor
_capture_accept_loopoverwrites_capture_reader_threadon each new connection.Only the last reader thread is joined in
_stop_capture_server. Prior reader threads for dropped/reconnected addon connections become un-joinable orphans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1731 - 1736, _capture_accept_loop currently assigns a single Thread to the instance attribute _capture_reader_thread for every new connection, so previous reader threads become orphaned and only the last one is joined in _stop_capture_server; change this to track all reader threads (e.g., maintain a list or dict like self._capture_reader_threads and append the Thread created in _capture_accept_loop for each conn) and update _stop_capture_server to iterate over and join every thread in that collection; ensure threads are removed from the collection when _capture_read_loop exits to avoid memory leaks.
546-548:⚠️ Potential issue | 🟠 MajorCapture server left running if
_load_startup_mocks()or_write_mock_config()raises.
_start_capture_server()is called before_apply_mode_flags(). Any exception from_load_startup_mocks()or_write_mock_config()leaves the capture socket and background threads orphaned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 546 - 548, The capture server is started before applying mode flags, so if _load_startup_mocks() or _write_mock_config() raises the capture server stays running; fix by either moving the call to _start_capture_server() to after _apply_mode_flags() (so mocks/config are set up before the server starts) or wrap the mode handling in a try/finally that calls self._stop_capture_server() on exception; update code references around _start_capture_server(), _apply_mode_flags(), _load_startup_mocks(), and _write_mock_config() to implement one of these approaches so the capture socket/threads are always cleaned up on error.
1175-1182:⚠️ Potential issue | 🟠 MajorMissing
isinstance(endpoints, dict)guard before_convert_url_endpointsinload_mock_scenario.If the YAML/JSON value under
"endpoints"(or the root object) is a list or scalar,_convert_url_endpointswill raise anAttributeErrororTypeErrorat.items().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1175 - 1182, In load_mock_scenario, ensure the value passed to _convert_url_endpoints is a mapping: check isinstance(endpoints, dict) (after selecting raw["endpoints"] or raw) and if not, raise a clear error (e.g., ValueError with context including the raw type and scenario name) or coerce/skip as appropriate; specifically update the block that sets endpoints and the subsequent call to _convert_url_endpoints so you only call _convert_url_endpoints(endpoints) when endpoints is a dict, otherwise handle the invalid type with an explanatory exception or fallback behavior.
1649-1652:⚠️ Potential issue | 🔴 Critical
upload_mock_fileallows arbitrary file writes via path traversal.
relative_path(client-controlled) is joined tofiles_dirwithout a containment check. An absolute or../-prefixed path writes outside the intended directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1649 - 1652, The upload_mock_file handler currently concatenates client-controlled relative_path to self.directories.files allowing absolute/../ path traversal; fix by normalizing and validating the target path: construct dest = (Path(self.directories.files) / relative_path).resolve(), ensure it is inside Path(self.directories.files).resolve() (e.g., dest_path_str.startswith(base_path_str) or use Path comparison), reject or raise an error for absolute paths or any dest outside the base directory, then create parents and write the file as before; update the function (upload_mock_file / the block that sets dest, dest.parent.mkdir, dest.write_bytes) to perform this containment check before writing.
207-216:⚠️ Potential issue | 🔴 CriticalPath traversal in
_write_captured_filestill allows writes outsidefiles_dir.The sanitisation at Line 209 permits both
.and/, so..sequences survive (e.g.,/api/../../etc/shadow). Thedest = files_dir / relat Line 212 then resolves outside the intended directory. Theis_relative_toguard from the previous review is not present in the current code.🔒 Proposed fix — strip `..` segments and verify containment
def _write_captured_file( method: str, url_path: str, ext: str, data: bytes, endpoint: dict, files_dir: Path, ) -> str: clean = url_path.lstrip("/") clean = "".join(c if (c.isalnum() or c in "/-_.") else "_" for c in clean) clean = clean.strip("_") + # Remove empty, current-dir, and parent-dir path segments + clean = "/".join(p for p in clean.split("/") if p not in ("", ".", "..")) + if not clean: + clean = "root" rel = f"responses/{method}/{clean}{ext}" - dest = files_dir / rel + base = files_dir.resolve() + dest = (files_dir / rel).resolve() + if not dest.is_relative_to(base): + raise ValueError(f"Unsafe export path from URL: {url_path!r}") dest.parent.mkdir(parents=True, exist_ok=True) dest.write_bytes(data) endpoint["file"] = rel return rel🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 207 - 216, In _write_captured_file: the current sanitisation allows ".." segments so rel can escape files_dir; instead remove or collapse any "."/".." path segments before constructing rel (e.g., split on "/" and skip empty, "." and ".." segments) and then build rel from the safe segments plus ext; after computing dest = files_dir / rel, resolve both dest and files_dir and assert dest.is_relative_to(files_dir) (or compare resolved paths) and raise/abort if not contained, then write_bytes and set endpoint["file"] only after the containment check passes.
1622-1624:⚠️ Potential issue | 🔴 Critical
get_captured_fileis vulnerable to path traversal attacks.The method at line 1609 joins
relative_pathdirectly toself.directories.fileswithout validating that the resolved path remains within the base directory. An attacker can passrelative_path = "../../etc/passwd"or other traversal sequences to read arbitrary files from the exporter host.Recommended fix
src = Path(self.directories.files) / relative_path + base = Path(self.directories.files).resolve() + src = (base / relative_path).resolve() + if not src.is_relative_to(base): + logger.error("Blocked path traversal in get_captured_file: %s", relative_path) + return if not src.exists(): return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 1622 - 1624, The get_captured_file code currently constructs src = Path(self.directories.files) / relative_path and returns if not src.exists(), which allows path traversal; fix by resolving both the base directory (base = Path(self.directories.files).resolve()) and the candidate path (candidate = (base / relative_path).resolve()), then verify candidate is inside base (e.g., candidate == base or base in candidate.parents or use os.path.commonpath) and reject or raise an error if it is outside; ensure you still check candidate.exists() before returning and handle symlinks by using resolve().python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py (2)
1431-1438:⚠️ Potential issue | 🔴 CriticalPath traversal via
file:entries is still unguarded.
file_reffrom the scenario YAML is joined directly tobase_dirwithout validating that the resolved path stays insidebase_dir. Absolute paths (e.g.,file: /etc/passwd) and..sequences will upload arbitrary local files to the exporter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py` around lines 1431 - 1438, The loop processing entries from _collect_file_entries uses file_ref joined to base_dir without validating the resolved path; prevent path traversal by resolving both base_dir and the candidate file_path (e.g., file_path = (base_dir / file_ref).resolve()) and then verify the resolved file_path is under the resolved base_dir (use Path.is_relative_to or compare os.path.commonpath of their str() values); skip or error if the check fails (also reject absolute paths or entries that resolve outside base_dir) before calling client.upload_mock_file(file_ref, ...). Ensure you reference the same symbols: _collect_file_entries, file_ref, base_dir, file_path, and client.upload_mock_file.
914-917:⚠️ Potential issue | 🟡 MinorFragile error detection via
startswith("Error:")— structural check preferred.Any change to the driver's error prefix silently breaks
RuntimeErrorpropagation here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py` around lines 914 - 917, Replace the fragile string-prefix check with structural error handling: change the RPC helper self.call("get_ca_cert") to either raise an exception on failure or return a structured response (e.g., a dict with an "error" key), then update this method (the block with result = self.call("get_ca_cert")) to handle that structure — either wrap the call in try/except and re-raise a RuntimeError with the caught exception, or if call returns a dict check isinstance(result, dict) and if "error" in result raise RuntimeError(result["error"]) and otherwise return the successful payload.
🧹 Nitpick comments (4)
python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py (2)
1-11: Demo test file noted —driver_test.pyis the canonical test location per guidelines.This demo is useful for showcasing driver capabilities. Per coding guidelines, the primary tests for the driver package belong in
driver_test.py. Ensuredriver_test.pycovers the core driver behaviors exercised here. Based on learnings, "Driver package tests should focus on end-to-end testing with server and client."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py` around lines 1 - 11, This demo test content belongs in the canonical driver test module: move the end-to-end demo tests from test_demo.py into driver_test.py and ensure the tests exercise the core driver behaviors (server startup, request/response flows through the proxy) using the existing backend server fixture and the proxy fixture; update imports and test class/function names as needed, remove or convert test_demo.py to a minimal example-only file, and run pytest to verify the driver_test.py covers the same scenarios previously shown in test_demo.py.
82-83:time.sleep(1)for mock hot-reload is unnecessary and makes tests fragile.The bundled addon reloads
endpoints.jsonsynchronously in itsrequest()hook (checked at every incoming request). No pre-request sleep is needed. Remove thetime.sleep(1)calls before HTTP requests; they only add test latency without providing synchronization guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py` around lines 82 - 83, Remove the unnecessary time.sleep(1) call(s) used to wait for the mock hot-reload in test_demo.py: delete the time.sleep(1) lines that precede the HTTP requests (they add latency and are fragile); the bundled addon's synchronous reload in its request() hook already checks endpoints.json on each incoming request so no pre-request sleep is required. Ensure any tests that depended on the delay still perform the HTTP request immediately after setup and assert responses normally.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py (1)
439-440:self.listen.port = portpermanently mutates the config on everystart()call.Once
start(port=X)is called, subsequentrestart()andstop()/start()invocations will useXeven ifport=0is passed. This is intentional but should be documented, or the override should be stored separately so the original config is preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py` around lines 439 - 440, The code currently mutates the stored config by assigning the runtime port in start() via self.listen.port = port; instead, preserve the original config and store the runtime override separately (e.g., set a new attribute like self._override_port when start(port=...) is called) and use that override only when binding/listening (referenced where listen is used), or if you prefer the current behavior document it clearly in start(), restart(), and stop() docs; ensure you remove direct assignment to self.listen.port and read the effective port from self._override_port or fallback to the original self.listen.port when performing the actual bind.python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py (1)
493-495:stopEvent is never set — misleading name.The
Eventobject is used solely as a timed sleep (to keep the main thread responsive toKeyboardInterrupt), butstop.wait(timeout=0.5)always times out and returnsFalse. Consider renaming it_tickeror replacing it with a plaintime.sleep(0.5)inside the loop for clarity.♻️ Suggested simplification
- stop = Event() - while not stop.wait(timeout=0.5): - pass + import time as _time + while True: + _time.sleep(0.5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py` around lines 493 - 495, The local Event named "stop" is never set and is used only as a timed sleep; update the loop in jumpstarter_driver_mitmproxy.client (the block creating stop = Event() and while not stop.wait(timeout=0.5): pass) to either rename the variable to something like _ticker to avoid implying it will be set, or replace the Event usage with a plain time.sleep(0.5) (add the time import if needed) so the intent is clear and no misleading "stop" Event remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py`:
- Around line 141-202: Test uses load_mock_scenario with client-side absolute
paths (str(SCENARIOS_DIR / "...")) which fails for remote exporters; replace
each call to proxy_client.load_mock_scenario(...) in TestScenarioLoading
(including inside mock_scenario context manager) with
proxy_client.load_mock_scenario_content(...), reading the scenario file content
on the test runner (e.g. open(SCENARIOS_DIR / "happy-path").read()) and passing
that string so the driver sends the scenario payload to the exporter instead of
a local path; ensure all four scenario-loading sites are updated to use
load_mock_scenario_content and that the mock_scenario context usage similarly
loads content before entering the context.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 1386-1402: The function _collect_file_entries currently skips
endpoints whose value is a list (v2 format); update it to handle list-valued
endpoints by normalizing ep into an iterable of candidate entries (e.g., treat
single dicts and lists uniformly) and then for each candidate dict append it and
also inspect its "rules" and "sequence" children the same way; ensure you check
isinstance(candidate, dict) before appending and when iterating nested "rules"
and "sequence" so _upload_scenario_files can discover file: references in v2
scenarios.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 1241-1244: The function list_flow_files currently calls Path.glob
on self.directories.flows which raises FileNotFoundError if the flows directory
doesn't exist yet; update list_flow_files to first check whether
Path(self.directories.flows).exists() (or is_dir()) and return an empty list
immediately if not, otherwise proceed with the existing for-loop that sorts
glob("*.bin") and collects file stats; this keeps directory creation lazy in
start() while preventing FileNotFoundError when list_flow_files is called
earlier.
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 1431-1438: The loop processing entries from _collect_file_entries
uses file_ref joined to base_dir without validating the resolved path; prevent
path traversal by resolving both base_dir and the candidate file_path (e.g.,
file_path = (base_dir / file_ref).resolve()) and then verify the resolved
file_path is under the resolved base_dir (use Path.is_relative_to or compare
os.path.commonpath of their str() values); skip or error if the check fails
(also reject absolute paths or entries that resolve outside base_dir) before
calling client.upload_mock_file(file_ref, ...). Ensure you reference the same
symbols: _collect_file_entries, file_ref, base_dir, file_path, and
client.upload_mock_file.
- Around line 914-917: Replace the fragile string-prefix check with structural
error handling: change the RPC helper self.call("get_ca_cert") to either raise
an exception on failure or return a structured response (e.g., a dict with an
"error" key), then update this method (the block with result =
self.call("get_ca_cert")) to handle that structure — either wrap the call in
try/except and re-raise a RuntimeError with the caught exception, or if call
returns a dict check isinstance(result, dict) and if "error" in result raise
RuntimeError(result["error"]) and otherwise return the successful payload.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Line 1087: In set_state, json.loads(value_json) is unguarded and can raise
JSONDecodeError; wrap the parse in a try/except (catch json.JSONDecodeError)
around the json.loads call in the set_state function, mirror the error handling
used by set_mock_sequence/set_mock_conditional (return the same friendly RPC
error/response with a descriptive message and the invalid payload info) so
malformed JSON from the client is caught and a consistent error is returned
instead of letting the exception propagate.
- Around line 783-789: In set_mock, JSONDecodeError can be raised by
json.loads(body) and json.loads(headers); wrap each json.loads call in a
try/except that catches json.JSONDecodeError and raises a ValueError (or
otherwise mirrors the error handling used in set_mock_sequence /
set_mock_template / set_mock_conditional) with a clear message identifying the
endpoint key and whether the failure was parsing "body" or "headers", then
proceed to set self._mock_endpoints[key] and call self._write_mock_config() as
before.
- Around line 1731-1736: _capture_accept_loop currently assigns a single Thread
to the instance attribute _capture_reader_thread for every new connection, so
previous reader threads become orphaned and only the last one is joined in
_stop_capture_server; change this to track all reader threads (e.g., maintain a
list or dict like self._capture_reader_threads and append the Thread created in
_capture_accept_loop for each conn) and update _stop_capture_server to iterate
over and join every thread in that collection; ensure threads are removed from
the collection when _capture_read_loop exits to avoid memory leaks.
- Around line 546-548: The capture server is started before applying mode flags,
so if _load_startup_mocks() or _write_mock_config() raises the capture server
stays running; fix by either moving the call to _start_capture_server() to after
_apply_mode_flags() (so mocks/config are set up before the server starts) or
wrap the mode handling in a try/finally that calls self._stop_capture_server()
on exception; update code references around _start_capture_server(),
_apply_mode_flags(), _load_startup_mocks(), and _write_mock_config() to
implement one of these approaches so the capture socket/threads are always
cleaned up on error.
- Around line 1175-1182: In load_mock_scenario, ensure the value passed to
_convert_url_endpoints is a mapping: check isinstance(endpoints, dict) (after
selecting raw["endpoints"] or raw) and if not, raise a clear error (e.g.,
ValueError with context including the raw type and scenario name) or coerce/skip
as appropriate; specifically update the block that sets endpoints and the
subsequent call to _convert_url_endpoints so you only call
_convert_url_endpoints(endpoints) when endpoints is a dict, otherwise handle the
invalid type with an explanatory exception or fallback behavior.
- Around line 1649-1652: The upload_mock_file handler currently concatenates
client-controlled relative_path to self.directories.files allowing absolute/../
path traversal; fix by normalizing and validating the target path: construct
dest = (Path(self.directories.files) / relative_path).resolve(), ensure it is
inside Path(self.directories.files).resolve() (e.g.,
dest_path_str.startswith(base_path_str) or use Path comparison), reject or raise
an error for absolute paths or any dest outside the base directory, then create
parents and write the file as before; update the function (upload_mock_file /
the block that sets dest, dest.parent.mkdir, dest.write_bytes) to perform this
containment check before writing.
- Around line 207-216: In _write_captured_file: the current sanitisation allows
".." segments so rel can escape files_dir; instead remove or collapse any
"."/".." path segments before constructing rel (e.g., split on "/" and skip
empty, "." and ".." segments) and then build rel from the safe segments plus
ext; after computing dest = files_dir / rel, resolve both dest and files_dir and
assert dest.is_relative_to(files_dir) (or compare resolved paths) and
raise/abort if not contained, then write_bytes and set endpoint["file"] only
after the containment check passes.
- Around line 1622-1624: The get_captured_file code currently constructs src =
Path(self.directories.files) / relative_path and returns if not src.exists(),
which allows path traversal; fix by resolving both the base directory (base =
Path(self.directories.files).resolve()) and the candidate path (candidate =
(base / relative_path).resolve()), then verify candidate is inside base (e.g.,
candidate == base or base in candidate.parents or use os.path.commonpath) and
reject or raise an error if it is outside; ensure you still check
candidate.exists() before returning and handle symlinks by using resolve().
---
Nitpick comments:
In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py`:
- Around line 1-11: This demo test content belongs in the canonical driver test
module: move the end-to-end demo tests from test_demo.py into driver_test.py and
ensure the tests exercise the core driver behaviors (server startup,
request/response flows through the proxy) using the existing backend server
fixture and the proxy fixture; update imports and test class/function names as
needed, remove or convert test_demo.py to a minimal example-only file, and run
pytest to verify the driver_test.py covers the same scenarios previously shown
in test_demo.py.
- Around line 82-83: Remove the unnecessary time.sleep(1) call(s) used to wait
for the mock hot-reload in test_demo.py: delete the time.sleep(1) lines that
precede the HTTP requests (they add latency and are fragile); the bundled
addon's synchronous reload in its request() hook already checks endpoints.json
on each incoming request so no pre-request sleep is required. Ensure any tests
that depended on the delay still perform the HTTP request immediately after
setup and assert responses normally.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`:
- Around line 493-495: The local Event named "stop" is never set and is used
only as a timed sleep; update the loop in jumpstarter_driver_mitmproxy.client
(the block creating stop = Event() and while not stop.wait(timeout=0.5): pass)
to either rename the variable to something like _ticker to avoid implying it
will be set, or replace the Event usage with a plain time.sleep(0.5) (add the
time import if needed) so the intent is clear and no misleading "stop" Event
remains.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`:
- Around line 439-440: The code currently mutates the stored config by assigning
the runtime port in start() via self.listen.port = port; instead, preserve the
original config and store the runtime override separately (e.g., set a new
attribute like self._override_port when start(port=...) is called) and use that
override only when binding/listening (referenced where listen is used), or if
you prefer the current behavior document it clearly in start(), restart(), and
stop() docs; ensure you remove direct assignment to self.listen.port and read
the effective port from self._override_port or fallback to the original
self.listen.port when performing the actual bind.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
python/packages/jumpstarter-driver-mitmproxy/README.mdpython/packages/jumpstarter-driver-mitmproxy/demo/exporter.yamlpython/packages/jumpstarter-driver-mitmproxy/demo/scenarios/backend-outage/scenario.yamlpython/packages/jumpstarter-driver-mitmproxy/demo/scenarios/happy-path/scenario.yamlpython/packages/jumpstarter-driver-mitmproxy/demo/scenarios/update-available/scenario.yamlpython/packages/jumpstarter-driver-mitmproxy/demo/test_demo.pypython/packages/jumpstarter-driver-mitmproxy/examples/exporter.yamlpython/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.pypython/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-mitmproxy/demo/scenarios/update-available/scenario.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-mitmproxy/examples/exporter.yaml
- python/packages/jumpstarter-driver-mitmproxy/demo/exporter.yaml
- python/packages/jumpstarter-driver-mitmproxy/README.md
Comment on lines
+141
to
+202
| def test_load_happy_path( | ||
| self, backend_server, proxy_client, http_session, | ||
| ): | ||
| proxy_client.load_mock_scenario(str(SCENARIOS_DIR / "happy-path")) | ||
| time.sleep(1) | ||
|
|
||
| resp = http_session.get( | ||
| f"{BACKEND_URL}/api/v1/status", timeout=10, | ||
| ) | ||
| data = resp.json() | ||
| assert data["source"] == "mock" | ||
| assert data["device_id"] == "DUT-MOCK-999" | ||
| assert data["firmware_version"] == "2.5.1" | ||
|
|
||
| def test_load_update_available( | ||
| self, backend_server, proxy_client, http_session, | ||
| ): | ||
| proxy_client.load_mock_scenario( | ||
| str(SCENARIOS_DIR / "update-available"), | ||
| ) | ||
| time.sleep(1) | ||
|
|
||
| resp = http_session.get( | ||
| f"{BACKEND_URL}/api/v1/updates/check", timeout=10, | ||
| ) | ||
| data = resp.json() | ||
| assert data["source"] == "mock" | ||
| assert data["update_available"] is True | ||
| assert data["latest_version"] == "3.0.0" | ||
|
|
||
| def test_load_backend_outage( | ||
| self, backend_server, proxy_client, http_session, | ||
| ): | ||
| proxy_client.load_mock_scenario( | ||
| str(SCENARIOS_DIR / "backend-outage"), | ||
| ) | ||
| time.sleep(1) | ||
|
|
||
| resp = http_session.get( | ||
| f"{BACKEND_URL}/api/v1/status", timeout=10, | ||
| ) | ||
| assert resp.status_code == 503 | ||
| assert resp.json()["error"] == "Service Unavailable" | ||
|
|
||
| def test_mock_scenario_context_manager( | ||
| self, backend_server, proxy_client, http_session, | ||
| ): | ||
| """mock_scenario() loads on entry, clears on exit.""" | ||
| with proxy_client.mock_scenario( | ||
| str(SCENARIOS_DIR / "happy-path"), | ||
| ): | ||
| time.sleep(1) | ||
| resp = http_session.get( | ||
| f"{BACKEND_URL}/api/v1/status", timeout=10, | ||
| ) | ||
| assert resp.json()["source"] == "mock" | ||
|
|
||
| # After exit: mocks cleared → passthrough to real backend | ||
| time.sleep(1) | ||
| resp = http_session.get( | ||
| f"{BACKEND_URL}/api/v1/status", timeout=10, | ||
| ) |
Contributor
There was a problem hiding this comment.
load_mock_scenario passes client-side absolute paths — breaks remote exporter usage.
str(SCENARIOS_DIR / "happy-path") resolves to an absolute path on the test client machine. The driver's load_mock_scenario then tries to open that path on the exporter host, which won't exist in a real remote setup.
Use load_mock_scenario_content (which transmits the file content) to make these tests portable:
🐛 Proposed fix for load_mock_scenario calls
- proxy_client.load_mock_scenario(str(SCENARIOS_DIR / "happy-path"))
+ scenario_path = SCENARIOS_DIR / "happy-path" / "scenario.yaml"
+ proxy_client.load_mock_scenario_content(
+ scenario_path.name, scenario_path.read_text()
+ )The same fix applies to all four scenario-loading calls in TestScenarioLoading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-driver-mitmproxy/demo/test_demo.py` around lines
141 - 202, Test uses load_mock_scenario with client-side absolute paths
(str(SCENARIOS_DIR / "...")) which fails for remote exporters; replace each call
to proxy_client.load_mock_scenario(...) in TestScenarioLoading (including inside
mock_scenario context manager) with
proxy_client.load_mock_scenario_content(...), reading the scenario file content
on the test runner (e.g. open(SCENARIOS_DIR / "happy-path").read()) and passing
that string so the driver sends the scenario payload to the exporter instead of
a local path; ensure all four scenario-loading sites are updated to use
load_mock_scenario_content and that the mock_scenario context usage similarly
loads content before entering the context.
Comment on lines
+1386
to
+1402
| def _collect_file_entries(endpoints: dict) -> list[dict]: | ||
| """Collect all dicts that might contain a ``file`` key from a scenario. | ||
|
|
||
| Walks top-level endpoints plus nested ``rules`` and ``sequence`` entries. | ||
| """ | ||
| entries: list[dict] = [] | ||
| for ep in endpoints.values(): | ||
| if not isinstance(ep, dict): | ||
| continue | ||
| entries.append(ep) | ||
| for rule in ep.get("rules", []): | ||
| if isinstance(rule, dict): | ||
| entries.append(rule) | ||
| for step in ep.get("sequence", []): | ||
| if isinstance(step, dict): | ||
| entries.append(step) | ||
| return entries |
Contributor
There was a problem hiding this comment.
_collect_file_entries silently skips all v2 URL-keyed list endpoints.
In the v2 scenario format (e.g., happy-path/scenario.yaml), each endpoint value is a list of response entries. The isinstance(ep, dict) guard at Line 1393 causes the function to skip every such list, so _upload_scenario_files never finds or uploads any file: references from v2 scenarios.
🐛 Proposed fix — handle list-valued endpoints
def _collect_file_entries(endpoints: dict) -> list[dict]:
entries: list[dict] = []
for ep in endpoints.values():
- if not isinstance(ep, dict):
- continue
- entries.append(ep)
- for rule in ep.get("rules", []):
- if isinstance(rule, dict):
- entries.append(rule)
- for step in ep.get("sequence", []):
- if isinstance(step, dict):
- entries.append(step)
+ if isinstance(ep, list):
+ # v2 URL-keyed format: list of response entry dicts
+ for item in ep:
+ if isinstance(item, dict):
+ entries.append(item)
+ for rule in item.get("rules", []):
+ if isinstance(rule, dict):
+ entries.append(rule)
+ for step in item.get("sequence", []):
+ if isinstance(step, dict):
+ entries.append(step)
+ elif isinstance(ep, dict):
+ entries.append(ep)
+ for rule in ep.get("rules", []):
+ if isinstance(rule, dict):
+ entries.append(rule)
+ for step in ep.get("sequence", []):
+ if isinstance(step, dict):
+ entries.append(step)
return entries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/client.py`
around lines 1386 - 1402, The function _collect_file_entries currently skips
endpoints whose value is a list (v2 format); update it to handle list-valued
endpoints by normalizing ep into an iterable of candidate entries (e.g., treat
single dicts and lists uniformly) and then for each candidate dict append it and
also inspect its "rules" and "sequence" children the same way; ensure you check
isinstance(candidate, dict) before appending and when iterating nested "rules"
and "sequence" so _upload_scenario_files can discover file: references in v2
scenarios.
Comment on lines
+1241
to
+1244
| flow_path = Path(self.directories.flows) | ||
| files = [] | ||
| for f in sorted(flow_path.glob("*.bin")): | ||
| stat = f.stat() |
Contributor
There was a problem hiding this comment.
list_flow_files raises FileNotFoundError if the flows directory doesn't exist yet.
Path.glob() on a non-existent directory raises FileNotFoundError. The directory is created lazily in start(), so calling list_flow_files() before the first start() call fails.
🐛 Proposed fix
flow_path = Path(self.directories.flows)
+ if not flow_path.exists():
+ return json.dumps([])
files = []
for f in sorted(flow_path.glob("*.bin")):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-mitmproxy/jumpstarter_driver_mitmproxy/driver.py`
around lines 1241 - 1244, The function list_flow_files currently calls Path.glob
on self.directories.flows which raises FileNotFoundError if the flows directory
doesn't exist yet; update list_flow_files to first check whether
Path(self.directories.flows).exists() (or is_dir()) and return an empty list
immediately if not, otherwise proceed with the existing for-loop that sorts
glob("*.bin") and collects file stats; this keeps directory creation lazy in
start() while preventing FileNotFoundError when list_flow_files is called
earlier.
mangelajo
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
jumpstarter-driver-mitmproxy, a new driver that manages amitmdump/mitmwebsubprocess on the exporter host for HTTP(S) traffic interception, backend mocking, recording, and replay — purpose-built for Hardware-in-the-Loop (HiL) testingjmp shellMotivation
When testing connected devices, tests often need to control the HTTP backend the device communicates with — simulating outages, injecting specific responses, verifying what requests the device makes, or recording real traffic for later replay. This driver wraps mitmproxy to provide all of these capabilities through the Jumpstarter interface, making backend behavior a first-class part of the test harness.
Driver capabilities
Operational modes:
Mock endpoint features:
{{now_iso}},{{uuid}},{{random_int()}},{{state(key)}}, etc.)Test infrastructure:
wait_for_request()andassert_request_made()helperssession(),mock_endpoint(),mock_scenario(),capture(),recording()start,stop,status,mock,scenario,capture,web(port-forward mitmweb UI),cert(download CA certificate)Scenario system:
config+endpointsstructurehappy-path,update-available,backend-outageOther changes
jumpstarter-driver-ridesx: Simplify error handling in flash operations — removeRideSXFlashErrorwrapper class and redundant try/except nesting in favor of letting exceptions propagate naturally withtry/finallyfor cleanupjumpstarter/driver/base.py: Inline_resource_from_client_streamand_resource_from_presignedhelper methods back intoresource()and remove error-wrapping logic in presigned request handlingjumpstarter/client/core.py: Remove_format_rpc_errorhelper, passe.details()directly to exception constructorsjumpstarter/streams/common.py: Replacetry/exceptwithcontextlib.suppressincopy_streamand remove warning-level logging for expected stream interruptionsSummary by CodeRabbit
Release Notes
New Features
Documentation