fix: retry version resolution, and stop examples passing on exit 0 (#121, #102) - #134
Conversation
, #102) Two independent false signals, both diagnosed in their issues. #121 — resolveVersion() had no retry. fetchResilient() is one attempt per transport (system resolver, then a public-DNS IPv4 fallback); if the fallback also threw, the error propagated. #116/#119 gave the download path a retry policy and left version resolution — on the critical path of start(), the wizard and the CLI — with a single shot, so one refused connection reddened a whole leg. It now retries a connection-class failure 3x with downloadToFile's backoff shape and names the attempts and host on exhaustion; an HTTP status stays terminal on the first attempt so a bad channel still fails fast. Its second half: the fallback's error was the only one surfaced, so the message read "https://159.148.147.251/routeros/..." with no hostname — the one thing that cannot be mapped back to what was attempted. Both failures now raise a ResilientFetchError naming the URL, each transport's error, the fallback address and which resolvers produced it, with the direct failure as `cause`. #102 — the smoke harness asserted `code === 0` and `out.length > 0`, which a PowerShell example that ran no quickchr command at all satisfied for months. Closing the class, not the instance: - each RUNNABLE names output markers only a working run produces, including one from the END of the script and one the CLI itself emitted; - each .ps1 sets $ErrorActionPreference = 'Stop' BEFORE dot-sourcing common.ps1, so a common.ps1 that fails to load can no longer let the example run past it (validate-examples enforces this); - mndp.py's "no announcement" branch raised the same false green in Python — a bare return leaves main() through the finally and never reaches sys.exit(rc). It raises SystemExit now. Grounded locally, Intel macOS 2026-08-03. pwsh 7.4.6 against the real files with #102's ParserError reinstated: rc=0 unguarded (reproducing "-> starting (stable channel...)", double space and all), rc=1 guarded. mndp.py with the identity match forced to fail: rc=0 before, rc=1 after. Every stdout marker verified against a real CHR boot (quickstart.ts, rollback.ts, quickstart.sh, mndp.py). PSScriptAnalyzer + the ParseFile gate run clean on all nine .ps1. Not included: #121's item 3 (trying more than one A record / preferring the system resolver's answer). It needs evidence from a hosted runner that the balancer is what refused us; the chained error above is what will supply it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds retry handling and combined transport errors to RouterOS version resolution. It also strengthens example validation with output markers, PowerShell terminating errors, and non-zero MNDP failure exits. Tests and changelog entries cover these behaviors. ChangesNetwork resilience
Example failure detection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves robustness of RouterOS version resolution (network/transient failures) and tightens the examples smoke harness so “exit 0 + any output” can’t silently pass broken examples.
Changes:
- Add connection-failure retry behavior to
resolveVersion()and propagate retry options throughresolveAllVersions(). - Improve
fetchResilient()error reporting by chaining direct + IPv4-fallback failures into a structuredResilientFetchError. - Make examples smoke tests assert per-example output markers; enforce PowerShell guard rails (
$ErrorActionPreference = 'Stop'before dot-sourcing) and fix a Python example’s “return → exit 0” failure path.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/lib/versions.ts |
Adds retry policy for version resolution and plumbs options through resolveAllVersions(). |
src/lib/net.ts |
Introduces ResilientFetchError and preserves both transport failures when IPv4 fallback fails. |
test/unit/versions.test.ts |
Adds unit coverage for retry/no-retry behaviors in version resolution. |
test/unit/net.test.ts |
Adds unit coverage for ResilientFetchError and updated isConnectionFailure() behavior. |
test/integration/examples-smoke.test.ts |
Replaces “non-empty output” with required stdout markers per runnable example. |
scripts/validate-examples.ts |
Enforces PowerShell $ErrorActionPreference guard before dot-sourcing common.ps1. |
examples/common.ps1 |
Documents the load-bearing duplication of $ErrorActionPreference guarding. |
examples/quickstart/quickstart.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/rollback/rollback.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/device-mode/device-mode.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/dude/dude.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/grounding/grounding.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/harness/harness.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/service-forward/service-forward.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/trial-license/trial-license.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers (manual example). |
examples/version-matrix/version-matrix.ps1 |
Arms $ErrorActionPreference = 'Stop' before dot-sourcing shared helpers. |
examples/mndp/mndp.py |
Fixes failure path to exit non-zero via SystemExit instead of bare return. |
.github/instructions/examples.instructions.md |
Documents “Exit 0 is not evidence” and the enforced PowerShell guard. |
CHANGELOG.md |
Records the user-facing reliability fixes (version retry/error reporting, examples false-green closure). |
…135) `run_quickchr()` used subprocess.run(check=True), whose CalledProcessError names the command and the exit code and DISCARDS both streams -- and those streams are the only place quickchr says what went wrong. Run 30852139131 (linux/aarch64, currently red on main) failed on the first post-boot exec and recorded nothing but "returned non-zero exit status 1", which is not enough to diagnose it. Same family as the rest of this branch: a failure that cannot say what it was. Instrumentation, not a fix for #135 -- that stays open until the aarch64 failure is understood. Verified locally by pointing the exec at a machine that does not exist: rc=1, and the CLI's own "Machine ... not found. Available: ..." now reaches stderr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI status: 8/9 green; the one red check is not this PR
That gate reads One follow-on commit hereThe aarch64 failure recorded nothing but the exit code: That is the same defect class as the rest of this branch, in the file this branch already fixes, so It is instrumentation, not a fix — #135 stays open until the aarch64 failure is understood. Merging this PR still needs |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/common.ps1`:
- Around line 9-13: Update the explanatory comment in common.ps1 around the
caller’s $ErrorActionPreference to state that the caller must set it to 'Stop'
before dot-sourcing common.ps1; describe the false-green behavior when that
preference is missing, while preserving the existing load-order and helper-call
context.
In `@examples/mndp/mndp.py`:
- Around line 45-56: Update the subprocess invocation in the mndp wrapper to
pass check=False explicitly, then forward proc.stdout and proc.stderr for
successful calls before returning. Preserve the existing nonzero-return handling
and diagnostic SystemExit message, and ensure output is not duplicated on
failures.
In `@examples/version-matrix/version-matrix.ps1`:
- Around line 5-6: Configure each Start-Job script block in the version-matrix
flow to set $PSNativeCommandUseErrorActionPreference = $true and
$ErrorActionPreference = 'Stop' before invoking quickchr, or explicitly validate
$LASTEXITCODE immediately afterward and fail the job on nonzero results.
In `@scripts/validate-examples.ts`:
- Line 14: Update scripts/validate-examples.ts to replace every readFileSync
usage, including the require("node:fs").readFileSync call, with
Bun.file(...).text() and await the resulting promises where needed; then remove
readFileSync from the node:fs import while preserving the existing validation
flow.
In `@src/lib/versions.ts`:
- Around line 103-134: Update the retry loop around fetchResilient so response
reading and version parsing occur within the try/catch that handles retryable
connection failures. Preserve HTTP status failures and invalid-version results
as terminal QuickCHRError outcomes, while allowing retry-eligible body-read
failures to update lastError and retry or surface after maxAttempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8bf30c1f-b0ee-4fba-b9bf-1bfc52c77b73
📒 Files selected for processing (19)
.github/instructions/examples.instructions.mdCHANGELOG.mdexamples/common.ps1examples/device-mode/device-mode.ps1examples/dude/dude.ps1examples/grounding/grounding.ps1examples/harness/harness.ps1examples/mndp/mndp.pyexamples/quickstart/quickstart.ps1examples/rollback/rollback.ps1examples/service-forward/service-forward.ps1examples/trial-license/trial-license.ps1examples/version-matrix/version-matrix.ps1scripts/validate-examples.tssrc/lib/net.tssrc/lib/versions.tstest/integration/examples-smoke.test.tstest/unit/net.test.tstest/unit/versions.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In Bun-based TypeScript code, use
Bun.spawn(),Bun.write(),Bun.sleep(),bun:test, and ESM imports with.tsextensions.
Files:
test/unit/versions.test.tsscripts/validate-examples.tstest/integration/examples-smoke.test.tssrc/lib/versions.tssrc/lib/net.tstest/unit/net.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not turn a red integration test green by broadening timeouts, skipping it, or platform-gating it before reproducing and root-causing the failure.
Files:
test/unit/versions.test.tstest/integration/examples-smoke.test.tstest/unit/net.test.ts
**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.ts: Use Bun APIs and tooling rather than Node.js equivalents:Bun.spawn(),Bun.write(),Bun.sleep(),bun test, andbun:test. Use ESM with.tsextensions in imports; do not use CommonJS.
For ARM64virtmachines, never useif=virtiofor drives; use an explicit-device virtio-blk-pci,drive=drive0configuration.
When using HVF acceleration, use-cpu host, notcortex-a710.
For arm64 guests on macOS, automatically select TCG with-cpu cortex-a710; HVF cannot run the CHR image's 32-bit ARM userspace on Apple Silicon.--accelandQUICKCHR_ACCELmust override this selection for testing.
UEFI pflash code and vars units must be identical in size.
QGA is x86-only; do not assume the guest agent starts for arm64 CHR.
Use tabs for indentation.
Do not add unnecessary comments to obvious code.
Errors must be thrown asQuickCHRError(code, message, installHint?).
Preserve the documented public API types and behavior:QuickCHR.start(opts)returnsChrInstance;ChrInstanceprovidesstop(),remove(),rest(),monitor(),serial(), andqga(); andMachineStaterepresents persistedmachine.jsonstate.
Files:
test/unit/versions.test.tsscripts/validate-examples.tstest/integration/examples-smoke.test.tssrc/lib/versions.tssrc/lib/net.tstest/unit/net.test.ts
test/unit/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be fast and must not require QEMU.
Files:
test/unit/versions.test.tstest/unit/net.test.ts
test/integration/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Integration tests may require QEMU and must be guarded by
QUICKCHR_INTEGRATION=1.
Files:
test/integration/examples-smoke.test.ts
src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the rules in
general.instructions.mdfor files undersrc/**, including layer boundaries, error-code usage, port layout, and the RouterOS “expired admin” caveat.
Files:
src/lib/versions.tssrc/lib/net.ts
src/lib/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep
src/lib/as pure library code: do not import fromsrc/cli/and do not callprocess.exit()there.Keep
src/lib/as pure library modules: do not add CLI dependencies or callprocess.exit().
Files:
src/lib/versions.tssrc/lib/net.ts
🧠 Learnings (2)
📚 Learning: 2026-07-31T11:56:40.455Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:68-68
Timestamp: 2026-07-31T11:56:40.455Z
Learning: In Bun TypeScript files, do not flag use of `node:fs.appendFileSync` when append semantics are required and `Bun.write()` cannot provide them. This applies to cases such as appending multiple entries to `$GITHUB_OUTPUT` or writing to the boot log; use append-capable file operations rather than overwriting existing content.
Applied to files:
test/unit/versions.test.tsscripts/validate-examples.tstest/integration/examples-smoke.test.tssrc/lib/versions.tssrc/lib/net.tstest/unit/net.test.ts
📚 Learning: 2026-07-29T22:35:22.878Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 108
File: test/lab/www-abort-damage/www-abort-damage.test.ts:0-0
Timestamp: 2026-07-29T22:35:22.878Z
Learning: In this Bun TypeScript repo, QUICKCHR_INTEGRATION is a presence/opt-in flag for integration/lab tests. Follow the existing convention that gates execution using the presence check form `!process.env.QUICKCHR_INTEGRATION` (e.g., in describe/test skip conditions or equivalent gating). Any defined value (including `QUICKCHR_INTEGRATION=0`) should be treated as “present” and thus enables the tests per the repo’s convention. Do not add explicit value parsing/comparisons inside individual test files (e.g., `=== "0"`, `=== "1"`, or number/boolean conversions); only change this behavior via a repo-wide convention update.
Applied to files:
test/integration/examples-smoke.test.ts
🪛 LanguageTool
CHANGELOG.md
[style] ~117-~117: Consider an alternative for the overused word “exactly”.
Context: ...ad address" indistinguishable, which is exactly the question the log has to answer. T...
(EXACTLY_PRECISELY)
🪛 Ruff (0.16.0)
examples/mndp/mndp.py
[error] 45-45: subprocess call: check for execution of untrusted input
(S603)
[warning] 45-45: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[warning] 52-56: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 252-252: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (18)
src/lib/net.ts (2)
22-24: LGTM!Also applies to: 37-56, 184-203
57-117: 🗄️ Data Integrity & IntegrationNo action needed.
test/unit/net.test.ts (1)
3-8: LGTM!Also applies to: 43-54, 136-167
src/lib/versions.ts (1)
7-7: LGTM!Also applies to: 68-83, 146-154
test/unit/versions.test.ts (1)
264-335: LGTM!CHANGELOG.md (1)
102-122: LGTM!examples/common.ps1 (1)
4-4: LGTM!.github/instructions/examples.instructions.md (1)
17-17: LGTM!Also applies to: 53-83
examples/device-mode/device-mode.ps1 (1)
2-2: LGTM!examples/dude/dude.ps1 (1)
3-3: LGTM!test/integration/examples-smoke.test.ts (1)
50-68: LGTM!Also applies to: 77-113, 212-216
examples/mndp/mndp.py (1)
249-252: LGTM!examples/grounding/grounding.ps1 (1)
3-4: LGTM!examples/harness/harness.ps1 (1)
4-5: LGTM!examples/quickstart/quickstart.ps1 (1)
3-4: LGTM!examples/rollback/rollback.ps1 (1)
2-3: LGTM!examples/service-forward/service-forward.ps1 (1)
2-3: LGTM!examples/trial-license/trial-license.ps1 (1)
3-4: LGTM!
…tart-Job
Six review findings; four applied, two declined with reasons.
versions.ts, from CodeRabbit: `response.text()` sat OUTSIDE the retry-eligible
try, so a body that failed after the headers arrived (a reset mid-response)
escaped the loop unwrapped — no retry, no terminal QuickCHRError. The read now
happens inside the try; an HTTP status is thrown as a QuickCHRError so the catch
can tell a server verdict from a transport failure and let it straight out, and
parsing stays terminal after the loop. Anchored with a stream that errors after
a 200.
versions.ts, from Copilot: `Math.max(1, NaN)` is NaN, so a caller passing NaN
skipped the loop entirely and got "all NaN attempts: undefined" — a message
describing nothing. maxAttempts/retryDelayMs are public options and are now
sanitized to a whole count >= 1 and a finite non-negative delay. Anchored.
version-matrix.ps1, from CodeRabbit: Start-Job runs in a separate process and
inherits no preference variables, so neither the caller's nor common.ps1's copy
reaches the job block. Grounded with pwsh 7.4.6 rather than taken on faith:
- silent `exit 3` inside a job, preferences unset -> parent continues, rc=0
- same, with both set inside the block -> NativeCommandExitException, rc=1
- a job whose command writes to stderr propagates either way, but via the
stderr-to-error-record mapping, not the exit code — not something to rely on
That is #102's exact class (a failed start booting nothing, reported green), so
the rule is recorded in examples.instructions.md alongside the others.
common.ps1: reworded so the false-green condition is stated as the conditional
it is — the helper calls are non-terminating only when the CALLER has not armed
the preference, which is why the caller's copy exists.
mndp.py: explicit check=False, naming why the wrapper handles returncode itself.
Declined:
- forwarding child stdout/stderr on SUCCESSFUL run_quickchr calls — the
example curates its own narrative output, and echoing every CLI invocation
would bury it and the smoke markers in noise. The gap was diagnosing
FAILURES, which is fixed.
- converting validate-examples.ts to Bun.file().text() — the file is uniformly
sync node:fs (readdirSync/statSync/existsSync), so a lone async read would
read worse, not better. Worth doing as a whole-file cleanup, not here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hit while byte-compiling mndp.py to check the review edits: any local `python3 -m py_compile` or import of a .py example creates examples/<name>/__pycache__, and the dir scan flagged it as an unexpected subdir — turning `bun run check` red for a build artifact. It is gitignored, so it never reaches CI and can never be a real violation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #121. Closes #102.
Two independent false signals, both already diagnosed in their issues.
#121 —
resolveVersion()had no retry, and the fallback's error hid the real onefetchResilient()is one attempt per transport: a plain fetch on the systemresolver, then — only on a connection-class error — a public-DNS A record →
IPv4 literal with
Host/SNI preserved. If that second attempt also threw, theerror propagated. #116/#119 gave
downloadToFile()a retry policy and leftversion resolution with a single shot, even though it sits on the critical path
of
start()(quickchr.ts:1404,:1523,:1531), the wizard and the CLI. Onerefused connection reddened a full integration leg on a run where nothing about
RouterOS, QEMU or the platform was involved.
resolveVersion()retries a connection-class failure up to 3× with the samebackoff shape as
downloadToFile, and names the attempt count and host whenit exhausts them. An HTTP status is still terminal on the first attempt —
a 404 for a bad channel stays fast (asserted).
fetchResilient()no longer lets the fallback's error stand alone. That errorreports an IP literal and no hostname, which leaves "the system resolver was
broken", "the host refused us" and "public DNS handed back a bad address"
indistinguishable — exactly the question a CI log has to answer. Both failures
now raise a
ResilientFetchErrornaming the URL, each transport's error, theaddress the fallback used and which resolvers produced it, with the direct
failure kept as
cause.isConnectionFailure()recognizes it (it isconnection-class by construction), so a caller retrying connection failures
keeps retrying once the fallback fails too.
Deliberately not included: #121's item 3 (trying more than one A record /
preferring the system resolver's answer). The issue asks for hosted-runner
evidence that the balancer is what refused us before touching that — and the
chained error above is what will supply it on the next occurrence.
#102 —
code === 0+out.length > 0is not evidence an example workedThe open half of #102. A
ParserErrorincommon.ps1left every helperundefined, so
quickstart.ps1ran no quickchr command at all — printed oneline, exited 0, and the smoke harness reported
(pass)for months. The parseitself was fixed in #101; this closes the class:
RUNNABLEinexamples-smoke.test.tsnamessubstrings only a working run produces — always one from the end of the
script (a mid-run silent failure must not pass) and one the CLI/library
emitted rather than the example's own
echo. The interpolatedexamples-<slug>-machine name is deliberate: the tell in examples: common.ps1 ParserError made every PowerShell example a silent false green #102 was a doublespace where that name should have been. The failure reports the list of
missing markers, so "wrong result" reads differently from "did nothing".
.ps1arms$ErrorActionPreference = 'Stop'before dot-sourcing,duplicating what
common.ps1sets. Load-bearing: ifcommon.ps1fails toload, the preference it sets never takes effect and every undefined helper is
an ordinary non-terminating
CommandNotFound.scripts/validate-examples.tsenforces it (cheap gate, no QEMU).
examples/mndp/mndp.pyhad the same bug in Python. Found while choosingits markers: the "no MNDP announcement" branch used a bare
return, whichleaves
main()through thefinallyand never reaches the trailingsys.exit(rc)— so the failure path exited 0. Nowraise SystemExit;cleanup still runs.
Grounding (Intel macOS, 2026-08-03)
.ps1guard converts the false green$LASTEXITCODE:reinstated: unguarded prints-> starting (stable channel...)(double space) and exits 0; guarded exits 1 at the dot-source, having run nothingmndp.pyfixquickstart.ts,rollback.ts,quickstart.shandmndp.py— 0 markers missing in each.ps1edits pass their own CI gatesParseFileover all nine.ps1+ PSScriptAnalyzer with the repo settings, run locally: clean (it caught an em dash I had introduced intocommon.ps1—PSUseBOMForUnicodeEncodedFile)bun run checkandbun test(960 pass / 0 fail) are green.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Validation