Skip to content

[0.61.0] Expose Antigravity terminal strategy outcomes - #574

Open
Finesssee wants to merge 1 commit into
codex/port-0.61.0-provider-detailsfrom
codex/port-0.61.0-antigravity-outcome-parity
Open

Finesssee wants to merge 1 commit into
codex/port-0.61.0-provider-detailsfrom
codex/port-0.61.0-antigravity-outcome-parity

Conversation

@Finesssee

Copy link
Copy Markdown
Collaborator

Summary

  • Add a stable Antigravity strategy vocabulary for local, cli, and offline outcomes.
  • Add terminal strategy metadata to Antigravity JSON usage and diagnostic output.
  • Keep fallback internals opaque: output reports only the final selected strategy and never fabricates a probe trace.

Upstream reference

Ports the Windows analogue of upstream commit b6d885ca (Antigravity strategy outcome and diagnostic parity). Existing fallback order and provider fetch semantics are unchanged.

Terminal errors report strategy_outcome: "error" without a strategy_id because the current ProviderError surface does not preserve a trustworthy final-strategy marker. This avoids attributing an aggregate error to a fallback that may only have been attempted internally.

Validation

  • cargo fmt --all -- --check
  • cargo test --manifest-path rust/Cargo.toml antigravity --lib --locked (110 passed)
  • cargo test --manifest-path rust/Cargo.toml antigravity_json --lib --locked (4 passed)
  • git diff --check

No full build, dependency install, or generated artifact was run because the worktree is operating under a 50 GiB free-space floor.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d308e318-f6f6-4b20-9ea5-b2f7c770feda

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Finesssee

Copy link
Copy Markdown
Collaborator Author

Thermo-Nuclear Review: PR #574 — [0.61.0] Expose Antigravity terminal strategy outcomes

Verdict: APPROVE

Introduces a real typed model (AntigravityStrategyId) where stringly "local"/"cli"/"offline" literals were scattered, keeps fallback internals opaque (no fabricated probe traces), and adds focused coverage. The main structural opportunity — naming the strategy once at construction and carrying it on ProviderFetchResult instead of round-tripping through source_label parsing — is visible but not realized; that keeps this at approve-with-follow-up rather than request-changes given the PR is honest about the ProviderError limitation.

Structural regressions

  • none found (no behavior change to fallback order; confirmed the diff only rewrites source labels to enum values and adds output fields).

Missed simplification opportunities (code-judo)

  • The PR creates AntigravityStrategyId, then immediately degrades it back to a string and reconstructs it at the consumer. fetch_result(usage, strategy) converts to strategy.as_str() for source_label; cli_fallback.rs and mod.rs pass the enum; then cli/usage.rs and cli/diagnose.rs call strategy_from_source_label(&result.source_label) to recover it. That's a parse-the-string round trip across a boundary the PR itself just typed. The judo move: store Option<StrategyId>-like metadata on ProviderFetchResult (e.g. a generic source_tag: Option<&'static str> or a typed strategy: Option<AntigravityStrategyId> if core can't know provider types) at construction, and let the CLI read it directly. Then strategy_from_source_label — whose only reason to exist is that the typed info was thrown away at the boundary — disappears, along with the provider_id == ProviderId::Antigravity guards in two CLI files (the generic mechanism would emit strategy_id/strategy_outcome for any provider that populates the field, which is also how a future provider with strategies gets the same output for free). Three provider_id == Antigravity special cases across two CLI files is precisely the scatter rule 6 warns about; the field-on-result design deletes all three. Follow-up PR candidate, not a blocker: the round trip is total within this diff (every emit site passes the enum; every read site parses it back), so behavior is coherent and the vocabulary is stable.
  • render_json_error/render_json_result add strategy_outcome via json_result["strategy_outcome"] = ... string keys. With the field-on-result approach these three blocks (success id, success outcome, error outcome) collapse to one if let Some(strategy) = result.strategy { ... } writer. Keep in the same follow-up.

Spaghetti / branching complexity

  • Two new provider_id == ProviderId::Antigravity conditionals in cli/usage.rs and one in cli/diagnose.rs (final_strategy_id / final_strategy_outcome are at least named helpers with the guard inside — better than inline ifs, but still feature checks in shared CLI paths; see judo above). Within this PR's stated constraint (can't trust ProviderError to carry the strategy), the helpers are a reasonable containment; the constraint itself is the thing to fix in a follow-up.

Boundary / abstraction / type problems

  • strategy_from_source_label returning Option and the CLI treating None as "omit strategy_id" is honest (unknown source labels like "managed" produce no fabricated id) — good. But the dual encoding (enum in provider, string in result, enum again in CLI) is the boundary smell described above. Also strategy_outcome for errors is hardcoded "error" with no strategy_id — documented in the PR body and defensible; if the follow-up carries the strategy on the result, error paths via ProviderError remain the one untyped gap, and that should be called out in core rather than patched per-provider in the CLI.
  • AntigravityStrategyId is pub(crate) with as_str() — appropriate visibility; no cast/optionality churn.

File-size / decomposition concerns

  • none found. antigravity/mod.rs is 978 lines on the branch — approaching but under 1k; this PR adds ~40 lines. The 1k threshold is a real risk for the next antigravity PR: mod.rs already holds the provider, strategy enum, fallback orchestration, and parsing. Recommend splitting fallback orchestration (cli_fallback.rs exists — the managed-agy/strategy resolution in mod.rs could join it) before it crosses.

Lower-priority notes

  • Test naming: strategy_ids_are_stable_and_reject_unknown_sources covers the round trip and the "managed" rejection — good.
  • final_strategy_outcome(provider_id, succeeded) with the boolean guard is a thin wrapper whose only content is the Antigravity check; if kept, fine, but it's the same three-line pattern the follow-up would delete.
  • Diagnose output only reports the final attempt's strategy (single-attempt array today); the schema's Option fields with skip_serializing_if match the file's existing conventions.

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.

1 participant