ci(nitpick): resync inlined workflow with nitpick-flow 2d8a694 - #41
Conversation
Nitpick has failed on every PR since 2026-08-29. The live error is
fatal: Model gateway returned HTTP 404: Model not found, inaccessible,
and/or not deployed
at the `coordinate` station: COORDINATOR_MODEL was still qwen3p7-plus,
which Fireworks sunset in 2026-09. Upstream retired it on 2026-09-05 by
changing a workflow_call input default, and this repo inlines the job
(public repo, private reusable workflow), so it never saw the change.
Both models go to glm-5p3-flash, matching upstream's defaults. The
reviewer swap matters as much as the coordinator one: gpt-oss-120b never
hard-failed, it just had ~zero recall above ~12k tokens, so the last
green runs on this repo were empty verdicts on ~12k-token prompts.
Also ports the rest of the drift, code byte-identical to upstream:
- diff fetch: fall back to a paginated per-file rebuild when the one-shot
diff media type hard-fails ("diff too_large" past 20k lines), plus a
60k-line safety valve that keeps source files over tests and emits an
omission manifest.
- spider step: coupling graph (graph.json) for shard planning and the
reviewers' RELATED CODE pack (related.txt) from unchanged files the
diff reaches. Smoke-tested against this repo: 46 files, 99 import
edges. continue-on-error, so a failure degrades rather than blocks.
- whole-run retry (3 attempts, fresh state volume each) for gateway
stalls, which are fatal to a conduit run today, gated so a
deterministic token-budget andon is not retried three times.
- related.txt existence guard, since the review stations declare it as an
input and the spider step may skip.
Kept local and must survive the next sync: the fork guard, secret names
FIREWORKS_AI_API_KEY / GHCR_READ_PAT, job id `nitpick` (the check name
branch protection matches), and the env block itself. The header now says
to diff that env block against upstream's input defaults first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nitpick review — comment
This rewrite substantially expands the PR-review workflow: a new spider step, a diff-truncation safety valve, and a retry loop with a token-budget short-circuit, all executing a cross-org container image on every pull request. The main risk is that the flow image is pinned to the mutable :main tag while the diff deepens reliance on it, so an upstream push silently changes what runs with repo credentials; the retry loop also re-runs the full flow (roughly 3x token spend) on deterministic failures that can never succeed on retry. Human review is recommended before merge given the CI and supply-chain surface; verdict is comment, not block.
5 inline comment(s).
| @@ -20,12 +27,29 @@ on: | |||
|
|
|||
| env: | |||
| FLOW_IMAGE: ghcr.io/queso/nitpick-flow:main | |||
There was a problem hiding this comment.
warning · Flow image pinned to the mutable main tag while the diff deepens reliance on it
FLOW_IMAGE is ghcr.io/queso/nitpick-flow:main — a floating tag the publisher can move at any time, and a cross-org image (the GHCR login comment confirms GITHUB_TOKEN cannot see queso's package). This diff materially increases what that image does in the pipeline: it now also executes the spider step (graph.json / related.txt generation, run as --entrypoint bun), donates the baked flow.yaml that gets rendered and re-injected, and its report-failure.mjs runs in the post-mortem step. Whatever is at :main when a PR opens is what executes, and that code receives CONDUIT_API_KEY, GITHUB_TOKEN-scoped permissions, and a read-only mount of the full checkout. The diff documents careful pinning discipline elsewhere (sync commit hashes, model versions) and even records being burned by upstream drift (the qwen3p7-plus default change); the image is the one unpinned input.
| FLOW_IMAGE: ghcr.io/queso/nitpick-flow:main | |
| Resolve the image to a content digest (`ghcr.io/queso/nitpick-flow@sha256:<digest>`) and update it via a deliberate bump alongside the existing 'last synced' comment, or at minimum pin an immutable version tag instead of `main`. |
| "split it, or raise budgets.run.max_tokens with the shard/fan-out sizing it implies." | ||
| break | ||
| fi | ||
| echo "conduit run attempt $attempt failed (exit $rc); retrying with a fresh state volume" |
There was a problem hiding this comment.
warning · Retry loop re-runs the whole flow (3x spend) on deterministic non-token failures too
The retry loop correctly treats the token-budget andon as terminal, but every other non-zero exit is retried up to 3 times with a fresh volume — including deterministic failures the diff itself calls out as 'real failures': scrap/hold lanes and doctor preflight rejections. A flow that is 'a pure function of the diff' (the comment's own words) will produce the same scrap/hold/preflight result on attempts 2 and 3, costing the measured ~1.7M input tokens per attempt (~5M total) plus ~3x wall-clock latency for zero additional review — exactly the waste the token-andon guard was written to prevent, via a different terminal lane. The log-matching here only distinguishes the token case.
| echo "conduit run attempt $attempt failed (exit $rc); retrying with a fresh state volume" | |
| Extend terminal-lane detection beyond the token andon: match the engine's other deterministic halt markers (scrap/hold verdicts, doctor preflight failure lines) and break on those too, reserving retries for transient stalls. Alternatively, gate retries on the absence of any terminal-lane marker rather than the presence of one specific string. |
| run: | | ||
| # World-writable scratch dir: the image runs as its non-root `conduit` user, whose uid | ||
| # does not match the runner's, so a plain bind-mounted dir is EACCES on first write. | ||
| mkdir -p .spider-out && chmod 777 .spider-out |
There was a problem hiding this comment.
suggestion · World-writable scratch directory (chmod 777) shared with the container
mkdir -p .spider-out && chmod 777 .spider-out makes the output directory writable by every user and process on the runner, and it is then bind-mounted into the flow container. On an ephemeral GitHub-hosted runner the practical risk is low, but a 777 directory feeding graph.json/related.txt into the review pipeline is an unnecessary loosening; a group-writable mode matching the container uid, or a named volume, achieves the same EACCES fix more tightly.
| mkdir -p .spider-out && chmod 777 .spider-out | |
| Prefer `chmod 1777` (sticky) or create the dir with a uid/gid matching the image's `conduit` user instead of 777. |
| # | ||
| # Fork guard: fork PRs don't receive secrets, so the job only runs for same-repo PRs — | ||
| # external contributions skip review instead of failing on a missing key. | ||
| # Last synced against nitpick-flow 2d8a694 (2026-09-18). What is DELIBERATELY local, and so |
There was a problem hiding this comment.
suggestion · Deliberately-local divergence list is incomplete relative to the new local-only additions
The rewritten header establishes an explicit sync contract with nitpick-flow's nitpick.yml, enumerating what is "DELIBERATELY local ... must survive the next sync" (fork guard, secret names, job id, env block). This diff adds three substantial local-only behaviors not in that list: the per-file diff-rebuild fallback for >20k-line diffs, the Node safety-valve truncation script, and the whole-run retry loop with the andon short-circuit (the retry-loop:begin/end markers note upstream slices that region into retry-gate.test.mjs, but the header's must-survive-sync list doesn't mention it). Since the file still instructs keeping steps in sync with upstream, a future sync trusting the header's list as complete could silently drop the fallback, the valve, or the retry loop.
| # Last synced against nitpick-flow 2d8a694 (2026-09-18). What is DELIBERATELY local, and so | |
| Extend the "DELIBERATELY local" header list to cover the diff-rebuild fallback, the 60k-line safety valve, and the retry loop / andon gate (noting the retry-loop:begin/end lockstep markers). |
| # keep whole files up to the budget, SOURCE files before tests, and close with an | ||
| # explicit manifest of what was left out — a partial review with honest coverage | ||
| # beats a dead job. | ||
| node -e ' |
There was a problem hiding this comment.
suggestion · Inline diff-truncation script ships with no test, unlike its sibling retry-loop logic
The node -e block contains real, failure-prone logic: the isTest classifier (which decides which files get dropped first), the greedy budget packing, and the omitted-files manifest. A bug here silently removes source files from review. The retry loop below it is explicitly carved out for upstream testing (retry-gate.test.mjs slices the retry-loop:begin/end region verbatim), demonstrating the project's established pattern for testing inline workflow script; the truncation valve has no equivalent marker or test. The isTest regex also only recognizes __tests__/, tests?/ dirs, and .test.[jt]sx? — .spec. files and pytest-style test_*.py would be classified as source and prioritized, which may be intended but is untested either way.
| node -e ' | |
| Add a retry-gate.test.mjs-style test that feeds the valve a synthetic multi-file diff (mixed test/source, over-budget, exactly-at-budget) and asserts which files survive, or mark the script region with a slice comment like the retry loop's so it can be tested where it lands. |
1 participant
What broke
Nitpick has failed on every PR since 2026-08-29. Two different causes, in sequence:
Model not found, inaccessible, and/or not deployedThe 404 hits at the
coordinatestation. The post-mortem localizes it: 7 model calls succeeded, all onREVIEWER_MODEL, and the entry card's lane journey ends atgather_evidence → coordinate (forward)with zero calls recorded for anycoordinatestation.COORDINATOR_MODELwas stillaccounts/fireworks/models/qwen3p7-plus, which Fireworks sunset in 2026-09. nitpick-flow retired it on 2026-09-05 by changing aworkflow_callinput default — and this repo inlines the job (public repo cannot call a private reusable workflow), so it never saw the change.flow.yamland the scripts come from the floating:mainimage and do update themselves; only theenv:block is frozen. That asymmetry is the whole bug.nitpick-flow's own comment predicted it:
The reviewer model was also stale, and that was worse
REVIEWER_MODELwasgpt-oss-120b, replaced upstream the same day. It never hard-failed; it had ~zero recall above ~12k tokens (empty findings 48/50 runs on the DevTrack #23 fixtures, 0/11 human ground-truth findings vs glm's 4/11). This repo's six review stations ran on ~12k-token prompts and returned 48–270 output tokens. The green checks through 2026-08-24 were passing because they found nothing.Both models now go to
glm-5p3-flash, matching upstream's defaults exactly.Other drift ported
Executable code is byte-identical to upstream; only comments are localized.
diff too_largepast 20k lines), plus a 60k-line safety valve that prioritizes source over tests and emits an omission manifest.graph.json(coupling graph for shard planning) andrelated.txt(RELATED CODE excerpts from unchanged files the diff reaches, where contract-class bugs live).continue-on-error, so a failure degrades the review instead of blocking it.related.txtguard — the review stations declare it as an input and the spider step can skip.Verification
yqparses the workflow;bash -nclean on all 6run:blocks.run:body diffed against upstream with comments stripped: identical.files=46 routes=0 edges=99 (import=99), 9.5 KBgraph.json. The related-pack comes back empty on a docs-only diff, which is correct, and spider.mjs emits the same(no related code identified)string as the fallback guard./flow/src/spider.mjsexists in the image (Dockerfilecopiessrc/to/flow/src/).Not verified locally: no Fireworks key in the environment, so I could not confirm
glm-5p3-flashagainst the live catalog. It is upstream's current default and upstream is presumably green on it. This PR's own Nitpick run is the real test.Kept local
Must survive the next sync, and the header now says so: the fork guard, secret names
FIREWORKS_AI_API_KEY/GHCR_READ_PAT, job idnitpick(the check name branch protection matches), and theenv:block. The header tells the next syncer to diff that env block against upstream's input defaults first.Note on #40
PR #40 will keep showing the old failure until it picks this up —
pull_requestruns the workflow from the PR's own merge commit. Merge this, then rebase #40.🤖 Generated with Claude Code