Repository navigation
Reset resolved baseline review blocks without approving - #149
Conversation
Add frozen obligation snapshots, native completion validation, bounded historical artifact verification, and exact-target reset helpers. Runtime wiring and verification follow separately. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Use native read-only tool inventory, frozen original obligations, structured completion evidence, required accepted-result persistence, and artifact-backed baseline publication before exact resets. Add mocked regression coverage and document retention and runtime-validation limits. Preserve lossless diff decoding and canonical redaction; use structured audit status rather than prose heuristics. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| require(len(init) == 1 and isinstance(init[0].get("tools"), list) | ||
| and set(init[0]["tools"]).issubset(MODEL_TOOLS) | ||
| and "Read" in init[0]["tools"] and "Bash" not in init[0]["tools"], | ||
| "runner did not use the constrained model tool inventory") | ||
| require(init[0].get("permissionMode") == "default", "unexpected model permission mode") |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): These checks expect a specific runner format: an exact tool set, permissionMode == "default", only the two named MCP servers, and structured_output on the terminal message. They also depend on --json-schema accepting the pattern, minLength/maxItems and ["string","null"] keywords. None of this has been confirmed against a live pinned runner. If any guess is wrong, every review fails at validate, and publish is skipped, so the PR gets no report and no verdict at all. Before enabling, run the candidate smoke test, and consider a separate failure annotation that makes this case easy to spot.
Superseded — see the current review report for commit
|
Bind native completion to the current runner session instead of transporting the full structured body through env. Add trusted read-path settings, realistic MCP fixtures and file-transport regressions, and document required pre-merge runtime smoke. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Restore the baseline reviewer tools, permissions, report publisher and tests. Capture optional run-bound audit evidence without changing ordinary publication, then dismiss only exact positively owned prior blocks after persistence and completed clean publication. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
|
|
||
|
|
||
| def capture(): | ||
| snapshot = read_json(SNAPSHOT) |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): capture re-reads the frozen snapshot from the workspace (.github/review-reset.json) after the model step and only checks identity. dismiss re-derives each target's findings but never the audit list. If anything rewrites that file between prepare and capture (the model step holds Bash(gh api:*) and a write-capable token), entries for still-present blockers can be dropped and the proof would still validate. Consider writing a sha256 of the snapshot as a prepare step output and verifying it in capture, or re-deriving the audit IDs from the targets/prior inputs in dismiss.
Superseded — see the current review report for commit
|
Validate the returned workflow URL against the exact GitHub API root and repository, permitting standard and org-required workflow namespaces before paginating competing runs. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Replace owned reset files atomically, clear stale reset inputs before preparation, and capture from a runner-temporary authoritative snapshot. Keep all original blocking obligations and typed prior findings while leaving scraped deduplication context informational. Skip optional proof capture and upload when no eligible target exists. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Preserve supporting details in each original blocking finding and report short redacted per-review retention reasons during preparation. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| groups.append(current) | ||
| else: | ||
| # Wrapped prose, nested items and quotes must have a real owner. | ||
| require(current is not None, error) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): The summary template in base-pr-review.md:339-344 asks for "one line per distinct active issue" and never requires a -/1. list marker. When a prior report writes findings as plain lines (e.g. **New** — file.go:12 ...), this branch raises on the first one. The old row scraper accepted those reports; now they are always retained and need manual dismissal. Either have the template require top-level list items for Security/Correctness, or accept an unowned non-indented line as its own group.
Superseded — see the current review report for commit
|
Limit optional model proof to review completion, coverage, blockers and audit evidence. Reuse the existing frozen snapshot and runner completion envelope for provenance, preserving host and session binding checks. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
General PR Review: [CXH-2780] Reset resolved baseline review blocks without approvingBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit takes provenance fields (
The prompt, README and tests were updated to match. The old model-echoed fields were copies of snapshot values, so removing them loses no real binding. I scanned the full PR diff (5 files) for security and correctness. The incremental diff is complete, with no dropped paths and no truncation. No repo-local criteria loaded, so the base criteria applied. Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed commit: |
gontzess
changed the title
2 participants
Why
A clean
COMMENTreview does not clear the bot's earlierCHANGES_REQUESTEDreview. After the reported issues are fixed, that old block can remain until someone dismisses it manually.This PR lets the reviewer dismiss its own resolved blocking reviews after a complete, successful re-review—without submitting
APPROVEor satisfying an approval requirement.What changes
This applies to both baseline review profiles without an opt-in. Human and other reviewer profiles' reviews are never dismissal targets; unverifiable legacy reviews remain untouched. The existing model, tools, permissions and ordinary review publisher are unchanged.
Validation
python3 -m unittest discover -s .github/actions/pr-review/scripts -p 'test_*.py'git diff --checkpassed.Validation limit: the final simplification that makes provenance exclusively CI-owned has automated-test and source-review coverage, but has not yet been rerun in the live workflow. The live results are not evidence of deployment across all caller configurations.