diff --git a/internal/qualitygate/semantic/client_test.go b/internal/qualitygate/semantic/client_test.go index 93bbed2b..b9647ab1 100644 --- a/internal/qualitygate/semantic/client_test.go +++ b/internal/qualitygate/semantic/client_test.go @@ -252,6 +252,64 @@ func TestClientRejectsOversizedRequestWithDefaultLimitBeforeHTTP(t *testing.T) { } } +func TestClientPostsBroadChangedSurfaceWithinRequestLimit(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + _, _ = w.Write([]byte(`{"choices":[{"message":{"content":"{\"verdict\":\"pass\",\"findings\":[]}"}}]}`)) + })) + defer srv.Close() + + c := Client{ + BaseURL: srv.URL, + APIKey: "test-key", + Model: "semantic-review-v1", + Timeout: time.Second, + MaxRequestBytes: 64 * 1024, + AllowedModels: map[string]bool{"semantic-review-v1": true}, + } + if _, err := c.Review(context.Background(), broadChangedFacts(434, 44)); err != nil { + t.Fatalf("Review() broad changed surface error = %v", err) + } + if calls != 1 { + t.Fatalf("server calls = %d, want 1", calls) + } +} + +func TestClientPostsBroadOutputCandidatesWithinRequestLimit(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("read request body: %v", err) + } + if len(body) > 64*1024 { + t.Fatalf("request bytes = %d, want <= 65536", len(body)) + } + if strings.Contains(string(body), "verbose_output_field_") { + t.Fatalf("request leaked verbose output fields: %s", string(body)) + } + _, _ = w.Write([]byte(`{"choices":[{"message":{"content":"{\"verdict\":\"pass\",\"findings\":[]}"}}]}`)) + })) + defer srv.Close() + + c := Client{ + BaseURL: srv.URL, + APIKey: "test-key", + Model: "semantic-review-v1", + Timeout: time.Second, + MaxRequestBytes: 64 * 1024, + AllowedModels: map[string]bool{"semantic-review-v1": true}, + } + if _, err := c.Review(context.Background(), broadOutputCandidateFacts(40)); err != nil { + t.Fatalf("Review() broad output candidates error = %v", err) + } + if calls != 1 { + t.Fatalf("server calls = %d, want 1", calls) + } +} + func TestClientFallsBackToJSONObjectWhenJSONSchemaIsRejected(t *testing.T) { var formats []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/qualitygate/semantic/prompt.go b/internal/qualitygate/semantic/prompt.go index 92e82a03..29c5d0e5 100644 --- a/internal/qualitygate/semantic/prompt.go +++ b/internal/qualitygate/semantic/prompt.go @@ -22,6 +22,7 @@ func BuildPrompt(f facts.Facts) []Message { {Role: "system", Content: strings.Join([]string{ "You review a projected lark-cli quality-gate semantic input view.", "Use only the provided JSON view.", + "The changed_summary may summarize broad changed surfaces; review only listed facts, not omitted summarized items.", "Use fact_ref values exactly when writing finding evidence.", "Only facts.commands, facts.skills, facts.errors, and facts.outputs fact_ref values may be blocker evidence.", "Evidence entries must be exact fact_ref strings such as \"facts.commands[0]\" with no explanations, labels, or suffix text.", diff --git a/internal/qualitygate/semantic/prompt_contract_test.go b/internal/qualitygate/semantic/prompt_contract_test.go index 997b2578..98948383 100644 --- a/internal/qualitygate/semantic/prompt_contract_test.go +++ b/internal/qualitygate/semantic/prompt_contract_test.go @@ -43,7 +43,7 @@ func TestBuildPromptContainsSemanticReviewContract(t *testing.T) { } } -func TestBuildInputViewSelectsChangedFactsWithStableRefs(t *testing.T) { +func TestBuildInputViewSelectsChangedReviewCandidatesWithStableRefs(t *testing.T) { view := BuildInputView(facts.Facts{ SchemaVersion: 1, Commands: []facts.CommandFact{ @@ -78,17 +78,17 @@ func TestBuildInputViewSelectsChangedFactsWithStableRefs(t *testing.T) { if got := singleRef(t, view.Skills); got != "facts.skills[1]" { t.Fatalf("skill ref = %q, want facts.skills[1]", got) } - if got := singleRef(t, view.SkillQuality); got != "facts.skill_quality[1]" { - t.Fatalf("skill quality ref = %q, want facts.skill_quality[1]", got) + if len(view.SkillQuality) != 0 { + t.Fatalf("skill quality len = %d, want 0 without diagnostics", len(view.SkillQuality)) } if got := singleRef(t, view.Errors); got != "facts.errors[1]" { t.Fatalf("error ref = %q, want facts.errors[1]", got) } - if got := singleRef(t, view.Outputs); got != "facts.outputs[1]" { - t.Fatalf("output ref = %q, want facts.outputs[1]", got) + if len(view.Outputs) != 0 { + t.Fatalf("outputs len = %d, want 0 without reject diagnostics", len(view.Outputs)) } - if got := singleRef(t, view.Examples); got != "facts.examples[1]" { - t.Fatalf("example ref = %q, want facts.examples[1]", got) + if len(view.Examples) != 0 { + t.Fatalf("examples len = %d, want 0 without diagnostics", len(view.Examples)) } if view.ChangedSummary.Commands != 1 || view.ChangedSummary.Skills != 1 || diff --git a/internal/qualitygate/semantic/view.go b/internal/qualitygate/semantic/view.go index c332dbb4..a0e6f6dc 100644 --- a/internal/qualitygate/semantic/view.go +++ b/internal/qualitygate/semantic/view.go @@ -61,8 +61,6 @@ type SkillQualityInput struct { facts.SkillQualityFact } -func (i SkillQualityInput) ref() string { return i.FactRef } - type ErrorInput struct { FactRef string `json:"fact_ref"` facts.ErrorFact @@ -71,8 +69,14 @@ type ErrorInput struct { func (i ErrorInput) ref() string { return i.FactRef } type OutputInput struct { - FactRef string `json:"fact_ref"` - facts.OutputFact + FactRef string `json:"fact_ref"` + Command string `json:"command"` + Domain string `json:"domain,omitempty"` + Changed bool `json:"changed,omitempty"` + Source string `json:"source,omitempty"` + IsList bool `json:"is_list"` + HasDefaultLimit bool `json:"has_default_limit"` + HasDecisionField bool `json:"has_decision_field"` } func (i OutputInput) ref() string { return i.FactRef } @@ -82,11 +86,9 @@ type ExampleInput struct { facts.CommandExample } -func (i ExampleInput) ref() string { return i.FactRef } - func BuildInputView(f facts.Facts) InputView { selected := newInputSelection(f) - selected.addChangedFacts() + selected.addChangedReviewCandidates() var viewDiagnostics []facts.DiagnosticFact for _, diag := range f.Diagnostics { @@ -115,37 +117,44 @@ func BuildInputView(f facts.Facts) InputView { } } -func (s *inputSelection) addChangedFacts() { +func (s *inputSelection) addChangedReviewCandidates() { for i, cmd := range s.f.Commands { - if cmd.Changed { + if cmd.Changed && commandReviewCandidate(cmd) { s.commands[i] = true } } for i, skill := range s.f.Skills { - if skill.Changed { + if skill.Changed && skillReviewCandidate(skill) { s.skills[i] = true } } - for i, skill := range s.f.SkillQuality { - if skill.Changed { - s.skillQuality[i] = true - } - } for i, errFact := range s.f.Errors { - if errFact.Changed { + if errFact.Changed && errorReviewCandidate(errFact) { s.errors[i] = true } } for i, output := range s.f.Outputs { - if output.Changed { + if output.Changed && outputReviewCandidate(output) { s.outputs[i] = true } } - for i, example := range s.f.Examples { - if example.Changed { - s.examples[i] = true - } - } +} + +func commandReviewCandidate(cmd facts.CommandFact) bool { + return cmd.NameConflictsExisting || cmd.FlagAliasConflict +} + +func skillReviewCandidate(skill facts.SkillFact) bool { + return skill.ReferencesInvalidCommand +} + +func errorReviewCandidate(errFact facts.ErrorFact) bool { + return errFact.Boundary && errFact.RequiredHint && errFact.HintActionCount == 0 +} + +func outputReviewCandidate(_ facts.OutputFact) bool { + // default_output is observe-first in the current rollout; reject diagnostics add exact output context. + return false } type inputSelection struct { @@ -172,6 +181,24 @@ func newInputSelection(f facts.Facts) *inputSelection { func (s *inputSelection) diagnosticContext(diag facts.DiagnosticFact) *inputSelection { out := newInputSelection(s.f) + switch { + case diag.Rule == "command_naming" || diag.Rule == "flag_naming": + s.addDiagnosticCommands(out, diag) + case strings.HasPrefix(diag.Rule, "default_output"): + s.addDiagnosticOutputs(out, diag) + case strings.HasPrefix(diag.Rule, "skill_"): + s.addDiagnosticSkills(out, diag) + s.addDiagnosticSkillQuality(out, diag) + s.addDiagnosticExamples(out, diag) + case strings.HasPrefix(diag.Rule, "example_dry_run"): + s.addDiagnosticExamples(out, diag) + case diag.Rule == "no_bare_helper_error": + s.addDiagnosticErrors(out, diag) + } + return out +} + +func (s *inputSelection) addDiagnosticCommands(out *inputSelection, diag facts.DiagnosticFact) { for i, cmd := range s.f.Commands { if diagnosticCommandMatches(diag, cmd.Path, cmd.CanonicalPath) || diagnosticMentions(diag, cmd.Path) || @@ -179,6 +206,9 @@ func (s *inputSelection) diagnosticContext(diag facts.DiagnosticFact) *inputSele out.commands[i] = true } } +} + +func (s *inputSelection) addDiagnosticSkills(out *inputSelection, diag facts.DiagnosticFact) { for i, skill := range s.f.Skills { if diagnosticLocationMatches(diag.File, diag.Line, skill.SourceFile, skill.Line) || diagnosticCommandMatches(diag, skill.CommandPath) || @@ -186,11 +216,17 @@ func (s *inputSelection) diagnosticContext(diag facts.DiagnosticFact) *inputSele out.skills[i] = true } } +} + +func (s *inputSelection) addDiagnosticSkillQuality(out *inputSelection, diag facts.DiagnosticFact) { for i, skill := range s.f.SkillQuality { if samePath(diag.File, skill.SourceFile) { out.skillQuality[i] = true } } +} + +func (s *inputSelection) addDiagnosticErrors(out *inputSelection, diag facts.DiagnosticFact) { for i, errFact := range s.f.Errors { if diagnosticLocationMatches(diag.File, diag.Line, errFact.File, errFact.Line) || diagnosticCommandMatches(diag, errFact.CommandPath, errFact.Command) || @@ -199,12 +235,18 @@ func (s *inputSelection) diagnosticContext(diag facts.DiagnosticFact) *inputSele out.errors[i] = true } } +} + +func (s *inputSelection) addDiagnosticOutputs(out *inputSelection, diag facts.DiagnosticFact) { for i, output := range s.f.Outputs { if diagnosticCommandMatches(diag, output.Command) || diagnosticMentions(diag, output.Command) { out.outputs[i] = true } } +} + +func (s *inputSelection) addDiagnosticExamples(out *inputSelection, diag facts.DiagnosticFact) { for i, example := range s.f.Examples { if diagnosticLocationMatches(diag.File, diag.Line, example.SourceFile, example.Line) || diagnosticCommandMatches(diag, example.CommandPath) || @@ -212,11 +254,10 @@ func (s *inputSelection) diagnosticContext(diag facts.DiagnosticFact) *inputSele out.examples[i] = true } } - return out } func includeDiagnosticInView(diag facts.DiagnosticFact, selected, context *inputSelection) bool { - if diag.Action != report.ActionWarning { + if diag.Action == report.ActionReject { return true } return selected.intersects(context) @@ -284,7 +325,17 @@ func (s *inputSelection) outputInputs() []OutputInput { out := make([]OutputInput, 0, countSelected(s.outputs)) for i, ok := range s.outputs { if ok { - out = append(out, OutputInput{FactRef: factRef("outputs", i), OutputFact: s.f.Outputs[i]}) + output := s.f.Outputs[i] + out = append(out, OutputInput{ + FactRef: factRef("outputs", i), + Command: output.Command, + Domain: output.Domain, + Changed: output.Changed, + Source: output.Source, + IsList: output.IsList, + HasDefaultLimit: output.HasDefaultLimit, + HasDecisionField: output.HasDecisionField, + }) } } return out diff --git a/internal/qualitygate/semantic/view_test.go b/internal/qualitygate/semantic/view_test.go index aed83cc6..05200933 100644 --- a/internal/qualitygate/semantic/view_test.go +++ b/internal/qualitygate/semantic/view_test.go @@ -5,6 +5,7 @@ package semantic import ( "encoding/json" + "strconv" "strings" "testing" @@ -12,15 +13,17 @@ import ( "github.com/larksuite/cli/internal/qualitygate/report" ) -func TestInputViewKeepsChangedFactsWithOriginalRefs(t *testing.T) { +func TestInputViewKeepsChangedReviewCandidatesWithOriginalRefs(t *testing.T) { f := facts.Facts{ SchemaVersion: 1, Commands: []facts.CommandFact{ {Path: "old noisy command", Source: "shortcut"}, + {Path: "docs +clean", Changed: true, Source: "shortcut"}, {Path: "docs +fetch", Changed: true, Source: "shortcut", NameConflictsExisting: true}, }, Skills: []facts.SkillFact{ {SourceFile: "skills/lark-old/SKILL.md", Line: 3, Raw: "old noisy skill"}, + {SourceFile: "skills/lark-doc/SKILL.md", Line: 8, Raw: "changed clean skill", Changed: true}, {SourceFile: "skills/lark-doc/SKILL.md", Line: 9, Raw: "changed skill", Changed: true, ReferencesInvalidCommand: true}, }, SkillQuality: []facts.SkillQualityFact{ @@ -29,10 +32,12 @@ func TestInputViewKeepsChangedFactsWithOriginalRefs(t *testing.T) { }, Errors: []facts.ErrorFact{ {File: "old.go", Line: 10, Boundary: true, RequiredHint: true}, + {File: "cmd/docs.go", Line: 19, Changed: true, Boundary: true, RequiredHint: true, HintActionCount: 1}, {File: "cmd/docs.go", Line: 20, Changed: true, Boundary: true, RequiredHint: true}, }, Outputs: []facts.OutputFact{ {Command: "old list", IsList: true}, + {Command: "docs clean-list", Changed: true, IsList: true, HasDefaultLimit: true, HasDecisionField: true}, {Command: "docs list", Changed: true, IsList: true}, }, Examples: []facts.CommandExample{ @@ -42,31 +47,155 @@ func TestInputViewKeepsChangedFactsWithOriginalRefs(t *testing.T) { } view := BuildInputView(f) - if got := singleRef(t, view.Commands); got != "facts.commands[1]" { - t.Fatalf("command ref = %q, want facts.commands[1]", got) + if got := singleRef(t, view.Commands); got != "facts.commands[2]" { + t.Fatalf("command ref = %q, want facts.commands[2]", got) } - if got := singleRef(t, view.Skills); got != "facts.skills[1]" { - t.Fatalf("skill ref = %q, want facts.skills[1]", got) + if got := singleRef(t, view.Skills); got != "facts.skills[2]" { + t.Fatalf("skill ref = %q, want facts.skills[2]", got) } - if got := singleRef(t, view.SkillQuality); got != "facts.skill_quality[1]" { - t.Fatalf("skill quality ref = %q, want facts.skill_quality[1]", got) + if len(view.SkillQuality) != 0 { + t.Fatalf("skill quality len = %d, want 0 without diagnostics", len(view.SkillQuality)) } - if got := singleRef(t, view.Errors); got != "facts.errors[1]" { - t.Fatalf("error ref = %q, want facts.errors[1]", got) + if got := singleRef(t, view.Errors); got != "facts.errors[2]" { + t.Fatalf("error ref = %q, want facts.errors[2]", got) } - if got := singleRef(t, view.Outputs); got != "facts.outputs[1]" { - t.Fatalf("output ref = %q, want facts.outputs[1]", got) + if len(view.Outputs) != 0 { + t.Fatalf("outputs len = %d, want 0 without reject diagnostics", len(view.Outputs)) } - if got := singleRef(t, view.Examples); got != "facts.examples[1]" { - t.Fatalf("example ref = %q, want facts.examples[1]", got) + if len(view.Examples) != 0 { + t.Fatalf("examples len = %d, want 0 without diagnostics", len(view.Examples)) } data, err := json.Marshal(view) if err != nil { t.Fatalf("marshal view: %v", err) } - if strings.Contains(string(data), "old noisy") { - t.Fatalf("view leaked unchanged noisy facts: %s", data) + for _, forbidden := range []string{"old noisy", "docs +clean", "changed clean skill", "docs clean-list"} { + if strings.Contains(string(data), forbidden) { + t.Fatalf("view leaked non-candidate fact %q: %s", forbidden, data) + } + } +} + +func TestInputViewSummarizesBroadChangedCommandSurface(t *testing.T) { + f := broadChangedFacts(434, 44) + + view := BuildInputView(f) + if view.ChangedSummary.Commands != 434 || view.ChangedSummary.Outputs != 44 { + t.Fatalf("changed summary = %#v", view.ChangedSummary) + } + if len(view.Commands) != 0 || len(view.Outputs) != 0 { + t.Fatalf("broad clean surface leaked details: commands=%d outputs=%d", len(view.Commands), len(view.Outputs)) + } + + messages := BuildPrompt(f) + if len(messages) != 2 { + t.Fatalf("messages len = %d, want 2", len(messages)) + } + if got := len(messages[1].Content); got > 8192 { + t.Fatalf("prompt user content bytes = %d, want <= 8192", got) + } + if strings.Contains(messages[1].Content, "service command 433") { + t.Fatalf("prompt leaked broad command details: %s", messages[1].Content) + } +} + +func TestInputViewKeepsSemanticCandidateInsideBroadChangedSurface(t *testing.T) { + f := broadChangedFacts(200, 20) + f.Commands[137].NameConflictsExisting = true + f.Outputs[11].HasDefaultLimit = false + + view := BuildInputView(f) + if got := singleRef(t, view.Commands); got != "facts.commands[137]" { + t.Fatalf("command ref = %q, want facts.commands[137]", got) + } + if len(view.Outputs) != 0 { + t.Fatalf("outputs len = %d, want 0 without reject diagnostics", len(view.Outputs)) + } +} + +func TestInputViewOmitsVerboseOutputFields(t *testing.T) { + f := facts.Facts{ + SchemaVersion: 1, + Outputs: []facts.OutputFact{{ + Command: "base +record-list", + Domain: "base", + Changed: true, + Source: "shortcut", + Fields: []string{"items", "has_more", strings.Repeat("verbose_output_field_", 200)}, + IsList: true, + HasDefaultLimit: true, + HasDecisionField: false, + }}, + Diagnostics: []facts.DiagnosticFact{{ + Rule: "default_output_contract", + Action: report.ActionReject, + File: "command-manifest", + CommandPath: "base +record-list", + }}, + } + + view := BuildInputView(f) + if got := singleRef(t, view.Outputs); got != "facts.outputs[0]" { + t.Fatalf("output ref = %q, want facts.outputs[0]", got) + } + data, err := json.Marshal(view) + if err != nil { + t.Fatalf("marshal view: %v", err) + } + text := string(data) + if strings.Contains(text, "verbose_output_field_") || strings.Contains(text, "fields") { + t.Fatalf("semantic view leaked verbose output fields: %s", text) + } + if !strings.Contains(text, `"has_decision_field":false`) { + t.Fatalf("semantic view should make missing decision field explicit: %s", text) + } +} + +func TestBuildPromptKeepsManyOutputCandidatesWithinRequestLimit(t *testing.T) { + f := broadOutputCandidateFacts(40) + for i := 0; i < 80; i++ { + f.Commands = append(f.Commands, facts.CommandFact{ + Path: "shortcut list " + strconv.Itoa(i), + Domain: "shortcut", + Changed: true, + Source: "shortcut", + Flags: []string{strings.Repeat("verbose_flag_", 100)}, + }) + f.Skills = append(f.Skills, facts.SkillFact{ + SourceFile: "skills/lark-shortcut/SKILL.md", + Line: i + 1, + Raw: strings.Repeat("verbose skill guidance ", 100), + CommandPath: "shortcut list " + strconv.Itoa(i), + Changed: true, + }) + } + f.Diagnostics = append(f.Diagnostics, facts.DiagnosticFact{ + Rule: "default_output", + Action: report.ActionWarning, + File: "command-manifest", + Message: "shortcut list 1 looks like a list command without an explicit default limit flag", + CommandPath: "shortcut list 1", + }) + + messages := BuildPrompt(f) + var view InputView + if err := json.Unmarshal([]byte(messages[1].Content), &view); err != nil { + t.Fatalf("prompt user content is not input view JSON: %v", err) + } + if len(view.Commands) != 0 || len(view.Skills) != 0 { + t.Fatalf("default-output view leaked unrelated context: commands=%d skills=%d", len(view.Commands), len(view.Skills)) + } + if len(view.Outputs) != 0 { + t.Fatalf("default-output warnings should not enter semantic view without reject diagnostics: outputs=%d", len(view.Outputs)) + } + if got := len(messages[1].Content); got > 16*1024 { + t.Fatalf("prompt user content bytes = %d, want <= 16384", got) + } + if strings.Contains(messages[1].Content, "verbose_output_field_") || + strings.Contains(messages[1].Content, "verbose_flag_") || + strings.Contains(messages[1].Content, "verbose skill guidance") { + t.Fatalf("prompt leaked verbose output fields: %s", messages[1].Content) } } @@ -163,6 +292,32 @@ func TestInputViewDropsUnchangedWarningDiagnostics(t *testing.T) { } } +func TestInputViewDropsUnselectedLabelDiagnostics(t *testing.T) { + f := facts.Facts{ + SchemaVersion: 1, + Commands: []facts.CommandFact{{ + Path: "drive +task_result", + Changed: true, + Source: "shortcut", + }}, + Diagnostics: []facts.DiagnosticFact{{ + Rule: "command_naming", + Action: report.ActionLabel, + File: "command-manifest", + Message: "drive +task_result has non-kebab-case command segments", + CommandPath: "drive +task_result", + }}, + } + + view := BuildInputView(f) + if len(view.Commands) != 0 { + t.Fatalf("commands len = %d, want 0 for label-only diagnostic", len(view.Commands)) + } + if len(view.Diagnostics) != 0 { + t.Fatalf("diagnostics len = %d, want 0 for label-only diagnostic", len(view.Diagnostics)) + } +} + func TestBuildPromptUsesInputViewInsteadOfFullFacts(t *testing.T) { f := facts.Facts{ SchemaVersion: 1, @@ -198,6 +353,49 @@ func TestBuildPromptDescribesErrorHintRubric(t *testing.T) { } } +func broadChangedFacts(commands, outputs int) facts.Facts { + f := facts.Facts{SchemaVersion: 1} + for i := 0; i < commands; i++ { + f.Commands = append(f.Commands, facts.CommandFact{ + Path: "service command " + strconv.Itoa(i), + Domain: "service", + Changed: true, + Source: "service", + Flags: []string{"tenant_key", "page_token", "page_size", "user_id_type"}, + }) + } + for i := 0; i < outputs; i++ { + f.Outputs = append(f.Outputs, facts.OutputFact{ + Command: "service command " + strconv.Itoa(i), + Domain: "service", + Changed: true, + Source: "service", + Fields: []string{"items", "has_more", "page_token"}, + IsList: true, + HasDefaultLimit: true, + HasDecisionField: true, + }) + } + return f +} + +func broadOutputCandidateFacts(outputs int) facts.Facts { + f := facts.Facts{SchemaVersion: 1} + for i := 0; i < outputs; i++ { + f.Outputs = append(f.Outputs, facts.OutputFact{ + Command: "shortcut list " + strconv.Itoa(i), + Domain: "shortcut", + Changed: true, + Source: "shortcut", + Fields: []string{"items", "has_more", strings.Repeat("verbose_output_field_", 200)}, + IsList: true, + HasDefaultLimit: i%2 == 0, + HasDecisionField: false, + }) + } + return f +} + type refItem interface { ref() string }