Skip to content

fix(drive): a malformed settings file reports "not authenticated", not the parse error #1636

Description

@newhoggy

Problem

A settings file that fails to parse is swallowed everywhere on the Drive path:

// src/cli/drive/helpers.rs:46
let settings = Settings::load().unwrap_or_default();
// src/drive/auth.rs:356, :471, :505, :618, :1120 — same

The result is fail-closed but inexplicable. Settings::default() has an empty
drive.accounts, so resolve returns ResolvedAccount::Unconfigured,
active_account_rules() returns vec![] (every write denied), and credential
resolution falls through to the DRIVE_CLIENT_ID/DRIVE_CLIENT_SECRET/DRIVE_REFRESH_TOKEN
environment variables — failing with DriveError::CredentialsNotFound, whose message
tells the operator to run drive auth login.

So a single typo in one permission rule presents as "you are not authenticated",
and re-running drive auth login will not fix it. The actual serde_json error —
which carries the line, column and field name — is discarded.

Why this matters more since #1612

ADR-0074 §7 leans on this path deliberately, and records it
as blunt. It is the mechanism that makes an older binary fail closed when it meets a
config containing a file_id rule (folder_id is a required field there, so the load
errors). That is the correct safety behaviour and should not change.

But #1612 also added three new ways for a current binary to reject a rule — naming
neither key, naming both, or recursive: true on a file_id — each a deliberate hard
error precisely so it cannot be silently inert. Every one of those now surfaces as
CredentialsNotFound, which is the least useful thing the tool could say about them.

The validation was designed to be loud. This path makes it quiet.

Proposal

Propagate the load error rather than discarding it. active_account_rules() already
returns Result, so the change is local:

let settings = Settings::load().context(
    "Failed to load ~/.omni-dev/settings.json; Drive write-permission rules cannot be \
     evaluated. Fix the file, or unset it to run with no rules (every write denied).",
)?;

Both before and after are fail-closed — the difference is only whether the operator is
told what actually happened.

Design questions this needs answering first

  • Scope. active_account_rules() alone, or the five auth.rs call sites too? The
    auth sites have a legitimate reason to degrade: an absent settings file is normal
    and must keep working via env-var credentials. Distinguishing "absent" (fine) from
    "present but malformed" (an error) is the real fix, and Settings::load may need to
    expose that difference rather than each caller guessing.
  • Blast radius. Any command touching Drive settings would start hard-failing on a
    malformed file where it currently degrades. That is the point, but it is a behaviour
    change for anyone whose settings file is quietly broken today and who has not noticed
    because env-var credentials carried them.
  • Does the same swallow exist elsewhere? Settings::load_mcp does it intentionally
    (issue feat(mcp): expose MCP server configuration in settings.json #620: the MCP server must boot regardless). Gmail and other integrations should
    be audited rather than assumed to differ.
  • Test posture. The fail-closed guarantee ADR-0074 §7 relies on must stay pinned —
    whatever changes, "an unparseable rule set never widens a permission" has to remain a
    test, not a claim.

Scope

src/cli/drive/helpers.rs, src/drive/auth.rs, possibly src/utils/settings.rs
(to distinguish absent from malformed), and ADR-0074 §7's text once the diagnostic
improves.

Refs #1612, ADR-0074 §7, ADR-0073 §3

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions