Stage cargo-binstall archives and simplify the binstall metadata - #534
Conversation
|
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:
Summary
Publishing WalkthroughThe change packages cargo-binstall releases as staged ChangesRelease packaging and installation contracts
Test-support and snapshot testing
Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant HoistScript
participant Distribution
participant AssetUpload
ReleaseWorkflow->>HoistScript: pass resolved release version
HoistScript->>Distribution: validate and move archive/checksum pairs
ReleaseWorkflow->>AssetUpload: upload assets from dist/
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideSwitches cargo-binstall support from hand-maintained per-target binary assets to staged tar.gz archives produced by the shared stage-release-artefacts action, hoisted to the release root and described by a single pkg-url template, with tests updated to enforce the new contract and shared GitHub Actions pins bumped. Sequence diagram for cargo-binstall using staged archivessequenceDiagram
participant User
participant cargo_binstall
participant GitHub_Releases
User->>cargo_binstall: run netsuke-build install
cargo_binstall->>GitHub_Releases: GET {repo}/releases/download/v{version}/{name}-{version}-{target}.tar.gz
GitHub_Releases-->>cargo_binstall: {name}-{version}-{target}.tar.gz
cargo_binstall->>cargo_binstall: extract tgz
cargo_binstall->>User: install {bin}{binary-ext}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
|
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address the review round on #534: - Replace the inline hoist step with `scripts/hoist_binstall_archives.py`: the expected archive set is derived from `.github/release-staging.toml` and `Cargo.toml`, every target's archive and `.sha256` sidecar is validated before anything moves, expected and found names are logged, and a shortfall fails the release listing the missing assets instead of uploading a partial set. Behavioural coverage lives in `tests/workflow_contracts/hoist_binstall_archives_test.py` (happy path, missing target, missing sidecar, empty dist, duplicate candidates, wiring), and `tests/binstall_metadata_tests.rs` now pins the workflow to the script rather than a step name. - Move the diagnostic-JSON snapshot settings into `snapshot_test_support::diagnostic_json_snapshot_settings()`, extending the module's existing shared helper so the documented policy holds for any module; tighten the redaction anchor to the enclosing `"generator"` object and prove non-generator `version` fields survive via an inline-snapshot test. - Cover `try_is_file`'s absent-path branch, add an exhaustive presence-combination test pinning the locator's first-match and error invariants, and surface candidate-probe filesystem errors in a dedicated test. - Drop the `example.id.clone()` in the registry-install contract by recording matches per example. - Refresh the developers' guide binstall bullet for the archive scheme and point the insta guide at the shared helper. The suggestion to make the `documentation_examples` module declaration private is deliberately not applied: with a private `mod`, the shared module's helpers used only by sibling test binaries fail the `-D warnings` gate as dead code; `pub mod` follows the existing loader-tests precedent. The ADR-007 reference is also unactioned: its binstall passage describes the package-name decision, which remains accurate under the archive scheme.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address the review round on #534: - Replace the inline hoist step with `scripts/hoist_binstall_archives.py`: the expected archive set is derived from `.github/release-staging.toml` and `Cargo.toml`, every target's archive and `.sha256` sidecar is validated before anything moves, expected and found names are logged, and a shortfall fails the release listing the missing assets instead of uploading a partial set. Behavioural coverage lives in `tests/workflow_contracts/hoist_binstall_archives_test.py` (happy path, missing target, missing sidecar, empty dist, duplicate candidates, wiring), and `tests/binstall_metadata_tests.rs` now pins the workflow to the script rather than a step name. - Move the diagnostic-JSON snapshot settings into `snapshot_test_support::diagnostic_json_snapshot_settings()`, extending the module's existing shared helper so the documented policy holds for any module; tighten the redaction anchor to the enclosing `"generator"` object and prove non-generator `version` fields survive via an inline-snapshot test. - Cover `try_is_file`'s absent-path branch, add an exhaustive presence-combination test pinning the locator's first-match and error invariants, and surface candidate-probe filesystem errors in a dedicated test. - Drop the `example.id.clone()` in the registry-install contract by recording matches per example. - Refresh the developers' guide binstall bullet for the archive scheme and point the insta guide at the shared helper. The suggestion to make the `documentation_examples` module declaration private is deliberately not applied: with a private `mod`, the shared module's helpers used only by sibling test binaries fail the `-D warnings` gate as dead code; `pub mod` follows the existing loader-tests precedent. The ADR-007 reference is also unactioned: its binstall passage describes the package-name decision, which remains accurate under the archive scheme.
Address the second review round on #534: - Make the hoist script's filesystem probes explicitly fallible: discovery walks with `os.walk(onerror=...)` so traversal failures propagate instead of being suppressed by `Path.rglob`, and a `stat`-based probe distinguishes absent assets from unreadable ones. Validate destination collisions at the dist root before any move, and document why rollback is deliberately absent (the dist tree is rebuilt from workflow artefacts on every job run, and the upload step is gated on this script succeeding). - Give every public function a NumPy-style docstring, matching the convention of the repository's existing Python. - Rework the behavioural tests: diagnostic messages on every assert, a justified E402 suppression, source-absence and content-integrity assertions on the happy path, an exhaustive parametrization proving nothing moves for any failing state combination, varied manifest/config inputs, destination-collision and traversal-error cases, a `main()` CLI invocation test, and a YAML-parsed assertion that the hoist step precedes `upload_assets`. - Cover `try_is_file`'s regular-file branch, and split the fs unit tests into `fs_tests.rs` via the crate's `#[path]` convention to keep `fs.rs` within the Whitaker module-length cap.
cfb1052 to
a9365bb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Complete the second review round on #534: - Rewrite the developers' guide release-maintenance passage: the per-target binstall overrides and artefact-name table it referenced no longer exist. New targets are added to `.github/release-staging.toml` and the workflow target matrices; the single `pkg-url` template resolves them automatically, and the binstall and hoist contract tests fail if that drifts. The registry-install pin reference now names `tests/documentation_installation_tests.rs`, where those tests moved. - Correct ADR-007's Decision and Consequences to describe the single-template mechanism (repo ADRs carry no amendment convention, so the prose is corrected in place); the core decision to publish as `netsuke-build` is unchanged. - Annotate the fs unit tests with `#[rstest]`, per the AGENTS.md mandate for unit tests; the module previously used plain `#[test]` and was the outlier.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Responding to the failed pre-merge checks, row by row (fixes in Testing (Overall) — fixed. The suite now asserts source absence and Unit Architecture — fixed. Discovery now walks with Developer Documentation — fixed. The developers' guide Testing (Unit And Behavioural) — fixed. Testing (Property / Proof) — partially actioned, remainder dismissed Concurrency And State — dismissed with reasoning, plus a mitigation: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== commits =='
git show --no-ext-diff --format='%H %s' --no-patch a9365bb d53faab
printf '%s\n' '== changed files =='
git diff --no-ext-diff --name-status a9365bb^..d53faab
printf '%s\n' '== implementation outline =='
ast-grep outline scripts/hoist_binstall_archives.py --items all || true
printf '%s\n' '== implementation =='
cat -n scripts/hoist_binstall_archives.py
printf '%s\n' '== behavioural test outline =='
ast-grep outline tests/workflow_contracts/hoist_binstall_archives_test.py --items all || true
printf '%s\n' '== behavioural tests =='
cat -n tests/workflow_contracts/hoist_binstall_archives_test.py
printf '%s\n' '== release workflow hoist and upload context =='
rg -n -C 12 'hoist_binstall_archives|upload_assets' .github/workflows/release.yml
printf '%s\n' '== documentation references =='
rg -n -C 5 'binstall|hoist|archive|staging' docs/developers-guide.md docs/adr/ADR-007* 2>/dev/null || trueLength of output: 1505 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '== current revision =='
git rev-parse --short HEAD || true
git status --short || true
printf '%s\n' '== candidate files =='
fd -a 'hoist_binstall_archives|ADR-007|developers-guide|release\.yml' . || true
printf '%s\n' '== implementation outline =='
ast-grep outline scripts/hoist_binstall_archives.py --items all 2>&1 || true
printf '%s\n' '== implementation =='
cat -n scripts/hoist_binstall_archives.py 2>&1 || true
printf '%s\n' '== behavioural test outline =='
ast-grep outline tests/workflow_contracts/hoist_binstall_archives_test.py --items all 2>&1 || true
printf '%s\n' '== behavioural tests =='
cat -n tests/workflow_contracts/hoist_binstall_archives_test.py 2>&1 || true
printf '%s\n' '== release workflow context =='
rg -n -C 12 'hoist_binstall_archives|upload_assets' .github/workflows/release.yml 2>&1 || true
printf '%s\n' '== documentation files and references =='
fd -a -i 'ADR-007*' docs 2>&1 || true
rg -n -i -C 5 'binstall|hoist|archive|staging' docs/developers-guide.md docs 2>&1 || true
printf '%s\n' '== claimed coverage markers =='
rg -n -C 3 'permission|collision|duplicate|sidecar|missing|parametr|product|exhaust|onerror|os\.walk|main\(' \
scripts/hoist_binstall_archives.py tests/workflow_contracts/hoist_binstall_archives_test.py 2>&1 || trueLength of output: 155472
Keep the no-rollback design. The release job recreates Mark the testing, unit architecture, documentation, behavioural testing, workflow wiring, and finite-state coverage findings as verified. The current files implement and test those changes. Fix one remaining destination-collision case. Reject every existing destination entry before the move loop. Add tests for archive and sidecar destination directories. ✏️ Learnings added
You are interacting with an AI system. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/workflow_contracts/hoist_binstall_archives_test.py Comment on lines +85 to +112 def stage_pair(
dist: Path,
artefact: str,
staging_dir: str,
name: str,
*,
with_sidecar: bool = True,
) -> None:
"""Create an archive (and optionally its sidecar) in the nested layout.
Parameters
----------
dist
Dist root to stage below.
artefact
Workflow-artefact directory name (first nesting level).
staging_dir
Staging directory name (second nesting level).
name
Archive file name to create.
with_sidecar
Whether to create the matching ``.sha256`` sidecar.
"""
nested = dist / artefact / staging_dir
nested.mkdir(parents=True, exist_ok=True)
(nested / name).write_bytes(f"archive:{name}".encode())
if with_sidecar:
(nested / f"{name}.sha256").write_text(f"checksum:{name}", encoding="utf-8")❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== revision =='
git rev-parse --short HEAD
printf '%s\n' '== outline =='
ast-grep outline tests/workflow_contracts/hoist_binstall_archives_test.py --items all
printf '%s\n' '== stage_pair definition and uses =='
rg -n -C 6 '\bstage_pair\s*\(' tests/workflow_contracts/hoist_binstall_archives_test.py
printf '%s\n' '== nearby test configuration =='
sed -n '1,260p' tests/workflow_contracts/hoist_binstall_archives_test.py
printf '%s\n' '== Python test runner configuration =='
fd -a -t f 'pyproject.toml|pytest.ini|tox.ini|requirements.*|Makefile' . \
-E .git -E target | sortLength of output: 15318
Do not make a further code change. The current codegraph at revision
def stage_pair(
dist: Path,
nested_dir: str,
name: str,
*,
with_sidecar: bool = True,
) -> None:The previous Paste this into the CodeScene diagnostic suppression field:
You are interacting with an AI system. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReview finished.
|
|
@coderabbitai Reconciling the round-3 pre-merge rows against head f01797c, row by row: Testing (Overall) — ❌ — valid; both gaps are now closed in f01797c. Performance And Resource Use — ❓ — validated against release input sizes; no findings. Discovery walks the dist tree once with |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== revision =='
git rev-parse --short HEAD
git status --short
printf '%s\n' '== relevant test symbols =='
ast-grep outline tests/workflow_contracts/hoist_binstall_archives_test.py \
--items all --match 'test_release_workflow_disables_the_uv_cache|test_hoist_rejects_a_dangling_symlink_at_the_destination'
printf '%s\n' '== cache and dangling-symlink test context =='
rg -n -C 12 \
'def test_release_workflow_disables_the_uv_cache|def test_hoist_rejects_a_dangling_symlink_at_the_destination|enable-cache|dangling|destination already occupied|symlink' \
tests/workflow_contracts/hoist_binstall_archives_test.py .github/workflows/release.yml
printf '%s\n' '== discovery structure =='
ast-grep outline scripts/hoist_binstall_discovery.py --items all \
--match '_walk_files|_resolve_archive|locate_archives'
rg -n -C 8 \
'^(def (_walk_files|_resolve_archive|locate_archives)|class )|os\.walk|onerror|for .* in .*files|lstat|staged' \
scripts/hoist_binstall_discovery.py
printf '%s\n' '== test command definition =='
rg -n -C 3 '^test-workflow-contracts:|test-workflow-contracts|hypothesis' MakefileLength of output: 29306
Do not open a follow-up issue for either row. The source inspection did not run ✏️ Learnings added
You are interacting with an AI system. |
The `v0.1.0-beta1` release left `cargo binstall netsuke-build` falling
back to a source build: the crates.io metadata was published, but the
GitHub release assets it pointed at were never uploaded, and the six
per-target `pkg-url` overrides depended on a fragile
workflow-artefact/staging-dir naming convention.
Adopt the `stage-release-artefacts` action's cargo-binstall support
instead:
- Enable `[common.binstall]` in `.github/release-staging.toml`, so
every target stages a `{package_name}-{version}-{target}.tar.gz`
archive (with a `.sha256` sidecar) containing the bare `netsuke`
binary.
- Hoist those archives to the `dist/` root in the release job before
`upload-release-assets` runs, so they upload under their plain
names rather than the namespaced `<artifact>__<staging-dir>-` form;
the step fails when no archives are found.
- Replace the six per-target overrides in `Cargo.toml` with a single
`pkg-url` template (`{ name }-{ version }-{ target }.tar.gz`,
`pkg-fmt = "tgz"`), keeping `Cargo.toml` as the single source of
truth for the asset names.
- Rewrite `tests/binstall_metadata_tests.rs` to hold the metadata,
the staging configuration, and the release workflow (including the
hoist step) to the new contract.
Address the review round on #534: - Replace the inline hoist step with `scripts/hoist_binstall_archives.py`: the expected archive set is derived from `.github/release-staging.toml` and `Cargo.toml`, every target's archive and `.sha256` sidecar is validated before anything moves, expected and found names are logged, and a shortfall fails the release listing the missing assets instead of uploading a partial set. Behavioural coverage lives in `tests/workflow_contracts/hoist_binstall_archives_test.py` (happy path, missing target, missing sidecar, empty dist, duplicate candidates, wiring), and `tests/binstall_metadata_tests.rs` now pins the workflow to the script rather than a step name. - Move the diagnostic-JSON snapshot settings into `snapshot_test_support::diagnostic_json_snapshot_settings()`, extending the module's existing shared helper so the documented policy holds for any module; tighten the redaction anchor to the enclosing `"generator"` object and prove non-generator `version` fields survive via an inline-snapshot test. - Cover `try_is_file`'s absent-path branch, add an exhaustive presence-combination test pinning the locator's first-match and error invariants, and surface candidate-probe filesystem errors in a dedicated test. - Drop the `example.id.clone()` in the registry-install contract by recording matches per example. - Refresh the developers' guide binstall bullet for the archive scheme and point the insta guide at the shared helper. The suggestion to make the `documentation_examples` module declaration private is deliberately not applied: with a private `mod`, the shared module's helpers used only by sibling test binaries fail the `-D warnings` gate as dead code; `pub mod` follows the existing loader-tests precedent. The ADR-007 reference is also unactioned: its binstall passage describes the package-name decision, which remains accurate under the archive scheme.
Address the second review round on #534: - Make the hoist script's filesystem probes explicitly fallible: discovery walks with `os.walk(onerror=...)` so traversal failures propagate instead of being suppressed by `Path.rglob`, and a `stat`-based probe distinguishes absent assets from unreadable ones. Validate destination collisions at the dist root before any move, and document why rollback is deliberately absent (the dist tree is rebuilt from workflow artefacts on every job run, and the upload step is gated on this script succeeding). - Give every public function a NumPy-style docstring, matching the convention of the repository's existing Python. - Rework the behavioural tests: diagnostic messages on every assert, a justified E402 suppression, source-absence and content-integrity assertions on the happy path, an exhaustive parametrization proving nothing moves for any failing state combination, varied manifest/config inputs, destination-collision and traversal-error cases, a `main()` CLI invocation test, and a YAML-parsed assertion that the hoist step precedes `upload_assets`. - Cover `try_is_file`'s regular-file branch, and split the fs unit tests into `fs_tests.rs` via the crate's `#[path]` convention to keep `fs.rs` within the Whitaker module-length cap.
Complete the second review round on #534: - Rewrite the developers' guide release-maintenance passage: the per-target binstall overrides and artefact-name table it referenced no longer exist. New targets are added to `.github/release-staging.toml` and the workflow target matrices; the single `pkg-url` template resolves them automatically, and the binstall and hoist contract tests fail if that drifts. The registry-install pin reference now names `tests/documentation_installation_tests.rs`, where those tests moved. - Correct ADR-007's Decision and Consequences to describe the single-template mechanism (repo ADRs carry no amendment convention, so the prose is corrected in place); the core decision to publish as `netsuke-build` is unchanged. - Annotate the fs unit tests with `#[rstest]`, per the AGENTS.md mandate for unit tests; the module previously used plain `#[test]` and was the outlier.
CodeScene flagged `stage_pair` at five arguments against the four-argument ceiling. The artefact and staging-directory names only ever combine into one relative path, so accept that path directly as `nested_dir` and let call sites write `"artefact/staging"`.
Address the third review round on #534: - Destination validation now probes with `lstat` via `_exists_any`: any entry — regular file, directory, symlink (including dangling), socket, FIFO, or device node — occupying either the archive or sidecar destination is a collision; only `FileNotFoundError` maps to "absent" and every other `OSError` propagates. Directory collisions at both destinations are covered by tests asserting failure, reporting, preserved sources, and no partial movement. - The move phase is all-or-nothing: `_move_all` records completed moves and rolls them back in reverse on any failure, re-raising the original; a rollback failure surfaces both causes as an exception group. An injection test fails the third move, asserts propagation, an empty release root, and restored sources with original content, then clears the fault and proves a rerun succeeds. The module docstring's "rollback is deliberately absent" statement is replaced with the actual guarantee. - Hypothesis joins the workflow-contract harness (via the Makefile's uv dependency list) with a bounded, derandomized generated-layout test: package names, versions, unique target sets, nesting layouts, and per-target states (ok, missing archive, missing sidecar, duplicate, collision), asserting the all-or-none and name-derivation invariants without hard-coded fixture names. - Convert the locator tests to `rstest`: a `temp_root` fixture, a `LocatorScenario`-parametrized layout test, and eight named presence-mask cases for the exhaustive precedence test. - Remove the comma before "because" in ADR-007's template sentence.
CodeScene flagged the Hypothesis test for excess arguments, nested conditionals, and cyclomatic complexity. Group the four generated values into a frozen `GeneratedHoistCase` dataclass (with the archive name derivation and failure expectation as members), generate one case per example, and extract workspace construction, per-target staging, and the valid/invalid outcome assertions into focused helpers. The test body is now linear: build, stage, run, dispatch. Input domains, bounds, determinism, and every assertion are unchanged; `make test-workflow-contracts` passes 36 tests.
CodeScene flagged the module's overall complexity, centred on `_move_all` carrying both the forward-move transaction and the reverse-order restoration loop. Move the latter into `_rollback_completed_moves`, which restores completed moves in reverse order and lets its failures propagate; `_move_all` now only orchestrates — forward moves, delegation to the rollback on failure, and the unchanged combination semantics (re-raise the original after a clean rollback, or surface both failures as an exception group). Behaviour, move order, and the all-or-nothing guarantee are unchanged; the existing rollback-injection test exercises the helper through `_move_all`, so no duplicate filesystem scenario is added.
The estate-wide en-GB-oxendict base dictionary in agent-helper-scripts now ignores "mis-grouping", which `docs/netsuke-design.md` uses. The `spelling-config` gate regenerates `typos.toml` and fails on any drift from the tracked copy, so commit the refreshed output.
The shared en-GB-oxendict base dictionary no longer exempts inline code spans, so `typos` began flagging verbatim identifiers such as tokio's `flavor` attribute argument. Re-add the code-span ignore in `typos.local.toml`, where repository-specific policy belongs, and regenerate `typos.toml`. Also correct two prose uses of "hand-written" (backticked as if code) to "handwritten", matching the shared dictionary's phrase correction.
Address the outstanding CodeRabbit findings on the cargo-binstall hoist. - Reject duplicate target triples in `release-staging.toml`. A repeated triple derived the same archive name twice, so `locate_archives` claimed the same staged file twice and the second move failed on an already-relocated source, rolling back an otherwise valid release. - Probe staged assets with `lstat` and validate the archive as well as the sidecar. `Path.stat()` follows symlinks, so a symlink named like the archive was previously hoisted in its place, publishing a broken link. - Run the hoist under a Python 3.13 installed by `setup-uv` rather than the runner's bare `python3`; the script relies on `BaseExceptionGroup`, which needs Python 3.11 or later. - Split the read-only discovery and validation half into `scripts/hoist_binstall_discovery.py` so both files stay within the 400-line cap and CodeScene's complexity thresholds, with a hard seam between validation and mutation. - Replace the `monkeypatch` stub in the rollback test with a scoped `unittest.mock.patch.object`, keeping the retry outside the mock scope. - Extract a shared temporary-directory fixture in `fs_tests.rs`, and drop the snapshot collection from the presence-combination test in `netsuke.rs` in favour of direct traversal. - Correct the ADR and developers' guide: release archives are named after the Cargo package, not the binary, and `cargo binstall`'s default patterns miss them because they order the target before the version.
Reconcile the two actionable CodeRabbit pre-merge check rows. - Add `test_hoist_surfaces_both_errors_when_the_rollback_also_fails`, which injects a forward move failure and then a rollback failure and asserts the raised group carries both. Verified by mutation: replacing the `BaseExceptionGroup` with a bare re-raise fails the new test, so the branch is no longer removable without a test failing. - Refresh the developers' guide test map, which described `binstall_metadata_tests.rs` as holding per-target `cargo binstall` overrides — the opposite of the single `pkg-url` template it now pins — and document that `make test-workflow-contracts` runs a Hypothesis-backed suite provisioned through `uv run --with`.
- Disable the `setup-uv` cache in the release job: the hoist script is stdlib-only under `--no-project`, so a restored cache adds a supply-chain input to a `contents: write` job for no benefit. - Report an omitted `[targets]` table through the named contract failure instead of a bare `KeyError`, and cover it alongside the empty-table case. - Decompose the `StagedArchive | str` resolution with structural pattern matching, per the path instructions. - Correct "fall back" to the noun "fallback" in the developers' guide.
The third review round noted two behaviours no test asserted: the release job's `enable-cache: false` on `setup-uv`, and rejection of a dangling symlink occupying an archive or sidecar destination. Both regressions would previously have passed unnoticed. Add a workflow-contract test for the cache setting and a parametrised behavioural case staging a link to nowhere at each destination, pinning the `lstat`-based probe's treatment of any entry as a collision.
|
@coderabbitai Have the following now been resolved? tests/workflow_contracts/hoist_binstall_archives_test.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== revision =='
git rev-parse --short HEAD
git status --short
printf '%s\n' '== workflow-contract Python files and physical line counts =='
fd -a -e py . tests/workflow_contracts -x wc -l | sort -n
printf '%s\n' '== workflow-contract file tree =='
fd -a -t f . tests/workflow_contracts | sort
printf '%s\n' '== module outlines =='
fd -a -e py . tests/workflow_contracts -x sh -c '
printf "\n--- %s ---\n" "$1"
ast-grep outline "$1" --items all || true
' sh {} \;
printf '%s\n' '== shared fixture and helper definitions/usages =='
rg -n -C 3 \
'^(def (workspace|stage_pair|run_hoist|assert_nothing_moved|prepare_generated|_stage_generated|assert_(valid|invalid)|test_hoist_invariants)|`@pytest`\.fixture|class (Generated|Prepared))|\b(workspace|stage_pair|run_hoist|assert_nothing_moved|prepare_generated|test_hoist_invariants)\b' \
tests/workflow_contracts -g '*.py'
printf '%s\n' '== imports among workflow-contract modules =='
rg -n -C 2 '^(from |import )' tests/workflow_contracts -g '*.py'
printf '%s\n' '== current diff file list =='
git diff --name-status HEAD^..HEAD || trueLength of output: 53950
Do not mark this finding as resolved. At current Apply this split:
✏️ Learnings added
You are interacting with an AI system. |
f01797c to
a6eba4a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The workflow-contract hoist tests had grown to 901 lines against the repository's 400-line cap. Divide the module by concern, with no test behaviour changed: - `conftest.py` holds the shared `workspace` fixture, the `stage_pair`, `run_hoist`, and `assert_nothing_moved` helpers, the shared staging constants, and the `sys.path` seam to the production script. - `hoist_binstall_archives_test.py` keeps the example-based naming, validation-failure, filesystem-safety, and CLI tests. - `hoist_binstall_rollback_test.py` holds the move-rollback and failed-rollback transaction tests. - `hoist_binstall_archives_generated_test.py` holds the Hypothesis generated-layout suite with its strategies and bounds unchanged. - `release_workflow_hoist_test.py` holds the `release.yml` wiring contracts.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Summary
This branch makes
cargo binstall netsuke-buildfetch real prebuiltarchives instead of falling back to a source build. It adopts the
cargo-binstall archive support in the shared
stage-release-artefactsaction: every release target now stages a
{package_name}-{version}-{target}.tar.gzarchive (plus a.sha256sidecar) containing the bare
netsukebinary, the release job hoiststhose archives to the release root so they upload under their plain
names, and the six hand-maintained per-target
pkg-urloverrides inCargo.tomlcollapse to a single template withpkg-fmt = "tgz".The branch is stacked on
fix-snapshot(#532) so its tests run
against the repaired version contracts; it also carries the
shared-actions pin bump that landed on
main(#528). GitHub willretarget it to
mainonce #532 merges.Review walkthrough
[common.binstall]enablement.dist/root beforeupload-release-assetsruns (it fails the job when no archives are found, so a staging regression cannot pass silently).{ name }-{ version }-{ target }.tar.gztemplate.Validation
make check-fmt: passmake lint(Clippy + Whitaker, warnings denied): passmake test(full workspace nextest + doctests): passNotes
v0.1.0-beta1assets remains blocked on the WindowsMSI fix in shared-actions#406;
once that merges, netsuke's shared-actions pins need bumping past it
and the release workflow re-running for the tag.
netsuke-build), matchingbinstall's
{ name }interpolation, while the other release assetskeep their
netsuke-based names.Summary by Sourcery
Adopt staged cargo-binstall archives for netsuke-build releases and simplify the corresponding binstall metadata and workflows.
New Features:
Enhancements:
CI:
Tests: