feat(operator_inbox): message triage + draft + follow-up engine#4147
feat(operator_inbox): message triage + draft + follow-up engine#4147Sathvik-1007 wants to merge 2 commits into
Conversation
- Cargo.toml: sqlparser, whatlang, nnnoiseless, strsim, lettre async - util.rs: uuid_v4(), now_epoch() shared helpers - about_app: UserContent privacy variant, 6 capability catalog entries - docs: capability plan + full API reference
9-file domain: types, engine (priority scoring), connection (TLS IMAP/SMTP), imap_client (async fetch UNSEEN), parser (body extraction), poller (background), rpc handlers, schemas. E2E: triage_message -> generate_draft.
📝 WalkthroughWalkthroughThis PR adds the ChangesOperator Inbox Rollout
Sequence Diagram(s)sequenceDiagram
participant Client as "JSON-RPC client"
participant RPC as "operator_inbox::rpc"
participant Engine as "operator_inbox::engine"
participant Connection as "operator_inbox::connection"
participant SMTP as "SMTP server"
Client->>RPC: openhuman.operator_inbox_triage_message
RPC->>Engine: triage_message(...)
Engine-->>RPC: TriageRecord
RPC-->>Client: ok + triage_id
Client->>RPC: openhuman.operator_inbox_generate_draft
RPC->>Engine: generate_draft(...)
Engine-->>RPC: DraftReply
RPC-->>Client: ok + draft
Client->>RPC: openhuman.operator_inbox_send_reply
RPC->>Engine: get_triage(...)
RPC->>Connection: send_reply(...)
Connection->>SMTP: deliver mail
SMTP-->>Connection: accepted
Connection-->>RPC: message sent
RPC-->>Client: ok + message_id
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6634a088ad
ℹ️ 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".
| &rpc_base, | ||
| 501, | ||
| "openhuman.voice_actions_list_mappings", | ||
| json!({}), | ||
| ) |
There was a problem hiding this comment.
Remove the orphaned RPC call fragment
After the previous test closes, this new fragment starts with arguments to a missing post_json_rpc( call and is outside any function, so tests/json_rpc_e2e.rs no longer parses when the Rust test target is built. Restore the enclosing let list = post_json_rpc( block/test or remove the orphaned lines.
Useful? React with 👍 / 👎.
| /// Non-sensitive metadata (capability ids, feature flags, settings shape). | ||
| Metadata, | ||
| /// User-generated content (audio, text, queries) sent to cloud LLM for inference. | ||
| UserContent, |
There was a problem hiding this comment.
Handle the new privacy kind in the UI
PrivacyDataKind::UserContent serializes as user_content, but the existing frontend contract/rendering only recognizes raw | derived | credentials | diagnostics | metadata (app/src/utils/tauriCommands/aboutApp.ts and PrivacyPanel.tsx). Any new capability using this value will show an undefined badge/label in Settings > Privacy; update the client type, label, badge mapping, and i18n keys together with the enum.
Useful? React with 👍 / 👎.
| if RUNNING.swap(true, Ordering::SeqCst) { | ||
| return false; | ||
| } | ||
| let interval = Duration::from_secs(interval_secs.unwrap_or(DEFAULT_POLL_INTERVAL_SECS)); |
There was a problem hiding this comment.
Reject zero poll intervals
If openhuman.operator_inbox_start_poller is called with interval_secs: 0, this builds a zero-duration interval and the loop immediately repeats after every fetch/error, which can hammer the IMAP server and spin CPU until stopped. Reject zero or treat it as the default before spawning the loop.
Useful? React with 👍 / 👎.
| if !RUNNING.load(Ordering::SeqCst) { | ||
| info!("{LOG_PREFIX} stopped"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Prevent old pollers from surviving restarts
Stopping only flips the shared RUNNING flag, but if start_poller is called again before the old task wakes from sleep(interval), the old loop sees RUNNING=true here and continues alongside the new task with its previous config. That restart path duplicates IMAP fetches/triage; keep a task handle/generation token or await/abort the old task before allowing a new start.
Useful? React with 👍 / 👎.
| info!( | ||
| "{LOG_PREFIX} connecting to {}:{} user={}", | ||
| config.host, config.port, config.username |
There was a problem hiding this comment.
Redact the IMAP username in logs
This logs config.username on every IMAP fetch; for common providers that is the user's full email address, and AGENTS.md explicitly says not to log full PII. Redact to a domain/hash or omit it so support logs do not capture mailbox identities.
Useful? React with 👍 / 👎.
| let mut session = client | ||
| .login(&config.username, &config.password) | ||
| .await |
There was a problem hiding this comment.
Implement or reject OAuth-only IMAP auth
validate_imap_config allows passwordless configs when oauth2_token is set, but this path still authenticates with LOGIN using config.password, so Gmail/Outlook OAuth-only configs that pass validation fail with an empty password. Either implement XOAUTH2 here or reject OAuth-only configs until it is supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
src/openhuman/about_app/types.rs-159-160 (1)
159-160: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBackfill existing cloud-bound chat/voice capabilities to
UserContent.Adding this variant without migrating the existing message-sending entries leaves the privacy catalog internally inconsistent:
conversation.send_text,conversation.send_voice, andvoice.pttinsrc/openhuman/about_app/catalog_data.rsstill describe the same cloud-bound text/audio flow asPrivacyDataKind::Derived. The catalog API will now report identical data movement two different ways.Based on the catalog entries already present in
src/openhuman/about_app/catalog_data.rs, the same user text/audio flow is still classified asDerivedthere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/about_app/types.rs` around lines 159 - 160, The new PrivacyDataKind::UserContent variant in about_app/types.rs needs to be applied to the existing cloud-bound chat and voice catalog entries so the privacy model stays consistent. Update the matching entries in catalog_data.rs for conversation.send_text, conversation.send_voice, and voice.ptt to use UserContent instead of Derived, keeping the same user text/audio flow classification across the catalog API and the enum definition.docs/VC_CAPABILITIES.md-3-18 (1)
3-18: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReframe this as roadmap material, not shipped capability documentation.
The intro says all six modules are production-grade and already implemented, but the supplied PR scope only substantiates
operator_inboxplus capability foundations/docs for the others. Publishing this as current-state documentation will advertise RPC methods, limits, and security guarantees for modules that are not in this stack.As per coding guidelines, features should be surfaced after they are proved in Rust and over RPC; the supplied PR context only shows that proof for
operator_inbox.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/VC_CAPABILITIES.md` around lines 3 - 18, Reframe the document in terms of planned roadmap rather than shipped capability status: the current intro and module table in VC_CAPABILITIES.md present all six modules as production-grade and implemented, but the PR scope only supports operator_inbox plus foundation docs for the rest. Update the wording in the intro and module descriptions so only operator_inbox is described as current capability, while voice_assistant, live_captions, voice_actions, chat_with_data, and guided_flows are clearly marked as planned or forthcoming. Keep the existing structure, but avoid claiming RPC exposure, safety guarantees, or Rust-core implementation for modules not evidenced by the current stack.Source: Coding guidelines
docs/boost-vc-capability-plan.md-58-65 (1)
58-65: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe operator inbox non-goal is already false for this PR.
This section says operator inbox is "schema-only, no transport", but the supplied PR scope includes IMAP fetching, SMTP reply sending, connection handling, polling, and the
triage_message -> generate_draftflow. Leaving this here will mislead anyone using the plan to understand what actually shipped.Based on the PR objectives and review-stack context,
operator_inboxin this stack is not transport-free or schema-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/boost-vc-capability-plan.md` around lines 58 - 65, Update the non-goals list in the capability plan to match the shipped operator inbox behavior. The current “No actual email/Slack integration” entry is no longer accurate given the implemented operator inbox flow; revise or remove it so it does not claim the inbox is schema-only or transport-free. Keep the wording aligned with the actual operator inbox modules and flow, especially the triage_message to generate_draft path, IMAP fetching, SMTP reply sending, connection handling, and polling.src/openhuman/about_app/catalog_data.rs-1452-1538 (1)
1452-1538: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t publish the unimplemented VC foundations as live Beta capabilities yet.
src/openhuman/about_app/catalog.rsserves this array directly to clients, so these entries are the live capability contract. In the supplied PR scope, onlyoperator_inboxis actually wired through runtime/RPC; the other five are still described as foundations/planned work. Shipping them here asBetawill make the catalog advertise capabilities and RPC surfaces that do not exist yet.As per coding guidelines,
src/openhuman/about_app/**should follow "Specify → prove in Rust → prove over RPC → surface in UI → test", andsrc/openhuman/about_app/catalog.rsexposes this data directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/about_app/catalog_data.rs` around lines 1452 - 1538, The new VC foundation entries are being exposed through the live capability catalog as if they are shipped Beta features. Update the `Capability` entries in `catalog_data.rs` so only actually wired capabilities stay surfaced as live Beta items, and move the unimplemented foundations out of the public contract or mark them as non-live/planned instead of `CapabilityStatus::Beta`. Use the existing `Capability` records and `CapabilityStatus` values to keep `operator_inbox.triage` published while preventing the other five `id` entries from advertising unavailable RPC surfaces.Source: Coding guidelines
src/openhuman/operator_inbox/mod.rs-12-13 (1)
12-13: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRoute schema handlers through
ops.rs, not a separaterpc.rs.This new domain is bypassing the repo’s controller-migration boundary: the paired handlers in
src/openhuman/operator_inbox/schemas.rs:334-336dispatch intosuper::rpc::...instead of anops.rsbusiness-logic layer. That makesoperator_inboxnon-canonical from the start and pushes controller flow away from the requiredschemas.rs -> ops.rsshape.As per coding guidelines,
src/openhuman/*/{mod,schemas}.rsrequires “schemas with handlers inschemas.rsdelegating toops.rs,” andsrc/openhuman/*/defines the canonical shape withops.rsas the business-logic module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/mod.rs` around lines 12 - 13, The operator_inbox module is routing schema handlers through a separate rpc layer instead of the canonical ops business-logic layer. Update the module layout and handler wiring so the relevant functions in schemas.rs delegate to ops.rs rather than super::rpc, and ensure the module exports/include structure in mod.rs reflects ops.rs as the handler target instead of rpc.rs.Source: Coding guidelines
src/openhuman/util.rs-663-668 (1)
663-668: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t turn clock failures into a real-looking timestamp.
now_epoch()feedsTriageRecord.created_at, andsrc/openhuman/operator_inbox/engine.rs:27-66later uses that field for archived-record eviction. Falling back to0silently makes fresh records look oldest, so a bad system clock corrupts ordering/retention instead of surfacing the failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/util.rs` around lines 663 - 668, The now_epoch() helper is swallowing SystemTime clock errors by falling back to 0, which can make fresh records appear oldest and break retention/ordering. Update now_epoch() in util.rs to surface the time error instead of returning a real-looking timestamp, and adjust callers such as TriageRecord.created_at and the archived-record eviction path in operator_inbox::engine to handle the failure explicitly.src/openhuman/util.rs-652-669 (1)
652-669: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove new shared helpers out of
util.rs.This file is already at least 669 lines long, so adding more shared APIs here keeps it above the repo’s module-size cap and further entrenches the catch-all utility layer.
As per coding guidelines,
src/**/*.rs: “Rust modules must be ≤ ~500 lines in size”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/util.rs` around lines 652 - 669, The new shared helpers in util.rs should be moved out of the oversized catch-all module to keep it within the module-size cap. Relocate uuid_v4 and now_epoch into a smaller, more focused shared module used by the domain modules, and update any call sites to reference the new location instead of openhuman::util so util.rs stops accumulating generic APIs.Source: Coding guidelines
src/openhuman/operator_inbox/schemas.rs-316-345 (1)
316-345: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDelegate schema handlers to the ops layer.
The schema wrappers currently route through
rpc.rs; the repo migration rule expects schema handlers to delegate toops.rs, keeping controller wiring separate from RPC/business logic.As per coding guidelines, “define schemas with handlers in
schemas.rsdelegating toops.rs.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/schemas.rs` around lines 316 - 345, The schema handler wrappers in h_triage, h_draft, h_followup, h_get, h_list, h_archive, h_fetch_inbox, h_send_reply, h_start_poller, and h_stop_poller still delegate to super::rpc, but they should route through the ops layer instead. Update each controller future to call the corresponding super::ops function so schemas.rs stays as wiring only and business logic remains in ops.rs.Source: Coding guidelines
src/openhuman/operator_inbox/schemas.rs-365-369 (1)
365-369: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDon’t expose raw mailbox passwords as controller inputs.
These schemas put IMAP/SMTP passwords directly on the JSON-RPC/tool surface. Prefer a credential/secret reference and resolve it server-side.
Also applies to: 442-446, 508-512
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/schemas.rs` around lines 365 - 369, The inbox schemas currently expose raw IMAP/SMTP passwords as required string inputs on the controller/tool surface. Update the relevant schema builders in operator_inbox/schemas.rs, including the FieldSchema entries for password in the IMAP and SMTP-related schema definitions, to accept a credential or secret reference instead of a plaintext password, and resolve the secret server-side inside the corresponding request handling logic rather than in the JSON-RPC surface.src/openhuman/operator_inbox/rpc.rs-106-107 (1)
106-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn
ok:falseinstead of propagating lookup errors.These
?paths bypass the handler’s JSON response shape, unlike the surrounding handlers that return{ok:false,error}.Also applies to: 256-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/rpc.rs` around lines 106 - 107, The triage lookup path in the handler currently propagates errors with `?`, which breaks the JSON `{ok:false,error}` response shape used elsewhere. Update the handler around `engine::get_triage(id)` so lookup failures are caught and converted into the existing error response instead of bubbling up, and apply the same treatment to the related `try_llm_draft` flow mentioned in the comment. Keep the fix localized to the handler logic so the response format stays consistent with the surrounding RPC methods.src/openhuman/operator_inbox/rpc.rs-60-63 (1)
60-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd timeouts around LLM calls.
These awaits sit on the RPC path; if the provider stalls, triage/draft requests can hang instead of falling back or returning
ok:false.Also applies to: 149-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/rpc.rs` around lines 60 - 63, The RPC path currently awaits LLM provider calls directly, so stalled requests can hang triage/draft handling instead of failing fast. Wrap the async calls in the relevant RPC handlers in rpc.rs (including the chat_with_system usage and the other call at the referenced later location) with a timeout boundary, and on timeout return the existing fallback/ok:false path rather than blocking the request indefinitely. Use the surrounding handler functions and provider call sites to apply the timeout consistently.src/openhuman/operator_inbox/parser.rs-95-100 (1)
95-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t log sender addresses or subjects.
fromandsubjectcan contain PII/user content; keep this debug log to non-sensitive derived metadata.Proposed fix
debug!( - from = %from, - subject = %subject, + has_from = !from.is_empty(), + has_subject = !subject.is_empty(), attachments = attachments.len(), is_reply = is_reply, "[operator-inbox-parser] email parsed" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/parser.rs` around lines 95 - 100, The debug log in parser.rs’s email parsing path is exposing PII by recording the raw sender address and subject. Update the debug! call in the operator-inbox parser to remove from and subject entirely, and keep only non-sensitive derived metadata such as attachments.len() and is_reply so the log still identifies parse behavior without user content.src/openhuman/operator_inbox/imap_client.rs-17-41 (1)
17-41: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact credentials from
Debug.Both configs derive
Debugwhile containing plaintext passwords/tokens, so any{:?}log or error context can leak mailbox credentials.Proposed fix
-#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct ImapConfig { @@ } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct SmtpConfig { @@ }Add custom
Debugimpls that redactpasswordand only expose whetheroauth2_tokenis present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/imap_client.rs` around lines 17 - 41, `ImapConfig` and `SmtpConfig` currently derive `Debug`, which can leak plaintext `password` and `oauth2_token` values in logs or error contexts. Remove the auto-derived `Debug` on these structs and add custom `Debug` implementations that redact sensitive fields, such as hiding `password` entirely and only indicating whether `oauth2_token` is present. Keep the existing `Serialize` and `Deserialize` derives unchanged.src/openhuman/operator_inbox/rpc.rs-203-203 (1)
203-203: 🎯 Functional Correctness | 🟠 MajorValidate ports before converting to
u16The
as u16cast on lines 203 and 246 silently wraps values exceeding 65535 (e.g., 65536 becomes 0), allowing invalid RPC parameters to connect to unintended ports.Code context
// Line 203 let port = p.get("port").and_then(|v| v.as_u64()).unwrap_or(993) as u16; // Line 246 let smtp_port = p.get("smtp_port").and_then(|v| v.as_u64()).unwrap_or(587) as u16;Replace the cast with explicit range validation returning an error for out-of-range values:
let port_val = p.get("port").and_then(|v| v.as_u64()).unwrap_or(993); if port_val > 65535 { return Err("port must be <= 65535".to_string()); } let port = port_val as u16;Apply the same fix to
smtp_port.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/rpc.rs` at line 203, Validation is missing before narrowing RPC port values to u16 in the operator inbox request handling, which can silently wrap invalid values. Update the port parsing in the relevant request handlers that read port and smtp_port from p to first keep them as u64, check they are within 0..=65535, and return an error when out of range; only then convert to u16. Use the existing parsing sites around the port and smtp_port extraction logic to apply the same fix consistently.src/openhuman/operator_inbox/schemas.rs-97-114 (1)
97-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign triage required fields with the handler.
The schema requires
sender,subject, andbody, buthandle_triage_messageaccepts any one of them. This makes generated clients reject requests the server supports.Proposed fix
FieldSchema { name: "sender", ty: TypeSchema::String, - comment: "Sender.", - required: true, + comment: "Sender. At least one of sender, subject, or body is required.", + required: false, }, @@ FieldSchema { name: "subject", ty: TypeSchema::String, - comment: "Subject.", - required: true, + comment: "Subject. At least one of sender, subject, or body is required.", + required: false, }, @@ FieldSchema { name: "body", ty: TypeSchema::String, - comment: "Body.", - required: true, + comment: "Body. At least one of sender, subject, or body is required.", + required: false, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/schemas.rs` around lines 97 - 114, The triage schema is stricter than the handler: `handle_triage_message` accepts any one of `sender`, `subject`, or `body`, but the `FieldSchema` entries currently mark all three as required. Update the schema definition in `operator_inbox::schemas` so the generated contract matches `handle_triage_message` by making these fields optional/allowing any single field to be present, while keeping the `FieldSchema` entries for `sender`, `subject`, and `body` aligned with the handler’s validation logic.src/openhuman/operator_inbox/poller.rs-18-23 (1)
18-23: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReject
interval_secs = 0.
Duration::from_secs(0)turns this into a busy loop that hammers IMAP and logs as fast as Tokio can reschedule it. Clamp to a sane minimum or return an error/falsefor zero.Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/poller.rs` around lines 18 - 23, Reject zero-valued polling intervals in start_polling by handling interval_secs before constructing the Duration; a value of 0 currently creates a busy loop. Update the start_polling path to either return false/error when interval_secs is Some(0) or clamp it to a safe minimum, and keep the behavior consistent in the polling loop that uses the interval derived from DEFAULT_POLL_INTERVAL_SECS.src/openhuman/operator_inbox/engine.rs-15-24 (1)
15-24: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEmail threading metadata is lost during triage.
fetch_new_emailsalready extractsmessage_id,in_reply_to, andreferences, but this engine only accepts/stores sender/subject/body. By the timesend_replyruns, the original threading headers are gone, so SMTP replies cannot setIn-Reply-To/Referencesand will go out as new conversations instead of true replies.Also applies to: 27-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/engine.rs` around lines 15 - 24, The triage flow drops email threading metadata because triage_message and triage_message_with_priority only carry sender, subject, and body, so send_reply cannot preserve the original conversation headers. Update the operator_inbox engine to thread through message_id, in_reply_to, and references from fetch_new_emails into the TriageRecord and the triage_message/triage_message_with_priority path, then have send_reply use those stored values when building the SMTP reply headers.src/openhuman/operator_inbox/connection.rs-113-156 (1)
113-156: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMalformed emails are silently dropped.
If
msg.body()is missing orMessageParser::parsereturnsNone, the loop just skips that message and still returnsOk. The caller then sees fewer inbox items with no indication that parsing failed, which makes message loss very hard to detect or recover from.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/connection.rs` around lines 113 - 156, The inbox fetch loop is silently dropping malformed messages when `msg.body()` is missing or `mail_parser::MessageParser::parse` returns `None`, so `fetch` can return `Ok` with missing emails and no signal of failure. Update the logic in the stream-processing loop in `connection.rs` to detect these cases and surface an error instead of skipping them, using the existing `LOG_PREFIX`, `msg_result`, and `FetchedEmail` flow so the caller can tell which message failed to parse.src/openhuman/operator_inbox/engine.rs-50-62 (1)
50-62: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
MAX_RECORDSis not actually enforced.When the map is full and there are no archived entries, this branch removes nothing and still inserts the new record. Under the poller path that means the in-memory store can grow past the advertised cap indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/engine.rs` around lines 50 - 62, The eviction logic in operator_inbox::engine does not enforce MAX_RECORDS when there are no archived entries to remove, so the store can still grow past the cap. Update the insertion path around the archive eviction block so that insert only proceeds when capacity is available, or evict a record unconditionally when store.len() >= MAX_RECORDS; use the existing store, MAX_RECORDS, and TriageStatus::Archived logic in this branch to ensure the limit is always respected.src/openhuman/operator_inbox/connection.rs-31-166 (1)
31-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAll remote mail operations are unbounded.
TCP connect, TLS handshake, IMAP login/select/search/fetch, and SMTP send all await without a timeout. A slow or half-open server can therefore pin the RPC call or the background poller forever. These external calls need
tokio::time::timeout(...)around them and timeout-specific errors back to callers.Also applies to: 169-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/connection.rs` around lines 31 - 166, The remote mail operations in fetch_new_emails are unbounded, so a slow or half-open server can hang the RPC or poller indefinitely. Wrap each external await in tokio::time::timeout, including TcpStream::connect, the rustls TlsConnector::connect handshake, async_imap::Client::login, session.select, session.uid_search, and session.uid_fetch, and return timeout-specific errors from fetch_new_emails. Apply the same timeout handling to the SMTP send path mentioned in the review, using the existing LOG_PREFIX and the relevant connection/session methods to locate the changes.src/openhuman/operator_inbox/poller.rs-15-24 (1)
15-24: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
stop_pollingdoes not retire the old task before a restart.The running flag is shared across all poller generations. If a caller does stop → start while the first task is sleeping or mid-fetch, the old task can observe
RUNNING = trueagain and continue alongside the new one, duplicating IMAP fetches and triage inserts. This needs a per-run cancellation token /JoinHandle, not just a global boolean.Also applies to: 25-48, 51-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/poller.rs` around lines 15 - 24, `start_polling`/`stop_polling` currently rely on the shared `RUNNING` atomic only, so an old `tokio::spawn` can survive a stop→start cycle and run alongside the new poller. Update the poller lifecycle to use a per-run cancellation mechanism and retain the spawned task handle inside `start_polling`/`stop_polling`, so stopping first signals the active task to exit and awaits or aborts its `JoinHandle` before allowing a restart. Keep the fix anchored around `RUNNING`, `start_polling`, and `stop_polling` so only one IMAP polling loop can exist at a time.src/openhuman/operator_inbox/connection.rs-35-38 (1)
35-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't log the full mailbox username.
This writes the account identifier to info logs on every IMAP fetch. Redact it or log only host/mailbox metadata so inbox credentials are not exposed through routine logs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/connection.rs` around lines 35 - 38, The connection logging in the IMAP setup is exposing the full mailbox username in routine info logs. Update the logging in the connection flow around the existing info! call in operator_inbox::connection so it redacts the username or omits it entirely, while still keeping non-sensitive context like host and port. Use the existing LOG_PREFIX and the connection setup path to locate the log statement, and ensure any replacement metadata does not reveal account identifiers.
🟡 Minor comments (3)
src/openhuman/operator_inbox/rpc.rs-310-319 (1)
310-319: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid live LLM dependency in this unit test.
handle_triage_messagetries the configured chat provider first, so this test can become network/config-dependent and flaky when a provider is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/rpc.rs` around lines 310 - 319, The triage_rpc test is still exercising the live chat provider path through handle_triage_message, which makes it flaky and environment-dependent. Update the test to avoid any real LLM/network dependency by mocking or stubbing the chat provider used by handle_triage_message, or by injecting a deterministic test provider before calling it, so the assertion only validates the triage logic and not external configuration.src/openhuman/operator_inbox/imap_client.rs-142-142 (1)
142-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount recipients as thread participants too.
participant_countcurrently only includes senders, so one inbound email to the operator reports one participant instead of sender + recipient(s).Proposed fix
let email = &emails[idx]; participants.insert(email.from.clone()); + for recipient in &email.to { + participants.insert(recipient.clone()); + } messages.push(ThreadMessage {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/imap_client.rs` at line 142, participant_count is only tracking senders via the IMAP parsing path, so recipients are missing from the thread participant set. Update the participant collection in the inbox parsing logic around imap_client and the email/thread aggregation so each message adds both the sender (email.from) and all recipient addresses to the same participants set. Make the change in the code that builds the thread metadata/participant list so participant_count reflects sender plus recipient(s), not just the inbound sender.src/openhuman/operator_inbox/parser.rs-92-93 (1)
92-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake reply-prefix detection case-insensitive.
Lowercase
re:subjects withoutIn-Reply-Toare currently treated as new messages, which can skew reply/thread scoring.Proposed fix
- let is_reply = - in_reply_to.is_some() || subject.starts_with("Re:") || subject.starts_with("RE:"); + let is_reply = in_reply_to.is_some() + || subject.trim_start().to_ascii_lowercase().starts_with("re:");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/parser.rs` around lines 92 - 93, The reply detection in parser.rs is still case-sensitive because it only checks subject prefixes in a fixed form. Update the is_reply logic in the parser function to treat the reply prefix case-insensitively so subjects like lowercase re: are classified as replies even without In-Reply-To, while preserving the existing in_reply_to check.
🧹 Nitpick comments (5)
docs/boost-vc-capability-plan.md (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this architecture section with the repo’s canonical module layout.
This still documents
engine.rs,rpc.rs, andabout_app/catalog.rs, but the repo guidance forsrc/openhuman/*/is export-onlymod.rs, business logic inops.rs, and controller wiring inschemas.rs(withtypes.rs/store.rs/tools.rs/bus.rsalongside it). As written, this page points follow-up work at the wrong files.As per coding guidelines,
src/openhuman/*/uses the canonicalmod.rs/types.rs/store.rs/ops.rs/schemas.rs/tools.rs/bus.rsshape, andsrc/openhuman/*/mod.rsmust stay export-focused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/boost-vc-capability-plan.md` around lines 69 - 77, The architecture section is pointing contributors to the wrong module layout for src/openhuman/*/. Update the capability-plan wording to match the repo’s canonical shape: export-focused mod.rs plus types.rs, store.rs, ops.rs, schemas.rs, tools.rs, and bus.rs, with business logic in ops.rs and controller wiring in schemas.rs. Also remove references that imply engine.rs, rpc.rs, or about_app/catalog.rs are the expected files, and align the guidance with the existing module symbols so follow-up work lands in the correct places.Source: Coding guidelines
src/openhuman/operator_inbox/imap_client.rs (2)
173-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse debug-level logging here.
The repo guidance asks Rust domain logging to use
debug/trace; this operational counter should not beinfoby default.Proposed fix
- info!( + debug!( thread_count = threads.len(), email_count = emails.len(), "[operator-inbox-imap] threads built" );As per coding guidelines, “Add debug logging … using
log/tracingatdebug/tracelevel in Rust.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/imap_client.rs` around lines 173 - 177, The logging in the thread-building step is too high-visibility for an operational counter. Update the log call in the IMAP thread construction path (the block emitting "[operator-inbox-imap] threads built") from info to debug, keeping the existing thread_count and email_count fields so it remains available for troubleshooting without defaulting to info-level output.Source: Coding guidelines
301-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the tests out to keep the module under the size limit.
This module is 538 lines; moving the
#[cfg(test)]block intoimap_client_tests.rsbrings the production module back under the repo’s ~500-line limit.As per coding guidelines, “Rust modules must be ≤ ~500 lines in size.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/imap_client.rs` around lines 301 - 537, The #[cfg(test)] block in imap_client.rs is pushing the module over the size limit. Move the entire tests module, including helpers like sample_emails and the unit tests for build_threads, validate_imap_config, validate_smtp_config, build_priority_prompt, build_draft_prompt, parse_priority_response, and extract_followup_deadline, into a separate imap_client_tests.rs file. Keep the production code in imap_client.rs and wire the new test module in so the existing test coverage remains unchanged.Source: Coding guidelines
src/openhuman/operator_inbox/schemas.rs (1)
538-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove schema tests or factor helpers to get under the module limit.
schemas.rsis 560 lines; moving tests toschemas_tests.rsand/or extracting repetitive field builders would satisfy the repo size guideline.As per coding guidelines, “Rust modules must be ≤ ~500 lines in size.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/schemas.rs` around lines 538 - 559, The schemas.rs module exceeds the repo size guideline, and the test-only block is contributing to the length. Move the #[cfg(test)] tests from the tests module into a separate schemas_tests.rs module, or factor repeated schema-building helpers out of all_controller_schemas/schemas-related code to reduce duplication and bring the main module under the limit. Keep the behavior of handlers_match, namespace, and unknown unchanged while relocating them so the core schemas module stays below the line cap.Source: Coding guidelines
src/openhuman/operator_inbox/engine.rs (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove this domain back to the canonical
openhuman/*layout.This PR introduces
engine.rs/connection.rs/poller.rsunderoperator_inbox, but the repo convention for newsrc/openhuman/*/domains is themod.rs+types.rs+store.rs+ops.rs+schemas.rs+tools.rs+bus.rsshape. Landing the inbox domain off that structure will make later controller wiring and maintenance inconsistent with the rest of the tree.As per coding guidelines,
src/openhuman/*/should use the canonicalmod.rs/types.rs/store.rs/ops.rs/schemas.rs/tools.rs/bus.rslayout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/operator_inbox/engine.rs` around lines 1 - 8, The operator_inbox domain is using non-canonical entry files, so move it back to the standard openhuman layout by replacing the standalone engine.rs-style entrypoint with a mod.rs-based module structure and splitting domain logic into the expected types.rs, store.rs, ops.rs, schemas.rs, tools.rs, and bus.rs modules. Update the operator_inbox module wiring so the engine-related symbols (like the triage/draft engine imports and now_epoch usage) are exposed through the canonical module tree instead of a custom file layout.Source: Coding guidelines
🤖 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.
Major comments:
In `@docs/boost-vc-capability-plan.md`:
- Around line 58-65: Update the non-goals list in the capability plan to match
the shipped operator inbox behavior. The current “No actual email/Slack
integration” entry is no longer accurate given the implemented operator inbox
flow; revise or remove it so it does not claim the inbox is schema-only or
transport-free. Keep the wording aligned with the actual operator inbox modules
and flow, especially the triage_message to generate_draft path, IMAP fetching,
SMTP reply sending, connection handling, and polling.
In `@docs/VC_CAPABILITIES.md`:
- Around line 3-18: Reframe the document in terms of planned roadmap rather than
shipped capability status: the current intro and module table in
VC_CAPABILITIES.md present all six modules as production-grade and implemented,
but the PR scope only supports operator_inbox plus foundation docs for the rest.
Update the wording in the intro and module descriptions so only operator_inbox
is described as current capability, while voice_assistant, live_captions,
voice_actions, chat_with_data, and guided_flows are clearly marked as planned or
forthcoming. Keep the existing structure, but avoid claiming RPC exposure,
safety guarantees, or Rust-core implementation for modules not evidenced by the
current stack.
In `@src/openhuman/about_app/catalog_data.rs`:
- Around line 1452-1538: The new VC foundation entries are being exposed through
the live capability catalog as if they are shipped Beta features. Update the
`Capability` entries in `catalog_data.rs` so only actually wired capabilities
stay surfaced as live Beta items, and move the unimplemented foundations out of
the public contract or mark them as non-live/planned instead of
`CapabilityStatus::Beta`. Use the existing `Capability` records and
`CapabilityStatus` values to keep `operator_inbox.triage` published while
preventing the other five `id` entries from advertising unavailable RPC
surfaces.
In `@src/openhuman/about_app/types.rs`:
- Around line 159-160: The new PrivacyDataKind::UserContent variant in
about_app/types.rs needs to be applied to the existing cloud-bound chat and
voice catalog entries so the privacy model stays consistent. Update the matching
entries in catalog_data.rs for conversation.send_text, conversation.send_voice,
and voice.ptt to use UserContent instead of Derived, keeping the same user
text/audio flow classification across the catalog API and the enum definition.
In `@src/openhuman/operator_inbox/connection.rs`:
- Around line 113-156: The inbox fetch loop is silently dropping malformed
messages when `msg.body()` is missing or `mail_parser::MessageParser::parse`
returns `None`, so `fetch` can return `Ok` with missing emails and no signal of
failure. Update the logic in the stream-processing loop in `connection.rs` to
detect these cases and surface an error instead of skipping them, using the
existing `LOG_PREFIX`, `msg_result`, and `FetchedEmail` flow so the caller can
tell which message failed to parse.
- Around line 31-166: The remote mail operations in fetch_new_emails are
unbounded, so a slow or half-open server can hang the RPC or poller
indefinitely. Wrap each external await in tokio::time::timeout, including
TcpStream::connect, the rustls TlsConnector::connect handshake,
async_imap::Client::login, session.select, session.uid_search, and
session.uid_fetch, and return timeout-specific errors from fetch_new_emails.
Apply the same timeout handling to the SMTP send path mentioned in the review,
using the existing LOG_PREFIX and the relevant connection/session methods to
locate the changes.
- Around line 35-38: The connection logging in the IMAP setup is exposing the
full mailbox username in routine info logs. Update the logging in the connection
flow around the existing info! call in operator_inbox::connection so it redacts
the username or omits it entirely, while still keeping non-sensitive context
like host and port. Use the existing LOG_PREFIX and the connection setup path to
locate the log statement, and ensure any replacement metadata does not reveal
account identifiers.
In `@src/openhuman/operator_inbox/engine.rs`:
- Around line 15-24: The triage flow drops email threading metadata because
triage_message and triage_message_with_priority only carry sender, subject, and
body, so send_reply cannot preserve the original conversation headers. Update
the operator_inbox engine to thread through message_id, in_reply_to, and
references from fetch_new_emails into the TriageRecord and the
triage_message/triage_message_with_priority path, then have send_reply use those
stored values when building the SMTP reply headers.
- Around line 50-62: The eviction logic in operator_inbox::engine does not
enforce MAX_RECORDS when there are no archived entries to remove, so the store
can still grow past the cap. Update the insertion path around the archive
eviction block so that insert only proceeds when capacity is available, or evict
a record unconditionally when store.len() >= MAX_RECORDS; use the existing
store, MAX_RECORDS, and TriageStatus::Archived logic in this branch to ensure
the limit is always respected.
In `@src/openhuman/operator_inbox/imap_client.rs`:
- Around line 17-41: `ImapConfig` and `SmtpConfig` currently derive `Debug`,
which can leak plaintext `password` and `oauth2_token` values in logs or error
contexts. Remove the auto-derived `Debug` on these structs and add custom
`Debug` implementations that redact sensitive fields, such as hiding `password`
entirely and only indicating whether `oauth2_token` is present. Keep the
existing `Serialize` and `Deserialize` derives unchanged.
In `@src/openhuman/operator_inbox/mod.rs`:
- Around line 12-13: The operator_inbox module is routing schema handlers
through a separate rpc layer instead of the canonical ops business-logic layer.
Update the module layout and handler wiring so the relevant functions in
schemas.rs delegate to ops.rs rather than super::rpc, and ensure the module
exports/include structure in mod.rs reflects ops.rs as the handler target
instead of rpc.rs.
In `@src/openhuman/operator_inbox/parser.rs`:
- Around line 95-100: The debug log in parser.rs’s email parsing path is
exposing PII by recording the raw sender address and subject. Update the debug!
call in the operator-inbox parser to remove from and subject entirely, and keep
only non-sensitive derived metadata such as attachments.len() and is_reply so
the log still identifies parse behavior without user content.
In `@src/openhuman/operator_inbox/poller.rs`:
- Around line 18-23: Reject zero-valued polling intervals in start_polling by
handling interval_secs before constructing the Duration; a value of 0 currently
creates a busy loop. Update the start_polling path to either return false/error
when interval_secs is Some(0) or clamp it to a safe minimum, and keep the
behavior consistent in the polling loop that uses the interval derived from
DEFAULT_POLL_INTERVAL_SECS.
- Around line 15-24: `start_polling`/`stop_polling` currently rely on the shared
`RUNNING` atomic only, so an old `tokio::spawn` can survive a stop→start cycle
and run alongside the new poller. Update the poller lifecycle to use a per-run
cancellation mechanism and retain the spawned task handle inside
`start_polling`/`stop_polling`, so stopping first signals the active task to
exit and awaits or aborts its `JoinHandle` before allowing a restart. Keep the
fix anchored around `RUNNING`, `start_polling`, and `stop_polling` so only one
IMAP polling loop can exist at a time.
In `@src/openhuman/operator_inbox/rpc.rs`:
- Around line 106-107: The triage lookup path in the handler currently
propagates errors with `?`, which breaks the JSON `{ok:false,error}` response
shape used elsewhere. Update the handler around `engine::get_triage(id)` so
lookup failures are caught and converted into the existing error response
instead of bubbling up, and apply the same treatment to the related
`try_llm_draft` flow mentioned in the comment. Keep the fix localized to the
handler logic so the response format stays consistent with the surrounding RPC
methods.
- Around line 60-63: The RPC path currently awaits LLM provider calls directly,
so stalled requests can hang triage/draft handling instead of failing fast. Wrap
the async calls in the relevant RPC handlers in rpc.rs (including the
chat_with_system usage and the other call at the referenced later location) with
a timeout boundary, and on timeout return the existing fallback/ok:false path
rather than blocking the request indefinitely. Use the surrounding handler
functions and provider call sites to apply the timeout consistently.
- Line 203: Validation is missing before narrowing RPC port values to u16 in the
operator inbox request handling, which can silently wrap invalid values. Update
the port parsing in the relevant request handlers that read port and smtp_port
from p to first keep them as u64, check they are within 0..=65535, and return an
error when out of range; only then convert to u16. Use the existing parsing
sites around the port and smtp_port extraction logic to apply the same fix
consistently.
In `@src/openhuman/operator_inbox/schemas.rs`:
- Around line 316-345: The schema handler wrappers in h_triage, h_draft,
h_followup, h_get, h_list, h_archive, h_fetch_inbox, h_send_reply,
h_start_poller, and h_stop_poller still delegate to super::rpc, but they should
route through the ops layer instead. Update each controller future to call the
corresponding super::ops function so schemas.rs stays as wiring only and
business logic remains in ops.rs.
- Around line 365-369: The inbox schemas currently expose raw IMAP/SMTP
passwords as required string inputs on the controller/tool surface. Update the
relevant schema builders in operator_inbox/schemas.rs, including the FieldSchema
entries for password in the IMAP and SMTP-related schema definitions, to accept
a credential or secret reference instead of a plaintext password, and resolve
the secret server-side inside the corresponding request handling logic rather
than in the JSON-RPC surface.
- Around line 97-114: The triage schema is stricter than the handler:
`handle_triage_message` accepts any one of `sender`, `subject`, or `body`, but
the `FieldSchema` entries currently mark all three as required. Update the
schema definition in `operator_inbox::schemas` so the generated contract matches
`handle_triage_message` by making these fields optional/allowing any single
field to be present, while keeping the `FieldSchema` entries for `sender`,
`subject`, and `body` aligned with the handler’s validation logic.
In `@src/openhuman/util.rs`:
- Around line 663-668: The now_epoch() helper is swallowing SystemTime clock
errors by falling back to 0, which can make fresh records appear oldest and
break retention/ordering. Update now_epoch() in util.rs to surface the time
error instead of returning a real-looking timestamp, and adjust callers such as
TriageRecord.created_at and the archived-record eviction path in
operator_inbox::engine to handle the failure explicitly.
- Around line 652-669: The new shared helpers in util.rs should be moved out of
the oversized catch-all module to keep it within the module-size cap. Relocate
uuid_v4 and now_epoch into a smaller, more focused shared module used by the
domain modules, and update any call sites to reference the new location instead
of openhuman::util so util.rs stops accumulating generic APIs.
---
Minor comments:
In `@src/openhuman/operator_inbox/imap_client.rs`:
- Line 142: participant_count is only tracking senders via the IMAP parsing
path, so recipients are missing from the thread participant set. Update the
participant collection in the inbox parsing logic around imap_client and the
email/thread aggregation so each message adds both the sender (email.from) and
all recipient addresses to the same participants set. Make the change in the
code that builds the thread metadata/participant list so participant_count
reflects sender plus recipient(s), not just the inbound sender.
In `@src/openhuman/operator_inbox/parser.rs`:
- Around line 92-93: The reply detection in parser.rs is still case-sensitive
because it only checks subject prefixes in a fixed form. Update the is_reply
logic in the parser function to treat the reply prefix case-insensitively so
subjects like lowercase re: are classified as replies even without In-Reply-To,
while preserving the existing in_reply_to check.
In `@src/openhuman/operator_inbox/rpc.rs`:
- Around line 310-319: The triage_rpc test is still exercising the live chat
provider path through handle_triage_message, which makes it flaky and
environment-dependent. Update the test to avoid any real LLM/network dependency
by mocking or stubbing the chat provider used by handle_triage_message, or by
injecting a deterministic test provider before calling it, so the assertion only
validates the triage logic and not external configuration.
---
Nitpick comments:
In `@docs/boost-vc-capability-plan.md`:
- Around line 69-77: The architecture section is pointing contributors to the
wrong module layout for src/openhuman/*/. Update the capability-plan wording to
match the repo’s canonical shape: export-focused mod.rs plus types.rs, store.rs,
ops.rs, schemas.rs, tools.rs, and bus.rs, with business logic in ops.rs and
controller wiring in schemas.rs. Also remove references that imply engine.rs,
rpc.rs, or about_app/catalog.rs are the expected files, and align the guidance
with the existing module symbols so follow-up work lands in the correct places.
In `@src/openhuman/operator_inbox/engine.rs`:
- Around line 1-8: The operator_inbox domain is using non-canonical entry files,
so move it back to the standard openhuman layout by replacing the standalone
engine.rs-style entrypoint with a mod.rs-based module structure and splitting
domain logic into the expected types.rs, store.rs, ops.rs, schemas.rs, tools.rs,
and bus.rs modules. Update the operator_inbox module wiring so the
engine-related symbols (like the triage/draft engine imports and now_epoch
usage) are exposed through the canonical module tree instead of a custom file
layout.
In `@src/openhuman/operator_inbox/imap_client.rs`:
- Around line 173-177: The logging in the thread-building step is too
high-visibility for an operational counter. Update the log call in the IMAP
thread construction path (the block emitting "[operator-inbox-imap] threads
built") from info to debug, keeping the existing thread_count and email_count
fields so it remains available for troubleshooting without defaulting to
info-level output.
- Around line 301-537: The #[cfg(test)] block in imap_client.rs is pushing the
module over the size limit. Move the entire tests module, including helpers like
sample_emails and the unit tests for build_threads, validate_imap_config,
validate_smtp_config, build_priority_prompt, build_draft_prompt,
parse_priority_response, and extract_followup_deadline, into a separate
imap_client_tests.rs file. Keep the production code in imap_client.rs and wire
the new test module in so the existing test coverage remains unchanged.
In `@src/openhuman/operator_inbox/schemas.rs`:
- Around line 538-559: The schemas.rs module exceeds the repo size guideline,
and the test-only block is contributing to the length. Move the #[cfg(test)]
tests from the tests module into a separate schemas_tests.rs module, or factor
repeated schema-building helpers out of all_controller_schemas/schemas-related
code to reduce duplication and bring the main module under the limit. Keep the
behavior of handlers_match, namespace, and unknown unchanged while relocating
them so the core schemas module stays below the line cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f5f966a-a943-40c1-9300-7109ac4fac56
📒 Files selected for processing (18)
Cargo.tomldocs/VC_CAPABILITIES.mddocs/boost-vc-capability-plan.mdsrc/core/all.rssrc/openhuman/about_app/catalog_data.rssrc/openhuman/about_app/types.rssrc/openhuman/mod.rssrc/openhuman/operator_inbox/connection.rssrc/openhuman/operator_inbox/engine.rssrc/openhuman/operator_inbox/imap_client.rssrc/openhuman/operator_inbox/mod.rssrc/openhuman/operator_inbox/parser.rssrc/openhuman/operator_inbox/poller.rssrc/openhuman/operator_inbox/rpc.rssrc/openhuman/operator_inbox/schemas.rssrc/openhuman/operator_inbox/types.rssrc/openhuman/util.rstests/json_rpc_e2e.rs
|
Closing — these need to be restacked properly with CI passing. Will resubmit once #4142 (shared wiring) lands and each domain PR compiles cleanly against it. |
Self-contained domain module — split from #2261 (PR 6/7). Depends on #4142.
9 files: types, engine, connection, imap_client, parser, poller, rpc, schemas, mod.
E2E: triage_message → generate_draft.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes