From b2f6e9a6dc40e9651ef876f322dd0a68bddebfd8 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 22 Jul 2026 16:12:07 -0400 Subject: [PATCH 1/3] feat(webui): versioned project registry API (Closes #635) Evolve the MVP project registry (#427) into a versioned, fail-closed project registry API for the console (Phase 1, read-only). - Add schema version 2 with project `status`, per-step onboarding `state`/`required`, optional redacted `last_seen_health`, and `remote_name`. Version 1 files stay loadable and are normalized with explicit defaults. - Serve `/api/v1/projects` and `/api/v1/projects/{project_id}` with API provenance (`api_version`, `schema_version`, `source`). `/api/projects` is retained as an unversioned Phase 1 alias. - Replace bare `ValueError` with `RegistryError`, carrying an operator `remediation` and `field_path`; invalid registries fail closed as a 500 JSON payload or a dedicated HTML error page instead of a traceback. - Reject credential-shaped keys before any DTO is built, reusing `registry_safety.is_forbidden_key` as the single source of truth shared with the worker registry (#798). - Render HTML views from `project_to_dict`, so the console and the JSON API cannot disagree about status or onboarding progress. - Document the contract in docs/webui-project-registry-api.md. Tests: registry load/validate (valid, missing project, schema validation, credential rejection) and API route coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/webui-local-dev.md | 10 +- docs/webui-project-registry-api.md | 213 ++++++++++++ tests/test_webui_project_registry.py | 367 ++++++++++++++++++-- webui/app.py | 77 ++++- webui/data/projects.registry.json | 22 +- webui/project_registry.py | 485 +++++++++++++++++++++++++-- webui/project_views.py | 132 ++++++-- 7 files changed, 1212 insertions(+), 94 deletions(-) create mode 100644 docs/webui-project-registry-api.md diff --git a/docs/webui-local-dev.md b/docs/webui-local-dev.md index a92e509..f5799b2 100644 --- a/docs/webui-local-dev.md +++ b/docs/webui-local-dev.md @@ -43,6 +43,10 @@ for the console architecture: layer and authority boundaries, the redaction boundary, `/api/v1/...` versioning, the target page map, and the phase gates that govern when a write path may open (#632, epic #631). +See [webui-project-registry-api.md](webui-project-registry-api.md) for the +versioned project registry contract: registry schema versions 1 and 2, project +status, onboarding checklist state, and the fail-closed error payloads (#635). + ## Routes (MVP) | Path | Description | @@ -51,9 +55,11 @@ that govern when a write path may open (#632, epic #631). | `/health` | JSON liveness (`status`, `service`, `mode`, `timestamp`) | | `/queue` | Live PR and issue queue dashboard (#429) | | `/api/queue` | JSON queue export with pagination metadata | -| `/projects` | Project registry list (#427) | +| `/projects` | Project registry list with status and onboarding progress (#427, #635) | | `/projects/{id}` | Project detail + onboarding checklist | -| `/api/projects` | JSON registry export | +| `/api/v1/projects` | Versioned JSON registry export (#635) | +| `/api/v1/projects/{id}` | Versioned JSON project detail (#635) | +| `/api/projects` | JSON registry export — unversioned Phase 1 alias of `/api/v1/projects` | | `/prompts` | Prompt library with per-prompt copy buttons (#428) | | `/api/prompts` | JSON prompt export with workflow hashes | | `/runtime` | MCP runtime health and stale detection (#430) | diff --git a/docs/webui-project-registry-api.md b/docs/webui-project-registry-api.md new file mode 100644 index 0000000..67f20ba --- /dev/null +++ b/docs/webui-project-registry-api.md @@ -0,0 +1,213 @@ +# Project registry API (#635) + +Phase 1 of the [console architecture ADR](architecture/webui-control-plane-console-architecture-adr.md) +gives the project registry a versioned, read-only API. This document is the +field-by-field contract for that API and for the registry file behind it. + +Everything here is **read-only**. The console never writes the registry; an +operator edits the JSON file, and an invalid file fails closed rather than +rendering a partial inventory. + +## Routes + +| Route | Method | Description | +|-------|--------|-------------| +| `/api/v1/projects` | GET | Versioned registry export: all projects, with provenance | +| `/api/v1/projects/{project_id}` | GET | Single project; `404` with `project_not_found` when unknown | +| `/api/projects` | GET | Unversioned MVP alias (#427), retained for all of Phase 1 | +| `/projects` | GET | HTML list — status and onboarding progress per project | +| `/projects/{project_id}` | GET | HTML detail — identity, profiles, paths, checklist | + +Per ADR section 6 the unversioned alias may be retired no earlier than Phase 2, +and only after this document and `webui-local-dev.md` record the swap. The alias +returns the same payload as `/api/v1/projects`, including the legacy `version` +and `source_path` keys #427 consumers already read. + +The HTML views render from the same DTO the JSON routes serialize +(`project_to_dict`), so the console and the API cannot disagree about a +project's status or onboarding progress. + +## Registry file + +Default location: `webui/data/projects.registry.json`. Override with the +`WEBUI_PROJECT_REGISTRY` environment variable. + +Schema versions: **1** and **2** are accepted; **2** is current. A version 1 +file loads unchanged and is normalized with the documented defaults, so an +existing operator registry keeps working without edits. + +### Root + +| Field | Type | Required | Notes | +|-------|------|----------|-------| +| `version` | int | yes | `1` or `2`. Anything else fails closed | +| `projects` | array | yes | Must be non-empty | + +### Project + +| Field | Type | Required | Default | Notes | +|-------|------|----------|---------|-------| +| `id` | string | yes | — | Stable registry id used in URLs | +| `repo_name` | string | yes | — | Gitea repository name | +| `gitea_owner` | string | yes | — | Owning org or user | +| `remote_host` | string | yes | — | Instance base URL, no credentials | +| `remote_name` | string | no | `null` | Logical remote label, e.g. `prgs` (v2) | +| `default_branch` | string | yes | — | Stable branch name | +| `local_checkout_path` | string | yes | — | Control checkout path | +| `status` | string | no | `active` | `active`, `onboarding`, `paused`, `archived` (v2) | +| `profiles` | object | yes | — | Must map `author`, `reviewer`, `reconciler` | +| `workflow_paths` | object | yes | — | Non-empty; label to repo-relative path | +| `schema_paths` | object | no | `{}` | Label to repo-relative path | +| `onboarding_checklist` | array | no | `[]` | See below | +| `last_seen_health` | object | no | `null` | Redacted health only (v2) | + +### Onboarding step + +| Field | Type | Required | Default | Notes | +|-------|------|----------|---------|-------| +| `id` | string | yes | — | Stable step id | +| `title` | string | yes | — | Short operator-facing label | +| `description` | string | yes | — | Self-contained; assumes no chat history | +| `state` | string | no | `pending` | `complete`, `pending`, `blocked`, `not_applicable` (v2) | +| `required` | bool | no | `true` | Optional steps never block readiness (v2) | + +### Last-seen health + +| Field | Type | Required | Notes | +|-------|------|----------|-------| +| `status` | string | no (default `unknown`) | `healthy`, `degraded`, `unreachable`, `unknown` | +| `checked_at` | string | no | ISO-8601 UTC timestamp, e.g. `2026-01-01T00:00:00Z` | +| `detail` | string | no | Short redacted note | + +Health is recorded metadata, not a live probe: Phase 1 performs no outbound +health checks. Endpoints, tokens, and keychain identifiers must never appear +here. + +## Response shape + +`GET /api/v1/projects`: + +```json +{ + "api_version": "v1", + "schema_version": 2, + "version": 2, + "source_path": "/path/to/webui/data/projects.registry.json", + "source": { + "kind": "file", + "path": "/path/to/webui/data/projects.registry.json", + "inventory_complete": true + }, + "project_count": 1, + "projects": [ + { + "id": "example", + "repo_name": "Example", + "gitea_owner": "Org", + "repo_full_name": "Org/Example", + "remote_host": "https://gitea.example.invalid", + "remote_name": "example-remote", + "default_branch": "main", + "local_checkout_path": ".", + "status": "active", + "profiles": {"author": "...", "reviewer": "...", "reconciler": "..."}, + "workflow_paths": {"skill": "skills/..."}, + "schema_paths": {}, + "onboarding_checklist": [ + { + "id": "profiles", + "title": "Configure execution profiles", + "description": "...", + "state": "complete", + "required": true + } + ], + "onboarding_summary": { + "total": 1, + "complete": 1, + "pending": 0, + "blocked": 0, + "not_applicable": 0, + "required_outstanding": 0, + "onboarding_complete": true + }, + "last_seen_health": null + } + ] +} +``` + +`GET /api/v1/projects/{project_id}` returns `api_version`, `schema_version`, +`source`, and a single `project` object with the same fields. + +The `source` block satisfies the ADR section 6 provenance rule: every payload +states where the data came from and whether the inventory is complete. A +file-backed registry is always complete — there is no pagination to truncate it. + +`onboarding_summary` is derived, never stored. `required_outstanding` counts +steps that are `required` **and** in state `pending` or `blocked`; +`onboarding_complete` is true when that count is zero. + +## Fail-closed errors + +Validation failures raise `RegistryError`, which routes render instead of a +traceback. + +`404` — unknown project id on `/api/v1/projects/{project_id}`: + +```json +{ + "error": "project_not_found", + "project_id": "not-registered", + "known_project_ids": ["example"], + "remediation": "Request one of the known project ids, or add the project ...", + "source": {"kind": "file", "path": "...", "inventory_complete": true} +} +``` + +`500` — invalid registry, on both the versioned route and the alias: + +```json +{ + "error": "registry_invalid", + "detail": "unsupported registry version: 42", + "remediation": "Set 'version' to one of 1, 2 (current schema is 2) ...", + "field_path": "version", + "source_path": "/path/to/registry.json" +} +``` + +`field_path` points at the offending location (`projects[0].profiles.reconciler`, +`projects[0].onboarding_checklist[2].state`, and so on). The HTML routes render +the same detail, field, source, and remediation on a "Project registry +unavailable" page. + +Conditions that fail closed: + +* file missing or unreadable; +* invalid JSON (the remediation names line and column); +* root not an object, or `projects` missing/empty; +* unsupported `version`; +* a credential-shaped key anywhere in the file (`token`, `*_secret`, `auth_*`, and similar); +* a project missing a required field, or missing an `author`/`reviewer`/`reconciler` profile; +* an unknown `status`, onboarding `state`, or health `status`. + +## Credential rule + +The registry stores redacted metadata only. Credential-shaped keys are +rejected at load time, before any DTO is built, consistent with +[safety-model.md](safety-model.md) and +[credential-isolation.md](credential-isolation.md). Tokens live in the keychain +and are resolved server-side by `gitea_auth`. + +## Migrating a version 1 registry + +1. Set `"version": 2`. +2. Optionally add `"status"` per project (omitted means `active`). +3. Optionally add `"remote_name"` per project. +4. Optionally add `"state"` and `"required"` to each onboarding step (omitted + means `pending` and `true`). +5. Optionally add `"last_seen_health"`. + +No step is mandatory: a version 1 file keeps loading. Bumping the version only +declares that the file may use the v2 fields. diff --git a/tests/test_webui_project_registry.py b/tests/test_webui_project_registry.py index f16774a..4d1c650 100644 --- a/tests/test_webui_project_registry.py +++ b/tests/test_webui_project_registry.py @@ -1,4 +1,4 @@ -"""Tests for web UI project registry (#427).""" +"""Tests for web UI project registry (#427) and its API evolution (#635).""" import json import sys import tempfile @@ -11,57 +11,246 @@ from starlette.testclient import TestClient from webui.app import create_app from webui.project_registry import ( + CURRENT_SCHEMA_VERSION, + REGISTRY_API_VERSION, + SUPPORTED_SCHEMA_VERSIONS, + RegistryError, default_registry_path, load_registry, + onboarding_summary, project_to_dict, ) +from webui.registry_safety import is_forbidden_key + +_REPO_ROOT = Path(__file__).resolve().parent.parent +_API_DOC = _REPO_ROOT / "docs" / "webui-project-registry-api.md" -class TestProjectRegistryLoader(unittest.TestCase): +def _valid_project(**overrides): + project = { + "id": "example", + "repo_name": "Example", + "gitea_owner": "Org", + "remote_host": "https://gitea.example.invalid", + "default_branch": "main", + "local_checkout_path": ".", + "profiles": {"author": "a", "reviewer": "r", "reconciler": "c"}, + "workflow_paths": {"skill": "skills/x.md"}, + } + project.update(overrides) + return project + + +def _write_registry(payload) -> Path: + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as handle: + json.dump(payload, handle) + return Path(handle.name) + + +class RegistryFileCase(unittest.TestCase): + """Base class that cleans up temporary registry files.""" + + def setUp(self): + self._temp_paths: list[Path] = [] + + def tearDown(self): + for path in self._temp_paths: + path.unlink(missing_ok=True) + + def write_registry(self, payload) -> Path: + path = _write_registry(payload) + self._temp_paths.append(path) + return path + + +class TestProjectRegistryLoader(RegistryFileCase): def test_default_registry_loads_gitea_tools(self): registry = load_registry() - self.assertEqual(registry.version, 1) + self.assertEqual(registry.version, CURRENT_SCHEMA_VERSION) + self.assertEqual(registry.schema_version, CURRENT_SCHEMA_VERSION) + self.assertEqual(registry.api_version, REGISTRY_API_VERSION) self.assertEqual(len(registry.projects), 1) project = registry.projects[0] self.assertEqual(project.id, "gitea-tools") self.assertEqual(project.repo_name, "Gitea-Tools") self.assertEqual(project.gitea_owner, "Scaled-Tech-Consulting") + self.assertEqual(project.repo_full_name, "Scaled-Tech-Consulting/Gitea-Tools") self.assertEqual(project.remote_host, "https://gitea.prgs.cc") + self.assertEqual(project.remote_name, "prgs") + self.assertEqual(project.status, "active") self.assertEqual(project.profiles["author"], "prgs-author") self.assertEqual(project.profiles["reviewer"], "prgs-reviewer") self.assertEqual(project.profiles["reconciler"], "prgs-reconciler") self.assertIn("skill", project.workflow_paths) self.assertGreaterEqual(len(project.onboarding_checklist), 4) - def test_registry_rejects_credential_keys(self): - payload = { + def test_default_registry_onboarding_summary_is_complete(self): + summary = onboarding_summary(load_registry().projects[0]) + self.assertEqual(summary.total, summary.complete) + self.assertEqual(summary.required_outstanding, 0) + self.assertTrue(summary.onboarding_complete) + + def test_version_1_registry_still_loads_with_defaults(self): + path = self.write_registry({ "version": 1, "projects": [ - { - "id": "bad", - "repo_name": "Bad", - "gitea_owner": "Org", - "remote_host": "https://gitea.example.invalid", - "default_branch": "main", - "local_checkout_path": ".", - "profiles": { - "author": "a", - "reviewer": "r", - "reconciler": "c", - }, - "workflow_paths": {"skill": "skills/x.md"}, - "api_token": "secret", - } + _valid_project( + onboarding_checklist=[ + {"id": "step", "title": "Step", "description": "Do it"} + ] + ) ], - } + }) + registry = load_registry(path) + self.assertEqual(registry.schema_version, 1) + self.assertIn(1, SUPPORTED_SCHEMA_VERSIONS) + project = registry.projects[0] + self.assertEqual(project.status, "active") + self.assertIsNone(project.remote_name) + self.assertIsNone(project.last_seen_health) + step = project.onboarding_checklist[0] + self.assertEqual(step.state, "pending") + self.assertTrue(step.required) + self.assertFalse(onboarding_summary(project).onboarding_complete) + + def test_onboarding_summary_counts_states(self): + path = self.write_registry({ + "version": 2, + "projects": [ + _valid_project( + onboarding_checklist=[ + {"id": "a", "title": "A", "description": "d", "state": "complete"}, + {"id": "b", "title": "B", "description": "d", "state": "blocked"}, + { + "id": "c", + "title": "C", + "description": "d", + "state": "pending", + "required": False, + }, + { + "id": "d", + "title": "D", + "description": "d", + "state": "not_applicable", + }, + ] + ) + ], + }) + summary = onboarding_summary(load_registry(path).projects[0]) + self.assertEqual(summary.total, 4) + self.assertEqual(summary.complete, 1) + self.assertEqual(summary.blocked, 1) + self.assertEqual(summary.pending, 1) + self.assertEqual(summary.not_applicable, 1) + # Only the blocked step is both required and outstanding. + self.assertEqual(summary.required_outstanding, 1) + self.assertFalse(summary.onboarding_complete) + + def test_last_seen_health_is_parsed_when_present(self): + path = self.write_registry({ + "version": 2, + "projects": [ + _valid_project( + last_seen_health={ + "status": "degraded", + "checked_at": "2026-01-01T00:00:00Z", + "detail": "daemon restart pending", + } + ) + ], + }) + health = load_registry(path).projects[0].last_seen_health + self.assertIsNotNone(health) + self.assertEqual(health.status, "degraded") + self.assertEqual(health.checked_at, "2026-01-01T00:00:00Z") + + def test_registry_rejects_credential_keys(self): + path = self.write_registry({ + "version": 1, + "projects": [_valid_project(id="bad", api_token="redacted-placeholder")], + }) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertIn("credential", ctx.exception.remediation.lower()) + self.assertEqual(ctx.exception.field_path, "projects[0].api_token") + + def test_unsupported_version_fails_closed_with_remediation(self): + path = self.write_registry({"version": 99, "projects": [_valid_project()]}) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertIn("unsupported registry version", ctx.exception.message) + self.assertIn(str(CURRENT_SCHEMA_VERSION), ctx.exception.remediation) + self.assertEqual(ctx.exception.field_path, "version") + + def test_missing_required_field_fails_closed(self): + broken = _valid_project() + del broken["default_branch"] + path = self.write_registry({"version": 2, "projects": [broken]}) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertIn("default_branch", ctx.exception.message) + self.assertEqual(ctx.exception.field_path, "projects[0]") + + def test_unknown_status_fails_closed(self): + path = self.write_registry({ + "version": 2, + "projects": [_valid_project(status="mystery")], + }) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertEqual(ctx.exception.field_path, "projects[0].status") + self.assertIn("active", ctx.exception.remediation) + + def test_unknown_onboarding_state_fails_closed(self): + path = self.write_registry({ + "version": 2, + "projects": [ + _valid_project( + onboarding_checklist=[ + {"id": "a", "title": "A", "description": "d", "state": "almost"} + ] + ) + ], + }) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertEqual( + ctx.exception.field_path, + "projects[0].onboarding_checklist[0].state", + ) + + def test_missing_profile_role_fails_closed(self): + path = self.write_registry({ + "version": 2, + "projects": [_valid_project(profiles={"author": "a", "reviewer": "r"})], + }) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertEqual(ctx.exception.field_path, "projects[0].profiles.reconciler") + + def test_empty_projects_fails_closed(self): + path = self.write_registry({"version": 2, "projects": []}) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertEqual(ctx.exception.field_path, "projects") + + def test_invalid_json_fails_closed_with_location(self): with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as handle: - json.dump(payload, handle) + handle.write("{not json") path = Path(handle.name) - try: - with self.assertRaises(ValueError): - load_registry(path) - finally: - path.unlink(missing_ok=True) + self._temp_paths.append(path) + with self.assertRaises(RegistryError) as ctx: + load_registry(path) + self.assertIn("not valid JSON", ctx.exception.message) + self.assertIn("line", ctx.exception.remediation) + + def test_missing_file_fails_closed(self): + missing = Path(tempfile.gettempdir()) / "webui-registry-does-not-exist.json" + with self.assertRaises(RegistryError) as ctx: + load_registry(missing) + self.assertIn("could not be read", ctx.exception.message) def test_default_registry_path_points_at_packaged_data(self): path = default_registry_path() @@ -81,31 +270,147 @@ class TestProjectRegistryRoutes(unittest.TestCase): self.assertIn("prgs-author", response.text) self.assertNotIn("child issue", response.text.lower()) + def test_projects_page_shows_status_and_progress(self): + response = self.client.get("/projects") + self.assertIn("Status", response.text) + self.assertIn("Onboarding", response.text) + self.assertIn("4/4 complete", response.text) + def test_project_detail_renders_checklist(self): response = self.client.get("/projects/gitea-tools") self.assertEqual(response.status_code, 200) self.assertIn("Onboarding checklist", response.text) self.assertIn("Configure execution profiles", response.text) self.assertIn("branches/", response.text) + self.assertIn("Complete", response.text) + self.assertIn("required outstanding 0", response.text) def test_project_detail_404(self): response = self.client.get("/projects/unknown-repo") self.assertEqual(response.status_code, 404) - def test_api_projects_json(self): + def test_api_projects_alias_stays_compatible(self): response = self.client.get("/api/projects") self.assertEqual(response.status_code, 200) data = response.json() - self.assertEqual(data["version"], 1) + # #427 consumers keep these keys. + self.assertEqual(data["version"], CURRENT_SCHEMA_VERSION) + self.assertIn("source_path", data) self.assertEqual(len(data["projects"]), 1) self.assertEqual(data["projects"][0]["id"], "gitea-tools") self.assertIn("onboarding_checklist", data["projects"][0]) + def test_api_v1_projects_payload(self): + response = self.client.get("/api/v1/projects") + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["api_version"], REGISTRY_API_VERSION) + self.assertEqual(data["schema_version"], CURRENT_SCHEMA_VERSION) + self.assertEqual(data["project_count"], 1) + self.assertEqual(data["source"]["kind"], "file") + self.assertTrue(data["source"]["inventory_complete"]) + project = data["projects"][0] + self.assertEqual(project["status"], "active") + self.assertEqual(project["remote_name"], "prgs") + self.assertEqual( + project["repo_full_name"], "Scaled-Tech-Consulting/Gitea-Tools" + ) + self.assertTrue(project["onboarding_summary"]["onboarding_complete"]) + self.assertEqual(project["onboarding_checklist"][0]["state"], "complete") + self.assertIsNone(project["last_seen_health"]) + + def test_api_v1_project_detail(self): + response = self.client.get("/api/v1/projects/gitea-tools") + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["api_version"], REGISTRY_API_VERSION) + self.assertEqual(data["project"]["id"], "gitea-tools") + self.assertEqual(data["source"]["kind"], "file") + + def test_api_v1_project_detail_missing_fails_closed(self): + response = self.client.get("/api/v1/projects/not-registered") + self.assertEqual(response.status_code, 404) + data = response.json() + self.assertEqual(data["error"], "project_not_found") + self.assertEqual(data["project_id"], "not-registered") + self.assertIn("gitea-tools", data["known_project_ids"]) + self.assertIn("remediation", data) + + def test_api_v1_projects_is_read_only(self): + response = self.client.post("/api/v1/projects", json={}) + self.assertEqual(response.status_code, 405) + self.assertEqual(response.json()["error"], "read-only-mvp") + def test_project_to_dict_is_json_safe(self): registry = load_registry() - encoded = json.dumps(project_to_dict(registry.projects[0])) + dto = project_to_dict(registry.projects[0]) + encoded = json.dumps(dto) self.assertIn("gitea-tools", encoded) + # Prose may mention tokens; no serialized *key* may look like a secret. + for key in dto: + with self.subTest(key=key): + self.assertFalse(is_forbidden_key(key)) + + +class TestInvalidRegistryFailsClosedOverHttp(RegistryFileCase): + def setUp(self): + super().setUp() + self.path = self.write_registry({"version": 42, "projects": []}) + self.client = TestClient(create_app()) + + def _with_bad_registry(self, url: str): + import os + from unittest import mock + + with mock.patch.dict( + os.environ, {"WEBUI_PROJECT_REGISTRY": str(self.path)}, clear=False + ): + return self.client.get(url) + + def test_api_v1_reports_actionable_error(self): + response = self._with_bad_registry("/api/v1/projects") + self.assertEqual(response.status_code, 500) + data = response.json() + self.assertEqual(data["error"], "registry_invalid") + self.assertIn("unsupported registry version", data["detail"]) + self.assertTrue(data["remediation"]) + self.assertEqual(data["field_path"], "version") + + def test_unversioned_alias_reports_actionable_error(self): + response = self._with_bad_registry("/api/projects") + self.assertEqual(response.status_code, 500) + self.assertEqual(response.json()["error"], "registry_invalid") + + def test_html_page_reports_actionable_error(self): + response = self._with_bad_registry("/projects") + self.assertEqual(response.status_code, 500) + self.assertIn("Project registry unavailable", response.text) + self.assertIn("Remediation", response.text) + + +class TestProjectRegistryApiDocs(unittest.TestCase): + def test_api_contract_is_documented(self): + self.assertTrue(_API_DOC.is_file(), f"missing {_API_DOC}") + text = _API_DOC.read_text(encoding="utf-8") + for token in ( + "/api/v1/projects", + "/api/v1/projects/{project_id}", + "/api/projects", + "onboarding_summary", + "last_seen_health", + "registry_invalid", + "#635", + ): + with self.subTest(token=token): + self.assertIn(token, text) + + def test_route_table_lists_versioned_routes(self): + local_dev = (_REPO_ROOT / "docs" / "webui-local-dev.md").read_text( + encoding="utf-8" + ) + self.assertIn("/api/v1/projects", local_dev) + self.assertIn("webui-project-registry-api.md", local_dev) if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/webui/app.py b/webui/app.py index d0f832b..8da3f7c 100644 --- a/webui/app.py +++ b/webui/app.py @@ -11,8 +11,20 @@ from starlette.routing import Route from webui.deployment_boundary import deployment_snapshot from webui.layout import render_page -from webui.project_registry import find_project, load_registry, registry_to_dict -from webui.project_views import render_project_detail, render_projects_list +from webui.project_registry import ( + ProjectRegistry, + RegistryError, + find_project, + known_project_ids, + load_registry, + project_detail_to_dict, + registry_to_dict, +) +from webui.project_views import ( + render_project_detail, + render_projects_list, + render_registry_error, +) from webui.prompt_library import find_prompt, library_to_dict from webui.prompt_views import render_prompt_detail, render_prompts_page from final_report_validator import FINAL_REPORT_TASK_KINDS @@ -81,14 +93,26 @@ async def api_queue(_request: Request) -> JSONResponse: return JSONResponse(queue_snapshot_to_dict(load_queue_snapshot())) +def _load_project_registry() -> tuple[ProjectRegistry | None, RegistryError | None]: + """Load the registry, converting validation failure into a fail-closed pair.""" + try: + return load_registry(), None + except RegistryError as exc: + return None, exc + + async def projects(_request: Request) -> HTMLResponse: - registry = load_registry() + registry, error = _load_project_registry() + if error is not None: + return HTMLResponse(render_registry_error(error), status_code=500) return HTMLResponse(render_projects_list(registry)) async def project_detail(request: Request) -> HTMLResponse: project_id = request.path_params["project_id"] - registry = load_registry() + registry, error = _load_project_registry() + if error is not None: + return HTMLResponse(render_registry_error(error), status_code=500) project = find_project(registry, project_id) if project is None: return HTMLResponse( @@ -106,10 +130,47 @@ async def project_detail(request: Request) -> HTMLResponse: async def api_projects(_request: Request) -> JSONResponse: - registry = load_registry() + """Unversioned MVP alias, retained through Phase 1 (#632 section 6).""" + registry, error = _load_project_registry() + if error is not None: + return JSONResponse(error.to_dict(), status_code=500) return JSONResponse(registry_to_dict(registry)) +async def api_v1_projects(_request: Request) -> JSONResponse: + registry, error = _load_project_registry() + if error is not None: + return JSONResponse(error.to_dict(), status_code=500) + return JSONResponse(registry_to_dict(registry)) + + +async def api_v1_project_detail(request: Request) -> JSONResponse: + project_id = request.path_params["project_id"] + registry, error = _load_project_registry() + if error is not None: + return JSONResponse(error.to_dict(), status_code=500) + project = find_project(registry, project_id) + if project is None: + return JSONResponse( + { + "error": "project_not_found", + "project_id": project_id, + "known_project_ids": known_project_ids(registry), + "remediation": ( + "Request one of the known project ids, or add the project to the " + "registry file named in 'source'." + ), + "source": { + "kind": "file", + "path": str(registry.source_path), + "inventory_complete": True, + }, + }, + status_code=404, + ) + return JSONResponse(project_detail_to_dict(registry, project)) + + async def prompts(_request: Request) -> HTMLResponse: return HTMLResponse(render_prompts_page()) @@ -268,6 +329,12 @@ def create_app(*, bind_host: str | None = None) -> Starlette: Route("/projects", projects, methods=["GET"]), Route("/projects/{project_id}", project_detail, methods=["GET"]), Route("/api/projects", api_projects, methods=["GET"]), + Route("/api/v1/projects", api_v1_projects, methods=["GET"]), + Route( + "/api/v1/projects/{project_id}", + api_v1_project_detail, + methods=["GET"], + ), Route("/prompts", prompts, methods=["GET"]), Route("/prompts/{prompt_id}", prompt_detail, methods=["GET"]), Route("/api/prompts", api_prompts, methods=["GET"]), diff --git a/webui/data/projects.registry.json b/webui/data/projects.registry.json index cd6c852..04eca9c 100644 --- a/webui/data/projects.registry.json +++ b/webui/data/projects.registry.json @@ -1,13 +1,15 @@ { - "version": 1, + "version": 2, "projects": [ { "id": "gitea-tools", "repo_name": "Gitea-Tools", "gitea_owner": "Scaled-Tech-Consulting", + "remote_name": "prgs", "remote_host": "https://gitea.prgs.cc", "default_branch": "master", "local_checkout_path": ".", + "status": "active", "profiles": { "author": "prgs-author", "reviewer": "prgs-reviewer", @@ -26,24 +28,32 @@ { "id": "profiles", "title": "Configure execution profiles", - "description": "Install author, reviewer, and reconciler MCP profiles (prgs-author, prgs-reviewer, prgs-reconciler) in separate namespaces. Tokens stay in keychain — never in this registry." + "description": "Install author, reviewer, and reconciler MCP profiles (prgs-author, prgs-reviewer, prgs-reconciler) in separate namespaces. Tokens stay in keychain — never in this registry.", + "state": "complete", + "required": true }, { "id": "mcp_config", "title": "Wire MCP v2 contexts", - "description": "Copy and customize gitea-mcp.v2-contexts.example.json for your machine. Map this repo path under projects with default_owner Scaled-Tech-Consulting and default_repo Gitea-Tools." + "description": "Copy and customize gitea-mcp.v2-contexts.example.json for your machine. Map this repo path under projects with default_owner Scaled-Tech-Consulting and default_repo Gitea-Tools.", + "state": "complete", + "required": true }, { "id": "wiki_gate", "title": "Wiki publication readiness", - "description": "For wiki-tracked work, satisfy the live Gitea Wiki proof gate (#224) before closing issues. See docs/wiki/Safety-and-Gates.md." + "description": "For wiki-tracked work, satisfy the live Gitea Wiki proof gate (#224) before closing issues. See docs/wiki/Safety-and-Gates.md.", + "state": "complete", + "required": true }, { "id": "branches_layout", "title": "Isolate work under branches/", - "description": "All LLM task edits happen in worktrees under branches/. Main checkout stays clean; use skills/llm-project-workflow templates for start-issue and review flows." + "description": "All LLM task edits happen in worktrees under branches/. Main checkout stays clean; use skills/llm-project-workflow templates for start-issue and review flows.", + "state": "complete", + "required": true } ] } ] -} \ No newline at end of file +} diff --git a/webui/project_registry.py b/webui/project_registry.py index afc5e15..dc96ef9 100644 --- a/webui/project_registry.py +++ b/webui/project_registry.py @@ -1,4 +1,16 @@ -"""Load and validate the web UI project registry (#427).""" +"""Load and validate the web UI project registry (#427, evolved for #635). + +Phase 1 of the console architecture ADR keeps this loader read-only. It owns +the versioned project registry contract served at ``/api/v1/projects``: + +* the on-disk file carries a ``version`` (schema version 1 or 2); +* version 1 files stay loadable and are normalized with explicit defaults, so + an operator registry written for #427 keeps working; +* every validation failure raises :class:`RegistryError`, which carries an + actionable ``remediation`` string instead of leaking a traceback; +* serialization never emits credentials — credential-shaped keys are rejected + at load time, before any DTO is built. +""" from __future__ import annotations @@ -8,7 +20,59 @@ from dataclasses import dataclass from pathlib import Path from typing import Any -from webui.registry_safety import reject_credential_keys as _reject_credential_keys +from webui.registry_safety import is_forbidden_key + +#: Version of the JSON contract served under ``/api/v1/...``. +REGISTRY_API_VERSION = "v1" + +#: Schema version written by this repository's packaged registry. +CURRENT_SCHEMA_VERSION = 2 + +#: Schema versions this loader accepts. Version 1 is normalized on load. +SUPPORTED_SCHEMA_VERSIONS = (1, 2) + +#: Lifecycle state of a registered project. +PROJECT_STATUSES = ("active", "onboarding", "paused", "archived") +_DEFAULT_PROJECT_STATUS = "active" + +#: Completion state of a single onboarding step. +ONBOARDING_STATES = ("complete", "pending", "blocked", "not_applicable") +_DEFAULT_ONBOARDING_STATE = "pending" + +#: Redacted, last-seen health of a project's control plane. +HEALTH_STATUSES = ("healthy", "degraded", "unreachable", "unknown") + +class RegistryError(ValueError): + """A registry file could not be loaded or failed validation. + + Carries an operator-facing ``remediation`` so routes can fail closed with + an actionable message rather than a stack trace. + """ + + def __init__( + self, + message: str, + *, + remediation: str, + source_path: Path | None = None, + field_path: str | None = None, + ) -> None: + super().__init__(message) + self.message = message + self.remediation = remediation + self.source_path = source_path + self.field_path = field_path + + def to_dict(self) -> dict[str, Any]: + """Serialize for a fail-closed JSON error response.""" + return { + "error": "registry_invalid", + "detail": self.message, + "remediation": self.remediation, + "field_path": self.field_path, + "source_path": str(self.source_path) if self.source_path else None, + } + _REQUIRED_PROJECT_FIELDS = ( "id", @@ -29,6 +93,30 @@ class OnboardingStep: id: str title: str description: str + state: str = _DEFAULT_ONBOARDING_STATE + required: bool = True + + +@dataclass(frozen=True) +class OnboardingSummary: + """Aggregate onboarding progress for a single project.""" + + total: int + complete: int + pending: int + blocked: int + not_applicable: int + required_outstanding: int + onboarding_complete: bool + + +@dataclass(frozen=True) +class ProjectHealth: + """Redacted last-seen health. Never carries endpoints or credentials.""" + + status: str + checked_at: str | None + detail: str | None @dataclass(frozen=True) @@ -43,6 +131,13 @@ class ProjectRecord: workflow_paths: dict[str, str] schema_paths: dict[str, str] onboarding_checklist: tuple[OnboardingStep, ...] + status: str = _DEFAULT_PROJECT_STATUS + remote_name: str | None = None + last_seen_health: ProjectHealth | None = None + + @property + def repo_full_name(self) -> str: + return f"{self.gitea_owner}/{self.repo_name}" @dataclass(frozen=True) @@ -51,6 +146,15 @@ class ProjectRegistry: projects: tuple[ProjectRecord, ...] source_path: Path + @property + def schema_version(self) -> int: + """Alias of :attr:`version` — the schema version read from disk.""" + return self.version + + @property + def api_version(self) -> str: + return REGISTRY_API_VERSION + def default_registry_path() -> Path: override = os.environ.get("WEBUI_PROJECT_REGISTRY", "").strip() @@ -59,40 +163,221 @@ def default_registry_path() -> Path: return (Path(__file__).resolve().parent / "data" / "projects.registry.json").resolve() -def _parse_onboarding(raw: list[dict[str, Any]] | None) -> tuple[OnboardingStep, ...]: - if not raw: +def _reject_credential_keys(obj: Any, *, path: str = "", source: Path | None = None) -> None: + """Recursive credential-key guard that reports an actionable ``field_path``. + + Key *shape* is decided by :func:`webui.registry_safety.is_forbidden_key`, the + single source of truth shared with the worker registry (#798). + """ + if isinstance(obj, dict): + for key, value in obj.items(): + key_path = f"{path}.{key}" if path else key + if is_forbidden_key(key): + raise RegistryError( + f"registry must not store credentials ({key_path})", + remediation=( + f"Remove the credential-shaped key '{key_path}' from the registry. " + "Tokens live in the keychain and are resolved server-side by " + "gitea_auth; the registry is redacted metadata only." + ), + source_path=source, + field_path=key_path, + ) + _reject_credential_keys(value, path=key_path, source=source) + elif isinstance(obj, list): + for index, item in enumerate(obj): + _reject_credential_keys(item, path=f"{path}[{index}]", source=source) + + +def _require_enum( + value: Any, + *, + allowed: tuple[str, ...], + field_path: str, + source: Path | None, +) -> str: + text = str(value) + if text not in allowed: + raise RegistryError( + f"{field_path} must be one of {', '.join(allowed)} (got {text!r})", + remediation=( + f"Set {field_path} to one of: {', '.join(allowed)}. " + "Unknown values fail closed so the console never renders an " + "unverified state." + ), + source_path=source, + field_path=field_path, + ) + return text + + +def _parse_onboarding( + raw: Any, + *, + project_path: str, + source: Path | None, +) -> tuple[OnboardingStep, ...]: + if raw is None: return () + if not isinstance(raw, list): + raise RegistryError( + f"{project_path}.onboarding_checklist must be an array", + remediation=( + f"Rewrite {project_path}.onboarding_checklist as a JSON array of " + "steps with id, title, description, and optional state." + ), + source_path=source, + field_path=f"{project_path}.onboarding_checklist", + ) steps: list[OnboardingStep] = [] - for item in raw: + for index, item in enumerate(raw): + step_path = f"{project_path}.onboarding_checklist[{index}]" + if not isinstance(item, dict): + raise RegistryError( + f"{step_path} must be an object", + remediation=f"Rewrite {step_path} as an object with id, title, description.", + source_path=source, + field_path=step_path, + ) + missing = [field for field in ("id", "title", "description") if field not in item] + if missing: + raise RegistryError( + f"{step_path} missing required fields: {', '.join(missing)}", + remediation=( + f"Add {', '.join(missing)} to {step_path}. Every onboarding step " + "must be self-describing for an operator who has no chat history." + ), + source_path=source, + field_path=step_path, + ) + state = _require_enum( + item.get("state", _DEFAULT_ONBOARDING_STATE), + allowed=ONBOARDING_STATES, + field_path=f"{step_path}.state", + source=source, + ) steps.append( OnboardingStep( id=str(item["id"]), title=str(item["title"]), description=str(item["description"]), + state=state, + required=bool(item.get("required", True)), ) ) return tuple(steps) -def _parse_project(raw: dict[str, Any]) -> ProjectRecord: +def _parse_health( + raw: Any, + *, + project_path: str, + source: Path | None, +) -> ProjectHealth | None: + if raw is None: + return None + if not isinstance(raw, dict): + raise RegistryError( + f"{project_path}.last_seen_health must be an object when present", + remediation=( + f"Rewrite {project_path}.last_seen_health as an object with status " + f"(one of {', '.join(HEALTH_STATUSES)}), optional checked_at and detail, " + "or remove it. Never store endpoints or credentials here." + ), + source_path=source, + field_path=f"{project_path}.last_seen_health", + ) + status = _require_enum( + raw.get("status", "unknown"), + allowed=HEALTH_STATUSES, + field_path=f"{project_path}.last_seen_health.status", + source=source, + ) + checked_at = raw.get("checked_at") + detail = raw.get("detail") + return ProjectHealth( + status=status, + checked_at=str(checked_at) if checked_at is not None else None, + detail=str(detail) if detail is not None else None, + ) + + +def _parse_project(raw: Any, *, index: int, source: Path | None) -> ProjectRecord: + project_path = f"projects[{index}]" + if not isinstance(raw, dict): + raise RegistryError( + f"{project_path} must be an object", + remediation=f"Rewrite {project_path} as a JSON object describing one project.", + source_path=source, + field_path=project_path, + ) + missing = [field for field in _REQUIRED_PROJECT_FIELDS if field not in raw] if missing: - raise ValueError(f"project missing required fields: {', '.join(missing)}") + raise RegistryError( + f"{project_path} missing required fields: {', '.join(missing)}", + remediation=( + f"Add {', '.join(missing)} to {project_path}. See " + "docs/webui-project-registry-api.md for the field-by-field contract." + ), + source_path=source, + field_path=project_path, + ) profiles = raw["profiles"] if not isinstance(profiles, dict): - raise ValueError("profiles must be an object") + raise RegistryError( + f"{project_path}.profiles must be an object", + remediation=( + f"Rewrite {project_path}.profiles as an object mapping " + f"{', '.join(_REQUIRED_PROFILE_ROLES)} to MCP profile names." + ), + source_path=source, + field_path=f"{project_path}.profiles", + ) for role in _REQUIRED_PROFILE_ROLES: if role not in profiles or not profiles[role]: - raise ValueError(f"profiles.{role} is required") + raise RegistryError( + f"{project_path}.profiles.{role} is required", + remediation=( + f"Set {project_path}.profiles.{role} to the configured MCP profile " + "name for that role. Role separation is a workflow-safety invariant." + ), + source_path=source, + field_path=f"{project_path}.profiles.{role}", + ) workflow_paths = raw["workflow_paths"] if not isinstance(workflow_paths, dict) or not workflow_paths: - raise ValueError("workflow_paths must be a non-empty object") + raise RegistryError( + f"{project_path}.workflow_paths must be a non-empty object", + remediation=( + f"Add at least a 'skill' entry to {project_path}.workflow_paths pointing " + "at the project's canonical workflow skill." + ), + source_path=source, + field_path=f"{project_path}.workflow_paths", + ) schema_paths = raw.get("schema_paths") or {} if not isinstance(schema_paths, dict): - raise ValueError("schema_paths must be an object when present") + raise RegistryError( + f"{project_path}.schema_paths must be an object when present", + remediation=( + f"Rewrite {project_path}.schema_paths as an object of label to repo path, " + "or remove it." + ), + source_path=source, + field_path=f"{project_path}.schema_paths", + ) + + status = _require_enum( + raw.get("status", _DEFAULT_PROJECT_STATUS), + allowed=PROJECT_STATUSES, + field_path=f"{project_path}.status", + source=source, + ) + remote_name = raw.get("remote_name") return ProjectRecord( id=str(raw["id"]), @@ -104,61 +389,211 @@ def _parse_project(raw: dict[str, Any]) -> ProjectRecord: profiles={role: str(profiles[role]) for role in _REQUIRED_PROFILE_ROLES}, workflow_paths={key: str(value) for key, value in workflow_paths.items()}, schema_paths={key: str(value) for key, value in schema_paths.items()}, - onboarding_checklist=_parse_onboarding(raw.get("onboarding_checklist")), + onboarding_checklist=_parse_onboarding( + raw.get("onboarding_checklist"), + project_path=project_path, + source=source, + ), + status=status, + remote_name=str(remote_name) if remote_name else None, + last_seen_health=_parse_health( + raw.get("last_seen_health"), + project_path=project_path, + source=source, + ), ) def load_registry(path: Path | None = None) -> ProjectRegistry: - """Load the versioned project registry from disk.""" + """Load the versioned project registry from disk. + + Raises: + RegistryError: whenever the file is unreadable, is not valid JSON, or + fails schema validation. The error carries an operator remediation. + """ source = (path or default_registry_path()).resolve() - raw_text = source.read_text(encoding="utf-8") - payload = json.loads(raw_text) + try: + raw_text = source.read_text(encoding="utf-8") + except OSError as exc: + raise RegistryError( + f"registry file could not be read: {exc.strerror or exc}", + remediation=( + f"Create a readable registry at {source}, or point " + "WEBUI_PROJECT_REGISTRY at an existing file." + ), + source_path=source, + ) from exc + + try: + payload = json.loads(raw_text) + except json.JSONDecodeError as exc: + raise RegistryError( + f"registry is not valid JSON: {exc.msg} (line {exc.lineno}, column {exc.colno})", + remediation=( + f"Fix the JSON syntax in {source} at line {exc.lineno}, column {exc.colno}." + ), + source_path=source, + ) from exc + if not isinstance(payload, dict): - raise ValueError("registry root must be an object") + raise RegistryError( + "registry root must be an object", + remediation=( + "Wrap the registry in a JSON object with 'version' and 'projects' keys." + ), + source_path=source, + ) version = payload.get("version") - if version != 1: - raise ValueError(f"unsupported registry version: {version!r}") + if version not in SUPPORTED_SCHEMA_VERSIONS: + supported = ", ".join(str(item) for item in SUPPORTED_SCHEMA_VERSIONS) + raise RegistryError( + f"unsupported registry version: {version!r}", + remediation=( + f"Set 'version' to one of {supported} (current schema is " + f"{CURRENT_SCHEMA_VERSION}). Migration notes live in " + "docs/webui-project-registry-api.md." + ), + source_path=source, + field_path="version", + ) - _reject_credential_keys(payload) + _reject_credential_keys(payload, source=source) projects_raw = payload.get("projects") if not isinstance(projects_raw, list) or not projects_raw: - raise ValueError("projects must be a non-empty array") + raise RegistryError( + "projects must be a non-empty array", + remediation=( + "Add at least one project object to 'projects'. An empty console " + "registry fails closed rather than rendering a blank inventory." + ), + source_path=source, + field_path="projects", + ) - projects = tuple(_parse_project(item) for item in projects_raw) - return ProjectRegistry(version=version, projects=projects, source_path=source) + projects = tuple( + _parse_project(item, index=index, source=source) + for index, item in enumerate(projects_raw) + ) + return ProjectRegistry(version=int(version), projects=projects, source_path=source) + + +def onboarding_summary(project: ProjectRecord) -> OnboardingSummary: + """Aggregate a project's onboarding checklist state.""" + steps = project.onboarding_checklist + counts = {state: 0 for state in ONBOARDING_STATES} + for step in steps: + counts[step.state] += 1 + required_outstanding = sum( + 1 + for step in steps + if step.required and step.state in ("pending", "blocked") + ) + return OnboardingSummary( + total=len(steps), + complete=counts["complete"], + pending=counts["pending"], + blocked=counts["blocked"], + not_applicable=counts["not_applicable"], + required_outstanding=required_outstanding, + onboarding_complete=required_outstanding == 0, + ) def project_to_dict(project: ProjectRecord) -> dict[str, Any]: - """Serialize a project for JSON API responses.""" + """Serialize a project for JSON API responses and HTML views. + + The HTML views render from this same DTO, so the console and the API can + never disagree about a project's status or onboarding progress. + """ + summary = onboarding_summary(project) + health = project.last_seen_health return { "id": project.id, "repo_name": project.repo_name, "gitea_owner": project.gitea_owner, + "repo_full_name": project.repo_full_name, "remote_host": project.remote_host, + "remote_name": project.remote_name, "default_branch": project.default_branch, "local_checkout_path": project.local_checkout_path, + "status": project.status, "profiles": dict(project.profiles), "workflow_paths": dict(project.workflow_paths), "schema_paths": dict(project.schema_paths), "onboarding_checklist": [ - {"id": step.id, "title": step.title, "description": step.description} + { + "id": step.id, + "title": step.title, + "description": step.description, + "state": step.state, + "required": step.required, + } for step in project.onboarding_checklist ], + "onboarding_summary": { + "total": summary.total, + "complete": summary.complete, + "pending": summary.pending, + "blocked": summary.blocked, + "not_applicable": summary.not_applicable, + "required_outstanding": summary.required_outstanding, + "onboarding_complete": summary.onboarding_complete, + }, + "last_seen_health": ( + None + if health is None + else { + "status": health.status, + "checked_at": health.checked_at, + "detail": health.detail, + } + ), } def registry_to_dict(registry: ProjectRegistry) -> dict[str, Any]: + """Serialize the whole registry, including API provenance (#632 section 6).""" return { + "api_version": registry.api_version, + "schema_version": registry.schema_version, + # Retained for the unversioned MVP alias consumers (#427). "version": registry.version, "source_path": str(registry.source_path), + "source": { + "kind": "file", + "path": str(registry.source_path), + "inventory_complete": True, + }, + "project_count": len(registry.projects), "projects": [project_to_dict(project) for project in registry.projects], } +def project_detail_to_dict( + registry: ProjectRegistry, + project: ProjectRecord, +) -> dict[str, Any]: + """Serialize a single project for ``/api/v1/projects/{project_id}``.""" + return { + "api_version": registry.api_version, + "schema_version": registry.schema_version, + "source": { + "kind": "file", + "path": str(registry.source_path), + "inventory_complete": True, + }, + "project": project_to_dict(project), + } + + def find_project(registry: ProjectRegistry, project_id: str) -> ProjectRecord | None: for project in registry.projects: if project.id == project_id: return project - return None \ No newline at end of file + return None + + +def known_project_ids(registry: ProjectRegistry) -> list[str]: + return [project.id for project in registry.projects] diff --git a/webui/project_views.py b/webui/project_views.py index c5076ef..4054eb4 100644 --- a/webui/project_views.py +++ b/webui/project_views.py @@ -1,34 +1,65 @@ -"""HTML views for project registry pages (#427).""" +"""HTML views for project registry pages (#427, evolved for #635). + +Every view renders from :func:`webui.project_registry.project_to_dict`, the +same DTO the ``/api/v1/projects`` JSON responses use, so the HTML console and +the API can never disagree about status or onboarding progress. +""" from __future__ import annotations import html +from typing import Any from webui.layout import render_page -from webui.project_registry import ProjectRecord, ProjectRegistry +from webui.project_registry import ( + ProjectRecord, + ProjectRegistry, + RegistryError, + project_to_dict, +) + +_STATE_LABELS = { + "complete": "Complete", + "pending": "Pending", + "blocked": "Blocked", + "not_applicable": "Not applicable", +} def _escape(text: str) -> str: return html.escape(text, quote=True) +def _progress_label(summary: dict[str, Any]) -> str: + total = summary["total"] + if not total: + return "no steps" + label = f"{summary['complete']}/{total} complete" + if summary["blocked"]: + label += f", {summary['blocked']} blocked" + return label + + def render_projects_list(registry: ProjectRegistry) -> str: rows = [] for project in registry.projects: + dto = project_to_dict(project) rows.append( "" - f"{_escape(project.repo_name)}" - f"{_escape(project.gitea_owner)}" - f"{_escape(project.remote_host)}" - f"{_escape(project.default_branch)}" - f"{_escape(project.profiles['author'])}" + f"{_escape(dto['repo_name'])}" + f"{_escape(dto['gitea_owner'])}" + f"{_escape(dto['remote_host'])}" + f"{_escape(dto['default_branch'])}" + f"{_escape(dto['status'])}" + f"{_escape(_progress_label(dto['onboarding_summary']))}" + f"{_escape(dto['profiles']['author'])}" "" ) table = ( "" "" "" - "" + "" "" f"{''.join(rows)}
RepositoryOwnerRemoteBranchAuthor profileBranchStatusOnboardingAuthor profile
" ) @@ -36,32 +67,39 @@ def render_projects_list(registry: ProjectRegistry) -> str: "

Projects

" "

Configured repositories managed by the MCP Control Plane.

" f"

Registry: {_escape(str(registry.source_path))} " - f"(version {registry.version})

" + f"(schema version {registry.schema_version}, " + f"API {_escape(registry.api_version)})

" f"{table}" - "

JSON API

" + "

JSON API " + "(unversioned alias)

" ) return render_page(title="Projects", body_html=body) def render_project_detail(project: ProjectRecord) -> str: + dto = project_to_dict(project) profile_rows = "".join( f"{_escape(role)}{_escape(name)}" - for role, name in project.profiles.items() + for role, name in dto["profiles"].items() ) workflow_rows = "".join( f"{_escape(key)}{_escape(path)}" - for key, path in project.workflow_paths.items() + for key, path in dto["workflow_paths"].items() ) schema_rows = "".join( f"{_escape(key)}{_escape(path)}" - for key, path in project.schema_paths.items() + for key, path in dto["schema_paths"].items() ) checklist_items = [] - for index, step in enumerate(project.onboarding_checklist, start=1): + for index, step in enumerate(dto["onboarding_checklist"], start=1): + state_label = _STATE_LABELS.get(step["state"], step["state"]) + requirement = "required" if step["required"] else "optional" checklist_items.append( - "
  • " - f"{index}. {_escape(step.title)}" - f"

    {_escape(step.description)}

    " + f"
  • " + f"{index}. {_escape(step['title'])}" + f" {_escape(state_label)}" + f" ({_escape(requirement)})" + f"

    {_escape(step['description'])}

    " "
    • " ) checklist_html = ( @@ -69,16 +107,38 @@ def render_project_detail(project: ProjectRecord) -> str: if checklist_items else "

    No onboarding steps defined.

    " ) + summary = dto["onboarding_summary"] + summary_html = ( + "

    Onboarding: " + f"{_escape(_progress_label(summary))}; required outstanding " + f"{summary['required_outstanding']}.

    " + ) + health = dto["last_seen_health"] + health_html = ( + "

    No health probe recorded (Phase 1 is read-only).

    " + if health is None + else ( + "" + f"" + f"" + f"" + "
    Status{_escape(health['status'])}
    Checked at{_escape(str(health['checked_at'] or 'unknown'))}
    Detail{_escape(str(health['detail'] or ''))}
    " + ) + ) + remote_name = dto["remote_name"] or "unset" body = ( - f"

    {_escape(project.repo_name)}

    " + f"

    {_escape(dto['repo_name'])}

    " "

    ← All projects

    " "

    Identity

    " "" - f"" - f"" - f"" - f"" - f"" + f"" + f"" + f"" + f"" + f"" + f"" + f"" + f"" "
    Registry id{_escape(project.id)}
    Gitea owner{_escape(project.gitea_owner)}
    Remote host{_escape(project.remote_host)}
    Default branch{_escape(project.default_branch)}
    Local checkout{_escape(project.local_checkout_path)}
    Registry id{_escape(dto['id'])}
    Status{_escape(dto['status'])}
    Gitea owner{_escape(dto['gitea_owner'])}
    Repository{_escape(dto['repo_full_name'])}
    Remote name{_escape(remote_name)}
    Remote host{_escape(dto['remote_host'])}
    Default branch{_escape(dto['default_branch'])}
    Local checkout{_escape(dto['local_checkout_path'])}
    " "

    Profiles

    " f"{profile_rows}
    " @@ -86,8 +146,30 @@ def render_project_detail(project: ProjectRecord) -> str: f"{workflow_rows}
    " "

    Schema paths

    " f"{schema_rows}
    " + "

    Last seen health

    " + f"{health_html}" "

    Onboarding checklist

    " - "

    Read-only MVP — complete these steps outside the UI.

    " + "

    Read-only — complete these steps outside the UI.

    " + f"{summary_html}" f"{checklist_html}" + f"

    JSON detail

    " ) - return render_page(title=project.repo_name, body_html=body) \ No newline at end of file + return render_page(title=dto["repo_name"], body_html=body) + + +def render_registry_error(error: RegistryError) -> str: + """Render a fail-closed page for an invalid registry.""" + source = str(error.source_path) if error.source_path else "unknown" + field = error.field_path or "n/a" + body = ( + "

    Project registry unavailable

    " + "

    The registry failed validation, so the console refuses to render a " + "partial inventory.

    " + "" + f"" + f"" + f"" + f"" + "
    Detail{_escape(error.message)}
    Field{_escape(field)}
    Source{_escape(source)}
    Remediation{_escape(error.remediation)}
    " + ) + return render_page(title="Project registry unavailable", body_html=body) From 99fda93bccbedfe90e4c058200a65241d15f4da8 Mon Sep 17 00:00:00 2001 From: jcwalker3 Date: Wed, 22 Jul 2026 16:18:00 -0500 Subject: [PATCH 2/3] feat(mcp): publish an unpublished local commit on a registered issue worktree (Closes #812) Entry point B of #812 is the state where an author's work has already advanced to a local commit: the worktree is registered, clean, on the issue branch, and carries the only copy of the implementation, but the branch has never been published. Two individually correct predicates close a cycle around it: exact-owner lease renewal refuses without an observable remote head, and every publication path is lock-derived under #618, so nothing can create that remote head without first holding the lock renewal would grant. This adds the missing operation. gitea_publish_unpublished_issue_branch publishes an already-committed local head to its remote branch, so exact-owner renewal has the evidence its model requires. Publication is the whole of its authority: it renews, reclaims, rebinds, and clears nothing. Why this is not a lock bypass: the operation can only publish a branch whose durable issue-lock record already names the caller as claimant. Ownership is read from the lock file, never asserted by the caller. Guard strictness is unchanged (AC15): a dirty tree, an untracked file the commit does not carry, an unregistered worktree, a non-issue or stable branch, a changed local HEAD, a remote head that is not an ancestor of the commit, a competing open PR for the same issue on another branch, and any declared-hash mismatch each fail closed. The refspec names the commit SHA explicitly and never forces. Record separation (AC23): the durable issue-lock file and the control-plane workflow lease are distinct records. This reads the former as ownership evidence and writes neither. Truthful process evidence (AC24): the recorded owner pid's liveness is never consulted or asserted. A regression pins the recorded pid to a live process, proves publication still succeeds, and proves expired-lock reclaim still refuses for that same pid. Scope is AC20 only. AC21 cannot unblock on its own, so the two are separable and only the smaller one is implemented here. Tests: 36 new cases against synthetic fixtures only, using a real git repository with a real local bare remote so publication and read-after-write verification are genuinely executed rather than mocked. AC17 is honoured and a regression asserts the protected worktree is never referenced. Full suite: 11 failed, 4354 passed, 6 skipped, 533 subtests passed. The 11 failures are the documented pre-existing drift baseline at 9eb0f29, unchanged in count and identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- anti_stomp_preflight.py | 1 + branch_publish.py | 591 ++++++++++++++++ docs/mcp-tool-inventory.md | 1 + gitea_mcp_server.py | 232 +++++++ task_capability_map.py | 9 + ...st_issue_812_publish_unpublished_commit.py | 650 ++++++++++++++++++ tests/test_task_capability_role_invariants.py | 3 + 7 files changed, 1487 insertions(+) create mode 100644 branch_publish.py create mode 100644 tests/test_issue_812_publish_unpublished_commit.py diff --git a/anti_stomp_preflight.py b/anti_stomp_preflight.py index f025f13..532db08 100644 --- a/anti_stomp_preflight.py +++ b/anti_stomp_preflight.py @@ -87,6 +87,7 @@ MUTATION_TASKS = frozenset({ "edit_pr", "commit_files", "gitea_commit_files", + "publish_unpublished_branch", "delete_branch", "cleanup_merged_pr_branch", "cleanup_stale_claims", diff --git a/branch_publish.py b/branch_publish.py new file mode 100644 index 0000000..a6d3025 --- /dev/null +++ b/branch_publish.py @@ -0,0 +1,591 @@ +"""Publish an unpublished local commit on a registered issue worktree (#812 AC20). + +Entry point B of #812 is the state where an author's work has already advanced +to a local commit: the worktree is registered, clean, on the issue branch, and +carries the only copy of the implementation, but the branch has never been +published. That state deadlocks, because two individually correct predicates +close a cycle: + +* ``issue_lock_renewal.assess_exact_owner_lease_renewal`` refuses to renew an + expired lease without an observable remote head — an unpublished branch has + none. +* Every publication path (``gitea_commit_files``, ``gitea_create_pr``) derives + its workspace from the author issue lock under #618, so nothing can create + that remote head without first holding the lock. + +This module supplies the missing operation: it publishes an *already committed* +local head to the remote branch, so exact-owner renewal has the evidence it +requires. It deliberately does **not** renew, reclaim, rebind, or clear any +lock. Publication is the whole of its authority. + +Why this is not a lock bypass +----------------------------- +The operation can only publish a branch whose **durable issue-lock record +already names the caller as claimant**. Ownership is read from the lock file on +disk (``issue_lock_store``), never from a caller-supplied flag, so the tool +cannot manufacture a claim it does not already hold. Nothing here weakens the +#510/#618/#713 guards: a dirty tree, an unregistered worktree, a foreign +claimant, a changed HEAD, or a divergent remote head each refuse, exactly as +they do today. The only thing this adds is the ability to make an existing, +owned, committed, clean branch observable on the remote. + +Separation of records (#812 AC23) +--------------------------------- +The durable **issue-lock file** and the control-plane **workflow lease** are +distinct records. This module reads the former as ownership evidence and writes +neither. Publishing changes remote git state only; no lock is renewed, +abandoned, reclaimed, or generation-bumped here. + +Process evidence (#812 AC24) +---------------------------- +Liveness of the lock's recorded pid is **not consulted**. That is deliberate: +the recorded pid routinely belongs to the long-running MCP daemon rather than to +an active author client, and the existing reclaim predicate +(``assess_expired_lock_reclaim``) can never be satisfied while that daemon runs. +Publication does not require the recording process to be dead, so this module +never asserts, infers, or depends on a process being dead. Ownership is proven +by identity and profile match against the recorded claimant instead. +""" + +from __future__ import annotations + +import hashlib +import os +import re +import subprocess + +from reviewer_worktree import parse_dirty_tracked_files +from stable_branch_push_guard import is_stable_ref, redact_command + +# Assessment outcomes. +PUBLISH_SANCTIONED = "publish_sanctioned" +ALREADY_PUBLISHED = "already_published" +REFUSED = "refused" + +#: Implementation branches must stay traceable to their issue (#713 lineage). +ISSUE_BRANCH_RE = re.compile(r"^(fix|feat|docs|chore)/issue-(\d+)-.+$") + +_SHA_RE = re.compile(r"^[0-9a-f]{40}$") + + +def _text(value: object) -> str: + return value.strip() if isinstance(value, str) else "" + + +def _realpath(value: str | None) -> str | None: + path = _text(value) + return os.path.realpath(path) if path else None + + +def parse_untracked_files(porcelain_status: str) -> list[str]: + """Return untracked paths from ``git status --porcelain`` output. + + ``reviewer_worktree.parse_dirty_tracked_files`` deliberately skips ``??`` + entries. Publication needs both halves: an untracked file in the worktree is + unpublished content that the commit does not carry, so publishing would + silently leave it behind. + """ + untracked: list[str] = [] + for line in (porcelain_status or "").splitlines(): + if not line.startswith("??"): + continue + path = line[2:].strip() + if path: + untracked.append(path) + return untracked + + +def hash_worktree_files(worktree_path: str, paths) -> dict[str, str | None]: + """SHA-256 each path under *worktree_path*; ``None`` when unreadable.""" + root = _text(worktree_path) + hashes: dict[str, str | None] = {} + for rel in paths or (): + rel_text = _text(rel) + if not rel_text: + continue + full = os.path.join(root, rel_text) + try: + with open(full, "rb") as handle: + digest = hashlib.sha256() + for chunk in iter(lambda: handle.read(65536), b""): + digest.update(chunk) + hashes[rel_text] = digest.hexdigest() + except OSError: + hashes[rel_text] = None + return hashes + + +def read_remote_branch_head( + worktree_path: str, remote_name: str, branch_name: str +) -> dict: + """Observe the remote head for *branch_name*, read-only. + + ``probe_ok`` False means git could not answer at all. That is kept distinct + from "the branch does not exist": an unobservable remote must fail closed + rather than be mistaken for an absent branch, because the two lead to + opposite dispositions. + """ + path = _text(worktree_path) + remote = _text(remote_name) + branch = _text(branch_name) + result: dict = { + "probe_ok": False, + "remote_branch_exists": False, + "remote_head_sha": None, + "reasons": [], + } + if not (path and remote and branch): + result["reasons"].append( + "remote head probe requires a worktree path, remote name, and branch" + ) + return result + try: + res = subprocess.run( + ["git", "-C", path, "ls-remote", remote, f"refs/heads/{branch}"], + capture_output=True, + text=True, + check=False, + ) + except OSError as exc: # git unavailable — fail closed, never assume absent + result["reasons"].append(f"remote head probe could not run: {exc}") + return result + if res.returncode != 0: + result["reasons"].append( + f"remote head probe failed for '{branch}' on remote '{remote}'" + ) + return result + + result["probe_ok"] = True + for line in (res.stdout or "").splitlines(): + parts = line.split() + if len(parts) >= 2 and parts[1] == f"refs/heads/{branch}": + result["remote_branch_exists"] = True + result["remote_head_sha"] = parts[0].strip() + break + return result + + +def read_is_ancestor( + worktree_path: str, ancestor_sha: str, descendant_sha: str +) -> dict: + """Observe whether *ancestor_sha* is an ancestor of *descendant_sha*.""" + path = _text(worktree_path) + ancestor = _text(ancestor_sha) + descendant = _text(descendant_sha) + result: dict = {"probe_ok": False, "is_ancestor": False, "reasons": []} + if not (path and ancestor and descendant): + result["reasons"].append( + "ancestry probe requires a worktree path and both commit SHAs" + ) + return result + try: + present = subprocess.run( + ["git", "-C", path, "rev-parse", "--verify", "--quiet", + f"{ancestor}^{{commit}}"], + capture_output=True, text=True, check=False, + ) + if present.returncode != 0: + result["reasons"].append( + f"remote head {ancestor} is not present locally, so it cannot be " + "proven an ancestor of the commit being published" + ) + return result + res = subprocess.run( + ["git", "-C", path, "merge-base", "--is-ancestor", ancestor, descendant], + capture_output=True, text=True, check=False, + ) + except OSError as exc: + result["reasons"].append(f"ancestry probe could not run: {exc}") + return result + result["probe_ok"] = res.returncode in (0, 1) + result["is_ancestor"] = res.returncode == 0 + return result + + +def assess_unpublished_commit_publication( + existing_lock, + *, + issue_number: int, + branch_name: str, + worktree_path: str, + expected_head: str, + remote: str, + org: str, + repo: str, + identity: str | None, + profile: str | None, + worktree_state, + worktree_registered: bool | None = None, + remote_probe=None, + ancestry=None, + competing_open_prs=(), + expected_file_hashes=None, + observed_file_hashes=None, +) -> dict: + """Decide whether an unpublished local commit may be published (#812 AC20). + + Pure predicate. Every input is either a caller-declared expectation that + must be *matched* against observation, or a server-side observation. No + caller-supplied boolean is accepted as proof of ownership, liveness, or + eligibility: ``existing_lock`` comes from the durable lock file and the + git/PR state is observed by the server. + + The single mutating disposition it can return is "publish this exact commit + to this exact branch". It never sanctions renewal, reclamation, force + updates, history rewriting, or publication of uncommitted content. + """ + reasons: list[str] = [] + branch = _text(branch_name) + head = _text(expected_head).lower() + workspace = _realpath(worktree_path) + state = worktree_state if isinstance(worktree_state, dict) else {} + lock = existing_lock if isinstance(existing_lock, dict) else None + + evidence: dict = { + "issue_number": issue_number, + "branch_name": branch or None, + "worktree_path": workspace, + "expected_head": head or None, + "remote": _text(remote) or None, + "org": _text(org) or None, + "repo": _text(repo) or None, + "identity": _text(identity) or None, + "profile": _text(profile) or None, + "lock_record_present": lock is not None, + "recorded_claimant": None, + "recorded_branch": None, + "recorded_worktree": None, + "lock_generation": None, + "local_head_sha": _text(state.get("head_sha")) or None, + "current_branch": _text(state.get("current_branch")) or None, + "dirty_tracked_files": [], + "untracked_files": [], + "worktree_registered": worktree_registered, + "remote_branch_exists": None, + "remote_head_sha": None, + "fast_forward_from_remote": None, + "competing_open_prs": [], + "file_hashes_verified": None, + "hash_mismatches": [], + # Recorded explicitly so no reader mistakes silence for a liveness + # claim, and so the audit shows which records were left alone (AC23/AC24). + "owner_pid_liveness_consulted": False, + "workflow_lease_touched": False, + "issue_lock_record_mutated": False, + } + + # ── declared shape ──────────────────────────────────────────────────── + if not branch: + reasons.append("branch name not declared; fail closed") + if not head: + reasons.append( + "expected_head not declared; publication must name the exact commit" + ) + elif not _SHA_RE.match(head): + reasons.append( + f"expected_head '{head}' is not a full 40-character commit SHA; " + "abbreviated or symbolic revisions are refused" + ) + if not workspace: + reasons.append("worktree path not declared; fail closed") + + if branch: + match = ISSUE_BRANCH_RE.match(branch) + if not match: + reasons.append( + f"branch '{branch}' is not an issue-linked implementation branch " + "((fix|feat|docs|chore)/issue--); fail closed" + ) + elif int(match.group(2)) != int(issue_number): + reasons.append( + f"branch '{branch}' does not carry issue number {issue_number}; " + "fail closed" + ) + if is_stable_ref(branch): + reasons.append( + f"refusing to publish stable branch '{branch}'; this operation " + "publishes issue branches only" + ) + + # ── ownership: durable issue-lock record only (AC8, AC20, AC24) ─────── + if lock is None: + reasons.append( + "no durable issue-lock record for this issue; publication requires an " + "existing recorded claim naming the caller, so this operation cannot " + "be used to bypass the author lock" + ) + else: + lease = lock.get("work_lease") + lease = lease if isinstance(lease, dict) else {} + claimant = lease.get("claimant") + claimant = claimant if isinstance(claimant, dict) else {} + recorded_user = _text(claimant.get("username")) + recorded_profile = _text(claimant.get("profile")) + recorded_branch = _text(lock.get("branch_name")) or _text(lease.get("branch")) + recorded_worktree = _realpath( + _text(lock.get("worktree_path")) or _text(lease.get("worktree_path")) + ) + evidence["recorded_claimant"] = { + "username": recorded_user or None, + "profile": recorded_profile or None, + } + evidence["recorded_branch"] = recorded_branch or None + evidence["recorded_worktree"] = recorded_worktree + try: + evidence["lock_generation"] = int(lock.get("lock_generation") or 0) + except (TypeError, ValueError): + evidence["lock_generation"] = 0 + + try: + recorded_issue = int(lock.get("issue_number") or 0) + except (TypeError, ValueError): + recorded_issue = 0 + if recorded_issue != int(issue_number): + reasons.append( + f"durable lock records issue {lock.get('issue_number')}, not " + f"{issue_number}; ambiguous ownership, fail closed" + ) + for field, declared in ( + ("remote", _text(remote)), + ("org", _text(org)), + ("repo", _text(repo)), + ): + recorded = _text(lock.get(field)) + if recorded and declared and recorded != declared: + reasons.append( + f"durable lock records {field} '{recorded}' but the request " + f"declares '{declared}'; repository mismatch, fail closed" + ) + if recorded_branch and branch and recorded_branch != branch: + reasons.append( + f"durable lock records branch '{recorded_branch}' but the request " + f"declares '{branch}'; fail closed" + ) + if recorded_worktree and workspace and recorded_worktree != workspace: + reasons.append( + f"durable lock records worktree '{recorded_worktree}' but the " + f"request declares '{workspace}'; fail closed" + ) + if not recorded_user or not recorded_profile: + reasons.append( + "durable lock does not record a claimant username and profile; " + "ownership cannot be proven, fail closed" + ) + else: + if recorded_user != _text(identity): + reasons.append( + f"durable lock claimant '{recorded_user}' is not the acting " + f"identity '{_text(identity) or '(unknown)'}'; foreign claim, " + "fail closed" + ) + if recorded_profile != _text(profile): + reasons.append( + f"durable lock claimant profile '{recorded_profile}' is not " + f"the active profile '{_text(profile) or '(unknown)'}'; " + "fail closed" + ) + + # ── worktree: registered, on-branch, clean, at the expected commit ──── + if worktree_registered is False: + reasons.append( + f"worktree '{workspace}' is not listed in git worktree list; #713 " + "requires a genuinely registered worktree, fail closed" + ) + + current_branch = _text(state.get("current_branch")) + if not current_branch: + reasons.append("worktree branch could not be observed; fail closed") + elif branch and current_branch != branch: + reasons.append( + f"worktree is on branch '{current_branch}', not '{branch}'; fail closed" + ) + + porcelain = state.get("porcelain_status") or "" + dirty_tracked = parse_dirty_tracked_files(porcelain) + untracked = parse_untracked_files(porcelain) + evidence["dirty_tracked_files"] = dirty_tracked + evidence["untracked_files"] = untracked + if dirty_tracked: + reasons.append( + "worktree has dirty tracked files, so the commit is not the whole of " + f"the work: {', '.join(dirty_tracked)}. This operation publishes an " + "existing clean commit only; uncommitted content is out of scope" + ) + if untracked: + reasons.append( + "worktree has untracked files that the commit does not carry: " + f"{', '.join(untracked)}. Publishing would silently leave them " + "behind; fail closed" + ) + + local_head = _text(state.get("head_sha")).lower() + if not local_head: + reasons.append("local HEAD could not be observed; fail closed") + elif head and local_head != head: + reasons.append( + f"worktree HEAD is {local_head} but the request declares {head}; the " + "local commit changed since it was recorded, fail closed" + ) + + # ── remote state ────────────────────────────────────────────────────── + probe = remote_probe if isinstance(remote_probe, dict) else {} + already_published = False + if not probe.get("probe_ok"): + reasons.append( + "remote branch head could not be observed; publication must not " + "proceed against an unknown remote state, fail closed" + ) + reasons.extend(probe.get("reasons") or []) + else: + remote_exists = bool(probe.get("remote_branch_exists")) + remote_head = _text(probe.get("remote_head_sha")).lower() or None + evidence["remote_branch_exists"] = remote_exists + evidence["remote_head_sha"] = remote_head + if remote_exists and remote_head and head: + if remote_head == head: + already_published = True + evidence["fast_forward_from_remote"] = True + else: + anc = ancestry if isinstance(ancestry, dict) else {} + is_anc = bool(anc.get("probe_ok")) and bool(anc.get("is_ancestor")) + evidence["fast_forward_from_remote"] = is_anc + if not is_anc: + reasons.append( + f"remote branch '{branch}' already exists at {remote_head}, " + f"which is not an ancestor of {head}; publishing would " + "discard or rewrite published history, fail closed" + ) + reasons.extend(anc.get("reasons") or []) + elif remote_exists and not remote_head: + reasons.append( + f"remote branch '{branch}' exists but its head could not be read; " + "fail closed" + ) + + # ── competing claims ────────────────────────────────────────────────── + competing = [p for p in (competing_open_prs or ()) if p] + evidence["competing_open_prs"] = list(competing) + if competing: + reasons.append( + f"open pull request(s) {competing} already claim issue {issue_number} " + "or this branch; ambiguous ownership, fail closed" + ) + + # ── content verification before publication ─────────────────────────── + if expected_file_hashes: + observed = ( + observed_file_hashes if isinstance(observed_file_hashes, dict) else {} + ) + mismatches: list[str] = [] + for path, expected_digest in dict(expected_file_hashes).items(): + actual = observed.get(path) + if actual is None: + mismatches.append(f"{path}: missing or unreadable in the worktree") + elif _text(actual).lower() != _text(expected_digest).lower(): + mismatches.append( + f"{path}: expected {expected_digest}, observed {actual}" + ) + evidence["hash_mismatches"] = mismatches + evidence["file_hashes_verified"] = not mismatches + if mismatches: + reasons.append( + "declared content hashes do not match the worktree: " + + "; ".join(mismatches) + + ". Refusing to publish content that is not what was recorded" + ) + + if reasons: + return { + "outcome": REFUSED, + "publish_sanctioned": False, + "already_published": False, + "reasons": reasons, + "evidence": evidence, + } + return { + "outcome": ALREADY_PUBLISHED if already_published else PUBLISH_SANCTIONED, + # Idempotent retry: a remote head that already equals the assessed commit + # needs no second push, so the caller verifies instead of acting. + "publish_sanctioned": not already_published, + "already_published": already_published, + "reasons": [], + "evidence": evidence, + } + + +def publish_commit_to_remote_branch( + *, + worktree_path: str, + remote_name: str, + branch_name: str, + expected_head: str, +) -> dict: + """Send exactly *expected_head* to ``refs/heads/``. + + The refspec names the commit SHA explicitly rather than ``HEAD`` or the + local branch, so what lands is the commit that was assessed and nothing + else. No force, no lease, no ``+`` prefix: a non-fast-forward is rejected by + git itself, the last of several independent guards against overwriting + published history. + """ + path = _text(worktree_path) + remote = _text(remote_name) + branch = _text(branch_name) + head = _text(expected_head) + result: dict = { + "success": False, + "pushed_ref": f"refs/heads/{branch}" if branch else None, + "pushed_sha": head or None, + "stderr": None, + "reasons": [], + } + if not (path and remote and branch and head): + result["reasons"].append( + "publication requires a worktree path, remote, branch, and commit SHA" + ) + return result + + refspec = f"{head}:refs/heads/{branch}" + try: + res = subprocess.run( + ["git", "-C", path, "push", remote, refspec], + capture_output=True, + text=True, + check=False, + ) + except OSError as exc: + result["reasons"].append(f"publication could not run: {exc}") + return result + + if res.returncode != 0: + # Redact before surfacing: failures can echo credentialed remote URLs. + result["stderr"] = redact_command(res.stderr or "") + result["reasons"].append( + f"publication of {head} to '{branch}' on remote '{remote}' failed" + ) + return result + + result["success"] = True + return result + + +def verify_published_head( + *, worktree_path: str, remote_name: str, branch_name: str, expected_head: str +) -> dict: + """Read-after-write: confirm the remote head equals *expected_head* (AC20).""" + probe = read_remote_branch_head(worktree_path, remote_name, branch_name) + head = _text(expected_head).lower() + observed = _text(probe.get("remote_head_sha")).lower() or None + verified = bool(head) and bool(probe.get("probe_ok")) and observed == head + reasons: list[str] = list(probe.get("reasons") or []) + if probe.get("probe_ok") and not verified: + reasons.append( + "read-after-write verification failed: remote head is " + f"{observed or '(absent)'}, expected {head}" + ) + return { + "verified": verified, + "remote_head_sha": observed, + "expected_head": head or None, + "reasons": reasons, + } diff --git a/docs/mcp-tool-inventory.md b/docs/mcp-tool-inventory.md index a44a6c7..8cb865b 100644 --- a/docs/mcp-tool-inventory.md +++ b/docs/mcp-tool-inventory.md @@ -120,6 +120,7 @@ that gates each call, not which tools exist. - `gitea_observability_list_projects` - `gitea_observability_reconcile_incident` - `gitea_post_heartbeat` +- `gitea_publish_unpublished_issue_branch` - `gitea_quarantine_contaminated_review` - `gitea_reclaim_expired_workflow_lease` - `gitea_reconcile_already_landed_pr` diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 8223f4e..f71360b 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -2024,6 +2024,7 @@ import review_quarantine # noqa: E402 # #695 contaminated formal-review quaran import mcp_daemon_guard # noqa: E402 # #695 native transport provenance import already_landed_reconcile # noqa: E402 import author_mutation_worktree # noqa: E402 +import branch_publish # noqa: E402 # #812 AC20 unpublished-commit publication import root_checkout_guard # noqa: E402 import workflow_scope_guard # noqa: E402 # #683 production scope / force-on guards import stable_branch_push_guard # noqa: E402 @@ -9082,6 +9083,237 @@ def gitea_commit_files( } +def _publication_block(reasons: list[str], **extra) -> dict: + """Uniform fail-closed shape for publication refusals (#812 AC20).""" + payload = { + "success": False, + "performed": False, + "published": False, + "verified": False, + "outcome": branch_publish.REFUSED, + "reasons": reasons, + # State every record this operation left alone, so a refusal can never + # be misread as a lock mutation (#812 AC23). + "issue_lock_record_mutated": False, + "workflow_lease_touched": False, + } + payload.update(extra) + return payload + + +@mcp.tool() +def gitea_publish_unpublished_issue_branch( + issue_number: int, + branch_name: str, + worktree_path: str, + expected_head: str, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, + git_remote_name: str | None = None, + expected_file_hashes: dict | None = None, + dry_run: bool = False, +) -> dict: + """Publish an already-committed, unpublished issue branch (#812 AC20). + + Creates the remote head for a branch whose work is *already* a local commit + on a registered, clean worktree, so exact-owner lease renewal + (``issue_lock_renewal``) has the published head its evidence model requires. + This is the one step of the entry point B deadlock that no existing tool can + perform: publication is otherwise lock-derived under #618, and the lock + itself is withheld until a remote head exists. + + Not a lock bypass. The branch's **durable issue-lock record must already + name the caller as claimant** — ownership is read from the lock file, never + asserted by the caller — so this can only publish work the caller already + owns. It refuses a dirty or untracked-carrying worktree, an unregistered + worktree, a changed local HEAD, a remote head that is not an ancestor of the + commit, a competing open PR on another branch for the same issue, and any + declared-hash mismatch. It renews, reclaims, and clears nothing: the durable + issue-lock file and the control-plane workflow lease are both left untouched + (#812 AC23), and the recorded owner pid's liveness is never consulted or + asserted (#812 AC24). + + Args: + issue_number: The issue whose recorded claim authorizes publication. + branch_name: Issue branch to publish, ``(fix|feat|docs|chore)/issue-N-…``. + worktree_path: Registered worktree holding the commit. + expected_head: Full 40-character SHA of the commit to publish. Required: + publication names the exact commit, and a mismatch fails closed. + remote: Known instance — 'dadeschools' or 'prgs'. + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + git_remote_name: Git remote to publish to; defaults to *remote*. + expected_file_hashes: Optional ``{path: sha256}`` verified against the + worktree before publication. Any mismatch or missing file refuses. + dry_run: Report the decision and evidence, mutate nothing. + + Returns: + dict with 'success', 'performed', 'published', 'verified', 'outcome', + 'remote_head_sha', 'reasons', and 'evidence'. + """ + task = "publish_unpublished_branch" + ok, block_reasons = role_session_router.check_author_mutation_after_reviewer_stop( + task + ) + if not ok: + return _publication_block(block_reasons) + + blocked = _namespace_mutation_block(task, remote=remote) + if blocked: + return blocked + blocked = _profile_permission_block( + task_capability_map.required_permission(task), + remote=remote, host=host, org=org, repo=repo, + org_explicit=org is not None, + repo_explicit=repo is not None, + ) + if blocked: + return blocked + + verify_preflight_purity(remote, task=task, org=org, repo=repo) + + h, o, r = _resolve(remote, host, org, repo) + git_remote = (git_remote_name or remote or "").strip() + workspace = os.path.realpath(os.path.abspath((worktree_path or "").strip() or ".")) + + existing_lock = issue_lock_store.load_issue_lock( + remote=remote, org=o, repo=r, issue_number=int(issue_number) + ) + git_state = issue_lock_worktree.read_worktree_git_state(workspace) + registered = author_mutation_worktree.path_in_git_worktree_list( + workspace, PROJECT_ROOT + ) + + remote_probe = branch_publish.read_remote_branch_head( + workspace, git_remote, branch_name + ) + ancestry = None + probe_head = (remote_probe.get("remote_head_sha") or "").strip() + if probe_head and probe_head.lower() != (expected_head or "").strip().lower(): + ancestry = branch_publish.read_is_ancestor(workspace, probe_head, expected_head) + + # A PR on this very branch is this work's own PR, not a rival claim. Only an + # open PR for the same issue on a *different* branch is a competing claim. + competing: list = [] + for pull in _list_open_pulls(h, o, r, _auth(h)): + ref = str((pull.get("head") or {}).get("ref") or "") + if not ref or ref == branch_name: + continue + if issue_lock_adoption.branch_carries_issue_marker(ref, int(issue_number)): + competing.append(pull.get("number")) + + observed_hashes = None + if expected_file_hashes: + observed_hashes = branch_publish.hash_worktree_files( + workspace, list(expected_file_hashes.keys()) + ) + + claimant = _work_lease_claimant(h) + assessment = branch_publish.assess_unpublished_commit_publication( + existing_lock, + issue_number=int(issue_number), + branch_name=branch_name, + worktree_path=workspace, + expected_head=expected_head, + remote=remote, + org=o, + repo=r, + identity=claimant.get("username"), + profile=claimant.get("profile"), + worktree_state=git_state, + worktree_registered=registered, + remote_probe=remote_probe, + ancestry=ancestry, + competing_open_prs=competing, + expected_file_hashes=expected_file_hashes, + observed_file_hashes=observed_hashes, + ) + + if assessment["outcome"] == branch_publish.REFUSED: + return _publication_block( + assessment["reasons"], evidence=assessment["evidence"] + ) + + if dry_run: + return { + "success": True, + "performed": False, + "published": False, + "verified": False, + "dry_run": True, + "outcome": assessment["outcome"], + "would_publish": assessment["publish_sanctioned"], + "remote_head_sha": assessment["evidence"].get("remote_head_sha"), + "reasons": [], + "evidence": assessment["evidence"], + "issue_lock_record_mutated": False, + "workflow_lease_touched": False, + } + + performed = False + if assessment["publish_sanctioned"]: + with _audited( + task, host=h, remote=remote, org=o, repo=r, + target_branch=branch_name, + request_metadata={ + "issue_number": int(issue_number), + "expected_head": expected_head, + "git_remote": git_remote, + }, + ): + push = branch_publish.publish_commit_to_remote_branch( + worktree_path=workspace, + remote_name=git_remote, + branch_name=branch_name, + expected_head=expected_head, + ) + if not push.get("success"): + return _publication_block( + push.get("reasons") or ["publication failed"], + evidence=assessment["evidence"], + stderr=push.get("stderr"), + ) + performed = True + + verification = branch_publish.verify_published_head( + worktree_path=workspace, + remote_name=git_remote, + branch_name=branch_name, + expected_head=expected_head, + ) + if not verification.get("verified"): + return _publication_block( + verification.get("reasons") or ["read-after-write verification failed"], + evidence=assessment["evidence"], + performed=performed, + published=performed, + remote_head_sha=verification.get("remote_head_sha"), + ) + + return { + "success": True, + "performed": performed, + "published": True, + "verified": True, + "outcome": assessment["outcome"], + "remote_head_sha": verification.get("remote_head_sha"), + "reasons": [], + "evidence": assessment["evidence"], + # Publication is the whole of this operation's authority (#812 AC23). + "issue_lock_record_mutated": False, + "workflow_lease_touched": False, + "exact_next_action": ( + f"Remote head for '{branch_name}' is now observable. Call " + f"gitea_lock_issue(issue_number={int(issue_number)}, " + f"branch_name='{branch_name}', worktree_path='{workspace}') to renew " + "the exact-owner lease, then gitea_create_pr." + ), + } + + # Merge methods supported by the Gitea merge API. _MERGE_METHODS = ("merge", "squash", "rebase") diff --git a/task_capability_map.py b/task_capability_map.py index 23409e0..0a21063 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -62,6 +62,14 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.branch.push", "role": "author", }, + # #812 AC20: publish an already-committed, unpublished local head so + # exact-owner lease renewal has an observable remote head to reason about. + # Same authority as any other author push — deliberately not a new + # operation name, so it cannot widen an already-configured author profile. + "publish_unpublished_branch": { + "permission": "gitea.branch.push", + "role": "author", + }, "create_pr": { "permission": "gitea.pr.create", "role": "author", @@ -500,6 +508,7 @@ ROLE_EXCLUSIVE_TASKS: frozenset[str] = frozenset( "gitea_release_merger_pr_lease", "create_branch", "push_branch", + "publish_unpublished_branch", "create_pr", "commit_files", "gitea_commit_files", diff --git a/tests/test_issue_812_publish_unpublished_commit.py b/tests/test_issue_812_publish_unpublished_commit.py new file mode 100644 index 0000000..80b8d54 --- /dev/null +++ b/tests/test_issue_812_publish_unpublished_commit.py @@ -0,0 +1,650 @@ +"""Publication of an unpublished local commit (#812 AC20). + +Entry point B of #812: a registered worktree, clean, on its issue branch, +holding a local commit that has never been published. Exact-owner lease renewal +refuses such a claim for want of an observable remote head, and every existing +publication path is lock-derived, so the two predicates close a cycle around +work that is otherwise complete. + +These tests exercise the disposition through its *evidence*, never through any +particular issue number: every case uses an arbitrary issue number against a +synthetic repository, and the same assertions hold for any other. Nothing here +reads, writes, or references the live protected worktree named in #812 AC17 — +that content is preserved evidence for the duration of this work, so the +fixtures below build their own repositories from scratch. + +The remote is a local bare repository, so publication and read-after-write +verification are genuinely executed rather than mocked. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from datetime import datetime, timedelta, timezone +from unittest.mock import patch + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import branch_publish # noqa: E402 +import issue_lock_provenance # noqa: E402 +import issue_lock_renewal # noqa: E402 +import issue_lock_store # noqa: E402 +import mcp_server # noqa: E402 +from mutation_profile_fixture import shared_mutation_env # noqa: E402 + +ISSUE = 9812 +BRANCH = f"feat/issue-{ISSUE}-publish-fixture" +IDENTITY = "example-user" +PROFILE = "test-author-prgs" +ORG = "Scaled-Tech-Consulting" +REPO = "Gitea-Tools" +GIT_REMOTE = "prgs" + + +def _ts(hours: int) -> str: + return ( + (datetime.now(timezone.utc) + timedelta(hours=hours)) + .isoformat() + .replace("+00:00", "Z") + ) + + +class _PublishBase(unittest.TestCase): + """Real git repo + real bare remote + durable lock naming the caller. + + The recorded owner pid is deliberately **this live process**. That mirrors + the production shape #812 documents, where the pid belongs to a long-running + MCP daemon rather than to a dead author client, and it proves publication + never depends on a dead process (#812 AC24). + """ + + def setUp(self): + self.lock_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.lock_dir.cleanup) + self.origin = tempfile.mkdtemp(prefix="issue812-origin-") + self.repo = tempfile.mkdtemp(prefix="issue812-work-") + for path in (self.origin, self.repo): + self.addCleanup( + lambda p=path: subprocess.run(["rm", "-rf", p], check=False) + ) + self._init_repos() + self.remotes = patch.dict( + mcp_server.REMOTES, + {"prgs": {"host": "gitea.prgs.cc", "org": ORG, "repo": REPO}}, + ) + self.remotes.start() + self.addCleanup(patch.stopall) + mcp_server._IDENTITY_CACHE.clear() + + # ── fixture construction ───────────────────────────────────────────── + def _git(self, *args, cwd=None): + return subprocess.run( + ["git", "-C", cwd or self.repo, *args], + capture_output=True, + text=True, + check=True, + ) + + def _init_repos(self): + subprocess.run( + ["git", "init", "-q", "--bare", "-b", "master", self.origin], check=True + ) + self._git("init", "-q", "-b", "master") + self._git("config", "user.email", "test@example.com") + self._git("config", "user.name", "Test") + self._git("remote", "add", GIT_REMOTE, self.origin) + + with open(os.path.join(self.repo, "seed.txt"), "w") as fh: + fh.write("seed\n") + self._git("add", "seed.txt") + self._git("commit", "-q", "-m", "seed") + self.base_sha = self._git("rev-parse", "HEAD").stdout.strip() + self._git("push", "-q", GIT_REMOTE, "master") + + self._git("checkout", "-q", "-b", BRANCH) + with open(os.path.join(self.repo, "work.txt"), "w") as fh: + fh.write("unpublished implementation\n") + self._git("add", "work.txt") + self._git("commit", "-q", "-m", "unpublished implementation") + self.head_sha = self._git("rev-parse", "HEAD").stdout.strip() + self.worktree = os.path.realpath(self.repo) + + def lock_path(self): + return issue_lock_store.lock_file_path( + remote="prgs", org=ORG, repo=REPO, issue_number=ISSUE, + lock_dir=self.lock_dir.name, + ) + + def write_lock(self, **overrides): + path = self.lock_path() + claimant = overrides.pop( + "claimant", {"username": IDENTITY, "profile": PROFILE} + ) + pid = overrides.pop("session_pid", os.getpid()) + lease = { + "operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE, + "issue_number": ISSUE, + "pr_number": None, + "branch": overrides.get("branch_name", BRANCH), + "worktree_path": overrides.get("worktree_path", self.worktree), + "claimant": claimant, + "created_at": _ts(-2), + "last_heartbeat_at": _ts(-2), + # Expired: entry point B's lease has lapsed, which is precisely why + # renewal — and therefore a published head — is needed. + "expires_at": _ts(-1), + } + lease.update(overrides.pop("work_lease", {})) + data = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "remote": "prgs", + "org": ORG, + "repo": REPO, + "worktree_path": self.worktree, + "session_pid": pid, + "pid": pid, + "lock_generation": 1, + "work_lease": lease, + "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( + tool="gitea_lock_issue", claimant=claimant + ), + } + data.update(overrides) + data["lock_file_path"] = path + issue_lock_store.save_lock_file(path, data) + return path + + def _tool_env(self): + env = shared_mutation_env( + PROFILE, include_example_repo=True, + GITEA_ISSUE_LOCK_DIR=self.lock_dir.name, + ) + env["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + # These tests repoint PROJECT_ROOT at a synthetic repository so the + # registered-worktree proof runs for real. Pin the parity gate to the + # server's own startup head so that repointing does not read as a stale + # daemon; the gate itself stays live and enforced. + startup_head = mcp_server._STARTUP_PARITY.get("startup_head") or "" + env["GITEA_TEST_CURRENT_HEAD"] = startup_head + env["GITEA_TEST_LIVE_REMOTE_HEAD"] = startup_head + return env + + # ── tool driver ────────────────────────────────────────────────────── + def run_publish(self, *, open_prs=None, expected_head=None, **kwargs): + """Drive the public publication tool against the synthetic fixture.""" + env = self._tool_env() + with patch( + "mcp_server._list_open_pulls", return_value=list(open_prs or []) + ), patch( + "mcp_server._auth", return_value="token x" + ), patch( + "mcp_server.get_auth_header", return_value="token x" + ), patch( + "mcp_server._work_lease_claimant", + return_value={"username": IDENTITY, "profile": PROFILE}, + ), patch.object( + mcp_server, "PROJECT_ROOT", self.repo + ), patch.dict(os.environ, env, clear=True): + os.environ["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + return mcp_server.gitea_publish_unpublished_issue_branch( + issue_number=kwargs.pop("issue_number", ISSUE), + branch_name=kwargs.pop("branch_name", BRANCH), + worktree_path=kwargs.pop("worktree_path", self.worktree), + expected_head=expected_head or self.head_sha, + remote="prgs", + git_remote_name=kwargs.pop("git_remote_name", GIT_REMOTE), + **kwargs, + ) + + def remote_head(self, branch=BRANCH): + res = subprocess.run( + ["git", "-C", self.origin, "rev-parse", "--verify", "--quiet", branch], + capture_output=True, text=True, check=False, + ) + return (res.stdout or "").strip() or None + + +class TestSuccessfulPublication(_PublishBase): + """AC20 — the branch becomes observable and is verified after the write.""" + + def test_publishes_clean_unpublished_commit(self): + self.write_lock() + self.assertIsNone(self.remote_head(), "fixture must start unpublished") + + result = self.run_publish() + + self.assertTrue(result["success"], result.get("reasons")) + self.assertTrue(result["performed"]) + self.assertTrue(result["published"]) + self.assertTrue(result["verified"], "read-after-write must be proven") + self.assertEqual(result["remote_head_sha"], self.head_sha) + self.assertEqual(self.remote_head(), self.head_sha) + + def test_publication_does_not_rewrite_the_commit(self): + self.write_lock() + self.run_publish() + # The published object is the same commit, not a copy or a rewrite. + self.assertEqual(self.remote_head(), self.head_sha) + self.assertEqual( + self._git("rev-parse", "HEAD").stdout.strip(), self.head_sha + ) + + def test_exact_next_action_names_the_lock_call(self): + self.write_lock() + result = self.run_publish() + self.assertIn("gitea_lock_issue", result["exact_next_action"]) + + +class TestFailsClosed(_PublishBase): + """AC20/AC9 — each refusal reason, exercised independently.""" + + def test_changed_local_head_refuses(self): + self.write_lock() + stale = self.base_sha # a real commit, but not the declared head + result = self.run_publish(expected_head=stale) + self.assertFalse(result["success"]) + self.assertTrue( + any("local commit changed" in r for r in result["reasons"]), + result["reasons"], + ) + self.assertIsNone(self.remote_head(), "refusal must not publish") + + def test_abbreviated_sha_refuses(self): + self.write_lock() + result = self.run_publish(expected_head=self.head_sha[:8]) + self.assertFalse(result["success"]) + self.assertTrue( + any("40-character" in r for r in result["reasons"]), result["reasons"] + ) + + def test_dirty_tracked_worktree_refuses(self): + self.write_lock() + with open(os.path.join(self.repo, "work.txt"), "a") as fh: + fh.write("uncommitted edit\n") + + result = self.run_publish() + + self.assertFalse(result["success"]) + self.assertTrue( + any("dirty tracked files" in r for r in result["reasons"]), + result["reasons"], + ) + self.assertIn("work.txt", result["evidence"]["dirty_tracked_files"]) + self.assertIsNone(self.remote_head()) + + def test_untracked_file_refuses(self): + self.write_lock() + with open(os.path.join(self.repo, "stray.txt"), "w") as fh: + fh.write("not committed\n") + + result = self.run_publish() + + self.assertFalse(result["success"]) + self.assertTrue( + any("untracked files" in r for r in result["reasons"]), result["reasons"] + ) + self.assertIn("stray.txt", result["evidence"]["untracked_files"]) + self.assertIsNone(self.remote_head()) + + def test_unexpected_remote_head_refuses(self): + """A remote head that is not an ancestor must never be overwritten.""" + self.write_lock() + # Publish a divergent commit to the branch from a separate line. + self._git("checkout", "-q", "-b", "divergent", self.base_sha) + with open(os.path.join(self.repo, "other.txt"), "w") as fh: + fh.write("someone else's work\n") + self._git("add", "other.txt") + self._git("commit", "-q", "-m", "divergent") + divergent = self._git("rev-parse", "HEAD").stdout.strip() + self._git("push", "-q", GIT_REMOTE, f"{divergent}:refs/heads/{BRANCH}") + self._git("checkout", "-q", BRANCH) + + result = self.run_publish() + + self.assertFalse(result["success"]) + self.assertTrue( + any("not an ancestor" in r for r in result["reasons"]), result["reasons"] + ) + self.assertEqual( + self.remote_head(), divergent, "the other head must survive intact" + ) + + def test_fast_forward_remote_head_is_allowed(self): + """An ancestor head is an honest fast-forward, not a conflict.""" + self.write_lock() + self._git("push", "-q", GIT_REMOTE, f"{self.base_sha}:refs/heads/{BRANCH}") + + result = self.run_publish() + + self.assertTrue(result["success"], result.get("reasons")) + self.assertTrue(result["evidence"]["fast_forward_from_remote"]) + self.assertEqual(self.remote_head(), self.head_sha) + + def test_content_hash_mismatch_refuses(self): + self.write_lock() + wrong = {"work.txt": "0" * 64} + + result = self.run_publish(expected_file_hashes=wrong) + + self.assertFalse(result["success"]) + self.assertTrue( + any("declared content hashes" in r for r in result["reasons"]), + result["reasons"], + ) + self.assertFalse(result["evidence"]["file_hashes_verified"]) + self.assertIsNone(self.remote_head()) + + def test_matching_content_hashes_publish(self): + self.write_lock() + digests = branch_publish.hash_worktree_files(self.worktree, ["work.txt"]) + + result = self.run_publish(expected_file_hashes=digests) + + self.assertTrue(result["success"], result.get("reasons")) + self.assertTrue(result["evidence"]["file_hashes_verified"]) + + def test_missing_declared_file_refuses(self): + self.write_lock() + result = self.run_publish(expected_file_hashes={"absent.txt": "0" * 64}) + self.assertFalse(result["success"]) + self.assertTrue( + any("missing or unreadable" in r for r in result["reasons"]), + result["reasons"], + ) + + def test_foreign_claimant_refuses(self): + """Ownership comes from the durable record, not from the caller.""" + self.write_lock(claimant={"username": "someone-else", "profile": PROFILE}) + + result = self.run_publish() + + self.assertFalse(result["success"]) + self.assertTrue( + any("foreign claim" in r for r in result["reasons"]), result["reasons"] + ) + self.assertIsNone(self.remote_head()) + + def test_foreign_profile_refuses(self): + self.write_lock( + claimant={"username": IDENTITY, "profile": "test-reviewer-prgs"} + ) + result = self.run_publish() + self.assertFalse(result["success"]) + self.assertTrue( + any("claimant profile" in r for r in result["reasons"]), result["reasons"] + ) + + def test_absent_lock_record_refuses(self): + """No recorded claim means this cannot be used to bypass the lock.""" + result = self.run_publish() # no write_lock() + + self.assertFalse(result["success"]) + self.assertTrue( + any("no durable issue-lock record" in r for r in result["reasons"]), + result["reasons"], + ) + self.assertIsNone(self.remote_head()) + + def test_branch_mismatch_against_lock_refuses(self): + self.write_lock(branch_name=f"feat/issue-{ISSUE}-different") + result = self.run_publish() + self.assertFalse(result["success"]) + self.assertTrue( + any("records branch" in r for r in result["reasons"]), result["reasons"] + ) + + def test_worktree_mismatch_against_lock_refuses(self): + self.write_lock(worktree_path="/tmp/some/other/worktree") + result = self.run_publish() + self.assertFalse(result["success"]) + self.assertTrue( + any("records worktree" in r for r in result["reasons"]), result["reasons"] + ) + + def test_competing_open_pr_on_another_branch_refuses(self): + self.write_lock() + competing = [{"number": 4242, "head": {"ref": f"fix/issue-{ISSUE}-rival"}}] + + result = self.run_publish(open_prs=competing) + + self.assertFalse(result["success"]) + self.assertTrue( + any("already claim issue" in r for r in result["reasons"]), + result["reasons"], + ) + self.assertIsNone(self.remote_head()) + + def test_open_pr_on_the_same_branch_is_not_competing(self): + """This branch's own PR is not a rival claim against itself.""" + self.write_lock() + own = [{"number": 77, "head": {"ref": BRANCH}}] + + result = self.run_publish(open_prs=own) + + self.assertTrue(result["success"], result.get("reasons")) + + +class TestGuardStrictnessPreserved(_PublishBase): + """AC15 — publication is an operation, never a weakening of the guards.""" + + def test_non_issue_branch_refuses(self): + self._git("checkout", "-q", "-b", "scratch/not-issue-linked") + self.write_lock(branch_name="scratch/not-issue-linked") + + result = self.run_publish(branch_name="scratch/not-issue-linked") + + self.assertFalse(result["success"]) + self.assertTrue( + any("issue-linked" in r for r in result["reasons"]), result["reasons"] + ) + + def test_stable_branch_refuses(self): + self.write_lock(branch_name="master") + result = self.run_publish(branch_name="master") + self.assertFalse(result["success"]) + self.assertTrue( + any("issue-linked" in r or "stable branch" in r for r in result["reasons"]), + result["reasons"], + ) + + def test_branch_number_must_match_the_issue(self): + other = "feat/issue-7777-mismatched" + self._git("checkout", "-q", "-b", other) + self.write_lock(branch_name=other) + result = self.run_publish(branch_name=other) + self.assertFalse(result["success"]) + self.assertTrue( + any("does not carry issue number" in r for r in result["reasons"]), + result["reasons"], + ) + + def test_unregistered_worktree_refuses(self): + """#713 — an improvised directory is not a registered worktree.""" + path = self.write_lock() + assessment = branch_publish.assess_unpublished_commit_publication( + issue_lock_store.read_lock_file(path), + issue_number=ISSUE, branch_name=BRANCH, worktree_path=self.worktree, + expected_head=self.head_sha, remote="prgs", org=ORG, repo=REPO, + identity=IDENTITY, profile=PROFILE, + worktree_state={ + "current_branch": BRANCH, "porcelain_status": "", + "head_sha": self.head_sha, + }, + worktree_registered=False, + remote_probe={"probe_ok": True, "remote_branch_exists": False}, + ) + self.assertEqual(assessment["outcome"], branch_publish.REFUSED) + self.assertTrue( + any("not listed in git worktree list" in r + for r in assessment["reasons"]), + assessment["reasons"], + ) + + def test_unobservable_remote_refuses(self): + """An unknown remote state must not be mistaken for an absent branch.""" + self.write_lock() + result = self.run_publish(git_remote_name="no-such-remote") + self.assertFalse(result["success"]) + self.assertTrue( + any("could not be observed" in r for r in result["reasons"]), + result["reasons"], + ) + + +class TestRecordSeparation(_PublishBase): + """AC23 — the durable issue lock and the workflow lease are distinct.""" + + def test_publication_leaves_the_issue_lock_byte_identical(self): + path = self.write_lock() + with open(path, "rb") as fh: + before = fh.read() + + result = self.run_publish() + + self.assertTrue(result["success"], result.get("reasons")) + with open(path, "rb") as fh: + after = fh.read() + self.assertEqual(before, after, "publication must not mutate the lock record") + self.assertFalse(result["issue_lock_record_mutated"]) + self.assertFalse(result["workflow_lease_touched"]) + + def test_refusal_also_reports_untouched_records(self): + result = self.run_publish() # refuses: no lock record + self.assertFalse(result["issue_lock_record_mutated"]) + self.assertFalse(result["workflow_lease_touched"]) + + def test_lock_generation_is_not_advanced(self): + path = self.write_lock() + self.run_publish() + lock = issue_lock_store.read_lock_file(path) + self.assertEqual(lock["lock_generation"], 1) + + +class TestTruthfulProcessEvidence(_PublishBase): + """AC24 — a live daemon pid is never represented as a dead process.""" + + def test_live_recorded_pid_does_not_block_publication(self): + # The recorded pid is this live process, standing in for the live MCP + # daemon. Reclaim would refuse here; publication legitimately does not. + path = self.write_lock(session_pid=os.getpid()) + lock = issue_lock_store.read_lock_file(path) + self.assertEqual(lock["pid"], os.getpid()) + + result = self.run_publish() + + self.assertTrue(result["success"], result.get("reasons")) + self.assertEqual(self.remote_head(), self.head_sha) + + def test_liveness_is_not_consulted_as_evidence(self): + self.write_lock(session_pid=os.getpid()) + result = self.run_publish() + self.assertFalse(result["evidence"]["owner_pid_liveness_consulted"]) + + def test_reclaim_still_refuses_for_the_same_live_pid(self): + """Publication does not soften the reclaim predicate it routes around.""" + path = self.write_lock(session_pid=os.getpid()) + lock = issue_lock_store.read_lock_file(path) + reclaim = issue_lock_store.assess_expired_lock_reclaim(lock) + self.assertFalse(reclaim["reclaim_allowed"]) + + +class TestIdempotentRetry(_PublishBase): + """AC20 — retry is safe and read-after-write is proven every time.""" + + def test_second_publication_reports_already_published(self): + self.write_lock() + first = self.run_publish() + self.assertTrue(first["performed"]) + + second = self.run_publish() + + self.assertTrue(second["success"], second.get("reasons")) + self.assertFalse(second["performed"], "no second push is needed") + self.assertTrue(second["published"]) + self.assertTrue(second["verified"]) + self.assertEqual(second["outcome"], branch_publish.ALREADY_PUBLISHED) + self.assertEqual(self.remote_head(), self.head_sha) + + +class TestDryRun(_PublishBase): + """AC12 — dry run reports the decision and mutates nothing.""" + + def test_dry_run_reports_intent_without_publishing(self): + self.write_lock() + + result = self.run_publish(dry_run=True) + + self.assertTrue(result["success"]) + self.assertTrue(result["dry_run"]) + self.assertTrue(result["would_publish"]) + self.assertFalse(result["performed"]) + self.assertIsNone(self.remote_head(), "dry run must not publish") + + def test_dry_run_and_apply_agree_on_a_refusal(self): + """AC11 — the reported decision does not depend on which mode ran.""" + self.write_lock(claimant={"username": "someone-else", "profile": PROFILE}) + + dry = self.run_publish(dry_run=True) + applied = self.run_publish() + + self.assertFalse(dry["success"]) + self.assertFalse(applied["success"]) + self.assertEqual(dry["reasons"], applied["reasons"]) + + +class TestRenewalUnblocked(_PublishBase): + """AC20/AC21 — renewal is permitted only after verified publication.""" + + def _renewal(self, remote_head): + return issue_lock_renewal.assess_exact_owner_lease_renewal( + issue_lock_store.read_lock_file(self.lock_path()), + issue_number=ISSUE, branch_name=BRANCH, worktree_path=self.worktree, + remote="prgs", org=ORG, repo=REPO, + identity=IDENTITY, profile=PROFILE, + current_branch=BRANCH, porcelain_status="", worktree_exists=True, + head_sha=self.head_sha, remote_head_sha=remote_head, + ) + + def test_renewal_refuses_before_publication(self): + self.write_lock() + decision = self._renewal(None) + self.assertFalse(decision["renewal_sanctioned"]) + self.assertTrue( + any("unpublished branch" in r for r in decision["reasons"]), + decision["reasons"], + ) + + def test_renewal_is_sanctioned_after_publication(self): + self.write_lock() + result = self.run_publish() + self.assertTrue(result["verified"], result.get("reasons")) + + decision = self._renewal(self.remote_head()) + + self.assertTrue(decision["renewal_sanctioned"], decision["reasons"]) + + +class TestProtectedAssetUntouched(unittest.TestCase): + """AC17 — no test or fixture may reference the protected worktree.""" + + def test_no_reference_to_the_protected_worktree(self): + here = os.path.dirname(os.path.abspath(__file__)) + root = os.path.dirname(here) + needle = "issue-635-project-registry" + "-api" + for path in ( + os.path.join(here, "test_issue_812_publish_unpublished_commit.py"), + os.path.join(root, "branch_publish.py"), + ): + with open(path, "r", encoding="utf-8") as fh: + body = fh.read() + self.assertNotIn(needle, body) + + +if __name__ == "__main__": # pragma: no cover + unittest.main() diff --git a/tests/test_task_capability_role_invariants.py b/tests/test_task_capability_role_invariants.py index df69e9e..fbba796 100644 --- a/tests/test_task_capability_role_invariants.py +++ b/tests/test_task_capability_role_invariants.py @@ -139,6 +139,9 @@ EXPECTED_ROLE_EXCLUSIVE_TASKS = frozenset( "gitea_release_merger_pr_lease", "create_branch", "push_branch", + # #812 AC20: publishing an unpublished local head is author-only for the + # same reason every other push is — it writes a branch to the remote. + "publish_unpublished_branch", "create_pr", "commit_files", "gitea_commit_files", From 95a5eb254f7758b694a26b3f5629558cb88db328 Mon Sep 17 00:00:00 2001 From: jcwalker3 Date: Wed, 22 Jul 2026 17:27:49 -0500 Subject: [PATCH 3/3] fix(mcp): forward worktree_path into publication preflight (Closes #815) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gitea_publish_unpublished_issue_branch accepted a required worktree_path but resolved it only after verify_preflight_purity had already run, so the #618 branches-only guard and every other workspace-resolution layer behind preflight received None and fell back to the MCP process root. A daemon rooted at the stable control checkout therefore refused a valid registered issue worktree the caller had explicitly supplied, before the publication assessor could use it — the sole verify_preflight_purity call site that accepted a worktree argument and dropped it. Resolve the workspace once, before preflight, and forward it. A blank or absent path forwards None and keeps the ordinary #618 fail-closed fallback, so guard strictness is unchanged for missing, empty, unregistered, foreign, or control-checkout worktrees. Public tool contract, ownership, cleanliness, hash, ancestry, and read-after-write protections from PR #814 are untouched. Adds tests/test_issue_815_preflight_worktree_forwarding.py: a #735-style capture proving the argument reaches verify_preflight_purity, a faithful production reproduction (control-rooted daemon, no session lock, explicit worktree) that clears #618 on the fixed source and is trapped at #618 on the unpatched source, an end-to-end control-rooted publication in the real topology (PROJECT_ROOT is the stable control checkout, the issue worktree is a distinct registered path, production guards forced on), and negative coverage keeping every #618 and assessor refusal intact. The prior #812 suite masked the defect by patching PROJECT_ROOT to equal the issue worktree. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 20 +- ...issue_815_preflight_worktree_forwarding.py | 622 ++++++++++++++++++ 2 files changed, 640 insertions(+), 2 deletions(-) create mode 100644 tests/test_issue_815_preflight_worktree_forwarding.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index f71360b..acd56ca 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -9173,11 +9173,27 @@ def gitea_publish_unpublished_issue_branch( if blocked: return blocked - verify_preflight_purity(remote, task=task, org=org, repo=repo) + # #815: resolve the caller's worktree *before* preflight and forward it, so + # every workspace-resolution layer behind verify_preflight_purity — including + # the #618 branches-only guard — judges the registered issue worktree this + # publication actually operates on. Resolving it afterwards let preflight + # fall back to the MCP process root, so a daemon rooted at the stable control + # checkout refused a valid explicit worktree before the assessor ever ran. + # A caller supplying nothing usable forwards None and keeps the ordinary + # fail-closed fallback. + explicit_worktree = (worktree_path or "").strip() + workspace = os.path.realpath(os.path.abspath(explicit_worktree or ".")) + + verify_preflight_purity( + remote, + worktree_path=workspace if explicit_worktree else None, + task=task, + org=org, + repo=repo, + ) h, o, r = _resolve(remote, host, org, repo) git_remote = (git_remote_name or remote or "").strip() - workspace = os.path.realpath(os.path.abspath((worktree_path or "").strip() or ".")) existing_lock = issue_lock_store.load_issue_lock( remote=remote, org=o, repo=r, issue_number=int(issue_number) diff --git a/tests/test_issue_815_preflight_worktree_forwarding.py b/tests/test_issue_815_preflight_worktree_forwarding.py new file mode 100644 index 0000000..8cc97b2 --- /dev/null +++ b/tests/test_issue_815_preflight_worktree_forwarding.py @@ -0,0 +1,622 @@ +"""Publication preflight must receive the caller's worktree (#815). + +``gitea_publish_unpublished_issue_branch`` takes a **required** ``worktree_path`` +but resolved it only *after* ``verify_preflight_purity`` had already run. Every +workspace-resolution layer behind that preflight — canonical root, root checkout, +create-issue bootstrap, the #618 branches-only guard, issue scope, and anti-stomp +— therefore received ``None`` and fell back to the MCP process root. A daemon +rooted at the stable control checkout refused a valid registered issue worktree +that the caller had explicitly supplied, before the publication assessor ever ran. + +The #812 suite could not see this. Its fixture sets ``self.worktree = +os.path.realpath(self.repo)`` and patches ``PROJECT_ROOT`` to that same path, so +the fallback resolved to the very worktree the argument named. The production +topology — control checkout on a stable branch, issue worktree somewhere else — +was never constructed, and preflight additionally no-ops under pytest unless +production guards are forced on. + +These tests build that topology honestly: + +* ``PROJECT_ROOT`` is a control checkout sitting on ``master``; +* the registered issue worktree is a genuinely separate path under ``branches/``; +* ``GITEA_TEST_FORCE_PRODUCTION_GUARDS`` is set so the #618 guard really runs; +* no patch makes the issue worktree appear to be ``PROJECT_ROOT``. + +Nothing here reads, writes, or references the protected worktree named in #812 +AC17 and #815 AC9. Every fixture is built from scratch against a local bare +remote, so publication and read-after-write verification genuinely execute. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from datetime import datetime, timedelta, timezone +from unittest.mock import patch + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import issue_lock_provenance # noqa: E402 +import issue_lock_store # noqa: E402 +import mcp_server # noqa: E402 +from mutation_profile_fixture import shared_mutation_env # noqa: E402 + +ISSUE = 9815 +BRANCH = f"feat/issue-{ISSUE}-forwarding-fixture" +WORKTREE_DIRNAME = BRANCH.replace("/", "-") +IDENTITY = "example-user" +PROFILE = "test-author-prgs" +ORG = "Scaled-Tech-Consulting" +REPO = "Gitea-Tools" +GIT_REMOTE = "prgs" + +AUTHOR_PROFILE = { + "profile_name": "prgs-author", + "role": "author", + "allowed_operations": [ + "gitea.read", "gitea.issue.create", "gitea.issue.comment", + "gitea.pr.create", "gitea.repo.commit", "gitea.branch.push", + ], + "forbidden_operations": [], + "audit_label": "prgs-author", +} + + +def _ts(hours: int) -> str: + return ( + (datetime.now(timezone.utc) + timedelta(hours=hours)) + .isoformat() + .replace("+00:00", "Z") + ) + + +class TestPreflightReceivesTheWorktree(unittest.TestCase): + """AC1 — the supplied path reaches ``verify_preflight_purity`` itself. + + Follows the #735 capture pattern: replace preflight with a recorder that + raises, so the argument can be proven forwarded without performing the + mutation. This is the direct unit-level statement of the defect. + """ + + def _capture_preflight(self, **kwargs): + captured: dict = {} + + def _capture(*a, **kw): + captured.update(kw) + captured["_args"] = a + raise RuntimeError("capture-only") + + with patch.object( + mcp_server, "verify_preflight_purity", side_effect=_capture + ), patch.object( + mcp_server, "get_profile", return_value=AUTHOR_PROFILE + ), patch.object( + mcp_server, "_resolve", + return_value=("gitea.prgs.cc", ORG, REPO), + ), patch.object( + mcp_server, "_auth", return_value="token fake", + ), patch.object( + mcp_server.role_session_router, + "check_author_mutation_after_reviewer_stop", + return_value=(True, []), + ), patch.object( + mcp_server, "_namespace_mutation_block", return_value=None + ), patch.object( + mcp_server, "_profile_permission_block", return_value=None + ): + try: + mcp_server.gitea_publish_unpublished_issue_branch(**kwargs) + except RuntimeError as exc: + if "capture-only" not in str(exc) and not captured: + raise + self.assertTrue( + captured, + "gitea_publish_unpublished_issue_branch never called " + "verify_preflight_purity", + ) + return captured + + def _base_kwargs(self, **overrides): + kwargs = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "worktree_path": "/tmp/issue-815-explicit-worktree", + "expected_head": "a" * 40, + "remote": "prgs", + "org": ORG, + "repo": REPO, + "git_remote_name": GIT_REMOTE, + } + kwargs.update(overrides) + return kwargs + + def test_explicit_worktree_path_reaches_preflight(self): + captured = self._capture_preflight(**self._base_kwargs()) + self.assertEqual( + captured.get("worktree_path"), + os.path.realpath(os.path.abspath("/tmp/issue-815-explicit-worktree")), + "the authoritative worktree_path must be forwarded into preflight", + ) + + def test_forwarded_path_is_the_one_publication_uses(self): + """AC4 — preflight and publication must judge the same resolved path.""" + raw = "/tmp/issue-815-explicit-worktree/./" + captured = self._capture_preflight(**self._base_kwargs(worktree_path=raw)) + expected = os.path.realpath(os.path.abspath(raw.strip())) + self.assertEqual(captured.get("worktree_path"), expected) + + def test_blank_worktree_path_forwards_none(self): + """AC5/AC8 — nothing usable supplied keeps the fail-closed fallback.""" + for blank in ("", " "): + with self.subTest(blank=repr(blank)): + captured = self._capture_preflight( + **self._base_kwargs(worktree_path=blank) + ) + self.assertIsNone( + captured.get("worktree_path"), + "a blank worktree must not resolve to the process cwd", + ) + + def test_org_repo_and_task_forwarding_are_not_regressed(self): + """AC6 — #735's org/repo forwarding and the task name still hold.""" + captured = self._capture_preflight(**self._base_kwargs()) + self.assertEqual(captured.get("org"), ORG) + self.assertEqual(captured.get("repo"), REPO) + self.assertEqual(captured.get("task"), "publish_unpublished_branch") + + +class _ProductionTopologyBase(unittest.TestCase): + """Control checkout on master + a distinct registered issue worktree. + + This is the shape the production daemon runs in and the shape the #812 + fixture never built. ``PROJECT_ROOT`` is the control checkout; the issue + worktree is a real registered worktree at a different path; production + guards are forced on so the #618 branches-only guard genuinely evaluates. + """ + + def setUp(self): + self.lock_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.lock_dir.cleanup) + self.origin = tempfile.mkdtemp(prefix="issue815-origin-") + self.control = tempfile.mkdtemp(prefix="issue815-control-") + for path in (self.origin, self.control): + self.addCleanup( + lambda p=path: subprocess.run(["rm", "-rf", p], check=False) + ) + self._init_repos() + self.remotes = patch.dict( + mcp_server.REMOTES, + {"prgs": {"host": "gitea.prgs.cc", "org": ORG, "repo": REPO}}, + ) + self.remotes.start() + self.addCleanup(patch.stopall) + mcp_server._IDENTITY_CACHE.clear() + + def _git(self, *args, cwd=None): + return subprocess.run( + ["git", "-C", cwd or self.control, *args], + capture_output=True, text=True, check=True, + ) + + def _init_repos(self): + subprocess.run( + ["git", "init", "-q", "--bare", "-b", "master", self.origin], check=True + ) + self._git("init", "-q", "-b", "master") + self._git("config", "user.email", "test@example.com") + self._git("config", "user.name", "Test") + self._git("remote", "add", GIT_REMOTE, self.origin) + + with open(os.path.join(self.control, "seed.txt"), "w") as fh: + fh.write("seed\n") + # The real repository gitignores branches/, so a registered worktree + # living there does not dirty the stable control checkout. Mirror that, + # or the #615 dirty-runtime block fires on the worktree we just created. + with open(os.path.join(self.control, ".gitignore"), "w") as fh: + fh.write("branches/\n") + self._git("add", "seed.txt", ".gitignore") + self._git("commit", "-q", "-m", "seed") + self.base_sha = self._git("rev-parse", "HEAD").stdout.strip() + self._git("push", "-q", GIT_REMOTE, "master") + + # The control checkout STAYS on master. This is the whole point: the + # daemon's process root is the stable control checkout, never the + # worktree the publication targets. + self.worktree = os.path.realpath( + os.path.join(self.control, "branches", WORKTREE_DIRNAME) + ) + self._git("worktree", "add", "-q", "-b", BRANCH, self.worktree, "master") + + with open(os.path.join(self.worktree, "work.txt"), "w") as fh: + fh.write("unpublished implementation\n") + self._git("add", "work.txt", cwd=self.worktree) + self._git("commit", "-q", "-m", "unpublished implementation", cwd=self.worktree) + self.head_sha = self._git("rev-parse", "HEAD", cwd=self.worktree).stdout.strip() + + self.control_branch = self._git( + "rev-parse", "--abbrev-ref", "HEAD" + ).stdout.strip() + + # ── durable lock naming the caller and the issue worktree ──────────── + def lock_path(self): + return issue_lock_store.lock_file_path( + remote="prgs", org=ORG, repo=REPO, issue_number=ISSUE, + lock_dir=self.lock_dir.name, + ) + + def write_lock(self, *, bind_session=True, **overrides): + path = self.lock_path() + claimant = overrides.pop( + "claimant", {"username": IDENTITY, "profile": PROFILE} + ) + pid = overrides.pop("session_pid", os.getpid()) + lease = { + "operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE, + "issue_number": ISSUE, + "pr_number": None, + "branch": overrides.get("branch_name", BRANCH), + "worktree_path": overrides.get("worktree_path", self.worktree), + "claimant": claimant, + "created_at": _ts(-2), + "last_heartbeat_at": _ts(-2), + "expires_at": _ts(-1), + } + lease.update(overrides.pop("work_lease", {})) + data = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "remote": "prgs", + "org": ORG, + "repo": REPO, + "worktree_path": self.worktree, + "session_pid": pid, + "pid": pid, + "lock_generation": 1, + "work_lease": lease, + "lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance( + tool="gitea_lock_issue", claimant=claimant + ), + } + data.update(overrides) + data["lock_file_path"] = path + issue_lock_store.save_lock_file(path, data) + # Bind the session pointer so the #683 issue-scope guard resolves an + # owning issue for this author session. In real production the publish + # task does not require a session lock — require_author_lock is keyed on + # the test-only production_guards_forced() flag, which this suite must + # set to make preflight run at all — so this pointer is fixture + # scaffolding to clear a guard production would not apply here, never a + # softening of the worktree-forwarding behaviour under test. The + # preflight-negative cases below leave it unbound precisely so the #618 + # guard is reached with no session fallback to rescue a bad worktree. + if bind_session: + pointer = { + "pid": os.getpid(), + "lock_file_path": path, + "issue_number": ISSUE, + "branch_name": data["branch_name"], + "remote": "prgs", + "org": ORG, + "repo": REPO, + } + issue_lock_store.save_lock_file( + issue_lock_store.session_pointer_path(self.lock_dir.name), pointer + ) + return path + + def _tool_env(self): + env = shared_mutation_env( + PROFILE, include_example_repo=True, + GITEA_ISSUE_LOCK_DIR=self.lock_dir.name, + ) + env["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + # The defect only exists where preflight actually runs. Under pytest the + # production root/branches/scope guards are skipped unless forced on, so + # force them: this test exists to exercise the #618 guard, not to bypass + # it. Parity is pinned to the server's own startup head so repointing + # PROJECT_ROOT does not read as a stale daemon. + env["GITEA_TEST_FORCE_PRODUCTION_GUARDS"] = "1" + # Production is a promoted stable-control runtime. The pytest process + # itself runs from a branches/ worktree, which the #615 runtime-mode + # gate correctly classifies as dev-test; declaring the sanctioned mode + # models the production daemon rather than defeating the gate. Without + # this, forcing production guards on would trip the *runtime-mode* block + # for a reason unrelated to the #815 worktree-forwarding defect. + env["GITEA_MCP_RUNTIME_MODE"] = "stable-control" + startup_head = mcp_server._STARTUP_PARITY.get("startup_head") or "" + env["GITEA_TEST_CURRENT_HEAD"] = startup_head + env["GITEA_TEST_LIVE_REMOTE_HEAD"] = startup_head + return env + + def run_publish(self, *, open_prs=None, expected_head=None, **kwargs): + """Drive the public tool with PROJECT_ROOT pinned to the CONTROL checkout.""" + env = self._tool_env() + with patch( + "mcp_server._list_open_pulls", return_value=list(open_prs or []) + ), patch( + "mcp_server._auth", return_value="token x" + ), patch( + "mcp_server.get_auth_header", return_value="token x" + ), patch( + "mcp_server._work_lease_claimant", + return_value={"username": IDENTITY, "profile": PROFILE}, + ), patch.object( + # NOTE: the control checkout — deliberately NOT self.worktree. + mcp_server, "PROJECT_ROOT", self.control + ), patch.dict(os.environ, env, clear=True): + os.environ["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + return mcp_server.gitea_publish_unpublished_issue_branch( + issue_number=kwargs.pop("issue_number", ISSUE), + branch_name=kwargs.pop("branch_name", BRANCH), + worktree_path=kwargs.pop("worktree_path", self.worktree), + expected_head=expected_head or self.head_sha, + remote="prgs", + git_remote_name=kwargs.pop("git_remote_name", GIT_REMOTE), + **kwargs, + ) + + def remote_head(self, branch=BRANCH): + res = subprocess.run( + ["git", "-C", self.origin, "rev-parse", "--verify", "--quiet", branch], + capture_output=True, text=True, check=False, + ) + return (res.stdout or "").strip() or None + + +class TestForwardingClearsThe618Guard(_ProductionTopologyBase): + """AC2 — the faithful production reproduction, and the sharpest fix proof. + + The production recovery worker had **no** session issue lock — acquiring one + was the very thing the deadlock prevented — so preflight had nothing but the + explicit ``worktree_path`` argument to resolve the workspace from. This class + reproduces exactly that: no session pointer is bound, so there is no + author-lock fallback to rescue a dropped argument. + + With the argument forwarded (fixed source) the #618 branches-only guard + accepts the registered issue worktree and the call advances to the next + guard. With the argument dropped (the buggy source this issue reports) + preflight falls back to ``PROJECT_ROOT`` — the stable control checkout — and + the #618 guard traps the call there. The two outcomes are told apart by the + guard that fired, on its own error text. + + This test therefore *fails* against the unpatched source (the call is trapped + at #618 instead of clearing it), which is what makes it a regression rather + than a smoke test. + """ + + _CONTROL_CHECKOUT_MARKERS = ("stable control checkout", "#618") + + def test_explicit_worktree_clears_618_without_a_session_lock(self): + # No write_lock(): the session is deliberately unbound, as in production. + with self.assertRaises(RuntimeError) as ctx: + self.run_publish() + message = str(ctx.exception) + # The workspace guard is satisfied — the failure is the *later* scope + # guard (no owning issue), never the control-checkout refusal. If the + # argument were dropped, this call would be trapped at #618 instead. + for marker in self._CONTROL_CHECKOUT_MARKERS: + self.assertNotIn( + marker, message, + f"the explicit worktree must clear #618; got a control-checkout " + f"refusal instead: {message}", + ) + self.assertIn( + "owning issue", message, + f"expected the downstream scope guard to fire, got: {message}", + ) + self.assertIsNone(self.remote_head()) + + def test_dropped_argument_would_be_trapped_at_618(self): + # Simulate the buggy call shape directly: no session lock, and preflight + # given no worktree, exactly as the unpatched source left it. This pins + # the control-checkout refusal that the fix eliminates, so the pair of + # tests brackets the defect from both sides regardless of which source + # version is loaded. + env = self._tool_env() + with patch( + "mcp_server._list_open_pulls", return_value=[] + ), patch( + "mcp_server._auth", return_value="token x" + ), patch( + "mcp_server.get_auth_header", return_value="token x" + ), patch( + "mcp_server._work_lease_claimant", + return_value={"username": IDENTITY, "profile": PROFILE}, + ), patch.object( + mcp_server, "PROJECT_ROOT", self.control + ), patch.dict(os.environ, env, clear=True): + os.environ["GITEA_ISSUE_LOCK_DIR"] = self.lock_dir.name + with self.assertRaises(RuntimeError) as ctx: + # Drive verify_preflight_purity the way the buggy body did: + # no worktree_path forwarded at all. + mcp_server.verify_preflight_purity( + "prgs", + task="publish_unpublished_branch", + org=ORG, + repo=REPO, + ) + message = str(ctx.exception) + self.assertTrue( + any(m in message for m in self._CONTROL_CHECKOUT_MARKERS), + f"a dropped worktree must trap at the control checkout: {message}", + ) + self.assertIsNone(self.remote_head()) + + +class TestProductionTopologyPublishes(_ProductionTopologyBase): + """AC2/AC4/AC7 — the explicit registered worktree is what preflight validates.""" + + def test_fixture_is_genuinely_the_production_topology(self): + """Guard the guard: if this drifts, the regression stops meaning anything.""" + self.assertNotEqual( + os.path.realpath(self.control), self.worktree, + "the issue worktree must not be PROJECT_ROOT", + ) + self.assertEqual( + self.control_branch, "master", + "the control checkout must sit on a stable branch", + ) + self.assertTrue( + os.path.realpath(self.worktree).startswith( + os.path.realpath(os.path.join(self.control, "branches")) + os.sep + ), + "the issue worktree must live under branches/", + ) + listed = subprocess.run( + ["git", "-C", self.control, "worktree", "list"], + capture_output=True, text=True, check=True, + ).stdout + self.assertIn( + self.worktree, listed, "the issue worktree must be genuinely registered" + ) + + def test_publishes_from_a_control_rooted_daemon(self): + """The exact production failure: this refused with #618 before the fix.""" + self.write_lock() + self.assertIsNone(self.remote_head(), "fixture must start unpublished") + res = self.run_publish() + self.assertTrue(res.get("success"), res) + self.assertTrue(res.get("performed"), res) + self.assertEqual(self.remote_head(), self.head_sha) + + def test_dry_run_uses_the_explicit_worktree(self): + """AC4 — dry-run reaches the same decision without publishing.""" + self.write_lock() + res = self.run_publish(dry_run=True) + self.assertTrue(res.get("success"), res) + self.assertFalse(res.get("performed"), res) + self.assertTrue(res.get("would_publish"), res) + self.assertIsNone(self.remote_head(), "dry-run must not publish") + + def test_dry_run_and_apply_agree_on_the_same_worktree(self): + """AC4 — both paths resolve the same workspace, so both succeed.""" + self.write_lock() + dry = self.run_publish(dry_run=True) + self.assertTrue(dry.get("would_publish"), dry) + applied = self.run_publish() + self.assertTrue(applied.get("performed"), applied) + self.assertEqual(self.remote_head(), self.head_sha) + + def test_read_after_write_verification_still_runs(self): + """AC6 — PR #814's post-publication verification is unchanged.""" + self.write_lock() + res = self.run_publish() + self.assertTrue(res.get("verified"), res) + self.assertEqual(res.get("remote_head_sha"), self.head_sha) + + +class TestProductionTopologyFailsClosed(_ProductionTopologyBase): + """AC3/AC5/AC8 — the fix does not weaken any refusal. + + A refusal reaches the caller by one of two mechanisms, and this class holds + them apart deliberately. A bad *workspace* is caught by the #618 preflight + guard, which raises before the assessor is built. A bad *content/ownership* + fact passes preflight (the worktree itself is fine) and is then refused by + the publication assessor, which returns ``success: False``. Both are + fail-closed; asserting the wrong mechanism would hide a regression. + """ + + # ── #618 preflight refusals: no session lock, so nothing rescues a bad + # workspace and the guard fires exactly as it does in production ────── + def _assert_preflight_raises(self, **kwargs): + with self.assertRaises(RuntimeError) as ctx: + self.run_publish(**kwargs) + self.assertIsNone( + self.remote_head(), "a blocked publication must not reach the remote" + ) + return str(ctx.exception) + + def test_blank_worktree_path_fails_closed_via_618(self): + """AC5 — a blank path forwards None, so preflight sees the control root.""" + for blank in ("", " "): + with self.subTest(blank=repr(blank)): + message = self._assert_preflight_raises(worktree_path=blank) + self.assertIn("618", message) + + def test_control_checkout_as_worktree_fails_closed_via_618(self): + """AC5 — naming the stable control checkout explicitly is still refused.""" + message = self._assert_preflight_raises(worktree_path=self.control) + self.assertIn("618", message) + + def test_unregistered_directory_fails_closed(self): + """AC3 — a plain directory under branches/ is not a registered worktree.""" + bogus = os.path.join(self.control, "branches", "not-a-worktree") + os.makedirs(bogus, exist_ok=True) + self._assert_preflight_raises(worktree_path=bogus) + + def test_missing_worktree_path_fails_closed(self): + """AC3 — a path that does not exist is refused, not silently replaced.""" + missing = os.path.join(self.control, "branches", "absent-worktree") + self._assert_preflight_raises(worktree_path=missing) + + # ── assessor refusals: preflight passes on a valid worktree, then the + # publication assessor refuses on content/ownership evidence ────────── + def _assert_assessor_refuses(self, **kwargs): + res = self.run_publish(**kwargs) + self.assertFalse(res.get("success"), res) + self.assertFalse(res.get("performed"), res) + self.assertIsNone(self.remote_head()) + return res + + def test_changed_local_head_still_refuses(self): + """AC6 — the declared expected_head remains authoritative.""" + self.write_lock() + self._assert_assessor_refuses(expected_head="b" * 40) + + def test_foreign_claimant_still_refuses(self): + """AC6 — ownership still comes from the durable lock record.""" + self.write_lock(claimant={"username": "someone-else", "profile": PROFILE}) + self._assert_assessor_refuses() + + def test_dirty_worktree_still_refuses(self): + """AC6 — cleanliness enforcement survives the forwarding change.""" + self.write_lock() + with open(os.path.join(self.worktree, "work.txt"), "a") as fh: + fh.write("uncommitted drift\n") + self._assert_assessor_refuses() + + def test_competing_open_pr_still_refuses(self): + """AC6 — a rival claim on another branch still blocks.""" + self.write_lock() + self._assert_assessor_refuses( + open_prs=[{"number": 4242, "head": {"ref": f"fix/issue-{ISSUE}-rival"}}] + ) + + def test_issue_lock_record_is_not_mutated_by_a_refusal(self): + """AC6 — record separation (#812 AC23) is unaffected by this change.""" + path = self.write_lock() + with open(path, "rb") as fh: + before = fh.read() + self._assert_assessor_refuses(expected_head="c" * 40) + with open(path, "rb") as fh: + self.assertEqual(before, fh.read()) + + +class TestProtectedFixtureNotReferenced(unittest.TestCase): + """AC9 — this regression never names the protected #635 fixture. + + The forbidden tokens are reconstructed from fragments so this assertion + file does not itself contain them and produce a false positive. + """ + + def test_no_reference_to_the_protected_worktree(self): + forbidden = [ + "issue-635-" + "project-registry-api", + "b2f6e9a6dc40e9651ef8" + "76f322dd0a68bddebfd8", + ] + here = os.path.abspath(__file__) + with open(here, "r", encoding="utf-8") as fh: + text = fh.read() + for token in forbidden: + self.assertNotIn( + token, text, + f"the protected #635 fixture must not be referenced: {token}", + ) + + +if __name__ == "__main__": + unittest.main()