Skip to content

Wave-40 C11: blocking signing-readiness gate - #326

Merged
KooshaPari merged 4 commits into
mainfrom
feat/sl-w40-signing-hard
Jul 19, 2026
Merged

Wave-40 C11: blocking signing-readiness gate#326
KooshaPari merged 4 commits into
mainfrom
feat/sl-w40-signing-hard

Conversation

@KooshaPari

Copy link
Copy Markdown
Owner

Summary

  • Promote platform signing-readiness SelfCheck to blocking PR evidence via \signing-hard.yml\ and \ci.yml\ anchor job
  • Add blocking
    elease.yml\ \signing-readiness\ job before Release publish
  • Extend \signing-readiness-check.ps1\ with done/unpaid gate rows and hard CI anchors; add \ ests/signing_hard.rs\
  • Authenticode / notarization credentials remain unpaid per ADR 0003 (no live keys)

Test plan

  • \pwsh ./scripts/signing-readiness-check.ps1 -SelfCheck\
  • \cargo test --test signing_hard --locked\
  • CI \signing hard\ workflow green on PR

Made with Cursor

Promote platform signing-readiness SelfCheck to blocking PR and release evidence via signing-hard.yml, ci.yml/release.yml anchors, and signing_hard.rs. Authenticode and notarization credentials remain unpaid per ADR 0003.
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@KooshaPari, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cd188c5c-30ec-4c5b-96fd-364ba12c1023

📥 Commits

