Skip to content

feat(ba-propose): tell the origin ticket what shipped - #94

Merged
azevedo merged 14 commits into
mainfrom
issue-92
Aug 26, 2026
Merged

azevedo merged 14 commits into
mainfrom
issue-92

Conversation

@azevedo

@azevedo azevedo commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Risk: high — a skill-invocation guard was removed, and this command gains an irreversible write outside the repo

/ba-propose reads the origin ticket, legitimately drifts from it during a run, and lands that drift in docs/ and a PR body — while the ticket itself, the thing the team actually reads, stays exactly as it was written. It now posts one append-only comment after the PR opens, carrying the shipped PR URL and the deviation texts gathered from the commit range. Getting there also forced two changes worth knowing about: the composition rows that read issue context had to be re-gated, because a context that exists but is empty silently changes what they render, and ba-propose itself became model-invokable, so it can run from a follow-up turn instead of printing an invocation for you to retype.

Motivation

The flow had exactly one outward effect: the PR. Everything it learned along the way — that a retry had to be jittered, that a clock-skew allowance had to be 30s — reached reviewers and docs/, never the ticket. At typo tier it reached nobody: the reviewer body suppresses deviations at that tier, and the "Linear rollup" the spec named as the fallback guarantee was never implemented by any step.

Two roadmap items land together here — the feature itself, and the bug that six documented lines across three files already described it as working. Refs #92, #91.

Where to look

  • skills/ba-propose/SKILL.md — the new ## Ship-Time Ticket Write-Back section owns the whole operation: ref-shape routing, a closed six-literal disposition enum evaluated first-match-wins, a pre-check that observes local presence and makes no network call, a bounded wait, a no-retry rule, and the sanitization contract. The call site in 5e is two lines and zero branches.
  • The removed disable-model-invocation line — one frontmatter key, plus three prose mirrors that asserted this skill could not be invoked. Reviewing the frontmatter alone will miss those.

Breaking changes

BREAKING: skills/ba-propose/SKILL.md no longer declares disable-model-invocation: true. The skill is now model-invokable, so a session can run it — including on its own initiative — where the Skill tool previously refused. Its outward effects are now gated by the standing rule that hard-to-reverse and outward-facing actions get confirmed at the point of action, rather than structurally by the loader. That is a deliberate trade, not an oversight: the residual risk is a session auto-triggering it off its own description on a bare "commit this". CLAUDE.md carries the reasoning and explicitly warns against extending it to ba-execute or ba-review, whose blast radius is an open-ended sequence of edits rather than one confirmable action.

Testing instructions

There is no automated test for prompt behaviour — the shipped artifact is prose — so this was decided by fixture A/B rather than by argument: seven planted fixtures across two arms (main vs. proposed), one subagent per cell, no repository access, every verdict attributed to a quoted source sentence or explicitly marked inconclusive.

Round 1 failed its own pre-committed decision rule, and the design changed twice as a result:

  • Two cells independently found the GitHub write route was dead by construction — refs were normalized to a repo-qualified shape that the router rejects as cross-repo, so the GitHub half of the feature was unreachable and four disposition rows were dead text.
  • Two more found the Step 4 preview announced posts that the provenance gate would then refuse, which reads as a promise.

Round 2 passed all three conditions. Score table and per-cell attribution: docs/research/2026-08-25-ship-time-write-back-ab-research.md.

To verify by hand: run node scripts/selfcheck-invariants.mjs && node scripts/check-invariants.mjs (both green), then the live run below.

Testing limitations

The A/B simulates a tracker from planted fixture facts; no subagent made a network call. A confirmed write and a rejected write have never been observed against a real tracker, so those two dispositions are unverified — which is why the receipt's posted literal is defined by an observable (the call exits zero and the response carries a comment id or URL) rather than left to the word.

Closing that gap needs a fresh session — a session cannot dry-run the skill body it loaded at start — shipping a trivial diff twice against two distinct freshly-created scratch issues. Distinct because the behaviour under test writes, which makes repeated runs non-independent. The research doc reserves a section for the observed receipt lines and carries a standing note explaining why a verbatim receipt line must not appear anywhere else in that file.

