diff --git a/author_issue_bootstrap.py b/author_issue_bootstrap.py index 20862c5..10834f1 100644 --- a/author_issue_bootstrap.py +++ b/author_issue_bootstrap.py @@ -279,6 +279,36 @@ def _verify_assignment_and_lease_ids( return None +def _branch_exists(canonical_repo_root: str, branch_name: str) -> bool: + """Whether *branch_name* still resolves in the canonical checkout (#953 F2). + + Used after compensating recovery to observe what survived rather than infer + it from the journal. Fails closed to ``True``: an unobservable branch is + reported as present, so the recommendation stays conservative rather than + telling an author to re-bootstrap over something that may still be there. + """ + if not branch_name: + return False + try: + res = subprocess.run( + [ + "git", + "-C", + canonical_repo_root, + "rev-parse", + "--verify", + "--quiet", + f"refs/heads/{branch_name}", + ], + capture_output=True, + text=True, + check=False, + ) + except Exception: + return True + return res.returncode == 0 + + def run_compensating_recovery( journal: dict[str, Any], canonical_repo_root: str, @@ -315,10 +345,21 @@ def run_compensating_recovery( issue_number=issue_num, session=session_id, lock_dir=journal_dir, + remote=journal.get("remote"), + # The same defaults the lock was written under, so the + # rollback targets the exact file bind_session_lock keyed. + org=journal.get("org") or "Scaled-Tech-Consulting", + repo=journal.get("repo") or "Gitea-Tools", ) rolled_back.append(f"lock:issue-{issue_num}") - except Exception: - pass + except Exception as exc: + # #953 F2: a swallowed failure here is what made the rollback + # report success while leaving an unrecoverable lock behind. + # Record it so the post-compensation classification can see the + # lock survived and recommend accordingly. + rolled_back.append( + f"lock_release_failed:issue-{issue_num}:{type(exc).__name__}" + ) artifacts["lock_created"] = False worktree_created = ( @@ -1255,7 +1296,21 @@ def bootstrap_author_issue_worktree( journal["failure_reason"] = author_lock_contract.format_contract_refusal( contract ) - run_compensating_recovery(journal, root, journal_dir=lock_dir) + compensation = run_compensating_recovery( + journal, root, journal_dir=lock_dir + ) + # AC5/AC15: the recommendation must describe the state compensation + # actually left, not the state that provoked it. + # ``run_compensating_recovery`` has by now released the lock, removed + # the worktree, and deleted the branch, so recommending + # incomplete-lock recovery for those exact artifacts would refuse + # twice over. Observe what survived and answer for that. + post_state = author_lock_contract.assess_post_compensation_state( + compensation, + lock_present=bool(lock_res) and os.path.exists(lock_res), + worktree_present=os.path.isdir(target_worktree), + branch_present=_branch_exists(root, target_branch), + ) return { "success": False, "reason_code": "incomplete_issue_lock_contract", @@ -1267,9 +1322,18 @@ def bootstrap_author_issue_worktree( "lock_contract": contract, "missing_fields": contract["missing_fields"], "implementation_allowed": False, + "compensating_recovery": compensation, + "post_compensation_state": post_state, # AC15: never strand a branch or worktree without a structured - # recovery recommendation. - "exact_next_action": author_lock_contract.recommended_action(contract), + # recovery recommendation — and never name an artifact the + # rollback has already deleted. + "exact_next_action": author_lock_contract.post_compensation_action( + post_state, + issue_number=issue_number, + branch_name=target_branch, + worktree_path=target_worktree, + missing_fields=contract["missing_fields"], + ), "phase_journal": journal, } diff --git a/author_lock_contract.py b/author_lock_contract.py index 05cf42a..47db9e2 100644 --- a/author_lock_contract.py +++ b/author_lock_contract.py @@ -43,8 +43,12 @@ import lease_policy # safety requirements forbid. SOURCE_BOOTSTRAP = issue_lock_provenance.SOURCE_LOCK_ISSUE -#: Recovery of an incomplete bootstrap lock (#953 AC8-AC11). -SOURCE_BOOTSTRAP_LOCK_RECOVERY = "gitea_recover_incomplete_bootstrap_lock" +# Recovery of an incomplete bootstrap lock (#953 AC8-AC11) deliberately writes +# through SOURCE_LOCK_ISSUE too, and records its distinctness in +# ``lock_provenance.written_by_tool`` plus the ``bootstrap_lock_recovery`` +# transition block instead. There is no distinct recovery *source* constant, for +# the same reason bootstrap has none: minting one would require widening +# SANCTIONED_LOCK_SOURCES, which the #447 safety requirements forbid. #: Top-level keys every canonical author issue lock must carry. REQUIRED_LOCK_FIELDS: tuple[str, ...] = ( @@ -440,3 +444,180 @@ def recommended_action(assessment: Mapping[str, Any]) -> str: "and worktree to upgrade the lock to the canonical contract, or " "gitea_lock_issue while the worktree is still base-equivalent." ) + + +# ── Post-compensation recovery guidance (#953 AC5/AC15, review 632 F2) ── +# +# ``recommended_action`` above answers "what can be done about a lock in this +# shape". That is the wrong question on the bootstrap AC7 refusal path, because +# ``run_compensating_recovery`` has already run by the time the answer is +# reported: it releases the lock, removes the worktree when clean — which it +# always is there, no implementation bytes having been written — and deletes the +# created branch. Recommending incomplete-lock recovery for those artifacts +# hands the author two refusals in a row (``no_durable_lock``, then +# ``worktree_invalid``) for a state that a plain bootstrap retry would fix. The +# advice must describe the state that actually *remains*. + +#: Compensation removed every artifact this transition created. +CLEANUP_COMPLETE = "complete" +#: Compensation removed some artifacts; others survive and are still actionable. +CLEANUP_PARTIAL = "partial" +#: Compensation itself failed or could not be observed; nothing is provable. +CLEANUP_FAILED = "failed" + + +def assess_post_compensation_state( + recovery: Mapping[str, Any] | None, + *, + lock_present: bool, + worktree_present: bool, + branch_present: bool, +) -> dict[str, Any]: + """Classify what survived compensation, from observed durable state. + + Pure. The caller observes the filesystem and git; this decides. Observation + is authoritative over the journal's ``rolled_back`` list, which records what + compensation *attempted*: ``run_compensating_recovery`` swallows a failed + lock release and appends nothing, so an absent marker proves nothing either + way. The list is still carried through as corroborating evidence. + + The three states are distinct facts, not degrees of the same one: + + * ``CLEANUP_COMPLETE`` — compensation ran and nothing it created remains. + * ``CLEANUP_PARTIAL`` — compensation ran and artifacts survive, whether by + design (a worktree dirty at rollback time, a branch carrying commits) or + because a rollback step errored. Either way the surviving set was observed + directly, so it is known and actionable; ``failed_rollback_steps`` records + which cause applies. + * ``CLEANUP_FAILED`` — compensation never ran to completion, so nothing it + would have removed can be assumed removed. + """ + rolled_back = list((recovery or {}).get("rolled_back") or []) + executed = bool((recovery or {}).get("executed")) + failed_steps = [entry for entry in rolled_back if "_failed" in entry] + + surviving: list[str] = [] + if lock_present: + surviving.append("lock") + if worktree_present: + surviving.append("worktree") + if branch_present: + surviving.append("branch") + + if not executed: + state = CLEANUP_FAILED + elif surviving: + state = CLEANUP_PARTIAL + else: + state = CLEANUP_COMPLETE + + return { + "cleanup_state": state, + "compensation_executed": executed, + "lock_present": bool(lock_present), + "worktree_present": bool(worktree_present), + "branch_present": bool(branch_present), + "surviving_artifacts": surviving, + "removed_artifacts": [ + name + for name, present in ( + ("lock", lock_present), + ("worktree", worktree_present), + ("branch", branch_present), + ) + if not present + ], + "failed_rollback_steps": failed_steps, + "rolled_back": rolled_back, + } + + +def post_compensation_action( + state: Mapping[str, Any], + *, + issue_number: int, + branch_name: str, + worktree_path: str, + missing_fields: list[str] | None = None, +) -> str: + """The one executable next step for the state compensation actually left. + + Every branch names only artifacts the classification says still exist, so no + recommendation can point at something the rollback deleted. + """ + missing = ", ".join(missing_fields or []) or "the reported missing fields" + cleanup_state = state.get("cleanup_state") + lock_present = bool(state.get("lock_present")) + worktree_present = bool(state.get("worktree_present")) + branch_present = bool(state.get("branch_present")) + + if not state.get("compensation_executed"): + # Compensation never ran, so nothing was rolled back and nothing about + # the remaining state was decided. The read-only surface is the only + # action executable under any state. + return ( + "Compensating rollback did not complete, so the remaining state is " + f"not proven. Call gitea_inspect_issue_lock_contract for issue " + f"#{issue_number} (read-only) to establish what survives before any " + "further action. Do not retry bootstrap until it is known." + ) + + prefix = "" + failed_steps = state.get("failed_rollback_steps") or [] + if failed_steps: + prefix = ( + "Compensating rollback reported a failed step " + f"({', '.join(failed_steps)}); what survives was observed directly " + "and the action below is scoped to exactly that. " + ) + + if cleanup_state == CLEANUP_COMPLETE: + return ( + "Compensating rollback removed the malformed lock, the branch, and " + f"the worktree, so nothing from this attempt remains. Resolve " + f"{missing} and re-run gitea_bootstrap_author_issue_worktree for " + f"issue #{issue_number} from the clean pre-bootstrap state. Do not " + "call gitea_recover_incomplete_bootstrap_lock: there is no lock, " + "branch, or worktree left for it to act on." + ) + + if lock_present and worktree_present and branch_present: + return prefix + ( + "The lock, branch, and worktree all survive. Call " + "gitea_recover_incomplete_bootstrap_lock for issue " + f"#{issue_number}, branch '{branch_name}', and worktree " + f"'{worktree_path}', passing the worktree's current head as " + "expected_head, to upgrade the lock to the canonical contract." + ) + + if not lock_present and worktree_present and branch_present: + return prefix + ( + "The malformed lock was released but the branch and worktree " + "survive. No implementation bytes were written, so the worktree is " + f"still base-equivalent: call gitea_lock_issue for issue " + f"#{issue_number} on branch '{branch_name}' from worktree " + f"'{worktree_path}' to acquire a canonical lock." + ) + + if lock_present and not worktree_present: + return prefix + ( + f"The worktree for issue #{issue_number} is gone but the durable " + "lock survived, so neither gitea_recover_incomplete_bootstrap_lock " + "(it would refuse worktree_invalid) nor gitea_lock_issue (it has no " + "worktree to bind) is executable. Call " + "gitea_inspect_issue_lock_contract for issue " + f"#{issue_number} (read-only) to confirm the surviving lock; it " + "must be released by its recorded owner before bootstrap is " + "retried." + ) + + # Lock gone, worktree gone, some git artifact left (a branch with commits, + # or a branch this transition did not create). + return prefix + ( + "Compensating rollback removed the lock and worktree; branch " + f"'{branch_name}' survives and was not deleted. Call " + f"gitea_inspect_issue_lock_contract for issue #{issue_number} " + "(read-only) to confirm no durable lock remains, then re-run " + "gitea_bootstrap_author_issue_worktree, which will adopt the existing " + "branch rather than recreating it." + ) diff --git a/docs/author-issue-lock-contract.md b/docs/author-issue-lock-contract.md index f9f360d..0fb51c6 100644 --- a/docs/author-issue-lock-contract.md +++ b/docs/author-issue-lock-contract.md @@ -38,6 +38,44 @@ If bootstrap returns `success: false` with `exact_next_action` names the executable recovery step. Bootstrap's reported next action always matches the state it actually returned. +### What that refusal leaves behind + +The AC7 refusal runs `run_compensating_recovery` *before* it reports, so the +advice has to describe the post-rollback state rather than the shape of the lock +that provoked it. Recommending incomplete-lock recovery for artifacts the +rollback already deleted would produce `no_durable_lock` and then +`worktree_invalid` — two refusals for a state a plain retry fixes. + +The refusal therefore carries `compensating_recovery` and +`post_compensation_state`, and derives `exact_next_action` from what was +observed on disk. `cleanup_state` is one of: + +| `cleanup_state` | Meaning | Next action | +| --- | --- | --- | +| `complete` | lock, branch, and worktree all removed | resolve `missing_fields` and re-run `gitea_bootstrap_author_issue_worktree` | +| `partial` | rollback ran; some artifacts survive, by design or because a step errored | scoped to exactly what survives — see below | +| `failed` | rollback never completed, so nothing is proven removed | `gitea_inspect_issue_lock_contract` (read-only) before anything else | + +Within `partial`, the surviving set decides the action: + +| Survives | Next action | +| --- | --- | +| lock + branch + worktree | `gitea_recover_incomplete_bootstrap_lock` for that exact issue, branch, and worktree | +| branch + worktree (lock released) | `gitea_lock_issue` — no implementation bytes were written, so the worktree is still base-equivalent | +| lock only (worktree removed) | `gitea_inspect_issue_lock_contract`; the surviving lock must be released by its recorded owner before bootstrap is retried | +| branch only | `gitea_inspect_issue_lock_contract`, then re-run bootstrap, which adopts the existing branch | + +`failed_rollback_steps` names any rollback step that errored, and the returned +action says so rather than presenting the surviving state as intentional. + +> The lock half of that rollback was dead code until #953 review 632 F2: +> `run_compensating_recovery` called `issue_lock_store.release_session_lock`, +> which did not exist, inside a bare `except Exception: pass`. Every rollback +> removed the branch and worktree and silently left the lock — the exact +> uninspectable, unrecoverable state this issue exists to eliminate. The +> function now exists, releases only a lock whose recorded `owner_session` +> matches, and its failures are recorded rather than swallowed. + ## The contract A canonical lock carries every field in @@ -125,6 +163,29 @@ the transition — prior contract, prior missing fields, prior generation, prior owning session, the replacement `task_session_id`, and the preserved head — so a recovered claim never reads as an original one. +### Gates, in order + +`gitea_recover_incomplete_bootstrap_lock` is an author-only durable-lock +mutation and carries the same three gates as every comparable author operation, +in this order: + +1. `role_session_router.check_author_mutation_after_reviewer_stop` — no author + fallback after a reviewer `wrong_role_stop`. +2. `_namespace_mutation_block(task, remote=remote, author_role_exclusive=True)` — + the namespace wall. It refuses a reviewer-bound session and, because this + task's required permission is `gitea.issue.comment` (which merger, + controller, and reconciler profiles also hold), additionally requires the + active profile's derived role kind to be exactly `author`. A refusal carries + `namespace_block: true` and emits a `BLOCKED` audit record naming the + namespace and profile. +3. `_profile_permission_block` — operation, provenance, and session-context + gates. + +Exact-owner claimant matching inside `assess_bootstrap_lock_recovery` runs +*after* all three. It is a further layer, never a substitute for them: on its +own it refuses one step too late and leaves the audit trail silent about the +attempt. + ### Refusals | `refusal_code` | Meaning | diff --git a/issue_lock_store.py b/issue_lock_store.py index 278e8d0..8237305 100644 --- a/issue_lock_store.py +++ b/issue_lock_store.py @@ -640,6 +640,99 @@ def iter_lock_files(lock_dir: str | None = None) -> list[str]: return sorted(paths) +def release_session_lock( + *, + issue_number: int, + session: str, + lock_dir: str | None = None, + remote: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> str: + """Remove exactly the durable lock *session* created for *issue_number*. + + ``author_issue_bootstrap.run_compensating_recovery`` has called this name + since #850, but it was never defined: the call raised ``AttributeError`` + into a bare ``except Exception: pass``, so the lock half of every + compensating rollback silently did nothing. The branch and worktree were + removed and the lock was left behind — a state no sanctioned tool can act + on, since recovery refuses ``worktree_invalid`` and ``gitea_lock_issue`` has + no worktree to bind (#953 review 632 F2). + + Ownership is proven, not asserted. A record is removed only when its + recorded ``owner_session`` equals *session* and its issue number matches; + ``remote``/``org``/``repo`` narrow it further when supplied. Zero matches or + more than one both raise, so a caller can never delete a lock it does not + own and an ambiguous directory is never guessed at. The ``.json.lock`` flock + sidecar is deliberately left in place — it is a zero-byte mutex another + process may hold, and removing it under contention would be a race. + + Returns the removed lock file path. + """ + target_issue = int(issue_number) + owner = str(session or "").strip() + if not owner: + raise ValueError( + "release_session_lock requires the owning session id (fail closed)" + ) + + def _is_owned_durable_lock(record: dict[str, Any] | None) -> bool: + # A durable lock, not a bootstrap phase journal or a session pointer, + # both of which can share a directory and carry the same issue number + # and owner_session. + if not record or "lock_generation" not in record: + return False + if not str(record.get("branch_name") or "").strip(): + return False + if not str(record.get("worktree_path") or "").strip(): + return False + try: + if int(record.get("issue_number") or 0) != target_issue: + return False + except (TypeError, ValueError): + return False + return str(record.get("owner_session") or "").strip() == owner + + # Prefer the exact keyed path when the caller knows the repository; scanning + # is the fallback for callers that only carry the issue number. + if remote and org and repo: + exact = lock_file_path( + remote=remote, + org=org, + repo=repo, + issue_number=target_issue, + lock_dir=lock_dir, + ) + if not _is_owned_durable_lock(read_lock_file(exact)): + raise FileNotFoundError( + f"durable issue lock '{exact}' is absent or is not owned by " + f"session '{owner}' (fail closed; nothing released)" + ) + os.remove(exact) + return exact + + matches: list[str] = [] + for path in iter_lock_files(lock_dir): + if _is_owned_durable_lock(read_lock_file(path)): + matches.append(path) + + if not matches: + raise FileNotFoundError( + f"no durable issue lock for issue #{target_issue} is owned by " + f"session '{owner}' (fail closed; nothing released)" + ) + if len(matches) > 1: + raise RuntimeError( + f"{len(matches)} durable locks for issue #{target_issue} claim " + f"session '{owner}'; refusing to guess which to release " + "(fail closed)" + ) + + path = matches[0] + os.remove(path) + return path + + def find_lock_for_branch( *, remote: str,