fix(ebuild): quote openocd flash path so spaces don't split it - #116
fix(ebuild): quote openocd flash path so spaces don't split it#116Muhammad-Baqir22 wants to merge 2 commits into
Conversation
ebuild flash --tool openocd` interpolates the image path straight into
the -c "program <path> <addr> verify reset exit" argument. OpenOCD's
own Tcl interpreter re-parses that string and splits on whitespace, so
an image path containing a space (e.g. "my firmware.bin", or a Windows
path like "C:\Users\Jane Doe\firmware.bin") was read as multiple
arguments instead of one filename -- OpenOCD tried to flash "my", not
the real file.
Wrap the path in Tcl brace-grouping ({...}) so it parses as one token
regardless of embedded spaces. subprocess.run is called without a
shell, so no other escaping is needed.
Added tests/unit/test_flash.py to cover the regression (spaced path,
plain path, missing image, unknown tool) -- ebuild/firmware/flash.py
had no test coverage before this change.
Signed-off-by: Muhammad-Baqir22 <bakaransari686@gmail.com>
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#116 "fix(ebuild): quote openocd flash path so spaces don't split it"
head: 5fe84fc author: Muhammad-Baqir22 ci: pending (no checks reported)
Verdict: The diagnosis is right, the fix is right, and I reproduced both the bug and the
fix in tclsh rather than taking the PR body's word for it. Conforms to master design §9.1
(eBuild Flash stage) and §21 Tier 1; no dependency-direction change. Four small findings
below, none blocking.
Verification I actually ran
Against an isolated export of 5fe84fc5 (the local ebuild checkout is dirty and was left
untouched):
| Check | Command | Result |
|---|---|---|
| New tests | pytest tests/unit/test_flash.py -q |
4 passed |
| Full suite | pytest -q |
565 passed, 0 skipped |
Lint (CI's own invocation, ci.yml:52) |
ruff check ebuild/firmware/flash.py tests/unit/test_flash.py --select=E,F,W --ignore=E501 |
All checks passed |
Your "562 passed, 3 skipped" reconciles: same 565 collected items, three of which are
environment-conditional skips that ran here. The count is credible.
Tcl behaviour, confirmed directly with tclsh against a stub proc program {file args}:
program /tmp/my firmware.bin 0x08000000 verify reset exit -> FILE=</tmp/my> # the bug
program {/tmp/my firmware.bin} 0x08000000 verify reset exit -> FILE=</tmp/my firmware.bin> # the fix
program {C:\Users\Jane Doe\firmware.bin} ... -> FILE=<C:\Users\Jane Doe\firmware.bin> # Windows case holds
Braces are the correct choice over " here specifically because Tcl performs no backslash
substitution inside a braced word, so Windows separators survive. Worth stating in the code
comment — that is the non-obvious half of why this fix is right.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | EoSim/eosim/integrations/openocd.py:130-133 |
The identical defect is unfixed in EoSim — f'program {firmware_path} verify reset exit' interpolates the path into the same Tcl string with no grouping. Same Tier 1 (§21), same -c re-parse, same silent wrong-file flash. Not a defect in this diff; flagged so the fix does not land in one of two places. |
Out of scope for this PR — do not widen it. Open a follow-up on EoSim applying the same brace grouping. No open EoSim PR covers it (checked gh pr list). |
| 2 | Low | ebuild/firmware/flash.py:57-60 |
The comment says brace grouping "needs no other escaping". It needs one more thing: braces must balance. Verified in tclsh — /tmp/build{v1}/fw.bin parses correctly, but /tmp/fw}.bin raises extra characters after close-brace and /tmp/a{b/fw.bin raises missing close-brace. Tcl gives no way to escape a brace inside a braced word without altering the string. Mitigating: this fails loudly in OpenOCD rather than flashing the wrong file, which is why it is Low and not the original bug again. |
Either narrow the comment to "spaces and backslashes" so it stops overclaiming, or reject the case up front next to the existing Image not found check: raise FlashError when str(image_path) has unbalanced {/}. Prefer the second — §9.2 asks for actionable diagnostics, and "unbalanced brace in image path" beats an OpenOCD Tcl parse error the user has to decode. |
| 3 | Low | tests/unit/test_flash.py:53-55,67 |
Both openocd assertions are built with the same f-string shape as the implementation (f"program {{{image_path}}} …"), so they mirror the code rather than pin the property. They do still fail if the braces are dropped, so the regression is genuinely covered — but a literal expected string would be strictly stronger. Neither the unbalanced-brace case nor the returncode != 0 → FlashError("Flash failed: …") path has a test. |
In test_openocd_program_command_is_unchanged_for_a_plain_path, assert against a literal built from tmp_path without reusing the production format string. Add one test for the non-zero-return failure path while you are in this file. |
| 4 | Low | PR body, "Testing" | - [x] Unit tests pass (ctest --test-dir build --output-on-failure) is ticked on a pure-Python change. ctest was not run and could not have been meaningful here — you say as much yourself when explaining the unticked C-warnings box. Per project rules an unsupported claim is itself the finding, even a checkbox one. |
Untick it and keep the pytest -q line, which is the claim that is actually backed. |
Architecture conformance
Conforms.
- §5.1 architectural law — no new import, link or manifest entry. The diff adds six
characters of string formatting inside an existing function and one test file. No
dependency direction changes, nothing points up a tier. - §21 tier placement —
ebuildis Tier 1 Foundation. Flash-tool invocation is the
Flashleaf of the eBuild engine graph in §9.1;ebuild/firmware/flash.pyis where it
belongs.tests/unit/is the repo's largest test directory (21 files) and the right home
for this. - §9.2 SDK design rules — "actionable diagnostics with remediation guidance" is the rule
finding 2 leans on. Nothing here weakens it; the finding is about extending it. - No public signature, struct layout, manifest field or serialized format changed, so
brief item 8 (API/wire compatibility) does not apply. Nothing is on a hot path. No check
was disabled, loosened or removed. - CHANGELOG entry is in
## [Unreleased] / ### Fixedmatching the surrounding entries, and
correctly describes OpenOCD rather than the shell as the thing doing the re-splitting.
Documentation obligation is met: no user-facing behaviour beyond the bug is different, and
theopenocd -c "program …"snippets incore/eos/GETTING_STARTED.md:112and
core/eos/docs/quickstart-stm32.md:46,50are hand-typed examples with space-free paths,
so this change does not make them wrong.
Proposed changes
Smallest sequence, in order, none of which requires restructuring anything:
-
Untick the
ctestcheckbox in the PR body (finding 4). Nothing else in the body needs
touching — thepytestnumbers hold up. -
In
ebuild/firmware/flash.py, alongside the existing pre-flight checks:if str(image_path).count("{") != str(image_path).count("}"): raise FlashError( f"Image path has unbalanced braces, which OpenOCD's Tcl parser " f"cannot group: {image_path}" )
and trim the comment's "needs no other escaping" to "no other escaping for spaces or
backslashes". Guard it ontool == "openocd"so the other four tools are unaffected. -
Add the two tests from finding 3 to
tests/unit/test_flash.py. -
Separately, a follow-up PR on EoSim for finding 1. Keep it out of this one.
Steps 2–4 are improvements on an already-correct fix. If the maintainers would rather land
the one-line correctness fix now and take these as follow-ups, that is a reasonable call —
the fix as written is right for every path that does not contain a brace.
Not checked
- CI.
checks.txtin the bundle was empty andgh pr checks 116reports "no checks
reported on the branch" — the PR was opened roughly one minute before this review ran.
I am not calling CI green; it has not reported.mergeStateStatusisBLOCKED, which
here reflectsreviewDecision: REVIEW_REQUIRED, not a red check. - Real hardware. No OpenOCD binary and no target were involved. The Tcl argument
parsing is verified against a stocktclshwith a stubprogramproc; OpenOCD's actual
programhelper (which takesfilename argsand substitutes$filenamewithout
re-splitting) is reasoned about from its interface, not executed. - Windows. The
C:\Users\Jane Doe\firmware.bincase is verified at the Tcl parsing
layer on Linux only. No Windows run. - The screenshots in the PR body were not opened or verified; the test evidence above is
my own run, not a reading of those images. - The three environment-conditional skips you saw: they passed in my environment, so I
did not identify which tests they are or why they skip on yours.
Automated architecture review of 5fe84fc5718b — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Review on PR embeddedos-org#116 found two follow-up gaps: - The "needs no other escaping" comment overclaimed: an image path with an unmatched { or } still breaks OpenOCD's Tcl parser (unbalanced brace grouping), just with a confusing OpenOCD-side error instead of a wrong-file flash. Reject it up front with a clear FlashError next to the existing "Image not found" check, and narrow the comment to what it actually covers (spaces, backslashes). - The plain-path test built its expected string with the same triple-brace f-string shape as the implementation, so it mirrored the code rather than pinning the property. Replaced with a literal built independently. Also added coverage for the new unbalanced- brace rejection and for the non-zero-return -> FlashError path, which had no test before. Signed-off-by: Muhammad-Baqir22 <bakaransari686@gmail.com>
|
Thanks for the thorough review I fixed the things that were actually wrong:
Pushed in ec8f579. Ran the full suite again after: 564 passed, 3 skipped, nothing broken. |
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#116 "fix(ebuild): quote openocd flash path so spaces don't split it"
head: ec8f579 author: Muhammad-Baqir22 ci: none run (all three workflows action_required; check-runs: 0; combined status pending, count: 0)
Verdict: Follow-up, round two. ec8f579a did everything the previous review asked, in
the shape it asked for: the brace guard is inside the openocd branch only, the comment no
longer overclaims, the plain-path assertion is a literal rather than a mirror of the
production f-string, and the two missing tests exist. All four prior findings are closed,
and I confirmed each new test has regression value by reverting the code it guards. One
residual: the new guard tests brace counts, not Tcl brace grouping, so three classes
of path still reach OpenOCD's parser — verified in tclsh. All of them fail loudly, so the
original wrong-file bug does not return; that is why it is Low and not a repeat of the
finding this PR exists to fix.
Follow-up on the previous review (ebuild-116-5fe84fc5.md)
| Prior finding | Status | Evidence |
|---|---|---|
| 1 · Med — identical defect unfixed in EoSim | untouched, as directed | The previous review said "out of scope for this PR — do not widen it", and the author correctly did not. Re-checked today: EoSim master still has f'program {firmware_path} verify reset exit' at eosim/integrations/openocd.py:131 and :133, and gh pr list --repo embeddedos-org/EoSim --state open shows no PR covering it (26, 25, 24, 23, 22, 21, 16, 15 — all CI/dependency work). So the recommended follow-up was never opened. Carried as finding 4; not this author's to fix. |
| 2 · Low — comment overclaims "needs no other escaping"; prefer rejecting unbalanced braces | resolved in ec8f579a, with a gap |
Both halves done, and the stronger of the two options taken: flash.py:57-61 raises FlashError("Image path has unbalanced braces, which OpenOCD's Tcl parser cannot group: …"), and it is placed inside if tool == "openocd": so the other four tools are unaffected — exactly as recommended. flash.py:66-67 now reads "no other escaping for spaces or backslashes". The guard's predicate is not quite the property Tcl needs — finding 1. |
3 · Low — assertions mirror the production f-string; no test for unbalanced braces or returncode != 0 |
resolved in ec8f579a |
test_flash.py:66-68 builds the expectation by concatenation ("program {" + str(image_path) + "} 0x8000000 verify reset exit") rather than reusing the format string, and it pins 0x8000000 literally — which incidentally pins hex()'s leading-zero drop, worth having. test_openocd_rejects_an_image_path_with_unbalanced_braces (:71) and test_flash_failure_raises_flash_error_with_tool_stderr (:80) cover the two missing paths. Both confirmed to fail against the code they guard (below). |
4 · Low — ctest checkbox ticked on a pure-Python change |
resolved in ec8f579a |
PR body "Testing": - [ ] Unit tests pass (ctest --test-dir build --output-on-failure) is now unticked. The pytest line remains, which is the claim that is actually backed. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | ebuild/firmware/flash.py:57 |
The guard compares brace counts; Tcl requires correct grouping, which counting does not imply. Three classes pass count("{") == count("}") and still break OpenOCD's parser — all three verified in tclsh against a stub proc program {file args}: (a) wrong order — /tmp/fw}x{.bin and /tmp/a}b{/fw.bin → extra characters after close-brace; (b) a backslash-escaped brace — C:\dir\{v1}\fw.bin, i.e. a Windows path through a directory literally named {v1}, → extra characters after close-brace, because Tcl does not count a brace preceded by \; (c) a path ending in a backslash — reproduced end to end through flash() with a real file named fw\ (legal on POSIX, where pathlib does not strip it): the emitted -c value is program {…/fw\} … and Tcl reports missing close-brace. Balanced-and-well-formed cases are unaffected: /tmp/a{b}c/fw.bin, C:\dir{v1}\fw.bin and C:\Users\Jane Doe\firmware.bin all parse to the correct single filename. Mitigating, and the reason this is Low: every bypass is a loud parse error, not a wrong-file flash, so the class of bug this PR fixes does not return — but the guard exists precisely to turn these into the clear message §9.2 asks for, and for these three it does not fire. |
Replace the count with a scan for the property Tcl actually needs: d = 0for i, ch in enumerate(image_str): if ch == "\\" and image_str[i+1:i+2] in ("{", "}"): break_out # escaped brace never groups if ch == "{": d += 1 elif ch == "}": d -= 1 if d < 0: rejectthen reject unless d == 0, and reject a trailing \. Keep the same message — the class it names is right. Extend test_openocd_rejects_an_image_path_with_unbalanced_braces into a parametrize over fw}.bin, fw}x{.bin and fw\; the first already passes today, the other two are the regression. |
| 2 | Low | pr.json body vs flash.py:57-61, tests/unit/test_flash.py |
The body still describes the pre-review state, in three places. (a) "Summary": "subprocess.run isn't going through a shell here, so nothing else needs escaping" — the diff now does add another check, the brace guard, which is the one thing brace grouping cannot handle by itself. This is the same overclaim prior finding 2 removed from the code comment, left standing in the body. (b) "Changes": the test file "covered a spaced path, a normal path, a missing image, and an unknown tool" — four cases; the head has six, adding the unbalanced-brace and tool-failure tests that prior finding 3 asked for. (c) "Testing": "562 passed, 3 skipped" is the figure from 5fe84fc5; at this head it is 564 passed, 3 skipped, which the author states correctly in their PR comment but not in the body. Per brief §5 the stale claim is itself the finding. Deliberately not rated Medium, unlike the equivalent on ebuild#110: nothing here asserts a fix is absent that is present, the direction of error is understatement, and the correction is in the visible comment thread 20 minutes later. |
Three sentences: drop "nothing else needs escaping" from the Summary and mention the brace rejection, say six tests, and move the 564 passed, 3 skipped figure out of the comment and into "Testing". |
| 3 | Low | CHANGELOG.md:6-12 |
The entry documents the space fix and is silent on the new refusal. ebuild flash --tool openocd now raises FlashError for a brace-unbalanced image path where it previously invoked OpenOCD — a new user-visible failure mode, added by this diff, in the file that is the user-facing record of it. Brief §11: behaviour changed without the documentation changing with it. No compatibility impact — no previously-working path stops working, because a brace-unbalanced path never flashed correctly either — which is why it is Low, not the §8 finding it would otherwise be. |
One sentence on the existing bullet: "A path whose braces do not balance is now rejected up front with a clear error rather than reaching OpenOCD as a Tcl parse failure." |
| 4 | Medium | checks.txt (empty); GitHub API |
No CI has run on this head. At ec8f579a: commits/…/check-runs → total_count: 0; commits/…/status → {"state":"pending","count":0}; actions/runs?head_sha=… → CI — ebuild, Simulation Test, CodeQL, all status=completed, conclusion=action_required — queued behind the outside-contributor Actions approval gate (headRepositoryOwner: Muhammad-Baqir22), not misconfigured. mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. Same gate as ebuild#110, and identical to the state at the previous review — so "pending because the PR was one minute old", which was the previous review's reading, is no longer the explanation. |
Maintainer action, not the author's: approve the queued workflow runs. Nothing the author can push produces this evidence. ebuild#103 exists to close this gap. |
| 5 | Medium | EoSim eosim/integrations/openocd.py:131, :133 |
Carried from prior finding 1 and still open, with nobody assigned. OpenOCDManager.flash() interpolates firmware_path into the same -c 'program … verify reset exit' Tcl string with no grouping, so EoSim flashes the wrong file for any path containing a space — the exact defect fixed here. It is a separate Tier-1 repo (§21) with its own ADAPTER_CONFIGS/TARGET_CONFIGS tables covering 6 adapters and 14 targets, against eBuild's hardcoded interface/stlink.cfg + target/{target}.cfg: two independent implementations that have already diverged in capability as well as in this bug. Recorded as a design gap in .ai/autoreview/proposals/2026-09.md → "2026-09-04 — Name a single owner for external-tool invocation, or accept it triplicated" (Trigger: ebuild#116). Not a defect in this diff and not this author's to fix — the previous review told them to keep it out, correctly. |
A separate PR on EoSim applying the same grouping plus the finding-1 guard. No fix PR opened from here: the brief authorises them only for High. |
No Critical and no High findings. Weakened checks, checked specifically: none. No test
was disabled or skipped, no assertion removed, no lint loosened, no permission widened, no
|| true. The diff only adds a refusal path and tests; the one behavioural change to an
existing path is the brace grouping itself.
Verified by execution
Scratch trees only. The local ebuild clone is dirty (3 files staged plus untracked
smart-sensor/, on branch v90) and the sync step skipped it; it was not touched.
Everything below is a fresh shallow clone of refs/pull/116/head (git rev-parse HEAD =
ec8f579a…, so this is the real head, not diff.patch re-applied) plus git archive master at e5d8052 for the baseline. Linux, CPython 3.12.14, pytest 9.1.1, tclsh 8.6.
full suite: master e5d8052 561 passed, 0 skipped, exit 0
head ec8f579a 567 passed, 0 skipped, exit 0 (+6 = the new file)
tests/unit/test_flash.py alone 6 passed
regression value — the code each new test guards, reverted:
brace grouping removed (program {path} -> program path):
test_openocd_program_command_keeps_a_spaced_path_as_one_token FAIL
test_openocd_program_command_is_unchanged_for_a_plain_path FAIL
unbalanced-brace guard removed:
test_openocd_rejects_an_image_path_with_unbalanced_braces FAIL (DID NOT RAISE)
lint, CI's own invocation (ci.yml:52):
ruff check ebuild/firmware/flash.py tests/unit/test_flash.py --select=E,F,W --ignore=E501
-> All checks passed!
Tcl grouping, tclsh 8.6 against `proc program {file args}` (finding 1):
count-guard tcl result
passes?
yes /tmp/my firmware.bin -> FILE=</tmp/my firmware.bin> OK
yes /tmp/a{b}c/fw.bin -> FILE=</tmp/a{b}c/fw.bin> OK
yes C:\dir{v1}\fw.bin -> FILE=<C:\dir{v1}\fw.bin> OK
yes C:\Users\Jane Doe\firmware.bin-> FILE=<C:\Users\Jane Doe\...> OK
yes /tmp/fw}x{.bin -> ERROR extra characters after close-brace
yes /tmp/a}b{/fw.bin -> ERROR extra characters after close-brace
yes C:\dir\{v1}\fw.bin -> ERROR extra characters after close-brace
yes <file named `fw\`> -> ERROR missing close-brace
(this last one driven end to end through flash() with a real temp file,
not a hand-written Tcl line: the emitted -c value was
'program {/tmp/…/fw\} 0x8000000 verify reset exit')
branch currency: compare master...head -> ahead_by 2, behind_by 0.
duplicate interpolation sites inside ebuild/: none — flash.py:68 is the only place a path
is interpolated into an OpenOCD -c string; reset() (:86) passes no path.
Your "564 passed, 3 skipped" reconciles exactly: 564 + 3 = 567, which is what this suite
yields here where those three environment-conditional skips run. The count is credible, and
behind_by: 0 confirms the "Branch is rebased on latest master" box you ticked.
Architecture conformance
Conforms.
- §5.1, architectural law. No new import, link line or manifest entry; nothing points up
a tier. The diff adds a guard and a format change inside one existing function plus one
test file. - §21, Tier 1 – Foundation.
ebuildis Tier 1; flash-tool invocation is theFlash
leaf of the eBuild engine graph in §9.1, andebuild/firmware/flash.pyis where it
belongs.tests/unit/is the right home for the test. - §9.2, "actionable diagnostics with remediation guidance." This is what round two
delivers, and it is the right reading of the rule: an OpenOCD Tcl parse error is not an
actionable diagnostic, and the newFlashErrornames the path and the reason. Finding 1
is that the guard's predicate under-covers the class its own message claims, not that the
diagnostic is wrong to add. - Brief §8, API and wire compatibility. No public signature, struct layout, manifest
field or serialized format changed.flash()'s signature is untouched. The new
FlashErroris a new failure on input that already failed downstream, so there is no
migration to state. - Brief §9, performance. Nothing on a hot path: one
str(), twostr.count()calls
and one f-string, once perebuild flashinvocation. - Brief §11, documentation. README needs nothing — no user-facing behaviour beyond the
bug differs, and theopenocd -c "program …"snippets incore/eos/GETTING_STARTED.md:112
andcore/eos/docs/quickstart-stm32.md:46,50are hand-typed examples with space-free
paths, so this change does not make them wrong. The CHANGELOG is the one documentation
obligation that is not fully met — finding 3.
The master-design gap this PR exposes — that §9.2 names one source of truth for the CLI,
VS Code and EoStudio but never says which repository owns invoking an external hardware
tool, which is why the same defect exists independently in EoSim — is already recorded
in .ai/autoreview/proposals/2026-09.md under "2026-09-04 — Name a single owner for
external-tool invocation, or accept it triplicated", triggered by this PR in the previous
round. No new proposal appended; a duplicate would be worse than none.
Proposed changes
Smallest sequence. None of it changes the fix, which is correct.
- Swap the count check for a nesting scan and extend the rejection test into a
parametrize(finding 1). Roughly eight lines and two test cases; it is the only code
edit here. - Three sentences in the PR body (finding 2) and one in the CHANGELOG (finding 3).
- Maintainer: approve the queued workflow runs (finding 4).
- Separately, on
EoSim: the same grouping plus the finding-1 guard (finding 5). Keep it
out of this PR.
Items 2–3 do not touch code. This PR is landable as it stands — every finding above is
Low except the two that are not the author's — and finding 1 is a strict improvement to a
guard that did not exist a day ago and that nothing on master has at all.
No fix PR opened: findings 1–3 are Low (the brief authorises fixes only for High) and live
on fix/flash-openocd-path-with-spaces, which belongs to this open PR and must not be
touched; finding 4 is not fixable by a code change; finding 5 is Medium and in another repo.
Not checked
- The project's own CI has not run on either head of this PR (finding 4). Every result
above is mine, on one host, one OS, one Python version. - No OpenOCD binary and no target hardware. Tcl argument parsing is verified against a
stocktclsh 8.6with a stubprogramproc. OpenOCD's realprogramhelper — which
takesfilename argsand substitutes$filenamewithout re-splitting — is reasoned
about from its interface, not executed. Whether OpenOCD's embedded Jim-Tcl interpreter
matches Tcl 8.6 on the three finding-1 cases was not verified; Jim implements the same
brace-grouping rule, but I did not run it. - Windows not exercised. The
C:\Users\Jane Doe\firmware.binandC:\dir\{v1}\fw.bin
cases are verified at the Tcl parsing layer on Linux. Note that finding 1(c) — the
trailing-backslash case — is reachable only on POSIX, becausepathlibstrips a trailing
separator on Windows and does not on Linux; I confirmed that asymmetry but did not run it
underWindowsPath. - The screenshots in the PR body were not opened. All evidence above is my own
execution. - Python 3.10 and 3.11, the other two legs of
ci.yml's matrix, were not run.mypywas
not run on this branch. Thepytest-benchmarkjob (ci.yml:89) was not run. - The three environment-conditional skips you report did not skip here, so I still have
not identified which tests they are or why they skip on your host. - EoSim's own suite was not run; finding 5 is from reading
openocd.py:131,133onmaster
and the open-PR list, not from executing anything in that repo.
Automated architecture review of ec8f579afad9 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Summary
While going through the flash tooling I found a bug in the OpenOCD path:
ebuild flash --tool openocdbuilds the OpenOCD command by dropping the image path straight into the-c "program <path> <addr> verify reset exit"string. OpenOCD's Tcl interpreter splits that on whitespace, so any path with a space in it —my firmware.bin, or a normal Windows path likeC:\Users\Jane Doe\firmware.bin— gets cut into pieces and OpenOCD tries to flashmyinstead of the actual file.Fix is simple: wrap the path in Tcl brace-grouping (
{...}) so it's always treated as one token, spaces or not.subprocess.runisn't going through a shell here, so nothing else needs escaping.Type of Change
Changes
ebuild/firmware/flash.pybefore it gets passed to OpenOCD.tests/unit/test_flash.py— there wasn't any test coverage for this file before, so I covered a spaced path, a normal path, a missing image, and an unknown tool.Testing
Ran the full suite with
python -m pytest -q: 562 passed, 3 skipped, nothing broken. The 3 skips are pre-existing and have nothing to do with this change.Pre-Submission Checklist
Related Issues
Related to the open flash-tooling item (T-002) in
TASKS.md.Screenshots / Logs
Additional Notes
Left the "compiles without warnings" box unchecked since that's a C-specific check and this is a pure Python fix — didn't want to check something that doesn't really apply. No public API changed either, so no docs to update.