Skip to content

fix(cli): report a served write refused at auth, policy or contract as not started - #981

Merged
ragnorc merged 4 commits into
ModernRelay:mainfrom
ragnorc:claude/gracious-hermann-fd3657
Oct 16, 2026
Merged

ragnorc merged 4 commits into
ModernRelay:mainfrom
ragnorc:claude/gracious-hermann-fd3657

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #976.

Problem

Failure::classify reported every failure of a dispatched served write as execution: unknown, effects: unknown, action: reconcile, except a typed 429 and a verified 412. A server that refused the write at authentication (401 unauthorized), by policy (403 forbidden) or at the HTTP contract check (400 api_contract_mismatch) therefore told callers to reconcile a write that never started.

Change

A dispatched write answered with one of those three codes, under its own status, in a response that carries the server's API contract header, now reports execution: not_started, effects: none, action: refresh. The exit code stays 1. admission_refusal maps each pre-admission refusal (the 429 retry and these three) from its status and code to its action, and the guards apply once:

  • the refusal must be the command's only effectful request and no writable open happened (single), so a compound command keeps its earlier effects;
  • the error message must be non-empty;
  • the body must carry no detail that contradicts a plain refusal (is_plain_refusal); a 403 that also reports recovery_required stays recover;
  • a bare status, a code under another status, or a response without the contract header (for example a proxy's 403) stays unknown / reconcile.

Server audit: these codes are produced only before effects

  • 401 unauthorized: require_bearer_auth is the outermost per-graph route layer and runs before the contract check, graph resolution and the handler. The other producers (authorize with a policy but no actor, graph discovery) are also pre-admission.
  • 400 api_contract_mismatch: produced only by http_contract::require_contract, which runs after authentication and before graph resolution and body work.
  • 403 forbidden, HTTP layer: every write handler calls authorize_request before workload.try_admit and owned_write (run_mutate, authorize_load_scope in both load routes, branch_create_body, branch_delete_body, admit_branch_merge, authorize_blob_write). The one policy check that follows an effect, the source deletion after branch merge --delete-branch, converts its denial into branch_delete_error_details on a 200 response, never a 403. Commit publish/drop and unattached-child denials answer 404.
  • 403 forbidden, engine layer: OmniError::Policy (the only source of an engine 403 in ApiError::from_omni) is produced only by Omnigraph::enforce and the offline storage upgrade. Every enforce call is the first step of its _as entry point (resolve_write_route for mutate, load_input_as, branch_create_as, branch_create_from_as, branch_delete_as, branch_merge_as_with_options, write_blob_cell_as, schema apply, table ops). The one nested call, a load's implicit branch_create_from_as for --from, checks branch_create before the fork; nothing after the fork calls enforce. operations.rs already pins a policy error as not completion-uncertain.
  • schema apply never runs against a server, and cluster apply is outside the data-write outcome contract.

Tests

  • cli_data::data_write_outcomes_and_retry_permission_issue_466_issue_976 (renamed owner of the outcome matrix): each row now states its full expected outcome. New rows run the eight served write forms (mutate stored, inline and branch create statement, branch create, branch delete, branch merge, load append, load --branch --from), plus blob put and blob clear, against 401 unauthorized, 403 forbidden and 400 api_contract_mismatch (not_started / none / refresh, exit 1), plus three controls: a 403 without a code, forbidden under 400, and a 403 that also reports recovery work.
  • cli_data::remote_json_errors_preserve_server_codes_and_details: the 403 forbidden mutate row now expects not_started.
  • system_remote::remote_policy_enforces_branch_first_cli_workflow (real server and policy): the denied mutate and merge, plus a new denied load, denied branch delete and unknown-token mutate, report not_started / none / refresh with their status, and main's snapshot is unchanged across the denials.
  • client::tests::command_outcome_keeps_earlier_effects_and_scopes_independent_invocations (in-source): a 403 alone is not_started; a 403 after an earlier successful write keeps effects: unknown.

After merging main (#967, #990 and the rest up to b6ad205): the CLI integration binary passes (309 passed, 0 failed, 10 ignored), and the CLI's in-source tests pass (160 passed). Clippy on omnigraph-cli and omnigraph-server (--all-targets -D warnings) was clean on a0b3ee8, before the merge; CI's Lint (clippy) covers the merged tree. The new authorization sites from main (diff reads, load csv) also check before admission.

Earlier local runs (RUST_MIN_STACK=16777216):

  • Before the fix, both cli_data tests failed on every 401, 403 and contract-400 cell reached (left: unknown/unknown/reconcile, right: not_started/none/refresh); no control row failed.
  • After the fix: the three focused tests pass; the whole CLI integration binary passes (294 passed, 0 failed, 8 ignored); the CLI's in-source tests pass (143 passed, 6 ignored).
  • cargo fmt --all --check, scripts/check-docs.py, scripts/check-agents-md.sh and the release-note preview pass.

Docs

  • docs/user/operations/troubleshooting.md, Failed data-write commands: the three refusals and the conditions under which they count.
  • docs/dev/writes.md, Policy and attribution: the server and engine promise this relies on (these refusals come only before a write's first durable effect), with a checklist item for new writers and a pointer at the server's Policy→403 mapping.
  • changelog.d/served-write-refusals-are-not-started.changed.md.

Related: #466.

…s not started

The server answers 401 `unauthorized`, 403 `forbidden` and 400
`api_contract_mismatch` before it admits a write: bearer authentication,
the contract header check and every policy gate (HTTP and engine) run
ahead of admission and staging. The CLI still reported these refusals as
`execution: unknown, effects: unknown, action: reconcile`, sending callers
to reconcile writes that never began.

Classify a dispatched write answered with one of these codes under its
own status, in a response that carries the contract header, as
`not_started` / `none` / `refresh`. Like the 429 retry rule it requires
the refusal to be the command's only effectful request with no writable
open, and a body with no contradicting detail. Exit code stays 1.

Fixes ModernRelay#976.
Comment thread crates/omnigraph-cli/src/command_outcome.rs Outdated
Comment thread crates/omnigraph-cli/src/command_outcome.rs Outdated
Comment thread crates/omnigraph-cli/tests/integration/cli_data.rs
@cursor

cursor Bot commented Oct 14, 2026

Copy link
Copy Markdown

Review of #981 at a0aedd5

Recommendation: approve. I found no blocking issues. I post this review as a comment because this account cannot submit a formal approval. The three inline comments are optional or recommended follow-ups.

Why, what, how, impact

  • Why. A served write that the server refuses at bearer auth, at the API contract check or at policy did not change the graph. Before this PR, the CLI reported such a refusal as unknown/unknown/reconcile. That told the operator to check for effects that cannot exist.

  • What. Failure::classify adds a refused rule. 401 unauthorized, 403 forbidden and 400 api_contract_mismatch now give not_started/none/refresh. The exit code stays 1.

  • How. The rule applies only when all of these are true:

    • the status and the code match exactly
    • the body has no detail fields (is_plain_refusal)
    • the command sent exactly one effectful request and opened no local writable storage (single)

    A bare proxy 403, a misplaced code, a refusal with recovery work and a refusal after an earlier write all stay unknown. Every RemoteErrorCli comes from a response that has the contract header, because GraphHttpClient::send rejects other responses as ApiContractError. So a proxy cannot fake the refusal.

  • Impact. Scripts and agents get a correct "nothing happened, fix the input" signal for credential, grant and client-version errors. No exit code, wire format, server path or Lance surface changes.

Blocking findings

None.

Optional and recommended findings (inline)

  1. Recommended. Write the cross-component contract in docs/dev/writes.md (comment on command_outcome.rs 179-181). The CLI now depends on a server and engine promise that this PR does not state: auth, contract and policy refusals occur only before the first durable effect. I audited all served data-write paths, and the promise holds today.
  2. Recommended. Add blob put and blob clear rows to the outcome matrix (comment on cli_data.rs 2447). The rule changes blob outcomes too, but no test pins that. I ran the rows locally: they pass at the head and fail at the base.
  3. Optional. Fold retry and refused into one pre-admission refusal table (comment on command_outcome.rs 182-193). Today the two closures repeat the same guards, and the copies already differ on the non-empty error check.

Tradeoffs and software liability

  • Consistent with the existing pattern. The rule copies the shape of the 429 too_many_requests retry rule. It trusts a typed code only under its own status and only for a single effectful request. That keeps the evidence model in one function.
  • What the rule trusts. It trusts the server's ordering: auth, then contract, then policy, then admission. The ordering is correct now (see inline 1). The ongoing risk is a future enforce call after a fork or a staged write. The server cannot tell that 403 apart from an early one, so the CLI would report effects: none wrongly. One sentence in the developer guide, plus the existing snapshot tests, makes that drift visible in review.
  • Embedded writes do not change. An embedded policy denial stays unknown, because writable_open is true. The docs already describe this, and this PR keeps it.
  • Five more changes of this kind. If 408, 413 or new typed refusals join, the closures grow in parallel (see inline 3). That is a small cost today and not a reason to hold the PR.
  • Related work. fix(policy)!: refuse branch names that are not canonical #990 adds 400 bad_request refusals before effects. It is compatible and overlaps only as text in client.rs and cli_data.rs. No Lance version direction is relevant here.

Code references checked

  • crates/omnigraph-cli/src/command_outcome.rs 164-230 (classify, single, retry, refused, not_started)
  • crates/omnigraph-cli/src/graph_http.rs (send, has_contract), helpers.rs ~608-643 (remote_refusal)
  • Server: lib.rs (build_app layer order, ApiError::from_omni Policy→403), http_contract.rs, and handlers.rs:
    • require_bearer_auth 412-448
    • authorize_request ~660-715
    • authorize_load_scope 2323-2367
    • run_mutate 1540
    • branch create/delete 2727/2823
    • merge 2894/2988
    • blob 1041/1150/1199
  • Engine: db/omnigraph.rs enforce 1028-1048, loader/mod.rs load_input_as 318+, exec/merge.rs 4969, exec/mutation.rs 973-1031, exec/blob_write.rs 119
  • Existing guards: omnigraph-server/tests/integration/auth_policy.rs 295-312, parity_matrix.rs 685-709

Validation

Clean isolated checkout at head a0aedd5efa8cc5ed8c3fffbff8834a917dc7104e, base d8d2e3a8. Linux, 4 cores, toolchain from rust-toolchain.toml.

Command Result
cargo test --locked -p omnigraph-cli --test integration -- --exact cli_data::data_write_outcomes_and_retry_permission_issue_466_issue_976 cli_data::remote_json_errors_preserve_server_codes_and_details system_remote::remote_policy_enforces_branch_first_cli_workflow 3 passed
cargo test --locked -p omnigraph-cli --bin omnigraph command_outcome_keeps_earlier_effects_and_scopes_independent_invocations 1 passed
cargo test --locked -p omnigraph-cli --test integration (whole binary) 294 passed, 8 ignored (see limitations)
Red proof: command_outcome.rs reverted to d8d2e3a8, same 3 integration tests 3 failed. Every new 401/403/400 row reports unknown/reconcile
Blob experiment: 2 blob rows added to commands (inline 2), at head and at base Pass at head. At base the 6 blob refusal cases fail
python3 scripts/check-docs.py Documentation OK (267 files)
bash scripts/check-agents-md.sh OK
Release-note preview --target HEAD The new note renders. Its link resolves
PR CI at a0aedd5 28 pass, 5 skipped, CI Gate and Fix Regression Gate pass

I restored the worktree after each experiment.

Limitations.

  • The first full run of the integration binary had 57 failures. All of them were "server did not become ready" with empty stderr, and all came from host contention: parallel debug servers on 4 cores, while the harness fell back to cargo run. I reran those 57 with CARGO_BIN_EXE_omnigraph-server set to the built binary and --test-threads=2. All 57 passed.
  • I did not run these locally, and I rely on green CI for them:
    • cargo fmt, clippy and typos
    • the workspace test graph
    • the GQT corpus
    • S3 and Azure suites
  • I verified the server-ordering claims by reading the code and by the real-server test. I did not fault-inject a policy denial after an effect.

Fold the 429 retry rule and the auth, contract and policy refusal rule
into `admission_refusal`, one map from a code under its own status to
the action, with the shared guards applied once. The refusal rule now
also requires a non-empty error, as the retry rule did.

State the server and engine promise the classification relies on in
docs/dev/writes.md: 401, contract 400 and 403 refuse a served write
only before its first durable effect. Add blob put and blob clear to
the outcome matrix.
@ragnorc
ragnorc merged commit 2074a74 into ModernRelay:main Oct 16, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI: report a write refused at authentication, policy or the HTTP contract as not started

1 participant