Repository navigation
Conversation
561e036 to
5efc6f6
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a draft, breadth-spanning migration across the compiler, two platforms, argv semantics, and a window I/O rewrite whose correctness hinges on new-toolchain compilation and a CI harness-compiler step that has not yet run on the runners, which I cannot verify statically.
Review effort: Balanced
Findings: None
What changed in this PR
This PR advances three toolchain pins in lockstep: the Roc compiler nightly (2026-09-16 → 2026-09-27), basic-cli (0.23.0-rc1 → 0.23.0), and roc-ray (0.10.0-rc5 → 0.10.0). Because all three now name the same nightly, the previous "tolerate one pin-mismatch warning" machinery is retired, and the source is adapted to three forced API/compiler changes.
Changes:
- Engine: basic-cli 0.23 no longer passes the program name in argv, so
Command.parsearms, its expect fixtures, the two e2e extractors, andreexec_with_format!all drop the index-0 slot;Strava.opt_realand the viz draw signatures drop now-redundant..on return unions; a compiler false-positive is worked around by lifting closures (Metrics.lens_scorable,Csv.field). - Window (roc-ray 0.10): every external service routes through
App.Io(threaded asAccess { io, dir }); the one run-time filesystem grant (~/.stride) is passed via argv throughApp.init_for_args; SQLite calls move todb.query!/db.execute!; the CP-fit shell-out is replaced by a directstridesubprocess plus a newjson_numberparser. - Harness/CI/docs:
tests/e2e.rockeeps its own compiler pin (basic-webserver still writes..), gated by a newHARNESS_ROC_TAGinstall step andpin-check.shlogic; docs/ADRs/AGENTS.md updated.
| File | Description |
|---|---|
| src/cli/Command.roc | Parse arms + all expects drop program name; bare call becomes [] |
| src/cli/main.roc | main!/reexec_with_format! stop skipping index 0; comments updated |
| src/cli/Csv.roc | New field helper (extracted from ReportSessions) + expects |
| src/cli/ReportSessions.roc | Uses Csv.field in place of the inline closure |
| src/cli/Metrics.roc | lens_scorable lifted to module level (compiler workaround) |
| src/cli/Strava.roc | opt_real return union loses .. |
| src/viz/main.roc | App.Io/Access rewrite; argv-based dir grant; skeleton refactor |
| src/viz/Db.roc | db.query!/db.execute! migration; json_number + runner-based load_fit! |
| src/viz/Ui.roc | Model field home → stride_dir |
| src/viz/{Heat,Table,Ramp,Plan,Trace,Zones,Curve,Career,Board}.roc | Drop redundant .. on draw! return unions |
| tests/e2e.roc | Verb/pair extractors anchor arms at line-opening [; Db query greps updated |
| tools/roc-viz.sh | Rewritten to forgive no warnings; keeps assertion-count pins (189/182) |
| tools/pin-check.sh | Adds harness-pin reconciliation against HARNESS_ROC_TAG |
| tools/make-viz-app.sh | Launcher passes $HOME/.stride; comment update |
| tools/command-claims.sh | EXPECTED_UNPARSED 50 → 49 |
| justfile | Adds harness_pin/roc_e2e; bumps viz_pin; drops viz-check-strict |
| .github/workflows/build.yml | Harness-compiler install step; Windows launcher grep retargeted |
| .github/workflows/release-please.yml | Linux/Windows launchers pass the stride dir |
| .github/actions/setup-roc/action.yml | Default nightly bumped to 2026-09-27-a3ce7f1 |
| README.md, AGENTS.md, docs/viz.md, docs/roc-new-compiler-notes.md, docs/adr/0015, docs/adr/0017 | Doc/ADR updates for the three pins and the new grant model |
I confirmed: the parser arms and 100+ expects translate consistently (bare invocation [], catch-all [name, ..]); main!/reexec no longer skip index 0; removed rr.Cmd/rr.Files imports have no remaining uses in main.roc; the home→stride_dir rename is complete; json_number expects are correct; the e2e anchored extractors yield the unchanged 36-verb set while correctly excluding the probe invocations; the viz assertion count is exactly 182 (matching the pin); and the harness-install asset name matches the macos-latest (arm64) runner.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is a repo-wide toolchain migration touching both apps whose e2e job is intentionally left red pending an upstream release and whose GUI-window changes CI cannot exercise, so it needs human verification of the gates and live run.
Review effort: Balanced
Findings: None
….10.0, basic-webserver 0.17.0 Fixes #568's wait: basic-webserver 0.17.0 carries the return-position union removal, so the harness builds at zero warnings on this pin and both suites pass on the released package. The three toolchain pins move to their current releases with one compiler for both apps. basic-cli 0.23 no longer passes the program name in argv, so the parser and every fixture that feeds it stop skipping the first element. roc-ray 0.10 passes an App.Io value to update!, and the window reaches files, SQLite, subprocesses and captures through it under grants it declares at startup: the athlete's directory read-write (from argv, since a home is known only at run time), the launch directory read-only, and two programs, stride for the CP fit and sh for the zone offset date reports under the configured timezone. The launchers pass the launch directory as the second argument, read once in init! and kept in the model, so a capture's status row names its file by an absolute path; captures need no grant beneath ./captures. Everything main gained while this branch waited is carried onto the new API: the bus captures (the tagged screenshot answer, the start called from the frame, the stop as a task), the focus identities, the athlete's today resolved once per load and passed into the loaders that anchor on it, the record and delta labels kept inside the plot.
da25c2b to
cf1b1cc
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It is a broad cross-cutting toolchain migration (CLI argv semantics, roc-ray effect routing, grant model) whose correctness rests on gates and a live window run that must be re-verified by a human rather than confirmed here.
1 open finding
🧠 Review effort: Balanced
| @@ -1,5 +1,5 @@ | |||
| app [Context, program] { | |||
| pf: platform "https://github.com/roc-lang/basic-webserver/releases/download/0.15.0/HcMFsVT26qeMvqWtG5rfNhVMWjceYbKh1An4uYpheBVW.tar.zst", | |||
| pf: platform "https://github.com/roc-lang/basic-webserver/releases/download/0.17.0/AC9goxhsjJJdrQtnc2ga3eTiESyh6ZLraZJsCVdEfeZT.tar.zst", | |||
cf1b1cc to
1f2a36a
Compare
….10.0, basic-webserver 0.17.0 The three toolchain pins move to their current releases with one compiler for both apps, and the e2e harness moves to basic-webserver 0.17.0, the first release whose platform source carries no `..` on a return-position union (roc-lang/basic-webserver#237), so the harness builds at zero warnings on this pin and both suites pass. basic-cli 0.23 no longer passes the program name in argv, so the parser and every fixture that feeds it stop skipping the first element. roc-ray 0.10 passes an App.Io value to update!, and the window reaches files, SQLite, subprocesses and captures through it under grants it declares at startup: the athlete's directory read-write (from argv, since a home is known only at run time), the launch directory read-only, and two programs, stride for the CP fit and sh for the zone offset date reports under the configured timezone. The launchers pass the launch directory as the second argument, read once in init! and kept in the model, so a capture's status row names its file by an absolute path; captures need no grant beneath ./captures. The CP fit runs stride directly and reads each JSON field on its own (json_number). The three checks that need neither a window nor the harness run in their own CI job. Everything main gained while this branch waited is carried onto the new API: the bus captures (the tagged screenshot answer through io.capture(), the start called from the frame, the stop as a task), the focus identities, the athlete's today resolved once per load and passed into the loaders that anchor on it (the wrappers that resolved it per call are gone), the record and delta labels kept inside the plot. An expect pins the two argv positions and the four grants.
1f2a36a to
8c9e481
Compare

Summary: move the three toolchain pins to their current releases and keep one compiler for both apps. The Roc nightly goes from 2026-09-16 to 2026-09-27, which is the compiler roc-ray 0.10.0 declares. basic-cli goes from 0.23.0-rc1 to 0.23.0, roc-ray from 0.10.0-rc5 to 0.10.0, and the e2e harness from basic-webserver 0.15.0 to 0.17.0, the first release whose platform source carries no
..on a return-position union, so the harness builds at zero warnings on this pin and the e2e job is green.Engine changes: basic-cli 0.23 no longer passes the program name in argv, so the parser and the code around it stop skipping the first element:
Command.parse(91 arms) and every expect fixture that feeds it, the two e2e extractors that read the parser's arms out ofCommand.roc, andreexec_with_format!.Strava.opt_realloses the explicit..in its return type, because the compiler now opens return unions on its own. The compiler also treats a let-bound closure called with literal arguments as known at compile time, even when the closure captures a runtime value, so two such closures inMetrics.lens_scorableandCsv.fieldare lifted into functions;Csv.fieldgets its own expect.Window changes: roc-ray 0.10 passes an
App.Iovalue toupdate!, and the window reaches files, SQLite, subprocesses and screen captures through it under grants it declares at startup. A home directory resolved at run time cannot be declared from a literal, so the launcher passes~/.strideas the first argument and the launch directory as the second, the config is built from argv (App.init_for_args), and both are read once ininit!(the one place argv may be read) and kept in the model. The grants are that directory read-write, the launch directory read-only, and two programs:stridefor the CP fit andshfor the one script that asksdatefor the zone offset under the configured timezone; captures need no grant beneath./captures. Every launcher passes both arguments: thevizrecipe, the macOS.applauncher, and the linux and windows launchers the release workflow writes. Because roc-ray now declares the pinned compiler,tools/roc-viz.shtolerates no warning at all.The rebase: this branch was written against an older main and is rebased over the twelve commits that landed meanwhile. Everything they added to the window is carried onto the new API: the bus captures (the directive's screenshot answers through
io.capture()tagged with its row, the start called from the frame, the stop as a task, the capture's path absolute under the launch directory the launcher passes), the focus identities, the athlete's today resolved once per load from config throughDb.today_mod!(io.commands(), db)and passed into the six loaders that anchor on it,Career.spine_at, and the Curve label clamps. The SQLite calls main added take thedb.query!/db.execute!form, and the e2e extractor that reads the plan-week SQL out ofDb.rocby regex reads that form.Docs: AGENTS.md, README, docs/viz.md, docs/roc-new-compiler-notes.md (the harness's wait and its end), dated amendments to ADR 0015 and ADR 0017, and comments in
.github/workflows/build.ymlandtools/make-viz-app.sh. The three checks that need neither a window nor the harness (command names, pins, issue-state claims) run in their own CI job.An expect pins the two argv positions and the four grants; dropping the
shgrant or reading the launch directory from the wrong position fails it. The CP fit runsstridedirectly and reads each JSON field on its own (Db.json_number, five expects). The wrappers that resolved the athlete's today per call are gone;load_model!resolves it once and passes it in, andresolve_cwd!is replaced by the second argument.Gates, each run on the 2026-09-27 nightly with its exit code read:
just check,just build,just viz-check,just viz-test(224 tests, pins 224/263),just viz-app,just skill-shapes,just blob-safety(84/155),just command-claims,just layer-check,just pin-check,tools/adapter-fixtures.sh,roc test src/core/Bus.roc;just e2egreen at 1261 checks andjust e2e-syncgreen on basic-webserver 0.17.0 at zero warnings. Live, against a copy of the current database under a scratch home launched through the app bundle's own launcher: the window opened with its database and wrote a live focus row, a png directive wrote/tmp/x/captures/power.pngand the status row named that absolute path, and a recording start and stop wrote/tmp/x/captures/session-trace.webm. I did not runjust schema-check, because it migrates the live database.After merge:
just install,just viz-app, and point~/.local/bin/rocat the 2026-09-27 nightly.