diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index c79818d..4e55c9c 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -81,6 +81,7 @@ runs: --event-head "${{ github.event.pull_request.head.sha }}" \ --event-base "${{ github.event.pull_request.base.sha }}" \ --state-dir "$STATE_DIR" \ + --resolve-threads "${{ inputs.github_identity_token != '' }}" \ --review-depth "$REVIEW_DEPTH" \ --premortem "$PREMORTEM" @@ -175,9 +176,10 @@ runs: 1. Respect the frozen mode and SHA scope in review-input.json. For an incremental review, discover new findings only in review.diff. Use the current files and full.diff to decide whether prior findings were fixed. - 2. Examine every open manifest finding and return exactly one prior_findings - disposition (`open` or `resolved`) for it. Never resolve human-authored - threads. + 2. Examine every open manifest finding. Return a prior_findings disposition + (`open` or `resolved`) for each finding you assessed. An omitted finding + remains open; only an explicit `resolved` disposition closes one. Never + resolve human-authored threads. 3. Put only high-confidence, actionable defects in findings. Severity and confidence are separate: uncertain concerns belong in open_questions with medium or low confidence, phrased as concrete verification requests. diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 8c6879f..3f5c73e 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -585,6 +585,7 @@ def import_legacy_findings( manifest["findings"][legacy_id] = { "fingerprint": None, "status": "open", + "thread_resolution": "unresolved", "severity": "major", "confidence": "high", "title": title, @@ -616,9 +617,23 @@ def sync_manifest_threads( resolved = thread_status.get(str(finding["thread_id"])) if resolved is True: finding["status"] = "resolved" + finding["thread_resolution"] = "confirmed" elif resolved is False: - finding["status"] = "open" - finding["resolved_sha"] = None + if ( + finding.get("status") == "resolved" + and finding.get("thread_resolution") == "confirmed" + ): + finding["status"] = "open" + finding["resolved_sha"] = None + finding["thread_resolution"] = "unresolved" + elif finding.get("status") == "resolved": + # An unresolved GitHub thread is expected when publication used + # the job token and deliberately skipped thread resolution. + # Preserve the code-level disposition until a capable identity + # can resolve the thread. + finding["thread_resolution"] = "skipped" + else: + finding["thread_resolution"] = "unresolved" def write_github_output(values: dict[str, Any]) -> None: @@ -853,6 +868,7 @@ def prepare(args: argparse.Namespace) -> None: "commentable_lines": commentable_lines, "sticky_comment_id": sticky_comment_id, "publisher_login": publisher_login, + "thread_resolution_enabled": args.resolve_threads == "true", "pipeline_version": pipeline_version, "rubric_version": rubric_version, } @@ -974,12 +990,11 @@ def compile_review( ] if len(returned_prior_ids) != len(set(returned_prior_ids)): raise PipelineError("prior_findings contains duplicate finding IDs") - if set(returned_prior_ids) != expected_prior_ids: - missing = sorted(expected_prior_ids - set(returned_prior_ids)) - unknown = sorted(set(returned_prior_ids) - expected_prior_ids) + unknown = sorted(set(returned_prior_ids) - expected_prior_ids) + if unknown: raise PipelineError( - "prior_findings must disposition every open finding exactly once " - f"(missing={missing}, unknown={unknown})" + "prior_findings references findings that are not open " + f"(unknown={unknown})" ) scope_summary = public_text( @@ -992,6 +1007,9 @@ def compile_review( ) resolution_ids: list[str] = [] + resolution_enabled = bool( + review_input.get("thread_resolution_enabled", False) + ) for disposition in model_output["prior_findings"]: if not isinstance(disposition, dict): raise PipelineError("prior_findings contains a non-object") @@ -1014,7 +1032,23 @@ def compile_review( ) item["resolved_sha"] = head if item.get("thread_id"): - resolution_ids.append(item["thread_id"]) + item["thread_resolution"] = ( + "pending" if resolution_enabled else "skipped" + ) + elif item.get("thread_id"): + item["thread_resolution"] = "unresolved" + + if resolution_enabled: + resolution_ids = sorted( + { + str(item["thread_id"]) + for item in manifest["findings"].values() + if isinstance(item, dict) + and item.get("status") == "resolved" + and item.get("thread_id") + and item.get("thread_resolution") != "confirmed" + } + ) changed_paths = set(scope["full_pr_paths"]) commentable = { @@ -1119,6 +1153,7 @@ def compile_review( manifest["findings"][item_id] = { "fingerprint": item_id, "status": "open", + "thread_resolution": "unresolved", "severity": finding["severity"], "confidence": finding["confidence"], "title": finding["title"], @@ -1768,13 +1803,19 @@ def require_frozen_pull() -> None: payload, before_write=require_frozen_pull, ) - resolve_threads( + resolved_threads = resolve_threads( payload["resolve_thread_ids"], enabled=os.environ.get("GH_RESOLVE_THREADS", "").lower() == "true", before_write=require_frozen_pull, ) manifest = payload["manifest"] + for finding in manifest["findings"].values(): + if ( + isinstance(finding, dict) + and finding.get("thread_id") in resolved_threads + ): + finding["thread_resolution"] = "confirmed" for finding_id_value, posted in posted_comments.items(): finding = manifest["findings"].get(finding_id_value) if isinstance(finding, dict): @@ -1848,6 +1889,11 @@ def parser() -> argparse.ArgumentParser: prepare_parser.add_argument("--event-head") prepare_parser.add_argument("--event-base") prepare_parser.add_argument("--state-dir", required=True) + prepare_parser.add_argument( + "--resolve-threads", + choices=("true", "false"), + default="false", + ) prepare_parser.add_argument( "--review-depth", choices=("standard", "deep"), diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 93684e0..c3b56fa 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -42,6 +42,7 @@ def review_input(*, mode: str = "full"): "commentable_lines": {"src/example.py": [10, 11, 12]}, "sticky_comment_id": None, "publisher_login": "github-actions[bot]", + "thread_resolution_enabled": False, "pipeline_version": "sha256:pipeline", "rubric_version": "sha256:rubric", } @@ -98,8 +99,29 @@ def test_action_exposes_optional_github_identity_token(self): "${{ inputs.github_identity_token != '' }}", action, ) + self.assertIn( + '--resolve-threads "${{ inputs.github_identity_token != \'\' }}"', + action, + ) self.assertNotIn("GH_TOKEN: ${{ steps.review", action) + def test_workflow_loads_actions_from_trusted_main(self): + workflow = ( + Path(pipeline.__file__).parents[2] / "workflows" / "claude.yml" + ).read_text(encoding="utf-8") + + self.assertIn( + "uses: megaeth-labs/documentation/" + ".github/actions/claude-pr-review@main", + workflow, + ) + self.assertIn( + "uses: megaeth-labs/documentation/" + ".github/actions/claude-interactive@main", + workflow, + ) + self.assertNotIn("uses: ./.github/actions/claude-", workflow) + def test_output_schema_uses_action_compatible_dialect(self): schema_path = Path(pipeline.__file__).with_name( "review-output.schema.json" @@ -419,12 +441,13 @@ def test_configured_identity_thread_links_from_stateful_review(self): self.assertEqual(finding["thread_id"], "THREAD") self.assertEqual(finding["comment_id"], 99) - def test_unresolved_github_thread_reopens_manifest_finding(self): + def test_manually_reopened_github_thread_reopens_manifest_finding(self): manifest = review_input()["manifest"] manifest["findings"]["F-1"] = { "status": "resolved", "resolved_sha": "old", "thread_id": "THREAD", + "thread_resolution": "confirmed", } pipeline.sync_manifest_threads( manifest, @@ -432,6 +455,49 @@ def test_unresolved_github_thread_reopens_manifest_finding(self): ) self.assertEqual(manifest["findings"]["F-1"]["status"], "open") self.assertIsNone(manifest["findings"]["F-1"]["resolved_sha"]) + self.assertEqual( + manifest["findings"]["F-1"]["thread_resolution"], + "unresolved", + ) + + def test_skipped_github_resolution_does_not_reopen_finding(self): + manifest = review_input()["manifest"] + manifest["findings"]["F-1"] = { + "status": "resolved", + "resolved_sha": "old", + "thread_id": "THREAD", + "thread_resolution": "skipped", + } + + pipeline.sync_manifest_threads( + manifest, + [{"id": "THREAD", "isResolved": False}], + ) + + self.assertEqual(manifest["findings"]["F-1"]["status"], "resolved") + self.assertEqual( + manifest["findings"]["F-1"]["thread_resolution"], + "skipped", + ) + + def test_legacy_resolved_finding_migrates_to_skipped_resolution(self): + manifest = review_input()["manifest"] + manifest["findings"]["F-1"] = { + "status": "resolved", + "resolved_sha": "old", + "thread_id": "THREAD", + } + + pipeline.sync_manifest_threads( + manifest, + [{"id": "THREAD", "isResolved": False}], + ) + + self.assertEqual(manifest["findings"]["F-1"]["status"], "resolved") + self.assertEqual( + manifest["findings"]["F-1"]["thread_resolution"], + "skipped", + ) def test_clean_incremental_review_is_compact(self): payload = pipeline.compile_review( @@ -476,18 +542,36 @@ def test_uncertain_item_becomes_open_question(self): payload["review_body"], ) - def test_every_open_prior_finding_requires_a_disposition(self): + def test_omitted_prior_finding_remains_open(self): value = review_input() value["manifest"]["findings"]["F-existing"] = { "status": "open", "severity": "major", "thread_id": "THREAD", } + payload = pipeline.compile_review(value, clean_output()) + + self.assertEqual( + payload["manifest"]["findings"]["F-existing"]["status"], + "open", + ) + self.assertEqual(payload["resolve_thread_ids"], []) + + def test_unknown_prior_finding_still_fails(self): + output = clean_output() + output["prior_findings"] = [ + { + "finding_id": "F-unknown", + "disposition": "open", + "reason": "The issue remains.", + } + ] + with self.assertRaisesRegex( pipeline.PipelineError, - "disposition every open finding", + "not open", ): - pipeline.compile_review(value, clean_output()) + pipeline.compile_review(review_input(), output) def test_skip_with_open_finding_preserves_manifest(self): value = review_input(mode="skip") @@ -520,6 +604,7 @@ def test_invalid_finding_path_fails_loudly(self): def test_resolved_prior_finding_produces_thread_resolution(self): value = review_input() + value["thread_resolution_enabled"] = True value["manifest"]["findings"]["F-existing"] = { "status": "open", "severity": "major", @@ -539,6 +624,52 @@ def test_resolved_prior_finding_produces_thread_resolution(self): payload["manifest"]["findings"]["F-existing"]["status"], "resolved", ) + self.assertEqual( + payload["manifest"]["findings"]["F-existing"][ + "thread_resolution" + ], + "pending", + ) + + def test_resolved_prior_finding_skips_thread_without_identity(self): + value = review_input() + value["manifest"]["findings"]["F-existing"] = { + "status": "open", + "severity": "major", + "thread_id": "THREAD", + } + output = clean_output() + output["prior_findings"] = [ + { + "finding_id": "F-existing", + "disposition": "resolved", + "reason": "The retry now clears partial state.", + } + ] + + payload = pipeline.compile_review(value, output) + + self.assertEqual(payload["resolve_thread_ids"], []) + self.assertEqual( + payload["manifest"]["findings"]["F-existing"][ + "thread_resolution" + ], + "skipped", + ) + + def test_capable_identity_resolves_skipped_thread_without_reopening(self): + value = review_input() + value["thread_resolution_enabled"] = True + value["manifest"]["findings"]["F-existing"] = { + "status": "resolved", + "severity": "major", + "thread_id": "THREAD", + "thread_resolution": "skipped", + } + + payload = pipeline.compile_review(value, clean_output()) + + self.assertEqual(payload["resolve_thread_ids"], ["THREAD"]) def test_inline_finding_cannot_resolve_without_thread_id(self): value = review_input() @@ -586,6 +717,7 @@ def test_existing_open_finding_is_not_reposted(self): def test_resolved_prior_does_not_suppress_distinct_same_symbol_finding(self): value = review_input() + value["thread_resolution_enabled"] = True value["manifest"]["findings"]["F-existing"] = { "status": "open", "severity": "major", diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 53fbe16..be3c4ee 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -40,7 +40,7 @@ jobs: with: fetch-depth: 1 - - uses: ./.github/actions/claude-interactive + - uses: megaeth-labs/documentation/.github/actions/claude-interactive@main with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} allowed_bots: "mega-putin" @@ -88,7 +88,7 @@ jobs: echo "skip=false" >> "$GITHUB_OUTPUT" fi - - uses: ./.github/actions/claude-pr-review + - uses: megaeth-labs/documentation/.github/actions/claude-pr-review@main if: steps.workflow-change.outputs.skip != 'true' with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}