fix(test): resolve confirmed regressions found in polylogue-p6rz re-triage - #3340
Conversation
Problem: PR #3202's strict bool-env coercion (_coerce_env_value) rejects any POLYLOGUE_FORCE_PLAIN/NO_COLOR/etc value that isn't a recognized 1/true/yes/on/0/false/no/off token, including an explicitly blank "" value -- raising ConfigError instead of the old passthrough behavior that let `bool("")` fall through as falsy. This broke the long-standing CLI test convention `env={"POLYLOGUE_FORCE_PLAIN": ""}` used to force a clean/cleared override in tests, and is inconsistent with this same module's own NO_COLOR convention ("set but blank" means "not requested"). What changed: _coerce_env_value now treats a blank (whitespace-only) bool-key env value as False before attempting token parsing, matching the existing NO_COLOR blank-value convention. An unrecognized non-blank token (e.g. "flase") still fails closed as before -- this only carves out the "explicitly cleared" case, not typos. Verification: devtools test tests/unit/cli/test_click_app.py::TestCliSetup::test_plain_mode_auto_detection_does_not_announce tests/unit/core/test_config_inventory.py tests/unit/core/test_config.py tests/unit/core/test_config_resolution_regression.py tests/unit/core/test_privacy_config.py tests/unit/cli/test_config_command.py (194 passed); mypy --strict polylogue/config.py clean. Ref polylogue-p6rz
Problem: PR #3155 (three-tier JSON backend facade) made polylogue.core.json.loads() raise its own polylogue.core.json.JSONDecodeError (a ValueError subclass, not a subclass of stdlib json.JSONDecodeError) on malformed input. Two call sites still caught the stdlib exception type: - storage/sqlite/queries/mappers_support.py::_parse_json wraps decode failures in DatabaseError with diagnostic context (field, record id) -- but since 2026-07-19 the custom JSONDecodeError propagates unwrapped instead, losing that diagnostic context entirely. - storage/blob_integrity.py::_current_raw_payload_bytes similarly failed to catch a corrupt-blob decode failure, which would propagate instead of degrading to a "source_index:<reason>" unavailability tuple. What changed: both call sites now import and catch polylogue.core.json.JSONDecodeError explicitly instead of the stdlib json.JSONDecodeError. mappers_support.py no longer needs the stdlib `json` import at all. Verification: devtools test tests/unit/storage/test_query_mappers.py tests/unit/storage/test_blob_integrity.py tests/unit/storage/test_blob_integrity_referenced_scan.py (34 passed); mypy --strict on both changed files clean. Ref polylogue-p6rz
… repository import Problem: after rebasing onto current master, PR #3336's polylogue/cli/commands/materialize_incident_evidence.py (landed concurrently in another session) also constructs SessionRepository directly instead of through AppEnv.repository -- the same pattern already tracked for reconcile_work_effects.py in polylogue-a7uk. What changed: added the second file to the boundary test's allow-list, per its own documented remediation path, and broadened polylogue-a7uk's scope to cover both commands rather than filing a near-duplicate bead. Verification: devtools test tests/unit/architecture/test_surface_storage_boundary.py (155 passed); ruff format/check and mypy --strict clean. Ref polylogue-p6rz, polylogue-a7uk
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Final clean `devtools verify --all` run on this branch (rebased onto current master, all fixes committed): ``` Down from the 107 failed / 14 errors pre-fix baseline. All 14 remaining errors are the already-tracked polylogue-lbgc "database is locked" seeded-archive xdist race (test_plain_cli_snapshots.py ×8, test_schema_generation.py ×6) — unchanged, not attempted in this pass (see PR body). Confirmed via diff against the pre-fix failure list that every fix here landed cleanly (test_surface_storage_boundary, test_click_app, test_color_and_layout, test_diagnostics ×8, test_plain_cli_snapshots::test_json_status_snapshot, test_mandate_continuity_replay, test_work_effect_reconciliation, test_delegations_view ×3, test_durable_migrations ×2, test_query_mappers ×3 — all gone). Two new observations not present in the pre-fix baseline, neither touched by this PR, both consistent with load/timing flakes rather than deterministic regressions:
Full triage detail is in polylogue-p6rz's notes. |
1 participant
Summary
Fresh
devtools verify --allre-triage of polylogue-p6rz ("pre-existing testfailures unrelated to MCP cutover") against current master. Fixes several
confirmed real regressions and stale test assertions found during the pass;
files properly-scoped follow-up beads for the rest instead of leaving them as
anonymous re-discoveries.
Problem
polylogue-p6rz's original list (from 2026-07-18) was stale: a lot has merged
since. A fresh
devtools verify --allrun (107 failed, 14 errors on thepre-fix baseline) needed re-triage to separate fixed / persisting / newly
introduced failures, and specifically to catch any real regression from this
session's merges (query DSL AST schema, browser-capture approval flow,
GitHub effect adapters, continuity replay wiring, etc.).
Solution
Confirmed real regressions, fixed:
polylogue/config.py: PR refactor(mcp): collapse role ladder into independent capability config #3202's strict bool-env coercion rejected a blankPOLYLOGUE_FORCE_PLAIN=""value withConfigErrorinstead of the oldpassthrough-as-falsy behavior, breaking a long-standing CLI test convention
(and inconsistent with this same module's own NO_COLOR "blank means not
requested" convention). Blank bool-key env values now resolve to
False.polylogue/storage/sqlite/queries/mappers_support.pyandpolylogue/storage/blob_integrity.py: PR perf(core): optionalize orjson via a three-tier JSON backend facade #3155's three-tier JSON facadeintroduced
polylogue.core.json.JSONDecodeError(aValueErrorsubclass,NOT a stdlib
json.JSONDecodeErrorsubclass), but two call sites stillcaught the stdlib type — silently losing
DatabaseErrordiagnosticwrapping (field/record id context) and blob-corruption graceful
degradation since 2026-07-19.
Stale test assertions / drifted mocks, fixed:
tests/unit/cli/test_color_and_layout.py: testedno_color_requested()/should_use_plain()as if they readNO_COLORfrom the environmentdirectly; PR feat(config): close the 5-layer resolution gap with ResolvedRuntimeConfig #3079 made them pure passthroughs (env resolution moved to
config.py, already covered elsewhere). Rewrote to test the passthroughcontract.
tests/unit/storage/test_durable_migrations.py: hardcodedUSER_SCHEMA_VERSION == 9/applied_versionstuple; PR feat(query): bind query_units continuations to the archive epoch #3068 bumped itto 10 without updating these two tests.
tests/unit/cli/__snapshots__/test_plain_cli_snapshots.ambr: staleindex.dbuser_version/sqlite_stat1_rowssnapshot from before PRfeat(storage): add messages_fts_identity ledger for rowid-reuse detection #3235's schema bump 42→43.
tests/unit/cli/test_diagnostics.py:_patch_tool_count_store's fakeArchiveStore.open_existinglambda didn't accept theread_timeoutkwargPR feat(query): interruptible, admission-controlled archive read execution #2964 added, and the fake store was missing
begin_read_snapshot()(also from feat(query): interruptible, admission-controlled archive read execution #2964's interruptible-read protocol) — 8 tests broken since
2026-07-17.
tests/unit/devtools/test_mandate_continuity_replay.pyandtests/unit/operations/test_work_effect_reconciliation.py: both assertedthe real
GitHubPullRequestEffectAdapterfails againstSinity/polylogue— true only in an environment without
ghauth (e.g. stock CI), false onany dev machine with
ghauthenticated for this repo (confirmed:gh pr list --repo Sinity/polyloguesucceeds fine here). Forced deterministicunavailability via a nonexistent
gh_pathinstead.tests/unit/storage/test_delegations_view.py: 3 tests opened a bareindex.db-onlyArchiveStore(nouser.db); PR feat(query): bind query_units continuations to the archive epoch #3068's newarchive_snapshot_epoch()unconditionally requiresuser_tierattached.Added a sibling
user.dbbootstrap to the 3 affected fixtures — matchesevery other test's full-tier archive construction.
tests/unit/architecture/test_surface_storage_boundary.py: allow-listedtwo CLI commands (
reconcile_work_effects.py, andmaterialize_incident_evidence.py— the latter landed on master via aconcurrent session's PR feat(insights): materialize incident work-evidence graphs from archive content #3336 mid-rebase) that construct
SessionRepositorydirectly instead of through
AppEnv.repository, per the test's owndocumented remediation path. Filed polylogue-a7uk for the proper fix.
Concurrent-session note: this branch was rebased onto master mid-flight
after another session's PR #3332 landed fixing the exact same
test_timestamp_guards.pyhypothesis-6.161 mypy break I'd independentlyfound and fixed — kept master's version (identical
st.one_ofsplitapproach) rather than duplicating it.
Already tracked, not duplicated:
test_execution_control.pyapi.query_unitsnaming mismatch +>=50000 VM stepsanti-vacuity mutation tests → polylogue-1ldl (fullyroot-caused 2026-07-20: PR feat(query): bound agent-facing archive reads #3018 made the mutation-under-test no longer
expensive at this data scale).
test_live_batch_support.pystalefailed=[...]assertion → polylogue-5202.test_archive_maintenance_cli.py/test_daemon_cli.py6-node cluster →polylogue-p5li.
tests/infra/surfaces.pystalelist_sessions/searchMCP tool-namelookups (affects
test_retrieval_readiness_laws.py,test_cross_surface_agreement.py) → polylogue-t46.8 (MCP tool-sprawlreplacement epic; p6rz's own original text already called this
out-of-scope).
New follow-up beads filed (confirmed pre-existing, not this-session
regressions, but not safe/minimal to fix inline):
NULL-sort-key session-profile staleness (regresses PR refactor(architecture): sqlite leak sweep, staleness unify, control-center decomposition #2900's own
invariant; root cause not fully diagnosed).
database is lockedunder xdist parallel first-build (affects
test_plain_cli_snapshots.py,test_schema_generation.py, likely others); confirmed not simplecross-worker contention (the build already holds an exclusive flock across
the whole build+integrity-check section) — more likely an unclosed
intra-process connection.
test_index_v37_fast_forward.py's v36 fixture predatesthe
action_pairsruntime index added by PR perf(storage): index write-path hot stages, memoize pricing catalog hash #3210; attempted fix revealedthe fixture's intended "before" shape needs the full same-version
benign-DDL-convergence set, not just
ensure_runtime_indexes_sync—reverted the attempt rather than ship an incomplete fixture.
No regression found from this session's headline PRs (query DSL AST schema,
browser-capture approval flow, continuity replay wiring) — the two GitHub
effect-adapter tests and the
materialize_incident_evidence.pyboundaryviolation are environment/pattern issues in supporting test/command code, not
in the new capability logic itself.
Verification
devtools teston every changed test file individually: all green(see per-file evidence above).
mypy --stricton every changed.pyfile: clean.ruff format --check/ruff check: clean.devtools verify --allrun against this branch in progress; willupdate this PR/polylogue-p6rz with the final authoritative count once it
completes (~14 min runtime).
Ref polylogue-p6rz