Tell the reader what to do when their bootloader has no && - #118
Merged
Conversation
PR Summary by QodoExplain safe flashing when bootloaders lack compound commands
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. Fallback skips erase check
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
guarded_flash joins the transfer to the erase and the write with `&&` so a failed transfer cannot reach the erase. That fix is why #55 exists and it should stay. But `&&` is a hush feature, and the bootloader somebody is running the first time they follow this page is the stock vendor one -- which does not always have it. The reporter in OpenIPC/firmware#2299 hit exactly that: "the boot loader does NOT work with compound commands, I don't think it recognizes the && and just gives me the help entry for the first command. I had to execute each one separately." That failure is safe. The bootloader parses the line as one command with too many arguments, prints usage, and transfers nothing, erases nothing. It is also completely silent about why, and the line is not one a reader can take apart unaided -- splitting it wrongly is how you get an erase without a transfer, which is the thing `&&` was added to prevent. So list_of_commands now appends a note to any block that chains, and to no other, naming the symptom and saying to check the transfer reported success before running the erase. Keyed off the rendered text rather than the caller, so a block that stops chaining loses the note by itself. Ten locales. `i18n-tasks missing` and `unused` do not mention the new key. Two of the translations needed an em dash where a colon would have been, because a plain YAML scalar cannot contain ": ". Three tests at the helper level, where the include/exclude rule lives, and two through the rendered page, one of them in Russian -- the English string is the fallback, so an English assertion cannot tell a translated heading from a hardcoded one. Removing the note fails four of the five. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5
Qodo's review, and it is right. guarded_flash is `transfer && erase && write` -- two gates, not one. The note rebuilt only the first: it said to run the erase and the write once the transfer had reported success, which leaves a reader whose erase failed writing into flash that was never cleared. On NOR that is a write over unerased bytes; where the block starts at 0x0 it is a write over the bootloader. The rule is sequential, so say that: run each part only after the part before it has reported success. That covers both gates and is shorter. Rewritten in all ten locales. The new helper test pins the sequential form and rejects the transfer-only one, so this cannot come back as a rewording; putting the old sentence back in en.yml fails it. 208 runs, 786 assertions. 31 rubocop offences across the touched files, identical to master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5
openipc-ai
force-pushed
the
compound-command-fallback
branch
from
August 25, 2026 12:22
7b91f08 to
a3f3a17
Compare
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
1 participant
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.
From
OpenIPC/firmware#2299, found while triaging website bugs filed againstthe firmware repo.
The problem
guarded_flashchains the transfer to the erase and the write with&&, so afailed transfer cannot reach the erase. That is the #55 fix and it should stay.
But
&&is a hush feature, and the bootloader somebody is running the firsttime they follow this page is the stock vendor one. The reporter in #2299:
The bootloader parses the whole line as one command with too many arguments,
prints usage, and does nothing. Safe — nothing transferred, nothing erased —
but silent about why, and the line is not one a reader can take apart unaided.
Splitting it wrongly is how you get an erase without a transfer, which is
precisely what
&&was added to prevent.The change
list_of_commandsappends a note to any block whose text chains, and to noother:
Keyed off the rendered text rather than the calling helper, so a block that
stops chaining loses the note on its own.
run setnor8m,run uknand; run urnandand themw.blines are untouched.Ten locales;
i18n-tasks missingandunuseddo not mention the new key. Twotranslations needed an em dash where a colon would naturally have gone — a plain
YAML scalar cannot contain
": ", and it fails at load rather than at review.Verification
bin/rails test— 203 runs, 765 assertions, 0 failures (200 before).Three tests at the helper level, where the include/exclude rule actually lives,
and two through the rendered page. One asserts the Russian string: English is
the fallback for a missing translation, so an English assertion cannot tell a
translated string from a hardcoded one.
Removing the note fails four of the five; the "does not chain" test correctly
keeps passing. One new rubocop offence appeared during development (a stray
blank line) and is fixed; the rest is the two class/module length counters
rising.
Note on scope
This does not weaken the guard — the
&&is still there for every bootloaderthat understands it. It only stops the failure being mute for the ones that do
not.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5