Skip to content

fix(release): use CNB asset download URLs - #5308

Merged
Hmbown merged 6 commits into
mainfrom
fix/cnb-release-download-url
Aug 9, 2026
Merged

fix(release): use CNB asset download URLs#5308
Hmbown merged 6 commits into
mainfrom
fix/cnb-release-download-url

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • use the canonical codewhale.net/codewhale CNB repository slug in both updater implementations
  • add the required /-/releases/download/vX.Y.Z/ path so mirror mode receives asset bytes instead of release HTML
  • preserve explicit mirror override precedence and the deliberate v0.9.6 compatibility asset inventory
  • serialize the shared CNB Rust gate, omit debug metadata only from disposable test artifacts, and give the full gate an explicit 45-minute bound
  • run the full workspace test command with the same 16 MiB test-thread stack used by GitHub CI and the release workflow

#5306 has merged. This PR is now the focused CNB URL and mirror-pipeline reliability slice on current main.

Live proof

  • old npm-shaped v0.9.5 manifest URL: HTTP 200 text/html, 118624 bytes
  • corrected URL: HTTP 200 text/plain, 258-byte checksum manifest
  • old Rust Hmbown/CodeWhale slug: HTTP 404
  • exact head cb1e994829783047111aafde05b4ba35a33ddf5c reached CNB but failed while parallel Rust jobs compiled the TUI test crate
  • exact head b899c42be969aec53debd3138bef9fcd538ab17e passed dependency setup, then exposed the next concrete failure: runtime_api::tests::retry_endpoint_reuses_dropped_user_text_to_start_a_turn overflowed the platform-default test stack and aborted with SIGABRT
  • with CARGO_BUILD_JOBS=1, the all-target workspace check and all-feature clippy pass in an 8 GiB container; CNB's cpus: 16 runner contract provides 32 GiB
  • RUST_MIN_STACK=16777216 is scoped only to the workspace test command, matching the established GitHub CI/release contract without raising compiler-thread stack reservations

Verification

  • node --test npm/codewhale/test/artifacts.test.js (11 passed)
  • cargo test --locked -p codewhale-release --lib (37 passed)
  • cargo test --locked -p codewhale-cli --lib update::tests (67 passed)
  • cargo check --locked -p codewhale-release -p codewhale-cli
  • RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib runtime_api::tests::retry_endpoint_reuses_dropped_user_text_to_start_a_turn --locked -- --exact --nocapture (1 passed)
  • node .github/scripts/release-workflows.test.js
  • cargo fmt --all -- --check
  • git diff --check

Exact head 5162341fd1df799f94f398d70781edee0463f83f must pass both GitHub and CNB before merge.

Closes #5307

CodeWhale Bot added 3 commits August 8, 2026 17:58
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@cursor

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@Hmbown Hmbown added this to the v0.9.6 milestone Aug 9, 2026
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 3s —— View job


Claude finished reviewing PR #5308

Todo list

  • Gather context (diff, CLAUDE.md/AGENTS.md, changed files)
  • Understand the request (code review of PR fix(release): use CNB asset download URLs #5308)
  • Review correctness bugs
  • Review provider/model/route safety implications
  • Review reuse/simplification opportunities
  • Review test coverage
  • Review security implications
  • Post final review summary

Summary

This PR's own commit (cb1e994, the last of the 3) is small and focused: it aligns CNB_RELEASE_ASSET_BASE in crates/release/src/lib.rs and releaseBaseUrl() in npm/codewhale/scripts/artifacts.js on the same slug (codewhale.net/codewhale) and the same /-/releases/download/vX.Y.Z path, matching the live-proof HTTP checks in the PR description. I verified by grep that no stale Hmbown/CodeWhale CNB URLs or old /releases/vX.Y.Z (no /download/) paths remain anywhere in the tree — Rust, JS, and docs (docs/CNB_MIRROR.md, docs/INSTALL.md, docs/RELEASE_RUNBOOK.md) are all consistent with the corrected format. The remaining Hmbown/CodeWhale references (crates/cli/src/update.rs, LATEST_RELEASE_URL in crates/release/src/lib.rs) are legitimately for the actual GitHub repo, not the CNB mirror, so they're correctly left alone.

The other two commits (4a69337, 5763000) are the stacked #5306 content (crate-publish-order validator + tests, publish-crates.sh refactor, crates.sh reorder). Since this PR currently merges all three on top of main, I reviewed that content too:

  • Good catch, not a regression: scripts/release/crates.sh reorders codewhale-core before codewhale-tui. I confirmed crates/tui/Cargo.toml depends on codewhale-core as a path dependency, so the old order was backwards — this fix is correct and would have been caught by the new validate-crate-publish-order.py topological check anyway.
  • scripts/release/validate-crate-publish-order.py reimplements and extends what was previously an inline Python heredoc in publish-crates.sh (mixed-version check, missing/extra package check) and adds real topological-order validation using cargo metadata dependency edges, correctly special-casing dev dependencies (since Cargo doesn't compile them during publish verification). This is a solid consolidation, not a duplication — the old inline script is fully removed.
  • publish-crates.sh moves the require-release-tag-checkout.sh / verify-release-assets.sh calls to run after the metadata-inventory/version-resolution step instead of before. I didn't find a problem with this — both checks still execute before the actual cargo publish loop later in the script — but flagging the reorder in case it was unintentional relative to the original commit.
  • No provider/model routing code is touched by this diff — the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant doesn't apply here.
  • Test coverage is solid: crates/release/crates/cli unit tests were updated in lockstep with both URL constants, the npm artifacts.test.js suite covers the JS side, and validate-crate-publish-order.test.sh exercises duplicate/missing/extra/non-topological failure fixtures via a synthetic cargo metadata JSON fixture.

I was not able to execute cargo test, node --test, or the shell test scripts myself — this sandbox required approval for any command beyond read-only git/grep, which wasn't available in this run. I relied on static review of the diff and cross-referencing dependency graphs/URLs directly. If you want me to actually execute the verification commands from the PR description, the --allowedTools for this job would need to permit Bash(cargo test:*), Bash(node --test:*), and Bash(bash scripts/release/*.test.sh:*).

No correctness bugs, security issues, or reuse/simplification concerns found beyond the note above.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 37s —— View job


Claude finished reviewing PR #5308 (updated after new push)

Todo list

  • Diff the new commit against the previously reviewed HEAD
  • Review ci(cnb): bound Rust gate memory (b899c42) for correctness
  • Verify the new regression test matches the actual .cnb.yml content
  • Re-confirm prior findings on the CNB URL fix still hold (no changes there)
  • Post review

What changed since the last review

Only one new commit landed: b899c42ci(cnb): bound Rust gate memory, touching .cnb.yml (+7) and .github/scripts/release-workflows.test.js (+15). The previously-reviewed commits (4a69337, 5763000, cb1e994 — crate publish-order validation and the CNB asset URL fix) are unchanged; my prior findings on those still stand (no issues, good catch on the codewhale-core/codewhale-tui publish order fix).

Review of b899c42

  • .cnb.yml: adds timeout: 45m and export CARGO_BUILD_JOBS=1 / export CARGO_PROFILE_TEST_DEBUG=0 to the shared .rust_workspace_gates_stage YAML anchor. Since this is a real anchor/alias (- *rust_workspace_gates_stage), the change applies identically to both consumers (linux_rust_gates and linux_release_preflight) — correct and consistent with the stated goal of bounding memory on the shared CNB runner.
  • The exports are scoped to that stage's own script block (each CNB stage runs its own shell), so they don't leak into the later cargo build --jobs 2 --release ... steps in the same jobs, which intentionally keep parallelism 2. No unintended interaction there.
  • CARGO_PROFILE_TEST_DEBUG=0 and CARGO_BUILD_JOBS=1 are both valid, real Cargo env vars for this purpose (profile override + build parallelism), used correctly.
  • .github/scripts/release-workflows.test.js: the new assertions regex-match the .rust_workspace_gates_stage block for the timeout: 45m / env exports / cargo check ordering, and assert the anchor is reused exactly twice (linux_rust_gates + linux_release_preflight). I manually diffed this against the actual .cnb.yml content and the expected pattern and reuse count line up correctly.
  • This test was already wired into ci.yml's existing node .github/scripts/release-workflows.test.js step (added in an earlier commit in this stack), so it will run in CI without further changes.

No correctness bugs, no provider/model routing implications (none of this touches routing code), and no reuse/duplication concerns — this is a small, self-contained CI tuning change with matching test coverage.

Sandbox note: as with the prior review pass, this run's --allowedTools only permitted read-only git commands — I couldn't execute node .github/scripts/release-workflows.test.js, cargo test, or python3 -c "yaml.safe_load(...)" to mechanically confirm the YAML parses and the test passes. I verified both by careful static comparison of the regex against the file content instead. If you'd like this run to actually execute the verification commands, --allowedTools would need Bash(node .github/scripts/*.test.js:*) and Bash(cargo test:*).

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 15s —— View job


Claude finished reviewing PR #5308 (updated after new push)

Todo list

  • Diff the new commit against the previously reviewed HEAD
  • Confirm the effective PR base (branch was rebased onto main after fix(release): validate crate publication order #5306 merged — GitHub's stale changed_files list still shows 12 files, but the live diff against current main is only 6 files)
  • Review ci(cnb): match workspace test stack (5162341) for correctness
  • Cross-check RUST_MIN_STACK usage against ci.yml / release.yml precedent
  • Re-confirm prior findings on the CNB URL fix and Rust-gate memory bound still hold
  • Post review

What changed since the last review

Only one new commit landed: 5162341ci(cnb): match workspace test stack, touching .cnb.yml (+3/-2 net) and .github/scripts/release-workflows.test.js (regex update). Note: the branch was rebased onto current main (which now includes #5306), so the PR's live diff against origin/main is just these 6 files:
.cnb.yml, .github/scripts/release-workflows.test.js, crates/cli/src/update.rs, crates/release/src/lib.rs, npm/codewhale/scripts/artifacts.js, npm/codewhale/test/artifacts.test.js. The GitHub-supplied changed_files list (12 files, including validate-crate-publish-order.py) is stale from before that rebase — I verified those files are already present on origin/main. The update.rs / lib.rs / artifacts.js / artifacts.test.js diffs are unchanged from what was reviewed previously (no new findings there).

Review of 5162341

  • .cnb.yml: adds RUST_MIN_STACK=16777216 as an inline prefix to cargo test --workspace --all-features --locked, matching the exact value used in .github/workflows/ci.yml:452 and .github/workflows/release.yml:171. Good catch — this addresses the same class of stack-overflow risk documented in ci.yml's comment (deep tokio-runtime futures in #[tokio::test] vs. the product's own CODEWHALE_MAIN_STACK_BYTES).
  • The two subsequent parity-gate commands (cargo test -p codewhale-protocol --test parity_protocol --locked, cargo test -p codewhale-state --test parity_state --locked) are not prefixed with RUST_MIN_STACK. I checked release.yml:172-175 — the same asymmetry exists there (the env var is scoped to the earlier workspace-test step only, not the parity steps). The deep-stack test that originally motivated this (start_turn_accepts_dynamic_tools_and_environment_id) lives in crates/tui, not codewhale-protocol/codewhale-state, so this is consistent with established precedent, not an oversight.
  • .github/scripts/release-workflows.test.js: the regex was extended to require cargo clippy ... -D warnings immediately followed by RUST_MIN_STACK=16777216 cargo test --workspace --all-features --locked in that exact order. I diffed this against the actual .cnb.yml script block — the order matches.

No correctness bugs, no provider/model routing implications, no reuse/duplication concerns. This is a small, well-justified CI reliability fix with matching test coverage, consistent with the pattern already established in ci.yml/release.yml.

Sandbox note: as with prior review passes on this PR, this run's --allowedTools only permitted read-only git/grep/sed — I couldn't execute node .github/scripts/release-workflows.test.js, cargo test, or a YAML parse to mechanically confirm the assertions pass. I verified by careful static comparison of the regex against .cnb.yml's actual content instead. If you'd like this run to execute the verification commands, --allowedTools would need Bash(node .github/scripts/*.test.js:*) and Bash(cargo test:*).

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 38s —— View job


Claude finished reviewing PR #5308 (updated after new push)

Todo list

  • Diff the new commit against the previously reviewed HEAD
  • Review test(cnb): make failure fixtures root-safe (11f6c99) for correctness
  • Trace the config.toml.bak fixture through mutate_config_documentwrite_one_time_config_backupchecked_config_backup_path to confirm it actually exercises the intended failure path
  • Sweep the workspace for other permission-bit test fixtures that could be root-unsafe on CNB
  • Re-confirm prior findings on the CNB URL fix / Rust gate memory bound / test stack size still hold
  • Post review

What changed since the last review

Only one new commit landed: 11f6c99test(cnb): make failure fixtures root-safe, touching crates/tui/src/utils.rs (+16/-22) and crates/tui/src/xai_oauth.rs (+2/-4). All previously-reviewed commits (crate publish-order validation, CNB asset URL fix, Rust-gate memory bound, RUST_MIN_STACK match) are unchanged — prior findings still stand (no issues found across those).

Review of 11f6c99

CNB's rust:1.88-bookworm image runs as root (.cnb.yml:47), so any test fixture that relies on chmod denying access to root itself is a false green/red depending on privilege level — a real portability bug in the test suite, not the product code. This commit replaces two such fixtures with mechanisms that fail independent of privilege:

  • crates/tui/src/utils.rs:1027 (write_atomic_workspace_replaces_self_referential_symlink_without_following): replaces a chmod 000 "unreadable directory" fixture with a self-referential symlink (symlink(&link, &link)). Following it always fails with ELOOP/FilesystemLoop, which is a kernel loop-detection error, not a permission check — root hits it exactly like any other user. The test correctly asserts follow_error.kind() != NotFound (portable across Rust versions where ErrorKind::FilesystemLoop may or may not be stable) rather than pinning an exact ErrorKind. I checked write_atomic_with_permissions (utils.rs:294-314) — it already uses fs::symlink_metadata (not fs::metadata) specifically to avoid following links, so this fixture is verifying real, already-correct behavior, not chasing a bug.
  • crates/tui/src/xai_oauth.rs:2177 (activation_failure_cleans_unreferenced_stage_and_keeps_live_config_inert): replaces a chmod 500 "read-only config dir" fixture with fs::create_dir(config_dir.join("config.toml.bak")). I traced this through activate_device_login_lockedcodewhale_config::mutate_config_documentwrite_one_time_config_backup (crates/config/src/lib.rs:5135) → checked_config_backup_path/config_backup_file_name, which does construct config.toml.bak as the backup destination, then calls persistence::atomic_write(&backup, ...). Writing to a path where a directory already exists fails regardless of the calling user's privilege (EISDIR/rename-onto-directory failure), so this is a genuinely root-safe substitute that still exercises the intended "activation must fail closed and leave live config/credentials untouched" assertions later in the test.

Both replacements preserve the original test's assertions (link/file state, "not activated" error message, byte-identical legacy credentials) — this isn't a weakened test, just a different failure trigger.

Proactive check: other root-unsafe fixtures

I grepped the workspace for other from_mode(0o000/0o400/0o500/0o200/0o444) test fixtures to see if this class of CNB flake is fully addressed:

  • crates/telemetry/src/tests.rs:906 (make_read_only) already self-guards with a geteuid_is_root() check and returns false (skips the assertion) under root — already root-safe, no action needed.
  • crates/config/src/tests.rs:3664 chmods a seed file to 0o400 before asserting store.save() is a no-op when content is unchanged — it doesn't assert on a permission-denied error, so it passes identically whether or not root actually enforces the mode bit. Not root-unsafe.
  • crates/tui/src/plugins/registry.rs:1540 (set_staged_read_only_directory) is production hardening code invoked from harden_staged_tree/stage, not a test assertion on write failure — out of scope.

No other CNB-root-unsafe fixtures found; this commit appears to close out the class of flake it targets.

No correctness bugs, no provider/model routing implications (none of this touches routing code), no reuse/duplication concerns, and no security issues — this is a well-targeted test-infrastructure fix with verified mechanism (I traced the actual .bak code path rather than taking the commit message at face value).

Sandbox note: as with prior review passes on this PR, this run's git fetch was blocked by the sandbox (approval required, none available in this run), so I worked from the pre-fetched refs already present locally and confirmed the diff by inspecting the actual commit content and tracing the exercised code paths directly, rather than running cargo test. If you'd like this run to execute cargo test -p codewhale-tui --lib utils::atomic_write_tests xai_oauth::tests, --allowedTools would need to permit Bash(cargo test:*).

@Hmbown
Hmbown merged commit 7aa5c6b into main Aug 9, 2026
26 checks passed
@Hmbown
Hmbown deleted the fix/cnb-release-download-url branch August 9, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v0.9.6: CNB mirror mode must download assets, not release HTML

1 participant