david/pi-extensions-and-skills

Watch

Fork 0

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

New issue

Closed

opened 2026-08-20 01:23:08 +00:00 by yousuf · 2 comments

yousuf commented

2026-08-20 01:23:08 +00:00

Collaborator

Summary

commit-changes, commit-docs and furnish-repo currently squash-merge the PR
themselves, gated only on CI. There is no review gate: the human never sees the PR
before it lands on the default branch.

Merging should not be a skill's decision. Change the default to stop at the PR, and
make merging opt-in.

Why this is wrong today

commit-changes/SKILL.md Step 5 states it explicitly:

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

Two problems:

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.
It duplicates and pre-empts pi-loop. pi-loop already owns merging: it has
--auto-merge / PILOOP_AUTO_MERGE (default false), pollUntilMergeable, and
githubMergePoller / gitlabMergePoller. Without --auto-merge pi-loop deliberately
leaves a draft MR for review — and a skill that squash-merges inside that stage
silently overrides that decision. Today they are two independent merge authorities
racing each other.

Decision

Skills never merge. They stop at the PR and report the URL. Same rule in every
context — plain pi and pi-loop alike.

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

Context	After this change
Plain pi	Skill stops at PR → human reviews → human merges
pi-loop, no `--auto-merge`	Skill stops at PR; pi-loop leaves it draft — both agree
pi-loop `--auto-merge`	Skill stops at PR; pi-loop still merges via its own poller

Override precedence

