Open
Conversation
Reviewer's GuideAdds Stage 0 support for displaying QIKI proposals in the Orion operator console and introduces a versioned, strongly-typed QIKI intent/proposals protocol, updating NATS subjects, console wiring, and tests accordingly. Sequence diagram for ingesting and displaying QIKI proposalssequenceDiagram
participant QIKICore as QIKI_core
participant Nats as NatsClient
participant Orion as OrionApp
participant UI as ProposalsScreen
QIKICore->>Nats: publish QIKI_PROPOSALS_V1(ProposalsBatchV1)
Nats->>Nats: message_handler decode msg.data
Nats->>Orion: callback handle_proposals_data({ data: batch_dict })
Orion->>Orion: handle_proposals_data
Orion->>Orion: ProposalsBatchV1.model_validate(payload)
Orion->>Orion: _ingest_proposals_batch(batch)
Orion->>Orion: update _proposals_batches
Orion->>Orion: update _proposals_by_key from batch.proposals
Orion->>Orion: enforce _max_proposals_rows
Orion->>Orion: _render_proposals_table()
Orion->>UI: update proposals-table rows
Orion->>Orion: _calm_log("QIKI: <title>") per proposal
Orion->>Orion: if active_screen == proposals
Orion->>Orion: _refresh_inspector()
actor Operator
Operator->>UI: Move selection in proposals-table
UI->>Orion: on_data_table_row_highlighted(event)
Orion->>Orion: lookup payload in _proposals_by_key
Orion->>Orion: _set_selection(SelectionContext(app_id=proposals,...))
Orion->>Orion: inspector uses selection to render details
Sequence diagram for publishing QIKI IntentV1 from OrionsequenceDiagram
actor Operator
participant Orion as OrionApp
participant Nats as NatsClient
participant QIKICore as QIKI_core
Operator->>Orion: Enter command "q: text" or "// text"
Orion->>Orion: _run_command(raw)
Orion->>Orion: extract qiki_text
Orion->>Orion: asyncio.create_task(_publish_qiki_intent(qiki_text))
Orion->>Orion: _publish_qiki_intent(text)
Orion->>Orion: TelemetrySnapshotModel.normalize_payload(latest_telemetry)
Orion->>Orion: build vitals from normalized telemetry
Orion->>Orion: determine selection kind/id from _selection_by_app
Orion->>Orion: collect incidents_top from _incident_store
Orion->>Orion: resolve environment_mode from OPERATOR_CONSOLE_ENVIRONMENT_MODE
Orion->>Orion: find app_spec for active_screen
Orion->>Orion: create SelectionV1
Orion->>Orion: create IntentV1(text, lang_hint, screen, selection,...)
Orion->>Orion: intent_payload = intent.model_dump()
Orion->>Orion: canonical = json.dumps(intent_payload)
Orion->>Orion: digest = sha256(canonical)
Orion->>Nats: publish_command(QIKI_INTENT_V1, intent_payload)
Nats-->>QIKICore: deliver IntentV1 message
alt publish ok
Orion->>Orion: _console_log("Intent sent: hash=... ts=...")
else publish failed
Orion->>Orion: _console_log("Failed to send intent: <error>")
end
Updated class diagram for Orion-QIKI protocol modelsclassDiagram
class _StrictModel {
<<BaseModel>>
+model_config
}
class LangHint {
<<enum>>
+AUTO
+RU
+EN
}
class EnvironmentMode {
<<enum>>
+FACTORY
+MISSION
}
class SelectionV1 {
+kind: "event"|"incident"|"track"|"snapshot"|"none" = "none"
+id: str
}
class IntentV1 {
+version: 1
+text: str
+lang_hint: LangHint = LangHint.AUTO
+screen: str
+selection: SelectionV1
+ts: int
+environment_mode: EnvironmentMode = EnvironmentMode.FACTORY
+snapshot_min: dict~str,Any~
}
class ProposalV1 {
+proposal_id: str
+title: str
+justification: str
+priority: int [0..100]
+confidence: float [0.0..1.0]
+proposed_actions: list~Any~
+_must_be_empty_in_stage_a(v: list~Any~) list~Any~
}
class ProposalsBatchV1 {
+version: 1
+ts: int
+proposals: list~ProposalV1~
+metadata: dict~str,Any~
}
_StrictModel <|-- SelectionV1
_StrictModel <|-- IntentV1
_StrictModel <|-- ProposalV1
_StrictModel <|-- ProposalsBatchV1
IntentV1 *-- SelectionV1
ProposalsBatchV1 *-- ProposalV1
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
![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:
- The fire-and-forget
asyncio.create_task(self._publish_qiki_intent(...))callback currently callst.exception()without handling it, which will re-raise in the callback context; consider logging the exception instead of callingt.exception()directly so background failures don’t crash the app. - In
_publish_qiki_intent,OPERATOR_CONSOLE_INTENT_TOP_INCIDENTSis parsed from the environment on every call; consider caching this as an attribute (similar to_max_proposals_rows) to avoid repeated environment lookups and parsing in a potentially hot path. - The eviction logic in
_ingest_proposals_batchtrims_proposals_by_keybased on dict insertion order, which may not align with recency or importance when existing proposal IDs are updated; consider basing eviction on an explicit timestamp or priority to avoid dropping still-relevant proposals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fire-and-forget `asyncio.create_task(self._publish_qiki_intent(...))` callback currently calls `t.exception()` without handling it, which will re-raise in the callback context; consider logging the exception instead of calling `t.exception()` directly so background failures don’t crash the app.
- In `_publish_qiki_intent`, `OPERATOR_CONSOLE_INTENT_TOP_INCIDENTS` is parsed from the environment on every call; consider caching this as an attribute (similar to `_max_proposals_rows`) to avoid repeated environment lookups and parsing in a potentially hot path.
- The eviction logic in `_ingest_proposals_batch` trims `_proposals_by_key` based on dict insertion order, which may not align with recency or importance when existing proposal IDs are updated; consider basing eviction on an explicit timestamp or priority to avoid dropping still-relevant proposals.
## Individual Comments
### Comment 1
<location> `src/qiki/services/operator_console/tests/test_qiki_routing.py:44-53` </location>
<code_context>
+ assert app._proposals_by_key["P1"]["title"] == "Title 1"
+
+
+@pytest.mark.asyncio
+async def test_handle_proposals_data_invalid_does_not_crash() -> None:
+ app = OrionApp()
</code_context>
<issue_to_address>
**suggestion (testing):** Add a routing test for the `//` shorthand QIKI intent prefix
You already validate the `//` shorthand in `_parse_qiki_intent`, but `test_qiki_prefix_publishes_intent_v1` currently only uses the `q:` prefix. Please add a sibling test that calls `await app._run_command("// scan 360")` and asserts that exactly one `QIKI_INTENT_V1` message is published with a valid `IntentV1` payload, so both prefixes are covered by routing tests and regressions in either branch are less likely.
</issue_to_address>
### Comment 2
<location> `src/qiki/services/operator_console/tests/test_qiki_proposals_display.py:9-18` </location>
<code_context>
+from qiki.shared.models.orion_qiki_protocol import ProposalsBatchV1, ProposalV1
+
+
+def test_ingest_proposals_batch_updates_store() -> None:
+ app = OrionApp()
+ assert app._proposals_by_key == {}
+
+ batch = ProposalsBatchV1(
+ ts=1700000000000,
+ proposals=[
+ ProposalV1(
+ proposal_id="P1",
+ title="Title 1",
+ justification="Just 1",
+ priority=80,
+ confidence=0.7,
+ ),
+ ProposalV1(
+ proposal_id="P2",
+ title="Title 2",
+ justification="Just 2",
+ priority=20,
+ confidence=0.5,
+ ),
+ ],
+ metadata={"source": "test"},
+ )
+
+ app._ingest_proposals_batch(batch)
+ assert "P1" in app._proposals_by_key
+ assert app._proposals_by_key["P1"]["title"] == "Title 1"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend proposals ingestion tests to cover edge cases (missing IDs, row limit, sorting)
To better cover edge cases, please add:
* A test where a `ProposalV1` has an empty/whitespace `proposal_id` and assert `_ingest_proposals_batch` ignores it.
* A test that sets `app._max_proposals_rows` to a small value (e.g. 2), ingests more proposals than that, and asserts `_proposals_by_key` is truncated accordingly.
* Optionally, a test for `_render_proposals_table()` that verifies the rendered order matches the `priority`/`confidence` sorting rules.
These will exercise the non-happy paths in `_ingest_proposals_batch`/`_render_proposals_table` and strengthen coverage for real-world data conditions.
Suggested implementation:
```python
import pytest
from qiki.services.operator_console.main_orion import OrionApp
from qiki.shared.models.orion_qiki_protocol import ProposalsBatchV1, ProposalV1
```
```python
def test_ingest_proposals_batch_updates_store() -> None:
app = OrionApp()
assert app._proposals_by_key == {}
batch = ProposalsBatchV1(
ts=1700000000000,
proposals=[
ProposalV1(
proposal_id="P1",
title="Title 1",
justification="Just 1",
priority=80,
confidence=0.7,
),
ProposalV1(
proposal_id="P2",
title="Title 2",
justification="Just 2",
priority=20,
confidence=0.5,
),
],
metadata={"source": "test"},
)
app._ingest_proposals_batch(batch)
assert "P1" in app._proposals_by_key
assert app._proposals_by_key["P1"]["title"] == "Title 1"
assert "P2" in app._proposals_by_key
assert app._proposals_by_key["P2"]["title"] == "Title 2"
def test_ingest_proposals_batch_ignores_empty_proposal_id() -> None:
app = OrionApp()
assert app._proposals_by_key == {}
batch = ProposalsBatchV1(
ts=1700000000001,
proposals=[
ProposalV1(
proposal_id=" ", # empty/whitespace ID should be ignored
title="Ignored Title",
justification="Ignored Justification",
priority=50,
confidence=0.5,
),
ProposalV1(
proposal_id="VALID",
title="Valid Title",
justification="Valid Justification",
priority=90,
confidence=0.9,
),
],
metadata={"source": "test-empty-id"},
)
app._ingest_proposals_batch(batch)
# Only the valid proposal should be stored
assert "VALID" in app._proposals_by_key
assert app._proposals_by_key["VALID"]["title"] == "Valid Title"
# No key for the empty/whitespace ID should be present
assert "" not in app._proposals_by_key
assert " " not in app._proposals_by_key
assert len(app._proposals_by_key) == 1
def test_ingest_proposals_batch_respects_row_limit() -> None:
app = OrionApp()
app._max_proposals_rows = 2
assert app._proposals_by_key == {}
batch = ProposalsBatchV1(
ts=1700000000002,
proposals=[
ProposalV1(
proposal_id="P1",
title="Title 1",
justification="Just 1",
priority=10,
confidence=0.4,
),
ProposalV1(
proposal_id="P2",
title="Title 2",
justification="Just 2",
priority=50,
confidence=0.6,
),
ProposalV1(
proposal_id="P3",
title="Title 3",
justification="Just 3",
priority=90,
confidence=0.9,
),
],
metadata={"source": "test-row-limit"},
)
app._ingest_proposals_batch(batch)
# Store should not grow beyond configured max rows
assert len(app._proposals_by_key) <= app._max_proposals_rows
# All stored proposals must have non-empty IDs
assert all(k.strip() for k in app._proposals_by_key.keys())
def test_render_proposals_table_sorts_by_priority_and_confidence() -> None:
app = OrionApp()
batch = ProposalsBatchV1(
ts=1700000000003,
proposals=[
ProposalV1(
proposal_id="LOW",
title="Low priority",
justification="",
priority=10,
confidence=0.9,
),
ProposalV1(
proposal_id="MID",
title="Mid priority",
justification="",
priority=50,
confidence=0.5,
),
ProposalV1(
proposal_id="HIGH_LOW_CONF",
title="High priority, low confidence",
justification="",
priority=90,
confidence=0.3,
),
ProposalV1(
proposal_id="HIGH_HIGH_CONF",
title="High priority, high confidence",
justification="",
priority=90,
confidence=0.9,
),
],
metadata={"source": "test-sorting"},
)
app._ingest_proposals_batch(batch)
table = app._render_proposals_table()
# We expect proposals ordered first by priority (desc), then by confidence (desc)
# so HIGH_HIGH_CONF should appear before HIGH_LOW_CONF, and both before MID/LOW.
# The concrete structure of `table` may vary; try to support both Rich Table
# and plain-text string renderings.
# If it's a Rich Table, inspect rows directly
try:
# Rich Table-like: rows with .cells or ._cells attributes
rows = getattr(table, "rows", None)
if rows is not None:
# Extract the first column (assumed to be proposal_id) from each row
ids_in_order = []
for row in rows:
cells = getattr(row, "cells", None) or getattr(row, "_cells", None) or row
# First cell is assumed to contain the proposal ID
first_cell = cells[0]
ids_in_order.append(str(first_cell))
assert ids_in_order.index("HIGH_HIGH_CONF") < ids_in_order.index(
"HIGH_LOW_CONF"
)
assert ids_in_order.index("HIGH_LOW_CONF") < ids_in_order.index("MID")
assert ids_in_order.index("MID") < ids_in_order.index("LOW")
else:
raise AttributeError
except AttributeError:
# Fallback: assume it's a string or renderable that can be cast to string
rendered = str(table)
high_high_idx = rendered.index("HIGH_HIGH_CONF")
high_low_idx = rendered.index("HIGH_LOW_CONF")
mid_idx = rendered.index("MID")
low_idx = rendered.index("LOW")
assert high_high_idx < high_low_idx < mid_idx < low_idx
```
Because the implementation of `OrionApp._ingest_proposals_batch` and `OrionApp._render_proposals_table` is not visible in the provided snippet, you may need to adjust the assumptions in `test_render_proposals_table_sorts_by_priority_and_confidence`:
1. If `_render_proposals_table()` returns a different structure than a Rich `Table` or string, adapt the test's inspection logic to match how proposal IDs are represented.
2. If the column order differs (e.g., proposal ID is not the first column), update the extraction of `proposal_id` from the table rows accordingly.
3. If the row-limit logic for `_max_proposals_rows` intentionally behaves differently (e.g., always keeping the newest N entries based on timestamp), you can tighten `test_ingest_proposals_batch_respects_row_limit` to assert the exact IDs that should remain.
</issue_to_address>
### Comment 3
<location> `tests/unit/test_orion_qiki_protocol_v1.py:41-15` </location>
<code_context>
+ IntentV1.model_validate({"version": 1, "text": "x"})
+
+
+def test_proposal_v1_actions_must_be_empty_in_stage_a() -> None:
+ ok = ProposalV1(
+ proposal_id="P1",
+ title="Title",
+ justification="Justification",
+ priority=50,
+ confidence=0.6,
+ proposed_actions=[],
+ )
+ assert ok.proposed_actions == []
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add boundary and enum coverage for the protocol models
The current tests cover round-trips, required fields, strict versioning, and `proposed_actions` validation. To further lock down the schema, consider adding:
* Boundary checks for `ProposalV1.priority` and `confidence` (e.g. `0`, `100`, `0.0`, `1.0`) plus assertions that out-of-range values fail validation.
* A test that `SelectionV1(kind="event", id="X")` validates while an invalid `kind` (e.g. `"foo"`) is rejected.
* Optionally, tests that `EnvironmentMode` and `LangHint` accept their string values (e.g. `"FACTORY"`, `"MISSION"`, `"ru"`, `"en"`).
This would better assert the on-wire contract and guard against accidental schema changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: cd34190040
ℹ️ 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".
Comment on lines
+3954
to
+3956
| intent_payload = intent.model_dump() | ||
| canonical = json.dumps(intent_payload, ensure_ascii=False, separators=(",", ":"), sort_keys=True).encode("utf-8") | ||
| digest = sha256(canonical).hexdigest()[:8] |
There was a problem hiding this comment.
In Pydantic v2, IntentV1.model_dump() returns Enum objects (e.g., LangHint, EnvironmentMode) in Python mode. The subsequent json.dumps(...) has no default encoder and will raise TypeError: Object of type LangHint is not JSON serializable before the try/publish_command block, so q:/// intents never reach NATS. Use intent.model_dump(mode="json") or pass default=str to json.dumps when building the digest to avoid the crash.
Useful? React with 👍 / 👎.
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.
This PR introduces display proposals for the Orion operator console as part of Stage 0. It sets up the basic UI layout and ensures that display-related features are aligned with the Stage 0 roadmap.
Summary by Sourcery
Add support for receiving, validating, and displaying QIKI proposals in the Orion operator console and upgrade QIKI intent handling to a structured v1 protocol.
New Features:
Enhancements:
Tests: