test(ai): clear the memoized proxy cache between proxy tests (RIG-3393) - #45
Open
rigel-mintaka wants to merge 9 commits into
Open
rigel-mintaka wants to merge 9 commits into
rigel-mintaka wants to merge 9 commits into
Conversation
…() (RIG-3255) (#37) * fix(eval): stream one coalesced agent progress event from handle.wait() (RIG-3255) `runEvalWait` emitted agent progress three ways: eagerly before waiting, on a 1-second interval, and once after settlement. The eager pre-wait emit surfaced whatever `latestDetails.progress` held at that instant, so a fast `agent()` that had already reported interim "running" progress before the wait ran would stream that interim snapshot as well as the final "completed" one: two `op:"agent"` events instead of the single coalesced "latest" the caller expects. The ordering is load-sensitive. In isolation the job has not scheduled its first progress callback by the time the eager emit runs (the snapshot guard returns), so only the final emit fires; under parallel test-bucket load the scheduling shifts and both leak through. That is the flake behind the intermittent `agent-bridge-policy` "streams the latest enriched agent progress" failure (`toHaveLength(1)` receiving 2). Drop the eager pre-wait emit. The interval covers heartbeats for a long wait and the post-settlement emit carries the final snapshot, so a wait that settles inside the first interval window emits exactly once (the settled state) and a long wait still streams periodic snapshots. Deterministic, and matches the test's stated "stream the latest" contract. This is upstream code (handle-bridge.ts is byte-identical to can1357/oh-my-pi's current tip); the same fix will be proposed upstream. Spec-impact: none. Refs RIG-3255 Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs(eval): scope the handle.wait() single-emit comment to what holds The removed eager emit left a comment claiming a settling wait "emits exactly once (the settled state)". Neither half is unconditional: on the failure path the final progress report never runs, so the post-settlement emit carries the last interim snapshot, not the settled state; and a subagent that never reported progress emits nothing. Scope the claim to "at most once" and name both sub-paths so a maintainer does not build on an invariant the code does not hold. Co-authored-by: Matt Wilkinson <matt@rigel.build> * test(eval): add deterministic guard for coalesced agent progress emit runEvalWait's single-emit contract had no red->green guard: the existing agent-bridge-policy assertion is load-dependent and passes unloaded whether or not the eager pre-wait emit is present, so a re-added eager emit would slip through local runs and most CI runs. Drive runEvalWait directly against a controlled AsyncJobManager job under fake timers, so the 1s heartbeat interval provably cannot tick. The job reports an interim "running" snapshot, parks until released, then reports the final "completed" snapshot; awaiting the interim signal guarantees latestDetails holds a snapshot before the wait runs. A re-added eager pre-wait emit fires with the interim snapshot (two agent events); the settle-only path emits once. Co-authored-by: Matt Wilkinson <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com> Co-authored-by: Matt Wilkinson <matt@rigel.build>
…rt timeout (#38) The "resumes the long-lived GET stream with Last-Event-ID" test intermittently hung under CPU-starved CI oversubscription, tripping the pending-promise guard. Root cause is a timeout misconfiguration, not reconnect latency. The test built its transport through connectedTransport(), which sets timeout: 50ms (the value the negative-path timeout tests need). startSSEListener derives its GET startup budget as min(1000, floor(requestTimeout / 4)), so a 50ms request timeout yields a 12ms connect budget. Under load the initial GET crosses 12ms, the listener is aborted, and the second notification never arrives, an unbounded hang the guard then surfaces as a timeout. Give this positive-path test its own transport with a generous timeout (matching the sibling resume tests), lifting the SSE startup budget to the full 1000ms cap. Stress at 40-way oversubscription goes from intermittent hangs to 60/60 clean. The guard constant moves 500 to 4000 (staying below bun's 5000ms default per-test timeout so the labeled failure wins) and its comment is corrected to describe the measured behavior: a healthy localhost resume finishes in well under 100ms, so a trip indicates a stalled listener rather than a slow one. Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com>
…4) (#36) * ci: run release and main-event jobs on GitHub-hosted runners (RIG-3144) The `omp-kata` self-hosted ARC runner scale set exists only in the upstream `can1357/oh-my-pi` org and was never registered in RigelBuild, so every non-pull_request job that selected it queues forever with no runner. Pull request runs already land on `ubuntu-22.04`; releases and main pushes did not, which is why the CI-gated npm publish never completed. Collapse the per-job runner selector to `ubuntu-22.04` at all eleven sites (`check`, `rust_validate`, `native_addons`, the six TS test buckets, and `install_methods`) so the GitHub-hosted branch is the only branch. No cache or native-build wiring changes: the `bazel-cache`, `bazel-natives`, and `bun-install` composite actions already auto-select the GitHub-hosted `actions/cache` backend whenever `BAZEL_REMOTE_USER`/`BAZEL_REMOTE_PASSWORD` are absent, which they are off cluster, and `bazel-cache-warm.yml` already seeds the hosted darwin scopes and the bun store. The step-level `github.event_name` branching that picks prebuilt-from-npm addons on pull requests versus a bazel build on main is unchanged. Delete `.github/actionlint.yaml`, whose only purpose was registering the now unused `omp-kata` label; actionlint passes clean without it, and the four `.github/actions/*` composites keep their `omp-kata` mentions only in descriptive comments (no `runs-on:`), so nothing else needs the registration. Re-laid onto upstream v18.1.7 as part of the fork re-sync (see docs/fork-resync.md, Task 2). Refs RIG-3144, RIG-2777 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: fetch fork-built natives on PRs instead of upstream's npm scope (RIG-3144) (#33) * ci: fetch fork-built natives on PRs instead of upstream's npm scope (RIG-3144) The native_addons PR path fetched @oh-my-pi/pi-natives-linux-x64@latest, which is upstream can1357's npm scope. Upstream advanced that package to 18.1.7, whose Linux binding surface no longer matches the fork's 18.0.x test tree, so every fork PR started reddening two required checks (Test TS native/integration, Test coding-agent native/unit) with "MacOSPowerAssertion.start is undefined". The main path was unaffected because it builds natives fresh from the fork commit. The fetch string was missed by the fork scope-rename (it is a hardcoded literal in the workflow, not a rewritten manifest), so fork PRs were validating against upstream's moving registry instead of the fork's own published addons. - Repoint the PR-path fetch and its explanatory comment to the fork's own @rigelbuild/omp-natives-linux-x64 leaf. Its tarball layout is identical (package/pi_natives.linux-x64-{baseline,modern}.node), so the copy steps are unchanged, and its version tracks the fork's own release cadence. - Guard the MacOSPowerAssertion test with a darwin-only early return, matching the platform-gated tests already in the file. A macOS-only power-assertion binding must not run on Linux, and this keeps the test green even if a future fork Linux build drops the macOS export the way upstream's 18.1.7 did. Verified: the fork's @rigelbuild/omp-natives-linux-x64@18.0.3 addon exports MacOSPowerAssertion.start on Linux (reproduced locally: upstream's build does not, fork's does), so the repoint alone turns the checks green; the guard is defense in depth. actionlint clean; the guarded test passes under bun:test. Spec-impact: none Co-authored-by: Matt Wilkinson <matt@rigel.build> * test(natives): drop the darwin guard on the power-assertion test (RIG-3144) The prior commit added a `process.platform !== "darwin"` early-return to the MacOSPowerAssertion test on the premise that the binding is macOS-only. That premise is wrong: the class is compiled unconditionally and start() has an explicit non-macOS arm returning a no-op handle (crates/pi-natives/src/power.rs start/stop cfg arms), and the published contract documents the cross-platform no-op handle (packages/natives/native/index.d.ts). The addon exports it on Linux. Because every bun-test job runs on ubuntu, the guard was unconditional dead code that skipped the body on 100% of CI runs, deleting coverage rather than gating it, and would let a future fork Linux build that drops the export pass green instead of reddening. The scope repoint in the parent commit is the complete root-cause fix on its own; the unguarded body passes against the fork's published addon on Linux. Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: persist the linux bazel disk cache across main runs (RIG-3144) (#32) * ci: persist the linux bazel disk cache across main runs (RIG-3144) ## Problem The two main-event Rust/Bazel jobs, `rust_validate` (workspace tests + clippy + rustfmt over `//crates/...`) and `native_addons` (the six cross-compiled `//:natives-*` addon links), both requested the `linux` disk-cache scope but nothing ever **saved** it. The only Bazel cache-save step lives in the `bazel-natives` composite action, which only the `release-*` and warm scopes use. So the `linux` scope had a restore side with no producer: every main push rebuilt the entire Rust workspace and every native addon fully cold. Confirmed at source: the repo's Actions cache held zero `bazel-disk-v3-linux-*` archives, only `release-darwin-*` and `bun-*`. The cold native build is the roughly hour-long long pole on every main run, and every main push also recompiled the Rust workspace from scratch for `rust_validate`. ## Change Give each job its own save. The two jobs build under different Bazel configs (test + clippy vs release-codegen addon links), so their actions are keyed differently and cannot share one archive. Splitting the scope keeps each job's archive coherent: - `rust_validate` uses scope `linux-rust` and saves on `save-needed`. - `native_addons` uses scope `linux-natives` and saves on `save-needed` (guarded, like its build steps, to the non-PR path). Each save mirrors the existing `bazel-natives` save exactly: same `actions/cache/save` pin, same `~/.cache/omp-bazel-disk` + `~/.cache/omp-bazel-repo` path list, keyed by the `bazel-cache` action's `cache-key` output. The `save-needed` output is already false on an exact-key hit, so an unchanged run is a pure cache replay that saves nothing; a first run on a new key (or after a crates change) restores via the prefix/bare fallback, builds, and saves the refreshed archive. ## Effect Between upstream re-syncs `crates/**` does not change, which is the common case, so a later main run replays the cached Rust actions and the cross-compiled addon links instead of paying the multi-GiB rustc cost cold. This is the fork-side lever that makes frequent releases cheap: the Rust closure only rebuilds when it actually changes. Job names are unchanged, so the branch-protection required checks are unaffected. No warm workflow is needed for these scopes because both jobs run on every main push and self-seed continuously, unlike the darwin release scopes that only build at release time. Scope note: on the shared repo cache quota, the toolchain half of the repo cache now exists under two linux scopes rather than one. Eviction is graceful content-addressed LRU and self-heals, and a real remote cache (reachable from our own CI) removes the quota ceiling entirely as the durable follow-up. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: document the linux bazel cache quota budget and path-mirror invariant (RIG-3144) Self-review follow-up on the linux bazel cache persistence change. - Add a quota note above both new save steps: four GiB-scale bazel scopes (linux-rust, linux-natives, release-darwin-x64, release-darwin-arm64) now share the 10 GiB repo cache, so a fifth scope needs an eviction plan first. GitHub evicts least-recently-used archives silently on overflow, and a quota-driven save failure only degrades to a warning line, so the symptom would be a silently slow CI rather than a red build. - Add the "must mirror the restore path list" warning above the path list in both new save steps, matching the note the bazel-cache restore action and the bazel-natives save already carry. The restore path list is now replicated at four sites; without the note the two new copies can silently drift and produce a partial cache, the same defect class this change fixes. Spec-impact: none Co-authored-by: Matt Wilkinson <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com> Co-authored-by: Matt Wilkinson <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com> Co-authored-by: Matt Wilkinson <matt@rigel.build> * test(pi-shell): disable job-control pipeline kill test on hosted CI (#29) `kill_builtin_signals_every_process_in_a_jobspec_pipeline` self-suspends a two-process pipeline with `kill -STOP` and expects the shell's job-control to observe the stop within a 5s timeout. Its SIGSTOP/SIGCONT and process-group semantics do not hold under the bazel linux-sandbox on GitHub-hosted runners, so `run_string` never returns and the test times out (`pipeline did not stop`). It passed only on the previous self-hosted infrastructure; it has never completed on a hosted runner. Mark it `#[ignore]` so the `rust_validate` gate is green on hosted CI. The test stays compiled (no bitrot) and is trivially re-enabled with `--include-ignored` or by removing the attribute if the shell's job-control or the CI process model changes. The other 862 pi-shell tests are unaffected; primary coverage is intact. Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com> Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs: re-add the fork re-sync design record after the reset (RIG-3144) The reset to upstream v18.1.7 (Task 1) discards `docs/fork-resync.md`, which lived only on the pre-reset fork history: the record is absent from upstream `main` (the reset target) and from the pre-reset fork tip, so a bare reset leaves the frozen contract off `main`. Re-lay it here as the first thing on the reset base so Tasks 3 through 8 execute against the record on `main`, matching the design's Task 2 step 0 (T1b). The content is byte-for-byte the record frozen by PR #35; no edits. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: pin PR-path native fetch to the checkout's upstream base version (RIG-3144) The reset to upstream tip (v18.1.10) inverted the premise of the fork-scope natives repoint. The PR path fetched `@rigelbuild/omp-natives-linux-x64@latest`, the fork's own scope, whose only published addon is `18.0.3` (the fork's last release, roughly a hundred commits behind the reset tree). The v18.1.10 TS tree calls native functions that addon predates (`vcsGitDiscover`, `editDescription`, `extractInlineSloppyRegions`), so every native-backed TS test job died with "X is not a function" and the required `CI` check stayed red. Immediately after an upstream re-sync the fork's `crates/` tree equals upstream's (the reset base has zero `crates/` drift from the v18.1.10 tag), so upstream's own `@oh-my-pi/pi-natives-linux-x64@18.1.10` is the exact binding match and exports all three functions. Point the PR-path fetch back at the upstream scope, pinned to the checkout's own upstream base version rather than a floating `@latest`. The version is read from `packages/natives/package.json` and any `-rigel.N` fork suffix is stripped, so the pin resolves the upstream base for both a plain upstream version (`18.1.10` -> `18.1.10`) and a post-release fork version (`18.1.10-rigel.1` -> `18.1.10`). This tracks future re-syncs without another edit: the fork's own native line only diverges once it accrues `crates/` changes of its own, at which point the durable fix is publishing a fork addon at the fork version, tracked as the post-reset native-release follow-up. Verified: upstream's 18.1.10 addon exports the three functions (116 exports total); staged into the CI `bazel-bin/` layout it passes the job's own smoke check; the reset tree has zero `crates/` drift from the v18.1.10 tag. actionlint rc=0. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: correct the addon-safety comment and bound the hosted bazel jobs (RIG-3144) Review follow-ups on the hosted-runner + natives-fetch changes. - Narrow the native_addons comment to the guarantee that actually holds. The workspace loader skips its version sentinel for workspace loads (packages/natives loader-state), so an upstream addon under a fork checkout is not validated against the fork's own bindings. Visible failure is guaranteed only for a newly-added export; a change to the behavior of an existing export tests silently against upstream semantics until the fork publishes its own @RigelBuild addon. The old wording claimed any fork-only native change "fails visibly", which overstates the protection. - Add timeout-minutes to the three bazel jobs relocated onto GitHub-hosted runners (check 30, rust_validate 90, native_addons 90). They were the only jobs in the workflow without a bound, and the diff's own premise is that a hosted bazel run can hang rather than fail (the job-control test times out under the linux-sandbox). rust_validate gates the release chain, so an unbounded hang would stall it for the GitHub default of 360 minutes; a bounded fail-loud is the house posture. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * test(pi-shell): re-run the ignored kill test's body in the isolated child (RIG-3144) The re-lay disabled `kill_builtin_signals_every_process_in_a_jobspec_pipeline` with `#[ignore]` (it times out under the CI bazel sandbox, which lacks full job-control stop/continue semantics). That test is a self-re-exec harness: the outer invocation calls `run_isolated_kill_test`, which spawns the test binary with `--exact <name>` and a marker env var, and the real body only runs in that child. The child argv carried `--exact` but not `--include-ignored`, so once the outer test is `#[ignore]`d the child filters it out, runs zero tests, exits 0, and the outer `assert!(status.success())` passes vacuously. Anyone re-enabling the test by running with `--include-ignored` would get a green result while the assertions never execute, and a regression in `kill %1` pipeline signalling (the sole assertion of `process_ids()` in the crate) would ship undetected. Pass `--include-ignored` in `run_isolated_kill_test` so the re-exec child actually runs an ignored body. It is a no-op for the three non-ignored callers (`kill_builtin_refuses_own_pid`, `kill_builtin_refuses_own_process_group`, `pkill_builtin_excludes_own_pid`). This mirrors the existing precedent in this file: `process_test_sleeper` is `#[ignore]`d and its spawner already passes `--ignored`. Also cite RIG-3249 in the `#[ignore]` reason: that issue owns re-enabling the test (root-causing the sandbox job-control gap or making it deterministic), so the temporary ignore has a tracked owner rather than becoming permanent. Verified: the three non-ignored isolated kill tests still pass, and with the fix the ignored test's body executes and passes when run outside the sandbox with `--include-ignored` (it stays ignored inside the CI bazel sandbox). Spec-impact: none. Refs RIG-3144, RIG-3249 Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs: reconcile the frozen re-sync record with the executed reset (RIG-3144) The record froze via PR #35 naming reset target v18.1.7 (c4da0d0). Upstream advanced before the reset ran, so main actually reset to v18.1.10 (ddde7db). Left unreconciled, the on-main contract that Tasks 3/7/8 execute from would instruct the opposite of what shipped and carry a live, copy-pasteable force-push command naming a three-tags-stale SHA. - Add a dated post-execution reconciliation note after the status line: reset target is v18.1.10 (ddde7db); the PR-path natives fetch ships from the upstream scope pinned to base version (the @RigelBuild scope has only 18.0.3, which lacks v18.1.10 bindings) and flips back to @RigelBuild once the fork cuts its first post-reset native release; version scheme is the Matt-ruled <upstream-version>-rigel.N, superseding the old npm-floor-18.1.8 + rigel-v* proposal. - Neutralize the Task-1 force-push runbook: it has already executed, so its commands are commented out and marked DO NOT RE-RUN, with the SHA corrected to the real target for provenance. - Title v18.1.7 -> v18.1.10; Status Draft -> Frozen (it merged as PR #35). Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: drop the dead kata clause from the warm-cache header comment (RIG-3144) The bazel-cache-warm.yml header still justified per-image warming with "a kata-produced disk cache misses every action on a hosted image". With no omp-kata runners in the fork, that clause describes nothing real. Keep the still-true constraint (hosted runners see only default-branch actions/cache entries, and bazel action keys do not transfer across runner images) and drop the kata reference, matching the same rewrite already applied to the warm_bun rationale lower in the file. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: raise native_addons timeout above the measured cold build (RIG-3144) Review round 2 follow-ups. - `native_addons` timeout-minutes 90 -> 150. The prior 90 sat below this repo's measured 84-minute cold cross-compile (the six addon links, run 33789708655), and a job-level timeout kills the job before its `Save bazel disk cache` step runs, so a killed build persists nothing and every retry starts equally cold and times out again. 150 gives ~1.8x margin over the measured cold run while still bounding a genuine hang inside GitHub's 360-minute ceiling. The first post-reset main run is cold, so this is load-bearing for Task 2's acceptance gate. - Correct the ignored kill test's header comment: "Disabled on GitHub-hosted CI" -> "Disabled under the CI bazel sandbox", matching the actual mechanism (the bazel linux-sandbox, which applies on any runner bazel executes on) and the #[ignore] reason string. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * docs: correct the reconciliation note's blast-radius and supersession scope (RIG-3144) Review round 2 follow-ups on the re-sync record's reconciliation note. - The runbook's `sha=` was corrected to the executed target in the prior commit, which falsified the note's own prose. Item 1 and the runbook preamble said the SHA was "stale" and that re-running "would roll main back three tags", both describing the old v18.1.7 value. With the corrected SHA, re-running would reset main to the reset point and discard every post-reset re-lay, a strictly larger blast radius. Reword both to say the SHA is the executed target and re-running discards the re-lays. - Broaden item 3's OQ1 supersession from the one Open-Questions entry to a blanket rewrite, matching item 1: the Global Constraints "Version + tag scheme" bullet and Task 3's "Depends on OQ1" paragraph both still stated a hard block on a question that has landed, so an agent picking up Task 3 would park work that is actually unblocked. State that Tasks 3/7 are no longer gated and the `--match v*` globs need no change under the `-rigel.N` scheme. - Add item 4: the pre-reset safety tags (OQ6) are pushed, so OQ6 no longer reads as outstanding. Spec-impact: none. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: re-add the aggregate CI gate the ruleset requires as its status context The branch ruleset requires a single status context named `CI`, but the re-sync's ci.yml carried only the per-job check-runs (their display names), not the aggregate job whose check-run posts that context. With nothing emitting `CI`, every PR sat mergeStateStatus=BLOCKED on a forever-pending required check and could only be merged by org-owner override. Re-add the `ci` job (name `CI`) that `needs` every PR-relevant job and fails closed on any non-success result. rust_validate is excluded because it is `if: github.event_name != 'pull_request'` and a skipped need would strand the gate; native_addons is excluded because every test job already needs it, so its failure propagates transitively. `if: !cancelled()` makes the gate run even when a need fails, so it posts `CI` = failure rather than a never-created, pending context. This is a fork-only gate: upstream defines no aggregate CI job, so it stays out of the upstream-bound branches. Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: bound the CI gate's runtime and scope its comment to what holds Two review nits on the aggregate CI gate. Add timeout-minutes: 5 so the job holding the sole required context cannot pin CI pending for the 360-minute default if a runner wedges (every other job in the file sets an explicit bound). Narrow the comment's "always posts a CI check-run" to "whenever the workflow runs it posts", since a run excluded by the pull_request paths filter posts nothing and a genuine cancel posts a cancelled conclusion, not a pass/fail. Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: always-trigger ci.yml with a change-detection gate (RIG-3144) The branch ruleset requires the `CI` rollup on every PR, but ci.yml carried a `pull_request.paths` filter, so a docs-only PR never triggered the workflow and the required `CI` check sat forever pending, unmergeable (PR #35 hit exactly this). Drop the trigger's `paths:` filter so the workflow always runs on PRs, and recreate the filter one level down as a per-job gate: a new `setup` job runs `scripts/ci-paths-affected.ts` against the PR diff and outputs `affected`; `check` and `native_addons` gate on it (the eight test/install jobs already cascade off `native_addons.result == 'success'`), so a docs-only PR skips the expensive matrix while the cheap rollup still posts `CI`. The change detector is fail-safe by construction: a push/dispatch event, an unresolvable base ref, or any git failure resolves to affected=true, so the only way to skip the heavy jobs is a clean diff that provably touches no code path. A bug can waste CI minutes; it can never let untested code merge. The rollup now treats a gated job's `skipped` as pass ONLY when `setup.outputs.affected == 'false'`; a failed setup leaves that output empty, so an excused skip is never granted and the rollup reds. Never a blanket skipped-is-green. Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: fix two change-detector fail-opens found in review (RIG-3144) Review of the always-trigger CI gate (commit c93871c) surfaced two ways the change detector could wrongly report `affected=false` and skip the matrix on a PR that does change code, which is the one failure mode the gate must never have. Both are fixed here as an additive commit on the reviewed head. ### `git diff` rename detection hid moved-out code (fail-open) `git diff --name-only` runs with rename detection on by default, so a rename prints only its destination path. A PR that moved code out of a gated directory (for example `packages/x.ts` to `docs/x.md`) reported only the docs destination and yielded `affected=false`, skipping every test while deleting code the workspace still imports. Verified against real history: commit 2e45297 moves 36 files out of `packages/` and reported 0 `packages/` hits under the old flags. Fix: pass `--no-renames` so every rename decomposes into its delete+add pair and the source path is seen. The same commit now reports 36 `packages/` hits. ### `CODE_PATHS` omitted config the `check` job reads (fail-open) The path list was a faithful mirror of the old `pull_request.paths` trigger, but that inherited list omitted root config the gated `check` job consumes (`tsconfig*.json`, `.oxlintrc.json`, `.oxfmtrc.json`, the `types/` root). As a trigger the gap was benign: such a PR simply never started CI, and the missing required check blocked the merge. As a job-level gate it became load-bearing. A config-only regression now yielded `affected=false`, skipped `check`, and the rollup excused the skip, so a merge-blocking gap turned into a silent green. Fix: add those inputs to `CODE_PATHS` and document that a gate must cover every gated job's inputs, not just mirror the old trigger. ### Also - Empty diff now fails safe to `affected=true`. An empty file list is doubt (a force-push race or botched merge-base), not proof nothing changed, so this restores the module's stated "any doubt runs the matrix" invariant. - `writeOutput` appends to `$GITHUB_OUTPUT` directly instead of read-modify-write. - Rollup comment: fix `reding` typo; document the three-edit sync a new gated job needs; note nix.yml deliberately keeps its own path filter (not a required context). - Tests now defend the fail-closed contract end to end. A temp-repo suite drives `main()` for the rename, docs-only, code, push-event, and unset-SHA cases (red-green on the rename guard), plus config-input assertions. Verified: bun test 14/14 (rename guard proven red without `--no-renames`); check:tools (oxlint + oxfmt) clean; actionlint rc=0; tsgo reports zero errors in both files; real-ref smokes (2e45297 to true, docs-only to false, code to true). Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: cover two more gated-job inputs in the change detector (RIG-3144) The confirming review of the fail-open fixes (commit 77fb62e) found the same class of bug on two more inputs the gated jobs read but the detector's path list did not cover, so a PR touching only those paths was classified affected=false, the job skipped, and the rollup excused it: CI green on a change that would have failed. ### `.cargo/config.toml` feeds the gated `check` job The `check` job runs `cargo fetch --locked` then `cargo deny --locked --offline` (ci.yml, gated on affected=='true'). Cargo reads `.cargo/config.toml` from the workspace root before doing anything, and a malformed or semantically-wrong one reds `cargo fetch` (verified: exit 101 on a broken table). The file pins the CMake policy version, the generator, and `embed-metadata`, and its own comments tie it to the rust-toolchain pin, so it is exactly the kind of file a toolchain PR edits. nix.yml already lists `.cargo/**` in its own paths filter, so the gate was strictly less careful than the workflow it is told not to align with. Fixed by adding a `.cargo/` prefix to CODE_PATHS. ### `python/robomp/web` is a real bun workspace member Root package.json declares it in `workspaces.packages` and bun.lock carries it, so every gated job's `bun install --frozen-lockfile` reads its manifest. A dependency edit to that manifest without a regenerated lockfile fails the frozen install (verified: exit 1, "lockfile had changes, but lockfile is frozen"), yet nothing under `python/` matched CODE_PATHS. Fixed with a `python/robomp/web/` prefix, scoped to the web member so the 149-file python tree (whose pytest suites this workflow never runs) does not force the matrix. ### The detector's own test now runs in CI `test_workspace` invokes a hardcoded test list, and ci-paths-affected.test.ts was not on it, so the red-green rename guard added last commit was never enforced on any future PR: a deletion of `--no-renames` would have stayed green in CI. Added the file to that list. It is fast (no native artifacts) and, because editing the detector matches the `scripts/` prefix, the gated job that runs it is never skipped on a PR that touches it. ### Test-suite integrity and fail-safe coverage - GITHUB_OUTPUT moved to a temp dir OUTSIDE the fixture repo; inside it, the harness's own output file was tracked by `git add -A` and showed up in the diff under test. - The fixture git env is now hermetic (GIT_CONFIG_GLOBAL/SYSTEM/NOSYSTEM), so a host `commit.gpgsign` or `diff.renames` config cannot skew a run. - `headSha` now checks the git exit code, so a rev-parse failure throws instead of silently returning an empty SHA that would mask a real result. - Added end-to-end cases for the highest-value fail-safe paths: a cargo-config change (affected=true), an unresolvable base SHA hitting the catch (affected=true), and an empty same-sha diff (affected=true). Verified: bun test 17/17 (the cargo-config guard is red-green: removing `.cargo/` from CODE_PATHS fails exactly the two cargo assertions); check:tools clean; actionlint rc=0; tsgo zero errors in both files; real-ref smokes correct (rename to true, docs-only to false, code to true). Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * ci: gate the last two cross-root fixtures the change detector missed (RIG-3144) A confirming review of the prior fold proved the same fail-open class one directory over: two gated test suites read repo-root fixtures outside their package, and neither path was in the detector's allowlist, so a fixture-only change was classified affected=false, its suite skipped, and the rollup excused the skip green. - docs/tools/: the tool-doc coverage test asserts a page exists for every builtin tool, so a docs-only rename reds test_coding_agent_runtime. - assets/: the terminal-image test reads assets/python.webp, so an assets-only change reds test_coding_agent_native. Both added to CODE_PATHS with matcher + end-to-end guards proven red-green. Also hardened the test fixture the detector relies on: the GITHUB_OUTPUT relocation now has a guarding assertion, runMain defaults the SHA/event vars so an ambient PR_BASE_SHA/PR_HEAD_SHA can't invert a fail-safe case, headSha shares the hermetic git env, and the unset-SHA fail-safe is split into its three shapes. Registered the suite in package.json test:scripts so the local aggregate matches CI. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> * test: name the F3 output-file guard and drop a vacuous diff assertion (RIG-3144) Round-3 review of the change-detector reworking flagged two test-quality lows on the M2 fixture guards: - The `diffNames(base, head)` assertion in the docs-only test ran before the first `runMain()` wrote GITHUB_OUTPUT, so it stayed green even when the output file was moved back inside the fixture worktree. It never defended F3 and its comment claimed otherwise. Removed the assertion and the now-unused helper; the test's own `affected=false` expectation already fails if a stray file leaks into the diff. - The real F3 defense lived in `beforeAll`, so a regression reported an unnamed failure. Extracted it into a named `it("keeps GITHUB_OUTPUT outside the fixture worktree (F3 guard)")` so a future break points at itself. Red-green: moving the output file back inside the repo now reds that named test specifically. Suite 22 pass / 0 fail; oxlint, oxfmt, actionlint, tsgo clean. Refs RIG-3144 Co-authored-by: Matt Wilkinson <matt@rigel.build> --------- Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com> Co-authored-by: Matt Wilkinson <matt@rigel.build>
…ng/tier on resume (RIG-3225) (#34) ## Problem On `--resume`, an explicit `--config`/`--profile` overlay's `modelRoles` were inert: the resumed session always restored the model, thinking level, and service tier baked into it at its original launch, so an updated profile never took effect. This is why the 2026-09-03 dream-team relaunch (`spawn-agent --resume <sid> --profile dream-team`) ran the fleet on each session's stale baked model instead of the profile's opus-5 stack. The restore is deliberate for a bare resume — a conversation should not be silently moved off the model it is running — so the fix is an explicit opt-in, not a behavior change to the default resume path. ## Change Add a `--reapply-config` CLI flag and a `reapplyConfig` SDK option (`CreateAgentSessionOptions`). When set, a resume adopts the config-resolved default **model**, its **thinking level** (including a `:high`/`:xhigh`/`:max` selector suffix on the default role), and its **service tier** instead of the values baked into the session. Default (unset) preserves the existing resume-restores behavior exactly. Adoption is **per-knob**: a knob is taken from config only when config actually specifies it, otherwise the session's own value is kept. So an overlay that only retunes a non-default role (or only a tier, or tombstones the default) never silently yanks a resumed conversation onto an arbitrary fallback model — config with nothing to say for a knob leaves that knob on the session's value. Locus is `createAgentSession` in `sdk.ts`, where both the CLI and SDK embedders (Compass) converge, so one option fixes both surfaces. Three restore sites are gated: - session-model restore array construction is always populated, and its early restore + the later reclaim loop are skipped only when `adoptConfigModel` (`reapplyConfig && !hasExplicitModel && config names a default role`); the session model therefore stays available as the fallback. "Config names a default" mirrors the resolver's own notion — a blank value, the bare `default` sentinel, or a tombstoned/absent `modelRoles.default` all count as no default, so an overlay that only retunes a non-default role (or only a tier) keeps the session model. When config names a default it cannot resolve (a typo), a post-`tryResolveDefaultRole` block restores the baked session model + thinking rather than dropping to an arbitrary `pickDefaultAvailableModel`. Extension-provided `litellm/*` defaults resolve on the post-extension path. - persisted `thinking_level_change` is skipped only when `adoptConfigThinking` (`adoptConfigModel && the default role carries an explicit selector`) — so `--model --reapply-config` keeps the session's level rather than dropping to a bare model default - session `service_tier_change` is merged per family when config specifies any `tier.*`: config's specified families override, families the config omits keep the session's baked tier. Unlike model/thinking, the tier is NOT gated on `--model` — `--model` pins only the model, and the dedicated `--service-tier` override still wins over the merge. A resume under `--reapply-config` is otherwise silent, so it now surfaces one notice via `modelFallbackMessage`: naming the adopted model when config swaps the session onto a different one; naming the unresolved config default (with its resolver warning) when a broken default falls back to the session's own model; and naming both the unresolved default and the unrestorable session model when neither resolves and the model comes from an arbitrary availability pick (the case a bare resume also warns about). The notice is gated on a baked session model existing, so a resume that adopts the same model the session already ran — or a fresh/no-model session — stays silent. Like `--model`, the adopted values are a per-run intent and are not persisted back as `*_change` entries, so a later bare resume still restores the session's own values; `spawn-agent` passes `--reapply-config` on every relaunch to keep the fleet on the profile. ## Tests `agent-session-model-persistence.test.ts`: model, thinking, and service tier each adopt the config value on resume with `reapplyConfig`, and each restore the baked session value on a bare resume without it; the session model/thinking are kept (silently) when config specifies no default, tombstones it (`null`), names an empty string, or names the bare `default` sentinel, and are kept with a notice when config names an unresolvable/typo'd default; a double failure (unresolvable config default AND unrestorable baked model) is reported naming both; a session with no `model_change` entry stays silent (no notice naming a nonexistent session model); `--model` + `reapplyConfig` keeps the session's thinking level; service tier merges per family (session `openai:priority` + config `google:flex` keeps both); the swap and broken-config notices are each asserted on their own branch (`resumed on` vs `kept the session's`, neither matching the double-failure `could not be restored`) so an inverted discrimination fails; and a `reapplyConfig` resume appends no new `model_change`/`thinking_level_change` (the per-run non-persistence contract). `flag-tables.test.ts`: `--reapply-config` parses to `reapplyConfig` without consuming the initial message. `profile-bootstrap.test.ts`: `--reapply-config` is exempted as a valueless flag so a trailing `--profile` still activates. Spec-impact: none. Refs RIG-3225 Co-authored-by: Matt Wilkinson <matt@rigel.build>
getProxyForProvider memoizes a miss, so a resolution that happens before its env var is set is cached as undefined and never re-read. The suite's beforeEach restores env but not the cache, so the outcome depends on whether anything resolved that provider id earlier in the module: cold miss -> undefined set PI_PROXY_<ID>, retry -> undefined (stale, never re-resolved) The module already exports __resetProxyCache as a test seam and two other test files call it; this one, which owns the cached function, did not. Co-authored-by: Matt Wilkinson <matt@rigel.build>
The `beforeEach` reset added in the previous commit was inert: the cache is keyed by provider id, and all 15 ids resolved anywhere in the file were distinct, so no cached entry was ever read back. Suppressing the reset left the suite fully green, which meant a future edit deleting the call — or an author removing an apparently unused import — would silently reintroduce the order-dependence the commit set out to remove. Adds a `resolver cache isolation` describe that shares one provider id across two cases, the only shape that can observe a stale entry. Red-green measured on this tree: with the reset suppressed, 46 pass / 1 fail (the first case reads back the miss memoized by the second's predecessor); with it active, 47 pass / 0 fail. Also corrects the block comment above `beforeEach`. It had claimed `github-copilot` was "reachable from any other test in the file" as present fact, which was not true of the file as written — the id was resolved exactly once. The comment now points at the isolation tests that make the guard observable. Co-authored-by: Matt Wilkinson <matt@rigel.build>
…3393) The witness added in the previous commit fired off `github-copilot`, an id it shared with `normalizes hyphenated provider ids to underscores`. That made it work for a reason it did not document and did not own: the entry its first case read back was a memoized hit planted by that unrelated test, not the memoized miss the case name claimed. Re-idding or deleting the other test moved which case failed, and the case that looked like the real assertion was a verbatim duplicate of it — so deleting the odd-looking one as cleanup would have restored the inertness silently. The block now owns `cache-probe-hit` and `cache-probe-miss` outright, planting its own entry and reading it back in the next case. Each direction needs its own id: whichever entry is planted first is what every later case for that id reads back, so a single shared id lets the second direction pass on the exact leak it exists to catch. Measured on this tree with one id — deleting the hit observer left the miss case green under a suppressed reset, which is the original defect in miniature. Red-green, reset suppressed vs active, both in full-file order and under `-t 'resolver cache isolation'`: 2 fail / 0 fail in both orders, the same two cases each time. Deleting either observer still leaves the other failing, so no single-case cleanup blinds the guard. Co-authored-by: Matt Wilkinson <matt@rigel.build>
…RIG-3393) The previous commit claimed no single-case cleanup could blind the guard. That generalized from the two observer cases without measuring the other two. Both `memoizes ...` cases plant the entry their successor reads back, so deleting one blinded that direction, and deleting both left the reset deletable with a fully green suite — the original inertness, restored by an edit that looked like deduplication in either direction. They were tempting to delete because they asserted nothing the file did not already cover: each was a one-liner differing from an existing case only in the provider id, and neither failed when memoization was removed from `getProxyForProvider` outright. A case named for memoization that survives the cache's deletion is not defending it. Each planter now resolves twice across an env change, which is the assertion its name always implied: the hit planter deletes the variable between the two resolutions and still expects the proxy; the miss planter sets the variable and still expects undefined. Both fail against a cache-free `getProxyForProvider`, so pruning them now costs visible coverage. Measured on this tree. Reset active: 49 pass / 0 fail full-file, 4 pass / 0 fail under `-t 'resolver cache isolation'`. Reset suppressed: 2 fail in both orders, the same two observer cases, unchanged from before. Against a `getProxyForProvider` with the memoization read deleted: both planters fail where previously the whole file stayed green. Deleting any single case in the block leaves at least one failure under a suppressed reset. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Every sentence of the preceding comment is true per case, and together they still leave the wrong impression. Deleting one `memoizes ...` case blinds that direction loudly, but deleting both goes quiet: 47 pass / 0 fail with the reset suppressed, and the two observers survive in a vacuous state, still named for an entry nothing plants. That hazard cannot be closed inside a test file — removing every test for a behavior always removes its coverage silently. The tractable part was the motive, and the previous commit handled it: the planters no longer look like deletable duplicates. What was left was the over-inference, so the comment now says the quiet case out loud. Comment only. 49 pass / 0 fail, oxfmt/oxlint/tsgo clean. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 6, 2026 15:37
mattwilkinsonn
force-pushed
the
main
branch
from
September 19, 2026 16:13
6d0108c to
71c5eec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
proxy.test.tspassed alone and failed in full-suite runs. The cause is amemoized miss, not the env handling the suite already does carefully.
getProxyForProvidermemoizes into a module-levelMap, including a miss:So a provider resolved before its variable is set stays
undefinedfor therest of the module however the environment changes afterwards. The suite's
beforeEachsnapshots and clears every proxy env var but leaves the cacheintact, which makes each test's outcome depend on whether anything resolved
that same provider id earlier in the file.
The old comment reasoned that unique provider ids made cross-contamination
impossible. That holds for invented ids like
sakana, but the normalizationtest uses
github-copilot— a real provider id reachable from any other testin the file.
__resetProxyCache()already exists as a test seam and two other test filescall it. This file, which owns the cached function, did not.
Verification
Red → green on the arrangement that reproduces it (a prior lookup of the same
id, before the env var is set):
The failure is
Expected: "http://127.0.0.1:24560"/Received: undefined,matching what the full-suite runs showed.
__resetProxyCache()line reddens that arrangementagain (45/1), so the added line is the sole cause of the pass.
proxy.test.tsalone: 45 pass / 0 fail.openai-codex-zstd,openai-codex-stream):98 pass / 0 fail.
packages/aisuite, credential-free, CI shape(
--parallel=8 --timeout=30000): 4436 pass / 332 skip / 0 fail.oxfmt --checkrc=0;tsgo -p tsconfig.json --noEmitclean.Test-only change; no production behaviour is altered. The cache itself is
correct for its stated purpose (env values are static for the process
lifetime) — it is only tests that mutate the environment mid-process.
Spec-impact: none — test-only fix to a suite's cache teardown, no specified
behaviour changes.