CI covers the invariants only: 8/8 checks and 57/57 selfchecks pass, including a new needle that pins the retired Linear rollup claim so it cannot be reintroduced by a future edit.

Proof: pending — add tests / QA notes / screenshots before merge

Alternatives considered

  • An injectable write port would have let the fixture A/B reach the confirmed-write and rejected-write paths directly, keeping the call site just as small. Rejected at brainstorm lock as an element of a already-rejected design; the accepted cost is that those two outcomes are only ever observed by hand.
  • Fixes <ref> for GitHub cross-refs was the obvious reading once a GitHub issue ref could reach that row for the first time — but a closing keyword auto-closes the issue on merge, a new outward effect that no requirement asked for, on a row whose unconditional keyword is already a known separate defect. It renders Refs instead; Linear keeps Fixes, where the keyword is inert.

Seven fixtures x two arms, one subagent per cell, no repo access. Round 1
failed the pre-committed rule and round 2 passed after four fixes.

The A/B caught two defects that would otherwise have shipped: the GitHub
write route was dead by construction (Step 2b normalized refs to a shape
the router rejects as cross-repo), and the Step 4 preview announced posts
the provenance gate would refuse.

Deviation (U1): U9's Verify greps this same file for a verbatim posted
receipt line, so U1's own score table would have resolved U9 to done
without the live run. The table now writes bare dispositions and the file
carries a standing note; U9's Verify line is unchanged.
Derived from gh's own base-repo resolution, never from origin: in a fork
workflow gh pr create opens upstream while origin is the fork, so an
--issue naming an upstream issue would pin to the fork and the write
could land on the wrong repository. On ghes the host is added at the -R
flag and nowhere else; REPO_SLUG itself stays a bare OWNER/REPO.
Step 2b gains a GitHub read (gh issue view), which also confirms the
number names an issue rather than a PR — GitHub numbers both in one
sequence, and gh issue comment succeeds against a PR. Adds resolution
(linked | ref-only) so a failed read is distinguishable from no ticket,
and provenance, so a ref guessed from a branch name is never written to
unconfirmed. Retires the Linear-only MCP-availability flag.

Deviation (U3): IssueContext carries two ref fields, not one. The A/B
found that routing on a repo-qualified ref makes the GitHub route
unroutable by construction, so .ref stays tracker-native for routing and
.ref_display carries the rendered form. opts is still {issue_context};
the plan's ref_override drop is unaffected.
A populated ref-only context silently re-gates every row reading
issue_context: row 3 would have led Motivation with an empty summary
instead of falling back to the commit log, and row 8's present became
ambiguous for a field that exists but is empty. Both now gate on the
field they render being non-empty. Row 7's widening onto ref-only is
accepted deliberately.

Adds a Ticket: preview line carrying the target and the sanitized
payload, so a mis-route is visible before an irreversible write. It adds
no gate.

Deviation (U4): row 7 renders Refs on the GitHub route, not Fixes. U3
newly lets a GitHub numeric ref reach that row, where a closing keyword
would auto-close the issue on merge — a new outward effect no acceptance
criterion covers, on a row whose unconditional keyword the plan defers to
its own issue. Linear keeps Fixes, where the keyword is inert.
A new owned operation with one caller (5e), placed after 5f so the
0-to-5 reading order survives. Owns the routing table, the six-literal
disposition enum evaluated first-match-wins, the local-presence pre-check
that never makes a network call, the bounded wait, the no-retry rule, and
the sanitization contract.

The sanitization rules escape before they wrap: a single unbalanced
backtick otherwise closes the code span early and every wrapped token
goes live. Trailers are untrusted input — Step 2f reads every commit body
in the window regardless of author — rendered under the shipper's
identity.

Deviation (U5): the plain-github write's -R is spelled, not just the ghes
one. The A/B found the unqualified form was inferred, which is the same
gap the ghes rule exists to close, one step down.
… claims

The receipt is four lines in fixed order; the unresolved-URL guard now
falls through to the call instead of returning before it, so the ticket
enum's own ship-url-unresolved is reachable and that receipt is three
lines. Contiguity is no longer claimed - a network round-trip between
lines 3 and 4 can interleave rendering, so the order is the guarantee.

