From 62d2262b274bfaa07906a9be3d52ae476730e721 Mon Sep 17 00:00:00 2001 From: gitea-spec-agent Date: Mon, 12 Oct 2026 07:51:32 +0000 Subject: [PATCH] [verified] Document empty review-comments readback without count header --- .../references/native-review.md | 6 ++ harness/tests/review/test_native_review.py | 81 +++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/harness/agents/review/skills/gitea-code-review/references/native-review.md b/harness/agents/review/skills/gitea-code-review/references/native-review.md index cb4e144..ef4e433 100644 --- a/harness/agents/review/skills/gitea-code-review/references/native-review.md +++ b/harness/agents/review/skills/gitea-code-review/references/native-review.md @@ -50,6 +50,12 @@ The request callback is `authenticated_request(relative_api_path, payload=None) `ApiAdapter.snapshot(target)` collects repository/Issue/PR metadata, identity/spec identity, all changed files, commits, reviews, historical review comments, Issue/PR comments and head statuses. Use this same collector for the baseline and subsequent snapshots. Every collection records contiguous 50-item pages, declared totals and Link headers; duplicates, inconsistent totals and truncated enumeration fail closed. A saved older collector snapshot may be replayed if it provides these complete facts, but differently shaped collectors must not be mixed for live source-drift comparison. +### Empty review-comments readback without a count header + +For `GET /repos/{owner}/{repository}/pulls/{pull}/reviews/{review_id}/comments`, a successful direct JSON `[]` response without `X-Total-Count` is valid evidence of zero comments only when the exact review has integer `comments_count=0` and the complete collection agrees. Preserve the actual pagination evidence as `declared_total=None`; never fabricate a zero header or infer an empty response from an unavailable/malformed response or a missing/null count. Use `ApiAdapter.snapshot` for baseline and subsequent collections and `verify_publication` with the exact review readback. + +This is not a general pagination bypass: preserve contiguous pages, identity uniqueness, observed totals and Link headers. A `rel=next` continuation must be followed to completion; a final page still advertising next, an unresolved full 50-item page without a total, conflicting totals, count mismatch or unexpected inline comments blocks verification. Endpoints supplying `X-Total-Count` retain total/cardinality checks. A readback failure is not authorization for POST blind retry: retain the same journal and reconcile the uncertain attempt through authoritative reads. + ### Broker response preflight before publication Use the [operation-specific broker response contract](../../../../../operations/repository-cache.md#operation-specific-broker-responses). `renew` supplies only `status`/`deadline`; read `repository_held` and identity from a separate `status` request. Python uses `ttl=`, not the wire/CLI name `ttl_seconds`. Keep the admitted handle and the retained canonical `harness/lib/managed_lock_broker.py` module as `broker`; do not read its token or load another profile's helper. diff --git a/harness/tests/review/test_native_review.py b/harness/tests/review/test_native_review.py index 9fbfb73..21b4946 100644 --- a/harness/tests/review/test_native_review.py +++ b/harness/tests/review/test_native_review.py @@ -189,6 +189,87 @@ class NativeReviewTests(unittest.TestCase): m.publish(p, before, api, lease, self.canonical(m, p), declaration={'mode': 'none'}) self.assertNotIn('secret transport detail', str(error.exception)) self.assertEqual(api.posts, 1) + def test_empty_review_comments_no_header_contract_is_explicit(self): + text = (ROOT / 'harness/agents/review/skills/gitea-code-review/references/native-review.md').read_text() + self.assertIn('### Empty review-comments readback without a count header', text) + section = text.split('### Empty review-comments readback without a count header', 1)[1].split('\n### ', 1)[0] + for rule in ['X-Total-Count', 'comments_count=0', 'JSON `[]`', + 'declared_total=None', 'rel=next', 'blind retry']: + self.assertIn(rule, section) + + def test_api_adapter_empty_comments_without_header_verifies_publication(self): + m = self.module() + p, before, after, exact = publication_fixture() + + def collect(source, comments=None, headers=None): + calls = [] + def request(path, payload=None): + self.assertIsNone(payload, 'readback regression must be GET-only') + calls.append(path) + direct = {'/user': p['reviewer'], '/users/gitea-spec-agent': p['spec_reviewer'], + '/repos/acme/repo': source['repository'], + '/repos/acme/repo/issues/3': source['issue'], + '/repos/acme/repo/pulls/7': source['pr'], + '/repos/acme/repo/pulls/7/reviews/10': exact} + if path in direct: + return direct[path], {}, 200 + from urllib.parse import urlsplit + endpoint = urlsplit(path).path + for name, suffix in [('files', '/pulls/7/files'), ('commits', '/pulls/7/commits'), + ('reviews', '/pulls/7/reviews'), ('issue_comments', '/issues/3/comments'), + ('pr_comments', '/issues/7/comments'), + ('statuses', '/statuses/' + p['target']['head'])]: + if endpoint == '/repos/acme/repo' + suffix: + return copy.deepcopy(source[name]), {'X-Total-Count': str(len(source[name]))}, 200 + if '/reviews/' in endpoint and endpoint.endswith('/comments'): + return ([] if comments is None else comments), ({} if headers is None else headers), 200 + raise AssertionError('unexpected synthetic readback endpoint') + api = m.ApiAdapter(request, origin=m.ORIGIN + '/api/v1', + credential_source='$HERMES_HOME/.env:GITEA_TOKEN') + snapshot = api.snapshot(p['target']) + self.assertEqual(calls[0], '/user') + self.assertTrue(any('/reviews/' in path and '/comments?' in path for path in calls)) + return api, snapshot + + _, initial = collect(before) + api, final = collect(after) + m.validate_snapshot(p, initial) + m.validate_snapshot(p, final) + authoritative = api.read_review(p['target'], exact['id']) + m.verify_publication(p, initial, final, authoritative) + for snapshot in [initial, final]: + for rid in snapshot['review_comments']: + self.assertEqual(snapshot['review_comments'][rid], []) + self.assertEqual(snapshot['pagination']['review_comments_' + rid], + [{'page': 1, 'count': 0, 'declared_total': None, 'link': None}]) + + # The admitted control above distinguishes intentional rejection from + # a broken fixture. Missing headers never waive the review count or + # the complete-collection and newly-created no-inline-comment checks. + for count in [1, None, True]: + with self.subTest(count=count): + drift = copy.deepcopy(final) + drift['reviews'][-1]['comments_count'] = count + with self.assertRaisesRegex(m.ValidationError, 'review comment cardinality mismatch'): + m.validate_snapshot(p, drift) + _, unexpected = collect(after, comments=[{'id': 999}]) + for review in unexpected['reviews']: + review['comments_count'] = 1 + m.validate_snapshot(p, unexpected) + exact_with_inline = copy.deepcopy(authoritative) + exact_with_inline['comments_count'] = 1 + with self.assertRaisesRegex(m.ValidationError, 'exact review binding mismatch'): + m.verify_publication(p, initial, unexpected, exact_with_inline) + with self.assertRaisesRegex(m.ValidationError, 'truncated pages'): + collect(after, headers={'X-Total-Count': '1'}) + truncated = copy.deepcopy(final) + truncated['pagination']['review_comments_' + str(exact['id'])][0]['link'] = '; rel=next' + with self.assertRaisesRegex(m.ValidationError, 'truncated pagination'): + m.validate_snapshot(p, truncated) + with self.assertRaisesRegex(m.ValidationError, 'truncated pagination'): + m.check_pages([{'id': i + 1} for i in range(50)], + [{'page': 1, 'count': 50, 'declared_total': None, 'link': None}]) + def test_api_adapter_checks_identity_first_and_collects_all_pages(self): m = self.module() self.assertTrue(callable(getattr(m, 'ApiAdapter', None)), 'REST bridge is missing')