Skip to content

feat(worker): check routability between turns - #771

Merged
moncher-dev merged 8 commits into
mainfrom
feat/722-routable-turn-check
Aug 30, 2026
Merged

feat(worker): check routability between turns#771
moncher-dev merged 8 commits into
mainfrom
feat/722-routable-turn-check

Conversation

@moncher-dev

@moncher-dev moncher-dev commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

TL;DR

PR #771 makes tracker state reads routable at every worker turn boundary. A run now stops cleanly when the refreshed issue is no longer dispatchable or loses a required label.

Change-point diagram

worker turn boundarytracker-state readfresh normalized snapshotissueRoutablecontinue | clean routability stop

Start here

  • packages/worker/src/turn-lease.ts consumes the per-turn routability result.
  • packages/orchestrator/src/service.ts derives lifecycle state and routability from the same refreshed snapshot.
  • e2e/run-e2e.sh and e2e/stub-worker.ts cover both required-label boundary cases.

Changes

  • Preserve refreshed rate-limit accounting even when a tracker snapshot is filtered out.
  • Repair orchestration test isolation and event-order assertions for the workflow-unavailable state-read path.
  • Remove and ignore the temporary Docker E2E synchronization signal after each run.
  • Retain the earlier worker, documentation, and E2E implementation for required-label routability.

Evidence

  • pnpm build — pass
  • pnpm test --silent — pass
  • pnpm lint — pass
  • pnpm typecheck — pass
  • ./e2e/run-e2e.sh required-label-missing 45 — pass; candidate was not dispatched
  • ./e2e/run-e2e.sh required-label-removed 45 — pass; turn two was prevented after label removal

Risks and rollback

  • The extra refreshed tracker read remains bounded by the worker turn-lease timeout. Revert this PR to restore the prior state-only turn-boundary behavior.

Changed files

  • Worker/orchestrator routing and tests
  • README and configuration documentation
  • Docker E2E fixtures, runner, scenario guide, and ignored runtime signal

Post-merge / Human validation

  • Confirm production tracker credentials and latency remain within the turn-boundary request budget.
  • Confirm GitHub and Linear label updates result in the expected clean worker stop.

Issues — Closed #722

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 40.81% 44202 / 108294
🔵 Statements 40.81% 44202 / 108294
🔵 Functions 72.83% 2169 / 2978
🔵 Branches 78.42% 10060 / 12827
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
e2e/stub-worker.ts 0% 100% 100% 0% 18-454
packages/core/src/contracts/tracker-adapter.ts 53.84% 100% 0% 53.84% 14-19
packages/core/src/observability/structured-events.ts 100% 100% 100% 100%
packages/orchestrator/src/service.ts 91.58% 83.83% 95.69% 91.58% 247-248, 271-273, 315, 449-450, 455-456, 478, 482, 486, 523-527, 550-568, 597-611, 744-745, 750-751, 776-777, 848-849, 885-886, 898-899, 904-915, 917-918, 950-967, 989-996, 1018-1019, 1031-1032, 1039, 1046-1050, 1066, 1076, 1107, 1133-1134, 1172-1174, 1186-1187, 1194, 1245-1246, 1407, 1409, 1564-1565, 1577-1579, 1707, 1722, 1732-1739, 1747-1751, 1798, 1872-1873, 1887-1897, 1901, 1926, 1939-1940, 1955-1956, 1983, 2029, 2061-2065, 2133, 2164-2165, 2169-2173, 2177-2178, 2187-2188, 2203, 2230, 2255, 2258-2261, 2384-2400, 2476-2481, 2483-2488, 2552-2553, 2572-2573, 2620-2623, 2702-2703, 2710-2711, 2740-2741, 2775-2778, 2846, 2881-2884, 2919-2922, 2976-2979, 3088-3098, 3177, 3185-3186, 3205-3206, 3225-3229, 3237-3238, 3268, 3280-3285, 3315-3316, 3359, 3362, 3454, 3747, 3783, 3872, 3874, 3877, 3879-3881, 3884-3885, 3904, 3969-3970, 3975-3979, 3983, 3996-3997, 4160-4164, 4174-4176, 4222-4223, 4253-4259, 4319-4323, 4445-4446, 4501-4502, 4532-4533, 4662-4670, 4694-4701, 4719-4722, 4853, 4867-4873, 4925-4931, 4958, 4968, 5008-5011, 5018-5024, 5150, 5153, 5160-5163, 5166-5171, 5264-5265, 5277-5286, 5346-5347, 5380-5381, 5439-5440, 5601-5602, 5666-5673, 5676-5680, 5762-5763, 5776, 5815-5816, 5835-5836, 5838-5839, 5841-5842, 5844-5845, 5852-5853, 5874-5875, 5979-5980, 5983-5984, 6004, 6012-6013, 6021-6022, 6083-6086, 6116, 6223-6224, 6292-6294, 6300-6301, 6314-6315, 6320-6326, 6387-6389, 6465-6466
packages/worker/src/turn-lease.ts 93.38% 76.31% 100% 93.38% 17-18, 45-46, 136-137, 143-144
Generated in workflow #1158 for commit b69c951 by the Vitest Coverage Report Action

@moncher-dev
moncher-dev marked this pull request as ready for review August 30, 2026 02:09
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-30T02:12:59.030299Z ef84aeb Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef84aeba00

ℹ️ 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".

Comment thread packages/orchestrator/src/service.ts Outdated
Comment on lines +584 to +589
result = {
...result,
routable: issueRoutable(
refreshedIssue,
workflowResolution.lifecycle
).routable,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Derive state and routability from one snapshot

When the tracker state changes between requestState and this second fetch, the response combines the old result.state with routability from refreshedIssue. For example, if the first read returns In progress and the refreshed snapshot is already Done with its labels intact, the worker receives state: "In progress", routable: true and starts another coding turn on a completed issue. Replace the returned state with refreshedIssue.state (or otherwise derive both decisions from one snapshot) so the required between-turn check is atomic.

AGENTS.md reference: AGENTS.md:L5-L7

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f6d8830. The confirmed response now replaces both state and routability from the same freshly normalized snapshot; the initial state-read can no longer combine an old state with new labels.

Comment thread e2e/run-e2e.sh Outdated
if [ -z "$SCENARIO_RUN_ID" ]; then
SCENARIO_RUN_ID=$(echo "$STATUS_JSON" | python3 -c "import sys,json;d=json.load(sys.stdin);r=d['activeRuns'];print(r[0].get('runId','') if r else '')" 2>/dev/null || echo "")
fi
if [ "$SCENARIO" = "required-label-removed" ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exercise the worker turn boundary in this scenario

This removes the label and immediately triggers reconciliation as soon as the first worker is merely running; moreover, required-label-removed is not a recognized stub-worker scenario and falls back to happy. The existing reconciliation path can therefore supply the asserted SIGTERM before any turn completes, so this Docker test still passes if the newly added refreshTrackerState turn-boundary behavior is broken. Use a deterministic multi-turn scenario, remove the label after turn one, and assert that turn two never starts.

AGENTS.md reference: AGENTS.md:L29-L33

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c03af8e. required-label-removed is now a deterministic two-turn stub: it logs completion of turn one, invokes the actual worker refreshTrackerState, and asserts that turn two is prevented after the label is removed.

Comment thread README.md Outdated
`gh-symphony repo start --assigned-only` also applies to Linear trackers. It is an input to the Linear adapter's `dispatchable` derivation: the adapter keeps candidate issues observable, compares each returned `assignee.id` with the authenticated viewer, and marks nonmatching or unassigned issues non-dispatchable. With a personal API key this viewer is that person; with a service-account key it is the service account. Symphony does not fail fast because Linear does not expose enough token metadata in the issue query path to distinguish those cases reliably.

Linear workflows may also configure `tracker.provider.pickup_labels.include` and `tracker.provider.pickup_labels.exclude` as routing gates. Excluded labels always win; when include labels are configured, an issue needs at least one include label before a worker starts. Label comparison is case-insensitive and ignores surrounding whitespace, so labels that differ only by case or outer whitespace cannot be used as separate gates. Removing a required routing label blocks new dispatches and due retries; on the next reconciliation tick it terminates an already running worker while preserving its workspace for recovery.
GitHub and Linear workflows may configure `tracker.provider.pickup_labels.include` and `tracker.provider.pickup_labels.exclude` as candidate filters. Excluded labels always win; when include labels are configured, an issue needs any one include label before it is considered for dispatch. This pre-filter does not terminate an already-running worker when its labels change. By contrast, `tracker.required_labels` is an all-of routability gate: removing one blocks new dispatches and due retries, and the worker stops before its next turn after a refreshed tracker read reports the issue is no longer routable. Label comparison is case-insensitive and ignores surrounding whitespace, so labels that differ only by case or outer whitespace cannot be used as separate gates.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct the Linear pickup-label lifecycle claim

This assurance is false for Linear: packages/tracker-linear/src/orchestrator-adapter.ts calls ID refresh with applyPickupLabels: true, and packages/orchestrator/src/service.ts terminates an active process when that filtered refresh no longer returns the issue. Thus removing the sole included pickup label can stop an already-running Linear worker despite this new documentation promising otherwise; either keep pickup labels out of Linear ID refreshes or document the actual lifecycle behavior.

AGENTS.md reference: AGENTS.md:L29-L34

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f6d8830. README now distinguishes GitHub from Linear: Linear applies pickup filtering during ID refreshes, so removing the sole included label can stop an active worker during reconciliation.

@hojinzs
hojinzs self-requested a review August 30, 2026 03:26

@hojinzs hojinzs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약 (1/6 라운드) — Request changes

#722 의 요구사항 자체는 빠짐없이 반영되어 있습니다. 다만 핵심 로직이 검증되지 않은 상태이고, 기존 Codex 지적 3건이 모두 미응답으로 남아 있어 변경 요청드립니다.

요구사항 대조 (#722)

요구사항 상태
turn-lease.ts 재검사를 state → issueRoutable 로 확장
README.md:397 (repo init 이 셋 다 적용) 정정
README.md:593 (라벨 변경이 워커를 중단하지 않음) 정정 ⚠️ 정정했으나 Linear 에서 새 서술이 사실과 다름 (Codex #discussion_r3888236827)
docs/configuration.md required vs pickup 관계
Docker E2E 2 시나리오 + AGENT_TEST.md 갱신 ⚠️ 추가되었으나 신규 경로를 실행하지 않음
changeset .changeset/turn-routability.md (patch)

범위를 넘는 오버엔지니어링은 없습니다. 변경은 타이트합니다. 다만 성공한 read 를 ok: false 로 뒤집는 결정은 이슈에 명시되지 않은 임의 판단이고, 그 부작용이 아래 P2 두 건으로 나타납니다.

Smoke test 결과

Preview 환경이 없어 로컬에서 수행했습니다.

항목 결과
pnpm build ✅ pass
pnpm typecheck ✅ pass
pnpm lint ✅ pass
turn-lease / convergence-lifecycle / issue-routable 테스트 ✅ 19 passed
CI (Test, Container Smoke) ✅ 둘 다 success
./e2e/run-e2e.sh required-label-{missing,removed} ⚠️ 미검증 — 이 컨테이너에 docker 데몬이 없어 실행 불가 (/var/run/docker.sock 없음). PR 본문의 통과 기록에 의존합니다.

Linear pickup-label 동작은 빌드된 core 로 직접 재현해 확인했습니다 — filterIssuesByPickupLabels 는 이슈를 마킹하는 게 아니라 배열에서 제거합니다.

이번 라운드 신규 지적

  • P1 service.ts 신규 블록의 오케스트레이터 측 테스트 커버리지 0 — 커버리지 봇도 554-593 을 uncovered 로 표시
  • P2 state-read 당 트래커 왕복 추가 → 워커 5초 예산 압박 + refreshed.rateLimits 유실
  • P2 tracker_issue_snapshot_missing 이 정상 중단을 orchestrator_unavailable 로 보고
  • P2 AGENT_TEST.md 의 E2E 커버리지 서술 과장 (스텁 워커는 refreshTrackerState 를 호출하지 않음)
  • P2 턴 경계 중단 사유가 로그·이벤트에 남지 않음
  • P3 routable 계약 optional vs 소비자 필수
  • nit run-e2e.sh 폴링 루프의 refresh 반복 POST

미해결 기존 지적 (ef84aeb 기준, 답변·수정 없음)

  1. #discussion_r3888236824 (P1) — result.state 는 1차 read, routable 은 2차 스냅샷에서 와 원자적이지 않음. 코드 재확인 결과 유효한 지적입니다.
  2. #discussion_r3888236825 (P2) — E2E 가 턴 경계를 실행하지 않음. 유효 (스텁 워커 확인 완료).
  3. #discussion_r3888236827 (P2) — Linear pickup-label 서술이 거짓. 유효 (재현 완료).

수정 또는 미반영 근거를 스레드에 남겨 주세요.

다음 라운드 통과 기준

P1 2건(원자성, 오케스트레이터 테스트)과 Linear 문서/동작 불일치가 해소되면 approve 하겠습니다. P2·P3 는 별도 이슈 분리도 수용합니다.


Generated by Claude Code

Comment thread packages/orchestrator/src/service.ts Outdated
Comment thread packages/orchestrator/src/service.ts
Comment thread packages/orchestrator/src/service.ts Outdated
Comment thread packages/worker/src/turn-lease.ts
Comment thread AGENT_TEST.md Outdated
Comment thread packages/core/src/contracts/tracker-adapter.ts
Comment thread e2e/run-e2e.sh

@hojinzs hojinzs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약 (2/6 라운드) — Approve

1 라운드에서 막았던 P1 2건과 Linear 문서/동작 불일치가 모두 해소되어 approve 합니다. 남은 지적은 전부 P3·nit 이고, 이 PR 을 더 붙잡을 근거가 되지 않습니다.

1 라운드 지적 처리 현황 (10 스레드 전부 응답됨)

# 항목 등급 결과
r3888236824 state/routability 원자성 (Codex) P1 ✅ 해결 — applyStateReadRoutability 가 refreshed 스냅샷 하나에서 state+routable 을 함께 도출
r3888367417 오케스트레이터 테스트 커버리지 0 P1 ✅ 실질 해결 — 순수 함수 추출 + 3 케이스. 잔여 1개 분기는 P3 로 강등
r3888236827 Linear pickup-label 서술 거짓 (Codex) P2 ✅ 해결 — README 가 GitHub/Linear 를 구분
r3888368366 snapshot_missingorchestrator_unavailable 둔갑 P2 ✅ 해결 — 정상 routable: false 중단으로 전환. 재현 검증 완료
r3888367895 추가 왕복 + rateLimits 유실 P2 ◐ rateLimits 는 병합됨. 왕복은 유지 — 근거 수용 (아래)
r3888236825 E2E 가 턴 경계를 실행 안 함 (Codex) P2 ✅ 해결 — 결정적 2턴 스텁이 실제 refreshTrackerState 호출
r3888368990 AGENT_TEST 커버리지 과장 P2 ✅ 해결 — 실제 동작으로 서술 교체
r3888368671 중단 사유 유실 P2 ◐ stderr 는 해결, run 이벤트는 미반영 → P3 로 재제기
r3888369311 routable optional 계약 P3 ✅ 미반영 근거 수용 — 같은 타입이 transition-request 응답도 표현하므로 전역 필수화는 부적절
r3888369514 폴링 루프 반복 POST nit ✅ 해결 — LABEL_REMOVED 가드 추가

왕복 유지 건: 초기 provider read 의 state/rateLimits 가 모두 버려지므로 남는 산출물은 ok/outcome 확인뿐입니다. 다만 requestState 가 labels 를 반환하지 않는 이상 두 번째 읽기는 불가피하고, "state-request 확인 경계"라는 근거도 타당합니다. 수용합니다.

요구사항 대조 (#722) — 전부 충족

요구사항 상태
turn-lease.ts 재검사를 issueRoutable(dispatchable + required_labels)로 확장
README.md:397 / README.md:593 정정
docs/configuration.md required vs pickup 관계
Docker E2E 2 시나리오 + AGENT_TEST.md 갱신
changeset ✅ patch / @gh-symphony/cli

범위를 넘는 오버엔지니어링 없음. dispatchable: false(블로커) 도 턴 경계에서 중단시키는 동작은 이슈가 명시한 §7.1 routability 정의에 포함되므로 임의 확장이 아닙니다.

Smoke test 결과

Preview 환경이 없어 로컬에서 수행했습니다(이 컨테이너에 docker 데몬 없음 — /var/run/docker.sock 부재로 Docker E2E 직접 실행은 이번에도 불가).

항목 결과
pnpm build / typecheck / lint ✅ 전부 pass
pnpm test (전체) ✅ pass
CI (Test, Container Smoke) ✅ 둘 다 success
./e2e/run-e2e.sh required-label-{missing,removed} ⚠️ 미실행 (docker 부재). 아래 계약 검증으로 대체

Docker 대신 서버→워커 계약을 실제 HTTP 왕복으로 직접 검증했습니다. 빌드된 applyStateReadRoutability 출력을 packages/cli/src/commands/start.ts 의 상태코드 매핑 그대로 서빙하고, 빌드된 워커 refreshTrackerState 로 받았습니다:

케이스 결과 사유 전달
turn 1 (라벨 있음) active
turn 2 (라벨 제거) non-actionable Issue is missing required labels ("agent").
스냅샷 필터아웃 (Linear pickup) non-actionable tracker_issue_snapshot_missing
state 가 active set 이탈 non-actionable — (사유 없음, 정상)
dispatchable: false (블로커) non-actionable Blocked by #9.

3번 케이스가 이전에는 unknownorchestrator_unavailable 로 새던 경로입니다. 이제 깔끔한 중단으로 떨어지는 것을 확인했습니다. routableReason 이 JSON 직렬화를 건너 워커까지 도달하는 것도 함께 확인됩니다.

E2E 시나리오 유효성은 코드로 확인했습니다: 픽스처가 state: "Ready" 고정이고 이 시나리오는 transition 을 하지 않으므로, 스텁 호출 시점에 active=true 가 보장되어 결과가 오로지 routable 에만 의존합니다. 라우터빌리티가 깨지면 stub_turn_two_should_be_prevented:active 로 실패합니다 — 1 라운드의 "깨져도 통과" 문제는 사라졌습니다.

이번 라운드 지적 (전부 non-blocking)

  • P3 errorok: true 와 공존하나 lastError 로 넘어가지 않아 사문화 (인라인)
  • P3 tracker.state 이벤트에 routable/routableReason 미포함 — 사후 분석에서 사유 유실 (인라인)
  • P3 workflow_unavailable_for_routability_check 분기만 여전히 미커버 (인라인)
  • nit E2E 의 2초 고정 창이 부하 걸린 CI 에서 빠듯 — false-pass 가 아니라 시끄러운 실패라 안전 (인라인)

네 건 모두 별도 이슈 분리를 권합니다. 이 PR 에서 추가로 고쳐도 되지만, 재리뷰 없이 머지해도 무방합니다.


Generated by Claude Code

Comment thread packages/orchestrator/src/service.ts Outdated
Comment thread packages/worker/src/turn-lease.ts
Comment thread e2e/stub-worker.ts Outdated
Comment thread packages/orchestrator/src/service.test.ts
@moncher-dev
moncher-dev requested a review from hojinzs August 30, 2026 05:28

@hojinzs hojinzs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약 (3/6 라운드) — Request changes (CI 레드)

2 라운드에서 남긴 P3·nit 4건은 모두 해소됐습니다. 다만 후속 커밋 d8b96a4기존 테스트를 깨뜨려 CI Test 잡이 실패하고 있어, 승인 상태를 유지할 수 없습니다. 원인은 프로덕션 코드가 아니라 새로 추가된 테스트의 배선 문제이고, 수정은 두 줄 수준입니다.

🔴 블로커 — CI Test 실패

run 33294715678 / Test conclusion=failure. 로컬 pnpm test 로 동일하게 재현했습니다.

FAIL src/service.test.ts > OrchestratorService > authorizes tracker transitions
     against the current run and persists diagnosable results
AssertionError: expected "loadProjectWorkflow" to not be called at all,
                but actually been called 1 times
 ❯ src/service.test.ts:1359:33
  1. P1 service.test.ts:1340 — 새 state-read 블록이 mockClear() 뒤에 삽입돼 loadProjectWorkflow 호출 1회를 남기고, 1359 줄 단언이 깨집니다. (인라인)
  2. P1 service.test.ts:1374 — 1번을 고치면 다음 실패로 드러나는 이벤트 순서 오류. expected_state_mismatch 가 새 state-read 보다 먼저 append 되는데 단언은 순서를 뒤집어 두었습니다. (인라인)

두 줄을 함께 적용해 로컬에서 그린을 확인했습니다:

✓ packages/orchestrator/src/service.test.ts (199 tests | 198 skipped)
  Test Files  1 passed (1)

2 라운드 지적 처리 현황 (4건 전부 해소)

# 항목 등급 결과
r3888525226 errorok: true 와 공존해 사문화 P3 ✅ 해결 — error 제거, 사유는 routableReason 단일 경로. 스모크로 확인
r3888525617 tracker.state 이벤트에 라우터빌리티 필드 없음 P3 ✅ 해결 — routable/routableReason 이 타입·appendTrackerStateEvent 양쪽에 추가됨
r3888526397 workflow_unavailable_for_routability_check 미커버 P3 ◐ 케이스는 추가됐으나 바로 그 테스트가 위 P1 2건의 원인. 수정하면 해소
r3888526003 E2E 2초 고정 창 nit ✅ 해결 — 러너가 쓰는 시그널 파일 대기로 교체. 마운트·env 전달 경로까지 확인

Smoke test 결과

Preview 환경이 없어 로컬에서 수행했습니다(이 컨테이너에 docker 데몬 없음 — /var/run/docker.sock 부재로 Docker E2E 직접 실행은 이번에도 불가).

항목 결과
pnpm build / typecheck / lint ✅ 전부 pass
pnpm test (전체) 🔴 1 failed / 372 passed (orchestrator) — 위 블로커
service.test.ts -t "state-read routability" ✅ 3 passed
turn-lease.test.ts ✅ 12 passed
CI Container Smoke ✅ success
CI Test 🔴 failure

빌드된 산출물로 서버→워커 계약을 실제 HTTP 왕복으로 재검증했습니다. error 제거가 워커 판정을 바꾸지 않는지 확인하는 게 목적이었고, 정상입니다:

케이스 ok routable routableReason error
라벨 있음 true true null
라벨 제거 true false Issue is missing required labels ("agent"). null
스냅샷 필터아웃 true false tracker_issue_snapshot_missing null

워커 판정: non-actionable, stderr 에 [worker] issue no longer routable: Issue is missing required labels ("agent"). 정상 출력.

E2E 시그널 방식도 코드로 검증했습니다 — docker-compose.e2e.yml:8 이 fixtures 디렉터리를 바인드하고 entrypoint.sh:41 이 issues 경로를 워커 env 까지 전달하므로, 스텁의 dirname(issuesPath) 가 러너가 쓰는 호스트 경로와 일치합니다. 15초 바운드 + 명시적 throw 라 실패 모드도 조용하지 않습니다.

이번 라운드 신규 지적

  • P1 ×2 — 위 CI 블로커 (인라인)
  • P3 applyStateReadRoutability 의 스냅샷 누락 분기만 refreshedRateLimits 를 버림 (인라인)
  • nit 시그널 파일이 .gitignore·cleanup() 양쪽에서 누락 (인라인)

요구사항 대조 (#722)

전부 충족 상태 그대로입니다. 오버엔지니어링 없음. 이번 커밋도 2 라운드 지적에 정확히 대응하는 범위이고, 임의 확장은 없습니다.

다음 라운드 통과 기준

P1 2건만 고쳐 CI 가 그린이 되면 approve 합니다. P3·nit 는 이 PR 에서 고쳐도 되고 별도 이슈로 빼도 무방하며, 재리뷰 없이 머지해도 되는 수준입니다.


Generated by Claude Code

Comment thread packages/orchestrator/src/service.test.ts
Comment thread packages/orchestrator/src/service.test.ts Outdated
Comment thread packages/orchestrator/src/service.ts
Comment thread e2e/run-e2e.sh
@moncher-dev
moncher-dev requested a review from hojinzs August 30, 2026 06:17

@hojinzs hojinzs left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약 (4/6 라운드) — Approve (최종)

3 라운드 블로커였던 P1 2건이 모두 해소되어 CI 가 그린입니다. 함께 남긴 P3·nit 2건도 처리됐습니다. 신규 지적 없음 — 이번 라운드는 인라인 코멘트를 남기지 않았습니다. 머지 가능합니다.

3 라운드 지적 처리 현황 (4건 전부 해소)

# 항목 등급 결과
r3888566503 service.test.ts:1340 mockClear() 누락 → CI 레드 P1 ✅ 해결 — 1343 줄에 loadWorkflowSpy.mockClear() 추가. 단언을 지우지 않고 스파이를 초기화하는 쪽으로, 권고대로
r3888567103 이벤트 순서 역전 P1 ✅ 해결 — 기대 순서가 append 순서(expected_state_mismatchworkflow_unavailable_for_routability_check → provider 실패)와 일치
r3888567441 스냅샷 누락 분기만 refreshedRateLimits 유실 P3 ✅ 해결 — rateLimits: refreshedRateLimits ?? result.rateLimits 추가. 테스트도 { remaining: 7 } 로 고정
r3888567760 시그널 파일이 .gitignore·cleanup() 양쪽에 없음 nit ✅ 해결 — 양쪽 다 반영. 파일명이 러너(run-e2e.sh:242)와 스텁(stub-worker.ts:317) 사이에 일치하는 것도 확인

Smoke test 결과

Preview 환경이 없어 로컬에서 수행했습니다(이 컨테이너에 docker 데몬 없음 — /var/run/docker.sock 부재로 Docker E2E 직접 실행은 이번에도 불가).

항목 결과
pnpm build / typecheck / lint ✅ 전부 pass
pnpm test (전체) pass (exit 0) — 3 라운드 실패 해소
service.test.ts -t "authorizes tracker transitions" ✅ 1 passed (3 라운드에서 깨졌던 바로 그 테스트)
service.test.ts -t "state-read routability" ✅ 3 passed
CI Test / Container Smoke ✅ 둘 다 success (run 33296533695)
./e2e/run-e2e.sh required-label-{missing,removed} ⚠️ 미실행 (docker 부재). 아래 계약 검증으로 대체

빌드된 산출물로 서버→워커 계약을 실제 HTTP 왕복으로 재검증했습니다. 이번 목적은 rateLimits 병합이 워커 판정을 바꾸지 않는지, 그리고 누락 분기에서 실제로 값이 살아남는지 확인하는 것이었습니다:

케이스 워커 판정 routable routableReason rateLimits
turn 1 (라벨 있음) active true 7
turn 2 (라벨 제거) non-actionable false Issue is missing required labels ("agent"). 7
스냅샷 필터아웃 non-actionable false tracker_issue_snapshot_missing 7
state 가 active set 이탈 non-actionable true 7
dispatchable: false (블로커) non-actionable false Blocked by #9. 7

3번 행이 이번 P3 수정 지점입니다 — 3 라운드에서 rl=null 로 새던 자리에 이제 refreshed 값이 남습니다. stderr 사유 출력도 세 경로 모두 정상이고, error 는 전 케이스 null 을 유지합니다.

요구사항 대조 (#722) — 전부 충족

요구사항 상태
turn-lease.ts 재검사를 issueRoutable(dispatchable + required_labels)로 확장
README.md:397 / README.md:593 정정 (GitHub/Linear 구분 포함)
docs/configuration.md required vs pickup 관계
Docker E2E 2 시나리오 + AGENT_TEST.md 갱신
changeset .changeset/turn-routability.md — patch / @gh-symphony/cli

오버엔지니어링·임의 결정 없음. 최종 diff 는 19 파일 / +494 −34 이고, 증가분의 상당수(+106 orchestrator 테스트, +69 turn-lease 테스트, +59 스텁)가 리뷰 라운드에서 요청한 테스트 커버리지입니다. 프로덕션 변경은 service.ts +68, turn-lease.ts +12, 계약·이벤트 타입 +11 로 좁게 유지됐습니다.

남은 항목

없습니다. 2 라운드에서 근거를 수용한 2건(routable optional 계약 — 같은 타입이 transition-request 응답도 표현함 / 추가 트래커 왕복 — requestState 가 labels 를 반환하지 않는 한 불가피)은 그대로 유효하며 별도 이슈도 필요하지 않다고 봅니다.

제기한 스레드는 전부 해소 확인 후 resolve 처리했습니다. 재리뷰 없이 머지하셔도 됩니다.


Generated by Claude Code

@moncher-dev
moncher-dev force-pushed the feat/722-routable-turn-check branch from 6d7d9b2 to b69c951 Compare August 30, 2026 06:52
@moncher-dev
moncher-dev merged commit a9d32e7 into main Aug 30, 2026
2 checks passed
@moncher-dev
moncher-dev deleted the feat/722-routable-turn-check branch August 30, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(worker,docs,e2e): per-turn routable check on tracker-state reads; README pickup/required_labels semantics; Docker E2E

2 participants