Skip to content

docs(orchestrate): batch review fixes and clarify actor routing - #890

Merged
thomasluizon merged 7 commits into
redesign/mainfrom
chore/orchestrate-review-fix-actor
Sep 15, 2026
Merged

thomasluizon merged 7 commits into
redesign/mainfrom
chore/orchestrate-review-fix-actor

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Sep 10, 2026 •

Copy link
Copy Markdown
Owner

Fixes thomasluizon/orbit-tickets#537.

What changed

  • Both canonical launcher signatures include --tier <default|mechanical> and --relaunch-reason <text>.
  • One accepted Pullfrog review produces one deduplicated finding set and at most one fix commit.
  • A batch without a commit spends no caps.reviewFixAttempts budget.
  • The orchestrator handles narrow review fixes directly. Larger batches produce one worker order.
  • After a worker pushes, the orchestrator resolves every identified thread and requests a fresh review.
  • --re-review can accept any current-head review submitted after its request.
  • A registered exact-head pullfrog-approval SUCCESS is the verdict.
  • When that check is absent, reviewState APPROVED with reviewedCommit equal to the head stands in for it.
  • COMMENTED and empty-body reviews are progress markers, not verdicts.
  • thomasluizon/orbit-tickets#542 tracks the stop-after-commit contract that will remove this review race.
  • Body-only FILE and not-applicable findings request a fresh same-head review after recording their disposition.

External interface evidence

This run confirmed the launcher flags with git show 94701d569cd6d7a0a2ff04a41264acc12f6aa346:tools/launch-worker.mjs. The source declares --tier <default|mechanical> and --relaunch-reason <text>, so the documentation matches the source.

This run also read tools/list-bot-threads.mjs. Its --re-review path accepts any completed current-head review submitted after the request, including COMMENTED.

This run read tools/lib/readiness-receipt.mjs, tools/verify-delivery.mjs, and tools/record-readiness.mjs. They accept a registered green check, or an exact-head APPROVED review only when that check is absent.

Contract timing

This contract change takes effect for the next run. The run producing it kept executing the contract it read at entry.

Test evidence

  • node tools/test-tools.mjs: ORBIT TOOLS GATE OK.
  • node .claude/hooks/test-hooks.mjs: ORBIT HOOKS OK.

…e ban

The skill named two acts and no actor. Step 7 forbids writing implementation
code during a SALVAGE; step 8 orders "FIX every finding that breaks behaviour,
security or data integrity". Neither says who performs that fix, and D89 says
Codex writes every code change and Claude never edits code.

Sessions filled the gap by carrying a claimed standing override forward in
handoff prompts, worded "Thomas keeps the standing override for harness and
review work, do NOT route those through /orchestrate, edit the code directly".
It appears in no ADR, no decision register and no hot.md entry: only in prompts,
copied session to session, drifting wider each time. On 2026-09-10 it was read
back to Thomas, who deleted it.

What replaces it is narrower and written down here. The orchestrator edits the
code for a review finding on a pull request it is already driving, inside the
caps.reviewFixAttempts bound, and nothing else. A finding large enough to be its
own work order is composed and dispatched to a worker. Implementing a ticket is
never this step.

Step 7's prohibition is now explicitly about salvage, which is the question of
whether a dead worker's ticket gets finished by hand, and points at the new
rule rather than appearing to contradict it.

node .claude/hooks/test-hooks.mjs: ORBIT HOOKS OK.
node tools/test-tools.mjs: ORBIT TOOLS GATE OK.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017mJSFcVpGKF5C2wedbTpeN
@vercel

vercel Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
orbit-ui-mobile-web Ignored Ignored Sep 15, 2026 7:22am UTC

Request Review

@pullfrog pullfrog 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.

ℹ️ One documentation reference needs correction.

Reviewed changes I reviewed the ownership and salvage-policy changes in the current one-commit patch.

  • Salvage scope — Limits the existing no-implementation prohibition to dead-worker salvage and points Pullfrog fixes to step 8.
  • Review-fix actor — Allows the orchestrator to implement bounded review fixes while leaving ticket-sized work with workers.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md Outdated
@thomasluizon thomasluizon changed the title docs(orchestrate): name who writes a review fix, and scope the salvage ban docs(orchestrate): batch review fixes and clarify actor routing Sep 15, 2026

@pullfrog pullfrog 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.

Important

The new batch contract has unresolved execution contradictions that can route a review fix to the wrong actor or leave a same-head review stuck.

Reviewed changes I reviewed the second commit added since the prior Pullfrog review.

  • Worker-budget routing - Added explicit default and mechanical tier selection plus recorded reasons for launches beyond the branch cap.
  • Batched review findings - Combined thread and body findings into one deduplicated batch with one commit and one attempt per accepted review.
  • Contract clarification - Corrected the actor-rule citation and scoped self-edit restrictions to the current run and D95.

