Merge pull request 'Harden PR queue reconciliation against stale or conflicting PR state reports' (#168) from feat/issue-166-pr-queue-reconciliation-harden into master
This commit was merged in pull request #168.
This commit is contained in:
@@ -345,6 +345,25 @@ touching anything.
|
||||
- **Prompt:** `Use any eligible reviewer profile to review PR #N. Approve only
|
||||
if scope matches issue #M and checks pass; otherwise request changes.`
|
||||
|
||||
**Live queue reconciliation (mandatory before any review/merge decision):**
|
||||
|
||||
- Reconcile live state first. Do **not** assume prior handoffs, cached tool
|
||||
output, or chat summaries are current.
|
||||
- Steps (in order):
|
||||
1. Call `gitea_list_prs` (open state) with explicit remote/org/repo.
|
||||
2. Immediately `gitea_view_pr <number>` for the candidate; capture head SHA,
|
||||
state, mergeable, updated_at, merged_at/merge_commit_sha if present.
|
||||
3. Verify against any prior report: state, head SHA, updated timestamp,
|
||||
linked issue state (use `gitea_view_issue` + `gitea_list_issues`).
|
||||
4. `git fetch <remote> --prune && git checkout master && git pull <remote> master --ff-only`
|
||||
5. If conflict/staleness detected (prior said "merged" but live open; head or
|
||||
updated_at differs from claimed; merge commit missing on master), report
|
||||
the inconsistency explicitly and **STOP** before review or merge.
|
||||
- After a successful merge: re-run list_prs + view_pr on the PR, confirm
|
||||
master advanced, and include the live post-merge verification in the handoff.
|
||||
- Treat any ambiguous queue state as a blocker until a fresh, consistent live
|
||||
picture is obtained.
|
||||
|
||||
### Merge a PR
|
||||
|
||||
- **Profile:** merger (allowed to merge; must **not** be the PR author).
|
||||
|
||||
+35
-6
@@ -402,8 +402,9 @@ def gitea_list_prs(
|
||||
repo: Override the repository name.
|
||||
|
||||
Returns:
|
||||
List of dicts with 'number', 'title', 'state', 'head', 'base', and
|
||||
'mergeable' ('url' only with the reveal opt-in).
|
||||
List of dicts with 'number', 'title', 'state', 'head', 'base',
|
||||
'mergeable', 'updated_at' ('url' only with the reveal opt-in).
|
||||
The additional 'updated_at' aids stale/conflicting queue detection.
|
||||
"""
|
||||
h, o, r = _resolve(remote, host, org, repo)
|
||||
auth = _auth(h)
|
||||
@@ -418,6 +419,7 @@ def gitea_list_prs(
|
||||
"head": pr["head"]["ref"],
|
||||
"base": pr["base"]["ref"],
|
||||
"mergeable": pr.get("mergeable"),
|
||||
"updated_at": pr.get("updated_at"),
|
||||
}
|
||||
results.append(_with_optional_url(entry, pr.get("html_url")))
|
||||
return results
|
||||
@@ -441,7 +443,10 @@ def gitea_view_pr(
|
||||
repo: Override the repository name.
|
||||
|
||||
Returns:
|
||||
dict with PR details.
|
||||
dict with PR details including 'updated_at', 'merged_at',
|
||||
'merge_commit_sha', 'closed_at' (when present) to support
|
||||
live-state reconciliation and staleness detection against
|
||||
prior reports or handoffs.
|
||||
"""
|
||||
h, o, r = _resolve(remote, host, org, repo)
|
||||
auth = _auth(h)
|
||||
@@ -456,6 +461,10 @@ def gitea_view_pr(
|
||||
"base": pr["base"]["ref"],
|
||||
"mergeable": pr.get("mergeable"),
|
||||
"user": pr.get("user", {}).get("login", ""),
|
||||
"updated_at": pr.get("updated_at"),
|
||||
"merged_at": pr.get("merged_at"),
|
||||
"merge_commit_sha": pr.get("merge_commit_sha"),
|
||||
"closed_at": pr.get("closed_at"),
|
||||
}
|
||||
return _with_optional_url(result, pr.get("html_url"))
|
||||
|
||||
@@ -1404,6 +1413,7 @@ def gitea_review_pr(
|
||||
"linked_issues": list(set(linked)),
|
||||
"labels": labels,
|
||||
"head_sha": pr_head_sha,
|
||||
"updated_at": pr.get("updated_at"),
|
||||
"requires_non_author_reviewer": req_non_author
|
||||
})
|
||||
except Exception as e:
|
||||
@@ -1429,6 +1439,7 @@ def gitea_review_pr(
|
||||
f" Author: {item['author']}\n"
|
||||
f" Base/Head: {item['base']} / {item['head']}\n"
|
||||
f" Head SHA: {item['head_sha']}\n"
|
||||
f" Updated: {item.get('updated_at') or 'unknown'}\n"
|
||||
f" Mergeability: {item['mergeable']}\n"
|
||||
f" Linked Issues: {item['linked_issues']}\n"
|
||||
f" Labels: {item['labels']}\n"
|
||||
@@ -1968,6 +1979,12 @@ _GUIDE_RULES = {
|
||||
"Do not hardcode Gitea identities; always dynamically verify your "
|
||||
"identity and resolve task capability via gitea_resolve_task_capability.",
|
||||
],
|
||||
"live_state_reconciliation": [
|
||||
"Never rely on prior handoffs, chat history, or cached reports for PR queue state.",
|
||||
"Before review or merge decisions: always call gitea_list_prs + gitea_view_pr (for the specific PR), re-fetch remotes, pull master --ff-only, and compare live fields (state, mergeable, head SHA, updated_at, merged_at, merge_commit_sha) against any prior claims.",
|
||||
"Conflicting or stale PR state (e.g. prior said merged but live open, or head/updated mismatch) is a hard blocker: report it explicitly and stop until live state is unambiguous.",
|
||||
"After merge, re-list PRs and re-view the PR (plus verify master contains the merge) before claiming completion.",
|
||||
],
|
||||
}
|
||||
|
||||
_COMMON_WORKFLOWS = [
|
||||
@@ -1978,10 +1995,15 @@ _COMMON_WORKFLOWS = [
|
||||
"implementation: claim issue, branch from fresh master, implement only "
|
||||
"the issue scope, test, open a PR referencing the issue, stop.",
|
||||
"PR review: verify reviewer identity (must be a reviewer profile/namespace), "
|
||||
"pin the head SHA, validate independently, post a verdict; never review your own work.",
|
||||
"reconcile live state first (list_prs + view_pr selected PR immediately; "
|
||||
"verify state/mergeable/head SHA/updated_at/merged info vs any prior reports; "
|
||||
"git fetch --prune and pull --ff-only master; check linked issue state); "
|
||||
"pin the head SHA; treat conflicting/stale reports as blocker and stop; "
|
||||
"validate independently, post a verdict; never review your own work. "
|
||||
"After merge, re-list and re-view.",
|
||||
"PR merge: reviewer identity (must be a reviewer profile/namespace) + "
|
||||
"eligibility check + pinned head + explicit 'MERGE PR <n>' confirmation, "
|
||||
"only with operator authorization.",
|
||||
"only with operator authorization. Reconcile live state before deciding.",
|
||||
]
|
||||
|
||||
# Skill registry (#128). status: 'available' = backed by tools in this
|
||||
@@ -2042,11 +2064,18 @@ _PROJECT_SKILLS = {
|
||||
"to confirm reviewer namespace and avoid author-profile blocks.",
|
||||
"Verify reviewer identity with gitea_whoami; the PR author "
|
||||
"must be a different user.",
|
||||
"Pin the PR head SHA (gitea_view_pr) before validating.",
|
||||
"Reconcile live queue state FIRST (do not trust prior handoffs): "
|
||||
"call gitea_list_prs, then immediately gitea_view_pr on the target; "
|
||||
"verify state, mergeable, head vs current, updated_at/merged info, "
|
||||
"linked issue; git fetch --prune; git checkout master; git pull <remote> master --ff-only. "
|
||||
"If prior report conflicts with live (e.g. 'merged' but live open, or head/updated mismatch), "
|
||||
"report inconsistency and STOP before review/merge.",
|
||||
"Pin the reviewed head SHA (gitea_view_pr) before validating.",
|
||||
"Validate independently: scope vs the linked issue, tests, "
|
||||
"diff check, secret sweep.",
|
||||
"Post the verdict with gitea_review_pr / "
|
||||
"gitea_submit_pr_review.",
|
||||
"After any merge, re-list open PRs and re-view the PR to confirm landed state.",
|
||||
"Do not merge unless the operator explicitly authorizes it.",
|
||||
],
|
||||
},
|
||||
|
||||
@@ -303,7 +303,8 @@ class TestListPRs(unittest.TestCase):
|
||||
{
|
||||
"number": 1, "title": "PR 1", "state": "open",
|
||||
"head": {"ref": "branch1"}, "base": {"ref": "main"},
|
||||
"html_url": "http://url1", "mergeable": True
|
||||
"html_url": "http://url1", "mergeable": True,
|
||||
"updated_at": "2026-07-05T12:00:00Z"
|
||||
}
|
||||
]
|
||||
with patch.dict(os.environ, {}, clear=True):
|
||||
@@ -312,6 +313,8 @@ class TestListPRs(unittest.TestCase):
|
||||
self.assertEqual(result[0]["number"], 1)
|
||||
self.assertEqual(result[0]["head"], "branch1")
|
||||
self.assertNotIn("url", result[0])
|
||||
# Staleness fields for queue reconciliation (#166)
|
||||
self.assertEqual(result[0]["updated_at"], "2026-07-05T12:00:00Z")
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@@ -320,7 +323,8 @@ class TestListPRs(unittest.TestCase):
|
||||
{
|
||||
"number": 1, "title": "PR 1", "state": "open",
|
||||
"head": {"ref": "branch1"}, "base": {"ref": "main"},
|
||||
"html_url": "http://url1", "mergeable": True
|
||||
"html_url": "http://url1", "mergeable": True,
|
||||
"updated_at": "2026-07-05T12:00:00Z"
|
||||
}
|
||||
]
|
||||
with patch.dict(os.environ, {"GITEA_MCP_REVEAL_ENDPOINTS": "1"}, clear=True):
|
||||
@@ -340,7 +344,11 @@ class TestViewPR(unittest.TestCase):
|
||||
"number": 1, "title": "PR 1", "state": "open",
|
||||
"head": {"ref": "branch1"}, "base": {"ref": "main"},
|
||||
"html_url": "http://url1", "mergeable": True, "body": "description",
|
||||
"user": {"login": "user1"}
|
||||
"user": {"login": "user1"},
|
||||
"updated_at": "2026-07-05T12:00:00Z",
|
||||
"merged_at": None,
|
||||
"merge_commit_sha": None,
|
||||
"closed_at": None
|
||||
}
|
||||
with patch.dict(os.environ, {}, clear=True):
|
||||
result = gitea_view_pr(pr_number=1)
|
||||
@@ -348,6 +356,10 @@ class TestViewPR(unittest.TestCase):
|
||||
self.assertEqual(result["body"], "description")
|
||||
self.assertEqual(result["user"], "user1")
|
||||
self.assertNotIn("url", result)
|
||||
# Staleness / merge fields for live reconciliation (#166) - passthrough
|
||||
self.assertEqual(result["updated_at"], "2026-07-05T12:00:00Z")
|
||||
self.assertIsNone(result["merged_at"])
|
||||
self.assertIsNone(result["merge_commit_sha"])
|
||||
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@@ -356,7 +368,8 @@ class TestViewPR(unittest.TestCase):
|
||||
"number": 1, "title": "PR 1", "state": "open",
|
||||
"head": {"ref": "branch1"}, "base": {"ref": "main"},
|
||||
"html_url": "http://url1", "mergeable": True, "body": "description",
|
||||
"user": {"login": "user1"}
|
||||
"user": {"login": "user1"},
|
||||
"updated_at": "2026-07-05T12:00:00Z", "merged_at": None, "merge_commit_sha": None, "closed_at": None
|
||||
}
|
||||
with patch.dict(os.environ, {"GITEA_MCP_REVEAL_ENDPOINTS": "1"}, clear=True):
|
||||
result = gitea_view_pr(pr_number=1)
|
||||
|
||||
@@ -56,7 +56,8 @@ class TestPRQueueInventory(unittest.TestCase):
|
||||
"mergeable": True,
|
||||
"user": {"login": "jcwalker3"},
|
||||
"labels": [{"name": "bug"}],
|
||||
"body": "Fixes #10"
|
||||
"body": "Fixes #10",
|
||||
"updated_at": "2026-07-05T10:00:00Z"
|
||||
},
|
||||
{
|
||||
"number": 2,
|
||||
@@ -67,7 +68,8 @@ class TestPRQueueInventory(unittest.TestCase):
|
||||
"mergeable": True,
|
||||
"user": {"login": "other_user"},
|
||||
"labels": [],
|
||||
"body": ""
|
||||
"body": "",
|
||||
"updated_at": "2026-07-05T11:00:00Z"
|
||||
}
|
||||
]
|
||||
mock_api.return_value = {"login": "jcwalker3"} # whoami
|
||||
@@ -80,6 +82,8 @@ class TestPRQueueInventory(unittest.TestCase):
|
||||
self.assertIn("Review/merge blocked (Self-author). Requires a genuine non-author reviewer", result["message"])
|
||||
self.assertIn("Review/merge blocked (Current profile is Author)", result["message"])
|
||||
self.assertIn("gitea_activate_profile", result["message"])
|
||||
# updated_at surfaced for reconciliation (#166)
|
||||
self.assertIn("Updated: 2026-07-05T10:00:00Z", result["message"])
|
||||
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.api_request")
|
||||
|
||||
Reference in New Issue
Block a user