fix(review): accept bounded Gitea supersession by default
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
---
|
---
|
||||||
name: gitea-code-review
|
name: gitea-code-review
|
||||||
description: Spec pass된 Gitea PR의 코드 품질과 안전성을 검토한다.
|
description: Spec pass된 Gitea PR의 코드 품질과 안전성을 검토한다.
|
||||||
version: 0.1.4
|
version: 0.1.5
|
||||||
author: genichin, Hermes Agent
|
author: genichin, Hermes Agent
|
||||||
license: MIT
|
license: MIT
|
||||||
platforms: [linux]
|
platforms: [linux]
|
||||||
@@ -130,7 +130,7 @@ Finding마다 `F-001` 같은 ID, 파일·행, 발생 조건, 영향·심각도,
|
|||||||
|
|
||||||
## Native review 게시와 재검토
|
## Native review 게시와 재검토
|
||||||
|
|
||||||
게시 단계는 [공통 Native review adapter](references/native-review.md)와 canonical `scripts/native_review.py`의 검증된 API를 사용한다. 매 실행마다 gate·게시·검증 bootstrap을 다시 생성하거나 지원하지 않는 CLI·legacy JSON schema를 추정하지 않는다. 기존 role-authenticated transport와 현재 보유한 Issue lease를 연결하고, durable journal을 유지해 timeout·partial 이후에는 POST 재시도 없이 readback만 수행한다. `provider_contract`는 확인된 Gitea 자동 supersession/retouch 및 commit `files` 순서 변화만 제한적으로 허용하며 head·본문·기존 리뷰 내용을 비교에서 제외하지 않는다. Snapshot/plan schema, 실제 지원 interface 및 read-only replay와 live 게시의 차이는 참조 문서대로 확인한다.
|
게시 단계는 [공통 Native review adapter](references/native-review.md)와 canonical `scripts/native_review.py`의 검증된 API를 사용한다. 매 실행마다 gate·게시·검증 bootstrap을 다시 생성하거나 지원하지 않는 CLI·legacy JSON schema를 추정하지 않는다. 기존 role-authenticated transport와 현재 보유한 Issue lease를 연결하고, durable journal을 유지해 timeout·partial 이후에는 POST 재시도 없이 readback만 수행한다. 정상적인 같은 작성자의 자동 supersession/retouch는 별도 opt-in 없이 기본 인정하되, exact before/after/new-review 대조로 정해진 `official`·`dismissed` 전이와 새 리뷰의 시각만 허용한다. 이전 작성자·본문·commit·판정·댓글과 다른 필드는 보존해야 하며 다른 작성자의 변경은 거부한다. `supersede_same_author`와 `retouch_dismissed`는 v1 호환 필드이지 이 정상 전이의 허가 스위치가 아니다. Commit `files` 순서 정규화만 `unordered_commit_files`와 별도 근거가 필요한 예외로 유지한다. 기존 plan의 bytes/fingerprint와 journal을 변경하지 않는다. Snapshot/plan schema, 실제 지원 interface 및 read-only replay와 live 게시의 차이는 참조 문서대로 확인한다.
|
||||||
|
|
||||||
1. Exact PR 리뷰 요청은 진입 조건을 충족한 native review 하나의 게시만 승인한다. 단순 조사·역할 보고 요청은 write를 승인하지 않는다. `approve → APPROVED`, `request_changes → REQUEST_CHANGES`로 매핑하고 `blocked`는 게시 없이 보고한다.
|
1. Exact PR 리뷰 요청은 진입 조건을 충족한 native review 하나의 게시만 승인한다. 단순 조사·역할 보고 요청은 write를 승인하지 않는다. `approve → APPROVED`, `request_changes → REQUEST_CHANGES`로 매핑하고 `blocked`는 게시 없이 보고한다.
|
||||||
2. 게시 직전 Issue 계약, PR base·head, spec pass와 review baseline을 재확인한다. Drift면 중단한다. Body에는 결과 artifact의 요약과 아래 marker를 넣는다.
|
2. 게시 직전 Issue 계약, PR base·head, spec pass와 review baseline을 재확인한다. Drift면 중단한다. Body에는 결과 artifact의 요약과 아래 marker를 넣는다.
|
||||||
|
|||||||
@@ -28,7 +28,7 @@ All fields are explicit; extra/missing fields fail closed.
|
|||||||
- `verdict`: exactly `approve` or `request_changes`. Do not normalize malformed identifiers, hashes, states or verdicts.
|
- `verdict`: exactly `approve` or `request_changes`. Do not normalize malformed identifiers, hashes, states or verdicts.
|
||||||
- `body`: finalized UTF-8 review body, beginning with the exact code-review marker and containing exactly one `- Verdict: \`<verdict>\`` line. `marker(plan)` creates the binding line:
|
- `body`: finalized UTF-8 review body, beginning with the exact code-review marker and containing exactly one `- Verdict: \`<verdict>\`` line. `marker(plan)` creates the binding line:
|
||||||
`<!-- gitea-agent:code-review/v1 owner=<owner> repository=<repository> issue=<issue> pr=<pull> head=<head> contract-sha256=<digest> -->`.
|
`<!-- gitea-agent:code-review/v1 owner=<owner> repository=<repository> issue=<issue> pr=<pull> head=<head> contract-sha256=<digest> -->`.
|
||||||
- `provider_contract`: three explicit booleans `supersede_same_author`, `retouch_dismissed`, `unordered_commit_files`, and `evidence`, a list of redacted, verifiable provider-contract/observation references. Disable exceptions lacking evidence; never set every flag simply to make a failing check pass.
|
- `provider_contract`: retain the explicit booleans `supersede_same_author`, `retouch_dismissed`, `unordered_commit_files`, and `evidence`, a list of redacted, verifiable references. The first two booleans are typed v1 compatibility fields only: normal same-author supersession/retouch is verified by the fixed readback rules below whether they are `true` or `false`. They neither grant permission nor require external evidence. Do not rewrite them in a sealed plan. `unordered_commit_files` remains an opt-in exception and requires nonempty evidence; never enable unrelated normalization merely to make a failing check pass.
|
||||||
|
|
||||||
Owner/repository currently support ASCII alphanumeric, underscore, dot and hyphen identifiers, with an alphanumeric/underscore first character. Unsupported names are rejected, not rewritten. Inline comments in a new review are not supported: the Native payload always explicitly sets `comments: []`.
|
Owner/repository currently support ASCII alphanumeric, underscore, dot and hyphen identifiers, with an alphanumeric/underscore first character. Unsupported names are rejected, not rewritten. Inline comments in a new review are not supported: the Native payload always explicitly sets `comments: []`.
|
||||||
|
|
||||||
@@ -123,7 +123,8 @@ A durable `intent.json` is fsynced **before** the POST. After timeout, partial s
|
|||||||
- Confirm precisely one new review and unchanged historical author/body/head/state/comments. No unrelated native review, comment, repository, head, branch, Issue contract or source change is silently accepted.
|
- Confirm precisely one new review and unchanged historical author/body/head/state/comments. No unrelated native review, comment, repository, head, branch, Issue contract or source change is silently accepted.
|
||||||
- Validate a source Issue repository binding when the provider includes it. A PR head may legitimately belong to a fork: the fixed base repository, exact head SHA and unchanged complete PR metadata bind it, rather than incorrectly requiring every head repository to equal the base.
|
- Validate a source Issue repository binding when the provider includes it. A PR head may legitimately belong to a fork: the fixed base repository, exact head SHA and unchanged complete PR metadata bind it, rather than incorrectly requiring every head repository to equal the base.
|
||||||
- `official` is a provider merge-policy flag, not an additional verdict or role-identity requirement. Current accepted entry/publication rules require canonical role/head/contract/state and nondismissed/nonstale status; they do not require `official=True`. Changing that policy requires an explicit acceptance-criteria change.
|
- `official` is a provider merge-policy flag, not an additional verdict or role-identity requirement. Current accepted entry/publication rules require canonical role/head/contract/state and nondismissed/nonstale status; they do not require `official=True`. Changing that policy requires an explicit acceptance-criteria change.
|
||||||
- When explicitly supported by provider evidence, same-author official-review supersession or retouching an already-dismissed same-author formal review may change only `official`, `dismissed`, `updated_at`. Require the specific expected false/true flag transitions and a timestamp equal to this exact new review's submitted/updated timestamp. This is not a global mutable-field mask. Timestamp/pattern agreement is provider-contract consistency evidence, not proof of a unique server-side causal transaction; a coincident manual mutation matching the full permitted pattern cannot be distinguished without additional authoritative event evidence.
|
- Normal same-author formal-review supersession is accepted by default, not rejected for a missing opt-in flag: an old `official=true, dismissed=false` review may become `official=false, dismissed=true`; an already-dismissed `official=false, dismissed=true` review may retain those flags with an updated timestamp. In both cases, only `official`, `dismissed`, `updated_at` may differ; the timestamp must equal this exact new review's submitted/updated timestamp. Preserve the old author, body, commit, verdict, submitted time, comments and every other field, and reject other-author changes or unexpected flag patterns. The complete before/after/exact readback is the evidence for this bounded transition; `supersede_same_author` and `retouch_dismissed` do not disable it. This is not a global mutable-field mask. Timestamp/pattern agreement is consistency evidence, not proof of a unique server-side causal transaction; a coincident manual mutation matching the full permitted pattern cannot be distinguished without additional authoritative event evidence.
|
||||||
|
- Existing v1 plans keep their original bytes and fingerprint. A retained uncertain attempt may be reconciled through the existing same-plan, held-lease readback path under the current verifier; never change the plan, delete the journal, or retry POST to accommodate supersession. A saved-wire replay is read-only evidence and does not itself finalize a live journal.
|
||||||
- Only commit `files` arrays are conditionally sorted by unique filename. File record contents, commit order, parent order and other arrays are still compared without blanket normalization. Changed-file records are compared by unique filename.
|
- Only commit `files` arrays are conditionally sorted by unique filename. File record contents, commit order, parent order and other arrays are still compared without blanket normalization. Changed-file records are compared by unique filename.
|
||||||
- Source Issue/PR timestamp, PR review-count and requested-reviewer changes are narrowly tied to the exact added review; unrelated derived-state mutation fails.
|
- Source Issue/PR timestamp, PR review-count and requested-reviewer changes are narrowly tied to the exact added review; unrelated derived-state mutation fails.
|
||||||
- The CLI is **saved-artifact replay**, not fresh network verification or approval authority. Its `read_only: true` output must not be presented as a newly published review. In live results `wrote` refers to a Native POST by that invocation; local journal writes still occur for no-ops.
|
- The CLI is **saved-artifact replay**, not fresh network verification or approval authority. Its `read_only: true` output must not be presented as a newly published review. In live results `wrote` refers to a Native POST by that invocation; local journal writes still occur for no-ops.
|
||||||
|
|||||||
@@ -369,7 +369,10 @@ def validate_plan(plan):
|
|||||||
fail('invalid provider projection flags')
|
fail('invalid provider projection flags')
|
||||||
if any(type(x) is not str or not x.strip() for x in provider['evidence']):
|
if any(type(x) is not str or not x.strip() for x in provider['evidence']):
|
||||||
fail('invalid provider evidence')
|
fail('invalid provider evidence')
|
||||||
if any(flags) and not provider['evidence']:
|
# Replacement booleans remain typed v1 compatibility fields, not permissions.
|
||||||
|
# Normal same-author metadata transitions are verified from before/after/exact.
|
||||||
|
# File-order normalization is still a separate, evidence-backed exception.
|
||||||
|
if provider['unordered_commit_files'] and not provider['evidence']:
|
||||||
fail('provider projection requires evidence')
|
fail('provider projection requires evidence')
|
||||||
return plan
|
return plan
|
||||||
|
|
||||||
@@ -613,9 +616,11 @@ def verify_publication(plan, before, after, exact):
|
|||||||
or set(review) != set(fresh)
|
or set(review) != set(fresh)
|
||||||
or canonical({k: v for k, v in review.items() if k not in permitted}) != canonical({k: v for k, v in fresh.items() if k not in permitted})):
|
or canonical({k: v for k, v in review.items() if k not in permitted}) != canonical({k: v for k, v in fresh.items() if k not in permitted})):
|
||||||
fail('historical review immutable content changed')
|
fail('historical review immutable content changed')
|
||||||
provider = plan['provider_contract']
|
# Normal same-author Gitea replacement is part of the readback contract,
|
||||||
superseded = provider['supersede_same_author'] and review.get('official') is True and review.get('dismissed') is False
|
# not an opt-in exception. Immutable fields/comments were checked above;
|
||||||
retouched = provider['retouch_dismissed'] and review.get('official') is False and review.get('dismissed') is True
|
# only these exact flag transitions and the new review timestamp qualify.
|
||||||
|
superseded = review.get('official') is True and review.get('dismissed') is False
|
||||||
|
retouched = review.get('official') is False and review.get('dismissed') is True
|
||||||
if (not (superseded or retouched) or fresh.get('official') is not False or fresh.get('dismissed') is not True
|
if (not (superseded or retouched) or fresh.get('official') is not False or fresh.get('dismissed') is not True
|
||||||
or fresh.get('updated_at') not in {exact['submitted_at'], exact['updated_at']}):
|
or fresh.get('updated_at') not in {exact['submitted_at'], exact['updated_at']}):
|
||||||
fail('unauthorized historical review projection')
|
fail('unauthorized historical review projection')
|
||||||
|
|||||||
@@ -518,13 +518,108 @@ class NativeReviewTests(unittest.TestCase):
|
|||||||
p, before, after, exact = publication_fixture(); change(before)
|
p, before, after, exact = publication_fixture(); change(before)
|
||||||
with self.assertRaises(m.ValidationError): m.validate_snapshot(p, before)
|
with self.assertRaises(m.ValidationError): m.validate_snapshot(p, before)
|
||||||
|
|
||||||
|
def test_normal_supersession_is_allowed_without_projection_opt_in(self):
|
||||||
|
for already_dismissed in (False, True):
|
||||||
|
with self.subTest(already_dismissed=already_dismissed):
|
||||||
|
m = self.module()
|
||||||
|
p, before, after, exact = publication_fixture()
|
||||||
|
p['provider_contract'].update(supersede_same_author=False,
|
||||||
|
retouch_dismissed=False,
|
||||||
|
unordered_commit_files=False, evidence=[])
|
||||||
|
if already_dismissed:
|
||||||
|
before['reviews'][0].update(official=False, dismissed=True)
|
||||||
|
after['reviews'][0].update(official=False, dismissed=True,
|
||||||
|
updated_at=exact['submitted_at'])
|
||||||
|
saved_plan = copy.deepcopy(p)
|
||||||
|
saved_before = copy.deepcopy(before)
|
||||||
|
try:
|
||||||
|
result = m.verify_publication(p, before, after, exact)
|
||||||
|
except m.ValidationError as exc:
|
||||||
|
self.fail(f'normal same-author replacement was rejected: {exc}')
|
||||||
|
self.assertTrue(result['verified'])
|
||||||
|
self.assertEqual(result['projected_review_ids'], [8])
|
||||||
|
self.assertEqual(p, saved_plan)
|
||||||
|
self.assertEqual(before, saved_before)
|
||||||
|
|
||||||
|
def test_legacy_replacement_flags_do_not_require_external_evidence(self):
|
||||||
|
m = self.module()
|
||||||
|
p, before, after, exact = publication_fixture()
|
||||||
|
p['provider_contract'].update(unordered_commit_files=False, evidence=[])
|
||||||
|
after['reviews'][0].update(official=False, dismissed=True,
|
||||||
|
updated_at=exact['submitted_at'])
|
||||||
|
try:
|
||||||
|
result = m.verify_publication(p, before, after, exact)
|
||||||
|
except m.ValidationError as exc:
|
||||||
|
self.fail(f'compatibility-only replacement flags required an opt-in: {exc}')
|
||||||
|
self.assertTrue(result['verified'])
|
||||||
|
self.assertEqual(m.build_payload(p)['body'], p['body'])
|
||||||
|
# Unordered file comparisons remain a separate, evidence-backed exception.
|
||||||
|
p['provider_contract']['unordered_commit_files'] = True
|
||||||
|
with self.assertRaisesRegex(m.ValidationError, 'provider projection requires evidence'):
|
||||||
|
m.validate_plan(p)
|
||||||
|
|
||||||
|
def test_normal_replacement_does_not_allow_historical_content_mutation(self):
|
||||||
|
m = self.module()
|
||||||
|
for field, value in [('body', 'altered'), ('commit_id', 'e' * 40),
|
||||||
|
('state', 'APPROVED'), ('user', {'id': 72, 'login': 'other'}),
|
||||||
|
('submitted_at', '2026-10-04T00:03:00Z'),
|
||||||
|
('official', True), ('dismissed', False),
|
||||||
|
('updated_at', '2026-10-04T00:03:00Z')]:
|
||||||
|
with self.subTest(field=field):
|
||||||
|
p, before, after, exact = publication_fixture()
|
||||||
|
p['provider_contract'].update(supersede_same_author=False,
|
||||||
|
retouch_dismissed=False,
|
||||||
|
unordered_commit_files=False, evidence=[])
|
||||||
|
after['reviews'][0].update(official=False, dismissed=True,
|
||||||
|
updated_at=exact['submitted_at'])
|
||||||
|
after['reviews'][0][field] = value
|
||||||
|
with self.assertRaises(m.ValidationError):
|
||||||
|
m.verify_publication(p, before, after, exact)
|
||||||
|
p, before, after, exact = publication_fixture()
|
||||||
|
p['provider_contract'].update(supersede_same_author=False, retouch_dismissed=False,
|
||||||
|
unordered_commit_files=False, evidence=[])
|
||||||
|
after['reviews'][0].update(official=False, dismissed=True, updated_at=exact['submitted_at'])
|
||||||
|
for snap in (before, after):
|
||||||
|
snap['review_comments']['8'] = [{'id': 71, 'body': 'original'}]
|
||||||
|
snap['reviews'][0]['comments_count'] = 1
|
||||||
|
snap['pagination']['review_comments_8'][0].update(count=1, declared_total=1)
|
||||||
|
after['review_comments']['8'][0]['body'] = 'altered'
|
||||||
|
with self.assertRaisesRegex(m.ValidationError, 'historical review comments changed'):
|
||||||
|
m.verify_publication(p, before, after, exact)
|
||||||
|
|
||||||
|
def test_normal_replacement_reconciles_original_intent_without_second_post(self):
|
||||||
|
m = self.module()
|
||||||
|
p, before, after, exact = publication_fixture()
|
||||||
|
p['provider_contract'].update(supersede_same_author=False, retouch_dismissed=False,
|
||||||
|
unordered_commit_files=False, evidence=[])
|
||||||
|
after['reviews'][0].update(official=False, dismissed=True, updated_at=exact['submitted_at'])
|
||||||
|
api = FakeAPI(p, before, after, exact)
|
||||||
|
original_read = api.read_review
|
||||||
|
api.read_review = lambda target, rid: (_ for _ in ()).throw(TimeoutError('unavailable'))
|
||||||
|
with self.assertRaises(m.ValidationError):
|
||||||
|
m.publish(p, before, api, FakeLease(), self.canonical(m, p), declaration={'mode': 'none'})
|
||||||
|
self.assertEqual(api.posts, 1)
|
||||||
|
journal = self.canonical(m, p)
|
||||||
|
intent = (journal / 'intent.json').read_bytes()
|
||||||
|
self.assertFalse((journal / 'verified.json').exists())
|
||||||
|
api.read_review = original_read
|
||||||
|
result = m.publish(p, before, api, FakeLease(), journal, declaration={'mode': 'none'})
|
||||||
|
self.assertTrue(result['verified'])
|
||||||
|
self.assertFalse(result['wrote'])
|
||||||
|
self.assertEqual(result['projected_review_ids'], [8])
|
||||||
|
self.assertEqual(api.posts, 1)
|
||||||
|
self.assertEqual((journal / 'intent.json').read_bytes(), intent)
|
||||||
|
self.assertEqual(json.loads(intent)['fingerprint'],
|
||||||
|
__import__('hashlib').sha256(m.canonical(p)).hexdigest())
|
||||||
|
self.assertTrue(json.loads((journal / 'verified.json').read_text())['verified'])
|
||||||
|
|
||||||
def test_provider_projection_requires_explicit_bound_evidence(self):
|
def test_provider_projection_requires_explicit_bound_evidence(self):
|
||||||
m = self.module()
|
m = self.module()
|
||||||
for kind in ['disabled', 'wrong_timestamp', 'wrong_author']:
|
for kind in ['wrong_initial_flags', 'wrong_timestamp', 'wrong_author']:
|
||||||
with self.subTest(kind=kind):
|
with self.subTest(kind=kind):
|
||||||
p, before, after, exact = publication_fixture()
|
p, before, after, exact = publication_fixture()
|
||||||
after['reviews'][0].update(official=False, dismissed=True, updated_at=exact['submitted_at'])
|
after['reviews'][0].update(official=False, dismissed=True, updated_at=exact['submitted_at'])
|
||||||
if kind == 'disabled': p['provider_contract']['supersede_same_author'] = False
|
if kind == 'wrong_initial_flags': before['reviews'][0].update(official=True, dismissed=True)
|
||||||
elif kind == 'wrong_timestamp': after['reviews'][0]['updated_at'] = '2026-10-04T00:02:00Z'
|
elif kind == 'wrong_timestamp': after['reviews'][0]['updated_at'] = '2026-10-04T00:02:00Z'
|
||||||
else:
|
else:
|
||||||
before['reviews'][0]['user'] = {'id': 72, 'login': 'other'}
|
before['reviews'][0]['user'] = {'id': 72, 'login': 'other'}
|
||||||
|
|||||||
Reference in New Issue
Block a user