refactor(examples): runnable-script convention, 5 new examples, durable CI#22
Merged
Conversation
…le CI Rework examples/ from a mix of bun:test files + Makefiles into one durable convention: every example is a runnable `bun run` <name>.ts plus .sh/.ps1 CLI mirrors (and .py where useful). grounding/ stays the sole bun:test, expanded as the reference for CHR integration-test patterns. Captured in a new .github/instructions/examples.instructions.md + examples/_template/, with shared infra (lib.ts runExample/exampleMachineName/freePort/check, common.sh/common.ps1). Renames: vienk->quickstart, matrica->version-matrix. Makefiles deleted; child.ts -> harness/tool/; rb5009-arm64.rsc -> version-matrix/config/; Python -> uv run. New single-concept examples: rollback (snapshots), service-forward (--forward), file-transfer (upload/download), device-mode, trial-license (manual-only). Fix: dude is NOT x86-only — dude-<ver>-arm64.npk ships for 7.21.1-7.23.1 and the resolver finds it. Removed the arm64 skip + arch pin; added a findPackageFile unit test grounding the correction. Also fixed the old --packages flag (silently ignored) -> repeated --add-package. CI-checkable: check now runs lint:examples (convention/drift validator) + lint:shell (shellcheck -s sh); biome covers examples/**. verify-extended gains include-examples/example-filter -> examples-smoke job (curated subset + one representative per language + an intentional failure-path teardown case) and a lint-powershell PSScriptAnalyzer job. examples/COVERAGE.md maps the surface; DESIGN/CONTRIBUTING/README/MANUAL/BACKLOG + routeros-quickchr skill updated. Surfaced gaps (tracked in BACKLOG): no `quickchr cp` CLI; rootless-l2 socket-lan example deferred pending a 1-page design. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds runnable example conventions, shared helpers, example scripts, validation, and CI workflow wiring for ChangesExamples rework and verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors examples/ into a consistent, runnable-script convention (bun run-able <name>.ts plus CLI mirrors), adds an examples smoke harness and drift validators to keep examples from rotting, and corrects the “dude is x86-only” claim with a grounding unit test.
Changes:
- Standardize
examples/on runnable scripts + shared infra (examples/lib.ts,examples/common.sh,examples/common.ps1) and addscripts/validate-examples.ts+ shellcheck linting. - Add curated integration smoke coverage for examples (
test/integration/examples-smoke.test.ts) and extendverify-extended.ymlwith optional examples smoke + PowerShell lint jobs. - Add/rename examples (quickstart, version-matrix, rollback, service-forward, file-transfer, device-mode, trial-license) and update docs/backlog to match.
Reviewed changes
Copilot reviewed 79 out of 81 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/packages.test.ts | Adds a unit test grounding that findPackageFile resolves dude on arm64 as well as x86. |
| test/integration/examples-smoke.test.ts | New curated integration test that executes a small set of runnable examples and asserts teardown on failure. |
| scripts/validate-examples.ts | New no-QEMU validator enforcing example structure, naming, and README link/run-command reference correctness. |
| scripts/lint-shell.sh | New shellcheck runner for examples/**/*.sh (POSIX sh). |
| README.md | Updates top-level docs to describe the new examples convention and entries. |
| project-words.txt | Adds new dictionary words to satisfy cSpell for the examples/CI tooling. |
| package.json | Extends check with examples lint + shell lint; adds smoke:examples. |
| MANUAL.md | Updates manual references to examples as runnable scripts and links to the convention doc. |
| biome.json | Extends Biome’s include set to cover examples/**/*.ts. |
| BACKLOG.md | Records the examples rework and links newly surfaced gaps (e.g., missing CLI file transfer). |
| .github/workflows/verify-extended.yml | Adds optional examples-smoke (linux) and lint-powershell (windows) jobs controlled by workflow inputs. |
| .github/instructions/examples.instructions.md | New contributor/agent-facing rules defining the examples convention and CI expectations. |
| examples/README.md | Rewrites examples index to reflect runnable-script convention, coverage matrix, and running instructions. |
| examples/COVERAGE.md | New coverage map that ties CLI/library surface areas to examples (or explains why absent). |
| examples/lib.ts | New shared helpers (runExample, deterministic naming, freePort, check) for runnable examples. |
| examples/common.sh | New POSIX sh helpers for $QUICKCHR resolution, naming, port allocation, and teardown trap. |
| examples/common.ps1 | New PowerShell helpers for $env:QUICKCHR resolution, naming, free port, and teardown. |
| examples/_template/README.md | New canonical template README for new examples. |
| examples/_template/example.ts | New canonical TS runnable-script template. |
| examples/vienk/vienk.test.ts | Removes the old bun:test-based “vienk” example (superseded by quickstart). |
| examples/vienk/README.md | Removes the old “vienk” README (superseded by quickstart). |
| examples/quickstart/README.md | New quickstart example documentation. |
| examples/quickstart/quickstart.ts | New runnable library-script quickstart example. |
| examples/quickstart/quickstart.sh | New CLI mirror for quickstart (POSIX sh). |
| examples/quickstart/quickstart.ps1 | New CLI mirror for quickstart (PowerShell). |
| examples/rollback/README.md | New rollback example documentation. |
| examples/rollback/rollback.ts | New runnable snapshot save/load/list example. |
| examples/rollback/rollback.sh | New CLI mirror for rollback. |
| examples/rollback/rollback.ps1 | New CLI mirror for rollback (PowerShell). |
| examples/service-forward/README.md | New service-forward example documentation. |
| examples/service-forward/service-forward.ts | New runnable extraPorts/--forward example with reachability probe. |
| examples/service-forward/service-forward.sh | New CLI mirror for service-forward. |
| examples/service-forward/service-forward.ps1 | New CLI mirror for service-forward (PowerShell). |
| examples/file-transfer/README.md | New file-transfer example documentation, notes missing CLI file transfer. |
| examples/file-transfer/file-transfer.ts | New runnable upload/download round-trip example. |
| examples/file-transfer/file-transfer.sh | CLI stub that documents the lack of quickchr cp and shows inspection output. |
| examples/device-mode/README.md | New device-mode example documentation. |
| examples/device-mode/device-mode.ts | New runnable device-mode provisioning example. |
| examples/device-mode/device-mode.sh | New CLI mirror for device-mode. |
| examples/device-mode/device-mode.ps1 | New CLI mirror for device-mode (PowerShell). |
| examples/trial-license/README.md | New manual-only trial-license example documentation (rate limit aware). |
| examples/trial-license/trial-license.ts | New runnable manual-only license example (env-gated apply). |
| examples/trial-license/trial-license.sh | New CLI mirror for trial-license. |
| examples/trial-license/trial-license.ps1 | New CLI mirror for trial-license (PowerShell). |
| examples/version-matrix/README.md | New version-matrix example documentation (formerly matrica). |
| examples/version-matrix/version-matrix.ts | New runnable multi-channel parallel boot and diff example. |
| examples/version-matrix/version-matrix.sh | New CLI mirror for parallel start + list. |
| examples/version-matrix/version-matrix.ps1 | New CLI mirror for parallel start + list (PowerShell). |
| examples/version-matrix/version-matrix.py | New Python CLI driver for version-matrix (uv run). |
| examples/version-matrix/config/rb5009-arm64.rsc | Adds sample config used by the library version-matrix upload demo. |
| examples/udp-gateway/udp-gateway.ts | Converts udp-gateway from bun:test into a runnable script. |
| examples/udp-gateway/udp-gateway.sh | New CLI-side script for configuring RouterOS syslog to SLIRP gateway. |
| examples/udp-gateway/README.md | Updates udp-gateway docs for runnable-script convention and new CLI helper. |
| examples/udp-gateway/udp-gateway.test.ts | Removes old udp-gateway bun:test file. |
| examples/mndp/mndp.ts | Converts MNDP capture from bun:test style to runnable script using runExample. |
| examples/mndp/mndp.sh | Adds a CLI “show the invocation” helper for --add-network socket:connect:<port>. |
| examples/mndp/mndp.py | Updates Python driver to prefer uv run and to support $QUICKCHR. |
| examples/mndp/README.md | Updates MNDP docs for runnable-script convention. |
| examples/mndp/Makefile | Removes Makefile-based workflow for MNDP in favor of scripts. |
| examples/matrica/README.md | Removes old matrica README (superseded by version-matrix). |
| examples/matrica/matrica.test.ts | Removes old matrica bun:test example. |
| examples/matrica/matrica.py | Removes old matrica Python driver. |
| examples/matrica/Makefile | Removes old matrica Makefile. |
| examples/dude/README.md | Updates docs to correct the “x86-only” claim and align with the new convention. |
| examples/dude/dude.ts | Adds runnable dude package install/enable example (library path). |
| examples/dude/dude.sh | Adds CLI mirror for dude install + enable. |
| examples/dude/dude.ps1 | Adds PowerShell mirror for dude install + enable. |
| examples/dude/dude.py | Adds Python CLI driver for dude (uv run). |
| examples/dude/dude.test.ts | Removes old dude bun:test example. |
| examples/harness/README.md | Updates harness docs for runnable-script convention and tool location. |
| examples/harness/harness.ts | Adds runnable harness example demonstrating descriptor() and subprocessEnv() usage. |
| examples/harness/harness.sh | Adds CLI mirror using quickchr env output to feed a child tool. |
| examples/harness/harness.ps1 | Adds PowerShell mirror using quickchr env --json to feed a child tool. |
| examples/harness/tool/child.ts | Adds external-tool stand-in that consumes env and calls the CHR REST API. |
| examples/harness/harness.test.ts | Removes old harness bun:test example. |
| DESIGN.md | Updates architecture doc to describe examples as runnable scripts and Python uv run preference. |
| CONTRIBUTING.md | Updates repository structure docs to reflect examples being runnable scripts (grounding is the lone test). |
| return [...refs]; | ||
| } | ||
|
|
||
| const entries = readdirSync(EXAMPLES); |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
Author
|
@coderabbitai full review - you were rate limited earlier and I request a full review which only output "review completed" so I'm not sure it ran. so do a full review, or suggest a time that I can run a review that won't be rate limited |
Contributor
|
I can’t guarantee an exact time that won’t hit rate limits, since that depends on current platform load, but if it happens again, the best practical workaround is usually to retry after a short cooldown like 10–15 minutes. ✅ Action performedFull review finished. |
![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: 23
🤖 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 @.github/instructions/examples.instructions.md:
- Around line 47-50: The documented bun run check workflow is missing cspell, so
the examples guidance is drifting from the actual CI gate. Update the CI steps
in examples.instructions.md to match the current bun run check command in
package.json by adding cspell alongside tsc, biome, markdownlint, shellcheck,
and scripts/validate-examples.ts, and keep the smoke harness and PowerShell
verification note unchanged.
In @.github/workflows/verify-extended.yml:
- Around line 731-732: The workflow jobs using actions/checkout and
oven-sh/setup-bun need hardening: pin both actions to immutable commit SHAs
instead of floating version tags, and configure actions/checkout to disable
credential persistence so the workflow token is not written to .git/config. Make
the same checkout change in the PowerShell lint job as well, and keep the fix
centered around the checkout/setup-bun steps in verify-extended.yml.
In `@DESIGN.md`:
- Line 147: The “Examples Philosophy” section in DESIGN.md still contradicts the
new runnable-script guidance, so update or remove that section to match the
`examples/` convention described earlier. Use the same source of truth as the
existing paragraph and the documented examples convention (including `bun
run`-able scripts, `.sh`/`.ps1`/`.py` siblings, and `grounding/` as the only
`bun:test` reference) so the design doc is consistent throughout.
In `@examples/common.ps1`:
- Around line 12-33: `Invoke-Qc` currently doesn’t fail fast on non-zero native
process exits, so the example can continue after a failed `quickchr` or `bun`
invocation. Update `examples/common.ps1` by enabling native error handling with
`$PSNativeCommandUseErrorActionPreference = $true`, or by checking
`$LASTEXITCODE` after the `& $exe @($pre + $Rest)` call and throwing on failure,
so `Invoke-Qc` becomes terminating on native command errors.
- Around line 14-32: Stop treating Quickchr as a whitespace-delimited command
string in the shared PowerShell helpers; update the logic in common.ps1 and the
matching version-matrix script to store the executable and its arguments
separately so paths or overrides with spaces are preserved. Then fix Invoke-Qc
so native command failures actually fail the script by enabling native error
propagation or checking the exit code explicitly after calling Quickchr, using
the Invoke-Qc function and the $script:Quickchr setup as the main places to
adjust.
In `@examples/common.sh`:
- Around line 19-31: The qc wrapper currently re-splits QUICKCHR, which breaks
when the in-repo CLI path contains spaces. Update the logic around QUICKCHR and
qc() so the CLI path is preserved as a separate argument, and invoke bun run
directly with the stored _qc_repo_cli path instead of expanding QUICKCHR as a
single scalar.
- Line 60: The signal handling in the cleanup setup only runs _qc_cleanup and
does not terminate the script for interrupts, so update the trap logic in the
common.sh cleanup section to separate EXIT from INT and TERM. Make the INT and
TERM handlers call _qc_cleanup and then exit with the appropriate codes 130 and
143, while keeping EXIT for normal cleanup, so interrupted examples stop
immediately after cleanup.
In `@examples/device-mode/device-mode.ps1`:
- Around line 7-16: The device-mode script can continue past a failed native
Invoke-Qc CLI call, so update the shared examples/common.ps1 behavior to fail
fast for native commands by enabling $PSNativeCommandUseErrorActionPreference or
explicitly checking $LASTEXITCODE after Invoke-Qc. Make sure the logic around
Invoke-Qc start in device-mode.ps1 stops before the read-back step when the
start command fails, so CI failures surface immediately.
In `@examples/dude/dude.py`:
- Around line 35-38: `dude.py` currently parses `args.quickchr` and the
`QUICKCHR` environment variable with plain string splitting, which breaks quoted
arguments. Update the quickchr parsing logic in the argument-handling block to
use `shlex.split()` for both `args.quickchr` and `os.environ["QUICKCHR"]`,
keeping the existing `qc` assignment flow intact so quoted commands are handled
correctly.
In `@examples/dude/dude.ts`:
- Around line 35-44: The assertion failures in the dude example are currently
using the shared check helper, which turns them into plain Error instances and
breaks the structured error contract. Update the relevant assertions in dude.ts
to use QuickCHRError directly, or change the shared check utility in
examples/lib.ts so it throws QuickCHRError(code, message, installHint?) instead
of a generic Error, keeping the behavior aligned with the repository’s
error-handling convention.
In `@examples/grounding/grounding.test.ts`:
- Around line 67-114: The remaining RouterOS tests in grounding.test.ts still
rely on Bun’s default timeout while performing real chr.exec() and chr.rest()
round-trips, which can make them flaky under load. Add explicit per-test
timeouts to the REST sanity test, the system identity round-trip test, and the
serialize/JSON test, matching the pattern already used in the address-list test.
Locate the affected cases by their test names and update the test(...) /
test.skipIf(...) calls directly.
- Around line 50-57: The beforeAll setup in grounding.test.ts can exceed Bun’s
default hook timeout because it awaits QuickCHR.start(), so add an explicit
timeout as the second argument to that beforeAll call. Update the beforeAll
block that initializes chr with QuickCHR.start so the suite has enough time to
complete startup before tests run.
In `@examples/harness/harness.ts`:
- Around line 48-57: The spawned child in the harness currently pipes stderr but
never reads it, so failures lose diagnostics and can hang if the buffer fills.
Update the harness logic around Bun.spawn and the subsequent code that checks
the exit status so stderr is fully drained alongside stdout, then include the
captured stderr text in the check(code === 0) failure message; use the proc
handle and the existing out/code checks to locate the fix.
In `@examples/lib.ts`:
- Around line 12-13: The free-port helper in lib.ts still uses the Node socket
API via createServer, which conflicts with the Bun example runtime. Update
freePort() to allocate and release the port using Bun’s native listener API
instead, keeping the helper self-contained in Bun and removing the node:net
dependency while preserving the existing ChrInstance-based behavior.
In `@examples/mndp/mndp.ts`:
- Around line 166-172: The teardown in the example’s finally block only closes
the server, leaving the accepted connection open; update the cleanup in mndp.ts
to destroy the established conn before calling server.close(). Use the existing
conn variable in the accept/handler flow so the one accepted socket is
deterministically torn down during shutdown.
In `@examples/service-forward/README.md`:
- Around line 13-14: The README wording in the service-forward example
overstates the port safety guarantee; `freePort()`/`Get-FreePort()` only checks
availability and releases the port before binding, so parallel runs can still
race. Update the text near the service-forward example to soften “don’t collide”
language and describe the host port as merely selected from an available port
rather than guaranteed collision-free unless the allocation becomes atomic.
In `@examples/service-forward/service-forward.ps1`:
- Around line 6-10: The port selection in the service-forward script is racy
because Get-FreePort releases the port before Invoke-Qc start --forward binds
it, so parallel runs can still collide. Update the service-forward flow to use
an atomic reservation approach like the TS example instead of probing then
releasing, and keep the reservation held until the WinBox forwarding command has
successfully started. Use the existing Get-FreePort and Invoke-Qc start logic as
the anchor points when refactoring.
In `@examples/service-forward/service-forward.ts`:
- Around line 23-33: The service-forward example still has a port bind race
because freePort() releases the socket before QuickCHR.start() begins, allowing
another process to claim hostPort in between. Update the service-forward flow to
keep the reservation alive until the VM is launched, or switch
QuickCHR.start()/extraPorts to use port 0 and read back the assigned mapping
after startup; use the freePort() call and the QuickCHR.start() invocation as
the main spots to adjust.
In `@examples/udp-gateway/udp-gateway.ts`:
- Around line 35-37: The UDP listener is currently bound in the udp-gateway
setup using sock.bind with 0.0.0.0, which exposes it on all interfaces instead
of only loopback. Update the bind address in the UDP gateway flow so the
listener is restricted to localhost/loopback while still obtaining the assigned
port from sock.address(), keeping the rest of the gateway logic unchanged.
In `@examples/version-matrix/version-matrix.py`:
- Around line 36-40: The run() helper currently relies on subprocess.run’s
implicit check=False default while manually handling returncode, which triggers
the lint warning. Update the subprocess.run call inside run() to pass
check=False explicitly, keeping the existing returncode-based error handling and
preserving the current behavior.
In `@scripts/validate-examples.ts`:
- Line 86: The `validate-examples.ts` script is mixing CommonJS and ESM by
calling `require("node:fs")` inline. Update the existing `node:fs` ESM import to
include `readFileSync`, then replace the `require` usage in the `readme`
assignment with the सीधे imported function so the file stays ESM-only.
In `@test/integration/examples-smoke.test.ts`:
- Around line 26-30: The smoke test filter handling in examples-smoke.test.ts
currently treats unknown EXAMPLE_FILTER values as “match nothing,” which lets
typos silently skip all cases. Add a fail-fast validation step around the FILTER
parsing/want logic so any non-empty EXAMPLE_FILTER entries that do not
correspond to known example names are detected immediately and cause the test
run to fail. Use the existing FILTER and want helpers, and apply the same guard
to the smoke-case sections covered by the other related ranges so
workflow_dispatch.example-filter cannot accidentally disable the whole job.
- Around line 43-44: The smoke test entry for the Python example currently uses
python3 directly instead of the documented uv run path, so update the mndp-py
case in the integration smoke harness to invoke the example through uv run like
the rest of the Python examples. Also make sure the workflow job that runs this
suite installs uv before executing the test so the representative launcher path
is actually exercised.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d143cfa4-9f7d-4e10-956a-1cfff9574865
📒 Files selected for processing (81)
.github/instructions/examples.instructions.md.github/workflows/verify-extended.ymlBACKLOG.mdCONTRIBUTING.mdDESIGN.mdMANUAL.mdREADME.mdbiome.jsonexamples/COVERAGE.mdexamples/README.mdexamples/_template/README.mdexamples/_template/example.tsexamples/common.ps1examples/common.shexamples/device-mode/README.mdexamples/device-mode/device-mode.ps1examples/device-mode/device-mode.shexamples/device-mode/device-mode.tsexamples/dude/README.mdexamples/dude/dude.ps1examples/dude/dude.pyexamples/dude/dude.shexamples/dude/dude.test.tsexamples/dude/dude.tsexamples/file-transfer/README.mdexamples/file-transfer/file-transfer.shexamples/file-transfer/file-transfer.tsexamples/grounding/README.mdexamples/grounding/grounding.ps1examples/grounding/grounding.shexamples/grounding/grounding.test.tsexamples/harness/README.mdexamples/harness/harness.ps1examples/harness/harness.shexamples/harness/harness.test.tsexamples/harness/harness.tsexamples/harness/tool/child.tsexamples/lib.tsexamples/matrica/Makefileexamples/matrica/README.mdexamples/matrica/matrica.pyexamples/matrica/matrica.test.tsexamples/mndp/Makefileexamples/mndp/README.mdexamples/mndp/mndp.pyexamples/mndp/mndp.shexamples/mndp/mndp.tsexamples/quickstart/README.mdexamples/quickstart/quickstart.ps1examples/quickstart/quickstart.shexamples/quickstart/quickstart.tsexamples/rollback/README.mdexamples/rollback/rollback.ps1examples/rollback/rollback.shexamples/rollback/rollback.tsexamples/service-forward/README.mdexamples/service-forward/service-forward.ps1examples/service-forward/service-forward.shexamples/service-forward/service-forward.tsexamples/trial-license/README.mdexamples/trial-license/trial-license.ps1examples/trial-license/trial-license.shexamples/trial-license/trial-license.tsexamples/udp-gateway/README.mdexamples/udp-gateway/udp-gateway.shexamples/udp-gateway/udp-gateway.test.tsexamples/udp-gateway/udp-gateway.tsexamples/version-matrix/README.mdexamples/version-matrix/config/rb5009-arm64.rscexamples/version-matrix/version-matrix.ps1examples/version-matrix/version-matrix.pyexamples/version-matrix/version-matrix.shexamples/version-matrix/version-matrix.tsexamples/vienk/README.mdexamples/vienk/vienk.test.tspackage.jsonproject-words.txtscripts/lint-shell.shscripts/validate-examples.tstest/integration/examples-smoke.test.tstest/unit/packages.test.ts
💤 Files with no reviewable changes (10)
- examples/vienk/README.md
- examples/matrica/README.md
- examples/vienk/vienk.test.ts
- examples/harness/harness.test.ts
- examples/mndp/Makefile
- examples/dude/dude.test.ts
- examples/matrica/matrica.test.ts
- examples/udp-gateway/udp-gateway.test.ts
- examples/matrica/matrica.py
- examples/matrica/Makefile
Comment on lines
+731
to
+732
| - uses: actions/checkout@v5 | ||
| - uses: oven-sh/setup-bun@v2 |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the new Actions and disable checkout credential persistence.
These jobs add new uses: steps without commit-SHA pinning, and actions/checkout will keep the workflow token in .git/config unless you turn it off. Since the jobs immediately execute repository-controlled scripts (bun install, shell, PowerShell), that widens the blast radius of a compromised action or script unnecessarily.
Suggested hardening
- - uses: actions/checkout@v5
+ - uses: actions/checkout@<full-length-commit-sha>
+ with:
+ persist-credentials: false
- - uses: oven-sh/setup-bun@v2
+ - uses: oven-sh/setup-bun@<full-length-commit-sha>
- uses: actions/cache@v5
+ uses: actions/cache@<full-length-commit-sha>Apply the same actions/checkout change to the PowerShell lint job as well.
Also applies to: 749-749, 773-773
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 731-731: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 731-731: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 732-732: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/verify-extended.yml around lines 731 - 732, The workflow
jobs using actions/checkout and oven-sh/setup-bun need hardening: pin both
actions to immutable commit SHAs instead of floating version tags, and configure
actions/checkout to disable credential persistence so the workflow token is not
written to .git/config. Make the same checkout change in the PowerShell lint job
as well, and keep the fix centered around the checkout/setup-bun steps in
verify-extended.yml.
Source: Linters/SAST tools
Comment on lines
+35
to
+37
| const port: number = await new Promise((res) => { | ||
| sock.bind(0, "0.0.0.0", () => res((sock.address() as AddressInfo).port)); | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind the UDP listener to loopback only.
The example relies on SLIRP relaying to a host-local socket, but sock.bind(0, "0.0.0.0", ...) exposes that listener on every interface. That widens the receive surface for no benefit and contradicts the loopback-only contract described above.
Proposed fix
const port: number = await new Promise((res) => {
- sock.bind(0, "0.0.0.0", () => res((sock.address() as AddressInfo).port));
+ sock.bind(0, "127.0.0.1", () => res((sock.address() as AddressInfo).port));
});📝 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
| const port: number = await new Promise((res) => { | |
| sock.bind(0, "0.0.0.0", () => res((sock.address() as AddressInfo).port)); | |
| }); | |
| const port: number = await new Promise((res) => { | |
| sock.bind(0, "127.0.0.1", () => res((sock.address() as AddressInfo).port)); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/udp-gateway/udp-gateway.ts` around lines 35 - 37, The UDP listener
is currently bound in the udp-gateway setup using sock.bind with 0.0.0.0, which
exposes it on all interfaces instead of only loopback. Update the bind address
in the UDP gateway flow so the listener is restricted to localhost/loopback
while still obtaining the assigned port from sock.address(), keeping the rest of
the gateway logic unchanged.
Comment on lines
+36
to
+40
| def run(*args: str, check: bool = True) -> str: | ||
| r = subprocess.run([*QC, *args], capture_output=True, text=True) | ||
| if check and r.returncode != 0: | ||
| raise RuntimeError(r.stderr.strip() or f"quickchr {args[0]} exit {r.returncode}") | ||
| return r.stdout |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the subprocess.run check argument explicit.
run() relies on the implicit check=False default while manually inspecting returncode. Passing check=False explicitly is behavior-preserving and clears Ruff PLW1510. The S603/ast-grep command-injection hints are false positives here since args are program-controlled, not request-derived.
♻️ Proposed change
- r = subprocess.run([*QC, *args], capture_output=True, text=True)
+ r = subprocess.run([*QC, *args], capture_output=True, text=True, check=False)📝 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
| def run(*args: str, check: bool = True) -> str: | |
| r = subprocess.run([*QC, *args], capture_output=True, text=True) | |
| if check and r.returncode != 0: | |
| raise RuntimeError(r.stderr.strip() or f"quickchr {args[0]} exit {r.returncode}") | |
| return r.stdout | |
| def run(*args: str, check: bool = True) -> str: | |
| r = subprocess.run([*QC, *args], capture_output=True, text=True, check=False) | |
| if check and r.returncode != 0: | |
| raise RuntimeError(r.stderr.strip() or f"quickchr {args[0]} exit {r.returncode}") | |
| return r.stdout |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 36-36: Command coming from incoming request
Context: subprocess.run([*QC, *args], capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.18)
[error] 37-37: subprocess call: check for execution of untrusted input
(S603)
[warning] 37-37: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/version-matrix/version-matrix.py` around lines 36 - 40, The run()
helper currently relies on subprocess.run’s implicit check=False default while
manually handling returncode, which triggers the lint warning. Update the
subprocess.run call inside run() to pass check=False explicitly, keeping the
existing returncode-based error handling and preserving the current behavior.
Source: Linters/SAST tools
| // README references resolve. | ||
| const readmePath = join(dir, "README.md"); | ||
| if (existsSync(readmePath)) { | ||
| const readme = require("node:fs").readFileSync(readmePath, "utf8") as string; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace require("node:fs") with ESM import.
Line 86 uses CommonJS require("node:fs") in an ESM file. As per coding guidelines, use ESM imports only — add readFileSync to the existing node:fs import and use it directly.
🛠️ Proposed fix
-import { readdirSync, statSync } from "node:fs";
+import { readdirSync, readFileSync, statSync } from "node:fs";
import { existsSync } from "node:fs";- const readme = require("node:fs").readFileSync(readmePath, "utf8") as string;
+ const readme = readFileSync(readmePath, "utf8") as string;📝 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
| const readme = require("node:fs").readFileSync(readmePath, "utf8") as string; | |
| const readme = readFileSync(readmePath, "utf8") as string; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/validate-examples.ts` at line 86, The `validate-examples.ts` script
is mixing CommonJS and ESM by calling `require("node:fs")` inline. Update the
existing `node:fs` ESM import to include `readFileSync`, then replace the
`require` usage in the `readme` assignment with the सीधे imported function so
the file stays ESM-only.
Source: Coding guidelines
Address PR #22 review and the "examples as well-tested as the code" ask. verify-extended.yml — restructure around modes × platforms: - new `run-integration`/`run-examples`/`coverage` toggles; one dispatch runs integration and/or the examples smoke harness across the selected OSes against a chosen routeros-target (branch = the dispatch ref). - `plan-smoke` emits a dynamic smoke matrix from the platform toggles, mirroring the integration jobs. A broken example GATES the workflow on KVM/HVF platforms; macOS/x86 + Windows (TCG) stay non-gating, like integration. - new `coverage` job: informational unit+integration coverage numbers (never gates) — the integration-exercised coverage the push pipeline can't see. - lint-powershell uses examples/PSScriptAnalyzerSettings.psd1. examples smoke harness — picks per-OS representatives (.ts everywhere; .sh/.py via `uv run` on POSIX; .ps1 on Windows); fails fast on a typo'd EXAMPLE_FILTER. PowerShell — .ps1 made ASCII-clean (kills PSUseBOMForUnicodeEncodedFile); common.ps1 Invoke-Qc throws on non-zero native exits and the cleanup catch logs via Write-Verbose; PSScriptAnalyzerSettings.psd1 waives PSAvoidUsingWriteHost (intentional for interactive demos) with rationale. Other review fixes: drop unused TOP_FILES in validate-examples; document cspell in examples.instructions CI list; rewrite the stale DESIGN "Examples Philosophy" section; ci.yml Repo Checks installs shellcheck so lint:shell actually enforces POSIX-sh on every push. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Thanks — this is getting very close. Before merging I'd like to clean up a few remaining items. Some are blockers for this PR, while others should become follow-up GitHub issues rather than expanding the scope here. Merge blockers1. Resolve the remaining CodeQL findingThere is still one outstanding CodeQL security finding: My suspicion is that this is mostly a false-positive because this is example/test-harness code, but I'd still prefer we eliminate it if possible. If this is the 2. Investigate the Extended Verification failuresI manually dispatched Extended Verification (including the new example tasks) and it exposed problems that did not appear in the normal PR CI: https://github.com/tikoci/quickchr/actions/runs/28275634733 Please investigate and resolve the failures before merge. In particular:
I'd like to understand why these differ before merging. 3. Rework the Coverage (metrics only) implementationThe intent of the plan was not to execute another complete integration run. The goal was:
The new Coverage workflow currently appears to rerun essentially the same integration suite, increasing Extended Verification runtime without adding much value. Please revisit this implementation so coverage is produced from the existing integration execution instead of duplicating work. The motivation is comparison of coverage across platforms (Windows/macOS/Linux, x64/arm64), not a second execution of the same tests. Please open follow-up issues (do not solve in this PR)A. Rename
|
This was referenced Jun 27, 2026
Resolve the three merge blockers from the PR #22 review and file the follow-ups, without growing the PR's scope. CodeQL (js/insecure-randomness): replace Math.random() with crypto.randomUUID() in the example helpers (examples/lib.ts, examples/grounding/grounding.test.ts). The taint flowed through example-built machine names into the credential sinks CodeQL reported in src/lib — fixing the source clears both alerts and models good practice. Extended Verification failures: - qemu-args.test.ts "x86 HVF uses host CPU model" was a latent test bug: KVM (like HVF) adds `-cpu host`, so the else branch's `cpuIdx == -1` assertion was wrong on KVM runners. It only surfaced in the coverage job because that is the sole job that runs test/unit/ on a KVM-enabled runner (the unit job has no KVM; the integration job skips test/unit/). Rewritten as an anchor test that accepts hvf OR kvm. - version-matrix.ps1: switch Start-Job from param()+-ArgumentList to $using:, which is the form PSScriptAnalyzer's PSUseUsingScopeModifierInNewRunspaces accepts (4 warnings cleared). Coverage (metrics only): remove the standalone job + `coverage` input — it re-ran the full unit+integration suite as a second job. Reworking coverage as a byproduct of the existing integration jobs is tracked in #30. Updated ci.instructions.md + BACKLOG.md accordingly. Follow-up issues opened: #26 (rename tzspGatewayIp), #27 (troubleshooting capture example), #28 (PowerShell workflow org), #29 (release/verification reuse), #30 (coverage byproduct). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Pushed Merge blockers1. CodeQL finding — resolved. It was 2. Extended Verification failures — root-caused and fixed.
3. Coverage (metrics only) — reworked. You’re right that it re-ran the whole suite as a second job; that was unavoidable with the per-file integration loop (bun can’t merge coverage across separate Follow-up issues opened
Final review
Note: one pre-existing flaky unit test, |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
BACKLOG.md (1)
570-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate historical example references to current names/locations.
The "Completed" section still references the old example paths and names that were renamed in this PR:
examples/matrica/matrica.test.ts→ nowexamples/version-matrix/version-matrix.tsexamples/matrica/Makefile→ deleted (Makefiles disallowed)examples/vienk/vienk.test.ts→ nowexamples/quickstart/quickstart.tsUpdate these historical notes to point to the current artifacts, or add a parenthetical note like "(now
examples/version-matrix/)" so readers can find the current examples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BACKLOG.md` around lines 570 - 578, The BACKLOG.md “Completed” note still points to renamed or removed example artifacts, so update the historical references to the current names used by the examples. In the Completed section, change the `matrica` and `vienk` entries to refer to the new `examples/version-matrix/version-matrix.ts` and `examples/quickstart/quickstart.ts` symbols, and remove or annotate the deleted `examples/matrica/Makefile` reference so readers can locate the current artifacts.project-words.txt (1)
265-266: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSorting broken by the new entries.
The header states "Keep sorted", but
shellcheck/srandwere inserted beforesetuid(267) andsfc(268). Correct order around here issetuid,sfc,shellcheck, … andsrandbelongs much later (afterskyfi). Re-sort so the "Add to dictionary" quick-fix keeps inserting cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project-words.txt` around lines 265 - 266, The new entries in the sorted dictionary list are out of order, breaking the “Keep sorted” requirement. Update the ordering around the affected entries so `setuid` and `sfc` remain before `shellcheck`, and move `srand` to its proper later position after `skyfi`. Make sure the list stays alphabetically sorted in `project-words.txt` so the “Add to dictionary” quick-fix continues inserting cleanly.
🤖 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 @.github/workflows/ci.yml:
- Around line 60-61: The Install shellcheck step in the CI workflow runs apt-get
install without refreshing the package index first, which can cause flaky 404
failures. Update the shellcheck install step to mirror the other QEMU-related
apt steps by running apt-get update before apt-get install, keeping the existing
workflow structure in .github/workflows/ci.yml.
---
Outside diff comments:
In `@BACKLOG.md`:
- Around line 570-578: The BACKLOG.md “Completed” note still points to renamed
or removed example artifacts, so update the historical references to the current
names used by the examples. In the Completed section, change the `matrica` and
`vienk` entries to refer to the new `examples/version-matrix/version-matrix.ts`
and `examples/quickstart/quickstart.ts` symbols, and remove or annotate the
deleted `examples/matrica/Makefile` reference so readers can locate the current
artifacts.
In `@project-words.txt`:
- Around line 265-266: The new entries in the sorted dictionary list are out of
order, breaking the “Keep sorted” requirement. Update the ordering around the
affected entries so `setuid` and `sfc` remain before `shellcheck`, and move
`srand` to its proper later position after `skyfi`. Make sure the list stays
alphabetically sorted in `project-words.txt` so the “Add to dictionary”
quick-fix continues inserting cleanly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99c48afc-fd7b-44ed-9ae5-1f6e293b402a
📒 Files selected for processing (23)
.github/instructions/ci.instructions.md.github/instructions/examples.instructions.md.github/workflows/ci.yml.github/workflows/verify-extended.ymlBACKLOG.mdDESIGN.mdexamples/PSScriptAnalyzerSettings.psd1examples/common.ps1examples/device-mode/device-mode.ps1examples/dude/dude.ps1examples/grounding/grounding.ps1examples/grounding/grounding.test.tsexamples/harness/harness.ps1examples/lib.tsexamples/quickstart/quickstart.ps1examples/rollback/rollback.ps1examples/service-forward/service-forward.ps1examples/trial-license/trial-license.ps1examples/version-matrix/version-matrix.ps1project-words.txtscripts/validate-examples.tstest/integration/examples-smoke.test.tstest/unit/qemu-args.test.ts
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win apt-get install without a preceding apt-get update. If the runner's package index is stale, install can fail with a 404. Every other QEMU install step in verify-extended.yml runs apt-get update first; mirror that here for resilience (shellcheck is typically preinstalled on ubuntu-latest, so this is a low-frequency but real flake risk). Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Jun 27, 2026
mobileskyfi
added a commit
that referenced
this pull request
Jun 27, 2026
…ll lint workflow Post-merge Extended Verification flagged PSUseBOMForUnicodeEncodedFile: the $using: fix from #22 left a non-ASCII em-dash in a comment in version-matrix.ps1 (the exact case examples/PSScriptAnalyzerSettings.psd1 documents as its ASCII-clean guard). Made the file ASCII-clean again, matching the other 9 .ps1 examples. Closes #28: extracted the PSScriptAnalyzer job into a dedicated reusable lint-powershell.yml that runs on its own for any push/PR touching examples/**/*.ps1 (early signal in normal PR CI) and is `uses:`-d by verify-extended.yml, so .ps1 regressions no longer hide behind a manual dispatch. Split out of the held PR #32; the arm64/snapshot half stays there (#31). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mobileskyfi
added a commit
that referenced
this pull request
Jun 27, 2026
…ll lint workflow (#34) Post-merge Extended Verification flagged PSUseBOMForUnicodeEncodedFile: the $using: fix from #22 left a non-ASCII em-dash in a comment in version-matrix.ps1 (the exact case examples/PSScriptAnalyzerSettings.psd1 documents as its ASCII-clean guard). Made the file ASCII-clean again, matching the other 9 .ps1 examples. Closes #28: extracted the PSScriptAnalyzer job into a dedicated reusable lint-powershell.yml that runs on its own for any push/PR touching examples/**/*.ps1 (early signal in normal PR CI) and is `uses:`-d by verify-extended.yml, so .ps1 regressions no longer hide behind a manual dispatch. Split out of the held PR #32; the arm64/snapshot half stays there (#31). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Reworks
examples/from a mix ofbun:testfiles + Makefiles into one durable convention, fixes a factual bug (dude is not x86-only), adds five single-concept examples, and makes examples CI-checkable. Plan went through four review passes (Copilot, Gemini, GPT-5.5, plus grounding of the QEMU-cleanup and dude-arm64 claims).The convention
Every
examples/<name>/is a runnablebun run<name>.ts(library API) +.sh/.ps1CLI mirrors +.pywhere useful + README.grounding/stays the solebun:test, expanded as the reference for writing CHR integration tests. Captured in:.github/instructions/examples.instructions.md(the rules) +examples/_template/examples/lib.ts(runExampleguaranteed-teardown /exampleMachineName/freePort/check),examples/common.sh+common.ps1($QUICKCHRresolution + naming + cleanup)examples/COVERAGE.md— maps every CLI/API surface to its example or a "reason if absent"Changes
vienk→quickstart,matrica→version-matrix. Makefiles deleted;child.ts→harness/tool/;rb5009-arm64.rsc→version-matrix/config/; Python switched touv run.rollback(snapshots),service-forward(--forward/extraPorts),file-transfer(upload/download),device-mode,trial-license(manual-only — MikroTik rate limits, excluded from CI).dude-<ver>-arm64.npkships for 7.21.1–7.23.1 andfindPackageFileresolves it. Removed the skip/pin; added a unit test grounding the correction.--packages a,b(no such flag) silently installed nothing → repeated--add-package.CI
bun run checknow runslint:examples(convention/drift validator: structure, naming, no Makefiles, README links resolve) +lint:shell(shellcheck-s sh); biome now coversexamples/**.verify-extended.ymlgainsinclude-examples/example-filter→ anexamples-smokejob (curated subset + one representative per language + an intentional failure-path case asserting teardown fires on error) and alint-powershellPSScriptAnalyzer job.Surfaced gaps (filed as issues, linked in BACKLOG)
quickchr cpCLI → file-transfer/version-matrix CLI drivers can't transfer files.rootless-l2(socket-lan) example deferred pending a 1-page topology design.Validation
bun run checkfully green; 580+ unit tests pass.quickstart.ts,quickstart.sh(shell infra + trap teardown, no leftover machine),rollback.ts(snapshot save→change→restore).🤖 Generated with Claude Code
Summary by CodeRabbit
examplesconvention with shared script helpers and consistent copy-and-run instructions.