fix(actions): Lopu controller repair β automation status comments defeat the "human comment" gate - #624
Conversation
β 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. |
|
Lopu review β PR #624 (head 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 I also confirmed 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 (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. I did not touch the expression. Every simple narrowing is worse ( 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 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 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 ( |
Lopu repository reviewLopu 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
What I compared
FindingsThe diagnosis is correct and non-obvious. The gate tested
The accepted limitation is stated honestly and is the right call. A human The contract does more than restate the diff. The marker scan walks all of Both gates are updated, symmetrically, and the Follow-through this createsBecause the gate now depends on the marker convention, every future Interaction with the sibling controller PRsDisjoint from #625 and #626 (see #626's report). All three Changes madeNone. The fix is correct, minimal, and self-pinning. Validation runThis exercises the new gate assertions and the marker/spacing scan against the RecommendationApprove and merge. |
|
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 changeThe preview #625 already fixes this, and its fix is better than the one I'd written β it probes the head for Note the stale β above won't retract itself once #625 lands β it's a deterministic status comment, not mine to edit. Ignore it here.
|
|
Lopu review β PR #624 This session was dispatched as The failing check is real, but it is not this PREvery check on
I made no controller edit and opened no fix branch: #625 already repairs precisely this, in the right file, with a Worth flagging that the two defects compound: each control-plane PR draws a failing-preview comment (#624 Verification of the changeAll four validations in the description reproduce at I also ran an independent replay, wider than the #610 one β the 100 most recent repo-wide issue comments:
The 3 survivors are genuine human comments (the TestFlight notes on #596). Zero false negatives; the 25 suppressed are all deterministic status comments. Separately, The scoping is right for a reason worth stating explicitly: comment events feed nothing in this workflow except the Lopu conversation handoff β the The spacing assertion is the sharpest part of the contract, and I'd call it out as the thing most worth keeping: One gap for your judgement β not changedThe contract pins markers by scanning source text, so a marker assembled at runtime is invisible to it. `<!-- ${PROMOTION_STANDASIDE_MARKER} -->`
Two smaller notes: your follow-up on 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. |
β Lopu β reviewed and validated; no changes neededCompared the full head ( Requiring both a What raises this above a one-line patchIt pins the convention the gate depends on. The contract scans every The spacing assertion is the sharp part: the gate matches The accepted limitation is the right call, and worth flagging to humans
Accepting it is right, but it's the one behaviour here most likely to surprise The self-test exercises the new marker/spacing scan against the real Checks: 27 pass / 0 fail. Ready to merge β worth landing alongside #626; Lopu Β· automated repository review |
π΅οΈ Lopu β gate fix is right; flagging the byte headroom before it bitesReviewed against The diagnosis holdsAuthor identity never established "human": deterministic controller comments are posted with 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 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
|
π€ Lopu detected an out-of-date PR branchStatus: 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)
Los Angeles and Melbourne use their real IANA time zones, so PDT/PST and AEST/AEDT offsets change automatically. Lopu queue and PR pulse
Related PR context
Exact branch pair: Timeline
|
π΅οΈ Lopu β confirmed, plus the obligation this gate quietly createsThe diagnosis is right and non-obvious, and I want to name the part that makes it non-obvious: Two things worth adding.
The contract does the load-bearing work, and it's the spacing assertion that matters. Pinning the The obligation: this gate now depends on a convention, so every future controller-authored comment must carry 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. That run exercises the new gate assertions and the marker/spacing scan against the live Merge-order note for all three controller repairs is on #626: disjoint files, all No changes made to this branch. |
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/trustedonly):.github/workflows/resolve-pr-conflicts.ymlβ therouteandreview_detectconversation gates, plus theon: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
5541425923on PR #610 is the deterministic develop-preview status comment:.github/workflows/develop-pr-preview.ymlwrites it withGH_TOKEN: ${{ secrets.CONFLICT_RESOLVER_PAT || github.token }}(lines 243/253/261/308/438), and
.github/scripts/deploy-develop-pr-preview.mjsrewrites 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"β notBot. Both conversationgates tested identity alone:
so the comment passed as human. Three things compound it:
issue_comment: types: [created, edited]β the preview comment is edited onevery redeploy.
updated_atwas18:15:19Z; run33904904840was created at18:15:21Z.(
issue_comment | pull_request_review_comment) coalescible=false), which iscorrect for a real human question but means every edit gets its own worker.
PR feat(marketing): generated marketing suite β 1,600+ pages, animated walkthroughs, social image suiteΒ #610
3eea1252had been reviewed 11 minutes earlier, by the same gate, withno change justified.
The workflow's own trigger comment asserted the invariant this broke:
Scale.
event=issue_commentruns attributed tolopugit(the PAT identity, sothe 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 alsofrequent but are
type: Bot, so they were already stopped atrouteand costnothing.
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:
Applied identically in
routeandreview_detect. Both branches sit behindgithub.event_name != '<comment event>' ||, so no other event type is affected.contains()in anif:is evaluated by the runner, never shell-expanded, soreading
comment.bodyhere carries no injection risk.Marker families are now distinguished explicitly, because they are not the same
thing:
thingtime-β¦, 30 of them) label controller status andconversation comments and arrive as comment events. These must keep the prefix.
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 themwould be wrong, so the contract exempts them by name.
Validation
resolve-pr-conflicts-routing-contract.mjs --self-testnode --test resolve-pr-conflicts-routing-contract.test.mjs(blocking lane)workflow-control-plane-contract.mjs --self-testdeploy-develop-pr-preview.mjs --self-testpromotion-worker-routing-contract.mjsrebase-ownership-routing-contract.shNegative control 1 β reverted only the
review_detectgate to the identity-onlyform: 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 withcontrol-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:
5541425923(the one that woke this session): wakes β blocked.type: Bot, unchanged (already blocked).type: User, no marker): still wakes Lopu.1/19woke a review session, new0/19.Follow-up worth a decision (not changed here)
Comment
5542805370β the rebase-result comment ("𧬠Rebasedclaude/marketing-suiteonto
developβ¦") β carries no hidden marker. It isgithub-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 italone: 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