Skip to content

Fix the defects the agents page turned up - #21

Merged
AlexeyShalaev merged 6 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

Fix the defects the agents page turned up#21
AlexeyShalaev merged 6 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Seven findings from reading the source against the docs while docs/agents.md was being
written. Each one was reproduced first; all seven reproduced. Nothing here is breaking:
no name, default or signature changed.

1. OperationDeadlineConfig(call_caps=...) silently produced a config with no caps

The settings field is calls_caps, BudgetContext.create takes call_caps. Pydantic
ignores unknown keys, so the near-miss built a config whose caps were empty, and
DeadlineContextFactory then handed every call of that operation the full remaining
budget, uncapped, with no error anywhere:

>>> OperationDeadlineConfig(call_caps={"identity_create_user": 3.0}).calls_caps
{}

The field keeps its name — it is what OperationDeadlineConfigProtocol requires, what
existing configuration files and model_dump() use — and now carries
validation_alias=AliasChoices("calls_caps", "call_caps"), so both spellings are accepted
on input and both land in calls_caps. Serialisation is unchanged.

I did not reach for extra="forbid": it would turn every unrelated key in a caller's
configuration into a hard error, which is a much wider change than this finding needs, and
it makes the mistake fail rather than work.

Tests: test__operation_config__with_call_caps_spelling__populates_calls_caps and
test__operation_config__dump__keeps_calls_caps_as_the_field_name. Both fail on master.

2. BudgetContext stored the caller's caps dict, so a request could rewrite the settings

ctx.call_caps handed back the very mapping the context was built from, and
DeadlineContextFactory.create_for_operation passes config.calls_caps straight through.
The settings object is APP-scoped and shared by every request, so one context could write
into it:

>>> ctx1 = factory.create_for_operation("signup")
>>> ctx1.call_caps["identity_create"] = 0.5
>>> factory.create_for_operation("signup").call_caps
{'identity_create': 0.5}
>>> settings.operations["signup"].calls_caps
{'identity_create': 0.5}

BudgetContext.__init__ now takes dict(call_caps). Mutating ctx.call_caps still changes
that context's later calls — that is documented and unchanged — but it no longer reaches the
settings, and mutating the dict you passed to create() after the fact no longer changes
the context. That second half is a behaviour change, so docs/agents.md rule 13 changed
with it.

Tests: two in tests/unit/test_context.py, one in tests/unit/contrib/test_dishka.py, all
three failing on master.

3. make test-unit failed on a fresh checkout

make install ran uv sync --group dev, which leaves the optional extras out, so the two
contrib test modules failed at collection:

ERROR tests/unit/contrib/test_dishka.py    - ModuleNotFoundError: No module named 'dishka'
ERROR tests/unit/contrib/test_settings.py  - ModuleNotFoundError: No module named 'pydantic'
!!!! Interrupted: 2 errors during collection !!!!

CI already syncs with --all-extras; make install and the CONTRIBUTING.md setup block
now do the same, with a line saying why.

4. The guides did not say which of the budget, the floor and the cap wins

docs/guide/configuration.md said timeout_for(cap=...) "returns min(cap, remaining)",
which holds only while remaining is above min_timeout. Both departures are real:

>>> DeadlineBudget(total_seconds=1.0, min_timeout=5.0).timeout_for()
5.0                     # more time than the budget has left
>>> DeadlineBudget(total_seconds=10.0, min_timeout=0.1).timeout_for(cap=0.05)
0.05                    # the cap wins over the floor

I read this as the doc drifting, not the code: the floor over the budget is what the safety
margin exists to pay for, and a service-level cap that got widened to reach a floor would be
worse than one that does not. Behaviour is unchanged; the configuration guide now carries
the two lines of arithmetic and both consequences, the quickstart carries a sentence each,
and the timeout_for / timeout_for_call docstrings no longer claim a [min_timeout, cap]
bound they do not enforce.

5. DeadlineProvider's docstring promised a BudgetContext provider

It said it "Provides DeadlineContextFactory and optionally per-request BudgetContext"; the
class has one @provide and it returns the factory. The docstring now says what it
provides, what binding it needs from you, and why a context is not provided — it belongs to
one operation and its countdown starts when it is built.