Reviewing files that changed from the base of the PR and between 487f0db and 3c814cd.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml
  • .github/workflows/release.yml
  • .github/workflows/signing-hard.yml
  • docs/ops/signing-readiness.md
  • scripts/signing-readiness-check.ps1
  • tests/signing_hard.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sl-w40-signing-hard
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feat/sl-w40-signing-hard

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: high. Cursor Bugbot was not present on this PR; signing and release CI gate changes exceed the low-risk approval threshold and need human review. No reviewers assigned — the sole code owner is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request establishes a hard blocking CI gate for platform signing readiness by updating the checklist documentation, enhancing the PowerShell verification script, and adding a Rust test wrapper to run the self-check hermetically. The review feedback highlights critical regex matching issues in the PowerShell script that could lead to false positives or silently skipped validations, as well as a cross-platform test failure risk when pwsh is missing on local developer machines.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread scripts/signing-readiness-check.ps1 Outdated
Comment on lines +198 to +200
if ($ciWorkflow -match '(?ms)^ signing-readiness-policy:.*?continue-on-error:\s*true') {
throw "ci.yml signing-readiness-policy job must be blocking (no continue-on-error)."
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The regex (?ms)^ signing-readiness-policy:.*?continue-on-error:\s*true is highly prone to false positives. Because .*? matches any character including newlines (due to (?s)), it will match from signing-readiness-policy: all the way to a continue-on-error: true defined in any subsequent job in ci.yml. This will cause the script to throw a false error even if signing-readiness-policy itself does not have continue-on-error: true.

To fix this, extract the job's block first (similar to how it is done for signing-readiness in release.yml) and then check for continue-on-error within that block only.

if ($ciWorkflow -match '(?ms)^  signing-readiness-policy:\s*\r?\n(?<block>(?:    .*\r?\n)*?)(?=^  [a-z][\w-]+:|\z)') {
    $policyBlock = $Matches['block']
    if ($policyBlock -match 'continue-on-error:\s*true') {
        throw "ci.yml signing-readiness-policy job must be blocking (no continue-on-error)."
    }
}

Comment thread scripts/signing-readiness-check.ps1 Outdated
Comment on lines +168 to +173
if ($release -match '(?ms)^ signing-readiness:\s*\r?\n(?<block>(?: .*\r?\n)*?)(?=^ [a-z][\w-]+:)') {
$signingBlock = $Matches['block']
if ($signingBlock -match 'continue-on-error:\s*true') {
throw "release.yml signing-readiness job must be blocking (no continue-on-error)."
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If the signing-readiness job is positioned at the end of release.yml, the lookahead (?=^ [a-z][\w-]+:) will fail to match because there is no subsequent job starting with two spaces and a word. This causes the validation to silently skip checking continue-on-error for this job.

To make this check robust regardless of the job's position in the file, update the lookahead to also match the end of the string (\z).

if ($release -match '(?ms)^  signing-readiness:\s*\r?\n(?<block>(?:    .*\r?\n)*?)(?=^  [a-z][\w-]+:|\z)') {
    $signingBlock = $Matches['block']
    if ($signingBlock -match 'continue-on-error:\s*true') {
        throw "release.yml signing-readiness job must be blocking (no continue-on-error)."
    }
}

Comment thread tests/signing_hard.rs Outdated
Comment on lines +24 to +32
let output = Command::new("pwsh")
.args([
"-NoProfile",
"-File",
script.to_str().expect("utf-8 script path"),
"-SelfCheck",
])
.output()
.unwrap_or_else(|error| panic!("failed to spawn pwsh for SelfCheck: {error}"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Running cargo test on non-Windows systems (like macOS or Linux) where PowerShell Core (pwsh) is not installed will cause this test to panic and fail.

To improve the developer experience for contributors who may not have pwsh installed locally, you can gracefully catch the NotFound error and skip the test (or print a warning) rather than panicking, while still letting it run and block on CI where pwsh is guaranteed to be present.

    let output = Command::new("pwsh")
        .args([
            "-NoProfile",
            "-File",
            script.to_str().expect("utf-8 script path"),
            "-SelfCheck",
        ])
        .output();

    let output = match output {
        Ok(out) => out,
        Err(ref e) if e.kind() == std::io::ErrorKind::NotFound => {
            println!("pwsh not found, skipping signing_hard test");
            return;
        }
        Err(error) => panic!("failed to spawn pwsh for SelfCheck: {error}"),
    };

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: high. Cursor Bugbot was not present on this PR; signing and release CI gate changes exceed the low-risk approval threshold and need human review. No reviewers assigned — the sole code owner is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: high. Signing and release CI gate changes exceed the low-risk approval threshold; Cursor Bugbot was not present on this PR. Human review is needed; no reviewers assigned because the sole code owner is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@kilo-code-bot

kilo-code-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown

Code Review Summary

Status: 1 Active Finding (1 prior, re-verified) | Recommendation: Approve with fix

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING (active, re-verified on changed code)

File Line Issue
tests/signing_hard.rs 29 cargo test --test signing_hard panics when pwsh is not installed (Linux/macOS contributors) because `.unwrap_or_else(
Prior findings resolved in this incremental pass
  • scripts/signing-readiness-check.ps1 (was line 200): the cross-job ci.yml signing-readiness-policy regex false positive is fixed — commit 3c814cd replaces the (?ms)^ signing-readiness-policy:.*?continue-on-error:\s*true regex with a Get-YamlJobBlock function that extracts the job block first, then tests continue-on-error within it. No longer matches across jobs.
  • scripts/signing-readiness-check.ps1 (was line 173): the release.yml signing-readiness lookahead that silently skipped the check when the job was last in the file is fixedGet-YamlJobBlock defaults $end = $lines.Count and walks to the next top-level job, so last-job positions are handled.
Files Reviewed (6 files)
  • .github/workflows/ci.yml - adds blocking signing-readiness-policy anchor job (OK)
  • .github/workflows/release.yml - adds blocking signing-readiness job before release (OK)
  • .github/workflows/signing-hard.yml - new blocking PR SelfCheck workflow; verified no continue-on-error, triggers on pull_request, invokes script (OK)
  • docs/ops/signing-readiness.md - soft/hard gate matrix + CI scheduling (docs only)
  • scripts/signing-readiness-check.ps1 - new Get-YamlJobBlock helper; both prior regex defects resolved
  • tests/signing_hard.rs - hermetic SelfCheck wrapper; 1 active finding (pwsh-absent panic)

Consider

  • Get-YamlJobBlock now THROWS if signing-readiness-policy is absent from ci.yml (previously the check was "blocking when present"). This is consistent because the PR adds the job, but it makes the job mandatory rather than optional — worth keeping in mind for future edits that might remove it.

Recommendation

Approve with fix: the two PowerShell regex defects from the prior review are resolved by the line-by-line Get-YamlJobBlock extraction. The remaining tests/signing_hard.rs pwsh-absent panic on Linux/macOS should skip gracefully before merge. No new blocking issues were found in this pass.

Previous Review Summary (commit 0478c45)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 0478c45)

Status: 3 Active Findings (from prior review) | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 0

Three prior inline findings from gemini-code-assist[bot] remain active and are reproduced below. This review pass found no additional new issues on changed lines beyond those already reported.

Issue Details (click to expand)

WARNING (active, pre-existing)

File Line Issue
scripts/signing-readiness-check.ps1 200 ci.yml gate regex (?ms)^ signing-readiness-policy:.*?continue-on-error:\s*true matches across jobs (.*? with s flag spans newlines) and throws a false positive if ANY later job sets continue-on-error: true. Extract the job block first, then test within it.
scripts/signing-readiness-check.ps1 173 release.yml signing-readiness block-extraction lookahead (?=^ [a-z][\w-]+:) fails to terminate when that job is last in the file, silently skipping the continue-on-error check. Add `
tests/signing_hard.rs N/A cargo test --test signing_hard panics when pwsh is not installed (Linux/macOS contributors). Catch io::ErrorKind::NotFound and skip instead of panic!.
Files Reviewed (6 files)
  • .github/workflows/ci.yml - anchors signing-readiness-policy smoke job (no new issues beyond line 200 context)
  • .github/workflows/release.yml - adds blocking signing-readiness job before release (no new issues beyond line 173 context)
  • .github/workflows/signing-hard.yml - new blocking PR SelfCheck workflow (verified: no continue-on-error, triggers on pull_request, invokes script)
  • docs/ops/signing-readiness.md - soft/hard gate matrix + CI scheduling (docs only)
  • scripts/signing-readiness-check.ps1 - extended anchors + block checks (2 active findings above)
  • tests/signing_hard.rs - hermetic SelfCheck wrapper (1 active finding above)

Consider

  • tests/signing_hard.rs is a repo-root integration test. cargo test --test signing_hard requires the repo root to be a Cargo package (not a virtual/workspace-only manifest). Confirm the root Cargo.toml declares a [lib]/[bin] target so the test is discoverable; otherwise the test plan command will fail with "no test target named signing_hard".
  • The release.yml signing-readiness check only asserts the job is blocking; it does not verify needs: [build] or that it runs before release. The needs wiring is correct, but a future edit could silently reorder it.

Recommendation

Approve with fixes: the two PowerShell regex defects (lines 173/200) can produce false CI failures/non-blocking gates and should be corrected; the tests/signing_hard.rs pwsh-absent panic should skip gracefully. No new blocking issues were found in this pass.


Reviewed by hy3:free · Input: 77.1K · Output: 3.8K · Cached: 169K

Replace the catastrophic multiline regex with line-by-line YAML job extraction so Linux SelfCheck completes within the five-minute gate.

Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: high. Signing and release CI gate changes exceed the low-risk approval threshold; Cursor Bugbot was not present on this PR. Human review is needed; no reviewers assigned because the sole code owner is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Comment thread tests/signing_hard.rs
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
output.status.success(),
"signing-readiness-check.ps1 -SelfCheck failed\nstdout:\n{stdout}\nstderr:\n{stderr}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: cargo test --test signing_hard panics when pwsh is not installed (most Linux/macOS contributor machines), because .unwrap_or_else(|error| panic!(...)) does not special-case io::ErrorKind::NotFound.

The PR description lists cargo test --test signing_hard --locked as a test-plan item and the docs claim it runs in the default ci.yml test suite. For contributors without PowerShell Core this hard-panics instead of skipping. Catch NotFound and return (with a println! skip notice) so local runs degrade gracefully while CI (where pwsh is guaranteed) still blocks.

let output = match Command::new("pwsh")
    .args(["-NoProfile", "-File", script.to_str().expect("utf-8 script path"), "-SelfCheck"])
    .output()
{
    Ok(out) => out,
    Err(ref e) if e.kind() == std::io::ErrorKind::NotFound => {
        println!("pwsh not found, skipping signing_hard test");
        return;
    }
    Err(error) => panic!("failed to spawn pwsh for SelfCheck: {error}"),
};

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@KooshaPari
KooshaPari merged commit 2cee1dc into main Jul 19, 2026
72 of 73 checks passed
KooshaPari added a commit that referenced this pull request Jul 19, 2026
Conservative +2 (394→396/402): L111 update-check (#328), L112 signing-hard (#326). Held L79/L40/L7 at pillar max.

Co-authored-by: Cursor <cursoragent@cursor.com>
@KooshaPari
KooshaPari deleted the feat/sl-w40-signing-hard branch August 12, 2026 09:00
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.

1 participant