fix(author): unify the bootstrap and lock_issue issue-lock contract (Closes #953) #954
@@ -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,
|
||||
}
|
||||
|
||||
|
||||
+183
-2
@@ -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."
|
||||
)
|
||||
|
||||
@@ -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 |
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user