Skip to content

fix(actions): Lopu controller repair β€” automation status comments defeat the "human comment" gate - #624

Merged
lopugit merged 2 commits into
github-actionsfrom
lopu/workflow-check-fix-33904964053
Sep 5, 2026
Merged

fix(actions): Lopu controller repair β€” automation status comments defeat the "human comment" gate#624
lopugit merged 2 commits into
github-actionsfrom
lopu/workflow-check-fix-33904964053

Conversation

@lopugit

@lopugit lopugit commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Lopu controller check repair

Lopu identified a failed PR check whose root cause is in the protected controller/workflow code.

Lopu controller repair β€” automation status comments defeat the "human comment" gate

Files changed (in $GITHUB_WORKSPACE/trusted only):

  • .github/workflows/resolve-pr-conflicts.yml β€” the route and review_detect conversation gates, plus the on: trigger comment that documents them.
  • .github/scripts/resolve-pr-conflicts-routing-contract.mjs β€” contract assertions pinning both gates and the marker convention they depend on.

No product branch was touched. PR #610's worktree is unrelated to this repair and carries no controller edit.

Diagnosis

This session was dispatched as lopu-review:issue-comment:5541425923:33904904840 β€” i.e. woken by what the controller classified as a human PR conversation comment. It was not one.

Comment 5541425923 on PR #610 is the deterministic develop-preview status comment:

<!-- thingtime-develop-pr-preview -->
### βœ… Develop S3 preview ready
- Commit: `3eea1252` …

.github/workflows/develop-pr-preview.yml writes it with
GH_TOKEN: ${{ secrets.CONFLICT_RESOLVER_PAT || github.token }}
(lines 243/253/261/308/438), and .github/scripts/deploy-develop-pr-preview.mjs
rewrites that same comment in place on every preview refresh.

Because the PAT is the repository owner's, GitHub reports the comment as
user.login == "lopugit", user.type == "User" β€” not Bot. Both conversation
gates tested identity alone:

&& (github.event_name != 'issue_comment'
    || (github.event.issue.pull_request
        && github.event.comment.user.type == 'User'))

so the comment passed as human. Three things compound it:

  1. issue_comment: types: [created, edited] β€” the preview comment is edited on
    every redeploy. updated_at was 18:15:19Z; run 33904904840 was created at
    18:15:21Z.
  2. Comment handoffs are deliberately non-coalescible
    (issue_comment | pull_request_review_comment) coalescible=false), which is
    correct for a real human question but means every edit gets its own worker.
  3. The result is a full model review session against a head that had not moved β€”
    PR feat(marketing): generated marketing suite β€” 1,600+ pages, animated walkthroughs, social image suiteΒ #610 3eea1252 had been reviewed 11 minutes earlier, by the same gate, with
    no change justified.

The workflow's own trigger comment asserted the invariant this broke:

Only human-authored events reach the review handoff below, so Lopu's own
comments cannot loop back into another model session.

Scale. event=issue_comment runs attributed to lopugit (the PAT identity, so
the ones that clear the gate): 12 on 08-30, 37 on 08-31, 33 on 09-01, 4 on 09-02,
7 on 09-03, 70 on 09-04. vercel[bot] / gitguardian[bot] comments are also
frequent but are type: Bot, so they were already stopped at route and cost
nothing.

This is a controller defect, not a defect in PR #610.

Fix

Identity is not sufficient evidence of a human, so the gates now also require the
absence of the marker that every control-plane comment carries and no human writes:

&& (github.event_name != 'issue_comment'
    || (github.event.issue.pull_request
        && github.event.comment.user.type == 'User'
        && !contains(github.event.comment.body, '<!-- thingtime-')))
&& (github.event_name != 'pull_request_review_comment'
    || (github.event.comment.user.type == 'User'
        && !contains(github.event.comment.body, '<!-- thingtime-')))

Applied identically in route and review_detect. Both branches sit behind
github.event_name != '<comment event>' ||, so no other event type is affected.
contains() in an if: is evaluated by the runner, never shell-expanded, so
reading comment.body here carries no injection risk.

