[kornelski]

No description provided.

[clanky-the-clanker]

Superseded by the review of 13cf3f84.

This review, of 153f137c

💬 Approved with comments — There are two WARNINGs about the publish step: the rerun detection probably never matches current cargo output, and the registry token is now present during verification builds. The other items are minor.

Findings: 2 warnings · 1 info · 1 suggestion (2 are inline comments on the diff)

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #547.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (2)

### [Security] `cargo publish` without `--no-verify` runs build scripts and proc-macros, including the BoringSSL build in `boring-sys`, while `CARGO_REGISTRY_TOKEN` is in the environment. The old workflow avoided this on purpose.
- **File:** .github/workflows/publish.yml:74

### [Code Quality] The duplicate-upload regex matches the old `error: api errors ...` wording. Current cargo reports a duplicate as `... already exists on crates.io index` or `the remote server responded with an error`, so reruns of a partly finished release probably fail instead of continuing as PUBLISH.md says.
- **File:** .github/workflows/publish.yml:76

## INFO (1)

### [Release] Reruns accept an already-published version without checking its checksum or yank status (documented in PUBLISH.md), so a mixed release could be reported as a success.
- **File:** .github/workflows/publish.yml:75

## SUGGESTION (1)

### [Code Quality] The tag check gives a bare `gh` 404 when the tag is missing, and its error message calls the checked-out commit "the master tip" even if master has moved since dispatch.
- **File:** .github/workflows/publish.yml:59

---

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

The duplicate-upload detection probably never matches current cargo output, the token is exposed during the verification build, and the tag-check messages are minor.

Recommendation: 📝 Minor Comments

• [WARNING] The duplicate-upload regex matches the old error: api errors ... wording. Current cargo reports a duplicate as ... already exists on crates.io index or the remote server responded with an error, so reruns of a partly finished release probably fail instead of continuing as PUBLISH.md says. (.github/workflows/publish.yml:76)
• [SUGGESTION] The tag check gives a bare gh 404 when the tag is missing, and its error message calls the checked-out commit "the master tip" even if master has moved since dispatch. (.github/workflows/publish.yml:59)

View full review

Duplicate-upload detection (.github/workflows/publish.yml:76)

• The regex expects ^error: api errors ... is already uploaded. That api errors (status ...) wording comes from older cargo.
• Recent cargo, including the pinned 1.85.0, reports server errors as "the remote server responded with an error ...".
• Before uploading, cargo also checks the index and fails with crate <name>@<version> already exists on crates.io index. That message does not match the regex either.
• So a rerun of a partly finished release most likely fails at the first crate that is already published. PUBLISH.md:49-51 says this case works.
• Cargo could not be run here, so this is based on cargo's known error messages, not on output seen in this run.

Token present during the verification build (.github/workflows/publish.yml:69-81)

• The old workflow used --no-verify explicitly, so that no build scripts ran while the registry token was present.
• cargo publish now builds the packaged crate with CARGO_REGISTRY_TOKEN set. That runs boring-sys's build.rs and every dependency's build script and proc-macros.

Tag check (.github/workflows/publish.yml:58-63)

• A missing tag gives a bare 404 from gh api, with no explanation.
• The error message calls the checked-out commit "the master tip", which is not quite right if master has moved since the run was dispatched.

---

🚀 Release: 💬 1 info

Reruns accept an already-uploaded version without verifying its contents or yank status.

Recommendation: 💬 Optional Nits

• [INFO] Reruns accept an already-published version without checking its checksum or yank status (documented in PUBLISH.md), so a mixed release could be reported as a success. (.github/workflows/publish.yml:75)

View full review

Risk: Low

• .github/workflows/publish.yml:74-76 treats a duplicate-upload error as success. It does not check that the existing crate matches this release's archive, or that it is not yanked.
• PUBLISH.md:50-51 documents this trade-off.

---

🔒 Security: ⚠️ 1 warning

The crates.io token is now present while cargo publish runs build verification.

Recommendation: 📝 Minor Comments

• [WARNING] cargo publish without --no-verify runs build scripts and proc-macros, including the BoringSSL build in boring-sys, while CARGO_REGISTRY_TOKEN is in the environment. The old workflow avoided this on purpose. (.github/workflows/publish.yml:74)

View full review

• Token exposure: .github/workflows/publish.yml:74 dropped --no-verify. Cargo now compiles each crate before uploading it, with CARGO_REGISTRY_TOKEN set. That runs the BoringSSL build in boring-sys and every transitive build script and proc-macro.
• The token is short-lived and the code comes from master, so the risk is limited. Still, a compromised build dependency could take the token and publish any of the four crates before it expires.
• Input handling: expected_version is only passed through environment variables and jq --arg, so it cannot be used for shell injection.

---

✅ All other checks passed

