src/schemas/configuration.py (1)

86-100: ⚠️ Potential issue | 🟠 Major

Validator propagates a server-config error to API clients; see src/config.py comment on effective_max_custom_instructions_tokens.

settings.DERIVER.effective_max_custom_instructions_tokens raises ValueError when MAX_CUSTOM_INSTRUCTIONS_TOKENS is None. Pydantic will wrap that into a 422 ValidationError whose detail string is the internal config-path message (set [deriver].MAX_CUSTOM_INSTRUCTIONS_TOKENS in config.toml). This is the user-facing manifestation of the root-cause concern raised in src/config.py; fixing it at the property (return int | None) and guarding here — if max_tokens is None: raise ValueError("custom_instructions are not enabled on this deployment") — would produce a much better API error.

Also note that because this validator reads the global settings lazily, it's sensitive to test ordering and import-time settings construction; the existing tests work around this with monkeypatch.setattr(settings.DERIVER, ...), which is fine but worth being aware of for future schema-construction outside request scope.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/schemas/configuration.py` around lines 86 - 100, The validator
_validate_custom_instructions_budget must not propagate the internal config
ValueError — fetch max_tokens via
settings.DERIVER.effective_max_custom_instructions_tokens, guard for None and
raise a clear API-facing ValueError like "custom_instructions are not enabled on
this deployment" (do this before calling estimate_tokens), and only perform the
token estimate and comparison if max_tokens is an int; this prevents the
internal config error from being wrapped into a 422 and ensures a deterministic,
testable behavior.

.env.template (1)

115-116: Consider mirroring the explanatory comment from config.toml.example.

Optional nit: in config.toml.example this key carries # Required for non-blank reasoning.custom_instructions; max supported value is 2000. Adding the same inline note here helps users understand that unsetting it disables non-blank custom instructions.

📝 Suggested diff

-# DERIVER_MAX_CUSTOM_INSTRUCTIONS_TOKENS=2000
+# DERIVER_MAX_CUSTOM_INSTRUCTIONS_TOKENS=2000  # Required for non-blank reasoning.custom_instructions; max supported value is 2000

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.env.template around lines 115 - 116, Add an inline explanatory comment next
to the DERIVER_MAX_INPUT_TOKENS and DERIVER_MAX_CUSTOM_INSTRUCTIONS_TOKENS
entries in .env.template mirroring the wording from config.toml.example (e.g.,
note that DERIVER_MAX_CUSTOM_INSTRUCTIONS_TOKENS is "Required for non-blank
reasoning.custom_instructions; max supported value is 2000" and that leaving it
unset disables non-blank custom instructions) so users understand the setting's
effect and max value; update the commented lines for
DERIVER_MAX_CUSTOM_INSTRUCTIONS_TOKENS and optionally DERIVER_MAX_INPUT_TOKENS
to include this guidance.

tests/test_config.py (1)

39-43: Test name implies "defaults" but it exercises an explicit override.

test_deriver_defaults_allow_larger_custom_instruction_budget reads as "with defaults, a larger budget is allowed", but the test passes MAX_CUSTOM_INSTRUCTIONS_TOKENS=2000 explicitly — the default is None. The actual assertion of interest is that MAX_INPUT_TOKENS defaults to 25000 (the other assertion is redundant with test_effective_custom_instructions_tokens_uses_explicit_limit).

♻️ Suggested rename / tightening

