Skip to content

feat(pr-pipeline): answer lane review threads on owner-login PRs from the lanes App - #10848

Merged
kyle-sexton merged 7 commits into
mainfrom
feat/owner-pr-feedback
Oct 29, 2026
Merged

kyle-sexton merged 7 commits into
mainfrom
feat/owner-pr-feedback

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Refs: #10763

Hold paths: this changes enforcement-class floor scripts (.github/scripts/pr-unblock/address-feedback.mjs, publish-feedback.mjs, feedback-dispatch.mjs, fix-ci-dispatch.mjs), the trust-class docs/conventions/pr-pipeline.yaml (comments only) and .github/workflows/pr-unblock.yml (a comment only). It also changes what the lanes do unattended on PRs the owner opened, so the owner decides the merge.

Summary

Agent sessions answer review threads under the owner's login, so on an owner-login PR no lane answer is visible and #10841's worker tier, which resolves only outdated threads a lane login answered, resolves nothing. This PR lets the pr-unblock address-feedback round, which already posts as the lanes App, serve adopted owner PRs too. Its replies then mark lane answers, and the worker tier can resolve them.

Which config gates owner PRs: the existing adoption section of docs/conventions/pr-pipeline.yaml (its idle-hours) plus the kind: human entries of .github/standards/trusted-actors/trusted-actors.json, both read at the base. That is fix-ci's adoption rule. No new key, no hard-coded login.

New powers on owner PRs: the lanes App now posts review replies, round markers and adjudication replies on an idle adopted owner PR, and apply-feedback-patch pushes review fixes there under the same patch rules as on App PRs. Pushing fix commits to an idle adopted owner PR is the grant fix-ci already uses (README, Adoption), under the same idle rule, so no permission, token or broker grant is added. I kept the push because a replies-only round would end every VALID answer feedback-not-landed: publish-feedback requires the lane's fix commit. The lane resolves no thread on an owner PR and merges nothing.

