david/pi-extensions-and-skills

Watch

Fork 0

issue-57: prompt before merging instead of auto-merging PRs #58

Merged

david merged 3 commits from feature/issue-57/skills-must-not-auto-merge-prs into main

2026-08-20 06:48:32 +00:00

yousuf commented

2026-08-20 03:21:04 +00:00

Collaborator

Closes #57

Problem

Skills squash-merged their own PRs, gated only on CI, so a change could reach the
default branch without the human ever seeing it. commit-changes said so explicitly:

Failing or still running → stop and report (fail fast — do not merge).
No second confirmation prompt is used; CI is the merge gate.

CI is a weak proxy for review. In at least one consuming repo the pipeline is
secret-detection only — no build, no tests. "CI green" there means "no credentials
committed", not "this change is correct". The skill merged on that basis, with no human
checkpoint anywhere in the flow.

Behaviour after this PR

Skills never merge on their own initiative. They open the PR and ask.

#	Condition	Action
1	User explicitly asked ("commit and merge it")	Merge, no prompt
2	Otherwise	Ask — report PR URL + CI status, wait for yes/no

Two rules override the prompt:

Never merge without a clear human yes. Silence, ambiguity, or an unanswered prompt
resolves to no. The agent must never answer on the user's behalf.
Non-interactive runs must not hang. pi --print / -p and scripted invocations
skip the prompt and stop at the PR, reporting the URL and merge command.

CI remains a hard gate on every merge path, and the prompt is not offered when CI
is red — the failure is reported instead.

Scope: pi only

This changes how the skills behave under pi. It does not touch, reference, or depend
on any other tool, and leaves other tooling's behaviour exactly as it was.

Earlier commits on this branch had drifted out of that scope — reasoning about another
tool's internals in the skill files, and reading its config file for a merge override.
Commit 31624c6 removed all of it. main had zero such references in these skills, so
this restores the intended separation rather than introducing one.

skills/create-issues/SKILL.md is deliberately untouched: its §2.1 config read predates
this branch and is a separate, pre-existing coupling, out of scope here.

Files

Each skill asserts its merge behaviour in several places that would otherwise
contradict each other, so all sites moved together. The frontmatter description is
the highest-leverage one — it is always in the agent's context and is what drove the
original auto-merging behaviour.

File	Sites
`skills/commit-changes/SKILL.md`	description, overview, Merge policy, Step 5, failure table, both Quick Reference flows, completion report
`skills/commit-docs/SKILL.md`	same treatment, Step 4
`skills/furnish-repo/SKILL.md`	same treatment, §3.7, Phase-2 prompt text, plus the canonical Workflow Conventions block it writes into other repos
`AGENTS.md`	"PRs are squash-merged" now says by whom, and describes the ask

`furnish-repo` has two distinct gates

Its existing Phase-2 yes/no authorises furnishing (create labels, open the PR). That
is now called out in three places as not merge approval — merging is a separate
question asked later, once the PR exists and CI is known. The Phase-2 prompt text says
so to the user: "I'll ask separately before merging anything."

Labels are still created directly, without waiting: they are additive and reversible.
Only the AGENTS.md change waits for a merge.

Commits

Commit
`8231439`	Stop auto-merging; skills stop at the PR
`b62f367`	Revised on owner feedback: prompt to merge rather than always stopping
`31624c6`	Scope to pi only; drop all other-tool coupling

Verification

No cross-tool reference remains in any of the four changed files
Skill frontmatter parses on all three
npm test — 31/31 pass
Every merge assertion site agrees; no stale "does not merge / do not ask" text

Note for the reviewer

Per the policy it introduces, this PR is deliberately left unmerged — it is the
first PR to follow its own rule.

Two follow-ups, not addressed here:

Repos already furnished keep the old Workflow Conventions wording until furnish-repo
is re-run against them.
The prompt wording is specified in the skills but, being Markdown-driven, is not
enforced by a test. Worth checking during review that the phrasing reads well.

Closes #57 ## Problem Skills squash-merged their own PRs, gated only on CI, so a change could reach the default branch without the human ever seeing it. `commit-changes` said so explicitly: > **Failing or still running** → stop and report (fail fast — do not merge). > **No second confirmation prompt is used; CI is the merge gate.** **CI is a weak proxy for review.** In at least one consuming repo the pipeline is *secret-detection only* — no build, no tests. "CI green" there means "no credentials committed", not "this change is correct". The skill merged on that basis, with no human checkpoint anywhere in the flow. ## Behaviour after this PR **Skills never merge on their own initiative. They open the PR and ask.** | # | Condition | Action | |---|---|---| | 1 | User explicitly asked ("commit and merge it") | Merge, no prompt | | 2 | Otherwise | **Ask** — report PR URL + CI status, wait for yes/no | Two rules override the prompt: - **Never merge without a clear human yes.** Silence, ambiguity, or an unanswered prompt resolves to **no**. The agent must never answer on the user's behalf. - **Non-interactive runs must not hang.** `pi --print` / `-p` and scripted invocations **skip the prompt and stop at the PR**, reporting the URL and merge command. **CI remains a hard gate on every merge path**, and the prompt is *not* offered when CI is red — the failure is reported instead. ## Scope: pi only This changes how the skills behave **under pi**. It does not touch, reference, or depend on any other tool, and leaves other tooling's behaviour exactly as it was. Earlier commits on this branch had drifted out of that scope — reasoning about another tool's internals in the skill files, and reading its config file for a merge override. Commit `31624c6` removed all of it. `main` had zero such references in these skills, so this restores the intended separation rather than introducing one. `skills/create-issues/SKILL.md` is deliberately untouched: its §2.1 config read predates this branch and is a separate, pre-existing coupling, out of scope here. ## Files Each skill asserts its merge behaviour in several places that would otherwise contradict each other, so all sites moved together. **The frontmatter `description` is the highest-leverage one** — it is always in the agent's context and is what drove the original auto-merging behaviour. | File | Sites | |---|---| | `skills/commit-changes/SKILL.md` | description, overview, *Merge policy*, Step 5, failure table, **both** Quick Reference flows, completion report | | `skills/commit-docs/SKILL.md` | same treatment, Step 4 | | `skills/furnish-repo/SKILL.md` | same treatment, §3.7, Phase-2 prompt text, **plus the canonical Workflow Conventions block it writes into other repos** | | `AGENTS.md` | "PRs are squash-merged" now says by whom, and describes the ask | ### `furnish-repo` has two distinct gates Its existing Phase-2 yes/no authorises **furnishing** (create labels, open the PR). That is now called out in three places as **not** merge approval — merging is a separate question asked later, once the PR exists and CI is known. The Phase-2 prompt text says so to the user: *"I'll ask separately before merging anything."* Labels are still created directly, without waiting: they are additive and reversible. Only the AGENTS.md change waits for a merge. ## Commits | Commit | | |---|---| | `8231439` | Stop auto-merging; skills stop at the PR | | `b62f367` | Revised on owner feedback: **prompt** to merge rather than always stopping | | `31624c6` | Scope to pi only; drop all other-tool coupling | ## Verification - No cross-tool reference remains in any of the four changed files - Skill frontmatter parses on all three - `npm test` — 31/31 pass - Every merge assertion site agrees; no stale "does not merge / do not ask" text ## Note for the reviewer Per the policy it introduces, **this PR is deliberately left unmerged** — it is the first PR to follow its own rule. Two follow-ups, not addressed here: - Repos already furnished keep the old Workflow Conventions wording until `furnish-repo` is re-run against them. - The prompt wording is specified in the skills but, being Markdown-driven, is not enforced by a test. Worth checking during review that the phrasing reads well.

yousuf added 1 commit

2026-08-20 03:21:05 +00:00

issue-57: stop skills auto-merging PRs; make merging opt-in 823143928a