Explicit instruction in the request ("commit and merge it") → merge
piloop-config.yaml → auto-merge: true → merge (reuses the file
create-issues/SKILL.md §2.1 already reads, and pi-loop's own key name)
Otherwise → stop at the PR. Do not merge, and do not ask.

Unspecified means stop, not prompt. An interactive confirmation would hang a
non-interactive pi-loop run; stopping with a URL is safe in every context.

CI remains a hard gate on the override path — if merging was requested and CI is
red or still running, still stop.

Not implemented: pi-loop env detection

A rule like "merge when inside pi-loop" was considered and rejected as
unimplementable: 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 entirely.

If per-context behaviour is ever wanted, the prerequisite is a pi-loop change to pass
PILOOP_AUTO_MERGE through to the agent env — tracked separately, not here.

Scope

Each skill asserts its merge behaviour in four places; all must change together or
the file contradicts itself (the frontmatter description matters most — it is always
in the agent's context and is what drives the behaviour):

skills/commit-changes/SKILL.md — frontmatter description, workflow overview, Step 5, failure-modes table
skills/commit-docs/SKILL.md — frontmatter description, overview, Step 4, failure-modes table
skills/furnish-repo/SKILL.md — frontmatter description, overview, §3.7, confirmation prompt text, failure-modes table
AGENTS.md — "PRs are squash-merged and the source branch is deleted" needs to say by whom

Acceptance criteria

No skill merges a PR by default in any context
Each skill ends by reporting the PR URL and a copy-paste merge command
Explicit instruction and piloop-config.yaml's auto-merge: true both still merge
CI still gates the override path
No skill prompts interactively when merge intent is unspecified
No file contradicts itself across its four assertion sites

## Summary `commit-changes`, `commit-docs` and `furnish-repo` currently **squash-merge the PR themselves**, gated only on CI. There is no review gate: the human never sees the PR before it lands on the default branch. Merging should not be a skill's decision. Change the default to **stop at the PR**, and make merging opt-in. ## Why this is wrong today `commit-changes/SKILL.md` Step 5 states it explicitly: > **Failing or still running** → stop and report (fail fast — do not merge). > **No second confirmation prompt is used; CI is the merge gate.** Two problems: 1. **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. 2. **It duplicates and pre-empts pi-loop.** pi-loop already owns merging: it has `--auto-merge` / `PILOOP_AUTO_MERGE` (**default `false`**), `pollUntilMergeable`, and `githubMergePoller` / `gitlabMergePoller`. Without `--auto-merge` pi-loop deliberately leaves a **draft** MR for review — and a skill that squash-merges inside that stage silently overrides that decision. Today they are two independent merge authorities racing each other. ## Decision **Skills never merge. They stop at the PR and report the URL.** Same rule in every context — plain pi and pi-loop alike. This does not regress pi-loop: it merges via its own poller, not by asking a skill. | Context | After this change | |---|---| | Plain pi | Skill stops at PR → human reviews → human merges | | pi-loop, no `--auto-merge` | Skill stops at PR; pi-loop leaves it draft — **both agree** | | pi-loop `--auto-merge` | Skill stops at PR; **pi-loop still merges** via its own poller | ### Override precedence 1. **Explicit instruction** in the request ("commit and merge it") → merge 2. **`piloop-config.yaml` → `auto-merge: true`** → merge (reuses the file `create-issues/SKILL.md` §2.1 already reads, and pi-loop's own key name) 3. **Otherwise → stop at the PR.** Do not merge, and **do not ask.** **Unspecified means stop, not prompt.** An interactive confirmation would hang a non-interactive pi-loop run; stopping with a URL is safe in every context. **CI remains a hard gate on the override path** — if merging was requested and CI is red or still running, still stop. ### Not implemented: pi-loop env detection A rule like "merge when inside pi-loop" was considered and **rejected as unimplementable**: 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 entirely. If per-context behaviour is ever wanted, the prerequisite is a pi-loop change to pass `PILOOP_AUTO_MERGE` through to the agent env — tracked separately, not here. ## Scope Each skill asserts its merge behaviour in **four** places; all must change together or the file contradicts itself (the frontmatter `description` matters most — it is always in the agent's context and is what drives the behaviour): - [ ] `skills/commit-changes/SKILL.md` — frontmatter description, workflow overview, Step 5, failure-modes table - [ ] `skills/commit-docs/SKILL.md` — frontmatter description, overview, Step 4, failure-modes table - [ ] `skills/furnish-repo/SKILL.md` — frontmatter description, overview, §3.7, confirmation prompt text, failure-modes table - [ ] `AGENTS.md` — "PRs are squash-merged and the source branch is deleted" needs to say by whom ## Acceptance criteria - [ ] No skill merges a PR by default in any context - [ ] Each skill ends by reporting the PR URL and a copy-paste merge command - [ ] Explicit instruction and `piloop-config.yaml`'s `auto-merge: true` both still merge - [ ] CI still gates the override path - [ ] No skill prompts interactively when merge intent is unspecified - [ ] No file contradicts itself across its four assertion sites

yousuf referenced this issue from a commit

2026-08-20 03:20:34 +00:00

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

yousuf referenced this issue from a pull request that will close it,

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

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

yousuf added the

enhancement

label

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

yousuf commented

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

Author

Collaborator

Decision revised after discussion with the repo owner.

The original issue below specified that skills should never merge and never ask —
open the PR and stop, full stop. The owner asked for a middle ground between
merge-by-default and never-merge: the agent opens the MR, then prompts the user to
merge or not.

That is now the implemented behaviour in PR #58:

#	Condition	Action
1	User explicitly asked ("commit and merge it")	Merge, no prompt
2	`piloop-config.yaml` → `auto-merge: true`	Merge, no prompt
3	Otherwise	Ask — report PR URL + CI status, wait for yes/no

With two overrides:

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.

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

Correction to the "Not implemented" section below

The original issue justified banning prompts with "an interactive confirmation would
hang a non-interactive pi-loop run". That premise was false. pi-loop does not
invoke these skills — it bundles its own stage skills and ships via deterministic code,
and its ADR-014 explicitly rejected agent-driven shipping through gh/glab/fj as
breaking its REST-only invariant. The real hang risk is pi --print, which the
non-interactive rule now handles directly.

The rest of the issue — the problem statement, the CI-is-a-weak-proxy argument, and the
scope/acceptance list — still stands.

Acceptance criteria, as revised

No skill merges on its own initiative in any context
Skills report the PR URL + CI status and ask whether to merge
Explicit instruction and piloop-config.yaml's auto-merge: true skip the prompt
An unanswered/ambiguous prompt is never treated as consent
Non-interactive runs stop at the PR instead of prompting
CI still gates every merge path
No file contradicts itself across its assertion sites

**Decision revised after discussion with the repo owner.** The original issue below specified that skills should **never merge and never ask** — open the PR and stop, full stop. The owner asked for a **middle ground** between merge-by-default and never-merge: *the agent opens the MR, then prompts the user to merge or not.* That is now the implemented behaviour in PR #58: | # | Condition | Action | |---|---|---| | 1 | User explicitly asked ("commit and merge it") | Merge, no prompt | | 2 | `piloop-config.yaml` → `auto-merge: true` | Merge, no prompt | | 3 | Otherwise | **Ask** — report PR URL + CI status, wait for yes/no | With two overrides: - **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. CI remains a hard gate on every merge path, and the prompt is not offered when CI is red. ### Correction to the "Not implemented" section below The original issue justified banning prompts with *"an interactive confirmation would hang a non-interactive pi-loop run"*. **That premise was false.** pi-loop does not invoke these skills — it bundles its own stage skills and ships via deterministic code, and its ADR-014 explicitly rejected agent-driven shipping through `gh`/`glab`/`fj` as breaking its REST-only invariant. The real hang risk is `pi --print`, which the non-interactive rule now handles directly. The rest of the issue — the problem statement, the CI-is-a-weak-proxy argument, and the scope/acceptance list — still stands. ### Acceptance criteria, as revised - [x] No skill merges on its own initiative in any context - [x] Skills report the PR URL + CI status and **ask** whether to merge - [x] Explicit instruction and `piloop-config.yaml`'s `auto-merge: true` skip the prompt - [x] An unanswered/ambiguous prompt is never treated as consent - [x] Non-interactive runs stop at the PR instead of prompting - [x] CI still gates every merge path - [x] No file contradicts itself across its assertion sites

yousuf commented

2026-08-20 06:36:10 +00:00

Author

Collaborator

Scope correction.

The issue text and my earlier comment both reasoned about pi-loop — its --auto-merge
subsystem, its bundled stage skills, and a piloop-config.yaml override. That was out
of scope.

This change is for the skills as used with pi. Other tooling's behaviour is unchanged
and is not referenced.

Removed in 31624c6:

All explanatory prose about other tools' internals in the skill files.
The piloop-config.yaml → auto-merge: true override. This was the more substantive
issue: it made a pi-only skill read another tool's config file and gave an existing
key a second, skill-specific meaning — so a user setting it for that tool would have
silently changed how these skills behave under pi.

main had zero such references in these three skills, so this restores the intended
separation rather than creating one.

Final behaviour

#	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

Plus: never merge without a clear human yes (silence and ambiguity resolve to no);
non-interactive runs (pi --print / -p) skip the prompt and stop at the PR; CI is a
hard gate on every merge path.

skills/create-issues/SKILL.md is deliberately untouched — its §2.1 config read
predates this work and is a separate, pre-existing coupling.

**Scope correction.** The issue text and my earlier comment both reasoned about pi-loop — its `--auto-merge` subsystem, its bundled stage skills, and a `piloop-config.yaml` override. That was out of scope. **This change is for the skills as used with pi. Other tooling's behaviour is unchanged and is not referenced.** Removed in `31624c6`: - All explanatory prose about other tools' internals in the skill files. - The `piloop-config.yaml` → `auto-merge: true` override. This was the more substantive issue: it made a pi-only skill read another tool's config file and gave an existing key a second, skill-specific meaning — so a user setting it for that tool would have silently changed how these skills behave under pi. `main` had zero such references in these three skills, so this restores the intended separation rather than creating one. ### Final behaviour | # | 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 | Plus: never merge without a clear human yes (silence and ambiguity resolve to *no*); non-interactive runs (`pi --print` / `-p`) skip the prompt and stop at the PR; CI is a hard gate on every merge path. `skills/create-issues/SKILL.md` is deliberately untouched — its §2.1 config read predates this work and is a separate, pre-existing coupling.