feat: improve error summary formatting and add more details #39
Conversation
Summary of ChangesHello @MatuzMark, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the error reporting capabilities by restructuring and enriching the end-of-report summary. It aims to provide users with a clearer, more detailed, and visually distinct overview of analysis results, making it easier to identify, categorize, and address various types of errors, including those that cannot be ignored or those related to outdated baselines. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Hi, thanks for this contribution! The new error summary format is a great improvement, adding more detail and better readability with colors. The code changes look good overall. I've added a few minor suggestions to improve maintainability and consistency. Welcome to the project, and great job on your first PR!
- change to sprintf formatted strings - use class constant - use helper class for pluralization logic
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the error summary, making it more detailed and readable. However, a medium-severity Terminal Output Injection vulnerability was identified. Untrusted error identifiers are rendered in the terminal without proper escaping, which could allow an attacker with commit access to manipulate the CLI report output for malicious purposes. Additionally, I've provided specific comments with suggestions to improve test coverage, maintainability, and consistency. Welcome to the project, and thank you for your contribution!
| private const IDENTIFIER_NO_IDENTIFIER = '<no-identifier>'; | ||
| private const IDENTIFIER_IGNORE_UNMATCHED = 'ignore.unmatched'; | ||
|
|
||
| public function writeGroupedErrorsSummary(AnalysisResult $analysisResult, Output $output): void |
There was a problem hiding this comment.
This is a great improvement to the error summary. However, the new functionality is not covered by automated tests. The existing tests in tests/FriendlyErrorFormatterTest.php will likely fail because the output format has changed. Please update the tests to assert the new summary, notes, and colors. It would also be beneficial to add specific test cases for ignore.unmatched errors and non-ignorable errors to ensure the new logic is fully covered.
| $output->writeLineFormatted(\sprintf( | ||
| " <fg={$color}>%d</> <fg=yellow>%s</> (in <fg={$color}>%d</> %s)", | ||
| $count, | ||
| $identifier, | ||
| $fileCount, | ||
| $suffix | ||
| )); |
There was a problem hiding this comment.
The $identifier variable, which originates from a PHPStan error identifier, is directly used in a sprintf call that generates a formatted string for terminal output. The writeLineFormatted function interprets <...> tags for styling. If a specially crafted error identifier (e.g., containing </fg=yellow><bg=red>malicious-text</>) is present in the codebase, it can manipulate the terminal output. This could be used to obscure legitimate errors or create misleading reports. In terminals that support hyperlinks, this could also potentially be used for phishing attacks against developers. To prevent this, the identifier should be escaped before being rendered.
$output->writeLineFormatted(\sprintf(
" <fg={$color}>%d</> <fg=yellow>%s</> (in <fg={$color}>%d</> %s)",
$count,
str_replace('<', '\\<', $identifier),
$fileCount,
$suffix
));| $output->writeLineFormatted(\sprintf(' %d %s', $count, $identifier)); | ||
| $fileCount = \count($files[$identifier]); | ||
| $suffix = $this->getFileSuffix($fileCount); | ||
| $color = self::IDENTIFIER_IGNORE_UNMATCHED === $identifier ? 'green' : 'red'; |
There was a problem hiding this comment.
The note for errors with <no-identifier> is colored yellow, which suggests a warning. However, in the error summary list, it's colored red. For consistency, it would be better to also color it yellow in the summary list. You can use a match expression (for PHP 8.0+) for a cleaner implementation.
$color = match ($identifier) {
self::IDENTIFIER_IGNORE_UNMATCHED => 'green',
self::IDENTIFIER_NO_IDENTIFIER => 'yellow',
default => 'red',
};| if (isset($errorCounter[self::IDENTIFIER_IGNORE_UNMATCHED]) || isset($errorCounter[self::IDENTIFIER_NO_IDENTIFIER]) || 0 !== $nonignorableCounter) { | ||
| $output->writeLineFormatted(''); | ||
| $output->writeLineFormatted('ℹ️ Note:'); | ||
| $output->writeLineFormatted('────────'); | ||
| } | ||
|
|
||
| if (isset($errorCounter[self::IDENTIFIER_IGNORE_UNMATCHED])) { | ||
| $output->writeLineFormatted(\sprintf('🎉 <fg=green>%d</> errors can be removed after updating the baseline.', $errorCounter[self::IDENTIFIER_IGNORE_UNMATCHED])); | ||
| } | ||
|
|
||
| if (isset($errorCounter[self::IDENTIFIER_NO_IDENTIFIER])) { | ||
| $output->writeLineFormatted(\sprintf('⚠️ <fg=yellow>%d</> errors have no identifier. Consider upgrading to PHPStan v2, which requires identifiers.', $errorCounter[self::IDENTIFIER_NO_IDENTIFIER])); | ||
| } | ||
|
|
||
| if (0 !== $nonignorableCounter) { | ||
| $output->writeLineFormatted(\sprintf('🚨 <fg=red>%d</> errors cannot be ignored by baseline!', $nonignorableCounter)); | ||
| } |
There was a problem hiding this comment.
The logic for printing notes is a bit repetitive with the main if condition and then individual ifs. You can make it more maintainable by first collecting all notes into an array and then printing them if the array is not empty. This avoids repeating the conditions and makes the code more DRY (Don't Repeat Yourself).
$notes = [];
if (isset($errorCounter[self::IDENTIFIER_IGNORE_UNMATCHED])) {
$notes[] = \sprintf('🎉 <fg=green>%d</> errors can be removed after updating the baseline.', $errorCounter[self::IDENTIFIER_IGNORE_UNMATCHED]);
}
if (isset($errorCounter[self::IDENTIFIER_NO_IDENTIFIER])) {
$notes[] = \sprintf('⚠️ <fg=yellow>%d</> errors have no identifier. Consider upgrading to PHPStan v2, which requires identifiers.', $errorCounter[self::IDENTIFIER_NO_IDENTIFIER]);
}
if (0 !== $nonignorableCounter) {
$notes[] = \sprintf('🚨 <fg=red>%d</> errors cannot be ignored by baseline!', $nonignorableCounter);
}
if (!empty($notes)) {
$output->writeLineFormatted('');
$output->writeLineFormatted('ℹ️ Note:');
$output->writeLineFormatted('────────');
foreach ($notes as $note) {
$output->writeLineFormatted($note);
}
}1 participant
Hi,
I made several improvements to the end of the report when errors are present.
Changes included:
I’ve attached screenshots showing both a minimal and a full output example.
Minimal output:
Full output:
This is my first pull request — feedback is very welcome.