From 8c8c6a9d2efd8395bbc3c6d6dcab5ed29517301e Mon Sep 17 00:00:00 2001 From: Virgil Date: Mon, 30 Mar 2026 08:14:46 +0000 Subject: [PATCH] fix(ax): clean php structured output modes --- cmd/qa/cmd_php.go | 161 ++++++++++++++++++++++++++++++++++++--- cmd/qa/cmd_php_test.go | 169 +++++++++++++++++++++++++++++++++++++++++ pkg/php/security.go | 32 ++++---- pkg/php/test.go | 65 ++++++++++++++-- pkg/php/test_test.go | 4 +- 5 files changed, 397 insertions(+), 34 deletions(-) diff --git a/cmd/qa/cmd_php.go b/cmd/qa/cmd_php.go index 3956f81..3eebda1 100644 --- a/cmd/qa/cmd_php.go +++ b/cmd/qa/cmd_php.go @@ -67,7 +67,7 @@ func addPHPFmtCommand(parent *cli.Command) { return cli.Err("not a PHP project (no composer.json found)") } - if !phpFmtJSON { + if !isMachineReadableOutput(phpFmtJSON) { cli.Print("%s %s\n", headerStyle.Render("PHP Format"), dimStyle.Render("(Pint)")) cli.Blank() } @@ -115,7 +115,7 @@ func addPHPStanCommand(parent *cli.Command) { return cli.Err("no static analyser found (install PHPStan: composer require phpstan/phpstan --dev)") } - if !phpStanJSON { + if !isMachineReadableOutput(phpStanJSON, phpStanSARIF) { cli.Print("%s %s\n", headerStyle.Render("PHP Static Analysis"), dimStyle.Render(fmt.Sprintf("(%s)", analyser))) cli.Blank() } @@ -131,7 +131,7 @@ func addPHPStanCommand(parent *cli.Command) { return cli.Err("static analysis found issues") } - if !phpStanJSON { + if !isMachineReadableOutput(phpStanJSON, phpStanSARIF) { cli.Blank() cli.Print("%s\n", successStyle.Render("Static analysis passed")) } @@ -176,7 +176,7 @@ func addPHPPsalmCommand(parent *cli.Command) { return cli.Err("Psalm not found (install: composer require vimeo/psalm --dev)") } - if !phpPsalmJSON { + if !isMachineReadableOutput(phpPsalmJSON, phpPsalmSARIF) { cli.Print("%s\n", headerStyle.Render("PHP Psalm Analysis")) cli.Blank() } @@ -194,7 +194,7 @@ func addPHPPsalmCommand(parent *cli.Command) { return cli.Err("Psalm found issues") } - if !phpPsalmJSON { + if !isMachineReadableOutput(phpPsalmJSON, phpPsalmSARIF) { cli.Blank() cli.Print("%s\n", successStyle.Render("Psalm analysis passed")) } @@ -232,7 +232,7 @@ func addPHPAuditCommand(parent *cli.Command) { return cli.Err("not a PHP project (no composer.json found)") } - if !phpAuditJSON { + if !isMachineReadableOutput(phpAuditJSON) { cli.Print("%s\n", headerStyle.Render("Dependency Audit")) cli.Blank() } @@ -321,7 +321,7 @@ func addPHPSecurityCommand(parent *cli.Command) { return cli.Err("not a PHP project (no composer.json found)") } - if !phpSecurityJSON { + if !isMachineReadableOutput(phpSecurityJSON, phpSecuritySARIF) { cli.Print("%s\n", headerStyle.Render("Security Checks")) cli.Blank() } @@ -339,6 +339,20 @@ func addPHPSecurityCommand(parent *cli.Command) { result.Checks = sortSecurityChecks(result.Checks) + if phpSecuritySARIF { + data, err := json.MarshalIndent(mapSecurityResultForSARIF(result), "", " ") + if err != nil { + return err + } + fmt.Println(string(data)) + + summary := result.Summary + if summary.Critical > 0 || summary.High > 0 { + return cli.Err("security checks failed") + } + return nil + } + if phpSecurityJSON { data, err := json.MarshalIndent(result, "", " ") if err != nil { @@ -596,8 +610,10 @@ func addPHPTestCommand(parent *cli.Command) { } runner := php.DetectTestRunner(cwd) - cli.Print("%s %s\n", headerStyle.Render("PHP Tests"), dimStyle.Render(fmt.Sprintf("(%s)", runner))) - cli.Blank() + if !isMachineReadableOutput(phpTestJUnit) { + cli.Print("%s %s\n", headerStyle.Render("PHP Tests"), dimStyle.Render(fmt.Sprintf("(%s)", runner))) + cli.Blank() + } var groups []string if phpTestGroup != "" { @@ -616,8 +632,10 @@ func addPHPTestCommand(parent *cli.Command) { return cli.Err("tests failed") } - cli.Blank() - cli.Print("%s\n", successStyle.Render("All tests passed")) + if !isMachineReadableOutput(phpTestJUnit) { + cli.Blank() + cli.Print("%s\n", successStyle.Render("All tests passed")) + } return nil }, } @@ -646,3 +664,124 @@ func getSeverityStyle(severity string) *cli.AnsiStyle { return dimStyle } } + +func isMachineReadableOutput(flags ...bool) bool { + for _, flag := range flags { + if flag { + return true + } + } + return false +} + +type sarifLog struct { + Version string `json:"version"` + Schema string `json:"$schema"` + Runs []sarifRun `json:"runs"` +} + +type sarifRun struct { + Tool sarifTool `json:"tool"` + Results []sarifResult `json:"results"` +} + +type sarifTool struct { + Driver sarifDriver `json:"driver"` +} + +type sarifDriver struct { + Name string `json:"name"` + Rules []sarifRule `json:"rules"` +} + +type sarifRule struct { + ID string `json:"id"` + Name string `json:"name"` + ShortDescription sarifMessage `json:"shortDescription"` + FullDescription sarifMessage `json:"fullDescription"` + Help sarifMessage `json:"help,omitempty"` + Properties any `json:"properties,omitempty"` +} + +type sarifResult struct { + RuleID string `json:"ruleId"` + Level string `json:"level"` + Message sarifMessage `json:"message"` + Properties any `json:"properties,omitempty"` +} + +type sarifMessage struct { + Text string `json:"text"` +} + +func mapSecurityResultForSARIF(result *php.SecurityResult) sarifLog { + rules := make([]sarifRule, 0, len(result.Checks)) + sarifResults := make([]sarifResult, 0, len(result.Checks)) + + for _, check := range result.Checks { + rule := sarifRule{ + ID: check.ID, + Name: check.Name, + ShortDescription: sarifMessage{Text: check.Name}, + FullDescription: sarifMessage{Text: check.Description}, + } + if check.Fix != "" { + rule.Help = sarifMessage{Text: check.Fix} + } + if check.CWE != "" { + rule.Properties = map[string]any{"cwe": check.CWE} + } + rules = append(rules, rule) + + if check.Passed { + continue + } + + message := check.Message + if message == "" { + message = check.Description + } + + properties := map[string]any{ + "severity": check.Severity, + } + if check.CWE != "" { + properties["cwe"] = check.CWE + } + if check.Fix != "" { + properties["fix"] = check.Fix + } + + sarifResults = append(sarifResults, sarifResult{ + RuleID: check.ID, + Level: sarifLevel(check.Severity), + Message: sarifMessage{Text: message}, + Properties: properties, + }) + } + + return sarifLog{ + Version: "2.1.0", + Schema: "https://json.schemastore.org/sarif-2.1.0.json", + Runs: []sarifRun{{ + Tool: sarifTool{ + Driver: sarifDriver{ + Name: "core qa security", + Rules: rules, + }, + }, + Results: sarifResults, + }}, + } +} + +func sarifLevel(severity string) string { + switch strings.ToLower(severity) { + case "critical", "high": + return "error" + case "medium": + return "warning" + default: + return "note" + } +} diff --git a/cmd/qa/cmd_php_test.go b/cmd/qa/cmd_php_test.go index 0b0053b..97d4faa 100644 --- a/cmd/qa/cmd_php_test.go +++ b/cmd/qa/cmd_php_test.go @@ -1,6 +1,7 @@ package qa import ( + "encoding/json" "io" "os" "path/filepath" @@ -55,6 +56,126 @@ func TestPHPPsalmJSONOutput_DoesNotAppendSuccessBanner(t *testing.T) { assert.NotContains(t, output, "PHP Psalm Analysis") } +func TestPHPStanSARIFOutput_DoesNotAppendSuccessBanner(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "composer.json"), "{}") + writeExecutable(t, filepath.Join(dir, "vendor", "bin", "phpstan"), "#!/bin/sh\nprintf '%s\\n' '{\"version\":\"2.1.0\",\"runs\":[]}'\n") + + restoreWorkingDir(t, dir) + resetPHPStanFlags(t) + + parent := &cli.Command{Use: "qa"} + addPHPStanCommand(parent) + command := findSubcommand(t, parent, "stan") + require.NoError(t, command.Flags().Set("sarif", "true")) + + output := captureStdout(t, func() { + require.NoError(t, command.RunE(command, nil)) + }) + + assert.Equal(t, "{\"version\":\"2.1.0\",\"runs\":[]}\n", output) + assert.NotContains(t, output, "Static analysis passed") + assert.NotContains(t, output, "PHP Static Analysis") +} + +func TestPHPPsalmSARIFOutput_DoesNotAppendSuccessBanner(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "composer.json"), "{}") + writeExecutable(t, filepath.Join(dir, "vendor", "bin", "psalm"), "#!/bin/sh\nprintf '%s\\n' '{\"version\":\"2.1.0\",\"runs\":[]}'\n") + + restoreWorkingDir(t, dir) + resetPHPPsalmFlags(t) + + parent := &cli.Command{Use: "qa"} + addPHPPsalmCommand(parent) + command := findSubcommand(t, parent, "psalm") + require.NoError(t, command.Flags().Set("sarif", "true")) + + output := captureStdout(t, func() { + require.NoError(t, command.RunE(command, nil)) + }) + + assert.Equal(t, "{\"version\":\"2.1.0\",\"runs\":[]}\n", output) + assert.NotContains(t, output, "Psalm analysis passed") + assert.NotContains(t, output, "PHP Psalm Analysis") +} + +func TestPHPSecurityJSONOutput_UsesMachineFriendlyKeys(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "composer.json"), "{}") + writeTestFile(t, filepath.Join(dir, ".env"), "APP_DEBUG=true\nAPP_KEY=short\nAPP_URL=http://example.com\n") + writeExecutable(t, filepath.Join(dir, "bin", "composer"), "#!/bin/sh\nprintf '%s\\n' '{\"advisories\":{}}'\n") + + restoreWorkingDir(t, dir) + prependPath(t, filepath.Join(dir, "bin")) + resetPHPSecurityFlags(t) + + parent := &cli.Command{Use: "qa"} + addPHPSecurityCommand(parent) + command := findSubcommand(t, parent, "security") + require.NoError(t, command.Flags().Set("json", "true")) + + output := captureStdout(t, func() { + require.Error(t, command.RunE(command, nil)) + }) + + assert.Contains(t, output, "\"checks\"") + assert.Contains(t, output, "\"summary\"") + assert.Contains(t, output, "\"app_key_set\"") + assert.NotContains(t, output, "\"Checks\"") + assert.NotContains(t, output, "Security Checks") +} + +func TestPHPSecuritySARIFOutput_IsStructuredAndChromeFree(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "composer.json"), "{}") + writeTestFile(t, filepath.Join(dir, ".env"), "APP_DEBUG=true\nAPP_KEY=short\nAPP_URL=http://example.com\n") + writeExecutable(t, filepath.Join(dir, "bin", "composer"), "#!/bin/sh\nprintf '%s\\n' '{\"advisories\":{}}'\n") + + restoreWorkingDir(t, dir) + prependPath(t, filepath.Join(dir, "bin")) + resetPHPSecurityFlags(t) + + parent := &cli.Command{Use: "qa"} + addPHPSecurityCommand(parent) + command := findSubcommand(t, parent, "security") + require.NoError(t, command.Flags().Set("sarif", "true")) + + output := captureStdout(t, func() { + require.Error(t, command.RunE(command, nil)) + }) + + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(output), &payload)) + assert.Equal(t, "2.1.0", payload["version"]) + assert.Contains(t, output, "\"ruleId\": \"app_key_set\"") + assert.NotContains(t, output, "Security Checks") + assert.NotContains(t, output, "Summary:") +} + +func TestPHPTestJUnitOutput_PrintsOnlyXML(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "composer.json"), "{}") + writeExecutable(t, filepath.Join(dir, "vendor", "bin", "phpunit"), "#!/bin/sh\njunit=''\nwhile [ $# -gt 0 ]; do\n if [ \"$1\" = \"--log-junit\" ]; then\n shift\n junit=\"$1\"\n fi\n shift\ndone\nprintf '%s\\n' 'human output should be suppressed'\nprintf '%s' '' > \"$junit\"\n") + + restoreWorkingDir(t, dir) + resetPHPTestFlags(t) + + parent := &cli.Command{Use: "qa"} + addPHPTestCommand(parent) + command := findSubcommand(t, parent, "test") + require.NoError(t, command.Flags().Set("junit", "true")) + + output := captureStdout(t, func() { + require.NoError(t, command.RunE(command, nil)) + }) + + assert.Equal(t, "\n", output) + assert.NotContains(t, output, "human output should be suppressed") + assert.NotContains(t, output, "PHP Tests") + assert.NotContains(t, output, "All tests passed") +} + func writeTestFile(t *testing.T, path string, content string) { t.Helper() require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) @@ -119,6 +240,45 @@ func resetPHPPsalmFlags(t *testing.T) { }) } +func resetPHPSecurityFlags(t *testing.T) { + t.Helper() + oldSeverity := phpSecuritySeverity + oldJSON := phpSecurityJSON + oldSARIF := phpSecuritySARIF + oldURL := phpSecurityURL + phpSecuritySeverity = "" + phpSecurityJSON = false + phpSecuritySARIF = false + phpSecurityURL = "" + t.Cleanup(func() { + phpSecuritySeverity = oldSeverity + phpSecurityJSON = oldJSON + phpSecuritySARIF = oldSARIF + phpSecurityURL = oldURL + }) +} + +func resetPHPTestFlags(t *testing.T) { + t.Helper() + oldParallel := phpTestParallel + oldCoverage := phpTestCoverage + oldFilter := phpTestFilter + oldGroup := phpTestGroup + oldJUnit := phpTestJUnit + phpTestParallel = false + phpTestCoverage = false + phpTestFilter = "" + phpTestGroup = "" + phpTestJUnit = false + t.Cleanup(func() { + phpTestParallel = oldParallel + phpTestCoverage = oldCoverage + phpTestFilter = oldFilter + phpTestGroup = oldGroup + phpTestJUnit = oldJUnit + }) +} + func findSubcommand(t *testing.T, parent *cli.Command, name string) *cli.Command { t.Helper() for _, command := range parent.Commands() { @@ -151,3 +311,12 @@ func captureStdout(t *testing.T, fn func()) string { require.NoError(t, err) return string(output) } + +func prependPath(t *testing.T, dir string) { + t.Helper() + oldPath := os.Getenv("PATH") + require.NoError(t, os.Setenv("PATH", dir+string(os.PathListSeparator)+oldPath)) + t.Cleanup(func() { + require.NoError(t, os.Setenv("PATH", oldPath)) + }) +} diff --git a/pkg/php/security.go b/pkg/php/security.go index bb60d34..fc6803d 100644 --- a/pkg/php/security.go +++ b/pkg/php/security.go @@ -22,30 +22,30 @@ type SecurityOptions struct { // SecurityResult holds the results of security scanning. type SecurityResult struct { - Checks []SecurityCheck - Summary SecuritySummary + Checks []SecurityCheck `json:"checks"` + Summary SecuritySummary `json:"summary"` } // SecurityCheck represents a single security check result. type SecurityCheck struct { - ID string - Name string - Description string - Severity string - Passed bool - Message string - Fix string - CWE string + ID string `json:"id"` + Name string `json:"name"` + Description string `json:"description"` + Severity string `json:"severity"` + Passed bool `json:"passed"` + Message string `json:"message,omitempty"` + Fix string `json:"fix,omitempty"` + CWE string `json:"cwe,omitempty"` } // SecuritySummary summarises security check results. type SecuritySummary struct { - Total int - Passed int - Critical int - High int - Medium int - Low int + Total int `json:"total"` + Passed int `json:"passed"` + Critical int `json:"critical"` + High int `json:"high"` + Medium int `json:"medium"` + Low int `json:"low"` } // capitalise returns s with the first letter upper-cased. diff --git a/pkg/php/test.go b/pkg/php/test.go index 8e5643c..f05287b 100644 --- a/pkg/php/test.go +++ b/pkg/php/test.go @@ -1,6 +1,7 @@ package php import ( + "bytes" "context" "io" "os" @@ -33,6 +34,9 @@ type TestOptions struct { // JUnit outputs results in JUnit XML format via --log-junit. JUnit bool + // JUnitPath overrides the JUnit report path. Defaults to test-results.xml. + JUnitPath string + // Output is the writer for test output (defaults to os.Stdout). Output io.Writer } @@ -73,6 +77,18 @@ func RunTests(ctx context.Context, opts TestOptions) error { opts.Output = os.Stdout } + if opts.JUnit && opts.JUnitPath == "" { + reportFile, err := os.CreateTemp("", "core-qa-junit-*.xml") + if err != nil { + return coreerr.E("php.RunTests", "create JUnit report file", err) + } + if closeErr := reportFile.Close(); closeErr != nil { + return coreerr.E("php.RunTests", "close JUnit report file", closeErr) + } + opts.JUnitPath = reportFile.Name() + defer os.Remove(opts.JUnitPath) + } + // Detect test runner runner := DetectTestRunner(opts.Dir) @@ -89,14 +105,27 @@ func RunTests(ctx context.Context, opts TestOptions) error { cmd := exec.CommandContext(ctx, cmdName, args...) cmd.Dir = opts.Dir - cmd.Stdout = opts.Output - cmd.Stderr = opts.Output cmd.Stdin = os.Stdin // Set XDEBUG_MODE=coverage to avoid PHPUnit 11 warning cmd.Env = append(os.Environ(), "XDEBUG_MODE=coverage") - return cmd.Run() + if !opts.JUnit { + cmd.Stdout = opts.Output + cmd.Stderr = opts.Output + return cmd.Run() + } + + var machineOutput bytes.Buffer + cmd.Stdout = &machineOutput + cmd.Stderr = &machineOutput + + runErr := cmd.Run() + reportErr := emitJUnitReport(opts.Output, opts.JUnitPath) + if runErr != nil { + return runErr + } + return reportErr } // RunParallel runs tests in parallel using the appropriate runner. @@ -140,7 +169,7 @@ func buildPestCommand(opts TestOptions) (string, []string) { } if opts.JUnit { - args = append(args, "--log-junit", "test-results.xml") + args = append(args, "--log-junit", junitReportPath(opts)) } return cmdName, args @@ -185,8 +214,34 @@ func buildPHPUnitCommand(opts TestOptions) (string, []string) { } if opts.JUnit { - args = append(args, "--log-junit", "test-results.xml", "--testdox") + args = append(args, "--log-junit", junitReportPath(opts)) } return cmdName, args } + +func junitReportPath(opts TestOptions) string { + if opts.JUnitPath != "" { + return opts.JUnitPath + } + return "test-results.xml" +} + +func emitJUnitReport(output io.Writer, reportPath string) error { + report, err := os.ReadFile(reportPath) + if err != nil { + return coreerr.E("php.emitJUnitReport", "read JUnit report", err) + } + + if _, err := output.Write(report); err != nil { + return coreerr.E("php.emitJUnitReport", "write JUnit report", err) + } + + if len(report) == 0 || report[len(report)-1] != '\n' { + if _, err := io.WriteString(output, "\n"); err != nil { + return coreerr.E("php.emitJUnitReport", "terminate JUnit report", err) + } + } + + return nil +} diff --git a/pkg/php/test_test.go b/pkg/php/test_test.go index dc79c08..02c0ae1 100644 --- a/pkg/php/test_test.go +++ b/pkg/php/test_test.go @@ -288,7 +288,7 @@ func TestBuildPHPUnitCommand_Good_JUnit(t *testing.T) { assert.Contains(t, args, "--log-junit") assert.Contains(t, args, "test-results.xml") - assert.Contains(t, args, "--testdox") + assert.NotContains(t, args, "--testdox") } func TestBuildPHPUnitCommand_Good_AllFlags(t *testing.T) { @@ -313,5 +313,5 @@ func TestBuildPHPUnitCommand_Good_AllFlags(t *testing.T) { assert.Contains(t, args, "--group") assert.Contains(t, args, "feature") assert.Contains(t, args, "--log-junit") - assert.Contains(t, args, "--testdox") + assert.NotContains(t, args, "--testdox") }