-def test_deriver_defaults_allow_larger_custom_instruction_budget() -> None:
-    settings = _make_deriver_settings(MAX_CUSTOM_INSTRUCTIONS_TOKENS=2000)
-
-    assert settings.MAX_INPUT_TOKENS == 25000
-    assert settings.effective_max_custom_instructions_tokens == 2000
+def test_deriver_default_max_input_tokens_is_25000() -> None:
+    settings = _make_deriver_settings()
+
+    assert settings.MAX_INPUT_TOKENS == 25000

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_config.py` around lines 39 - 43, Rename or tighten the test so its
name matches what it asserts: either remove the explicit override and call
_make_deriver_settings() with default (no MAX_CUSTOM_INSTRUCTIONS_TOKENS) to
verify settings.MAX_INPUT_TOKENS == 25000, or rename the test to reflect that it
verifies an explicit override (e.g.,
test_deriver_explicit_custom_instruction_budget_allows_larger_value) and remove
the redundant assertion on settings.effective_max_custom_instructions_tokens
since test_effective_custom_instructions_tokens_uses_explicit_limit already
covers that; update references to _make_deriver_settings,
settings.MAX_INPUT_TOKENS and settings.effective_max_custom_instructions_tokens
accordingly.

tests/utils/test_config_helpers.py (2)

26-28: raising=False is unnecessary here and weakens the guard.

MAX_CUSTOM_INSTRUCTIONS_TOKENS is a declared field on DeriverSettings, so the attribute will always exist on settings.DERIVER. Passing raising=False hides a would-be typo (e.g., renaming the field) that these tests would otherwise catch. Consider dropping it (same on Line 50).

♻️ Proposed change

-    monkeypatch.setattr(
-        settings.DERIVER, "MAX_CUSTOM_INSTRUCTIONS_TOKENS", 100, raising=False
-    )
+    monkeypatch.setattr(settings.DERIVER, "MAX_CUSTOM_INSTRUCTIONS_TOKENS", 100)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/utils/test_config_helpers.py` around lines 26 - 28, Remove the
unnecessary raising=False on the monkeypatch.setattr calls that modify
settings.DERIVER attributes: ensure monkeypatch.setattr(settings.DERIVER,
"MAX_CUSTOM_INSTRUCTIONS_TOKENS", 100) (and the analogous call around line 50)
are called without raising=False so attribute existence is enforced; update both
occurrences to omit the raising parameter to let tests fail if the
DeriverSettings field is renamed or missing.

---

23-78: Coverage gap: no negative test for message-level custom_instructions exceeding the cap.

Both tests set MAX_CUSTOM_INSTRUCTIONS_TOKENS=100 and use short strings that easily fit. Since MessageConfiguration → ReasoningConfiguration.validate_custom_instructions is the user-facing enforcement point for this PR, it would be valuable to assert that get_configuration (or MessageConfiguration construction) raises when the resolved/message-level custom_instructions blows the budget, and that it does not raise when the workspace-level string is within budget. The PR summary says tests/test_schema_validations.py covers schema-level rejection, so this is an optional belt-and-suspenders addition at the get_configuration layer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/utils/test_config_helpers.py` around lines 23 - 78, Add a negative test
that monkeypatches settings.DERIVER.MAX_CUSTOM_INSTRUCTIONS_TOKENS to a small
value, then constructs a MessageConfiguration whose
reasoning.custom_instructions exceeds that cap and assert that get_configuration
(or MessageConfiguration construction) raises via
ReasoningConfiguration.validate_custom_instructions; also include a control
asserting that a workspace-level custom_instructions string that is within the
same cap does not raise when passed to get_configuration. Reference
MAX_CUSTOM_INSTRUCTIONS_TOKENS, MessageConfiguration,
ReasoningConfiguration.validate_custom_instructions, and get_configuration to
locate the relevant code paths.

src/schemas/configuration.py (2)

153-160: Duplicated validator on ResolvedReasoningConfiguration is redundant given upstream enforcement.

ResolvedReasoningConfiguration is only ever constructed from dicts that were themselves merged from already-validated ReasoningConfiguration instances (see src/utils/config_helpers.py:get_configuration). Since _validate_custom_instructions_budget is deterministic over the string, running it again on the resolved model just repeats the same estimate_tokens call on the deriver hot path. Not a correctness issue — I'd keep it only if you specifically want belt-and-suspenders against ResolvedConfiguration(**dict) being constructed from untrusted input elsewhere.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/schemas/configuration.py` around lines 153 - 160, The
ResolvedReasoningConfiguration class currently re-runs
_validate_custom_instructions_budget via the field validator method
validate_custom_instructions, which is redundant because
ResolvedReasoningConfiguration instances are constructed from already-validated
ReasoningConfiguration dicts (see get_configuration). Remove the
`@field_validator`("custom_instructions") method validate_custom_instructions from
ResolvedReasoningConfiguration so the model only defines enabled and
custom_instructions without re-invoking _validate_custom_instructions_budget;
keep the helper _validate_custom_instructions_budget and the original validator
on ReasoningConfiguration unchanged.

---

91-92: Whitespace-only custom_instructions is returned verbatim — consider normalizing to None.

When custom_instructions = " ", the validator returns the original whitespace string untouched. Downstream, _normalized_custom_instructions in src/deriver/prompts.py correctly collapses that to no section, so there's no user-visible bug today. However, persisting a whitespace-only string in the workspace/session/message configuration is surprising and will fail any future exact-match comparisons to "no custom instructions set". Consider normalizing blank/whitespace to None at validation time for consistency.

♻️ Proposed change

-    if custom_instructions is None:
-        return None
-    if not custom_instructions.strip():
-        return custom_instructions
+    if custom_instructions is None or not custom_instructions.strip():
+        return None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/schemas/configuration.py` around lines 91 - 92, The validator currently
returns the original whitespace-only string when it hits the check "if not
custom_instructions.strip(): return custom_instructions"; change this to return
None instead so blank/whitespace-only custom_instructions are normalized to None
at validation time (update the branch in the validator that inspects
custom_instructions.strip()). Ensure callers that expect None (e.g.,
_normalized_custom_instructions in src/deriver/prompts.py) continue to work
unchanged.

src/deriver/prompts.py (1)

83-105: Token-estimation strategy is consistent with the existing pattern.

Using empty peer_id/messages and letting the caller add those tokens separately matches how estimate_base_prompt_tokens() is intended to be used, and including custom_instructions in the estimate here is correct because the caller cannot substitute it as a simple string concatenation (it adds the CUSTOM INSTRUCTIONS: header).

One small concern: estimate_minimal_deriver_prompt_tokens() is @cached, but estimate_deriver_prompt_tokens() re-renders the full prompt and re-tokenizes on every call when custom_instructions is non-empty. For the deriver hot path (called per message) this is likely fine, but if you want, the token count for a given custom_instructions string could be cached too.

Based on learnings: "the approach uses empty inputs to get the base prompt template token count via estimate_base_prompt_tokens(), then manually adds tokens for actual inputs (peer_card, working_representation, new_turns). This avoids double-counting wrapper overhead since the base prompt already includes template structure."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/deriver/prompts.py` around lines 83 - 105, The per-call tokenization for
non-empty custom instructions in estimate_deriver_prompt_tokens causes
unnecessary work; cache the token count per normalized custom instructions
string to avoid re-rendering/minimizing heat on the deriver hot path: normalize
via _normalized_custom_instructions(custom_instructions) as you already do, then
compute the prompt and estimate_tokens using minimal_deriver_prompt and
estimate_tokens for that normalized string once and memoize it (e.g., add a
small cached helper or apply `@cache` to a helper like
_estimate_deriver_prompt_tokens_cached(normalized_custom_instructions) that
calls minimal_deriver_prompt and estimate_tokens), while keeping the current
fast-return to estimate_minimal_deriver_prompt_tokens() when
normalized_custom_instructions is None.

src/config.py (1)

771-778: Cross-field check is effectively unreachable given field bounds.

MAX_CUSTOM_INSTRUCTIONS_TOKENS is bounded by le=2000 and MAX_INPUT_TOKENS is bounded by ge of REPRESENTATION_BATCH_MAX_TOKENS (ge=128) and below by gt=0. So this validator can only fire when MAX_INPUT_TOKENS is set to something in the narrow range where it's below MAX_CUSTOM_INSTRUCTIONS_TOKENS (≤2000), which is almost certainly a broken deployment for unrelated reasons. Keeping the check is fine (it's defensive), but the error message could tell the operator the real action — usually "raise MAX_INPUT_TOKENS" — rather than implying the custom-instructions budget is at fault.

Nit only; happy to defer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config.py` around lines 771 - 778, The cross-field check in the validator
block referencing MAX_CUSTOM_INSTRUCTIONS_TOKENS and MAX_INPUT_TOKENS should
keep the guard but change the ValueError to direct operators to increase
MAX_INPUT_TOKENS (since MAX_CUSTOM_INSTRUCTIONS_TOKENS is tightly bound by
le=2000 and MAX_INPUT_TOKENS has lower bounds like
REPRESENTATION_BATCH_MAX_TOKENS); update the error raised in that block to
include both current values and a clear actionable suggestion such as "increase
MAX_INPUT_TOKENS to at least X" or "raise MAX_INPUT_TOKENS" while still
reporting the offending values (reference the variables
MAX_CUSTOM_INSTRUCTIONS_TOKENS, MAX_INPUT_TOKENS, and
REPRESENTATION_BATCH_MAX_TOKENS to locate the code).

Fix all unresolved CodeRabbit comments on this PR:

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

tests/test_schema_validations.py (3)

230-245: Tighten the oversized-instructions test.

This test only asserts that some error is raised at loc == ("custom_instructions",), but Pydantic would also produce an error at that location for unrelated validation failures (e.g., a type error in the future). Consider also asserting that the message contains the expected "exceeds" text to distinguish it from the "not enabled" path covered by the previous test — otherwise both tests could pass even if the validator collapsed both branches into one.

♻️ Proposed tightening

         assert any(
             error["loc"] == ("custom_instructions",)
+            and "exceeds" in error["msg"]
             for error in exc_info.value.errors()
         )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_schema_validations.py` around lines 230 - 245, The test
test_reasoning_configuration_rejects_oversized_custom_instructions should not
only check the error location but also assert the error message indicates the
size limit was exceeded; update the assertion that iterates
exc_info.value.errors() to require both error["loc"] == ("custom_instructions",)
and that the error["msg"] contains the expected substring (e.g., "exceeds" or
"exceeds maximum") so the test specifically verifies the oversized-instructions
branch of ReasoningConfiguration validation rather than any unrelated validation
error.

---

214-216: Drop raising=False on the monkeypatch calls.

MAX_CUSTOM_INSTRUCTIONS_TOKENS is a declared field on DeriverSettings (see src/config.py:738), so the attribute always exists. Using raising=False silently swallows AttributeError, which would mask a typo in the attribute name and turn a real test failure into a passing no-op. Prefer the default (raising=True) so the tests fail loudly if the setting is ever renamed.

♻️ Proposed change

-        monkeypatch.setattr(
-            settings.DERIVER, "MAX_CUSTOM_INSTRUCTIONS_TOKENS", None, raising=False
-        )
+        monkeypatch.setattr(
+            settings.DERIVER, "MAX_CUSTOM_INSTRUCTIONS_TOKENS", None
+        )

(apply the equivalent change at the three other call sites)

Also applies to: 233-235, 250-252, 280-282

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_schema_validations.py` around lines 214 - 216, Remove
raising=False from the monkeypatch.setattr calls that target
settings.DERIVER.MAX_CUSTOM_INSTRUCTIONS_TOKENS in
tests/test_schema_validations.py so that monkeypatch will raise on missing
attributes; specifically, update the monkeypatch.setattr invocations referencing
settings.DERIVER and the attribute MAX_CUSTOM_INSTRUCTIONS_TOKENS (and the three
equivalent call sites in the same file) to use the default raising=True by
omitting the raising parameter.

---

221-228: Weak regression guard on line 228.

The "not in" check targets the literal "[deriver].MAX_CUSTOM_INSTRUCTIONS_TOKENS", but the current validator in src/schemas/configuration.py never emits that exact bracketed-prefix format in any branch — the "not enabled" branch doesn't reference the setting at all, and the "exceeds" branch uses DERIVER.MAX_CUSTOM_INSTRUCTIONS_TOKENS (no brackets). As written, this assertion will pass regardless of what leaks into the message, so it doesn't actually guard against settings-key leakage.

Consider either removing it or broadening it to catch any form of the settings key (e.g., assert that "MAX_CUSTOM_INSTRUCTIONS_TOKENS" does not appear in the "not enabled" message).

♻️ Proposed change

-        assert "[deriver].MAX_CUSTOM_INSTRUCTIONS_TOKENS" not in str(exc_info.value)
+        assert "MAX_CUSTOM_INSTRUCTIONS_TOKENS" not in str(exc_info.value)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_schema_validations.py` around lines 221 - 228, The regression
guard is too specific and will always pass because the validator never emits the
bracketed key "[deriver].MAX_CUSTOM_INSTRUCTIONS_TOKENS"; update the test in
tests/test_schema_validations.py to assert against the broader token name
instead of the bracketed form (e.g., assert "MAX_CUSTOM_INSTRUCTIONS_TOKENS" not
in str(exc_info.value)) or remove the assertion entirely; target the same
objects used in the test (errors and exc_info.value) so the "not enabled" branch
in src/schemas/configuration.py is validated to not leak the settings key in any
form.

tests/utils/test_config_helpers.py (1)

23-74: Consider adding a session-over-workspace precedence test.

The two tests cover "workspace only" and "message overrides all" cases, but not the intermediate "session overrides workspace" case (with no message configuration). Given that get_configuration applies overrides in a specific Workspace → Session → Message order in src/utils/config_helpers.py, a dedicated test for session-level override would round out the precedence matrix and guard against regressions in the middle tier.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/utils/test_config_helpers.py` around lines 23 - 74, Add a new unit test
that asserts session-level reasoning.custom_instructions overrides
workspace-level when no MessageConfiguration is provided: set monkeypatch for
settings.DERIVER.MAX_CUSTOM_INSTRUCTIONS_TOKENS (as in other tests), create a
workspace via _workspace(...) with reasoning.custom_instructions = "Use the
workspace-specific guidance.", create a session via _session(...) with
reasoning.custom_instructions = "Use the session-specific guidance.", call
get_configuration(None, session, workspace) and assert
configuration.reasoning.custom_instructions == "Use the session-specific
guidance."; reference get_configuration, _workspace, _session, and
ReasoningConfiguration to locate where to add the test.

tests/test_config.py (1)

6-20: ⚡ Quick win

Use snake_case parameter names in test helper.

Helper parameters should be snake_case to match project Python conventions.

Proposed change

 def _make_deriver_settings(
     *,
-    MAX_INPUT_TOKENS: int = 25000,
-    MAX_CUSTOM_INSTRUCTIONS_TOKENS: int = 2000,
-    REPRESENTATION_BATCH_MAX_TOKENS: int = 1024,
+    max_input_tokens: int = 25000,
+    max_custom_instructions_tokens: int = 2000,
+    representation_batch_max_tokens: int = 1024,
 ) -> DeriverSettings:
     return DeriverSettings(
         MODEL_CONFIG=ConfiguredModelSettings(
             model="gpt-5.4-mini",
             transport="openai",
         ),
-        MAX_INPUT_TOKENS=MAX_INPUT_TOKENS,
-        MAX_CUSTOM_INSTRUCTIONS_TOKENS=MAX_CUSTOM_INSTRUCTIONS_TOKENS,
-        REPRESENTATION_BATCH_MAX_TOKENS=REPRESENTATION_BATCH_MAX_TOKENS,
+        MAX_INPUT_TOKENS=max_input_tokens,
+        MAX_CUSTOM_INSTRUCTIONS_TOKENS=max_custom_instructions_tokens,
+        REPRESENTATION_BATCH_MAX_TOKENS=representation_batch_max_tokens,
     )

As per coding guidelines: "**/*.py: Use snake_case for variables and functions; PascalCase for classes".

🤖 Prompt for 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.

In `@tests/test_config.py` around lines 6 - 20, The helper _make_deriver_settings
currently uses Pascal/SCREAMING_STYLE parameter names; rename its parameters to
snake_case (e.g., max_input_tokens, max_custom_instructions_tokens,
representation_batch_max_tokens) and update the function body to pass those
snake_case variables into DeriverSettings (mapping to MAX_INPUT_TOKENS,
MAX_CUSTOM_INSTRUCTIONS_TOKENS, REPRESENTATION_BATCH_MAX_TOKENS) so callers and
the test follow project Python naming conventions; also update any test calls to
use the new snake_case argument names.