Skip to content

docs(smoke.yml): improve comments - #62

Closed
michen00 wants to merge 1 commit into
docs/smoke-gates-the-aliasfrom
michen00-patch-1
Closed

docs(smoke.yml): improve comments#62
michen00 wants to merge 1 commit into
docs/smoke-gates-the-aliasfrom
michen00-patch-1

Conversation

@michen00

@michen00 michen00 commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Refactor comments for clarity and conciseness in smoke.yml

What changes, and why

Corpus

The corpus is the specification, and both implementations answer to it. Tick what applies.

  • This changes no behavior the corpus specifies.
  • This changes what gets joined, and a case in corpus/ pins the new behavior. The case was written first and failed first.
  • The change makes the tool join more than it did. The section above says what it will not eat.

Checks

  • make check passes, or make test does and this touches no Rust.

Refactor comments for clarity and conciseness in smoke.yml

Signed-off-by: Michael I Chen <michen00.github@gmail.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.40%. Comparing base (bc00d20) to head (1fd39c5).

Additional details and impacted files
@@                     Coverage Diff                     @@
##           docs/smoke-gates-the-alias      #62   +/-   ##
===========================================================
  Coverage                       87.40%   87.40%           
===========================================================
  Files                               3        3           
  Lines                             691      691           
===========================================================
  Hits                              604      604           
  Misses                             87       87           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michen00

michen00 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Closing this in favor of a curated subset, landed directly on #35 whose branch this targeted: 552564c and 458e18a.

What I kept

Four of the sixteen hunks were adopted, and a fifth was reworded rather than adopted.

  • The header's job inventory goes. The names hook, action, build and verify were four job names held in a comment in a third file, and the general claim states the point without them.
  • Three restating clauses go. "A Linux-only smoke would exercise the one least likely to break", "because there is nothing else for it to be", and "failing to install a version nobody has published" each restate the sentence before them.
  • The trusted-publisher warning becomes a rule. This was the clearest improvement in the PR, and it is reworded rather than adopted because "Do not add publishing steps here" names the wrong file. The risk is not a publishing step inside smoke.yml. The risk is that pypi or crates gets moved out of release.yml into a reusable workflow the way smoke was, because that changes the filename the trusted publisher matches on. The comment now says the publishing jobs must stay where they are, and then gives that mechanism.

What I did not keep

The other eleven hunks each replace a reason with a restatement of the line below it. These comments exist to record what the code cannot state.

The clearest case is the actions/checkout comment. It names RUST_BINARY, REQUIRE_RUST_BINARY and REQUIRE_INSTALLED_PACKAGE because a tag published before those switches existed carries a harness that ignores them, and because the first run of this workflow checked out v0.0.1 and tested the checkout without reporting that it had. The replacement, "Check out current repo state (HEAD) so test harness flags are recognized", names neither the switches nor that run. Adding REQUIRE_RUST_BINARY to that list is part of what #35 is for.

The same loss recurs elsewhere:

hunk what the replacement drops
post-publish paragraph that a yank hides a version from resolvers rather than removing it
alias paragraph that moving v0 is reversible where a publish is not, which is what makes the exception safe
weekly schedule the reason the period is a week, which is that what this finds changes slowly
60-day note that a quiet stretch stops the runs and reports nothing
tag pinning that a ref which is not a version tag fails with a message about the resolver, so it is rejected here first
corpus step that taking the corpus from this ref would fail a scheduled run every week between a fix and the release carrying it
toolchain pin that cargo install is the channel, which is what makes the floor the right pin here
runner Python that the wheel names no upper bound on the Python it supports
retry loops that the retry is bounded rather than patient, so a propagation delay cannot become a job that never finishes
checksums that one wrong entry is a broken release for whoever needed that target
matrix the reference to parity, and it adds the three operating system names that the line below already lists

Recent commits here have been reducing comment verbosity, so the direction is right. What those commits removed was commentary and rhetoric, and the mechanism and the failure stayed. Compare 60dfc8f, which dropped the clause "which is why it is not written down here" and kept the whole cargo install consequence.