Fix

  • feedback-dispatch.mjs: ownerAuthored admits a PR whose author is a User with a kind: human trusted id under an adoption section. ownerSkip then requires no Hold: body line and both updated_at and the head commit's committer date at least idle-hours old. On an owner PR only a thread the select job keeps dispatches. Pass-only rounds stay App-only. A review usually completes while the session is active, so the tick's sweep gains an idle-owner pass: it skips on the listing's updated_at with no read and otherwise spends one check-run read. A head with no round goes to dispatchAtHead (about six reads) only within IDLE_OWNER_WINDOW_HOURS (2 h, four ticks) of the PR turning idle; past that it costs the one read, and any new update reopens the window. A head with a round spends a reply check-run read and a status read, and a failing pr-unblock/address-feedback stop with no unresolved thread is dispatched to clear it (sweepStale, under the owner's Hold: and idle gates, with no window). ownerAdoption reads the base section; an unreadable one serves no owner, and App PRs proceed.
  • address-feedback.mjs select: on a PR a User opened, an unresolved thread the lane does not take (one the person wrote, answered, or a lane answer left open) skips not-applicable with no stop, instead of escalating feedback-untaken-threads.
  • publish-feedback.mjs: on a PR the lanes App did not open, read through getPull, a VALID reply and a lane adjudication post, and the thread is left open and logged. That case is not counted as feedback-partial.
  • Unchanged on every path: the select job keeps the same threads (the reviewer bot's, a listed bot's or a lane's, under the trusted-person and fix-request rules), so person-written and person-directed threads still go to the person. CRITICAL and security findings still escalate (needsPerson, settleDisputes). App PRs take the same path as before.
  • Docs: a new README section, Owner pull requests. The address-feedback dispatch and pass-only lines, the Adoption section, and the comments in pr-pipeline.yaml and pr-unblock.yml now say this. Changelog fragment under .changes/pr-pipeline/.

Verification

  • node --test .github/scripts/pr-unblock/*.test.mjs: 959 pass. .github/scripts/pr-merge/*.test.mjs and .github/actions/resolve-config/*.test.mjs also pass (2345 across the three suites). actionlint is clean on pr-unblock.yml, and markdownlint is clean on the README.
  • New tests:
    • An idle owner PR's reviewer-bot thread and its lane-footer thread both dispatch the round, whose replies post as the lanes App.
    • On an owner PR, a VALID reply and a lane verdict post and leave their threads open with no escalation. On an App PR the same round still resolves both.
    • On an owner PR, a thread the owner opened, and a reviewer thread the owner answered, start no round. The select job skips the owner-answered thread with no stop and keeps no person-opened thread.
    • App PRs are unchanged: they skip the idle read and still escalate feedback-untaken-threads. All existing tests pass unchanged.
    • Still not served: an untrusted person's PR, a trusted id typed Bot, a PR with no adoption section, an active PR, an unreadable date, a Hold: line, and a pass-only round to an owner PR.
    • Sweep: an idle owner PR dispatches. An active one spends no read, an answered head spends two check-run reads and one status read, and a stranger's PR is never listed.
    • Sweep: a no-round owner head past the idle window costs one check-run read and dispatches nothing; an update reopens the window. An owner PR whose round stopped and whose threads are all resolved is dispatched past the window, and not with a Hold: line, a recent head commit or an open thread.
  • Left out:
    • No live round on an owner PR, since that needs this merged.
    • Whether the lanes token broker's publish-artifact and mutate-branch grants work on a person-opened PR is UNVERIFIED. The broker is external. The trigger gate needs only a listed author (check-trusted-trigger README, Author), and fix-ci's adopted rounds already push there.
    • When pullsForCommit falls back to GraphQL, the PR has no updated_at or body. An owner PR then fails the idle test closed, and the tick's REST listing picks it up later.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 28, 2026 22:58
… the lanes App

address-feedback now also serves an adopted owner PR (a kind: human
trusted actor's, under the adoption section, idle for adoption.idle-hours
with no Hold: line), so its replies post as the lanes App and the babysit
worker tier can resolve the outdated threads a lane answered. On such a
PR publish-feedback leaves answered threads open, and the select job
leaves a person's thread with no stop.

Refs: #10763

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner PRs join only the sweep's idle pass, which reads the head's
address-feedback check runs before any other read. The select job's
no-stop skip is limited to PRs a User opened. Comments in pr-pipeline.yaml
and pr-unblock.yml name owner PRs.

Refs: #10763

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 29, 2026 03:05
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T03:07:20.373204Z ff18491 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

The pr-verify verifier failed head 9581cc3628cd61e1e1790c323e38b6ce32349858. What it wrote, as data:

Summary: This PR is linked to #10763 by a `Refs:` line, so it is judged as a partial delivery. Its body names the part it delivers and the work that remains. The route it takes goes against the issue's spec. The trusted intake refinement, which counts as spec, recommends opening agent PRs as the lanes App (`create --pushed`). It also recommends leaving PRs a person opens to that person, and the owner's recorded decision is "go w/ recommendations". This PR does neither. Instead, address-feedback now serves PRs opened on the owner's login. On those PRs the lanes App posts replies and round markers, and `apply-feedback-patch` pushes review-fix commits to the owner's branches. The issue's rule limits any permission change to resolving threads. Pushing fixes to owner-login PRs is a new unattended write that goes beyond that rule, even though the lane uses a power fix-ci already holds. The PR also delivers none of the acceptance criteria itself. Criterion 1 is left to an external worker tier, and criteria 2–4 hold only because this PR resolves nothing.
- docs/conventions/pr-pipeline/README.md:245: The new 'Owner pull requests' route goes against #10763's spec. The intake refinement's intended outcome, endorsed by the owner's 'go w/ recommendations', is to open agent PRs as the lanes App (`create --pushed`) and leave PRs a person opens to that person (open question 2). Either deliver that route (agent PRs opened as the lanes App, so the existing App-PR resolve covers them) or point to a trusted owner decision that picks the owner-PR adoption route. The PR body does not weigh the 'remove the cause' option the issue says to weigh first.
- `.github/scripts/pr-unblock/feedback-dispatch.mjs:344`: `dispatchFeedback` now starts full address-feedback rounds on owner-login PRs, so `apply-feedback-patch` pushes model-written fix commits to the owner's branches unattended. The issue says 'Any permission change is limited to these resolves'. Remove the owner-PR dispatch, or restrict it so it neither pushes nor posts beyond what thread resolution needs, unless the owner records approval for this wider behavior.
- `.github/scripts/pr-unblock/address-feedback.mjs:661`: On an adopted owner PR, an unresolved thread the lane does not take is now skipped as `not-applicable`. It previously escalated `feedback-untaken-threads`. This silently drops a person-visible stop the issue never asked to remove. Keep the escalation, or have the owner approve dropping it.
- `.github/scripts/pr-unblock/publish-feedback.mjs:631`: `postReplies` now calls `api.getPull(pr)`, which this diff adds only to the test fakes. Confirm that the real `createApi` used by publish-feedback provides `getPull`, and add a test that exercises the real client's method. Otherwise a production round throws there.
- PR body: criterion 1 (an outdated thread the lane answered on an owner-login PR gets resolved with no owner step) is left to #10841 and a user-scope `babysit_lane_logins` setting. Criteria 2–4 are met only because this PR resolves nothing. The part delivered here is a precondition for a route the spec did not choose, not a slice of the decided one.
- PR body: the body reports that the token broker's `publish-artifact` and `mutate-branch` grants on person-opened PRs are UNVERIFIED. The new owner-PR push path therefore depends on broker behavior that has not been tested.

This pull request leaves the merge queue, and ci-status holds it out while it carries pr-verify-failed. A pr-verify pass at a new commit removes the label.

@github-actions github-actions Bot added the pr-verify-failed pr-verify failed this head; a pass at a new commit removes it. label Oct 29, 2026
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed fdec2b145ca32d8ec9c66725b20da2b5424c15b4: 0 finding(s), 0 as inline comments.

No security findings. critical-findings: 0

I read the full diff and PR context. I also read the surrounding code that consumes the changed pieces: appId / APP_SENDER_ID handling in publish-feedback.mjs, getPull in address-feedback.mjs, and the sender gates in pr-unblock.yml.

What the change widens. Owner PRs now get address-feedback rounds. These post as the lanes App and can push review fixes through apply-feedback-patch.

Who passes. ownerAuthored requires all of these, and nothing in the diff loosens them:

  • pull.user.type === "User";
  • the user's id is a kind: human entry in the base's trusted-actors list (address-feedback.mjs:1055-1073, read at the base, not the PR head);
  • an adoption.idle-hours value is set in the base config.

An unreadable section returns NO_ADOPTION and serves no owner (fail closed). Pass-only rounds are refused for owner PRs (feedback-dispatch.mjs:346-349).

Idle and Hold: gates. ownerSkip is applied in three places: dispatchFeedback, the new sweepIdleOwner path, and sweepStale. An unreadable date fails closed (!(Date.parse(...) <= since)). HOLD_LINE moved from fix-ci-dispatch.mjs to feedback-dispatch.mjs with an identical regex, and fix-ci-dispatch.mjs now imports it.

What stays unchanged:

  • The token broker, pr-pipeline.yaml keys and the workflow gates. The pr-pipeline.yaml and pr-unblock.yml edits are comments only.
  • The thread-selection rules in the select job and the CRITICAL / security escalation paths.
  • The patch rules, since the push goes through the same apply-feedback-patch.
  • The push to an idle adopted owner branch, which is the grant fix-ci already holds. The diff adds no new reach.

Resolve behaviour. publish-feedback compares the PR author with appId (the sender id, per APP_SENDER_ID at publish-feedback.mjs:780). On a PR the lanes App did not open it posts the reply and leaves the thread open. It never resolves a person's thread, so the lane gains no resolve power here.

Merge holds. Skipping feedback-untaken-threads with no stop on an owner PR (address-feedback.mjs:652-663) removes an escalation for a person-written, person-answered or lane-answered open thread. The open thread still blocks the merge through the ruleset's thread-resolution requirement. The PR states this, and the diff leaves that gate untouched. The skip applies only when ownerAuthored holds, so App PRs and other authors still escalate (the new test covers this).

Residual notes, not findings:

  • Any kind: human trusted id counts as an owner, not one named login. This matches fix-ci's adoption rule and the PR text.
  • The PR itself marks unverified that the external broker's publish-artifact and mutate-branch grants work on a person-opened PR. If they don't, the round fails closed.
  • Spend: more unattended lane runs and check-run reads on owner PRs, covered by prepaid credit or subscription usage. No hold.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff18491fde

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/scripts/pr-unblock/feedback-dispatch.mjs
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed fdec2b145ca32d8ec9c66725b20da2b5424c15b4: 0 finding(s), 0 as inline comments.

No findings. I read the full diff and the PR context, then followed the changed paths into feedback-dispatch.mjs, address-feedback.mjs (select, ownerAdoption), publish-feedback.mjs and apply-patch.mjs. I had no shell, so I ran no tests; the 950-test pass and the lint results are author-claimed and unverified.

What I checked

  • Gating. An owner PR is served only when its author is a User with a kind: human trusted id and the base adoption section sets idle-hours. The dispatcher then requires no Hold: line and an idle PR and head commit (ownerSkip). An unreadable date or missing updated_at fails closed. A pass-only round never goes to an owner PR.
  • Push path. apply-patch.mjs (TARGETS.pr.held, ~L254) still re-reads the PR at push time and refuses a push on a body BODY_HOLD for any PR the lanes App did not open. That covers the case where the owner adds a Hold: line after the dispatch. The push also needs the PR head to be unchanged.
  • Async change. selectThreads is now async. Both callers (address-feedback.mjs ~L1203 and ~L1209) await it.
  • Resolve stand-in. publish-feedback.mjs decides the owner case with getPull(pr).user.id === appId. LEFT_OPEN is truthy, so settleDisputes and the reply loop do not raise feedback-partial. The App-PR path is unchanged and a test covers it.
  • Sweep. The sweep lists owner PRs separately from App PRs. The new idle pass runs only after the App-PR passes find nothing. It spends no read on an active PR and one check-run read on an idle head with no round. sweepStale applies the Hold: and idle gates to owner PRs.
  • Closing issue. The PR says "Refs feat(pr-pipeline): let lanes resolve their answered threads on owner-login PRs #10763", not "Closes", so no issue section is in the context file. The PR says criterion 1 of feat(pr-pipeline): let lanes resolve their answered threads on owner-login PRs #10763 is not met yet (babysit_lane_logins is not set) and calls that out itself.
  • Manifests. The diff changes no plugin.json or marketplace.json. The changelog fragment's bump: minor fits an added capability.

Spend: more address-feedback rounds, which use prepaid and subscription credit only. No billing change, so no hold.

Trusted paths. Several matched globs (address-feedback.mjs, publish-feedback.mjs, feedback-dispatch.mjs, adjudicate-feedback.mjs, pr-unblock.yml, pr-pipeline.yaml). No new permission, token or broker grant is added, and the workflow and YAML changes are comments only.

Not verified: the PR itself says it did not test whether the external lanes token broker's publish-artifact and mutate-branch grants work on a person-opened PR. It rests on the check-trusted-trigger rule that only a listed author is needed, and on fix-ci's adopted rounds already pushing there. The first live round after merge will show whether the grants work.

melodic-automation-lanes Bot and others added 3 commits October 29, 2026 05:24
Lane-Activity: pr-unblock/fix-ci
…report owner-PR adjudications as left open

The select job's no-stop skip read any User-opened PR as an owner PR, so a
hand-dispatched round on another person's PR lost its
feedback-untaken-threads stop. It now uses ownerAuthored with the base
adoption section, moved into address-feedback.mjs so dispatch and select
share one predicate.

On a PR the lanes App did not open, the resolve stand-in returns LEFT_OPEN,
and settleDisputes logs an adjudicator-closed thread as left open for the
person instead of resolved.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on 9581cc3628cd61e1e1790c323e38b6ce32349858; the summary is in this lane's pull request comment.

claude-lane: claude-review

Comment thread .github/scripts/pr-unblock/feedback-dispatch.mjs
kyle-sexton and others added 2 commits October 29, 2026 08:28
… sweep

An adopted owner PR whose feedback round escalated kept its failing
pr-unblock/address-feedback status after the owner resolved every thread
without pushing: owner PRs were left out of the stale-stop recovery, and
the idle pass ignored a non-releasing round. sweepIdleOwner now runs that
recovery for a head with a round, and sweepStale applies ownerSkip's Hold:
and idle gates to an owner PR.

An idle owner head with no round re-paid about six reads every tick when
the dispatch skipped. The idle pass now decides such a head only within
IDLE_OWNER_WINDOW_HOURS (two hours, four ticks) of the PR turning idle;
past it the head costs one check-run read, and any new update opens the
window again. The stale recovery takes no window, since resolving a
thread may not move the update time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the pr-verify-failed pr-verify failed this head; a pass at a new commit removes it. label Oct 29, 2026
@kyle-sexton

Copy link
Copy Markdown
Member Author

I'm arming at fdec2b14 under R85. The PR touches .github/workflows/pr-unblock.yml, but only its comment lines (header lines 12-17). It changes no job, condition, permission, input or step, so it cannot change what the review lanes or ci-status run.

A fresh independent security review at this head found no CRITICAL, HIGH or IMPORTANT finding. On every automatic path, the adoption and idle gates hold:

  • dispatchFeedback and ownerSkip;
  • the openPulls admission;
  • the trusted-actors list, read from .base;
  • the fast-forward-only push at pr-run-activity-write.yml:1849-1855.

It raised two LOW gaps, which I'll harden in a follow-up PR:

  • A Hold: body line added after dispatch is not re-checked when select runs.
  • On owner PRs, a lane-escalated needs-human label does not hold an address-feedback round, whereas fix-ci treats it as a hold.

The route matches the decision recorded on #10763.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 29, 2026
Merged via the queue into main with commit a9fc1c2 Oct 29, 2026
64 checks passed
@kyle-sexton
kyle-sexton deleted the feat/owner-pr-feedback branch October 29, 2026 13:23
@github-actions

Copy link
Copy Markdown
Contributor

Post-merge mutation testing

Killed 0, survived 0, timeout 0, no coverage 0, invalid 0, not run 7.
Covered-code score n/a; mutation score n/a.

File Killed Survived Timeout No coverage Invalid Not run
.github/scripts/pr-unblock/address-feedback.mjs 0 0 0 0 0 2
.github/scripts/pr-unblock/feedback-dispatch.mjs 0 0 0 0 0 4
.github/scripts/pr-unblock/publish-feedback.mjs 0 0 0 0 0 1

The run's summary has the full report.

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