Stop hard-coding the previous checkout SHA in lint workflow tests

[justin808]

Why

The lint workflow test encoded one historical actions/checkout commit, so a legitimate dependency update failed Validate even when the repository security policy accepted the new immutable pin.

Fixes #605.

What changed

• Parse the lint workflow structurally instead of asserting one old checkout SHA.
• Require checkout to exist before the canonical bin/lint step.
• Require persist-credentials: false.
• Leave immutable external-action pin validation with the repository security policy contract.
• Add a regression that substitutes another 40-character checkout revision and keeps the structural contract green.

Validation

• TDD red reproduced against PR #301s updated checkout head.
• Full bin/validate completed with exit 0 and PASS agent-workflows validation before current-main reconciliation.
• Current-main focused lint workflow contract: 12 runs, 117 assertions, green.
• Repository security policy: 16 runs, 45 assertions, green, 2 expected skips.
• Prior focused secure-actions coverage: 76 runs, 322 assertions, green, 2 expected skips.
• RuboCop and diff check: clean.

Hosted exact-head Lint, Validate, Claude review, and CodeRabbit checks are green on 84d732191207b5f9c300dcb7f0de34de57cb37eb; all review threads are resolved.

Summary by CodeRabbit

• Tests
  • Improved automated validation of the lint workflow to confirm checkout steps run in the correct order and credential persistence is disabled.
  • Expanded coverage for workflow variations, including rewritten action references, capitalization differences, and YAML aliases.
  • Removed brittle checks tied to specific revision strings, making workflow validation more reliable.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 89d28458-88b1-43c2-a63c-c8c00b644df0

📥 Commits

Reviewing files that changed from the base of the PR and between 27bf0d3 and 74a902e.

📒 Files selected for processing (1)

• bin/lint-test.rb

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

• 🔍 Trigger review

Walkthrough

The lint test now parses the workflow YAML and validates checkout ordering, disabled credential persistence, and checkout action references without hard-coding one checkout SHA. Additional tests cover rewritten references, case variants, and YAML anchors and aliases.

Changes

Checkout contract validation

Layer / File(s)	Summary
YAML workflow contract parser bin/lint-test.rb	Adds YAML support, permitted timestamp classes, alias handling, checkout action matching, and assertions for checkout ordering and persist-credentials: false.
Contract test cases bin/lint-test.rb	Replaces literal SHA assertions and adds coverage for rewritten checkout references, case variants, YAML anchors, aliases, and the canonical lint command.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 74a90

This change makes lint workflow tests resilient to legitimate checkout-pin updates while preserving the existing security checks; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description explains the problem, scope, linked issue, implementation, and validation results. It does not follow the required template because it omits the ## How to review and verify and `## T…	Add the required ## How to review and verify and ## Test plan sections. Record the required bin/validate, bin/lint, behavior-level check, and changelog classification entries as checked items with exact commands and observed results…

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue #605 by testing checkout behavior structurally, requiring checkout before bin/lint, requiring persist-credentials: false, accepting revised immutable pins, and leaving immuta…
Out of Scope Changes check	✅ Passed	The changes are limited to updating lint workflow tests and adding regression coverage for the checkout contract. They align with issue #605 and introduce no unrelated changes.
Title check	✅ Passed	The title clearly identifies the main change: removing the historical checkout SHA from lint workflow tests.

Full details: Linked Issues check

Explanation

The changes satisfy issue #605 by testing checkout behavior structurally, requiring checkout before bin/lint, requiring persist-credentials: false, accepting revised immutable pins, and leaving immutable action-reference validation to the secure-actions suite.

Full details: Description check

Explanation

The description explains the problem, scope, linked issue, implementation, and validation results. It does not follow the required template because it omits the ## How to review and verify and ## Test plan sections, including the required merge-gate checklist and changelog classification.

Resolution

