diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c150cf..fc91169 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ All notable changes to diffly are documented here. The format follows [Keep a Ch - `diffly setup` no longer runs the update check twice in a row (once in `main()` and again inside the wizard it launches). - Pressing an arrow key in the interactive review menu no longer crashes with `NameError: name '_read_escape_sequence' is not defined`; the escape-sequence reader is defined again and a lone Escape still exits without blocking. +### Changed + +- Verdicts rebalanced around merge-readiness: pending required checks and version-bump-only manifest edits are now review notes instead of quarantine gates, so healthy pull requests pass. +- `BLOCK` is reserved for major signals only — failed required checks, or credential-like values added to production code. Credential-like values limited to tests, fixtures, or docs quarantine for confirmation instead of blocking. +- `QUARANTINE` now targets newly added dependencies; lockfile refreshes and version bumps without new packages stay visible as notes. + ## [1.0.0] - 2026-08-22 Diffly 1.0.0 is the first production-ready release of the deterministic pull-request triage workflow. It stabilizes the command-line experience, interactive review, local analysis, GitHub Action, and explanation behavior around a clear three-outcome policy: healthy pull requests pass, focused review gates quarantine, and severe failures block. diff --git a/README.md b/README.md index 445b4be..12c340e 100644 --- a/README.md +++ b/README.md @@ -154,9 +154,9 @@ Default model is `gpt-5-mini`; override with `DIFFLY_LLM_MODEL` or `--llm-model` | Verdict | Rule | | --- | --- | -| **BLOCK** | A required check failed, or the changed hunk appears to add a credential-like value. | -| **QUARANTINE** | Security-sensitive code, database schema/migrations, dependency changes, or still-pending checks need focused review. | -| **PASS** | No blocking or quarantine rule fired. Missing obvious tests and unavailable checks stay visible as review notes, but do not turn an otherwise healthy PR into `QUARANTINE`. `SHIP` remains accepted as a legacy alias. | +| **BLOCK** | A required check failed, or production code appears to add a credential-like value. | +| **QUARANTINE** | Security-sensitive code, database schema/migrations, or newly added dependencies need focused review. Credential-like values limited to tests, fixtures, or docs also quarantine rather than block. | +| **PASS** | Healthy changes, ready to merge. Pending or unavailable checks, version-bump-only manifest edits, and missing obvious tests stay visible as review notes, not gates. `SHIP` remains accepted as a legacy alias. | `PASS` is the normal healthy outcome. A verdict is a review signal, not a claim that a PR is correct or safe in every context. diff --git a/src/diffly_cli/cli.py b/src/diffly_cli/cli.py index 752a989..70f02a2 100644 --- a/src/diffly_cli/cli.py +++ b/src/diffly_cli/cli.py @@ -300,7 +300,7 @@ def render_markdown(result: TriageResult, explanation: ExplanationResult | None for file in result.files: symbols = ", ".join(file.touched_symbols) if file.touched_symbols else "—" lines.append(f"| `{file.path}` | {file.status} | {file.additions} | {file.deletions} | {symbols} |") - lines += ["", "## Deterministic policy", "", "- `BLOCK`: failed checks or a credential-like value added to a changed hunk.", "- `QUARANTINE`: security-sensitive code, database schema or migration changes, dependency changes, or still-pending checks.", "- `PASS`: no blocking or quarantine signal fired. Missing obvious tests or unavailable checks remain visible as review notes, not verdict gates.", "", "_Generated by diffly. Deterministic triage is authoritative; any literate-diff prose is optional generated explanation._"] + lines += ["", "## Deterministic policy", "", "- `BLOCK`: failed required checks, or a credential-like value added to production code.", "- `QUARANTINE`: security-sensitive code, database schema or migration changes, newly added dependencies, or credential-like values limited to tests, fixtures, or docs.", "- `PASS`: healthy changes ready to merge. Pending or unavailable checks, version-bump-only manifest edits, and missing obvious tests remain visible as review notes, not gates.", "", "_Generated by diffly. Deterministic triage is authoritative; any literate-diff prose is optional generated explanation._"] return "\n".join(lines) + "\n" diff --git a/src/diffly_cli/triage.py b/src/diffly_cli/triage.py index 99ea20e..8d35b9b 100644 --- a/src/diffly_cli/triage.py +++ b/src/diffly_cli/triage.py @@ -48,6 +48,25 @@ def _added_dependency_names(file: ChangedFile) -> list[str]: return sorted(dict.fromkeys(values)) +def _removed_dependency_names(file: ChangedFile) -> list[str]: + if file.path.rsplit("/", 1)[-1] not in DEPENDENCY_FILES: + return [] + values: list[str] = [] + for line in hunk_body_lines(file.patch): + if line.startswith("-"): + match = re.search(r"[\"']([@A-Za-z0-9_./-]+)[\"']\s*[:=]", line) + if match: + values.append(match.group(1)) + elif re.search(r"^[-]\s*[A-Za-z0-9_.-]+[=<>~]", line): + values.append(line[1:].strip().split()[0]) + return sorted(dict.fromkeys(values)) + + +def _dependency_base_name(name: str) -> str: + """Strip version constraints so a bumped pin matches its original install.""" + return re.split(r"[=<>~!;\s@\[]", name, maxsplit=1)[0].lower() + + def _test_files(files: list[ChangedFile]) -> list[str]: return [file.path for file in files if _matches(file.path, TEST_PATTERNS)] @@ -94,9 +113,11 @@ def compute_flags(metadata: PRMetadata, files: list[ChangedFile], checks: dict[s dependency_evidence: list[str] = [] for file in files: - names = _added_dependency_names(file) - if names: - dependency_evidence.append(f"{file.path}: {', '.join(names)}") + file_added = _added_dependency_names(file) + removed_bases = {_dependency_base_name(name) for name in _removed_dependency_names(file)} + new_here = [name for name in file_added if _dependency_base_name(name) not in removed_bases] + if new_here: + dependency_evidence.append(f"{file.path}: {', '.join(new_here)}") elif file.path.rsplit("/", 1)[-1] in DEPENDENCY_FILES and file.additions > 0: dependency_evidence.append(file.path) if dependency_evidence: @@ -133,20 +154,28 @@ def verdict_for(flags: list[RiskFlag], checks: dict[str, Any]) -> tuple[str, lis if "CHECKS_FAILED" in codes: reasoning.append("BLOCK because at least one status check failed.") return "BLOCK", reasoning - if "EXPOSED_SECRET" in codes: - reasoning.append("BLOCK because the pull request appears to add a credential-like value.") + secret_flag = next((flag for flag in flags if flag.code == "EXPOSED_SECRET"), None) + production_hits = [path for path in (secret_flag.evidence if secret_flag else []) if _is_production_file(path)] + if production_hits: + reasoning.append("BLOCK because a credential-like value was added to production code: " + ", ".join(f"`{path}`" for path in production_hits[:5]) + ".") return "BLOCK", reasoning if "AUTH_OR_SECRET" in codes: reasoning.append("QUARANTINE because authentication or security-sensitive code changed and needs focused review.") if "DATABASE_CHANGE" in codes: reasoning.append("QUARANTINE because database schema or migration changes require an explicit review gate.") - if "NEW_DEPENDENCY" in codes: - reasoning.append("QUARANTINE because dependency changes expand the supply-chain and runtime surface.") - if "CHECKS_PENDING" in codes: - reasoning.append("QUARANTINE because required checks are still running.") + dependency_flag = next((flag for flag in flags if flag.code == "NEW_DEPENDENCY"), None) + named_new = [evidence for evidence in (dependency_flag.evidence if dependency_flag else []) if ": " in evidence] + if named_new: + reasoning.append("QUARANTINE because newly added dependencies expand the supply-chain and runtime surface.") + if secret_flag is not None: + reasoning.append("QUARANTINE because a credential-like value appears only outside production code (tests, fixtures, or docs); confirm it is intentionally fake.") if reasoning: return "QUARANTINE", reasoning observations: list[str] = [] + if "CHECKS_PENDING" in codes: + observations.append("required checks were still running") + if "NEW_DEPENDENCY" in codes and not named_new: + observations.append("dependency manifests changed without newly added packages") if "NO_TEST_COVERAGE" in codes: observations.append("no obvious test coverage was found for one or more production files") if "CHECKS_UNKNOWN" in codes: diff --git a/tests/test_regressions.py b/tests/test_regressions.py index 19082b3..619c828 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -468,23 +468,24 @@ def test_truncated_diff_block_with_prelude_but_no_hunks_counts_nothing(): assert file.deletions == 0 -def test_secret_on_plus_prefixed_content_line_blocks_the_pr(): +def test_secret_on_plus_prefixed_content_line_is_detected(): """Regression: content beginning with ++ made the whole diff line look like - the file's +++ header, hiding it from counting and the secret scan.""" + the file's +++ header, hiding it from counting and the secret scan. The + credential must be flagged; production paths block, tests/docs quarantine.""" from diffly_cli.models import ChangedFile from diffly_cli.triage import compute_flags, verdict_for patch = ( - "diff --git a/docs/deploy.md b/docs/deploy.md\n" - "--- a/docs/deploy.md\n" - "+++ b/docs/deploy.md\n" + "diff --git a/deploy/config.py b/deploy/config.py\n" + "--- a/deploy/config.py\n" + "+++ b/deploy/config.py\n" "@@ -1,2 +1,3 @@\n" " intro\n" "+++ postgresql://admin:hunter2@db.internal.example/prod\n" ) - files = [ChangedFile(path="docs/deploy.md", status="modified", additions=1, deletions=0, changes=1, patch=patch)] + files = [ChangedFile(path="deploy/config.py", status="modified", additions=1, deletions=0, changes=1, patch=patch)] checks = {"state": "success", "count": 1, "repository_tree_complete": True} - flags = compute_flags(metadata(), files, checks, ["docs/deploy.md"]) + flags = compute_flags(metadata(), files, checks, ["deploy/config.py"]) codes = {flag.code for flag in flags} assert "EXPOSED_SECRET" in codes assert files[0].additions == 1 diff --git a/tests/test_triage.py b/tests/test_triage.py index 2522c46..4cb6881 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -36,16 +36,41 @@ def test_exposed_credential_blocks(): assert "credential-like value" in reasoning[0] -def test_dependency_and_missing_tests_quarantines(): +def test_manifest_touch_without_new_packages_is_a_review_note(): + """Regression: any manifest edit used to quarantine; version bumps are routine.""" files = [ ChangedFile("pyproject.toml", "modified", 1, 0, 1, '+dependencies = ["new-lib"]\n'), ChangedFile("src/new_module.py", "added", 3, 0, 3, "+def run():\n+ return 1\n"), ] flags = compute_flags(metadata(), files, {"state": "success", "count": 1}, ["pyproject.toml", "src/new_module.py"]) - verdict, _ = verdict_for(flags, {"state": "success"}) + verdict, reasoning = verdict_for(flags, {"state": "success"}) assert "NEW_DEPENDENCY" in {flag.code for flag in flags} assert "NO_TEST_COVERAGE" in {flag.code for flag in flags} + assert verdict == "PASS" + assert any("dependency manifests changed" in item for item in reasoning) + + +def test_truly_new_dependency_quarantines(): + files = [ + ChangedFile("requirements.txt", "modified", 1, 0, 2, "-flask==3.0.0\n+flask==3.1.0\n+left-pad==1.0.0\n"), + ] + flags = compute_flags(metadata(), files, {"state": "success", "count": 1}, ["requirements.txt"]) + verdict, reasoning = verdict_for(flags, {"state": "success"}) assert verdict == "QUARANTINE" + assert any("newly added dependencies" in item for item in reasoning) + named = next(flag for flag in flags if flag.code == "NEW_DEPENDENCY") + assert any("left-pad" in ev for ev in named.evidence) + + +def test_version_bump_only_passes_with_a_note(): + files = [ + ChangedFile("requirements.txt", "modified", 1, 1, 2, "-requests==2.31.0\n+requests==2.32.0\n"), + ] + flags = compute_flags(metadata(), files, {"state": "success", "count": 1}, ["requirements.txt"]) + verdict, reasoning = verdict_for(flags, {"state": "success"}) + assert "NEW_DEPENDENCY" in {flag.code for flag in flags} + assert verdict == "PASS" + assert any("dependency manifests changed" in item for item in reasoning) def test_all_clear_ships(): @@ -74,11 +99,13 @@ def test_unknown_checks_do_not_downgrade_an_otherwise_healthy_pr(): assert "status checks were unavailable" in reasoning[0] -def test_pending_checks_quarantine(): - flags = compute_flags(metadata(), [], {"state": "pending", "count": 1}, []) +def test_pending_checks_are_a_review_note_not_a_gate(): + """Regression: pending checks used to quarantine every PR analyzed before CI finished.""" + file = ChangedFile("src/app.py", "modified", 1, 0, 1, "+value = compute()\n") + flags = compute_flags(metadata(), [file], {"state": "pending", "count": 1, "pending": ["ci/build"]}, ["tests/test_app.py"]) verdict, reasoning = verdict_for(flags, {"state": "pending"}) assert "CHECKS_PENDING" in {flag.code for flag in flags} - assert verdict == "QUARANTINE" + assert verdict == "PASS" assert any("still running" in item for item in reasoning) @@ -99,3 +126,35 @@ def test_changed_line_numbers_ignore_no_newline_markers(): ) file.hunks = parse_hunks(file.patch) assert changed_line_numbers(file) == [1, 2] + + +def test_credential_in_tests_quarantines_instead_of_blocking(): + """Regression: fake credentials inside test fixtures used to hard-block PRs.""" + file = ChangedFile( + "tests/fixtures/deploy.py", + "modified", + 1, + 0, + 1, + '+DB_URL = "postgresql://admin:hunter2@db.internal.example/prod"\n', + ) + flags = compute_flags(metadata(), [file], {"state": "success", "count": 1}, []) + verdict, reasoning = verdict_for(flags, {"state": "success"}) + assert "EXPOSED_SECRET" in {flag.code for flag in flags} + assert verdict == "QUARANTINE" + assert any("outside production code" in item for item in reasoning) + + +def test_credential_in_production_still_blocks(): + file = ChangedFile( + "deploy/config.py", + "modified", + 1, + 0, + 1, + '+DB_URL = "postgresql://admin:hunter2@db.internal.example/prod"\n', + ) + flags = compute_flags(metadata(), [file], {"state": "success", "count": 1}, []) + verdict, reasoning = verdict_for(flags, {"state": "success"}) + assert verdict == "BLOCK" + assert "production code" in reasoning[0]