6. Nothing said the settings models read no environment

BaseDeadlineSettings is a pydantic.BaseModel, not a pydantic_settings.BaseSettings,
despite the extra being called settings; docs/guide/integrations.md never said so.
Added a paragraph there. The example in that section also used BaseModel and Field
without importing them — fixed in the same block.

7. total_seconds reports the usable budget, and the quickstart read as if it were the total

DeadlineBudget(total_seconds=10.0, safety_margin=0.5).total_seconds is 9.5, and
DeadlineExceededError.budget_seconds is the same 9.5. Correct, deliberate, and only
written down on the agents page. The quickstart's safety_margin section now says it.

Verification

make check          ruff check: All checks passed
                    ruff format --check: 19 files already formatted
                    mypy: Success: no issues found in 8 source files
make install        Resolved 55 packages, uv.lock unchanged
make test-unit      67 passed
make test           67 passed, coverage 99.22% (threshold 90%)

make test-integration exits 5 — tests/integration/ holds only an __init__.py, so
nothing is selected and pytest reports "no tests ran". That is the state on master too; I
left it alone.

 Alex Shalaev added 6 commits September 6, 2026 21:10
OperationDeadlineConfig spells the field calls_caps while BudgetContext.create takes
call_caps. Pydantic ignores unknown keys, so OperationDeadlineConfig(call_caps={...})
produced a config with no caps at all and no complaint; the deadline factory then built
every context for that operation uncapped.

The field keeps its name -- it is what OperationDeadlineConfigProtocol requires and what
existing configuration and dumps use -- and now accepts both spellings on input.
The context stored the caller's dict, so ctx.call_caps handed back the very mapping it was
built from. DeadlineContextFactory passes config.calls_caps straight through, which made
the settings object -- APP-scoped, shared by every request -- writable through any context
built from it: mutating ctx.call_caps changed the caps of every later request for that
operation.

The context now takes a copy. Mutating ctx.call_caps still changes that context's later
calls; it no longer reaches anything else.
make install ran uv sync --group dev, which leaves Pydantic and Dishka out, so make
test-unit -- the command CONTRIBUTING tells a contributor to run -- failed at collection on
tests/unit/contrib with two ModuleNotFoundError. CI already syncs with --all-extras.
The configuration guide said timeout_for(cap=...) returns min(cap, remaining), which holds
only while remaining is above min_timeout. The floor outranks the remaining budget and the
cap, applied last, outranks the floor -- neither was written down anywhere outside the
agents page. Behaviour is unchanged; the guides and the two docstrings now describe it.
The class docstring promised "DeadlineContextFactory and optionally per-request
BudgetContext"; there is one provider on it and it returns the factory.
…sable budget

Two things a reader could only find by opening the source: the models behind the settings
extra are BaseModel and not BaseSettings, and DeadlineBudget.total_seconds reports the total
minus the safety margin -- as does DeadlineExceededError.budget_seconds.
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@AlexeyShalaev
AlexeyShalaev merged commit 8cd303d into master Sep 6, 2026
6 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:45
AlexeyShalaev added a commit that referenced this pull request Sep 6, 2026
…xt (#22)

The work landed in #21. Its squash subject lost the Conventional Commit prefix --
my mistake on the merge, not the author's -- so release-please skipped the merge
and these fixes would never have reached a release. This commit carries the
record. It changes no code: #21 is already on master.

* OperationDeadlineConfig spells the field calls_caps while BudgetContext.create
  takes call_caps, and the model ignored the unknown key, so a configuration
  written the second way silently carried no caps at all. Both spellings are
  accepted now through AliasChoices, and serialisation is unchanged.
* BudgetContext held the caps dict it was handed rather than a copy. Because the
  factory passes the settings object's own dict straight through, mutating one
  request's caps rewrote the application-scoped settings and every later context
  for that operation.
* make install and the contributing guide now sync the extras CI uses, so
  make test-unit works on a fresh checkout instead of failing at collection.
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.

1 participant