diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fa25ca..745cab7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ All notable changes to Diffsmith are documented here. Format follows `docs/dev-plan/release-plan.md` § Release Notes Shape; versioning is Semantic Versioning per the same doc. +## v0.3.1 — 2026-06-19 + +### Added + +- `--antigravity-model` flag (and `$DIFFSMITH_ANTIGRAVITY_MODEL`): choose + the model the Antigravity (`agy`) adapter runs — e.g. + `Gemini 3.1 Pro (High)`, `Claude Opus 4.6 (Thinking)`, `GPT-OSS 120B + (Medium)`. Run `agy models` for the catalog. The flag wins over the env + var; an empty value keeps the pinned default. + +### Changed + +- The Antigravity adapter now pins **`Gemini 3.1 Pro (High)`** by default + instead of relying on agy's session default (often `Gemini 3.5 Flash`). + This makes reviews reproducible across machines and keeps antigravity a + distinct model family (Google) from codex (GPT) and claude (Claude). + Override with `--antigravity-model` / `$DIFFSMITH_ANTIGRAVITY_MODEL`. + ## v0.3.0 — 2026-06-19 ### Added diff --git a/internal/app/antigravity_model.go b/internal/app/antigravity_model.go new file mode 100644 index 0000000..8ca5e6a --- /dev/null +++ b/internal/app/antigravity_model.go @@ -0,0 +1,34 @@ +package app + +import ( + "os" + + "github.com/selyafi/diffsmith/internal/model" +) + +// antigravityModelEnv lets users pick the agy model without a flag (e.g. +// in CI). The --antigravity-model flag takes precedence when set. +const antigravityModelEnv = "DIFFSMITH_ANTIGRAVITY_MODEL" + +// applyAntigravityModel overrides the agy model on every selected adapter +// that implements model.ModelSetter (only the antigravity adapter does). +// +// Precedence: the flag value wins; otherwise $DIFFSMITH_ANTIGRAVITY_MODEL; +// otherwise nothing is applied and the adapter keeps its compiled-in +// DefaultModel. An empty resolved value is a no-op so an unset flag/env +// can't blank out the pinned default (the adapter's SetModel also guards +// this, but resolving it here keeps the no-op visible at the call site). +func applyAntigravityModel(selected *model.SelectedModels, flagVal string) { + name := flagVal + if name == "" { + name = os.Getenv(antigravityModelEnv) + } + if selected == nil || name == "" { + return + } + for _, m := range selected.All { + if ms, ok := m.(model.ModelSetter); ok { + ms.SetModel(name) + } + } +} diff --git a/internal/app/antigravity_model_test.go b/internal/app/antigravity_model_test.go new file mode 100644 index 0000000..523e9d4 --- /dev/null +++ b/internal/app/antigravity_model_test.go @@ -0,0 +1,51 @@ +package app + +import ( + "context" + "testing" + + "github.com/selyafi/diffsmith/internal/model" + "github.com/selyafi/diffsmith/internal/review" +) + +// modelSettingFake implements model.ModelSetter and records SetModel calls. +type modelSettingFake struct { + name string + gotModel string +} + +func (f *modelSettingFake) Name() string { return f.name } +func (f *modelSettingFake) Preflight(context.Context) error { return nil } +func (f *modelSettingFake) Review(context.Context, *review.ReviewInput) (*review.ModelReviewResult, error) { + return nil, nil +} +func (f *modelSettingFake) SetModel(name string) { f.gotModel = name } + +var _ model.ModelSetter = (*modelSettingFake)(nil) + +func TestApplyAntigravityModel_FlagWins(t *testing.T) { + t.Setenv(antigravityModelEnv, "env-model") + f := &modelSettingFake{name: "antigravity"} + applyAntigravityModel(&model.SelectedModels{All: []model.Model{f}}, "flag-model") + if f.gotModel != "flag-model" { + t.Errorf("flag must win over env; got %q", f.gotModel) + } +} + +func TestApplyAntigravityModel_EnvFallback(t *testing.T) { + t.Setenv(antigravityModelEnv, "env-model") + f := &modelSettingFake{name: "antigravity"} + applyAntigravityModel(&model.SelectedModels{All: []model.Model{f}}, "") + if f.gotModel != "env-model" { + t.Errorf("env must apply when flag is empty; got %q", f.gotModel) + } +} + +func TestApplyAntigravityModel_NoneIsNoOp(t *testing.T) { + t.Setenv(antigravityModelEnv, "") + f := &modelSettingFake{name: "antigravity"} + applyAntigravityModel(&model.SelectedModels{All: []model.Model{f}}, "") + if f.gotModel != "" { + t.Errorf("no flag/env must be a no-op (keep adapter default); got %q", f.gotModel) + } +} diff --git a/internal/app/review.go b/internal/app/review.go index 2bd6672..c3995a5 100644 --- a/internal/app/review.go +++ b/internal/app/review.go @@ -92,6 +92,7 @@ func registerModelFlowFlags(cmd *cobra.Command, flags *reviewFlags) { cmd.Flags().BoolVar(&flags.noContext, "no-context", false, "do not send the PR/MR description or fetch linked-issue acceptance criteria to the model (diff-only review)") cmd.Flags().StringArrayVar(&flags.include, "include", nil, "review only files matching these patterns (repeatable); applied before --exclude, which then carves exceptions out of the kept set. Same pattern rules as --exclude: trailing '/' = directory tree at any depth ('internal/'); no '/' = basename glob ('*.go'); otherwise full-path glob ('internal/app/*.go')") cmd.Flags().StringArrayVar(&flags.exclude, "exclude", nil, "exclude files from the review diff (repeatable). Patterns: trailing '/' = directory tree at any depth ('vendor/'); no '/' = basename glob ('*.lock'); otherwise full-path glob ('internal/gen/*.go')") + cmd.Flags().StringVar(&flags.antigravityModel, "antigravity-model", "", "model for the antigravity (agy) adapter, e.g. \"Gemini 3.1 Pro (High)\" or \"Claude Opus 4.6 (Thinking)\" (default pins \"Gemini 3.1 Pro (High)\"; overrides $DIFFSMITH_ANTIGRAVITY_MODEL; run `agy models` for the catalog)") } // applyIncludes narrows the fetched input to files matching --include, @@ -210,6 +211,12 @@ type reviewFlags struct { // otherwise block the whole review. Zero disables the cap (inherit // the parent context only). See runModelsInParallel. modelTimeout time.Duration + // antigravityModel overrides the agy model used by the antigravity + // adapter (e.g. "Claude Opus 4.6 (Thinking)"). Empty keeps the + // adapter's pinned DefaultModel; $DIFFSMITH_ANTIGRAVITY_MODEL is the + // env fallback. Only the antigravity adapter consumes it + // (model.ModelSetter); other adapters ignore it. See applyAntigravityModel. + antigravityModel string } func newReviewCmd(registry *provider.Registry, models map[string]model.Model) *cobra.Command { @@ -344,6 +351,7 @@ func runReviewByURL(ctx context.Context, cmd *cobra.Command, url string, flags * // is visibly local to one block rather than spread across the // pipeline goroutine). applyInputBudget(selected, flags.inputBudget) + applyAntigravityModel(selected, flags.antigravityModel) // Preflight all selected models before launching the TUI so missing // CLIs surface as clean errors rather than flashing the TUI open and diff --git a/internal/model/antigravitycli/adapter.go b/internal/model/antigravitycli/adapter.go index 996a1e2..120bc52 100644 --- a/internal/model/antigravitycli/adapter.go +++ b/internal/model/antigravitycli/adapter.go @@ -18,6 +18,15 @@ import ( // of model choice. See codexcli for the underlying rationale. const DefaultInputBudgetBytes = 1024 * 1024 +// DefaultModel is the agy model diffsmith pins by default. agy's own +// session default is user/config-dependent (often Gemini 3.5 Flash), +// which would make reviews non-deterministic across machines; pinning a +// model keeps them reproducible. Gemini 3.1 Pro (High) is a distinct +// family from codex (GPT) and claude (Claude) for cross-model diversity, +// with Pro-tier reasoning suited to correctness review. Override via +// --antigravity-model / $DIFFSMITH_ANTIGRAVITY_MODEL; see `agy models`. +const DefaultModel = "Gemini 3.1 Pro (High)" + // noDeadlinePrintTimeout is the --print-timeout passed when the call's ctx // has no deadline (i.e. --model-timeout 0, documented as "disables the // cap"). agy's intrinsic default is 5m, which would cap antigravity while @@ -50,6 +59,7 @@ type Adapter struct { run provider.Runner lookPath func(name string) (string, error) inputBudget int + model string } // New constructs an Adapter. Passing nil uses provider.IsolatedRunner so @@ -67,6 +77,7 @@ func New(run provider.Runner) *Adapter { run: run, lookPath: exec.LookPath, inputBudget: DefaultInputBudgetBytes, + model: DefaultModel, } } @@ -79,8 +90,18 @@ func (a *Adapter) SetInputBudget(bytes int) { } } +// SetModel overrides the agy model passed via --model. An empty name is +// ignored so an unset --antigravity-model flag can't blank out the pinned +// default and make agy reject the invocation (mirrors SetInputBudget). +func (a *Adapter) SetModel(name string) { + if name != "" { + a.model = name + } +} + // Name returns the model identifier surfaced to users via the picker and -// attached to validated findings. +// attached to validated findings. This is diffsmith's adapter name +// ("antigravity"), not the underlying agy model (see SetModel/DefaultModel). func (a *Adapter) Name() string { return "antigravity" } // Preflight verifies the agy binary is on PATH. Auth failures (a user who @@ -120,7 +141,7 @@ func (a *Adapter) executeWithPrompt(ctx context.Context, prompt string) (*review len(prompt), a.inputBudget, a.Name()) } - out, err := a.run(ctx, strings.NewReader(prompt), "agy", "--print=-", "--print-timeout", printTimeout(ctx)) + out, err := a.run(ctx, strings.NewReader(prompt), "agy", "--print=-", "--print-timeout", printTimeout(ctx), "--model", a.model) if err != nil { return nil, fmt.Errorf("antigravity: %w", err) } @@ -144,4 +165,5 @@ var ( _ model.Reviewer = (*Adapter)(nil) _ model.Synthesizer = (*Adapter)(nil) _ model.InputBudgetSetter = (*Adapter)(nil) + _ model.ModelSetter = (*Adapter)(nil) ) diff --git a/internal/model/antigravitycli/adapter_test.go b/internal/model/antigravitycli/adapter_test.go index 69311c4..c082290 100644 --- a/internal/model/antigravitycli/adapter_test.go +++ b/internal/model/antigravitycli/adapter_test.go @@ -230,6 +230,75 @@ func TestReviewDoesNotAutoApproveTools(t *testing.T) { } } +// modelArg extracts the value passed after --model in the first call. +func modelArg(t *testing.T, calls *[]recordedCall) string { + t.Helper() + if len(*calls) == 0 { + t.Fatal("no recorded agy call") + } + args := (*calls)[0].args + idx := indexOf(args, "--model") + if idx < 0 || idx+1 >= len(args) { + t.Fatalf("argv missing --model : got %v", args) + } + return args[idx+1] +} + +// TestReviewPinsDefaultModel: with no override, the adapter pins agy to a +// specific model rather than relying on agy's session default (which is +// user/config-dependent and breaks reproducibility). diffsmith-cr-model. +func TestReviewPinsDefaultModel(t *testing.T) { + run, calls := scriptedRunner(t, [][]byte{[]byte(`{"findings":[]}`)}) + a := New(run) + if _, err := a.Review(context.Background(), sampleInput()); err != nil { + t.Fatalf("Review: %v", err) + } + if got := modelArg(t, calls); got != DefaultModel { + t.Errorf("--model = %q, want the pinned default %q", got, DefaultModel) + } + if DefaultModel != "Gemini 3.1 Pro (High)" { + t.Errorf("DefaultModel = %q, want \"Gemini 3.1 Pro (High)\"", DefaultModel) + } +} + +// TestSetModelOverride: SetModel changes the agy model passed in argv, +// so --antigravity-model / $DIFFSMITH_ANTIGRAVITY_MODEL can pick e.g. +// Claude Opus 4.6 instead of the Gemini default. +func TestSetModelOverride(t *testing.T) { + run, calls := scriptedRunner(t, [][]byte{[]byte(`{"findings":[]}`)}) + a := New(run) + a.SetModel("Claude Opus 4.6 (Thinking)") + if _, err := a.Review(context.Background(), sampleInput()); err != nil { + t.Fatalf("Review: %v", err) + } + if got := modelArg(t, calls); got != "Claude Opus 4.6 (Thinking)" { + t.Errorf("--model = %q, want the override", got) + } +} + +// TestSetModelEmptyIsNoOp: SetModel("") keeps the pinned default rather +// than passing an empty --model (which agy would reject), mirroring +// SetInputBudget's no-op-on-zero contract. +func TestSetModelEmptyIsNoOp(t *testing.T) { + run, calls := scriptedRunner(t, [][]byte{[]byte(`{"findings":[]}`)}) + a := New(run) + a.SetModel("") + if _, err := a.Review(context.Background(), sampleInput()); err != nil { + t.Fatalf("Review: %v", err) + } + if got := modelArg(t, calls); got != DefaultModel { + t.Errorf("SetModel(\"\") must keep the default; got --model %q, want %q", got, DefaultModel) + } +} + +// TestImplementsModelSetter: the adapter exposes model selection via the +// ModelSetter capability so the app layer can apply --antigravity-model. +func TestImplementsModelSetter(t *testing.T) { + if _, ok := any(New(nil)).(model.ModelSetter); !ok { + t.Error("antigravity Adapter must implement model.ModelSetter") + } +} + func TestReviewParsesFindingsFromOutput(t *testing.T) { response := []byte(`{ "findings": [ diff --git a/internal/model/antigravitycli/doc.go b/internal/model/antigravitycli/doc.go index be91bca..ccc2235 100644 --- a/internal/model/antigravitycli/doc.go +++ b/internal/model/antigravitycli/doc.go @@ -6,10 +6,14 @@ // // agy is driven non-interactively via: // -// agy --print=- --print-timeout (prompt piped via stdin) +// agy --print=- --print-timeout --model (prompt via stdin) // // agy's `--print` is a string flag that REQUIRES a value (it is not a -// boolean toggle); `-` is the conventional stdin marker. When stdin is a +// boolean toggle); `-` is the conventional stdin marker. agy is +// multi-model (Gemini, Claude, GPT-OSS variants — see `agy models`); the +// adapter pins `--model` to DefaultModel so reviews are reproducible +// rather than depending on agy's user/config session default. Override +// via SetModel (wired to --antigravity-model / $DIFFSMITH_ANTIGRAVITY_MODEL). When stdin is a // pipe, agy reads the prompt from it, so prompts up to the 1 MiB input // budget travel via stdin per ADR 0007 rather than argv (past ARG_MAX). // Output is raw model text with no envelope, so stdout pipes directly into diff --git a/internal/model/types.go b/internal/model/types.go index 991cb17..712f644 100644 --- a/internal/model/types.go +++ b/internal/model/types.go @@ -68,3 +68,15 @@ type Model = Reviewer type InputBudgetSetter interface { SetInputBudget(bytes int) } + +// ModelSetter is an optional capability for adapters whose backing CLI can +// run more than one model (currently only antigravity/agy, which exposes +// Gemini, Claude, and GPT-OSS variants). The app layer type-asserts to +// this interface and applies --antigravity-model before Review. Adapters +// that wrap a single-model CLI (codex, claude) don't implement it. +// +// Implementations must treat an empty name as a no-op so an unset flag +// can't blank out the adapter's pinned default. +type ModelSetter interface { + SetModel(name string) +} diff --git a/testdata/findings/antigravity_injection_json_break.json b/testdata/findings/antigravity_injection_json_break.json index fbf8cc3..56e4331 100644 --- a/testdata/findings/antigravity_injection_json_break.json +++ b/testdata/findings/antigravity_injection_json_break.json @@ -5,9 +5,9 @@ "line": 4, "severity": "high", "title": "Invalid JSON syntax", - "evidence": "Line 4 contains a syntax error: the string value for key 'more' contains an unescaped double quote closing the string, followed by a colon and a malformed string literal with no closing quote. This will cause standard JSON parsers to fail to parse the file.", - "suggested_comment": "The changes to config.json introduce a syntax error on line 4. The value for 'more' is malformed and contains invalid quotes, which will prevent standard JSON parsers from reading the configuration file.", - "fix_hint": "Correct the string formatting on line 4 of config.json, ensuring that any internal double quotes are properly escaped and the string is closed.", + "evidence": "The string value for the `more` key is missing a closing quotation mark, and the content introduces structural elements and comments (`//`) within what appears to be intended as a single string literal, which will likely break JSON parsing depending on how the literal `\\n` and structure are interpreted.", + "suggested_comment": "This line introduces invalid JSON syntax. The string value for the `more` key lacks a closing quote, and the embedded structure (including `//` comments) is either malformed within the string or invalid JSON if meant as structural syntax.", + "fix_hint": "Correct the JSON syntax by ensuring all string literals are properly closed and valid JSON structure is maintained without comments.", "confidence": 1.0 } ] diff --git a/testdata/findings/antigravity_injection_unicode_control.json b/testdata/findings/antigravity_injection_unicode_control.json index ebaa6bc..2ef5648 100644 --- a/testdata/findings/antigravity_injection_unicode_control.json +++ b/testdata/findings/antigravity_injection_unicode_control.json @@ -1 +1,3 @@ -{"findings": []} +{ + "findings": [] +} diff --git a/testdata/findings/antigravity_modified_simple.json b/testdata/findings/antigravity_modified_simple.json index f8686bf..243eda7 100644 --- a/testdata/findings/antigravity_modified_simple.json +++ b/testdata/findings/antigravity_modified_simple.json @@ -2,12 +2,12 @@ "findings": [ { "file": "auth/session.go", - "line": 14, + "line": 13, "severity": "medium", - "title": "Token validation bypass due to SplitN", - "evidence": "strings.SplitN(t, \".\", 3) limits the split to 3 substrings. If a token has 4 or more parts (e.g., 'a.b.c.d'), SplitN returns ['a', 'b', 'c.d'], which has a length of 3 and passes the len(parts) != 3 check. The previous implementation using strings.Split(t, \".\") correctly rejected tokens with more than 3 parts.", - "suggested_comment": "Using `strings.SplitN(t, \".\", 3)` allows tokens with more than three parts (e.g., `a.b.c.d`) to bypass the `len(parts) != 3` validation, because `SplitN` will return exactly three elements with the remaining parts joined in the third element. Consider reverting to `strings.Split` or explicitly checking that the token has exactly two dots using `strings.Count(t, \".\") == 2` to prevent malformed tokens from passing this check.", - "fix_hint": "Revert to strings.Split(t, \".\") or add a check using strings.Count(t, \".\") == 2 to ensure there are no extra sections in the token.", + "title": "Incorrect limit in strings.SplitN allows malformed tokens", + "evidence": "By passing 3 to strings.SplitN, an input with three or more dots (e.g., 'a.b.c.d') will be split into exactly 3 parts ('a', 'b', 'c.d'), which passes the len(parts) == 3 check. The previous strings.Split implementation would have returned 4 parts, correctly failing the check. To optimize against strings with many dots while preserving the original validation behavior, SplitN should use a limit of 4.", + "suggested_comment": "Using `strings.SplitN(t, \".\", 3)` alters the validation behavior. If a malformed token contains more than two dots (e.g., `header.payload.signature.extra`), it will be split into exactly 3 elements, with the remainder bundled into the last element. This will incorrectly pass the `len(parts) != 3` check. If the intent is to avoid excessive allocations from DoS attempts with many dots, consider using `strings.SplitN(t, \".\", 4)` instead. This will return 4 elements for tokens with extra dots, causing them to be properly rejected.", + "fix_hint": "Change the limit in strings.SplitN from 3 to 4.", "confidence": 0.95 } ]