Marker families are now distinguished explicitly, because they are not the same
thing:

  • Comment markers (thingtime-…, 30 of them) label controller status and
    conversation comments and arrive as comment events. These must keep the prefix.
  • PR-body markers (promotion-of, promotion-group, promotion-changelog*)
    label the pull request itself, are often hand-written by an operator, and arrive
    as pull_request_target: edited β€” never as a comment event. Filtering on them
    would be wrong, so the contract exempts them by name.

Validation

Check Result
resolve-pr-conflicts-routing-contract.mjs --self-test OK
node --test resolve-pr-conflicts-routing-contract.test.mjs (blocking lane) 10/10 pass
workflow-control-plane-contract.mjs --self-test OK
deploy-develop-pr-preview.mjs --self-test 120/120 pass
promotion-worker-routing-contract.mjs OK
rebase-ownership-routing-contract.sh OK
YAML parse of the edited workflow OK β€” 26 jobs, both gates carry the new clause

Negative control 1 β€” reverted only the review_detect gate to the identity-only
form: contract fails with
review_detect admits an issue_comment only from a User author with no control-plane marker.
Restored β†’ OK.

Negative control 2 β€” appended an unclassified <!-- lopu-unclassified-marker:v1 -->
to develop-pr-preview.yml: contract fails with
control-plane marker 'lopu-unclassified-marker:v1' must either keep the 'thingtime-' prefix … or be classified as a PR-body marker.
Restored β†’ OK.

Gate replay against the real payloads β€” every comment on PR #610 (19), old gate
vs new:

  • Comment 5541425923 (the one that woke this session): wakes β†’ blocked.
  • All 18 others: type: Bot, unchanged (already blocked).
  • Synthetic human comment (type: User, no marker): still wakes Lopu.
  • Totals: old 1/19 woke a review session, new 0/19.

Follow-up worth a decision (not changed here)

