Uh oh!
There was an error while loading. Please reload this page.
promote: FR gate fail-closed (#71) to main - #74
Merged
Merged
Conversation
* fix(fr-gate): fail closed on an unreadable or absent Status The gate treated "could not evaluate this item" as "this item is fine". An unreadable or missing Status hit `continue`, and if every item was unreadable the gate printed "PASSED". A missing or expired PROJECTS_KANBAN_TOKEN, a repo-level secret shadowing the org one, or a transient GraphQL 5xx all collapsed into a silent pass — the only promotion gate disarming itself, with failure indistinguishable from success. Two changes: - resolve_status() retries up to 5 times and distinguishes an API failure (rc=1) from a PR genuinely not on the board (rc=2). The empty-result retry is the important half: the empty case is usually a RACE, not an absence. fr-gate-caller.yml and add-to-kanban.yml both fire on `pull_request: opened` and run concurrently, so the gate routinely queries the board before the card exists. Observed on #61 — gate finished at 16:48:09, card created at 16:48:14, and it passed on "not on kanban". Measured 4/32, 3/20, 3/17 items skipped this way on real promotion runs. - The verdict blocks on BLOCKED, MISSING, or UNREADABLE, each with its own actionable message, instead of only on BLOCKED. This also closes the delete-card-then-add-a-label bypass: the label trigger re-runs the gate, add-to-kanban does not recreate the card, and the retry now gives the card time to reappear rather than passing on its absence. Verified: the return-code contract holds under a stubbed gh (card -> 0, no-card -> 2, API-failure -> 1), and actionlint passes. Refs RFC-BACKEND-0008 D27-L2/L6 (tracebloc/backend#1266) * fix(fr-gate): make the Status parse non-fatal and stop corrupting JSON Bugbot on #71 caught two robustness bugs I introduced: - The gh success capture used 2>&1, so any stderr warning was merged into the JSON body and fed to jq — a spurious parse failure on an otherwise good call. Capture stdout only (2>/dev/null). - The jq|head pipeline was unguarded under set -euo pipefail. A jq error, or a SIGPIPE from head closing the pipe early, would abort resolve_status mid-loop, skip the remaining retries, and return an ambiguous code that the caller then mislabelled as MISSING with a '5 attempts' message that never ran. Parse is now non-fatal (2>/dev/null on jq, || true on the pipeline) so a failed parse falls through to the next retry. Also made the end-of-loop sleep an explicit if, not a '[ ] && sleep' list, which is a set -e footgun as a bare statement. Return-code contract re-verified under a stubbed gh, including a case where gh writes to stderr but returns 0 — the JSON now parses cleanly where 2>&1 would have broken it.
LukasWodka
commented
Jul 26, 2026
ContributorAuthor
👋 Heads-up — Code review queue is at 42 / 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
commented
Jul 26, 2026
ContributorAuthor
Applying |
* fix(fr-gate): gate the diff, not just commit subjects (re-land #72 onto develop) #72 was reviewed and merged, but into its stacked base (fix/1266) rather than develop — #71 merged to develop first and the child was never retargeted, so the attribution code never reached develop (stacked-PR merge-order hazard). This ports exactly that delta onto develop: the only change vs develop's current fr-gate.yml is the additive attribution block + its wiring (verified by diff — all additions, nothing removed). develop already carries #71's fail-closed + robustness fixes via the earlier squash. Attribution: every non-merge commit in BASE..HEAD must carry a (#N) or be introduced by a PR merge (in M^1..M^2), else the gate fails closed. Fixture- tested incl. the Bugbot #72 false-negative (direct push absorbed by a feature branch). actionlint clean. Refs RFC-BACKEND-0008 D27-L1 (tracebloc/backend#1265), re-lands #72 * fix(fr-gate): only PR-merges may vouch — close the sync-merge laundering hole Bugbot #73: the attribution loop vouched a commit via ANY merge in range whose M^1..M^2 contained it. A routine 'Merge branch develop into feature' sync merge is itself in range, and a direct push on develop sits in that sync merge's S^1..S^2 — so the push was laundered into attribution, reopening the exact fail-open the #72 ^1 check closed, on merge-commit workflows. Fix: a merge may vouch only if its own subject carries a (#N) or 'Merge pull request #N'. Sync merges (no PR ref) no longer vouch. Reproduced with a fixture where the feature branches before the direct push and syncs develop in: the pre-fix logic MISSES the push (the hole), the fix FLAGS it. Earlier fixtures (basic direct push, feature-absorbs-base) still flag correctly. actionlint clean. Refs RFC-BACKEND-0008 D27-L1 (tracebloc/backend#1265)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 08beba8. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
…ecker (#65) * feat(quality): shared code-quality reusable workflow + house-rules checker The org has shipped the reusable-workflow + thin-caller pattern 16 times, every one of them for process automation (kanban, FR gate, WIP, PII). This is the first one that looks at the code. Four independent jobs, so one failure never hides another's output: ruff Python lint. Input-gated. Falls back to a deliberately small default selection (E4,E7,E9,F) when the repo has no ruff config, and defers entirely to the repo's own config when it has one. No formatting-opinion families — those would report thousands of findings on legacy code and get the job removed. shellcheck Preinstalled on the runner, so no download and no third-party action. Severity configurable, default `error`. gitleaks Credential scanning, where there was none. Installed from the release tarball pinned by version AND verified against a pinned SHA-256: gitleaks-action needs a licence for organization-owned repos, and pinning the artefact we execute beats pinning a wrapper that fetches it. Scans exactly the commit range the PR adds, so a value added and then removed again is still found. house-rules scripts/house-rules.sh — POSIX sh + awk, no jq/yq/python. The grep-level subset of the rules the automated reviewer keeps re-teaching us: --tlsv1.2 on curl, timeouts on curl/kubectl/helm, and pipelines in scripts with no `set -o pipefail` (a `curl … | bash` of a 404 exits 0). Shell cannot be linted with grep, so the checker carries a small lexer: quote-aware comment stripping, heredoc bodies skipped, `\`-continuations joined while reporting the original line, descent into `$( … )` inside double quotes, tool names matched only in command position, and resolution of flag-holding variables so `curl $CURL_SECURE` satisfies the TLS rule. Verified against the real installer scripts in client and cli: 20 and 9 true findings, zero false positives, 0.5s for a whole repo. Every rule and FP guard is covered by a fixture pair, and results are identical under dash and under BSD awk. `soft-fail` defaults to true: findings are annotated on the diff and written to the job summary, and the job exits 0. Landing non-required first is the settled rollout decision — a linter switched on against an unlinted backlog gets the check removed, not the backlog fixed. The header documents the flip to `soft-fail: false` plus branch protection as the intended destination. Repos extend it without touching this workflow via `.house-rules.conf` (exclude / wrapper / timeout-wrapper / risky / disable / custom `rule:` lines that work on any language), and silence one line with a `# house-rules: ignore=<rule>` pragma. No callers are added here: `@main` callers cannot resolve until this merges to main. Refs tracebloc/backend#930 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(quality): NUL-delimit the file lists fed to ruff and shellcheck `xargs -a file` splits on whitespace, so a path containing a space (they exist — client/scripts/lib has one) would be passed as two broken paths. Convert to NUL delimiters and read with `xargs -0`, which also avoids the `-d '\n'` trailing-empty-argument ambiguity between xargs implementations. `-r` stays: with an empty list, ruff would otherwise fall back to scanning the entire tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(house-rules): three parsing bugs found by Bugbot on #65 1. Timeout hid the tool from every rule. `findtok` recognised command position after separators, shell keywords and repo-declared wrappers, but not after a time bound. So in `timeout 30 curl ...` the `curl` was invisible and `curl-tls` silently stopped applying to the exact hardening pattern the config documents as supported. Added TIMEOUT_PREFIX_RE to the command- position checks. 2. The timeout wrapper demanded a bare digit. `TIMEOUT_WRAPPER_RE` required a numeric token immediately after the wrapper name, so `timeout -k 5 30 ...`, `timeout --foreground 30 ...`, `timeout 30s ...` and the documented `timeout-wrapper: guard` form (`guard curl ...`, no duration) all failed the bounded check and still raised *-timeout findings on correctly bounded calls. Replaced with TW_ARG, which accepts flags, flag arguments, unit suffixes, variables, and the empty case. 3. The wrapper match was not end-anchored. `WRAPPER_RE` was applied unanchored to the text before a tool, unlike the shell-keyword check beside it, so any earlier wrapper name in the segment marked later tokens as command position. Anchored it to match the keyword check's shape. Verified against a fixture: before, only the bare unbounded call was detected and all five timeout-wrapped variants were invisible. After, curl-tls fires on every call missing --tlsv1.2 and curl-timeout fires only on the genuinely unbounded one. Refs #65 * fix(house-rules): correct pipefail detection (2 more Bugbot findings) - False positive: pf_has only matched the glued form (set -euo pipefail), so the split forms 'set -eu -o pipefail' and 'set -e -o pipefail' failed to register and safe scripts still got pipefail findings. Regex now allows earlier flag clusters before '-o pipefail'. - False negative: pf_has was tested on lraw, which still contains quoted string contents, so 'echo "... set -o pipefail ..."' marked the whole file safe and suppressed every real pipeline finding. Now tested on lmask (strings blanked). Verified against fixtures with a risky producer (curl | grep): glued and split safe forms report 0; a genuinely-missing pipefail and a string-decoy both report 1. Against origin, the split form false-positived and the string decoy false-negatived — both now correct. * fix(house-rules): 3 more Bugbot findings (path-less source, ERE alternation, quoted heredoc) - Path-less source: the extractor ran s|.*/|| before stripping the source/. keyword, so 'source helpers.sh' collapsed to 'source' and never entered $SOURCED — sourced libs still got pipefail findings. Strip the keyword first. - Custom rule alternation: rule: lines split on every | took field 3 as the ERE, truncating any pattern with alternation. Treat the LAST field as the message and rejoin 3..n-1 as the pattern; a rule message may no longer contain | (ids/globs never do). - Quoted heredoc: heredoc detection ran on lraw, so a <<TAG inside a string opened in_hd and swallowed real following lines. Match on lraw (to keep <<'EOF' tags working) but require the << to survive masking, i.e. not be inside a quoted string. Each proven against a fixture: before/after diverge in the expected direction, no regressions. shellcheck clean. * fix(code-quality): round-4 Bugbot findings — 3 parser edges + 1 CI-integrity house-rules.sh: - Quoted path-less source: quote stripping deleted from the first quote to EOL, so 'source "helpers.sh"' collapsed to empty and the lib never entered $SOURCED. Delete quote characters instead. - Env-assignment prefixes: 'HTTPS_PROXY=... curl' / 'KUBECONFIG=... kubectl' left the tool invisible to command-position detection, so tls/timeout/pipefail never ran on it. findtok now treats one-or-more VAR=val prefixes as command position. - Glued -m: 'curl -m30' / 'curl -fsm30' still raised curl-timeout despite a max-time being present; the short-option check now accepts a glued digit. code-quality.yml: - Ruff silently no-op'd when 'git diff' against BASE_SHA failed (empty file list -> skipped with success). Now falls back to scanning all files, matching shellcheck/house-rules — a gate must not skip itself on a bad diff. Each parser fix proven against before/after fixtures incl. a regression check that a genuinely unbounded curl still flags. shellcheck + actionlint clean. * fix(house-rules): round-5 Bugbot findings — fix 4 classes at the root Rather than patch each reported instance, fixed the underlying class so adjacent variants are covered too: - Masked-arg (cmd position): findtok runs on the quote-masked line, where a quoted duration "$DUR" is blanked to '_'. TW_ARG now accepts a '_+' run, so 'timeout "$DUR" curl' keeps curl in command position. Covers the reported case plus any quoted arg between a wrapper and its tool. - externally_bounded: was a substring match, so 'myprog timeout 30' (timeout as an ARGUMENT) suppressed real curl/kubectl/helm-timeout findings. Now requires the wrapper to be in COMMAND position via findtok. - kubectl subcommand: 'wait'/'delete' were matched as any word, so a resource or file named 'wait' triggered kubectl-timeout. Now anchored to follow 'kubectl' (global flags allowed between). - Source scan: extensionless shebang libs (bin/deploy) never entered $SOURCED and still got pipefail findings. Filter now accepts extensionless names. 13-case fixture matrix covering the reported cases AND adjacent variants (timeout "$DUR", -fsm30, timeout-as-arg, kubectl get pod named 'wait') all pass; a genuinely unbounded curl and real kubectl wait/delete still flag. shellcheck + actionlint clean; dogfooded over the repo with no self-findings. * fix(house-rules): kubectl matcher handles value-taking global flags Round-6 Bugbot: separate flag arguments ('-n ns', '--context ctx') between kubectl and the subcommand broke the match, so 'kubectl -n ns wait' — a real unbounded blocking call — was missed. The matcher now models value-taking global flags (a bare word after -n/--namespace/--context/... is that flag's value, not the subcommand). Fixture matrix: all real blocking calls flag (kubectl wait, -n ns wait, --namespace ns wait, --context c -n n wait, delete, -n ns rollout status); resource/file names (get pod wait, apply -f delete.yaml) and bounded calls (--timeout, timeout 30 kubectl wait) do not. Known residual, documented rather than hidden: 'kubectl -n wait get pods' where a namespace is literally named 'wait' still false-positives, because '-n' is regex-ambiguous between a boolean flag and a value flag — no ERE can resolve it without kubectl's own flag semantics. It is a rare, safe-direction FP (a dismissible spurious finding, not a missed hang). See PR discussion re: whether the kubectl-timeout rule is worth this irreducible ambiguity. * feat(house-rules): drop the kubectl-timeout rule (regex-irreducible) Decision (Lukas): remove kubectl-timeout rather than keep chasing it. It was the single biggest source of review findings — 'kubectl -n wait get' (namespace named 'wait') vs 'kubectl -n ns wait' are identical in token shape and only kubectl's own flag semantics disambiguate them, which an ERE cannot do. Every round re-surfaced a variant of that ambiguity. Removes the kubectl subcommand matcher and its header docs. Keeps the rules that detect robustly on flag presence, not CLI-grammar parsing: curl-tls, curl-timeout, helm-timeout (flag-based: --wait/--atomic without --timeout), and pipefail. kubectl stays in the pipefail producer list, so 'kubectl … | x' without pipefail is still caught. Verified: the four remaining rules fire correctly, kubectl-timeout is never emitted, the kubectl pipefail case still works; shellcheck + actionlint clean; dogfooded with no self-findings. * fix(house-rules): apply exclude globs to the custom-rule scan too Round-7 Bugbot: the custom-rule file gathering appended candidates from $CAND directly, without the exclude filter the shell-file scan applies. So documented 'exclude:' paths and the workflow's '--exclude .quality-tools/*' guard were ignored for custom rules — an excluded/vendored tree could still produce findings. Factored the exclude check into a shared is_excluded() helper used by both the shell-file scan and the custom-rule scan, so the two can't drift again. Fixtures: a custom rule now skips an excluded vendor/ file while still firing on a non-excluded one, and --exclude applies to custom-rule scanning. Rule matrix still green; shellcheck + actionlint clean; dogfooded. * fix(house-rules): per-occurrence timeout bounding (round-8 Bugbot) externally_bounded() asked 'is a timeout wrapper anywhere in this segment' — segment-global. So a curl nested in $() was treated as bounded whenever the OUTER command was timeout-wrapped ('timeout 30 foo $(curl x)'), suppressing a real curl-timeout on the unbounded subshell call. Replaced with tool_bounded(seg, tool): a tool is bounded only when a timeout prefix sits IMMEDIATELY before THAT occurrence (its 'pre' text ends with TIMEOUT_PREFIX_RE). This is strictly more correct and also subsumes the round-5 'timeout as an argument' case — timeout-as-arg simply isn't a prefix of any tool, so no special-casing is needed. Removed the now-dead externally_bounded() and TW_NAMES. Fixtures: nested $(curl) now flags; 'timeout 30 curl', 'timeout "$DUR" curl', 'timeout -k5 30 curl' stay bounded; bare curl and timeout-as-arg still flag; helm --wait still flags. shellcheck + actionlint clean; dogfooded. Note (out of scope): helm inside a captured $() subshell isn't detected at all, timeout or not — a pre-existing helm-in-subshell quirk, not a bounding issue. * fix(house-rules): don't treat redirection '&' as a command separator Round-9 Bugbot: check_line split segments on every bare '&', so a redirection like '2>&1' cut the producer segment before the '|' was seen. 'curl … 2>&1 | grep' never reached note_pipeline with the risky tool, so the pipefail rule missed a very common form of the defect it exists to catch. A bare '&' now splits only when it backgrounds a command — not when it is part of a redirection: '2>&1'/'>&2' (previous char '>' or '<') or '&>file' (next char '>'). '&&' was already handled separately. Fixtures: 'curl 2>&1|grep', 'curl >&2|grep', 'curl &>/tmp/f|grep' all flag pipefail; real backgrounding 'curl & echo' still splits (no pipeline); plain 'curl|grep' still flags; pipefail-set stays silent. All prior matrices green; shellcheck + actionlint clean; dogfooded. * fix(code-quality): gitleaks operational error must not look clean Round-10 Bugbot: when the gitleaks commit-range scan errored (missing base.sha, bad log-opts), COUNT stayed 0 -> summary said 'Nothing detected', and with the default soft-fail:true the job exited 0. A broken credential scan looked green for the whole migration window — unlike ruff/shellcheck/house-rules, which fall back to a full scan. gitleaks uses exit 0 = clean, 1 = leaks; any other code is operational. On such a code, fall back to a full-history scan instead of reporting clean. Verified the RC branch: 0/1 -> normal, 2/126 -> fallback. actionlint clean. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Bugbot on #74: GitHub rebase-and-merge REPLAYS a PR's commits unchanged — no (#N) subject, no merge commit — so neither the subject check nor the merge-vouch loop attributes them. They landed in UNATTRIB and would HARD-FAIL the gate for any repo using rebase-and-merge, once fr-gate is armed from @main across all 15 callers. A blocker for #74. Add an authoritative fallback: for a commit the local git heuristics can't place, ask GitHub which PR introduced it (repos/{repo}/commits/{sha}/pulls). A MERGED PR attributes it (covers rebase-merge); a direct push has none and stays flagged. Retries 3x and fails CLOSED on a persistent API error — an unverifiable commit never passes. The local checks stay as fast-paths, so the API is hit only for the residual. Needs a token: added GH_TOKEN (PROJECTS_KANBAN_TOKEN) + REPO to the items step. Decision logic verified with a stubbed gh: merged-PR->attribute, direct-push->flag, API-error->flag(fail-closed). actionlint clean.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes the FR-gate fail-closed fix (#71) from
developtomain.This is the step that arms it. All caller workflows read
tracebloc/.github/.github/workflows/fr-gate.yml@main, so until this lands every promotion gate inthe org is still running the OLD fail-OPEN version — the one that passes on an unreadable Status.
Contains:
resolve_status()retries the board read 5×, distinguishes API-failure fromnot-on-board, and the verdict fails closed on BLOCKED / MISSING / UNREADABLE. Closes the
add-to-kanban race and the delete-card-add-label bypass.
Not included yet: #72's gate-the-diff attribution — that's re-landing via #73 (it was stranded
on a stacked branch). It'll ride the next promotion once #73 merges to develop.
developis ahead 1 / behind 1 (the behind-1 is #70's merge commit, self-correcting).Merge with a merge commit, not squash — keeps develop and main in sync for the next promotion.
Note
High Risk
Changes the org’s promotion gate on
main(fail-closed kanban reads plus new commit-attribution blocking) and introduces shared CI enforcement; misconfiguration or API/git edge cases could block legitimate promotions until retried or overridden.Overview
Promotion to
mainarms org-wide reusable workflows that callers pin at@main— especiallyfr-gate.yml, which stops treating “couldn’t verify” as a pass.FR gate (
fr-gate.yml) now fails closed: kanban Status is read viaresolve_status()with up to five retries (API failure vs missing card), and the step fails on BLOCKED, MISSING, UNREADABLE, or unattributed commits instead of skipping or printing PASSED when nothing was readable. A new commit attribution pass flags range commits without(#N)/ PR-merge provenance (with merge-parent checks and agh api…/commits/…/pulls fallback for rebase merges); unattributed SHAs block like other failures.New reusable
code-quality.ymladds separate optional jobs for ruff, shellcheck, gitleaks (version + SHA-256 pinned tarball), and house-rules, with PR-scoped diffs, diff-failure fallbacks, annotations/summaries, andsoft-fail: trueby default for adoption.New
scripts/house-rules.shis a POSIX sh/awk shell linter (curl TLS/timeouts, helm--timeout, pipefail, custom rules via.house-rules.conf) invoked from the house-rules job.Reviewed by Cursor Bugbot for commit 72cd3f4. Bugbot is set up for automated code reviews on this repo. Configure here.