Skip to content

fix(source-control): hold Pulumi .yml/.json, Terraform JSON and ARM templates as cost - #11395

Merged
kyle-sexton merged 4 commits into
mainfrom
fix-9241-iac-cost-paths
Oct 31, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
fix-9241-iac-cost-paths

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #9241

Summary

The pr-merge cost hold (cost-review-required) missed infrastructure-as-code files that can create billed cloud resources: Pulumi project and stack files in their .yml and .json forms, Terraform's JSON forms, and Azure ARM templates. A change to one of them could merge without the owner's cost decision. This covers the two items the 2026-10-30 decision on #9241 left active: the IaC shapes, and the moved secret reference.

Fix

  • babysit_risk.COST_PATHS gains **/Pulumi.yml, **/Pulumi.json, **/Pulumi.*.yml, **/Pulumi.*.json, **/*.tf.json, **/*.tfvars.json, and the ARM names **/azuredeploy.json, **/azuredeploy.*.json, **/*.azuredeploy.json, **/*.azuredeploy.*.json. Pulumi reads a project file in any of .json, .yml, .yaml (encoding.Exts); the ARM names are the ones the Azure quickstart contribution guide prescribes (azuredeploy.json, azuredeploy.parameters.json, prereq.azuredeploy.json, prereq.azuredeploy.parameters.json).
  • merge-holds.mjs's generated COST_PATHS block is regenerated with scripts/sync-hold-floor.py (not hand-edited); --check passes.
  • pulumi_dirs_at counts a directory holding Pulumi.yaml, Pulumi.yml or Pulumi.json as a Pulumi project; a stack-only file such as Pulumi.prod.yml still does not.
  • Moves: accepted, with the reason in docs/conventions/pr-pipeline/README.md. A secrets reference moved within one file adds no net reference, so secret-exposure-review does not see it, and it adds no charge, so the cost hold does not either; where it moved is the review lanes' call. The existing probe test_a_moved_secret_does_not_hold covers it.
  • An ARM template under any other name is left to the review lanes' cost hold, and the README says so.

Verification

  • Red before the fix: the new probe rows failed on main (11 test_infrastructure_as_code_holds_floor_or_not subtests and PulumiDirs).
  • Green after: test_babysit_risk_cost.py, test_babysit_merge_cost.py, test_merge_holds_sync.py (159 passed); the whole babysit-prs/scripts/tests directory (3681 passed); node --test .github/scripts/pr-merge/merge-holds.test.mjs (65 passed); scripts/sync-hold-floor.py --check clean; scripts/check-affected.sh --tests exit 0 (its suite step left the selected suites to CI).
  • Expected merge-gate state: this PR will likely read verify:repeated-fail at once. The issue-wide cap counts three FAILed heads from earlier PRs for pr-merge cost hold: LOW-rated shapes S&C's probe still passes #9241 (#10525, #10583, and fbe89c5 "Resolve issue pr-merge cost hold: LOW-rated shapes S&C's probe still passes #9241", which no open or closed PR lists now). The capped-close record excuses only a closed PR's own FAILs, so those three never stop counting, and #11294 and #11312 were closed at the cap with "No FAIL findings were counted for this pull request". This change repeats their reviewed-clean approach. It needs a person's merge, or a fix to the cap.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01KeBVgauhM9QMc1vRhujf3L

kyle-sexton and others added 4 commits October 30, 2026 18:38
…emplates as cost

The pr-merge cost hold's COST_PATHS matched only Pulumi.yaml, *.tf,
*.tfvars and Bicep, so an IaC change in Pulumi.yml, Pulumi.json,
*.tf.json, *.tfvars.json or an azuredeploy ARM template merged without
the owner's cost decision. Add those globs to babysit_risk.COST_PATHS
and its generated copy in merge-holds.mjs, count a directory holding any
Pulumi project file as a Pulumi project, and record in the README why a
secrets reference moved within one file stays accepted.

Closes #9241

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KeBVgauhM9QMc1vRhujf3L
…cost

