fix(daemon): persist judgment scheduler receipts (#3896) - #3896
Conversation
📝 WalkthroughWalkthroughJudgment automation now persists correlated parked, completed, and failed outcome receipts. Queue health reads these receipts to classify pending work and expose freshness metadata. Daemon and CLI status output render the receipt details and new queue states. Tests cover scheduler, persistence, projection, configuration, and rendering behavior. ChangesJudgment scheduler receipts
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant JudgmentScheduler
participant ReceiptOutbox
participant DaemonEvents
participant OpsDatabase
participant QueueHealth
participant StatusCLI
JudgmentScheduler->>ReceiptOutbox: persist receipt marker
ReceiptOutbox-->>JudgmentScheduler: return marker
JudgmentScheduler->>DaemonEvents: persist outcome receipt
DaemonEvents->>OpsDatabase: write judgment-automation event
QueueHealth->>OpsDatabase: read latest receipt
OpsDatabase-->>QueueHealth: return receipt metadata
QueueHealth-->>StatusCLI: return queue state and receipt fields
StatusCLI-->>StatusCLI: render scheduler status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Sinity
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da091bac03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| receipt_reason = queue.get("judgment_scheduler_receipt_reason") | ||
| suffix = f" ({receipt_reason})" if receipt_reason else "" | ||
| env.ui.console.print(f" judgment scheduler receipt: {receipt_status}{suffix}") |
There was a problem hiding this comment.
When polylogue status receives a judgment-scheduler receipt, this renderer prints only its status and reason, even though the queue-health payload supplies judgment_scheduler_receipt_at_ms and judgment_scheduler_receipt_age_ms. Operators therefore cannot tell from the CLI whether the displayed receipt is recent or stale, leaving the tracked requirement to expose state, timestamp, age, and reason through CLI status only partially implemented. Render the receipt time and/or age alongside the existing text.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied at the starting head. The renderer includes receipt timestamp and age, and the CLI regression verifies both fields.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The CLI renderer already includes receipt timestamp and age, and the production CLI regression asserts both fields.
| _record_judgment_automation_receipt( | ||
| root, | ||
| status="parked", | ||
| reason="capability_gate_disabled", | ||
| now_ms=None, |
There was a problem hiding this comment.
When judgment automation is disabled—the default configuration—this synchronous call writes ops.db directly from the async loop instead of using daemon_write_coordinator().run_sync(...) like the enabled sweep. If another ingest or maintenance writer is active, the event-loop thread can block waiting on SQLite, and _record_judgment_automation_receipt then swallows any write failure and drops the parked receipt. Route this write through the coordinator so it neither races daemon writers nor stalls unrelated async work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied at the starting head. Parked receipt writes run through the daemon write coordinator, preserving serialized ops-tier ownership.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Parked receipt writes run through the daemon write coordinator, preserving serialized ops-tier ownership.
| FROM daemon_events | ||
| WHERE kind = 'judgment-automation' | ||
| ORDER BY ts_ms DESC, rowid DESC | ||
| LIMIT 1 |
There was a problem hiding this comment.
If the system clock moves backward between scheduler attempts, a newly inserted receipt can have a lower ts_ms than the preceding receipt, so this query continues projecting the older state even though the event ledger contains a newer outcome. The new get_latest_daemon_event helper already defines latest by monotonically increasing id; use the same ordering here so a later completed or parked transition cannot remain hidden behind an earlier failed receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
False positive against the current code. Queue health selects ORDER BY id DESC, and the reversed-clock regression proves ledger order wins over wall-clock order.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Queue health selects the latest daemon event by ledger id, and the reversed-clock regression proves it does not use wall-clock ordering.
| judgment_receipt_fresh = ( | ||
| judgment_scheduler_receipt_status == "completed" | ||
| and judgment_receipt_age_ms is not None | ||
| and judgment_receipt_age_ms <= 24 * 60 * 60 * 1000 |
There was a problem hiding this comment.
When judgment_automation_interval_s is configured above 24 hours, a successful receipt becomes stale at this fixed cutoff before the next scheduled sweep is due. Any pending candidate during that interval is therefore reported as scheduler-stalled even though the scheduler is operating exactly on its configured cadence. Base the freshness window on the configured interval, with an appropriate grace period, rather than an unconditional 24-hour limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied at the starting head. Queue health consumes the resolved Config interval, and the explicit-runtime regression verifies that authority.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Queue health consumes the resolved Config interval, including intervals longer than 24 hours, through the explicit-runtime regression.
| record_receipt( | ||
| status="failed" if result.failed else "completed", | ||
| reason="candidate_judgment_failures" if result.failed else "sweep_completed", | ||
| now_ms=now_ms, | ||
| result=result, | ||
| retryable=bool(result.failed), | ||
| ) |
There was a problem hiding this comment.
If the user-tier sweep succeeds but writing its ops-tier receipt fails—for example because ops.db is temporarily locked or unavailable—record_receipt returns False, this result is ignored, and the sweep returns normally. The periodic caller consequently never enters its fallback handlers and the enabled attempt emits no receipt at all, contrary to the tracked exactly-one-receipt contract. Propagate the failed receipt outcome to the periodic caller or explicitly retry it before treating the sweep as successful.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. A failed enabled receipt outcome reaches the periodic fallback through the receipt context and preserves the operation id.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The receipt context preserves the committed operation identity and result metadata when the first receipt write fails, so fallback cannot replace it with an unrelated outcome.
| if not (cfg.judgment_automation_enabled and cfg.mcp_judge_enabled): | ||
| _record_judgment_automation_receipt( | ||
| root, | ||
| status="parked", | ||
| reason="capability_gate_disabled", |
There was a problem hiding this comment.
Because cfg is loaded before the interval sleep, enabling both capability flags while the loop is asleep still causes this next tick to persist capability_gate_disabled from the stale snapshot. Queue health can then report the scheduler as parked for an entire additional configured interval even though the current configuration enables it. Reload the configuration after waking, before deciding whether to emit the parked receipt or run the sweep.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. The post-sleep configuration reload controls the gate and batch settings for the current tick.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The post-sleep configuration snapshot controls the capability gate and batch settings for the current tick.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@polylogue/api/archive.py`:
- Around line 1716-1735: Update _archive_assertion_candidate_queue_health to
load the Polylogue configuration only when a scheduler receipt exists and
pending candidates require the configured interval. Guard
load_polylogue_config() so ConfigError and other configuration-load failures
preserve the existing unavailable/caveat health-report contract instead of
propagating to status or daemon callers.
- Around line 292-295: Update _archive_assertion_candidate_queue_health to use
JUDGMENT_AUTOMATION_SWEEP_INTERVAL_FLOOR_SECONDS imported from
polylogue.daemon.judgment_automation, matching
periodic_judgment_automation_sweep. Remove the duplicate
_JUDGMENT_AUTOMATION_INTERVAL_FLOOR_SECONDS constant and retain the existing
receipt grace constants.
In `@polylogue/cli/commands/status.py`:
- Around line 2490-2492: Update the receipt age formatting in the status
rendering flow around receipt_age_ms so the displayed unit scales with
magnitude, using seconds for smaller ages and days for large ages such as
multi-day scheduler-stalled receipts. Preserve the existing numeric conversion
and ensure the 90,000 ms case continues to render as age=90.0s.
In `@polylogue/daemon/judgment_automation.py`:
- Around line 543-554: The fallback receipt handlers must not discard a failed
_record_judgment_automation_receipt result. In
polylogue/daemon/judgment_automation.py lines 543-554 and 563-578, capture each
coordinator.run_sync result and raise when it is False; preserve successful
receipt handling and existing exception-specific behavior.
In `@tests/unit/api/test_assertion_candidate_queue_health.py`:
- Around line 237-263: The test coverage currently exercises only a valid failed
scheduler receipt; add a regression test for the malformed-receipt path in
_archive_assertion_candidate_queue_health. Insert a non-JSON payload_json
directly into daemon_events, run the health projection, and assert
judgment_scheduler_receipt_status remains unknown, the caveats include “latest
judgment scheduler receipt is malformed,” and the resulting state is
parked-pending.
In `@tests/unit/cli/test_status.py`:
- Around line 130-152: Update
test_status_renderer_includes_judgment_scheduler_receipt_time_and_age to assert
the scheduler-stalled rendered label uses red markup. Add a companion test for
_render_assertion_candidate_queue with state "parked-pending", asserting its
rendered label is red and its receipt reason is preserved.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: d3a34e19-c6ef-43ef-a86a-31bc681b0817
📒 Files selected for processing (9)
.beads/issues.jsonlpolylogue/api/archive.pypolylogue/cli/commands/status.pypolylogue/daemon/events.pypolylogue/daemon/judgment_automation.pypolylogue/surfaces/payloads.pytests/unit/api/test_assertion_candidate_queue_health.pytests/unit/cli/test_status.pytests/unit/daemon/test_judgment_automation.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d20d92bb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -305,14 +436,37 @@ def run_judgment_automation_sweep_once( | |||
| _write_escalation_handoff(conn, candidates_by_ref[ref], decisions[ref], now_ms=now_ms) | |||
| conn.commit() | |||
There was a problem hiding this comment.
When the ops-tier receipt insertion fails after a batch has accepted or rejected candidates, this commit() has already made the user-tier changes durable. The subsequent _JudgmentAutomationReceiptPersistenceError rollback cannot undo them, while the periodic fallback emits a retryable failed/receipt_persistence_failed receipt without the successful counts; the next tick then finds those candidates no longer pending. Preserve the committed result for the fallback receipt or introduce an explicit cross-tier protocol rather than treating a post-commit receipt failure as rollbackable.
AGENTS.md reference: AGENTS.md:L114-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair and strengthened in 174fa3b81955efd279e61ee2fd5cc0c3f7471258. Receipt-persistence fallback retains committed result counts and degraded authority.
There was a problem hiding this comment.
Disposition: satisfied and strengthened in 5879f57. A committed success remains authoritative after acknowledgement failure, and recovery keeps the marker until the matching valid event is durable.
| configured_interval_s = max( | ||
| load_polylogue_config().judgment_automation_interval_s, | ||
| _JUDGMENT_AUTOMATION_INTERVAL_FLOOR_SECONDS, |
There was a problem hiding this comment.
When a caller constructs Polylogue(runtime=...) from an explicitly resolved environment whose judgment interval differs from the process-global configuration, this reload ignores that runtime snapshot and computes freshness from the ambient interval. For example, a runtime configured for 48-hour sweeps can be reported as scheduler-stalled after two hours if the ambient default remains one hour. This global reload is fresh evidence that the earlier configured-interval issue remains for supported explicit-runtime callers; retain the interval in the facade's resolved configuration and use that value here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. Explicit runtime construction projects its interval into Config, and queue health reads that projection.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Explicit runtime construction projects its interval into Config, and queue health reads that projection.
| if not root.exists(): | ||
| continue | ||
| if not (cfg.judgment_automation_enabled and cfg.mcp_judge_enabled): | ||
| await coordinator.run_sync( |
There was a problem hiding this comment.
When the capability gate is disabled and this ops-tier receipt write raises—for example because ops.db encounters a transient I/O or SQLite error—the await is outside the loop's exception handling. run_daemon_services includes this maintenance task in its asyncio.gather, so the exception initiates full daemon shutdown even though judgment automation is disabled by default; ingestion and serving remain unavailable until restart. Catch this telemetry failure and retry on a later tick rather than allowing a disposable ops-tier write to terminate the daemon.
AGENTS.md reference: AGENTS.md:L121-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. Parked receipt persistence is inside the periodic loop handling, so telemetry failure does not terminate daemon services.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Parked receipt persistence is inside the periodic loop handling, so telemetry failure does not terminate daemon services.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348aa5d0a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| now_ms=None, | ||
| batch_limit=cfg.judgment_automation_batch_limit, | ||
| retryable=True, | ||
| suppress_identical_for_ms=JUDGMENT_AUTOMATION_PARKED_RECEIPT_FRESHNESS_MS, |
There was a problem hiding this comment.
With the default 3,600-second sweep interval, the next disabled tick always arrives after this fixed five-minute suppression window, so every tick still appends an identical capability_gate_disabled receipt—the exact default-deployment ledger growth this coalescing is meant to prevent. Derive the suppression window from the effective configured interval (with bounded grace) so at least the next identical tick can be coalesced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. Parked coalescing uses the effective scheduler interval plus bounded grace, including the default one-hour cadence.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Parked coalescing uses the effective scheduler interval plus bounded grace, including the default one-hour cadence.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97c7ce0daf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except _JudgmentAutomationReceiptPersistenceError as receipt_exc: | ||
| raise receipt_exc from exc |
There was a problem hiding this comment.
When the sweep itself raises and the attempt to record its detailed sweep_failed:<Exception> receipt also raises, this rethrows a persistence error whose status and reason are unset. The periodic fallback consequently records only receipt_persistence_failed, masking the underlying sweep failure in the operator-facing ledger even if the fallback write succeeds. Carry the original failed status and reason into the persistence exception so the fallback preserves the detailed outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. Persistence errors carry the attempted failed status and detailed sweep reason into the fallback receipt.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Persistence errors carry the attempted failed status and detailed sweep reason into the fallback receipt.
| else: | ||
| if recorded is not True: | ||
| logger.warning("judgment_automation: parked receipt was not persisted; retrying next tick") |
There was a problem hiding this comment.
On capability-disabled ticks inside the coalescing horizon, _record_judgment_automation_receipt intentionally returns False without writing a duplicate, but this branch interprets that result as a persistence failure and emits a warning. Default disabled deployments therefore produce recurring false failure warnings during normal coalescing, obscuring genuine ops-tier write failures; return a distinct coalesced outcome or treat this expected False result as success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. The typed receipt outcome distinguishes persisted, coalesced, and failed writes, so normal coalescing is not warned as failure.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The typed receipt outcome distinguishes persisted, coalesced, and failed writes, so normal coalescing is not warned as failure.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
polylogue/polylogue/daemon/status.py
Line 2796 in 02c5064
Reuse the periodic registry for queue-health scans
When the periodic daemon status snapshot passes its process-wide persistent registry, this code still creates a fresh StatusComponentRegistry for the queue scan on every refresh. On a large user.db where the multiple assertion aggregates exceed the three-second deadline, the unfinished worker cannot be cancelled, its result is discarded, and the ten-second snapshot cadence repeatedly starts additional scans; sufficiently slow scans overlap and accumulate reader threads indefinitely. Add this collector to the persistent registry (or otherwise retain its in-flight snapshot) instead of constructing an ephemeral registry here.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(oldest_age, int | float): | ||
| line += f", oldest={float(oldest_age) / (24 * 60 * 60 * 1000):.1f}d" | ||
| env.ui.console.print(line) | ||
| receipt_status = queue.get("judgment_scheduler_receipt_status") |
There was a problem hiding this comment.
When the daemon is running—the normal polylogue status path—the response contains assertion_candidate_queue, but _show_daemon_status never calls this renderer; it is invoked only by the direct SQLite fallback. Consequently operators still see no receipt state, timestamp, age, or reason while the daemon is reachable, and the focused tests that call the renderer directly do not exercise that production route. Fresh evidence beyond the earlier renderer-field comment is the missing call in the daemon-backed status path; render the queue from _show_daemon_status as well.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. The daemon-backed status route renders assertion-candidate queue health from its response payload.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The daemon-backed status route renders assertion-candidate queue health from its response payload.
| status="completed", | ||
| reason="queue_empty", | ||
| now_ms=now_ms, | ||
| result=JudgmentAutomationSweepResult(), |
There was a problem hiding this comment.
When an enabled sweep finds no candidates and its first ops-tier receipt write fails, passing an all-zero result here causes _record_judgment_automation_receipt and the wrapper to infer user_tier_committed=True, even though this branch returns before any user-tier write or commit. The periodic fallback therefore replaces the exact queue_empty reason with receipt_persistence_degraded and sets the degraded flag, falsely reporting a durable user-tier commit; track whether a commit actually occurred separately from whether a result object exists.
AGENTS.md reference: AGENTS.md:L114-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already satisfied before the final repair. Queue-empty persistence carries explicit user-tier commit state, preserving completed and queue_empty on fallback.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Queue-empty persistence carries explicit user-tier commit state, preserving the queue_empty outcome on fallback.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 022f28fbf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path = (archive_root() if archive_root_path is None else archive_root_path) / "ops.db" | ||
| conn = _ensure_events_db(path) |
There was a problem hiding this comment.
When archive_root_path is omitted, this bypasses _events_db_path() and writes directly to archive_root()/ops.db, while all existing query helpers still resolve their ledger through _events_db_path(). Any isolated or overridden ledger therefore splits readers from writers; for example, the empty_events_db fixture in tests/unit/daemon/test_daemon_events_endpoint.py overrides _events_db_path, so calls to emit_daemon_event() now write elsewhere and the poll/SSE tests observe no emitted events. Pass no explicit path to _ensure_events_db in the default case, using the new root-derived path only when archive_root_path is supplied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Repaired in 174fa3b81955efd279e61ee2fd5cc0c3f7471258. Default event writes and latest-event reads now use the overridable event-ledger resolver; explicit roots retain their explicit path.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Default event writes and latest-event reads use the overridable event-ledger resolver, while explicit roots retain their explicit path.
| except sqlite3.OperationalError as exc: | ||
| if is_transient_sqlite_lock(exc): | ||
| logger.info("judgment_automation: archive busy; retrying on next tick: %s", exc) | ||
| continue | ||
| logger.warning("judgment_automation: sweep failed", exc_info=True) | ||
| except Exception: | ||
| reason = "transient_sqlite_lock" if is_transient_sqlite_lock(exc) else "operational_error" | ||
| if not receipt_context.recorded: | ||
| await persist_failure_fallback( | ||
| exc, | ||
| default_reason=reason, | ||
| operation_id=receipt_context.operation_id, | ||
| ) | ||
| logger.info("judgment_automation: archive busy; retrying on next tick: %s", exc) |
There was a problem hiding this comment.
When the sweep raises a sqlite3.OperationalError that is_transient_sqlite_lock() rejects—such as a read-only database, missing schema, or disk I/O failure—this path still logs at info level and describes the archive as merely busy. The previous implementation warned for these non-transient errors, and deployments that suppress info logs can now miss a persistent scheduler outage even though it will not be resolved by waiting for contention to clear. Retain the info message only for recognized lock errors and warn for the other operational failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Repaired in 174fa3b81955efd279e61ee2fd5cc0c3f7471258. Recognized lock contention remains informational, while other SQLite OperationalError failures now log at warning severity.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Recognized lock contention remains informational, while other SQLite OperationalError failures log at warning severity.
| elif pending_count and judgment_scheduler_receipt_status in {"parked", "unknown"}: | ||
| state = "parked-pending" | ||
| if judgment_scheduler_receipt_status == "parked": | ||
| caveats.append("judgment scheduler is parked; the bounded retry route is the next enabled daemon tick") |
There was a problem hiding this comment.
When the latest parked receipt becomes old while the daemon heartbeat remains fresh—for example, after the capability is enabled but every subsequent scheduler receipt fails—this branch continues reporting parked-pending forever because it ignores the receipt age. Even disabled ticks coalesce only for a bounded horizon and must eventually persist another parked receipt, so a parked receipt older than that horizon plus the next scheduled tick is evidence that the scheduler is no longer reporting. Apply a parked-receipt freshness bound calibrated to the coalescing cadence so an indefinitely old parked state becomes scheduler-stalled rather than looking deliberately parked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Repaired in 174fa3b81955efd279e61ee2fd5cc0c3f7471258. Parked queue health now expires after the same cadence plus bounded grace used for receipt coalescing.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. Parked queue health expires after the same cadence plus bounded grace used for receipt coalescing.
|
@coderabbitai Please perform a fresh substantive review at exact head e6fb3da. The prior CodeRabbit result was rate limited and is not substantive review. |
|
|
|
Top-level review record 4890135864 was the persistent queue-collector finding and is satisfied by the daemon status registry in 022f28f. The other automated review records were boilerplate or summaries of the inline findings; CodeRabbit issue comments 5228235198 and 5229129218 were rate-limit notices without substantive findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 174fa3b819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| judgment_scheduler_receipt_status: Literal["completed", "parked", "failed", "unknown"] = "unknown" | ||
| judgment_scheduler_receipt_at_ms: int | None = None | ||
| judgment_scheduler_receipt_age_ms: int | None = None | ||
| judgment_scheduler_receipt_reason: str | None = None |
There was a problem hiding this comment.
When operators use polylogued status, polylogue/daemon/cli.py::status_command passes the new receipt fields to format_daemon_status_lines, but that formatter still prints only queue state, producer, scheduler heartbeat, and debt. Consequently this production CLI hides the receipt status, timestamp, age, and reason even though they are present in the payload; extend the daemon text formatter alongside the existing polylogue status renderer.
AGENTS.md reference: AGENTS.md:L480-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 981e5f1. format_daemon_status_lines now appends receipt status, ISO timestamp, age, and reason. tests/unit/daemon/test_daemon_status.py asserts the exact plain operator line, while the structured payload remains unchanged.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. format_daemon_status_lines renders receipt status, ISO timestamp, age, and reason.
| judgment_parked_receipt_fresh = ( | ||
| judgment_scheduler_receipt_status == "parked" | ||
| and judgment_receipt_age_ms is not None | ||
| and judgment_receipt_age_ms <= receipt_freshness_window_ms |
There was a problem hiding this comment.
With pending candidates and the capability disabled, a healthy scheduler becomes scheduler-stalled during every coalescing cycle: the tick one interval after a parked receipt is coalesced because its age is within interval + grace, but the following opportunity to emit is not until two intervals after the receipt, while this check expires it at interval + grace. At the default one-hour interval, status is therefore falsely stalled from 1.1 hours until the two-hour tick. Fresh evidence beyond the earlier parked-cadence repair is this gap between the coalescing schedule and the current-head freshness bound; either retain freshness through the next possible write tick or shorten the suppression scheme accordingly.
AGENTS.md reference: AGENTS.md:L477-L482
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 981e5f1. The shared freshness helper gives parked receipts two effective cadence windows plus bounded grace, matching the next possible coalesced write. The boundary test proves parked-pending through that horizon and scheduler-stalled only afterward; no synthetic receipt is emitted.
There was a problem hiding this comment.
Disposition: satisfied at the pushed exact head. The shared freshness helper gives parked receipts two effective cadence windows plus bounded grace, matching the next possible coalesced write.
|
@codex review Please perform a fresh substantive review at exact head 981e5f1. Inspect the five acceptance criteria: durable cross-tier receipt recovery, plain and structured status projection, parked coalesced freshness, positive batch limits, and malformed reload and root-resolution survival. Do not reuse prior review summaries. Keep this PR non-draft and report only current-head findings. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@polylogue/daemon/judgment_automation.py`:
- Around line 378-389: Update _judgment_automation_receipt_outbox_pending to
import AssertionStatus and replace the hardcoded 'active' SQL condition with a
bound status parameter using AssertionStatus.ACTIVE.value, alongside the
existing scope and kind parameters, keeping the probe aligned with
list_judgment_automation_receipt_outbox.
In `@polylogue/daemon/status.py`:
- Around line 2816-2831: Update daemon_status_payload’s registry path so
assertion_candidate_queue_status_summary receives the caller-supplied config
instead of resolving ambient runtime configuration; scope the registered queue
collector by configuration authority or bypass the shared collector when config
is explicit. Preserve the existing ephemeral path and add a regression covering
a Config that differs from ambient runtime settings.
In `@tests/unit/daemon/test_daemon_status.py`:
- Around line 2924-2969: Update
test_periodic_status_registry_resumes_slow_queue_health_across_ticks to use
frozen_clock and controllable registry timing instead of real sleep-based
polling and wall-clock deadlines. Advance the frozen clock explicitly until the
background collection completes, preserving the existing assertions for
timed_out, refreshing, fresh, and the single health-check invocation.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 55d90b2a-043b-43d3-a970-17f73aadb421
📒 Files selected for processing (13)
docs/plans/layering.yamlpolylogue/api/archive.pypolylogue/cli/commands/status.pypolylogue/config.pypolylogue/daemon/events.pypolylogue/daemon/judgment_automation.pypolylogue/daemon/status.pypolylogue/storage/sqlite/archive_tiers/user_write.pytests/unit/api/test_assertion_candidate_queue_health.pytests/unit/cli/test_status.pytests/unit/core/test_config.pytests/unit/daemon/test_daemon_status.pytests/unit/daemon/test_judgment_automation.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 981e5f1d08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| await _await_catch_up_gate(catch_up_complete, loop_name="judgment automation sweep") | ||
| coordinator = daemon_write_coordinator() | ||
| last_valid_root: Path | None = None |
There was a problem hiding this comment.
When a daemon using a non-default configured archive starts successfully, then its config becomes malformed while the initial catch-up gate is still held, last_valid_root has not yet captured the root already resolved by run_daemon_services. After the gate releases, receipt_root() falls back to data_home(), so recovery searches the wrong user.db and leaves the durable outbox marker unprojected into the custom archive's ops.db until configuration is repaired. Preserve or pass the daemon's startup root before awaiting the gate; the regression's data_home() == tmp_path setup currently masks this custom-root case.
AGENTS.md reference: AGENTS.md:L121-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition: repaired in 5879f57. The daemon passes its already resolved startup archive root into the periodic loop, so malformed reloads recover the custom archive rather than falling back to data_home(). The regression uses distinct configured and ambient roots.
| root, | ||
| status=raw_status, | ||
| reason=raw_reason, | ||
| now_ms=now_ms, |
There was a problem hiding this comment.
When recovery happens substantially after the user-tier commit, this passes the recovery invocation's now_ms—or the current clock when it is None—as the recovered event timestamp instead of using the marker's original creation time. Queue health interprets daemon_events.ts_ms as scheduler freshness, so an old completed outcome is made newly fresh; if automation is now disabled or its configuration is malformed while candidates remain, status can report an active pending queue rather than parked/stalled until the next cadence. Preserve the attempt timestamp from the durable marker and record recovery time separately if needed.
AGENTS.md reference: AGENTS.md:L121-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition: repaired in 5879f57. Recovery records the marker's original created_at_ms, preserving the original sweep freshness instead of refreshing it with recovery time. The regression asserts the durable event timestamp.
| conn.row_factory = sqlite3.Row | ||
| acknowledged = 0 | ||
| try: | ||
| for marker in list_judgment_automation_receipt_outbox(conn): |
There was a problem hiding this comment.
After a sustained ops-tier outage while user-tier sweeps continue, every committed attempt can leave another active outbox marker, but recovery loads and drains the entire backlog in one coordinator call. Each marker performs an ops lookup/write plus a user-tier acknowledgement, so thousands of accumulated markers can monopolize the daemon's sole SQLite writer and block ingestion and other maintenance for an unbounded interval. Apply a recovery batch limit and leave the remaining markers for subsequent ticks.
AGENTS.md reference: AGENTS.md:L181-L183
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition: repaired in 5879f57. Durable outbox recovery reads at most 100 markers per tick and the production SQLite regression proves the remaining marker is drained on the next tick.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660f2312fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| conn.row_factory = sqlite3.Row | ||
| acknowledged = 0 | ||
| try: | ||
| for marker in list_judgment_automation_receipt_outbox(conn): |
There was a problem hiding this comment.
When multiple outbox markers accumulate during an ops-tier outage, list_judgment_automation_receipt_outbox() returns them newest-first, but this loop inserts each missing event in that same order. Because queue health defines the authoritative receipt with ORDER BY id DESC, the oldest recovered attempt is inserted last and incorrectly becomes the current scheduler state, potentially hiding a newer completed or failed outcome. Iterate oldest-first so the newest attempt receives the highest ledger id.
Useful? React with 👍 / 👎.
| if config is None: | ||
| from polylogue.config import resolve_runtime_config | ||
|
|
||
| config = resolve_runtime_config().as_config() |
There was a problem hiding this comment.
When judgment_automation_interval_s is malformed, this ambient projection raises while ResolvedRuntimeConfig.as_config() reads the interval, before any of the status collectors or their error containment run. The scheduler itself catches configuration reload failures and remains alive, but periodic rich snapshots collapse to the minimal refresh-error payload and direct polylogued status fallback can fail instead of exposing the scheduler failure receipt. Resolve the queue-specific configuration inside its contained collector, or degrade only that component to unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5879f5704a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| for marker in list_judgment_automation_receipt_outbox( | ||
| conn, | ||
| limit=JUDGMENT_AUTOMATION_RECEIPT_RECOVERY_BATCH_LIMIT, |
There was a problem hiding this comment.
If the newest 100 active outbox markers are malformed—for example after corruption or an incompatible persisted payload—this limit selects the same markers on every tick, while the validation branches merely continue and leave them active. Valid older markers are then permanently starved and their committed user-tier outcomes never reach the disposable event ledger; quarantine invalid markers or paginate past every examined row.
AGENTS.md reference: AGENTS.md:L121-L122
Useful? React with 👍 / 👎.
| batch_limit=last_valid_batch_limit, | ||
| ) | ||
| logger.warning("judgment_automation: post-sleep configuration reload failed", exc_info=True) | ||
| continue |
There was a problem hiding this comment.
If initial recovery fails because ops.db is temporarily unavailable and scheduler configuration remains malformed, this continue skips recover_receipt_outbox on every later tick. Even after the ops tier becomes writable again, the durable marker remains stranded until configuration is repaired or the daemon restarts, despite recovery being intended to run independently of feature configuration; attempt recovery before this config-gated exit.
AGENTS.md reference: AGENTS.md:L121-L122
Useful? React with 👍 / 👎.
| from polylogue.daemon.status import assertion_candidate_queue_status_summary | ||
| from polylogue.paths import render_root | ||
|
|
||
| queue_config = Config( |
There was a problem hiding this comment.
When the daemon is unreachable and polylogue status --json takes this direct fallback, the synthetic Config omits judgment_automation_interval_s and therefore always uses the 3,600-second default. A deployment configured for a longer cadence can consequently be reported as scheduler-stalled even though its receipt is still fresh. Fresh evidence beyond the repaired explicit-runtime path is this separate production fallback, which should carry the resolved interval into the queue-health configuration.
Useful? React with 👍 / 👎.
| if not isinstance(receipt_payload, dict) or raw_status not in { | ||
| "completed", | ||
| "parked", | ||
| "failed", | ||
| }: |
There was a problem hiding this comment.
If the latest event contains only a recognized status, such as {"status":"completed"}, this check treats it as a valid typed receipt even though it has no reason or other production receipt evidence. With pending candidates and a recent timestamp, queue health then reports pending rather than stalled and exposes no reason, allowing a partial or corrupted ops event to masquerade as a successful scheduler attempt; require at least the mandatory receipt fields before accepting its status.
Useful? React with 👍 / 👎.
95bc346 to
f26cdf3
Compare
Record completed, parked, and failed judgment-automation outcomes in the existing ops daemon event ledger. Project the latest receipt into queue health so bounded retry, parked-pending, and scheduler-stalled states remain visible through API and CLI status surfaces. Preserve the existing judge write chokepoint and rollback behavior while retaining compatibility for callers that do not provide event timestamps or archive roots.
Prevent the default-disabled scheduler from appending an identical parked capability receipt on every minute-level tick by comparing structured receipt state within a bounded freshness window. Correlate enabled attempts with unique operation ids and let the inner sweep own its detailed failure receipt, while retaining an outer fallback for coordinator failures that occur before the sweep enters its receipt route. Add real-route regressions for parked-state coalescing, state transitions, detailed failure preservation, and coordinator fallback visibility.
Problem: scheduler health could select a receipt by wall-clock time, use a fixed freshness cutoff, write parked receipts outside the daemon writer, and report success after receipt persistence failed. The periodic loop also used the pre-sleep capability snapshot. What changed: order receipts by ledger id, derive freshness from the configured interval with bounded grace, render receipt timing evidence, reload config after sleep, serialize every periodic receipt write, and use one explicit fallback when no detailed receipt was recorded. Added real-route regressions for clock regression, long intervals, config flips, coordinator serialization, failure injection, duplicate prevention, and public rendering. Verification: focused daemon and API coverage passed; the new renderer test passed independently; devtools verify --quick passed; git diff --check passed.
Carry committed sweep results into receipt-persistence fallbacks so user-tier judgments and the authoritative ops receipt cannot disagree. Thread resolved scheduler cadence through status and queue-health projections, bound malformed authority evidence, and keep parked telemetry failures inside the serialized periodic loop. Add real-route regressions for receipt counts, runtime cadence, malformed receipts, retry behavior, and CLI rendering.
Problem: identical capability-gate-disabled receipts were suppressed for five minutes even though the default scheduler interval is one hour, so each normal disabled tick appended duplicate telemetry. What changed: derive the coalescing horizon from the effective post-sleep scheduler interval with the same bounded grace policy used for receipt freshness. Add a production-route regression covering two default-interval disabled ticks. Compatibility: receipt state comparison, persistence fallbacks, coordinator serialization, runtime interval authority, and malformed/status handling remain unchanged.
Preserve the attempted sweep status and reason when detailed receipt persistence fails so the periodic fallback retains the operational failure. Distinguish persisted, coalesced, and failed receipt outcomes so normal parked coalescing does not produce false warnings.
Carry user-tier commit state explicitly through receipt persistence so a queue-empty sweep remains a completed queue_empty outcome when its ops-tier receipt write fails. Add a production scheduler regression for the fallback ledger receipt.
Include assertion-candidate queue health in the persistent periodic status registry and render the daemon-backed payload in human status output. Add regressions for collector reuse and receipt rendering.
Retrigger Circle after the previous job read the pre-update PR carrier.
Problem: current-head review found that default event-ledger overrides could split writers from readers, persistent SQLite failures were logged as routine contention, stale parked receipts could hide scheduler failure, and a failed ops receipt result could lose committed sweep metadata. What changed: preserve the event resolver for default writes and reads, carry receipt result authority through false outcomes, classify non-lock SQLite failures at warning severity, and expire parked receipt health at the bounded scheduler cadence. Add real user/ops ledger regressions for each route. Verification: focused production-route tests passed 245 of 246, with one inherited route-catalog failure for the unrelated absent regenerate_private_fable_packet method. devtools verify --quick passed all 24 steps. git diff --check passed. Co-Authored-By: Codex <noreply@openai.com>
Problem: a failure while acknowledging the user-tier outbox marker could append a competing failure event after a successful ops receipt, and malformed durable events could be treated as acknowledgement evidence. What changed: preserve the recorded receipt context across acknowledgement failures, validate durable recovery payloads, classify policy parsing at the configuration boundary, and bind the outbox probe to the typed active status value. Verification: focused daemon tests and the quick verification gate pass.
Problem: compact daemon JSON and direct fallback JSON omitted scheduler queue health, while persistent collectors could answer for ambient configuration instead of an explicit Config. What changed: include the queue-health object in both JSON projections, bypass the ambient persistent collector for explicit authority, and replace slow-registry polling with controlled completion signaling in production-route tests. Verification: focused status tests and the quick verification gate pass.
f26cdf3 to
6b083b4
Compare
💡 Codex Review Preserve commit order when replaying receipt markers
When AGENTS.md reference: AGENTS.md:L114-L122 https://github.com/Sinity/polylogue/blob/6b083b412307611c9597af52c17387d33c684df1/.beads/issues.jsonl#L1754 This Beads export drops the parent commit's existing records AGENTS.md reference: AGENTS.md:L288-L299 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Repair judgment scheduler receipts and their daemon, API, and CLI projections. The change keeps one authoritative receipt per committed operation, makes durable outbox recovery bounded and failure-atomic, and exposes fresh queue health through the production status routes.
Problem
The scheduler could lose operation identity during receipt coalescing, accept malformed durable receipt evidence, append a competing failure after an acknowledgement failure, use an ambient archive root instead of an explicit one, and report stale or incomplete queue health. Direct CLI fallback also omitted the configured interval.
Solution
No durable schema migration or production archive mutation is part of this change.
Verification
POLYLOGUE_PYTEST_WORKERS=1 PATH="$PWD/.venv/bin:$PATH" devtools test tests/unit/daemon/test_judgment_automation.py tests/unit/daemon/test_daemon_status.py tests/unit/cli/test_status.py tests/unit/api/test_assertion_candidate_queue_health.py -k 'not archive_facade_route_catalog_covers_public_async_facade': 230 passed.PATH="$PWD/.venv/bin:$PATH" devtools test tests/unit/daemon/test_judgment_automation.py -k 'operation_owned_receipts or receipt_context_rejects or semantically_invalid_counter or replays_receipt_markers': 4 passed.PATH="$PWD/.venv/bin:$PATH" devtools verify --quick: all 24 steps passed, including mypy, layering, policy, lint, and generated-surface checks.git diff --check origin/master...HEAD: passed.The unfiltered focused selection also exposed one inherited repository baseline failure in
tests/unit/cli/test_status.py::test_archive_facade_route_catalog_covers_public_async_facade: the currentorigin/masterfrom Ref #3902 has the public route without the catalog entry. That unrelated route catalog was not changed; the affected selection above excludes only that test.Whole-Bead disposition
polylogue-5bxpydevtools verify --quickin the structured carrier belowReview disposition
The six requested review dimensions and the exact-head recovery findings are covered by the commits and tests above. The final carrier binds this body to the rebased PR head. The PR remains non-draft and is not being merged by this change.