Skip to content

feat(source-control): hold a PR that could add or raise a charge for an owner decision - #9224

Merged
kyle-sexton merged 12 commits into
mainfrom
feat/pr-merge-cost-hold
Oct 22, 2026
Merged

kyle-sexton merged 12 commits into
mainfrom
feat/pr-merge-cost-hold

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Closes #9221

Summary

The owner's rule (2026-10-22): anything that would increase cost or spending, or affect billing,
stops for the owner unless a human-recorded decision already covers it. The unattended merge
scorer now holds such a pull request as HIGH cost-review-required in both lanes, whatever
merge.diff-check.high-path-floor says, and clears it only on a cited owner decision.

Fix

  • babysit_risk.py _cost_reasons adds cost-review-required. It is HIGH but not a high-
    code, so _floor_held stays false, the reasons comment says the PR waits for a person, and
    needs-human lands after merge.risk-escalation-minutes. It runs outside the floor gates, the
    lane switches and floor_exempt, because adoption is not an owner decision. It catches:
    • IaC files (Pulumi.yaml, Pulumi.*.yaml, *.tf, *.tfvars, *.bicep, *.bicepparam),
      plugins/multi-agent/reference/defaults.yaml, and any file under a directory that holds a
      Pulumi.yaml at the base (RiskInput.pulumi_dirs, read by pulumi_dirs_at). A base tree
      that can't be listed whole (None) holds every file;
    • a file renamed or copied into a workflow or pr-pipeline.yaml path from outside one;
    • in workflows:
      • an added cron key in any spelling, or a --model or --max-turns flag;
      • any net-new secrets reference (secrets.X, secrets['X'], toJSON(secrets),
        secrets: inherit), a net-new run-claude-cli or claude-code* action or image
        (case-insensitive), or a run line invoking @anthropic-ai/claude-code;
      • a timeout raised on a workflow whose head text starts a model leg;
      • a runner value the head text gains over the base text, or an added inline runs-on::
        on a public repository only when it is not a free literal (standard labels, managed
        self-hosted labels, a self-hosted list), and on any other repository every one. Off a
        public repository, Windows and macOS lines also hold;
    • any pr-pipeline.yaml change unless every added and removed line is on the safe
      allowlist: blank lines, comment-only lines, and description: or title: values. A
      change that only edits trailing comments also passes. Everything else holds, including
      flow style, quoted keys and new list items;
    • a workflow or pipeline file with no patch.
  • babysit_merge_policy.cost_decision: the first body link to a comment on the closing issue or
    the PR itself, in this repository, whose first line starts **Decision recorded.** Cost:, by
    an admin User who is not the agent identity or a gate self login, not posted through an App,
    never edited. A comment on the closing issue must also be created after the PR's
    created_at, which the author cannot forge the way they can commit dates. A comment on the
    PR itself counts as it is. A link inside an HTML comment (closed or not) counts for nothing. Any failed
    read clears nothing. with_capability_contents also reads workflow text. pulumi_dirs_at lists
    Pulumi.yaml directories from the base tree.
  • babysit_merge.score_unattended scores once, then reads decisions only when the cost reason
    fires. It reads workflow text whenever a workflow changed.
  • Docs: the pr-pipeline README merge-authority list, reference/safety.md, the scorer docstring
    and two changelog fragments.

Limits, stated in the README:

  • A decision clears only cost-review-required. In this repository every workflow file and
    docs/conventions/pr-pipeline.yaml is also on the HIGH path floor, so such a PR still waits
    for a person to merge it.
  • A body edit does not remove the human-gated label once it lands. Cite the decision before
    merge.risk-escalation-minutes passes, or a person clears the label.
  • Not detected: budgets, billing settings and new paid services outside these files.
    azure-iac's Pulumi program is C#, and its deploy gate (azure-iac#75) stops any Azure change for
    the owner at deploy time.
  • Dependabot PRs are not scored, so the hold does not apply to them.
  • Accepted: a local session acting as the admin account could post the decision comment itself.
    select-trusted-text.mjs has the same limit.
  • A push after the decision is an accepted LOW residual: the decision is bound to the PR's
    lifetime, not to a head.
  • Shapes S&C rated LOW that still pass are tracked in pr-merge cost hold: LOW-rated shapes S&C's probe still passes #9241.

Verification

  • uv run --with pytest python -m pytest -q tests in
    plugins/source-control/skills/babysit-prs/scripts: 2601 passed, 2077 subtests passed in 67.87s
  • New tests: tests/test_babysit_risk_cost.py (each class holds with the floor off, a cited
    decision clears it, standard runners on a public repository and removed lines do not hold),
    tests/test_babysit_merge_cost.py (each decision condition), and CostHold in
    tests/test_babysit_merge_unattended.py (a cost hit refuses risk:HIGH, a decision clears it,
    a non-cost pull request never reads a decision, a raised timeout holds only on a workflow
    whose read text starts a model leg). Each S&C detection and each finding from poteto's review landed with a test
    that failed before the code (22, then 15, then 43 failures, then green).
    test_babysit_risk.py::test_rename_into_a_high_path_is_high now also expects
    cost-review-required, which is the new rename rule firing.
  • tests/test_babysit_verify_evidence.py: test_a_lane_pr_touching_the_merge_gate_merges_on_a_pass
    gave the workflow file no patch, which now holds as unreadable. It now carries a patch with no
    cost line, so it still proves a non-cost workflow change merges with the floor off.
  • scripts/run-ruff.sh check on the scripts directory and markdownlint on the changed docs: clean.

Related

  • Owner rule relayed by the GitHub Security & Cost session; the brief's NEVER_FLOOR_EXEMPT
    constant does not exist on main, so the reason sits in the scorer beside
    manifest-review-required.
  • This pull request touches plugins/source-control/skills/babysit-prs/**, which is on the HIGH
    floor, so it is itself held for a human merge.

🤖 Generated with Claude Code

kyle-sexton and others added 5 commits October 22, 2026 18:30
…an owner decision

The unattended merge scorer adds a HIGH cost-review-required reason in both
lanes, whatever high-path-floor says: Pulumi, Terraform and Bicep files; a
workflow patch adding a cron line or a larger-runner label, or, off a public
repository, a runs-on or macos line; and a pr-pipeline.yaml patch adding a
model, effort or max-turns line. The gate clears it only when the body links
an unedited **Decision recorded.** comment by an admin User on the closing
issue or the pull request, and reads that comment only for a held PR.

Closes #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…leg timeouts and pipeline spend limits

Adds the four detections Security & Cost asked for to cost-review-required:
a workflow that gains a secrets reference to a model credential or a
run-claude-cli or claude-code-action leg (net of removed lines, so a move or
repin is not new); a timeout-minutes or leg-timeout-minutes raised on a
workflow whose head text starts a model leg, which the gate now reads; and
pr-pipeline.yaml lines on spend keys only (rounds, restarts, turn caps,
per-run and WIP caps, admission, pause-from, leg timeouts, credential route,
model, effort, max-turns). The README names azure-iac's C# program and its
deploy gate as the backstop for Pulumi program edits.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py
#	plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py
poteto's review of the cost hold found ways around it; each now holds, with a
test that failed before the change:
- a file renamed or copied into a workflow or pr-pipeline.yaml path;
- a cron key in flow style or JSON spelling;
- a runner that is not a free literal: the workflow's head text is compared
  with its base text, so an expression, a group: mapping, a labels: entry and
  a larger runner hold on a public repository, and every new runner off one;
  the check reads runs-on values only, and Windows or macOS matrix lines hold
  off a public repository;
- --model and --max-turns flags in a workflow;
- any file under a directory holding a Pulumi.yaml at the base.
The hold stays independent of an adopted author's floor exemption. Tests
cover a null decision node, a node without lastEditedAt, and a record with
no repository visibility.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 6 commits October 22, 2026 19:06
- A cost decision's first line must start `**Decision recorded.** Cost:`,
  the gate's self logins are excluded with the agent identity, and a link
  inside an HTML comment in the body cites nothing.
- pr-pipeline.yaml holds on any change except blank, comment-only,
  description: and title: lines, or an edit to trailing comments only.
- A workflow holds on any net-new secrets reference (dotted, bracket,
  toJSON(secrets), secrets: inherit), any claude-code* action or image and
  run-claude-cli in any case, and a run line invoking @anthropic-ai/claude-code.
- plugins/multi-agent/reference/defaults.yaml is a cost path, and an unlisted
  base tree (pulumi_dirs None) holds every file.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cost decision comment clears the hold only when its created_at is after
the pull request's created_at, which the author cannot forge the way commit
dates can. A missing time on either side clears nothing. A push after the
decision stays an accepted residual.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ssue

A decision comment on the closing issue counts only when created after the
pull request; one on the pull request itself postdates it already and counts
as it is.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 22, 2026 23:20
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 22, 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-22T23:24:08.844304Z 3b0a63a 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

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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

Security review: incremental, head 2795674

No security findings. I reviewed only the five files changed since 3b0a63a, and read the surrounding code in babysit_risk.py and babysit_merge_policy.py for context. All three changes tighten the cost-hold gate.

  • DECISION_LINE (babysit_merge_policy.py#L1850): \s+ became [ \t]+, so Cost: must sit on the same line as the marker. A comment with Cost: on the next line no longer counts as an owner cost decision. The author check and the admin-permission check are unchanged. The new tests cover the next-line and later-line cases.

  • _unsafe_pipeline_lines (babysit_risk.py#L1608): the old check compared the whole file's added and removed lines as multisets. A value swapped between two places in pr-pipeline.yaml (for example max-turns: 60 and max-turns: 10) therefore looked like no change. The new code compares each run of removed lines with the added lines that follow it, in order. Context lines, hunk headers and \ No newline markers end a run. Two things fail toward holding rather than passing:

    • A swap across hunks, or a reorder inside a run, is held.
    • A change that only adds or removes an end-of-file newline is also held, which is a false positive and not a bypass.

    patch is guaranteed to be a str at the call site, because _patch_lines returns None for any non-string and that case is held earlier. So the getattr(raw, "patch", "") call can't crash on None. The --- and +++ prefix skipping matches _patch_lines, so it adds no new gap.

  • docs/conventions/pr-pipeline/README.md only gains the "on the same lines in place" wording, which matches the new behaviour.

  • Tests only add cases. The new swap test holds.

Manifest and instruction-surface lens: no plugin.json or marketplace.json changes in this increment, and no standing instruction was deleted or softened. The doc change narrows a pass condition, which makes the hold stricter. I did not consult the instruction exception register, because nothing here relaxes a rule.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed 27956742fbb607f64bb7cfd7e58d38989ea2a0d0: 0 finding(s), 0 as inline comments.

Incremental review of 2795674 (since 3b0a63a)

No findings. I read the incremental diff and the surrounding code. I did not run the tests: the sandbox refused pytest, so I reviewed the change by reading it.

What I checked

  • DECISION_LINE (babysit_merge_policy.py). [ \t]+ replaces \s+, so the marker and Cost: must share a line. The two new cases in test_babysit_merge_cost.py cover **Decision recorded.**\nCost: and a Cost: on a later line.
  • _unsafe_pipeline_lines(patch) (babysit_risk.py). It now pairs each run of removed lines with the added lines that follow it, and a hunk header or context line ends the run.
    • A value swap between two hunks (the new test_a_value_swap_between_pipeline_profiles_holds) holds, because each run differs on its own.
    • Reordered adjacent lines, and a pure addition or removal of an unsafe line, hold.
    • A \ No newline at end of file marker between the removed and added lines also ends the run. That can only over-hold, so I left it.
    • The existing trailing-comment test at test_babysit_risk_cost.py:156 (- review-rounds: 3 # old note then + review-rounds: 3 # new note) still passes, since both lines sit in one run.
    • The --- and +++ exclusions match _patch_lines.
  • Call site. _patch_lines has already returned a non-None result before the new call, so getattr(raw, "patch", "") is always a str there.

Minor, not counted as a finding

The module docstring at babysit_risk.py:89-90 still says the pipeline hold passes a change that "only edits trailing comments". The README now adds "on the same lines in place". The docstring sits on an unchanged line, so I couldn't anchor a comment to it. Consider adding the same qualifier there.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b0a63a660

ℹ️ 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".

…re pipeline edits in place

Review findings on #9224 at 3b0a63a:
- DECISION_LINE took `\s+` between the decision marker and `Cost:`, so a
  `Cost:` on the next line matched. It now takes spaces or tabs only.
- A pr-pipeline.yaml value swap between two profiles cancelled out in the
  file-wide multiset compare. Each run of removed lines and the added lines
  after it now passes only when they match in order once trailing comments
  are stripped.

Refs: #9221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: hold a PR that could add or raise a charge for an owner decision

1 participant