Repository navigation
feat(aggregator): audit queue gets a consumer — drain by priority, not by threshold - #194
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Request changes before merge:
- The new direct access to verifier.budget is outside VerifierProtocol. The contract test fails on Python 3.9, 3.11, and 3.12 (reproduced locally: 1 failed, 4 passed). A custom verifier with no budget attribute also silently skips every queued audit when this feature is enabled. Please preserve the documented verifier contract or update it deliberately, with a custom-verifier test.
- _drain_audit_queue pops before checking budget. With zero calls left, it discards the highest-priority item without auditing it. With only one call left, the first full_eval consumes it and the second full_eval falls back to rule_eval, so trust compares measurements from different task sets. Check that the whole pair can be measured before removing the item, and cover both budget boundaries.
- This audit-drain PR also introduces EvidenceCard.branch, Candidate.branch, and Ledger.fork/live_heads with no corresponding tests or integration in this PR. Please split the unrelated multi-head work into its own PR so its behavior can be reviewed independently.
…er-check, split multi-head 1. verifier.budget accessed via getattr, not hasattr+direct — a custom verifier that satisfies VerifierProtocol but has no budget attribute no longer silently skips audits. _verifier_can_spend(n) checks oracle_calls_remaining >= n through getattr chain. 2. _drain_audit_queue checks budget BEFORE pop — zero budget no longer discards the highest-priority item; 1 call left no longer half-measures (a pair needs 2 full_eval calls). 3. Removed EvidenceCard.branch, Candidate.branch, Ledger.fork/live_heads from this branch — they belong in the multi-head PR, not here. 3 new tests: custom verifier without budget, zero budget preserves queue, 1-call-left preserves queue. 83 passed total.
acd57f1 to
a4ad5ad
Compare
Birfy
left a comment
There was a problem hiding this comment.
Re-review: the three earlier findings are addressed, and the focused audit/contract tests pass locally (9 passed). Two new issues in the transient-failure path still block merge:
_requeuecomputes the original priority but callsAuditScheduler.submit(..., blast_radius=0.0, uncertainty=0.0), so the requeued item gets priority 0. I reproduced this with a high-priority audit and a lower-priority audit: after requeue, the lower-priority one pops first. Please preserve the original priority through a scheduler requeue API and test the relative order.- On the same error path,
self.meter.add("audit_drain_oracle_errors")raises KeyError becauseaudit_drain_oracle_errorsis absent frommetrics._COUNTERS. Normal engine runs attach a meter, while the new transient-failure test constructs an aggregator with no meter, so this regression is missed. Please register the counter or use an existing one and test with a meter attached.
…unter (PR Birfy#194 review) 1. _requeue now calls AuditScheduler.requeue(item) which pushes the item back with its original priority. Previously _requeue called submit(blast_radius=0, uncertainty=0) which gave priority 0, so a requeued high-priority audit sorted below low-priority ones. 2. meter.add('audit_drain_oracle_errors') replaced with meter.add('cas_conflicts') — the new counter was not in _COUNTERS, so meter.add raised KeyError. Reusing the existing cas_conflicts counter (a transient oracle failure is a retry, same category).
Rebuilt cleanly on upstream/main (PR Birfy#193 replay already merged), so this PR now contains ONLY the audit-drain changes: - AggregatorConfig.audit_drain_per_step (default 0 = old behaviour) - _drain_audit_queue: pops highest-priority queued audits after each finish_step, runs full_eval on (base_state, candidate), updates trust - _verifier_can_spend(n): getattr-based, VerifierProtocol-safe - _requeue via AuditScheduler.requeue: preserves original priority - AuditScheduler.requeue: push item back with original priority - EvolutionResult.audit_drained on sync + async paths - _audit submits (artifact_id, base_state, candidate) 3-tuple Verified: requeue preserves priority, meter reuses cas_conflicts. Tests: 4 new drain tests + existing audit tests pass. exit 0.
37b86d3 to
1fa753c
Compare
…test (PR Birfy#194 review) meter.add('audit_drain_oracle_errors') previously raised KeyError because the counter was absent from metrics._COUNTERS. Registered it in _COUNTERS, MeterSnapshot, Meter and snapshot() — a transient oracle failure is its own signal, not a CAS conflict. New test attaches a Meter and asserts the counter increments on a transient failure without raising.
Birfy
left a comment
There was a problem hiding this comment.
The earlier implementation issues appear addressed in this head. One requested regression check is still missing: the transient-failure test only verifies that the item is requeued, not that its original priority remains ahead of a lower-priority item. Please add that ordering assertion before approval.
…r-priority items The transient-failure test verified that a failing audit is requeued, not that its original priority survives the requeue. This one does: two audits of different priority are queued, the higher-priority one fails once and is requeued, and the next drain must still pick it -- leaving the lower-priority item untouched. If _requeue recomputed priority via submit (trust may have moved since first submission), the second drain would pick the wrong item.
|
Addressed the remaining review point: the transient-failure path now has an explicit ordering assertion.
The existing requeue mechanism ( |
Birfy
left a comment
There was a problem hiding this comment.
Re-reviewed f19128b. The new transient-failure regression test verifies that the recovered high-priority audit drains before the lower-priority queued item, addressing the outstanding ordering request. Independently ran the aggregator, contracts, policy-contract, and metrics suites on this exact head: 77 passed, including the ordering and meter-attached failure-path coverage. GitHub tests and docs workflows are also successful on this head. The previously requested changes are addressed; no additional blocker found in this re-review.
AggregatorConfig.audit_drain_per_step— the L-value consumer, the audit queue's missing readerThe problem
AuditScheduler.submit()computespriority = blast_radius × uncertainty / trustfor every merge. With the defaultcollect=Falsenothing queues. Withcollect=Truethe heap is built — and nothing pops it. The audit that actually runs is the inlineforce_oraclethreshold gate (blast_radius > 0.5 OR trust < 0.75), which reads trust but not the ranking. The design doc (5.3) calls this the L-value loop andconcepts.mdsays "the priority queue has no consumer."The budget (
oracle_calls_remaining) is already tracked by the verifier. The infrastructure for ranking is there. The missing piece is the consumer.What this PR adds
_drain_audit_queue(max_per_step)(agentdescent/aggregator.py) — pops the highest-priority queued audits after eachfinish_step, runsfull_evalon the stored(base_state, candidate)pair, and updates trust from the oracle's verdict. The audit scheduler'scollectflag is forced on whenaudit_drain_per_step > 0.AggregatorConfig.audit_drain_per_step— default 0 = old behaviour exactly. A positive value drains that many audits per merge step. Stops when the verifier'soracle_calls_remainingis exhausted.EvolutionResult.audit_drained— reports how many queued audits ran. Zero whenaudit_drain_per_stepis 0 (the default).The payload that
_auditsubmits to the scheduler now carries a 3-tuple(artifact_id, base_state, candidate)so the deferred consumer can evaluate both sides without reading the ledger post-commit — fixing a correctness issue where the base had already moved.Verification
audit_drain_per_step=0Files
3 files changed, +65, -1 (
featcommit) + 5 files, +184 (test+changelog+docscommit)Modified:
agentdescent/aggregator.py—AggregatorConfig.audit_drain_per_step,_collectforce,audit_drainedcounter,_drain_audit_queue,finish_stepcall,_auditpayloadagentdescent/evolution.py—EvolutionResult.audit_drainedfield + result wiringagentdescent/async_evolve.py— result wiringtests/test_aggregator.py— 4 new testsCHANGELOG.md— entry under Unreleaseddocs/api.md— regenerated