Skip to content

fix(fr-pass): never no-op in silence — job if swallowed self-signoffs, reaction was 403ing - #106

Merged
LukasWodka merged 3 commits into
developfrom
fix/fr-pass-silent-noop
Jul 30, 2026
Merged

fix(fr-pass): never no-op in silence — job if swallowed self-signoffs, reaction was 403ing#106
LukasWodka merged 3 commits into
developfrom
fix/fr-pass-silent-noop

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#1319

Root cause — two separate defects, both invisible

1. The job-level if decided policy, so the "no" had no voice

if: | contains(github.event.comment.body, '/fr-pass') && (author_association == 'MEMBER' || author_association == 'OWNER') && github.event.comment.user.login != github.event.issue.user.login # <-- D30

That last clause is the self-signoff rule (D30). A false job if skips the job
with zero steps executed — so there is no step left to react with, no step
left to comment with, and the run reports skipped. The reviewer sees a green-ish
run and assumes the card moved.

Every one of the 13 comments from 2026-07-29 is explained by that clause alone:

repoPRPR authorcommenterrunconclusion
cli#415LukasWodkaLukasWodka30437971574skipped
cli#416saadqbalLukasWodka30437971958success
cli#417LukasWodkaLukasWodka30437974071skipped
cli#422LukasWodkaLukasWodka30437975834skipped
py-package#346LukasWodkaLukasWodka30437979317skipped
py-package#347LukasWodkaLukasWodka30437977885skipped
design-system#74LukasWodkaLukasWodka30438031083skipped
design-system#75app/dependabotLukasWodka30438022951success
design-system#77waqaskhanroghaniLukasWodka30438024051success
design-system#78saqlainsyed007LukasWodka30438026339success
design-system#79saqlainsyed007LukasWodka30438027561success
design-system#80LukasWodkaLukasWodka30438034691skipped
design-system#81LukasWodkaLukasWodka30438032330skipped

13/13 correlation, no residue: author == commenter → skipped, author != commenter → success.
All 13 comments were /fr-pass verbatim, all by LukasWodka, all author_association: MEMBER,
all inside 51 seconds. So the body test and the association test were never the problem, and there
was no event-delivery variance or API flake — the payloads differed only in issue.user.login.

The ticket lists 6 non-advances; the actual figure is 8. cli#415/#417/#422 and
py-package#346/#347 also died here, and their runs are outside the default
gh run list window unless you filter by workflow.

2. The reaction never worked — on any path, including the successes

All 13 comments still carry zero reactions today, the four design-system
successes included. From the log of a run that did advance its card
(30438027561):

→ #79: FR on staging → Ready for prod
##[group]Run if [ "$RESULT" = "advanced" ]; then
RESULT: advanced
gh: Resource not accessible by integration (HTTP 403)

The step used secrets.GITHUB_TOKEN, and the same log shows the token it got:

##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
Packages: read

tracebloc sets default_workflow_permissions: read at org level (verified on
the org and on backend/cli/design-system), the callers declare no permissions:,
and a called workflow can only narrow the caller job's token, never widen it
so adding permissions: issues: write here would look like a fix and silently do
nothing. || true then swallowed the 403. Net effect: 👍/👎 has never once been
posted by this workflow.

Ruled out

  • Board rename — verified against the live board. Project chore: add auto-add to engineer kanban workflow #2's Status
    options are Backlog, North Stars, Ready, In progress, Code review, On dev, FR on staging, Ready for prod, Prod, Cancelled. The code matches
    FR on staging → Ready for prod, which is current and correct. Only the
    header comment was stale (it described the retired two-gate flow).
  • conclusion: skipped masking a required check — "FR pass comment" is not a
    required status check on any protected branch (those are gate / gate, tests,
    lint). Nothing was gated on it.
  • Most skipped runs are correct. The caller fires on every issue_comment,
    so ordinary comments legitimately skip. Only the 13 above were commands.

The fix

