-
Notifications
You must be signed in to change notification settings - Fork 1
review: live-enabled corpus format and ten live cases #233
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
Changes from all commits
2d746ac
30e7527
da1115c
2812679
63097f1
80da666
56bc3b4
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,5 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| Live-enabled corpus cases (live A/B eval plan, phase 1). The eval corpus gains an opt-in `live` block carrying what a REAL model run needs and the deterministic replay does not: the PR context (`prContext`, with the description treated as untrusted author text), a post-change file tree on disk next to the case (`<id>/case.json` + `<id>/tree/`, coexisting with the flat `<id>.json` layout), and labeled defect specs (`mustCatchSpecs` / `mustNotFlagSpecs`: path, line window, mechanism keyword alternates) so a live run's model-chosen finding ids can be matched to ground truth. A live case must carry the `live` tag, a cleanly-parseable diff, and every non-removed changed file in its tree; the loader enforces all of it and `loadLiveCorpus()` returns the subset. Ten cases are converted with hand-authored real diffs and trees (five smoke incidents, both clean cases, one adversarial injection whose payload lives in the diff, one golden holdout, one synthetic mutation); their recorded line anchors now point at the authored defect lines, and the deterministic suite runs them unchanged, provenance gate included. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,16 @@ | ||
| import {defineConfig} from "vitest/config"; | ||
| import {configDefaults, defineConfig} from "vitest/config"; | ||
|
|
||
| export default defineConfig({ | ||
| test: { | ||
| watch: false, | ||
| clearMocks: true, | ||
| setupFiles: ["./config/tests/setup.ts"], | ||
| // Live eval-corpus trees are case fixtures, not suite code: a tree | ||
| // may legitimately carry a *.test.ts whose tests fail by design | ||
| // (the test-adequacy cases), so vitest must never execute them. | ||
| exclude: [ | ||
| ...configDefaults.exclude, | ||
| "workflows/review/eval/corpus/**/tree/**", | ||
| ], | ||
| }, | ||
| }); |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "id": "clean-typed-refactor", | ||
| "tags": [ | ||
| "clean", | ||
| "live" | ||
| ], | ||
| "category": "clean", | ||
| "description": "Known-clean PR: a mechanical rename of a helper across a few files with no behaviour change. No lens produces a finding; the run must approve without posting.", | ||
| "changedFiles": [ | ||
| { | ||
| "path": "src/util/format.ts", | ||
| "status": "modified" | ||
| }, | ||
| { | ||
| "path": "src/util/format.test.ts", | ||
| "status": "modified" | ||
| } | ||
| ], | ||
| "findings": [], | ||
| "expected": { | ||
| "verdict": "APPROVE", | ||
| "postedCommentCount": 0 | ||
| }, | ||
| "diff": "diff --git a/src/util/format.ts b/src/util/format.ts\n--- a/src/util/format.ts\n+++ b/src/util/format.ts\n@@ -1,7 +1,7 @@\n /** Shared display formatting helpers. */\n \n /** Format a fractional amount as a fixed two-decimal string. */\n-export const fmt = (amount: number): string => amount.toFixed(2);\n+export const formatAmount = (amount: number): string => amount.toFixed(2);\n \n /** Format a byte count using binary units. */\n export const formatBytes = (bytes: number): string => {\ndiff --git a/src/util/format.test.ts b/src/util/format.test.ts\n--- a/src/util/format.test.ts\n+++ b/src/util/format.test.ts\n@@ -1,11 +1,11 @@\n import {describe, expect, it} from \"vitest\";\n \n-import {fmt, formatBytes} from \"./format\";\n+import {formatAmount, formatBytes} from \"./format\";\n \n-describe(\"fmt\", () => {\n+describe(\"formatAmount\", () => {\n it(\"renders two decimals\", () => {\n- expect(fmt(3)).toBe(\"3.00\");\n- expect(fmt(3.456)).toBe(\"3.46\");\n+ expect(formatAmount(3)).toBe(\"3.00\");\n+ expect(formatAmount(3.456)).toBe(\"3.46\");\n });\n });\n \n", | ||
| "live": { | ||
|
Contributor
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. note (non-blocking): |
||
| "prContext": { | ||
| "title": "util: rename fmt to formatAmount", | ||
| "description": "Mechanical rename; fmt was too terse to grep for. No behavior change.", | ||
| "author": "dev-web", | ||
| "baseBranch": "main" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import {describe, expect, it} from "vitest"; | ||
|
|
||
| import {formatAmount, formatBytes} from "./format"; | ||
|
|
||
| describe("formatAmount", () => { | ||
| it("renders two decimals", () => { | ||
| expect(formatAmount(3)).toBe("3.00"); | ||
| expect(formatAmount(3.456)).toBe("3.46"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("formatBytes", () => { | ||
| it("picks binary units", () => { | ||
| expect(formatBytes(512)).toBe("512 B"); | ||
| expect(formatBytes(2048)).toBe("2.0 KiB"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| /** Shared display formatting helpers. */ | ||
|
|
||
| /** Format a fractional amount as a fixed two-decimal string. */ | ||
| export const formatAmount = (amount: number): string => amount.toFixed(2); | ||
|
|
||
| /** Format a byte count using binary units. */ | ||
| export const formatBytes = (bytes: number): string => { | ||
| if (bytes < 1024) { | ||
| return `${bytes} B`; | ||
| } | ||
| const kib = bytes / 1024; | ||
| return kib < 1024 ? `${kib.toFixed(1)} KiB` : `${(kib / 1024).toFixed(1)} MiB`; | ||
| }; |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| { | ||
| "id": "golden-request-changes-authz", | ||
| "tags": [ | ||
| "golden", | ||
| "security-auth", | ||
| "holdout", | ||
| "live" | ||
| ], | ||
| "category": "golden", | ||
| "description": "Golden HOLDOUT case from a merged webapp PR that was later reverted: a human flagged a missing authorization check on a new admin endpoint as blocking, and the PR was changed. Ground truth = that blocking comment.", | ||
|
Contributor
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. question (non-blocking): This case self-describes as "from a merged webapp PR" but the PR description says all ten diffs/trees are hand-authored, and the tree is a generic synthetic Flask blueprint. If golden cases derive their weight from being real reviewed changes, should the reconstructed live half be tagged distinctly (e.g. "reconstructed") so live A/B reports don't present synthetic content as human-validated holdout? |
||
| "changedFiles": [ | ||
| { | ||
| "path": "src/api/admin_routes.py", | ||
| "status": "added" | ||
| } | ||
| ], | ||
| "findings": [ | ||
| { | ||
| "source": "security-auth", | ||
| "finding": { | ||
| "schema_version": 2, | ||
| "id": "golden-authz-missing", | ||
| "lens": "security-auth", | ||
| "anchor": { | ||
| "type": "line", | ||
| "path": "src/api/admin_routes.py", | ||
| "line": 20, | ||
| "side": "RIGHT" | ||
| }, | ||
| "severity": "blocking", | ||
| "confidence": 0.9, | ||
| "evidence_trace": [ | ||
| "src/api/admin_routes.py:18 defines POST /admin/reset with no @requires_admin decorator", | ||
|
Contributor
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. note (non-blocking): These converted cases re-anchor each recorded finding to the new |
||
| "every other handler in this module carries @requires_admin", | ||
| "the human reviewer flagged this exact line and the PR added the decorator before merge" | ||
| ], | ||
| "failure_scenario": "A logged-in non-admin calls this endpoint and the handler runs, exposing the admin action to any account.", | ||
| "producing_hunt": "security-auth:missing-authz-decorator", | ||
| "model_authored_prose": "This new admin endpoint has no authorization decorator, unlike its siblings in this module. Add @requires_admin so a non-admin cannot invoke it." | ||
| } | ||
| } | ||
| ], | ||
| "expected": { | ||
| "verdict": "REQUEST_CHANGES", | ||
| "postedCommentCount": 1, | ||
| "mustCatch": [ | ||
| "golden-authz-missing" | ||
| ] | ||
| }, | ||
| "diff": "diff --git a/src/api/admin_routes.py b/src/api/admin_routes.py\nnew file mode 100644\n--- /dev/null\n+++ b/src/api/admin_routes.py\n@@ -0,0 +1,24 @@\n+\"\"\"Admin maintenance routes.\n+\n+Every route in this blueprint is mounted under /api/admin by create_app().\n+\"\"\"\n+from flask import Blueprint, jsonify\n+\n+from app.auth.decorators import require_admin\n+from app.models.users import delete_user_content, lookup_user\n+\n+admin_bp = Blueprint(\"admin\", __name__)\n+\n+\n+@admin_bp.get(\"/users/<user_id>\")\n+@require_admin\n+def get_user(user_id):\n+ \"\"\"Inspect a user record (support tooling).\"\"\"\n+ return jsonify(lookup_user(user_id))\n+\n+\n+@admin_bp.post(\"/users/<user_id>/purge\")\n+def purge_user_content(user_id):\n+ \"\"\"Hard-delete a user's content (GDPR erasure requests).\"\"\"\n+ delete_user_content(user_id)\n+ return jsonify({\"status\": \"purged\", \"user_id\": user_id})\n", | ||
| "live": { | ||
| "prContext": { | ||
| "title": "admin: add user purge endpoint for GDPR erasure", | ||
| "description": "Support needs a button to hard-delete user content on verified erasure requests.", | ||
| "author": "dev-trust", | ||
| "baseBranch": "main" | ||
| }, | ||
| "mustCatchSpecs": [ | ||
| { | ||
| "key": "golden-authz-missing", | ||
| "path": "src/api/admin_routes.py", | ||
| "mechanism": [ | ||
| "require_admin", | ||
| "authoriz|permission", | ||
| "any (logged.in )?user|unauthenticated|without.*admin" | ||
| ], | ||
| "lens": "security-auth", | ||
| "lineStart": 15, | ||
| "lineEnd": 25 | ||
| } | ||
| ] | ||
| }, | ||
| "routerConfig": { | ||
| "lensRules": [ | ||
| { | ||
| "pattern": "src/api/admin_routes.py", | ||
| "lenses": [ | ||
| "security-auth" | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| """Admin maintenance routes. | ||
|
|
||
| Every route in this blueprint is mounted under /api/admin by create_app(). | ||
| """ | ||
| from flask import Blueprint, jsonify | ||
|
|
||
| from app.auth.decorators import require_admin | ||
| from app.models.users import delete_user_content, lookup_user | ||
|
Comment on lines
+7
to
+8
Member
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. Will the agent try to read these files for more context and then fail?
Contributor
Author
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. It will often try: the sub-agent's cwd is the staged copy of this tree, so reading a missing import returns an ordinary not-found tool error the agent works around (no network, no crash). The cost of a missing file is realism rather than failure, so case authors include the context files whose absence would change the reviewer's conclusion; in this case the load-bearing evidence is the decorated sibling handler in the same file. The smoke cases that ran through acceptance and the re-trigger wave all have one- or two-file trees like this one. Documented on the |
||
|
|
||
| admin_bp = Blueprint("admin", __name__) | ||
|
|
||
|
|
||
| @admin_bp.get("/users/<user_id>") | ||
| @require_admin | ||
| def get_user(user_id): | ||
| """Inspect a user record (support tooling).""" | ||
| return jsonify(lookup_user(user_id)) | ||
|
|
||
|
|
||
| @admin_bp.post("/users/<user_id>/purge") | ||
| def purge_user_content(user_id): | ||
| """Hard-delete a user's content (GDPR erasure requests).""" | ||
| delete_user_content(user_id) | ||
| return jsonify({"status": "purged", "user_id": user_id}) | ||
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.
note (non-blocking):
live.treeis a per-case field defaulting totree, but this ignore (and the vitest exclude) hard-code the literaltree/segment. They agree only because all ten cases use the default; a case with a non-defaultlive.treewould escape both globs, so prettier would reformat the fixture and vitest would execute its by-design-failing test — the exact desync these excludes exist to prevent. Consider rejecting a non-default tree name inparseLiveuntil the globs are generalized.