Skip to content

#202: Grade to 100% — rate-limit throttle + degraded-pair sweep + require-complete - #203

Merged
wjduenow merged 32 commits into
devfrom
feature/202-grade-to-100
Jun 9, 2026
Merged

#202: Grade to 100% — rate-limit throttle + degraded-pair sweep + require-complete#203
wjduenow merged 32 commits into
devfrom
feature/202-grade-to-100

Conversation

@wjduenow

@wjduenow wjduenow commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #202. Drives the grade stage to 100% scored — or fails loud with the exact ungraded pairs named — for any run that didn't opt into an explicit operator ceiling.

Base: dev · Plan: plans/super/202-grade-to-100.md (DEC-203…210) · 11 stories, all merged & validated.

Root cause (from the #179 retest)

The 10-way concurrent grade fan-out shared an asyncio.Semaphore but each coroutine retried 429s with blind backoff and ignored the rate-limit headers — a thundering herd that burst past the per-minute cap, exhausted retries, and degraded: 70/408 pairs (40-col) and 34/220 (16-col) as GradeLLMError.

Changes

  • Stage 1 — rate-limit-aware throttle. A shared, header-honoring limiter (retry-after / anthropic-ratelimit-* via a neutral RateLimitBudget provider seam, SDK-confined per DEC-012) plus an adaptive concurrency gate (AsyncConcurrencyGate: an asyncio.Condition + in-flight counter whose cap tracks the limiter's effective_concurrency — ÷2 on a 429, +1 on a clean completion, bounded [1, max_concurrent_calls], shared across the grade TaskGroup via a ContextVar). Wired into both call_llm and call_llm_async.
  • Stage 2 — bounded sweep. Always-on re-grade of transient score=None pairs (sequential, cool-down, sweep_max_rounds), wrapped in its own asyncio.timeout(sweep_budget_seconds). Reuses the cli: --no-grade flag + persistent grade cache for fast iteration #189 cache so only failures are re-touched.
  • Stage 3 — require_complete (default true). New tier-2 GradeIncompleteError names the ungraded pairs (exit 2) when a non-exempt pair survives recovery. Only intentional ceilings (max_grade_*, an explicit total_budget_seconds) are exempt; a default-scaled-budget trip fails loud. --require-complete / --no-require-complete CLI flag with a no-clobber default=None sentinel; 6-surface parity.
  • Stage 4 — defaults + posture. max_retries_429 default 3→6; a structured degrade_reason_type discriminator (transient/budget/ceiling, audit schema v3) replacing fragile reason-string matching; the bias-to-completion posture (DEC-210) documented in grade-ops.md + rules: incompleteness is only ever a deliberate operator choice, never a passive default.

Testing

  • Full canonical gate green: ruff check · ruff format --check · pyright (0 errors) · pytest 3709 passed, 8 skipped, coverage 97%.
  • 4 code-review passes + an adversarial concurrency review of the new gate (no deadlock / lost-wakeup / bounds bug).
  • Live metered retest (2026-06-05, adaptive gate), PASS on both arms — recorded in docs/research/179-runtime-benchmark.md:
Prod 0.5.0 Dev #198 #202
40-col scored 83/408 338/408 (70 GradeLLMError) 412/412, 0 degraded
16-col scored 186/220 (34 score=None) 220/220, 0 degraded
aggregate_complete False False True
40-col grade wall-clock 302.8s (capped) 447.7s 561.0s (0.73/s)

The adaptive gate reached 100% completeness and was faster than the non-adaptive pass (561.0s vs 622.0s) — pacing the fan-out at the provider's rate cuts 429-retry churn (a throughput win, not just a completeness win).

Compounding Update

  • .claude/rules/llm-drafter.md — rate-limit pacing pattern (pace the fan-out, not just per-call backoff) + the validated throughput lesson.
  • .claude/rules/grade-layer.md — recovery taxonomy (transient-recoverable / operator-ceiling / unrecoverable) + the structured-discriminator-over-string-matching lesson + bias-to-completion.
  • docs/grade-ops.md, docs/cli-ops.md, docs/research/179-runtime-benchmark.md updated.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added --require-complete / --no-require-complete CLI flags to enforce or relax grade completeness; incomplete runs now surface a GradeIncompleteError mapped to exit code 2.
    • Adaptive, shared rate-limiting and an always-on bounded transient-recovery sweep to reduce transient failures and improve completion.
  • Configuration

    • Increased default 429 retry budget (3 → 6) and added knobs to bound sweep behavior (rounds, cooldown, wall-clock budget).
  • Documentation

    • Audit schema bumped to v3 and documents structured degrade-reason classification (transient/budget/ceiling).

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e57d5cfc-9d15-48d5-a034-5389ce96aa36

📥 Commits

Reviewing files that changed from the base of the PR and between 22c84af and 7170bee.

📒 Files selected for processing (6)
  • src/signalforge/grade/audit.py
  • src/signalforge/llm/_rate_limiter.py
  • tests/grade/test_audit.py
  • tests/grade/test_config.py
  • tests/llm/test_rate_limiter.py
  • tests/research/179-runtime-benchmark/benchmark_runtime.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/signalforge/grade/audit.py
  • tests/llm/test_rate_limiter.py
  • tests/grade/test_config.py
  • src/signalforge/llm/_rate_limiter.py
  • tests/research/179-runtime-benchmark/benchmark_runtime.py

📝 Walkthrough

Walkthrough

Adds adaptive, header-honouring LLM rate-limiter, bounded transient re-grade sweep, require-complete enforcement (GradeIncompleteError), audit/schema v3 fields, CLI flag overrides, and comprehensive tests and benchmark updates to realize #202.

Changes

Grade to 100% Multi-Stage Plan

Layer / File(s) Summary
Plans and operational docs
plans/super/202-grade-to-100.md, .claude/rules/grade-layer.md, .claude/rules/llm-drafter.md, docs/grade-ops.md, docs/cli-ops.md, docs/research/179-runtime-benchmark.md
Super plan, bias-to-completion rules, CLI/ops docs, and benchmark retest writeup describing stages, taxonomy, and operator-facing controls.
LLM rate-limit primitives & provider seam
src/signalforge/llm/_rate_limiter.py, src/signalforge/llm/providers.py, src/signalforge/llm/__init__.py
Adds RateLimitBudget, AIMD-based Sync/Async rate limiters, AsyncConcurrencyGate, provider extract_rate_limit_info, and public exports.
LLM client retry wiring
src/signalforge/llm/client.py
Integrates optional ContextVar-scoped rate limiters into sync/async retry paths, computes header-honouring delays, and records headroom on success.
Grade contracts, audit, config, and error types
src/signalforge/grade/models.py, src/signalforge/grade/audit.py, src/signalforge/grade/config.py, src/signalforge/grade/errors.py, src/signalforge/grade/__init__.py
Adds DegradeReasonType, threads degrade_reason_type and sweep_round into GradingResult/GradeEvent, bumps audit schema to v3, introduces sweep knobs and validators, and adds GradeIncompleteError export.
Adaptive grading core, sweep, completeness
src/signalforge/grade/engine.py
Refactors async dispatch to use a shared async rate limiter and AsyncConcurrencyGate, classifies degrade reasons, implements bounded transient re-grade sweep with sweep_round tagging, and enforces require_complete by raising GradeIncompleteError (post-sidecar write).
CLI flags and mapping
src/signalforge/cli/generate.py, src/signalforge/cli/_helpers.py, src/signalforge/skills/signalforge/SKILL.md
Adds --require-complete/--no-require-complete flags with default=None override semantics, revalidates GradeConfig overlay, and maps GradeIncompleteError to exit code 2; updates help and skill docs.
Tests — LLM limiter and client wiring
tests/llm/test_rate_limiter.py, tests/llm/test_rate_limiter_aimd.py, tests/llm/test_client_rate_limit_wiring.py, tests/llm/test_public_api.py, tests/llm/_fake.py
Extensive unit tests for RateLimitBudget, header parsing, AIMD behaviour, async gate semantics, ContextVar wiring, and client 429 handling.
Tests — Grade engine, sweep, completeness, audit
tests/grade/*, tests/cli/*, tests/fixtures/grade/*, tests/draft/test_schema.py
Adds/updates tests and fixtures to validate degrade_reason_type, bounded sweep, require_complete semantics, GradeIncompleteError contract, audit schema v3 fixtures, CLI exit-code behavior, and conftest sweep neutralization.
Benchmark harness
tests/research/179-runtime-benchmark/benchmark_runtime.py, tests/research/179-runtime-benchmark/test_benchmark_runtime.py
Adds --require-complete gating, structured degrade parsing, ungraded-pair extraction, and unit tests for the harness parsing helpers.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as signalforge generate
  participant Grade as grade_artifacts
  participant Core as _grade_artifacts_async_core
  participant Gate as AsyncConcurrencyGate
  participant LLM as call_llm_async
  participant Audit as _build_grade_event
  CLI->>Grade: start run (maybe --require-complete)
  Grade->>Core: dispatch main pass
  Core->>Gate: acquire slot (effective_concurrency)
  Gate->>LLM: evaluate pair
  LLM-->>Core: score or degrade (RateLimitBudget seen)
  Core->>Audit: write GradeEvent (degrade_reason_type, sweep_round)
  Core->>Core: bounded transient sweep retries transient degradations
  Grade->>CLI: write grade.json (sidecar)
  Grade->>CLI: raise GradeIncompleteError if non-exempt ungraded remain
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Poem

🐰 I nudged the rate limits, soft and neat,
Retries now whisper, not a herd's loud beat.
Sweep hums twice, then silence — all pairs scored,
Or names the few that stayed and were ignored.
A rabbit cheers: complete the grade, repeat!

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

wjduenow added 28 commits June 4, 2026 17:06
…info seam

Add the neutral, frozen RateLimitBudget value object (requests/tokens
remaining, requests/tokens reset, retry_after — all optional) plus a
vendor-neutral header-parse helper in a new
src/signalforge/llm/_rate_limiter.py.

Add LLMProvider.extract_rate_limit_info(exc, *, response=None) with a
base default returning an EMPTY budget (OpenAI/Gemini inherit it,
graceful degradation). AnthropicProvider overrides it to populate the
budget from retry-after + anthropic-ratelimit-* headers, reaching them
purely via duck-typed getattr so no anthropic.*/httpx.* type crosses the
provider seam (DEC-012 confinement upheld without an SDK import).

Parsing is fully tolerant: present/absent/malformed headers never raise;
a malformed numeric leaves that field None.

Tests: FakeRateLimitError/FakeResponseWithHeaders test doubles in
tests/llm/_fake.py + tests/llm/test_rate_limiter.py covering Anthropic
populated, OpenAI/Gemini empty, tolerant parsing, response>exc
precedence, and no-vendor-type-leak. Public-surface pins updated.

This is US-002 scope only — limiter classes and call_llm wiring are
US-003/US-004.
…_schema_version to 3

Add a structured degrade_reason_type discriminator (Literal["transient",
"budget","ceiling"] | None) to GradingResult and GradeEvent (None when
scored; set for every degraded pair). The prose->discriminator mapping is
centralised in _build_degraded via _classify_degrade_reason so the later
sweep / require_complete logic classifies degrades without string-matching
the human-readable reason text. Unknown reasons default conservatively to
"transient".

Bump GradeEvent.audit_schema_version 2 -> 3. Field defaults safely so
pre-#202 (v1/v2) records still load through the extra="ignore" production
model; the v2 replay anchor still validates.

Refresh fixtures + Strict drift mirrors: add grade_event_v3.jsonl (scored
+ all three degrade causes), add StrictGradeEventV3 (field-set-current
mirror), keep StrictGradeEvent / StrictGradeEventV2 as v1/v2 replay
anchors. Add degrade_reason_type to StrictGradingResult. Switch the
provider-neutrality end-to-end JSONL checks and the field-set parity gate
to the v3 mirror. Update docs/audits.md + docs/grade-ops.md.
…g (Stage-1)

Implement SyncRateLimiter / AsyncRateLimiter over a shared _RateLimiterState
cell with AIMD adaptive concurrency (multiplicative-decrease on 429,
additive-increase on headroom, clamped to [1, max_concurrent_calls]). Wire
both into the client 429 retry branches: on a 429 the limiter computes the
wait from retry-after / reset headers (via strategy.extract_rate_limit_info)
to pace AT the rate limit, falling back to the historical blind backoff when
the budget is empty (no headers / OpenAI / Gemini). Limiters are threaded via
current_sync_rate_limiter / current_async_rate_limiter ContextVars (optional,
default None) so signature parity between call_llm and call_llm_async is
preserved. _backoff_warn WARNING shape and the _sleep/_async_sleep/_rand_uniform
override seams are unchanged.

Headline acceptance: a 429+retry-after burst that previously exhausted the
3x429 budget now succeeds (limiter paces per the header). AIMD decrease/increase
+ bounds, header-vs-blind-fallback, and both client branches are unit-tested.

grade/engine.py untouched (US-004 owns the TaskGroup wiring).
…1 complete)

Wire the shared async rate limiter into the grade orchestrator so every
concurrent grade coroutine paces against ONE shared AIMD budget instead of
each blind-backing-off independently and bursting past the rate limit.

In `_grade_artifacts_async_core` (the `asyncio.run` core), build a cooperating
limiter pair via `make_rate_limiters(resolved_config.max_concurrent_calls)`
(seeding initial effective concurrency from `max_concurrent_calls`) and publish
the async sibling on the `current_async_rate_limiter` ContextVar so every
coroutine dispatched in the TaskGroup (`_one` -> `_grade_one_async` ->
`call_llm_async`) resolves the SAME limiter. The ContextVar is set inside the
core (which `asyncio.run` runs in a copied context) and reset in a try/finally
so it never leaks past the run.

The three mechanisms stay orthogonal: the limiter PACES (per-429 wait via
`retry-after`, blind-backoff fallback), the `asyncio.Semaphore` CAPS raw
concurrency, the `asyncio.timeout(effective_budget)` is the runaway backstop.
Limiter waits legitimately count against the budget, so the #198 over-budget
degrade path is unchanged.

Builds on US-002 (`RateLimitBudget`) and US-003 (`SyncRateLimiter`/
`AsyncRateLimiter` + ContextVars + `call_llm_async` 429 wiring); does not touch
`client.py` or `_rate_limiter.py`.

Tests (`tests/grade/test_engine_rate_limiter.py`):
- a concurrent 429-burst fan-out that previously left N transient
  `GradeLLMError` degradations now reaches 0 (all pairs scored,
  `aggregate_complete=True`);
- every concurrent coroutine observes the SAME non-None limiter, and the
  ContextVar resets to None after `grade_artifacts` returns (no leakage);
- the budget-timeout degrade path still degrades every pair and resets the
  ContextVar on the timeout path too (#198 no-regression).

Full gate green: ruff check/format, pyright, 3657 passed / 8 skipped, 97% cov.
…ield (Stage 2)

Add an ALWAYS-ON, bounded transient-recovery sweep (DEC-206) folded into the
async core so the GradingReport is built from POST-sweep results — a run
reaches 100% scored when transient LLM failures recover on a calmer retry.

- engine: after the main pass (and a test-overridable cool-down) re-grade
  ONLY score=None / degrade_reason_type=="transient" pairs SEQUENTIALLY
  (concurrency 1) via _grade_one_async, looping until zero transient pairs
  remain or sweep_max_rounds is reached. budget/ceiling degrades are never
  swept. results_by_index stays the single source of truth the sidecar +
  report read, so aggregates reflect the sweep. Each swept attempt appends a
  NEW sweep_round-tagged audit record (immutable log); a recovered pair is
  written to the grade cache via the existing fail-soft path.
- config: add sweep_max_rounds=3 (non-negative) and sweep_cooldown_seconds=2.0
  (non-negative finite; 0 disables the wait, sweep stays always-on). Cool-down
  routes through the _async_sleep test-override seam.
- models/audit: add additive optional GradeEvent.sweep_round (None main pass;
  1+ for sweep rounds). No audit_schema_version bump (stays 3).
- drift detector / v3 fixture / Strict mirror updated in lockstep.
- tests: recovery-to-completion, sweep_max_rounds cap (no infinite loop),
  budget/ceiling never swept, sweep_round-tagged record + cache reuse on
  recovery, cool-down seam, sweep_max_rounds=0 no-sweep, config validators.
  Autouse conftest neutralises the cool-down by default.
…ck (Stage 3 core)

Add the fail-loud completeness contract (DEC-204 + DEC-207):

- New tier-2 GradeIncompleteError (parent GradeError) carrying
  incomplete_pairs / require_complete / aggregate_complete; the message
  names the first ~20 ungraded pairs then '… and N more' (full list in
  the JSONL audit), repr-quoting ids for log-injection safety.
- Register GradeIncompleteError at tier 2 in _EXCEPTION_TO_EXIT_CODE
  (same tier as GradeBelowThresholdError); scan-7 + planted self-check
  pass.
- GradeConfig.require_complete: bool = True (extra='forbid').
- Engine check in grade_artifacts after the sidecar write + INFO log and
  before the fail_on_below_threshold raise. Branches on
  degrade_reason_type: 'transient' always trips; 'budget' trips only when
  total_budget_seconds is None (default-scaled); 'ceiling' and explicit
  'budget' are exempt. Raised after the sidecar is durably written.

No CLI flag (US-007 owns --require-complete); limiter/sweep untouched.
…age 3 CLI)

DEC-208. Surface the grade-completeness contract on the generate CLI with
--require-complete / --no-require-complete via argparse.BooleanOptionalAction
(default=None no-clobber sentinel). Overrides grade.require_complete ONLY when
explicitly passed, via GradeConfig.model_validate so validators re-run; a bare
run never re-arms a grade.require_complete: false set in signalforge.yml.

When armed, the engine (US-006, merged) raises GradeIncompleteError (tier 2,
exit 2) after the sidecar write, naming the still-ungraded pairs in stderr.

Six parity surfaces in lockstep: argparse help, cmd_generate +
_run_single_model docstrings, docs/cli-ops.md (flag ref + exit-code table +
Grade-completeness behaviour stderr shape), tests/cli/test_generate.py
(override True/False, no-clobber preserve, e2e transient-incomplete exit 2),
DEC-208 in plans/super/9-cli-entrypoint.md, and SKILL.md (skill-parity gate).
… docs/rules (Stage 4)

Raise GradeConfig.max_retries_429 default 3 -> 6 (DEC-209; belt-and-braces
over the #202 header-honoring rate limiter, the primary 429 fix). Update
its docstring, the defaults-inventory comment, and the config test pinning
the default.

Add a deterministic guard test
(test_default_scaled_budget_is_non_binding_at_representative_scale) that
asserts the DEFAULT scaled budget leaves clear headroom (>=1.5x) over a
modelled limiter-paced completion at a representative scale, so a default
run never passively binds on the budget (DEC-210). Pure arithmetic on
_compute_effective_budget — no sleeping or LLM calls.

Docs/rules brought into line with the bias-to-completion posture and the
transient-recoverable / operator-ceiling / unrecoverable taxonomy:
- docs/grade-ops.md: new Bias-to-completion posture section (opt-in limit
  knobs, default-budget-trip-fails-loud, three degrade classes,
  require_complete contract); raised max_retries_429 reflected; cost-knob
  section no longer claims a default-budget trip silently completes.
- docs/cli-ops.md: grade-completeness section points to the posture.
- .claude/rules/grade-layer.md: new bias-to-completion + degrade-class
  section; 'partial is acceptable' scoped to operator-ceiling only.
- .claude/rules/llm-drafter.md: document the rate-limit-aware limiter seam
  (header-honoring backoff + shared AIMD limiter via ContextVars) and the
  concurrency<->rate-limit relationship that caused the original ~70
  degradations.

Logic in the limiter/sweep/require_complete path is unchanged.

Full gate green: ruff check + format, pyright (0 errors), pytest
3682 passed / 8 skipped, coverage 97.26%.
…up scaffold (live run pending)

Wire the #202 grade-to-completion changes into the standalone #179 runtime
benchmark harness (AUTOMATABLE PREP ONLY — the live metered run is operator-only):

- benchmark_runtime.py: add a version-gated --require-complete flag (omitted on
  the prod arm exactly like --no-cache); classify degradations on the #202
  degrade_reason_type discriminator (US-001) with a prose fallback for pre-#202
  sidecars; report aggregate_complete, the per-reason split, and the ungraded
  (artifact, criterion) pair list; explain exit code 2 = GradeIncompleteError.
- test_benchmark_runtime.py: unit-test the pure sidecar-parsing helpers (no API
  key, no subprocess) — loaded by file path so collection never mutates sys.path.
- docs/research/179-runtime-benchmark.md: add a clearly-marked '#202
  grade-to-completion retest' section with EMPTY _pending live run_ placeholder
  cells alongside the prod / dev #198 baselines, plus exact live-run instructions
  (command, env vars, 40-col + 16-col models, cold grade cache, fixed rate tier).

No benchmark numbers fabricated; every #202 result cell is an explicit
placeholder for the operator's live run. Full gate green.
…ess + writeup scaffold (live run pending)
…408/408 + 212/212 scored, aggregate_complete=True)
…ew fixes

#202 Quality-Gate review fixes (4-pass review: 2 MAJOR + 3 minor).

FIX 1 (MAJOR) — Wire the AIMD adaptive concurrency for real.
- New AsyncConcurrencyGate in _rate_limiter.py: an asyncio.Condition + in-flight
  counter that admits at most the limiter's live effective_concurrency in-flight
  grade calls (replacing the fixed asyncio.Semaphore(max_concurrent_calls) in the
  grade engine). Reads its cap from the SAME shared _RateLimiterState cell the
  limiter pair publishes (one source of truth). acquire() blocks while
  in_flight >= cap (floored at 1 so a fully-throttled run still makes progress);
  release() (in __aexit__/finally) notifies all waiters so a freed slot OR a
  headroom-widened cap wakes a blocked acquirer (no lost wakeup). The threading
  lock is never held across an await; the in-flight counter mutates only on the
  event loop under the Condition.
- Engine _one now uses `async with gate:` instead of the semaphore.
- call_llm_async calls limiter.record_headroom() on a clean (non-429) return,
  gated on a limiter being present (sync drafter / limiter-free async unaffected),
  so a 429 narrows the cap and clean completions probe it back toward
  max_concurrent_calls. Bounds rigorously [1, max_concurrent_calls].
- Banner/docstrings in _rate_limiter.py + engine.py updated to describe the now
  FUNCTIONAL adaptive concurrency.
- Tests: gate admits up-to-cap, blocks the (N+1)th, narrows under a 429 storm,
  widens back on headroom (wakes waiter), stays in [1,max], releases in finally;
  engine-level proof that in-flight concurrency narrows below max and widens back.

FIX 2 (MAJOR) — Bound the sweep's wall-clock.
- New GradeConfig.sweep_budget_seconds (default 300, positive validator). The
  always-on sweep loop is wrapped in its own asyncio.timeout(sweep_budget_seconds);
  on TimeoutError the sweep STOPS (does not raise) and leaves remaining transient
  pairs degraded (fail loud under require_complete, or honest partial otherwise).
  One forensic WARNING. Test: a never-recovering slow sweep is bounded.

FIX 3 (MINOR) — reset-header honesty. Implemented the reset-derived fallback:
when a 429 carries no retry-after but a requests/tokens reset instant,
_wait_from_budget derives a bounded wait (soonest reset, capped 60s vs clock
skew) via an injectable _utcnow clock. Code + comments now agree (reset IS
honoured). Deterministic tests via a pinned clock.

FIX 4 (MINOR) — burst acceptance baseline now uses the SAME input on both arms
(3x429+success, retry-after:30) and asserts the limiter paces every retry at the
header value (30,30,30) vs the no-limiter blind backoff (1,2,4); separate test
pins that an over-budget burst still exhausts (the limiter paces, it does not
enlarge the budget).

FIX 5 (TRIVIAL) — grade-layer.md degrade_reason_type cite DEC-203 -> US-001.

Design choices:
- Adaptive gate = asyncio.Condition + in-flight counter sharing the limiter's
  state cell (Semaphore can't be resized; Condition gives correct wakeups on cap
  growth + release-in-finally under cancellation).
- Sweep budget = explicit config knob (default 300s) rather than a derived value:
  simplest, most testable, mirrors the main-pass effective_budget posture.

Full canonical gate green: ruff check + format + pyright clean; pytest
3709 passed, 8 skipped; coverage 97.24% (>= 80).
…y patterns and the validated throughput lesson
@wjduenow
wjduenow marked this pull request as ready for review June 5, 2026 21:38
@wjduenow
wjduenow requested a review from Copilot June 5, 2026 21:38

Copilot AI 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.

Pull request overview

Adds a “super plan” document for issue #202 describing the staged approach to achieving 100% graded pairs (or failing loud) via shared rate-limit-aware throttling, a bounded degraded-pair sweep, and a default-on require_complete contract, plus related docs/defaults and a benchmark retest.

Changes:

  • Introduces a detailed, staged implementation plan (Stages 1–4) including DECs, acceptance criteria, and story breakdown.
  • Captures architecture decisions (rate limiter shape, sweep semantics, GradeIncompleteError / require_complete behavior) and enumerates the required repo gates (drift detectors, AST scans, logger grep, parity surfaces).
  • Defines a beads manifest / sequencing for the planned work.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plans/super/202-grade-to-100.md Outdated

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plans/super/202-grade-to-100.md`:
- Line 115: The markdown table row under "Sweep audit forensics" contains
"sweep_round: int|None" which breaks the table because the pipe is treated as a
column separator; fix it by escaping the pipe or wrapping the type in a code
span (e.g., `sweep_round: int\|None` or ``sweep_round: `int|None` ``) so the
cell remains a single column and the table renders correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da6074ec-6a42-4889-956b-f45c19c4df5b

📥 Commits

Reviewing files that changed from the base of the PR and between 365a89d and e88b23e.

📒 Files selected for processing (1)
  • plans/super/202-grade-to-100.md

Comment thread plans/super/202-grade-to-100.md Outdated
@wjduenow wjduenow changed the title #202: Grade to 100% — rate-limit throttle + degraded-pair sweep + require-complete (plan) #202: Grade to 100% — rate-limit throttle + degraded-pair sweep + require-complete Jun 5, 2026
@wjduenow

wjduenow commented Jun 5, 2026

Copy link
Copy Markdown
Owner Author

Closeout — #202 complete

All 11 stories + the epic are closed and merged onto feature/202-grade-to-100. Full canonical gate green locally (ruff · ruff format --check · pyright 0 errors · pytest 3709 passed). 4 code-review passes + an adversarial concurrency review of the new AsyncConcurrencyGate (clean).

Live metered retest (2026-06-05) — PASS both arms: 40-col 412/412 scored (#198's 70 transient GradeLLMError → 0), 16-col 220/220 scored (#186's 34 score=None → 0), aggregate_complete=True, and grade faster than the non-adaptive pass (561.0s vs 622.0s, 0.73/s). Recorded in docs/research/179-runtime-benchmark.md.

Ready for review.

@wjduenow

wjduenow commented Jun 5, 2026

Copy link
Copy Markdown
Owner Author

PR Review Summary

Fixed (1)

File Line Issue Commit
plans/super/202-grade-to-100.md 115 Unescaped int|None pipe broke the 3-col table (CodeRabbit, MD056) escaped the pipe

False positives / already-resolved (1)

File Issue Reason
plans/super/202-grade-to-100.md Plan meta "devolved" vs PR description "detailing" (Copilot) Stale at plan-PR time. Both updated since: the PR body now describes the full merged implementation, and the plan meta is Complete. No longer inconsistent.

All review threads resolved.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/signalforge/grade/audit.py`:
- Around line 103-105: The function creating a GradeEvent accepts sweep_round
but doesn't enforce the contract that sweep_round must be None or >=1; add an
explicit validation early in the GradeEvent construction seam: if sweep_round is
not None and sweep_round < 1, raise a ValueError (or similar) with a clear
message. Apply the same guard to the other GradeEvent/factory location
referenced (the block around lines 138-143) so both creation sites (symbols:
sweep_round, degrade_reason_type, GradeEvent, DegradeReasonType) validate inputs
before persisting.

In `@src/signalforge/llm/_rate_limiter.py`:
- Around line 114-127: The _parse_float function currently converts values like
"NaN" or "inf" into non-finite floats; update _parse_float to treat non-finite
results as malformed by importing math and after parsing call
math.isfinite(parsed) and return None if it's not finite, preserving the current
behavior of returning None for None, empty, or invalid strings; ensure you
update the function named _parse_float to perform this finite check before
returning the float so "nan"/"inf" do not propagate into retry/wait handling.

In `@tests/grade/test_config.py`:
- Around line 242-244: Extend the defaults guard inside
test_grade_config_defaults_match_dec_023_to_027 to also assert the new
`#202-related` default settings: add assertions that cfg.sweep_max_rounds,
cfg.sweep_cooldown_seconds, cfg.sweep_budget_seconds, and cfg.require_complete
equal their expected default values (use the canonical defaults defined for
DEC_023..027/#202); update the test body around the existing cfg.max_retries_429
assertion so these four new assertions are included to pin those knobs against
accidental drift.

In `@tests/research/179-runtime-benchmark/benchmark_runtime.py`:
- Around line 398-408: Update the help string passed to parser.add_argument for
the "--require-complete" flag (the parser.add_argument call defining that flag)
to clarify that omitting the flag does not guarantee "off/report-only" behavior
because the effective behavior inherits grade.require_complete from the
configuration; explicitly state the default is determined by config
(grade.require_complete) and that runs may still exit non‑zero if that config
enables strict behavior; make the analogous change to the second occurrence of
the same flag help text (the other parser.add_argument block around the 456-462
area) so both help messages reflect that the flag overrides config but omission
defers to grade.require_complete in config.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 662269b5-af9e-4f0a-b911-aceff342ba90

📥 Commits

Reviewing files that changed from the base of the PR and between e88b23e and 22c84af.

📒 Files selected for processing (44)
  • .claude/rules/grade-layer.md
  • .claude/rules/llm-drafter.md
  • docs/audits.md
  • docs/cli-ops.md
  • docs/grade-ops.md
  • docs/research/179-runtime-benchmark.md
  • plans/super/202-grade-to-100.md
  • plans/super/9-cli-entrypoint.md
  • src/signalforge/cli/_helpers.py
  • src/signalforge/cli/generate.py
  • src/signalforge/grade/__init__.py
  • src/signalforge/grade/audit.py
  • src/signalforge/grade/config.py
  • src/signalforge/grade/engine.py
  • src/signalforge/grade/errors.py
  • src/signalforge/grade/models.py
  • src/signalforge/llm/__init__.py
  • src/signalforge/llm/_rate_limiter.py
  • src/signalforge/llm/client.py
  • src/signalforge/llm/providers.py
  • src/signalforge/skills/signalforge/SKILL.md
  • tests/cli/test_exit_codes.py
  • tests/cli/test_generate.py
  • tests/draft/test_schema.py
  • tests/fixtures/grade/example_config.yml
  • tests/fixtures/grade/grade_event_v3.jsonl
  • tests/grade/conftest.py
  • tests/grade/test_audit.py
  • tests/grade/test_config.py
  • tests/grade/test_drift_detector.py
  • tests/grade/test_engine.py
  • tests/grade/test_engine_rate_limiter.py
  • tests/grade/test_errors.py
  • tests/grade/test_gemini_neutrality.py
  • tests/grade/test_models.py
  • tests/grade/test_provider_neutrality.py
  • tests/grade/test_provider_neutrality_openai.py
  • tests/llm/_fake.py
  • tests/llm/test_client_rate_limit_wiring.py
  • tests/llm/test_public_api.py
  • tests/llm/test_rate_limiter.py
  • tests/llm/test_rate_limiter_aimd.py
  • tests/research/179-runtime-benchmark/benchmark_runtime.py
  • tests/research/179-runtime-benchmark/test_benchmark_runtime.py
✅ Files skipped from review due to trivial changes (5)
  • tests/fixtures/grade/example_config.yml
  • docs/audits.md
  • .claude/rules/llm-drafter.md
  • src/signalforge/grade/init.py
  • plans/super/202-grade-to-100.md

Comment thread src/signalforge/grade/audit.py
Comment thread src/signalforge/llm/_rate_limiter.py
Comment thread tests/grade/test_config.py
Comment thread tests/research/179-runtime-benchmark/benchmark_runtime.py
- _rate_limiter._parse_float: reject non-finite (nan/inf) retry-after values
  (a non-finite wait would poison backoff arithmetic) + tests
- audit._build_grade_event: enforce sweep_round >= 1 at the construction seam + tests
- test_config: pin the new #202 defaults (sweep_max_rounds/cooldown/budget, require_complete)
- benchmark_runtime: correct misleading --require-complete default messaging
@wjduenow

wjduenow commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

PR Review Summary (CodeRabbit round 2)

All 4 addressed — fixed in 7170bee, validation green (3717 passed).

Fixed (4)

File Issue Fix
src/signalforge/llm/_rate_limiter.py 🟠 Major — _parse_float accepted non-finite nan/inf retry-after Added math.isfinite guard → non-finite parses to None (malformed posture); added nan/inf/Infinity parametrized tests
src/signalforge/grade/audit.py 🟡 sweep_round lower bound not enforced _build_grade_event now raises ValueError on sweep_round < 1 at the single construction seam; added accept/reject tests
tests/grade/test_config.py 🟡 defaults guard didn't cover new #202 knobs Pinned sweep_max_rounds=3, sweep_cooldown_seconds=2.0, sweep_budget_seconds=300, require_complete=True
tests/research/179-runtime-benchmark/benchmark_runtime.py 🟡 misleading --require-complete default messaging Reworded help + require_note: omitting the flag follows project grade.require_complete config/default, not "off"

False positives (0)

All review threads resolved.

@wjduenow
wjduenow merged commit a0d6093 into dev Jun 9, 2026
6 checks passed
@wjduenow
wjduenow deleted the feature/202-grade-to-100 branch June 9, 2026 15:12
Sign up for free to 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