Comment 5542805370 β€” the rebase-result comment ("🧬 Rebased claude/marketing-suite
onto develop…") β€” carries no hidden marker. It is github-actions[bot] today,
so the identity check still stops it and there is no live leak. But it is the one
controller comment that would slip through if its writer ever moved to the PAT, and
the new contract cannot catch a missing marker. Adding
<!-- thingtime-ai-rebase-result:v1 --> to that writer would close it. I left it
alone: it is a different workflow, not the failure being repaired, and expanding the
change surface of a protected controller for a latent-only gap is the author's call.

Source Lopu workflow run

@lopugit

lopugit commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

❌ Develop S3 preview failed

The ordinary generated Vercel Preview remains available on the shared development runtime. Re-run this workflow after correcting the deployment, DNS, or CORS configuration.

Generic Vercel Preview deployments use the shared development runtime; this controller adds the stable exact-SHA alias and marker-scoped cleanup.

@github-actions github-actions Bot added the lopu: mergeable The PR branches can currently be merged without conflicts label Sep 4, 2026
@lopugit
lopugit temporarily deployed to develop-pr-624 September 4, 2026 18:46 Destroyed
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lopu review β€” PR #624 (head 1259700 vs base ae9a113)

The diagnosis holds up. I replayed it rather than taking it on trust, and the real numbers are slightly worse than the PR body claims: across the last 15 PRs, every type: User comment was the develop-preview controller comment posted under lopugit via the PAT β€” 17 of 17, with no genuine human comments in that window at all. The identity-only gate wasn't just leaky; on recent history it admitted nothing but controller traffic. Replaying both gate forms over the live payloads: PR #610 goes 1/20 β†’ 0/20 and PR #624 goes 1/1 β†’ 0/1, with 5541425923 the only changed verdict on #610, and a synthetic markerless User comment still waking Lopu.

I also confirmed route and review_detect are the complete set (lines 698/699 and 1025/1026 exclude comment events outright, and everything downstream keys on needs.route.outputs.execute), and that contains() is the right primitive here β€” startsWith would leak the promotion attestation/resolved/paused comments, which are posted as issue comments with the marker on the last line.

Two things I changed, and one I deliberately didn't:

1. The contract pinned a looser convention than the gate enforces. The new block scans with /<!--\s*([\w:.-]+)/gu, but the gate matches the literal <!-- thingtime- with exactly one space. A marker written with no space β€” or two β€” passes the contract and still wakes a full session. I verified the leak, then fixed it by capturing the spacing and pinning it alongside the prefix. Negative controls: spaceless fails, double-space fails, unclassified still fails as before, well-formed and promotion-* still pass.

(Amusingly, my first draft spelled the bad example literally in a code comment and the scanner β€” which reads every control-plane file, including itself β€” flagged it as a real violation. There's now a note in the block so the next person doesn't lose the same ten minutes.)

2. The gate silently drops human comments that quote a marker. contains() matches anywhere in the body, so GitHub's Quote reply β€” which copies the quoted comment's raw Markdown including its hidden marker β€” gets swallowed. Replying to Lopu is exactly the interaction most at risk, and it fails with no feedback to the author.

I did not touch the expression. Every simple narrowing is worse (startsWith leaks the trailing-marker comments; Actions expressions have no regex or newline literal, so line-anchoring isn't expressible), and doing it properly means moving classification into route's script or making "marker on the first line" a convention across several writers. Both are real changes to a protected controller and your call, not something to slip into a review. What I did fix is the comment this PR added, which asserted "no human writes one" β€” the one claim in the block that isn't true. It now records the limitation and the workaround.


Unrelated to your diff: the ❌ preview on this PR is a control-plane defect.

The red "Develop S3 preview failed … correct the deployment, DNS, or CORS configuration" comment is misleading β€” none of those is broken. Run 33907615031 failed with ENOENT … /product/remix: the build checks the head out into product/ and runs pnpm --dir product/remix install, and github-actions has no remix/ tree, so a control-plane PR can never build a product preview. #624/#623/#621/#619/#617/#614 all lack remix/package.json at head and all fail; #613/#612/#610 all have it and all pass. That's 11 consecutive control-plane PRs each burning a failed 45-minute-timeout build plus a privileged publish job β€” and it's invisible to gh pr checks, since the preview reports only as a comment.

Per the control-plane rule I did not put that on this branch. It's staged in the trusted checkout for its own PR against github-actions: preview eligibility now also requires the exact head to carry remix/package.json, so those PRs take the existing skip/reconcile path. Worth flagging that an early draft of that probe used product/remix/package.json and 404'd for every PR including the healthy ones β€” it would have disabled previews repository-wide. Replaying it against live heads before shipping is what caught it.

Everything green: routing contract OK, blocking lane 10/10, control-plane/promotion/rebase contracts OK, YAML parses to 26 jobs with both gates carrying both marker clauses.

Approving with the contract fix folded in.

πŸ€– Posted by Lopu (github-actions[bot]) from this review run.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lopu repository review

Lopu reviewed this PR against github-actions as Thingtime's principal PR and repository manager. Using Claude Opus 5.

Lopu found no justified local change to publish from this review pass.

Lopu review β€” PR #624 Β· automation status comments must not wake a Lopu session

lopu/workflow-check-fix-33904964053 β†’ github-actions @ a5465255
Compared against base 82786d9f (current github-actions tip).

What I compared

  • Full head-vs-base diff (2 files, +116/βˆ’4).
  • Both conversation gates in .github/workflows/resolve-pr-conflicts.yml
    (route and review_detect) and the on: trigger block they document.
  • The new contract assertions in
    .github/scripts/resolve-pr-conflicts-routing-contract.mjs, including the
    marker-convention scan.
  • The marker convention itself across .github/**.

Findings

The diagnosis is correct and non-obvious. The gate tested
github.event.comment.user.type == 'User', which is a statement about the
token
, not about the author. develop-pr-preview.yml posts its status comment
with secrets.CONFLICT_RESOLVER_PAT || github.token; when the PAT is present the
comment is attributed to a real User, so the identity check admits it. Combined
with issue_comment: types: [created, edited] and the deliberate
non-coalescibility of comment handoffs, every preview refresh spent a full model
review session on an unmoved head. Requiring the absence of the hidden
<!-- thingtime- marker closes it at the only place the two families of comment
actually differ.

contains() is the right primitive here. GitHub Actions' contains(search, item) is case-insensitive for strings, so a marker written <!-- Thingtime-…
would still be caught. There is no regex and no newline literal in the expression
language, which is why the PR documents the residual case rather than pretending
to solve it.

The accepted limitation is stated honestly and is the right call. A human
"Quote reply" copies the quoted comment's raw Markdown, marker included, so a
quoted reply to Lopu is silently dropped. The alternatives are worse: anchoring to
the start of the body would leak every controller comment that carries a
trailing marker (the promotion attestation is written that way). The workflow
comment tells a reader what to do instead ("post it as a new comment"). I would
keep it as-is.

The contract does more than restate the diff. The marker scan walks all of
.github/**, partitions markers into comment markers (must carry the
thingtime- prefix) and PR-body markers (promotion-*, exempt by name), and
pins the spacing β€” <!-- with exactly one space β€” because the gate matches
that literal. Pinning spacing is the part that actually protects the gate: a
marker written <!--thingtime-foo would satisfy a prefix-only check and still
sail through. That is a genuinely good invariant, not ceremony.

Both gates are updated, symmetrically, and the check_run /
workflow_run / push branches of the same if: are untouched, so no other
wake path is affected.

Follow-through this creates

Because the gate now depends on the marker convention, every future
controller-authored comment must carry <!-- thingtime-… -->. The contract
enforces that for anything written inside .github/**, which is where the
controller lives β€” so the invariant is machine-checked rather than remembered.
Worth calling out explicitly for anyone adding a comment writer elsewhere.

Interaction with the sibling controller PRs

Disjoint from #625 and #626 (see #626's report). All three MERGEABLE / CLEAN.

Changes made

None. The fix is correct, minimal, and self-pinning.

Validation run

$ node .github/scripts/resolve-pr-conflicts-routing-contract.mjs --self-test
resolve-pr-conflicts routing contract: self-test OK    (exit 0)

This exercises the new gate assertions and the marker/spacing scan against the
live .github tree, so a passing run means the convention actually holds today.

Recommendation

Approve and merge.

View Lopu workflow run

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lopu review β€” both red signals on this PR are control-plane defects, not yours.

The diff is correct and I changed nothing in this PR's worktree.

The ❌ preview comment above is not about your change

The preview build job checks the PR head out into product/ and then runs corepack pnpm --dir product/remix install --frozen-lockfile. github-actions carries no remix/ workspace, so it dies with ENOENT … /product/remix and the controller publishes the failure comment. It's deterministic for this PR shape β€” every control-plane preview worker today failed identically (33909387490, 33907615031 here; 33888936460, 33887678048, 33886516079, 33885726085, 33885038530, 33883924348, 33882576994 for codex/preview-manager-reliability), interleaved with successes for product PRs. Spot-checked two of the older jobs: same step, same error.

#625 already fixes this, and its fix is better than the one I'd written β€” it probes the head for remix/package.json instead of hardcoding a base-branch name, and it rethrows non-404 statuses so a transport fault can't be silently misread as "nothing to preview". I reverted mine rather than open a competing change. Verified its script independently: --self-test 138/138.

Note the stale ❌ above won't retract itself once #625 lands β€” it's a deterministic status comment, not mine to edit. Ignore it here.

Contract advisories (non-blocking) β€” fail is a cancellation, not a failure

Run 33909377070 finished every step of both jobs successfully β€” verify 15/15, advisories 5/5, both job outputs set β€” and is still concluded cancelled, which gh pr checks renders as fail.

control-plane-ci.yml uses the constant group workflow-control-plane-ci with cancel-in-progress: true β€” one global slot for the whole repository. #625's run 33909382726 was created 4 seconds after yours and took it; yours was cancelled at 19:06:34, after its work had already finished. Recurring: 33880764258, 33880195132 and 33879742035 were each cancelled by the push run to github-actions that landed seconds later.

I've opened a separate controller repair scoping the group to workflow-control-plane-ci-${{ github.event.pull_request.number || github.ref }}, keeping cancel-in-progress (superseding an earlier push on the same subject is the point), plus a contract assertion so the global scope can't come back. Different files from #625, so they can land in either order.

Two things your change confirms

  1. This session is live evidence for your diagnosis. It was dispatched by comment 5545037869 β€” the <!-- thingtime-develop-pr-preview --> status comment, authored under the PAT so user.type == 'User', updated_at 19:07:00Z. Exactly the class of comment you're blocking, admitted by exactly the identity-only gate you're replacing.
  2. Merging here is sufficient β€” no promotion to main needed. resolve-pr-conflicts.yml on main is a 152-line metadata-only listener that forwards unconditionally to …@github-actions; both gates you edited live in the implementation on this branch.

The documented "Quote reply" limitation is the right call to write down rather than work around β€” Actions expressions have no regex and no newline literal, and anchoring to the start of the body instead would leak every controller comment that carries a trailing marker.

CodeQL: no open alerts on a5465255.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lopu review β€” PR #624

This session was dispatched as lopu-review:issue-comment:5545037869 β€” i.e. woken by the <!-- thingtime-develop-pr-preview --> "❌ Develop S3 preview failed" comment on this PR. That is the exact defect this PR fixes, reproducing itself on the branch that fixes it. Under the proposed gate the comment contains <!-- thingtime-, so it would have been dropped at route and this session would never have started.

The failing check is real, but it is not this PR

Every check on a5465255 is green or skipped. The red signal is the develop preview, which reports by comment rather than by a check on this head β€” the pull_request_target legs pass (metadata handoff only) and the repository_dispatch worker legs on main fail. From run 33911073742:

ERROR  ENOENT: no such file or directory, lstat '.../thingtime/product/remix'

develop-pr-preview.yml checks the head out into product/ and runs corepack pnpm --dir product/remix install. prepare authorizes on trust only β€” pullRequestShapeIssue never asks whether the head is buildable β€” and a github-actions-based head carries just .github, PRs, graphify-out and root docs. So it cannot ever succeed for a control-plane PR. Reproducible, not an outage or a stale result.

I made no controller edit and opened no fix branch: #625 already repairs precisely this, in the right file, with a remix/package.json head probe. A second Lopu branch on that file would only have competed with it.

Worth flagging that the two defects compound: each control-plane PR draws a failing-preview comment (#624 5545037869, #625 5545256554, #626 5545460907), and each of those comments currently spends a full Lopu session. #625 stops the comments; #624 stops the wake-ups.

Verification of the change

All four validations in the description reproduce at a5465255 (routing contract self-test OK; node --test 10/10; control-plane contract OK; preview self-test 120/120), as does negative control 1 β€” reverting only the review_detect gate fails with review_detect admits an issue_comment only from a User author with no control-plane marker. The slice bounds are tight (route: 632 β†’ manage_rebases: 692), so the route assertion can't accidentally satisfy itself on review_detect's gate.

I also ran an independent replay, wider than the #610 one β€” the 100 most recent repo-wide issue comments:

comments that wake a review
old gate (identity only) 28
new gate (identity + no marker) 3

The 3 survivors are genuine human comments (the TestFlight notes on #596). Zero false negatives; the 25 suppressed are all deterministic status comments. Separately, resolve-pr-conflicts.yml runs on issue_comment: 56 / 20 / 39 / 85 across 09-01 β†’ 09-04.

The scoping is right for a reason worth stating explicitly: comment events feed nothing in this workflow except the Lopu conversation handoff β€” the lopu-conversation-pr{n} concurrency key and the issue-comment:/inline-comment: dispatch id. No controller flow observes a marked comment, so filtering them regresses no behaviour.

The spacing assertion is the sharpest part of the contract, and I'd call it out as the thing most worth keeping: <!--thingtime-… passes a prefix-only check but does not match the gate's literal, so it would still wake a session. Confirmed it fails on both no-space and two-space forms.

One gap for your judgement β€” not changed

The contract pins markers by scanning source text, so a marker assembled at runtime is invisible to it. promote-features-to-main.mjs:1655,1681 emits:

`<!-- ${PROMOTION_STANDASIDE_MARKER} -->`

<!--(\s*)([\w:.-]+) can't match past ${, so this one is never pinned. It's safe today β€” PROMOTION_STANDASIDE_MARKER = "thingtime-promotion-standaside:v1" (line 1613) renders to the exact literal with one space β€” but nothing holds it there. This is the same class as the missing-marker gap you already documented for the rebase-result comment, and I agree with the disposition you took there, so I've left it alone rather than widening a protected controller for a latent-only gap. Your call.

Two smaller notes: your follow-up on rebase-pr-stacks.yml:2916 checks out exactly as described β€” no marker, but GH_TOKEN: ${{ github.token }} means it posts as github-actions[bot], so the identity half still stops it. And the marker scan is a broad net: <!-- -->, <!-- ... --> and <!-- prettier-ignore --> all fail it. .md is excluded, which removes the likeliest source of benign HTML comments, and fail-closed with an actionable message is the right bias β€” just noting it so it isn't a surprise later.

No CodeQL alerts on this head. No changes made to this branch β€” the diff is correct, minimal, and its claims hold. Recommend merging it together with #625; between them they close both halves of the loop that produced this session.

πŸ€– Posted by Lopu, Thingtime's automated PR reviewer.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

βœ… Lopu β€” reviewed and validated; no changes needed

Compared the full head (a5465255) against github-actions @ ae9a113a.
The root cause is correctly identified: author identity alone never
established "human".
Deterministic controller comments are posted with
CONFLICT_RESOLVER_PAT, so GitHub reports comment.user.type == 'User' under
the repository owner β€” exactly what the old gate admitted. With edited as a
trigger type and comment handoffs deliberately non-coalescible, every refresh of
a long-lived status comment burned a full Lopu session on an unchanged head.

Requiring both a User author and the absence of <!-- thingtime- is the
right fix, and it's applied symmetrically across issue_comment and
pull_request_review_comment in both route and review_detect.

What raises this above a one-line patch

It pins the convention the gate depends on. The contract scans every .github
file and requires each <!-- marker to either carry the thingtime- prefix or
be a named PR-body marker.

The spacing assertion is the sharp part: the gate matches
<!-- thingtime- as one literal, so a marker written with zero or two spaces
would satisfy a prefix-only check and still wake a session. Capturing and
pinning the spacing closes that gap. The scanned > 20 / markers.size > 10 /
commentMarkers > 10 floors stop the whole block silently degrading to a no-op
β€” good instinct, that's the usual failure mode for a scan-based contract.

The accepted limitation is the right call, and worth flagging to humans

contains() matches anywhere in the body, so GitHub's "Quote reply" β€” which
copies the quoted comment's raw Markdown including its hidden marker β€” gets
dropped silently. The workflow comment documents this honestly and explains why
it isn't fixable here (Actions expressions have no regex and no newline literal,
so it can't be anchored; anchoring to the start of the body would leak
controller comments carrying a trailing marker).

Accepting it is right, but it's the one behaviour here most likely to surprise
someone later: if a question to Lopu goes unanswered, post it as a new comment
rather than a quoted reply.

$ node .github/scripts/resolve-pr-conflicts-routing-contract.mjs --self-test
resolve-pr-conflicts routing contract: self-test OK

The self-test exercises the new marker/spacing scan against the real .github
tree, so it also confirms the current control plane already satisfies the
convention being pinned.

Checks: 27 pass / 0 fail. Ready to merge β€” worth landing alongside #626;
together they remove the two largest sources of wasted sessions and false-red
checks.

Lopu Β· automated repository review

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

πŸ•΅οΈ Lopu β€” gate fix is right; flagging the byte headroom before it bites

Reviewed against github-actions@ae9a113a. Contract self-tests pass:

node .github/scripts/resolve-pr-conflicts-routing-contract.mjs --self-test  β†’ OK
node .github/scripts/workflow-control-plane-contract.mjs      --self-test  β†’ OK

The diagnosis holds

Author identity never established "human": deterministic controller comments are posted with CONFLICT_RESOLVER_PAT, so GitHub reports comment.user.type == 'User' under the repository owner β€” exactly what the old gate admitted. With edited as a trigger type, every rewrite of a self-updating status comment spent a full, non-coalescible Lopu session on an unchanged head. Requiring both a User author and the absence of <!-- thingtime-, on both gates, is the right shape.

The convention-pinning block is the strongest part. A gate is only as good as the convention it reads, and pinning the spacing isn't over-engineering: the gate matches '<!-- thingtime-' as one literal, so a marker written <!--thingtime-x would satisfy a prefix-only check and still wake a session. Real hole, closed. Classifying promotion-* as body markers by name rather than by prefix is also right β€” those arrive as pull_request_target edited, never as a comment event.

I agree with shipping the "Quote reply" limitation as documented rather than half-fixing it. Actions expressions have no regex and no newline literal, so it genuinely can't be anchored to a line start, and anchoring to the body start would leak every controller comment carrying a trailing marker.

(For what it's worth, this comment carries <!-- thingtime-lopu-conversation:v1 -->, so once this lands Lopu's own conversational comments stop re-waking Lopu β€” which is the intended outcome and worth confirming in practice after merge.)

⚠️ The thing I'd weigh before merging

resolve-pr-conflicts.yml   505,005 β†’ 507,011 bytes
MAX_WORKFLOW_BYTES                   510,000
headroom left                          2,989

That's ~0.6% of the limit remaining. The guard exists because GitHub leaves an oversized revision permanently pre-queued β€” no jobs, no logs, no cancellable run graph β€” a failure mode with no in-band signal at all. The contract catches it, so nothing breaks silently, but the next controller change of this size will simply be unable to land in this file.

This PR adds ~26 lines of header commentary to the workflow explaining the gate. Most of that reasoning is already duplicated in the routing-contract block this same PR adds. Moving the workflow-side prose there would buy back most of the headroom without losing the explanation β€” the contract file has no byte ceiling.

I haven't made that edit: it's a judgement call about where you want the narrative to live, not a defect. But at 2,989 bytes it's worth deciding now rather than discovering it on the next controller repair.

Merge ordering

No file overlap with #625 (deploy-develop-pr-preview.mjs) or #626 (control-plane-ci.yml + workflow-control-plane-contract.mjs), so the three controller PRs merge cleanly in any order.

@github-actions github-actions Bot added the lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

πŸ€– Lopu detected an out-of-date PR branch

Status: Work detected β€” Lopu is taking ownership.

Current phase: Entering Lopu's serialized PR-resolution queue.

Estimated completion: around 06:28 UTC (~20 minutes; this adjusts as the queue moves).

Next automatic check-in: within 10 minutes while work remains active. You can stay on this PR; there is no need to find the Actions run.

Time conversion (UTC source)

Moment UTC Los Angeles Melbourne
Updated 2026-09-05 06:08 UTC (UTC+00:00) 2026-09-04 23:08 PDT (UTC-07:00) 2026-09-05 16:08 AEST (UTC+10:00)
Estimated finish 2026-09-05 06:28 UTC (UTC+00:00) 2026-09-04 23:28 PDT (UTC-07:00) 2026-09-05 16:28 AEST (UTC+10:00)

Los Angeles and Melbourne use their real IANA time zones, so PDT/PST and AEST/AEDT offsets change automatically.

Lopu queue and PR pulse

Scope Metric Count
Repository Open PRs 25
Repository Conflicting 1
Repository Out-of-date with target 0
Repository GitHub state unknown 7
Repository Part of an open stack 2
Repository Touch files changed by another open PR 15
Repository Target a non-root branch without an open parent PR 0
This resolver batch Admitted snapshots 3
This resolver batch Currently resolving 0
This resolver batch Waiting 3
This resolver batch Finished 0

Related PR context

  • Stack: No open parent or child PR currently links to this branch.
  • Target: github-actions is a repository root/integration branch.
  • Changed-file overlap: No changed paths overlap another open PR in this snapshot.

Exact branch pair: github-actions β†’ lopu/workflow-check-fix-33904964053.

Timeline

  • 06:08 UTC β€” Detected that github-actions needs to be merged into lopu/workflow-check-fix-33904964053; assigning the exact snapshot to the resolver queue.

@github-actions github-actions Bot added lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue and removed lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

πŸ•΅οΈ Lopu β€” confirmed, plus the obligation this gate quietly creates

The diagnosis is right and non-obvious, and I want to name the part that makes it non-obvious: comment.user.type == 'User' is a statement about the token, not about the author. develop-pr-preview.yml posts with secrets.CONFLICT_RESOLVER_PAT || github.token, so when the PAT is present GitHub reports the deterministic status comment as a real User. Combined with issue_comment: types: [created, edited] and the deliberate non-coalescibility of comment handoffs, every preview refresh bought a full model review session on an unmoved head. Requiring the absence of <!-- thingtime- closes it at the only place the two comment families actually differ.

Two things worth adding.

contains() is case-insensitive for strings, which the diff doesn't mention but is a real property here β€” a marker written <!-- Thingtime-… would still be caught. Free robustness.

The contract does the load-bearing work, and it's the spacing assertion that matters. Pinning the thingtime- prefix alone would leave the gate defeatable by a marker written <!--thingtime-foo: prefix check passes, contains(body, '<!-- thingtime-') doesn't match, session wakes. Capturing and pinning the spacing is what actually protects the expression. Partitioning markers into comment markers (prefix required) versus PR-body markers (promotion-*, exempt by name, since those arrive as pull_request_target edited and never as a comment event) is the right cut, and failing loudly on a marker in neither set is the right default.

The obligation: this gate now depends on a convention, so every future controller-authored comment must carry <!-- thingtime-… -->. The contract enforces that for anything inside .github/**, which is where the controller lives β€” so it's machine-checked rather than remembered. Worth knowing for anyone adding a comment writer outside that tree, where nothing would catch it.

On the accepted limitation β€” a human "Quote reply" copies the quoted comment's raw Markdown, marker included, so replying to Lopu is exactly the case that can be swallowed β€” I'd keep it as written. The alternatives are worse (anchoring to the start of the body leaks every controller comment carrying a trailing marker, and the promotion attestation is written that way), Actions expressions have no regex or newline literal to narrow it with, and the workflow comment tells a reader what to do instead. Documenting a sharp edge you can't remove beats pretending it isn't there.

$ node .github/scripts/resolve-pr-conflicts-routing-contract.mjs --self-test
resolve-pr-conflicts routing contract: self-test OK   (exit 0)

That run exercises the new gate assertions and the marker/spacing scan against the live .github tree β€” so a pass means the convention actually holds today, not just that the assertions parse.

Merge-order note for all three controller repairs is on #626: disjoint files, all MERGEABLE/CLEAN against the current tip, any order works, but #626 first is easiest to read.

No changes made to this branch.

@lopugit
lopugit merged commit 1485e32 into github-actions Sep 5, 2026
83 of 85 checks passed
@github-actions github-actions Bot removed the lopu: mergeable The PR branches can currently be merged without conflicts label Sep 5, 2026
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