policy(land): gate rework on post-approval threads - #881
Conversation
Coverage Report
File CoverageNo changed files found. |
moncher-dev
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f236392161
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
moncher-dev
left a comment
moncher-dev
left a comment
There was a problem hiding this comment.
Reviewed at head f2363921. The Land-lane half of this change is correct, but WORKFLOW.md Step 0 item 4 has a blocking regression. Returning to Ready.
What passes
- The query is now sufficient for the policy.
LandContextgainsreviewThreads.comments(first: 1) { createdAt }, andreviewsalready carriedsubmittedAt, so both sides of the comparison the policy requires are fetchable. I ran the query at this head against the live API with the exact text from.codex/skills/land/SKILL.md; it returnsreviews[].submittedAtand per-threadcomments.nodes[].createdAtas expected. - The change does not lean on
isOutdatedas a proxy for "created after the approval". Correct —isOutdatedonly means the code under the thread moved. - Pre-flight 1's base-merge exemption survives verbatim ("A merge-update never invalidates an approval under this policy"), as does the
Landfailure-classification copy at WORKFLOW.md ~277. - The merged-PR precedence guard still runs first in both files (SKILL.md "immediately after loading Required Context and before every pre-flight check", WORKFLOW.md Step 0 item 2, Land classification "always first").
node packages/cli/dist/index.js workflow validatepasses on this head — front matter parses, so the live-policy swap on merge is not at risk.- Diff size is in line with the issue: a few lines in each of three files plus a test. The
Status Maptable's column re-padding is Prettier reflow from the longerLandcell, not scope creep.
Blocking: the guard no longer fires when there is no approval at all
WORKFLOW.md:168 previously read "unresolved actionable review threads" unconditionally. It now reads:
an unresolved actionable review thread created after the latest qualifying human approval on the current head (compare the thread's first comment
createdAtwith the approval review'ssubmittedAt) ... An unresolved actionable thread created at or before that approval is absorbed by the approval and does not trigger rework.
Both branches presuppose that a qualifying approval exists. The Ready-return rework guard runs on every Ready entry, including entries that never passed through Land and were never approved.
Repro — this is the board's most common rework path, exercised every review cycle:
- Worker opens a PR and moves the issue to
In review. - A human reviews with a
COMMENTEDreview carrying an unresolved actionable thread (noAPPROVED, noCHANGES_REQUESTED— GitHub's own review UI produces exactly this when a reviewer submits comments without a decision). - The human moves the issue
In review→Ready. - The worker runs Step 0. Item 4: there is no "latest qualifying human approval on the current head", so neither the "created after" clause nor the "at or before ... absorbed" clause can be evaluated.
CHANGES_REQUESTEDis absent, and this was not aLand→Readytransition, so no Land-return rework marker exists. - Item 6 therefore matches — "no actionable feedback ... proceed to Step 1 normally as a fresh pickup".
The worker skips the rework preamble (Step 2.2) and its obligations to answer and resolve every thread. Before this PR the same scenario matched item 4 and entered rework. This is a live regression against the reviewing loop this repository runs on, and it is the same point chatgpt-codex-connector raised in the unresolved thread on WORKFLOW.md:168.
Note the Land lane is unaffected: pre-flight 1 requires an approval before Land can proceed at all, so SKILL.md Failure Handling 7 and the WORKFLOW.md Land classification always have an approval to compare against. The defect is specific to the Ready-return guard.
Suggested shape
Scope the timestamp exception to the case it was decided for: when a qualifying human approval exists on the current head, only threads whose first comment createdAt is later than its submittedAt trigger rework; when no qualifying approval exists, any unresolved actionable thread triggers rework as before. Please keep the change to the same three files.
|
Re-reviewed at head The fix
This is the right shape for a policy doc: the default is rework, and the exception must be affirmatively established. If a worker is unsure whether a qualifying approval exists, it falls to "no qualifying approval" and reworks — the conservative outcome. The dangerous direction (silently skipping the rework preamble) is no longer reachable. Repro re-run — the no-approval path specificallyTracing the exact scenario that failed before, against the file text at this head:
Item 5 runs, the rework preamble executes. Previously this fell through to item 6 as a fresh pickup. The post-approval path still behaves as you decided (option 3): with a qualifying approval on the current head, a thread whose first comment predates Everything from the previous head re-confirmed at
|
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
dd49dd4 기준으로 리뷰했습니다. Approve — 이슈 #878의 완료 조건 6개가 모두 충족되고, 지난 라운드의 blocking 회귀가 실제로 고쳐졌습니다. 인라인 4건은 전부 비-blocking(P2 하나, P3 둘, nit 하나)이라 이 PR에서 고치든 후속으로 빼든 판단에 맡깁니다.
이전 라운드 처리 확인
chatgpt-codex-connector의 P2와moncher-dev의 blocking 지적은 같은 결함이었습니다 — 승인이 아예 없는 경로에서 Ready 복귀 가드가 발화하지 않음.dd49dd4가WORKFLOW.md:168에 no-approval fallback을 추가해 해결했고, 스레드에 근거 있는 답변이 달린 뒤 resolve됐습니다. 회귀 어서션(land.test.ts:115-118)도 함께 들어왔습니다.- 지적한 시나리오를 문언으로 다시 밟았습니다:
COMMENTED리뷰 + 미해결 actionable 스레드 + 승인 없음 → "When no qualifying approval exists, any unresolved actionable review thread triggers rework as before" → 항목 4 발화 → 항목 6으로 안 빠짐. 고쳐졌습니다.
완료 조건 대조
| 완료 조건 | 결과 | 근거 |
|---|---|---|
| 승인 있고 미해결 스레드가 전부 승인보다 먼저 → 머지 | ✅ | SKILL.md:128 말미 + :180 FH7 |
승인 이후 스레드 하나라도 있으면 Land → Ready |
✅ | SKILL.md:180, WORKFLOW.md:281 |
| 쿼리가 판정에 필요한 시각 필드를 실제로 가져옴 | 시각 필드는 충족. actionable 판정은 미충족 — 인라인 P3 참조 |
|
세 파일 문구 무모순 (WORKFLOW.md:63, :168, SKILL.md:175) |
의미는 일치. qualifying 미정의 — 인라인 P2 참조 |
|
| 베이스 머지가 승인을 무효화하지 않는 규칙 유지 | ✅ | SKILL.md:128 "A merge-update never invalidates an approval" 원문 유지, WORKFLOW.md:279 무변경 |
| Merged-PR 우선 가드가 항상 먼저 | ✅ | SKILL.md의 ## Merged-PR Precedence Guard가 ## Pre-flight Checks보다 앞, WORKFLOW.md Step 0 항목 2가 항목 4보다 앞 — 기존 테스트 2건이 순서를 강제 |
Land 레인은 no-approval fallback이 없어도 됩니다. pre-flight 1이 승인 없이는 통과하지 못하므로 FH7과 WORKFLOW.md:281은 항상 비교할 승인을 갖습니다. 결함 아님.
오버엔지니어링 / 임의 결정
없습니다.
- 3파일
+38 / -12. 이슈가 못박은 "각각 몇 줄" 예산 안입니다. - Status Map 표의 컬럼 재정렬은
Land셀이 길어져 생긴 Prettier reflow입니다. 범위 확장이 아닙니다. - 채택되지 않은 선택지 1·2·4의 흔적이 없고,
isOutdated를 시각 대용으로 쓰지 않았습니다 —isOutdated는 스레드 아래 코드가 움직였다는 뜻일 뿐이라 다른 개념이고, 이슈도 범위 밖으로 명시했습니다. - Changeset 없음이 맞습니다. #878에 changeset 라벨이 없고 런타임 동작 변경이 없습니다.
- 참고:
packages/cli/src/skills/templates/land.ts(배포되는 CLI land 스킬)는 원래부터 리뷰 스레드 게이트가 없는 단순gh pr merge템플릿입니다. 이 PR이 만든 괴리가 아니고, 손댔다면 오히려 범위 초과였습니다.
Smoke Test
Preview 환경이 없고 런타임 코드 변경이 없는 정책 PR이라, 로컬에서 dd49dd4를 체크아웃해 확인했습니다.
node packages/cli/dist/index.js workflow validate— 통과. front matter 파싱 정상. 이슈가 "파싱이 깨지면 오케스트레이션이 멈춥니다"라고 경고한 지점이라 별도로 확인했습니다.pnpm test— 41개 파일 567 테스트 전부 통과.pnpm exec vitest run packages/cli/src/skills/templates/land.test.ts— 5 통과.pnpm lint/pnpm typecheck/pnpm build— 전부 통과.pnpm exec prettier --check(변경된 3파일) — 통과. 리포지토리 전체pnpm format은 72파일에서 실패하지만pnpm-lock.yaml·prettier.config.mjs등 이 PR이 건드리지 않은 파일들이라 기존 상태입니다.- CI:
Test,Container Smoke둘 다 success.mergeable_state: clean. WORKFLOW.md:432의prReviewState쿼리가reviews{...submittedAt commit{oid}}와reviewThreads{...comments(first:30){nodes{...createdAt}}}를 이미 가져오므로, Ready 복귀 가드도 새 판정에 필요한 데이터를 확보합니다 —LandContext만 고치고 Ready 쪽 쿼리를 방치한 게 아닌지 별도로 확인했습니다.
Generated by Claude Code
| 2. **Merged-PR precedence guard:** if the current delivery PR is `MERGED`, refresh its merged commit SHA into the current workpad when one exists, prepare the `🔁 Status: Ready → Done` body, and send it as `comment_body` through `/gh-project`. After confirmed readback, append the matching workpad Status Transitions line when the worker remains alive, then exit. Do not open a new cycle, inspect review feedback, or enter any rework path. | ||
| 3. For each remaining open linked PR, read `reviewDecision`, latest human reviews, review threads (`prReviewState` query), top-level PR comments, and recent issue comments. | ||
| 4. If any open linked PR has `CHANGES_REQUESTED`, unresolved actionable review threads, a human instruction indicating rework, or a recent `Land` → `Ready` transition recorded as a **Land-return rework**, this `Ready` state means **review rework return** — not a fresh pickup and not drift. | ||
| 4. If any open linked PR has `CHANGES_REQUESTED`, an unresolved actionable review thread, a human instruction indicating rework, or a recent `Land` → `Ready` transition recorded as a **Land-return rework**, this `Ready` state means **review rework return** — not a fresh pickup and not drift. When a latest qualifying human approval exists on the current head, narrow the thread condition to unresolved actionable threads created after that approval (compare the thread's first comment `createdAt` with the approval review's `submittedAt`); a thread created at or before that approval is absorbed by the approval and does not trigger rework. When no qualifying approval exists, any unresolved actionable review thread triggers rework as before. |
There was a problem hiding this comment.
P2 — qualifying가 WORKFLOW.md 안에서 정의되지 않습니다.
이 문장은 "latest qualifying human approval"에 판정을 걸지만, 무엇이 승인을 qualifying하게 만드는지는 .codex/skills/land/SKILL.md:128에만 있습니다 (사람일 것, 오케스트레이션 계정이 아닐 것, commit.oid == headRefOid, 그리고 베이스 머지 커밋 조상 예외). Ready 복귀 가드는 Ready 상태에서 도는데 그 시점에 /land 스킬은 로드되지 않고, WORKFLOW.md는 이 용어를 어디서도 정의하지 않습니다. 바로 아래 :281의 Land 분류가 같은 용어를 쓰는데도요.
비교 대상인 WORKFLOW.md:279는 같은 개념을 풀어서 씁니다 — "no human APPROVED review on the current head (and the merge-update exemption above does not apply)". 즉 이 문서는 이미 정의를 인라인하는 관례를 갖고 있습니다.
실제 갈리는 지점:
- 베이스 머지로 헤드가 A→B로 바뀐 뒤 A에 달린 승인이 여전히 qualifying인가?
SKILL.md는 예(명시적 면제),WORKFLOW.md는 침묵. - 오케스트레이션 계정의 승인이 qualifying인가?
SKILL.md는 아니오,WORKFLOW.md는 침묵.
과하게 넓게 읽으면 예전 동작(안전)으로 떨어지지만, 좁게 읽으면 — 자격 없는 승인을 qualifying으로 취급하면 — 승인 이전 스레드가 전부 흡수되어 지난 라운드에서 잡힌 것과 같은 종류의 회귀가 됩니다. 하필 그 회귀가 났던 바로 그 문장입니다.
제안: 한 구절만 인라인하면 됩니다. 예 — "a human (not the orchestration account) APPROVED review whose commit.oid is the current headRefOid". 세 파일 각각 몇 줄이라는 이슈의 diff 예산 안에 들어갑니다.
nit — 규칙을 먼저 단언하고 뒤에서 좁히는 구조.
항목 4의 첫 문장이 "an unresolved actionable review thread"만으로 이미 rework return이라는 결론을 냅니다. 좁히는 조건은 그 다음 두 문장에 옵니다. 이 문서는 매 턴 에이전트가 읽고 그대로 실행하는 정책이고, 결론이 먼저 나온 뒤 뒤집히는 형태는 오독하기 쉬운 형태입니다 (그리고 이 항목은 이미 한 번 오독 사고가 난 항목입니다). 목록 항목 안에서 조건을 한 번에 서술하는 쪽이 안전합니다.
두 지적 모두 blocking은 아닙니다. 승인합니다.
Generated by Claude Code
| All must pass before merging. If any fails, record the failure in the workpad and **do not** merge. | ||
|
|
||
| 1. **At least one human approval on the current head.** A review with `state == APPROVED` from a human (not the orchestration account) whose `commit.oid` is the current `headRefOid`, **or** whose approved commit is an ancestor of the current head where every commit after it is a base-branch merge commit produced by this Land cycle (server-side `updatePullRequestBranch` or a trivial-conflict `git merge`). A merge-update never invalidates an approval under this policy; if branch protection dismissed it anyway, that is an external wait (Failure Handling 6). | ||
| 1. **At least one human approval on the current head.** A review with `state == APPROVED` from a human (not the orchestration account) whose `commit.oid` is the current `headRefOid`, **or** whose approved commit is an ancestor of the current head where every commit after it is a base-branch merge commit produced by this Land cycle (server-side `updatePullRequestBranch` or a trivial-conflict `git merge`). A merge-update never invalidates an approval under this policy; if branch protection dismissed it anyway, that is an external wait (Failure Handling 6). Save the latest qualifying approval's `submittedAt`; any unresolved actionable review thread whose first comment's `createdAt` is later than that approval fails Land as rework, while unresolved actionable threads created at or before the approval do not block Land. |
There was a problem hiding this comment.
P3 — pre-flight 1이 두 개의 서로 다른 실패 분류를 갖게 됐습니다.
이슈가 "기존 pre-flight 항목의 번호와 순서를 바꾸지 마세요"라고 했으니 항목 1에 붙인 선택 자체는 타당합니다. 다만 결과적으로 항목 1은 이제 결과가 갈리는 두 가지 이유로 실패할 수 있습니다.
- 승인 자체가 없음 → Failure Handling 6 →
Land→In review(외부 대기) - 승인 이후 생성된 미해결 actionable 스레드 → Failure Handling 7 →
Land→Ready(재작업)
## Pre-flight Checks 머리말은 "If any fails, record the failure in the workpad and do not merge"라고만 하고 분류는 Failure Handling에 위임합니다. 후자로 실패한 에이전트가 "pre-flight 1(승인 게이트)이 실패했다"에서 출발하면 FH6로 잘못 라우팅될 여지가 있습니다 — 그러면 재작업 대신 사람 대기로 떨어져 이슈가 조용히 멈춥니다.
실제로는 판정 가능합니다. 이 문장이 "fails Land as rework"라고 명시하고, FH6/FH7이 각각 구체 조건을 열거하니까요. 그래서 P3입니다. 다만 FH7을 명시적으로 가리키는 한 마디("→ Failure Handling 7")를 붙이면 추론 없이 결정됩니다.
Generated by Claude Code
| comments(first: 1) { | ||
| nodes { | ||
| createdAt | ||
| } | ||
| } |
There was a problem hiding this comment.
P3 — 시각 필드는 채워졌지만 actionable 판정은 여전히 쿼리만으로 불가능합니다.
이슈 완료 조건 3은 "LandContext 쿼리가 이 판정에 필요한 시각 필드를 실제로 가져오며, 쿼리만 보고 판정 가능함이 확인된다"입니다. 시각 쪽은 이 추가로 충족됩니다 — 라이브 API에서 확인했고, reviews[].submittedAt(이미 있음)과 스레드별 comments.nodes[].createdAt이 둘 다 돌아옵니다.
남는 절반은 actionable입니다. reviewThreads가 가져오는 건 id, isResolved, isOutdated, path, 그리고 이제 createdAt 하나뿐입니다. 코멘트 body도 author.login도 없습니다. 그래서 Land 에이전트는 이 쿼리만으로
- 실제 수정 요구인 스레드와 봇 nit / 칭찬 코멘트를 구분할 수 없고,
- 오케스트레이션 계정이 스스로 연 스레드를 걸러낼 수 없습니다.
두 번째가 이 PR에서 더 무거워집니다. 이제 스레드 하나의 createdAt 비교 결과가 머지 여부를 직접 가르는데, 그 스레드가 애초에 게이트 대상인지는 여전히 추측이거나 두 번째 쿼리에 의존합니다.
이건 이 PR이 만든 결함이 아닙니다 — unresolved actionable review feedback은 변경 전 FH7에도 있었고, 필드를 더 넣으면 이슈가 못박은 diff 예산을 넘습니다. 그래서 blocking으로 보지 않습니다. 다만 완료 조건 3이 직접 겨냥하는 지점이라, 못 채운 게 아니라 의식적으로 미룬 것으로 남기려고 적습니다. 후속 이슈감으로 적당합니다.
Generated by Claude Code
| expect(landSkill).toContain("comments(first: 1)"); | ||
| expect(landSkill).toContain("createdAt"); | ||
| expect(landSkill).toContain("approval's `submittedAt`"); |
There was a problem hiding this comment.
nit — toContain("createdAt")는 단독으로는 거의 아무것도 막지 않습니다.
바로 위 toContain("comments(first: 1)")가 쿼리 추가를 실제로 붙잡고 있어서 이 줄이 없어도 회귀는 잡힙니다. 반대로 이 줄만 있으면, 쿼리 필드가 지워져도 FH7 본문의 "compare its first comment's createdAt" 때문에 통과합니다. 지금 .codex/skills/land/SKILL.md에서 createdAt은 81행(쿼리)과 180행(FH7) 두 곳뿐이라 실제로 그렇습니다.
toContain("comments(first: 1) {") 같은 형태로 합치거나, 정말 쿼리 블록을 겨냥한다면 기존 section() 헬퍼로 쿼리 구간을 잘라 그 안에서 확인하는 편이 의도에 맞습니다.
또 이 테스트가 커버하지 않는 곳이 둘 있습니다 — WORKFLOW.md:63 Land 레인 문구와 SKILL.md:128 pre-flight 1의 경계 문장입니다. 둘 다 이슈 완료 조건("세 파일의 문구가 서로 모순되지 않는다")이 직접 지목한 지점인데, 지금은 지워져도 테스트가 초록입니다. 각각 한 줄짜리 toContain이면 충분합니다.
전부 nit입니다. blocking 아닙니다.
Generated by Claude Code
2 participants
Issues
Summary
createdAtfor direct comparison with the approval review'ssubmittedAt.Change-point diagram
createdAtto approvalsubmittedAt→ after approval: rework; at/before approval: absorbedStart here
.codex/skills/land/SKILL.md:73— timestamp-bearingLandContextreview-thread query.codex/skills/land/SKILL.md:125— Land pre-flight timestamp decisionWORKFLOW.md:168— matching Ready-return guard with the no-approval fallbackUser-Visible Behavior / Operational Impact
Validation
pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— passpnpm exec vitest run packages/cli/src/skills/templates/land.test.ts— pass (5 tests)pnpm exec prettier --check WORKFLOW.md packages/cli/src/skills/templates/land.test.ts— passnode packages/cli/dist/index.js workflow validate— pass; front matter and prompt checks validgit diff --check origin/main...HEAD— passChangeset
Risks & rollback
Changed files
.codex/skills/land/SKILL.md— fetch thread creation time and apply the approval-time boundary in Land pre-flight/failure handlingWORKFLOW.md— align the Land lane, Ready-return guard (including no-approval rework), and Land failure classificationpackages/cli/src/skills/templates/land.test.ts— assert query sufficiency, before/after semantics, and the no-approval fallbackPost-merge / human validation
Security
.envfiles, or generated installation tokens are committed