Skip to content

Commit f0b5d61

Browse files
Testclaude
andcommitted
fix(output): humanize tool JSON in CI section body (BUG-18)
When --format=codeclimate or --format=sarif is active, GitHooks reconfigures each tool to its JSON variant so the file-based formatters can parse it. That JSON used to leak into the KO section body of GitLab CI / GitHub Actions / framed-error blocks, leaving operators with a raw blob instead of a readable listing. Adds HumanIssueFormatter, which translates the per-tool JSON via the existing ToolOutputParser registry into "file\n line N message [rule]" plus an aggregated Totals line. FlowExecutor::buildResult invokes it just before OutputHandler::onJobError when structuredFormat=true; JobResult.output still carries the raw JSON so JSON / JUnit / SARIF / CodeClimate file reports remain unchanged. Fallback policy: missing parser, broken JSON or 0 parsed issues all return the raw output; whitespace-only collapses to empty. No regression for local-script / custom jobs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e234674 commit f0b5d61

6 files changed

Lines changed: 671 additions & 3 deletions

File tree

docs/changelog.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22

33
All notable changes to this project are documented here.
44

5+
## [3.4]
6+
7+
### Fixed
8+
9+
- **GitLab CI / GitHub Actions sections no longer leak raw tool JSON when `--format=codeclimate` or `--format=sarif` (or `reports.codeclimate` / `reports.sarif` in config) is active.** When a structured format is requested, GitHooks reconfigures every supported tool to its JSON variant (`phpstan --error-format=json`, `phpcs --report=json`, `phpmd … json`, `psalm --output-format=json`, `parallel-lint --json`) so the file-based formatters can parse the output. The side-effect: failing jobs printed that JSON blob inside their CI section as the visible body — a one-line minified string for PHPStan/PHPCS (with absolute runner paths like `/builds/<group>/<project>/…`), pretty-printed JSON for PHPMD. The operator who opened a KO section saw raw JSON instead of `file line N message [rule]`. JUnit/SARIF/CodeClimate file reports were already correct (they parse the JSON before emitting). What was missing was humanising the **display** layer. Fix: a new `HumanIssueFormatter` translates the per-tool JSON into a human listing (path + indented `line N` / `line N:C` rows + `Totals: X file(s), Y issue(s)`) using the existing `ToolOutputParser` registry; `FlowExecutor::buildResult` invokes it just before `OutputHandler::onJobError` when `structuredFormat = true`. The raw output is preserved unchanged in `JobResult.output`, so the file-based formatters (`JsonResultFormatter`, `JunitResultFormatter`, `SarifResultFormatter`, `CodeClimateResultFormatter`) and any `reports.*` paths keep getting the JSON they need. Fallbacks: jobs without a registered parser (`local-script`, `custom`) and tools whose JSON is broken / contains no issues fall back to the raw output (no regression vs the prior behaviour); whitespace-only outputs collapse to empty. Same upstream fix benefits `GitHubActionsDecorator`, `DashboardOutputHandler::printFramedError` and `Printer::framedErrorBlock`.
10+
511
## [3.3.3]
612

713
### Fixed

src/Execution/FlowExecutor.php

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313
use Wtyd\GitHooks\Execution\Admission\GreedyAdmission;
1414
use Wtyd\GitHooks\Jobs\JobAbstract;
1515
use Wtyd\GitHooks\Output\DashboardOutputHandler;
16+
use Wtyd\GitHooks\Output\HumanIssueFormatter;
1617
use Wtyd\GitHooks\Output\OutputHandler;
18+
use Wtyd\GitHooks\Output\ToolOutputParser\ToolOutputParserRegistry;
1719
use Wtyd\GitHooks\Execution\ThreadBudgetAllocator;
1820
use Wtyd\GitHooks\Utils\GitStagerInterface;
1921

@@ -29,6 +31,8 @@ class FlowExecutor
2931

3032
private ?GitStagerInterface $gitStager;
3133

34+
private HumanIssueFormatter $humanFormatter;
35+
3236
private bool $structuredFormat = false;
3337

3438
private bool $thresholdsDisabled = false;
@@ -47,10 +51,14 @@ class FlowExecutor
4751
/** @var (callable():float)|null */
4852
private $clockOverride = null;
4953

50-
public function __construct(OutputHandler $outputHandler, ?GitStagerInterface $gitStager = null)
51-
{
54+
public function __construct(
55+
OutputHandler $outputHandler,
56+
?GitStagerInterface $gitStager = null,
57+
?HumanIssueFormatter $humanFormatter = null
58+
) {
5259
$this->outputHandler = $outputHandler;
5360
$this->gitStager = $gitStager;
61+
$this->humanFormatter = $humanFormatter ?? new HumanIssueFormatter(new ToolOutputParserRegistry());
5462
}
5563

5664
public function getOutputHandler(): OutputHandler
@@ -720,7 +728,14 @@ private function buildResult(JobAbstract $job, Process $process, float $start):
720728
} elseif ($success) {
721729
$this->outputHandler->onJobSuccess($displayName, $time);
722730
} else {
723-
$this->outputHandler->onJobError($displayName, $time, $output);
731+
// BUG-18: when structuredFormat is on the tools emit JSON for the
732+
// SARIF/CodeClimate file formatters; humanise it before passing to
733+
// the display layer, but keep the raw $output in the JobResult so
734+
// file-based formatters keep getting the JSON they parse.
735+
$displayOutput = $this->structuredFormat
736+
? $this->humanFormatter->format($job->getType(), $output)
737+
: $output;
738+
$this->outputHandler->onJobError($displayName, $time, $displayOutput);
724739
}
725740

726741
$command = $job->buildCommand();

