This is an automated email from the ASF dual-hosted git repository.
terrymanu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shardingsphere.git
The following commit(s) were added to refs/heads/master by this push:
new 84723bf7dee Improve PR review evidence reuse and ledger cleanup
(#40183)
84723bf7dee is described below
commit 84723bf7dee2d3acaddb721f4e92d5794952f5c2
Author: Liang Zhang <[email protected]>
AuthorDate: Tue Oct 13 22:14:19 2026 +0800
Improve PR review evidence reuse and ledger cleanup (#40183)
---
.codex/harness/agents/cases.toml | 40 ++++++++
.codex/skills/review-pr/SKILL.md | 9 ++
.codex/skills/review-pr/scripts/review_ledger.py | 36 +++++--
.../skills/review-pr/tests/test_review_ledger.py | 112 +++++++++++++++++++++
4 files changed, 187 insertions(+), 10 deletions(-)
diff --git a/.codex/harness/agents/cases.toml b/.codex/harness/agents/cases.toml
index 8a14e86d4f1..1ea20f8c1b7 100644
--- a/.codex/harness/agents/cases.toml
+++ b/.codex/harness/agents/cases.toml
@@ -3595,3 +3595,43 @@ forbidden_summary_terms =
["origin=introduced-by-latest-commits", "origin=expose
critical = true
policy_binding_profile = "review-pr"
policy_assertions = [{source = "review-pr", statements = ["For a proven
previous-review miss, contradictory assessments on an unchanged basis, a
triggered high-risk review criterion, or an explicit user request for
independent review, obtain an independent read-only assessment through this
workflow and the Completion Gate using the current basis and complete effective
candidate without the previous verdict or an expected finding count.", "Never
use a previous Formal Review result as\nevi [...]
+
+[[cases]]
+id = "review_ledger_reuses_facts_without_inheriting_completion"
+group = "review-pr evidence reuse"
+description = "Shares current facts across discovery lenses while requiring
fresh review and reacquisition of changed evidence."
+phase = "read-only evidence-reuse assessment"
+ordinary_loop = false
+profile = "review-pr"
+semantic_policy_assertions = true
+prompt = "Answer this self-contained read-only policy question using only the
supplied facts and bound rules; do not inspect files, run commands, edit
anything or produce a Formal Review report. A: the current review ledger
contains raw evidence whose source versions, supporting inputs and applicable
coverage are proved current. The root-cause lens used that material, and the
blast-radius and tests-runtime lenses still need to assess the complete
candidate. Decide whether they may share [...]
+decision = "proceed"
+required_actions = []
+allowed_actions = []
+forbidden_actions = ["inspect_local", "edit_code", "edit_non_code",
"run_local_checks", "measure_performance", "run_pre_handoff_review",
"propose_commit_message", "mutate_git", "mutate_remote"]
+required_reasons = ["read_only_request"]
+required_summary_terms = ["A=share-material", "B=fresh-review",
"C=reacquire-evidence", "D=pending-and-candidate"]
+forbidden_summary_terms = ["A=reacquire-material", "B=inherit-completion",
"C=reuse-old-fact", "D=reviewed-and-confirmed"]
+critical = true
+policy_binding_profile = "review-pr"
+policy_assertions = [{source = "review-pr", statements = ["Reuse only facts
and verification results whose current-candidate validity is established; do
not inherit reviewed-file statuses, discovery passes, proof conclusions, or
convergence results.", "Before repeating evidence acquisition, consult the
current ledger's sources, block coverage, candidate findings and verification
material, and reuse facts only while their supporting inputs and applicable
coverage remain valid.", "All thre [...]
+
+[[cases]]
+id = "review_temporary_ledger_cleanup_preserves_report_and_scope"
+group = "review-pr temporary ledger lifecycle"
+description = "Cleans the exact current-review resources on terminal paths
while preserving external sources, the report and the original error."
+phase = "read-only review-cleanup assessment"
+ordinary_loop = false
+profile = "review-pr"
+semantic_policy_assertions = true
+prompt = "Answer this self-contained read-only policy question using only the
supplied facts and bound rules; do not inspect files, execute cleanup or
produce a Formal Review report. For every state the current review's temporary
ledger path was returned by initialization and recorded, resource ownership is
established and the exact cleanup route is available. A, B and C are
independent completed reviews whose self-contained reports already contain all
required evidence; their results ar [...]
+decision = "proceed"
+required_actions = []
+allowed_actions = []
+forbidden_actions = ["inspect_local", "delete_local", "edit_code",
"edit_non_code", "run_local_checks", "run_pre_handoff_review",
"propose_commit_message", "mutate_git", "mutate_remote"]
+required_reasons = ["read_only_request"]
+required_summary_terms = ["A=cleanup", "B=cleanup", "C=cleanup", "D=preserve",
"E=report-cleanup-failure", "assessment=unchanged",
"F=attempt-cleanup-and-preserve-error"]
+forbidden_summary_terms = ["A=retain", "B=retain", "C=retain", "D=delete",
"E=claim-cleanup-success", "assessment=change-verdict",
"F=skip-cleanup-or-hide-error"]
+critical = true
+policy_binding_profile = "review-pr"
+policy_assertions = [{source = "review-pr", statements = ["Use the existing
private system temporary directory and record the exact ledger path returned by
initialization for this review.", "Keep ledger data private and remove only the
exact current-review ledger.", "Before returning a completed review result,
form a self-contained report and automatically run `scripts/review_ledger.py
cleanup --ledger <recorded-ledger>` for the recorded current-review instance,
including Mergeable, Not [...]
diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md
index 37ec75e104e..6a5324f6f08 100644
--- a/.codex/skills/review-pr/SKILL.md
+++ b/.codex/skills/review-pr/SKILL.md
@@ -176,8 +176,11 @@ Apply this workflow to the canonical review basis without
letting output mode or
Start every Formal Review round with fresh coverage of the complete effective
candidate and its affected behavior and contracts, preserving the original task
baseline and attributed scope.
Reuse only facts and verification results whose current-candidate validity is
established; do not inherit reviewed-file statuses, discovery passes, proof
conclusions, or convergence results.
+Before repeating evidence acquisition, consult the current ledger's sources,
block coverage, candidate findings and verification material, and reuse facts
only while their supporting inputs and applicable coverage remain valid.
+All three discovery lenses may share the same current source material, but
each must still cover the complete candidate and sharing material does not
replace the convergence pass.
For a proven previous-review miss, contradictory assessments on an unchanged
basis, a triggered high-risk review criterion, or an explicit user request for
independent review, obtain an independent read-only assessment through this
workflow and the Completion Gate using the current basis and complete effective
candidate without the previous verdict or an expected finding count.
+Give the independent reviewer the current requirements and their provenance,
complete candidate inventory and relevant raw evidence rather than the author's
full session history.
1. Complete `Repository Code Policy References`, then establish the
authoritative effective-candidate scope and applicable requirements.
2. Complete the Problem and Project Commitment Gate and establish the accepted
behavior boundary.
@@ -207,13 +210,19 @@ Apply the Completion Gate to every Formal Review and to a
discussion reply that
- Use `scripts/build_review_inventory.py --format json` when local refs are
available; its Markdown is only a bounded summary.
- Use `scripts/review_ledger.py` for multi-file, high-risk, or omission-prone
review; it accounts for review state but does not judge correctness.
- Initialize a new ledger for each round that requires one, from the full
current inventory with pending file statuses and no inherited review passes.
+- Use the existing private system temporary directory and record the exact
ledger path returned by initialization for this review.
- Initialize with `--review-basis <JSON>` containing the selected focus and
readable sources identified by role and origin, so the ledger binds their
actual content versions to the effective candidate, original baseline and
authoritative files.
- Record each substantive changed block's behavior, applicable risk axes and
requirement origins, strongest counter-evidence and current source anchors with
`mark-file --coverage <JSON>`, and attach finding anchors with `add-finding
--anchors <JSON>`.
+- Record partial coverage with pending files and keep unproved findings as
candidates; recording evidence does not establish a completed review.
- Rebuild ledgers with an older schema or stale inputs without inheriting
their completion state.
- Validate the current basis and coverage before selecting a result; a passing
ledger proves mechanical coverage and evidence freshness, while the reviewer
must still establish semantic proof under the existing gates.
- For a standalone local candidate with index or working-tree changes, provide
the same exact task-path file to both scripts with `--candidate-files <path>`.
The scripts resolve those paths from the computed merge-base through the
working tree, include matching untracked files, and fail when a listed path is
unchanged.
- Mutate one ledger sequentially; do not run ledger commands concurrently.
- Keep ledger data private and remove only the exact current-review ledger.
+- Before returning a completed review result, form a self-contained report and
automatically run `scripts/review_ledger.py cleanup --ledger <recorded-ledger>`
for the recorded current-review instance, including Mergeable, Not Mergeable
and Review Incomplete results.
+- Before cleaning the ledger, remove only temporary input materials created
and explicitly recorded for this review, preserving declared sources owned by
the repository, the user or another review.
+- On a handled failure or cancellation, attempt the same cleanup for resources
whose ownership is established, preserve the original failure and report any
cleanup failure or remaining resources.
+- Keep the report independent of temporary files and report cleanup failures
without changing the review assessment.
Use these JSON shapes in local files passed to the arguments, replacing the
example values with current evidence and block IDs from the initialized ledger:
Sources use `rule`, `requirement` or `evidence` roles; `--anchors` takes an
anchor-array file shaped like `anchors` below.
diff --git a/.codex/skills/review-pr/scripts/review_ledger.py
b/.codex/skills/review-pr/scripts/review_ledger.py
index 913c70ba1cc..4d68a4069a1 100755
--- a/.codex/skills/review-pr/scripts/review_ledger.py
+++ b/.codex/skills/review-pr/scripts/review_ledger.py
@@ -26,7 +26,6 @@ import hashlib
import json
import os
import re
-import shutil
import stat
import subprocess
import sys
@@ -81,11 +80,12 @@ def resolve_ledger_file(value: str | Path) -> Path:
return result / LEDGER_FILE_NAME if result.is_dir() else result
-def ensure_safe_ledger_file(ledger_file: Path) -> None:
+def ensure_safe_ledger_file(ledger_file: Path) -> Path:
root = ledger_root().resolve()
target = ledger_file.resolve()
if LEDGER_FILE_NAME != target.name or root != target.parent.parent:
raise RuntimeError(f"Ledger is outside the private review ledger root:
{target}")
+ return target
def validate_schema(ledger: object) -> None:
@@ -163,15 +163,14 @@ def validate_schema(ledger: object) -> None:
def read_ledger(value: str | Path) -> dict[str, Any]:
- ledger_file = resolve_ledger_file(value)
- ensure_safe_ledger_file(ledger_file)
+ ledger_file = ensure_safe_ledger_file(resolve_ledger_file(value))
ledger = json.loads(ledger_file.read_text(encoding="utf-8"))
validate_schema(ledger)
return ledger
def write_ledger(ledger_file: Path, ledger: dict[str, Any]) -> None:
- ensure_safe_ledger_file(ledger_file)
+ ledger_file = ensure_safe_ledger_file(ledger_file)
ledger["updated_at"] = int(time.time())
temporary_file = ledger_file.with_suffix(".tmp")
temporary_file.write_text(json.dumps(ledger, indent=2, sort_keys=True) +
"\n", encoding="utf-8")
@@ -487,7 +486,14 @@ def cmd_init(args: argparse.Namespace) -> int:
"findings": [],
"passes": [],
}
- write_ledger(ledger_file, ledger)
+ try:
+ write_ledger(ledger_file, ledger)
+ except (OSError, RuntimeError):
+ try:
+ remove_ledger_dir(ledger_file)
+ except (OSError, RuntimeError) as ex:
+ print(f"error: Initialization cleanup failed; current-review
resources may remain at {ledger_dir}: {ex}", file=sys.stderr)
+ raise
print(f"ledger={ledger_file}")
print(f"files={len(changed_files)}")
return 0
@@ -751,12 +757,22 @@ def cmd_status(args: argparse.Namespace) -> int:
return 0
+def remove_ledger_dir(ledger_file: Path) -> None:
+ ledger_file = ensure_safe_ledger_file(ledger_file)
+ ledger_dir = ledger_file.parent
+ entries = list(ledger_dir.iterdir())
+ owned_names = {LEDGER_FILE_NAME, ledger_file.with_suffix(".tmp").name}
+ if any(each.name not in owned_names or each.is_symlink() or not
each.is_file() for each in entries):
+ raise RuntimeError("Ledger directory contains unowned resources;
cleanup refused")
+ for entry in entries:
+ entry.unlink()
+ ledger_dir.rmdir()
+
+
def cmd_cleanup(args: argparse.Namespace) -> int:
- ledger_file = resolve_ledger_file(args.ledger)
+ ledger_file = ensure_safe_ledger_file(resolve_ledger_file(args.ledger))
read_ledger(ledger_file)
- ledger_dir = ledger_file.parent
- ensure_safe_ledger_file(ledger_file)
- shutil.rmtree(ledger_dir)
+ remove_ledger_dir(ledger_file)
print("removed=1")
return 0
diff --git a/.codex/skills/review-pr/tests/test_review_ledger.py
b/.codex/skills/review-pr/tests/test_review_ledger.py
index 986431d3434..72594645100 100644
--- a/.codex/skills/review-pr/tests/test_review_ledger.py
+++ b/.codex/skills/review-pr/tests/test_review_ledger.py
@@ -230,6 +230,118 @@ class ReviewLedgerTest(unittest.TestCase):
with self.assertRaisesRegex(RuntimeError, "outside the private
review ledger root"):
ledger.ensure_safe_ledger_file(outside_file)
+ def test_file_alias_read_and_write_use_canonical_ledger(self):
+ actual = self.create_valid_ledger()
+ outside = self.private_root / "outside"
+ outside.mkdir()
+ alias = outside / "alias.json"
+ alias.symlink_to(self.ledger_file)
+ actual["review_revision"] += 1
+ ledger.write_ledger(alias, actual)
+ self.assertTrue(alias.is_symlink())
+ self.assertFalse(alias.with_suffix(".tmp").exists())
+ self.assertEqual(actual, ledger.read_ledger(alias))
+ self.assertEqual(actual, ledger.read_ledger(self.ledger_file))
+
+ def
test_cleanup_file_alias_preserves_external_and_sibling_directories(self):
+ actual = self.create_valid_ledger()
+ sibling = ledger.create_ledger_dir(self.repo, "sibling",
self.base_sha) / ledger.LEDGER_FILE_NAME
+ ledger.write_ledger(sibling, actual)
+ sibling_content = sibling.read_bytes()
+ outside = self.private_root / "outside"
+ outside.mkdir()
+ marker = outside / "keep.txt"
+ marker.write_text("Unrelated external resource.\n", encoding="utf-8")
+ alias = outside / "alias.json"
+ alias.symlink_to(self.ledger_file)
+ with redirect_stdout(StringIO()):
+ self.assertEqual(0,
ledger.cmd_cleanup(Namespace(ledger=str(alias))))
+ self.assertFalse(self.ledger_file.parent.exists())
+ self.assertTrue(alias.is_symlink())
+ self.assertEqual("Unrelated external resource.\n",
marker.read_text(encoding="utf-8"))
+ self.assertEqual(sibling_content, sibling.read_bytes())
+
+ def test_cleanup_rejects_ledger_outside_private_root(self):
+ actual = self.create_valid_ledger()
+ outside = self.private_root / "outside"
+ outside.mkdir()
+ outside_file = outside / ledger.LEDGER_FILE_NAME
+ outside_file.write_text(json.dumps(actual), encoding="utf-8")
+ content = outside_file.read_bytes()
+ with self.assertRaisesRegex(RuntimeError, "outside the private review
ledger root"):
+ ledger.cmd_cleanup(Namespace(ledger=str(outside_file)))
+ self.assertEqual(content, outside_file.read_bytes())
+ self.assertTrue(self.ledger_file.exists())
+
+ def test_cleanup_rejects_unowned_directory_contents(self):
+ self.create_valid_ledger()
+ unknown = self.ledger_file.parent / "coverage.json"
+ unknown.write_text("Unattributed temporary input.\n", encoding="utf-8")
+ content = self.ledger_file.read_bytes()
+ with self.assertRaisesRegex(RuntimeError, "unowned resources; cleanup
refused"):
+ ledger.cmd_cleanup(Namespace(ledger=str(self.ledger_file)))
+ self.assertEqual(content, self.ledger_file.read_bytes())
+ self.assertEqual("Unattributed temporary input.\n",
unknown.read_text(encoding="utf-8"))
+
+ def test_cleanup_preserves_invalid_ledger(self):
+ self.create_valid_ledger()
+ self.ledger_file.write_text("{}", encoding="utf-8")
+ with self.assertRaisesRegex(RuntimeError, "Unsupported or invalid
review coverage ledger"):
+ ledger.cmd_cleanup(Namespace(ledger=str(self.ledger_file)))
+ self.assertEqual("{}", self.ledger_file.read_text(encoding="utf-8"))
+
+ def test_cleanup_failure_is_reported(self):
+ self.create_valid_ledger()
+ content = self.ledger_file.read_bytes()
+ output, error = StringIO(), StringIO()
+ arguments = ["review_ledger.py", "cleanup", "--ledger",
str(self.ledger_file)]
+ with patch.object(sys, "argv", arguments), patch.object(Path,
"unlink", side_effect=PermissionError("cleanup denied")), \
+ redirect_stdout(output), redirect_stderr(error):
+ self.assertEqual(1, ledger.main())
+ self.assertEqual("", output.getvalue())
+ self.assertEqual("error: cleanup denied\n", error.getvalue())
+ self.assertEqual(content, self.ledger_file.read_bytes())
+
+ def test_init_write_failure_removes_only_new_directory(self):
+ self.create_valid_ledger()
+ sibling = self.ledger_file
+ content = sibling.read_bytes()
+ failure = OSError("initial write failed")
+
+ def fail_write(ledger_file, _):
+ ledger_file.with_suffix(".tmp").write_text("Partial initial
write.\n", encoding="utf-8")
+ raise failure
+
+ with patch.object(ledger, "write_ledger", side_effect=fail_write),
self.assertRaises(OSError) as caught:
+ self.initialize_ledger()
+ self.assertIs(failure, caught.exception)
+ self.assertEqual([sibling.parent],
list(ledger.ledger_root().iterdir()))
+ self.assertEqual(content, sibling.read_bytes())
+
+ def
test_init_cleanup_failure_preserves_original_error_and_reports_residuals(self):
+ failure = OSError("initial write failed")
+
+ def fail_write(ledger_file, _):
+ ledger_file.with_suffix(".tmp").write_text("Partial initial
write.\n", encoding="utf-8")
+ raise failure
+
+ error = StringIO()
+ with patch.object(ledger, "write_ledger", side_effect=fail_write),
patch.object(Path, "unlink", side_effect=PermissionError("cleanup denied")), \
+ redirect_stderr(error), self.assertRaises(OSError) as caught:
+ self.initialize_ledger()
+ self.assertIs(failure, caught.exception)
+ directories = list(ledger.ledger_root().iterdir())
+ self.assertEqual(1, len(directories))
+ self.assertEqual("Partial initial write.\n", (directories[0] /
"ledger.tmp").read_text(encoding="utf-8"))
+ self.assertEqual(f"error: Initialization cleanup failed;
current-review resources may remain at {directories[0]}: cleanup denied\n",
error.getvalue())
+
+ def test_init_write_failure_before_output_removes_new_directory(self):
+ failure = PermissionError("initial write denied")
+ with patch.object(ledger, "write_ledger", side_effect=failure),
self.assertRaises(PermissionError) as caught:
+ self.initialize_ledger()
+ self.assertIs(failure, caught.exception)
+ self.assertEqual([], list(ledger.ledger_root().iterdir()))
+
def test_old_ledger_version_requires_new_review(self):
actual = self.create_valid_ledger()
actual["version"] = 3
