diff --git a/.github/repo-baseline.json b/.github/repo-baseline.json index 03f076e..02f1d05 100644 --- a/.github/repo-baseline.json +++ b/.github/repo-baseline.json @@ -23,15 +23,15 @@ }, "scripts/check_review_status.py": { "source": "scripts/check_review_status.py", - "sha256": "16d7953bb342ecca655601dd0b34bdf513c1546f151f32b031495304095a9f8e" + "sha256": "256b28db2301b4e8e92a7eda812f498425a5a6b080ac6c941dbb69dd98ccc5eb" }, "scripts/source-only/test_review_status.py": { "source": "scripts/test_review_status.py", - "sha256": "e31ce97852424e8ce44d88ff7fa4bb2e54301d997eb7ea206607c44d07f29b0d" + "sha256": "703cc77fb484402df8522b68f54317f660163029838e5b5f744b94a4b713453b" }, "scripts/rerun_review_status.py": { "source": "scripts/rerun_review_status.py", - "sha256": "a1ed0736a9ade00cd43e63774fe4deae7acf8b1e95c7f871b176abd06237353e" + "sha256": "f72be336e122e2cea6d0c3325afb6e8baf90d7b0799bea5b0f7465a5333814d1" }, "scripts/workflow_policy.py": { "source": "scripts/workflow_policy.py", @@ -75,5 +75,5 @@ } }, "source": "minjunkim-dev/repo-baseline", - "source_commit": "c6405a09a7d06af943cb08db5b600fe0f655b0a2" + "source_commit": "9a3920b942695257772f25083e8a4fde5d4e3208" } diff --git a/.github/repository-settings.json b/.github/repository-settings.json index 5e8db5f..5aff3e0 100644 --- a/.github/repository-settings.json +++ b/.github/repository-settings.json @@ -34,6 +34,10 @@ { "context": "CI", "app_id": 15368 + }, + { + "context": "Review status", + "app_id": 15368 } ] }, diff --git a/.github/workflows/review-status.yml b/.github/workflows/review-status.yml index 0c0b3a4..13314a3 100644 --- a/.github/workflows/review-status.yml +++ b/.github/workflows/review-status.yml @@ -14,6 +14,10 @@ name: Review status # request, and only a comment whose author is the repository owner (github.repository_owner, the same login # the checker compares). An organization name is not a user, so a member comment does not rerun and is not an # approval. Deleting that comment still reruns, because the payload's comment author stays the owner. +# Editing it still reruns for the same reason: comment.user stays the author, so another account's +# edit of the owner's comment also reruns. The checker counts a comment only when updated_at equals +# created_at. An edited comment is not approval; post a new comment. Skipping edited would leave +# the previous success green. The job does not read the comment body. # The run is chosen by its own head_sha. pull_requests[].head.sha can change to the current head and point at an older run. # If that run cannot be listed, the job posts a failing check named Review status on the current head. # It does not write review/independent or review/security. review-status-policy.json allows only these two events. @@ -78,6 +82,7 @@ jobs: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.issue.number }} + COMMENT_ACTION: ${{ github.event.action }} run: | set -euo pipefail for _ in 1 2 3 4 5 6; do diff --git a/AGENTS.md b/AGENTS.md index 8f5291e..733b5d6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ Single-context — `CONTEXT.md` and `docs/adr/` at the repo root. See `docs/agen Follow `docs/agents/review.md` for independent review, direct fixes, final-head verification, and merge risk. Reviewers read `docs/coding-standards.md`; do not inline the standards here. -The agent merges its own easy-to-reverse pull request (`Door: 되돌리기 쉬움`) after the ticket's verification, independent review, and required `CI` check pass on the final head. Hard-to-reverse (`Door: 되돌리기 어려움`) or broad-impact changes require human approval of that final head. Green CI does not replace measurements required by the ticket. 이 저장소의 검사기가 기본 브랜치에 다시 적용되기 전(이 PR 자신 포함)에는 본문 `Door:`와 소유자 승인 댓글에 옛 형식 `양쪽 문`/`한쪽 문`, `한쪽 문 승인: `를 쓴다. +The agent merges its own easy-to-reverse pull request (`Door: 되돌리기 쉬움`) after the ticket's verification, independent review, and the required checks on the final head. `.github/repository-settings.json` records `CI` and `Review status` (`app_id` 15368, `strict: true`). Until the owner applies that record from a Mac, the enforced check is `CI`. After the apply, every pull request except a Dependabot version-only pull request also needs an owner-posted `review/independent` status, and `review/security` when the diff touches the trust boundary. The agent does not post those statuses. Hard-to-reverse (`Door: 되돌리기 어려움`) or broad-impact changes require human approval of that final head. Green CI does not replace measurements required by the ticket. `Door:` 값은 `되돌리기 쉬움` 또는 `되돌리기 어려움`이다. 옛 `양쪽 문`/`한쪽 문`과 `한쪽 문 승인: `는 deprecated이고 검사기가 같은 의미로 받는다. ### 작업 진입 diff --git a/docs/REPOSITORY_SETUP.md b/docs/REPOSITORY_SETUP.md index d039f2b..ed233ad 100644 --- a/docs/REPOSITORY_SETUP.md +++ b/docs/REPOSITORY_SETUP.md @@ -27,7 +27,7 @@ PR의 source-only 검사는 codeql.yml blob과 `codeql_main_source.sha`를 비 | Allow auto-merge | On. 일괄 예약 없음. 검증 후 exact-head 직접 병합 | | main 보호 | PR 필수·strict·관리자 적용·선형 이력·대화 해결 On | | 승인 | GitHub 0명. 되돌리기 어려움/broad 최종 head는 사람 승인 | -| 필수 체크 | 기존 `CI`, GitHub Actions App `15368` | +| 필수 체크 | 기대값 `CI`, `Review status`. 둘 다 GitHub Actions App `15368`. `strict`는 true. 서버에 적용되기 전 보호가 강제하는 체크는 `CI`다. 적용 뒤 Dependabot 버전 전용 PR을 뺀 모든 PR에 소유자의 `review/independent`가 필요하다 | | force push·main 삭제 | Off | | Actions | GitHub 공식만, 전체 SHA 필수, 기본 token read, PR 생성·승인 Off | | fork | 모든 외부 기여자 승인. Secret과 쓰기 token 분리 | @@ -64,13 +64,13 @@ Copilot MCP tools·PR 승인과 추가 자동 AI 리뷰를 켜지 않는다. Cha ## 공통 기준 갱신과 설정 관측 기록 -적용 원본은 `minjunkim-dev/repo-baseline`의 main `c6405a09a7d06af943cb08db5b600fe0f655b0a2`다. 복사본의 경로·생태계를 유지하며 원본 도구로 갱신했다. 기존 테스트 대상 경로는 `scripts/source-only/`다. `scripts/test_workflow_policy.py`만 원본과 같은 `scripts/`에 둔다. 그 테스트가 자기 위치의 부모를 저장소 루트로 읽기 때문이다. Dependabot `ecosystems`는 빈 목록이다. `.claude/settings.json`은 적용 시점의 원본 템플릿과 바이트가 같다. 해시 manifest에는 없다. 기존 파일을 합치지 않았다. `templates/review-status.yml`은 별도 파일 `.github/workflows/review-status.yml`로 다시 복사했다. `templates/pr-policy.yml`은 `.github/workflows/pr-policy.yml`로 다시 복사했다. 둘 다 해시 대상이 아니고 pin은 템플릿과 같다. 다른 workflow와 합치지 않았다. 두 workflow의 `branches`는 `main`이다. `review-status.yml`의 이벤트는 `pull_request_target`과 `issue_comment`뿐이다. `pr-policy.yml`은 `pull_request`만 쓴다. `Door:` 값은 `되돌리기 쉬움` 또는 `되돌리기 어려움`이다. 옛 `양쪽 문`·`한쪽 문`과 옛 `한쪽 문 승인: `는 deprecated로 계속 인정한다. `.github/workflow-policy.json`은 원본 저장소 파일을 복사하지 않는다. `require_agents_md`는 기본값 true다. `reject_claude_md`는 false다. `CLAUDE.md`는 `@AGENTS.md` 한 줄이라 지우지 않는다. `feature_flags`는 false다. 이 저장소에 flag 레지스트리가 없다. 봇 브랜치 접두사는 기본값 `claude`·`codex`·`dependabot`이다. `cursor`는 더하지 않는다. 로컬 스킬 설정과 일상 작업 진입은 [개발 절차](agents/development.md), 자체 리뷰와 사용자 요청 회고는 [리뷰 절차](agents/review.md)를 따른다. 공통 기준이 더하는 `main` 경계는 원본 [개발 절차](https://github.com/minjunkim-dev/repo-baseline/blob/c6405a09a7d06af943cb08db5b600fe0f655b0a2/docs/development-workflow.md)를 따른다. +적용 원본은 `minjunkim-dev/repo-baseline`의 main `9a3920b942695257772f25083e8a4fde5d4e3208`다. 복사본의 경로·생태계를 유지하며 `tools/apply_baseline.py`로 갱신했다. 기존 테스트 대상 경로는 `scripts/source-only/`다. `scripts/test_workflow_policy.py`만 원본과 같은 `scripts/`에 둔다. 그 테스트가 자기 위치의 부모를 저장소 루트로 읽기 때문이다. Dependabot `ecosystems`는 빈 목록이다. `.claude/settings.json`은 적용 시점의 원본 템플릿과 바이트가 같다. 해시 manifest에는 없다. 기존 파일을 합치지 않았다. `templates/review-status.yml`은 별도 파일 `.github/workflows/review-status.yml`로 다시 복사했다. `templates/pr-policy.yml`은 `.github/workflows/pr-policy.yml`과 바이트가 같다. 둘 다 해시 대상이 아니고 pin은 템플릿과 같다. 다른 workflow와 합치지 않았다. 두 workflow의 `branches`는 `main`이다. `review-status.yml`의 이벤트는 `pull_request_target`과 `issue_comment`뿐이다. `pr-policy.yml`은 `pull_request`만 쓴다. `Door:` 값은 `되돌리기 쉬움` 또는 `되돌리기 어려움`이다. 옛 `양쪽 문`·`한쪽 문`과 옛 `한쪽 문 승인: `는 deprecated로 계속 인정한다. 1인 프로필에서 `review/independent`와 `review/security`는 `creator.login`이 저장소 소유자이거나 `scripts/check_review_status.py`의 `STATUS_ACTORS`에 있을 때만 success다. `STATUS_ACTORS`는 비어 있다. 다른 작성자의 status는 실패다. 승인 댓글은 `updated_at`이 `created_at`과 같은 문자열이고 GraphQL `lastEditedAt`이 null이며 `includesCreatedEdit`가 아닐 때만 인정한다. 편집한 댓글은 무효이고 새 댓글을 단다. 이어받기 해시는 diff와 바뀐 파일의 변경 후 blob id다. `.github/workflow-policy.json`은 원본 저장소 파일을 복사하지 않는다. `require_agents_md`는 기본값 true다. `reject_claude_md`는 false다. `CLAUDE.md`는 `@AGENTS.md` 한 줄이라 지우지 않는다. `feature_flags`는 false다. 이 저장소에 flag 레지스트리가 없다. 봇 브랜치 접두사는 기본값 `claude`·`codex`·`dependabot`이다. `cursor`는 더하지 않는다. 로컬 스킬 설정과 일상 작업 진입은 [개발 절차](agents/development.md), 자체 리뷰와 사용자 요청 회고는 [리뷰 절차](agents/review.md)를 따른다. 공통 기준이 더하는 `main` 경계는 원본 [개발 절차](https://github.com/minjunkim-dev/repo-baseline/blob/9a3920b942695257772f25083e8a4fde5d4e3208/docs/development-workflow.md)를 따른다. -기존 기준선의 설명용 `exceptions` 배열은 내용을 보존해 `observations`로 옮겼다. 관측 기록은 API 감사 항목을 제외하지 않는다. 실제 기능 미지원·비해당을 감사 범위에서 제외하려면 원본의 [설정 기준선 형식](https://github.com/minjunkim-dev/repo-baseline/blob/c6405a09a7d06af943cb08db5b600fe0f655b0a2/docs/github-settings-profile.md)에 맞는 `exceptions` 객체와 사유·근거가 필요하다. 예외가 있으면 exit 0이어도 결과는 전체 PASS가 아닌 LIMITED다. 미선언 403·404·누락·권한 오류는 계속 실패한다. +기존 기준선의 설명용 `exceptions` 배열은 내용을 보존해 `observations`로 옮겼다. 관측 기록은 API 감사 항목을 제외하지 않는다. 실제 기능 미지원·비해당을 감사 범위에서 제외하려면 원본의 [설정 기준선 형식](https://github.com/minjunkim-dev/repo-baseline/blob/9a3920b942695257772f25083e8a4fde5d4e3208/docs/github-settings-profile.md)에 맞는 `exceptions` 객체와 사유·근거가 필요하다. 예외가 있으면 exit 0이어도 결과는 전체 PASS가 아닌 LIMITED다. 미선언 403·404·누락·권한 오류는 계속 실패한다. -`Review status`는 이미 기본 브랜치에 있다. 이번 갱신은 그 파일과 PR policy workflow를 템플릿으로 다시 복사한다. `pull_request_target`과 `pull_request`는 `branches: [main]`만 본다. 검사기는 PR base가 저장소 기본 브랜치가 아니거나 이벤트 base SHA가 그 브랜치 위에 없으면 실패한다. 실패 이름은 `review/base-branch`다. 사이드 base의 소유자 댓글은 체크를 게시하지 않는다. 이벤트 종류는 그대로다. 댓글 재실행 job의 `actions: write`와 `checks: write`는 이전 적용에서 이미 있다. Actions 이벤트 정책은 만들거나 고치지 않는다. 목록 조회가 실패하면 정책이 없는 것으로 보지 않는다. 필수 체크 `Review status`는 등록하지 않는다. +`Review status`는 기본 브랜치에 있다. 이번 갱신은 `templates/review-status.yml`을 다시 복사한다. 주석은 편집된 소유자 댓글도 재실행되고, 검사기가 그 댓글을 승인으로 세지 않는다고 적는다. 댓글 재실행 step은 `COMMENT_ACTION`을 넘긴다. `pull_request_target`과 `pull_request`는 `branches: [main]`만 본다. 검사기는 PR base가 저장소 기본 브랜치가 아니거나 이벤트 base SHA가 그 브랜치 위에 없으면 실패한다. 실패 이름은 `review/base-branch`다. 사이드 base의 소유자 댓글은 체크를 게시하지 않는다. 이벤트 종류는 그대로다. 댓글 재실행 job의 `actions: write`와 `checks: write`는 이전 적용에서 이미 있다. Actions 이벤트 정책은 만들거나 고치지 않는다. 목록 조회가 실패하면 정책이 없는 것으로 보지 않는다. job 이름 `Review status`는 기본 브랜치 workflow에서 이미 성공했다(run `42016041442`). `.github/repository-settings.json`의 `required_status_checks.checks`에 `{"context": "Review status", "app_id": 15368}`을 더했다. `strict`는 true다. 기존 `CI`는 그대로다. context는 `.github/workflows/review-status.yml`의 job `name`과 같다. 이 파일은 기대값이다. 이 변경은 branch protection을 쓰지 않는다. 병합 뒤 소유자가 Mac에서 원본 `tools/apply_github_settings.py`로 적용한다. 적용되면 Dependabot 버전 전용 PR을 뺀 모든 PR(되돌리기 쉬움 포함)에 소유자가 올리는 `review/independent`가 필요하다. 신뢰 경계에 걸리면 `review/security`도 필요하다. status는 소유자만 올린다는 규칙이다. 에이전트가 status를 올리는 것은 금지지만 GitHub가 기술적으로 막지는 않는다. 카나리 결과는 [repo-baseline Issue #32](https://github.com/minjunkim-dev/repo-baseline/issues/32) 댓글에 있다. -이번 갱신은 `.github/repository-settings.json`의 서버 기대값·권한·필수 체크·CodeQL·배포를 바꾸지 않는다. `actions: write`와 `checks: write`는 Review status의 댓글 재실행 job에만 있다. PR policy workflow의 권한은 `contents: read`다. 과거 실행 기록을 현재 검증 결과로 바꾸지 않는다. +이번 갱신은 `.github/repository-settings.json`의 필수 체크 기대값에 `Review status`를 더한다. 권한·CodeQL·배포·`strict`·기존 `CI`는 그대로다. 서버의 branch protection은 이 파일 변경으로 바뀌지 않는다. `actions: write`와 `checks: write`는 Review status의 댓글 재실행 job에만 있다. PR policy workflow의 권한은 `contents: read`다. 과거 실행 기록을 현재 검증 결과로 바꾸지 않는다. ## 과거 Bugbot 확인 기록 diff --git a/docs/agents/review.md b/docs/agents/review.md index ea01be1..13f1a70 100644 --- a/docs/agents/review.md +++ b/docs/agents/review.md @@ -19,7 +19,7 @@ CI 성공은 티켓의 실제 측정이나 독립 리뷰를 대체하지 않는 ## CI -필수 check 이름은 `CI`다. 모든 하위 job이 성공해야 `CI`가 성공한다. +필수 check 이름 `CI`는 `.github/workflows/ci.yml`의 집계 job이다. 모든 하위 job이 성공해야 `CI`가 성공한다. `Review status`는 그 하위 job이 아니다. `.github/repository-settings.json`이 `CI`와 `Review status`를 필수 체크로 기록한다. | 검사 | 확인하는 것 | 확인하지 않는 것 | | --- | --- | --- | @@ -49,27 +49,26 @@ source-only 테스트는 `test_*.py`를 자동 탐색한다. AST 검사는 정 ## PR 본문과 병합 `.github/pull_request_template.md`의 Summary, Evidence, Merge Danger, Author, Review evidence를 사용한다. -`Door:`는 `되돌리기 쉬움` 또는 `되돌리기 어려움`만 쓴다. 필드 이름은 `Door:`다. 옛 `양쪽 문`·`한쪽 문`은 deprecated이고, 재적용된 검사기가 같은 의미로 받는다. Author는 `Author: / `이다. -이 저장소의 검사기가 기본 브랜치에 다시 적용되기 전(이 PR 자신 포함)에는 본문 `Door:`와 소유자 승인 댓글에 옛 형식 `양쪽 문`/`한쪽 문`, `한쪽 문 승인: `를 쓴다. +`Door:`는 `되돌리기 쉬움` 또는 `되돌리기 어려움`만 쓴다. 필드 이름은 `Door:`다. 옛 `양쪽 문`·`한쪽 문`은 deprecated이고, 기본 브랜치의 검사기가 같은 의미로 받는다. Author는 `Author: / `이다. 승인 댓글의 현행 접두사는 `되돌리기 어려움 승인: `다. 옛 `한쪽 문 승인: `도 같은 의미다. Summary에는 변경 흐름을 보여주는 작은 도식을 둔다. Evidence에는 실제 명령과 결과를 둔다. 문서 변경이면 기존 규칙과 새 규칙을 비교한다. 실행하지 않은 검사를 성공으로 기록하지 않는다. | 위험 | 병합 조건 | | --- | --- | -| 되돌리기 쉬움 / narrow | 티켓 검증과 독립 리뷰 완료. 최종 head의 필수 `CI` 성공. 에이전트가 병합한다. | -| 되돌리기 어려움 또는 broad | 위 검증 완료. 사람이 최종 head와 영향 범위를 승인한 뒤 병합한다. | +| 되돌리기 쉬움 / narrow | 티켓 검증과 독립 리뷰 완료. 최종 head의 필수 체크 성공. 기대값이 서버에 적용되기 전에는 필수 체크가 `CI`이고 에이전트가 병합한다. 적용된 뒤에는 Dependabot 버전 전용 PR을 뺀 PR에 소유자의 `review/independent` success가 있어야 병합한다. | +| 되돌리기 어려움 또는 broad | 위 검증 완료. 사람이 최종 head와 영향 범위를 승인한 뒤 병합한다. 승인 댓글은 `되돌리기 어려움 승인: `다. 편집한 댓글은 무효다. | 되돌리기 어려운 데이터 변경, 외부 대량 발송, 파괴적인 정리, 권한 확대는 되돌리기 어려움으로 처리한다. 많은 프로젝트의 실행 동작이나 공개 계약을 바꾸는 변경은 broad로 처리한다. diff 크기로 위험을 결정하지 않는다. 정책상 사람이 승인할 변경은 `ready-for-human`으로 표시한다. -현재 GitHub 보호 설정은 `CI`만 강제한다. `Review status` workflow가 저장소에 있어도, 필수 체크로 등록되기 전에는 그 job이 병합을 막지 않는다. 되돌리기 어려움 승인 댓글은 `되돌리기 어려움 승인: `다. 옛 `한쪽 문 승인: `는 deprecated로 계속 인정한다. 형식은 repo-baseline `POLICY.md` §10이다. 위 사람 승인과 독립 리뷰는 에이전트 절차다. 서버가 자동으로 강제한다고 주장하지 않는다. +`.github/repository-settings.json`은 `CI`와 `Review status`를 필수 체크로 기록한다. `strict`는 true다. `app_id`는 15368이다. `Review status`의 context는 `.github/workflows/review-status.yml` job 이름과 같다. 이 기록의 PR은 branch protection을 쓰지 않는다. 병합 뒤 소유자가 Mac에서 적용한다. 적용되면 Dependabot 버전 전용 PR을 뺀 모든 PR(되돌리기 쉬움 포함)에 소유자가 올리는 `review/independent`가 필요하다. 신뢰 경계 경로면 `review/security`도 필요하다. status는 소유자만 올린다는 규칙이다. 에이전트가 status를 올리는 것은 금지지만 GitHub가 기술적으로 막지는 않는다. 1인 프로필에서 검사기는 `creator.login`이 저장소 소유자이거나 `STATUS_ACTORS`(지금은 비어 있음)인 status만 success로 센다. 적용 전에 서버 보호가 강제하는 체크는 `CI`다. 되돌리기 어려움 승인 댓글은 `되돌리기 어려움 승인: `다. 옛 `한쪽 문 승인: `는 deprecated로 계속 인정한다. 편집한 댓글은 승인이 아니다. 형식은 repo-baseline `POLICY.md` §10이다. 위 사람 승인과 독립 리뷰는 에이전트 절차다. 기대값 파일만으로 서버가 그 승인을 강제한다고 적지 않는다. 병합 직전에 GitHub API로 head·base와 검사 결과를 다시 확인한다. 최종 head SHA를 확인하고 `gh pr merge --repo --squash --match-head-commit `로 병합한다. `--admin`으로 보호를 우회하지 않는다. 리뷰 이후 커밋이나 base가 바뀌면 영향받는 검증을 갱신한다. 사용자가 병합을 금지한 작업은 검증 후에도 PR을 열어 둔다. 저장소 `Allow auto-merge`는 On으로 둔다. 모든 PR에 자동 병합을 예약하지 않는다. -기본 흐름은 최종 검증 후 직접 병합이다. GitHub 자동 병합 예약은 독립 리뷰 결과를 대상 SHA의 필수 체크로 연결한 뒤 별도 구성한다. +기본 흐름은 최종 검증 후 직접 병합이다. `Review status`가 필수로 적용된 뒤에는 Dependabot 버전 전용 PR을 뺀 되돌리기 쉬움도 소유자의 `review/independent`가 success이기 전에 병합되지 않는다. GitHub 자동 병합 예약은 그 status가 success인 뒤에만 한다. 되돌리기 어려움과 외부 리뷰 대상 PR은 예약하지 않는다. ## 로컬 보안 리뷰 diff --git a/scripts/check_review_status.py b/scripts/check_review_status.py index dd09d6d..564c517 100644 --- a/scripts/check_review_status.py +++ b/scripts/check_review_status.py @@ -1,13 +1,27 @@ #!/usr/bin/env python3 """Fail when a pull request head lacks the review statuses its changed paths need (POLICY.md §5). -Paths only: content signals stay a reviewer judgement. This shows the process ran on this exact -head; a status from the same account cannot tell a person from an agent, so it is no security boundary. -A base update creates a new head SHA. When the added and removed lines and binary blobs match a prior -success, that status covers this head. Reuse is still only a process record. +Paths only: content signals stay a reviewer judgement. On a 1-person profile, review/independent +and review/security count only when creator.login is the repository owner or a login in +STATUS_ACTORS (empty until the server app exists). Another creator is failure, not a missing +context, so an older success is not carried over it. The same account still cannot tell a person +from an agent. A team profile does not look at the creator. +A base update creates a new head SHA. Carry hashes the diff and the full post-change blob id of +each changed file. Line numbers do not count. Another file changing on the base leaves this file's +blob in place, so that success still covers the head. The same file changing, even when the diff +only shifts line numbers, is a new blob and is reviewed again. The function name after the second +@@ stays in the hash: it records which function git named, and the blob id is what separates files +where that name is missing or shared. A diff --git header that names one path twice is split in +the middle, so a directory named "svc b" is not read as the root file. A rename or copy takes the +new path from rename to or copy to. Anything else is not carried. Only the parent trees of changed +paths are read. A gitlink that is itself the changed path contributes its commit id. Reuse is still +only a process record, and it uses the same creator rule. A 1-person pull request that changes a POLICY.md §10 gate path also needs the repository owner's -approval comment on this head. That comment is not reused from an older head. +approval comment on this head. That comment is not reused from an older head. An edited comment +is not approval: updated_at must equal created_at, and lastEditedAt must be null. A same-second +edit keeps the timestamp strings equal and is still not approval. Post a new comment. The first line must be the current prefix or the deprecated prefix, then the full head SHA. -A comment rerun does not parse the body. It runs this checker, so both prefixes share this match. +A comment rerun does not parse the body. It runs this checker, so both prefixes share this match, +and an edit re-run drops the edited comment. The pull request body is parsed for the review evidence table. It is never executed. The pull request base ref must be the repository default branch, and the event base SHA must be that branch or an ancestor of it. A pull request with a different base is not evidence.""" @@ -36,6 +50,10 @@ DEPENDABOT = "dependabot[bot]" WEB_FLOW = "web-flow" TEAM_LINE = "프로필: 팀" +# Solo-profile logins that may post review/independent and review/security, besides the +# repository owner. Empty until the server app exists. This file runs from the default +# branch, so a pull request cannot add a login. A team profile does not read this tuple. +STATUS_ACTORS = () # Exact first-line prefixes. The second is deprecated and means the same approval. APPROVAL_LINE = re.compile( r"(?:되돌리기 어려움 승인:|한쪽 문 승인:)" @@ -80,6 +98,9 @@ def read_api(endpoint): if endpoint.startswith(FORCE_PUSHES): repo, number = endpoint[len(FORCE_PUSHES):].rsplit("/", 1) return force_push_befores(repo, number) + comments = comment_list_target(endpoint) + if comments is not None: + return approval_comment_pages(*comments) paged = ["--paginate", "--slurp"] if "per_page" in endpoint else [] result = subprocess.run(["gh", "api", "--method", "GET", endpoint, *paged], capture_output=True, text=True, check=True, timeout=60) @@ -135,43 +156,88 @@ def force_push_oids(nodes): return [event["oid"] for event in events] -def diff_identity(diff): - """SHA-256 of the exact added and removed lines, file paths, modes, renames, and binary blob ids. - Hunk positions and surrounding context do not count: a base update shifts them without changing the - pull request. Whitespace does count. None means the diff is not safe to treat as unchanged.""" +def full_oid(value): + """True for a full Git object id. An abbreviated index line is not one.""" + return isinstance(value, str) and len(value) == 40 and all(char in "0123456789abcdefABCDEF" for char in value) + + +def git_header_path(line): + """('path', path) when diff --git names one path on both sides, split at the middle. + ('rename', None) when the sides differ, so the new path comes from rename to or copy to. + ('bad', None) when the header is quoted or not diff --git a/. A last ' b/' is not a split: + a/svc b/package.json b/svc b/package.json is the file svc b/package.json, not package.json.""" + prefix = "diff --git " + if not isinstance(line, str) or not line.startswith(prefix + "a/"): + return "bad", None + body = line[len(prefix):] + if (len(body) - 5) % 2 == 0: + length = (len(body) - 5) // 2 + path = body[2:2 + length] + if length > 0 and body == f"a/{path} b/{path}": + return "path", path + return "rename", None + + +def destination_path(line, current): + """New path after a rename to or copy to line. current when this line is not one of those. + False when the path is empty, quoted, or a different path was already chosen.""" + dest = None + for prefix in ("rename to ", "copy to "): + if line.startswith(prefix): + dest = line[len(prefix):] + break + if dest is None: + return current + if dest == "" or dest.startswith('"') or (current is not None and current != dest): + return False + return dest + + +def diff_identity(diff, blobs=None): + """SHA-256 of the diff body plus each changed file's post-change blob id. + blobs maps the path after the change to a full blob SHA, or None when that path was deleted. + The numbers in an @@ line do not count. The text after the second @@ stays: it is the function + git named. The blob id is the whole file, so a move still differs when that name is missing, + shared, or cut off, and when the surrounding lines are blank. A lone CR is part of the line. + None blobs means the caller is comparing diff text only. A provided map that misses a path, or + a blob that is not a full id, is not safe to treat as unchanged.""" if diff is None: return None - # GitHub's diff is LF. splitlines() would also cut Unicode separators inside a line. - text = diff.replace("\r\n", "\n").replace("\r", "\n") + # GitHub's diff is LF. A bare CR is not a line ending here: stripping it would hide an injected CR. + text = diff.replace("\r\n", "\n") stripped = text.lstrip() if not stripped: return "empty" if stripped.startswith("{") or stripped.startswith("["): return None chunks = [] - current = index_line = None + current = new_path = None + deleted = False header, body = [], [] binary = False in_hunk = False def flush(): - nonlocal current, index_line, header, body, binary + nonlocal current, new_path, deleted, header, body, binary if current is None: return True - if binary and not index_line: + if new_path is None: return False - if binary: - header.append(index_line) - chunks.append((current, tuple(header), tuple(body))) - current = index_line = None - header, body, binary = [], [], False + chunks.append((current, new_path, deleted, binary, tuple(header), tuple(body))) + current = new_path = None + deleted = binary = False + header, body = [], [] return True for line in text.split("\n"): if line.startswith("diff --git "): if not flush(): return None + status, path = git_header_path(line) + if status == "bad": + return None current = line + new_path = path if status == "path" else None in_hunk = False continue if current is None: @@ -179,16 +245,25 @@ def flush(): continue return None if line.startswith("index "): - index_line = line + # Abbreviated blob ids in this line are not the post-change id. continue + # A hunk line "+ ++ /dev/null" is content. Only the header marks a deleted file. + if not in_hunk and (line.startswith("deleted file mode ") or line == "+++ /dev/null"): + deleted = True if line.startswith(("new file mode ", "deleted file mode ", "old mode ", "new mode ", "rename from ", "rename to ", "copy from ", "copy to ")): + chosen = destination_path(line, new_path) + if chosen is False: + return None + new_path = chosen header.append(line) continue # "--- a/file" is a header. A removed line "-- note" is also "--- note", so only skip headers # before the first hunk. if line.startswith("@@"): in_hunk = True + # Numbers sit between the markers. The function git named follows the second @@. + body.append("@@" + line.split("@@", 2)[-1]) continue if not in_hunk and line.startswith(("--- ", "+++ ", "similarity index ", "dissimilarity index ")): continue @@ -196,23 +271,149 @@ def flush(): binary = True body.append(line) continue - if line.startswith(("+", "-", "\\")): + if line.startswith(("+", "-", "\\")) or (in_hunk and line.startswith(" ")): body.append(line) continue - if line.startswith(" ") or line == "": + if line == "" or line.startswith(" "): continue return None if not flush() or not chunks: return None + blob_lines = [] + if blobs is not None: + if not isinstance(blobs, dict): + return None + for _, path, removed, _binary, _header, _body in chunks: + if path not in blobs: + return None + oid = blobs[path] + if removed: + if oid is not None: + return None + blob_lines.append("blob\tdeleted\t" + path) + continue + if not full_oid(oid): + return None + blob_lines.append("blob\t" + path + "\t" + oid.lower()) chunks.sort(key=lambda item: item[0]) lines = [] - for path, kept_header, kept_body in chunks: + for path, _new_path, _removed, _binary, kept_header, kept_body in chunks: lines.append(path) lines.extend(kept_header) lines.extend(kept_body) + lines.extend(sorted(blob_lines)) return hashlib.sha256("\n".join(lines).encode("utf-8")).hexdigest() +def path_oids(repo, sha, paths, read): + """Post-change id for each path. Only parent trees of those paths are read. + A blob contributes its blob id. A gitlink on the path itself contributes its commit id. + A gitlink anywhere else is not opened. Raises when a needed tree is truncated or an id is short.""" + if not paths: + return {} + commit = read(f"repos/{repo}/commits/{sha}") + root = ((commit.get("commit") or {}).get("tree") or {}).get("sha") if isinstance(commit, dict) else None + if not full_oid(root): + raise ValueError("tree") + cache = {} + + def load(oid): + if oid not in cache: + data = read(f"repos/{repo}/git/trees/{oid}") + if not isinstance(data, dict) or data.get("truncated") or not isinstance(data.get("tree"), list): + raise ValueError("tree") + entries = {} + for item in data["tree"]: + if not isinstance(item, dict): + raise ValueError("tree") + name = item.get("path") + kind = item.get("type") + child = item.get("sha") + if not isinstance(name, str) or name == "" or "/" in name or not full_oid(child): + raise ValueError("tree") + if kind not in ("blob", "tree", "commit"): + raise ValueError("tree") + entries[name] = (kind, child.lower()) + cache[oid] = entries + return cache[oid] + + found = {} + for path in paths: + parts = path.split("/") + if any(part == "" for part in parts): + raise ValueError("tree") + current = root + matched = False + for index, part in enumerate(parts): + item = load(current).get(part) + if item is None: + raise ValueError("tree") + kind, oid = item + last = index == len(parts) - 1 + if kind == "tree" and not last: + current = oid + continue + if last and kind in ("blob", "commit"): + found[path] = oid + matched = True + break + raise ValueError("tree") + if not matched: + raise ValueError("tree") + return found + + +def changed_blobs(repo, sha, diff, read): + """Post-change id for each path in the diff. None on a path means that file was deleted. + A gitlink contributes its commit id.""" + specs = [] + current = new_path = None + deleted = False + in_hunk = False + + def flush(): + nonlocal current, new_path, deleted, in_hunk + if current is None: + return + if new_path is None: + raise ValueError("blobs") + specs.append((new_path, deleted)) + current = new_path = None + deleted = False + in_hunk = False + + for line in diff.replace("\r\n", "\n").split("\n"): + if line.startswith("diff --git "): + flush() + status, path = git_header_path(line) + if status == "bad": + raise ValueError("blobs") + current = line + new_path = path if status == "path" else None + continue + if current is None: + continue + if line.startswith(("rename to ", "copy to ")): + chosen = destination_path(line, new_path) + if chosen is False: + raise ValueError("blobs") + new_path = chosen + if line.startswith("@@"): + in_hunk = True + if not in_hunk and (line.startswith("deleted file mode ") or line == "+++ /dev/null"): + deleted = True + flush() + if not specs: + raise ValueError("blobs") + needed = [path for path, removed in specs if not removed] + found = {path: None for path, removed in specs if removed} + for path, oid in path_oids(repo, sha, needed, read).items(): + found[path] = oid + if any(not removed and not full_oid(found.get(path)) for path, removed in specs): + raise ValueError("blobs") + return found + + def matches(path, patterns): name = path.rsplit("/", 1)[-1] # With "/" a pattern is a path from the root, also read literally (in app/[id]/route.ts, [id] is no glob). @@ -424,12 +625,129 @@ def event_base_on_default(repo, base, default, read): raise ValueError("base branch") -def owner_approved(repo, number, head, read): - """True when the repository owner has an issue comment for this exact head. Edits are the current body.""" +def repository_owner(repo, read): info = read(f"repos/{repo}") owner = (info.get("owner") or {}).get("login") if isinstance(info, dict) else None if not isinstance(owner, str) or owner == "": raise ValueError("repository owner") + return owner + + +def status_actors(repo, read): + """Owner plus STATUS_ACTORS. Blank entries are not logins. Solo profile only.""" + extra = tuple(login for login in STATUS_ACTORS if isinstance(login, str) and login != "") + return (repository_owner(repo, read), *extra) + + +def status_actor(status): + creator = status.get("creator") if isinstance(status, dict) else None + login = creator.get("login") if isinstance(creator, dict) else None + if not isinstance(login, str) or login == "": + return None + return login + + +def reviewed_state(status, allowed): + """State that counts for one review context. + + allowed is None on a team profile: the creator is not checked. + On a 1-person profile, a creator outside allowed is failure. Combined status keeps + the newest state per context, so dropping that status would look like a missing + context and carry could reuse an older success over a newer rejection.""" + if allowed is None: + return status["state"] + actor = status_actor(status) + if any(same_login(actor, login) for login in allowed): + return status["state"] + print(f"review status untrusted {status.get('context')} from {actor or 'unknown'} is not success") + return "failure" + + +def comment_unedited(comment): + """True when the comment was not edited. + + updated_at must be the same string as created_at. REST timestamps are whole seconds, + so a same-second edit can keep those strings equal. last_edited_at is GraphQL + lastEditedAt: null means GitHub recorded no edit, and a missing key is not approval. + includes_created_edit true means the history contains an edit.""" + if not isinstance(comment, dict): + return False + created = comment.get("created_at") + updated = comment.get("updated_at") + if not (isinstance(created, str) and created != "" and created == updated): + return False + if "last_edited_at" not in comment or comment.get("last_edited_at") is not None: + return False + return comment.get("includes_created_edit") is not True + + +def comment_list_target(endpoint): + """(repo, number) when endpoint is the issue-comment list. None otherwise.""" + parts = endpoint.split("/") + if (len(parts) != 6 or parts[0] != "repos" or parts[3] != "issues" + or parts[5] != "comments?per_page=100" or not parts[4].isdigit()): + return None + return f"{parts[1]}/{parts[2]}", parts[4] + + +def comment_record(node): + """One issue comment from GraphQL. Raises when the edit fields are missing.""" + if not isinstance(node, dict): + raise ValueError("approval comments") + if "lastEditedAt" not in node or "includesCreatedEdit" not in node: + raise ValueError("approval comments") + author = node.get("author") + login = author.get("login") if isinstance(author, dict) else None + return { + "user": {"login": login}, + "body": node.get("body"), + "created_at": node.get("createdAt"), + "updated_at": node.get("updatedAt"), + "last_edited_at": node.get("lastEditedAt"), + "includes_created_edit": node.get("includesCreatedEdit") is True, + } + + +def approval_comment_pages(repo, number): + """Issue comments, 100 per page. lastEditedAt is null when the body was never edited.""" + owner, name = repo.split("/", 1) + query = ( + "query($owner:String!,$name:String!,$number:Int!,$cursor:String){" + "repository(owner:$owner,name:$name){pullRequest(number:$number){" + "comments(first:100,after:$cursor){nodes{author{login} body createdAt updatedAt " + "lastEditedAt includesCreatedEdit}pageInfo{hasNextPage endCursor}}}}}" + ) + pages = [] + cursor = None + for _ in range(20): + command = ["gh", "api", "graphql", "-f", f"query={query}", "-f", f"owner={owner}", + "-f", f"name={name}", "-F", f"number={int(number)}"] + if cursor: + command.extend(["-f", f"cursor={cursor}"]) + result = subprocess.run(command, capture_output=True, text=True, check=True, timeout=60) + try: + pull = json.loads(result.stdout)["data"]["repository"]["pullRequest"] + connection = pull["comments"] + nodes = connection["nodes"] + info = connection["pageInfo"] + except (json.JSONDecodeError, KeyError, TypeError): + raise ValueError("approval comments") from None + if not isinstance(nodes, list) or not isinstance(info, dict): + raise ValueError("approval comments") + pages.append([comment_record(node) for node in nodes]) + if not info.get("hasNextPage"): + return pages + cursor = info.get("endCursor") + if not isinstance(cursor, str) or cursor == "": + raise ValueError("approval comments") + raise ValueError("approval comments") + + +def owner_approved(repo, number, head, read): + """True when the owner has an unedited issue comment for this exact head. + updated_at must equal created_at, and GitHub must have no edit (lastEditedAt null). + An edited comment is not approval, including a same-second edit; post a new comment.""" + owner = repository_owner(repo, read) pages = read(f"repos/{repo}/issues/{number}/comments?per_page=100") if not isinstance(pages, list): raise ValueError("approval comments") @@ -442,16 +760,35 @@ def owner_approved(repo, number, head, read): continue user = comment.get("user") login = user.get("login") if isinstance(user, dict) else None - if same_login(login, owner) and approval_sha(comment.get("body")) == wanted: + if (same_login(login, owner) and comment_unedited(comment) + and approval_sha(comment.get("body")) == wanted): return True return False -def latest_state(statuses, context): - """The combined status lists the newest state of each context first.""" +def status_list(pages): + """Newest-first status objects from GET /commits/{sha}/statuses. + + Each page is a list. The combined status object is a dict and has no creator, + so it is not this list and must not count as success.""" + if not isinstance(pages, list): + raise ValueError("commit statuses") + flat = [] + for page in pages: + if not isinstance(page, list): + raise ValueError("commit statuses") + for status in page: + if not isinstance(status, dict) or "context" not in status: + raise ValueError("commit statuses") + flat.append(status) + return flat + + +def latest_status(statuses, context): + """The status list is newest first. The first item of a context is the one that counts.""" for status in statuses: if status["context"] == context: - return status["state"] + return status return None @@ -497,17 +834,23 @@ def prior_shas(repo, number, head, read, commit_count): return ordered[:PRIOR_LIMIT] -def identity_of(repo, base, sha, read_diff, cache): +def identity_of(repo, base, sha, read_diff, cache, read): if sha not in cache: try: - cache[sha] = diff_identity(read_diff(f"repos/{repo}/compare/{base}...{sha}")) - except (OSError, subprocess.SubprocessError, ValueError, TypeError): + diff = read_diff(f"repos/{repo}/compare/{base}...{sha}") + if isinstance(diff, str) and diff.replace("\r\n", "\n").strip() == "": + cache[sha] = "empty" + else: + cache[sha] = diff_identity(diff, changed_blobs(repo, sha, diff, read)) + except (OSError, subprocess.SubprocessError, ValueError, TypeError, AttributeError): cache[sha] = None return cache[sha] -def carried_contexts(repo, number, head, base, contexts, read, read_diff, commit_count): - """Contexts whose newest prior status is success and whose diff matches this head. Anything unverified stays put.""" +def carried_contexts(repo, number, head, base, contexts, read, read_diff, commit_count, allowed=None): + """Contexts whose newest prior status is success and whose diff matches this head. Anything unverified stays put. + allowed is the solo-profile creator list. None keeps the team profile and does not read the creator. + An untrusted creator is failure, same as on the current head, so the walk does not skip it.""" candidates = prior_shas(repo, number, head, read, commit_count) if not candidates: return set() @@ -516,14 +859,17 @@ def carried_contexts(repo, number, head, base, contexts, read, read_diff, commit def state_at(sha, context): if sha not in fetched: try: - pages = read(f"repos/{repo}/commits/{sha}/status?per_page=100") - fetched[sha] = [status for page in pages for status in page["statuses"]] + pages = read(f"repos/{repo}/commits/{sha}/statuses?per_page=100") + fetched[sha] = status_list(pages) except (OSError, subprocess.SubprocessError, KeyError, ValueError, TypeError): fetched[sha] = None # A failed lookup must not skip this SHA for an older success: its state is unknown. if fetched[sha] is None: return "unreadable" - return latest_state(fetched[sha], context) + found = latest_status(fetched[sha], context) + if found is None: + return None + return reviewed_state(found, allowed) for context in contexts: donor = None @@ -533,8 +879,8 @@ def state_at(sha, context): continue if state != "success": break - current = identity_of(repo, base, head, read_diff, cache) - prior = identity_of(repo, base, sha, read_diff, cache) + current = identity_of(repo, base, head, read_diff, cache, read) + prior = identity_of(repo, base, sha, read_diff, cache, read) if current is not None and current == prior: donor = sha break @@ -724,6 +1070,10 @@ def missing(repo, number, head, base, read=read_api, read_diff=None): default = default_base(repo, pull, read) if default is None or not event_base_on_default(repo, base, default, read): return [BASE_REF] + # None means a team profile: any creator still counts. A 1-person profile counts the owner + # and STATUS_ACTORS only, and treats every other creator as failure. + solo = not team_profile(repo, base, read) + allowed = status_actors(repo, read) if solo else None # The open pull request's file and commit lists follow its current head, which can move # after this event. review/independent and review/security follow compare(base, head). patterns = COMMON_PATHS + DEPENDENCY_FILES + project_paths(repo, base, read) @@ -742,11 +1092,18 @@ def missing(repo, number, head, base, read=read_api, read_diff=None): # Compare stops at 300 files. A hidden path cannot be ruled out. if pinned is None or (touched and not bump): needed.append(SECURITY) - statuses = [status for page in read(f"repos/{repo}/commits/{head}/status?per_page=100") for status in page["statuses"]] - # The combined list is newest first. An older success in that list does not erase a newer failure. + statuses = status_list(read(f"repos/{repo}/commits/{head}/statuses?per_page=100")) + # The list is newest first. An older success does not erase a newer failure. + # An untrusted creator is failure, so it also does not reveal an older trusted success. newest = {} for status in statuses: - newest.setdefault(status["context"], status["state"]) + context = status["context"] + if context in newest: + continue + if allowed is not None and context in (INDEPENDENT, SECURITY): + newest[context] = reviewed_state(status, allowed) + else: + newest[context] = status["state"] present = set(newest) for context in (INDEPENDENT, SECURITY): if context not in needed and context in present: @@ -759,7 +1116,7 @@ def missing(repo, number, head, base, read=read_api, read_diff=None): result = list(absent) else: carried = carried_contexts( - repo, number, head, base, reusable, read, read_diff or read_diff_api, pull.get("commits")) + repo, number, head, base, reusable, read, read_diff or read_diff_api, pull.get("commits"), allowed) result = [context for context in absent if context not in carried] # The owner comment is tied to this SHA, so an unchanged diff does not carry it. A posted status does not either. if pinned is None: @@ -770,7 +1127,7 @@ def missing(repo, number, head, base, read=read_api, read_diff=None): needs_owner = one_way(pinned, False) or profile_line_changes(pinned) or introduced version_only = (pinned_commits is not None and dependabot_bump(pull, pinned_commits, pinned) and not major_change(pinned)) - if needs_owner and not version_only and not team_profile(repo, base, read) and not owner_approved(repo, number, head, read): + if needs_owner and not version_only and solo and not owner_approved(repo, number, head, read): result.append(OWNER) # The body is the current pull request text. This run's head is the event SHA, not a later push. if isinstance(pull, dict) and not _dependabot_author(pull): diff --git a/scripts/rerun_review_status.py b/scripts/rerun_review_status.py index db31efd..2f82cff 100644 --- a/scripts/rerun_review_status.py +++ b/scripts/rerun_review_status.py @@ -5,9 +5,15 @@ until the pull_request_target run for this head is re-run, or until a newer failing check named Review status is posted on that head. This does not write review/independent or review/security, and it does not read the comment body. -issue_comment has no branch filter. A pull request whose base is not the -default branch posts nothing, so that head is not turned red for the pull -request that does target the default branch. +created, edited, and deleted all re-run. An edited comment is not approval: +the checker counts a comment only when updated_at equals created_at and +lastEditedAt is null, so a same-second edit is dropped too. This re-run +clears the old success. Post a new comment. +comment.user on an edit stays the original author, so another account's edit +of the owner's comment still matches the workflow and still re-runs. Skipping +edited would leave that success in place. issue_comment has no branch filter. +A pull request whose base is not the default branch posts nothing, so that +head is not turned red for the pull request that does target the default branch. """ from datetime import datetime, timezone @@ -18,10 +24,24 @@ CHECK_NAME = "Review status" PAGE_LIMIT = 20 +# Same set as templates/review-status.yml issue_comment.types. edited stays: +# the checker, not this script, rejects the edited body. +COMMENT_ACTIONS = ("created", "edited", "deleted") + + +def comment_action_reruns(action): + """True for the comment events that must re-run the checker. + + edited is included on purpose. The checker ignores a comment whose + updated_at differs from created_at. Re-running is what clears a previous + success. The workflow matches comment.user, which stays the author, so a + write collaborator editing the owner's comment still re-runs.""" + return action in COMMENT_ACTIONS def comment_triggers(is_pull_request, state, author, owner): - """Same rule as the workflow `if`. A fork's author is not the base repository owner.""" + """Same rule as the workflow `if`. A fork's author is not the base repository owner. + This does not look at sender. An edit's author is still the original user.""" return bool(is_pull_request) and state == "open" and bool(owner) and author == owner @@ -64,8 +84,8 @@ def fail_closed_body(sha, now=None): "completed_at": stamp, "output": { "title": "Owner approval changed", - "summary": "The owner approval comment no longer matches this head, " - "and the previous Review status run could not be re-run.", + "summary": "The owner comment changed. An edited comment is not approval. " + "The previous Review status run could not be re-run.", }, } @@ -134,11 +154,24 @@ def main(env=None, execute=None, pages=None): if not on_default: print(f"side base; not posting a check for {number}", file=sys.stderr) return 0 + # Unset keeps older callers re-running. edited is a real re-run: the checker + # drops a comment whose updated_at is not created_at. An unknown action must + # not post a check on a head this event did not change. + action = (env.get("COMMENT_ACTION") or "").strip() + if action and not comment_action_reruns(action): + print(f"comment action {action} is not a review-status rerun", file=sys.stderr) + return 0 sha = ((pull.get("head") or {}).get("sha") or "").strip() if len(sha) != 40 or any(char not in "0123456789abcdefABCDEF" for char in sha): print("bad head sha", file=sys.stderr) return 1 - runs, complete = collect(pages if pages is not None else fetch_pages(repo, execute)) + # A failed or unreadable run list is incomplete. Post the failing check instead of + # leaving the previous Review status success in place. + try: + runs, complete = collect(pages if pages is not None else fetch_pages(repo, execute)) + except (subprocess.CalledProcessError, OSError, ValueError) as error: + print(f"run list unreadable: {error}", file=sys.stderr) + runs, complete = [], False action, run_id = plan(runs, int(number), sha) if action == "wait": print("matching run still in progress", file=sys.stderr) diff --git a/scripts/source-only/test_review_status.py b/scripts/source-only/test_review_status.py index 5cb8e8a..352c9a5 100644 --- a/scripts/source-only/test_review_status.py +++ b/scripts/source-only/test_review_status.py @@ -2,10 +2,13 @@ import base64 from contextlib import redirect_stdout +import hashlib from io import StringIO +import os from pathlib import Path import subprocess import sys +import tempfile import unittest sys.path.insert(0, str(Path(__file__).resolve().parents[1])) @@ -14,6 +17,15 @@ HEAD = "a" * 40 +def git(args, **kwargs): + """Run git without global or system config. + + commit.gpgsign and diff.noprefix in a developer config would sign the fixture + commit or drop the a/ b/ header that git_header_path requires. Local repo config still applies.""" + env = {**os.environ, "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_NOSYSTEM": "1"} + return subprocess.run(["git", *args], env=env, check=True, **kwargs) + + def passing_body(head): lines = [ "Author: Cursor 클라우드 에이전트 / grok-4.7", "", @@ -26,11 +38,39 @@ def passing_body(head): return "\n".join(lines) + "\n" +def status_rows(pairs, default_creator): + """One page of GET /commits/{sha}/statuses. The list endpoint includes creator.""" + rows = [] + for index, item in enumerate(pairs): + creator = default_creator + if len(item) == 3: + context, state, creator = item + else: + context, state = item + row = { + "avatar_url": "https://avatars.githubusercontent.com/u/1", + "context": context, + "created_at": f"2020-01-01T00:00:{index:02d}Z", + "description": "", + "id": index + 1, + "node_id": "S", + "state": state, + "target_url": None, + "updated_at": f"2020-01-01T00:00:{index:02d}Z", + "url": "https://api.github.com/repos/o/n/statuses/1", + } + if creator is not None: + row["creator"] = {"login": creator, "id": 1, "type": "User"} + rows.append(row) + return rows + + def api(files, statuses=(), author="alice", commits=("alice",), project=None, changed=None, verified=True, committer=tool.WEB_FLOW, commit_shas=None, extra_status=None, force_pushes=None, commit_count=None, owner="owner", comments=(), agents=None, head=HEAD, compare_files=None, compare_commits=None, head_agents=None, force_push_times=None, body=None, number="1", base_ref="main", base_sha="BASE", - default_branch="main", base_status="identical"): + default_branch="main", base_status="identical", status_creator=None, combined=False, + blobs=None): base = {"sha": base_sha} if base_ref is not None: base["ref"] = base_ref @@ -49,8 +89,11 @@ def api(files, statuses=(), author="alice", commits=("alice",), project=None, ch f"repos/o/n/pulls/{number}": pull, f"repos/o/n/pulls/{number}/files?per_page=100": [files], f"repos/o/n/pulls/{number}/commits?per_page=100": [commit_rows], - f"repos/o/n/commits/{head}/status?per_page=100": [{ - "statuses": [{"context": context, "state": state} for context, state in statuses]}], + f"repos/o/n/commits/{head}/statuses?per_page=100": ( + [{"state": "success", "statuses": status_rows(statuses, owner if status_creator is None else status_creator), + "sha": head, "total_count": len(tuple(statuses))}] + if combined else + [status_rows(statuses, owner if status_creator is None else status_creator)]), "repos/o/n": repo, f"repos/o/n/issues/{number}/comments?per_page=100": [list(comments)], } @@ -85,8 +128,8 @@ def api(files, statuses=(), author="alice", commits=("alice",), project=None, ch "content": base64.b64encode(head_agents.encode()).decode()} if extra_status: for sha, pairs in extra_status.items(): - routes[f"repos/o/n/commits/{sha}/status?per_page=100"] = [{ - "statuses": [{"context": context, "state": state} for context, state in pairs]}] + routes[f"repos/o/n/commits/{sha}/statuses?per_page=100"] = [ + status_rows(pairs, owner if status_creator is None else status_creator)] if force_pushes is not None: events = [] for index, sha in enumerate(force_pushes): @@ -94,6 +137,18 @@ def api(files, statuses=(), author="alice", commits=("alice",), project=None, ch else f"2030-01-{(28 - index):02d}T00:00:00Z") events.append({"oid": sha, "createdAt": when}) routes[tool.FORCE_PUSHES + f"o/n/{number}"] = events + commits_for_blobs = {head} + if commit_shas: + commits_for_blobs.update(commit_shas) + if force_pushes: + commits_for_blobs.update(force_pushes) + if extra_status: + commits_for_blobs.update(extra_status) + for commit in commits_for_blobs: + mapping = None if blobs is None else blobs.get(commit) + if mapping is None: + mapping = {path: BLOB for path in default_paths(files)} + install_blobs(routes, commit, mapping) def read(endpoint): if endpoint not in routes: @@ -106,8 +161,58 @@ def run(read): return tool.missing("o/n", "1", HEAD, "BASE", read) -def comment(body, login="owner"): - return {"user": {"login": login}, "body": body} +def comment(body, login="owner", created_at="2020-01-01T00:00:00Z", updated_at=None, omit_times=False, + last_edited_at=None, omit_edit=False, includes_created_edit=False): + row = {"user": {"login": login}, "body": body} + if not omit_times: + row["created_at"] = created_at + row["updated_at"] = created_at if updated_at is None else updated_at + if not omit_edit: + row["last_edited_at"] = last_edited_at + if includes_created_edit: + row["includes_created_edit"] = True + return row + + +BLOB = "b" * 40 +OTHER = "c" * 40 + + +def default_paths(files): + found = {entry.get("filename") for entry in files if entry.get("filename")} + found.add("src/app.py") + return found + + +def install_blobs(routes, commit, mapping): + """One tree object per directory, so a nested path is more than one response.""" + root = {} + for path, oid in mapping.items(): + if oid is None: + continue + node = root + parts = path.split("/") + for part in parts[:-1]: + node = node.setdefault(part, {}) + node[parts[-1]] = oid + + def emit(node, label): + tree_sha = hashlib.sha256(label.encode()).hexdigest()[:40] + entries = [] + for name in sorted(node): + child = node[name] + if isinstance(child, dict): + entries.append({"path": name, "mode": "040000", "type": "tree", "sha": emit(child, label + "/" + name)}) + elif isinstance(child, tuple): + kind, oid = child + entries.append({"path": name, "mode": "160000" if kind == "commit" else "100644", + "type": kind, "sha": oid}) + else: + entries.append({"path": name, "mode": "100644", "type": "blob", "sha": child}) + routes[f"repos/o/n/git/trees/{tree_sha}"] = {"truncated": False, "tree": entries} + return tree_sha + + routes[f"repos/o/n/commits/{commit}"] = {"commit": {"tree": {"sha": emit(root, "tree:" + str(commit))}}} OK = (tool.INDEPENDENT, "success") @@ -127,9 +232,28 @@ def f(): +++ b/src/app.py @@ -20,3 +20,4 @@ def f(): - return 0 + return 1 + return 2 """ +# Same added line, different function. The context lines are part of the identity. +MOVED_FROM = """diff --git a/src/app.py b/src/app.py +index 1111111..2222222 100644 +--- a/src/app.py ++++ b/src/app.py +@@ -10,3 +10,4 @@ + def format_name(): + return name ++ return True +""" +MOVED_TO = """diff --git a/src/app.py b/src/app.py +index aaaaaaa..bbbbbbb 100644 +--- a/src/app.py ++++ b/src/app.py +@@ -40,3 +40,4 @@ + def is_admin(): + return name ++ return True +""" def diffs(mapping): @@ -266,11 +390,498 @@ def test_a_major_action_bump_still_needs_the_owner_comment(self): minor = [{"filename": "package.json", "patch": '@@\n- "x": "^1.2.3",\n+ "x": "^2.0.0",'}] self.assertEqual(run(api(minor, author=bot, commits=[bot])), []) + def test_a_combined_status_payload_does_not_pass(self): + combined = [{"state": "success", "statuses": [ + {"context": tool.INDEPENDENT, "state": "success", "description": "", "id": 1}]}] + self.assertRaises(ValueError, tool.status_list, combined) + files = [{"filename": "src/app.py"}] + env = {"REPO": "o/n", "PR_NUMBER": "1", "HEAD_SHA": HEAD, "BASE_SHA": "BASE"} + self.assertEqual(tool.main(env, api(files, [OK], combined=True)), 2) + self.assertEqual(run(api(files, [OK])), []) + + def test_a_solo_profile_counts_an_owner_status_and_not_another_creator(self): + files = [{"filename": "src/app.py"}] + self.assertEqual(tool.STATUS_ACTORS, ()) + self.assertEqual(tool.status_actors("o/n", api(files)), ("owner",)) + self.assertEqual(run(api(files, [OK])), []) + self.assertEqual(run(api(files, [(tool.INDEPENDENT, "success", "Owner")], owner="owner")), []) + for creator in ("github-actions[bot]", "alice", None): + with self.subTest(creator=creator): + posted = [(tool.INDEPENDENT, "success", creator)] + self.assertEqual(run(api(files, posted)), [tool.INDEPENDENT]) + # Newest untrusted success hides an older owner success. Combined status is newest first. + hidden = [ + (tool.INDEPENDENT, "success", "github-actions[bot]"), + (tool.INDEPENDENT, "success", "owner"), + ] + self.assertEqual(run(api(files, hidden)), [tool.INDEPENDENT]) + newer_owner = [ + (tool.INDEPENDENT, "success", "owner"), + (tool.INDEPENDENT, "success", "github-actions[bot]"), + ] + self.assertEqual(run(api(files, newer_owner)), []) + # The server-app setting is empty. A login in that tuple counts; a blank entry does not. + original = tool.STATUS_ACTORS + try: + tool.STATUS_ACTORS = ("server-app[bot]", "") + self.assertEqual(tool.status_actors("o/n", api(files)), ("owner", "server-app[bot]")) + app = [(tool.INDEPENDENT, "success", "server-app[bot]")] + self.assertEqual(run(api(files, app)), []) + self.assertEqual(run(api(files, [(tool.INDEPENDENT, "success", "other-app[bot]")])), [tool.INDEPENDENT]) + finally: + tool.STATUS_ACTORS = original + team = api(files, [(tool.INDEPENDENT, "success", "github-actions[bot]")], agents="프로필: 팀\n") + self.assertEqual(run(team), []) + gate = [{"filename": "scripts/check_review_status.py"}] + bot_security = [OK, (tool.SECURITY, "success", "github-actions[bot]")] + self.assertEqual(run(api(gate, bot_security)), [tool.SECURITY, tool.OWNER]) + both = [OK, (tool.SECURITY, "success", "owner")] + self.assertEqual(run(api(gate, both)), [tool.OWNER]) + + def test_carry_does_not_reuse_another_creators_success(self): + files = [{"filename": "src/app.py"}] + same = diffs({"OLD": ADDED, "MID": SHIFTED, HEAD: SHIFTED}) + for creator in ("github-actions[bot]", "alice", None): + with self.subTest(creator=creator): + read = api(files, commit_shas=["OLD", HEAD], force_pushes=[], + extra_status={"OLD": [(tool.INDEPENDENT, "success", creator)]}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, same), [tool.INDEPENDENT]) + owner = api(files, commit_shas=["OLD", HEAD], force_pushes=[], extra_status={"OLD": [OK]}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", owner, same), []) + blocked = api(files, commit_shas=["OLDER", "MID", HEAD], force_pushes=[], extra_status={ + "OLDER": [OK], "MID": [(tool.INDEPENDENT, "success", "github-actions[bot]")]}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", blocked, same), [tool.INDEPENDENT]) + team = api(files, commit_shas=["OLD", HEAD], force_pushes=[], agents="프로필: 팀\n", + extra_status={"OLD": [(tool.INDEPENDENT, "success", "github-actions[bot]")]}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", team, same), []) + + def test_a_same_context_move_between_functions_does_not_carry(self): + def block(name): + return ( + f"def {name}():\n" + " a = 1\n b = 2\n c = 3\n" + " d = 4\n e = 5\n f = 6\n" + ) + + def blank(name): + return f"def {name}():\n" + "\n" * 4 + " marker = 0\n tail = 1\n tail2 = 2\n tail3 = 3\n" + + def git_diff(before, after): + with tempfile.TemporaryDirectory() as folder: + root = Path(folder) + git(["init", "-q"], cwd=root) + git(["config", "user.email", "t@example.com"], cwd=root) + git(["config", "user.name", "t"], cwd=root) + (root / "app.py").write_text(before) + git(["add", "app.py"], cwd=root) + git(["commit", "-q", "-m", "base"], cwd=root) + (root / "app.py").write_text(after) + result = git(["diff"], cwd=root, capture_output=True, text=True) + return result.stdout + + def added(source, name, needle, replacement): + return source.replace(f"def {name}():\n{needle}", f"def {name}():\n{replacement}", 1) + + same = block("format_name") + "\n" + block("is_admin") + needle = " a = 1\n b = 2\n c = 3\n" + planted = " a = 1\n b = 2\n c = 3\n return True\n" + from_diff = git_diff(same, added(same, "format_name", needle, planted)) + to_diff = git_diff(same, added(same, "is_admin", needle, planted)) + self.assertIn("@@", from_diff) + self.assertIn("def format_name():", from_diff.split("@@", 2)[-1].split("\n", 1)[0]) + self.assertIn("def is_admin():", to_diff.split("@@", 2)[-1].split("\n", 1)[0]) + self.assertNotEqual(tool.diff_identity(from_diff), tool.diff_identity(to_diff)) + # Rewriting the header to the same name matches the diff text. The post-change blob splits the files. + renamed = to_diff.replace("def is_admin():", "def format_name():") + self.assertEqual(tool.diff_identity(from_diff), tool.diff_identity(renamed)) + self.assertNotEqual(tool.diff_identity(from_diff, {"app.py": "a" * 40}), + tool.diff_identity(renamed, {"app.py": "b" * 40})) + shifted = from_diff.replace("@@ -2,6 +2,7 @@", "@@ -40,6 +40,7 @@", 1) + self.assertEqual(tool.diff_identity(from_diff), tool.diff_identity(shifted)) + + blanks = blank("format_name") + "\n" + blank("is_admin") + empty = "\n" * 4 + planted_blank = "\n" * 3 + " return True\n" + blank_from = git_diff(blanks, added(blanks, "format_name", empty, planted_blank)) + blank_to = git_diff(blanks, added(blanks, "is_admin", empty, planted_blank)) + self.assertNotEqual(tool.diff_identity(blank_from), tool.diff_identity(blank_to)) + + files = [{"filename": "src/app.py"}] + read = api(files, commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[]) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": from_diff, HEAD: to_diff})), + [tool.INDEPENDENT]) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": blank_from, HEAD: blank_to})), + [tool.INDEPENDENT]) + + def test_nameless_files_and_a_carriage_return_do_not_carry(self): + def git_blob(text): + result = git(["hash-object", "--stdin"], input=text, capture_output=True) + return result.stdout.decode().strip() + + def git_diff(path, before, after): + with tempfile.TemporaryDirectory() as folder: + root = Path(folder) + git(["init", "-q"], cwd=root) + git(["config", "user.email", "t@example.com"], cwd=root) + git(["config", "user.name", "t"], cwd=root) + target = root / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(before) + git(["add", path], cwd=root) + git(["commit", "-q", "-m", "base"], cwd=root) + target.write_bytes(after) + result = git(["diff"], cwd=root, capture_output=True) + return result.stdout.decode() + + def carry(path, before, left, right): + old_diff = git_diff(path, before, left) + new_diff = git_diff(path, before, right) + old_blob, new_blob = git_blob(left), git_blob(right) + self.assertNotEqual(old_blob, new_blob) + # The function header and the context match, so the diff text alone would carry. + self.assertEqual(tool.diff_identity(old_diff), tool.diff_identity(new_diff)) + self.assertNotEqual(tool.diff_identity(old_diff, {path: old_blob}), + tool.diff_identity(new_diff, {path: new_blob})) + read = api([{"filename": path}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, + force_pushes=[], blobs={"OLD": {path: old_blob}, HEAD: {path: new_blob}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": old_diff, HEAD: new_diff})), + [tool.INDEPENDENT]) + + def move(text, needle, insert): + left = text.replace(needle, insert, 1) + head, _sep, tail = text.rpartition(needle) + return left, head + insert + tail + + yaml = """name: ci +on: push +jobs: + one: + runs: echo + a: 1 + b: 2 + c: 3 + d: 4 + e: 5 + f: 6 + two: + runs: echo + a: 1 + b: 2 + c: 3 + d: 4 + e: 5 + f: 6 +""".encode() + carry("ci.yml", yaml, *move(yaml, b" c: 3\n d: 4\n", b" c: 3\n steps: true\n d: 4\n")) + payload = """{ + "a": { + "k": 1, + "m": 1, + "n": 1, + "p": 2, + "q": 3, + "r": 4 + }, + "b": { + "k": 1, + "m": 1, + "n": 1, + "p": 2, + "q": 3, + "r": 4 + } +} +""".encode() + carry("data.json", payload, *move(payload, b' "n": 1,\n "p": 2,\n', + b' "n": 1,\n "x": true,\n "p": 2,\n')) + source = ( + "def format_name():\n" + " a = 1\n b = 2\n c = 3\n d = 4\n e = 5\n f = 6\n" + " a = 1\n b = 2\n c = 3\n d = 4\n e = 5\n f = 6\n" + ).encode() + carry("app.py", source, *move(source, b" c = 3\n d = 4\n", + b" c = 3\n return True\n d = 4\n")) + plain = b"def f():\n return 1\n" + with_lf = b"def f():\n return 1\n return 2\n" + with_cr = b"def f():\n return 1\n return 2\r\n" + self.assertNotEqual(git_blob(with_lf), git_blob(with_cr)) + read = api([{"filename": "app.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[], + blobs={"OLD": {"app.py": git_blob(with_lf)}, HEAD: {"app.py": git_blob(with_cr)}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({ + "OLD": git_diff("app.py", plain, with_lf), HEAD: git_diff("app.py", plain, with_cr)})), + [tool.INDEPENDENT]) + injected = ADDED.replace("+ return 2", "+ return\r 2") + self.assertNotEqual(tool.diff_identity(ADDED, {"src/app.py": BLOB}), + tool.diff_identity(injected, {"src/app.py": BLOB})) + + def test_a_deleted_rename_and_a_truncated_tree_do_not_reuse_a_bad_blob(self): + removed = "diff --git a/src/app.py b/src/app.py\ndeleted file mode 100644\n--- a/src/app.py\n+++ /dev/null\n@@ -1 +0,0 @@\n-x\n" + renamed = ("diff --git a/src/app.py b/src/next.py\nsimilarity index 100%\nrename from src/app.py\n" + "rename to src/next.py\n") + self.assertEqual(tool.diff_identity(removed, {"src/app.py": None}), + tool.diff_identity(removed, {"src/app.py": None})) + self.assertIsNone(tool.diff_identity(removed, {"src/app.py": BLOB})) + self.assertIsNone(tool.diff_identity(renamed, {"src/next.py": "abc"})) + planted = ADDED.replace("+ return 2", "+ return 2\n+++ /dev/null") + self.assertIsNone(tool.diff_identity(planted, {"src/app.py": None})) + self.assertIsNotNone(tool.diff_identity(planted, {"src/app.py": BLOB})) + gone = api([{"filename": "src/app.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, + force_pushes=[], blobs={"OLD": {"src/app.py": None}, HEAD: {"src/app.py": None}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", gone, diffs({"OLD": removed, HEAD: removed})), []) + moved = api([{"filename": "src/next.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, + force_pushes=[], blobs={"OLD": {"src/next.py": BLOB}, HEAD: {"src/next.py": BLOB}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", moved, diffs({"OLD": renamed, HEAD: renamed})), []) + missing_new = api([{"filename": "src/app.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, + force_pushes=[], blobs={"OLD": {"src/app.py": BLOB}, HEAD: {"src/app.py": BLOB}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", missing_new, diffs({"OLD": renamed, HEAD: renamed})), + [tool.INDEPENDENT]) + nested = api([{"filename": "src/app.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, + force_pushes=[]) + seen = [] + + def walk(endpoint): + seen.append(endpoint) + return nested(endpoint) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", walk, diffs({"OLD": ADDED, HEAD: ADDED})), []) + self.assertGreaterEqual(sum("/git/trees/" in item for item in seen), 4) + base = api([{"filename": "src/app.py"}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[]) + + def truncated(endpoint): + if "/git/trees/" in endpoint: + return {"truncated": True, "tree": []} + return base(endpoint) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", truncated, diffs({"OLD": ADDED, HEAD: ADDED})), + [tool.INDEPENDENT]) + + def test_a_real_git_tree_response_is_the_blob_id(self): + # GitHub git/trees for scripts/ at 711c827a5826fb6913a34513532495f8a089eb29. truncated is false. + # git rev-parse of that commit's scripts/check_review_status.py is the same sha. + commit = "711c827a5826fb6913a34513532495f8a089eb29" + root = "0c98360ef0f4130634d19e60e81f5a682201df8f" + scripts = "0f5f688ef34043e581aca3bf3b632bc98aaff148" + routes = { + f"repos/minjunkim-dev/repo-baseline/commits/{commit}": { + "sha": commit, "commit": {"tree": {"sha": root}}}, + f"repos/minjunkim-dev/repo-baseline/git/trees/{root}": { + "sha": root, "truncated": False, "tree": [ + {"path": "scripts", "mode": "040000", "type": "tree", "sha": scripts}]}, + f"repos/minjunkim-dev/repo-baseline/git/trees/{scripts}": { + "sha": scripts, "truncated": False, "tree": [ + {"path": "check_github_settings.py", "mode": "100644", "type": "blob", + "sha": "58dde5aaeb1065834d78325e95579172fd697c78"}, + {"path": "check_repo_baseline.py", "mode": "100644", "type": "blob", + "sha": "46f0f4e9c29d4faa3191f3108f5b6196df1564cc"}, + {"path": "check_review_status.py", "mode": "100644", "type": "blob", + "sha": "6855001f4a45ae2eb22c93d4057642d7fc0f4740"}, + {"path": "pr_author.py", "mode": "100644", "type": "blob", + "sha": "f4bbf1a9601d2f642a3152973c689be00401b1c9"}, + {"path": "rerun_review_status.py", "mode": "100644", "type": "blob", + "sha": "2f82cff6b1667406f74e428480d334e506ff5eeb"}, + {"path": "test_github_settings.py", "mode": "100644", "type": "blob", + "sha": "96f46ff30ec78ac5f7a3a5170a8a7bd654a58a6e"}, + {"path": "test_repo_baseline.py", "mode": "100644", "type": "blob", + "sha": "89ea3259c58d0aa12c19f37389f6d24394a6d1c4"}, + {"path": "test_review_status.py", "mode": "100644", "type": "blob", + "sha": "751db6339899e53cf681dd04e088a46ac242edc4"}, + {"path": "test_workflow_policy.py", "mode": "100644", "type": "blob", + "sha": "d6c6826c89225fe0265e1825531336279c1a576e"}, + {"path": "workflow_policy.py", "mode": "100644", "type": "blob", + "sha": "eb9eb5cfca0be696ea371e9114ebc9f933d2289d"}]}} + + def read(endpoint): + if endpoint not in routes: + raise subprocess.CalledProcessError(1, "gh", stderr="gh: Not Found (HTTP 404)") + return routes[endpoint] + found = tool.path_oids("minjunkim-dev/repo-baseline", commit, ["scripts/check_review_status.py"], read) + self.assertEqual(found["scripts/check_review_status.py"], "6855001f4a45ae2eb22c93d4057642d7fc0f4740") + short = dict(routes) + short[f"repos/minjunkim-dev/repo-baseline/git/trees/{scripts}"] = { + "sha": scripts, "truncated": False, "tree": [ + {"path": "check_review_status.py", "mode": "100644", "type": "blob", "sha": "6855001"}]} + self.assertRaises(ValueError, tool.path_oids, "minjunkim-dev/repo-baseline", commit, + ["scripts/check_review_status.py"], lambda endpoint: short[endpoint]) + + def test_a_directory_named_with_b_does_not_hash_the_root_file(self): + self.assertEqual(tool.git_header_path("diff --git a/svc b/package.json b/svc b/package.json"), + ("path", "svc b/package.json")) + self.assertEqual(tool.git_header_path("diff --git a/svc/package.json b/svc/package.json"), + ("path", "svc/package.json")) + self.assertEqual(tool.git_header_path("diff --git a/svc b/old.json b/svc b/new.json"), ("rename", None)) + self.assertIsNone(tool.diff_identity("diff --git a/svc b/old.json b/svc b/new.json\n")) + renamed = ("diff --git a/svc b/old.json b/svc b/new.json\nsimilarity index 100%\n" + "rename from svc b/old.json\nrename to svc b/new.json\n") + self.assertIsNotNone(tool.diff_identity(renamed, {"svc b/new.json": BLOB})) + copied = renamed.replace("rename from", "copy from").replace("rename to", "copy to") + self.assertIsNotNone(tool.diff_identity(copied, {"svc b/new.json": BLOB})) + self.assertIsNone(tool.diff_identity('diff --git "a/svc b/x" "b/svc b/x"\n')) + + def git_blob(text): + return git(["hash-object", "--stdin"], input=text, capture_output=True).stdout.decode().strip() + + def git_diff(path, before, after): + with tempfile.TemporaryDirectory() as folder: + root = Path(folder) + git(["init", "-q"], cwd=root) + git(["config", "user.email", "t@example.com"], cwd=root) + git(["config", "user.name", "t"], cwd=root) + target = root / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(before) + git(["add", path], cwd=root) + git(["commit", "-q", "-m", "base"], cwd=root) + target.write_bytes(after) + return git(["diff"], cwd=root, capture_output=True).stdout.decode() + + def move(text, needle, insert): + left = text.replace(needle, insert, 1) + head, _sep, tail = text.rpartition(needle) + return left, head + insert + tail + + payload = """{ + "a": { + "k": 1, + "m": 1, + "n": 1, + "p": 2, + "q": 3, + "r": 4 + }, + "b": { + "k": 1, + "m": 1, + "n": 1, + "p": 2, + "q": 3, + "r": 4 + } +} +""".encode() + needle = b' "n": 1,\n "p": 2,\n' + insert = b' "n": 1,\n "x": true,\n "p": 2,\n' + for directory in ("svc b", "svc"): + path = directory + "/package.json" + before = payload + left, right = move(before, needle, insert) + old_diff, new_diff = git_diff(path, before, left), git_diff(path, before, right) + self.assertIn("diff --git a/" + path + " b/" + path, old_diff) + self.assertEqual(tool.diff_identity(old_diff), tool.diff_identity(new_diff)) + old_blob, new_blob = git_blob(left), git_blob(right) + self.assertNotEqual(tool.diff_identity(old_diff, {path: old_blob}), + tool.diff_identity(new_diff, {path: new_blob})) + # The root package.json blob is not the file that changed. + self.assertIsNone(tool.diff_identity(old_diff, {"package.json": old_blob})) + read = api([{"filename": path}], commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[], + blobs={"OLD": {path: old_blob, "package.json": BLOB}, + HEAD: {path: new_blob, "package.json": BLOB}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, + diffs({"OLD": old_diff, HEAD: new_diff})), + [tool.INDEPENDENT, tool.SECURITY]) + + def test_an_embedded_b_slash_splits_in_the_middle_and_rfind_does_not(self): + def old_split(line): + rest = line[len("diff --git a/"):] + return rest[rest.rfind(" b/") + 3:] + + same = "diff --git a/svc b/package.json b/svc b/package.json" + root = "diff --git a/package.json b/package.json" + self.assertEqual(old_split(same), "package.json") + self.assertEqual(tool.git_header_path(same), ("path", "svc b/package.json")) + self.assertNotEqual(tool.git_header_path(same)[1], old_split(same)) + self.assertEqual(tool.git_header_path(root), ("path", "package.json")) + body = "\n@@ -1 +1 @@\n-a\n+b\n" + full = same + body + self.assertIsNotNone(tool.diff_identity(full, {"svc b/package.json": BLOB})) + self.assertIsNone(tool.diff_identity(full, {"package.json": BLOB})) + self.assertIsNotNone(tool.diff_identity(root + body, {"package.json": BLOB})) + renamed = ("diff --git a/svc b/old.json b/svc b/new.json\n" + "rename from svc b/old.json\nrename to svc b/new.json\n") + copied = renamed.replace("rename from", "copy from").replace("rename to", "copy to") + for diff in (renamed, copied): + self.assertIsNotNone(tool.diff_identity(diff, {"svc b/new.json": BLOB})) + self.assertIsNone(tool.diff_identity(diff, {"new.json": BLOB})) + self.assertIsNone(tool.diff_identity("diff --git a/svc b/old.json b/svc b/new.json\n")) + self.assertIsNone(tool.diff_identity("diff --git a/left b/right\n@@ -1 +1 @@\n-a\n+b\n")) + + def test_tree_reads_follow_changed_paths_and_ignore_other_gitlinks(self): + prior = "1" * 40 + current = "2" * 40 + base_sha = "3" * 40 + + def tree_calls(path, siblings): + mapping = {path: BLOB, "vendor": ("commit", "d" * 40)} + for index in range(siblings): + mapping[f"side{index}/file.txt"] = BLOB + base = api([{"filename": path}], head=current, base_sha=base_sha, commit_shas=[prior, current], + extra_status={prior: [OK]}, force_pushes=[], + blobs={prior: mapping, current: mapping}) + seen = [] + + def read(endpoint): + seen.append(endpoint) + return base(endpoint) + diff = ADDED.replace("src/app.py", path) + self.assertEqual(tool.missing("o/n", "1", current, base_sha, read, diffs({prior: diff, current: diff})), []) + self.assertIn(f"repos/o/n/commits/{prior}", seen) + self.assertIn(f"repos/o/n/commits/{current}", seen) + self.assertFalse(any(endpoint.endswith("/commits/HEAD") or endpoint.endswith("/commits/OLD") for endpoint in seen)) + trees = sum("/git/trees/" in item for item in seen) + root_sha = read(f"repos/o/n/commits/{current}")["commit"]["tree"]["sha"] + listing = {item["path"]: item for item in read(f"repos/o/n/git/trees/{root_sha}")["tree"]} + self.assertEqual(listing["vendor"]["type"], "commit") + self.assertEqual(tool.path_oids("o/n", current, [path], read)[path], BLOB) + if siblings: + self.assertIn("side0", listing) + self.assertFalse(any(listing["side0"]["sha"] in endpoint for endpoint in seen)) + return trees + + self.assertEqual(tree_calls("pkg.json", 30), tree_calls("pkg.json", 0)) + self.assertEqual(tree_calls("pkg.json", 30), 2) + deep = tree_calls("a/b/c/d.txt", 30) + self.assertEqual(deep, tree_calls("a/b/c/d.txt", 0)) + self.assertEqual(deep, 8) + self.assertEqual(deep // 2, len("a/b/c/d.txt".split("/"))) + link = ("diff --git a/vendor b/vendor\nindex 1111111..2222222 160000\n--- a/vendor\n+++ b/vendor\n" + "@@ -1 +1 @@\n-Subproject commit " + ("a" * 40) + "\n+Subproject commit " + ("a" * 40) + "\n") + held = "e" * 40 + other = "d" * 40 + same = api([{"filename": "vendor"}], head=current, base_sha=base_sha, commit_shas=[prior, current], + extra_status={prior: [OK]}, force_pushes=[], + blobs={prior: {"vendor": ("commit", held), "other": ("commit", other)}, + current: {"vendor": ("commit", held), "other": ("commit", other)}}) + self.assertEqual(tool.missing("o/n", "1", current, base_sha, same, diffs({prior: link, current: link})), []) + self.assertEqual(tool.path_oids("o/n", current, ["vendor"], same)["vendor"], held) + self.assertNotEqual(tool.diff_identity(link, {"vendor": held}), + tool.diff_identity(link, {"vendor": "f" * 40})) + moved = api([{"filename": "vendor"}], head=current, base_sha=base_sha, commit_shas=[prior, current], + extra_status={prior: [OK]}, force_pushes=[], + blobs={prior: {"vendor": ("commit", held), "other": ("commit", other)}, + current: {"vendor": ("commit", "f" * 40), "other": ("commit", other)}}) + self.assertEqual(tool.missing("o/n", "1", current, base_sha, moved, diffs({prior: link, current: link})), + [tool.INDEPENDENT]) + self.assertEqual(tool.path_oids("o/n", current, ["vendor"], moved)["vendor"], "f" * 40) + + def test_moving_a_line_into_another_function_does_not_carry(self): + files = [{"filename": "src/app.py"}] + read = api(files, commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[]) + moved = diffs({"OLD": MOVED_FROM, HEAD: MOVED_TO}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, moved), [tool.INDEPENDENT]) + shifted = diffs({"OLD": ADDED, HEAD: SHIFTED}) + moved_lines = api(files, commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[], + blobs={"OLD": {"src/app.py": BLOB}, HEAD: {"src/app.py": OTHER}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", moved_lines, shifted), [tool.INDEPENDENT]) + touched = SHIFTED.replace(" return 1", " return 0", 1) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": ADDED, HEAD: touched})), + [tool.INDEPENDENT]) + def test_an_unchanged_diff_reuses_the_prior_success(self): files = [{"filename": "src/app.py"}] read = api(files, commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[]) - same = diffs({"OLD": ADDED, HEAD: SHIFTED}) + same = diffs({"OLD": ADDED, HEAD: ADDED}) self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, same), []) + shifted = diffs({"OLD": ADDED, HEAD: SHIFTED}) + moved_lines = api(files, commit_shas=["OLD", HEAD], extra_status={"OLD": [OK]}, force_pushes=[], + blobs={"OLD": {"src/app.py": BLOB}, HEAD: {"src/app.py": OTHER}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", moved_lines, shifted), [tool.INDEPENDENT]) indent = SHIFTED.replace("+ return 2", "+ return 2") self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": ADDED, HEAD: indent})), [tool.INDEPENDENT]) @@ -293,12 +904,17 @@ def read(endpoint): self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLDER": ADDED, "MID": SHIFTED, HEAD: SHIFTED})), [tool.INDEPENDENT]) - self.assertIn("repos/o/n/commits/MID/status?per_page=100", seen) + self.assertIn("repos/o/n/commits/MID/statuses?per_page=100", seen) def test_a_rebase_parent_can_carry_the_status(self): files = [{"filename": "src/app.py"}] read = api(files, commit_shas=[HEAD], force_pushes=["OLD"], extra_status={"OLD": [OK]}) + # The same blob means this file's bytes did not change. A line-number shift still carries. self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLD": ADDED, HEAD: SHIFTED})), []) + changed = api(files, commit_shas=[HEAD], force_pushes=["OLD"], extra_status={"OLD": [OK]}, + blobs={"OLD": {"src/app.py": BLOB}, HEAD: {"src/app.py": OTHER}}) + self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", changed, diffs({"OLD": ADDED, HEAD: SHIFTED})), + [tool.INDEPENDENT]) def test_security_reuses_the_same_diff_and_does_not_borrow_a_different_context(self): files = [{"filename": ".github/workflows/ci.yml", "patch": "@@\n-run: a\n+run: b"}] @@ -319,7 +935,7 @@ def read(endpoint): return base(endpoint) self.assertEqual(tool.missing("o/n", "1", HEAD, "BASE", read, diffs({"OLDER": ADDED, HEAD: SHIFTED})), [tool.INDEPENDENT]) - self.assertIn("repos/o/n/commits/MID/status?per_page=100", seen) + self.assertIn("repos/o/n/commits/MID/statuses?per_page=100", seen) def test_a_short_commit_list_does_not_carry(self): files = [{"filename": "src/app.py"}] @@ -375,14 +991,24 @@ def boom(endpoint): def test_diff_identity_ignores_position_and_keeps_whitespace_and_binary_blobs(self): self.assertEqual(tool.diff_identity(ADDED), tool.diff_identity(SHIFTED)) + self.assertEqual(tool.diff_identity(ADDED, {"src/app.py": BLOB}), + tool.diff_identity(SHIFTED, {"src/app.py": BLOB})) + self.assertNotEqual(tool.diff_identity(ADDED, {"src/app.py": BLOB}), + tool.diff_identity(SHIFTED, {"src/app.py": OTHER})) + self.assertNotEqual(tool.diff_identity(MOVED_FROM), tool.diff_identity(MOVED_TO)) + touched = SHIFTED.replace(" return 1", " return 0", 1) + self.assertNotEqual(tool.diff_identity(ADDED), tool.diff_identity(touched)) self.assertNotEqual(tool.diff_identity(ADDED), tool.diff_identity(SHIFTED.replace("+ return 2", "+ return 2"))) self.assertEqual(tool.diff_identity(""), "empty") self.assertIsNone(tool.diff_identity('{"message":"diff too large"}')) self.assertIsNone(tool.diff_identity("diff --git a/a b/a\nnot a diff line\n")) binary = "diff --git a/a.bin b/a.bin\nindex 1111111..2222222 100644\nBinary files a/a.bin and b/a.bin differ\n" other = binary.replace("1111111..2222222", "1111111..3333333") - self.assertNotEqual(tool.diff_identity(binary), tool.diff_identity(other)) - self.assertIsNone(tool.diff_identity(binary.replace("index 1111111..2222222 100644\n", ""))) + self.assertEqual(tool.diff_identity(binary), tool.diff_identity(other)) + self.assertIsNotNone(tool.diff_identity(binary.replace("index 1111111..2222222 100644\n", ""))) + self.assertIsNone(tool.diff_identity(binary, {"a.bin": "1111111"})) + self.assertNotEqual(tool.diff_identity(binary, {"a.bin": "a" * 40}), + tool.diff_identity(other, {"a.bin": "b" * 40})) swapped = SHIFTED + ADDED.replace("src/app.py", "src/b.py") reverse = ADDED.replace("src/app.py", "src/b.py") + SHIFTED self.assertEqual(tool.diff_identity(swapped), tool.diff_identity(reverse)) @@ -398,6 +1024,20 @@ def test_diff_identity_ignores_position_and_keeps_whitespace_and_binary_blobs(se self.assertNotEqual(tool.diff_identity(one_line), tool.diff_identity(two_lines)) self.assertIsNotNone(tool.diff_identity("diff --git a/a b/a\n@@ -1 +1 @@\n \x0c\n+x\n")) + def test_comment_edit_fields_come_from_graphql(self): + self.assertIsNone(tool.comment_list_target("repos/o/n")) + self.assertEqual(tool.comment_list_target("repos/o/n/issues/12/comments?per_page=100"), ("o/n", "12")) + node = {"author": {"login": "Owner"}, "body": "x", "createdAt": "t", "updatedAt": "t", + "lastEditedAt": None, "includesCreatedEdit": False} + self.assertEqual(tool.comment_record(node)["last_edited_at"], None) + self.assertFalse(tool.comment_record(node)["includes_created_edit"]) + edited = dict(node, lastEditedAt="t", includesCreatedEdit=True) + self.assertEqual(tool.comment_record(edited)["last_edited_at"], "t") + self.assertTrue(tool.comment_record(edited)["includes_created_edit"]) + for broken in ({}, {"lastEditedAt": None}, {"includesCreatedEdit": False}): + with self.assertRaises(ValueError): + tool.comment_record(broken) + def test_owner_comment_matches_only_this_head(self): head = "a" * 40 other = "b" * 40 @@ -408,7 +1048,20 @@ def check(comments, expected, **kwargs): read = api(files, reviews, comments=comments, head=head, **kwargs) self.assertEqual(tool.missing("o/n", "1", head, "BASE", read), expected) + fresh = "2020-01-01T00:00:00Z" check([comment(f"한쪽 문 승인: {head} 메모", login="Owner")], [], owner="owner") + check([comment(f"되돌리기 어려움 승인: {head}", created_at=fresh, updated_at=fresh)], []) + check([comment(f"한쪽 문 승인: {head}", created_at=fresh, updated_at=fresh)], []) + check([comment(f"되돌리기 어려움 승인: {head}", updated_at="2020-01-02T00:00:00Z")], [tool.OWNER]) + check([comment(f"되돌리기 어려움 승인: {head}", last_edited_at=fresh)], [tool.OWNER]) + check([comment(f"한쪽 문 승인: {head}", includes_created_edit=True)], [tool.OWNER]) + check([comment(f"되돌리기 어려움 승인: {head}", omit_edit=True)], [tool.OWNER]) + check([comment(f"한쪽 문 승인: {head}", updated_at="2020-01-01T00:00:00+00:00")], [tool.OWNER]) + check([comment(f"되돌리기 어려움 승인: {head}", omit_times=True)], [tool.OWNER]) + check([ + comment(f"되돌리기 어려움 승인: {head}", updated_at="2020-01-02T00:00:00Z"), + comment(f"한쪽 문 승인: {head}"), + ], []) check([comment(f"한쪽 문 승인: {head}\r\n메모")], []) check([comment(f"한쪽 문 승인: {other}")], [tool.OWNER]) check([comment(f"되돌리기 어려움 승인: {head}")], [])