Skip to content

pr-pipeline: let pr-unblock revert a file mode the pull request itself introduced #10080

Description

@kyle-sexton

Problem

pr-unblock's address-feedback mode edits file contents only, so a review finding about a file mode it introduced cannot be fixed and escalates feedback-uncertain.

Example: #10052 changed plugins/docs-hygiene/skills/audit-noise/scripts/lib/noise-shapes.sh from 100644 to 100755. claude-review flagged it; the lane agreed ("nothing needs it to be executable") but replied "It cannot run git or chmod" and escalated. A person-run git update-index --chmod=-x fixed it.

Owner decision (2026-10-25)

A stop is worth a person only for security, intent, or a real charge. Reverting a mode bit the same PR introduced is none of those.

Fix

Let the lane's patch output carry a mode change for a path the PR itself changed the mode of, restoring the base's mode only (never adding the executable bit). Apply it in the existing patch path (.github/scripts/apply-patch/apply-patch.mjs), not a second writer. Adding an executable bit stays out of scope: high-path mode changes remain a security hold.

Done when

  • A finding asking to revert a PR-introduced mode change is fixed by the lane on a test PR, with no needs-human.
  • A patch that would set an executable bit not on the base is refused.

Activity

  1. added
    needs-triageNot yet classified. Floor until a type and one priority tier are set.
    on Oct 25, 2026
  2. github-actions commented on Oct 25, 2026

    @github-actions
    Contributor

    Proposed work class: work-class: scoped
    Basis: automated intake triage; the lanes App applied this class.

    Size: Medium
    Complexity: Medium
    Uncertainty: Known
    Flags: none
    Blocked by: none

    Decision recorded. The owner answered in the Refinement session (2026-10-25), deciding the open items below on the facts:

    go with whatever the facts/data/metrics + /discovery:research says and ensure we're aligned with SOLID, DRY/SSOT principles

    [quote truncated; the full decision is at the link]

    #10080 (comment)

    The decision settles all five open questions. The lane gets a narrow chmod -x grant. apply-patch gets an always-on refusal of any new executable bit, except for the issue row's script-exec-bit. A PR-introduced mode revert is admitted only for paths the kept-threads predicate admits. The babysit_risk.py rating stays as it is.

    Automated triage, applied by the lanes App.

  3. added
    priority: mediumReal value, no hard deadline; normal backlog flow.
    and removed
    needs-triageNot yet classified. Floor until a type and one priority tier are set.
    on Oct 25, 2026
  4. added
    status: readyTriaged, unblocked, and fully specified; eligible to pick up.
    work-class: scopedA briefed fix or small feature; blast radius bounded by the brief, tests exist.
    status: needs-decisionAwaiting a human or maintainer judgment call.
    and removed
    status: readyTriaged, unblocked, and fully specified; eligible to pick up.
    on Oct 25, 2026
  5. melodic-automation-lanes commented on Oct 25, 2026

    @melodic-automation-lanes
    Contributor

    Not admitted to the issue lane: no trusted-human acceptance criteria: a listed human must put the confirmed - [ ] criteria in the body or post them as their own comment, or, on an issue a listed human filed, intake's newest refinement comment must hold them unedited. Admission takes the issue once each of those is met, when intake triages it again (.github/actions/resolve-config/admission-rules.json).

  6. melodic-automation-lanes commented on Oct 25, 2026

    @melodic-automation-lanes
    Contributor

    The latest triage verdict holds nothing: the issue is at status: ready.

  7. melodic-automation-lanes commented on Oct 25, 2026

    @melodic-automation-lanes
    Contributor

    Refinement. A read-only run explored this repository, searched its issues and pull requests, and read the allowed docs to draft what follows. On an issue a listed human filed, it counts as the issue's spec once posted, while unedited; it is still model text, so check it.

    Intended outcome: apply-patch.mjs refuses any patch entry that sets 100755 where the PR base had 100644, and the review-threads lane has a way to emit a PR-introduced mode revert that apply-patch accepts.

    Related work

    Open questions

    1. How should the lane express a mode revert, given collect-patch.sh builds patch.diff from the working tree and the lane cannot run chmod or git?
      RECOMMENDED: Grant the review-threads lane a narrow chmod -x tool rule, so collect-patch.sh emits the old mode/new mode lines itself and apply-patch.mjs only validates them. Basis: .base/.github/scripts/read-patch/collect-patch.sh:82 (the patch is a git diff of the staged working tree, so the model cannot write a mode line directly); .base/.github/actions/collect-base-activity/action.yml:692 (an existing Bash(chmod +x *) grant for commit-capable lanes)
    2. Which patch-rules.json rows should get the new mode check?
      RECOMMENDED: Every row, as a new always-on or listed refusal, so no row can set an executable bit the base lacks. Exempt the issue row only for its existing script-exec-bit behavior. Basis: .base/.github/actions/resolve-config/patch-rules.json:63-72 (the issue row adds exec bits itself via script-exec-bit); .base/.github/scripts/apply-patch/apply-patch.mjs:870-882
    3. What counts as a mode the PR itself introduced, and where does apply-patch get the base mode from?
      RECOMMENDED: A path whose mode in the merge-base of the PR head and its base ref differs from the head's mode, and a patch entry that sets exactly the merge-base mode. The applier needs the base ref, which it already receives as BASE_REF. Basis: judgment; .base/.github/scripts/apply-patch/apply-patch.mjs:45-55 lists BASE_REF as optional and PATCH_BASE_SHA as issue-only, so the PR target has no pinned base commit yet
    4. Should a mode-only change be admitted when the kept-threads predicate would otherwise exclude the path?
      RECOMMENDED: No. Admit it only for a path the kept-threads predicate already admits, since the finding names that file. Basis: .base/.github/scripts/apply-patch/predicates.mjs:14-23
    5. The issue text says high-path mode changes remain a security hold. Should that high-path rating in babysit_risk.py change?
      RECOMMENDED: No. Leave the risk rating alone, and have the lane's mode revert handled only inside apply-patch. Basis: .base/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py:413-436 (any mode-type change already rates HIGH); the issue's Fix section limits the change to apply-patch.mjs

    Sources read

    • .github/scripts/apply-patch/apply-patch.mjs:118-128: the REFUSALS table has no mode check, so a patch setting 100755 passes the refusals today.
    • .github/scripts/apply-patch/apply-patch.mjs:870-882: only the issue row (script-exec-bit) sets an exec bit.
    • .github/scripts/apply-patch/apply-patch.mjs:889-896: tree entries are written with entry.newMode, so a mode-only diff entry is already carried through.
    • .github/scripts/read-patch/collect-patch.sh:82-86: patch.diff is a git diff --cached --binary of the working tree, so the model needs a way to change a mode.
    • .github/actions/resolve-config/patch-rules.json:13-22: the review-threads row allows kept-threads paths only and sets no mode rule.
    • .github/scripts/apply-patch/predicates.mjs:14-23: the kept-threads predicate decides which paths are admitted.
    • .github/actions/collect-base-activity/action.yml:692: an existing chmod +x tool grant for commit-capable lanes.
    • plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py:413-436: mode-type changes rate high-path.

    Automated refinement by the lanes App. Answer with a comment whose first line is **Decision recorded.**.

  8. kyle-sexton commented on Oct 26, 2026

    @kyle-sexton
    MemberAuthor

    Decision recorded. The owner answered in the Refinement session (2026-10-25), deciding the open items below on the facts:

    go with whatever the facts/data/metrics + /discovery:research says and ensure we're aligned with SOLID, DRY/SSOT principles

    I should only ever be in the loop ONLY when necessary (intent/vision changed, something came up during implementation that impacts trajectory/alignment, OR a high security risk) - I've covered this all before

    Decided on the facts, as net hardening: lanes already hold Bash(chmod +x *) on the commit path (.github/actions/collect-base-activity/action.yml:692); chmod -x can only remove an executable bit; and apply-patch has no refusal for a new executable bit today (.github/scripts/apply-patch/apply-patch.mjs:117-129, EXECUTABLE defined at :84 but unused by any refusal). The change adds that refusal, so the lane ends with less power to set modes than it has now.

    Question asked: Grant the review-threads lane a narrow chmod -x and add an always-on "no new executable bit" refusal in apply-patch (trust-boundary call)?

    The body's owner decision (2026-10-25) settles the intent:

    Reverting a mode bit the same PR introduced is none of those.

    Open items come from intake's refinement: #10080 (comment)

    1. How does the lane express a mode revert, given the patch is a git diff of the working tree and the lane cannot run chmod or git? Trust-boundary call: it adds a tool grant to a lane.
      Decided: Grant the review-threads lane a narrow Bash(chmod -x *), so collect-patch.sh emits the mode lines itself and apply-patch.mjs only validates them. Basis: origin/main .github/scripts/read-patch/collect-patch.sh:84 (diff --cached --binary); .github/actions/collect-base-activity/action.yml:692 (the existing Bash(chmod +x *) grant). Intake's answer kept.
    2. Which patch-rules.json rows get the new mode check? Security call.
      Decided: Every row, as an always-on refusal, so no row can set an executable bit the base lacks; the issue row keeps its script-exec-bit behavior. Basis: .github/scripts/apply-patch/apply-patch.mjs:118-131 (REFUSALS has no mode check today), :870-873 (script-exec-bit). Intake's answer kept.
    3. What counts as a mode the PR introduced, and where does apply-patch get the base mode?
      Decided: A path whose mode at the merge base of head and base ref differs from the head's, and a patch entry that sets exactly the merge-base mode, read through BASE_REF. Basis: judgment; apply-patch.mjs:46-58 (BASE_REF optional, PATCH_BASE_SHA issue-only). Intake's answer kept.
    4. Is a mode-only change admitted for a path the kept-threads predicate excludes?
      Decided: No; only a path the kept-threads predicate admits. Basis: .github/scripts/apply-patch/predicates.mjs:13-24. Intake's answer kept.
    5. Does the high-path rating for mode changes in babysit_risk.py change?
      Decided: No. Basis: plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py:88 (a file mode change rates high-path); the body's Fix limits the change to apply-patch.mjs. Intake's answer kept.

    Acceptance criteria:

    • A finding asking to revert a PR-introduced mode change is fixed by the lane on a test PR, with no needs-human. (Converted from the body's Done when, wording unchanged.)
    • A patch that would set an executable bit not on the base is refused. (Converted from the body's Done when, wording unchanged.)
    • The refusal applies to every patch-rules.json row except the issue row's script-exec-bit, with a test case.
    • The mode change is applied in apply-patch.mjs, with no second writer, and babysit_risk.py's mode rating is unchanged.

    Builder note: the existing Bash(chmod +x *) grant sits on collect-base-activity's commit path. Confirm the new refusal leaves that path working for a lane that commits a new #! script there; a test with a new .sh on the commit path settles it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority: mediumReal value, no hard deadline; normal backlog flow.status: readyTriaged, unblocked, and fully specified; eligible to pick up.work-class: scopedA briefed fix or small feature; blast radius bounded by the brief, tests exist.

    Fields

    No fields configured for Feature.

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions