ci: harden PR gates (fmt, clippy, workspace tests, audit) + pin toolchain - #1
Merged
Conversation
…hain Expand ci.yml from test+docs to five jobs: cargo fmt --check, clippy -D warnings, full-workspace tests (was -p karnc), the existing mdBook build, and a cargo-audit RustSec scan. Add a weekly schedule so dependency/advisory drift is caught without a PR, and a concurrency group that cancels superseded PR runs. Pin the toolchain to 1.95.0 via rust-toolchain.toml (and match it in both workflows) so CI is reproducible and a new stable's clippy can't fail -D warnings on unrelated PRs. Add Dependabot for cargo, GitHub Actions, and the two npm packages. All four gates verified green locally before landing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 6, 2026
Merged
Merged
Merged
Merged
This was referenced Jul 16, 2026
This was referenced Jul 22, 2026
accuser
added a commit
that referenced
this pull request
Jul 27, 2026
…rop diagnostics) Review of PR #951 found three confirmed correctness gaps; this fixes the two the reviewer flagged as blocking: #1 — Miscompile: block_uses_emit fell through `_ => false` for Paren, Ok, Err, Question, Call args, RecordConstruction, BinOp — but lower_expr recurses into all of them. `do (Events.emit[E](event))` (one added paren) compiled clean but emitted TypeScript referencing an undeclared `__events` local, with no bynk diagnostic. Rewritten to drive off the existing exhaustive `walk_block_exprs`/`walk_exprs` visitor, so it cannot drift from the lowering again — a new ExprKind variant fails to compile here until the visitor itself is taught it. #3 — UnitTable::events was built and documented as the "is this specifically an event" answer but never actually read for validation. Both directions failed open: `Events.emit[NotAnEvent]` (a local type that isn't an event) silently buffered an emission no subscriber could ever match; `from Events(Typo)` silently wired zero subscribers with no diagnostic. Added `bynk.event.emit_not_an_event` (checker, using a new ResolvedCommons::event_type_names alongside local_type_names) and `bynk.event.unknown_subscription` (a new project-wide validation pass, check_event_subscriptions, since resolving a subscription's owner needs unit_tables + unit_consumes together, mirroring discover_event_ subscribers' own ownership resolution). Also fixed, riding along with #1's investigation: the owner-only- emission check was not first-party-gated (bare `type_name.name == "Events"` string match) unlike every other Events-special-cased site — a third-party capability coincidentally named `Events` with an `emit` method would have gotten a spurious `emit_outside_owner`. Now checks genuine first-party-ness the same way every other site does. Two new negative fixtures (504, 505). Full cargo test --workspace (plus fmt/clippy/doc gates) green.
accuser
added a commit
that referenced
this pull request
Jul 27, 2026
…mission, per-platform fan-out (#951) * wip(events): grammar for event/Events/on-event, symbol-table wiring Not yet complete: exports/consumes gate for events, Events capability + owner-only emission check, DO/non-DO fanout runtime, fixtures, docs, ADR. Part of #939 * wip(events): Events capability, owner-only emission enforcement - capability Events { fn emit[E](event: E) -> Effect[()] } added to the bynk.bynk adapter (external provider, generic-capability-method mechanism, ADR 0281), following the Idempotency precedent. - Owner-only emission enforcement in check_static_call: Events.emit[E] may only name an event E declared in the emitting context, checked via ResolvedCommons::is_local_type -- the primary threat-model guarantee (events.md §6, forged/cross-context emission). - Found and fixed a real pre-existing bug surfaced while grounding that check: check_context_declarations built ResolvedCommons.local_type_names from the already-merged typed.types instead of the unit's own local table, silently widening is_local_type (and therefore .raw/.unsafe() opaque-type access) to every visible type, local or not. - Found and fixed a second pre-existing gap: RESERVED_CONTEXTUAL keywords (case/messages/on/suite, now also event) were admitted as identifiers only at declaration sites (via expect_ident's exemption), not when referenced afterwards in expression position -- latent since messages/on/case/suite shipped, surfaced concretely by `event` colliding with the pre-existing examples/event-log handler `add(event: Event)`. - Registered the two new diagnostic codes and blessed the generated docs/keywords pages, textmate grammar, and golden bynk.ts fixtures that the new capability mechanically touches. Full bynkc test suite green (all binaries, tsc_verify, workerd smoke, ws_behaviour included). Part of #939 * test(events): negative fixture for owner-only emission enforcement The primary threat-model guarantee (events.md §6, forged/cross-context emission) was previously verified only by a manual two-context probe (#939 checkpoint), not a committed test. Two-context project fixture: commerce.order declares and exports PaymentConfirmed; commerce.notifications consumes it and attempts Events.emit[PaymentConfirmed], which must fail with bynk.event.emit_outside_owner. Part of #939 * wip(events): release-at-commit emission buffering Events.emit[E](event) never calls through a constructed provider like an ordinary capability op -- release-at-commit (events.md §3.0) buffers it into the handler's own `__events` local instead, mirroring the existing store-agent commitState boundary (generalised to services, which had no completion boundary before this). ADR 0106's own Consequences section already named this as new machinery deferred to Events ("the buffered/ at-commit tier... rides with Events"), so there was no reuse path to find. Gated on block_uses_emit (mirroring block_uses_send's gate on __exec) so a handler that never emits keeps byte-identical output. Events is dropped from every deps/compose surface -- the call-site interception means there is no EventsProvider for compose (or a synthesised DO's own reconstructed deps) to construct, unlike every other capability. Three separate deps-construction sites needed the same filter (bynk-emit/src/emitter/emit.rs's effective_given and cross_context_caps_used, bynk-emit/src/project.rs's handler_cross_caps and plan_agent_given_deps). Not yet complete: __events is buffered but never flushed -- the fanout dispatch (a Cloudflare Durable Object per ADR 0284, its non-Cloudflare equivalent) is #939's next remaining piece. An aborted handler correctly emits nothing today (the IIFE boundary means a throw skips past the buffer), but nothing yet proves it with a fixture. Full bynkc test suite green (workspace, e2e positive/negative, tsc_verify, examples) -- no regressions. Part of #939 * fix(events): close the first-party-Events guard asymmetry The three deps/compose exclusion sites from the last commit all checked "is this genuinely first-party bynk.Events" before dropping it from deps -- but the call-site interception (lower.rs) and the __events buffer gate (block_uses_emit's caller in emit.rs) matched on the bare name "Events" only. A context declaring its own unrelated `capability Events` would have had its calls silently rewritten into a buffer with no provider ever invoked -- emissions vanishing with no error, the exact class of bug #934 guarded against for Idempotency. Added LowerCtx::is_first_party_events() (mirroring the existing Idempotency distinction) and gated both the lowering interception and both uses_emit computations on it, so a real Events.emit only ever intercepts when it resolves to the shipped first-party capability. Verified the two reachable agent branches directly: is_store_agent is hardcoded true (the pre-ADR-0108 state-record agent kind is dead code), so a store-agent handler with/without a state write are the only paths -- probed both with `given Events`, read the emitted TS, and confirmed `tsc --strict` passes clean. Full bynkc test suite green (workspace, e2e, tsc_verify, examples) -- no regressions. Part of #939 * feat(events): non-Cloudflare fanout dispatch, real emit/subscribe/abort Wires the Bundle/node half of the fanout mechanism DECISION D calls for, proven end-to-end at runtime, not just tsc-checked: - `deps.__eventsDispatch` (mirroring `__exec`'s threading for `~>`) carries a compose-built dispatch closure into every handler that buffers events; the flush now actually calls it instead of doing nothing. - New project-wide subscriber discovery (`discover_event_subscribers`): groups every `from Events(E)` service by E's owning context. No prior art -- cross-context wiring elsewhere is driven by an explicit `consumes` clause; this is the first wiring driven by an implicit relationship (a bare event-type name shared between a publisher and a subscriber). - compose.ts builds each publishing context's dispatch closure from that table, calling straight into each subscriber's `.event()` method in-process (no isolate boundary to cross on this target) -- reusing whatever deps that subscriber already needs, built in the same loop. - `needs_compose` gains a third trigger: a context whose handlers emit, even with no other cross-context reason to compose (Events has no provider, so it was invisible to the existing triggers) -- otherwise a publish-only context's service would simply never get called. Two real gaps found and fixed while proving this against actual output, not just golden diffs: - An `event` declaration's TS type was never emitted at all -- the project-file driver's item loop only ever matched `CommonsItem::Type`; `CommonsItem::Event` fell through silently. A subscriber importing the event type across contexts got a real `tsc` "has no exported member" error, invisible to any single-context fixture. - `__eventsDispatch` only reached a handler that emits *directly*. A handler that merely calls a local agent method which itself emits (the ordinary "wrap an agent behind a service" shape) had no way to forward the field its callee needed -- a real `tsc` type error, caught by building the transitive case, not by any existing fixture. Fixed by reading `cx.agent_given_caps_used` (the same mechanism that already propagates an invoked agent's other `given` capabilities) alongside the direct-emit check, while keeping the `__events` buffer/IIFE wrap itself gated on the direct case only -- a pure forwarder has nothing of its own to flush. New `bynkc/tests/events_behaviour.rs`: composes a real two-context project and runs it under node, observing the subscriber's own side effect (`Logger.info`, captured via stdout) rather than trusting a type check. Proves both halves events.md §3.0 promises: a plain service's emission is delivered exactly once, and an agent handler that emits then violates its own invariant delivers nothing -- `commitState`'s throw happens before the flush is ever reached, the same completion boundary the Bundle wrapper gives services for free. Full bynkc test suite green (workspace, e2e, tsc_verify, examples, the new behavioural test with BYNK_REQUIRE_TSC=1) -- no regressions. Part of #939 * fix(events): Workers target was silently broken for the whole feature Every probe and fixture so far exercised only --target bundle. Compiling the same two-context project with --target workers surfaced three independent, previously-invisible bugs -- none caught by tsc_verify or the e2e goldens, since nothing before this combined Events with a Workers-mode project: - `worker_cross_caps` (workers.rs, the Workers-mode analogue of handler_cross_caps) had no first-party-Events exclusion, so compose.ts still tried to `new bynk__binding.EventsProvider()` -- a class that does not exist. Same fix as the three Bundle-mode sites, a fourth location that needed it. - A subscriber's `on event` handler body was never emitted at all on Workers -- `emit_service`'s per-handler loop skipped it alongside the WebSocket lifecycle handlers, but unlike those (whose real body lives in the hosting agent's DO), an event handler has no other home. The exported service object came out empty (`export const OnPayment = {};`), so there was nothing for any future dispatch mechanism to call into. - A subscriber importing the publisher's event type came out as a genuine *value* import (`import { PaymentConfirmed } from "../commerce-order/ handlers.js"`) -- meaningless under Workers, where each context deploys as its own separate Worker script with no shared module graph (the same #661 hazard already fixed once for cross-context codecs). Narrowly scoped the fix to event types specifically (tracked via each context's own `from Events(E)` headers): a blanket `import type` for every cross-context reference broke a real case first (`bynk`'s `Method`, used as a value via `Method.Get`) before landing on the narrower one. Also: `discover_event_subscribers` iterated two `HashMap`s and pushed into a `Vec` with no sort, so a multi-subscriber event's dispatch order (and the emitted closure's `await` sequence) would vary build to build with no source change. Sorted before use. `deps.__eventsDispatch` is still absent from Workers-mode compose -- that's the fanout Durable Object, the next piece, not fixed here. Full bynkc test suite green (workspace, e2e, tsc_verify, examples, events_behaviour) -- no regressions; caught the `Method` breakage via the examples gate before it reached this commit. Part of #939 * feat(events): Cloudflare fan-out Durable Object + wrangler wiring (Workers target) ADR 0284 commits slice 0 to one fan-out Durable Object per publishing context; this wires it in. A publishing context's compose.ts gets an `__eventsDispatch` that calls into its own `EventsFanout` DO (a new per-context `events_fanout.ts`, since a fan-out DO has no backing AgentDecl for `emit_agent` to emit it from); the DO's compile-time routing table (from the already-built `discover_event_subscribers`) fans out over each subscriber's Service Binding, isolating one subscriber's failure from its siblings. wrangler.toml gains the DO binding/migration and the reverse-direction Service Binding a publisher needs to reach a subscriber it does not itself `consumes`. Subscriber contexts gain a `/_bynk/event/<service>` entry route. Also closes a wire-crossing gap `emit_agent` shares with #527's provider problem: `deps.__eventsDispatch` is a function, so an agent's own DO fetch dispatch (JSON) drops it just like it would a capability provider — an agent whose handler emits now rebuilds it from `env.EVENTS_FANOUT` the same way `given` providers already are. Verified via a hand-compiled two-context Workers-target probe (tsc --strict clean on both Workers and the pre-existing Bundle target) before writing the automated gate: bynkc/tests/ events_workers_wiring.rs asserts the wrangler.toml/events_fanout.ts content directly (nothing else reads it) and runs real tsc --strict over the emitted tree. Real Cloudflare DO delivery stays unverified locally (no workerd/wrangler dev in this suite) — named in the ADR. * docs(events): pending ADRs resolving slice 0's DECISION D and DECISION E Issue #939 (slice 0's implementing proposal) explicitly left two forks for the implementing PR to settle, not assume: DECISION D (the concrete non-Cloudflare fan-out mechanism, and confirming the DO's actual wire shape on Cloudflare) and DECISION E (owner-only emission as a new checker pass, keyed on ctx.input.is_local_type). Both are now shipped code, not proposals — this records the resolutions actually taken, including the reverse-direction Service Binding requirement and the #527-mirroring deps.__eventsDispatch wire-crossing fix found along the way. Version/ADR numbers are assigned by the merge-time stamp, not here. * docs(events): slice 0 book pages, currency banner, roadmap, and track status Docs delta from issue #939: a new "Understand events" guide (the track's first on-ramp for the pub-sub model — event declarations, Events.emit, from Events(E), owner-only enforcement, release-at-commit semantics, and the per-platform delivery mechanism), the Events capability documented in the first-party capabilities reference, the currency banner and appendix-version-history bumped to the version this increment is expected to ship as (v0.238.0, per the pending-file stamp dry-run — corrected at merge if another PR lands first), a changelog row, and the roadmap's "deferred to v1" list updated to reflect slice 0 shipping. design/tracks/events.md's own slice-status and done-when checklists are updated to match what's actually shipped vs. still open (pattern refinement, envelope, versioning, replay). The LSP tooling delta (hover/completion/semantic-tokens/signature-help) needed no new code: all four are already generic over the real first-party bynk.bynk source (which already declares `capability Events`), confirmed by reading bynk-ide's completion module and bynk-lsp's semantic-token classification rather than assumed. Verified: `npm run build` in site/ (starlight-links-validator finds no broken links), the regenerated sidebar.json and llms-full.txt, and the Rust doc-drift guards (diagnostics_registry, grammar_reference, grammar_coverage, keywords_reference, doc_examples) all pass. * fix(docs): revert the currency-banner version bump — the stamp does that bump-version.sh (run by the merge-time stamp, not a feature PR) is what advances the book's "current version" banners, guarded by bynkc/tests/doc_version.rs against the crate's actual released version — the same "don't hand-pick the version" rule the pending-file mechanism already enforces for Cargo.toml/ADR numbers, just also covering these six doc banners. The previous commit bumped them to the version this increment is only *predicted* to ship as; doc_version.rs caught it. Reverted to v0.237; the changelog table row (which the stamp does not write) stays. * fix(docs): revert the manual changelog table row — the stamp inserts it xtask::stamp::apply already inserts a Recent-increments row from the pending file's own changelog field (site/src/content/docs/book/ reference/changelog.md, insert_after_table_separator) — the same memory rule that governs the version banners covers this table too. A hand-added row would have duplicated it at merge. * fix(events): CI fixes — runtime.ts bundling, actor-identity regression Two real CI failures, both pre-existing on this branch (bisected to before this session's work resumed), now fixed: - `dispatchToEventsFanout`/`deliverEvent` had been appended directly to the generated `bynk-emit/src/emitter/runtime.ts` instead of their proper @bynk/runtime source modules (agent.ts/boundary.ts) — the runtime package's own drift guard (`npm run check` in bynk-emit/runtime) caught it. Moved to source, regenerated via `npm run bundle`. - The book's chapter-12 print snippet (an unrelated actor/identity example) started failing `bynk.actor.identity_not_sealed`. Bisected to the owner-only-emission work's `local_type_names` narrowing (bynk-check/src/resolver.rs comment at project/validate.rs:920): that field was correctly narrowed to "declared directly in this unit" for owner-only emission and `.raw`/`.unsafe()` gating, but the actor- identity-sealing check (bynk-emit/src/project/validate.rs) had been relying on its *previous*, broader "local + uses-rebranded" meaning to recognise a `uses`-imported commons type's per-context rebrand as context-ownable. Fixed by having that one check also read `uses_commons_type_names` (the exact set `emit_context_rebrands` already uses to decide what it rebrands) instead of widening `local_type_names` back and re-breaking owner-only emission. Verified: `scripts/check-book-snippets.sh` (all 26 pass), `npm run verify` in bynk-emit/runtime, and the full `cargo test --workspace` (plus fmt/clippy/doc gates) all green. * fix(events): address review findings #1 and #3 (miscompile + silent-drop diagnostics) Review of PR #951 found three confirmed correctness gaps; this fixes the two the reviewer flagged as blocking: #1 — Miscompile: block_uses_emit fell through `_ => false` for Paren, Ok, Err, Question, Call args, RecordConstruction, BinOp — but lower_expr recurses into all of them. `do (Events.emit[E](event))` (one added paren) compiled clean but emitted TypeScript referencing an undeclared `__events` local, with no bynk diagnostic. Rewritten to drive off the existing exhaustive `walk_block_exprs`/`walk_exprs` visitor, so it cannot drift from the lowering again — a new ExprKind variant fails to compile here until the visitor itself is taught it. #3 — UnitTable::events was built and documented as the "is this specifically an event" answer but never actually read for validation. Both directions failed open: `Events.emit[NotAnEvent]` (a local type that isn't an event) silently buffered an emission no subscriber could ever match; `from Events(Typo)` silently wired zero subscribers with no diagnostic. Added `bynk.event.emit_not_an_event` (checker, using a new ResolvedCommons::event_type_names alongside local_type_names) and `bynk.event.unknown_subscription` (a new project-wide validation pass, check_event_subscriptions, since resolving a subscription's owner needs unit_tables + unit_consumes together, mirroring discover_event_ subscribers' own ownership resolution). Also fixed, riding along with #1's investigation: the owner-only- emission check was not first-party-gated (bare `type_name.name == "Events"` string match) unlike every other Events-special-cased site — a third-party capability coincidentally named `Events` with an `emit` method would have gotten a spurious `emit_outside_owner`. Now checks genuine first-party-ness the same way every other site does. Two new negative fixtures (504, 505). Full cargo test --workspace (plus fmt/clippy/doc gates) green. * fix(events): review medium/low findings — rejection handling, prototype collision, name collision - dispatchToEventsFanout now wraps the whole DO round trip in try/catch, not just the !response.ok check — a rejected stub.fetch (network error, the DO throwing) was propagating into the already-committed publishing handler instead of being swallowed like every other fan-out transport failure. - __eventRoutes[ev.type] is guarded with Array.isArray before the for...of — an event type literally named "toString"/"constructor"/etc would otherwise resolve off Object.prototype (truthy, non-array, `??` never catches it) and throw outside every per-subscriber try/catch. - The fan-out DO's class name is now __EventsFanout (double-underscore, matching every other compiler-synthesised identifier in emitted output) instead of EventsFanout — a user agent literally named EventsFanout would have collided, producing a duplicate DO binding and a duplicated new_classes entry. A Bynk `agent` name can never start with `_` (checked directly), so this makes the collision structurally impossible rather than merely unlikely. bynkc/tests/events_workers_wiring.rs updated for the renamed class; full cargo test --workspace and npm run verify (bynk-emit/runtime) green. * fix(events): Bundle-mode subscriber dispatch now isolates failures like the DO does The Cloudflare fan-out DO (events_fanout.rs) already caught per- subscriber; the Bundle/node in-process __eventsDispatch closure (project.rs) was a bare await chain — one throwing subscriber aborted delivery to its siblings and propagated into the already-committed publishing handler, the opposite of ADR 0284's failure-isolation guarantee on the other target. Verified as a real regression, not just plausible: added a sibling subscriber (commerce.audit) to bynkc/tests/events_behaviour.rs whose handler always throws (a local agent invariant, tripped unconditionally), dispatched before the existing notifications subscriber in sort order. Confirmed the test actually fails without the fix (an uncaught InvariantViolation crashes the driver process) and passes with it — the existing "order-123 delivered exactly once" assertion is now a genuine isolation proof rather than a happy-path check that would pass regardless. * test(events): coverage for the two out-of-scope changes riding this PR Review of PR #951 noted two language-visible changes ride along, explained in code comments but with no fixture in either direction: - local_type_names narrowed from typed.types (local+uses+consumes) to table.types (declared here only) — correctly closes the owner-only- emission gap, but also newly rejects .raw()/.unsafe() on a uses-imported commons opaque type inside a context, where it previously compiled. Confirmed against a clean checkout of main (compiles there, rejected here) before adding fixture 506 locking in the new, narrower, "within its defining commons" reading. - parser/expressions.rs's reserved-contextual fix admits case/event/ messages/on/suite as expression-position identifiers (reading back a binding declared with one of these names), not just at declaration sites. Added a parser unit test naming a parameter after each of the five and reading it back in the body — the exact shape that was previously a parse error. * docs(events): record review findings in the pending ADR Adds DECISION F to the fan-out-implementation ADR: the /_bynk/event/ route's lack of boundary decoding, contract-skew guard, and auth is now an explicit, named accepted gap (mirroring /_bynk/call/'s own unauthenticated posture, but named here since Events uniquely advertises an ownership guarantee calls don't). Names two further follow-ups review surfaced: the wrangler.toml migration tag staying "v1" (skips creating the fan-out DO on a redeploy that adds it to an already-deployed context), and the fully-synchronous, single-DO- instance dispatch latency/throughput coupling. Also records what was fixed rather than merely named (the miscompile, the silent-drop diagnostics), and the two out-of-scope changes riding this PR (the local_type_names narrowing's .raw/.unsafe() side effect, the reserved-contextual expression-position parser fix) with pointers to their new fixture/test coverage.
8 tasks
accuser
added a commit
that referenced
this pull request
Jul 27, 2026
Closes the four confirmed lowering bugs the review's "typed hoist" wave names, plus the source-map corruption it shares a root cause with: - #1/#3 (High): lower_expr's stmts: &mut Vec<String> sink let three call sites mishandle hoisted statements. Introduced Lowered { pre, expr } as the honest return type for the canonical lower_expr, keeping the ~90 correctly-behaving call sites on a renamed lower_expr_into wrapper so the fix stays surgical instead of a blind 226-site rewrite: - simple_expr's "hoists nothing" classifier returned simple_expr(value) for any `is` check, missing that a refined variant pattern with no bindings always hoists a receiver temp — panicked the ternary-form if/else path's own debug_assert on valid code (945). - lower_match_as_iife wrapped a hoisting discriminant in a fresh synchronous arrow; a `?`'s early return exited that arrow instead of the enclosing function, so the propagated Err silently became the match's own value (946). - lower_and_with_is flattened the right-hand operand's hoisted statements into a string and spliced it where an expression was required; and the general &&/||/implies path lowered both operands into the same sink unconditionally, defeating short-circuit evaluation for any hoisting right-hand operand (947). - lower_if's own value-position IIFE never consulted maybe_async_iife at all, so a branch containing a genuine `let y <- expr` emitted a bare await with no async keyword — confirmed with `node --check` as a hard syntax error, not merely a missed optimisation (948). The "over-matching" half of #2 (maybe_async_iife's text scan) was investigated and found unreachable: the checker's own bynk.effect.fn_value_in_pure_context rule already guarantees the enclosing scope is async wherever an effectful combinator like forEach could trigger it, so the LowerCtx-flag replacement was not implemented — the confirmed, reachable half (lower_if never calling the helper) is what's fixed. - #4 (Medium): record_span's byte offset is only meaningful against the top-level module buffer; a checkpoint taken while building a local IIFE buffer (lower_if's wrapper, lower_block_as_expr, lower_lambda's block-bodied arrow, build_match_iife) corrupted the source map with an offset resolved against the wrong buffer once spliced elsewhere. SourceMapBuilder::merge already solves the equivalent problem one level up (handler/test bodies), but needs a sub-builder that outlives the call — not available to source_map's &'a RefCell<...> reference for a function-local buffer. Added LowerCtx::without_source_map to suppress recording for the local buffer's duration instead of mis-recording; confirmed the corruption (and the fix) by reverting locally and inspecting the decoded source map directly. - #7 (Low): the CodeWriter proposal, re-scoped to lower.rs — explicitly deferred to Wave 8 ("at leisure" in the review's own ordering), given its scope (a large, mechanical conversion of ~90+ manual indent-tracking sites) is disproportionate to layer onto the same file this wave already changes for correctness. Each of the four lowering fixes is covered by a regression fixture proving the bug reproduces (panic, wrong value, defeated short-circuit, or a real JS syntax error confirmed with `node --check`) without the change and resolves with it; the source-map fix is covered by a decode-based test proving the corruption and its absence directly. Full workspace test suite, clippy, and fmt all pass, including `tsc --strict` over all fixtures; the only failing tests are the pre-existing npx/tsc-sandbox environmental failures already present on main. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
8 tasks
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.
Hardens CI toward a production posture. Merge this first — it introduces the
ci.ymljob names that themainbranch-protection ruleset requires as status checks.What changed
ci.yml: addscargo fmt --check,clippy -D warnings, full-workspace tests (was-p karnc), and acargo auditRustSec scan, alongside the existing mdBook build. Weekly schedule + concurrency cancellation for superseded PR runs.rust-toolchain.toml: pins1.95.0(reproducible CI; stops a new stable's clippy from failing-D warningson unrelated PRs).dependabot.yml: weekly updates for cargo, GitHub Actions, and both npm packages.pages.yml: toolchain pinned to match.All four gates verified green locally across the workspace before landing.
🤖 Generated with Claude Code