Move non-compile CI jobs to GitHub-hosted runners - #322
Open
leynos wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Move non-compile CI jobs to GitHub-hosted runners
WalkthroughThe pull request defines compile-cost-based CI runner policy, adds workflow contract tests and documentation, makes test fixtures propagate errors, separates attachment tests, and excludes byte-sensitive fixtures from formatter discovery. ChangesCI runner policy and test reliability
Possibly related PRs
Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates five non-compile GitHub Actions workflows to run on GitHub-hosted ubuntu-latest runners instead of Ubicloud, without changing any job logic, triggers, or permissions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
scope: ci
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
August 5, 2026 20:54
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
ci/free-runners-for-glue-jobs
branch
from
August 5, 2026 21:59
0334f5f to
d1a42ec
Compare
github-actions
Bot
added
size: XL
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
Bot
added
scope: docs
The PR label classify and scope jobs, the regression-test check, the Claude Code review, and the weekly dependency audit all ran on `ubicloud-standard-8`. None of them compiles the workspace: they are single-threaded script or API-bound jobs (the audit installs `cargo-audit` as a prebuilt binary and only reads the lockfile), so an 8-vCPU paid runner is wasted on them. Axinite is a public repository, so `ubuntu-latest` runs these jobs at no cost. A July 2026 Ubicloud usage audit attributed roughly 1,600 billed premium-8 minutes per month to these five workflows alone. Compile-bound workflows (tests, staging CI, coverage, CodeScene coverage) stay on Ubicloud. Release-plz is left untouched because its jobs are gated to the `nearai` owner and never execute here.
leynos
force-pushed
the
ci/free-runners-for-glue-jobs
branch
from
August 5, 2026 22:01
d1a42ec to
db44a91
Compare
github-actions
Bot
added
size: S
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
added 3 commits
August 8, 2026 01:48
The previous commit moved five non-compile jobs to GitHub-hosted runners but left the split as an undocumented convention. The failure mode is silent: a new job copied from an existing workflow inherits whichever runner the template used, and nothing surfaces the mistake until the next billing audit. Add tests/workflow_contracts/runner_policy_test.py, which records the runner for every job in the repository. Adding a workflow or a job, or moving one between pools, now fails until RUNNER_POLICY is updated deliberately. The suite also asserts that no job on the free pool runs a compile command or installs a Rust build cache, and that each of the five migrated jobs kept its triggers, permissions, guards, and steps — so the runner move is provably behaviour-preserving. Reusable-workflow callers are matched by shape, not by SHA value, per the repository's Dependabot guidance. Document the policy in the developer's guide and record the decision, its rationale, and the alternatives considered in ADR 013.
The runner policy table records GitHub's own job identifiers and trigger values verbatim, so `build-local-artifacts`, `build-global-artifacts`, and the `labeled` pull-request trigger type cannot be respelled without breaking the assertions they anchor. Add narrow ignore patterns for those three literals to the typos overlay rather than excluding the whole file, so the test's prose stays under the Oxford spelling gate. Reword the one occurrence that was prose rather than an upstream identifier.
`whitaker-installer` 0.2.7 narrowed the test-detection heuristic behind `no_expect_outside_tests`: a function inside a `#[cfg(test)]` module is no longer treated as test code unless it is itself a recognised test. Attributes are gone by the time the lint sees HIR, so rstest fixtures, rstest-bdd steps and plain helper functions all read as production code. That surfaced 102 findings across 32 modules. Convert them the way the suite intends: arrangement is not a verdict, so a helper that can fail returns `Result` and propagates, and only the test body unwraps. No `#[allow]`, no `#[expect]`, and no helper renamed to look like a test. Three cases needed judgement rather than a mechanical rewrite: - The `tokio::spawn`ed test servers cannot propagate from inside the closure, so the fallible work moved out of the closure and the serve result now surfaces through the join handle. Every call site already aborted the handle and discarded the result, so no verdict changes. - Trait-impl methods with fixed signatures (`complete_with_tools`, `set_setting`) were already fallible, so a poisoned lock now maps to the trait's own error rather than panicking. A poisoned lock means the double panicked while holding it, which is worth reporting, not swallowing. - rstest-bdd's return classifier only recognises a bare `Result<..>`, not the `anyhow::Result` alias, so fallible steps spell the type out in full. Aliased steps would have discarded their errors silently. Splitting the pipeline and dispatch test doubles out of their parent modules keeps both within the 400-line module limit that the added error handling pushed them over. The CI pin stays at 0.2.6; this only removes the backlog that a future bump would otherwise surface all at once.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
Bot
added
scope: tool/builtin
`convert_test_pages_to_markdown` has been failing on the Yahoo page since d0efc5d adopted `mdformat-all` and normalised every tracked Markdown file. That pass reflowed `tests/test-pages/*/expected.md`, which are not prose: they are a byte-for-byte record of converter output, and the converter does not wrap. The test compares line by line, so a fixture wrapped to 80 columns no longer describes any real conversion. Restore the three fixtures to their pre-reformat content. The test passes against them unchanged, which confirms the converter itself never regressed — only the fixtures did. `.markdownlint-cli2.jsonc` already excludes these paths, with that exact rationale, but `mdformat-all` runs `mdtablefix --wrap` before `markdownlint-cli2` and `mdtablefix` has no exclusion flag. Since both tools discover files through `fd`, withhold the fixtures there instead. `.fdignore` rather than `.ignore`, so ripgrep still searches them.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr-013-split-ci-runners-by-compile-cost.md`:
- Line 5: Update the ADR headings and metadata by adding separate ## Status and
## Date sections, and rename ## Context to ## Context and Problem Statement.
Remove the combined Status/Date metadata line while preserving its information
under the corresponding sections.
In `@docs/developers-guide.md`:
- Around line 95-98: Update the runner-policy explanation in the developer guide
to remove the incorrect claim that release.yml pins ubuntu-22.04, and accurately
describe build-local-artifacts as using its matrix-selected runner. Clarify the
relevant job scope, distinguishing release.yml’s audit behavior from test.yml’s
separate audit job that remains on Ubicloud.
In `@src/agent/dispatcher/tests/auth.rs`:
- Around line 18-36: Remove the detected.is_some() assertion in
assert_auth_detected and let detected.context(...)? return the documented
missing-detection error. Also update the timeout helper in
src/agent/dispatcher/tests/loop_guard.rs at lines 191-215 by removing its
timeout assertion and allowing result.map_err(...)? to return the timeout error;
both sites require direct changes.
- Around line 54-55: Replace the affected helper-call .expect() sites with ? and
update each enclosing test or scenario function to return anyhow::Result<()>,
preserving normal success behavior. Apply this in
src/agent/dispatcher/tests/auth.rs lines 54-55 for assert_auth_detected;
src/agent/dispatcher/tests/image_sentinel.rs lines 245-247 for
run_image_generate_and_count_statuses; src/agent/dispatcher/tests/loop_guard.rs
lines 243-244 for assert_agentic_loop_text_response; both
assert_rendered_snapshot sites in
src/agent/dispatcher/tests/skill_bundle_context_bdd.rs lines 222-223 and
233-234; and every changed skill-construction .expect() site in
src/agent/dispatcher/tests/skills.rs, using proper error propagation throughout
src/agent/**/*.rs.
In `@src/agent/dispatcher/tests/pipeline/support.rs`:
- Around line 155-158: Update the custom-tool registration loop in the pipeline
test support helper to propagate failures from the awaited
deps.tools.register(tool) call with ?, while keeping builtin registration and
successful registrations unchanged.
In `@src/agent/scheduler/tests/approval.rs`:
- Around line 117-123: Update the result-matching helper around result to match
the Result directly without expect_err, assert_eq!, or panic!. Return Ok(())
only for an AuthRequired error whose name matches tool_name; convert unexpected
successes, other errors, and name mismatches into descriptive anyhow errors so
every unexpected outcome propagates through the Result<()> return type.
In `@src/bootstrap/tests/migration_support.rs`:
- Around line 202-214: Refactor RenameFixture::prepare to remove the three-way
match and apply setup through predicates such as should_write_legacy_file and
should_make_directory_read_only derived from RenameSetup. Keep prepare limited
to filesystem mutations, preserving legacy-file creation and Unix
read-only-directory behavior for the corresponding setups.
In `@src/channels/wasm/wrapper/tests/dispatch/attachments.rs`:
- Around line 150-181: Update
test_dispatch_emitted_messages_no_attachments_backward_compat to use the
existing dispatch_messages_for_test helper instead of constructing its own
channel, rate limiter, metadata, and DispatchContext setup. Preserve the test’s
assertions for successful dispatch, message content, and empty attachments while
keeping the shared helper as the sole dispatch harness.
In `@src/skills/registry/tests/install.rs`:
- Around line 34-66: Move the recursive installed-file traversal from the local
collect_installed_files implementation in
src/skills/registry/tests/install.rs:34-66 into shared test support, preserving
relative-path validation and file-read error context; replace that
implementation with an import of the shared collector. In
src/skills/registry/tests/prop_tests.rs:59-91, import the same shared collector
and retain only the TestCaseError conversion, removing its duplicate traversal
logic.
In `@src/worker/api/tests/client_methods.rs`:
- Around line 37-47: Update the server shutdown handling so expected
cancellation after abort is ignored, but all other JoinError values and inner
std::io::Error results are propagated from handle.await and server.await. Apply
this in src/worker/api/tests/client_methods.rs at lines 37-47 and 61-61, and
src/worker/container/tests/hosted_fidelity.rs at lines 31-32 and 97-122;
preserve the existing test-server behavior while removing any unconditional
result ignoring or unwrapping.
In `@src/worker/container/tests/shutdown.rs`:
- Around line 35-49: The shutdown cleanup around spawn_event_server must
gracefully await the server task instead of aborting it and ignoring its result.
Await the tokio::task::JoinHandle with ?, then apply ? again to propagate the
axum::serve std::io::Result error; update the enclosing test/helper flow as
needed to return Result and preserve existing cleanup behavior.
In `@tests/workflow_contracts/runner_policy_test.py`:
- Line 385: Add a descriptive assertion message to the `assert` validating
`checkout.get("uses", "")` in the workflow contract test, while preserving the
existing `startswith("actions/checkout@")` condition.
- Around line 392-394: Split the compound assertion in the workflow contract
test into separate assertions for “git diff” and “git log,” giving each
assertion a failure message that identifies its missing probe while preserving
the existing git-history validation.
- Around line 180-189: Update _workflow_files to include workflow filenames
ending in both .yml and .yaml, while preserving the sorted inventory returned to
test_workflow_inventory_matches_the_recorded_policy. Leave _load and other call
sites unchanged.
- Around line 320-321: Update the action-input handling around the prompt
assertion to assign the action’s “with” value, verify it is a mapping before
accessing it, and only then read its “prompt” field. Ensure an empty “with:”
produces a failed contract assertion rather than an AttributeError, while
preserving the existing prompt string assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fcfe878d-6fb5-4851-8f84-1bd000e24ea2
📒 Files selected for processing (59)
.fdignore.github/workflows/audit.yml.github/workflows/claude-review.yml.github/workflows/pr-label-classify.yml.github/workflows/pr-label-scope.yml.github/workflows/regression-test-check.ymlMakefiledocs/adr-013-split-ci-runners-by-compile-cost.mddocs/contents.mddocs/developers-guide.mdsrc/agent/dispatcher/tests/auth.rssrc/agent/dispatcher/tests/image_sentinel.rssrc/agent/dispatcher/tests/loop_guard.rssrc/agent/dispatcher/tests/pipeline.rssrc/agent/dispatcher/tests/pipeline/support.rssrc/agent/dispatcher/tests/skill_bundle_context_bdd.rssrc/agent/dispatcher/tests/skills.rssrc/agent/scheduler/tests/approval.rssrc/agent/thread_ops/document_store/tests/mod.rssrc/bootstrap/tests/env_format.rssrc/bootstrap/tests/migration.rssrc/bootstrap/tests/migration_disk_to_db.rssrc/bootstrap/tests/migration_rename.rssrc/bootstrap/tests/migration_support.rssrc/channels/wasm/wrapper/tests/channel/typing.rssrc/channels/wasm/wrapper/tests/dispatch.rssrc/channels/wasm/wrapper/tests/dispatch/attachments.rssrc/channels/web/handlers/skills/tests/helpers.rssrc/channels/web/handlers/skills/tests/json.rssrc/channels/web/handlers/skills/tests/multipart.rssrc/channels/web/server/tests/fixtures.rssrc/channels/web/server/tests/oauth.rssrc/channels/web/server/tests/relay_oauth.rssrc/history/migrations/tests.rssrc/history/migrations/tests/postgres_testing.rssrc/llm/rig_adapter/tests/unsupported_params.rssrc/orchestrator/api/tests/fixtures/remote_tool_helpers.rssrc/orchestrator/api/tests/remote_tools/execute.rssrc/skills/registry/tests/discovery.rssrc/skills/registry/tests/fixtures.rssrc/skills/registry/tests/install.rssrc/skills/registry/tests/install/lifecycle.rssrc/skills/registry/tests/install/payloads.rssrc/skills/registry/tests/prop_tests.rssrc/tools/builtin/skill_tools/tests/read_file_adapter.rssrc/tools/schema_validator/tests/fixture_groups.rssrc/worker/api/tests/client_methods.rssrc/worker/api/tests/transport_types.rssrc/worker/claude_bridge/tests/claude_fs_setup.rssrc/worker/container/tests/hosted_fidelity.rssrc/worker/container/tests/pre_loop.rssrc/worker/container/tests/remote_tools.rssrc/worker/container/tests/shutdown.rstests/test-pages/cnn/expected.mdtests/test-pages/medium/expected.mdtests/test-pages/yahoo/expected.mdtests/workflow_contracts/runner_policy_test.pytypos.local.tomltypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/ironclaw(auto-detected)leynos/memoryd(auto-detected)
|
|
||
| **Status:** Accepted **Date:** 2026-08-05 **Deciders:** `@leynos` | ||
|
|
||
| ## Context |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare second-level headings across all ADRs.
fd -t f 'adr-[0-9]{3}-.*\.md' docs --exec sh -c 'echo "== $1"; rg -n "^## " "$1"' _ {}Repository: leynos/axinite
Length of output: 4415
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- docs/adr-013-split-ci-runners-by-compile-cost.md ---'
sed -n '1,30p' docs/adr-013-split-ci-runners-by-compile-cost.md
printf '%s\n' '--- ADR filename and required-heading checks ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path("docs").glob("adr-[0-9][0-9][0-9]-*.md")):
text = path.read_text(encoding="utf-8")
headings = re.findall(r"^## (.+)$", text, re.MULTILINE)
required = {
"Status": bool(re.search(r"^## Status$", text, re.MULTILINE)),
"Date": bool(re.search(r"^## Date$", text, re.MULTILINE)),
"Context and Problem Statement": bool(
re.search(r"^## Context and Problem Statement$", text, re.MULTILINE)
),
"Context and problem statement": bool(
re.search(r"^## Context and problem statement$", text, re.MULTILINE)
),
}
print(f"{path}: {required}; first headings={headings[:4]}")
PYRepository: leynos/axinite
Length of output: 4969
Add the required ADR sections.
Use ## Status, ## Date, and ## Context and Problem Statement. The ADR currently combines Status and Date in one metadata line and uses ## Context.
Triage: [type:docstyle]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr-013-split-ci-runners-by-compile-cost.md` at line 5, Update the ADR
headings and metadata by adding separate ## Status and ## Date sections, and
rename ## Context to ## Context and Problem Statement. Remove the combined
Status/Date metadata line while preserving its information under the
corresponding sections.
Source: Coding guidelines
Comment on lines
+95
to
+98
| Windows jobs use `windows-latest` and `release.yml` pins `ubuntu-22.04`, both | ||
| for reproducibility rather than cost. `release-plz.yml` is left on Ubicloud | ||
| because its jobs are gated to the `nearai` repository owner and never execute | ||
| here. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the release.yml runner claim.
The text states that release.yml pins ubuntu-22.04. RUNNER_POLICY records build-local-artifacts as "${{ matrix.runner }}", so that job takes its runner from a matrix rather than a pin. The sentence contradicts the recorded policy it is meant to describe.
Also consider naming job scope for the audit. test.yml declares its own audit job that stays on Ubicloud, so a reader may infer that all audit work moved to the free pool.
♻️ Proposed fix
-Windows jobs use `windows-latest` and `release.yml` pins `ubuntu-22.04`, both
-for reproducibility rather than cost. `release-plz.yml` is left on Ubicloud
-because its jobs are gated to the `nearai` repository owner and never execute
-here.
+Windows jobs use `windows-latest`. `release.yml` pins `ubuntu-22.04` for every
+job except `build-local-artifacts`, which selects its runner from the release
+matrix. Both choices serve reproducibility rather than cost. `release-plz.yml`
+is left on Ubicloud because its jobs are gated to the `nearai` repository owner
+and never execute here.Triage: [type:docstyle]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Windows jobs use `windows-latest` and `release.yml` pins `ubuntu-22.04`, both | |
| for reproducibility rather than cost. `release-plz.yml` is left on Ubicloud | |
| because its jobs are gated to the `nearai` repository owner and never execute | |
| here. | |
| Windows jobs use `windows-latest`. `release.yml` pins `ubuntu-22.04` for every | |
| job except `build-local-artifacts`, which selects its runner from the release | |
| matrix. Both choices serve reproducibility rather than cost. `release-plz.yml` | |
| is left on Ubicloud because its jobs are gated to the `nearai` repository owner | |
| and never execute here. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/developers-guide.md` around lines 95 - 98, Update the runner-policy
explanation in the developer guide to remove the incorrect claim that
release.yml pins ubuntu-22.04, and accurately describe build-local-artifacts as
using its matrix-selected runner. Clarify the relevant job scope, distinguishing
release.yml’s audit behavior from test.yml’s separate audit job that remains on
Ubicloud.
Comment on lines
+18
to
+36
| /// | ||
| /// Returns an error when detection did not fire, leaving the calling test to | ||
| /// decide how that failure surfaces. | ||
| fn assert_auth_detected( | ||
| detected: Option<(String, String)>, | ||
| expected_name: &str, | ||
| expected_instructions_fragment: &str, | ||
| ) { | ||
| ) -> anyhow::Result<()> { | ||
| assert!(detected.is_some(), "expected auth detection to fire"); | ||
| let (name, instructions) = | ||
| detected.expect("expected auth detection to fire and return (name, instructions)"); | ||
| detected.context("expected auth detection to fire and return (name, instructions)")?; | ||
| assert_eq!(name, expected_name); | ||
| assert!( | ||
| instructions.contains(expected_instructions_fragment), | ||
| "instructions did not contain {:?}: {:?}", | ||
| expected_instructions_fragment, | ||
| instructions, | ||
| ); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return precondition failures through the helper result.
Remove the assertion that precedes error conversion in each helper. Each assertion panics before the later Context or map_err call can return the documented error.
src/agent/dispatcher/tests/auth.rs#L18-L36: remove thedetected.is_some()assertion and letdetected.context(...)?return the missing-detection error.src/agent/dispatcher/tests/loop_guard.rs#L191-L215: remove the timeout assertion and letresult.map_err(...)?return the timeout error.
📍 Affects 2 files
src/agent/dispatcher/tests/auth.rs#L18-L36(this comment)src/agent/dispatcher/tests/loop_guard.rs#L191-L215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agent/dispatcher/tests/auth.rs` around lines 18 - 36, Remove the
detected.is_some() assertion in assert_auth_detected and let
detected.context(...)? return the documented missing-detection error. Also
update the timeout helper in src/agent/dispatcher/tests/loop_guard.rs at lines
191-215 by removing its timeout assertion and allowing result.map_err(...)? to
return the timeout error; both sites require direct changes.
Comment on lines
+54
to
+55
| ) | ||
| .expect("expected auth detection to fire"); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Propagate helper errors from all affected agent tests.
Replace the newly added .expect() calls with ?. Return anyhow::Result<()> from the affected test or scenario functions.
src/agent/dispatcher/tests/auth.rs#L54-L55: propagateassert_auth_detected.src/agent/dispatcher/tests/image_sentinel.rs#L245-L247: propagaterun_image_generate_and_count_statuses.src/agent/dispatcher/tests/loop_guard.rs#L243-L244: propagateassert_agentic_loop_text_response.src/agent/dispatcher/tests/skill_bundle_context_bdd.rs#L222-L223: propagateassert_rendered_snapshot; apply the same change at Lines 233-234.src/agent/dispatcher/tests/skills.rs#L92-L93: propagate skill-construction failures; apply the same change to the other changed.expect()sites in this file.
As per path instructions, src/agent/**/*.rs requires: “Never call .unwrap() or .expect(); use ? with proper error mapping.”
📍 Affects 5 files
src/agent/dispatcher/tests/auth.rs#L54-L55(this comment)src/agent/dispatcher/tests/image_sentinel.rs#L245-L247src/agent/dispatcher/tests/loop_guard.rs#L243-L244src/agent/dispatcher/tests/skill_bundle_context_bdd.rs#L222-L223src/agent/dispatcher/tests/skills.rs#L92-L93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agent/dispatcher/tests/auth.rs` around lines 54 - 55, Replace the
affected helper-call .expect() sites with ? and update each enclosing test or
scenario function to return anyhow::Result<()>, preserving normal success
behavior. Apply this in src/agent/dispatcher/tests/auth.rs lines 54-55 for
assert_auth_detected; src/agent/dispatcher/tests/image_sentinel.rs lines 245-247
for run_image_generate_and_count_statuses;
src/agent/dispatcher/tests/loop_guard.rs lines 243-244 for
assert_agentic_loop_text_response; both assert_rendered_snapshot sites in
src/agent/dispatcher/tests/skill_bundle_context_bdd.rs lines 222-223 and
233-234; and every changed skill-construction .expect() site in
src/agent/dispatcher/tests/skills.rs, using proper error propagation throughout
src/agent/**/*.rs.
Source: Path instructions
Comment on lines
+155
to
+158
| deps.tools.register_builtin_tools()?; | ||
| for tool in tools { | ||
| let _ = deps.tools.register(tool).await; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate custom-tool registration failures.
Replace the discarded result on Line 157 with ?. A rejected custom tool leaves the helper with a different tool configuration, so a pipeline test can exercise the wrong setup and still pass.
Proposed fix
for tool in tools {
- let _ = deps.tools.register(tool).await;
+ deps.tools.register(tool).await?;
}As per coding guidelines, shared fixtures must propagate errors with Result and ?.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deps.tools.register_builtin_tools()?; | |
| for tool in tools { | |
| let _ = deps.tools.register(tool).await; | |
| } | |
| deps.tools.register_builtin_tools()?; | |
| for tool in tools { | |
| deps.tools.register(tool).await?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agent/dispatcher/tests/pipeline/support.rs` around lines 155 - 158,
Update the custom-tool registration loop in the pipeline test support helper to
propagate failures from the awaited deps.tools.register(tool) call with ?, while
keeping builtin registration and successful registrations unchanged.
Source: Coding guidelines
Comment on lines
+35
to
49
| /// Spawns an ephemeral Axum server that records posted job events. | ||
| /// | ||
| /// Binding is fallible, so the base URL is returned as a `Result`. The spawned | ||
| /// task cannot propagate with `?`, so it yields the serve outcome through its | ||
| /// join handle instead of unwrapping. | ||
| async fn spawn_event_server( | ||
| state: Arc<EventState>, | ||
| ) -> Result<(String, tokio::task::JoinHandle<()>)> { | ||
| ) -> Result<(String, tokio::task::JoinHandle<std::io::Result<()>>)> { | ||
| let listener = TcpListener::bind("127.0.0.1:0").await?; | ||
| let addr = listener.local_addr()?; | ||
| let app = Router::new() | ||
| .route(EVENT_ROUTE, post(event_handler)) | ||
| .with_state(state); | ||
| let handle = tokio::spawn(async move { | ||
| axum::serve(listener, app) | ||
| .await | ||
| .expect("event test server should run"); | ||
| }); | ||
| let handle = tokio::spawn(async move { axum::serve(listener, app).await }); | ||
| Ok((format!("http://{addr}"), handle)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate the server task result before cleanup.
Replace task abortion and the ignored join result with graceful shutdown. Await the
join handle and propagate both the Tokio join error and the axum::serve error.
The current cleanup discards the std::io::Result<()>, so an event-server
failure still becomes a timeout or an unrelated test failure.
As per coding guidelines, shared test support must propagate errors with
Result and ?.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/worker/container/tests/shutdown.rs` around lines 35 - 49, The shutdown
cleanup around spawn_event_server must gracefully await the server task instead
of aborting it and ignoring its result. Await the tokio::task::JoinHandle with
?, then apply ? again to propagate the axum::serve std::io::Result error; update
the enclosing test/helper flow as needed to return Result and preserve existing
cleanup behavior.
Source: Coding guidelines
Comment on lines
+180
to
+189
| def _workflow_files() -> list[str]: | ||
| """Return every workflow file name, sorted.""" | ||
| return sorted(path.name for path in WORKFLOW_DIR.glob("*.yml")) | ||
|
|
||
|
|
||
| def test_workflow_inventory_matches_the_recorded_policy() -> None: | ||
| """Every workflow file is accounted for in the runner policy.""" | ||
| assert _workflow_files() == sorted(RUNNER_POLICY), ( | ||
| "a workflow was added or removed without updating RUNNER_POLICY" | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include .yaml workflows in the inventory glob.
_workflow_files globs *.yml only. GitHub Actions also loads .github/workflows/*.yaml. A job added in a .yaml file escapes RUNNER_POLICY entirely, and test_workflow_inventory_matches_the_recorded_policy still passes. That is exactly the silent drift this suite exists to catch.
🛡️ Proposed fix to cover both extensions
def _workflow_files() -> list[str]:
"""Return every workflow file name, sorted."""
- return sorted(path.name for path in WORKFLOW_DIR.glob("*.yml"))
+ return sorted(
+ path.name
+ for path in WORKFLOW_DIR.iterdir()
+ if path.suffix in {".yml", ".yaml"}
+ )Note that _load already takes a bare file name, so no other call site changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _workflow_files() -> list[str]: | |
| """Return every workflow file name, sorted.""" | |
| return sorted(path.name for path in WORKFLOW_DIR.glob("*.yml")) | |
| def test_workflow_inventory_matches_the_recorded_policy() -> None: | |
| """Every workflow file is accounted for in the runner policy.""" | |
| assert _workflow_files() == sorted(RUNNER_POLICY), ( | |
| "a workflow was added or removed without updating RUNNER_POLICY" | |
| ) | |
| def _workflow_files() -> list[str]: | |
| """Return every workflow file name, sorted.""" | |
| return sorted( | |
| path.name | |
| for path in WORKFLOW_DIR.iterdir() | |
| if path.suffix in {".yml", ".yaml"} | |
| ) | |
| def test_workflow_inventory_matches_the_recorded_policy() -> None: | |
| """Every workflow file is accounted for in the runner policy.""" | |
| assert _workflow_files() == sorted(RUNNER_POLICY), ( | |
| "a workflow was added or removed without updating RUNNER_POLICY" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflow_contracts/runner_policy_test.py` around lines 180 - 189,
Update _workflow_files to include workflow filenames ending in both .yml and
.yaml, while preserving the sorted inventory returned to
test_workflow_inventory_matches_the_recorded_policy. Leave _load and other call
sites unchanged.
Comment on lines
+320
to
+321
| prompt = action.get("with", {}).get("prompt") | ||
| assert isinstance(prompt, str), "the review action must supply a prompt" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the typecheck target and any Python type-checker configuration.
rg -n -A5 '^typecheck:' Makefile
fd -H -t f 'pyproject.toml|mypy.ini|setup.cfg|pyrightconfig.json' --exec rg -n -C4 'mypy|pyright|ty\b|include|exclude|files' {}Repository: leynos/axinite
Length of output: 507
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- target file ---'
sed -n '270,335p' tests/workflow_contracts/runner_policy_test.py
printf '%s\n' '--- related definitions and uses ---'
rg -n -C4 'def _job|_job\(|action\.get\("with"|runner_policy' tests/workflow_contracts
printf '%s\n' '--- Python type-checker configuration and test tooling ---'
fd -H -t f 'pyproject.toml|mypy.ini|setup.cfg|tox.ini|noxfile.py|pyrightconfig.json|Makefile|justfile' \
--exec sh -c 'echo "--- $1"; rg -n -C4 "mypy|pyright|basedpyright|ty\\b|typecheck|pytest|tests/" "$1"' sh {}Repository: leynos/axinite
Length of output: 21780
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import ast
path = Path("tests/workflow_contracts/runner_policy_test.py")
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == "_job":
print(f"_job: lines {node.lineno}-{node.end_lineno}")
print(ast.unparse(node))
PYRepository: leynos/axinite
Length of output: 380
🏁 Script executed (no clone):
python3 - <<'PY'
# Model the relevant YAML-shaped values without importing or running repository code.
def evaluate(action):
try:
prompt = action.get("with", {}).get("prompt")
return ("value", prompt)
except Exception as exc:
return (type(exc).__name__, str(exc))
cases = [
{"name": "missing with", "value": {}},
{"name": "empty with", "value": {"with": None}},
{"name": "mapping with prompt", "value": {"with": {"prompt": "x"}}},
{"name": "scalar with", "value": {"with": "x"}},
]
for case in cases:
print(case["name"], evaluate(case["value"]))
PY
Length of output: 333
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- parser helpers and step typing ---'
sed -n '1,175p' tests/workflow_contracts/runner_policy_test.py
printf '%s\n' '--- Claude workflow action ---'
fd -H -t f 'claude-review.yml' --exec sh -c 'echo "--- $1"; cat -n "$1"' sh
printf '%s\n' '--- all relevant with blocks ---'
rg -n -C3 '^[[:space:]]+with:|claude-code-action|prompt:' .github tests/workflow_contractsRepository: leynos/axinite
Length of output: 50370
🏁 Script executed (no clone):
python3 - <<'PY'
try:
import yaml
except ImportError:
print("PyYAML is unavailable")
else:
samples = [
("missing with", "steps:\\n - uses: example/action"),
("empty with", "steps:\\n - uses: example/action\\n with:"),
("empty mapping with", "steps:\\n - uses: example/action\\n with: {}"),
("prompt mapping", "steps:\\n - uses: example/action\\n with:\\n prompt: hello"),
]
for name, text in samples:
value = yaml.safe_load(text)["steps"][0]
with_value = value.get("with", {})
print(name, repr(with_value), type(with_value).__name__)
PY
Length of output: 147
Guard the action inputs before reading prompt.
If with: is empty, PyYAML loads it as None. The chained .get then raises AttributeError instead of reporting a failed contract assertion. Assert that inputs is a mapping before reading prompt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflow_contracts/runner_policy_test.py` around lines 320 - 321,
Update the action-input handling around the prompt assertion to assign the
action’s “with” value, verify it is a mapping before accessing it, and only then
read its “prompt” field. Ensure an empty “with:” produces a failed contract
assertion rather than an AttributeError, while preserving the existing prompt
string assertion.
| ], "the regression check must stay a checkout plus a single script step" | ||
|
|
||
| checkout = _steps(job)[0] | ||
| assert str(checkout.get("uses", "")).startswith("actions/checkout@") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an assertion message.
This is the only bare assert in the file. The repository guidelines require a message on every assertion.
♻️ Proposed fix
- assert str(checkout.get("uses", "")).startswith("actions/checkout@")
+ assert str(checkout.get("uses", "")).startswith("actions/checkout@"), (
+ "the regression check must check out the repository with actions/checkout"
+ )As per coding guidelines: "Use assert …, "message" over bare asserts".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert str(checkout.get("uses", "")).startswith("actions/checkout@") | |
| assert str(checkout.get("uses", "")).startswith("actions/checkout@"), ( | |
| "the regression check must check out the repository with actions/checkout" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflow_contracts/runner_policy_test.py` at line 385, Add a
descriptive assertion message to the `assert` validating `checkout.get("uses",
"")` in the workflow contract test, while preserving the existing
`startswith("actions/checkout@")` condition.
Source: Path instructions
Comment on lines
+392
to
+394
| assert "git diff" in script and "git log" in script, ( | ||
| "the check must remain a git-history inspection, not a build" | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Split the compound assertion.
Ruff flags PT018. A single failure message cannot tell which of the two probes is missing. Split it so the report names the missing command.
♻️ Proposed fix
- assert "git diff" in script and "git log" in script, (
- "the check must remain a git-history inspection, not a build"
- )
+ assert "git diff" in script, (
+ "the check must remain a git-history inspection, not a build"
+ )
+ assert "git log" in script, (
+ "the check must inspect commit messages, not a build"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert "git diff" in script and "git log" in script, ( | |
| "the check must remain a git-history inspection, not a build" | |
| ) | |
| assert "git diff" in script, ( | |
| "the check must remain a git-history inspection, not a build" | |
| ) | |
| assert "git log" in script, ( | |
| "the check must inspect commit messages, not a build" | |
| ) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 392-394: Assertion should be broken down into multiple parts
(PT018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflow_contracts/runner_policy_test.py` around lines 392 - 394, Split
the compound assertion in the workflow contract test into separate assertions
for “git diff” and “git log,” giving each assertion a failure message that
identifies its missing probe while preserving the existing git-history
validation.
Source: Linters/SAST tools
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
2 participants
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.
Pull Request
Summary
ubicloud-standard-8to GitHub-hostedubuntu-latest: the two pull-request labelling workflows, theregression-test check, the Claude Code review, and the weekly dependency
audit. None of them compiles the workspace — they are single-threaded shell
scripts, an action call, or an API-bound agent run — and Axinite is a public
repository, so hosted runners execute them at no cost.
minutes per month to these five workflows.
tests/workflow_contracts/runner_policy_test.pyrecords the runner forevery job in the repository, so adding a job or moving one between pools
fails until
RUNNER_POLICYis updated deliberately. The suite also assertsthat no free-runner job runs a compile command or installs a Rust build
cache, and that the five migrated jobs kept their triggers, permissions,
guards, and steps.
docs/developers-guide.md§5 and record the decisionin ADR 013.
test.yml,code_style.yml,coverage.yml,codescene-coverage.yml,e2e.yml,staging-ci.yml) stay on Ubicloud.release-plz.ymlis deliberately untouched: its jobs are gated to thenearairepository owner and never execute here.Two further commits fix pre-existing gate failures that this branch's
commit gates surfaced. Neither originates here — the branch changes no
.rsfiles — so they are self-contained and can be split out on request:f872cb09clears 102no_expect_outside_testsfindings. Localwhitaker-installeris 0.2.7 while CI pins 0.2.6, and 0.2.7 no longertreats
#[cfg(test)]helper functions as test code. Test arrangementhelpers now return
Resultand only test bodies unwrap. The CI pin isunchanged; this removes the backlog a future bump would surface at once.
1bf74b4efixeshtml_to_markdown::convert_test_pages_to_markdown,failing since
d0efc5d1reflowed the converter's golden fixtures to 80columns. The fixtures record converter output, which does not wrap.
This PR is stacked on #323
(cargo-audit fixes), which should merge first; the base branch is
fix/rustsec-wasmtime-rkyv.Change Type
Linked Issue
None.
Review walkthrough
tests/workflow_contracts/runner_policy_test.py— theRUNNER_POLICYtable is the whole change in one place.pr-label-classify.yml,
pr-label-scope.yml, and
regression-test-check.yml —
labelling scripts and a
git diff/grepcheck, no build step.action prompt already instructs the agent not to build or test, and the
contract test now asserts that instruction stays.
cargo-auditas aprebuilt binary and reads the lockfile only.
docs/adr-013-split-ci-runners-by-compile-cost.mdfor therationale and the alternatives considered.
Validation
make check-fmtmake lintmake typecheckmake test-workflow-contracts— 60 passed,including the new
runner_policy_test.pysuite.make test— 4244 tests run, 4244 passed, 8 skipped.make markdownlint,make spelling,make nixie— clean.actionlinton the five changed workflows is clean. Therunner-policy table was verified to fail closed by temporarily editing a
recorded runner and confirming the drift assertion fires.
Security Impact
None. The change is
runs-ononly; no permissions, secrets, triggers, orstep logic were altered, and the contract tests now assert each migrated
workflow's
permissionsblock and trigger set explicitly.Worth noting for reviewers: the two labelling workflows use
pull_request_targetand therefore run with repository write scope. Theirtrigger, permissions, checkout ref, and step list are unchanged and are now
pinned by tests — moving to a GitHub-hosted runner does not widen that
surface, and the jobs continue to check out the base branch rather than
untrusted head code.
Database Impact
None. No migrations, schema changes, or store code are touched.
Blast Radius
CI and test code only; no runtime, library, or binary code changes. The
Whitaker commit touches 43 test modules but changes no production code and
no assertion outcomes.
concurrency instead of Ubicloud's. If hosted capacity is congested, these
checks queue longer. They run on every pull-request event, so a regression
is visible on the next PR rather than at a distance.
jobs not on the pull-request critical path.
make test-workflow-contractsuntilRUNNER_POLICYis updated. That is intended friction, but it is a new wayfor an unrelated CI PR to go red.
Rollback Plan
Revert the merge commit. The change is five one-line
runs-onedits plusadditive tests, docs, and test-only refactoring, so a straight
git revertrestores the previous behaviour with no data, schema, or deployment state to
unwind. The four commits are independent and revert cleanly on their own.
To roll back a single workflow instead, set its
runs-onback toubicloud-standard-8andupdate the matching entry in
RUNNER_POLICYandFREE_RUNNER_JOBS.Review track: C (CI)
References
https://lody.ai/leynos/sessions/79f76904-33bf-4cdf-bff4-99ae02f88408