Repository navigation
Milestone 24: deterministic run queue overview summaries and inspection rollups - #48
Merged
Merged
Conversation
…rollups - Add RunsQueueOverview method and handler for deterministic queue inspection - Add RunQueueOverview, RunsQueueOverviewParams, PriorityCounts types to protocol - Implement overview counts: total, ready, blocked, attention, pinned, snoozed, overdue, archived - Add grouping by priority, assignee, and status - Wire up dispatch in handlers.rs - Fix clippy issues in run_readiness.rs and existing code
Reviewer's GuideAdds a new deterministic runs.overview protocol method and daemon handler to provide aggregate run-queue summaries (counts and groupings) using existing list_runs and run_readiness semantics, plus minor formatting and lint-related updates. Sequence diagram for the new runs.overview queue summary methodsequenceDiagram
actor ChatGPT
participant Client
participant Daemon as DeterministicDaemon
participant Store
participant RunReadiness as RunReadinessModule
ChatGPT->>Client: build RunsQueueOverviewParams
Client->>Client: select Method.RunsQueueOverview
Client->>Daemon: JSON RPC runs.overview(params)
Daemon->>Daemon: dispatch matches Method::RunsQueueOverview
Daemon->>Daemon: handle_runs_queue_overview(params, store)
Daemon->>Store: list_runs(limit=10000, workspace_id, include_archived, include_snoozed, ...)
Store-->>Daemon: all_runs
loop for each run in all_runs
Daemon->>RunReadiness: derive_readiness(status, is_archived, is_snoozed, blocked_by_count, priority, due_date, is_pinned, today)
RunReadiness-->>Daemon: RunReadinessSummary
Daemon->>Daemon: update ready_count, blocked_count, needs_attention_count, pinned_count, snoozed_count, overdue_count
Daemon->>Daemon: update by_priority, by_assignee, by_status
end
Daemon->>Daemon: build RunQueueOverview
Daemon-->>Client: RunQueueOverview as JSON
Client-->>ChatGPT: render deterministic queue overview
Updated class diagram for runs.overview protocol types and methodclassDiagram
class RunsQueueOverviewParams {
+workspace_id OptionString
+include_archived OptionBool
+include_snoozed OptionBool
+today OptionString
}
class PriorityCounts {
+low usize
+normal usize
+high usize
+urgent usize
}
class RunQueueOverview {
+total_visible usize
+ready_count usize
+blocked_count usize
+needs_attention_count usize
+pinned_count usize
+snoozed_count usize
+overdue_count OptionUsize
+archived_count OptionUsize
+by_priority PriorityCounts
+by_assignee HashMapStringUsize
+by_status HashMapStringUsize
}
class Method {
<<enum>>
+RunSetDueDate
+RunSetDependencies
+RunsQueueOverview
+as_str() &str
+from_str(name &str) OptionMethod
+all_methods() VecMethod
}
Method --> RunsQueueOverviewParams : uses_as_request
Method --> RunQueueOverview : uses_as_response
RunQueueOverview --> PriorityCounts : has
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
handle_runs_queue_overview,archived_countis always 0 andtotal_visibleappears to include archived runs wheninclude_archivedis true, which conflicts with theRunQueueOverviewdocs; consider explicitly separating archived vs visible counts and actually populatingarchived_count. - The
list_runscall inhandle_runs_queue_overviewuses a hard-coded limit of 10,000 to approximate "all runs"; if queues can grow larger, consider either a dedicated aggregate query or a paginated/streaming approach to avoid silently dropping runs and potential performance issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handle_runs_queue_overview`, `archived_count` is always 0 and `total_visible` appears to include archived runs when `include_archived` is true, which conflicts with the `RunQueueOverview` docs; consider explicitly separating archived vs visible counts and actually populating `archived_count`.
- The `list_runs` call in `handle_runs_queue_overview` uses a hard-coded limit of 10,000 to approximate "all runs"; if queues can grow larger, consider either a dedicated aggregate query or a paginated/streaming approach to avoid silently dropping runs and potential performance issues.
## Individual Comments
### Comment 1
<location path="codex-rs/deterministic-daemon/src/handlers.rs" line_range="1314-1315" />
<code_context>
+ let mut attention_count = 0;
+ let mut pinned_count = 0;
+ let mut snoozed_count = 0;
+ let mut overdue_count = 0;
+ let archived_count = 0;
+
+ let mut priority_counts = std::collections::HashMap::new();
</code_context>
<issue_to_address>
**issue (bug_risk):** Archived count is always zero, making `archived_count` misleading when `include_archived` is true.
`archived_count` is initialized but never updated, yet the result always returns `Some(0)` when `include_archived` is true. This exposes a bogus count to callers. Either increment `archived_count` whenever an archived item is included, or update the API/docs so this field is not returned (or is clearly marked) when the value is not meaningful.
</issue_to_address>
### Comment 2
<location path="codex-rs/deterministic-protocol/src/types.rs" line_range="947-948" />
<code_context>
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct RunQueueOverview {
+ /// Total count of visible (non-archived) runs matching the filter.
+ pub total_visible: usize,
+ /// Count of runs that are ready to work on now.
+ pub ready_count: usize,
</code_context>
<issue_to_address>
**issue (bug_risk):** `total_visible` docs don't match behavior when `include_archived` is true.
`handle_runs_queue_overview` increments `total_count` for every run from `list_runs`, including archived ones when `include_archived` is true. As a result, `total_visible` is actually "total including archived" in that case. Please either (a) filter out archived runs when computing this value, or (b) update the field name and docs to clarify that archived runs are counted when requested.
</issue_to_address>
### Comment 3
<location path="codex-rs/deterministic-daemon/src/handlers.rs" line_range="1280" />
<code_context>
+
+#[allow(clippy::collapsible_if)]
+/// Handle the runs.overview method - deterministic queue overview.
+fn handle_runs_queue_overview(
+ params: serde_json::Value,
+ store: &Store,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `handle_runs_queue_overview` by extracting small helpers, cleaning up redundant branches, and either properly tracking or removing unused counters to keep the function shallow and focused.
You can simplify `handle_runs_queue_overview` without changing behavior by extracting a few helpers and removing redundant branching/state:
1. **Unconditional `total_count` increment**
The `if readiness.is_ready { total_count += 1 } else { total_count += 1 }` adds unnecessary branching:
```rust
// before
if readiness.is_ready {
ready_count += 1;
total_count += 1;
} else {
total_count += 1;
}
// after (same behavior)
if readiness.is_ready {
ready_count += 1;
}
total_count += 1;
```
2. **Flatten overdue computation**
You can avoid nested `if today.is_some()` / `if let` chains by matching both `today` and `due_date` at once:
```rust
// before
if today.is_some() {
if let Some(due) = &run.due_date {
if let Some(t) = today {
if due.as_str() < t {
overdue_count += 1;
}
}
}
}
// after (equivalent)
if let (Some(t), Some(due)) = (today, run.due_date.as_deref()) {
if due < t {
overdue_count += 1;
}
}
```
3. **Status prefix helper**
The status prefix logic is inline in the loop; extracting it makes the loop easier to scan and reuse:
```rust
// helper
fn status_prefix(status: &str) -> &str {
if status.starts_with("finalized:") {
"finalized"
} else if status.starts_with("awaiting_approval:") {
"awaiting_approval"
} else if status.starts_with("pending:") {
"pending"
} else {
status
}
}
// usage in loop
let status_prefix = status_prefix(&run.status);
*status_counts.entry(status_prefix.to_string()).or_insert(0) += 1;
```
4. **Archived count either track or drop**
`archived_count` is currently always `0`, which is misleading when exposed:
- If you want to track it, increment inside the loop:
```rust
let mut archived_count = 0;
for run in &all_runs {
let is_archived = run.is_archived.unwrap_or(false);
if is_archived {
archived_count += 1;
}
// ...
}
let result = RunQueueOverview {
// ...
archived_count: if include_archived { Some(archived_count) } else { None },
// ...
};
```
- If you don’t need this metric, remove the local variable and the field from `RunQueueOverview` to reduce mental overhead.
5. **Optional: small aggregation helper**
If the loop keeps growing, consider extracting the per-run aggregation into a small function to keep `handle_runs_queue_overview` focused on I/O and parameter handling:
```rust
struct Aggregates {
total: usize,
ready: usize,
// ... other counters
}
fn accumulate_run(agg: &mut Aggregates, run: &RunSummary, today: Option<&str>) {
// counting logic here (ready, blocked, overdue, maps, etc.)
}
```
Then in the handler, you just initialize `Aggregates` and call `accumulate_run` in the loop, keeping the top-level function shallow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1314
to
+1315
| let mut overdue_count = 0; | ||
| let archived_count = 0; |
There was a problem hiding this comment.
issue (bug_risk): Archived count is always zero, making archived_count misleading when include_archived is true.
archived_count is initialized but never updated, yet the result always returns Some(0) when include_archived is true. This exposes a bogus count to callers. Either increment archived_count whenever an archived item is included, or update the API/docs so this field is not returned (or is clearly marked) when the value is not meaningful.
Comment on lines
+947
to
+948
| /// Total count of visible (non-archived) runs matching the filter. | ||
| pub total_visible: usize, |
There was a problem hiding this comment.
issue (bug_risk): total_visible docs don't match behavior when include_archived is true.
handle_runs_queue_overview increments total_count for every run from list_runs, including archived ones when include_archived is true. As a result, total_visible is actually "total including archived" in that case. Please either (a) filter out archived runs when computing this value, or (b) update the field name and docs to clarify that archived runs are counted when requested.
|
|
||
| #[allow(clippy::collapsible_if)] | ||
| /// Handle the runs.overview method - deterministic queue overview. | ||
| fn handle_runs_queue_overview( |
There was a problem hiding this comment.
issue (complexity): Consider simplifying handle_runs_queue_overview by extracting small helpers, cleaning up redundant branches, and either properly tracking or removing unused counters to keep the function shallow and focused.
You can simplify handle_runs_queue_overview without changing behavior by extracting a few helpers and removing redundant branching/state:
- Unconditional
total_countincrement
The if readiness.is_ready { total_count += 1 } else { total_count += 1 } adds unnecessary branching:
// before
if readiness.is_ready {
ready_count += 1;
total_count += 1;
} else {
total_count += 1;
}
// after (same behavior)
if readiness.is_ready {
ready_count += 1;
}
total_count += 1;- Flatten overdue computation
You can avoid nested if today.is_some() / if let chains by matching both today and due_date at once:
// before
if today.is_some() {
if let Some(due) = &run.due_date {
if let Some(t) = today {
if due.as_str() < t {
overdue_count += 1;
}
}
}
}
// after (equivalent)
if let (Some(t), Some(due)) = (today, run.due_date.as_deref()) {
if due < t {
overdue_count += 1;
}
}- Status prefix helper
The status prefix logic is inline in the loop; extracting it makes the loop easier to scan and reuse:
// helper
fn status_prefix(status: &str) -> &str {
if status.starts_with("finalized:") {
"finalized"
} else if status.starts_with("awaiting_approval:") {
"awaiting_approval"
} else if status.starts_with("pending:") {
"pending"
} else {
status
}
}
// usage in loop
let status_prefix = status_prefix(&run.status);
*status_counts.entry(status_prefix.to_string()).or_insert(0) += 1;- Archived count either track or drop
archived_count is currently always 0, which is misleading when exposed:
- If you want to track it, increment inside the loop:
let mut archived_count = 0;
for run in &all_runs {
let is_archived = run.is_archived.unwrap_or(false);
if is_archived {
archived_count += 1;
}
// ...
}
let result = RunQueueOverview {
// ...
archived_count: if include_archived { Some(archived_count) } else { None },
// ...
};- If you don’t need this metric, remove the local variable and the field from
RunQueueOverviewto reduce mental overhead.
- Optional: small aggregation helper
If the loop keeps growing, consider extracting the per-run aggregation into a small function to keep handle_runs_queue_overview focused on I/O and parameter handling:
struct Aggregates {
total: usize,
ready: usize,
// ... other counters
}
fn accumulate_run(agg: &mut Aggregates, run: &RunSummary, today: Option<&str>) {
// counting logic here (ready, blocked, overdue, maps, etc.)
}Then in the handler, you just initialize Aggregates and call accumulate_run in the loop, keeping the top-level function shallow.
There was a problem hiding this comment.
Pull request overview
Adds a new deterministic inspection surface (runs.overview) to summarize the run queue at a glance (counts + rollups), intended to help ChatGPT quickly understand queue shape without adding autonomy.
Changes:
- Introduces
RunsQueueOverviewParams,RunQueueOverview, andPriorityCountsto the protocol, and wires the newruns.overviewmethod into method parsing. - Implements
handle_runs_queue_overviewin the daemon to compute aggregate queue counts and groupings (priority/assignee/status), reusing core readiness derivation. - Adds/suppresses a few clippy lints (core readiness function; daemon blocker-impact helper).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| codex-rs/deterministic-protocol/src/types.rs | Adds request/response model types for runs.overview (overview counts + rollups). |
| codex-rs/deterministic-protocol/src/methods.rs | Registers the new JSON-RPC method name runs.overview. |
| codex-rs/deterministic-daemon/src/persistence.rs | Suppresses clippy for unwrap usage in blocker-impact map helper. |
| codex-rs/deterministic-daemon/src/handlers.rs | Dispatch wiring + new handler that computes the overview summary. |
| codex-rs/deterministic-core/src/run_readiness.rs | Exposes readiness derivation (with clippy allowances) for reuse by the daemon overview. |
| codex-rs/deterministic-core/src/lib.rs | Exports the new run_readiness module. |
Comments suppressed due to low confidence (1)
codex-rs/deterministic-daemon/src/persistence.rs:1031
- This change suppresses
clippy::unwrap_used, but the function still callsself.conn.lock().unwrap()andserde_json::from_str(...).unwrap_or_default(). A poisoned mutex will panic the daemon, and silently defaulting on JSON parse errors can hide data corruption. Prefer propagating errors (or at least logging) instead of suppressing the lint.
#[allow(clippy::unwrap_used)]
pub fn get_blocker_impact_map(&self) -> Result<std::collections::HashMap<String, usize>> {
let conn = self.conn.lock().unwrap();
let mut stmt = conn.prepare(
"SELECT blocked_by_run_ids FROM runs WHERE blocked_by_run_ids IS NOT NULL AND blocked_by_run_ids != '[]'",
)?;
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+1314
to
+1316
| let mut overdue_count = 0; | ||
| let archived_count = 0; | ||
|
|
Comment on lines
+1409
to
+1418
| let result = RunQueueOverview { | ||
| total_visible: total_count, | ||
| ready_count, | ||
| blocked_count, | ||
| needs_attention_count: attention_count, | ||
| pinned_count, | ||
| snoozed_count, | ||
| overdue_count: if today.is_some() { Some(overdue_count) } else { None }, | ||
| archived_count: if include_archived { Some(archived_count) } else { None }, | ||
| by_priority: PriorityCounts { |
Comment on lines
+1290
to
+1294
| // Use list_runs with high limit to get all runs for overview | ||
| // This uses the existing filter semantics from RunsListParams | ||
| let archived_only = false; // We handle filtering ourselves | ||
| let all_runs = store.list_runs( | ||
| 10000, // High limit to get all runs |
| archived_only, | ||
| None, // label_filter | ||
| false, // pinned_only | ||
| include_snoozed, |
Comment on lines
+1372
to
+1381
| // Count overdue | ||
| if today.is_some() { | ||
| if let Some(due) = &run.due_date { | ||
| if let Some(t) = today { | ||
| if due.as_str() < t { | ||
| overdue_count += 1; | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+1278
to
+1285
| #[allow(clippy::collapsible_if)] | ||
| /// Handle the runs.overview method - deterministic queue overview. | ||
| fn handle_runs_queue_overview( | ||
| params: serde_json::Value, | ||
| store: &Store, | ||
| ) -> Result<(serde_json::Value, Option<RunState>)> { | ||
| let p: RunsQueueOverviewParams = serde_json::from_value(params)?; | ||
|
|
Comment on lines
+969
to
+974
| /// Unassigned runs are stored under key `"unassigned"`. | ||
| #[serde(default)] | ||
| pub by_assignee: std::collections::HashMap<String, usize>, | ||
| /// Counts grouped by status prefix (e.g., "awaiting_approval", "finalized", etc.). | ||
| #[serde(default)] | ||
| pub by_status: std::collections::HashMap<String, usize>, |
4 tasks done
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
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.
Milestone 24 for ChatCodex: adds deterministic queue overview summaries and inspection rollups so ChatGPT can inspect the shape of the run queue at a glance without introducing backend autonomy.
What's in this PR
Protocol (
deterministic-protocol/src/types.rs)RunsQueueOverviewParams- parameters for queue overview queryRunQueueOverview- deterministic queue overview with aggregate countsPriorityCounts- breakdown by priority levelMethods (
deterministic-protocol/src/methods.rs)RunsQueueOverviewmethod variant and wire up parsingCore (
deterministic-core/src/run_readiness.rs)Daemon (
deterministic-daemon/src/handlers.rs)handle_runs_queue_overviewfunctionVerification
Summary by Sourcery
Add a deterministic runs.overview API to provide aggregate run queue summaries for inspection without mutating state.
New Features:
Enhancements: