diff --git a/.changeset/turn-routability.md b/.changeset/turn-routability.md new file mode 100644 index 00000000..256ff910 --- /dev/null +++ b/.changeset/turn-routability.md @@ -0,0 +1,5 @@ +--- +"@gh-symphony/cli": patch +--- + +Stop a worker before its next turn when a refreshed tracker snapshot shows the issue is no longer dispatchable or no longer satisfies `required_labels` (#722). diff --git a/.gitignore b/.gitignore index 85fafd2b..5bb54ed2 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ packages/control-plane/client/src/routeTree.gen.ts # E2E runtime file (injected at test time, not committed) e2e/fixtures/issues.json +e2e/fixtures/required-label-removed.signal diff --git a/AGENT_TEST.md b/AGENT_TEST.md index 22d14950..8e004547 100644 --- a/AGENT_TEST.md +++ b/AGENT_TEST.md @@ -201,7 +201,7 @@ Control worker behavior with the `STUB_SCENARIO` environment variable: | Opaque tracker native reference boundary | `packages/tracker-github/src/tracker-github.test.ts` exercises GitHub linked-PR canonicalization through `resolveCanonicalIssues`; `packages/orchestrator/src/dispatch.test.ts` verifies dispatch uses adapter hooks rather than service-level provider payload inspection. | TC-18 runs the Docker `happy` scenario with the file tracker fixture, which derives its opaque item reference and completes dispatch without provider-specific orchestration branches. | | Configured repository workflow path and startup preflight | `packages/cli/src/commands/start.test.ts` verifies unsupported tracker kinds fail before daemon construction, project `.env` values resolve during preflight, and missing configured files give a remediation. `packages/orchestrator/src/service.test.ts` verifies repo-typed configured paths are loaded and warn when shadowing the repository root workflow; `doctor.test.ts` and `repo-explain.test.ts` verify diagnostics use the same path. | `./e2e/run-e2e.sh happy 60` confirms the Docker repository-init → repository-start lifecycle and worker dispatch remain healthy after startup-validation changes. The custom-path and invalid-kind edge cases are unit-isolated because the standard Docker fixture uses a fixed repository policy. | | Adapter-owned dispatch eligibility | `packages/orchestrator/src/dispatch.test.ts` verifies the scheduler suppresses `dispatchable: false` issues, while each tracker adapter's suite verifies its own provider-specific derivation. | `./e2e/run-e2e.sh non-dispatchable 30` injects a file-tracker issue with `dispatchable: false`, waits for two post-injection reconciliation ticks, confirms no worker or `run-dispatched` event exists, and verifies `repo explain` retains the adapter reason. | -| Required-label routability | `packages/orchestrator/src/dispatch.test.ts` verifies normalized missing-label explanations and active-run cancellation without workspace cleanup; `packages/orchestrator/src/service.test.ts` verifies a due retry losing its required label releases its claim and persists the routability reason. | TC-19 removes a required label from an active file-tracker issue, confirms SIGTERM and retained workspace, and checks `repo explain` reports the missing-label reason. | +| Required-label routability | `packages/worker/src/turn-lease.test.ts` verifies a confirmed active state with `routable: false` stops before the next turn; `packages/orchestrator/src/dispatch.test.ts` verifies normalized missing-label explanations and active-run cancellation without workspace cleanup; `packages/orchestrator/src/service.test.ts` verifies a due retry losing its required label releases its claim and persists the routability reason. | [TC-19](e2e/scenarios/19-required-label-routability.md) verifies that a missing required label prevents dispatch, then removes the label after a deterministic stub's first turn and asserts that its actual worker `state-read` prevents turn two. | | Shared worktree-cache agent branch collection | `packages/orchestrator/src/git.test.ts` verifies that every detached ref under `refs/heads/` in the shared bare cache is deleted only when its tip is reachable from `refs/remotes/origin/*`; unpushed branches and branches linked to live worktrees are retained, including branches from other projects sharing the cache. | The Docker file tracker does not create real agent commits or remote branches; repository-fixture coverage is the authoritative TC for this Git reachability guarantee. | | Normalized per-state concurrency limits | `packages/core/src/workflow-loader.test.ts` verifies trimmed/lowercased map keys and ignored invalid entries; `packages/orchestrator/src/dispatch.test.ts` verifies a padded mixed-case key caps matching tracker states; `packages/orchestrator/src/explain.test.ts` verifies `repo explain` reports the same mixed-case per-state cap as dispatch. | `./e2e/run-e2e.sh happy 60` confirms the Docker dispatch lifecycle remains healthy with the orchestrator's canonical workflow-state lookup path. | diff --git a/README.md b/README.md index bdd2314b..88b9f9c9 100644 --- a/README.md +++ b/README.md @@ -398,7 +398,7 @@ gh-symphony project list # List cached standalone projects gh-symphony instances --json # List active repository and standalone instances ``` -The project folder is the source of truth and the address: every command derives the runtime from the folder's `WORKFLOW.md` on each start, so editing the workflow takes effect on the next start with no registration step. `project start --help` lists its runtime flags, including `--once`, `--daemon`, `--assigned-only`, `--allow-duplicate`, `--bind-all`, `--http`, `--web`, `--log-level`, and `--project-dir`. `--assigned-only` is input to the tracker adapter's `dispatchable` derivation; the scheduler consumes that normalized eligibility result rather than interpreting provider-specific assignment rules. A verified live instance for the same project in another runtime is rejected by default; use `--allow-duplicate` only for intentional isolation. Starting refuses a tracker mapping that overlaps a project already running against the same repository, and asks for confirmation when the overlapping project is stopped. Two projects on one repository stay disjoint through `tracker.pickup_labels`, which the GitHub, Linear, and file trackers all apply when listing dispatch candidates. Label comparison is case-insensitive and ignores surrounding whitespace, so `Agent`, `agent`, and `" AGENT "` are the same label. `repository.clone_url` overrides the derived clone URL for mirrors, Enterprise hosts, or local paths. See [docs/configuration.md](docs/configuration.md) for the project `.env` loading order and skill layering details. +The project folder is the source of truth and the address: every command derives the runtime from the folder's `WORKFLOW.md` on each start, so editing the workflow takes effect on the next start with no registration step. `project start --help` lists its runtime flags, including `--once`, `--daemon`, `--assigned-only`, `--allow-duplicate`, `--bind-all`, `--http`, `--web`, `--log-level`, and `--project-dir`. `--assigned-only` is input to the tracker adapter's `dispatchable` derivation; the scheduler consumes that normalized eligibility result rather than interpreting provider-specific assignment rules. A verified live instance for the same project in another runtime is rejected by default; use `--allow-duplicate` only for intentional isolation. Starting refuses a tracker mapping that overlaps a project already running against the same repository, and asks for confirmation when the overlapping project is stopped. Two projects on one repository stay disjoint through `tracker.provider.pickup_labels.include`, which GitHub and Linear apply as an any-match candidate pre-filter. `tracker.required_labels` is separate: every configured label must remain present for an issue to be routable, including between worker turns. Label comparison is case-insensitive and ignores surrounding whitespace, so `Agent`, `agent`, and `" AGENT "` are the same label. `repository.clone_url` overrides the derived clone URL for mirrors, Enterprise hosts, or local paths. See [docs/configuration.md](docs/configuration.md) for the project `.env` loading order and skill layering details. ### Official Container Deployment @@ -595,7 +595,7 @@ tracker: `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. On GitHub, this pre-filter does not terminate an already-running worker when its labels change. Linear applies the pickup filter to ID refreshes too, so removing the sole included label can make an active worker stop during reconciliation. 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. Linear orchestration is polling-only. There is intentionally no Linear webhook setup command; state transitions, workpad comments, and PR handoff policy belong in `WORKFLOW.md`. See `docs/examples/linear-WORKFLOW.md` for a complete example. diff --git a/docker-compose.e2e.yml b/docker-compose.e2e.yml index 1ec581ff..56e13c33 100644 --- a/docker-compose.e2e.yml +++ b/docker-compose.e2e.yml @@ -10,4 +10,5 @@ services: - /e2e/work:size=100M,uid=1000,gid=1000,mode=1777 environment: STUB_SCENARIO: ${STUB_SCENARIO:-happy} + E2E_REQUIRED_LABELS: ${E2E_REQUIRED_LABELS:-} GH_SYMPHONY_HTTP_TOKEN: ${GH_SYMPHONY_HTTP_TOKEN:-e2e-http-token} diff --git a/docs/configuration.md b/docs/configuration.md index 9244933e..a6182c8f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -122,13 +122,23 @@ already occupied. A bare `--port` or `--http` keeps the legacy default-port auto-increment behavior, while omitting both options and `server.port` uses an ephemeral internal listener. -`tracker.required_labels` defaults to `[]`. Labels are compared after trimming -and lowercasing; every configured label must be present before an issue can be -routed. A blank configured label is preserved and therefore matches no issue. -Removing a required label blocks new dispatches and due retries. On the next -reconciliation tick, an active worker for that issue is terminated without +`tracker.required_labels` defaults to `[]`. It is an ALL-of routability gate: +labels are compared after trimming and lowercasing, and every configured label +must be present before an issue can be routed. A blank configured label is +preserved and therefore matches no issue. Removing a required label blocks new +dispatches and due retries; a worker also stops before its next turn when its +authenticated tracker-state read returns a refreshed, non-routable snapshot. +Reconciliation still terminates an active worker on its next tick without workspace cleanup so its work remains available for recovery. +`tracker.provider.pickup_labels.include` is separate from +`tracker.required_labels`. For GitHub and Linear it is an ANY-of candidate +pre-filter: an issue needs at least one include label to enter the dispatch +candidate set. `exclude` always wins. Pickup-label changes affect future +candidate listing only; they do not make an already-running issue non-routable +or stop its worker. Use `required_labels` when the label must remain true +throughout a run. + ## Label and Timestamp Normalization Core normalizes workflow pickup labels and tracker-provided labels before label diff --git a/e2e/fixtures/required-label-active.json b/e2e/fixtures/required-label-active.json new file mode 100644 index 00000000..8252bfb6 --- /dev/null +++ b/e2e/fixtures/required-label-active.json @@ -0,0 +1,20 @@ +[ + { + "id": "issue-required-label-1", + "identifier": "test-owner/test-repo#1", + "number": 1, + "title": "Required label removed during run", + "description": "This issue begins routable, then loses its label.", + "priority": null, + "state": "Ready", + "branchName": null, + "url": "https://tracker.example.test/issues/issue-required-label-1", + "labels": ["agent"], + "blockedBy": [], + "createdAt": "2026-03-17T00:00:00Z", + "updatedAt": "2026-03-17T00:00:00Z", + "repository": { "owner": "test-owner", "name": "test-repo", "cloneUrl": "/e2e/repos/test-owner/test-repo" }, + "tracker": { "adapter": "file", "bindingId": "e2e-test", "itemId": "issue-required-label-1" }, + "metadata": {} + } +] diff --git a/e2e/fixtures/required-label-missing.json b/e2e/fixtures/required-label-missing.json new file mode 100644 index 00000000..312782ab --- /dev/null +++ b/e2e/fixtures/required-label-missing.json @@ -0,0 +1,20 @@ +[ + { + "id": "issue-required-label-1", + "identifier": "test-owner/test-repo#1", + "number": 1, + "title": "Missing required label", + "description": "This issue must not dispatch.", + "priority": null, + "state": "Ready", + "branchName": null, + "url": "https://tracker.example.test/issues/issue-required-label-1", + "labels": [], + "blockedBy": [], + "createdAt": "2026-03-17T00:00:00Z", + "updatedAt": "2026-03-17T00:00:00Z", + "repository": { "owner": "test-owner", "name": "test-repo", "cloneUrl": "/e2e/repos/test-owner/test-repo" }, + "tracker": { "adapter": "file", "bindingId": "e2e-test", "itemId": "issue-required-label-1" }, + "metadata": {} + } +] diff --git a/e2e/run-e2e.sh b/e2e/run-e2e.sh index ceca5f14..e4734a6c 100755 --- a/e2e/run-e2e.sh +++ b/e2e/run-e2e.sh @@ -3,7 +3,7 @@ set -euo pipefail # E2E Test Runner — polls the standalone dashboard until the scenario completes. # Usage: ./e2e/run-e2e.sh [scenario] [timeout_seconds] -# scenario: happy (default), fail, stall, slow, transition-race, api-progress, api-progress-unknown, prompt-phase, retry-attempt, non-dispatchable +# scenario: happy (default), fail, stall, slow, transition-race, api-progress, api-progress-unknown, prompt-phase, retry-attempt, non-dispatchable, required-label-missing, required-label-removed # timeout: 30 (default) SCENARIO="${1:-happy}" @@ -40,6 +40,7 @@ cleanup() { "${COMPOSE[@]}" down --volumes --remove-orphans --timeout 5 2>/dev/null || true remove_e2e_compose_image echo "[]" > e2e/fixtures/issues.json 2>/dev/null || true + rm -f e2e/fixtures/required-label-removed.signal } # ── Setup ───────────────────────────────────────────────────── @@ -49,11 +50,16 @@ log "Compose project: ${COMPOSE_PROJECT_NAME}" assert_e2e_project_is_available docker-compose.e2e.yml echo "[]" > e2e/fixtures/issues.json +rm -f e2e/fixtures/required-label-removed.signal trap cleanup EXIT # Set scenario in environment export STUB_SCENARIO="$SCENARIO" -STUB_SCENARIO="$SCENARIO" "${COMPOSE[@]}" up -d --build 2>&1 | tail -1 +E2E_REQUIRED_LABELS="" +if [ "$SCENARIO" = "required-label-missing" ] || [ "$SCENARIO" = "required-label-removed" ]; then + E2E_REQUIRED_LABELS="agent" +fi +E2E_REQUIRED_LABELS="$E2E_REQUIRED_LABELS" STUB_SCENARIO="$SCENARIO" "${COMPOSE[@]}" up -d --build 2>&1 | tail -1 log "Waiting for dashboard state..." for i in $(seq 1 20); do @@ -105,6 +111,10 @@ log "Initial state: idle" if [ "$SCENARIO" = "non-dispatchable" ]; then cp e2e/fixtures/non-dispatchable.json e2e/fixtures/issues.json +elif [ "$SCENARIO" = "required-label-missing" ]; then + cp e2e/fixtures/required-label-missing.json e2e/fixtures/issues.json +elif [ "$SCENARIO" = "required-label-removed" ]; then + cp e2e/fixtures/required-label-active.json e2e/fixtures/issues.json else cp e2e/fixtures/happy-path.json e2e/fixtures/issues.json fi @@ -126,7 +136,7 @@ fi log "Issues injected; refresh trigger accepted (202). Polling for reconciliation" -if [ "$SCENARIO" = "non-dispatchable" ]; then +if [ "$SCENARIO" = "non-dispatchable" ] || [ "$SCENARIO" = "required-label-missing" ]; then # A tick can have started before the fixture copy, then publish after the # refresh request. Waiting for two new tick start timestamps means the # second observed tick must have started after that older tick finished. @@ -151,12 +161,24 @@ if [ "$SCENARIO" = "non-dispatchable" ]; then done ACTIVE=$(echo "$STATUS_JSON" | python3 -c "import sys,json; print(json.load(sys.stdin)['summary']['activeRuns'])" 2>/dev/null || echo '?') if [ "$ACTIVE" != "0" ]; then - fail "Non-dispatchable issue started a worker" + fail "Ineligible issue started a worker" exit 1 fi EXPLAIN_JSON=$("${COMPOSE[@]}" exec -T -w /e2e/work/test-repo -e GITHUB_GRAPHQL_TOKEN=e2e-token symphony-e2e \ node /app/packages/cli/dist/index.js repo explain test-owner/test-repo#1 --json) - echo "$EXPLAIN_JSON" | python3 -c ' + if [ "$SCENARIO" = "required-label-missing" ]; then + echo "$EXPLAIN_JSON" | python3 -c ' +import json +import sys + +report = json.load(sys.stdin) +assert report["dispatchable"] is False, report +assert report["summary"] == "Not dispatchable: not routable: Issue is missing required labels (\"agent\").", report +checks = {check["id"]: check for check in report["checks"]} +assert checks["workflow_routability"]["status"] == "block", checks +' + else + echo "$EXPLAIN_JSON" | python3 -c ' import json import sys @@ -167,13 +189,14 @@ checks = {check["id"]: check for check in report["checks"]} assert checks["tracker_dispatchability"]["status"] == "block", checks assert checks["tracker_dispatchability"]["details"]["dispatchReason"] == "fixture eligibility gate: assigned to another agent", checks ' + fi if "${COMPOSE[@]}" exec -T symphony-e2e sh -c 'find /e2e/work -name events.ndjson -exec grep -H "run-dispatched" {} + 2>/dev/null | grep -q .'; then fail "Non-dispatchable issue wrote a run-dispatched event" exit 1 fi log "=== Result ===" log " Worker dispatched: NO" - log " Explain reason: fixture eligibility gate: assigned to another agent" + log " Explain reason: $( [ "$SCENARIO" = "required-label-missing" ] && echo 'missing required label' || echo 'fixture eligibility gate' )" echo "" log "PASSED" exit 0 @@ -186,6 +209,7 @@ SAW_RETRY=false SAW_REDACTED_STATE=false SCENARIO_RUN_ID="" ELAPSED=0 +LABEL_REMOVED=false log "Polling..." while [ "$ELAPSED" -lt "$TIMEOUT" ]; do @@ -206,6 +230,18 @@ while [ "$ELAPSED" -lt "$TIMEOUT" ]; do 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" ] && [ "$LABEL_REMOVED" != true ]; then + python3 - <<'PY' +import json +from pathlib import Path +path = Path("e2e/fixtures/issues.json") +issues = json.loads(path.read_text()) +issues[0]["labels"] = [] +path.write_text(json.dumps(issues)) +PY + : > e2e/fixtures/required-label-removed.signal + LABEL_REMOVED=true + fi if echo "$STATUS_JSON" | python3 -c ' import json, sys data = json.load(sys.stdin) @@ -248,6 +284,24 @@ log "=== Event Logs ===" "${COMPOSE[@]}" exec -T symphony-e2e sh -c 'find /e2e/work -name events.ndjson -exec cat {} \; 2>/dev/null' 2>/dev/null || true echo "" +if [ "$SCENARIO" = "required-label-removed" ]; then + if [ "$SAW_RUNNING" != true ] || [ "$LABEL_REMOVED" != true ]; then + fail "Required-label removal did not reach an active worker" + exit 1 + fi + if ! "${COMPOSE[@]}" exec -T symphony-e2e sh -c 'grep -R -q "turn=1 completed" /e2e/work && grep -R -q "turn=2 prevented by routability refresh" /e2e/work'; then + fail "Expected the routability refresh to prevent turn two after label removal" + exit 1 + fi + log "=== Result ===" + log " Worker started with required label: YES" + log " Label removed during run: YES" + log " Turn one completed: YES" + log " Turn two prevented by refresh: YES" + log "PASSED" + exit 0 +fi + if [ "$SCENARIO" = "transition-race" ]; then if [ "$SAW_RUNNING" != true ]; then fail "=== Result ===" diff --git a/e2e/scenarios/19-required-label-routability.md b/e2e/scenarios/19-required-label-routability.md index 7c4d16fc..a3f63976 100644 --- a/e2e/scenarios/19-required-label-routability.md +++ b/e2e/scenarios/19-required-label-routability.md @@ -2,31 +2,44 @@ ## Purpose -Verify that an active issue which loses a required label is canceled on the -next reconciliation tick, retains its workspace, and exposes the routing -reason to diagnostics. +Verify both required-label boundaries: a candidate missing a required label is +never dispatched, and an active worker that loses one ends at the next turn +boundary. Both cases retain an explainable routability reason. ## Setup -1. Start the Docker E2E environment with `STUB_SCENARIO=stall`. +1. Start the Docker E2E environment with `STUB_SCENARIO=required-label-removed`. 2. Add `required_labels: [agent]` to the fixture repository `WORKFLOW.md` before starting the daemon. 3. Inject a `Ready` file-tracker issue with the `agent` label and wait until the stub worker is running. -## Steps +## Case A: missing label before dispatch + +1. Inject a `Ready` file-tracker issue without the `agent` label. +2. Trigger `POST /api/v1/refresh` with the E2E bearer token and wait for two + reconciliation ticks. +3. Inspect `/api/v1/state`, `events.ndjson`, and `repo explain`. + +### Expected results + +- No worker is started and no `run-dispatched` event is written. +- `gh-symphony repo explain ` reports + `not routable: Issue is missing required labels ("agent").`. + +## Case B: label removed during a run 1. Remove `agent` from the issue's `labels` array while preserving its active state. -2. Trigger `POST /api/v1/refresh` with the E2E bearer token. -3. Inspect `/api/v1/state` and the run's `events.ndjson`. +2. Let the deterministic stub complete turn one and issue its turn-boundary + `state-read`. +3. Inspect the worker log and the run's `events.ndjson`. ## Expected results -- The worker receives `SIGTERM` and the run is suppressed with - `runPhase: "canceled_by_reconciliation"`. -- The run error records the missing required label as its routability reason. -- The issue orchestration claim is released. -- The issue workspace remains present; no terminal cleanup occurs. +- The worker log records `turn=1 completed` followed by + `turn=2 prevented by routability refresh`; it must not begin turn two. +- The worker exits cleanly after its state-read reports the missing required + label as unroutable. - `gh-symphony repo explain ` reports `not routable: Issue is missing required labels ("agent").`. diff --git a/e2e/seed/entrypoint.sh b/e2e/seed/entrypoint.sh index 435c82bd..a1244cdb 100644 --- a/e2e/seed/entrypoint.sh +++ b/e2e/seed/entrypoint.sh @@ -10,6 +10,18 @@ rm -rf "$WORK_DIR" git clone "$REPO_DIR" "$WORK_DIR" git -C "$WORK_DIR" remote set-url origin test-owner/test-repo +if [ -n "${E2E_REQUIRED_LABELS:-}" ]; then + awk -v labels="$E2E_REQUIRED_LABELS" ' + /^ active_states:/ { + print " required_labels:" + count = split(labels, values, ",") + for (labelIndex = 1; labelIndex <= count; labelIndex += 1) print " - " values[labelIndex] + } + { print } + ' "$WORK_DIR/WORKFLOW.md" > /tmp/e2e-workflow.md + mv /tmp/e2e-workflow.md "$WORK_DIR/WORKFLOW.md" +fi + # GH_SYMPHONY_FILE_TRACKER_ISSUES_PATH is intentionally limited to the # file-tracker E2E workflow so repo init can bind the mounted fixture file. cd "$WORK_DIR" diff --git a/e2e/stub-worker.ts b/e2e/stub-worker.ts index 0bc9e943..e2f4401b 100644 --- a/e2e/stub-worker.ts +++ b/e2e/stub-worker.ts @@ -11,6 +11,8 @@ * transition-race — requests Ready → In review, then stalls for reconciliation * api-progress — requests Ready → Done, confirms readback, then completes * api-progress-unknown — confirms Done, removes the canonical item, then completes + * required-label-removed — completes turn one, then proves a routability + * refresh prevents turn two after its label is removed */ import { existsSync } from "node:fs"; @@ -36,7 +38,8 @@ type Scenario = | "retry-attempt" | "transition-race" | "api-progress" - | "api-progress-unknown"; + | "api-progress-unknown" + | "required-label-removed"; const VALID_SCENARIOS: ReadonlySet = new Set([ "happy", "fail", @@ -47,6 +50,7 @@ const VALID_SCENARIOS: ReadonlySet = new Set([ "transition-race", "api-progress", "api-progress-unknown", + "required-label-removed", ]); const rawScenario = process.env.STUB_SCENARIO ?? "happy"; const SCENARIO: Scenario = VALID_SCENARIOS.has(rawScenario) @@ -69,6 +73,7 @@ const SCENARIO_DURATIONS: Record = "transition-race": { startMs: 2000, runMs: Infinity }, "api-progress": { startMs: 2000, runMs: 1000 }, "api-progress-unknown": { startMs: 2000, runMs: 1000 }, + "required-label-removed": { startMs: 2000, runMs: 1000 }, }; function resolveCoreModuleUrl(): string { @@ -90,6 +95,25 @@ function resolveCoreModuleUrl(): string { throw new Error(`stub_core_module_not_found:${workerPath}`); } +function resolveWorkerModuleUrl(): string { + const workerPath = process.argv[1]; + if (!workerPath) { + throw new Error("stub_worker_path_unavailable"); + } + const workerDir = dirname(resolve(workerPath)); + for (const root of [ + workerDir, + resolve(workerDir, ".."), + resolve(workerDir, "../.."), + ]) { + const turnLeasePath = join(root, "packages/worker/dist/turn-lease.js"); + if (existsSync(turnLeasePath)) { + return pathToFileURL(turnLeasePath).href; + } + } + throw new Error(`stub_worker_turn_lease_not_found:${workerPath}`); +} + const ORCHESTRATOR_URL = process.env.SYMPHONY_ORCHESTRATOR_URL ?? ""; const ORCHESTRATOR_TOKEN = process.env.SYMPHONY_ORCHESTRATOR_TOKEN ?? ""; @@ -280,6 +304,36 @@ async function removeCanonicalTrackerItem(): Promise { console.error("[stub-worker] api-progress canonical item removed"); } +async function preventSecondTurnAfterLabelRemoval(): Promise { + const { refreshTrackerState } = (await import(resolveWorkerModuleUrl())) as { + refreshTrackerState: ( + env: NodeJS.ProcessEnv, + activeStates: readonly string[] + ) => Promise<"active" | "non-actionable" | "unknown">; + }; + console.error("[stub-worker] turn=1 completed"); + const issuesPath = process.env.GH_SYMPHONY_FILE_TRACKER_ISSUES_PATH; + const labelRemovalSignal = issuesPath + ? join(dirname(issuesPath), "required-label-removed.signal") + : null; + if (!labelRemovalSignal) { + throw new Error("stub_file_tracker_issues_path_missing"); + } + // Wait for the runner to update the fixture instead of racing its status + // poll with a fixed delay. The bounded wait still fails loudly in CI. + for (let attempt = 0; attempt < 30 && !existsSync(labelRemovalSignal); attempt += 1) { + await sleep(500); + } + if (!existsSync(labelRemovalSignal)) { + throw new Error("stub_required_label_removal_not_observed"); + } + const state = await refreshTrackerState(process.env, ["Ready"]); + if (state !== "non-actionable") { + throw new Error(`stub_turn_two_should_be_prevented:${state}`); + } + console.error("[stub-worker] turn=2 prevented by routability refresh"); +} + async function run() { const durations = SCENARIO_DURATIONS[SCENARIO]; @@ -355,6 +409,9 @@ async function run() { if (SCENARIO === "api-progress-unknown") { await removeCanonicalTrackerItem(); } + if (SCENARIO === "required-label-removed") { + await preventSecondTurnAfterLabelRemoval(); + } await sleep(durations.runMs); // Terminal phase diff --git a/packages/core/src/contracts/tracker-adapter.ts b/packages/core/src/contracts/tracker-adapter.ts index 26e370fd..5bb2ec3f 100644 --- a/packages/core/src/contracts/tracker-adapter.ts +++ b/packages/core/src/contracts/tracker-adapter.ts @@ -250,6 +250,13 @@ export type TrackerStateResult = { reason: string | null; rateLimits: Record | null; error: string | null; + /** + * State-read only: routability calculated from a freshly normalized tracker + * snapshot. `null` means that no routability decision was available. + */ + routable?: boolean | null; + /** State-read only: concrete reason when the refreshed issue is unroutable. */ + routableReason?: string | null; }; export type TrackerTerminalFact = { diff --git a/packages/core/src/observability/structured-events.ts b/packages/core/src/observability/structured-events.ts index 5354604e..cc874ca8 100644 --- a/packages/core/src/observability/structured-events.ts +++ b/packages/core/src/observability/structured-events.ts @@ -49,6 +49,10 @@ export type TrackerStateRequestEvent = { outcome: "confirmed" | "expected_state_mismatch" | "rejected" | "failed"; reason: string | null; error: string | null; + /** State-read routability decision from the refreshed tracker snapshot. */ + routable?: boolean | null; + /** Concrete reason when a refreshed state-read is not routable. */ + routableReason?: string | null; rateLimits?: Record | null; }; diff --git a/packages/orchestrator/src/service.test.ts b/packages/orchestrator/src/service.test.ts index ef96a98b..848ec2ee 100644 --- a/packages/orchestrator/src/service.test.ts +++ b/packages/orchestrator/src/service.test.ts @@ -23,6 +23,7 @@ import { type OrchestratorRunRecord, type OrchestratorTrackerDependencies, type RepositoryRef, + type TrackedIssue, type TrackedIssueList, type WorkflowResolution, } from "@gh-symphony/core"; @@ -31,6 +32,7 @@ import { OrchestratorFsStore } from "./fs-store.js"; import * as gitModule from "./git.js"; import { ensureGlobalBareRepositoryCache } from "./repository-cache.js"; import { + applyStateReadRoutability, clampPollInterval, OrchestratorService, shouldAwaitTrackerProgressExit, @@ -38,6 +40,80 @@ import { } from "./service.js"; import * as trackerAdapters from "./tracker-adapters.js"; +describe("state-read routability", () => { + const confirmed = { + ok: true, + outcome: "confirmed" as const, + state: "In progress", + expectedState: null, + targetState: null, + reason: null, + rateLimits: { source: "github", remaining: 10 }, + error: null, + }; + const lifecycle = { requiredLabels: ["agent"] }; + const issue = (overrides: Partial = {}) => + ({ + id: "issue-1", + identifier: "acme/platform#1", + title: "Issue", + description: null, + priority: null, + state: "In progress", + branchName: null, + url: null, + labels: ["agent"], + dispatchable: true, + assigneeId: null, + blockedBy: [], + createdAt: null, + updatedAt: null, + repository: { owner: "acme", name: "platform" }, + tracker: { adapter: "file", bindingId: "test" }, + metadata: {}, + ...overrides, + }) as TrackedIssue; + + it("derives confirmed state and routability from one refreshed snapshot", () => { + expect( + applyStateReadRoutability( + confirmed, + issue({ state: "Done" }), + { source: "github", remaining: 9 }, + lifecycle + ) + ).toMatchObject({ + state: "Done", + routable: true, + routableReason: null, + rateLimits: { remaining: 9 }, + }); + }); + + it("reports an active issue missing a required label as unroutable", () => { + expect( + applyStateReadRoutability(confirmed, issue({ labels: [] }), null, lifecycle) + ).toMatchObject({ + state: "In progress", + routable: false, + routableReason: 'Issue is missing required labels ("agent").', + }); + }); + + it("treats a filtered snapshot as a clean routing stop", () => { + expect( + applyStateReadRoutability(confirmed, undefined, { remaining: 7 }, lifecycle) + ).toMatchObject({ + ok: true, + outcome: "confirmed", + rateLimits: { remaining: 7 }, + routable: false, + routableReason: "tracker_issue_snapshot_missing", + error: null, + }); + }); +}); + describe("OrchestratorService", () => { const originalToken = process.env.GITHUB_GRAPHQL_TOKEN; const originalAllowWorkflowHooks = process.env.SYMPHONY_ALLOW_WORKFLOW_HOOKS; @@ -1242,6 +1318,29 @@ describe("OrchestratorService", () => { expect(persistedRun?.trackerProgressConfirmedAt).toBeNull(); expect(loadWorkflowSpy).toHaveBeenCalledOnce(); loadWorkflowSpy.mockClear(); + requestState.mockResolvedValueOnce({ + ok: true, + outcome: "confirmed", + state: "In progress", + expectedState: null, + targetState: null, + reason: null, + rateLimits: null, + error: null, + }); + loadWorkflowSpy.mockResolvedValueOnce({ + isValid: false, + usedLastKnownGood: false, + } as WorkflowResolution); + await expect( + service.requestTrackerState({ runId: "run-1", request: { type: "state-read" } }) + ).resolves.toMatchObject({ + ok: false, + outcome: "failed", + routable: null, + error: "workflow_unavailable_for_routability_check", + }); + loadWorkflowSpy.mockClear(); const providerError = Object.assign(new Error("rate limit exhausted"), { rateLimits: { source: "github", @@ -1279,6 +1378,13 @@ describe("OrchestratorService", () => { confirmedState: "Ready", rateLimits: expect.objectContaining({ cycleCost: 1 }), }), + expect.objectContaining({ + event: "tracker.state", + runId: "run-1", + outcome: "failed", + error: "workflow_unavailable_for_routability_check", + routable: null, + }), expect.objectContaining({ event: "tracker.state", runId: "run-1", diff --git a/packages/orchestrator/src/service.ts b/packages/orchestrator/src/service.ts index d0d76e7c..ca213370 100644 --- a/packages/orchestrator/src/service.ts +++ b/packages/orchestrator/src/service.ts @@ -182,6 +182,36 @@ export function shouldRecordConfirmedTrackerProgress( ); } +/** + * Replaces the initial state-read with facts from one freshly normalized + * snapshot so a worker never combines an old lifecycle state with new label + * routing. A missing snapshot is a clean routing stop (for example, Linear + * pickup filtering), rather than transport failure. + */ +export function applyStateReadRoutability( + result: TrackerStateResult, + refreshedIssue: TrackedIssue | undefined, + refreshedRateLimits: Record | null | undefined, + lifecycle: WorkflowLifecycleConfig +): TrackerStateResult { + if (!refreshedIssue) { + return { + ...result, + rateLimits: refreshedRateLimits ?? result.rateLimits, + routable: false, + routableReason: "tracker_issue_snapshot_missing", + }; + } + const routability = issueRoutable(refreshedIssue, lifecycle); + return { + ...result, + state: refreshedIssue.state, + rateLimits: refreshedRateLimits ?? result.rateLimits, + routable: routability.routable, + routableReason: routability.reason ?? null, + }; +} + type ProjectWorkflowResolution = Awaited< ReturnType >; @@ -537,7 +567,7 @@ export class OrchestratorService { } } - const result = await requestState( + let result = await requestState( this.projectConfig, { issueSubjectId: run.issueSubjectId, @@ -546,6 +576,40 @@ export class OrchestratorService { }, this.createTrackerDependencies() ); + if ( + input.request.type === "state-read" && + result.ok === true && + result.outcome === "confirmed" + ) { + const workflowResolution = await this.loadProjectWorkflow( + this.projectConfig, + run.repository + ); + if (!isUsableWorkflowResolution(workflowResolution)) { + result = { + ...result, + ok: false, + outcome: "failed", + routable: null, + error: "workflow_unavailable_for_routability_check", + }; + } else { + const refreshed = await trackerAdapter.fetchIssueStatesByIds( + this.projectConfig, + [run.issueSubjectId], + this.createTrackerDependencies() + ); + const refreshedIssue = refreshed.find( + (issue) => issue.id === run.issueSubjectId + ); + result = applyStateReadRoutability( + result, + refreshedIssue, + refreshed.rateLimits, + workflowResolution.lifecycle + ); + } + } let recordConfirmedTrackerProgress = false; if (input.request.type === "transition-request") { const workflowResolution = await this.loadProjectWorkflow( @@ -663,6 +727,8 @@ export class OrchestratorService { outcome: result.outcome, reason: result.reason, error: result.error, + routable: result.routable, + routableReason: result.routableReason, rateLimits: result.rateLimits, }); } diff --git a/packages/worker/src/convergence-lifecycle.test.ts b/packages/worker/src/convergence-lifecycle.test.ts index 2f1143f1..9e0cd313 100644 --- a/packages/worker/src/convergence-lifecycle.test.ts +++ b/packages/worker/src/convergence-lifecycle.test.ts @@ -90,6 +90,7 @@ describe("convergence threshold lifecycle", () => { ok: true, outcome: "confirmed", state: "In review", + routable: true, }) ) ) @@ -103,7 +104,12 @@ describe("convergence threshold lifecycle", () => { await expect( runConvergenceThreshold( new Response( - JSON.stringify({ ok: true, outcome: "confirmed", state: "Done" }) + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "Done", + routable: true, + }) ) ) ).resolves.toEqual({ @@ -118,7 +124,12 @@ describe("convergence threshold lifecycle", () => { await expect( runConvergenceThreshold( new Response( - JSON.stringify({ ok: true, outcome: "confirmed", state: "Land" }) + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "Land", + routable: true, + }) ) ) ).resolves.toEqual({ diff --git a/packages/worker/src/turn-lease.test.ts b/packages/worker/src/turn-lease.test.ts index 4eea244e..0383b351 100644 --- a/packages/worker/src/turn-lease.test.ts +++ b/packages/worker/src/turn-lease.test.ts @@ -109,7 +109,12 @@ describe("tracker refresh fail-closed threshold", () => { .fn() .mockResolvedValue( new Response( - JSON.stringify({ ok: true, outcome: "confirmed", state: "LAND" }) + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "LAND", + routable: true, + }) ) ); @@ -142,7 +147,12 @@ describe("tracker refresh fail-closed threshold", () => { .fn() .mockResolvedValue( new Response( - JSON.stringify({ ok: true, outcome: "confirmed", state: "Done" }) + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "Done", + routable: true, + }) ) ); @@ -179,6 +189,61 @@ describe("tracker refresh fail-closed threshold", () => { ).resolves.toBe("unknown"); }); + it("returns non-actionable when a refreshed active issue is not routable", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const fetchImpl = vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "In progress", + routable: false, + routableReason: 'Issue is missing required labels ("agent").', + }) + ) + ); + + await expect( + refreshTrackerState( + { + SYMPHONY_ORCHESTRATOR_URL: "http://localhost:4680", + SYMPHONY_ORCHESTRATOR_TOKEN: "worker-api-token", + SYMPHONY_RUN_ID: "run-1", + }, + ["Ready", "In progress", "Land"], + fetchImpl + ) + ).resolves.toBe("non-actionable"); + expect(errorSpy).toHaveBeenCalledWith( + '[worker] issue no longer routable: Issue is missing required labels ("agent").' + ); + errorSpy.mockRestore(); + }); + + it("fails closed when a confirmed read lacks a routability decision", async () => { + const fetchImpl = vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ + ok: true, + outcome: "confirmed", + state: "In progress", + }) + ) + ); + + await expect( + refreshTrackerState( + { + SYMPHONY_ORCHESTRATOR_URL: "http://localhost:4680", + SYMPHONY_ORCHESTRATOR_TOKEN: "worker-api-token", + SYMPHONY_RUN_ID: "run-1", + }, + ["Ready", "In progress", "Land"], + fetchImpl + ) + ).resolves.toBe("unknown"); + }); + it("returns unknown on transport failure for threshold accounting", async () => { const fetchImpl = vi.fn().mockRejectedValue(new Error("network error")); await expect( diff --git a/packages/worker/src/turn-lease.ts b/packages/worker/src/turn-lease.ts index 7a70b191..56bfe0fb 100644 --- a/packages/worker/src/turn-lease.ts +++ b/packages/worker/src/turn-lease.ts @@ -65,17 +65,25 @@ export async function refreshTrackerState( ok?: boolean; outcome?: string; state?: string | null; + routable?: boolean | null; + routableReason?: string | null; }; if ( result.ok !== true || result.outcome !== "confirmed" || - typeof result.state !== "string" + typeof result.state !== "string" || + typeof result.routable !== "boolean" ) { return "unknown"; } const active = matchesWorkflowState(result.state, activeStates); - return active ? "active" : "non-actionable"; + if (active && !result.routable) { + console.error( + `[worker] issue no longer routable: ${result.routableReason ?? "no reason provided"}` + ); + } + return active && result.routable ? "active" : "non-actionable"; } catch { return "unknown"; }