⚠️ The contract can activate before its launcher

PR #964 remains open and is independently based on redesign/main. If this PR merges first, the next orchestrate run reads instructions for flags and a branch cap that the base launcher and configuration do not yet implement.

Technical details
# Sequence the launcher implementation before its operating contract

## Affected sites
- `.claude/skills/orchestrate/SKILL.md:658-668` - Requires `--tier`, `--relaunch-reason`, and `caps.workerLaunchesPerBranch`.
- `tools/launch-worker.mjs:65-70,119-123` on the current base - Parses neither new flag and always resolves `default`.
- `.claude/orchestrator.json:12-18,30-35` on the current base - Declares neither the `mechanical` tier nor the branch launch cap.
- [PR #964](https://github.com/thomasluizon/orbit-ui-mobile/pull/964) - Carries the required implementation but is not merged or included in this branch.

## Required outcome
- Ensure PR #964 lands before this contract can reach `redesign/main`, or stack this branch on the implementation so the documented interface and implementation become available atomically.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md
Comment thread .claude/skills/orchestrate/SKILL.md Outdated
Comment thread .claude/skills/orchestrate/SKILL.md Outdated

@pullfrog pullfrog 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.

Important

The new attempt-accounting wording can exhaust caps.reviewFixAttempts inconsistently for batches that intentionally create no commit.

Reviewed changes I reviewed the third commit added since the prior Pullfrog review.

  • Actor routing — Assigned ordinary review fixes to the orchestrator and reserved worker dispatch for batches containing ticket-sized FIX findings.
  • Same-head dispositions — Added a PR comment and fresh APPROVED re-review path for body-only FILE and not-applicable findings.
  • Launcher contract — Added tier and relaunch flags to both canonical launcher signatures.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md Outdated

@pullfrog pullfrog 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.

ℹ️ No critical issues in the merge-forward; the PR evidence needs one refresh.

Reviewed changes I reviewed the merge of redesign/main since the prior Pullfrog review.

  • Merged launcher support — Brought the merged PR #964 implementation of worker tiers, relaunch reasons, and the per-branch launch cap onto this branch.
  • Preserved the skill patch — Left the one-file net patch unchanged while making its launcher contract available on both the base and head.

ℹ️ The test evidence describes the pre-merge state

The PR body still says commit 94701d56 is absent from redesign/main and that the branch launcher lacks both new flags. Merge commit 969a19ec brought that implementation onto the branch and advanced the base to it, so the durable test narrative is now inaccurate.

Technical details
# Refresh the PR evidence after the launcher merge

## Affected sites
- PR body, `## Test evidence` — Still records the five signature failures and says the dependency is absent from the base.
- `tools/launch-worker.mjs:38-41,73-83,288-299` — Now implements `--tier`, `--relaunch-reason`, and branch-cap enforcement.
- `.claude/orchestrator.json:12-20,34-39` — Now declares the mechanical tier and `caps.workerLaunchesPerBranch`.

## Required outcome
- Make the PR body's test evidence describe the current head and stop claiming that the merged launcher implementation is absent.

Pullfrog  | Fix it ➔ | View workflow run | Using GPT Sol | 𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog 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.

Important

The large review-fix path can run the mandatory push before its threads are resolved, allowing Pullfrog to review an unresolved thread state. Earlier attempt-accounting and PR-evidence feedback also remains open.

Reviewed changes I reviewed the full four-commit patch against redesign/main, including the merged launcher support and the existing worker delivery contract.

  • Launcher routing: Documents default and mechanical tiers plus recorded reasons for launches beyond the per-branch cap.
  • Review batching: Combines thread and body findings into one classified set with at most one worker order and one fix commit.
  • Fix ownership: Assigns narrow review fixes to the orchestrator, sends ticket-sized batches to a worker, and confines the no-edit prohibition to salvage.
  • Same-head adjudication: Records FILE and not-applicable dispositions before requesting a fresh same-head review.
  • Contract timing: Makes skill and harness changes effective for the next run without weakening D95.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md

@pullfrog pullfrog 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.

Important

The post-push re-review still cannot guarantee that the resolved-thread state supplies the final verdict.

Reviewed changes I reviewed the new ordering clarification and evidence updates added since the prior Pullfrog review.

  • Attempt accounting — Clarified that only the batch's single commit consumes caps.reviewFixAttempts, while no-commit disposition batches spend nothing.
  • Worker review ordering — Documented the current worker push-before-resolution behavior and added an explicit post-resolution --re-review step.
  • PR evidence — Replaced the stale launcher test narrative with current passing harness results and source evidence.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md Outdated
Comment thread .claude/skills/orchestrate/SKILL.md Outdated

@pullfrog pullfrog 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.

Important

The replacement verdict rule disagrees with the executable readiness contract, so compliant runs can enforce different completion conditions.

Reviewed changes I reviewed the verdict clarification added since the prior Pullfrog review.

  • Verdict routing — Recast reviews accepted by --re-review as progress and introduced a separate exact-head verdict requirement.
  • Review-state classification — Clarified that COMMENTED and empty-body reviews are progress markers rather than verdicts.
  • Ticket traceability — Qualified the stop-after-commit follow-up as thomasluizon/orbit-tickets#542.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using GPT Sol | 𝕏

Comment thread .claude/skills/orchestrate/SKILL.md Outdated

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes I reviewed the verdict fallback correction added since the prior Pullfrog review.

  • Verdict fallback — Replaced the check-and-review conjunction with the executable contract: a registered successful pullfrog-approval is sufficient, while an exact-head approval stands in only when that context is absent.
  • Canonical review fields — Named the reviewState and reviewedCommit output fields used by list-bot-threads.mjs while preserving COMMENTED and empty-body reviews as progress markers.

Pullfrog  | View workflow run | Using GPT Sol | 𝕏

@sonarqubecloud

Copy link
Copy Markdown

@thomasluizon
thomasluizon merged commit 56ddfdd into redesign/main Sep 15, 2026
46 checks passed
@thomasluizon
thomasluizon deleted the chore/orchestrate-review-fix-actor branch September 15, 2026 08:16
thomasluizon added a commit that referenced this pull request Sep 17, 2026
* docs(progress): make /progress effort-agnostic and add --full

/progress carried the redesign inside it: a hardcoded redesign/main, a
table of seven redesign ticket numbers, and a description that asked
"where is the redesign". It is a generic progress skill.

It now resolves its own working branch from git and its integration
branch from the open pull requests, finds the effort's spec under
.claude/specs/ the way /handoff does, and derives what each ticket means
to a person from that ticket at answer time. The ticket table is deleted
rather than generalised: a table of numbers rots the moment a ticket is
retitled, split or closed.

The default scope moves to THIS SESSION, because mid-run he is asking
what just happened and a whole-effort summary buries it. --full answers
the session first and then the whole spec underneath, labelled per scope,
and earns the extra room the writing contract normally refuses.

Also reseeds two calibration entries that had drifted before this change:
handoff/SKILL.md, last moved by a direct commit that ran no PR checks,
and orchestrate/SKILL.md from #890. Both verdicts were reread and still
describe their files, so they carry forward with new digests.

Harnesses: node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs
both pass. node tools/check-calibration.mjs reports 31 files stamped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(progress): derive branch and session state from evidence

* fix(progress): verify integration ancestry

* fix(progress): handle repository-specific squash evidence

* fix(progress): refuse a closed stacked parent as integration

The base-chain walk searched only open pull request heads, then accepted every
absent head as the integration branch. A stacked parent can be CLOSED while it
is still the child's recorded base: PR 575's base is
`feature/539-b5-apply-design`, that branch is PR 560's head, and 560 is CLOSED
with `mergeCommit` null. The walk therefore named a feature branch as the
integration branch for that whole chain.

Each candidate now resolves in every state through
`gh pr list --head <candidate> --state all`. An OPEN or MERGED parent is
followed to its base; a CLOSED unmerged parent ends the walk as an unresolved
chain that names the parent instead of reporting its head as integration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(progress): one rule for a reused branch head

Step 3 gave two incompatible decisions for the same multi-row result: take
the highest number, and treat a duplicate matching head as ambiguity. A
branch name can be reused, so `/progress` could either continue the walk or
refuse it depending on which sentence it read.

A reused head is normal, not ambiguous, and now has one rule. Exactly one
OPEN row decides whatever the numbers are, because a branch can have only
one live pull request. With no OPEN row the highest number decides, because
numbers increase. Two OPEN rows sharing a head is the real ambiguity and
still refuses. A cycle still refuses.

An empty array is what ends the walk: the candidate is no pull request's
head, so it is the integration branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(progress): never read a fork's pull request as a stacked parent

`gh pr list --head` filters on the branch NAME only, and its own help states
that the `<owner>:<branch>` form is not supported. This repository is public,
so a fork's pull request carrying the same branch name comes back in that
listing. It could then be the sole OPEN row, and the walk would follow a
stranger's `baseRefName`.

The listing now requests `isCrossRepository` and drops every row where it is
true before any decision runs. An array left empty by that filter terminates
the walk exactly as a genuinely empty one does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(progress): execute integration branch resolution

---------

Co-authored-by: Claude Opus 5 (1M context) <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.

1 participant