diff --git a/docs/operating.md b/docs/operating.md index 7ea3572..27c4e20 100644 --- a/docs/operating.md +++ b/docs/operating.md @@ -195,6 +195,12 @@ once, in any order, after the required flags. When the pinned model is absent from the runtime's effective catalog, `qa run` refuses at preflight with `model_unavailable` and prints this setup path — no launch, no silent fallback. +A profile source that exists but cannot be read safely is refused, never silently +replaced: a catalog root reached through a symlink, a symlinked or non-regular +profile file, an oversized or malformed file, or an unreadable `defaultProfile` +all end in `profile_invalid` before anything is launched. Only a catalog with no +`cortex/qa-profiles.json` file at all uses the placeholder `nan` route. + ## Walk the QA fleet end to end The showcase path (#286): from a fresh directory, install from a release, configure a diff --git a/internal/qapi/profile.go b/internal/qapi/profile.go index 8f0413b..7f84924 100644 --- a/internal/qapi/profile.go +++ b/internal/qapi/profile.go @@ -27,15 +27,23 @@ func resolveProfileRoute(root string, request AdmissionRequest) (qaroute.Resolve if request.Profile == "" { // A user-owned profile file with a defaultProfile configures the route // without a per-run flag: the fleet ships no models, each owner routes - // with their own (issue #298 decision). Without a valid source, the - // policy placeholder applies. + // with their own (issue #298 decision). Only an absent source leaves the + // policy placeholder in charge; a source that exists but cannot be read + // safely is refused, never silently bypassed (#324). if source, ok := inspectProfileSource(root); ok { - if bytes, ok := source.read(); ok { - if defaultID, ok := qaroute.DefaultProfileID(bytes); ok { - routeRequest.ProfileID = defaultID - return qaroute.Resolve(routeRequest, qaroute.Snapshot{Present: true, Bytes: bytes}) - } + bytes, read := source.read() + if !read { + return qaroute.ResolvedRoute{}, qaroute.Failure{Code: "profile_invalid"} } + defaultID, found := qaroute.DefaultProfileID(bytes) + if !found { + return qaroute.ResolvedRoute{}, qaroute.Failure{Code: "profile_invalid"} + } + routeRequest.ProfileID = defaultID + return qaroute.Resolve(routeRequest, qaroute.Snapshot{Present: true, Bytes: bytes}) + } + if profileSourcePresent(root) { + return qaroute.ResolvedRoute{}, qaroute.Failure{Code: "profile_invalid"} } return qaroute.Resolve(routeRequest, qaroute.Snapshot{}) } @@ -140,6 +148,24 @@ func profileDirectoryOrFileInfo(info os.FileInfo) bool { return profileDirectoryInfo(info) || profileFileInfo(info) } +// profileSourcePresent reports whether a profile source occupies the profile +// location below root. It observes existence only, never payload: an absent +// file is what entitles a default run to the policy placeholder, while a +// location that exists but cannot be inspected safely is a refusal. A profile +// directory without the file is still an absent profile (#324). +func profileSourcePresent(root string) bool { + directory := filepath.Join(root, profileDirectory) + info, err := os.Lstat(directory) + if os.IsNotExist(err) { + return false + } + if err != nil || info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return true + } + _, err = os.Lstat(filepath.Join(directory, profileFilename)) + return !os.IsNotExist(err) +} + // ProfileSelection reports the defaultProfile id and every profile id the // user-owned profile below root defines, from the same single safe snapshot // read route resolution uses. It reports ok=false when no source is present or diff --git a/internal/qapi/profile_test.go b/internal/qapi/profile_test.go index 22ca5d5..ec681a6 100644 --- a/internal/qapi/profile_test.go +++ b/internal/qapi/profile_test.go @@ -52,15 +52,21 @@ func TestProfileRouteUsesOneOriginalSnapshot(t *testing.T) { } } -func TestProfileRouteRejectsInvalidSources(t *testing.T) { - valid := []byte(`{"schemaVersion":1,"defaultProfile":"balanced","profiles":{"balanced":{"routes":{"test-runner":{"pi":{"provider":"nan","model":"qwen3.6","effort":"low"}}}}}}`) - for _, tc := range []struct { - name string - setup func(t *testing.T, root string) - }{ - {"missing", func(t *testing.T, root string) {}}, +const validProfileSnapshot = `{"schemaVersion":1,"defaultProfile":"balanced","profiles":{"balanced":{"routes":{"test-runner":{"pi":{"provider":"nan","model":"qwen3.6","effort":"low"}}}}}}` + +// profileSourceCase is one profile source that exists but cannot be inspected +// safely. An explicit --profile selection and a default run must refuse the +// same shapes, so both tests below share one table (#324). +type profileSourceCase struct { + name string + setup func(t *testing.T, root string) +} + +func unusableProfileSources(valid []byte) []profileSourceCase { + return []profileSourceCase{ {"symlink root", func(t *testing.T, root string) { target := testProfileRoot(t) + writeProfile(t, target, valid) if err := os.Remove(root); err != nil { t.Fatal(err) } @@ -70,7 +76,8 @@ func TestProfileRouteRejectsInvalidSources(t *testing.T) { }}, {"symlink parent", func(t *testing.T, root string) { target := t.TempDir() - if err := os.Symlink(target, filepath.Join(root, "cortex")); err != nil { + writeProfile(t, target, valid) + if err := os.Symlink(filepath.Join(target, profileDirectory), filepath.Join(root, profileDirectory)); err != nil { t.Fatal(err) } }}, @@ -105,7 +112,16 @@ func TestProfileRouteRejectsInvalidSources(t *testing.T) { writeProfile(t, root, []byte(`{"schemaVersion":1,"defaultProfile":"balanced","profiles":{"balanced":{"routes":{}},"balanced":{"routes":{}}}}`)) }}, {"trailing", func(t *testing.T, root string) { writeProfile(t, root, append(valid, []byte(" {}")...)) }}, - } { + {"default profile id not admitted", func(t *testing.T, root string) { + writeProfile(t, root, []byte(`{"schemaVersion":1,"defaultProfile":"9bad","profiles":{}}`)) + }}, + } +} + +func TestProfileRouteRejectsInvalidSources(t *testing.T) { + valid := []byte(validProfileSnapshot) + cases := append([]profileSourceCase{{"missing", func(t *testing.T, root string) {}}}, unusableProfileSources(valid)...) + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { root := testProfileRoot(t) tc.setup(t, root) @@ -117,6 +133,48 @@ func TestProfileRouteRejectsInvalidSources(t *testing.T) { } } +// TestProfileRouteRejectsUnusableDefaultProfile pins the fail-closed half of +// the default-profile rule (#324): a profile source that exists but cannot be +// read safely is a refusal, never a silent fallback to the policy placeholder. +func TestProfileRouteRejectsUnusableDefaultProfile(t *testing.T) { + for _, tc := range unusableProfileSources([]byte(validProfileSnapshot)) { + t.Run(tc.name, func(t *testing.T) { + root := testProfileRoot(t) + tc.setup(t, root) + got, failure := resolveProfileRoute(root, AdmissionRequest{Role: qarole.TestRunner, Backend: "pi"}) + if failure.Code != "profile_invalid" || got.PolicyVersion != "" { + t.Fatalf("resolveProfileRoute() = (%+v, %q), want profile_invalid instead of the placeholder fallback", got, failure.Code) + } + }) + } +} + +// TestProfileRouteDefaultPlaceholderRequiresAnAbsentSource keeps the +// legitimate half: with no profile source at all, the policy placeholder still +// applies (issue #298). +func TestProfileRouteDefaultPlaceholderRequiresAnAbsentSource(t *testing.T) { + for _, tc := range []struct { + name string + setup func(t *testing.T, root string) + }{ + {"no profile directory", func(t *testing.T, root string) {}}, + {"profile directory without a file", func(t *testing.T, root string) { + if err := os.MkdirAll(filepath.Join(root, profileDirectory), 0o755); err != nil { + t.Fatal(err) + } + }}, + } { + t.Run(tc.name, func(t *testing.T) { + root := testProfileRoot(t) + tc.setup(t, root) + got, failure := resolveProfileRoute(root, AdmissionRequest{Role: qarole.TestRunner, Backend: "pi"}) + if failure.Code != "" || got.ProfileID != "role-default" || got.Model != "qwen3.6" { + t.Fatalf("resolveProfileRoute() = (%+v, %q), want the placeholder route", got, failure.Code) + } + }) + } +} + func TestProfileRouteHonorsProfileDefinedModels(t *testing.T) { // A profile-covered role resolves with the profile's own provider/model // even when they differ from the placeholder defaults (issue #298: