Repository navigation
docs(spec-loop): propose a per-item failure budget and harness-outage exclusion for batch skills - #1930
docs(spec-loop): propose a per-item failure budget and harness-outage exclusion for batch skills#1930justinmclean wants to merge 2 commits into
Conversation
… exclusion for batch skills Every batch (sweep) skill documents "stop and surface" for any per-item error, so one bad issue sinks a whole sweep and a forge outage reads as a failure of every item it touched. This is a proposal only; no skill changes here. - New `proposed` spec batch-failure-budget.md: step 1 wraps the read/classify phase so an item failure records `failed` and the sweep continues until a skill-declared budget is spent; the write phase keeps stop-and-surface because a retried post can duplicate a comment or label. Step 2 classifies failures as item or harness (429 / 5xx / reset / timeout / auth expiry), excludes harness failures from the budget, retries once after a back-off, and trips a separate consecutive-failure breaker that stops the sweep as an outage with no item marked failed. The exclusion ships only together with the budget. - Two plan items, `batch-failure-budget` then `harness-outage-exclusion` (blocked on the first); later items renumbered. - Spec indexed in specs/README.md and specs/overview.md. Generated-by: Claude Code (Fable 5.1)
90c77c4 to
71999df
Compare
potiuk
left a comment
There was a problem hiding this comment.
I rebased this branch onto main (90c77c4 → 71999df) to clear the conflict in IMPLEMENTATION_PLAN.md: main had replaced the plan's work-item list, so I kept main's list and appended your two items as 7 and 8 (batch-failure-budget, then harness-outage-exclusion, blocked on 7), dropping the renumbering of items that no longer exist. Please pull before pushing again. The proposal itself reads well; splitting the budget (read phase) from the never-retried write phase is the right line. Two points below, the first applying to your other open PRs too.
Agents are assistants, not authors (tools/spec-loop/specs/batch-failure-budget.md)
MUST NOT use
Co-Authored-By:with an AI agent as co-author. Agents are assistants, not authors — attributing them as authors misrepresents contribution and is contrary to ASF policy on AI-assisted contributions.
The commit on this branch is authored as Claude <noreply@anthropic.com>; the Generated-by: trailer is right, but the author field makes the agent the author outright. It is the same on #1928, #1929 and #1931. When a PR is squash-merged, GitHub adds a Co-authored-by: trailer for every commit author other than the PR author, so the squash commit on main would carry Co-authored-by: Claude <noreply@anthropic.com>, which is the attribution this rule forbids (and which the commit-trailer guard cannot see, because the trailer is written by GitHub, not by a local git commit). None of the last 300 commits on main is authored that way.
Suggested fix: Reset the author on each commit to yourself, keeping the Generated-by: trailer: git rebase --exec 'git commit --amend --no-edit --reset-author' origin/main, then force-push; and set user.name / user.email in the environment the agent commits from so new commits are yours.
Smaller observations
- See inline comments on
tools/spec-loop/specs/batch-failure-budget.md:126.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| not failures. | ||
| - **Harness failure.** The failure is about the connection, not the | ||
| item: HTTP 429 or a rate-limit body, HTTP 5xx, connection reset or | ||
| timeout, an auth error (401/403 on an endpoint that worked earlier in |
There was a problem hiding this comment.
Item failure
Item failure. The item's own data made the step impossible: body missing, timestamp unavailable, malformed metadata, a 404 or 410 for that item, a classifier that returned no verdict for that item twice.
A 403 is not always about the session. On GitHub it is also returned for one item the token cannot read (a hidden or restricted item, a repository the app is not installed on) on an endpoint that worked for every other item, and GitHub's secondary rate limit is a 403 as well. With 403 classed as harness, three such items in a row trip the breaker and report a "harness outage" that is really three inaccessible items, and none of them is ever recorded as failed.
Suggested fix: Class a 403 as harness only when the next call to a known-good endpoint (the pre-flight auth check) also fails, or the body says rate limit; otherwise record it as an item failure. Leave the per-forge mapping to the tracker contract, as the Known gaps already say.
b24ba44 to
4720add
Compare
|
Thanks Jarek. On the author point: ASF policy doesn't forbid this. The generative tooling page (https://www.apache.org/legal/generative-tooling.html) recommends either "Generated-by: " or "Co-authored-by: " in the commit message. The no-Co-Authored-By rule is this repo's own convention in AGENTS.md, not ASF policy. |
…s too GitHub returns 403 for one item the token cannot read and for its secondary rate limit, not only for a dead session. Classing every 403 as harness would let three unreadable items in a row trip the breaker as a false outage with none recorded as failed. A 403 is now harness only when its body says rate limit or the pre-flight auth check, re-run straight after it, also fails; otherwise it is an item failure. Generated-by: Claude Code (Opus 5.5)
Summary
proposedspec plus two plan items, a two-step fix: a per-item failure budget on the read/classify phase (the write phase keeps stop-and-surface so a retried post can never double-comment), and a harness-outage exclusion that classifies 429 / 5xx / reset / timeout / auth-expiry failures separately, retries once, and trips a breaker that reports an outage with no item marked failed.Type of change
docs/,README.md,CONTRIBUTING.md)Test plan
uv run --project tools/spec-validator spec-validate tools/spec-loop/specs/passes with the new specpython3 tools/dev/check-doc-sync.pypasses (spec indexed inspecs/README.mdandspecs/overview.md)prek run --files <changed files>passes (markdownlint, typos, lychee, spec-validate, check-doc-sync)RFC-AI-0004 compliance
Linked issues
None yet; the plan items
batch-failure-budgetandharness-outage-exclusion(blocked on the first) track the implementation.Notes for reviewers (optional)
Generated-by:trailer.