Harden upstream pin freshness reporting - #39
Conversation
Summary by CodeRabbit
WalkthroughThe freshness checker now validates upstream release and branch data, reports explicit current, stale, and unknown statuses, and integrates with workflow issue reconciliation. Unit tests cover checker and CLI behavior, and CI runs the test suite. ChangesPin freshness lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (7 passed)
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 @.github/workflows/ci.yml:
- Around line 15-16: Add a push-specific concurrency configuration to the CI
workflow, using a group keyed to the relevant push workflow or ref and
configuring newer runs to cancel in-progress older runs. Keep the existing test
step and other workflow behavior unchanged.
In
`@src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py`:
- Around line 75-88: Update the lookup exception handling around
urllib.request.urlopen in the freshness check to map transport failures,
including http.client.RemoteDisconnected and IncompleteRead, to FreshnessUnknown
so callers produce EXIT_UNKNOWN. Catch OSError and http.client.HTTPException
without overriding the existing HTTPError-specific details or malformed-JSON
handling, and add fixtures covering these failure cases with the expected
unknown exit status.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1122cef2-c8cc-499f-8a37-a306daf84fd9
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/pin-freshness.ymlsrc/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.pytests/scripts/test_check_pin_freshness.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (1)
.github/**
⚙️ CodeRabbit configuration file
GitHub Actions workflows. Review for: action version pinning (use SHA not tags for third-party actions), proper secret handling (no secrets in logs, use GITHUB_TOKEN where possible), unnecessary workflow triggers, and job dependency correctness. Flag missing concurrency groups on push-triggered workflows. Ensure matrix strategies cover the supported .NET TFMs.
Files:
.github/workflows/ci.yml.github/workflows/pin-freshness.yml
🔇 Additional comments (4)
src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py (1)
2-17: LGTM!Also applies to: 39-63, 90-222
tests/scripts/test_check_pin_freshness.py (1)
1-178: LGTM!.github/workflows/pin-freshness.yml (2)
5-7: LGTM!Also applies to: 44-102
65-69: 🗄️ Data Integrity & IntegrationNo change needed.
The workflow header sets
permissions.issues: write, so the reconciliation step has the required issue permissions.
| - name: Test pin freshness checker | ||
| run: python3 -m unittest discover --start-directory tests/scripts --pattern 'test_*.py' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,18p' .github/workflows/ci.yml
rg -n '^\s*concurrency:' .github/workflows/ci.ymlRepository: ANcpLua/Qyl.OpenTelemetry.SemanticConventions
Length of output: 558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== top section with concurrency search =="
awk '
/^[^ ].*[^ ]/ { line=$0; lineno++; print lineno"\t"line }
/^ concurrency:/ { print "FOUND CONCUURRENCY LINE", NR, $0 }
' .github/workflows/ci.yml | sed -n '1,80p'
echo "== relevant workflow header =="
sed -n '1,25p' .github/workflows/ci.yml
echo "== full concurrency occurrences =="
rg -n 'concurrency|push:|pull_request|permissions:|jobs:' .github/workflows/ci.yml || trueRepository: ANcpLua/Qyl.OpenTelemetry.SemanticConventions
Length of output: 953
Add push concurrency to .github/workflows/ci.yml.
This push-triggered workflow has no concurrency group, so add a push-specific concurrency group so later push runs do not keep running in parallel.
🤖 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 @.github/workflows/ci.yml around lines 15 - 16, Add a push-specific
concurrency configuration to the CI workflow, using a group keyed to the
relevant push workflow or ref and configuring newer runs to cancel in-progress
older runs. Keep the existing test step and other workflow behavior unchanged.
Source: Path instructions
| try: | ||
| with urllib.request.urlopen(request, timeout=30) as response: | ||
| return json.load(response) | ||
| payload = json.load(response) | ||
| except urllib.error.HTTPError as error: | ||
| detail = f"HTTP {error.code}" | ||
| if error.code in (403, 429): | ||
| detail += " (rate limited; set GITHUB_TOKEN)" | ||
| elif error.code == 404: | ||
| detail += " (renamed, deleted, or unknown ref)" | ||
| raise LookupFailed(f"{path}: {detail}") from error | ||
| raise FreshnessUnknown(f"{path}: {detail}") from error | ||
| except (urllib.error.URLError, TimeoutError) as error: | ||
| raise LookupFailed(f"{path}: {error}") from error | ||
| raise FreshnessUnknown(f"{path}: {error}") from error | ||
| except json.JSONDecodeError as error: | ||
| raise LookupFailed(f"{path}: upstream returned malformed JSON: {error}") from error | ||
| raise FreshnessUnknown(f"{path}: upstream returned malformed JSON: {error}") from error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import http.client
import urllib.error
assert not issubclass(http.client.RemoteDisconnected, urllib.error.URLError)
assert not issubclass(http.client.RemoteDisconnected, TimeoutError)
assert issubclass(http.client.RemoteDisconnected, OSError)
assert issubclass(http.client.IncompleteRead, http.client.HTTPException)
print("These transport exceptions are outside the current handler.")
PYRepository: ANcpLua/Qyl.OpenTelemetry.SemanticConventions
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py
ast-grep outline src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py || true
sed -n '1,180p' src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py
printf '\n--- workflow references ---\n'
if [ -f .github/workflows/pin-freshness.yml ]; then
sed -n '1,220p' .github/workflows/pin-freshness.yml
else
rg -n "pin-freshness|EXIT_UNKNOWN|unknown status|exit" .github src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts || true
fiRepository: ANcpLua/Qyl.OpenTelemetry.SemanticConventions
Length of output: 12892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '189,226p' src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py
printf '\n--- fixture references ---\n'
rg -n "FreshnessUnknown|fixtures|fixtures|RemoteDisconnected|IncompleteRead|check_pin_freshness|pin-freshness" tests src -g '!bin' -g '!node_modules' -g '!dist' || true
printf '\n--- direct exception-path probes ---\n'
python3 - <<'PY'
from pathlib import Path
import subprocess
import sys
import tempfile
script = Path("src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py")
if not script.exists():
raise SystemExit("script not found")
# Static imports and current handler set.
text = script.read_text()
print("imports http.client:", "import http.client" in text or "from http import client" in text)
print("catches JSONDecodeError:", "json.JSONDecodeError" in text)
print("catches TimeoutError:", "TimeoutError" in text)
print("catches URLError:", "urllib.error.URLError" in text)
print("catches OSError:", "OSError" in text)
print("catches HTTPException:", "http.client.HTTPException" in text)
print("catches RemoteDisconnected:", "RemoteDisconnected" in text)
print("catches IncompleteRead:", "IncompleteRead" in text)
print("main handles FreshnessUnknown:", "FreshnessUnknown" in text[text.find("def main"):])
PYRepository: ANcpLua/Qyl.OpenTelemetry.SemanticConventions
Length of output: 6274
Map transport failures from urllib calls into EXIT_UNKNOWN.
http.client.RemoteDisconnected escapes the current URLError/TimeoutError handler, while http.client.IncompleteRead is not covered by JSONDecodeError when no JSON is read. These cases become unexpected non-0/non-10/non-2 exits in .github/workflows/pin-freshness.yml; catch OSError from the lookup, handle http.client.HTTPException, and add missing unknown-status fixtures for these failures.
Proposed fix
+import http.client
+
- except (urllib.error.URLError, TimeoutError) as error:
+ except (
+ urllib.error.URLError,
+ OSError,
+ http.client.HTTPException,
+ UnicodeDecodeError,
+ ) as error:
raise FreshnessUnknown(f"{path}: {error}") from error📝 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.
| try: | |
| with urllib.request.urlopen(request, timeout=30) as response: | |
| return json.load(response) | |
| payload = json.load(response) | |
| except urllib.error.HTTPError as error: | |
| detail = f"HTTP {error.code}" | |
| if error.code in (403, 429): | |
| detail += " (rate limited; set GITHUB_TOKEN)" | |
| elif error.code == 404: | |
| detail += " (renamed, deleted, or unknown ref)" | |
| raise LookupFailed(f"{path}: {detail}") from error | |
| raise FreshnessUnknown(f"{path}: {detail}") from error | |
| except (urllib.error.URLError, TimeoutError) as error: | |
| raise LookupFailed(f"{path}: {error}") from error | |
| raise FreshnessUnknown(f"{path}: {error}") from error | |
| except json.JSONDecodeError as error: | |
| raise LookupFailed(f"{path}: upstream returned malformed JSON: {error}") from error | |
| raise FreshnessUnknown(f"{path}: upstream returned malformed JSON: {error}") from error | |
| import http.client | |
| try: | |
| with urllib.request.urlopen(request, timeout=30) as response: | |
| payload = json.load(response) | |
| except urllib.error.HTTPError as error: | |
| detail = f"HTTP {error.code}" | |
| if error.code in (403, 429): | |
| detail += " (rate limited; set GITHUB_TOKEN)" | |
| elif error.code == 404: | |
| detail += " (renamed, deleted, or unknown ref)" | |
| raise FreshnessUnknown(f"{path}: {detail}") from error | |
| except ( | |
| urllib.error.URLError, | |
| OSError, | |
| http.client.HTTPException, | |
| UnicodeDecodeError, | |
| ) as error: | |
| raise FreshnessUnknown(f"{path}: {error}") from error | |
| except json.JSONDecodeError as error: | |
| raise FreshnessUnknown(f"{path}: upstream returned malformed JSON: {error}") from error |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 75-75: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(request, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🤖 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
`@src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py`
around lines 75 - 88, Update the lookup exception handling around
urllib.request.urlopen in the freshness check to map transport failures,
including http.client.RemoteDisconnected and IncompleteRead, to FreshnessUnknown
so callers produce EXIT_UNKNOWN. Catch OSError and http.client.HTTPException
without overriding the existing HTTPError-specific details or malformed-JSON
handling, and add fixtures covering these failure cases with the expected
unknown exit status.
1 participant
Summary
Validation
actionlinton both modified workflows