-
Notifications
You must be signed in to change notification settings - Fork 8
[ShanaBoo] [$30 BOUNTY] [Python] Add build diagnostic metadata tests #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
61f27f9
5ce8ce7
2b54872
9697446
43f78c4
c2c53e9
17052e6
3d7f336
4bac705
201596c
db99170
471d8f8
d85f270
94e0fb0
f44a3a9
ae6177a
bc3b0e3
6da74de
1c9a0b2
d43a80d
c2f5550
4669ded
bd14054
3ebb128
c2ad359
6013743
f25aed7
771581b
659ee1e
6a937c4
3b5b1e2
3471b0a
4929f27
2e54676
e4ccebb
50a9eea
9f45d52
09a93d3
b3b8613
3bb116d
c542c0c
9e013be
ec51fd5
ff0959b
ad68a57
812bdbd
3ffe48d
644e3f8
64bb2b7
44312c7
8ca9e05
decb62b
e58d7b0
141ba65
a30e524
76c0cbf
7b5231f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| name: Automatic Approve | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: "*/5 * * * *" | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| actions: write | ||
| contents: read | ||
| pull-requests: read | ||
|
|
||
| jobs: | ||
| automatic-approve: | ||
| name: Automatic Approve | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Automatic Approve | ||
| uses: mheap/automatic-approve-action@v1 | ||
| with: | ||
| token: ${{ secrets.AUTOMATIC_APPROVE_PAT }} | ||
| workflows: "Diagnostic build log" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,62 +1,241 @@ | ||
| name: Diagnostic build log | ||
|
|
||
| on: | ||
| pull_request: | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened, ready_for_review] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: read | ||
|
|
||
| jobs: | ||
| require-diagnostic-build-log: | ||
| name: Require diagnostic build log bundle in PR | ||
| name: Require valid script-generated diagnostic bundle | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Check out repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Verify new diagnostic .logd and .json are committed in this PR | ||
| shell: bash | ||
| - name: Validate committed diagnostic metadata and encrypted log | ||
| env: | ||
| GITHUB_TOKEN: ${{ github.token }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| BASE_REPO: ${{ github.repository }} | ||
| HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| shell: python | ||
| run: | | ||
| set -euo pipefail | ||
| import base64 | ||
| import json | ||
| import os | ||
| import re | ||
| import sys | ||
| import urllib.error | ||
| import urllib.parse | ||
| import urllib.request | ||
|
|
||
| base_sha="${{ github.event.pull_request.base.sha }}" | ||
| head_sha="${{ github.event.pull_request.head.sha }}" | ||
| token = os.environ["GITHUB_TOKEN"] | ||
| pr_number = os.environ["PR_NUMBER"] | ||
| base_repo = os.environ["BASE_REPO"] | ||
| head_repo = os.environ["HEAD_REPO"] | ||
| head_sha = os.environ["HEAD_SHA"] | ||
| head_short = head_sha[:8] | ||
|
|
||
| echo "Checking PR diff from ${base_sha} to ${head_sha}" | ||
| api = "https://api.github.com" | ||
|
|
||
| mapfile -t new_logd < <( | ||
| git diff --name-only --diff-filter=A "${base_sha}...${head_sha}" -- 'diagnostic/*.logd' | ||
| ) | ||
| def request(path): | ||
| url = f"{api}{path}" | ||
| req = urllib.request.Request( | ||
| url, | ||
| headers={ | ||
| "Authorization": f"Bearer {token}", | ||
| "Accept": "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| "User-Agent": "diagnostic-build-log-validator", | ||
| }, | ||
| ) | ||
| try: | ||
| with urllib.request.urlopen(req, timeout=30) as response: | ||
| return json.loads(response.read().decode("utf-8")) | ||
| except urllib.error.HTTPError as exc: | ||
| body = exc.read().decode("utf-8", errors="replace") | ||
| raise RuntimeError(f"GitHub API {exc.code} for {url}: {body}") from exc | ||
|
|
||
| def get_pr_files(): | ||
| files = [] | ||
| page = 1 | ||
| while True: | ||
| batch = request( | ||
| f"/repos/{base_repo}/pulls/{pr_number}/files?per_page=100&page={page}" | ||
| ) | ||
| if not batch: | ||
| break | ||
| files.extend(batch) | ||
| page += 1 | ||
| return files | ||
|
|
||
| mapfile -t new_json < <( | ||
| git diff --name-only --diff-filter=A "${base_sha}...${head_sha}" -- 'diagnostic/*.json' | ||
| def get_file_bytes(repo, path, ref): | ||
| quoted_path = urllib.parse.quote(path) | ||
| quoted_ref = urllib.parse.quote(ref) | ||
| obj = request(f"/repos/{repo}/contents/{quoted_path}?ref={quoted_ref}") | ||
| if obj.get("encoding") == "base64" and "content" in obj: | ||
| return base64.b64decode(obj["content"]) | ||
| if obj.get("download_url"): | ||
| with urllib.request.urlopen(obj["download_url"], timeout=30) as response: | ||
| return response.read() | ||
| raise RuntimeError(f"Could not read {repo}:{path}@{ref}") | ||
|
|
||
| def error(message, file=None): | ||
| if file: | ||
| print(f"::error file={file}::{message}") | ||
| else: | ||
| print(f"::error::{message}") | ||
|
|
||
| files = get_pr_files() | ||
| changed = { | ||
| item["filename"]: item | ||
| for item in files | ||
| if item.get("status") not in {"removed"} | ||
| } | ||
|
|
||
| diagnostic_json_paths = sorted( | ||
| path for path in changed | ||
| if re.fullmatch(r"diagnostic/build-[0-9a-f]{8}\.json", path) | ||
| ) | ||
| diagnostic_logd_paths = sorted( | ||
| path for path in changed | ||
| if re.fullmatch(r"diagnostic/build-[0-9a-f]{8}(?:-part\d{3})?\.logd", path) | ||
| ) | ||
|
|
||
| if [ "${#new_logd[@]}" -eq 0 ]; then | ||
| echo "::error::This PR must commit a new diagnostic .logd file under diagnostic/." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ "${#new_json[@]}" -eq 0 ]; then | ||
| echo "::error::This PR must commit a new diagnostic .json file under diagnostic/." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Found new diagnostic .logd file(s):" | ||
| printf ' - %s\n' "${new_logd[@]}" | ||
|
|
||
| echo "Found new diagnostic .json file(s):" | ||
| printf ' - %s\n' "${new_json[@]}" | ||
|
|
||
| for file in "${new_logd[@]}" "${new_json[@]}"; do | ||
| if [ ! -f "${file}" ]; then | ||
| echo "::error file=${file}::Diagnostic file path is not a file." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ ! -s "${file}" ]; then | ||
| echo "::error file=${file}::Diagnostic file is empty." | ||
| exit 1 | ||
| fi | ||
| done | ||
| if not diagnostic_json_paths: | ||
| error( | ||
| "This PR must include a script-generated diagnostic/build-<commit>.json file. " | ||
| "Rebase onto upstream/main, run `python3 build.py`, and push the commit it creates." | ||
| ) | ||
| sys.exit(1) | ||
|
|
||
| if not diagnostic_logd_paths: | ||
| error( | ||
| "This PR must include a script-generated encrypted diagnostic/build-<commit>.logd file. " | ||
| "Do not hand-create metadata; rebase onto upstream/main and run `python3 build.py`." | ||
| ) | ||
| sys.exit(1) | ||
|
|
||
| def ensure_commit_is_ancestor(commit): | ||
| comparison = request(f"/repos/{head_repo}/compare/{commit}...{head_sha}") | ||
| status = comparison.get("status") | ||
| if status not in {"ahead", "identical"}: | ||
| raise RuntimeError( | ||
| f"diagnostic commit {commit!r} is not an ancestor of PR head {head_short} " | ||
| f"(compare status: {status})" | ||
| ) | ||
|
Comment on lines
+122
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Require diagnostics for the PR head commit, not any ancestor.
Proposed fix- def ensure_commit_is_ancestor(commit):
- comparison = request(f"/repos/{head_repo}/compare/{commit}...{head_sha}")
- status = comparison.get("status")
- if status not in {"ahead", "identical"}:
- raise RuntimeError(
- f"diagnostic commit {commit!r} is not an ancestor of PR head {head_short} "
- f"(compare status: {status})"
- )
+ def ensure_commit_matches_head(commit):
+ if commit != head_short:
+ raise RuntimeError(
+ f"diagnostic commit {commit!r} does not match PR head {head_short}"
+ )Also applies to: 151-162 🤖 Prompt for AI Agents |
||
|
|
||
| failures = [] | ||
| valid_reports = [] | ||
|
|
||
| for json_path in diagnostic_json_paths: | ||
| try: | ||
| raw = get_file_bytes(head_repo, json_path, head_sha) | ||
| metadata = json.loads(raw.decode("utf-8")) | ||
| except Exception as exc: | ||
| failures.append((json_path, f"Diagnostic JSON could not be read/parsed: {exc}")) | ||
| continue | ||
|
|
||
| commit = metadata.get("commit") | ||
| diagnostic_logd = metadata.get("diagnostic_logd") | ||
| diagnostic_logd_error = metadata.get("diagnostic_logd_error") | ||
| password = metadata.get("password") | ||
|
|
||
| if not isinstance(commit, str) or not re.fullmatch(r"[0-9a-f]{8}", commit): | ||
| failures.append((json_path, f"Diagnostic metadata has invalid commit value: {commit!r}")) | ||
| continue | ||
|
|
||
| try: | ||
| ensure_commit_is_ancestor(commit) | ||
| except Exception as exc: | ||
| failures.append( | ||
| ( | ||
| json_path, | ||
| f"Diagnostic metadata is stale or not on this PR branch: {exc}. " | ||
| "Run `python3 build.py` after rebasing and after your code changes.", | ||
| ) | ||
| ) | ||
| continue | ||
|
|
||
| expected_json_path = f"diagnostic/build-{commit}.json" | ||
| if json_path != expected_json_path: | ||
| failures.append((json_path, f"Diagnostic JSON path must be {expected_json_path}.")) | ||
| continue | ||
|
|
||
| if diagnostic_logd_error: | ||
| failures.append((json_path, f"Build script reported diagnostic_logd_error: {diagnostic_logd_error}")) | ||
| continue | ||
|
|
||
| if not diagnostic_logd: | ||
| failures.append((json_path, "diagnostic_logd is empty. Run `python3 build.py`; do not hand-edit JSON.")) | ||
| continue | ||
|
|
||
| if isinstance(diagnostic_logd, str): | ||
| logd_paths = [diagnostic_logd] | ||
| elif isinstance(diagnostic_logd, list) and all(isinstance(p, str) for p in diagnostic_logd): | ||
| logd_paths = diagnostic_logd | ||
| else: | ||
| failures.append((json_path, "diagnostic_logd must be a string path or list of string paths.")) | ||
| continue | ||
|
|
||
| if not password or not isinstance(password, str): | ||
| failures.append((json_path, "Diagnostic metadata is missing the decrypt password emitted by build.py.")) | ||
| continue | ||
|
|
||
| expected_prefix = f"diagnostic/build-{commit}" | ||
| bad_paths = [p for p in logd_paths if not re.fullmatch(rf"{re.escape(expected_prefix)}(?:-part\d{{3}})?\.logd", p)] | ||
| if bad_paths: | ||
| failures.append((json_path, f"diagnostic_logd references unexpected path(s): {', '.join(bad_paths)}")) | ||
| continue | ||
|
|
||
| missing_from_diff = [p for p in logd_paths if p not in changed] | ||
| if missing_from_diff: | ||
| failures.append((json_path, f"Referenced .logd file(s) are not committed in this PR: {', '.join(missing_from_diff)}")) | ||
| continue | ||
|
|
||
| if len(logd_paths) > 1: | ||
| expected_parts = [f"{expected_prefix}-part{i:03d}.logd" for i in range(1, len(logd_paths) + 1)] | ||
| if logd_paths != expected_parts: | ||
| failures.append((json_path, f"Chunked .logd paths must be contiguous and ordered: {', '.join(expected_parts)}")) | ||
| continue | ||
|
|
||
| logd_failures = [] | ||
| for index, logd_path in enumerate(logd_paths): | ||
| try: | ||
| data = get_file_bytes(head_repo, logd_path, head_sha) | ||
| except Exception as exc: | ||
| logd_failures.append(f"{logd_path}: could not read file: {exc}") | ||
| continue | ||
|
|
||
| if len(data) < 10_240: | ||
| logd_failures.append(f"{logd_path}: file is too small to be a real encrypted diagnostic archive ({len(data)} bytes)") | ||
| continue | ||
|
Comment on lines
+213
to
+215
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Avoid rejecting valid small script-generated diagnostics. The fixed 10 KiB minimum can fail deterministic local diagnostics when module output is short or external toolchains are absent. Prefer validating naming, metadata, and archive format without an arbitrary size floor. 🤖 Prompt for AI Agents |
||
|
|
||
| # Unsplit encryptly archives start with DIAG. For split archives, | ||
| # only the first chunk is guaranteed to have the archive magic. | ||
| if index == 0 and data[:4] != b"DIAG": | ||
| logd_failures.append(f"{logd_path}: first bytes are not the expected encryptly DIAG archive magic") | ||
|
|
||
| if logd_failures: | ||
| failures.append((json_path, "; ".join(logd_failures))) | ||
| continue | ||
|
|
||
| valid_reports.append((json_path, logd_paths)) | ||
|
|
||
| if not valid_reports: | ||
| for file, message in failures: | ||
| error(message, file=file) | ||
| error( | ||
| "No valid script-generated diagnostic bundle found. " | ||
| "Fix: `git fetch upstream && git rebase upstream/main && python3 build.py && git push --force-with-lease`." | ||
| ) | ||
| sys.exit(1) | ||
|
|
||
| print("Valid diagnostic bundle(s):") | ||
| for json_path, logd_paths in valid_reports: | ||
| print(f" - {json_path}") | ||
| for logd_path in logd_paths: | ||
| print(f" - {logd_path}") | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,9 @@ | ||||||||||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||||||||||
| # | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Shell script for running the AI pipeline automation. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #!/bin/bash | ||||||||||||||||||||||
| set -euo pipefail | ||||||||||||||||||||||
|
Comment on lines
+1
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Fix invalid shell syntax: triple quotes are not shell comments. The Replace the triple-quoted docstring with standard shell comments: -"""
-Shell script for running the AI pipeline automation.
-"""
+#!/bin/bash
+# Shell script for running the AI pipeline automation.
-#!/bin/bash
set -euo pipefailNote: The shebang must be on the very first line for the kernel to recognize it; moving it to line 5 breaks that requirement as well. 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Shellcheck (0.11.0)[error] 1-3: This is interpreted as a command name containing a linefeed. Double check syntax. (SC2289) 🤖 Prompt for AI Agents
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Shebang must be on line 1. With the triple-quoted block preceding it, the shebang 🤖 Prompt for AI Agents |
||||||||||||||||||||||
| # ai_pipeline.sh - AI Training Pipeline Orchestrator | ||||||||||||||||||||||
| # ================================================== | ||||||||||||||||||||||
| # | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| """ | ||
| Backend package for the TentOfTrials trading and risk platform. | ||
| Implements core server logic, data models, and API endpoints in Rust. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major
Pin the third-party action to a commit SHA.
The action
mheap/automatic-approve-action@v1uses a mutable tag. Passing theAUTOMATIC_APPROVE_PATsecret to a mutable tag reference creates a supply chain risk. Pin the action to the specific commit SHA to ensure immutability.[preventive_hardening]
🧰 Tools
🪛 zizmor (1.26.1)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents