feat(forge): report unreadable repositories as unreadable (STATE-WP-0084-T01)
A private repository failed derivation the same way a broken one did, so "cannot read" and "does not exist" were indistinguishable from outside. They authorise opposite things: only the second can justify retiring a record. - ForgeUnreadableError (a ForgeDeriveError, so old callers still catch it) for permission-shaped clone failures, including Forgejo's 404 for an unauthenticated private repo — indistinguishable here, and the safe reading of an ambiguous answer cannot destroy a record. - GIT_TERMINAL_PROMPT=0: an unattended pass must fail, not block on a username prompt. Failing is what makes the case observable. - DerivedProjection.retirement_eligible separates "no records found" from "no records exist". A checkout with no workplans/ directory cannot evidence an absence — the empty-clone path that would have proposed every record in a repository for retirement. - Retirement from an ineligible source is refused even when acknowledged. - Fleet keeps unreadable out of the error bucket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Assistant: claude-code Assistant-Model: opus Assistant-Process: 3377672@bnt-lap001 Assistant-Session: 15463ccf-238f-4e13-b163-93aa25c6d166
This commit is contained in:
parent
8b38f815e5
commit
85181cd3e4
3 changed files with 294 additions and 6 deletions
|
|
@ -12,6 +12,7 @@ what makes the reset in `T03` verifiable: you can always ask what the projection
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import re
|
||||
import subprocess
|
||||
import tempfile
|
||||
|
|
@ -38,6 +39,50 @@ class ForgeDeriveError(RuntimeError):
|
|||
"""The repository could not be read from the forge."""
|
||||
|
||||
|
||||
class ForgeUnreadableError(ForgeDeriveError):
|
||||
"""Central is not permitted to read this repository — a policy, not a fault.
|
||||
|
||||
`ADR-012`'s premise (the forge is the projection source) holds only for
|
||||
repositories central can read, and until `STATE-WP-0084` nothing said so:
|
||||
a private repository failed the same way a broken one did, so "cannot read"
|
||||
and "does not exist" were indistinguishable from the outside.
|
||||
|
||||
They must never be confused, because they authorise opposite things. A
|
||||
repository that does not derive may have had its files removed deliberately;
|
||||
a repository we cannot read tells us nothing at all about its files. Only
|
||||
the first can justify retiring a record.
|
||||
|
||||
Forgejo answers an unauthenticated request for a private repository with a
|
||||
404, so "not found" is classified as unreadable too. That is deliberate: the
|
||||
two cases are genuinely indistinguishable at this layer, and the safe
|
||||
reading of an ambiguous answer is the one that cannot destroy a record.
|
||||
"""
|
||||
|
||||
|
||||
# git says this in several ways depending on version, transport, and whether a
|
||||
# credential helper is installed; all of them mean the same thing here.
|
||||
_UNREADABLE_MARKERS = (
|
||||
"could not read username",
|
||||
"could not read password",
|
||||
"authentication failed",
|
||||
"terminal prompts disabled",
|
||||
"invalid username or password",
|
||||
"403 forbidden",
|
||||
"the requested url returned error: 403",
|
||||
"the requested url returned error: 401",
|
||||
"repository not found",
|
||||
"remote: not found",
|
||||
"does not appear to be a git repository",
|
||||
)
|
||||
|
||||
|
||||
def _is_unreadable(message: str) -> bool:
|
||||
low = message.lower()
|
||||
if "not found" in low and "fatal: repository" in low:
|
||||
return True
|
||||
return any(marker in low for marker in _UNREADABLE_MARKERS)
|
||||
|
||||
|
||||
@dataclass
|
||||
class DerivedTask:
|
||||
record_id: str
|
||||
|
|
@ -63,11 +108,28 @@ class DerivedProjection:
|
|||
repo_slug: str
|
||||
commit: str
|
||||
workplans: list[DerivedWorkplan] = field(default_factory=list)
|
||||
# False when the checkout has no `workplans/` directory at all. An empty
|
||||
# projection then means "we did not find the records", which is not the same
|
||||
# claim as "this repository has no records" — and only the second one could
|
||||
# ever justify retiring anything.
|
||||
records_source_present: bool = True
|
||||
|
||||
@property
|
||||
def task_count(self) -> int:
|
||||
return sum(len(w.tasks) for w in self.workplans)
|
||||
|
||||
@property
|
||||
def retirement_eligible(self) -> bool:
|
||||
"""Whether an absence in this projection is evidence of an absence.
|
||||
|
||||
A projection that could not be read never reaches this: it raises. What
|
||||
this rules out is the quieter case — a clone that succeeded and returned
|
||||
nothing, which is what would have retired every record in a repository
|
||||
had a clone ever come back empty instead of failing (`STATE-WP-0084-T01`;
|
||||
the near-miss was `vergabe-teilnahme`).
|
||||
"""
|
||||
return self.records_source_present
|
||||
|
||||
def to_dict(self) -> dict[str, Any]:
|
||||
return {
|
||||
"schema": "state-hub.forge-projection.v1",
|
||||
|
|
@ -75,6 +137,8 @@ class DerivedProjection:
|
|||
# Provenance is not optional: a projection that cannot name the
|
||||
# commit it came from cannot be audited (ADR-012 decision 2).
|
||||
"commit": self.commit,
|
||||
"records_source_present": self.records_source_present,
|
||||
"retirement_eligible": self.retirement_eligible,
|
||||
"workplans": [
|
||||
{
|
||||
"record_id": w.record_id,
|
||||
|
|
@ -100,8 +164,12 @@ class DerivedProjection:
|
|||
|
||||
|
||||
def _run_git(*args: str, cwd: str | None = None, timeout: float = 120.0) -> str:
|
||||
# Without this a clone of a private repository blocks on a username prompt
|
||||
# instead of failing, and an unattended derivation pass hangs rather than
|
||||
# reporting. Failing fast is what makes the unreadable case observable.
|
||||
env = {**os.environ, "GIT_TERMINAL_PROMPT": "0", "GIT_ASKPASS": "", "GCM_INTERACTIVE": "never"}
|
||||
proc = subprocess.run(
|
||||
["git", *args], cwd=cwd, capture_output=True, text=True, timeout=timeout
|
||||
["git", *args], cwd=cwd, capture_output=True, text=True, timeout=timeout, env=env
|
||||
)
|
||||
if proc.returncode != 0:
|
||||
raise ForgeDeriveError((proc.stderr or proc.stdout).strip()[:400])
|
||||
|
|
@ -161,6 +229,7 @@ def derive_from_checkout(repo_root: Path, repo_slug: str, commit: str) -> Derive
|
|||
proj = DerivedProjection(repo_slug=repo_slug, commit=commit)
|
||||
wp_dir = repo_root / "workplans"
|
||||
if not wp_dir.is_dir():
|
||||
proj.records_source_present = False
|
||||
return proj
|
||||
for path in sorted(wp_dir.rglob("*.md")):
|
||||
if path.name.startswith("."):
|
||||
|
|
@ -208,6 +277,14 @@ def derive_from_forge(
|
|||
_run_git(*args, url, tmp)
|
||||
except subprocess.TimeoutExpired as exc:
|
||||
raise ForgeDeriveError(f"clone timed out for {repo_slug}") from exc
|
||||
except ForgeDeriveError as exc:
|
||||
# Classify before propagating. A caller that cannot tell "not
|
||||
# permitted" from "broken" will eventually treat one as the other.
|
||||
if _is_unreadable(str(exc)):
|
||||
raise ForgeUnreadableError(
|
||||
f"{repo_slug} could not be read from the forge: {exc}"
|
||||
) from exc
|
||||
raise
|
||||
commit = _run_git("rev-parse", "HEAD", cwd=tmp)
|
||||
return derive_from_checkout(Path(tmp), repo_slug, commit)
|
||||
|
||||
|
|
@ -241,6 +318,9 @@ class ProjectionDiff:
|
|||
missing: list[dict[str, Any]] = field(default_factory=list) # forge has, hub lacks
|
||||
stale: list[dict[str, Any]] = field(default_factory=list) # hub has, forge lacks
|
||||
differing: list[dict[str, Any]] = field(default_factory=list) # both, fields differ
|
||||
# Set when the source could not support a claim of absence, so `stale` was
|
||||
# deliberately left empty rather than computed (`STATE-WP-0084-T01`).
|
||||
stale_withheld: str | None = None
|
||||
|
||||
@property
|
||||
def clean(self) -> bool:
|
||||
|
|
@ -261,6 +341,7 @@ class ProjectionDiff:
|
|||
"stale": len(self.stale),
|
||||
"differing": len(self.differing),
|
||||
},
|
||||
"stale_withheld": self.stale_withheld,
|
||||
"missing": self.missing,
|
||||
"stale": self.stale,
|
||||
"differing": self.differing,
|
||||
|
|
@ -278,6 +359,14 @@ def diff_against_hub(
|
|||
is testable without a database and cannot accidentally mutate anything.
|
||||
"""
|
||||
d = ProjectionDiff(repo_slug=derived.repo_slug, commit=derived.commit)
|
||||
if not derived.retirement_eligible:
|
||||
# Compute what is missing and what differs as usual — those only ever
|
||||
# add or correct. Absence is the one conclusion this source cannot
|
||||
# support, so it is not drawn at all rather than drawn and then filtered.
|
||||
d.stale_withheld = (
|
||||
"the checkout has no workplans/ directory, so an absent record is "
|
||||
"unexplained rather than evidence of removal"
|
||||
)
|
||||
|
||||
# Match on canonical identity, never on UUID. Most hub records still carry
|
||||
# pre-ADR-007 random identifiers, so a UUID-keyed comparison reports every
|
||||
|
|
@ -314,6 +403,8 @@ def diff_against_hub(
|
|||
d.missing.append({"kind": "workplan", "record_id": w.record_id, "uuid": w.uuid})
|
||||
for key, w in have_wp.items():
|
||||
if key not in want_wp:
|
||||
if d.stale_withheld:
|
||||
continue
|
||||
uid = str(w["id"])
|
||||
d.stale.append(
|
||||
{
|
||||
|
|
@ -371,6 +462,8 @@ def diff_against_hub(
|
|||
)
|
||||
for key, t in have.items():
|
||||
if key not in want:
|
||||
if d.stale_withheld:
|
||||
continue
|
||||
uid = str(t["id"])
|
||||
d.stale.append(
|
||||
{"kind": "task", "uuid": uid, "title": t.get("title"),
|
||||
|
|
@ -397,7 +490,7 @@ def diff_against_hub(
|
|||
class ResetOutcome:
|
||||
repo_slug: str
|
||||
commit: str
|
||||
status: str # applied | refused | noop
|
||||
status: str # applied | refused | noop | unreadable
|
||||
created: list[str] = field(default_factory=list)
|
||||
updated: list[str] = field(default_factory=list)
|
||||
retired: list[str] = field(default_factory=list)
|
||||
|
|
@ -460,7 +553,23 @@ async def reset_repository_projection(
|
|||
from api.models.task import Task
|
||||
from api.models.workplan import Workplan
|
||||
|
||||
derived = derived or derive_from_forge(repo_slug, forge_base=forge_base)
|
||||
if derived is None:
|
||||
try:
|
||||
derived = derive_from_forge(repo_slug, forge_base=forge_base)
|
||||
except ForgeUnreadableError as exc:
|
||||
# Not an error: a statement about what central is permitted to see.
|
||||
# Reported as its own status so a caller cannot mistake it for a
|
||||
# repository whose records stopped deriving (`STATE-WP-0084-T01`).
|
||||
out = ResetOutcome(repo_slug=repo_slug, commit="", status="unreadable")
|
||||
out.refused.append(
|
||||
{"reason": "repository could not be read from the forge", "slug": repo_slug,
|
||||
"detail": str(exc)[:300]}
|
||||
)
|
||||
out.notes.append(
|
||||
"Nothing was changed and nothing was retired. This says nothing "
|
||||
"about whether the repository's records still exist."
|
||||
)
|
||||
return out
|
||||
outcome = ResetOutcome(repo_slug=repo_slug, commit=derived.commit, status="noop")
|
||||
|
||||
repo = (
|
||||
|
|
@ -571,6 +680,27 @@ async def reset_repository_projection(
|
|||
r for k, r in matched.items()
|
||||
if k not in want and r.projection_retired_at is None
|
||||
]
|
||||
if stale and not derived.retirement_eligible:
|
||||
# Acknowledgement cannot authorise this. The caller is confirming that
|
||||
# records which stopped deriving should be retired; here nothing has
|
||||
# been shown to have stopped deriving, because the source produced no
|
||||
# records to compare against. Consenting to a conclusion is not the
|
||||
# same as the evidence for it existing.
|
||||
outcome.status = "refused"
|
||||
for r in stale:
|
||||
outcome.refused.append(
|
||||
{
|
||||
"reason": "source produced no records; absence is unexplained",
|
||||
"slug": r.slug,
|
||||
"status": r.status,
|
||||
"backing_relative_path": r.backing_relative_path,
|
||||
}
|
||||
)
|
||||
outcome.notes.append(
|
||||
"The checkout has no workplans/ directory. Retirement withheld "
|
||||
"regardless of acknowledgement (STATE-WP-0084-T01)."
|
||||
)
|
||||
return outcome
|
||||
if stale and not acknowledge_retirements:
|
||||
outcome.status = "refused"
|
||||
for r in stale:
|
||||
|
|
@ -665,6 +795,11 @@ async def reset_repository_projection(
|
|||
class FleetResetOutcome:
|
||||
results: dict[str, dict[str, Any]] = field(default_factory=dict)
|
||||
errors: dict[str, str] = field(default_factory=dict)
|
||||
# Kept apart from `errors` on purpose. Nine repositories sitting in an error
|
||||
# bucket read as nine broken repositories; they were nine we were not
|
||||
# allowed to read, which is a different thing to go and fix
|
||||
# (`STATE-WP-0083-T04`, 2026-08-26).
|
||||
unreadable: dict[str, str] = field(default_factory=dict)
|
||||
|
||||
def to_dict(self) -> dict[str, Any]:
|
||||
by_status: dict[str, int] = {}
|
||||
|
|
@ -672,9 +807,11 @@ class FleetResetOutcome:
|
|||
by_status[r["status"]] = by_status.get(r["status"], 0) + 1
|
||||
return {
|
||||
"schema": "state-hub.fleet-projection-reset.v1",
|
||||
"repositories": len(self.results) + len(self.errors),
|
||||
"repositories": len(self.results) + len(self.errors) + len(self.unreadable),
|
||||
"by_status": by_status,
|
||||
"errored": len(self.errors),
|
||||
"unreadable_count": len(self.unreadable),
|
||||
"unreadable": self.unreadable,
|
||||
"totals": {
|
||||
k: sum(r["counts"][k] for r in self.results.values())
|
||||
for k in ("created", "updated", "retired", "refused")
|
||||
|
|
@ -719,7 +856,16 @@ async def reset_fleet_projection(
|
|||
await session.commit()
|
||||
else:
|
||||
await session.rollback()
|
||||
outcome.results[slug] = result.to_dict()
|
||||
if result.status == "unreadable":
|
||||
detail = next(
|
||||
(r.get("detail", "") for r in result.refused), ""
|
||||
)
|
||||
outcome.unreadable[slug] = detail or "could not be read from the forge"
|
||||
else:
|
||||
outcome.results[slug] = result.to_dict()
|
||||
except ForgeUnreadableError as exc:
|
||||
# Reachable when the caller supplied its own derivation path.
|
||||
outcome.unreadable[slug] = str(exc)[:300]
|
||||
except Exception as exc: # noqa: BLE001 - one repo must not end the pass
|
||||
outcome.errors[slug] = f"{type(exc).__name__}: {exc}"[:300]
|
||||
return outcome
|
||||
|
|
|
|||
|
|
@ -417,3 +417,118 @@ class TestSlugCollisionRefusal:
|
|||
assert out.status == "refused"
|
||||
assert out.refused[0]["reason"].startswith("slug already belongs")
|
||||
assert session.added == []
|
||||
|
||||
|
||||
class TestUnreadableIsNotMissing:
|
||||
"""STATE-WP-0084-T01.
|
||||
|
||||
A repository central is not permitted to read and a repository whose
|
||||
records no longer derive authorise opposite things. Every test here exists
|
||||
to keep the retirement path unreachable from an answer that cannot support
|
||||
it — by construction, not by the reset happening to fail first.
|
||||
"""
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"stderr",
|
||||
[
|
||||
"fatal: could not read Username for 'https://forgejo.coulomb.social'",
|
||||
"remote: Invalid username or password.\nfatal: Authentication failed",
|
||||
"fatal: could not read Username for 'https://f': terminal prompts disabled",
|
||||
"fatal: repository 'https://forgejo.coulomb.social/rapp-openbao.git' not found",
|
||||
"fatal: unable to access '...': The requested URL returned error: 403",
|
||||
],
|
||||
)
|
||||
def test_permission_shaped_failures_are_classified_unreadable(self, stderr, monkeypatch):
|
||||
def boom(*a, **k):
|
||||
raise fp.ForgeDeriveError(stderr)
|
||||
|
||||
monkeypatch.setattr(fp, "_run_git", boom)
|
||||
with pytest.raises(fp.ForgeUnreadableError):
|
||||
fp.derive_from_forge("rapp-openbao")
|
||||
|
||||
def test_a_genuine_fault_stays_a_plain_error(self, monkeypatch):
|
||||
def boom(*a, **k):
|
||||
raise fp.ForgeDeriveError("fatal: early EOF\nfatal: index-pack failed")
|
||||
|
||||
monkeypatch.setattr(fp, "_run_git", boom)
|
||||
with pytest.raises(fp.ForgeDeriveError) as exc:
|
||||
fp.derive_from_forge("demo")
|
||||
assert not isinstance(exc.value, fp.ForgeUnreadableError)
|
||||
|
||||
def test_unreadable_is_a_derive_error_so_old_callers_still_catch_it(self):
|
||||
assert issubclass(fp.ForgeUnreadableError, fp.ForgeDeriveError)
|
||||
|
||||
def test_a_checkout_without_workplans_cannot_evidence_absence(self, tmp_path):
|
||||
(tmp_path / "bare").mkdir()
|
||||
p = fp.derive_from_checkout(tmp_path / "bare", "bare", "abc")
|
||||
assert p.workplans == []
|
||||
assert p.records_source_present is False
|
||||
assert p.retirement_eligible is False
|
||||
|
||||
def test_a_real_checkout_can(self, tmp_path):
|
||||
p = fp.derive_from_checkout(_repo(tmp_path), "demo", "abc")
|
||||
assert p.records_source_present is True
|
||||
assert p.retirement_eligible is True
|
||||
|
||||
def test_the_diff_withholds_stale_rather_than_computing_it(self):
|
||||
derived = fp.DerivedProjection(
|
||||
repo_slug="demo", commit="c0ffee", records_source_present=False
|
||||
)
|
||||
hub = [{"id": "11111111-1111-1111-1111-111111111111", "slug": "demo-wp-0001",
|
||||
"status": "active", "backing_relative_path": "workplans/a.md"}]
|
||||
d = fp.diff_against_hub(derived, hub, {})
|
||||
assert d.stale == []
|
||||
assert d.would_remove == 0
|
||||
assert d.stale_withheld
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_empty_source_cannot_retire_even_when_acknowledged(self):
|
||||
derived = fp.DerivedProjection(
|
||||
repo_slug="demo", commit="c0ffee", records_source_present=False
|
||||
)
|
||||
row = _Row(slug="demo-wp-0001", status="active", path="workplans/a.md")
|
||||
session = _FakeSession(repo=_Repo(), rows=[row])
|
||||
out = await fp.reset_repository_projection(
|
||||
session, "demo", derived=derived, acknowledge_retirements=True
|
||||
)
|
||||
assert out.status == "refused"
|
||||
assert out.retired == []
|
||||
assert row.projection_retired_at is None
|
||||
assert out.refused[0]["reason"].startswith("source produced no records")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_unreadable_repository_reports_unreadable_not_error(self, monkeypatch):
|
||||
def boom(*a, **k):
|
||||
raise fp.ForgeUnreadableError("rapp-openbao could not be read from the forge")
|
||||
|
||||
monkeypatch.setattr(fp, "derive_from_forge", boom)
|
||||
session = _FakeSession(repo=_Repo(), rows=[])
|
||||
out = await fp.reset_repository_projection(session, "rapp-openbao")
|
||||
assert out.status == "unreadable"
|
||||
assert out.retired == [] and out.created == [] and out.updated == []
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_fleet_keeps_unreadable_out_of_the_error_bucket(self, monkeypatch):
|
||||
async def fake(session, slug, **kw):
|
||||
if slug == "private":
|
||||
out = fp.ResetOutcome(repo_slug=slug, commit="", status="unreadable")
|
||||
out.refused.append({"reason": "repository could not be read from the forge",
|
||||
"slug": slug, "detail": "could not read Username"})
|
||||
return out
|
||||
if slug == "broken":
|
||||
raise RuntimeError("index-pack failed")
|
||||
out = fp.ResetOutcome(repo_slug=slug, commit="c0ffee", status="applied")
|
||||
out.updated.append("A-WP-0001")
|
||||
return out
|
||||
|
||||
monkeypatch.setattr(fp, "reset_repository_projection", fake)
|
||||
sessions = [_FakeSession(repo=_Repo(), rows=[]) for _ in range(3)]
|
||||
outcome = await fp.reset_fleet_projection(
|
||||
TestFleetReset._Factory(sessions), ["ok", "private", "broken"]
|
||||
)
|
||||
assert list(outcome.unreadable) == ["private"]
|
||||
assert list(outcome.errors) == ["broken"]
|
||||
assert "private" not in outcome.results
|
||||
d = outcome.to_dict()
|
||||
assert d["unreadable_count"] == 1 and d["errored"] == 1
|
||||
assert d["repositories"] == 3
|
||||
|
|
|
|||
|
|
@ -54,7 +54,7 @@ wait on `MASON-WP-0003-T02`.
|
|||
|
||||
```task
|
||||
id: STATE-WP-0084-T01
|
||||
status: todo
|
||||
status: done
|
||||
priority: high
|
||||
state_hub_task_id: "e955067d-2152-59f7-b672-12755ceb26b0"
|
||||
```
|
||||
|
|
@ -81,6 +81,33 @@ Acceptance: an unreadable repository is reported as unreadable; a clone that
|
|||
succeeds but returns nothing produces no retirement proposals; both cases are
|
||||
covered by tests that fail if the retirement path is reachable from either.
|
||||
|
||||
**Done 2026-08-26.** `api/services/forge_projection.py`:
|
||||
|
||||
- `ForgeUnreadableError` subclasses `ForgeDeriveError`, so existing callers
|
||||
keep catching it while new ones can tell the cases apart. A permission-shaped
|
||||
git failure is classified into it; a genuine fault stays a plain error.
|
||||
Forgejo's 404 for an unauthenticated private repository classifies as
|
||||
unreadable, because the two are indistinguishable at this layer and the safe
|
||||
reading of an ambiguous answer is the one that cannot destroy a record.
|
||||
- `_run_git` runs with `GIT_TERMINAL_PROMPT=0`. Without it an unattended pass
|
||||
blocks on a username prompt instead of failing, and an unreadable repository
|
||||
is only observable if it fails.
|
||||
- `DerivedProjection.records_source_present` / `.retirement_eligible` separate
|
||||
"no records found" from "no records exist". A checkout with no `workplans/`
|
||||
directory can no longer evidence an absence.
|
||||
- `diff_against_hub` withholds `stale` rather than computing and filtering it,
|
||||
and says so in `stale_withheld`. `would_remove` is 0 for such a source.
|
||||
- `reset_repository_projection` returns `status="unreadable"` for a repository
|
||||
it may not read, and refuses retirement from an ineligible source **even when
|
||||
`acknowledge_retirements=True`** — consenting to a conclusion is not the same
|
||||
as the evidence for it existing.
|
||||
- `reset_fleet_projection` keeps unreadable repositories out of `errors`, in
|
||||
their own bucket with its own count. Nine repositories in an error bucket
|
||||
read as nine broken repositories; they were nine we were not allowed to read.
|
||||
|
||||
13 tests in `TestUnreadableIsNotMissing` (`tests/test_forge_projection.py`);
|
||||
58 pass across the forge/projection/backfill suites.
|
||||
|
||||
## Deliver the credential to the pod
|
||||
|
||||
```task
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue