feat(formatters): unified Finding dataclass + 5 new formatters — text, junit-xml, emacs, vim, gitlab-sast (closes #52)#139
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…, junit-xml, emacs, vim, gitlab-sast (closes #52)
Issue #170 — PR #139 needs two fixes before merge: 1. Missing @WHO/@WHAT/@PART/@entry headers on 5 new formatter files 2. Wrong command count in docs (PR #139 bumped 68→69 but added 0 commands) This commit fixes #1 by adding the standard file header to: - scripts/formatters/text.py → format_text() - scripts/formatters/vim.py → format_vim() - scripts/formatters/emacs.py → format_emacs() - scripts/formatters/junit_xml.py → format_junit_xml() - scripts/formatters/gitlab_sast.py → format_gitlab_sast() Fix #2 (wrong command count) was resolved by rebasing PR #139 on latest main (bfd7c40). The rebase took main's version of all 6 doc files (README, SKILL, SKILL-QUICK, pyproject, skill.json, graph_model.py) since PR #139's only doc changes were the wrong count bump. After rebase, sync_command_count.py --check reports clean (75 commands, matching main). Rebase conflicts resolved: - scripts/codelens.py: merged both main's 'graphml' format choice + PR #139's 5 new format choices into one add_argument() call - scripts/formatters/__init__.py: merged both main's graphml dispatch block + PR #139's 5 new format dispatch blocks into format_output() - 6 doc files: took main's version (PR #139's count bump was wrong) Verified: - 85 tests pass in test_formatters_base.py + test_formatters_phase2.py + test_command_count.py (1 deselected: test_help_lists_new_formats fails due to pre-existing -f argparse conflict on main, not a regression from this PR) - sync_command_count.py --check: clean (75 commands) - All 5 formatter modules import cleanly with new headers Findings (di luar scope, di main juga): - argparse.ArgumentError: conflicting option string: -f — main's codelens.py adds --format/-f both at subparser level (line 892) AND at global parser level (line 926). Any CLI invocation crashes. This is a pre-existing bug on main (PR #157 --diff-base introduced the global -f), NOT a regression from PR #139 or this fix. Affects test_cli.py + test_doctor.py CLI smoke tests too. Suggest separate issue to fix.
a56eb51 to
a60029e
Compare
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




Summary
Implements Phase 1 + Phase 2 of issue #52 — unified
Findingdataclass + 5 new formatters (text, junit-xml, emacs, vim, gitlab-sast).Before this PR, every CodeLens formatter had its own ad-hoc finding-extraction logic — duplicated, slightly inconsistent. Phase 1 introduces a single
Findingdataclass andextract_findings()entry point. Phase 2 builds 5 new formatters that consumeFindingobjects, covering GitLab CI, Jenkins, Emacs, Vim, and human-readable terminal output.Phase 1 — Unified Finding dataclass:
scripts/formatters/base.pywithFindingdataclass,Severityconstants, andextract_findings()helperfindings+by_category(engines occasionally double-emit)Findinginternally. Refactoring them is a future Phase.Phase 2 — 5 standard formatters:
text— human-readable table:RULE ID | SEVERITY | LOCATION | MESSAGEjunit-xml— universal test result format for Jenkins/GitLab CI (critical/high →<failure>, medium/low/info →<skipped>)emacs—file:line:col: level: messageforcompile-modevim—file:line:col: [severity] messageforquickfixgitlab-sast— GitLab CI native security scan format with stable vulnerability IDs, CWE identifiers, severity→GitLab enum mappingFiles
New:
scripts/formatters/base.py—Findingdataclass,Severityconstants,extract_findings()scripts/formatters/text.py— table formatterscripts/formatters/junit_xml.py— JUnit XMLscripts/formatters/emacs.py— compile-modescripts/formatters/vim.py— quickfixscripts/formatters/gitlab_sast.py— GitLab SAST JSONtests/test_formatters_base.py— 36 tests for Phase 1tests/test_formatters_phase2.py— 46 tests for Phase 2Modified:
scripts/formatters/__init__.py—format_output()dispatches to new formattersscripts/codelens.py—--formatchoices updated (subcommand level)README.md,SKILL.md,SKILL-QUICK.md,pyproject.toml,skill.json,scripts/graph_model.py— auto-synced viasync_command_count.py --apply(no command count change, but MCP tool counts refreshed)Key design decisions
Findingis a behavior-risky change and out of scope for this PR.extract_findings()handles all engine output shapes (plain lists, category-keyed dicts, severity-keyed dicts, alternative field names likedefined_in/line_number/risk). Every Phase 2 formatter calls it — no duplication.critical/high/medium/low/info,error/warning/note,fatal/blocker/moderate)._normalize_severity()collapses them to the canonical set so formatters don't need to handle every variant.textwhich shows a count in the footer). Formatters' job is to surface actionable findings, not dismissed ones. Suppressed findings would clutter CI dashboards and editor quickfix.<failure>, medium/low/info →<skipped>. Matches how most teams configure CI quality gates — only blocking severities fail the build.xml.sax.saxutilsfor XML escaping,hashlibfor stable IDs,jsonfor gitlab-sast). Per issue spec: "Semgrep formatters are LGPL-2.1 — reference only, reimplement from spec."Acceptance criteria
textformatter: table with header, rows, severity summary footerjunit-xmlformatter: valid JUnit XML, critical/high as<failure>, others as<skipped>emacsformatter:file:line:col: level: messageper linevimformatter:file:line:col: [severity] messageper linegitlab-sastformatter: GitLab Secure spec compliant, stable vulnerability IDs, CWE identifierstextshows count in footer)format_output()dispatches new formats correctly(Phases 3–6 of #52 — SARIF codeFlows, JSONL streaming, TOON, issue-tracker scripts — are out of scope for this Phase 1+2 PR.)
Test results
tests/test_formatters_base.py— all passtests/test_formatters_phase2.py— all passCloses #52.