[verified] Document empty review-comments readback without count header
This commit is contained in:
@@ -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.
|
`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
|
### 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.
|
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.
|
||||||
|
|||||||
@@ -189,6 +189,87 @@ class NativeReviewTests(unittest.TestCase):
|
|||||||
m.publish(p, before, api, lease, self.canonical(m, p), declaration={'mode': 'none'})
|
m.publish(p, before, api, lease, self.canonical(m, p), declaration={'mode': 'none'})
|
||||||
self.assertNotIn('secret transport detail', str(error.exception))
|
self.assertNotIn('secret transport detail', str(error.exception))
|
||||||
self.assertEqual(api.posts, 1)
|
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'] = '<next>; 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):
|
def test_api_adapter_checks_identity_first_and_collects_all_pages(self):
|
||||||
m = self.module()
|
m = self.module()
|
||||||
self.assertTrue(callable(getattr(m, 'ApiAdapter', None)), 'REST bridge is missing')
|
self.assertTrue(callable(getattr(m, 'ApiAdapter', None)), 'REST bridge is missing')
|
||||||
|
|||||||
Reference in New Issue
Block a user