fix: surface silent failures across agent, auth, and desktop#10084
fix: surface silent failures across agent, auth, and desktop#10084undivisible wants to merge 3 commits into
Conversation
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for tightening these fail-open/silent-failure paths. I reviewed the diff and the direction looks good: the Rust agent VM recovery paths now log and fail closed for authoritative Firestore transitions, use_custom_token=true no longer returns a partial success when custom-token minting is unavailable, and the desktop/Electron changes surface previously swallowed errors without changing the fallback behavior.
A few maintainer notes before this should merge:
- This touches auth, agent VM state, and CI/workflow behavior, so I’m not going to formally approve it from automation; it needs human maintainer sign-off on the intended fail-closed UX and the
no-changelog-neededCI bypass semantics. - GitHub currently reports the branch as not cleanly mergeable (
DIRTY), so it needs to be brought up to date withmainbefore final review/merge. - I could not run the Rust check locally in this sandbox because
cargois not installed here, but the PR’s GitHub checks show the Rust Backend and Desktop Swift Build & Tests jobs passing.
No blocking code issue jumped out from the patch review; after the merge conflict is resolved, I’d mainly want a maintainer to confirm the auth/agent failure behavior is the desired product behavior.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Scope and behavioral-proof follow-upThe fail-closed direction is promising, but this remains a cross-cutting auth/agent-state/workflow PR. Before merge:
A maintainer still needs to explicitly sign off on the intended user-facing fail-closed UX. |
adb59ed to
4e3b27d
Compare
533013a to
767f7ea
Compare
|
retriggering CI |
|
Hey @undivisible 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
767f7ea to
5d00a1f
Compare
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up on this head. The fail-closed/silent-failure direction still looks worthwhile, but I’m going to keep this in changes-requested state before merge.
Blocking items that still apply on f0955b8:
- The Desktop Swift CI
no-changelog-needed/--skip-changelogbehavior is still bundled with the auth and agent-state runtime hardening. That workflow policy change is independently reviewable/rollbackable, so please split it from the auth/agent/desktop failure-surfacing changes. - I still don’t see focused behavioral coverage for the Rust auth custom-token error contract (
use_custom_token=trueshould hard-error rather than return a partial payload) or for failed Firestore/GCE authoritative agent-VM transitions. The existing CI is helpful, but these are security/user-state-sensitive behavior changes and need direct regression coverage. - A human maintainer still needs to sign off on the intended fail-closed UX for the auth and agent-state paths before merge.
No security attack concern from my review; this is normal heightened review because the PR touches auth, agent VM state, and workflow/changelog enforcement.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
@undivisible this looks good to merge on review but has merge conflicts with |
Fail closed when custom-token minting is unavailable and when authoritative Firestore agent-VM transitions fail. Log previously swallowed Electron/macOS error paths. Drop the unused memories evidence placeholder. Add unit coverage for the custom-token error contract and Firestore fail-closed / recovered-IP helpers. Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
f0955b8 to
3748b45
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Addressed CHANGES_REQUESTED on
Human maintainer sign-off on fail-closed UX still needed per prior review. |
Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
Restore backend/database/memories.py so this PR no longer triggers the full backend unit suite (unrelated dead-code removal). Add HTTP-level auth token fail-closed tests and an async agent transition contract test alongside the existing helper unit coverage. Failure-Class: none Co-authored-by: Cursor <cursoragent@cursor.com>
Review blocker follow-up (
|
| Contract | Test |
|---|---|
use_custom_token=true hard-errors instead of partial payload |
auth_token_with_custom_token_fails_closed_when_mint_unavailable (POST /v1/auth/token → 400 custom_token_unavailable, no id_token) |
| Mint skipped when custom token not requested | auth_token_without_custom_token_returns_id_token_without_mint |
| Authoritative Firestore agent-VM transition fails closed | authoritative_agent_vm_transition_fails_closed_in_async_handler (+ existing fail_closed_firestore_transition_* helper tests) |
| Custom-token helper | existing require_custom_token_* unit tests |
Local verification: cargo test auth_token_ fail_closed authoritative_agent in desktop/macos/Backend-Rust.
3. Backend unit suite failure
Root cause: the earlier backend/database/memories.py deletion forced the full backend unit suite (backend/database/** → full run). That suite was failing in parallel CI due to unrelated langchain_core stub pollution in other test files — not caused by the auth/agent/desktop runtime changes.
Fix: restored memories.py on this branch so backend unit selection is now “no backend files changed” and the suite should skip for this PR.
Ready for human maintainer sign-off (cannot be automated)
Please confirm the intended fail-closed UX for:
- Auth token exchange — desktop clients sending
use_custom_token=truenow receive400 { error: "custom_token_unavailable" }when minting is unavailable, instead of a partial token payload. Clients must surface/handle that hard error. - Agent VM status recovery — when an authoritative Firestore write during stopped/missing-VM recovery fails, the handler returns 500 (via
?onfail_closed_firestore_transition) rather than proceeding as if the transition succeeded.
Desktop failure-surfacing (Swift APIClient+Apps, Windows token/screen/retention logging) remains unchanged from 3748b45a26.
Head for re-review: 8a15569024
What changed and why
Stops fail-open and silent-failure paths on the current (non-Tauri) desktop/backend stack so errors surface instead of looking healthy.
use_custom_token=truereturnscustom_token_unavailableinstead of a partial token payload when minting fails/unimplemented.recompute_evidenceplaceholder (no callers).catch {}.CI
no-changelog-needed/ Desktop Swift changelog-bypass was split out of this PR (not included).Product invariants affected
How it was verified
cargo test --offline --bin omi-desktop-backend require_custom_token— passcargo test --offline --bin omi-desktop-backend fail_closed_firestore— passcargo test --offline --bin omi-desktop-backend recovered_vm_external— passFailure class (fixes)
Failure-Class: none