Nothing may be decided in the job-level if. It is now only a cheap
"is this even about /fr-pass" filter; authorisation, self-signoff, board
membership and the column check all happen in steps that can report.

  • decide() + an EXIT trap publish an outcome on every exit, including an
    unplanned one — so a crash mid-script still yields 👎 and an explanation.
  • All writes use PROJECTS_KANBAN_TOKEN (already posts cross-repo PR comments
    from wip-limit-check.yml) and no || true — a write that fails now fails the
    run. There is deliberately no GITHUB_TOKEN fallback: permissions: contents: read
    zeroes every other scope, so a retry could never post (caught by Bugbot on the
    first commit — the original version had one, and it was dead code).
  • Reporting step is if: always() with no steps.* guard — the case where the
    first step produced nothing is the case most in need of reporting. It was
    previously gated on steps.advance.outputs.result != '', i.e. muted exactly then.
  • Real problems get a short PR comment, not just a bare 👎.
  • Zero caller changes. All 17 fr-pass-comment-caller.yml files stay as they
    are; secrets: inherit already passes the PAT.

Fixed in passing, all of them silent-no-op paths:

WasNow
2>/dev/null || PROJ='{}' — expired token, rate limit and "not on the board" all produced the same 👎distinct api-error, with the API's own message quoted, and a red run
projectItems(first: 10) with no totalCount — a truncated page read as absencefirst: 20 + totalCount; over-limit is reported, not guessed
duplicate cards resolved by head -1ambiguous-item, refuses to guess
author_association used as a permission check — a private org membership reads as CONTRIBUTOR/NONE, silently rejecting a real reviewerasks for the actual repository permission (write/maintain/admin), falling back to association only if that API is unreachable. Also states D30's "not a triage/read outside collaborator" intent directly instead of approximating it
a Status rename would report "wrong column" for every card foreverboth hard-coded column names are checked for existence first → missing-option + red run
contains() matched /fr-passport and quoted repliesmust be the first token on a line; leading whitespace, trailing text, CRLF and case all tolerated
comment body never used, but no env: discipline for when it isbody passed via env:, never interpolated into the script
header described the retired FR on dev → Ready for staging gaterewritten to the current single staging gate

One decision for you

The self-signoff rule is now allow-self-signoff, default false — i.e. D30 as
written, unchanged.
With that default, the 8 comments above would get 👎 + "needs a
second pair of eyes" instead of advancing. That fixes the reported bug (the board
stops lying) but does not satisfy the ticket's acceptance line as written
("10 rapid consecutive /fr-pass comments all advance"), because those were all
self-signoffs on release-train PRs.

I did not flip it, because quietly dropping a deliberate governance rule inside a
bug fix is the wrong place for that call. If you want your own batches to advance,
it is a one-line change to the default in this file (the callers pass no inputs, so
the default is the org-wide policy). Say the word and I will push it here.

Verification

actionlint -no-color -oneline -shellcheck shellcheck (this repo's exact CI gate,
run locally): 0 findings. YAML parses; both run: blocks pass bash -n and a
standalone shellcheck -s bash.

Beyond linting, both run: blocks were extracted and executed against the live
GitHub API
behind a gh shim that passes reads through and blocks every write
(board mutation, reaction, comment) — verified afterwards that no reaction and no
comment appeared on any real thread, and that the board was untouched.

Replay of all 13 real 2026-07-29 events (live API):

cli#415 LukasWodka | self-signoff react=-1 comment=yes
cli#416 LukasWodka | already-advanced react=-1 comment=yes
cli#417 LukasWodka | self-signoff react=-1 comment=yes
cli#422 LukasWodka | self-signoff react=-1 comment=yes
tracebloc-py-package#346 LukasWodka | self-signoff react=-1 comment=yes
tracebloc-py-package#347 LukasWodka | self-signoff react=-1 comment=yes
design-system#74 LukasWodka | self-signoff react=-1 comment=yes
design-system#75 LukasWodka | already-advanced react=-1 comment=yes
design-system#77 LukasWodka | already-advanced react=-1 comment=yes
design-system#78 LukasWodka | already-advanced react=-1 comment=yes
design-system#79 LukasWodka | already-advanced react=-1 comment=yes
design-system#80 LukasWodka | self-signoff react=-1 comment=yes
design-system#81 LukasWodka | self-signoff react=-1 comment=yes

The 8 that were silent are now self-signoff; the 5 that advanced back then read
already-advanced because they are in Ready for prod / Prod today. 13/13 now
react. Zero silent no-ops.

Every terminating path, driven with a stubbed board read:

pathoutcomereactioncommentrun
card in FR on stagingadvanced👍— (happy path stays quiet)green
body mentions but does not issue the commandnot-a-commandnone (by design)nogreen
author signs off own PRself-signoff👎yesgreen
commenter lacks write accessnot-authorised👎yesgreen
card in another columnwrong-column👎yesgreen
card already past the gatealready-advanced👎yesgreen
card on board, Status emptyno-status👎yesgreen
item not on the boardnot-on-project👎yesgreen
item resolves to 2 cardsambiguous-item👎yesgreen
item on >20 projects (page cut off)api-error👎yesred
Ready for prod option missing (rename)missing-option👎yesred
board read fails (401)api-error👎yesred
board write fails (502)mutation-failed👎yesred
first step produced no output at allinternal-error👎yesred

Body parsing, verified case by case — recognised: /fr-pass, /fr-pass ,
/FR-PASS, /fr-pass looks good to me, /fr-pass on its own line after prose,
CRLF. Correctly not the command: /fr-passport renewed, you can /fr-pass it later, > /fr-pass.

Happy path checked end to end: outputs from=FR on staging, to=Ready for prod,
result=advanced, and the mutation carries -f o=f69f1c12 — the real option ID of
Ready for prod on project #2.

Authorisation checked against the live API: octocatnot-authorised + 👎;
LukasWodka with author_association: NONE → still authorised, which is the
private-membership false-negative the old if would have dropped in silence.

Review

Bugbot: 1 finding on the first commit — the dead GITHUB_TOKEN write fallback
described above. Fixed in 2nd commit; re-linted and the whole path matrix re-run
against it.

Note on landing

Targets develop per the repo convention (#99/#104develop, then a
chore(promote): develop → main). The callers pin @main, so this changes nothing
in production until that promotion. Not merging — that is yours.


Note

Medium Risk
Changes org-wide release governance automation (who may sign off, kanban writes, and thread feedback) using a PAT with cross-repo write; behavior is more correct but materially affects every /fr-pass invocation once callers pick up @main.

Overview
Fixes backend#1319 by rewriting the reusable /fr-pass kanban handler so every real command gets visible feedback instead of a skipped job or a silent 403.

Policy and job gating: The job if now only checks that the comment body mentions /fr-pass. Authorisation, D30 self-signoff, board lookup, and column rules run inside a script step that can always react and explain. Self-signoff still blocks by default but posts 👎 and a clear message; release-train / hotfix-backmerge promotion PRs are exempt automatically. Optional allow-self-signoff (default false) can widen that org-wide.

Reporting and tokens: Reactions and explanation comments use PROJECTS_KANBAN_TOKEN with explicit permissions: contents: read—no GITHUB_TOKEN writes or || true. A follow-up step runs if: always() to post 👍/👎 and thread comments for failures; happy-path advances stay a bare 👍.

Handler robustness:decide() plus an EXIT trap publish outcomes on crashes. Command parsing requires /fr-pass as the first token on a line. Write access is checked via the collaborators permission API (with association fallback). GraphQL errors are surfaced distinctly; project item paging goes to 20 with totalCount; duplicate cards are rejected instead of guessed; required Status options are validated before moves.

Reviewed by Cursor Bugbot for commit 864a837. Bugbot is set up for automated code reviews on this repo. Configure here.

Two independent defects made /fr-pass unreliable and, worse, indistinguishable
from success. On 2026-07-29, 13 genuine /fr-pass comments produced 5 advances,
8 silent skips and zero reactions.
1. The job-level `if` decided policy. It carried
`comment.user.login != issue.user.login` (the D30 self-signoff rule). When the
reviewer was also the PR author, the whole job was skipped with zero steps —
nothing left to react or explain, and a run that reads `skipped`. All 8 drops
were Lukas signing off his own PRs; all 5 advances were someone else's. The
rule is unchanged, but it now lives in a step that reports it.
2. The reaction never worked, on any path. It used GITHUB_TOKEN, which is
read-only org-wide (`default_workflow_permissions: read`), and a called
workflow cannot widen the caller job's token. Every POST returned
"Resource not accessible by integration (HTTP 403)" and `|| true` swallowed
it — so even the 5 successful advances left no visible mark. Writes now go
through PROJECTS_KANBAN_TOKEN (already used cross-repo by wip-limit-check).
Every terminating path now leaves a signal: 👍 on advance, 👎 plus a short
explanatory comment on anything that is a real problem. The only silent outcome
is prose that mentions the string without issuing the command. Real errors also
fail the run, so a write that cannot reach the thread still shows up red.
Also closed while in here: swallowed GraphQL errors that made an expired token
look identical to "not on the board"; a truncated projectItems page reported as
absence; duplicate cards silently resolved by `head -1`; author_association used
as a permission check (a private org membership reads as CONTRIBUTOR); no
detection of a Status-column rename; and a stale header describing the
pre-RFC-0008 two-gate flow.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

👋 Heads-up — Code review queue is at 36 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment thread.github/workflows/fr-pass-comment.yml
Bugbot, correctly: `permissions: contents: read` pins GITHUB_TOKEN to that one
scope and zeroes the rest, so the retry could never post a reaction or a comment.
It was unreachable code whose ::warning:: claimed a retry that cannot happen.
PROJECTS_KANBAN_TOKEN is the only token here that can write, so the writes are now
single-attempt with a loud ::error:: and a failed run — same guarantee, no
misleading path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 8514ed2. Configure here.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Implemented the release-train exemption (Lukas's call on the decision in the PR body) — 864a837.

Promotion PRs (release-train/* or hotfix-backmerge/* head, targeting staging/main/master) are now exempt from D30 automatically, regardless of allow-self-signoff. The reasoning: the FR gate on a promotion PR evaluates the contained items, so the promotion PR itself carries nothing for a second reviewer to assess — D30 there only blocked the train on its own author. Everywhere else the two-pairs-of-eyes rule stands, and allow-self-signoff keeps its false default.

The comment event carries no branch refs, so the head/base are looked up and the check fails closed: an unreadable PR is treated as normal work and keeps the rule. Both conditions are required, since a release-train/* head should only ever target an integration branch.

Verified against real PRs:

PRhead → baseexempt?
cli#428release-train/to-mainmain✅ yes
client#480release-train/to-stagingstaging✅ yes
cli#417chore/version-file-0.10.1develop❌ no (D30 holds)
design-system#80ci/staging-dist-tag-channeldevelop❌ no
data-ingestors#416ci/train-owns-release-tagdevelop❌ no
nonexistent PRlookup fails❌ no (fail-closed)

actionlint clean, YAML parses, both run: blocks pass bash -n.

One consequence worth stating plainly: this does not unblock the eight comments from 2026-07-29 that started this ticket. Those were all normal self-authored PRs (cli#415/#417/#422, py-package#346/#347, DS#74/#80/#81), so they now get a 👍-visible 👎 with an explanation rather than silence — which is the real fix — but they still need someone else's /fr-pass, or a manual card move. That is D30 working as written; the ticket's acceptance line ("10 consecutive /fr-pass comments all advance") is deliberately not met, and shouldn't be without a separate decision to drop the rule.

Sign up for freeto 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

@LukasWodka@claude