📖 AGENTS.md: ✅ LGTM

No agent instruction files are affected.

---

⚡ Performance: ✅ LGTM

No performance concerns.

---

📝 Documentation: ✅ LGTM

PUBLISH.md clearly describes the one-time setup, the release steps and the rerun behaviour. Its rerun claim depends on the duplicate-upload detection, which is covered in the code quality section.

---

_{Reviewed commit: 153f137c}

[clanky-the-clanker]

Superseded by the review of 7603fba2.

This review, of 13cf3f84

💬 Approved with comments — Two warnings remain from the last review: rerun detection probably doesn't match current cargo output, and the registry token is set during verification builds. The other items are minor.

Findings: 2 warnings · 1 info · 1 suggestion (2 are inline comments on the diff)

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #547.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (2)

### [Security] `cargo publish` without `--no-verify` runs build scripts and proc-macros, including the BoringSSL build in `boring-sys`, while `CARGO_REGISTRY_TOKEN` is in the environment. The old workflow avoided this on purpose.
- **File:** .github/workflows/publish.yml:72

### [Code Quality] The duplicate-upload regex only matches cargo's old `api errors ... is already uploaded` wording. Current cargo reports `crate <name>@<version> already exists on crates.io index`, so a rerun of a partial release probably fails instead of continuing as PUBLISH.md says.
- **File:** .github/workflows/publish.yml:74

## INFO (1)

### [Release] Reruns accept an already-published version without checking its checksum or yank status (documented in PUBLISH.md), so a mixed release could be reported as a success.
- **File:** .github/workflows/publish.yml:73

## SUGGESTION (1)

### [Code Quality] If the tag is missing, `gh api` fails with a bare 404. The error message also calls HEAD "the master tip", which is wrong if master has moved since dispatch; "the checked-out commit" would be accurate.
- **File:** .github/workflows/publish.yml:57

---

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

The duplicate-upload regex probably doesn't match current cargo output, and the tag-check message needs work.

Recommendation: 📝 Minor Comments

• [WARNING] The duplicate-upload regex only matches cargo's old api errors ... is already uploaded wording. Current cargo reports crate <name>@<version> already exists on crates.io index, so a rerun of a partial release probably fails instead of continuing as PUBLISH.md says. (.github/workflows/publish.yml:74)
• [SUGGESTION] If the tag is missing, gh api fails with a bare 404. The error message also calls HEAD "the master tip", which is wrong if master has moved since dispatch; "the checked-out commit" would be accurate. (.github/workflows/publish.yml:57)

View full review

Duplicate-upload detection (.github/workflows/publish.yml:74)

• The regex expects ^error: api errors ... crate version pkg@​x is already uploaded. That is the older cargo wording for a server error.
• The workflow uses the stable toolchain. Current cargo checks the index before uploading and fails with crate <name>@<version> already exists on crates.io index, which does not match the regex.
• So a rerun of a partly finished release most likely exits at the first crate that is already published. PUBLISH.md:49-50 promises that this case works.
• Fix: match the already exists on crates.io index form as well, or check the version on crates.io before publishing.

Tag check (.github/workflows/publish.yml:57-59)

• A missing tag gives a bare gh api 404 with no explanation.
• The message calls HEAD "the master tip". HEAD is really the commit checked out at dispatch, and master may have moved since.

---

🚀 Release: 💬 1 info

Reruns accept already-published versions without checking them (documented trade-off).

Recommendation: 💬 Optional Nits

• [INFO] Reruns accept an already-published version without checking its checksum or yank status (documented in PUBLISH.md), so a mixed release could be reported as a success. (.github/workflows/publish.yml:73)

View full review

Risk: Low

• .github/workflows/publish.yml:72-77 treats a duplicate-upload error as success. It does not check that the existing crate matches this release's archive, or that it isn't yanked. PUBLISH.md:50-51 documents this trade-off.
• The rerun-detection mismatch is covered in the code quality section.

---

🔒 Security: ⚠️ 1 warning

The registry token is set while cargo runs verification builds.

Recommendation: 📝 Minor Comments

• [WARNING] cargo publish without --no-verify runs build scripts and proc-macros, including the BoringSSL build in boring-sys, while CARGO_REGISTRY_TOKEN is in the environment. The old workflow avoided this on purpose. (.github/workflows/publish.yml:72)

View full review

• Token exposure (.github/workflows/publish.yml:67-72): The Publish crates step sets CARGO_REGISTRY_TOKEN and runs cargo publish without --no-verify. Cargo then builds each packaged crate before uploading it, which runs boring-sys's BoringSSL build.rs and every dependency's build scripts and proc-macros with the token in the environment.
• The token is short-lived and the source comes from master, so the risk is limited. Still, a compromised build dependency could take the token and publish any of the four crates.
• Input handling: expected_version only reaches the shell through environment variables and jq --arg, so I found no injection path.

