fix Special output format for Github actions #1599 #1785
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements GitHub Actions annotations support for Pyrefly errors, addressing issue #1599. The implementation adds automatic emission of workflow commands (::error, ::warning, ::notice) when running in GitHub Actions environments, making errors visible as annotations in pull requests.
Key Changes
- Added
emit_github_actions_annotationsfunction that detects GitHub Actions environment and emits workflow commands - Implemented proper escaping for GitHub Actions workflow command format (data and property escaping)
- Integrated annotation emission into the main error reporting flow after console/file output
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn escape_helpers_follow_workflow_spec() { | ||
| assert_eq!( | ||
| escape_workflow_data("line1\nline2\r% done"), | ||
| "line1%0Aline2%0D%25 done" | ||
| ); | ||
| assert_eq!(escape_workflow_property("file:name,py"), "file%3Aname%2Cpy"); | ||
| } |
There was a problem hiding this comment.
The escape helper tests should include a case with a % character to ensure proper double-escaping doesn't occur. For example, test that escape_workflow_property("file%name.py") returns "file%25name.py" to verify the escape order is correct and prevents injection attacks.
| fn github_actions_path(path: &Path, relative_to: &Path) -> String { | ||
| let relative = if relative_to.as_os_str().is_empty() { | ||
| path | ||
| } else { | ||
| path.strip_prefix(relative_to).unwrap_or(path) | ||
| }; | ||
| let candidate = if relative.as_os_str().is_empty() { | ||
| path | ||
| } else { | ||
| relative | ||
| }; | ||
| let mut path_str = candidate.to_string_lossy().into_owned(); | ||
| if std::path::MAIN_SEPARATOR != '/' { | ||
| path_str = path_str.replace(std::path::MAIN_SEPARATOR, "/"); | ||
| } | ||
| path_str | ||
| } |
There was a problem hiding this comment.
Consider adding a test for the github_actions_path function to ensure it correctly handles edge cases such as:
- Empty
relative_topath (should use the full path) - Paths that don't start with
relative_to(should use the full path) - Windows path separators (should be converted to forward slashes)
- Relative paths that result in an empty string
Example test:
#[test]
fn github_actions_path_handles_edge_cases() {
assert_eq!(github_actions_path(Path::new("/repo/foo.py"), Path::new("")), "/repo/foo.py");
assert_eq!(github_actions_path(Path::new("/other/foo.py"), Path::new("/repo")), "/other/foo.py");
}
asukaminato0721
force-pushed
the
1599
branch
2 times, most recently
from
fd5169b to
9ba1557
Compare
| JQ: jq | ||
| TEST_PY: ${{ github.workspace }}/test.py | ||
| PYREFLY_PY: ${{ github.workspace }}/pyrefly/python | ||
| run: scrut test test |
There was a problem hiding this comment.
what's the reason for deleting this?
There was a problem hiding this comment.
not deleting, add env -u GITHUB_ACTIONS or the output will fail because of new output.
maggiemoss
left a comment
maggiemoss
left a comment
There was a problem hiding this comment.
We have an existing enum for outputting errors,
enum OutputFormat {
...
Can you add an entry there for the github output, and then refactor this to match the other writers / output formatters that exist?
asukaminato0721
force-pushed
the
1599
branch
from
9ba1557 to
899d42a
Compare
asukaminato0721
force-pushed
the
1599
branch
from
899d42a to
1432c97
Compare
asukaminato0721
force-pushed
the
1599
branch
from
1432c97 to
c5cdacb
Compare
3 participants
Summary
Fixes #1599
Added emit_github_actions_annotations plus helpers to build safe ::error/::warning/::notice workflow commands (severity mapping, relative paths, escaping, Windows handling) so every CLI error can surface as a GitHub Actions annotation when GITHUB_ACTIONS=true
Hooked the annotation emission into the main reporting path so it runs after standard console/file output without affecting existing formats.
Test Plan
Introduced unit tests covering the formatting logic, severity mapping, and escape helpers to guard against regressions.