cleveragents/cleveragents-core

Fork 3

fix(acms): align budget allocation formula and pipeline protocol signatures with specification #924

New issue

Closed

opened 2026-03-14 00:11:11 +00:00 by freemo · 1 comment

freemo commented

2026-03-14 00:11:11 +00:00

Owner

Background

The ACMS context budget allocation has three mismatches with the specification:

1. Allocation Formula: `confidence` vs `confidence * quality_score` (HIGH)

Spec (line 44909): Budget allocated proportional to confidence * quality_score
Code (acms_pipeline.py:267): raw = [(total_budget * c / total_confidence) for _, c in candidates] — proportional to confidence only
Currently equivalent by coincidence (strategy stubs return quality_score as confidence) but diverges for any custom strategy

2. `BudgetAllocatorProtocol.allocate` Missing `request` Parameter (MEDIUM)

Spec (lines 44664-44668): allocate(self, candidates, total_budget, request: ContextRequest)
Code (acms_service.py:259-264): allocate(self, candidates, total_budget) — no request
Prevents request-aware allocation (e.g., different allocation for strategize vs execute views)

3. `DetailDepthResolverProtocol.resolve` Missing `budget` Parameter (MEDIUM)

Spec (lines 44713-44720): resolve(self, fragments, budget: int)
Code (acms_service.py:299-305): resolve(self, fragments) — no budget
Prevents budget-aware depth resolution

4. Default Packer Not Wired (MEDIUM)

Spec (line 44919): Default packer is GreedyKnapsackPacker
Code (acms_service.py:631, acms_pipeline.py:536): Both default to DefaultBudgetPacker() (no-op)
Config registers builtin:GreedyKnapsackPacker (config_service.py:831-835) but pipelines don't read from config

Acceptance Criteria

BudgetAllocator.allocate() takes request: ContextRequest parameter
Allocation formula uses confidence * quality_score per spec
DetailDepthResolver.resolve() takes budget: int parameter
Pipeline constructors resolve default components from config (or use production implementations directly)
Existing pipeline and strategy tests updated to match new signatures
Existing tests pass

Metadata

Suggested commit message: fix(acms): align budget allocation formula and protocol signatures with spec
Suggested branch name: fix/acms-budget-allocation-spec-alignment

Definition of Done

Code merged to main, budget allocation formula matches spec, all pipeline protocol signatures match spec.

## Background The ACMS context budget allocation has three mismatches with the specification: ### 1. Allocation Formula: `confidence` vs `confidence * quality_score` (HIGH) - **Spec** (line 44909): Budget allocated proportional to `confidence * quality_score` - **Code** (`acms_pipeline.py:267`): `raw = [(total_budget * c / total_confidence) for _, c in candidates]` — proportional to `confidence` only - Currently equivalent by coincidence (strategy stubs return quality_score as confidence) but diverges for any custom strategy ### 2. `BudgetAllocatorProtocol.allocate` Missing `request` Parameter (MEDIUM) - **Spec** (lines 44664-44668): `allocate(self, candidates, total_budget, request: ContextRequest)` - **Code** (`acms_service.py:259-264`): `allocate(self, candidates, total_budget)` — no `request` - Prevents request-aware allocation (e.g., different allocation for strategize vs execute views) ### 3. `DetailDepthResolverProtocol.resolve` Missing `budget` Parameter (MEDIUM) - **Spec** (lines 44713-44720): `resolve(self, fragments, budget: int)` - **Code** (`acms_service.py:299-305`): `resolve(self, fragments)` — no `budget` - Prevents budget-aware depth resolution ### 4. Default Packer Not Wired (MEDIUM) - **Spec** (line 44919): Default packer is `GreedyKnapsackPacker` - **Code** (`acms_service.py:631`, `acms_pipeline.py:536`): Both default to `DefaultBudgetPacker()` (no-op) - Config registers `builtin:GreedyKnapsackPacker` (`config_service.py:831-835`) but pipelines don't read from config ## Acceptance Criteria - [x] `BudgetAllocator.allocate()` takes `request: ContextRequest` parameter - [x] Allocation formula uses `confidence * quality_score` per spec - [x] `DetailDepthResolver.resolve()` takes `budget: int` parameter - [x] Pipeline constructors resolve default components from config (or use production implementations directly) - [x] Existing pipeline and strategy tests updated to match new signatures - [x] Existing tests pass ## Metadata - **Suggested commit message:** `fix(acms): align budget allocation formula and protocol signatures with spec` - **Suggested branch name:** `fix/acms-budget-allocation-spec-alignment` ## Definition of Done Code merged to `main`, budget allocation formula matches spec, all pipeline protocol signatures match spec.

freemo added the

labels

2026-03-14 00:11:57 +00:00

freemo added this to the v3.4.0 milestone

2026-03-14 00:12:05 +00:00

aditya was assigned by freemo

2026-03-14 04:26:59 +00:00

aditya added

and removed

labels

2026-03-23 20:01:43 +00:00

aditya commented

2026-03-23 21:29:27 +00:00

Member

Implementation Notes

Changes Made

1. BudgetAllocator.allocate() now takes request: ContextRequest | None = None

Updated BudgetAllocator protocol in acms_service.py to add optional request parameter
Updated DefaultBudgetAllocator.allocate() and ProportionalBudgetAllocator.allocate() implementations
Threaded request through ACMSPipeline.assemble() and ContextAssemblyPipeline.assemble() call sites
StrategyCoordinator.coordinate() passes None for now (future: construct ContextRequest from dict)
Import ContextRequest from cleveragents.domain.models.acms.crp

2. Allocation formula now uses confidence * quality_score

Both DefaultBudgetAllocator and ProportionalBudgetAllocator now compute allocation weight as confidence * strategy.capabilities.quality_score instead of just confidence
Added _weighted_score() static method to both allocator classes
Uses getattr(strategy.capabilities, "quality_score", 1.0) as defensive fallback
Added quality_score: float = 1.0 to StrategyCapabilities in acms_service.py (was already present in domain model)
With quality_score=1.0 (default), formula is backward-compatible: confidence * 1.0 = confidence
_filter_by_min_budget() also uses weighted formula for proportional share calculation

3. DetailDepthResolver.resolve() now takes budget: int = 0

Updated DetailDepthResolver protocol in acms_service.py
Updated DefaultDepthResolver.resolve() and MaxDepthResolver.resolve() in acms_phase2.py
Updated call sites in ACMSPipeline.assemble(), ContextAssemblyPipeline.assemble(), and FusionEngine.fuse() to pass budget.available_tokens

4. Default packer wired to GreedyKnapsackPacker

ACMSPipeline.__init__() now uses GreedyKnapsackPacker via lazy import (_get_greedy_knapsack_packer_class()) to avoid circular imports
ContextAssemblyPipeline.__init__() directly imports and uses GreedyKnapsackPacker
Removed DefaultBudgetPacker import from acms_pipeline.py (no longer needed as default)

Test Updates

Updated _HalvingAllocator mock in acms_pipeline_steps.py to accept request parameter
Updated 5 feature scenarios in acms_pipeline.feature and acms_pipeline_orchestrator.feature where the GreedyKnapsackPacker (which sorts by relevance_score descending) changed fragment ordering vs. the old no-op DefaultBudgetPacker. Fixed by adjusting relevance scores so packer ordering matches test expectations.
Updated robot/helper_acms_pipeline.py tiered test data to match.
All mock strategies already had capabilities properties with quality_score=1.0 default.

Design Decisions

request parameter is optional (None default): The spec shows request: ContextRequest as required, but the pipeline's assemble() method doesn't always have a ContextRequest available. Making it optional preserves backward compatibility while enabling request-aware allocation when provided.
budget parameter has int = 0 default: Allows existing callers to omit it, matching the spec's intent while being backward-compatible.
Lazy import for GreedyKnapsackPacker: acms_service.py imports from acms_phase2.py which imports from acms_service.py, creating a circular dependency. Used a lazy-loading helper function to break the cycle.
Test data adjustments over assertion changes: Instead of weakening test assertions (e.g., checking set membership instead of order), adjusted test fragment relevance scores so the expected ordering is preserved after packing.

Quality Gates

nox -s lint: PASS
nox -s typecheck: 0 errors
nox -s unit_tests: 12,230 scenarios, 0 failed
nox -s integration_tests: 1,672 tests, 0 failed
nox -s coverage_report: 98.38% >= 97% threshold