src/Output/HumanIssueFormatter.php

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Wtyd\GitHooks\Output;
6+
7+
use Wtyd\GitHooks\Output\Concerns\RelativizesFilePath;
8+
use Wtyd\GitHooks\Output\ToolOutputParser\ToolOutputParserRegistry;
9+
10+
/**
11+
* Renders a tool's raw JSON output as a human-readable text block consumable
12+
* by a CI section reader. Falls back to the raw output for jobs without a
13+
* registered parser, broken JSON, or any case where no issues are parsed —
14+
* the operator must always see at least the original tool output when KO.
15+
*
16+
* Drives BUG-18: structured formats (codeclimate, sarif) reconfigure the
17+
* tools to emit JSON for the file-based formatters, and that JSON used to
18+
* leak into the KO section as the visible body.
19+
*/
20+
final class HumanIssueFormatter
21+
{
22+
use RelativizesFilePath;
23+
24+
private ToolOutputParserRegistry $registry;
25+
26+
public function __construct(ToolOutputParserRegistry $registry)
27+
{
28+
$this->registry = $registry;
29+
}
30+
31+
public function format(string $jobType, string $rawOutput): string
32+
{
33+
if (trim($rawOutput) === '') {
34+
return '';
35+
}
36+
if (!$this->registry->hasParser($jobType)) {
37+
return $rawOutput;
38+
}
39+
40+
$parser = $this->registry->getParser($jobType);
41+
if ($parser === null) {
42+
return $rawOutput;
43+
}
44+
45+
$issues = $parser->parse($rawOutput, $jobType);
46+
if (count($issues) === 0) {
47+
return $rawOutput;
48+
}
49+
50+
return $this->renderIssues($issues);
51+
}
52+
53+
/**
54+
* @param CodeIssue[] $issues
55+
*/
56+
private function renderIssues(array $issues): string
57+
{
58+
/** @var array<string, CodeIssue[]> $byFile */
59+
$byFile = [];
60+
foreach ($issues as $issue) {
61+
$file = $this->relativizePath($issue->getFile());
62+
if (!isset($byFile[$file])) {
63+
$byFile[$file] = [];
64+
}
65+
$byFile[$file][] = $issue;
66+
}
67+
68+
$lines = [];
69+
foreach ($byFile as $file => $fileIssues) {
70+
$lines[] = $file;
71+
foreach ($fileIssues as $issue) {
72+
$location = 'line ' . $issue->getLine();
73+
$column = $issue->getColumn();
74+
if ($column !== null) {
75+
$location .= ':' . $column;
76+
}
77+
$lines[] = ' ' . $location . ' ' . $issue->getMessage() . ' [' . $issue->getRuleId() . ']';
78+
}
79+
$lines[] = '';
80+
}
81+
82+
$fileCount = count($byFile);
83+
$issueCount = count($issues);
84+
$lines[] = sprintf(
85+
'Totals: %d %s, %d %s',
86+
$fileCount,
87+
$fileCount === 1 ? 'file' : 'files',
88+
$issueCount,
89+
$issueCount === 1 ? 'issue' : 'issues'
90+
);
91+
92+
return implode("\n", $lines) . "\n";
93+
}
94+
}
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Tests\Unit\Execution;
6+
7+
use PHPUnit\Framework\TestCase;
8+
use Tests\Doubles\OutputHandlerSpy;
9+
use Wtyd\GitHooks\Configuration\JobConfiguration;
10+
use Wtyd\GitHooks\Configuration\OptionsConfiguration;
11+
use Wtyd\GitHooks\Execution\FlowExecutor;
12+
use Wtyd\GitHooks\Execution\FlowPlan;
13+
use Wtyd\GitHooks\Jobs\CustomJob;
14+
use Wtyd\GitHooks\Jobs\PhpstanJob;
15+
16+
/**
17+
* BUG-18: covers the decision table of the humanisation step in
18+
* `FlowExecutor::buildResult`.
19+
*
20+
* Factors:
21+
* - structuredFormat: true / false
22+
* - Parser registered for the job type: yes (phpstan, …) / no (local-script)
23+
*
24+
* Invariants under test for every row:
25+
* - `JobResult.output` is ALWAYS the raw process output (never humanised).
26+
* The file-based formatters (JsonResultFormatter / JunitResultFormatter /
27+
* SarifResultFormatter / CodeClimateResultFormatter) all consume that
28+
* field and must keep getting the JSON they parse.
29+
* - The OutputHandler sees the humanised version ONLY when both
30+
* structuredFormat = true AND a parser is registered for the job type;
31+
* otherwise it sees the raw output (unchanged from pre-BUG-18 behaviour).
32+
*/
33+
class FlowExecutorHumanizeOutputTest extends TestCase
34+
{
35+
/**
36+
* Row A — structuredFormat=true, parser registered.
37+
* onJobError receives the human listing; JobResult.output stays JSON.
38+
*
39+
* @test
40+
*/
41+
public function row_a_structured_with_parser_humanises_for_handler_but_keeps_raw_in_jobresult(): void
42+
{
43+
$rawJson = '{"totals":{"errors":1},"files":{"src/Foo.php":'
44+
. '{"errors":1,"messages":[{"message":"Class Foo not found.","line":42,'
45+
. '"identifier":"class.notFound"}]}}}';
46+
47+
$job = $this->phpstanJobEmitting($rawJson);
48+
49+
$spy = new OutputHandlerSpy();
50+
$executor = new FlowExecutor($spy);
51+
$executor->setStructuredFormat(true);
52+
53+
$result = $executor->execute(new FlowPlan('test', [$job], new OptionsConfiguration(false, 1)));
54+
55+
$this->assertCount(1, $spy->errorJobs);
56+
$handlerOutput = $spy->errorJobs[0]['output'];
57+
58+
// Humanised content reached the handler.
59+
$this->assertStringContainsString('src/Foo.php', $handlerOutput);
60+
$this->assertStringContainsString('line 42', $handlerOutput);
61+
$this->assertStringContainsString('[class.notFound]', $handlerOutput);
62+
$this->assertStringNotContainsString('"totals":', $handlerOutput);
63+
64+
// JobResult preserves the raw JSON for file-based formatters.
65+
$jobResult = $result->getJobResults()[0];
66+
$this->assertStringContainsString('"totals":', $jobResult->getOutput());
67+
$this->assertStringContainsString('"files":', $jobResult->getOutput());
68+
}
69+
70+
/**
71+
* Row B — structuredFormat=true, NO parser for jobType (local-script).
72+
* Falls back to the raw output for both the handler and the JobResult.
73+
*
74+
* @test
75+
*/
76+
public function row_b_structured_without_parser_falls_back_to_raw_on_both_paths(): void
77+
{
78+
$raw = "custom failure: missing config\nexpected file not found";
79+
$job = $this->localScriptJobEmitting($raw);
80+
81+
$spy = new OutputHandlerSpy();
82+
$executor = new FlowExecutor($spy);
83+
$executor->setStructuredFormat(true);
84+
85+
$result = $executor->execute(new FlowPlan('test', [$job], new OptionsConfiguration(false, 1)));
86+
87+
$this->assertCount(1, $spy->errorJobs);
88+
$this->assertStringContainsString('custom failure', $spy->errorJobs[0]['output']);
89+
$this->assertStringContainsString('custom failure', $result->getJobResults()[0]->getOutput());
90+
}
91+
92+
/**
93+
* Row C — structuredFormat=false, parser registered.
94+
* No humanisation; the handler sees the raw output exactly as the JobResult does.
95+
*
96+
* @test
97+
*/
98+
public function row_c_unstructured_with_parser_does_not_humanise(): void
99+
{
100+
// With structuredFormat=false the tool would emit its native human
101+
// output, not JSON. Simulate that — and confirm the executor does
102+
// not invoke the formatter (which would otherwise fall back to raw
103+
// anyway, but we want to guarantee the structuredFormat guard short-
104+
// circuits BEFORE invoking the formatter).
105+
$raw = " ------ ------- \n Line src/Foo.php\n 42 Class Foo not found.\n";
106+
$job = $this->phpstanJobEmitting($raw);
107+
108+
$spy = new OutputHandlerSpy();
109+
$executor = new FlowExecutor($spy);
110+
// structuredFormat stays at its default (false).
111+
112+
$result = $executor->execute(new FlowPlan('test', [$job], new OptionsConfiguration(false, 1)));
113+
114+
$this->assertCount(1, $spy->errorJobs);
115+
$this->assertSame(
116+
$result->getJobResults()[0]->getOutput(),
117+
$spy->errorJobs[0]['output'],
118+
'When structuredFormat is off the handler must receive the raw output unchanged'
119+
);
120+
$this->assertStringContainsString('Class Foo not found.', $spy->errorJobs[0]['output']);
121+
}
122+
123+
/**
124+
* Row D — structuredFormat=false, no parser for jobType.
125+
* Same as C: raw on both paths.
126+
*
127+
* @test
128+
*/
129+
public function row_d_unstructured_without_parser_does_not_humanise(): void
130+
{
131+
$raw = "plain failure message\n";
132+
$job = $this->localScriptJobEmitting($raw);
133+
134+
$spy = new OutputHandlerSpy();
135+
$executor = new FlowExecutor($spy);
136+
137+
$result = $executor->execute(new FlowPlan('test', [$job], new OptionsConfiguration(false, 1)));
138+
139+
$this->assertCount(1, $spy->errorJobs);
140+
$this->assertSame(
141+
$result->getJobResults()[0]->getOutput(),
142+
$spy->errorJobs[0]['output']
143+
);
144+
$this->assertStringContainsString('plain failure message', $spy->errorJobs[0]['output']);
145+
}
146+
147+
/**
148+
* Returns a PhpstanJob subclass whose `buildCommand()` produces $raw on
149+
* stdout and exits 1, irrespective of the real `applyStructuredOutputFormat()`
150+
* the executor invokes when structuredFormat=true (those args would feed a
151+
* real phpstan binary; here we short-circuit with sh).
152+
*/
153+
private function phpstanJobEmitting(string $raw): PhpstanJob
154+
{
155+
$job = new class (new JobConfiguration('phpstan_src', 'phpstan', ['paths' => ['src']])) extends PhpstanJob {
156+
public string $rawPayload = '';
157+
public function buildCommand(): string
158+
{
159+
$b64 = base64_encode($this->rawPayload);
160+
return "sh -c 'echo {$b64} | base64 -d; exit 1'";
161+
}
162+
};
163+
$job->rawPayload = $raw;
164+
return $job;
165+
}
166+
167+
/**
168+
* Returns a CustomJob (type=local-script — not in ToolOutputParserRegistry)
169+
* that emits $raw and exits 1.
170+
*/
171+
private function localScriptJobEmitting(string $raw): CustomJob
172+
{
173+
$b64 = base64_encode($raw);
174+
return new CustomJob(new JobConfiguration(
175+
'my_local_script',
176+
'local-script',
177+
['script' => "sh -c 'echo {$b64} | base64 -d; exit 1'"]
178+
));
179+
}
180+
}

0 commit comments

Comments
 (0)