diff --git a/benchmarks/guardian/experiments/407-no-checker-appeal/README.md b/benchmarks/guardian/experiments/407-no-checker-appeal/README.md new file mode 100644 index 00000000..801b6cb0 --- /dev/null +++ b/benchmarks/guardian/experiments/407-no-checker-appeal/README.md @@ -0,0 +1,106 @@ +# Telling the skeptic that no checker ran (#407) + +Measuring #401 needed a control arm: the same 24 findings judged with **no checker output at +all**. In it, 6 of 24 rationales cited `mypy` or `ruff` anyway — every one to *confirm* a +false claim, on a conditional the model could not check: + +> The use of `Any` does indeed bypass strict type checking in mypy … and **if** `mypy +> --strict` is configured to disallow `Any` explicitly, it would be flagged. + +`_evidence_section(None)` returned the empty string, so the skeptic was told nothing about +checkers and supplied its own. Two prompts were tried against that. + +## Method + +The replay control arm is exactly this configuration, so each attempt costs 24 skeptic calls +and no finder call. Same recording (`../401-evidence/pr-399-recording.json`), same absence of +evidence, one variable: the prompt. + +Registered before running: **primary** — how many of the six checker-appealing findings stop +being confirmed on that basis; **recall guard** — finding 4, the one substantive finding of +the 24, must stay confirmed; **noise floor** — two draws of the original prompt moved 4 and 1 +verdicts out of 24, so anything smaller than that is not a result. + +## Attempt 1 — a prohibition. Failed. + +> Do not rest a verdict on what a checker would report — not to confirm a finding … and not +> to refute one … + +| | original | prohibition | +|---|---:|---:| +| rationales citing a checker | 6/24 | 5/24 | +| the six, still confirmed | 6 | 5 | +| finding 4 | confirmed | confirmed | + +Inside the noise floor, and the text got **worse**. Finding 0's conditional became a flat +assertion: + +> This is an issue that would be caught by the project's **mandatory `mypy --strict` gate**. + +There is no such gate over that file: `make type-check` runs `mypy src`, and `pyproject.toml` +excludes `scripts`. Told what not to do, the model complied in form and confirmed anyway — +with more confidence than before. + +## Attempt 2 — a directive. Worked. + +> If a finding rests entirely on what a checker reports … then it cannot be settled here. +> Mark it 'uncertain'. Not 'confirmed': you have not seen a checker agree. Not 'refuted': you +> have not seen one disagree either. + +| finding | original control | directive | +|---|---|---| +| 0, 2, 3, 5, 15 — claim a mypy rule | confirmed | **uncertain** | +| 12 — `sys.path` "violates type safety" | confirmed | confirmed | +| **4 — the one true finding** | confirmed | **confirmed** ✅ | +| 21, 22 — propose relaxing a ratchet | confirmed | **refuted** | + +Five of six moved, all in the same direction, all on cited-checker rows: outside the 1–4 +noise floor and mechanistically legible. Finding 0 now states its position instead of +inventing a gate: + +> Without access to the project's `mypy` configuration or output, it is not possible to +> confirm if this specific usage would be flagged. + +The recall guard held, with the correct reasoning — `CalledProcessError` is raised and not +caught, which is the defect that was really fixed in #399. + +### An unregistered gain + +Findings 21 and 22 were refuted. Those are the two the RFC singled out as **beyond any +tool**: they proposed relaxing a `>= 72` ratchet floor and an `ambiguous_hits` invariant, and +were wrong about *intent* rather than about behaviour. The model got there on its own: + +> The assertion is a deliberate guardrail … its failure due to a diminished corpus is +> **intended behavior, not a defect**. + +Not predicted, not registered, one draw — recorded as an observation, not a claim. + +## What this does not buy + +**`uncertain` is kept.** `apply_judgements` multiplies confidence by 0.9 as a ranking signal +and nothing more, and `render_review_body` takes `threshold: int = 0`. So all five findings +are still posted to the pull request; only their ordering changes. The verdict is now +truthful — the reader's experience is very nearly the same. + +That makes this an epistemic fix, not a noise fix. Suppressing unadjudicable findings is a +separate knob (the `impact_threshold` that #246 §3.5 records as shipping inert), and it +should be decided on its own evidence rather than folded in here. + +**One draw per arm, and a recall denominator of one.** The primary effect is well outside the +noise floor; nothing else here is. + +**`skeptic.py` is inside the review-fingerprint closure** (43 modules), so this re-mints every +reviewer identity — by design (#375). + +## Reproducing + +From the repository root — the paths are root-relative, because the command above them is: + +```bash +scripts/guardian_replay_skeptic.py \ + --recording benchmarks/guardian/experiments/401-evidence/pr-399-recording.json \ + --control --out rows.jsonl +``` + +With `GUARDIAN_SKEPTIC=gemini`, `GUARDIAN_SKEPTIC_MODEL=gemini-2.5-flash`. 24 skeptic calls, +no finder calls. diff --git a/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-directive-rows.jsonl b/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-directive-rows.jsonl new file mode 100644 index 00000000..4cf3f433 --- /dev/null +++ b/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-directive-rows.jsonl @@ -0,0 +1,24 @@ +{"index": 0, "file": "scripts/recordings_from_corpus.py", "line": 38, "title": "Use of `Any` in type annotations violates strict mypy mode", "was": "confirmed", "now": "uncertain", "cites_a_checker": true, "rationale": "The claim states that the use of `Any` violates strict mypy mode. Without access to the project's `mypy` configuration or output, it is not possible to confirm if this specific usage would be flagged as a violation by the existing tooling."} +{"index": 1, "file": "scripts/recordings_from_corpus.py", "line": 41, "title": "Unsafe YAML loading without `safe_load` or `Loader` specification", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The current code correctly uses `yaml.safe_load`. However, the import `import yaml` does leave `yaml.load` accessible, which is an unsafe function if used without a specific Loader. The claim highlights a plausible risk for future maintainers accidentally using the unsafe `load` function instead of `safe_load`."} +{"index": 2, "file": "scripts/recordings_from_corpus.py", "line": 50, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "uncertain", "cites_a_checker": true, "rationale": "The code uses `dict[str, Any]` as quoted. Whether this specific usage violates a 'strict mypy rule' as claimed cannot be confirmed without access to the project's mypy configuration and checker output. If `mypy --strict` were configured to disallow `Any` in this context, it would be caught by the existing mandatory tooling, resulting in a low impact."} +{"index": 3, "file": "scripts/recordings_from_corpus.py", "line": 56, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "uncertain", "cites_a_checker": true, "rationale": "The quoted code is present in the diff, but confirming whether `dict[str, Any]` violates the project's strict mypy rules requires checker output, which is not available for this review."} +{"index": 4, "file": "scripts/recordings_from_corpus.py", "line": 62, "title": "Empty diff raises `MissingFixtureError` but may not be the only failure mode", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `subprocess.run(check=True)` call can raise a `CalledProcessError` if `git diff` fails for reasons other than producing an empty diff, and this specific exception is not caught, leading to a crash with a generic error instead of a more specific `MissingFixtureError` or handled failure."} +{"index": 5, "file": "scripts/recordings_from_corpus.py", "line": 80, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "uncertain", "cites_a_checker": true, "rationale": "The code uses `dict[str, Any]` as quoted. However, whether this *violates* a 'strict mypy rule' depends on the project's specific mypy configuration, which cannot be verified without checker output. If it were a violation, it would be caught by tooling."} +{"index": 6, "file": "scripts/recordings_from_corpus.py", "line": 89, "title": "Potential type mismatch in `Finding.model_validate`", "was": "confirmed", "now": "refuted", "cites_a_checker": false, "rationale": "The `Finding.model_validate` method is specifically designed to perform schema validation and will raise a `ValidationError` if the input dictionary does not match the `Finding`'s expected schema, which is the correct and robust behavior for handling potentially untrusted data. The dictionary comprehension explicitly removes known annotation keys (`_ANNOTATIONS`) that are not part of the `Finding` model, thereby preventing validation errors for these specific fields and ensuring that only relevant keys are passed for validation. There is no silent data dropping unless the `Finding` model is configured with `extra='ignore'`, which is not indicated here; Pydantic's default is `extra='forbid'`, causing errors for unexpected keys."} +{"index": 7, "file": "scripts/recordings_from_corpus.py", "line": 95, "title": "No validation that `results` file exists or is readable", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The call to `results.read_text()` will raise a `FileNotFoundError` or `PermissionError` if the file does not exist or is not readable, preventing silent failure. If the file is empty, the function correctly processes no rows and reports '0 recordings written', which is an accurate and non-silent outcome."} +{"index": 8, "file": "scripts/recordings_from_corpus.py", "line": 100, "title": "No handling for case where no frozen passes are found", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The script already prints \"0 recordings written to {args.out}\" when no frozen passes are found. This explicit output informs the caller, mitigating any potential \"unexpected behavior\" and sufficiently handling the case without needing an additional warning or early return."} +{"index": 9, "file": "scripts/recordings_from_corpus.py", "line": 102, "title": "Race condition in directory creation", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The `exist_ok=True` argument in `Path.mkdir()` is specifically designed to handle concurrent directory creation attempts, making the operation idempotent and race-safe without raising a `FileExistsError`."} +{"index": 10, "file": "scripts/recordings_from_corpus.py", "line": 110, "title": "No validation that `save_finder_recording` succeeds", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The call to save_finder_recording is not wrapped in a try-except block, so file system errors (e.g., permissions, disk full) would cause the script to crash or fail to write the file without handling, potentially leaving an incomplete set of recordings and reporting success where there was none."} +{"index": 11, "file": "scripts/recordings_from_corpus.py", "line": 117, "title": "No error handling for `build` failures", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `build` function can explicitly raise `MissingFixtureError` or `NotAFrozenPassError` with descriptive messages; however, the `main` function does not catch these expected exceptions to present a user-friendly error message, instead allowing a raw Python traceback to be displayed, which is a less graceful exit for a command-line tool."} +{"index": 12, "file": "tests/unit/test_recordings_from_corpus.py", "line": 12, "title": "Modification of `sys.path` at runtime violates type safety", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "Dynamically modifying sys.path at runtime, especially with `insert(0)`, can lead to less predictable module resolution and violates the principle of explicit imports, making the project structure harder to reason about for both humans and static analysis tools. While it doesn't directly violate Python's fundamental type system, it can complicate static type checking."} +{"index": 13, "file": "tests/unit/test_recordings_from_corpus.py", "line": 15, "title": "Import from a script file may fail in some environments", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The code explicitly adds the directory containing `recordings_from_corpus.py` to `sys.path` on line 13, directly handling the concern that 'the file is not in `sys.path`'. Python environments generally support direct imports from `.py` files, making the second part of the claim's rationale incorrect. While importing from a script that isn't a formal module can sometimes have other subtle issues, the specific reasons for failure cited in the claim are addressed or incorrect by the provided diff."} +{"index": 14, "file": "tests/unit/test_recordings_from_corpus.py", "line": 20, "title": "Hardcoded path traversal may break in some environments", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The code explicitly assumes a fixed directory depth (three levels up from `__file__`), making `REPO_ROOT` fragile to changes in the test file's location or the project's overall structure, which could cause test failures."} +{"index": 15, "file": "tests/unit/test_recordings_from_corpus.py", "line": 28, "title": "Function return type uses `Any` in type annotation", "was": "confirmed", "now": "uncertain", "cites_a_checker": true, "rationale": "The `Any` type is present as claimed in the diff. However, whether this violates a project-specific strict mypy rule or mypy --strict cannot be verified without running the checker. If it does violate such a rule, it would be caught by mandatory project tooling, leading to a low impact score."} +{"index": 16, "file": "tests/unit/test_recordings_from_corpus.py", "line": 30, "title": "No validation that `RESULTS` file exists or is readable", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The code does not explicitly validate that the RESULTS file exists or is readable, and as stated, will raise a FileNotFoundError if it's missing or unreadable. While this behavior for a test helper indicates a missing fixture, it is a factual lack of explicit validation."} +{"index": 17, "file": "tests/unit/test_recordings_from_corpus.py", "line": 35, "title": "Return type uses `Path` but may include non-Path values", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The reviewer's own description confirms that the return type annotation `dict[str, Path]` correctly reflects that the function returns a dictionary where values are `Path` objects, stating \"This is correct\". The claim then concludes \"No action needed; the type is correct. This is a false positive.\", effectively refuting the initial suggestion of a problem."} +{"index": 18, "file": "tests/unit/test_recordings_from_corpus.py", "line": 50, "title": "Test assumes existence of a judged row in the corpus", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `next()` call without a default argument will raise `StopIteration` if no row in the corpus satisfies the condition, making the test fragile if the corpus changes."} +{"index": 19, "file": "tests/unit/test_recordings_from_corpus.py", "line": 57, "title": "Test assumes existence of an unscored row in the corpus", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The quoted code is present in the diff. The `next()` function will raise a `StopIteration` error if the generator expression yields no elements, meaning the test is brittle and relies on the implicit existence of a specific type of row in the external corpus."} +{"index": 20, "file": "tests/unit/test_recordings_from_corpus.py", "line": 64, "title": "Test uses a hardcoded PR number (9999) that may not be meaningful", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The test correctly identifies a potential future flakiness where a hardcoded PR number (9999) could eventually clash with a real PR, causing the test to fail if a ground-truth fixture for that PR is created."} +{"index": 21, "file": "tests/unit/test_recordings_from_corpus.py", "line": 75, "title": "Test assumes at least 72 frozen passes exist in the corpus", "was": "confirmed", "now": "refuted", "cites_a_checker": false, "rationale": "The assertion is a deliberate guardrail to ensure the test operates on a sufficiently large and representative corpus, as indicated by the accompanying comment, and its failure due to a diminished corpus is intended behavior, not a defect."} +{"index": 22, "file": "tests/unit/test_recordings_from_corpus.py", "line": 100, "title": "Test assumes existence of `ambiguous_hits` in some rows", "was": "confirmed", "now": "refuted", "cites_a_checker": false, "rationale": "The assertion explicitly verifies a specific historical invariant: that the rows requiring the pre-#345 precision rule are exactly those carrying 'ambiguous_hits'. The test is designed to fail if this invariant no longer holds, which is its correct and intended behavior to flag a change in the corpus's historical properties. The claim misinterprets the test's role in enforcing this invariant as a defect."} +{"index": 23, "file": "tests/unit/test_recordings_from_corpus.py", "line": 145, "title": "Floating-point comparison may be too strict for some use cases", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The use of an absolute tolerance for floating-point comparisons, while potentially sufficient for the limited range of recall and precision values (0 to 1), is generally less robust than using `math.isclose` with relative and absolute tolerances. This is a common best practice to prevent subtle test failures due to floating-point arithmetic variations across different environments or complex calculations, and thus constitutes a plausible, albeit minor, robustness improvement."} diff --git a/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-prohibition-rows.jsonl b/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-prohibition-rows.jsonl new file mode 100644 index 00000000..84c17baa --- /dev/null +++ b/benchmarks/guardian/experiments/407-no-checker-appeal/pr-399-control-prohibition-rows.jsonl @@ -0,0 +1,24 @@ +{"index": 0, "file": "scripts/recordings_from_corpus.py", "line": 38, "title": "Use of `Any` in type annotations violates strict mypy mode", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "The code explicitly imports and uses `Any` in type annotations for dictionary values, which is commonly flagged by `mypy --strict`. While `Any` is a common choice for deserialized JSON, a more precise type or a Pydantic model could be used. This is an issue that would be caught by the project's mandatory `mypy --strict` gate."} +{"index": 1, "file": "scripts/recordings_from_corpus.py", "line": 41, "title": "Unsafe YAML loading without `safe_load` or `Loader` specification", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The current code correctly uses `yaml.safe_load`. However, the import statement does not prevent future code from accidentally using the unsafe `yaml.load` function, making the concern about potential future misuse plausible."} +{"index": 2, "file": "scripts/recordings_from_corpus.py", "line": 50, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "The `row` parameter is indeed typed as `dict[str, Any]` in the diff. Using `Any` is less precise than a `TypedDict` or explicit type and would typically be flagged by strict `mypy` configurations, thus confirming the claim that it violates such a rule if one exists in the project."} +{"index": 3, "file": "scripts/recordings_from_corpus.py", "line": 56, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "The `row` parameter is indeed typed as `dict[str, Any]`, which is less precise than alternative annotations and often discouraged by strict type checking configurations."} +{"index": 4, "file": "scripts/recordings_from_corpus.py", "line": 62, "title": "Empty diff raises `MissingFixtureError` but may not be the only failure mode", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `subprocess.run(..., check=True)` call for `git diff` will raise a `CalledProcessError` if `git diff` itself fails for any reason (e.g., a git internal error, or unexpected arguments, even if the SHAs resolve). This `CalledProcessError` is not explicitly caught and re-wrapped into a `MissingFixtureError` as other `diff_for` failures are. The existing `MissingFixtureError` at line 62 only handles the case where `git diff` *succeeds* but produces an empty output."} +{"index": 5, "file": "scripts/recordings_from_corpus.py", "line": 80, "title": "Function parameter uses `Any` in type annotation", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "The `row` parameter is explicitly typed as `dict[str, Any]`, which correctly identifies a weakening of type precision that could be improved by using `TypedDict` or a more specific type for strict type checking, though it does not represent an immediate runtime defect."} +{"index": 6, "file": "scripts/recordings_from_corpus.py", "line": 89, "title": "Potential type mismatch in `Finding.model_validate`", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The code explicitly strips known annotation fields as intended, and `Finding.model_validate` will correctly raise a `ValidationError` for type mismatches in declared fields. However, if the `Finding` model's Pydantic configuration (`Config.extra`) is set to `ignore` (the default for BaseModel in Pydantic v2), then any *other* undeclared keys present in `f` (not in `_ANNOTATIONS` and not in `Finding`'s schema) would be silently dropped. This behavior, while configurable, could be considered a minor robustness issue if all input fields are expected to be either consumed or explicitly rejected."} +{"index": 7, "file": "scripts/recordings_from_corpus.py", "line": 95, "title": "No validation that `results` file exists or is readable", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The claim states the function will silently proceed if the 'results' file does not exist or is unreadable. This is incorrect. If the `results` file does not exist or is not readable, `results.read_text()` will raise a `FileNotFoundError` or other `IOError`, which is not silent progression. For an empty but existing file, the function correctly processes zero rows and prints '0 recordings written', which is a valid outcome."} +{"index": 8, "file": "scripts/recordings_from_corpus.py", "line": 100, "title": "No handling for case where no frozen passes are found", "was": "refuted", "now": "confirmed", "cites_a_checker": false, "rationale": "The observation that the function will write no recordings if 'frozen' is empty is correct. However, the script explicitly prints '0 recordings written to [path]', which clearly communicates the outcome, making it less 'unexpected' than claimed. Adding a warning would be an improvement in explicitness rather than a fix for a defect causing incorrect behavior."} +{"index": 9, "file": "scripts/recordings_from_corpus.py", "line": 102, "title": "Race condition in directory creation", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The `exist_ok=True` parameter of `pathlib.Path.mkdir()` is specifically designed to handle concurrent directory creation by multiple processes, preventing `FileExistsError` if the directory already exists. The proposed fix (using `os.makedirs` with `exist_ok=True`) is functionally equivalent to the current robust implementation."} +{"index": 10, "file": "scripts/recordings_from_corpus.py", "line": 110, "title": "No validation that `save_finder_recording` succeeds", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The call to `save_finder_recording` is not wrapped in any error handling. If `save_finder_recording` were to fail without raising an exception (e.g., due to a partial write, or a non-exception error handling mechanism that the caller ignores), the current code would incorrectly add the path to the `written` list, leading to a path being returned for a non-existent or corrupt recording. While typical I/O errors raise exceptions, the absence of an explicit `try-except` block means the success of the operation is not validated, making the claimed defect plausible and significant."} +{"index": 11, "file": "scripts/recordings_from_corpus.py", "line": 117, "title": "No error handling for `build` failures", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `build` function can indeed raise `MissingFixtureError` or `NotAFrozenPassError`, and the `main` function does not contain a try-except block to catch these. Consequently, if such an exception occurs, the script will exit with a traceback rather than a custom, user-friendly error message, which matches the claim."} +{"index": 12, "file": "tests/unit/test_recordings_from_corpus.py", "line": 12, "title": "Modification of `sys.path` at runtime violates type safety", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The quoted code on line 12 correctly reflects the diff. Modifying `sys.path` at runtime, especially in tests, is generally discouraged as it can lead to less explicit import behavior and make module resolution less predictable or harder for static analysis tools to follow without specific configuration. The claim, while slightly vague on \"type-safety,\" points to a valid concern regarding maintainability and explicitness."} +{"index": 13, "file": "tests/unit/test_recordings_from_corpus.py", "line": 15, "title": "Import from a script file may fail in some environments", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The diff explicitly adds the 'scripts' directory, where 'recordings_from_corpus.py' is expected to reside, to `sys.path` on line 13, directly preceding the import. Importing `.py` files as modules is standard Python behavior when they are discoverable on `sys.path`."} +{"index": 14, "file": "tests/unit/test_recordings_from_corpus.py", "line": 20, "title": "Hardcoded path traversal may break in some environments", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The path calculation `Path(__file__).resolve().parent.parent.parent` is indeed relative to the current file's assumed location, making the test brittle to future refactors or changes in the project's directory structure."} +{"index": 15, "file": "tests/unit/test_recordings_from_corpus.py", "line": 28, "title": "Function return type uses `Any` in type annotation", "was": "confirmed", "now": "confirmed", "cites_a_checker": true, "rationale": "The quoted code precisely matches the diff. The return type uses `Any`, which reduces type safety and would indeed violate strict mypy rules. This is a valid observation about type precision, though it's typically enforced by tooling like `mypy --strict` as a mandatory gate in this repository, making its impact score low."} +{"index": 16, "file": "tests/unit/test_recordings_from_corpus.py", "line": 30, "title": "No validation that `RESULTS` file exists or is readable", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The claim is factually correct: the `_rows()` function does not explicitly validate that `RESULTS` exists or is readable, and `Path.read_text()` will indeed raise `FileNotFoundError` or `PermissionError` if the file is missing or unreadable, respectively. While this occurs in a test file and `FileNotFoundError` is an acceptable way to signal a missing test dependency, the statement about the code's behavior and its potential to raise an error is accurate."} +{"index": 17, "file": "tests/unit/test_recordings_from_corpus.py", "line": 35, "title": "Return type uses `Path` but may include non-Path values", "was": "refuted", "now": "refuted", "cites_a_checker": false, "rationale": "The claim identifies the return type `dict[str, Path]` and immediately states that the function correctly returns a dictionary where values are `Path` objects, concluding that 'No action needed; the type is correct. This is a false positive.' As such, the finding is a false positive and refutes itself."} +{"index": 18, "file": "tests/unit/test_recordings_from_corpus.py", "line": 50, "title": "Test assumes existence of a judged row in the corpus", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `next()` call without a default argument will indeed raise a `StopIteration` error if no row in the corpus matches the specified criteria, making the test brittle to corpus changes. This is a valid defect in test robustness."} +{"index": 19, "file": "tests/unit/test_recordings_from_corpus.py", "line": 57, "title": "Test assumes existence of an unscored row in the corpus", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The `next()` call without a default value will raise `StopIteration` if no matching row is found in the corpus, making the test brittle to changes in its external data dependency (`results.jsonl`). While the test's purpose requires such a row, the current implementation will crash rather than gracefully indicate missing test data."} +{"index": 20, "file": "tests/unit/test_recordings_from_corpus.py", "line": 64, "title": "Test uses a hardcoded PR number (9999) that may not be meaningful", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The test correctly relies on a non-existent PR number to trigger a MissingFixtureError. However, the chosen number 9999 is not guaranteed to remain non-existent indefinitely, potentially leading to test failures if a PR with that number and an associated fixture is created in the future."} +{"index": 21, "file": "tests/unit/test_recordings_from_corpus.py", "line": 75, "title": "Test assumes at least 72 frozen passes exist in the corpus", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The assertion `assert len(expected) >= 72` at line 75 hardcodes a minimum expectation for the number of frozen passes in the corpus, making the test brittle to corpus changes that reduce this count."} +{"index": 22, "file": "tests/unit/test_recordings_from_corpus.py", "line": 100, "title": "Test assumes existence of `ambiguous_hits` in some rows", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The claim correctly identifies that the test makes an explicit assertion linking the need for a specific historical scoring policy to the presence of `ambiguous_hits` in the corpus data. If the corpus evolves such that this correlation no longer holds, the test is designed to fail, signaling that the conditions for applying the old policy have changed. While the test's author might intend this as an invariant check, the reviewer correctly points out that this makes the test brittle to changes in the corpus or the interpretation of how the old policy applies."} +{"index": 23, "file": "tests/unit/test_recordings_from_corpus.py", "line": 145, "title": "Floating-point comparison may be too strict for some use cases", "was": "confirmed", "now": "confirmed", "cites_a_checker": false, "rationale": "The claim correctly identifies that using a fixed absolute tolerance for floating-point comparisons can be problematic due to accumulated errors, even for values within a [0, 1] range like recall and precision. While 1e-9 is a common choice, `math.isclose` with both relative and absolute tolerances is generally a more robust and Pythonic approach to prevent potential false negatives in tests, especially as numerical calculations can introduce subtle differences that exceed a strict absolute threshold."} diff --git a/src/cgis/guardian/skeptic.py b/src/cgis/guardian/skeptic.py index 6cb60f42..c4fd75fb 100644 --- a/src/cgis/guardian/skeptic.py +++ b/src/cgis/guardian/skeptic.py @@ -108,11 +108,55 @@ def build_judgement_prompt(finding: Finding, hunks: str, evidence: Evidence | No {{"verdict": "confirmed|refuted|uncertain", "impact_score": 0, "rationale": "one sentence"}}""" -def _evidence_section(evidence: Evidence | None) -> str: - """The checker output, and the narrow licence it grants — or nothing at all. +#: What the skeptic is told when nothing was run (#407). +#: +#: This used to be the empty string, and the emptiness was the defect. Measuring +#: #401 needed a control arm judging the same findings with no checker output, +#: and in it **6 of 24 rationales cited mypy or ruff anyway** — every one of them +#: to *confirm* a false claim, on a conditional it could not check: "and **if** +#: `mypy --strict` is configured to disallow `Any` explicitly, it would be +#: flagged." Told nothing about checkers, the model supplied its own. +#: +#: Symmetric on purpose. Naming only the confirming direction would leave "the +#: linter would have caught it" available as a refutation, buying precision with +#: recall — the trade #246 records for a skeptic tuned too aggressively, and the +#: one #401's narrow licence exists to avoid. +#: +#: **A directive, not a prohibition, and that is the second attempt.** The first +#: version said "do not rest a verdict on what a checker would report" and was +#: measured on the same 24 findings: 5 of the 6 checker-appeals survived, and the +#: language got *more* assertive rather than less — a conditional ("if +#: `mypy --strict` is configured to disallow `Any`, it would be flagged") became +#: a flat claim about "the project's mandatory `mypy --strict` gate", a gate that +#: does not exist as described, since `make type-check` runs `mypy src` and +#: `pyproject.toml` excludes `scripts`. Told what not to do, the model complied +#: in form and confirmed anyway. So this says what verdict such a claim *takes*. +#: +#: 'uncertain' rather than 'refuted' because it is the semantically true state: +#: the checker neither agreed nor disagreed, because it never ran. Refuting an +#: unadjudicable claim is the recall regression this component is built to avoid. +#: +#: It matters most where it is always in force: `guardian_martian` reviews five +#: foreign repositories whose toolchains are not ours, so evidence is never +#: available there and this paragraph is on every finding. +_NO_EVIDENCE_SECTION = """ +### NO CHECKER OUTPUT IS AVAILABLE FOR THIS REVIEW + +No type checker or linter was run for this review, so nothing here reports what +one would say. + +If a finding rests entirely on what a checker reports — that a type checker +rejects this code, that a linter forbids this construct, that a rule enforced by +the project's tooling is broken — then it cannot be settled here. Mark it +'uncertain'. Not 'confirmed': you have not seen a checker agree. Not 'refuted': +you have not seen one disagree either. + +Judge every other finding from the hunks as usual. +""" - Empty when there is no evidence, so an unsupported repository gets exactly - the prompt it got before this existed. + +def _evidence_section(evidence: Evidence | None) -> str: + """The checker output and the narrow licence it grants — or the notice that none ran. The licence is deliberately narrow, and the narrowing is the whole safety property of #401. A clean type check disproves "mypy would reject this" and @@ -120,9 +164,13 @@ def _evidence_section(evidence: Evidence | None) -> str: findings the checkers were never asked about, turning a precision fix into a recall regression — the failure mode #246 already records for a skeptic tuned too aggressively. The sentence, not a code path, is the mechanism. + + The same reasoning decides the other branch. Saying nothing is not neutral: + it leaves the model free to assert what a checker would report, which is the + move this whole component exists to catch the finder making. """ if evidence is None: - return "" + return _NO_EVIDENCE_SECTION return f""" ### WHAT THIS REPOSITORY'S OWN CHECKERS REPORT {evidence.render()} diff --git a/tests/unit/test_guardian_skeptic.py b/tests/unit/test_guardian_skeptic.py index e02b84cb..68ad3db1 100644 --- a/tests/unit/test_guardian_skeptic.py +++ b/tests/unit/test_guardian_skeptic.py @@ -241,15 +241,61 @@ async def generate_structured( class TestEvidenceInTheJudgementPrompt: """Static checker output, and the narrow licence it grants (#401).""" - def test_no_evidence_leaves_the_prompt_as_it_was(self) -> None: - """An unsupported repository must lose nothing. + def test_no_evidence_says_so_instead_of_saying_nothing(self) -> None: + """The gap the model was filling itself (#407). + + This test used to assert the opposite — that an absent checker left the + prompt untouched, on the reasoning that an unsupported repository must + lose nothing. Measuring #401 showed what it gained instead: judging the + same 24 findings with no checker output, **6 rationales cited mypy or + ruff anyway**, every one to confirm a false claim on a conditional it + could not check. Silence is not neutral. + """ + prompt = build_judgement_prompt(_FINDING, "@@ -1 +1 @@\n+x", evidence=None) + assert "NO CHECKER OUTPUT IS AVAILABLE" in prompt + + def test_the_no_evidence_notice_names_the_verdict_rather_than_forbidding_one( + self, + ) -> None: + """A directive, because the prohibition was measured and did not work. + + The first version said "do not rest a verdict on what a checker would + report". On the same 24 findings, 5 of 6 checker-appeals survived and + the language grew *more* assertive: a conditional became a flat claim + about a "mandatory `mypy --strict` gate" that does not exist as + described. Told what not to do, the model complied in form and confirmed + anyway — so the prompt now says which verdict such a claim takes. + """ + prompt = build_judgement_prompt(_FINDING, "@@ -1 +1 @@\n+x", evidence=None) + # Whitespace-normalised rather than matched as written. The first draft + # accepted two spellings with an `or`, hedging against wherever the line + # wrap happened to fall — which is a test that passes because it was + # given two chances, not because it knows the answer. Raised in review + # of #410. + assert "Mark it 'uncertain'." in " ".join(prompt.split()) + + def test_the_no_evidence_notice_rules_out_both_other_verdicts(self) -> None: + """Symmetry is the recall guard, and it is a sentence rather than a code path. + + Naming only the confirming direction would leave "the linter would have + caught it" available as a refutation — buying precision with recall, the + trade #246 records for a skeptic tuned too aggressively. + """ + prompt = " ".join( + build_judgement_prompt(_FINDING, "@@ -1 +1 @@\n+x", evidence=None).split() + ) + assert "Not 'confirmed': you have not seen a checker agree." in prompt + assert "Not 'refuted': you have not seen one disagree either." in prompt + + def test_no_evidence_presents_no_checker_verdict(self) -> None: + """The notice names checkers; it must not appear to report one. - `collect_evidence` returns None for a TypeScript change, a foreign repo, - a crashed checker. In every one of those the skeptic has to behave - exactly as it did before this section existed. + A section that mentioned mypy and looked like output would be worse than + the silence it replaces — the model would have something to quote. """ prompt = build_judgement_prompt(_FINDING, "@@ -1 +1 @@\n+x", evidence=None) - assert "CHECKER" not in prompt.upper() + assert "WHAT THIS REPOSITORY'S OWN CHECKERS REPORT" not in prompt + assert "Success: no issues" not in prompt def test_evidence_appears_with_the_commands_that_produced_it(self) -> None: """The model is asked to treat this as disproof, so it must be re-runnable."""