diff --git a/.github/scripts/assignment1-feedback.md b/.github/scripts/assignment1-feedback.md new file mode 100644 index 000000000..9805b27ee --- /dev/null +++ b/.github/scripts/assignment1-feedback.md @@ -0,0 +1,63 @@ +# Assignment 1 execution feedback + +Assignment 1 runs separately in `sql_assignment1_runner.yml`. The existing +`sql_assignment_runner.yml`, `grade_assignments.py`, and Assignment 2 tests are +unchanged. A1 uses `a1-test-results.json` / `a1-grading-results`, so it cannot +replace A2's report or artifact. + +## Historical source and scope + +The implementation restores the behavior of `tests/test_assignment.py` immediately +before `3fe0023c0af279b275f746948ad08134a631a25f` (October 9, 2026). +The March 24 runner (`cf333a0`) evolved through the March 27 parser changes +(`0349f73`) and April 1 multistatement support (`3445382`). By April 5 +(`f7e32c2`, `ae700d8`), the workflow was manual-only with an explicit PR number. +The old workflow selected A2 in preference to A1 when both changed. The restoration +runs A1 independently, so an A2 submission no longer suppresses A1 feedback. + +The historical runner only executed SQL and displayed up to three rows. This +restoration likewise does **not** judge answer correctness or award marks. +Feedback explicitly distinguishes executed SQL, missing answers, and SQL errors. +All 12 A1 questions are represented. Instructors still mark answers and written work. + +The restored parser accepts `--QUERY 1` and `-- QUERY 1`, and closes blocks with +`--END QUERY` or the historical `--END STATEMENT`. It rejects malformed, duplicate, +unclosed, and out-of-range markers. Multistatement SQL uses SQLite statement +boundaries, preserving semicolons in strings and comments. Queries share a disposable +in-memory copy, preserving temporary tables between blocks; the course database is +opened read-only, external database attachment is denied, and queries have a +10-second execution limit plus a five-minute workflow limit. + +## Automatic and manual use + +On student repositories (owner other than `UofT-DSI`), A1 runs for PRs targeting +`main` from `assignment-one`, `assignment-1`, `assignment-two`, or `assignment-2` +when an `assignment1.sql` file changes. A mixed A1/A2 PR on an A2 branch runs both +workflows. Each runner selects its own assignment file and rejects multiple cohorts' +files rather than guessing. Existing A2 branch and owner rules remain unchanged: +A2 does not automatically run on an A1 branch. + +For the upstream `UofT-DSI` repository, or when automatic PR commenting is restricted +by GitHub token permissions, use the A1 workflow's **Run workflow** action. Choose +a workflow ref containing this restoration, enter the submission branch and target +PR number. The submission branch must also contain the restored runner and tests. +The workflow compares the branch against `origin/main`; exactly one changed +`assignment1.sql` is required. A branch already merged into main has no changed +submission to select. The JSON artifact is retained even if posting feedback fails. + +Use **SQL Unit Tests** for A2's existing manual workflow. No privileged +`pull_request_target` trigger is added. + +## Local validation + +From the repository root, after installing `.github/scripts/requirements.txt`: + +```sh +python -m pytest 03_instructional_team/tests/test_assignment1_regressions.py 03_instructional_team/tests/test_grader_regressions.py -q +``` + +The new tests exercise SQL execution (including an intentionally wrong but executable +answer), missing/invalid submissions, multi-statement state, bounded result previews, +database isolation, query timeout, and CLI selection from mixed A1/A2 changes. +A2's existing private-rubric tests require `A2_SOLUTION_RUBRIC`; without that input +pytest reports them as skipped. diff --git a/.github/scripts/grade_assignment1.py b/.github/scripts/grade_assignment1.py new file mode 100644 index 000000000..b7574ffa4 --- /dev/null +++ b/.github/scripts/grade_assignment1.py @@ -0,0 +1,116 @@ +"""Restore A1 execution feedback from tests/test_assignment.py at 3fe0023^. + +As in the historical runner, this executes SQL and shows up to three rows; +it does not award marks or establish answer correctness. +""" +import json +from pathlib import Path +import re +import sqlite3 +import sys +import time + +DATABASE = Path("05_src/sql/farmersmarket.db") + + +def load_queries(sql_file): + """Read the historical QUERY / END QUERY (or END STATEMENT) blocks.""" + queries = {} + number = None + buffer = [] + for line in Path(sql_file).read_text().splitlines(): + stripped = line.strip() + start = re.fullmatch(r"--\s*QUERY\s+(\d+)\s*", stripped, re.I) + end = re.fullmatch(r"--\s*END\s+(?:QUERY|STATEMENT)\s*", stripped, re.I) + if start: + if number is not None: + raise ValueError("Nested or unclosed QUERY block.") + number = int(start[1]) + if number not in range(1, 13) or number in queries: + raise ValueError(f"Unknown or duplicate QUERY number: {number}.") + buffer = [] + elif end: + if number is None: + raise ValueError("END marker has no matching QUERY marker.") + queries[number] = "\n".join(buffer).strip() + number = None + elif re.match(r"--\s*(QUERY|END\s+(?:QUERY|STATEMENT))\b", stripped, re.I): + raise ValueError(f"Invalid answer marker: {stripped}") + elif number is not None: + buffer.append(line) + if number is not None: + raise ValueError(f"QUERY {number} has no END marker.") + return [{"number": n, "query": queries.get(n, "")} for n in range(1, 13)] + + +def has_sql(query): + return bool(re.sub(r"--[^\n]*|/\*[\s\S]*?\*/|[\s;]+", "", query)) + + +def run_query(conn, query): + """Keep multistatement support without splitting quoted semicolons.""" + deadline = time.monotonic() + 10 + conn.set_progress_handler(lambda: int(time.monotonic() > deadline), 1000) + cursor = conn.cursor() + buffer = "" + try: + for character in query: + buffer += character + if character == ";" and sqlite3.complete_statement(buffer): + if has_sql(buffer): + cursor.execute(buffer) + buffer = "" + if has_sql(buffer): + cursor.execute(buffer) + return [dict(row) for row in cursor.fetchmany(3)] + finally: + conn.set_progress_handler(None, 0) + + +def run_assignment(file_path): + """Execute on a disposable database, retaining state between A1 blocks.""" + queries = load_queries(file_path) + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + try: + with sqlite3.connect(f"file:{DATABASE}?mode=ro", uri=True) as source: + source.backup(conn) + conn.set_authorizer(lambda action, *_: sqlite3.SQLITE_DENY + if action in (sqlite3.SQLITE_ATTACH, sqlite3.SQLITE_DETACH) + else sqlite3.SQLITE_OK) + results = [] + for parsed in queries: + result = dict(parsed, result=[], error=None) + if not has_sql(parsed["query"]): + result.update(status="missing", feedback="No SQL answer supplied.") + else: + try: + result["result"] = run_query(conn, parsed["query"]) + result.update(status="executed", feedback="SQL executed. Correctness requires instructor review.") + except Exception as error: + result.update(status="error", feedback="SQL could not execute.", error=str(error)) + results.append(result) + return results + finally: + conn.close() + + +def grade_files(files, output): + """Require one cohort's A1 file and always replace stale reports.""" + report = {"file": "Submission selection", "results": [], "runner_error": None} + try: + if len(files) != 1: + raise ValueError("No changed assignment1.sql file was found." if not files else + "More than one assignment1.sql file changed. Submit only your cohort's file.") + report["file"] = files[0] + report["results"] = run_assignment(files[0]) + except Exception as error: + report["runner_error"] = str(error) + Path(output).write_text(json.dumps([report], indent=2)) + return int(report["runner_error"] is not None) + + +if __name__ == "__main__": + changed = Path(sys.argv[1]).read_bytes().decode().split("\0") + files = sorted({path for path in changed if Path(path).name == "assignment1.sql"}) + sys.exit(grade_files(files, sys.argv[2])) diff --git a/.github/workflows/sql_assignment1_runner.yml b/.github/workflows/sql_assignment1_runner.yml new file mode 100644 index 000000000..535783b35 --- /dev/null +++ b/.github/workflows/sql_assignment1_runner.yml @@ -0,0 +1,143 @@ +name: SQL Assignment 1 Execution Feedback + +on: + pull_request: + branches: + - main + types: [opened, reopened, synchronize] + paths: + - '**/assignment1.sql' + workflow_dispatch: # Manual trigger from GitHub UI + inputs: + branch: + description: 'Assignment 1 branch to run, usually assignment-one' + required: true + default: assignment-one + type: string + pr_number: + description: 'Pull request number. Extract a number from the end of pull request url, i.e. https://github.com/khsergvl/sql/pull/17' + required: true + type: string + default: '1' +permissions: + contents: read + pull-requests: write + +jobs: + run-assignment-queries: + if: >- + github.event_name == 'workflow_dispatch' || + ((github.head_ref == 'assignment-one' || github.head_ref == 'assignment-1' || + github.head_ref == 'assignment-two' || github.head_ref == 'assignment-2') && + github.repository_owner != 'UofT-DSI') + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + pull-requests: write + + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + ref: ${{ github.event_name == 'workflow_dispatch' && inputs.branch || github.event.pull_request.head.sha }} + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Install dependencies + run: | + pip install -r ./.github/scripts/requirements.txt + + - name: Verify grader regressions + run: python -m pytest 03_instructional_team/tests/test_assignment1_regressions.py -q + + - name: Get changed files + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + base_ref="${BASE_SHA:-origin/main}" + git diff --name-only -z "$base_ref"...HEAD > changed_files.txt + + - name: Grade changed assignments + id: grading + continue-on-error: true + run: python .github/scripts/grade_assignment1.py changed_files.txt a1-test-results.json + + - name: Post grading feedback to PR + if: always() && steps.grading.outcome != 'skipped' + uses: actions/github-script@v7 + env: + PR_NUMBER: ${{ github.event_name == 'workflow_dispatch' && inputs.pr_number || github.event.pull_request.number }} + with: + script: | + const fs = require('fs'); + const labels = { + executed: '⚪ SQL executed; instructor review needed', error: '❌ Error', + missing: '❌ Missing answer', blocked: '⚪ Cannot assess: prerequisite failed', + manual_review: '⚪ Instructor review needed' + }; + function fenced(value, language = '') { + const text = String(value); + const runs = text.match(/`+/g) || []; + const fence = '`'.repeat(Math.max(3, ...runs.map(run => run.length + 1))); + return `${fence}${language}\n${text}\n${fence}\n`; + } + let reports; + try { + reports = JSON.parse(fs.readFileSync('a1-test-results.json', 'utf8')); + if (!Array.isArray(reports)) throw new Error('Invalid report format'); + } catch (error) { + reports = [{file: 'Grading runner', results: [], runner_error: error.message}]; + } + if (!reports.length) return; + let body = '### SQL Assignment 1 execution feedback\n\n'; + body += 'Restored A1 execution checks: up to three result rows per query. Successful execution does not establish correctness or award marks; instructor review is required.\n\n'; + const truncationNotice = '\nFeedback truncated; inspect the workflow report for remaining results.'; + const maxBodyLength = 60000; + let truncated = false; + function appendSection(section) { + if (body.length + section.length + truncationNotice.length > maxBodyLength) { + truncated = true; + return false; + } + body += section; + return true; + } + reportLoop: + for (const report of reports) { + let reportSection = fenced(report.file); + if (report.runner_error) reportSection += `Grading could not finish: ${report.runner_error}\n\n`; + if (!appendSection(reportSection)) break; + for (const result of report.results) { + let resultSection = `**Query ${result.number}: ${labels[result.status] || '⚪ Instructor review needed'}**\n\n`; + resultSection += `${result.feedback}\n\n`; + if (result.error) resultSection += fenced(result.error); + if (result.query) resultSection += fenced(result.query, 'sql'); + if (result.result?.length) resultSection += fenced(JSON.stringify(result.result, null, 2), 'json'); + resultSection += '\n'; + if (!appendSection(resultSection)) break reportLoop; + } + } + if (truncated) body += truncationNotice; + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: Number(process.env.PR_NUMBER), + body + }); + + - name: Upload grading reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: a1-grading-results + path: a1-test-results.json + if-no-files-found: ignore + + - name: Preserve runner failure status + if: always() && steps.grading.outcome == 'failure' + run: exit 1 diff --git a/03_instructional_team/tests/test_assignment1_regressions.py b/03_instructional_team/tests/test_assignment1_regressions.py new file mode 100644 index 000000000..2f6db660f --- /dev/null +++ b/03_instructional_team/tests/test_assignment1_regressions.py @@ -0,0 +1,106 @@ +"""Regression checks for restored A1 feedback; no private solutions required.""" +import hashlib +import json +from pathlib import Path +import runpy +import sqlite3 +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parents[2] +G = runpy.run_path(str(ROOT / '.github/scripts/grade_assignment1.py')) + + +class Assignment1RegressionTests(unittest.TestCase): + def submission(self, text): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / 'assignment1.sql' + path.write_text(text) + return G['run_assignment'](path) + + def test_execution_is_not_correctness_and_errors_do_not_hide_later_answers(self): + results = self.submission('-- QUERY 1\nSELECT 1 AS wrong_answer;\n-- END QUERY\n' + '--QUERY 2\nSELECT * FROM no_such_table;\n--END QUERY\n' + '--QUERY 3\nSELECT * FROM customer;\n--END QUERY') + self.assertEqual(results[0]['status'], 'executed') + self.assertIn('instructor review', results[0]['feedback']) + self.assertEqual(results[1]['status'], 'error') + self.assertEqual(results[2]['status'], 'executed') + self.assertEqual(len(results[2]['result']), 3) + self.assertTrue(all(r['status'] == 'missing' for r in results[3:])) + + def test_historical_multistatement_and_end_statement_preserve_state(self): + results = self.submission("--QUERY 10\nCREATE TEMP TABLE sample(value);\n" + "INSERT INTO sample VALUES ('Pie; special'); -- comment;\n" + "SELECT * FROM sample;\n--END STATEMENT\n" + "--QUERY 11\nSELECT * FROM sample;\n--END QUERY") + for number in (10, 11): + self.assertEqual(results[number-1]['result'], [{'value': 'Pie; special'}]) + + def test_empty_and_comment_only_answers_are_missing(self): + for text in ('', '--QUERY 1\n-- no answer\n/* empty */;\n--END QUERY'): + self.assertTrue(all(r['status'] == 'missing' for r in self.submission(text))) + + def test_malformed_or_missing_submissions_replace_stale_reports(self): + for text in ('--QUERY 1\nSELECT 1', '--QUERY nope', '--QUERY 99\n--END QUERY', + '--QUERY 1\n--QUERY 2', '--END QUERY', + '--QUERY 1\n--END QUERY\n--QUERY 1\n--END QUERY'): + with self.subTest(text=text), tempfile.TemporaryDirectory() as directory: + path = Path(directory) / 'assignment1.sql' + path.write_text(text) + output = Path(directory) / 'report.json' + output.write_text('[{"stale": true}]') + self.assertEqual(G['grade_files']([str(path)], output), 1) + report = json.loads(output.read_text())[0] + self.assertTrue(report['runner_error']) + self.assertEqual(report['results'], []) + with tempfile.TemporaryDirectory() as directory: + output = Path(directory) / 'report.json' + for files in ([], ['one/assignment1.sql', 'two/assignment1.sql'], ['missing/assignment1.sql']): + self.assertEqual(G['grade_files'](files, output), 1) + self.assertTrue(json.loads(output.read_text())[0]['runner_error']) + + def test_database_is_unchanged_and_disk_attachment_is_denied(self): + database = ROOT / '05_src/sql/farmersmarket.db' + before = hashlib.sha256(database.read_bytes()).digest() + with tempfile.TemporaryDirectory() as directory: + target = Path(directory) / 'disk.db' + results = self.submission(f"--QUERY 1\nATTACH DATABASE '{target}' AS disk;\n--END QUERY") + self.assertEqual(results[0]['status'], 'error') + self.assertFalse(target.exists()) + self.submission('--QUERY 1\nDELETE FROM customer;\n--END QUERY') + self.assertEqual(hashlib.sha256(database.read_bytes()).digest(), before) + + def test_long_query_is_interrupted(self): + conn = sqlite3.connect(':memory:') + try: + ticks = iter((0, 11)) + with patch('time.monotonic', side_effect=lambda: next(ticks, 11)): + with self.assertRaises(sqlite3.OperationalError): + G['run_query'](conn, 'WITH RECURSIVE n(x) AS (VALUES(1) UNION ALL SELECT x+1 FROM n WHERE x<10000000) SELECT SUM(x) FROM n;') + finally: + conn.close() + + def test_cli_selects_a1_from_mixed_assignment_changes(self): + with tempfile.TemporaryDirectory() as directory: + directory = Path(directory) + # A space in the path verifies NUL-delimited selection as well. + submission = directory / 'cohort with spaces' / 'assignment1.sql' + submission.parent.mkdir() + submission.write_text('--QUERY 1\nSELECT 1 AS value;\n--END QUERY') + changed = directory / 'changed.txt' + changed.write_bytes(f'other/assignment2.sql\0{submission}\0'.encode()) + output = directory / 'result.json' + result = subprocess.run([sys.executable, '.github/scripts/grade_assignment1.py', + str(changed), str(output)], check=False) + self.assertEqual(result.returncode, 0) + report = json.loads(output.read_text())[0] + self.assertEqual(report['file'], str(submission)) + self.assertEqual(report['results'][0]['status'], 'executed') + + +if __name__ == '__main__': + unittest.main()