---

✅ All other checks passed

📖 AGENTS.md: ✅ LGTM

This pull request does not add or change any agent instruction files.

---

⚡ Performance: ✅ LGTM

No performance concerns. The release is a rare manual run of four sequential cargo publish calls.

---

📝 Documentation: ✅ LGTM

PUBLISH.md clearly describes the setup and the release steps. Its rerun claim (PUBLISH.md:49-50) depends on the duplicate-upload detection covered in the code quality section. Its statement that CI rejects mismatched dependency requirements is accurate: the Cargo.toml boring versions consistency step in ci.yml enforces it.

---

_{Reviewed commit: 13cf3f84}

[clanky-the-clanker]

💬 Approved with comments — The token-exposure and tag-message issues from the last review are fixed. One WARNING remains: the duplicate-upload pattern requires backticks that cargo's current "already exists" message doesn't have, so the rerun path documented in PUBLISH.md probably still fails.

Findings: 1 warning (1 is an inline comment on the diff)

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #547.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Code Quality] The duplicate-upload pattern requires backticks around `pkg@version`, but cargo's current `crate <name>@<version> already exists on crates.io index` message has none. A rerun of a partial release probably fails at the first crate that is already published, which contradicts PUBLISH.md:49-50.
- **File:** .github/workflows/publish.yml:77

---

📋 Code Quality: ⚠️ 1 warning

The duplicate-upload pattern still doesn't match cargo's current "already exists" wording. The tag-check message is fixed.

Recommendation: 📝 Minor Comments

• [WARNING] The duplicate-upload pattern requires backticks around pkg@version, but cargo's current crate <name>@<version> already exists on crates.io index message has none. A rerun of a partial release probably fails at the first crate that is already published, which contradicts PUBLISH.md:49-50. (.github/workflows/publish.yml:77)

View full review

Duplicate-upload detection (.github/workflows/publish.yml:77)

• The pattern puts backticks around pkg@version for both alternatives.
• That fits the old server error, crate version `pkg@x` is already uploaded.
• Current cargo checks the index first and prints crate <name>@<version> already exists on crates.io index, with no backticks.
• So the new alternative most likely never matches. A rerun of a partial release would then exit at the first crate that is already published, which contradicts PUBLISH.md:49-50.

Since the last review

• The tag lookup now prints a clear error when the tag is missing (publish.yml:57-60).
• The mismatch message now calls HEAD "the checked-out commit".
• One earlier note, that a rerun doesn't check the contents or yank state of an already-published version, is still open. PUBLISH.md:50-51 documents it, so it is the author's choice and isn't repeated here.

---

✅ All other checks passed

🚀 Release: ✅ LGTM

Risk: Low

• Publishing uses trusted publishing, runs only from master, and requires the tag to point at the checked-out commit.
• The rerun-detection mismatch is covered in the code quality section.

---

🔒 Security: ✅ LGTM

• Token exposure, fixed: cargo publish now runs with --no-verify (publish.yml:75), so no build script or proc-macro runs while CARGO_REGISTRY_TOKEN is set.
• Input handling: expected_version reaches the shell only through environment variables and jq --arg, and it is quoted everywhere it is used.

---

📖 AGENTS.md: ✅ LGTM

This pull request does not add or change any agent instruction files.

---

⚡ Performance: ✅ LGTM

No performance concerns. The release is a rare, manual run of four sequential cargo publish calls.

---

📝 Documentation: ✅ LGTM

PUBLISH.md clearly describes the setup and the release steps. Its rerun claim (PUBLISH.md:49-50) depends on the duplicate-upload detection covered in the code quality section.

---

_{Reviewed commit: 7603fba2}

[clanky-the-clanker]

⚠️ WARNING · Code Quality

The duplicate-upload pattern requires backticks around pkg@version, but cargo's current crate <name>@<version> already exists on crates.io index message has none. A rerun of a partial release probably fails at the first crate that is already published, which contradicts PUBLISH.md:49-50.

Both alternatives in this pattern sit after `${package}@[^`]+`, so each one needs the crate and version wrapped in backticks. That fits the old crate version `pkg@x` is already uploaded error. Current cargo checks the index before uploading and prints crate boring-sys@X already exists on crates.io index with no backticks, so that form never matches. A rerun after a partial release then exits 1 at the first crate that is already published. Make the backticks optional, or check on crates.io whether the version exists before publishing.

Suggested change

	if ! grep -qE "\`${package}@[^\`]+\` (is already uploaded|already exists on crates.io index)" <<<"${output}"; then
	if ! grep -qE "\`?${package}@[^\\\` ]+\`? (is already uploaded|already exists on crates.io index)" <<<"${output}"; then