Repository navigation
Declare the bench once and let everything read it - #388
Conversation
Gate-tool versions lived in five places: the tools composite spelled every version three times over, the agent lanes and security.yml carried their own pins, and the ratchet hooks inlined the similarity version. tools.toml at the root is now the one authority. The composite parses it and keys its cache on the file's hash, the lanes and security.yml install through the composite, the ratchet hooks read their pin out of it at run time, and a documentation contract sweeps every workflow for a declared tool spelled at another version. The same contract pins the gh-aw dispatcher's setup action to a sha, which caught the compiler regenerating the tag within the hour, and the lane prompts now name the whole bench. Bumping a tool is one edit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe change centralizes repository tool versions in workspace metadata. A shared script and composite action install those tools. CI, agent workflows, hooks, documentation, and contract tests now consume or validate the centralized configuration. ChangesTool governance and CI installation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ToolsAction
participant ToolsScript
participant WorkspaceMetadata
Workflow->>ToolsAction: invoke tools action
ToolsAction->>ToolsScript: request tool lists and cache key
ToolsScript->>WorkspaceMetadata: read declared versions
ToolsScript-->>ToolsAction: return parsed metadata
ToolsAction-->>Workflow: install tools and update PATH
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/tools/action.yml:
- Around line 16-20: Change the cache step identified by id “cache” to use
actions/cache/restore so agent lanes only restore existing ~/.cargo-tools
entries. Add a separate actions/cache/save step after the successful tools
installation, guarded by github.ref == 'refs/heads/main', while preserving the
existing cache path and key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ee719ec-23e7-4145-a8f7-71186e705ce7
📒 Files selected for processing (13)
.github/actions/tools/action.yml.github/workflows/agent-mention.lock.yml.github/workflows/agent-mention.md.github/workflows/agent-review.lock.yml.github/workflows/agent-review.md.github/workflows/agent-triage.lock.yml.github/workflows/agent-triage.md.github/workflows/security.yml.pre-commit-config.yamlcrates/amiss/tests/documentation_contracts/main.rscrates/amiss/tests/documentation_contracts/tools.rsdocs/src/development.mdtools.toml
The maintainer's call: the tool tables live under workspace.metadata.tools in the root Cargo.toml rather than a separate tools.toml. The composite extracts the two tables and keys its cache on the hash of the extracted lists, so the key still moves exactly when a tool does and dependency bumps cannot churn it. The ratchet hooks, the contract test, the lane prompts, and the book follow the manifest.
The seds were three hand parsers for one TOML table and the review was right to call them unreadable. scripts/tools.sh now serves the tables out of cargo metadata through jq, the composite and both ratchet hooks call that one reader, and the contract test parses the manifest independently through the toml crate, with a teeth test making the two parsers agree on every tool. The manifest comment shrank to a line and the composite keeps only the constraint notes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
The single-source refactor itself reads well, but the lane prompt now promises a full pinned bench that never reaches the agent container.
The lane claims the full pinned bench is installed, but this lane runs with none of it.
The composite installs the bench on the host runner under $HOME/.cargo-tools and appends that dir to the host job's PATH (action.yml#L41), while the agent runs inside the awf firewall container, which mounts the workspace and tool cache but not the host ~/.cargo-tools; the container's own /home/runner/.cargo-tools does not exist. Empirically, in this lane (the agent run on the PR head, which uses the new composite): none of prek, typos, cargo-nextest, cargo-deny, cargo-shear, cargo-llvm-cov, cargo-mutants, cargo-fuzz, similarity-rs, or zizmor is on PATH or on disk, and cargo nextest run --workspace --locked fails with error: no such command: nextest. The same claim sits in agent-mention.md (#L61). Every cold lane run also spends the composite's full bench install on the host, where the agent cannot see it.
Confidence: High.
Nit: "Bumping a tool is one edit" is not true for cargo-nextest, whose pin is still hardcoded in the platform-tests job.
The new paragraph promises single-edit bumps, but the platform-tests job keeps tool: cargo-nextest@0.9.140 (ci.yml#L173), and the new sweep test workflow_tool_pins_match_the_bench (tools.rs#L68) forces that pin to equal the manifest, so a nextest bump edits both files together. The doc should name the exception, or the platform-tests install should move onto the composite.
Confidence: High.
Session details
Run: 31631839210
Reviewed head fb13636: ran scripts/tools.sh prebuilt/source/key/version against the head tree, traced the sweep logic in crates/amiss/tests/documentation_contracts/tools.rs against every workflow, and probed the agent lane environment directly (command -v over the ten bench tools, cargo nextest run --workspace --locked).
The review round proved the prompt lied: the composite installed onto the host under HOME and exported the host PATH, while the agent runs inside the awf container, which sees neither, so the lane held none of the bench it named. The bench now lives under the runner tool cache, the one directory the sandbox assembles onto its own PATH, and host jobs reach it through the same GITHUB_PATH line. The reader captures cargo metadata before piping, so a metadata failure kills the run instead of caching an empty bench under a fixed key, jq -e refuses null tables, and the digest falls back to shasum where sha256sum is absent. Platform-tests reads its nextest pin through the reader, which makes the one-edit sentence true, and the sweep learns that an expression version is a runtime read.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/tools.sh">
<violation number="1" location="scripts/tools.sh:21">
P2: Adding `-e` to this jq call breaks the unknown-tool guard. When a name isn't in the bench, `.metadata.tools | .prebuilt + .source | .[$name] // empty` produces no output and jq `-e` exits 4; under `set -e` the `result=$(...)` assignment aborts the script with rc 4 before the `[ -n "$result" ]` check runs, so the friendly `unknown tool: $2` message never prints and the error is silent. Drop `-e` (back to `-r`) so jq returns 0 on empty and the existing guard handles it.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Warning
One defect: the version reader's unknown-tool guard is dead code and fails silently.
- scripts/tools.sh
Problem: jq -e in the version mode defeats the unknown-tool guard, so an unrecognized tool name aborts the script with a silent exit 4 instead of the intended diagnostic.
Explanation: under set -eu, the result=$(...) substitution propagates jq's exit 4 on an empty last value, so line 23's [ -n "$result" ] check and its unknown tool: $2 message are unreachable; the fail-closed -e is only right on the prebuilt/source/key modes. Verified against the head tree.
Confidence: High.
Evidence:
Line 21 in 16c70fc
Both reviewers reproduced it: jq -e exits 4 on the empty result the version arm uses to detect an unknown name, and set -e killed the script before the diagnostic printed. The version arm goes back to -r so the guard answers, while the three table modes keep -e where a null table must abort.
There was a problem hiding this comment.
Warning
One defect: the bench composite masks reader failures, so the run does not fail when scripts/tools.sh does.
The composite bench step swallows reader failures
Problem: The three printf lines wrap each scripts/tools.sh call in a command substitution, so a failing reader still leaves the step green with empty prebuilt/source/key outputs and a fixed degenerate cache key.
The step runs under the Actions default bash --noprofile --norc -eo pipefail. A failing command substitution does not fail the enclosing command: printf 'nextest=%s\n' "$(false)" exits 0 with nextest= written to $GITHUB_OUTPUT (reproduced). So a cargo metadata failure or missing jq makes tools.sh exit nonzero while the step succeeds, the cache key collapses to tools-ubuntu-, and install-action either errors with a misleading message or is skipped on a cache hit of the degenerate key — the "empty bench under a fixed key" that commit 16c70fc claims to prevent. Capturing each value in an assignment first (prebuilt="$(scripts/tools.sh prebuilt)") fails under -e when the substitution fails (verified rc 1), which is the fix. See
amiss/.github/actions/tools/action.yml
Lines 11 to 13 in 594063a
Confidence: high.
Session details
- run
- Ran:
cargo nextest run -p amiss --test documentation_contracts tools --locked(6/6), full documentation_contracts suite (31/31),scripts/tools.shsubcommand probes, and bash reproductions of the Actions default shell behavior.
| printf 'prebuilt=%s\n' "$(scripts/tools.sh prebuilt)" | ||
| printf 'source=%s\n' "$(scripts/tools.sh source)" | ||
| printf 'key=%s\n' "$(scripts/tools.sh key)" |
There was a problem hiding this comment.
The three printf lines swallow a scripts/tools.sh failure, so a reader failure does not kill the run — it leaves the step green with empty outputs and a fixed degenerate cache key.
The step's shell is the Actions default bash --noprofile --norc -eo pipefail. A failing command substitution does not fail the enclosing command: I reproduced printf 'nextest=%s\n' "$(false)" under that exact shell and it exits 0 with nextest= in $GITHUB_OUTPUT. So when cargo metadata fails or jq is absent, tools.sh exits nonzero but the step still succeeds, emitting prebuilt=, source=, key=, and the cache key collapses to the fixed tools-ubuntu- string — the "empty bench under a fixed key" that commit 16c70fc says a metadata failure kills the run to avoid. The reader's diagnostic survives only as a stderr line inside a green step; downstream, install-action either errors with a misleading message or, on a cache hit of the degenerate key, is skipped entirely.
Fix: capture each value in an assignment before printing. Under -e, prebuilt="$(scripts/tools.sh prebuilt)" exits nonzero when the substitution fails (verified: bash -e -c 'x="$(false)"' → rc 1), so the step fails at the reader. The same pattern repeats at
amiss/.github/workflows/ci.yml
Line 173 in 594063a
Confidence: high.
Gate-tool versions lived in five places: the tools composite spelled every version three times over (cache key, prebuilt list, source list), the agent lanes and security.yml carried their own pins, and the ratchet hooks inlined the similarity version.
The workspace.metadata.tools tables in the root Cargo.toml are now the one authority, and cargo is the only thing that parses them. scripts/tools.sh serves the tables out of cargo metadata through jq (prebuilt list, source list, cache key, single version), the composite and both ratchet hooks call that one reader, and the cache key is a hash of the tables so it moves exactly when a tool does and dependency bumps cannot churn it. The three agent lanes and security.yml install through the composite, which gives the lanes the whole bench #383 asked for, with the lane prompts naming it. A documentation contract parses the manifest independently through the toml crate and holds a teeth test where the shell reader and that parser must agree on every tool; it also sweeps every workflow for a declared tool at another version, keeps the composite version-free, keeps the lanes on the composite, checks the prek floor, and pins the gh-aw dispatcher setup action to a sha, which caught the compiler regenerating the tag within the hour.
Platform-tests keeps its lone nextest install since compiling similarity-rs and cargo-fuzz on macOS and Windows buys nothing, and the sweep holds that pin to the declared version. Lane caches stay restore-only and hit the entries main CI saves under the same key. Bumping a tool is one edit in the manifest.
Summary by CodeRabbit
CI & Tooling
Documentation
Tests