Two mechanical notes are worth recording. The rewritten comments wrap at about 95 columns where this file wraps at 79. That is inside the limit of 100 in .yamllint, so no check fails, but the file is no longer consistent with itself. Separately, the rewrites say "test suite" and "MSRV" where this file says "the tier" and "the floor".

michen00-patch-1 is left in place. Delete it when convenient.

🤖 Generated with Claude Code

@michen00 michen00 closed this Sep 9, 2026
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Clarify smoke workflow comments

📝 Documentation 🕐 Less than 5 minutes

Grey Divider

AI Description

• Condenses smoke workflow commentary while preserving operational context.
• Clarifies artifact validation, scheduling, release targeting, and platform coverage rationale.
High-Level Assessment

The comment-only refactor is appropriate because it improves readability without altering workflow behavior. No architectural alternative is warranted.

Files changed (1) +31 / -80

Documentation (1) +31 / -80
smoke.ymlCondense and clarify smoke workflow comments +31/-80

Condense and clarify smoke workflow comments

• Rewrites verbose comments to explain the workflow's post-publication role, scheduling, release targeting, artifact installation, and cross-platform testing more concisely. Workflow triggers, steps, and behavior remain unchanged.

.github/workflows/smoke.yml

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (0)   📜 Skill insights (0)
🐞 ⚙ Maintainability (1)

Grey Divider


Remediation recommended

1. Artifact failures can be misdiagnosed 🐞
Description
The comment at line 182 says each installed artifact is tested independently, but both Rust runs
invoke pytest through the wheel's Python interpreter and exercise the wheel alongside the selected
Rust binary. When either Rust-focused step fails, maintainers cannot infer that the named Rust
artifact alone caused the failure, which undermines the stated failure attribution.
Code

.github/workflows/smoke.yml[182]

+      # Run test suite against each installed artifact independently to pinpoint failures.
Evidence
The workflow invokes all three test steps with the wheel's Python interpreter, while the crate and
released-binary steps additionally provide a Rust binary. The harness always creates a Python runner
and appends the Rust runner when present, proving that the latter two executions are combined
wheel-and-Rust tests rather than independent artifact tests.

.github/workflows/smoke.yml[182-204]
tests/test_cli_corpus.py[106-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new comment incorrectly says each installed artifact is tested independently. Clarify that the wheel runs in all three steps, while the latter two steps select a crate-installed or released Rust binary.

## Issue Context
Both Rust-focused steps invoke pytest with `steps.wheel.outputs.python`, and the test harness always adds the Python implementation before optionally adding the selected Rust binary.

## Fix Focus Areas
- .github/workflows/smoke.yml[182-183]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 9 rules
Review mode: ⚖️ Balanced: Although presented as comment refactoring, it modifies comments in a CI workflow covering releases, artifact installation, tags, and trusted publishing, so semantic drift or misleading operational guidance warrants a careful review.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

# nothing else for it to be; what changes is which build answers as the
# Rust one. `REQUIRE_INSTALLED_PACKAGE` is what stops an install step that
# quietly did nothing from leaving the tier testing this checkout.
# Run test suite against each installed artifact independently to pinpoint failures.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Artifact failures can be misdiagnosed 🐞 Bug ⚙ Maintainability

The comment at line 182 says each installed artifact is tested independently, but both Rust runs
invoke pytest through the wheel's Python interpreter and exercise the wheel alongside the selected
Rust binary. When either Rust-focused step fails, maintainers cannot infer that the named Rust
artifact alone caused the failure, which undermines the stated failure attribution.
Agent Prompt
## Issue description
The new comment incorrectly says each installed artifact is tested independently. Clarify that the wheel runs in all three steps, while the latter two steps select a crate-installed or released Rust binary.

## Issue Context
Both Rust-focused steps invoke pytest with `steps.wheel.outputs.python`, and the test harness always adds the Python implementation before optionally adding the selected Rust binary.

## Fix Focus Areas
- .github/workflows/smoke.yml[182-183]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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.

2 participants