Add the required ## How to review and verify and ## Test plan sections. Record the required bin/validate, bin/lint, behavior-level check, and changelog classification entries as checked items with exact commands and observed results. Move the existing validation details into the appropriate sections or into the Agent details disclosure.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/issue-605-lint-checkout-pin

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Review

Solid, well-scoped fix. Summary of what I checked:

Correctness

• assert_lint_workflow_checkout_contract correctly locates the checkout step via uses.start_with?("actions/checkout@") and the lint step via run == "bin/lint" (verified this matches .github/workflows/lint.yml:71-72 exactly), then asserts ordering and persist-credentials: false via dig, which safely returns nil (not a NoMethodError) when the with: key is absent.
• The new regression test (test_ci_checkout_contract_does_not_pin_one_historical_revision) substitutes the checkout SHA with a * 40 and re-runs the same structural contract - this is exactly the right way to prove the contract no longer cares about the specific historical revision.
• Delegating immutable-pin validation to RepositorySecurityPolicyTest#test_github_actions_are_pinned_to_full_commit_shas (and asserting bin/validate still wires that test in) is a clean separation of concerns - confirmed that test already enforces full-SHA pinning across all workflows, so no coverage is lost, just relocated to where it belongs.
• This mirrors an existing, established pattern already used in bin/push-downstream-test.rb, so it is consistent with the codebase existing conventions rather than introducing a new idiom.

No bugs or security issues found. YAML.safe_load usage is safe here (no aliases/custom types in the workflow file), and the regex substitution in the regression test only needs to hit the single checkout occurrence via sub.

Minor, non-blocking observation: test_ci_installs_pinned_linters_and_runs_the_canonical_command still hardcodes exact SHAs for ruby/setup-ruby, actions/setup-node, and actions/setup-python. Since the whole point of this PR is to stop a legitimate dependency bump from failing Validate, the same brittleness could resurface the next time Dependabot bumps one of those three pins. Not blocking this fix (out of scope per issue 605), but might be worth a follow-up applying the same structural-contract approach to those steps if that class of churn shows up again.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-01T08:22:42.398435Z	84d7321	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e73321514

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

I independently re-verified this fix (structural checkout-contract parsing, ordering assertion, persist-credentials: false check via dig, and the SHA-substitution regression test) against the current HEAD (7e73321) and it holds up — no bugs, no security regression. RepositorySecurityPolicyTest#test_github_actions_are_pinned_to_full_commit_shas already enforces full-SHA pinning across all workflows including lint.yml, so delegating that concern away from this test doesn't lose coverage.

This matches the analysis in the existing review comment on this PR, which I concur with, including the non-blocking observation that ruby/setup-ruby, actions/setup-node, and actions/setup-python are still asserted via hardcoded SHAs in test_ci_installs_pinned_linters_and_runs_the_canonical_command and could hit the same brittleness on their next bump (out of scope here, potential follow-up).

Left one additional minor nit inline: the YAML.safe_load call in the new helper doesn't pass aliases: true/permitted_classes: like the codebase's existing load_yaml_file convention does — low risk today, but worth aligning for consistency.

No blocking issues.

[claude]

Review

Solid fix for the underlying problem: pinning the lint workflow contract test to one literal actions/checkout SHA meant every legitimate Dependabot pin bump broke Validate. The new structural approach (parse the workflow YAML, assert a checkout step exists before bin/lint runs, and require persist-credentials: false) is the right fix, and responsibility is cleanly split from bin/repository-security-policy-test.rb, which still separately enforces that every action reference (including actions/checkout) is pinned to a full 40-char commit SHA via test_github_actions_are_pinned_to_full_commit_shas.

Correctness

• test_ci_checkout_contract_does_not_pin_one_historical_revision is a good regression test — it substitutes an arbitrary 40-hex-char checkout SHA and confirms the contract still passes, directly proving the original bug (Stop hard-coding the previous checkout SHA in lint workflow tests #605) can't recur.
• test_ci_checkout_contract_matches_policy_aliases_and_yaml_features reasonably exercises case-insensitive action identity matching and YAML anchor/alias steps, mirroring the case-aliasing coverage already in bin/repository-security-policy-test.rb.
• checkout_action_reference?'s exact-identity match (identity.casecmp?("actions/checkout")) correctly avoids false-positives like actions/checkout-helper or actions/checkout/subpath, consistent with the sibling policy test.
• I wasn't able to execute ruby bin/lint-test.rb / ruby bin/repository-security-policy-test.rb in this review sandbox to confirm green locally, so this review is based on static reading of the code and cross-referencing against the existing policy test's equivalent assertions — worth a final CI check before merge.

Quality nit (posted inline)

• assert_lint_workflow_checkout_contract / checkout_action_reference? in bin/lint-test.rb are byte-identical duplicates of logic already in bin/repository-security-policy-test.rb. Since the file comment explicitly calls out that the two tests divide ownership of the checkout contract, sharing the matching helper (rather than copy-pasting it) would prevent the two contracts from silently drifting apart if the identity-matching rule is ever refined.

Process note

• The PR description says "This PR remains draft until hosted exact-head validation completes," but the PR is currently open (not draft) and shows as mergeable. Worth confirming that hosted validation has actually completed before merging, or re-marking as draft if it hasn't.

No security or performance concerns — this is entirely test-code scoped to CI contract validation, and the change reduces coupling to a specific commit SHA rather than introducing new external inputs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74a902ef68

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

Overview

This PR replaces a brittle, hard-coded actions/checkout@<sha> string assertion in bin/lint-test.rb with a structural YAML-based contract: it parses .github/workflows/lint.yml, locates the checkout step by identity (actions/checkout, case-insensitive), and asserts (a) it occurs before the first bin/lint-invoking run step, and (b) persist-credentials: false is set. Immutable-SHA-pin enforcement is correctly left to bin/repository-security-policy-test.rb::test_github_actions_are_pinned_to_full_commit_shas, which already owns that contract — I verified this test exists and does enforce full-SHA pinning, so removing the duplicate hard-coded-SHA assertion here doesn't create a coverage gap.

Correctness

I traced through all three new tests by hand against the actual lint.yml step ordering (Checkout → Ruby → Node → Python → Install package linters → Install binary linters → Run repository lint):

• test_ci_checkout_contract_does_not_pin_one_historical_revision — substitutes a fake 40-char SHA and confirms the contract is agnostic to the actual pin value (correctly delegating pin validation elsewhere).
• test_ci_checkout_contract_matches_policy_aliases_and_yaml_features — exercises Actions/Checkout@... case-insensitivity and YAML anchor/alias (&checkout / *checkout) resolution, guarding against a regression to aliases: true or the identity check being dropped.
• test_ci_checkout_precedes_the_first_repository_dependent_lint_command — this one is subtle but correct: lint_index matches on any run step containing \bbin/lint\b, which actually first matches inside "Install package linters" (bin/lint --version rubocop), not the final "Run repository lint" step. Moving checkout to just before "Install binary linters" therefore correctly lands it after that first match, and the test asserts the contract now fails. Good negative-test design, though the "first repository-dependent lint command" framing may be non-obvious to future readers — worth a comment noting that bin/lint --version ... invocations count too, not just the final lint run.

Code quality

• Left an inline note: checkout_action_reference? and the YAML_TIMESTAMP_CLASSES / YAML.safe_load(permitted_classes:, aliases: true) setup are duplicated verbatim from bin/repository-security-policy-test.rb. Not a blocker, but extracting a shared helper would prevent the two contracts from silently drifting apart.
• YAML.safe_load is used only against a fixed, repo-controlled file path, so aliases: true doesn't introduce any real YAML-bomb/DoS exposure here.

Risks

• None significant. The PR is currently in draft per its description ("remains draft until hosted exact-head validation completes"), and scope is well-contained to test code with no production/runtime behavior change.

Overall: solid, well-tested fix for the root cause in #605 (test coupling to a specific historical SHA). The structural approach is a clear improvement over the previous brittle string match.