## Implementation Notes ### Changes Made **1. `BudgetAllocator.allocate()` now takes `request: ContextRequest | None = None`** - Updated `BudgetAllocator` protocol in `acms_service.py` to add optional `request` parameter - Updated `DefaultBudgetAllocator.allocate()` and `ProportionalBudgetAllocator.allocate()` implementations - Threaded `request` through `ACMSPipeline.assemble()` and `ContextAssemblyPipeline.assemble()` call sites - `StrategyCoordinator.coordinate()` passes `None` for now (future: construct `ContextRequest` from dict) - Import `ContextRequest` from `cleveragents.domain.models.acms.crp` **2. Allocation formula now uses `confidence * quality_score`** - Both `DefaultBudgetAllocator` and `ProportionalBudgetAllocator` now compute allocation weight as `confidence * strategy.capabilities.quality_score` instead of just `confidence` - Added `_weighted_score()` static method to both allocator classes - Uses `getattr(strategy.capabilities, "quality_score", 1.0)` as defensive fallback - Added `quality_score: float = 1.0` to `StrategyCapabilities` in `acms_service.py` (was already present in domain model) - With `quality_score=1.0` (default), formula is backward-compatible: `confidence * 1.0 = confidence` - `_filter_by_min_budget()` also uses weighted formula for proportional share calculation **3. `DetailDepthResolver.resolve()` now takes `budget: int = 0`** - Updated `DetailDepthResolver` protocol in `acms_service.py` - Updated `DefaultDepthResolver.resolve()` and `MaxDepthResolver.resolve()` in `acms_phase2.py` - Updated call sites in `ACMSPipeline.assemble()`, `ContextAssemblyPipeline.assemble()`, and `FusionEngine.fuse()` to pass `budget.available_tokens` **4. Default packer wired to `GreedyKnapsackPacker`** - `ACMSPipeline.__init__()` now uses `GreedyKnapsackPacker` via lazy import (`_get_greedy_knapsack_packer_class()`) to avoid circular imports - `ContextAssemblyPipeline.__init__()` directly imports and uses `GreedyKnapsackPacker` - Removed `DefaultBudgetPacker` import from `acms_pipeline.py` (no longer needed as default) ### Test Updates - Updated `_HalvingAllocator` mock in `acms_pipeline_steps.py` to accept `request` parameter - Updated 5 feature scenarios in `acms_pipeline.feature` and `acms_pipeline_orchestrator.feature` where the `GreedyKnapsackPacker` (which sorts by `relevance_score` descending) changed fragment ordering vs. the old no-op `DefaultBudgetPacker`. Fixed by adjusting relevance scores so packer ordering matches test expectations. - Updated `robot/helper_acms_pipeline.py` tiered test data to match. - All mock strategies already had `capabilities` properties with `quality_score=1.0` default. ### Design Decisions - **`request` parameter is optional (`None` default)**: The spec shows `request: ContextRequest` as required, but the pipeline's `assemble()` method doesn't always have a `ContextRequest` available. Making it optional preserves backward compatibility while enabling request-aware allocation when provided. - **`budget` parameter has `int = 0` default**: Allows existing callers to omit it, matching the spec's intent while being backward-compatible. - **Lazy import for `GreedyKnapsackPacker`**: `acms_service.py` imports from `acms_phase2.py` which imports from `acms_service.py`, creating a circular dependency. Used a lazy-loading helper function to break the cycle. - **Test data adjustments over assertion changes**: Instead of weakening test assertions (e.g., checking set membership instead of order), adjusted test fragment relevance scores so the expected ordering is preserved after packing. ### Quality Gates - `nox -s lint`: PASS - `nox -s typecheck`: 0 errors - `nox -s unit_tests`: 12,230 scenarios, 0 failed - `nox -s integration_tests`: 1,672 tests, 0 failed - `nox -s coverage_report`: 98.38% >= 97% threshold

aditya referenced this issue from a commit

2026-03-23 21:30:43 +00:00

fix(acms): align budget allocation formula and protocol signatures with spec

aditya referenced this issue from a pull request that will close it,

2026-03-23 21:31:06 +00:00

fix(acms): align budget allocation formula and protocol signatures with spec #1140

aditya added

and removed