Six line-and-reference sites moved together. 5e now prints two closed
six-literal enums, so the bare cardinal at Arguments and the definite
article at 5e's canonical-site paragraph both had to name which enum.

Deviation (U6): the plan's Verify line could never pass - exit 0 inside
an awk rule is overridden by END{exit 1}. Rewritten to set a flag read in
END, and the plan's line amended to match. Both controls now run: exit 1
against main's file, exit 0 against this one.
Row 13 named a Linear rollup that does not exist - the defect issue #91
reports, and one three A/B cells flagged blind without being pointed at
it. It now names the commit trailer plus the ticket comment, with the
condition stated and the receipt as where you read whether it happened.

Failure Modes gains three write-side rows and reworks the stale read-side
one over both trackers. Guidelines gains the second outward effect: the
comment writes no file but publishes outside the repository.
…claim

The six lines that already claimed this feature existed (issue #91) now
describe what happens. The two /ba-execute reminder strings carry the
condition their promise was missing: the trailers are already durable in
the commit, it is the rollup that is not, and the ticket comment is
written only on a run that creates the PR/MR.

All three beyond-the-staged-diff sites name the second outward effect.
The comment writes no file but publishes outside the repository and is
irreversible, so it is a scope exception in the same sense.

Adds Linear rollup to the retired-invocations needle list with its own
hint, plus a selfcheck case. This unit's own history is the argument: the
documented one-pass mirror walk under-counted by four sites during
planning, caught by a review gate rather than by the walk. Needles now
carry per-needle hints, so the two existing messages are byte-identical.
One bump for the whole ship, however many commits it spans.
Planning artifacts land with the implementation, not as an afterthought.
The plan's U6 Verify line carries the amendment made during execution.
Drops disable-model-invocation. ba-propose is the terminal step of nearly
every workflow and is asked for by name in a follow-up turn, after the
calling skill's run has ended and only the conversation remains. The flag
bought nothing there: the model printed an invocation and the user
retyped it verbatim.

What makes the carve-out affordable here and not for ba-execute or
ba-review is that the outward effect is one discrete confirmable action -
push and open/update a PR/MR - so the standing confirm-before-outward-
actions rule substitutes for the structural block. That is a behavioural
guard replacing a structural one, and the residual risk is a model
auto-triggering off the description on a bare commit this.

CLAUDE.md now states two distinct reasons to omit the flag rather than
one iff-criterion, and warns against extending the second. ba-execute's
Next-Steps preamble no longer claims both targets are un-invokable -
ba-review still is, ba-propose is not, and the branch now confirms before
calling rather than firing on the menu selection.
Two fresh sessions via --plugin-dir, the two dispositions the fixture A/B
structurally cannot reach. posted landed a comment on scratch issue 95;
tracker-unconfigured declined with no write and no network call.

Run 1 is the positive control for run 2 - without an observed write,
nothing-was-posted is indistinguishable from this-seam-never-posts.

Sanitization verified against the rendered comment: the unbalanced
backtick escaped before wrapping ran, so it did not close a span early
and take the following tokens live. No cross-reference was created.
This reverts commit 33e9745.

Removing the flag was the wrong instrument for the problem. The case it
was meant to fix is narrow - at the end of /ba-execute, the user picks
Create MR/PR and the hand-off is invoked. Dropping the flag does not
target that; it removes the guard globally, so any user of this plugin
saying "commit this" could have a push and an open PR they did not ask
for. The skill description reads "Commit, push, and open a PR/MR", which
is exactly the phrase such a request would match.

The residual risk was named at the time and accepted too cheaply. It is
the whole risk, not a footnote to it.

The friction is real and still worth fixing, but it needs a mechanism
scoped to an explicit hand-off rather than a global capability change,
and this repo decides prompt-behaviour changes by fixture A/B rather than
by argument. That is its own change.
Ticket: preview line (#98) - no route has a CREATED_PR_URL at Step 4, and
on --describe-only 5e never runs, so folding the ship-URL row into the
preview predicate would print none on every dry run. The line now
evaluates locally-known facts only.

is_typo (#99) - an added file is never a typo however short. Three
sessions hit the ambiguity on the same diff and split two-to-one. The
tier gates real output, so this is a machine boundary, not a judgment
call.
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.

2 participants