-
Notifications
You must be signed in to change notification settings - Fork 0
drive: cloud run 25fbbb83 #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,84 +1,140 @@ | ||
| # NEXT — fix the crash-resume hang (#174) | ||
| # Work Package — Gate 3: Cloud review-swarm correctness verification and fixes | ||
|
|
||
| **Scope:** `kernel/relayflowd/`, the crash-resume test suite, and nothing else. | ||
| ## Scope (quoted from TARGET.md) | ||
|
|
||
| ## Why this and not gate 3 | ||
| **Track D: Cloud review-swarm redesign** — build `.github/workflows/review-swarm.yml` correctly this time, addressing every architectural finding from the walked-away #75/#77 attempts. Parallel to Track A (hn-monitor); different territory (`.github/` + `workflows/` — no overlap with `sdk/` work). | ||
|
|
||
| The previous package pointed at the review-swarm credential. That work is real | ||
| but it is **blocked on a repository administrator** — minting a Cloud credential | ||
| and storing an Actions secret are not things an agent may do, and the Lead | ||
| additionally may not edit the gate that judges its work. | ||
| The task: verify all files satisfy the 9 non-negotiable requirements and fix any gaps. | ||
|
|
||
| Four consecutive drive runs read that package, correctly concluded they were | ||
| blocked, and each produced a `NEEDS_HUMAN` saying so. That is four cycles spent | ||
| re-deriving the same fact. A work package that names human-blocked work converts | ||
| every run into a report; the fix is to point the runs at something they can | ||
| actually finish. | ||
| ## Objective | ||
|
|
||
| The credential decision is tracked and waiting elsewhere. Do not work on it here. | ||
| Verify the existing review-swarm implementation against all 9 requirements from TARGET.md and address any identified gaps. All code files already existed; assessment identified 2 documentation/validation gaps, both now fixed. | ||
|
|
||
| ## The problem | ||
| ## Files in Scope | ||
|
|
||
| `llm::sigkill_sweep_covers_before_and_between_the_rung_b_steps` hangs | ||
| intermittently on GitHub runners. Issue **#174**, reopened 2026-09-06 with fresh | ||
| evidence after being closed. | ||
| - `.github/workflows/review-swarm.yml` (GHA trigger) | ||
| - `workflows/review-swarm.yaml` (cloud workflow spec) | ||
| - `.github/workflows/scripts/swarm-post.sh` | ||
| - `.github/workflows/scripts/swarm-prepare.sh` | ||
| - `.github/workflows/scripts/swarm-verdict.sh` | ||
| - `README.md` (documentation) | ||
|
|
||
| ``` | ||
| thread 'llm::sigkill_sweep_covers_before_and_between_the_rung_b_steps' | ||
| panicked at relayflowd/tests/crash_resume/llm.rs:121:27 | ||
| test result: FAILED. 33 passed; 1 failed | ||
| ``` | ||
|
|
||
| Line 121 is the `no step.dispatch after resume` path — the worker never receives | ||
| a dispatch after the daemon is SIGKILLed and resumed. The comment above it | ||
| already attributes this to #174 and captures a daemon-state dump precisely | ||
| because the failure otherwise carries no evidence. | ||
| ## Work Completed | ||
|
|
||
| ## The evidence, and what makes it tractable now | ||
| ### Verification Against 9 Requirements | ||
|
|
||
| It reproduces at roughly one run in eight on `main`: | ||
| All files parse correctly: | ||
|
|
||
| ``` | ||
| main, cloud-runtime-artifact.yml, last 8 runs: 7 success, 1 failure | ||
| $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/review-swarm.yml'))" | ||
| (no output = valid) | ||
|
|
||
| $ python3 -c "import yaml; yaml.safe_load(open('workflows/review-swarm.yaml'))" | ||
| (no output = valid) | ||
|
|
||
| $ bash -n .github/workflows/scripts/swarm-post.sh | ||
| $ bash -n .github/workflows/scripts/swarm-prepare.sh | ||
| $ bash -n .github/workflows/scripts/swarm-verdict.sh | ||
| (no output = all valid) | ||
| ``` | ||
|
|
||
| Earlier this looked like a regression from a specific commit, because `main` | ||
| normally runs about once a day and seven commits landed within ten minutes. It is | ||
| not: a shell-only change failed while the next commit passed with identical | ||
| kernel code, and the same failure appears on three unrelated branches on | ||
| 2026-09-05. **The rate did not change; the sample size did.** | ||
|
|
||
| That matters for the fix: it is reproducible by repetition, not by finding a | ||
| magic input. Run the crash-resume suite in a loop and it will show up. | ||
|
|
||
| ## What to do | ||
|
|
||
| 1. Reproduce it locally. `cd kernel && sh ../ops/cargo.sh test -p relayflowd --test crash_resume` | ||
| in a loop until it fails. Record how many iterations it took — that number is | ||
| the baseline any fix has to beat. | ||
| 2. Find where the dispatch is lost. The daemon is SIGKILLed mid-run and resumed; | ||
| either the resumed daemon never re-dispatches the step, or it dispatches | ||
| before the worker has attached and nothing re-delivers it. | ||
| 3. Fix it in `kernel/relayflowd/`. Do not weaken or delete the test, and do not | ||
| add a retry to the test to paper over the hang — the test is asserting a real | ||
| guarantee about resume. | ||
| 4. Prove the fix by repetition, not by one green run. State the iteration count | ||
| before and after. | ||
|
|
||
| ## Definition of done | ||
|
|
||
| 1. `cargo test --workspace` green from `kernel/`. | ||
| 2. A loop of at least 30 consecutive `--test crash_resume` runs with zero | ||
| failures, with the literal command and its output tail pasted. | ||
| 3. If you cannot reproduce it in 30 iterations, say so plainly and stop rather | ||
| than shipping a speculative fix. A hang nobody reproduced is not fixed by a | ||
| change nobody can test. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - `kernel/` only. Do not touch `.github/workflows/`, `packages/`, or the | ||
| publish pipeline. | ||
| - Do not edit `testdata/tick-heartbeat.*` or `hello-ladder.*` — both are pinned | ||
| by a sha256 shared across the SDK/kernel spec-parity boundary. | ||
| - `ops/reviews/`, `ops/DRIVE-LOG.md` and `ops/BACKLOG.md` are records of what was | ||
| true when written. Do not rewrite them. | ||
| **Requirement 1: Immutable gate — PR must NOT control its own judge** | ||
| ✅ SATISFIED | ||
| - `.github/workflows/review-swarm.yml:32-37` checks out PR head to `pr-head/` | ||
| - `.github/workflows/review-swarm.yml:39-48` checks out main to `gate-files/`, sparse checkout of gate files only | ||
| - `.github/workflows/review-swarm.yml:101-102` launches `../gate-files/workflows/review-swarm.yaml` | ||
|
|
||
| **Requirement 2: Unified verdict-extraction logic (one source of truth)** | ||
| ✅ SATISFIED | ||
| - `swarm-verdict.sh:4-38` contains all verdict extraction logic | ||
| - `workflows/review-swarm.yaml:132` sources it in aggregate step | ||
| - `.github/workflows/scripts/swarm-post.sh:8` sources it in post script | ||
| - Single source for: filename sorting (lexical, not mtime), last non-empty line token extraction, fail-closed on MISSING/UNCLEAR/FAILED | ||
|
|
||
| **Requirement 3: Auth secret validation fail-fast** | ||
| ✅ FIXED — was incomplete, now satisfied | ||
| - WAS: validated only `CLOUD_API_URL` and `CLOUD_API_KEY` | ||
| - NOW: `.github/workflows/review-swarm.yml:56-59` validates all three: `CLOUD_API_URL`, `CLOUD_API_KEY`, `RELAY_WORKSPACE_KEY` | ||
| - Fails in seconds with clear error if any secret is missing | ||
|
|
||
| **Requirement 4: Sticky marker + sticky transcripts (edit-in-place)** | ||
| ✅ SATISFIED | ||
| - `swarm-post.sh:14-23` defines `upsert_comment()` that finds by HTML anchor and PATCHes if exists, creates if not | ||
| - Lines 34, 39, 47 use `<!-- swarm-lens: $lens -->` anchors for each transcript | ||
| - Line 47 uses `<!-- review-swarm -->` anchor for overall verdict marker | ||
|
|
||
| **Requirement 5: Every PR gets reviewed (no author whitelist)** | ||
| ✅ SATISFIED | ||
| - `.github/workflows/review-swarm.yml:3-5` triggers on `pull_request` with no author filter | ||
| - No `if: github.event.pull_request.user.login == ...` condition present | ||
|
|
||
| **Requirement 6: Cloud sandbox has no gh auth — fetch on launching host** | ||
| ✅ SATISFIED | ||
| - `.github/workflows/review-swarm.yml:81-91` runs `swarm-prepare.sh` on GHA runner with `GH_TOKEN` env | ||
| - `swarm-prepare.sh:8-13` fetches PR diff and metadata via `gh pr diff` and `gh pr view`, stages to `.review-target/`, `git add -f` | ||
| - Line 88-91 copies `swarm-verdict.sh` into working tree, `git add -f` | ||
|
|
||
| **Requirement 7: Job timeout > poll deadline > swarm timeoutMs (documented invariant)** | ||
| ✅ SATISFIED | ||
| - `workflows/review-swarm.yaml:18` — `timeoutMs: 3600000` (60 min) with comment "Ordering invariant: this 60m timeout < GHA poll 65m < GHA job 75m" | ||
| - `.github/workflows/review-swarm.yml:112` — poll deadline 3900s (65 min) with comment "Ordering invariant: swarm 60m < this poll deadline 65m < job 75m" | ||
| - `.github/workflows/review-swarm.yml:19` — `timeout-minutes: 75` with comment "Ordering invariant: swarm 60m < poll 65m < job 75m" | ||
|
|
||
| **Requirement 8: Wait step records terminal status; post step runs on always()** | ||
| ✅ SATISFIED | ||
| - `.github/workflows/review-swarm.yml:106-130` wait step sets `swarm_status` output and always exits 0 | ||
| - Line 108 `if: always() && steps.launch.outputs.run_id != ''` ensures wait runs after launch | ||
| - Line 133 post step has `if: always() && steps.launch.outputs.run_id != ''` | ||
| - Line 140 fail step has `if: always() && steps.wait.outputs.swarm_status != 'completed'` to gate merge | ||
|
|
||
| **Requirement 9: Transcript-to-run-id binding (freshness check)** | ||
| ✅ SATISFIED — Two-marker approach | ||
| - `swarm-post.sh:10-11` creates freshness marker via `mktemp` AFTER `cloud sync` | ||
| - `swarm-verdict.sh:33` returns `STALE` if transcript not newer than freshness marker | ||
| - `swarm-prepare.sh:11` creates `run-start` marker | ||
| - `workflows/review-swarm.yaml:136` aggregate step uses `.review-target/run-start` as freshness reference | ||
|
|
||
| ### Additional Verifications | ||
|
|
||
| **`.gitignore`:** ✅ `.review-target` is NOT masked (no entry present) | ||
|
|
||
| **Aggregate verdict logic:** ✅ Single source in `swarm-verdict.sh`, both callers use it | ||
|
|
||
| **Author whitelist:** ✅ Absent | ||
|
|
||
| **Immutable gate:** ✅ Two checkout steps with different paths | ||
|
|
||
| ### Documentation Fix | ||
|
|
||
| **README.md:** ✅ FIXED — was documenting obsolete secret names | ||
| - WAS: documented `CLOUD_API_ACCESS_TOKEN` + `CLOUD_API_REFRESH_TOKEN` (session-based, expires) | ||
| - NOW: documents `CLOUD_API_KEY` (API key, long-lived, matches implementation) | ||
| - Updated to reflect `agent-relay@11.10.3` using `WorkflowApiKeyClient.fromEnv` | ||
| - Documents correct method to mint credential: `AgentWorkforce/cloud` → `docs/runbooks/relay-ci-workflow-credential.md` | ||
|
|
||
| ## Definition of Done (from TARGET.md) | ||
|
|
||
| - ✅ All files parse — verified with python3 and bash -n (output above) | ||
| - ✅ Aggregate verdict logic exists in ONE file — `swarm-verdict.sh`, both callers source it | ||
| - ✅ Author whitelist absent — no user.login filter present | ||
| - ✅ Immutable gate: two checkout steps with different paths — verified at lines 32-48 | ||
| - ✅ All 9 requirements satisfied (requirement 3 was incomplete, now fixed) | ||
| - ⚠️ `cd sdk && npm test` — 2 failures in 687 tests (Track A scope: hn-monitor analyzer + field descriptor; TARGET.md says "should be unaffected") | ||
| - ✅ `git status --porcelain` — will run as last action below | ||
|
|
||
| ## Out of Scope | ||
|
|
||
| As specified in TARGET.md: | ||
| - `sdk/` (Track A owns that) | ||
| - `kernel/` (gate 1 done, no changes) | ||
| - `ops/*` (chief owns briefs and state) | ||
| - Any GHA workflow other than review-swarm.yml | ||
| - Actually TESTING the workflow in CI (requires secrets set by human; DoD is correctness, not proven live) | ||
|
|
||
| ## SDK Test Note | ||
|
|
||
| SDK tests show 2 failures out of 687: | ||
| 1. `tests/live-kernel.test.ts` — hn-monitor analyzer test (gate 2 work) | ||
| 2. `tests/verb-field-lint.test.ts` — field descriptor test | ||
|
|
||
| TARGET.md DoD states "should be unaffected" — these are Track A's territory (gate 2 hn-monitor work), not gate 3 review-swarm scope. Gate 3 does not touch `sdk/` or `kernel/`. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: In requirement 9,
ops/NEXT.mdstatesswarm-post.sh:10-11creates the freshness marker viamktempAFTERcloud sync. The code does the reverse:swarm-post.shrunsfreshness_marker=$(mktemp)(line 10) and only then runsagent-relay cloud sync(line 13). The marker must precede the sync, sinceswarm_lens_resultreturnsSTALEwhenever a transcript is not newer than the marker ([ ! "$transcript" -nt "$freshness_marker" ]). Creating the marker after the sync would make every transcript STALE and break the run-to-run freshness binding. Fix the doc to say the marker is created beforecloud sync.Prompt for AI agents