diff --git a/cmd/codeaf/carried.go b/cmd/codeaf/carried.go index e04d20c3b6..821540366b 100644 --- a/cmd/codeaf/carried.go +++ b/cmd/codeaf/carried.go @@ -309,6 +309,25 @@ func runCarriedHost(ctx context.Context, inv *delegate.Invocation) error { } inv.Line = carriedResolvedModel(inv.Line, modelFlag, resolved) } + // AN AUXILIARY MODEL IS RESOLVED BY THE SAME DOOR. Leaving a light + // model as typed sent an unmarked floating alias where the main model + // had already been replaced by its catalog spelling. + for _, flag := range inv.Program.AuxiliaryModelFlags { + flag = strings.TrimSpace(flag) + word := strings.TrimSpace(inv.ExplicitFlags[flag]) + if word == "" { + continue + } + resolved := word + if road.resolveModel != nil { + resolved, err = road.resolveModel(word) + if err != nil { + return err + } + } + inv.Line = carriedResolvedModel(inv.Line, flag, resolved) + modelsToWarm = append(modelsToWarm, strings.Split(resolved, ",")...) + } if asked == "" && road.defaultSeat != nil { road.seat, err = road.defaultSeat() if err != nil { diff --git a/cmd/codeaf/carried_child_test.go b/cmd/codeaf/carried_child_test.go index 03c7fcf2f9..01ae4a6d37 100644 --- a/cmd/codeaf/carried_child_test.go +++ b/cmd/codeaf/carried_child_test.go @@ -141,6 +141,10 @@ func runAsCarriedChild() (int, bool) { restore := builtin.Override([]delegate.Delegate{fakeFolderProgram()}) defer restore() return execute(), true + case "model-flags": + restore := builtin.Override([]delegate.Delegate{fakeModelFlagsProgram()}) + defer restore() + return execute(), true case "real": return execute(), true } diff --git a/cmd/codeaf/carried_model_flags_test.go b/cmd/codeaf/carried_model_flags_test.go new file mode 100644 index 0000000000..3c07470ce9 --- /dev/null +++ b/cmd/codeaf/carried_model_flags_test.go @@ -0,0 +1,135 @@ +//go:build !windows + +package main + +import ( + "context" + "encoding/json" + "flag" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/Agent-Field/agentfield/sdk/go/ai" + "github.com/Agent-Field/codeaf/internal/agentsession" + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/delegate" + "github.com/Agent-Field/codeaf/internal/delegate/builtin" +) + +// A real child asks both declared model flags, so resolving only the main +// flag cannot hide behind a passing parent parser or a tolerant provider. +func fakeModelFlagsProgram() delegate.Delegate { + return delegate.Delegate{Name: "fake-model-flags", Summary: "tests model resolution", Guide: "Tests the shell's model flags.", Page: "delegates", Default: "run", ModelFlag: "model", AuxiliaryModelFlags: []string{"light"}, Commands: []delegate.Command{{Name: "run", Usage: "[flags] -- ", Summary: "asks both models", Bind: func(fs *flag.FlagSet) delegate.Body { + model := fs.String("model", "", "the working model") + light := fs.String("light", "", "the light model") + return func(ctx context.Context, host delegate.Host, _ []string) error { + client, err := agentsession.NewClient(host.Models(), 0) + if err != nil { + return err + } + for _, selected := range []string{*model, *light} { + if selected == "" { + selected = *model + } + reply, err := client.Complete(ctx, agentsession.Request{Model: selected, Messages: []ai.Message{{Role: "user", Content: []ai.ContentPart{{Type: "text", Text: "answer"}}}}}) + if err != nil { + host.Terminal(delegate.Ending{Status: delegate.StatusFail, Message: err.Error()}) + return nil + } + if reply.Text() != "done" { + return fmt.Errorf("unexpected answer") + } + } + host.Terminal(delegate.Ending{Status: delegate.StatusPass, Message: "both models answered"}) + return nil + } + }}}} +} + +func TestShellResolvesAuxiliaryModelsBeforeARealChildCallsTheProvider(t *testing.T) { + for _, tc := range []struct { + name, light string + refused bool + }{ + {"floating-alias", "fixture/floating-latest", false}, + {"native-id", "fixture/native", false}, + {"unknown-id", "fixture/missing", true}, + {"omitted-light", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + home := t.TempDir() + t.Setenv("CODEAF_HOME", home) + t.Setenv(config.ProfileDirEnv, home) + t.Setenv("OPENROUTER_API_KEY", "sk-or-v1-fixture") + t.Setenv(carriedChildEnv, "model-flags") + t.Setenv("DO_NOT_TRACK", "1") + t.Setenv("CODEAF_NO_UPDATE_CHECK", "1") + var mu sync.Mutex + var sent []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/models") { + rows := []any{} + for _, id := range []string{"~fixture/floating-latest", "fixture/native"} { + rows = append(rows, map[string]any{"id": id, "name": id, "context_length": 200000, "architecture": map[string]any{"input_modalities": []string{"text"}, "output_modalities": []string{"text"}}, "pricing": map[string]string{"prompt": "0", "completion": "0"}, "supported_parameters": []string{"tools", "tool_choice", "max_tokens"}}) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": rows}) + return + } + if r.Method != http.MethodPost || !strings.HasSuffix(r.URL.Path, "/chat/completions") { + http.NotFound(w, r) + return + } + var request struct { + Model string `json:"model"` + } + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + http.Error(w, err.Error(), 400) + return + } + mu.Lock() + sent = append(sent, request.Model) + mu.Unlock() + if request.Model != "~fixture/floating-latest" && request.Model != "fixture/native" { + http.Error(w, `{"error":{"message":"invalid model ID"}}`, 404) + return + } + w.Header().Set("Content-Type", "text/event-stream") + fmt.Fprintf(w, "data: {\"id\":\"fixture\",\"model\":%q,\"choices\":[{\"index\":0,\"delta\":{\"content\":\"done\"},\"finish_reason\":\"stop\"}],\"usage\":{\"prompt_tokens\":1,\"completion_tokens\":1,\"total_tokens\":2,\"cost\":0}}\n\ndata: [DONE]\n\n", request.Model) + })) + defer server.Close() + t.Setenv("CODEAF_BASE_URL", server.URL+"/api/v1") + oldCatalog, oldOut := sharedCatalog, carriedStdout + sharedCatalog, carriedStdout = newSharedCatalog(), &lockedBuffer{} + t.Cleanup(func() { sharedCatalog, carriedStdout = oldCatalog, oldOut }) + program := fakeModelFlagsProgram() + restore := builtin.Override([]delegate.Delegate{program}) + defer restore() + line := []string{"run", "--model", "fixture/floating-latest", "--dir", t.TempDir()} + if tc.light != "" { + line = append(line, "--light="+tc.light) + } + line = append(line, "--", "test both models") + err := runCarried(program, line) + mu.Lock() + got := append([]string(nil), sent...) + mu.Unlock() + if tc.refused { + if err == nil || !strings.Contains(err.Error(), "fixture/missing") || len(got) != 0 { + t.Fatalf("unknown light started calls or lacked its refusal: %v, %q", err, got) + } + return + } + wantLight := "~fixture/floating-latest" + if tc.light == "fixture/native" { + wantLight = tc.light + } + if exitCodeOf(err) != 0 || len(got) != 2 || got[0] != "~fixture/floating-latest" || got[1] != wantLight { + t.Fatalf("model flags = %q, error = %v", got, err) + } + }) + } +} diff --git a/cmd/codeaf/chatv3_surface_test.go b/cmd/codeaf/chatv3_surface_test.go index d0507cf1aa..fd3b1a2c3a 100644 --- a/cmd/codeaf/chatv3_surface_test.go +++ b/cmd/codeaf/chatv3_surface_test.go @@ -76,6 +76,12 @@ func TestRunSurfaceWiresTheDeferredLaunchCheckAndInstallerThroughRealInit(t *tes if err := os.WriteFile(target, []byte("old executable"), 0o755); err != nil { t.Fatal(err) } + // The installer reports the executable's physical path. A temporary + // directory on macOS can name that same file through a linked ancestor. + canonicalTarget, err := filepath.EvalSymlinks(target) + if err != nil { + t.Fatal(err) + } oldRun := runSurfaceProgram oldClient, oldExecutable := surfaceUpdateClient, surfaceExecutable oldRunningExecutable := surfaceRunningExecutable @@ -157,7 +163,7 @@ func TestRunSurfaceWiresTheDeferredLaunchCheckAndInstallerThroughRealInit(t *tes t.Fatal(err) } installed, err := os.ReadFile(target) - if err != nil || string(installed) != string(asset) || result.Path != target || resolved != 1 { + if err != nil || string(installed) != string(asset) || result.Path != canonicalTarget || resolved != 1 { t.Fatalf("result = %+v resolved = %d installed = %q, %v", result, resolved, installed, err) } } diff --git a/docs/changes/unreleased/1818-review-model-root-formatting.md b/docs/changes/unreleased/1818-review-model-root-formatting.md new file mode 100644 index 0000000000..868d0c53fd --- /dev/null +++ b/docs/changes/unreleased/1818-review-model-root-formatting.md @@ -0,0 +1,13 @@ +--- +kind: fixed +title: review resolves model words and linked roots and formats final answers +pr: 1818 +surface: [chat, engine, docs] +invalidates: + - "The shell resolved the main model flag but passed an explicit light model word unchanged. Review and security now resolve both through the same profile catalog, refusing unknown words before calls start." + - "Read tools compared resolved file paths against an unresolved repository root and refused ordinary files under a linked root. Both spellings of the owned root now work while external paths and links remain refused." + - "Final review answers requested JSON only through prompt instructions. They now request native schema formatting when the cached catalog publishes support, with one prompt-only fallback for an explicit unsupported-format refusal." + - "The macOS installer surface regression compared a physical output path with a linked temporary-path spelling. It now checks the exact resolved installation target while preserving installed-byte and invocation-count checks." +--- + +Every review answer is still checked against the original full schema. Invalid schemas, account or ceiling refusals, cancellation and provider outages do not take the formatting fallback. Model defaults and turn, time and cost ceilings are unchanged. diff --git a/internal/agentsession/app.go b/internal/agentsession/app.go index 91633917fd..fc9af45b7c 100644 --- a/internal/agentsession/app.go +++ b/internal/agentsession/app.go @@ -161,7 +161,8 @@ func (a *App) Harness(ctx context.Context, prompt string, schema map[string]any, // THE REPOSITORY IS THE APP'S, NOT THE CALLER'S. Every agent names the // folder it was handed; one that named another would be read nowhere. if project := strings.TrimSpace(opts.ProjectDir); project != "" && filepath.Clean(project) != filepath.Clean(root) { - if real, err := filepath.EvalSymlinks(project); err != nil || !within(root, real) { + realRoot, rootErr := filepath.EvalSymlinks(root) + if real, err := filepath.EvalSymlinks(project); err != nil || rootErr != nil || !within(realRoot, real) { return nil, fmt.Errorf("%s asked to read %s, which is not the repository this run reads", label, project) } } diff --git a/internal/agentsession/client.go b/internal/agentsession/client.go index 16a155f4ea..7f039da1dc 100644 --- a/internal/agentsession/client.go +++ b/internal/agentsession/client.go @@ -35,6 +35,7 @@ import ( "time" "github.com/Agent-Field/agentfield/sdk/go/ai" + "github.com/Agent-Field/codeaf/internal/config" "github.com/Agent-Field/codeaf/internal/delegate" "github.com/Agent-Field/codeaf/internal/provider/modelapi" ) @@ -57,6 +58,9 @@ type Client struct { // call waits; a test shortens both. pause func(attempt int) time.Duration held func() time.Duration + // supportsParameter reads only the profile's remembered capabilities. + // Unknown facts never opt a session into a new provider request field. + supportsParameter func(model, parameter string) (supported, known bool) } // NewClient opens the run's model API. It refuses an API with no address or no @@ -79,8 +83,9 @@ func NewClient(api delegate.ModelAPI, retries int) (*Client, error) { // NO CLIENT TIMEOUT: a thinking model can be quiet for minutes, and the // call's own context (the session's wall, the run's stop) is what ends // it. - http: &http.Client{}, - pause: func(attempt int) time.Duration { return time.Duration(2<