From ed3d55dfa0485f2a448c576d0aae3c55a08e653f Mon Sep 17 00:00:00 2001 From: mrjf Date: Tue, 2 Jun 2026 09:51:24 -0700 Subject: [PATCH 1/2] fix(crane): ignore stale completion for active migrations --- .github/workflows/crane.md | 13 ++++-- .github/workflows/scripts/crane_scheduler.py | 24 +++++++++-- tests/unit/test_crane_scheduler.py | 44 ++++++++++++++++++++ tests/unit/test_crane_workflow_prompt.py | 9 ++++ 4 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_crane_scheduler.py diff --git a/.github/workflows/crane.md b/.github/workflows/crane.md index bde68cae..8c229044 100644 --- a/.github/workflows/crane.md +++ b/.github/workflows/crane.md @@ -180,6 +180,7 @@ The pre-step fetches open issues with the `crane-migration` label via the GitHub When a migration is issue-based, `/tmp/gh-aw/crane.json` includes: - **`selected_issue`**: The issue number (e.g., `42`) if the selected migration came from an issue, or `null` if it came from a file. - **`issue_migrations`**: A mapping of migration name -> issue number for all issue-based migrations found. +- **`stale_completed_state`**: Issue-based migrations whose issue still has `crane-migration` even though repo-memory says `Completed: true`. ### Reading Migrations @@ -193,6 +194,7 @@ The pre-step has already determined which migration to run. Read `/tmp/gh-aw/cra - **`selected_strategy`**: The `strategy` value from the migration's frontmatter -- one of `"in-place"`, `"greenfield"`, or `"auto"`. If `"auto"`, the agent must pick on the first iteration and write the chosen strategy back into the state file's Machine State table. - **`state_file_size_bytes`** / **`state_file_max_bytes`**: For rolling-compaction decisions (see [Update Rules](#update-rules)). - **`issue_migrations`**: A mapping of migration name -> issue number for all discovered issue-based migrations. +- **`stale_completed_state`**: A list of active issue-based migrations where repo-memory still says `Completed: true`; treat this as stale memory, not as permission to finish. - **`deferred`**: Other migrations that were due but will be handled in future runs. - **`unconfigured`**: Migrations that still have the sentinel or placeholder content. - **`skipped`**: Migrations not due yet based on their per-migration schedule, or completed/paused. @@ -207,6 +209,7 @@ If `selected` is not null: 3. Read the current state of all source and target paths. 4. Read the state file `{selected}.md` from the repo-memory folder. This contains the Machine State table, the Migration Plan, lessons, blockers, and iteration history. 5. If `selected_issue` is not null, also read the issue comments for any human steering input. +6. If `selected` appears in `stale_completed_state`, ignore any pre-existing `Completed: true`, `Completed Reason`, or target-satisfying `best_metric` as a completion signal. First run the current verification contract and only re-complete after a fresh accepted iteration satisfies the current halting condition. ## Multiple Migrations @@ -237,7 +240,7 @@ schedule: every 1h ### Target Metric (Halting Condition) -Migrations should usually specify `target-metric: 1.0` in the frontmatter -- the typical "completed when fully migrated and verified" setting. When the health score reaches the target, the migration completes: the `crane-migration` label is removed, `crane-completed` is added (for issue-based migrations), and the state file is marked `Completed: true`. +Migrations should usually specify `target-metric: 1.0` in the frontmatter -- the typical "completed when fully migrated and verified" setting. When the fresh health score from an accepted iteration reaches the target, the migration completes: the `crane-migration` label is removed, `crane-completed` is added (for issue-based migrations), and the state file is marked `Completed: true`. Migrations without a `target-metric` are **open-ended** and run indefinitely (rare for migrations -- usually a sign you actually want goal-oriented). @@ -540,12 +543,13 @@ If `status == "failure"`, **fix and retry -- do not revert, do not accept**: 3. Ensure the migration issue exists (see [Migration Issue](#migration-issue) below) -- for file-based migrations with no migration issue yet (`selected_issue` is null in `/tmp/gh-aw/crane.json`), create one and record its number in the state file's `Issue` field. 4. Update the state file `{migration-name}.md` in the repo-memory folder: - **[*] Machine State** table: reset `consecutive_errors` to 0, set `best_metric` (the new `migration_score`), increment `iteration_count`, set `last_run` to current UTC, append `"accepted"` to `recent_statuses` (keep last 10), set `paused` to false. + - If this migration was listed in `stale_completed_state`, also set `Completed: false` and `Completed Reason: --` before checking the halting condition. Do not carry a stale completion marker forward unless the current accepted iteration completes again. - **[ladder] Milestones**: update the relevant milestone's status -- typically `done` if the milestone was fully completed, otherwise leave `in-progress` and update its notes. If the milestone is done, the next milestone in the list becomes the new **[target] Current Focus**. - Prepend an entry to **[chart] Iteration History** using the shared accepted iteration summary: status [+], score, **signed delta**, PR link, commit SHA, run URL, fix-attempt count if `> 0`, and a one-line summary of what milestone was advanced and how. - Update **[docs] Lessons Learned** if this iteration revealed something new (e.g. a bridging trick, a parity surprise, a perf trap). - Update **[scope] Future Work** if this iteration opened new threads. 5. **Update the migration issue**: edit the status comment and post a per-iteration comment using the same shared accepted iteration summary. -6. **Check halting condition** (see [Halting Condition](#halting-condition)): if `target-metric` is set, compare the new `best_metric` against it. For `higher` direction: completed when `best_metric >= target-metric`. When the target is met, mark the migration as completed. +6. **Check halting condition** (see [Halting Condition](#halting-condition)): if `target-metric` is set, compare the new `best_metric` from this accepted iteration against it. For `higher` direction: completed when `best_metric >= target-metric`. Never mark completed from stored `best_metric` alone or a pre-existing `Completed: true`; completion requires the current accepted iteration's fresh verification result. When the target is met, mark the migration as completed. **If the score did not improve**: 1. Discard the code changes (do not commit them to the long-running branch). @@ -648,6 +652,7 @@ After **every iteration** (accepted, rejected, or error), post a **new comment** - For issue-based migrations, the source issue body IS the migration definition -- do not modify it (the user owns it). - For file-based migrations, the migration issue body is informational and may be lightly updated, but the migration file (`migration.md`) remains the source of truth. - The `crane-migration` label must remain on the issue for the migration to be discovered. When a migration completes, the label is removed and replaced with `crane-completed`. +- If an issue has `crane-migration` but repo-memory says `Completed: true`, the active label wins. Treat the completed state as stale until the current verification contract passes in a fresh accepted iteration. - Closing the migration issue stops the migration from being discovered. Do NOT close the migration issue when the PR is merged -- the branch continues to accumulate future iterations until the target metric is reached. - Migration issues are labeled `[crane-migration, automation, crane]`. @@ -658,7 +663,7 @@ Migrations are usually **goal-oriented** -- you want to finish. Set `target-metr ### How It Works 1. Parse the `target-metric` value from the migration's YAML frontmatter (if present). -2. After each **accepted** iteration, compare the new `best_metric` against the `target-metric`. +2. After each **accepted** iteration, compare the new `best_metric` from that iteration against the `target-metric`. 3. For `higher` direction (default): completed when `best_metric >= target-metric`. 4. For `lower` direction: completed when `best_metric <= target-metric`. 5. When completed: @@ -669,6 +674,8 @@ Migrations are usually **goal-oriented** -- you want to finish. Set `target-metr - Post a celebratory per-iteration comment: `[+] **Migration complete!** {source} -> {target} finished after {N} iterations.` - The migration will not be selected for future runs. +Do not enter this path from repo-memory alone. A stored `Completed: true`, old `Completed Reason`, or historical `best_metric` is only evidence about a previous run; the current run must produce and accept a verification score that satisfies the current migration definition. + ### Open-Ended Migrations Migrations that omit `target-metric` run indefinitely. Useful if you want Crane to keep optimizing a polyglot system long after the initial migration is done (e.g. continuously identifying new hot paths to lift into the native core), but unusual for a one-shot port. diff --git a/.github/workflows/scripts/crane_scheduler.py b/.github/workflows/scripts/crane_scheduler.py index 88ff8d5a..40648151 100644 --- a/.github/workflows/scripts/crane_scheduler.py +++ b/.github/workflows/scripts/crane_scheduler.py @@ -214,9 +214,14 @@ def is_unconfigured(content): return False -def check_skip_conditions(state): +def is_completed_state(state): + """Return True when repo-memory says the migration is completed.""" + return str(state.get("completed", "")).lower() == "true" or state.get("completed") is True + + +def check_skip_conditions(state, issue_active=False): """Return ``(should_skip, reason)`` based on the migration state.""" - if str(state.get("completed", "")).lower() == "true" or state.get("completed") is True: + if is_completed_state(state) and not issue_active: return True, "completed: target metric reached" if state.get("paused"): return True, "paused: {}".format(state.get("pause_reason", "unknown")) @@ -606,6 +611,7 @@ def main(): "due": [], "skipped": [], "unconfigured": [], + "stale_completed_state": [], "no_migrations": True, "head_branch": None, "existing_pr": None, @@ -618,10 +624,12 @@ def main(): due = [] skipped = [] unconfigured = [] + stale_completed_state = [] all_migrations = {} # name -> file path for pf in migration_files: name = get_migration_name(pf) + issue_active = name in issue_migrations all_migrations[name] = pf with open(pf) as f: content = f.read() @@ -661,6 +669,14 @@ def main(): else: print(" {}: no state found (first run)".format(name)) + has_stale_completed_state = issue_active and is_completed_state(state) + if has_stale_completed_state: + stale_completed_state.append(name) + print( + f" {name}: issue still has crane-migration label; treating " + "Completed=true as stale until fresh verification passes" + ) + last_run = None lr = state.get("last_run") if lr: @@ -669,7 +685,7 @@ def main(): except ValueError: pass - should_skip, reason = check_skip_conditions(state) + should_skip, reason = check_skip_conditions(state, issue_active=issue_active) if should_skip: skipped.append({"name": name, "reason": reason}) continue @@ -692,6 +708,7 @@ def main(): "target_metric": target_metric, "metric_direction": metric_direction, "strategy": strategy, + "stale_completed_state": has_stale_completed_state, }) selected, selected_file, selected_issue, selected_target_metric, selected_metric_direction, selected_strategy, deferred, error = ( @@ -728,6 +745,7 @@ def main(): "issue_migrations": { name: info["issue_number"] for name, info in issue_migrations.items() }, + "stale_completed_state": stale_completed_state, "deferred": deferred, "skipped": skipped, "unconfigured": unconfigured, diff --git a/tests/unit/test_crane_scheduler.py b/tests/unit/test_crane_scheduler.py new file mode 100644 index 00000000..55c373a5 --- /dev/null +++ b/tests/unit/test_crane_scheduler.py @@ -0,0 +1,44 @@ +from __future__ import annotations + +import importlib.util +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +SCHEDULER_PATH = ROOT / ".github" / "workflows" / "scripts" / "crane_scheduler.py" + +spec = importlib.util.spec_from_file_location("crane_scheduler", SCHEDULER_PATH) +assert spec is not None +crane_scheduler = importlib.util.module_from_spec(spec) +assert spec.loader is not None +spec.loader.exec_module(crane_scheduler) + + +def test_completed_state_skips_inactive_migration() -> None: + should_skip, reason = crane_scheduler.check_skip_conditions({"completed": True}) + + assert should_skip is True + assert reason == "completed: target metric reached" + + +def test_active_issue_overrides_stale_completed_state() -> None: + should_skip, reason = crane_scheduler.check_skip_conditions( + {"completed": True}, + issue_active=True, + ) + + assert should_skip is False + assert reason is None + + +def test_active_issue_does_not_override_pause() -> None: + should_skip, reason = crane_scheduler.check_skip_conditions( + {"completed": True, "paused": True, "pause_reason": "manual hold"}, + issue_active=True, + ) + + assert should_skip is True + assert reason == "paused: manual hold" + + +def test_machine_state_completed_string_is_recognized() -> None: + assert crane_scheduler.is_completed_state({"completed": "true"}) is True diff --git a/tests/unit/test_crane_workflow_prompt.py b/tests/unit/test_crane_workflow_prompt.py index b754623f..c88afdc3 100644 --- a/tests/unit/test_crane_workflow_prompt.py +++ b/tests/unit/test_crane_workflow_prompt.py @@ -29,3 +29,12 @@ def test_crane_commit_guidance_provides_structured_summary_fallback() -> None: assert "Changes:" in text assert "Run: {run_url}" in text assert text.index("Changes:") < text.index("Run: {run_url}") + + +def test_crane_prompt_blocks_stale_completed_state_from_finishing() -> None: + text = _workflow_text() + + assert "stale_completed_state" in text + assert "active label wins" in text + assert "Never mark completed from stored `best_metric` alone" in text + assert "the current run must produce and accept a verification score" in text From eb650d1a610b6b693287b572ff09a4fdbc6503ac Mon Sep 17 00:00:00 2001 From: mrjf Date: Tue, 2 Jun 2026 09:53:23 -0700 Subject: [PATCH 2/2] docs(changelog): note Crane stale completion fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d80b477..de29c02b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Crane no longer treats stale repo-memory `Completed: true` as final when an issue still has `crane-migration`; active issue migrations must re-run fresh verification before completion. (#101) - Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. The hook source-id is now derived from `apm.yml`'s `name` field instead of `install_path.name`, and `apm install` silently heals stale same-content entries from prior checkout basenames. Copilot is unaffected (its hooks live in per-file namespaces under `.github/hooks/`, not a shared merged config). (#1392, closes #1329) ## [0.14.1] - 2026-05-20