[BOUNTY] Add diagnostic metadata diff tool (#5)#30
Conversation
Add tools/diagnostic_diff.py to compare two diagnostic build JSON reports and report metadata field changes, per-module status transitions, and added/removed modules. Supports human-readable and JSON output via --json, plus --output to write to a file. Includes unit tests covering metadata changes, module status transitions, added/removed modules, elapsed_seconds deltas, identical reports, formatting, and CLI parsing. Addresses bounty mannowell#5.
📝 WalkthroughWalkthroughAdds ChangesDiagnostic Build Report Diff Tool
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/diagnostic_diff.py`:
- Around line 157-173: The main() function calls load_report() for both args.old
and args.new without any error handling, but these calls can raise
FileNotFoundError or json.JSONDecodeError. Wrap the load_report() calls in a
try/except block to catch these exceptions and provide user-friendly error
messages instead of letting the exceptions propagate uncaught. When a
FileNotFoundError or json.JSONDecodeError occurs, print a clear error message
describing the issue and exit gracefully with an appropriate error code using
sys.exit(), ensuring users see helpful feedback instead of a traceback.
- Around line 178-179: The main() function returns an exit code, but the code in
the if __name__ == "__main__": block simply calls main() without using
sys.exit() to pass the return code to the shell. Modify the code to call
sys.exit(main()) instead of just main() to ensure the exit code is properly
communicated to the process, allowing CI and shell scripts to detect failures
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 291aa415-2496-4d31-8cad-06b4ffedff02
📒 Files selected for processing (4)
diagnostic/build-d8740957.jsondiagnostic/build-d8740957.logdtests/test_diagnostic_diff.pytools/diagnostic_diff.py
| def main() -> int: | ||
| args = parse_args() | ||
| old = load_report(args.old) | ||
| new = load_report(args.new) | ||
| diff = diff_reports(old, new) | ||
|
|
||
| if args.json: | ||
| output = json.dumps(diff, indent=2, default=str) | ||
| else: | ||
| output = format_diff(diff) | ||
|
|
||
| if args.output: | ||
| with open(args.output, "w", encoding="utf-8") as f: | ||
| f.write(output) | ||
| print(f"Diff written to {args.output}") | ||
| else: | ||
| print(output) |
There was a problem hiding this comment.
Add error handling for missing or invalid diagnostic reports.
Currently, load_report() can raise FileNotFoundError or json.JSONDecodeError (lines 159–160), but main() has no try/except block. Exceptions will propagate uncaught, producing a traceback instead of a user-friendly error message.
💡 Suggested error handling
def main() -> int:
args = parse_args()
+ try:
+ old = load_report(args.old)
+ new = load_report(args.new)
+ except FileNotFoundError as e:
+ print(f"Error: File not found: {e.filename}", file=sys.stderr)
+ return 1
+ except json.JSONDecodeError as e:
+ print(f"Error: Invalid JSON: {e.msg} at line {e.lineno}", file=sys.stderr)
+ return 1
old = load_report(args.old)
new = load_report(args.new)
diff = diff_reports(old, new)🧰 Tools
🪛 ast-grep (0.44.0)
[info] 163-163: use jsonify instead of json.dumps for JSON output
Context: json.dumps(diff, indent=2, default=str)
Note: Security best practice.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/diagnostic_diff.py` around lines 157 - 173, The main() function calls
load_report() for both args.old and args.new without any error handling, but
these calls can raise FileNotFoundError or json.JSONDecodeError. Wrap the
load_report() calls in a try/except block to catch these exceptions and provide
user-friendly error messages instead of letting the exceptions propagate
uncaught. When a FileNotFoundError or json.JSONDecodeError occurs, print a clear
error message describing the issue and exit gracefully with an appropriate error
code using sys.exit(), ensuring users see helpful feedback instead of a
traceback.
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
Use sys.exit() to communicate return code to the shell.
The main() function returns an exit code (0), but sys.exit() is not called to pass it along. This means the process will always exit with code 0 regardless of errors, preventing CI/shell scripts from detecting failures.
🔧 Proposed fix
if __name__ == "__main__":
- main()
+ sys.exit(main())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if __name__ == "__main__": | |
| main() | |
| if __name__ == "__main__": | |
| sys.exit(main()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/diagnostic_diff.py` around lines 178 - 179, The main() function returns
an exit code, but the code in the if __name__ == "__main__": block simply calls
main() without using sys.exit() to pass the return code to the shell. Modify the
code to call sys.exit(main()) instead of just main() to ensure the exit code is
properly communicated to the process, allowing CI and shell scripts to detect
failures correctly.
1 participant
Summary
Adds a diagnostic metadata diff tool (
tools/diagnostic_diff.py) that compares two diagnostic build JSON reports and reports metadata and per-module differences, addressing bounty #5.Changes
tools/diagnostic_diff.py:diff_reports(old, new)compares top-level metadata fields (commit, passed, failed, generated_at, etc.) and per-module status/elapsed_seconds/artifact, plus added/removed modules.format_diff()renders a human-readable diff with sections for metadata changes, added/removed modules, and module status transitions.--jsonoutputs the diff as JSON;--outputwrites to a file.load_report()reads a diagnostic build JSON file.tests/test_diagnostic_diff.py: 10 unit tests covering metadata changes, module status transitions, added/removed modules, elapsed_seconds deltas, identical reports, formatting, report loading, and CLI parsing.diagnostic/build-d8740957.logd+.json: required diagnostic bundle.Testing
python3 tests/test_diagnostic_diff.py -v-> 10 tests pass.python3 build.py-> diagnostic bundle generated and committed (diagnostic/build-d8740957.logd, 15182 bytes,DIAGmagic).Checklist
Addresses bounty issue #5. Please let me know the process for claiming the $25 bounty once merged.
Summary by CodeRabbit
New Features
Tests