From 775e5a518f1d3ee29172d6c3014cf173faaad643 Mon Sep 17 00:00:00 2001 From: forhappy Date: Sun, 16 Aug 2026 16:38:40 -0700 Subject: [PATCH] Improve pull request review readability --- .github/actions/pr-review/delivery.test.cjs | 12 +++++++ COMPATIBILITY.md | 8 +++++ action.yml | 11 ++++-- crates/compass-pr-intelligence/src/analyze.rs | 33 ++++++++++++++--- .../tests/report_contract.rs | 36 +++++++++++++++++++ docs/guides/github-pr-review.md | 12 +++++++ docs/reference/outputs.md | 3 ++ docs/reference/pr-intelligence.md | 12 ++++++- 8 files changed, 120 insertions(+), 7 deletions(-) diff --git a/.github/actions/pr-review/delivery.test.cjs b/.github/actions/pr-review/delivery.test.cjs index abd6a20e..5bfe272a 100644 --- a/.github/actions/pr-review/delivery.test.cjs +++ b/.github/actions/pr-review/delivery.test.cjs @@ -377,3 +377,15 @@ test("installer pins release identity, checksum, and archive layout", () => { assert.ok(installer.includes("$0 ~ /(^|\\/)\\.\\.(\\/|$)/")); assert.doesNotMatch(installer, /releases\/latest/); }); + +test("analysis selects a code-only history profile before review", () => { + const action = fs.readFileSync(path.join(__dirname, "../../../action.yml"), "utf8"); + const analysis = action.match( + /- name: Analyze exact candidate without write credentials([\s\S]*?)- name: Render bounded Markdown and SARIF/, + )?.[1] ?? ""; + const profile = analysis.indexOf("compass history build \"$COMPASS_ACTION_BASE\" --code-only"); + const review = analysis.indexOf('compass "${args[@]}"'); + assert.ok(profile >= 0, "the action must select an explicit code-only profile"); + assert.ok(review >= 0, "the action must invoke compass review"); + assert.ok(profile < review, "the profile must be selected before review materialization"); +}); diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index dfb9d5cc..aab14140 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -356,6 +356,14 @@ Advisory risk is never a merge gate. The Action supports only `GateResult::Fail` states rather than risk band, score, SARIF level, or prose. Its required `compass-version` input must name an exact released version containing `compass review`; there is no fallback binary version. +On a fresh checkout, the Action explicitly materializes the target revision +with the local `--code-only` history profile before invoking review; it does +not silently downgrade a configured semantic profile. + +PR review finding statements now resolve retained entity identities to +human-readable names. Stable entity identities remain in the canonical finding +`source_entities` and `target_entities` fields, so this presentation change +does not alter finding fingerprints or machine traceability. This is additive in the `0.3.x` line. Existing `compass prs`, graph, history, and MCP contracts are unchanged; `compass diff` gains only the optional typed diff --git a/action.yml b/action.yml index 32d039dd..b82f16bd 100644 --- a/action.yml +++ b/action.yml @@ -120,8 +120,15 @@ runs: args+=(--fingerprint "$COMPASS_ACTION_FINGERPRINT") fi set +e - compass "${args[@]}" >"$COMPASS_ACTION_LOG.stdout" 2>"$COMPASS_ACTION_LOG" - status=$? + : >"$COMPASS_ACTION_LOG" + compass history build "$COMPASS_ACTION_BASE" --code-only >>"$COMPASS_ACTION_LOG" 2>&1 + profile_status=$? + if [ "$profile_status" -eq 0 ]; then + compass "${args[@]}" >"$COMPASS_ACTION_LOG.stdout" 2>>"$COMPASS_ACTION_LOG" + status=$? + else + status=$profile_status + fi set -e if [ "$status" -eq 0 ]; then echo "ok=true" >> "$GITHUB_OUTPUT" diff --git a/crates/compass-pr-intelligence/src/analyze.rs b/crates/compass-pr-intelligence/src/analyze.rs index 369ae236..4253d6c7 100644 --- a/crates/compass-pr-intelligence/src/analyze.rs +++ b/crates/compass-pr-intelligence/src/analyze.rs @@ -1,3 +1,4 @@ +use std::cmp::Reverse; use std::collections::{BTreeMap, BTreeSet}; use compass_semantic_diff as semantic; @@ -28,7 +29,13 @@ pub fn analyze( .iter() .take(MAX_FINDINGS) .map(|finding| { - convert_finding(request, manifest, finding).map(|converted| (finding, converted)) + convert_finding( + request, + manifest, + finding, + &semantic_diff.entity_display_names, + ) + .map(|converted| (finding, converted)) }) .collect::, _>>()?; if semantic_diff.findings.len() > MAX_FINDINGS { @@ -164,6 +171,7 @@ fn convert_finding( request: &ChangeRequest, manifest: &EvidenceManifest, finding: &semantic::SemanticFinding, + entity_display_names: &BTreeMap, ) -> Result { let finding_type = match finding.finding_type { semantic::FindingType::ContractChange => FindingType::ContractChange, @@ -250,7 +258,7 @@ fn convert_finding( fingerprint, finding_type, classifier_version: semantic::CLASSIFIER_VERSION, - statement: finding.headline.clone(), + statement: humanize_text(&finding.headline, entity_display_names), source_entities: vec![finding.subject.clone()], target_entities: targets, witness, @@ -260,7 +268,7 @@ fn convert_finding( exact_tests, recommended_tests, gap: verification_gap, - reason: finding.verification.reason.clone(), + reason: humanize_text(&finding.verification.reason, entity_display_names), }, source_revision: request .revisions @@ -273,11 +281,28 @@ fn convert_finding( confidence, completeness: manifest.completeness, freshness: Freshness::ExactHead, - remediation: finding.reviewer_action.clone(), + remediation: humanize_text(&finding.reviewer_action, entity_display_names), deterministic, }) } +fn humanize_text(value: &str, entity_display_names: &BTreeMap) -> String { + let mut replacements = entity_display_names + .iter() + .filter(|(identity, display_name)| identity.as_str() != display_name.as_str()) + .map(|(identity, display_name)| (identity.as_str(), display_name.as_str())) + .collect::>(); + replacements.sort_by_key(|(identity, _)| Reverse(identity.len())); + + let mut humanized = value.to_owned(); + for (identity, display_name) in replacements { + if humanized.contains(identity) { + humanized = humanized.replace(identity, display_name); + } + } + humanized +} + fn map_verification_state(value: semantic::VerificationState) -> VerificationState { match value { semantic::VerificationState::Unknown => VerificationState::Unknown, diff --git a/crates/compass-pr-intelligence/tests/report_contract.rs b/crates/compass-pr-intelligence/tests/report_contract.rs index 06201e0c..1d66e976 100644 --- a/crates/compass-pr-intelligence/tests/report_contract.rs +++ b/crates/compass-pr-intelligence/tests/report_contract.rs @@ -184,6 +184,42 @@ fn identical_input_is_byte_identical_and_round_trips() -> Result<(), Box Result<(), Box> { + let mut finding = semantic_finding(Confidence::Exact); + finding.headline = "symbol:api signature changed".to_owned(); + let mut semantic = semantic_report(RESULT, vec![finding]); + semantic + .entity_display_names + .insert("symbol:api".to_owned(), "Api::run()".to_owned()); + let report = analyze( + &request(MergeOutcome::Clean { + object_id: RESULT.to_owned(), + }), + &snapshot(BASE), + Some(&snapshot(RESULT)), + &manifest(Completeness::DownstreamComplete)?, + &semantic, + )?; + + assert_eq!(report.findings[0].statement, "Api::run() signature changed"); + assert_eq!(report.findings[0].source_entities, vec!["symbol:api"]); + assert_eq!( + report.findings[0].fingerprint, + clean_report( + Completeness::DownstreamComplete, + Some(semantic_finding(Confidence::Exact)), + )? + .findings[0] + .fingerprint + ); + let canonical = String::from_utf8(canonical_json_bytes(&report)?)?; + assert!(canonical.contains("Api::run() signature changed")); + assert!(canonical.contains("\"source_entities\":[\"symbol:api\"]")); + Ok(()) +} + #[test] fn readiness_is_additive_deterministic_and_conservative_about_tests_and_docs() -> Result<(), Box> { diff --git a/docs/guides/github-pr-review.md b/docs/guides/github-pr-review.md index 6d0a6d23..801fcec8 100644 --- a/docs/guides/github-pr-review.md +++ b/docs/guides/github-pr-review.md @@ -22,6 +22,18 @@ deterministic synthetic merge without changing the checkout, materializes comparable graph history when needed, and rejects profile mismatch. Local mode never fetches missing objects. +If the checkout has no existing history profile and contains non-code files, +select the local structural profile explicitly before the first review: + +```bash +compass history build "$BASE_SHA" --code-only +``` + +This keeps the review fully local and does not install hooks. A semantic review +must use a configured semantic history profile instead; missing credentials are +never silently downgraded. The reusable GitHub Action performs the explicit +code-only preparation automatically. + To bind a frozen GitHub event without a second API read: ```bash diff --git a/docs/reference/outputs.md b/docs/reference/outputs.md index 9471a11a..7cf7effa 100644 --- a/docs/reference/outputs.md +++ b/docs/reference/outputs.md @@ -557,6 +557,9 @@ risk, deterministic gates, canonical omissions, and a content digest. Markdown and text expose the same fingerprints and finding count unless an explicit Markdown projection budget omits findings. In that case the footer states the exact omitted count; the canonical report and digest are unchanged. +Finding statements and SARIF messages resolve retained entity identities to +human-readable names. Stable source and target identities remain available in +the canonical JSON for machine traceability. SARIF 2.1.0 stores each Compass fingerprint in `partialFingerprints` and keeps report identity, completeness, factors, gates, evidence, and omissions in properties. SARIF severity is a presentation hint, not merge policy. diff --git a/docs/reference/pr-intelligence.md b/docs/reference/pr-intelligence.md index e0c93aab..88501a1e 100644 --- a/docs/reference/pr-intelligence.md +++ b/docs/reference/pr-intelligence.md @@ -124,6 +124,12 @@ requires the corresponding objects locally. `--output` uses atomic writing. Markdown bounds report the exact projection omission count and do not mutate the canonical digest. +On a fresh checkout with non-code files and no existing history profile, build +the target realization explicitly with `compass history build BASE --code-only` +before running review. This is the local structural path; semantic profiles +remain explicit and are never silently downgraded. The reusable GitHub Action +performs this preparation automatically. + Usage errors exit 2. Capture, history, profile, semantic, limit, and output errors exit 1. A valid report exits 0 even when advisory risk is critical or a deterministic gate reports `fail`; merge policy belongs to the Action or the @@ -164,7 +170,11 @@ transport `semanticResultDigest`. - JSON is the canonical report and round-trips through the strict schema. - Text and Markdown include exact identity, completeness, factors, gates, - findings, witness paths/locations, verification gaps, and omissions. + findings, witness paths/locations, verification gaps, and omissions. Finding + statements in the canonical JSON and human-facing projections resolve + retained entity identities to human-readable names; stable entity identities + remain in `source_entities`, `target_entities`, and fingerprints for machine + traceability. - SARIF 2.1.0 preserves Compass fingerprints in `partialFingerprints` and carries the report digest, completeness, advisory result, factors, gates, witness evidence, and omissions in typed properties.