Azure quickstart samples name a prereq parameters file
prereq.azuredeploy.parameters.json, which no COST_PATHS glob matched.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KeBVgauhM9QMc1vRhujf3L
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 30, 2026 23:36
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

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

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T23:37:56.871920Z 3666d21 Draft marked ready
ℹ️ 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.

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed 3666d2109deb4ae4b6aa6375e1c1f661aeb412e6: 0 finding(s), 0 as inline comments.

No findings. I read the diff and PR context only. I didn't run the tests or scripts/sync-hold-floor.py --check, so the passing counts in the PR body are author-claimed and unverified.

Closing issue #9241. The latest decision narrows the issue to two items, and the change meets both:

  • IaC shapes: COST_PATHS in babysit_risk.py and the generated block in merge-holds.mjs add Pulumi .yml and .json, *.tf.json, *.tfvars.json and the ARM azuredeploy names. test_infrastructure_as_code_holds_floor_or_not covers each shape. test_names_near_an_infrastructure_file_do_not_hold covers near-miss names.
  • Moves: the README records the reason for accepting a moved secret reference. The existing probe test_a_moved_secret_does_not_hold (cited in the PR body, not in this diff) covers it.

Checks:

  • The two path lists in babysit_risk.py and merge-holds.mjs match entry for entry, and test_merge_holds_sync.py adds the new paths to its shared PATHS fixture.
  • pulumi_dirs_at now accepts pulumi.yaml, pulumi.yml and pulumi.json case-insensitively. A stack-only file such as Pulumi.prod.yml still doesn't count. The expected tuple ("", "infra/x", "stack", "web") is in sorted order.
  • Docs and docstrings match the new behavior, and I found no other copy of the Pulumi path list in non-test code.
  • The change doesn't loosen any gate. It only widens the cost hold.

Items outside the lane:

  • Trusted paths: merge-holds.mjs, babysit_risk.py, babysit_merge_policy.py and docs/conventions/pr-pipeline/README.md match the trusted-path list. I read their full changes and found no loosening of any gate. A trusted-path match alone doesn't hold.
  • Manifests: the diff changes no plugin or marketplace manifest, so there are no manifest lines to report.
  • Cost: the change only widens the cost hold and enables no paid resource, so I set no hold.
  • Merge gate: the PR body says the issue-wide verify cap will likely read verify:repeated-fail and need a person's merge. I can't confirm that from the diff, and it isn't a code defect.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 3666d2109deb4ae4b6aa6375e1c1f661aeb412e6: 0 finding(s), 0 as inline comments.

Security review of PR #11395 found no security issues. I read the whole diff (9 files), the PR context, and the trusted-path files babysit_risk.py, merge-holds.mjs and babysit_merge_policy.py in full. I did not run the tests or the sync check.

What changes. The cost hold (cost-review-required) now also covers Pulumi.yml, Pulumi.json, Pulumi.*.yml, Pulumi.*.json, *.tf.json, *.tfvars.json, and four ARM template names built on azuredeploy.json. pulumi_dirs_at also treats a directory with a Pulumi.yml or Pulumi.json as a Pulumi project.

Trust controls.

  • Every hunk adds a hold or widens an existing one, so no merge hold is weakened or removed.
  • The generated COST_PATHS block in merge-holds.mjs matches babysit_risk.COST_PATHS entry for entry, and the sync test covers the new rows.
  • pulumi_dirs_at only adds file names to a case-insensitive membership test. It adds no injection or path-traversal surface.
  • Nothing touches the trusted-actor list, a token's scope, the kill switch, or what untrusted text reaches.

Accepted gaps. The PR accepts that a moved secrets reference does not hold, and that an ARM template under another name is left to the review lanes. Neither opens a leak, compromise or charge path that the base did not already have.

PR context. The text has no instruction addressed to the reviewer. Its note that the PR will likely read verify:repeated-fail is a process matter, not a security finding.

Manifests. No plugin.json or marketplace.json change.

Spend. The change spends nothing and cannot cause a charge, so no cost line applies.

critical-findings: 0

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 31, 2026
Merged via the queue into main with commit 2d8f007 Oct 31, 2026
81 checks passed
@kyle-sexton
kyle-sexton deleted the fix-9241-iac-cost-paths branch October 31, 2026 00:32
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.

pr-merge cost hold: LOW-rated shapes S&C's probe still passes

1 participant