Skills squash-merged their own PRs, gated only on CI, so a change could reach the
default branch without the human ever seeing it. Merging is now the repo owner's
decision (or pi-loop's), never the skill's.

Two reasons the old behaviour was wrong:

  * CI is a weak proxy for review. In at least one consuming repo the pipeline is
    secret-detection only — no build, no tests — so "CI green" meant "no credentials
    committed", not "this change is correct".
  * It duplicated and pre-empted pi-loop, which already owns merging via --auto-merge /
    PILOOP_AUTO_MERGE (default off) and its own mergeability poller. Without
    --auto-merge pi-loop deliberately leaves a draft MR for review; a skill merging
    inside that stage silently overrode it. They were two merge authorities racing.

New rule, identical in every context (plain pi and pi-loop alike): open the PR, report
the URL and CI status, stop. Merge only on an explicit user instruction, or
`auto-merge: true` in piloop-config.yaml (the file create-issues §2.1 already reads,
reusing pi-loop's key name). Unspecified means stop and NOT prompt — an interactive
confirmation would hang a non-interactive run. CI remains a hard gate on the
authorised-merge path.

This does not regress pi-loop: it merges through its own poller, not by asking a skill.

Rejected: "merge when inside pi-loop". pi-loop composes stage prompts in-process and
exports no PILOOP_* variable into the agent environment, so a skill has nothing to test
and would be guessing. Making the rule context-independent removes the need.

Each skill asserted its merge behaviour in several places that would otherwise
contradict each other, so all sites moved together:

skills/commit-changes/SKILL.md
  - frontmatter description (highest leverage — always in the agent's context, and what
    drove the old behaviour), workflow overview, new "Merge policy" section, Step 5
    rewritten as report-and-stop with an authorised-merge branch, failure table, both
    Quick Reference flows, cleanup/verify/report steps marked merged-path-only.

skills/commit-docs/SKILL.md
  - same treatment; Step 4.

skills/furnish-repo/SKILL.md
  - same treatment; §3.7. Its Phase-2 yes/no prompt now says the PR is left for review,
    and the failure table records that this gate authorises furnishing, not merging.
    Labels are still created directly (additive and reversible); only the AGENTS.md
    change waits for review.
  - The canonical Workflow Conventions block it writes INTO other repos also gained the
    merge-ownership line, so furnished repos inherit the rule.

AGENTS.md
  - "PRs are squash-merged and the source branch is deleted" now says by whom.

Verified: skill frontmatter still parses, `npm test` 31/31 pass, and no remaining
unconditional merge claim in any of the three files.

Closes #57

yousuf added 1 commit

2026-08-20 06:27:01 +00:00

issue-57: prompt to merge after opening the PR instead of always stopping b62f3678ec

Revises the previous commit on repo-owner feedback: a middle ground between
merge-by-default and never-merge. The skill now opens the PR and ASKS whether to merge,
rather than stopping unconditionally.

Why the previous "stop, and never ask" rule was over-cautious:

* It was justified by "an interactive prompt would hang a non-interactive pi-loop run".
That premise is wrong. pi-loop does NOT invoke these skills — it bundles its own
stage skills (src/skills/: implement, review, remediate, docs, fixer) and ships via
deterministic code. Its ADR-014 explicitly REJECTED agent-driven shipping through
gh/glab/fj ("literal commit-docs") as breaking its REST-only/no-CLI invariant. So
the hang scenario cannot arise through these skills.

The real hang risk is `pi --print` / `-p` (non-interactive mode) and scripted use, which
is now handled explicitly rather than by banning prompts everywhere.

New precedence, identical across the three skills:

1. Explicit user instruction ("commit and merge it") -> merge, no prompt
2. piloop-config.yaml `auto-merge: true` -> merge, no prompt
3. Otherwise -> ASK, and wait

Two rules override the prompt:

* Never merge without a clear human yes. Silence, ambiguity or an unanswered prompt
resolves to NO. The agent must never answer the question on the user's behalf.
* Non-interactive runs must not hang: `pi --print`/`-p` and scripted invocations skip
the prompt and stop at the PR with the URL + merge command.

CI remains a hard gate on every merge path, and the merge prompt is NOT offered when CI
is red — the failure is reported instead.

Changed in all assertion sites per skill (frontmatter description, overview, Merge
policy, merge step, failure table, Quick Reference, completion report):

skills/commit-changes/SKILL.md — Step 5
skills/commit-docs/SKILL.md — Step 4
skills/furnish-repo/SKILL.md — §3.7. Its Phase-2 furnish gate is now called out in
three places as explicitly NOT merge approval: merging is a second, separate
question asked after the PR exists and CI is known. The Phase-2 prompt text now says
so to the user ("I'll ask separately before merging anything").
AGENTS.md + the canonical Workflow Conventions block furnish-repo writes into other
repos — both updated to the ask-then-merge rule.

Verified: frontmatter parses on all three, `npm test` 31/31, and every merge assertion
site agrees.

yousuf changed title from ~~issue-57: stop skills auto-merging PRs; make merging opt-in~~ to issue-57: prompt before merging instead of auto-merging PRs

2026-08-20 06:27:40 +00:00

yousuf referenced this pull request

2026-08-20 06:28:09 +00:00

Skills must not auto-merge PRs; stop at the PR and make merging opt-in #57

yousuf added 1 commit

2026-08-20 06:35:35 +00:00

issue-57: scope the merge prompt to pi only; drop all pi-loop coupling 31624c682e

Removes every pi-loop reference introduced by the two previous commits. This change is
about how the skills behave under **pi**; pi-loop's behaviour is left exactly as it was.

`main` had zero pi-loop references in these three skills — the coupling was introduced
by this branch, not inherited, so removing it restores the intended separation.

Two distinct things were removed:

  1. Explanatory prose about pi-loop's internals (its bundled stage skills, ADR-014, the
     --auto-merge subsystem). It argued why the prompt was safe by reasoning about a
     different tool. The rule stands on its own under pi; the reasoning did not belong
     in these files.

  2. The `piloop-config.yaml` -> `auto-merge: true` override. This was the more
     substantive problem: it made a pi-only skill read pi-loop's config file and gave an
     existing pi-loop key a second, skill-specific meaning. A user setting `auto-merge`
     for pi-loop would have silently changed how these skills behave under pi. Merge
     pre-authorisation is now expressed only where it belongs — in the user's request.

Resulting precedence, with no reference to any tool other than pi:

  1. User explicitly asked ("commit and merge it")  -> merge, no prompt
  2. Otherwise                                       -> ASK, and wait

Unchanged from the previous commit: never merge without a clear human yes (silence and
ambiguity resolve to no; never answer on the user's behalf); non-interactive runs
(`pi --print` / `-p`, scripted) skip the prompt and stop at the PR; CI remains a hard
gate and the prompt is not offered when CI is red.

`skills/create-issues/SKILL.md` is deliberately untouched. Its §2.1 `piloop-config.yaml`
read predates this branch and is a separate, pre-existing coupling — removing it is not
in scope here.

Files: commit-changes, commit-docs, furnish-repo (frontmatter description, Merge policy,
merge step, failure table, Quick Reference), AGENTS.md, and the canonical Workflow
Conventions block furnish-repo writes into other repos.

Verified: no `piloop`/`pi-loop` match remains in any of the four files, frontmatter
parses on all three skills, `npm test` 31/31.