feat: add replay-safe playbook optimization foundations - #385
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces durable GEPA user-playbook publication with canonical proofs, projections, leases, recovery, SQLite staging, atomic successor commits, and frozen judge execution plans. It also updates configuration handling, aggregation coordination, governance erasure hooks, CLI resilience, client response models, and E2E coverage. ChangesOptimizer publication and lifecycle
Configuration, client, and compatibility updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/playbook/components/aggregator.py (1)
502-534: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
effect_scopeis assigned before__enter__()succeeds — an entry failure triggers an invalid__exit__()call.
effect_scope = self.effect_coordinator.apply_scope()assigns the context manager, theneffect_scope.__enter__()is called separately. If__enter__()itself raises,effect_scopeis already non-None, so the exception handler at Line 817 will callfailed_scope.__exit__(type(e), e, e.__traceback__)on a context manager that was never successfully entered — undefined/masking behavior for@contextmanager-based implementations (its body never ran, so there's nothing valid to roll back, and calling__exit__on an unentered generator-based CM can itself raise, hiding the original error).🛠️ Proposed fix: only assign after successful entry
if self.effect_coordinator is not None: self.effect_coordinator.prepare(new_playbooks) - effect_scope = self.effect_coordinator.apply_scope() - effect_scope.__enter__() + scope_cm = self.effect_coordinator.apply_scope() + scope_cm.__enter__() + effect_scope = scope_cmAlso applies to: 816-820
🤖 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 `@reflexio/server/services/playbook/components/aggregator.py` around lines 502 - 534, Update the effect-scope entry flow in the aggregation method so effect_scope remains None until apply_scope().__enter__() completes successfully, while preserving the existing cleanup behavior in the exception handler. Use a temporary context-manager variable for the unentered scope, then assign effect_scope only after successful entry so failed entry does not trigger __exit__.
🧹 Nitpick comments (16)
tests/server/services/playbook/test_publication_models.py (1)
352-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFirst two cases assert Python arity, not the verifier contract.
UserPlaybookPublicationService(object())raisesTypeErrorfor the missing requiredverifierargument, so the barepytest.raises(TypeError)passes without ever reaching thePublicationDecisionVerifiercheck. Theverifier=Nonecase is the meaningful one; consider dropping the first case or asserting the message.🤖 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/server/services/playbook/test_publication_models.py` around lines 352 - 359, Remove the first UserPlaybookPublicationService(object()) assertion from test_publication_service_requires_explicit_verifier, since it only verifies Python’s missing-argument error; retain the verifier=None case with its PublicationDecisionVerifier message assertion.tests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py (3)
960-967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the expected exception type.
pytest.raises(Exception, match="worker fence")will also pass on unrelated failures (e.g. anAssertionErroror aTypeErrorwhose message happens to match). Every sibling test asserts a concrete type; useStorageErrorhere too so a regression in fence enforcement can't be masked.♻️ Suggested change
- pytest.raises(Exception, match="worker fence"), + pytest.raises(StorageError, match="worker fence"),🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py` around lines 960 - 967, Update the pytest.raises assertion around claim_user_playbook_publication in the claim_then_stale test to expect StorageError instead of the broad Exception type, while preserving the existing “worker fence” message match.
1394-1402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour byte-identical
prepare_then_crashhelpers.The same "prepare, expire lease, raise RuntimeError" injection is duplicated verbatim across these tests. Extract one module-level helper (e.g.
_prepare_then_expire_lease(storage, original_prepare)) and reuse it to keep the crash semantics in a single place.Also applies to: 1488-1495, 1544-1551, 1591-1598
🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py` around lines 1394 - 1402, Extract the duplicated prepare-then-crash logic from the four test-local prepare_then_crash helpers into one module-level helper, such as _prepare_then_expire_lease(storage, original_prepare). Update each affected test to reuse it while preserving job_id recording, lease expiration, commit behavior, and the RuntimeError crash semantics.
1680-1695: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource-text greps are fragile fitness tests.
Both tests assert on raw source text, so they break on unrelated renames/reformatting and pass if the forbidden call is reintroduced via an alias or attribute access (e.g.
getattr(storage, "save_user_playbooks")). Prefer asserting on imported symbols/AST (e.g.assert not hasattr(PlaybookOptimizer, "_supersede_user_playbook")) or, for the constants check, importing the constants and asserting identity with the shared module.Also applies to: 1791-1798
🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py` around lines 1680 - 1695, Replace the raw source-text assertions in both affected tests with behavioral or symbol-based checks: assert the forbidden PlaybookOptimizer method is absent via the imported class, and import the publication metadata constants from their shared module to verify identity with the symbols used by SQLite optimization. Avoid grep-based checks so aliases, attribute access, formatting, and unrelated renames cannot bypass or break the tests.tests/server/services/playbook_optimizer/test_playbook_optimizer.py (1)
786-795: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTarget the publication-authority digest helper instead of
Path.read_bytesglobally.
PatchingPath.read_bytesprocess-wide makes this test brittle: any other file read duringoptimize()will fail for reasons unrelated to publication-authority hashing. Narrowing the patch to the hashing call site keeps the assertion focused.🤖 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/server/services/playbook_optimizer/test_playbook_optimizer.py` around lines 786 - 795, In the test around optimizer.optimize for the agent_playbook target, replace the process-wide Path.read_bytes patch with a patch targeting the publication-authority digest helper or its hashing call site. Keep the permission failure assertion focused on preventing publication-authority hashing from reading the agent backend, while allowing unrelated file reads during optimize() to proceed.reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py (1)
348-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDurable metadata is serialized with
json.dumps, notcanonical_json_bytes.Every other publication payload in this cohort goes through
canonical_json_bytes(RFC 8785). Here the durable metadata uses a hand-rolledsort_keys/compact form. It happens to round-trip because readers compare parsed values, but drifting from the single canonicalizer in a proof-bearing path is a maintenance trap. (The static-analysisuse-jsonifyhint on these lines is a false positive — this is DB persistence, not an HTTP response.)🤖 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 `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py` around lines 348 - 353, Update the durable metadata serialization in the surrounding optimization publication flow to use the existing canonical_json_bytes canonicalizer instead of json.dumps with sort_keys and compact separators. Preserve the resulting persisted metadata representation and remove only the hand-rolled serialization options; do not apply the HTTP-oriented use-jsonify change.Source: Linters/SAST tools
reflexio/server/services/playbook_optimizer/optimizer.py (1)
718-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed publish failure.
When
publishraises but a committed result exists, the original exception is discarded with no trace — the idempotent-replay path becomes indistinguishable from a clean publish in logs.♻️ Keep the failure signal
try: publication_result = service.publish(request) except Exception: committed = service.load_committed(request.job_id) if committed is None: raise + logger.warning( + "GEPA publication raised but a committed result exists; treating as replay job_id=%d", + request.job_id, + exc_info=True, + ) publication_result = committed🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 718 - 724, Update the exception handler around service.publish in the optimizer flow to log the caught publish failure before falling back to the committed result. Preserve the existing re-raise when service.load_committed returns None and the idempotent replay behavior when a committed result exists.reflexio/server/services/playbook_optimizer/judge.py (1)
340-345: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnexpected provider params surface as a transport failure instead of frozen-plan drift.
sanitize_pairwise_judge_provider_paramsraisesValueErroron any key outside_PROVIDER_PARAM_KEYS(Line 92-96). Raised here — inside_call_and_parse's try block — it is not aProviderRequestGuardError, so_litellm_text_generation.pywraps it asLiteLLMClientError(Line 1135) and the owned ladder walk advances rung by rung, logging transport-style errors. The provider is still never called, so the fail-closed property holds, but a genuine param-surface drift is reported as an API failure rather thanFrozenEvaluatorPlanDriftError.♻️ Convert param-surface drift into a guard error
- provider_params = sanitize_pairwise_judge_provider_params(params) + try: + provider_params = sanitize_pairwise_judge_provider_params(params) + except ValueError as exc: + raise FrozenEvaluatorPlanDriftError( + "PairwiseJudge provider param surface drifted after job creation" + ) from exc🤖 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 `@reflexio/server/services/playbook_optimizer/judge.py` around lines 340 - 345, Convert the ValueError raised by sanitize_pairwise_judge_provider_params within _call_and_parse into ProviderRequestGuardError before it reaches the transport wrapper, preserving the original validation details. Ensure unexpected provider-parameter keys fail as FrozenEvaluatorPlanDriftError through the existing guard-error path, without invoking the provider or treating the failure as a transport error.reflexio/server/services/playbook_optimizer/gepa_publication.py (1)
32-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the projector code digest or add a source-digest guard
GEPA_PROJECTOR_CODE_DIGESTis a pinned literal, and nothing checks it against the currentbuild_gepa_search_projectionsource. That lets the published projector identity drift from the code that produced it while the projection/proof still round-trips. Derive it from the projector source, or add a test/guard that compares the literal to the current source digest.🤖 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 `@reflexio/server/services/playbook_optimizer/gepa_publication.py` around lines 32 - 38, Add a guard for GEPA_PROJECTOR_CODE_DIGEST tied to build_gepa_search_projection, ensuring the pinned digest is compared with the current projector source and publication fails or tests fail on mismatch. Keep the existing projector identity and schema constants unchanged.tests/server/services/playbook_optimizer/test_judge_frozen_plan.py (1)
105-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding the judge prompt version
Use a version fromPromptManager.list_versions(PLAYBOOK_OPTIMIZER_JUDGE_PROMPT_ID)(or the active version) instead of"1.1.0"; this test only needs an existing prompt version, and the fixture will rot if the prompt bank changes.🤖 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/server/services/playbook_optimizer/test_judge_frozen_plan.py` around lines 105 - 110, Update the test setup around prompt_manager.version_override to select an existing version from PromptManager.list_versions(PLAYBOOK_OPTIMIZER_JUDGE_PROMPT_ID), or reuse the active version, instead of hardcoding "1.1.0". Preserve the FrozenEvaluatorPlanDriftError assertion and provider.assert_not_called() behavior.reflexio/server/services/extractor_interaction_utils.py (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
objecterases the little type information these helpers had.Both functions only need
window_size_override/stride_size_override/request_sources_enabledattributes. A smallProtocolwould keep call-site checking (and let Pyright verify thetuple[int, int]return) instead of forcinggetattron an opaqueobject.Also applies to: 61-62
🤖 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 `@reflexio/server/services/extractor_interaction_utils.py` around lines 17 - 18, Define a small typing Protocol for the extractor configuration attributes window_size_override, stride_size_override, and request_sources_enabled, then use it instead of object in get_extractor_window_params and the other helper around the referenced lines. Access these declared attributes directly rather than via getattr, preserving the existing tuple[int, int] return behavior and enabling static type checking.reflexio/server/services/storage/storage_base/playbook/_user.py (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffStorage interface now depends upward on a service module.
storage_baseis the lowest contract layer, yetPublicationClaim/PublicationRequest/PublicationResultlive inreflexio/server/services/playbook/publication.py. TheTYPE_CHECKINGguard hides it at runtime but the dependency direction is inverted, and SQLite already imports the same module at runtime. Consider relocating the publication contract dataclasses to a shared contracts/models module so both storage and the optimizer service depend downward on it.🤖 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 `@reflexio/server/services/storage/storage_base/playbook/_user.py` around lines 11 - 17, Move the PublicationClaim, PublicationRequest, and PublicationResult contract dataclasses out of services.playbook.publication into a shared lower-level contracts/models module, then update _user.py, SQLite storage imports, and the optimizer/publication service to import them from that shared module. Remove the upward storage_base dependency while preserving the existing public behavior and type interfaces.reflexio/models/api_schema/domain/entities.py (1)
140-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTwo different "canonical JSON" definitions now coexist for optimizer payloads.
canonicalize_artifact_jsonusesjson.dumps(sort_keys=True)(byte-order key sorting, Python number repr), whilereflexio/server/services/playbook/publication.py:49-85implements RFC 8785 (canonical_json_bytes, UTF-16BE key ordering, integer-only numbers). A digest produced under one scheme will not reproduce under the other for payloads with non-ASCII keys or floats. Since artifacts and publication proofs are part of the same replay/tamper-evidence story, consider having artifacts reusecanonical_json_bytes(or document explicitly that artifact digests are scheme-B and never cross-verified).Also note
json.JSONDecodeErrorin theexcepttuple is already covered byValueError.🤖 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 `@reflexio/models/api_schema/domain/entities.py` around lines 140 - 157, The artifact canonicalization path should use the existing RFC 8785 implementation, canonical_json_bytes, rather than json.dumps with Python-specific sorting and number formatting. Update canonicalize_artifact_json to reuse that shared canonicalization scheme and preserve its valid-JSON string contract, while removing the redundant json.JSONDecodeError entry from the ValueError-based exception handling.reflexio/server/services/playbook/publication.py (1)
413-430: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
__all__omits public names that other modules already import, and Ruff wants it sorted.
PublishableOptimizerKind,PublicationSource,publication_source_for_optimizer,PUBLICATION_PROOF_JSON_METADATA_KEY, andPUBLICATION_PROJECTION_JSON_METADATA_KEYare consumed outside this module (e.g.reflexio/server/services/playbook_optimizer/gepa_publication.py,tests/server/services/storage/test_user_playbook_publication_sqlite.py) but are not declared here, while the entry order also trips RUF022.🤖 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 `@reflexio/server/services/playbook/publication.py` around lines 413 - 430, Update the module’s __all__ to include PublishableOptimizerKind, PublicationSource, publication_source_for_optimizer, PUBLICATION_PROOF_JSON_METADATA_KEY, and PUBLICATION_PROJECTION_JSON_METADATA_KEY, then sort all exported names according to Ruff’s RUF022 requirement.Source: Linters/SAST tools
reflexio/server/services/storage/retention.py (1)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the consumer or narrow the types. No consumer for
OPTIMIZATION_RETENTION_CLASSESappears in the repository, so this reads as declaration-only. If it’s meant for an enterprise backend, add a short note here; otherwiseartifact_classandownershould beLiterals instead ofstr.🤖 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 `@reflexio/server/services/storage/retention.py` around lines 25 - 38, Update OptimizationRetentionClass to use Literal types for artifact_class and owner, restricting them to the declared values in OPTIMIZATION_RETENTION_CLASSES. Add the necessary typing import and preserve the existing tuple entries and dataclass structure.reflexio/lib/_base.py (1)
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: dedupe model-resolution boilerplate with
_get_query_reformulator.
create_generation_litellm_clientand_get_query_reformulator(lines 154-168, unchanged) both repeat the same site-var → config →resolve_model_namesequence for differentModelRoles. Since this PR already touches the pattern, a small shared helper (e.g._resolve_role_model_name(request_context, role, site_var_key)) would avoid future drift between the two call sites.🤖 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 `@reflexio/lib/_base.py` around lines 31 - 47, Optionally extract the repeated site-var, configuration, and resolve_model_name sequence from create_generation_litellm_client and _get_query_reformulator into a shared helper such as _resolve_role_model_name(request_context, role, site_var_key). Update both callers to use the helper while preserving their distinct ModelRole values and site-variable keys.
🤖 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 `@reflexio/models/api_schema/domain/entities.py`:
- Around line 469-490: Bind PlaybookOptimizationArtifact.content_digest to the
canonical content_json by computing or validating its SHA-256 digest after
canonicalization. Update the model validators or
upsert_playbook_optimization_artifact() so mismatched digests cannot be stored,
while preserving the existing lowercase 64-character format validation and
ensuring _row_to_playbook_optimization_artifact() receives consistent pairs.
In `@reflexio/server/services/governance/service.py`:
- Around line 164-170: Guard the subject_erasure_lifecycle.erase_subject call
with persistent purge-completion state, matching purge_targets_prepared and
_delete_targets_complete, so retries do not re-invoke the hook; document the
idempotency requirement in SubjectErasureLifecycle if the hook remains
retryable. Recompute deleted_counts after the hook completes so ERASE audit
details and UserEraseResult include its deletions.
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 181-194: In the user_playbook path of the optimizer setup,
validate the prompt_manager obtained from self.request_context before passing it
to build_pairwise_judge_request_plan. If it is missing, raise a clear explicit
error immediately, keeping prompt_manager as None only for non-user targets and
preserving the existing _run_gepa flow.
- Around line 130-136: Move the _config() and _enabled_for_target(config,
target) check ahead of _recover_gepa_user_playbook_publication() in the
optimizer flow, or otherwise gate recovery with the same configuration checks.
Ensure disabled optimizer or user-playbook optimization returns "skipped" before
any recovery or publication storage work occurs.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1417-1528: Update _enforce_playbook_optimization_job_constraints
to disable SQLite foreign-key enforcement before the table rebuild, preserving
the prior PRAGMA state if necessary. After recreating playbook_optimization_jobs
and its indexes, run PRAGMA foreign_key_check and restore the original
foreign-key setting, ensuring this cleanup also occurs when the rebuild fails.
- Around line 1389-1399: Update the migration flow in migrate() so legacy
pending/running duplicate jobs are retired or deduplicated before the
optimizer_kind classification UPDATE on playbook_optimization_jobs. Ensure this
ordering, or an equivalent combined statement, preserves only one active row per
target key before unique constraints uq_poj_active_target and the related
discovery/attempt indexes are enforced, allowing migration to complete.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py`:
- Around line 691-742: In advance_playbook_optimization_stage, restrict the
terminal applied finalization branch to jobs whose current stage is publishing
by using stage = 'publishing' in its update condition. Preserve the existing
broader live-stage handling for failed and abstained finalization, and ensure
the initial non-terminal transition still follows the existing predecessor
validation.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Around line 651-655: Guard the `user_playbooks_vec` insert in the playbook
save flow so it executes only when `request.projection.embedding` is non-empty,
matching the existing `save_user_playbooks` behavior. Keep the `_has_sqlite_vec`
check and insertion unchanged for valid embeddings, while allowing empty
embeddings to complete the atomic commit without inserting into the vector
table.
In `@reflexio/server/services/storage/storage_base/playbook/_optimization.py`:
- Around line 32-160: Prevent the new replay and publication APIs from making
existing storage backends uninstantiable: in
reflexio/server/services/storage/storage_base/playbook/_optimization.py lines
32-160, verify every OptimizationJobStoreMixin implementation supplies all 12
job, lease, stage, and artifact methods or move those primitives behind
default-raising capability hooks; in
reflexio/server/services/storage/storage_base/playbook/_user.py lines 22-53,
verify every UserPlaybookStoreMixin implementation supplies all 5
publication-lifecycle methods or apply the same capability-gated/default-raising
approach. Preserve SQLite support while allowing existing non-SQLite backends to
instantiate without implementing unused functionality.
In `@tests/e2e_tests/conftest.py`:
- Around line 361-366: Update _cleanup_storage so the drain_tagging result or
AssertionError is captured without escaping before the existing cleanup
try/except, allowing all storage deletion calls to run regardless of drain
failure; after cleanup completes, surface the original drain failure while
preserving cleanup error handling.
In `@tests/server/services/playbook_optimizer/test_judge_frozen_plan.py`:
- Around line 116-117: Silence Ruff S105 for the fixture credential assigned to
secret in the frozen-plan test, using the repository’s established per-file or
inline suppression convention. Keep the fixture value and test behavior
unchanged, and ensure the suppression applies only to this intentional test
credential.
In `@tests/server/services/storage/test_user_playbook_publication_sqlite.py`:
- Around line 1013-1014: Update the pytest.raises assertion around
service.publish(old_request) to match the specific stale-worker rejection
reason, rather than alternation across unrelated messages; use a raw regex
string to satisfy RUF043 while preserving the expected StorageError assertion.
- Line 645: Replace the ineffective incumbent.status assertion in the
failed-claim test with an assertion that reads and checks the persisted
incumbent row, verifying its status remains unchanged after the failed claim;
use the existing storage/query helper rather than the unmodified in-memory
model.
---
Outside diff comments:
In `@reflexio/server/services/playbook/components/aggregator.py`:
- Around line 502-534: Update the effect-scope entry flow in the aggregation
method so effect_scope remains None until apply_scope().__enter__() completes
successfully, while preserving the existing cleanup behavior in the exception
handler. Use a temporary context-manager variable for the unentered scope, then
assign effect_scope only after successful entry so failed entry does not trigger
__exit__.
---
Nitpick comments:
In `@reflexio/lib/_base.py`:
- Around line 31-47: Optionally extract the repeated site-var, configuration,
and resolve_model_name sequence from create_generation_litellm_client and
_get_query_reformulator into a shared helper such as
_resolve_role_model_name(request_context, role, site_var_key). Update both
callers to use the helper while preserving their distinct ModelRole values and
site-variable keys.
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 140-157: The artifact canonicalization path should use the
existing RFC 8785 implementation, canonical_json_bytes, rather than json.dumps
with Python-specific sorting and number formatting. Update
canonicalize_artifact_json to reuse that shared canonicalization scheme and
preserve its valid-JSON string contract, while removing the redundant
json.JSONDecodeError entry from the ValueError-based exception handling.
In `@reflexio/server/services/extractor_interaction_utils.py`:
- Around line 17-18: Define a small typing Protocol for the extractor
configuration attributes window_size_override, stride_size_override, and
request_sources_enabled, then use it instead of object in
get_extractor_window_params and the other helper around the referenced lines.
Access these declared attributes directly rather than via getattr, preserving
the existing tuple[int, int] return behavior and enabling static type checking.
In `@reflexio/server/services/playbook_optimizer/gepa_publication.py`:
- Around line 32-38: Add a guard for GEPA_PROJECTOR_CODE_DIGEST tied to
build_gepa_search_projection, ensuring the pinned digest is compared with the
current projector source and publication fails or tests fail on mismatch. Keep
the existing projector identity and schema constants unchanged.
In `@reflexio/server/services/playbook_optimizer/judge.py`:
- Around line 340-345: Convert the ValueError raised by
sanitize_pairwise_judge_provider_params within _call_and_parse into
ProviderRequestGuardError before it reaches the transport wrapper, preserving
the original validation details. Ensure unexpected provider-parameter keys fail
as FrozenEvaluatorPlanDriftError through the existing guard-error path, without
invoking the provider or treating the failure as a transport error.
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 718-724: Update the exception handler around service.publish in
the optimizer flow to log the caught publish failure before falling back to the
committed result. Preserve the existing re-raise when service.load_committed
returns None and the idempotent replay behavior when a committed result exists.
In `@reflexio/server/services/playbook/publication.py`:
- Around line 413-430: Update the module’s __all__ to include
PublishableOptimizerKind, PublicationSource, publication_source_for_optimizer,
PUBLICATION_PROOF_JSON_METADATA_KEY, and
PUBLICATION_PROJECTION_JSON_METADATA_KEY, then sort all exported names according
to Ruff’s RUF022 requirement.
In `@reflexio/server/services/storage/retention.py`:
- Around line 25-38: Update OptimizationRetentionClass to use Literal types for
artifact_class and owner, restricting them to the declared values in
OPTIMIZATION_RETENTION_CLASSES. Add the necessary typing import and preserve the
existing tuple entries and dataclass structure.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py`:
- Around line 348-353: Update the durable metadata serialization in the
surrounding optimization publication flow to use the existing
canonical_json_bytes canonicalizer instead of json.dumps with sort_keys and
compact separators. Preserve the resulting persisted metadata representation and
remove only the hand-rolled serialization options; do not apply the
HTTP-oriented use-jsonify change.
In `@reflexio/server/services/storage/storage_base/playbook/_user.py`:
- Around line 11-17: Move the PublicationClaim, PublicationRequest, and
PublicationResult contract dataclasses out of services.playbook.publication into
a shared lower-level contracts/models module, then update _user.py, SQLite
storage imports, and the optimizer/publication service to import them from that
shared module. Remove the upward storage_base dependency while preserving the
existing public behavior and type interfaces.
In
`@tests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py`:
- Around line 960-967: Update the pytest.raises assertion around
claim_user_playbook_publication in the claim_then_stale test to expect
StorageError instead of the broad Exception type, while preserving the existing
“worker fence” message match.
- Around line 1394-1402: Extract the duplicated prepare-then-crash logic from
the four test-local prepare_then_crash helpers into one module-level helper,
such as _prepare_then_expire_lease(storage, original_prepare). Update each
affected test to reuse it while preserving job_id recording, lease expiration,
commit behavior, and the RuntimeError crash semantics.
- Around line 1680-1695: Replace the raw source-text assertions in both affected
tests with behavioral or symbol-based checks: assert the forbidden
PlaybookOptimizer method is absent via the imported class, and import the
publication metadata constants from their shared module to verify identity with
the symbols used by SQLite optimization. Avoid grep-based checks so aliases,
attribute access, formatting, and unrelated renames cannot bypass or break the
tests.
In `@tests/server/services/playbook_optimizer/test_judge_frozen_plan.py`:
- Around line 105-110: Update the test setup around
prompt_manager.version_override to select an existing version from
PromptManager.list_versions(PLAYBOOK_OPTIMIZER_JUDGE_PROMPT_ID), or reuse the
active version, instead of hardcoding "1.1.0". Preserve the
FrozenEvaluatorPlanDriftError assertion and provider.assert_not_called()
behavior.
In `@tests/server/services/playbook_optimizer/test_playbook_optimizer.py`:
- Around line 786-795: In the test around optimizer.optimize for the
agent_playbook target, replace the process-wide Path.read_bytes patch with a
patch targeting the publication-authority digest helper or its hashing call
site. Keep the permission failure assertion focused on preventing
publication-authority hashing from reading the agent backend, while allowing
unrelated file reads during optimize() to proceed.
In `@tests/server/services/playbook/test_publication_models.py`:
- Around line 352-359: Remove the first UserPlaybookPublicationService(object())
assertion from test_publication_service_requires_explicit_verifier, since it
only verifies Python’s missing-argument error; retain the verifier=None case
with its PublicationDecisionVerifier message assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff18eb7f-27d5-4b65-84ab-1c05ba5076d8
📒 Files selected for processing (58)
reflexio/cli/commands/status_cmd.pyreflexio/cli/errors.pyreflexio/cli/utils.pyreflexio/integrations/openclaw/plugin/tests/test_publish.pyreflexio/lib/_base.pyreflexio/lib/_config.pyreflexio/lib/generation_client.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/prompt/prompt_manager.pyreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/extractor_interaction_utils.pyreflexio/server/services/generation_service.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/components/aggregator.pyreflexio/server/services/playbook/playbook_edit_apply.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/publication.pyreflexio/server/services/playbook_optimizer/gepa_publication.pyreflexio/server/services/playbook_optimizer/judge.pyreflexio/server/services/playbook_optimizer/optimizer.pyreflexio/server/services/storage/error.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/playbook/_optimization.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/playbook/_optimization.pyreflexio/server/services/storage/storage_base/playbook/_user.pyreflexio/server/services/storage/storage_base/profiles/_interaction_store.pyreflexio/server/tracing.pytests/cli/test_commands.pytests/cli/test_utils.pytests/deploy_guard/test_module_layout_contracts.pytests/e2e_tests/conftest.pytests/e2e_tests/test_complete_workflows.pytests/e2e_tests/test_knowledge_gap_real_llm.pytests/e2e_tests/test_resumable_extraction_e2e.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/playbook/test_aggregation_lineage_integration.pytests/server/services/playbook/test_apply_playbook_edit_integration.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/playbook/test_playbook_consolidator_integration.pytests/server/services/playbook/test_playbook_edit_apply.pytests/server/services/playbook/test_publication_models.pytests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.pytests/server/services/playbook_optimizer/test_judge_frozen_plan.pytests/server/services/playbook_optimizer/test_optimizer_supersede_integration.pytests/server/services/playbook_optimizer/test_playbook_optimizer.pytests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.pytests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.pytests/server/services/storage/test_playbook_optimization_replay_contract_integration.pytests/server/services/storage/test_sqlite_surface.pytests/server/services/storage/test_user_playbook_publication_sqlite.pytests/server/services/test_generation_service_scheduling.py
💤 Files with no reviewable changes (5)
- reflexio/server/services/playbook/playbook_edit_apply.py
- tests/server/services/playbook/test_apply_playbook_edit_integration.py
- reflexio/server/routes/config.py
- tests/server/services/playbook/test_playbook_edit_apply.py
- tests/deploy_guard/test_module_layout_contracts.py
Comment on lines
+181
to
+194
| prompt_manager=( | ||
| getattr(self.request_context, "prompt_manager", None) | ||
| if target.kind == "user_playbook" | ||
| else None | ||
| ), | ||
| include_publication_authority=target.kind == "user_playbook", | ||
| ) | ||
| frozen_judge_plan = ( | ||
| split_metadata[GEPA_PUBLICATION_AUTHORITY_METADATA_KEY][ | ||
| "evaluator_identity" | ||
| ]["pairwise_judge_request_plan"] | ||
| if target.kind == "user_playbook" | ||
| else None | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
prompt_manager is defaulted to None and then dereferenced.
getattr(self.request_context, "prompt_manager", None) anticipates a missing prompt manager, but for user targets it flows straight into build_pairwise_judge_request_plan(prompt_manager=...) → get_prompt_template_identity, producing an AttributeError on None outside the try that guards _run_gepa. Fail fast with a clear message instead.
🛡️ Fail with an explicit error
+ prompt_manager = getattr(self.request_context, "prompt_manager", None)
+ if target.kind == "user_playbook" and prompt_manager is None:
+ raise ValueError("GEPA user playbook publication requires a prompt manager")
split_metadata = _split_metadata(
windows,
train_windows,
validation_windows,
config=config,
assistant=assistant,
llm_client=self.llm_client,
- prompt_manager=(
- getattr(self.request_context, "prompt_manager", None)
- if target.kind == "user_playbook"
- else None
- ),
+ prompt_manager=prompt_manager if target.kind == "user_playbook" else None,
include_publication_authority=target.kind == "user_playbook",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| prompt_manager=( | |
| getattr(self.request_context, "prompt_manager", None) | |
| if target.kind == "user_playbook" | |
| else None | |
| ), | |
| include_publication_authority=target.kind == "user_playbook", | |
| ) | |
| frozen_judge_plan = ( | |
| split_metadata[GEPA_PUBLICATION_AUTHORITY_METADATA_KEY][ | |
| "evaluator_identity" | |
| ]["pairwise_judge_request_plan"] | |
| if target.kind == "user_playbook" | |
| else None | |
| ) | |
| prompt_manager = getattr(self.request_context, "prompt_manager", None) | |
| if target.kind == "user_playbook" and prompt_manager is None: | |
| raise ValueError( | |
| "GEPA user playbook publication requires a prompt manager" | |
| ) | |
| split_metadata = _split_metadata( | |
| windows, | |
| train_windows, | |
| validation_windows, | |
| config=config, | |
| assistant=assistant, | |
| llm_client=self.llm_client, | |
| prompt_manager=prompt_manager if target.kind == "user_playbook" else None, | |
| include_publication_authority=target.kind == "user_playbook", | |
| ) | |
| frozen_judge_plan = ( | |
| split_metadata[GEPA_PUBLICATION_AUTHORITY_METADATA_KEY][ | |
| "evaluator_identity" | |
| ]["pairwise_judge_request_plan"] | |
| if target.kind == "user_playbook" | |
| else None | |
| ) |
🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 181 -
194, In the user_playbook path of the optimizer setup, validate the
prompt_manager obtained from self.request_context before passing it to
build_pairwise_judge_request_plan. If it is missing, raise a clear explicit
error immediately, keeping prompt_manager as None only for non-user targets and
preserving the existing _run_gepa flow.
Comment on lines
+116
to
+117
| endpoint = "https://azure-one.example.test/" | ||
| secret = "azure-secret" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff flags S105 here at error severity.
If tests/** isn't covered by a per-file ignore for S105, this line fails the lint gate.
🔧 Silence the fixture credential
- secret = "azure-secret"
+ secret = "azure-secret" # noqa: S105 - test fixture value, not a real credential📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| endpoint = "https://azure-one.example.test/" | |
| secret = "azure-secret" | |
| endpoint = "https://azure-one.example.test/" | |
| secret = "azure-secret" # noqa: S105 - test fixture value, not a real credential |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 117-117: Possible hardcoded password assigned to: "secret"
(S105)
🤖 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/server/services/playbook_optimizer/test_judge_frozen_plan.py` around
lines 116 - 117, Silence Ruff S105 for the fixture credential assigned to secret
in the frozen-plan test, using the repository’s established per-file or inline
suppression convention. Keep the fixture value and test behavior unchanged, and
ensure the suppression applies only to this intentional test credential.
Source: Linters/SAST tools
guangyu-reflexio
force-pushed
the
codex/offline-tuner-credential-tests
branch
from
July 28, 2026 04:30
4fa275f to
3987781
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
reflexio/server/services/playbook/components/aggregator.py (1)
817-821: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
__exit__return value is ignored, so a scope that signals suppression is overridden by the re-raise at Line 855.Minor context-manager protocol deviation. If suppression is never intended for
apply_scope(), worth stating that in theAggregationEffectCoordinatordocstring; otherwise honor the returned flag.🤖 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 `@reflexio/server/services/playbook/components/aggregator.py` around lines 817 - 821, Update the exception handling in AggregationEffectCoordinator’s apply_scope flow to capture the boolean returned by failed_scope.__exit__(type(e), e, e.__traceback__) and honor it when deciding whether to re-raise the exception. If apply_scope intentionally never supports suppression, document that contract in the AggregationEffectCoordinator docstring instead.reflexio/server/services/governance/service.py (1)
306-330: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe lifecycle marker hijacks a generic
detail["status"]key on the sharedtarget_snapshotrow.
prepare_governance_erase_targetsowns that snapshot'sdetailpayload; a futurestatuskey there would silently be read as a lifecycle-completion marker (and this write already clobbers any existing one). Prefer a namespaced key.♻️ Namespaced marker key
-_LIFECYCLE_COMPLETION_STATUS = "complete" +_LIFECYCLE_COMPLETION_KEY = "subject_erasure_lifecycle_status" +_LIFECYCLE_COMPLETION_STATUS = "complete"- and (snapshot.detail or {}).get("status") == _LIFECYCLE_COMPLETION_STATUS + and (snapshot.detail or {}).get(_LIFECYCLE_COMPLETION_KEY) + == _LIFECYCLE_COMPLETION_STATUS- detail["status"] = _LIFECYCLE_COMPLETION_STATUS + detail[_LIFECYCLE_COMPLETION_KEY] = _LIFECYCLE_COMPLETION_STATUS(the assertion in
tests/server/services/governance/test_governance_local_e2e.pyLine 785 needs the same key.)🤖 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 `@reflexio/server/services/governance/service.py` around lines 306 - 330, Update _subject_erasure_lifecycle_complete and _record_subject_erasure_lifecycle_complete to use a dedicated namespaced lifecycle marker key in snapshot.detail instead of the generic "status" key, preserving the existing completion value and other prepare_governance_erase_targets-owned detail fields. Update the corresponding assertion in test_governance_local_e2e.py to reference the same namespaced key.tests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py (1)
49-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider marking this module
pytest.mark.integration.Every test here builds a real
SQLiteStorageon disk and drives a fulloptimize()pass, and several parametrized cases do it twice per test. The siblingtests/server/services/storage/test_user_playbook_publication_sqlite.pydeclarespytestmark = pytest.mark.integration; without it these run in the fast unit selection.🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py` around lines 49 - 63, Mark the test module containing the _storage helper and optimize() scenarios with pytest.mark.integration, matching the module-level marker used by the sibling SQLite publication tests. Keep the existing test implementations unchanged so these disk-backed, full optimization tests are excluded from fast unit selection.reflexio/server/services/storage/sqlite_storage/_base.py (2)
1563-1585: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHardcoded column list will silently drop future columns.
This rebuild enumerates every
playbook_optimization_jobscolumn twice; a column added to_DDLlater but not here disappears on upgrade with no error. Therequestsrebuild in this same file carries an explicit maintenance NOTE for exactly this hazard — mirror it here.📝 Add the maintenance warning
+ # NOTE: this rebuild hardcodes the full `playbook_optimization_jobs` + # column set. If a future migration adds a column, it MUST be added + # here (and to the SELECT below) or the rebuild silently drops it. self.conn.execute( """ INSERT INTO playbook_optimization_jobs_new (🤖 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 `@reflexio/server/services/storage/sqlite_storage/_base.py` around lines 1563 - 1585, Add a maintenance NOTE adjacent to the playbook_optimization_jobs rebuild INSERT, explicitly warning that the source and destination column lists must be updated whenever _DDL adds a column. Mirror the existing warning used by the requests rebuild in this file, without changing the migration logic.
1331-1340: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThis drops and rebuilds three partial UNIQUE indexes on every startup.
migrate()runs on everySQLiteStorageconstruction, so an already-migrated database still pays the full drop/classify/dedupe/recreate cycle and briefly runs with active-job uniqueness unenforced. Consider short-circuiting when there is nothing to classify and the indexes already exist.♻️ Early exit when already classified
if "optimizer_kind" not in columns: return + unclassified = self.conn.execute( + "SELECT 1 FROM playbook_optimization_jobs " + "WHERE optimizer_kind IS NULL LIMIT 1" + ).fetchone() + existing_indexes = { + row["name"] + for row in self.conn.execute( + "SELECT name FROM sqlite_master WHERE type = 'index'" + ).fetchall() + } + if unclassified is None and { + "uq_poj_active_discovery", + "uq_poj_active_attempt", + "uq_poj_active_target", + } <= existing_indexes: + return for index_name in (🤖 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 `@reflexio/server/services/storage/sqlite_storage/_base.py` around lines 1331 - 1340, Update the migration logic in migrate() to return early when the optimizer_kind column is present and all three partial unique indexes—uq_poj_active_discovery, uq_poj_active_attempt, and uq_poj_active_target—already exist. Perform the existence check before dropping any indexes, while preserving the current rebuild path for databases missing the classification column or one or more indexes.reflexio/server/services/playbook_optimizer/optimizer.py (2)
696-701: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRe-derive the subject-epoch bytes with
canonical_json_bytes, notjson.dumps.The original
subject_epochs_jsonwas produced bycanonical_json_bytes(RFC 8785), and_assert_staging_matchescompares this field byte-for-byte against the staged row. Reconstructing it here with a second, differently-specified canonicalizer (sort_keysuses code-point order, not UTF-16 code-unit order) means any future key that diverges between the two orderings turns a resumable recovery into a hard "staged publication conflicts on subject epochs" failure. Use the same encoder that produced the staged bytes.♻️ Use the canonical encoder
- subject_epochs_json=json.dumps( - subject_epochs, - ensure_ascii=False, - separators=(",", ":"), - sort_keys=True, - ), + subject_epochs_json=canonical_json_bytes(subject_epochs).decode("utf-8"),Add to the existing
reflexio.server.services.playbook.publicationimport block:from reflexio.server.services.playbook.publication import ( canonical_json_bytes, )🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 696 - 701, Replace the json.dumps construction of subject_epochs_json in the optimizer flow with the existing canonical_json_bytes encoder, and add canonical_json_bytes to the reflexio.server.services.playbook.publication imports. Preserve the subject_epochs input while ensuring the reconstructed bytes use the same RFC 8785 canonicalization as the staged row.
1166-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_json_digest/_text_digest/_code_digest/_decimal_stringare duplicated verbatim injudge.py.Both copies feed the same frozen-authority digests, so a future tweak to one (e.g. changing the
inspect.getsourcefallback) silently desynchronizes plan freezing from authority freezing. Consider extracting them into a shared helper module imported by both.🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 1166 - 1191, The digest and code-identity helpers are duplicated between optimizer.py and judge.py, risking inconsistent frozen-authority digests. Extract _json_digest, _text_digest, _code_identity, _code_digest, and _decimal_string into a shared helper module, then import and reuse those symbols from both call sites while preserving their current behavior.reflexio/server/services/storage/sqlite_storage/playbook/_user.py (1)
289-297: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUnconditional
BEGIN IMMEDIATEdiverges from the_own_transaction()convention used elsewhere in this mixin.
claim_user_playbook_publication,stage_user_playbook_publication, andcommit_user_playbook_publicationall begin unconditionally, whilesave_user_playbooksandupdate_user_playbookguard onself._own_transaction(). If any future caller wraps publication incommit_scope,BEGINraises and theexceptbranch'sself.conn.rollback()discards the outer scope's pending writes instead of surfacing a clean error. Either adopt the guard or add an explicit assertion that publication must own its transaction.🛡️ Explicit precondition
with self._lock: + if not self._own_transaction(): + raise StorageError( + "publication must own its transaction; " + "do not call inside commit_scope" + ) self.conn.execute("BEGIN IMMEDIATE")🤖 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 `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py` around lines 289 - 297, Update claim_user_playbook_publication, stage_user_playbook_publication, and commit_user_playbook_publication to follow the _own_transaction() convention used by save_user_playbooks and update_user_playbook: begin and roll back only when the method owns the transaction, while preserving commit behavior for owned transactions. Ensure publication calls within commit_scope do not issue a nested BEGIN or roll back the outer transaction.
🤖 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 `@docs/playbook_optimizer_assistant_backends.md`:
- Around line 267-275: Update the numbered workflow diagram in the playbook
optimizer documentation so feature, target-kind, and adoption kill-switch checks
plus staged-publication recovery occur before the _create_assistant step. Keep
backend selection as the subsequent step for new optimization runs, matching the
runtime order described above.
In `@reflexio/server/services/governance/service.py`:
- Around line 45-54: Update the SubjectErasureLifecycle protocol documentation
to state that erase_subject must be idempotent because retries may invoke it
again before completion is recorded. Keep the existing method signature and
deployment-specific erasure semantics unchanged.
In `@reflexio/server/services/playbook/publication.py`:
- Around line 413-430: Reorder the entries in __all__ according to Ruff
RUF022/isort-style ordering, placing all PUBLICATION_* constants before the
CamelCase symbols while preserving every existing export.
In
`@tests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py`:
- Line 970: Update the pytest.raises assertion in the worker-fence test to
expect StorageError instead of the broad Exception type, while preserving the
existing “worker fence” message match and surrounding test behavior.
- Around line 559-560: Pin REFLEXIO_LLM_HARD_TIMEOUT_GRACE_SECONDS to the value
producing 125.0 within the test covering the generation["rungs"] timeout
assertions, using the test’s existing environment monkeypatch fixture or
mechanism. Keep the exact-value assertions unchanged and ensure the environment
override applies before the playbook optimization runs.
---
Nitpick comments:
In `@reflexio/server/services/governance/service.py`:
- Around line 306-330: Update _subject_erasure_lifecycle_complete and
_record_subject_erasure_lifecycle_complete to use a dedicated namespaced
lifecycle marker key in snapshot.detail instead of the generic "status" key,
preserving the existing completion value and other
prepare_governance_erase_targets-owned detail fields. Update the corresponding
assertion in test_governance_local_e2e.py to reference the same namespaced key.
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 696-701: Replace the json.dumps construction of
subject_epochs_json in the optimizer flow with the existing canonical_json_bytes
encoder, and add canonical_json_bytes to the
reflexio.server.services.playbook.publication imports. Preserve the
subject_epochs input while ensuring the reconstructed bytes use the same RFC
8785 canonicalization as the staged row.
- Around line 1166-1191: The digest and code-identity helpers are duplicated
between optimizer.py and judge.py, risking inconsistent frozen-authority
digests. Extract _json_digest, _text_digest, _code_identity, _code_digest, and
_decimal_string into a shared helper module, then import and reuse those symbols
from both call sites while preserving their current behavior.
In `@reflexio/server/services/playbook/components/aggregator.py`:
- Around line 817-821: Update the exception handling in
AggregationEffectCoordinator’s apply_scope flow to capture the boolean returned
by failed_scope.__exit__(type(e), e, e.__traceback__) and honor it when deciding
whether to re-raise the exception. If apply_scope intentionally never supports
suppression, document that contract in the AggregationEffectCoordinator
docstring instead.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1563-1585: Add a maintenance NOTE adjacent to the
playbook_optimization_jobs rebuild INSERT, explicitly warning that the source
and destination column lists must be updated whenever _DDL adds a column. Mirror
the existing warning used by the requests rebuild in this file, without changing
the migration logic.
- Around line 1331-1340: Update the migration logic in migrate() to return early
when the optimizer_kind column is present and all three partial unique
indexes—uq_poj_active_discovery, uq_poj_active_attempt, and
uq_poj_active_target—already exist. Perform the existence check before dropping
any indexes, while preserving the current rebuild path for databases missing the
classification column or one or more indexes.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Around line 289-297: Update claim_user_playbook_publication,
stage_user_playbook_publication, and commit_user_playbook_publication to follow
the _own_transaction() convention used by save_user_playbooks and
update_user_playbook: begin and roll back only when the method owns the
transaction, while preserving commit behavior for owned transactions. Ensure
publication calls within commit_scope do not issue a nested BEGIN or roll back
the outer transaction.
In
`@tests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py`:
- Around line 49-63: Mark the test module containing the _storage helper and
optimize() scenarios with pytest.mark.integration, matching the module-level
marker used by the sibling SQLite publication tests. Keep the existing test
implementations unchanged so these disk-backed, full optimization tests are
excluded from fast unit selection.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eae8200a-dd0c-4267-94cb-29d1ae7f2008
📒 Files selected for processing (65)
docs/playbook_optimizer_assistant_backends.mdreflexio/cli/commands/status_cmd.pyreflexio/cli/errors.pyreflexio/cli/utils.pyreflexio/client/__init__.pyreflexio/client/client.pyreflexio/integrations/openclaw/plugin/tests/test_publish.pyreflexio/lib/_base.pyreflexio/lib/_config.pyreflexio/lib/generation_client.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/prompt/prompt_manager.pyreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/configurator/config_storage.pyreflexio/server/services/extractor_interaction_utils.pyreflexio/server/services/generation_service.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/components/aggregator.pyreflexio/server/services/playbook/playbook_edit_apply.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/publication.pyreflexio/server/services/playbook_optimizer/gepa_publication.pyreflexio/server/services/playbook_optimizer/judge.pyreflexio/server/services/playbook_optimizer/optimizer.pyreflexio/server/services/storage/error.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/playbook/_optimization.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/playbook/_optimization.pyreflexio/server/services/storage/storage_base/playbook/_user.pyreflexio/server/services/storage/storage_base/profiles/_interaction_store.pyreflexio/server/tracing.pytests/cli/test_commands.pytests/cli/test_utils.pytests/client/test_config_client.pytests/deploy_guard/test_module_layout_contracts.pytests/e2e_tests/conftest.pytests/e2e_tests/test_cleanup_storage.pytests/e2e_tests/test_complete_workflows.pytests/e2e_tests/test_knowledge_gap_real_llm.pytests/e2e_tests/test_resumable_extraction_e2e.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/playbook/components/test_aggregator_effect_scope.pytests/server/services/playbook/test_aggregation_lineage_integration.pytests/server/services/playbook/test_apply_playbook_edit_integration.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/playbook/test_playbook_edit_apply.pytests/server/services/playbook/test_publication_models.pytests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.pytests/server/services/playbook_optimizer/test_judge_frozen_plan.pytests/server/services/playbook_optimizer/test_optimizer_supersede_integration.pytests/server/services/playbook_optimizer/test_playbook_optimizer.pytests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.pytests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.pytests/server/services/storage/test_playbook_optimization_replay_contract_integration.pytests/server/services/storage/test_sqlite_surface.pytests/server/services/storage/test_user_playbook_publication_sqlite.pytests/server/services/test_generation_service_scheduling.py
💤 Files with no reviewable changes (4)
- tests/server/services/playbook/test_apply_playbook_edit_integration.py
- reflexio/server/services/playbook/playbook_edit_apply.py
- tests/server/services/playbook/test_playbook_edit_apply.py
- tests/deploy_guard/test_module_layout_contracts.py
🚧 Files skipped from review as they are similar to previous changes (32)
- reflexio/integrations/openclaw/plugin/tests/test_publish.py
- reflexio/server/services/extractor_interaction_utils.py
- reflexio/lib/_config.py
- reflexio/server/services/storage/error.py
- reflexio/lib/generation_client.py
- reflexio/server/services/playbook/playbook_service_utils.py
- reflexio/server/services/storage/storage_base/init.py
- tests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.py
- reflexio/server/services/generation_service.py
- tests/cli/test_utils.py
- tests/cli/test_commands.py
- reflexio/lib/_base.py
- tests/server/services/playbook/test_playbook_aggregator.py
- reflexio/server/services/storage/storage_base/profiles/_interaction_store.py
- tests/server/services/test_generation_service_scheduling.py
- tests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.py
- tests/e2e_tests/test_knowledge_gap_real_llm.py
- tests/e2e_tests/test_complete_workflows.py
- reflexio/server/services/storage/retention.py
- reflexio/server/services/storage/storage_base/playbook/_user.py
- reflexio/server/tracing.py
- reflexio/models/api_schema/domain/entities.py
- reflexio/server/llm/_litellm_text_generation.py
- tests/server/services/playbook/test_aggregation_lineage_integration.py
- reflexio/cli/utils.py
- reflexio/server/prompt/prompt_manager.py
- tests/e2e_tests/conftest.py
- reflexio/server/services/playbook_optimizer/gepa_publication.py
- tests/server/services/playbook_optimizer/test_optimizer_supersede_integration.py
- tests/server/services/playbook_optimizer/test_playbook_optimizer.py
- tests/e2e_tests/test_resumable_extraction_e2e.py
- reflexio/server/services/storage/storage_base/playbook/_optimization.py
Comment on lines
+413
to
+430
| __all__ = [ | ||
| "DecisionProofEnvelope", | ||
| "PublicationClaim", | ||
| "PublicationDecisionVerifier", | ||
| "PublicationOutcome", | ||
| "PublicationRequest", | ||
| "PublicationResult", | ||
| "PublicationSearchProjection", | ||
| "PUBLICATION_INCUMBENT_CONTENT_DIGEST_METADATA_KEY", | ||
| "PUBLICATION_INCUMBENT_SEMANTIC_DIGEST_METADATA_KEY", | ||
| "PUBLICATION_INCUMBENT_TRIGGER_METADATA_KEY", | ||
| "PUBLICATION_SUBJECT_EPOCHS_METADATA_KEY", | ||
| "UserPlaybookPublicationService", | ||
| "UserPlaybookPublicationStore", | ||
| "canonical_json_bytes", | ||
| "incumbent_user_playbook_semantic_digest", | ||
| "publish_user_playbook_successor", | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff RUF022: __all__ is not sorted.
The PUBLICATION_* constants must precede the CamelCase names under isort-style ordering.
🔧 Sorted `__all__`
__all__ = [
+ "PUBLICATION_INCUMBENT_CONTENT_DIGEST_METADATA_KEY",
+ "PUBLICATION_INCUMBENT_SEMANTIC_DIGEST_METADATA_KEY",
+ "PUBLICATION_INCUMBENT_TRIGGER_METADATA_KEY",
+ "PUBLICATION_SUBJECT_EPOCHS_METADATA_KEY",
"DecisionProofEnvelope",
"PublicationClaim",
"PublicationDecisionVerifier",
"PublicationOutcome",
"PublicationRequest",
"PublicationResult",
"PublicationSearchProjection",
- "PUBLICATION_INCUMBENT_CONTENT_DIGEST_METADATA_KEY",
- "PUBLICATION_INCUMBENT_SEMANTIC_DIGEST_METADATA_KEY",
- "PUBLICATION_INCUMBENT_TRIGGER_METADATA_KEY",
- "PUBLICATION_SUBJECT_EPOCHS_METADATA_KEY",
"UserPlaybookPublicationService",
"UserPlaybookPublicationStore",
"canonical_json_bytes",
"incumbent_user_playbook_semantic_digest",
"publish_user_playbook_successor",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| __all__ = [ | |
| "DecisionProofEnvelope", | |
| "PublicationClaim", | |
| "PublicationDecisionVerifier", | |
| "PublicationOutcome", | |
| "PublicationRequest", | |
| "PublicationResult", | |
| "PublicationSearchProjection", | |
| "PUBLICATION_INCUMBENT_CONTENT_DIGEST_METADATA_KEY", | |
| "PUBLICATION_INCUMBENT_SEMANTIC_DIGEST_METADATA_KEY", | |
| "PUBLICATION_INCUMBENT_TRIGGER_METADATA_KEY", | |
| "PUBLICATION_SUBJECT_EPOCHS_METADATA_KEY", | |
| "UserPlaybookPublicationService", | |
| "UserPlaybookPublicationStore", | |
| "canonical_json_bytes", | |
| "incumbent_user_playbook_semantic_digest", | |
| "publish_user_playbook_successor", | |
| ] | |
| __all__ = [ | |
| "PUBLICATION_INCUMBENT_CONTENT_DIGEST_METADATA_KEY", | |
| "PUBLICATION_INCUMBENT_SEMANTIC_DIGEST_METADATA_KEY", | |
| "PUBLICATION_INCUMBENT_TRIGGER_METADATA_KEY", | |
| "PUBLICATION_SUBJECT_EPOCHS_METADATA_KEY", | |
| "DecisionProofEnvelope", | |
| "PublicationClaim", | |
| "PublicationDecisionVerifier", | |
| "PublicationOutcome", | |
| "PublicationRequest", | |
| "PublicationResult", | |
| "PublicationSearchProjection", | |
| "UserPlaybookPublicationService", | |
| "UserPlaybookPublicationStore", | |
| "canonical_json_bytes", | |
| "incumbent_user_playbook_semantic_digest", | |
| "publish_user_playbook_successor", | |
| ] |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 413-430: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 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 `@reflexio/server/services/playbook/publication.py` around lines 413 - 430,
Reorder the entries in __all__ according to Ruff RUF022/isort-style ordering,
placing all PUBLICATION_* constants before the CamelCase symbols while
preserving every existing export.
Source: Linters/SAST tools
Comment on lines
+559
to
+560
| assert all(rung["timeout_seconds"] == "120.0" for rung in generation["rungs"]) | ||
| assert all(rung["hard_timeout_seconds"] == "125.0" for rung in generation["rungs"]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pin the hard-timeout grace env var for these exact-value assertions.
hard_timeout_seconds == "125.0" depends on the default REFLEXIO_LLM_HARD_TIMEOUT_GRACE_SECONDS; test_judge_frozen_plan.py explicitly monkeypatches it for the same reason. If it is set in the ambient environment this test fails for unrelated reasons.
🔧 Pin the env var
-def test_gepa_user_authority_freezes_generation_ladder_and_retry_contract(tmp_path):
+def test_gepa_user_authority_freezes_generation_ladder_and_retry_contract(
+ tmp_path, monkeypatch
+):
+ monkeypatch.setenv("REFLEXIO_LLM_HARD_TIMEOUT_GRACE_SECONDS", "5")
config = _optimizer_config(tmp_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert all(rung["timeout_seconds"] == "120.0" for rung in generation["rungs"]) | |
| assert all(rung["hard_timeout_seconds"] == "125.0" for rung in generation["rungs"]) | |
| def test_gepa_user_authority_freezes_generation_ladder_and_retry_contract( | |
| tmp_path, monkeypatch | |
| ): | |
| monkeypatch.setenv("REFLEXIO_LLM_HARD_TIMEOUT_GRACE_SECONDS", "5") | |
| assert all(rung["timeout_seconds"] == "120.0" for rung in generation["rungs"]) | |
| assert all(rung["hard_timeout_seconds"] == "125.0" for rung in generation["rungs"]) |
🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py`
around lines 559 - 560, Pin REFLEXIO_LLM_HARD_TIMEOUT_GRACE_SECONDS to the value
producing 125.0 within the test covering the generation["rungs"] timeout
assertions, using the test’s existing environment monkeypatch fixture or
mechanism. Keep the exact-value assertions unchanged and ensure the environment
override applies before the playbook optimization runs.
| "claim_user_playbook_publication", | ||
| side_effect=claim_then_stale, | ||
| ), | ||
| pytest.raises(Exception, match="worker fence"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert on StorageError rather than bare Exception.
pytest.raises(Exception, ...) will accept an unrelated failure (including a bug in the test harness) that happens to mention "worker fence"; every sibling fence test in this PR pins StorageError.
🔧 Tighten the expected type
- pytest.raises(Exception, match="worker fence"),
+ pytest.raises(StorageError, match="worker fence"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pytest.raises(Exception, match="worker fence"), | |
| pytest.raises(StorageError, match="worker fence"), |
🤖 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/server/services/playbook_optimizer/test_gepa_user_playbook_publication.py`
at line 970, Update the pytest.raises assertion in the worker-fence test to
expect StorageError instead of the broad Exception type, while preserving the
existing “worker fence” message match and surrounding test behavior.
guangyu-reflexio
force-pushed
the
codex/offline-tuner-credential-tests
branch
from
July 28, 2026 05:57
3987781 to
3636fe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/llm/_litellm_text_generation.py (1)
364-374: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
provider_request_guardis never forwarded and is undefined ingenerate_chat_response_with_provenance(NameError on every chat call).Two coupled defects with one root cause: the parameter was added to
generate_chat_response(Line 322) but not togenerate_chat_response_with_provenance(Lines 376-388), while the latter's body references it at Lines 427-428.
- Line 427 raises
NameError: name 'provider_request_guard' is not definedfor every call routed throughgenerate_chat_response_with_provenance— which is the delegation target ofgenerate_chat_response. Ruff already flags this as F821.- Even after fixing the signature, Lines 364-374 do not pass
provider_request_guarddown, soPairwiseJudge's frozen-plan guard (judge.pyLine 276) would silently never run and drift would reach the provider.🐛 Proposed fix
return self.generate_chat_response_with_provenance( messages, system_message, tools=tools, tool_choice=tool_choice, model_role=model_role, max_retries=max_retries, fallback_models=fallback_models, structured_output_validator=structured_output_validator, + provider_request_guard=provider_request_guard, **kwargs, ).valuedef generate_chat_response_with_provenance( self, messages: list[dict[str, Any]], system_message: str | None = None, *, tools: list[Any] | None = None, tool_choice: str | dict[str, Any] | None = None, model_role: ModelRole | None = None, max_retries: int | None = None, fallback_models: list[str] | None = None, structured_output_validator: StructuredOutputValidator | None = None, + provider_request_guard: ProviderRequestGuard | None = None, **kwargs: Any, ) -> CompletionResult[str | BaseModel | ToolCallingChatResponse]:🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 364 - 374, Update generate_chat_response_with_provenance to accept the provider_request_guard parameter, then forward it from generate_chat_response in the delegation call. Ensure the existing provider_request_guard usage within generate_chat_response_with_provenance receives the caller’s guard so NameError is eliminated and the frozen-plan validation remains active.Source: Linters/SAST tools
🧹 Nitpick comments (5)
reflexio/server/services/storage/sqlite_storage/_base.py (1)
1331-1346: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_classify_legacy_playbook_optimization_jobsre-runs its full drop/rebuild on every startup.The helper is invoked unconditionally from
migrate()(Line 789). On an already-migrated databaseoptimizer_kindis never NULL, so the classification UPDATE is a no-op, yet each boot still drops the three active-row UNIQUE indexes, runs two fullpending/runningrescans, and rebuilds the indexes. Besides the startup cost on large job tables, the uniqueness guard is briefly absent on every boot.Consider short-circuiting when no legacy work remains, e.g. return early if
SELECT 1 FROM playbook_optimization_jobs WHERE optimizer_kind IS NULL LIMIT 1finds nothing and all three indexes already exist insqlite_master.Also applies to: 1464-1481
🤖 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 `@reflexio/server/services/storage/sqlite_storage/_base.py` around lines 1331 - 1346, Update _classify_legacy_playbook_optimization_jobs to short-circuit when no legacy rows remain and all three active-row indexes already exist in sqlite_master. Check for an optimizer_kind IS NULL row and the presence of uq_poj_active_discovery, uq_poj_active_attempt, and uq_poj_active_target before dropping or rebuilding indexes; return immediately when migration is complete, while preserving the existing classification and rebuild path for legacy databases.reflexio/server/services/playbook_optimizer/judge.py (1)
340-345: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnexpected provider params escape as a retryable transport error instead of drift.
sanitize_pairwise_judge_provider_paramsraisesValueErrorfor keys outside_PROVIDER_PARAM_KEYS.ValueErroris not aProviderRequestGuardError, so_call_and_parse's generic handler wraps it inLiteLLMClientErrorand the ladder walk advances to the next rung and retries — the exact "unknown parameter reached the provider boundary" case fails open instead of stopping the evaluation.♻️ Proposed change
- provider_params = sanitize_pairwise_judge_provider_params(params) + try: + provider_params = sanitize_pairwise_judge_provider_params(params) + except ValueError as exc: + raise FrozenEvaluatorPlanDriftError( + f"PairwiseJudge provider params drifted after job creation: {exc}" + ) from exc🤖 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 `@reflexio/server/services/playbook_optimizer/judge.py` around lines 340 - 345, Update the provider-parameter handling in the method containing sanitize_pairwise_judge_provider_params and _call_and_parse so unexpected-parameter ValueError failures are converted to the existing ProviderRequestGuardError type before generic transport handling. Ensure the guard error propagates without advancing the ladder or retrying, while preserving normal timeout and provider selection behavior.reflexio/server/services/storage/sqlite_storage/playbook/_user.py (1)
374-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCalling the dunder
__post_init__()directly to re-validate is a smell.PublicationRequestis a frozen dataclass already validated at construction; if a defensive re-check is intended, expose a publicvalidate()on the contract and call that instead (same forcommit_user_playbook_publication).🤖 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 `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py` around lines 374 - 375, Replace direct __post_init__() calls in stage_user_playbook_publication and commit_user_playbook_publication with a public validate() method on the PublicationRequest contract. Implement validate() to perform the existing defensive checks while preserving the frozen dataclass’s construction-time validation.reflexio/server/services/playbook_optimizer/optimizer.py (1)
1045-1053: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOverwriting the manifest digest in place is easy to miss.
gepa_adoption_authority_from_configreturns avalidation_manifestwhosedigestis immediately replaced here, so the helper's own value is dead. Consider having the helper accept/return the digest so the authority payload has a single source of truth.🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 1045 - 1053, Update the interaction between gepa_adoption_authority_from_config and the surrounding authority-building flow so the validation manifest digest is computed and supplied through a single source of truth rather than overwritten in place. Pass or return the digest through the helper’s interface, then preserve it when constructing the final authority payload without assigning over validation_manifest["digest"].reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py (1)
310-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent transaction-ownership discipline across the new publication storage methods.
create_or_get_playbook_optimization_jobandupsert_playbook_optimization_artifactgateBEGIN IMMEDIATE/commit/rollbackonself._own_transaction(), but the new publication entry points issue them unconditionally. Nesting any of these inside acommit_scope()would raise "cannot start a transaction within a transaction" and, worse, theexceptbranch would roll back the caller's outer transaction. No current caller nests, so this is a consistency/robustness fix rather than a live bug.
reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py#L310-L312: gate theBEGIN IMMEDIATE(and the matchingcommit/rollbackat L398-L402) inprepare_gepa_user_playbook_publicationonself._own_transaction(), mirroringcreate_or_get_playbook_optimization_job.reflexio/server/services/storage/sqlite_storage/playbook/_user.py#L297-L299: apply the same_own_transaction()gate inclaim_user_playbook_publication.reflexio/server/services/storage/sqlite_storage/playbook/_user.py#L376-L378: apply the same gate instage_user_playbook_publication.reflexio/server/services/storage/sqlite_storage/playbook/_user.py#L536-L538: apply the same gate incommit_user_playbook_publication.🤖 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 `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py` around lines 310 - 312, Apply consistent transaction ownership handling to the publication methods: in reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py:310-312, update prepare_gepa_user_playbook_publication to guard BEGIN IMMEDIATE and its matching commit/rollback at 398-402 with self._own_transaction(). Apply the same gate in claim_user_playbook_publication at reflexio/server/services/storage/sqlite_storage/playbook/_user.py:297-299, stage_user_playbook_publication at 376-378, and commit_user_playbook_publication at 536-538, preserving the caller’s outer transaction.
🤖 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 `@reflexio/server/services/playbook_optimizer/judge.py`:
- Around line 270-276: Update the provenance wrapper’s generate_chat_response
method to forward provider_request_guard unchanged into _make_request so
frozen-plan validation remains enforced. Preserve the call from the judge flow,
and retain or add a regression test that mutates the model or API base after
freezing and verifies the request is rejected.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Line 614: Update the GEPA publication projection flow around
build_gepa_search_projection and the embedding conversion so empty embeddings
are handled like save_user_playbooks: either permit an empty embedding in
PublicationSearchProjection or skip vector insertion when no embedding is
available, while preserving vector behavior for populated embeddings.
In `@tests/cli/test_commands.py`:
- Around line 140-152: The test setup around mock_client.search must exercise
serialization of response.user_playbooks instead of hard-coding the serialized
output. Construct the actual response model, or configure response.model_dump to
derive from response.user_playbooks, so successor.model_dump contributes
user_playbook_id 77 and the test detects dropped successor IDs.
In `@tests/server/services/playbook/test_aggregation_lineage_integration.py`:
- Around line 318-320: Update the test helper’s complete method in the lifecycle
fixture so completion is asserted only after apply_scope() exits, matching the
managed post-scope completion order. Remove the active-scope assertion from
complete() while preserving storage of completed_result, and place any lifecycle
assertion at the scope boundary that verifies completion occurs afterward.
---
Outside diff comments:
In `@reflexio/server/llm/_litellm_text_generation.py`:
- Around line 364-374: Update generate_chat_response_with_provenance to accept
the provider_request_guard parameter, then forward it from
generate_chat_response in the delegation call. Ensure the existing
provider_request_guard usage within generate_chat_response_with_provenance
receives the caller’s guard so NameError is eliminated and the frozen-plan
validation remains active.
---
Nitpick comments:
In `@reflexio/server/services/playbook_optimizer/judge.py`:
- Around line 340-345: Update the provider-parameter handling in the method
containing sanitize_pairwise_judge_provider_params and _call_and_parse so
unexpected-parameter ValueError failures are converted to the existing
ProviderRequestGuardError type before generic transport handling. Ensure the
guard error propagates without advancing the ladder or retrying, while
preserving normal timeout and provider selection behavior.
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 1045-1053: Update the interaction between
gepa_adoption_authority_from_config and the surrounding authority-building flow
so the validation manifest digest is computed and supplied through a single
source of truth rather than overwritten in place. Pass or return the digest
through the helper’s interface, then preserve it when constructing the final
authority payload without assigning over validation_manifest["digest"].
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1331-1346: Update _classify_legacy_playbook_optimization_jobs to
short-circuit when no legacy rows remain and all three active-row indexes
already exist in sqlite_master. Check for an optimizer_kind IS NULL row and the
presence of uq_poj_active_discovery, uq_poj_active_attempt, and
uq_poj_active_target before dropping or rebuilding indexes; return immediately
when migration is complete, while preserving the existing classification and
rebuild path for legacy databases.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py`:
- Around line 310-312: Apply consistent transaction ownership handling to the
publication methods: in
reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py:310-312,
update prepare_gepa_user_playbook_publication to guard BEGIN IMMEDIATE and its
matching commit/rollback at 398-402 with self._own_transaction(). Apply the same
gate in claim_user_playbook_publication at
reflexio/server/services/storage/sqlite_storage/playbook/_user.py:297-299,
stage_user_playbook_publication at 376-378, and commit_user_playbook_publication
at 536-538, preserving the caller’s outer transaction.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_user.py`:
- Around line 374-375: Replace direct __post_init__() calls in
stage_user_playbook_publication and commit_user_playbook_publication with a
public validate() method on the PublicationRequest contract. Implement
validate() to perform the existing defensive checks while preserving the frozen
dataclass’s construction-time validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14dabecc-7b54-425b-b6bf-c76b37294c60
📒 Files selected for processing (64)
docs/playbook_optimizer_assistant_backends.mdreflexio/cli/commands/status_cmd.pyreflexio/cli/errors.pyreflexio/cli/utils.pyreflexio/client/__init__.pyreflexio/client/client.pyreflexio/integrations/openclaw/plugin/tests/test_publish.pyreflexio/lib/_base.pyreflexio/lib/_config.pyreflexio/lib/generation_client.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/prompt/prompt_manager.pyreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/configurator/config_storage.pyreflexio/server/services/extractor_interaction_utils.pyreflexio/server/services/generation_service.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/components/aggregator.pyreflexio/server/services/playbook/playbook_edit_apply.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/publication.pyreflexio/server/services/playbook_optimizer/gepa_publication.pyreflexio/server/services/playbook_optimizer/judge.pyreflexio/server/services/playbook_optimizer/optimizer.pyreflexio/server/services/storage/error.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/playbook/_optimization.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/storage_base/playbook/_optimization.pyreflexio/server/services/storage/storage_base/playbook/_user.pyreflexio/server/services/storage/storage_base/profiles/_interaction_store.pyreflexio/server/tracing.pytests/cli/test_commands.pytests/cli/test_utils.pytests/client/test_config_client.pytests/deploy_guard/test_module_layout_contracts.pytests/e2e_tests/conftest.pytests/e2e_tests/test_cleanup_storage.pytests/e2e_tests/test_complete_workflows.pytests/e2e_tests/test_knowledge_gap_real_llm.pytests/e2e_tests/test_resumable_extraction_e2e.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/playbook/components/test_aggregator_effect_scope.pytests/server/services/playbook/test_aggregation_lineage_integration.pytests/server/services/playbook/test_apply_playbook_edit_integration.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/playbook/test_playbook_edit_apply.pytests/server/services/playbook/test_publication_models.pytests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.pytests/server/services/playbook_optimizer/test_judge_frozen_plan.pytests/server/services/playbook_optimizer/test_optimizer_supersede_integration.pytests/server/services/playbook_optimizer/test_playbook_optimizer.pytests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.pytests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.pytests/server/services/storage/test_playbook_optimization_replay_contract_integration.pytests/server/services/storage/test_sqlite_surface.pytests/server/services/storage/test_user_playbook_publication_sqlite.pytests/server/services/test_generation_service_scheduling.py
💤 Files with no reviewable changes (4)
- reflexio/server/services/playbook/playbook_edit_apply.py
- tests/deploy_guard/test_module_layout_contracts.py
- tests/server/services/playbook/test_playbook_edit_apply.py
- tests/server/services/playbook/test_apply_playbook_edit_integration.py
🚧 Files skipped from review as they are similar to previous changes (42)
- reflexio/server/services/configurator/config_storage.py
- reflexio/client/init.py
- reflexio/server/services/storage/error.py
- reflexio/cli/errors.py
- reflexio/server/services/storage/retention.py
- tests/e2e_tests/test_cleanup_storage.py
- reflexio/server/services/playbook/playbook_service_utils.py
- tests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.py
- tests/lib/test_config_unit.py
- reflexio/server/tracing.py
- reflexio/server/services/storage/storage_base/profiles/_interaction_store.py
- reflexio/lib/_base.py
- tests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.py
- reflexio/lib/generation_client.py
- tests/server/services/storage/test_sqlite_surface.py
- reflexio/server/services/configurator/base_configurator.py
- tests/server/services/playbook/components/test_aggregator_effect_scope.py
- reflexio/cli/utils.py
- tests/e2e_tests/test_knowledge_gap_real_llm.py
- tests/server/services/test_generation_service_scheduling.py
- tests/server/services/playbook/test_playbook_aggregator.py
- tests/e2e_tests/conftest.py
- reflexio/server/services/governance/service.py
- reflexio/server/services/storage/retention_mixin.py
- reflexio/integrations/openclaw/plugin/tests/test_publish.py
- reflexio/server/services/extractor_interaction_utils.py
- reflexio/server/services/generation_service.py
- tests/e2e_tests/test_complete_workflows.py
- reflexio/lib/_config.py
- reflexio/server/prompt/prompt_manager.py
- tests/cli/test_utils.py
- tests/server/services/playbook_optimizer/test_optimizer_supersede_integration.py
- reflexio/client/client.py
- reflexio/server/services/playbook_optimizer/gepa_publication.py
- tests/server/api_endpoints/test_api_routes.py
- reflexio/models/api_schema/domain/entities.py
- reflexio/server/routes/config.py
- tests/server/services/playbook_optimizer/test_playbook_optimizer.py
- reflexio/server/services/storage/storage_base/playbook/_optimization.py
- reflexio/server/services/playbook/components/aggregator.py
- docs/playbook_optimizer_assistant_backends.md
- tests/e2e_tests/test_resumable_extraction_e2e.py
Comment on lines
+140
to
+152
| successor = MagicMock() | ||
| successor.model_dump.return_value = {"user_playbook_id": 77} | ||
| response = MagicMock() | ||
| response.model_dump.return_value = { | ||
| "success": True, | ||
| "profiles": [], | ||
| "agent_playbooks": [], | ||
| "user_playbooks": [{"user_playbook_id": 77}], | ||
| } | ||
| response.profiles = [] | ||
| response.agent_playbooks = [] | ||
| response.user_playbooks = [successor] | ||
| mock_client.search.return_value = response |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid mocking away the serialization path under test.
Because response.model_dump.return_value is already populated with {"user_playbook_id": 77}, this test can pass even if the CLI ignores response.user_playbooks and never serializes successor. Construct the actual response model, or make the top-level dump derive from response.user_playbooks, so the test fails when successor IDs are dropped.
🤖 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/cli/test_commands.py` around lines 140 - 152, The test setup around
mock_client.search must exercise serialization of response.user_playbooks
instead of hard-coding the serialized output. Construct the actual response
model, or configure response.model_dump to derive from response.user_playbooks,
so successor.model_dump contributes user_playbook_id 77 and the test detects
dropped successor IDs.
Comment on lines
+318
to
+320
| def complete(self, result: dict[str, Any]) -> None: | ||
| assert self.active | ||
| self.completed_result = result |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert completion after the effect scope exits.
The managed lifecycle is post-scope completion, but Line 319 requires complete() while apply_scope() is still active. This codifies the reverse ordering.
Proposed fix
def complete(self, result: dict[str, Any]) -> None:
- assert self.active
+ assert not self.active
self.completed_result = result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def complete(self, result: dict[str, Any]) -> None: | |
| assert self.active | |
| self.completed_result = result | |
| def complete(self, result: dict[str, Any]) -> None: | |
| assert not self.active | |
| self.completed_result = result |
🤖 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/server/services/playbook/test_aggregation_lineage_integration.py`
around lines 318 - 320, Update the test helper’s complete method in the
lifecycle fixture so completion is asserted only after apply_scope() exits,
matching the managed post-scope completion order. Remove the active-scope
assertion from complete() while preserving storage of completed_result, and
place any lifecycle assertion at the scope boundary that verifies completion
occurs afterward.
Harden publication, governance, optimizer recovery, SQLite migration, configuration conflict handling, and cleanup behavior found during PR review. Add focused regressions for the repaired contracts.
guangyu-reflexio
force-pushed
the
codex/offline-tuner-credential-tests
branch
from
July 28, 2026 21:59
3636fe8 to
2655f57
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
reflexio/server/services/playbook_optimizer/optimizer.py (1)
175-179: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
prompt_manageris defaulted toNoneand then dereferenced.Still unresolved: for
user_playbooktargets this flows intobuild_pairwise_judge_request_plan(prompt_manager=...)→prompt_manager.get_prompt_template_identity(...), raisingAttributeErroronNoneoutside thetrythat guards_run_gepa.🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 175 - 179, Update the prompt_manager selection in the optimizer flow so user_playbook targets always receive a valid prompt manager before build_pairwise_judge_request_plan dereferences it. Validate the request context dependency and handle a missing manager through the existing error path rather than passing None; keep prompt_manager unset for non-user_playbook targets.
🧹 Nitpick comments (6)
reflexio/lib/_base.py (1)
31-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one generation-client builder.
reflexio/lib/generation_client.pyalready defines this same helper. Remove one implementation and import the canonical helper so model/config resolution cannot drift between entry points.🤖 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 `@reflexio/lib/_base.py` around lines 31 - 53, Remove the duplicate create_generation_litellm_client implementation from _base.py and import the canonical helper from generation_client.py. Update callers in _base.py to use that imported symbol, preserving the existing model and configuration resolution behavior through the single shared builder.reflexio/server/services/playbook_optimizer/optimizer.py (2)
342-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
incumbent_changedpublications log as committed and leave the job row untouched.When
publication_result.outcome == "incumbent_changed",successor_idisNone, the run still emitsevent=playbook_optimization_committed, and noupdate_playbook_optimization_jobcall happens on the user-playbook path — so the durable job'sstatus/decision_reasondepend entirely on the storage commit. Consider logging the outcome explicitly so operators can distinguish an applied successor from a lost race.🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 342 - 364, Update the publication handling around the committed-event logger and update_playbook_optimization_job so publication_result.outcome == "incumbent_changed" is logged explicitly as a lost race rather than committed. Ensure the durable job is updated on the user-playbook path with the appropriate completed status and decision_reason, while preserving the applied-successor behavior for other outcomes.
707-738: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBare
except Exceptionfallback can mask pre-commit failures.
load_committed(request.job_id)correctly recovers the lost-response case, but the same handler also swallows verification/staging errors whenever an unrelated committed result already exists for the job. Consider logging the suppressed exception before falling back so the failure isn't silent.🤖 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 `@reflexio/server/services/playbook_optimizer/optimizer.py` around lines 707 - 738, The exception handler in _publish_prepared_user_playbook_request currently suppresses all publication failures when load_committed finds an existing result. Log the caught exception before assigning the committed result, while preserving the existing re-raise behavior when no committed result exists.reflexio/server/routes/config.py (1)
171-179: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant config read.
configurator.prepare_config_patch(partial)re-reads the config internally (base_configurator.pyLines 87-99), so the route performs twoget_config()calls per PATCH.existingis only needed for the no-op comparison; consider havingprepare_config_patchreturn or accept the already-loaded config if this path is hot.🤖 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 `@reflexio/server/routes/config.py` around lines 171 - 179, Remove the redundant config read in the PATCH route around configurator.prepare_config_patch and existing_config. Reuse the already-loaded configuration by updating prepare_config_patch to accept it, or have it return the data needed for the no-op comparison, while preserving the existing merge and comparison behavior. Ensure the route performs only one get_config call per PATCH.reflexio/models/api_schema/domain/entities.py (1)
467-505: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared SHA-256 hex predicate.
The same
len(value) != 64 or any(char not in "0123456789abcdef" ...)check now exists inPlaybookOptimizationJob.validate_sha256_digest,PlaybookOptimizationArtifact.validate_content_digest, and_validate_gepa_publication_prepare_metadatainreflexio/server/services/storage/sqlite_storage/playbook/_optimization.py. A single helper next toSha256Digestwould keep the three in sync.🤖 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 `@reflexio/models/api_schema/domain/entities.py` around lines 467 - 505, Introduce a shared lowercase SHA-256 hex validation predicate near the Sha256Digest definition, then update PlaybookOptimizationJob.validate_sha256_digest, PlaybookOptimizationArtifact.validate_content_digest, and _validate_gepa_publication_prepare_metadata to reuse it instead of duplicating the length and character checks. Preserve each method’s existing error behavior and optional-value handling.reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py (1)
297-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
prepare_gepa_user_playbook_publicationhardcodesself._lease_now(None)and an unconditionalBEGIN IMMEDIATE.Every sibling lease method exposes
now: int | None = Nonefor deterministic tests, andcreate_or_get_playbook_optimization_job/upsert_playbook_optimization_artifactguardBEGIN/commitwithself._own_transaction(). As written this method cannot be called from inside acommit_scope()(SQLite raises "cannot start a transaction within a transaction") and its lease clock cannot be pinned. Worth aligning for consistency and future nesting.🤖 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 `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py` around lines 297 - 311, The prepare_gepa_user_playbook_publication method should accept a now: int | None = None parameter, pass it to _lease_now, and replace the unconditional BEGIN IMMEDIATE with the established _own_transaction() guard used by sibling methods. Preserve the existing publication preparation behavior while allowing deterministic lease timing and invocation inside commit_scope().
🤖 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 `@reflexio/server/llm/_litellm_text_generation.py`:
- Around line 427-428: Update generate_chat_response_with_provenance to accept
provider_request_guard, then forward that parameter from generate_chat_response
when delegating to the provenance wrapper. Keep the existing kwargs forwarding
block there so the guard reaches the downstream generation call without raising
NameError or being dropped.
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1332-1347: Make _classify_legacy_playbook_optimization_jobs a
one-shot migration by adding a persistent completion guard, such as the
schema-version or marker mechanism used by other migrations in this file. Run
the legacy-row classification, index retirement, and index recreation only when
the guard indicates the migration is pending, then record completion after
success; preserve current pending/running jobs and avoid repeating the sweep on
later startups.
---
Duplicate comments:
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 175-179: Update the prompt_manager selection in the optimizer flow
so user_playbook targets always receive a valid prompt manager before
build_pairwise_judge_request_plan dereferences it. Validate the request context
dependency and handle a missing manager through the existing error path rather
than passing None; keep prompt_manager unset for non-user_playbook targets.
---
Nitpick comments:
In `@reflexio/lib/_base.py`:
- Around line 31-53: Remove the duplicate create_generation_litellm_client
implementation from _base.py and import the canonical helper from
generation_client.py. Update callers in _base.py to use that imported symbol,
preserving the existing model and configuration resolution behavior through the
single shared builder.
In `@reflexio/models/api_schema/domain/entities.py`:
- Around line 467-505: Introduce a shared lowercase SHA-256 hex validation
predicate near the Sha256Digest definition, then update
PlaybookOptimizationJob.validate_sha256_digest,
PlaybookOptimizationArtifact.validate_content_digest, and
_validate_gepa_publication_prepare_metadata to reuse it instead of duplicating
the length and character checks. Preserve each method’s existing error behavior
and optional-value handling.
In `@reflexio/server/routes/config.py`:
- Around line 171-179: Remove the redundant config read in the PATCH route
around configurator.prepare_config_patch and existing_config. Reuse the
already-loaded configuration by updating prepare_config_patch to accept it, or
have it return the data needed for the no-op comparison, while preserving the
existing merge and comparison behavior. Ensure the route performs only one
get_config call per PATCH.
In `@reflexio/server/services/playbook_optimizer/optimizer.py`:
- Around line 342-364: Update the publication handling around the
committed-event logger and update_playbook_optimization_job so
publication_result.outcome == "incumbent_changed" is logged explicitly as a lost
race rather than committed. Ensure the durable job is updated on the
user-playbook path with the appropriate completed status and decision_reason,
while preserving the applied-successor behavior for other outcomes.
- Around line 707-738: The exception handler in
_publish_prepared_user_playbook_request currently suppresses all publication
failures when load_committed finds an existing result. Log the caught exception
before assigning the committed result, while preserving the existing re-raise
behavior when no committed result exists.
In `@reflexio/server/services/storage/sqlite_storage/playbook/_optimization.py`:
- Around line 297-311: The prepare_gepa_user_playbook_publication method should
accept a now: int | None = None parameter, pass it to _lease_now, and replace
the unconditional BEGIN IMMEDIATE with the established _own_transaction() guard
used by sibling methods. Preserve the existing publication preparation behavior
while allowing deterministic lease timing and invocation inside commit_scope().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0926b4fb-3cca-4421-95f7-c730342b28a9
📒 Files selected for processing (64)
docs/playbook_optimizer_assistant_backends.mdreflexio/cli/commands/status_cmd.pyreflexio/cli/errors.pyreflexio/cli/utils.pyreflexio/client/__init__.pyreflexio/client/client.pyreflexio/integrations/openclaw/plugin/tests/test_publish.pyreflexio/lib/_base.pyreflexio/lib/_config.pyreflexio/lib/generation_client.pyreflexio/models/api_schema/domain/entities.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/prompt/prompt_manager.pyreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/configurator/config_storage.pyreflexio/server/services/extractor_interaction_utils.pyreflexio/server/services/generation_service.pyreflexio/server/services/governance/service.pyreflexio/server/services/playbook/components/aggregator.pyreflexio/server/services/playbook/playbook_edit_apply.pyreflexio/server/services/playbook/playbook_service_utils.pyreflexio/server/services/playbook/publication.pyreflexio/server/services/playbook_optimizer/gepa_publication.pyreflexio/server/services/playbook_optimizer/judge.pyreflexio/server/services/playbook_optimizer/optimizer.pyreflexio/server/services/storage/error.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/retention_mixin.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/playbook/_optimization.pyreflexio/server/services/storage/sqlite_storage/playbook/_user.pyreflexio/server/services/storage/storage_base/playbook/_optimization.pyreflexio/server/services/storage/storage_base/playbook/_user.pyreflexio/server/services/storage/storage_base/profiles/_interaction_store.pyreflexio/server/tracing.pytests/cli/test_commands.pytests/cli/test_utils.pytests/client/test_config_client.pytests/deploy_guard/test_module_layout_contracts.pytests/e2e_tests/conftest.pytests/e2e_tests/test_cleanup_storage.pytests/e2e_tests/test_complete_workflows.pytests/e2e_tests/test_knowledge_gap_real_llm.pytests/e2e_tests/test_resumable_extraction_e2e.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/playbook/components/test_aggregator_effect_scope.pytests/server/services/playbook/test_aggregation_lineage_integration.pytests/server/services/playbook/test_apply_playbook_edit_integration.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/playbook/test_playbook_edit_apply.pytests/server/services/playbook/test_publication_models.pytests/server/services/playbook_optimizer/test_gepa_user_playbook_publication.pytests/server/services/playbook_optimizer/test_judge_frozen_plan.pytests/server/services/playbook_optimizer/test_optimizer_supersede_integration.pytests/server/services/playbook_optimizer/test_playbook_optimizer.pytests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.pytests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.pytests/server/services/storage/test_playbook_optimization_replay_contract_integration.pytests/server/services/storage/test_sqlite_surface.pytests/server/services/storage/test_user_playbook_publication_sqlite.pytests/server/services/test_generation_service_scheduling.py
💤 Files with no reviewable changes (4)
- tests/server/services/playbook/test_playbook_edit_apply.py
- tests/server/services/playbook/test_apply_playbook_edit_integration.py
- tests/deploy_guard/test_module_layout_contracts.py
- reflexio/server/services/playbook/playbook_edit_apply.py
🚧 Files skipped from review as they are similar to previous changes (36)
- reflexio/lib/generation_client.py
- tests/e2e_tests/test_cleanup_storage.py
- reflexio/server/services/storage/retention_mixin.py
- tests/server/services/storage/sqlite_storage/test_playbook_optimization_candidate_metadata_migration.py
- reflexio/client/init.py
- reflexio/server/services/storage/storage_base/profiles/_interaction_store.py
- reflexio/server/services/playbook/playbook_service_utils.py
- tests/server/services/playbook/components/test_aggregator_effect_scope.py
- tests/lib/test_config_unit.py
- tests/cli/test_commands.py
- reflexio/server/services/generation_service.py
- tests/server/services/storage/sqlite_storage/test_playbook_remaining_methods_integration.py
- tests/server/services/test_generation_service_scheduling.py
- reflexio/server/services/configurator/config_storage.py
- reflexio/lib/_config.py
- reflexio/client/client.py
- reflexio/cli/errors.py
- reflexio/server/tracing.py
- reflexio/server/services/configurator/base_configurator.py
- reflexio/server/services/governance/service.py
- tests/e2e_tests/test_knowledge_gap_real_llm.py
- tests/server/services/playbook/test_playbook_aggregator.py
- tests/server/services/playbook_optimizer/test_optimizer_supersede_integration.py
- reflexio/server/services/storage/storage_base/playbook/_user.py
- reflexio/server/services/storage/retention.py
- tests/server/services/playbook_optimizer/test_playbook_optimizer.py
- tests/e2e_tests/conftest.py
- reflexio/server/services/storage/storage_base/playbook/_optimization.py
- reflexio/server/prompt/prompt_manager.py
- tests/e2e_tests/test_resumable_extraction_e2e.py
- docs/playbook_optimizer_assistant_backends.md
- tests/server/api_endpoints/test_api_routes.py
- tests/server/services/storage/test_sqlite_surface.py
- tests/server/services/playbook/test_aggregation_lineage_integration.py
- reflexio/server/services/playbook_optimizer/gepa_publication.py
- tests/cli/test_utils.py
Comment on lines
+427
to
+428
| if provider_request_guard is not None: | ||
| kwargs["provider_request_guard"] = provider_request_guard |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
provider_request_guard is referenced in the wrong function — NameError on every chat call.
The forwarding block landed in generate_chat_response_with_provenance (Line 376), which has no provider_request_guard parameter, so Line 427 raises NameError unconditionally (Ruff F821 confirms). Meanwhile generate_chat_response accepts the guard at Line 322 but never passes it down, so the frozen-plan enforcement would be a no-op even if the reference resolved. This breaks the entire PairwiseJudge.judge path.
Add the parameter to the provenance wrapper and forward it from generate_chat_response.
🐛 Proposed fix
structured_output_validator: StructuredOutputValidator | None = None,
+ provider_request_guard: ProviderRequestGuard | None = None,
**kwargs: Any,
) -> CompletionResult[str | BaseModel | ToolCallingChatResponse]:
"""Generate a chat response paired with observed model provenance."""And in generate_chat_response's delegation (Lines 364-374):
structured_output_validator=structured_output_validator,
+ provider_request_guard=provider_request_guard,
**kwargs,
).value🧰 Tools
🪛 Ruff (0.16.0)
[error] 427-427: Undefined name provider_request_guard
(F821)
[error] 428-428: Undefined name provider_request_guard
(F821)
🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 427 - 428,
Update generate_chat_response_with_provenance to accept provider_request_guard,
then forward that parameter from generate_chat_response when delegating to the
provenance wrapper. Keep the existing kwargs forwarding block there so the guard
reaches the downstream generation call without raising NameError or being
dropped.
Source: Linters/SAST tools
Comment on lines
+1332
to
+1347
| def _classify_legacy_playbook_optimization_jobs(self) -> None: | ||
| """Classify legacy optimizer history without choosing ambiguous rule order.""" | ||
| columns = { | ||
| row["name"] | ||
| for row in self.conn.execute( | ||
| "PRAGMA table_info(playbook_optimization_jobs)" | ||
| ).fetchall() | ||
| } | ||
| if "optimizer_kind" not in columns: | ||
| return | ||
| for index_name in ( | ||
| "uq_poj_active_discovery", | ||
| "uq_poj_active_attempt", | ||
| "uq_poj_active_target", | ||
| ): | ||
| self.conn.execute(f"DROP INDEX IF EXISTS {index_name}") # noqa: S608 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
_classify_legacy_playbook_optimization_jobs re-runs its full retirement sweep on every startup.
There is no completion guard: each boot drops and recreates the three partial unique indexes and re-executes the three table-wide UPDATEs. Two consequences:
- Any currently pending/running job whose
optimizer_kindis the model default'optimizer_legacy_unknown'(seePlaybookOptimizationJob.optimizer_kinddefault) is retired asskippedwithretired_by_replay_redesignon the next restart — not just historical rows. - Index drop/rebuild cost is paid on every process start rather than once.
Consider gating the sweep (e.g. a schema-version/marker row, or skipping when no optimizer_kind IS NULL rows exist and the indexes already exist), so it behaves like the other one-shot migrations in this file.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 1347-1347: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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 `@reflexio/server/services/storage/sqlite_storage/_base.py` around lines 1332 -
1347, Make _classify_legacy_playbook_optimization_jobs a one-shot migration by
adding a persistent completion guard, such as the schema-version or marker
mechanism used by other migrations in this file. Run the legacy-row
classification, index retirement, and index recreation only when the guard
indicates the migration is pending, then record completion after success;
preserve current pending/running jobs and avoid repeating the sweep on later
startups.
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 29, 2026
#385 deleted `tests/server/services/playbook/test_playbook_edit_apply.py` alongside the module it covers. Restoring only the source left `apply_playbook_edit` with no direct unit coverage — its behaviour was exercised only indirectly, through `review_service`. Restored verbatim from 4268ad1^; all 8 tests pass unmodified against current main, which independently confirms the supersede CAS semantics the restored module relies on are unchanged.
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 29, 2026
…ted (#394) ## Problem `main` cannot boot the API. PR #385 (`4268ad1`) deleted `reflexio/server/services/playbook/playbook_edit_apply.py` under the changelog line *"Remove the obsolete split-edit application path."* That module is **not** the split-edit path — the split/differentiate path lives in `playbook_generation_service.py`. `playbook_edit_apply.py` is the shared atomic supersede primitive, and its consumer survived the deletion: ``` review_service.py:33 from ...playbook.playbook_edit_apply import apply_playbook_edit review_service.py:297 successor_id = apply_playbook_edit(...) ``` `apply_playbook_edit` exists nowhere else on `main`, so no replacement was shipped. `review_service.py` is imported by `server/routes/playbooks.py`, which loads at app startup — a guaranteed `ModuleNotFoundError` on boot. ## How it surfaced A production deploy built from a **clean checkout of `main`**. The task started, migrations ran clean, then it failed container health checks on the API port and the ECS deployment circuit breaker rolled the service back. Logs showed 200×: ``` ModuleNotFoundError: No module named 'reflexio.server.services.playbook.playbook_edit_apply' ``` Every earlier build came from a working tree that still had the file on disk, so the image contained a file `origin/main` no longer has. A fresh checkout is the only build that exposes it. ## Fix Restore the file verbatim from `4268ad1^`. Every dependency it uses still exists on `main`: `supersede_record`, `commit_scope`, `save_user_playbooks`, `precompute_user_playbook_embeddings`, and `LineageContext`. This is deliberately the minimal fix. If the intent was genuinely to retire this primitive, the follow-up is to port `review_service.py` onto the publication API — but that is a behavioural change and does not belong in a build-unbreaking commit. ## Verification Import, reproducing the exact production failure and its resolution: | Tree | `import reflexio.server.routes.playbooks` | |---|---| | `origin/main` | `ModuleNotFoundError: ...playbook_edit_apply` | | this branch | OK | Tests (`tests/server/services/playbook`): | Tree | Result | |---|---| | `origin/main` | **collection error** — the entire directory fails to collect | | this branch | 531 passed, 8 failed | The 8 failures are all in `test_playbook_extractor.py` and are **pre-existing** — they fail identically on pristine `main` when run standalone (6 in that file alone), and are untouched by this change, which only adds a file. `ruff check` and `ruff format --check` pass on the restored file. ## Note CI did not catch a change that makes the app unbootable and breaks a whole test directory's collection. That gap is worth a separate look. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added atomic apply-playbook-edit behavior that replaces the current playbook safely while archiving the incumbent. * Preserve provenance and lineage for new revisions, including using the edit’s request correlation. * Added optional embedding generation skipping when applying an edit. * **Bug Fixes** * Improved handling of concurrent updates to prevent conflicting or orphaned successors; non-applicable incumbents now fail cleanly. * Reject empty request identifiers before any write operations. * **Tests** * Added comprehensive storage-backed coverage for success, concurrency, rollback/orphan prevention, and request-id validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 29, 2026
…oints (#395) ## Problem `provider_request_guard` was added by #385 and wired backwards on **both** sides: | Entry point | Defect | |---|---| | `generate_chat_response_with_provenance` | Forwards the name (line 427) **without declaring it** → `NameError` on every call | | `generate_chat_response` | **Declares** the parameter but omits it from its delegation → the caller's guard is silently dropped | The NameError is **live in production right now**. `run_tool_loop` calls the provenance entry point, so the tool loop dies mid-flight, playbook extraction returns no structured output, and it surfaces as: ``` ExtractorExecutionError: Extractor failed for playbook_generation ... Playbook extraction failed without structured output: error (type=RuntimeError) ``` Sentry: `PYTHON-FASTAPI-PW` (the NameError, root cause), with `PYTHON-FASTAPI-PV` and `PYTHON-FASTAPI-NQ` as the downstream extraction failures. **Ruff already caught this on `main`** — `F821 Undefined name 'provider_request_guard'` twice (lines 427, 428) plus `ARG002` for the unused declaration at line 322. The bug was statically detectable before it shipped; nothing gated on the result. ## Fix Two lines: declare the parameter on the provenance entry point, and pass it through in the plain entry point's delegation. ## Verification Reproduced the exact production error against `origin/main`, then confirmed it resolved (run with the repo first on `PYTHONPATH` — importing the installed editable copy instead silently tests the wrong file): | Tree | `generate_chat_response_with_provenance(...)` | |---|---| | `origin/main` | `NameError: name 'provider_request_guard' is not defined` | | this branch | returns normally | - New guard test: **all 5 cases fail on unpatched `main`**, pass here. - `tests/server/llm`: **654 passed**, 61 skipped. - Ruff on this file: **3 errors → 0**. Pyright: 0 errors. The test targets the **class**, not just these two sites: it asserts both entry points declare every forwarded option, and that each forwarded name is a local rather than an accidental global lookup. A future option pasted into one signature and not the other fails here instead of in production. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Preserved the configured provider request safeguards throughout chat response generation, including provenance-enabled flows. - Ensured the safeguard can be passed through without being dropped between the standard and provenance entry points. - **Tests** - Added a new test suite to verify signature parity and correct forwarding behavior for the provider request safeguard, including when it’s omitted or supplied. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Test Plan
2 passedin 19.31s; managed resumable1 passedin 44.14s; strict provider compatibility1 passed, 24 deselectedin 5.03s.Summary by CodeRabbit
New Features
Bug Fixes
Documentation