Improve fallback handling, logging, and add test

[sushantkhemalapure]

What does this PR do?

This PR improves fault tolerance and observability in the advisory workflow by:

• Adding structured logging for GitHub event handling and file operations
• Preventing crashes on invalid or missing GitHub event payloads
• Improving intent extraction fallback (PR body → issue body)
• Making advisory output path OS-safe using tempfile.gettempdir()
• Adding a unit test to validate fallback behavior

---

Why is this useful?

• Prevents silent failures in fallback scenarios
• Improves debugging with clearer logs
• Ensures consistent behavior when PR body is missing
• Adds test coverage for critical fallback logic

---

Changes made

• Updated github_integration.py
• Updated test_advisory.py

---

Testing

• Ran the new test case successfully

---

Related to

Partial contribution towards improving fallback fault tolerance (#4)

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for missing event files and JSON parsing failures.
  • Enhanced output file handling with a safe temporary fallback and reliable UTF‑8 writes.
  • Added logging for operational outcomes and failures; intent extraction now falls back to secondary content when needed.

• Tests

  • Added test coverage for intent fallback from issue content and related cleanup.

[owasp-blt]

📊 Monthly Leaderboard

Hi @sushantkhemalapure! Here's how you rank for March 2026:

Rank	User	Open PRs	PRs (merged)	PRs (closed)	Reviews	Comments	Total
94	@mujeeb-mughal1	1	0	0	0	2	5
95	@sushantkhemalapure ✨	1	0	0	0	2	5
96	@hinzwilliam52-ship-it	0	0	0	0	2	4

---

Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run /leaderboard on any issue or PR to see your rank!

[owasp-blt]

👋 Hi @sushantkhemalapure!

This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:

• The PR author
• coderabbitai
• copilot

Once a valid peer review is submitted, this check will pass automatically. Thank you!

⚠️ Peer review enforcement is active.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1cd96725-9375-4a57-b4f6-2e752c0766ec

📥 Commits

Reviewing files that changed from the base of the PR and between 26bddf6 and fd6c8f1.

📒 Files selected for processing (1)

• test_advisory.py

🚧 Files skipped from review as they are similar to previous changes (1)

• test_advisory.py

---

Walkthrough

Improved GitHub integration: added logging, explicit JSON parse/error handling, PR-intent fallback to issue body, safer output path handling using temp directories, UTF-8 file I/O, and a new test validating the issue-body intent fallback.

Changes

Cohort / File(s)	Summary
Production Code - GitHub Integration src/advisory_engine/github_integration.py	Added logger and imports; get_pr_context() now handles missing event file and json.JSONDecodeError, and falls back to issue.body for intent when PR body is absent; post_advisory_comment() uses a temp-directory default for output_file, writes with UTF-8, logs successes, and logs exceptions instead of printing.
Test Coverage test_advisory.py	Added test_github_issue_body_intent_fallback() which creates a temporary GitHub event JSON (with issue.body and issue.labels), sets GITHUB_EVENT_PATH/GITHUB_REPOSITORY, asserts contributor_intent and issue_labels, and restores environment state/cleans temp file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

quality: medium

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Improve fallback handling, logging, and add test' directly and accurately summarizes the main changes: enhanced fallback handling for intent extraction, added structured logging throughout, and introduced a new test case for fallback behavior validation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/advisory_engine/github_integration.py (1)

32-41: ⚠️ Potential issue | 🟠 Major

Harden payload parsing against file I/O errors and non-object JSON

open() failures (e.g., permissions) and valid-but-non-object JSON still escape this guard and can crash later at event_data.get(...). This weakens the fault-tolerance goal.

🔧 Proposed fix

-        try:
-            with open(self.event_path, 'r', encoding='utf-8') as f:
-                event_data = json.load(f)
-        except json.JSONDecodeError:
-            logger.exception("GitHub event payload is not valid JSON: %s", self.event_path)
-            return None
+        try:
+            with open(self.event_path, 'r', encoding='utf-8') as f:
+                event_data = json.load(f)
+        except (OSError, json.JSONDecodeError):
+            logger.exception("Failed to read/parse GitHub event payload: %s", self.event_path)
+            return None
+
+        if not isinstance(event_data, dict):
+            logger.warning("GitHub event payload must be a JSON object: %s", self.event_path)
+            return None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/advisory_engine/github_integration.py` around lines 32 - 41, The current
payload parsing only catches json.JSONDecodeError but misses file I/O errors and
valid-but-non-object JSON; update the try/except around opening and
json.load(self.event_path) to also catch FileNotFoundError/OSError (or a broad
Exception as e) and log via logger.exception including self.event_path and the
error, then return None; after loading, verify event_data is a dict (e.g., check
isinstance(event_data, dict)) and if not, logger.error that the payload is not a
JSON object for self.event_path and return None before accessing
event_data.get(...) for pr and issue.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test_advisory.py`:
- Line 245: The test output contains a mojibake sequence "✓" in the print
statement that prints success; update the string in the print call in
test_advisory.py (the success message for the GitHub issue-body fallback test)
to use a proper checkmark character (e.g., "✓") or a plain ASCII equivalent like
"OK" so the test output is readable and not mis-encoded.

---

Outside diff comments:
In `@src/advisory_engine/github_integration.py`:
- Around line 32-41: The current payload parsing only catches
json.JSONDecodeError but misses file I/O errors and valid-but-non-object JSON;
update the try/except around opening and json.load(self.event_path) to also
catch FileNotFoundError/OSError (or a broad Exception as e) and log via
logger.exception including self.event_path and the error, then return None;
after loading, verify event_data is a dict (e.g., check isinstance(event_data,
dict)) and if not, logger.error that the payload is not a JSON object for
self.event_path and return None before accessing event_data.get(...) for pr and
issue.

🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: b830a560-842d-4857-87b6-4b7dedc2b908

📥 Commits

Reviewing files that changed from the base of the PR and between 0115961 and 26bddf6.

📒 Files selected for processing (2)

• src/advisory_engine/github_integration.py
• test_advisory.py

[sushantkhemalapure]

Hi, I’ve opened a PR addressing part of this issue by improving fallback handling, logging, and adding a test.

Would appreciate your feedback.

[sushantkhemalapure]

Hi @coderabbitai,

I’ve fixed the encoding issue in the test output. Please take a look.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the changed files
echo "=== github_integration.py ==="
fd -e py -g 'github_integration.py' --exec cat {}

echo ""
echo "=== test_advisory.py ==="
fd -e py -g 'test_advisory.py' --exec cat {}

Length of output: 13055

---

@sushantkhemalapure Thanks for the update! The encoding fix (encoding="utf-8" in NamedTemporaryFile) is correct and important for cross-platform portability, especially on Windows where the default encoding varies. The overall changes look solid — the fallback logic, structured logging, and OS-safe temp path are all improvements. Here are a few things worth addressing:

---

🐛 logger.exception() logs the exception twice

In post_advisory_comment():

logger.exception("Error writing advisory output to %s: %s", output_file, e)

logger.exception() already appends the full traceback and exception message automatically. Passing e as an extra %s argument causes the exception text to appear twice in the log. Fix:

logger.exception("Error writing advisory output to %s", output_file)

---

⚠️ KeyError risk in label extraction

labels = [label['name'] for label in pr.get('labels', [])]

If a label dict in the payload is malformed (missing 'name'), this will raise an uncaught KeyError. Use .get() with filtering:

labels = [label.get('name') for label in pr.get('labels', []) if label.get('name')]

The same applies to the issue labels fallback below it.

---

🧪 Test isolation — one failure aborts the rest

In main(), a single AssertionError stops all subsequent tests. Consider running each test independently and collecting failures:

def main():
    tests = [
        test_advisory_generation,
        test_report_generation,
        test_feedback_recording,
        test_intent_capture,
        test_dashboard,
        test_pattern_matching,
        test_pf_check_command,
        test_github_issue_body_intent_fallback,
    ]
    failures = []
    for test in tests:
        try:
            test()
        except Exception as e:
            failures.append((test.__name__, e))
    if failures:
        for name, err in failures:
            print(f"\n❌ {name} failed: {err}")
        return 1
    print("\n✅ All tests passed!")
    return 0

---

ℹ️ Minor: test_pf_check_command() is an integration test

This test calls subprocess.run(["pf", ...]) which requires the pf CLI to be installed. If it's run in a clean CI environment without pf, it will fail unexpectedly. Consider separating it from the unit test suite or guarding it:

import shutil
if shutil.which("pf") is None:
    print("⚠️  Skipping pf check — 'pf' not installed")
    return

---

✅ What looks good

• encoding="utf-8" fix in both NamedTemporaryFile and open() calls — correct and portable.
• tempfile.gettempdir() replaces the hardcoded /tmp — right approach for cross-OS safety.
• json.JSONDecodeError is caught explicitly rather than a broad Exception — good practice.
• The finally block in test_github_issue_body_intent_fallback() properly restores env vars and deletes the temp file even on assertion failure.
• The intent fallback pr.get('body') or issue.get('body', '') correctly prefers PR body over issue body.

[S3DFX-CYBER]

@sushantkhemalapure Currently we are restricting PRs on gsoc labelled repos as they are supposed to be worked on gsoc period. Feel free to make PRs in any other repo

[sushantkhemalapure]

@S3DFX-CYBER
Thanks for the clarification!

I’ll look into other repositories to contribute.
Appreciate the guidance.