From 6032a02689ec4c98ea58e55e99006c079845dc68 Mon Sep 17 00:00:00 2001 From: Dana Burks Date: Fri, 24 Jul 2026 14:34:18 -0700 Subject: [PATCH] [bench] tests: drop unused original_cwd in cross-drive test F841. test_cross_drive_valueerror_normalizes_to_cwd captured os.getcwd() but never changed directory, so nothing needed restoring. Copy-paste residue from the test directly above it, which does chdir into a tmpdir and legitimately restores in its finally block. Checked that neighbour before deleting: a captured cwd with no restore call can mean a missing restore rather than a dead variable, and that would have been a real test-pollution bug. Here it is genuinely dead. The two remaining uses of original_cwd in this file are the tests that actually chdir. ruff now passes clean across the whole codebase. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013QFU7kbe53G7BPUZHprb9G --- ledger/bench-ledger.json | 192 +++++++++++++++++++++++++++++++++++++++ ledger/ledger-meta.json | 6 +- tests/test_hook.py | 1 - 3 files changed, 195 insertions(+), 4 deletions(-) diff --git a/ledger/bench-ledger.json b/ledger/bench-ledger.json index f41ff44..a6a9fe3 100644 --- a/ledger/bench-ledger.json +++ b/ledger/bench-ledger.json @@ -4128,5 +4128,197 @@ } }, "entry_hash": "61ff32084cf8da68e530749a43c9508c7fb54e9fe19bd5e62779a09b5b55184e" + }, + { + "entry_id": "ddbbcb51-67c0-4148-9750-5b3a6b84709e", + "timestamp": "2026-07-24T21:33:33.478293+00:00", + "previous_hash": "61ff32084cf8da68e530749a43c9508c7fb54e9fe19bd5e62779a09b5b55184e", + "constitution_hash": "8fe258d5a023a0ecc81f1cc6374156c8670d754529953b2b851e9ada53c60a75", + "verdict": "PASS", + "pipeline_error": false, + "change": { + "file": "tests\\test_hook.py", + "tool": "Edit", + "diff_summary": { + "file_path": "tests\\test_hook.py", + "change_type": "modify", + "old_string": " original = _hook_module._build_diff_info_hardened\n original_cwd: str = os.getcwd()\n try:\n _hook_module._build_diff_info_hardened = None\n original_relpath = os.path.relpath", + "new_string": " original = _hook_module._build_diff_info_hardened\n try:\n _hook_module._build_diff_info_hardened = None\n original_relpath = os.path.relpath" + } + }, + "challenger": { + "status": "FINDINGS", + "findings": [ + { + "constraint_id": "C-005", + "severity": "OBSERVATION", + "location": "tests/test_hook.py (removed `original_cwd: str = os.getcwd()`)", + "evidence": "original_cwd: str = os.getcwd()\n original_relpath = os.path.relpath (line removed)", + "reasoning": "The diff removes a saved-cwd variable typically used to restore process working directory in a finally/cleanup block after a test that patches os.path.relpath and _build_diff_info_hardened. Only the declaration line is shown, not the full function body, so it cannot be confirmed here whether `original_cwd` is referenced elsewhere (e.g., `os.chdir(original_cwd)` in teardown). If it is still referenced downstream, this edit would break the test with a NameError; if it was genuinely unused (a lint-driven cleanup, consistent with removing unused variables that fail CI), the change is benign. Full file context is needed to rule this out with certainty." + } + ], + "_tokens": { + "input": 10036, + "output": 1976 + } + }, + "defender": { + "status": "REBUTTAL", + "rebuttals": [ + { + "finding_index": 0, + "position": "REBUT", + "argument": "The finding is explicitly framed as an unresolved question ('cannot be confirmed... full file context is needed') rather than an identified violation, and it is filed under C-005 (Test Coverage for New Logic), which governs whether new logic has tests, not whether an edit to an existing test's setup code is safe. This diff adds no new logic and removes no test coverage; it deletes a single unused local variable declaration from within a test helper's setup block. If `original_cwd` were still referenced downstream (e.g. in a `finally: os.chdir(original_cwd)`), that would be a correctness bug, not a C-005 issue, and it would manifest immediately as a NameError the moment this test runs, which happens in CI on every PR per this project's own workflow rules. There is no plausible path where this defect ships silently.", + "evidence": "Diff scope: tests/test_hook.py, a single line removed (`original_cwd: str = os.getcwd()`) alongside the line above it being unchanged context. No other lines in the diff reference os.chdir or teardown logic. The project's own CLAUDE.md pre-ship checklist explicitly instructs removing unused variables that fail CI ('pre-ship audit must catch unused imports and unused variables that fail CI'), which is consistent with this being exactly that class of cleanup rather than an accidental break." + } + ], + "summary": "This is a minimal, test-only removal of what is almost certainly a dead variable, self-evidently verifiable by running the specific test (per project workflow rules), and the Challenger's own finding concedes it cannot establish a violation without more context, so it should not block the change.", + "_tokens": { + "input": 10430, + "output": 1415 + } + }, + "oracle": { + "verdict": "PASS", + "reasoning": "The change removes a single unused local variable declaration (`original_cwd: str = os.getcwd()`) from a test setup block in tests/test_hook.py. The Challenger raised only one finding, an OBSERVATION under C-005, and explicitly conceded it 'cannot be confirmed' whether the variable is referenced downstream without full file context. An OBSERVATION under a warning-severity constraint (C-005) does not meet the threshold for veto. The Defender correctly notes that C-005 governs test coverage for new logic, and this diff adds no new logic and removes no coverage; if `original_cwd` were still referenced (e.g. in `finally: os.chdir(original_cwd)`), the result would be an immediate NameError surfaced in CI, not a silent shipping defect. Evaluating the change against the constitution as a whole: C-007 (governance pipeline integrity) does not apply because tests/test_hook.py is a test artifact, not one of the enumerated pipeline components (challenger, defender, oracle, ledger, constitution). C-002 is satisfied because the change stays within a single stated-scope test file. No veto-severity constraint is clearly violated.", + "constraint_citations": [ + { + "constraint_id": "C-005", + "disposition": "NOT_APPLICABLE", + "note": "C-005 governs test coverage for new logic; this diff introduces no new logic and removes no coverage. The Challenger filed this as an OBSERVATION and conceded it could not establish a violation." + }, + { + "constraint_id": "C-002", + "disposition": "SATISFIED", + "note": "Change is confined to a single test file editing dead setup code; no out-of-scope files touched." + }, + { + "constraint_id": "C-007", + "disposition": "NOT_APPLICABLE", + "note": "A test file is not an enumerated governance-pipeline component. No enforcement, verification, or logging is weakened." + }, + { + "constraint_id": "C-001", + "disposition": "NOT_APPLICABLE", + "note": "No catch/except blocks are added or modified by this change." + } + ], + "advisories": [ + "Confirm via the targeted test run (per project workflow rule 13) that `original_cwd` is not referenced in any downstream finally/teardown block; if it were, the test would raise NameError. This is a correctness check, not a constitutional matter, and CI would surface it immediately." + ], + "remediation": null, + "confidence": "HIGH", + "_tokens": { + "input": 10969, + "output": 1107 + } + }, + "entry_hash": "f0f0610f9897728dc65f7088e257ef497eba10c62d776174a575c01a03b46bb9" + }, + { + "entry_id": "947037fe-fb32-44df-b9b1-e2293c4a95fe", + "timestamp": "2026-07-24T21:33:51.760177+00:00", + "previous_hash": "f0f0610f9897728dc65f7088e257ef497eba10c62d776174a575c01a03b46bb9", + "constitution_hash": "8fe258d5a023a0ecc81f1cc6374156c8670d754529953b2b851e9ada53c60a75", + "verdict": "PASS", + "pipeline_error": false, + "change": { + "file": "tests\\test_hook.py", + "tool": "Edit", + "diff_summary": { + "file_path": "tests\\test_hook.py", + "change_type": "modify", + "old_string": " original = _hook_module._build_diff_info_hardened\n original_cwd: str = os.getcwd()\n try:\n _hook_module._build_diff_info_hardened = None\n original_relpath = os.path.relpath", + "new_string": " original = _hook_module._build_diff_info_hardened\n try:\n _hook_module._build_diff_info_hardened = None\n original_relpath = os.path.relpath" + } + }, + "challenger": { + "status": "FINDINGS", + "findings": [ + { + "constraint_id": "C-005", + "severity": "CONCERN", + "location": "tests/test_hook.py (removal of `original_cwd: str = os.getcwd()`)", + "evidence": "original_cwd: str = os.getcwd() <- removed, while surrounding try/except/finally block is not shown in full", + "reasoning": "The diff removes the capture of the working directory at test setup without showing whether a corresponding restoration (e.g. `os.chdir(original_cwd)` in a finally block) elsewhere in the same test still references this variable. If any such reference remains, the test would raise a NameError; if the restoration logic was also removed elsewhere but not shown here, the test may no longer restore cwd after mutating it, risking test-isolation leakage into other tests. Without visibility into the full test body, this cannot be confirmed clean or broken from the diff alone." + } + ], + "_tokens": { + "input": 10036, + "output": 2461 + } + }, + "defender": { + "status": "REBUTTAL", + "rebuttals": [ + { + "finding_index": 0, + "position": "MITIGATE", + "argument": "The Challenger is honest that this can't be fully confirmed from the diff alone, and that caution is fair. But two things narrow the actual risk considerably. First, C-005 is a warning-severity constraint about missing test coverage for new logic; this diff removes a line, it doesn't add untested logic, so C-005 doesn't squarely fit even as a category (there's no new branch of logic here to cover). Second, look at what survives in the same old_string/new_string block: the try body goes directly from 'original = _hook_module._build_diff_info_hardened' to 'original_relpath = os.path.relpath' with no os.chdir call in between, and no chdir is shown before or after in the excerpt. That's consistent with original_cwd having been an unused capture (declared but never read), the kind of thing ruff/pyflakes flags as F841 and which this project's own pre-ship checklist requires removing before shipping. If a finally block elsewhere really does call os.chdir(original_cwd), the very next test run would raise a NameError immediately and loudly, it would not fail silently or leak cwd state quietly; that's a fail-fast failure mode, not the silent-corruption scenario C-005/C-001 exist to catch.", + "evidence": "old_string shows the try-block continuing straight to monkeypatching os.path.relpath with no interleaved os.chdir call visible; CLAUDE.md pre-ship checklist explicitly requires removing unused variables that would fail CI lint checks." + } + ], + "summary": "The change is a one-line test cleanup most consistent with removing an unused variable, and the Challenger's concern, while a legitimate call for more context, describes a hypothetical breakage (NameError) that would surface immediately rather than the silent failure C-005/C-001 are meant to guard against.", + "_tokens": { + "input": 10391, + "output": 2058 + } + }, + "oracle": { + "verdict": "PASS", + "reasoning": "The change removes a single line, `original_cwd: str = os.getcwd()`, from a test file. The only finding raised is by the Challenger under C-005 (Test Coverage for New Logic), which is a warning-severity constraint. Even if the concern were valid, C-005 cannot trigger a veto by its own declared severity. The Defender's rebuttal correctly notes that C-005 does not squarely apply: the diff removes a line rather than adding new logic or branches requiring coverage. The Defender further observes that the surrounding try-block proceeds directly from `original = _hook_module._build_diff_info_hardened` to `original_relpath = os.path.relpath` with no interleaved `os.chdir` call visible, which is consistent with `original_cwd` being an unused capture (F841-class), the exact kind of dead variable the project's pre-ship checklist requires removing. I independently evaluated the change against the full constitution: C-001 does not apply (no catch block is added or altered); C-002 is satisfied (the change is confined to the stated test file and is a narrow cleanup); C-003 does not apply (no imports or dependencies added); C-004 is satisfied (no function signature annotation is removed or weakened; a local variable annotation on dead code is not a governance type annotation on a signature); C-006 does not apply (no secrets); C-007 does not apply (test_hook.py is a test, not a pipeline/ledger/constitution file, and enforcement is not weakened); C-008 does not apply (no ledger interaction). No veto-severity constraint is clearly violated, and the Defender adequately addresses the sole warning-level concern.", + "constraint_citations": [ + { + "constraint_id": "C-001", + "disposition": "NOT_APPLICABLE", + "note": "No catch block is added, removed, or modified." + }, + { + "constraint_id": "C-002", + "disposition": "SATISFIED", + "note": "Change is confined to the single test file and is a narrow one-line cleanup." + }, + { + "constraint_id": "C-003", + "disposition": "NOT_APPLICABLE", + "note": "No new imports or dependencies introduced." + }, + { + "constraint_id": "C-004", + "disposition": "SATISFIED", + "note": "Removing an annotated local variable on dead code does not weaken any function signature annotation." + }, + { + "constraint_id": "C-005", + "disposition": "NOT_APPLICABLE", + "note": "Warning severity; the diff removes a line rather than adding new logic or branches requiring test coverage. Cannot trigger a veto regardless." + }, + { + "constraint_id": "C-006", + "disposition": "NOT_APPLICABLE", + "note": "No credentials or secrets present." + }, + { + "constraint_id": "C-007", + "disposition": "NOT_APPLICABLE", + "note": "test_hook.py is not a pipeline, ledger, or constitution file, and no enforcement is weakened." + }, + { + "constraint_id": "C-008", + "disposition": "NOT_APPLICABLE", + "note": "No ledger entry is modified, deleted, or appended." + } + ], + "advisories": [ + "Per the Challenger's honest note, the full test body is not visible in the diff. Before shipping, confirm that no `os.chdir(original_cwd)` or other reference to `original_cwd` remains in the same test's finally block. If a reference remains, the test would raise NameError on next run. This is a fail-fast, loud failure rather than a silent one, so it is an advisory, not a veto trigger." + ], + "remediation": null, + "confidence": "MODERATE", + "_tokens": { + "input": 10988, + "output": 1652 + } + }, + "entry_hash": "8c09fabf58dd1b8b7a33004f8224cfe20ee87c54242d7c1d618d4d596107e8a2" } ] \ No newline at end of file diff --git a/ledger/ledger-meta.json b/ledger/ledger-meta.json index 5e70ae1..6b3ed10 100644 --- a/ledger/ledger-meta.json +++ b/ledger/ledger-meta.json @@ -1,6 +1,6 @@ { - "entry_count": 41, - "latest_hash": "61ff32084cf8da68e530749a43c9508c7fb54e9fe19bd5e62779a09b5b55184e", + "entry_count": 43, + "latest_hash": "8c09fabf58dd1b8b7a33004f8224cfe20ee87c54242d7c1d618d4d596107e8a2", "created": "2026-07-24T20:13:33.889743+00:00", - "last_updated": "2026-07-24T21:29:23.211456+00:00" + "last_updated": "2026-07-24T21:33:51.760177+00:00" } \ No newline at end of file diff --git a/tests/test_hook.py b/tests/test_hook.py index f2adde7..6df3aa3 100644 --- a/tests/test_hook.py +++ b/tests/test_hook.py @@ -368,7 +368,6 @@ def test_cross_drive_valueerror_normalizes_to_cwd(self) -> None: import os original = _hook_module._build_diff_info_hardened - original_cwd: str = os.getcwd() try: _hook_module._build_diff_info_hardened = None original_relpath = os.path.relpath