Merge pull request #101 from githubnext/codex/fix-crane-stale-completion

fix(crane): ignore stale completion for active migrations

Author: mrjf

.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.

.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,

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

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

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