From 772b6485fb5e3ef8d32b39970ac9390f7d70cd04 Mon Sep 17 00:00:00 2001 From: David Hyrule Date: Tue, 21 Jul 2026 22:16:45 +0200 Subject: [PATCH 1/2] Stop repeated LHP decisions and queue starvation --- src/hyrule_engineering_loop/daemon.py | 107 +++++++++++----- src/hyrule_engineering_loop/governor.py | 162 ++++++++++++++++++------ src/hyrule_engineering_loop/lhp.py | 32 ++++- tests/test_phase24_daemon.py | 100 ++++++++++++++- tests/test_phase28_lhp.py | 73 ++++++++++- tests/test_phase29_governor.py | 161 ++++++++++++++++++++--- 6 files changed, 540 insertions(+), 95 deletions(-) diff --git a/src/hyrule_engineering_loop/daemon.py b/src/hyrule_engineering_loop/daemon.py index c49f088..e1a00a2 100644 --- a/src/hyrule_engineering_loop/daemon.py +++ b/src/hyrule_engineering_loop/daemon.py @@ -38,9 +38,11 @@ payload_hash, post_lhp_update, render_lhp_request, + validated_approval_scope, ) from hyrule_engineering_loop.intake import ( APPROVED_LABEL, + NEEDS_HUMAN_LABEL, GhClient, IntakeError, IntakeItem, @@ -70,7 +72,9 @@ "reliability-governor-cdr:", "loop-governor-cdr:", ) -RELIABILITY_DECISION_SCHEMA_VERSION = "reliability-governor.cdr.v1" +RELIABILITY_DECISION_SCHEMA_VERSIONS = frozenset( + {"reliability-governor.cdr.v1", "reliability-governor.cdr.v2"} +) REPO_CHECKOUT_NAMES: dict[str, str] = { "engineering-loop": "engineering-loop", @@ -189,6 +193,7 @@ class ReliabilityApprovalScope: record_id: str allowed_paths: tuple[str, ...] lhp_payload_hash: str | None = None + lhp_approval_scope_hash: str | None = None # --- lock --------------------------------------------------------------- @@ -482,7 +487,7 @@ def _approval_scope_from_record( *, current_body: str, ) -> tuple[ReliabilityApprovalScope | None, str | None]: - if payload.get("schema_version") != RELIABILITY_DECISION_SCHEMA_VERSION: + if payload.get("schema_version") not in RELIABILITY_DECISION_SCHEMA_VERSIONS: return None, "Reliability Decision Record schema version is unsupported" try: issue_number = int(payload.get("issue_number", -1)) @@ -510,6 +515,7 @@ def _approval_scope_from_record( record_id=str(payload.get("record_id", "unknown")), allowed_paths=allowed_paths, lhp_payload_hash=_record_lhp_payload_hash(payload), + lhp_approval_scope_hash=_record_lhp_approval_scope_hash(payload), ), None @@ -525,6 +531,16 @@ def _record_lhp_payload_hash(payload: dict[str, Any]) -> str | None: return value +def _record_lhp_approval_scope_hash(payload: dict[str, Any]) -> str | None: + lhp = payload.get("lhp") + if not isinstance(lhp, dict): + return None + value = str(lhp.get("approval_scope_hash") or "").strip().lower() + if len(value) != 64 or any(ch not in "0123456789abcdef" for ch in value): + return None + return value + + def _normalize_path_prefix(path: str) -> str: normalized = path.strip() if normalized.startswith("./"): @@ -597,21 +613,43 @@ def _approved_allowed_paths( return narrowed, scope, None -def _lhp_payload_hash_error( +def _lhp_approval_scope_hash_error( payload: dict[str, Any], scope: ReliabilityApprovalScope | None, ) -> str | None: if scope is None: return None - expected = scope.lhp_payload_hash + expected = scope.lhp_approval_scope_hash if expected is None: - return f"Reliability Decision Record {scope.record_id} has no LHP payload hash" - current = payload_hash(payload) - if current[: len(expected)] != expected: - return f"Reliability Decision Record {scope.record_id} LHP payload hash is stale" + return f"Reliability Decision Record {scope.record_id} has no LHP approval scope hash" + current = str(payload.get("approval_scope_hash") or "").strip().lower() + if current != expected: + return f"Reliability Decision Record {scope.record_id} LHP approval scope hash is stale" return None +def _park_issue_for_human(item: IntakeItem, *, client: GhClient) -> bool: + """Remove a completed attempt from the autonomous queue.""" + + try: + client.run( + [ + "issue", + "edit", + str(item.number), + "--repo", + item.repo, + "--remove-label", + APPROVED_LABEL, + "--add-label", + NEEDS_HUMAN_LABEL, + ] + ) + except Exception: + return False + return True + + def _change_id_for(item: IntakeItem) -> str: repo_slug = item.repo.rsplit("/", 1)[-1].upper().replace("-", "_") return f"ISSUE_{repo_slug}_{item.number}" @@ -701,6 +739,7 @@ def daemon_once( ) -> DaemonReport: """Run one autonomous cycle: pick one approved item, run, publish or journal.""" started = time.monotonic() + item: IntakeItem | None = None if os.environ.get("GITHUB_ACTIONS"): return _finish( DaemonReport( @@ -762,6 +801,7 @@ def daemon_once( trusted_authors=config.reliability_decision_authors, ) if approval_error is not None or effective_allowed_paths is None: + _park_issue_for_human(item, client=client) return _finish( DaemonReport( outcome="needs_triage", @@ -776,23 +816,11 @@ def daemon_once( lhp_pointer = parse_lhp_pointer(body) lhp_payload: dict[str, Any] | None = None if lhp_pointer is not None: - post_lhp_update( - lhp_pointer, - lhp_config, - update_type="accepted", - status="accepted", - summary=f"Engineering Loop accepted approved issue {item.repo}#{item.number}", - ) try: lhp_payload = fetch_lhp_payload(lhp_pointer, lhp_config) + validated_approval_scope(lhp_payload) except Exception as exc: - post_lhp_update( - lhp_pointer, - lhp_config, - update_type="blocked", - status="blocked", - summary=f"Could not fetch authoritative NOC LHP payload: {type(exc).__name__}: {exc}", - ) + _park_issue_for_human(item, client=client) return _finish( DaemonReport( outcome="needs_triage", @@ -803,15 +831,9 @@ def daemon_once( discord_poster, icinga_poster, ) - lhp_hash_error = _lhp_payload_hash_error(lhp_payload, approval_scope) + lhp_hash_error = _lhp_approval_scope_hash_error(lhp_payload, approval_scope) if lhp_hash_error is not None: - post_lhp_update( - lhp_pointer, - lhp_config, - update_type="blocked", - status="blocked", - summary=lhp_hash_error, - ) + _park_issue_for_human(item, client=client) return _finish( DaemonReport( outcome="needs_triage", @@ -822,6 +844,28 @@ def daemon_once( discord_poster, icinga_poster, ) + handoff_state = lhp_payload.get("handoff") + current_status = str(handoff_state.get("status") or "") if isinstance(handoff_state, dict) else "" + if current_status != "requested": + detail = f"LHP handoff state is {current_status or 'missing'}, expected requested" + _park_issue_for_human(item, client=client) + return _finish( + DaemonReport( + outcome="needs_triage", + detail=detail, + issue={"repo": item.repo, "number": item.number, "title": item.title}, + change_id=change_id, + ), + discord_poster, + icinga_poster, + ) + post_lhp_update( + lhp_pointer, + lhp_config, + update_type="accepted", + status="accepted", + summary=f"Engineering Loop accepted approved issue {item.repo}#{item.number}", + ) post_lhp_update( lhp_pointer, lhp_config, @@ -921,6 +965,7 @@ def daemon_once( summary=report.detail, ) + _park_issue_for_human(item, client=client) report.wall_clock_seconds = time.monotonic() - started update_ledger( state_dir, @@ -930,6 +975,8 @@ def daemon_once( ) return _finish(report, discord_poster, icinga_poster) except Exception as exc: + if item is not None: + _park_issue_for_human(item, client=client) report = DaemonReport(outcome="error", detail=str(exc)[:300]) report.wall_clock_seconds = time.monotonic() - started return _finish(report, discord_poster, icinga_poster) diff --git a/src/hyrule_engineering_loop/governor.py b/src/hyrule_engineering_loop/governor.py index 6ae833f..14c4c46 100644 --- a/src/hyrule_engineering_loop/governor.py +++ b/src/hyrule_engineering_loop/governor.py @@ -42,7 +42,7 @@ INTAKE_LABEL = "loop:intake" DECISION_MARKER = "reliability-governor-cdr:" LEGACY_DECISION_MARKER = "loop-governor-cdr:" -CDR_SCHEMA_VERSION = "reliability-governor.cdr.v1" +CDR_SCHEMA_VERSION = "reliability-governor.cdr.v2" WAKE_EVENT_SCHEMA_VERSION: Literal["reliability-governor.wake.v1"] = "reliability-governor.wake.v1" GOVERNOR_NAME = "Reliability Governor" GOVERNOR_ROLE = "staff_sre_autonomous_operations" @@ -99,6 +99,7 @@ "non_prod_tooling", "internal_service_code", "provisioning_helper", + "infrastructure_config", "production_network", "customer_provisioning", "routing_policy", @@ -158,6 +159,7 @@ class LhpAuthoritySummary(BaseModel): handoff_id: str case_id: str payload_hash: str + approval_scope_hash: str class WakeEventSubject(BaseModel): @@ -318,6 +320,10 @@ class GovernorConfig: lhp: LhpClientConfig | None = None limit: int = 20 dry_run: bool = False + trusted_comment_authors: tuple[str, ...] = ( + "hyrule-engineering-loop", + "hyrule-engineering-loop[bot]", + ) @dataclass @@ -524,13 +530,23 @@ def governor_once( path = decision_record_path(record, config.state_dir) record.storage_path = str(path) prior_record = find_matching_decision_record(record, config.state_dir) - if prior_record is not None: - if _labels_already_converged(issue, record): - report.skipped.append(f"{issue.issue_id}: unchanged decision {prior_record.record_id}") - report.records.append(record) - continue - else: + remote_match = False + if prior_record is None: + remote_match = github_has_decision_marker( + issue, + record.record_id, + client=client, + trusted_authors=config.trusted_comment_authors, + ) + if prior_record is not None or remote_match: + if remote_match and not path.exists(): + path = write_decision_record(record, config.state_dir) + record.storage_path = str(path) + if not _labels_already_converged(issue, record): apply_label_transition(issue, record, client=client) + report.skipped.append(f"{issue.issue_id}: unchanged decision {record.record_id}") + report.records.append(record) + continue else: post_decision_record(issue, record, client=client) path = write_decision_record(record, config.state_dir) @@ -585,19 +601,22 @@ def govern_issue( lhp_summary = LhpAuthoritySummary( handoff_id=pointer.handoff_id, case_id=pointer.case_id, - payload_hash=payload_hash(lhp_payload)[:16], + payload_hash=str(lhp_payload.get("payload_hash") or ""), + approval_scope_hash=str(lhp_payload.get("approval_scope_hash") or ""), ) except Exception as exc: lhp_summary = LhpAuthoritySummary( handoff_id=pointer.handoff_id, case_id=pointer.case_id, payload_hash=f"{LHP_FETCH_ERROR_PREFIX}{payload_hash(type(exc).__name__ + str(exc))[:12]}", + approval_scope_hash=f"{LHP_FETCH_ERROR_PREFIX}{payload_hash(type(exc).__name__ + str(exc))[:12]}", ) else: lhp_summary = LhpAuthoritySummary( handoff_id=pointer.handoff_id, case_id=pointer.case_id, payload_hash="unfetched", + approval_scope_hash="unfetched", ) task_text = _authority_text(issue, lhp_payload) @@ -641,9 +660,23 @@ def govern_issue( "issue": issue.issue_id, "authority_text_hash": authority_text_hash, "issue_text_hash": issue_text_hash, - "lhp": lhp_summary.model_dump(mode="json") if lhp_summary is not None else None, + "lhp": ( + { + "handoff_id": lhp_summary.handoff_id, + "case_id": lhp_summary.case_id, + "approval_scope_hash": lhp_summary.approval_scope_hash, + } + if lhp_summary is not None + else None + ), "classification": classification.model_dump(mode="json"), - "knowledge": knowledge.model_dump(mode="json"), + "knowledge": { + "status": knowledge.status, + "authority_level_used": knowledge.authority_level_used, + "policy_result": knowledge.policy_result, + "refs": knowledge.refs, + "reasons": knowledge.reasons, + }, "decision": decision, "capability": capability.model_dump(mode="json") if capability is not None else None, "allowed_paths": allowed_paths, @@ -735,6 +768,15 @@ def classify_issue_intent( text, ["customer-impacting", "customer impacting", "customer provisioning", "provisioning config"], ) + approval_scope = _stable_lhp_scope(lhp_payload) + raw_scope_handoff = approval_scope.get("handoff") + scope_handoff: dict[str, Any] = raw_scope_handoff if isinstance(raw_scope_handoff, dict) else {} + raw_scope_payload = scope_handoff.get("payload") + scope_payload: dict[str, Any] = raw_scope_payload if isinstance(raw_scope_payload, dict) else {} + disk_infrastructure_handoff = ( + scope_handoff.get("case_type") == "proactive_disk_condition" + or scope_payload.get("change_domain") == "infrastructure_capacity_or_retention" + ) if secrets: intent, risk_tier, domains = "secret", 4, ["secret"] @@ -757,6 +799,12 @@ def classify_issue_intent( expected_paths = ["compliance/"] blast_radius = "compliance" rationale = "compliance surfaces are Tier 4" + elif disk_infrastructure_handoff: + intent, risk_tier, domains = "infrastructure_config", 3, ["infrastructure_config"] + expected_paths = ["ansible/", "configs/"] + services = ["infrastructure capacity"] + blast_radius = "production host configuration" + rationale = "disk remediation requires infrastructure changes outside the monitoring capability" elif production_routing: intent = ( "routing_policy" @@ -880,6 +928,10 @@ def decide_policy( return "needs_context", None, denial_reasons, policy_rules if capability is None: + if classification.intent_type == "infrastructure_config": + denial_reasons.append("infrastructure change has no matching autonomous capability envelope") + policy_rules.append("infrastructure capacity changes require explicit human handling") + return "needs_human", None, denial_reasons, policy_rules denial_reasons.append("no matching capability envelope") policy_rules.append("without a capability, sufficiently specified work can only become candidate") return "allow_candidate", None, denial_reasons, policy_rules @@ -1033,6 +1085,44 @@ def post_decision_record( ) +def github_has_decision_marker( + issue: IssueSnapshot, + record_id: str, + *, + client: GhClient, + trusted_authors: tuple[str, ...], +) -> bool: + """Find a trusted semantic CDR marker even when local state was lost.""" + + try: + raw = client.run( + [ + "api", + "--paginate", + "--slurp", + f"repos/{issue.repo}/issues/{issue.number}/comments?per_page=100", + ] + ) + decoded = json.loads(raw or "[]") + except (OSError, RuntimeError, ValueError): + return False + pages = decoded if isinstance(decoded, list) else [] + comments: list[dict[str, Any]] = [] + for page in pages: + if isinstance(page, list): + comments.extend(comment for comment in page if isinstance(comment, dict)) + elif isinstance(page, dict): + comments.append(page) + marker = f"" + trusted = set(trusted_authors) + for comment in comments: + author = comment.get("user") or comment.get("author") or {} + login = str(author.get("login") or "") if isinstance(author, dict) else str(author or "") + if login in trusted and marker in str(comment.get("body") or ""): + return True + return False + + def apply_label_transition( issue: IssueSnapshot, record: CandidateDecisionRecord, @@ -1114,16 +1204,7 @@ def find_matching_decision_record( def stable_decision_signature(record: CandidateDecisionRecord) -> dict[str, Any]: """Return the decision fields that should make Governor posting idempotent.""" - data = record.model_dump(mode="json") - for field_name in ( - "record_id", - "created_at", - "knowledge_context_pack_id", - "knowledge_export_version", - "storage_path", - ): - data.pop(field_name, None) - return data + return {"record_id": record.record_id} def _load_governor_knowledge( @@ -1196,25 +1277,13 @@ def summarize_knowledge_pack(pack: dict[str, Any]) -> KnowledgeSummary: def _authority_text(issue: IssueSnapshot, lhp_payload: dict[str, Any] | None) -> str: if lhp_payload is None: return safe_text(f"{issue.title}\n{issue.body}", limit=5000) - selected = { - "handoff": lhp_payload.get("handoff"), - "case": lhp_payload.get("case"), - "verification_objectives": lhp_payload.get("verification_objectives"), - "knowledge_artifacts": lhp_payload.get("knowledge_artifacts"), - } - return safe_text(json.dumps(selected, sort_keys=True, default=str), limit=7000) + return safe_text(json.dumps(_stable_lhp_scope(lhp_payload), sort_keys=True, default=str), limit=7000) def _classification_text(issue: IssueSnapshot, lhp_payload: dict[str, Any] | None) -> str: if lhp_payload is None: return f"{issue.title}\n{issue.body}" - selected = { - "handoff": lhp_payload.get("handoff"), - "case": lhp_payload.get("case"), - "verification_objectives": lhp_payload.get("verification_objectives"), - "knowledge_artifacts": lhp_payload.get("knowledge_artifacts"), - } - return json.dumps(selected, sort_keys=True, default=str) + return json.dumps(_stable_lhp_scope(lhp_payload), sort_keys=True, default=str) def _eligible_for_governor(issue: IssueSnapshot) -> bool: @@ -1245,15 +1314,18 @@ def _verification_method( intent: IntentType, ) -> str: if lhp_payload is not None: + scope = _stable_lhp_scope(lhp_payload) + raw_scope_handoff = scope.get("handoff") + scope_handoff: dict[str, Any] = raw_scope_handoff if isinstance(raw_scope_handoff, dict) else {} objectives = [ str(item.get("name") or item.get("objective_key")) - for item in lhp_payload.get("verification_objectives", []) + for item in scope.get("verification_objectives", []) if isinstance(item, dict) ] criteria = [ str(item) - for item in (lhp_payload.get("handoff") or {}).get("acceptance_criteria", []) - ] if isinstance(lhp_payload.get("handoff"), dict) else [] + for item in scope_handoff.get("acceptance_criteria", []) + ] combined = [item for item in objectives + criteria if item] if combined: return "; ".join(safe_text(item, limit=160) for item in combined[:4]) @@ -1273,7 +1345,7 @@ def _rollback_plan(text: str, *, intent: IntentType) -> str: return "Use the rollback/revert procedure specified in the request." if intent in {"docs", "runbook", "dashboard", "tests"}: return "Close the draft PR or revert the docs/test commit before merge." - if intent in {"monitoring", "alert_tuning", "non_prod_tooling"}: + if intent in {"monitoring", "alert_tuning", "non_prod_tooling", "infrastructure_config"}: return "Revert the draft PR; for deployed alert tuning, restore the previous rule/config version." return "" @@ -1358,7 +1430,19 @@ def _authority_rank(level: str) -> int | None: def _lhp_payload_fetched(summary: LhpAuthoritySummary) -> bool: - return summary.payload_hash != "unfetched" and not summary.payload_hash.startswith(LHP_FETCH_ERROR_PREFIX) + return ( + summary.payload_hash != "unfetched" + and not summary.payload_hash.startswith(LHP_FETCH_ERROR_PREFIX) + and summary.approval_scope_hash != "unfetched" + and not summary.approval_scope_hash.startswith(LHP_FETCH_ERROR_PREFIX) + ) + + +def _stable_lhp_scope(payload: dict[str, Any] | None) -> dict[str, Any]: + if not isinstance(payload, dict): + return {} + scope = payload.get("approval_scope") + return scope if isinstance(scope, dict) else {} def _labels_already_converged(issue: IssueSnapshot, record: CandidateDecisionRecord) -> bool: diff --git a/src/hyrule_engineering_loop/lhp.py b/src/hyrule_engineering_loop/lhp.py index 0cddd3d..d3f3b13 100644 --- a/src/hyrule_engineering_loop/lhp.py +++ b/src/hyrule_engineering_loop/lhp.py @@ -19,6 +19,7 @@ from typing import Any, Callable LHP_SCHEMA_VERSION = "lhp.v1" +LHP_APPROVAL_SCOPE_SCHEMA_VERSION = "lhp.v1.approval-scope.v1" POINTER_RE = re.compile(r"```json\s*(\{.*?\"fetch_path\".*?\})\s*```", re.S) HANDOFF_MARKER_RE = re.compile(r"noc-lhp-handoff-id:([A-Za-z0-9_.:-]+)") CASE_MARKER_RE = re.compile(r"noc-case-id:([A-Za-z0-9_.:-]+)") @@ -100,9 +101,29 @@ def fetch_lhp_payload(pointer: LhpPointer, config: LhpClientConfig, *, requester handoff = _dict_value(payload.get("handoff")) if handoff.get("handoff_id") != pointer.handoff_id or handoff.get("case_id") != pointer.case_id: raise RuntimeError("NOC LHP payload identity mismatch") + expected_payload_hash = _hash_token(payload.get("payload_hash"), field_name="payload_hash") + unhashed_payload = {key: value for key, value in payload.items() if key != "payload_hash"} + if payload_hash(unhashed_payload) != expected_payload_hash: + raise RuntimeError("NOC LHP payload hash mismatch") + approval_scope = validated_approval_scope(payload) + scope_handoff = _dict_value(approval_scope.get("handoff")) + if scope_handoff.get("handoff_id") != pointer.handoff_id or scope_handoff.get("case_id") != pointer.case_id: + raise RuntimeError("NOC LHP approval scope identity mismatch") return payload +def validated_approval_scope(payload: dict[str, Any]) -> dict[str, Any]: + """Validate and return the immutable authorization projection.""" + + scope = _dict_value(payload.get("approval_scope")) + if scope.get("schema_version") != LHP_APPROVAL_SCOPE_SCHEMA_VERSION: + raise RuntimeError("NOC LHP approval scope schema mismatch") + expected = _hash_token(payload.get("approval_scope_hash"), field_name="approval_scope_hash") + if payload_hash(scope) != expected: + raise RuntimeError("NOC LHP approval scope hash mismatch") + return scope + + def render_lhp_request(payload: dict[str, Any], *, issue_url: str, issue_body: str) -> str: handoff = _dict_value(payload.get("handoff")) case = _dict_value(payload.get("case")) @@ -166,7 +187,9 @@ def post_lhp_update( def payload_hash(value: Any) -> str: - return hashlib.sha256(json.dumps(value, sort_keys=True, separators=(",", ":"), default=str).encode()).hexdigest() + return hashlib.sha256( + json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=False, default=str).encode() + ).hexdigest() def safe_text(value: Any, *, limit: int = 1000) -> str: @@ -235,3 +258,10 @@ def _list_value(value: Any) -> list[Any]: def _token(value: Any) -> str: text = str(value or "") return "".join(ch for ch in text if ch.isalnum() or ch in {"_", "-", ":", ".", "/"})[:180] + + +def _hash_token(value: Any, *, field_name: str) -> str: + rendered = str(value or "").strip().lower() + if len(rendered) != 64 or any(ch not in "0123456789abcdef" for ch in rendered): + raise RuntimeError(f"NOC LHP {field_name} is missing or invalid") + return rendered diff --git a/tests/test_phase24_daemon.py b/tests/test_phase24_daemon.py index 7618fd8..c2e63b1 100644 --- a/tests/test_phase24_daemon.py +++ b/tests/test_phase24_daemon.py @@ -93,10 +93,13 @@ def _issue_view_with_reliability_decision( approved_body: str | None = None, author_login: str = "trusted-governor", lhp_payload_hash: str | None = None, + lhp_approval_scope_hash: str | None = None, ) -> str: body_for_hash = approved_body if approved_body is not None else current_body payload = { - "schema_version": "reliability-governor.cdr.v1", + "schema_version": ( + "reliability-governor.cdr.v2" if lhp_approval_scope_hash is not None else "reliability-governor.cdr.v1" + ), "record_id": "record-1", "repo": repo, "issue_number": number, @@ -109,6 +112,7 @@ def _issue_view_with_reliability_decision( "handoff_id": "handoff-1", "case_id": "case-1", "payload_hash": lhp_payload_hash, + "approval_scope_hash": lhp_approval_scope_hash, } comment = "\n".join( [ @@ -144,16 +148,31 @@ def _lhp_body() -> str: def _lhp_payload(objective: str) -> dict[str, Any]: - return { + approval_scope = { + "schema_version": "lhp.v1.approval-scope.v1", + "case": {"case_id": "case-1", "occurrence_id": "occurrence-1"}, + "handoff": { + "handoff_id": "handoff-1", + "case_id": "case-1", + "objective": objective, + }, + "verification_objectives": [], + } + result = { "schema_version": "lhp.v1", "handoff": { "handoff_id": "handoff-1", "case_id": "case-1", "objective": objective, + "status": "requested", }, "case": {"case_id": "case-1"}, "verification_objectives": [], + "approval_scope": approval_scope, + "approval_scope_hash": payload_hash(approval_scope), } + result["payload_hash"] = payload_hash(result) + return result # --- AC1: run lock ---------------------------------------------------------- @@ -590,7 +609,7 @@ def test_daemon_rejects_stale_reliability_decision_after_long_body_tail_edit(tmp assert report.detail == "Reliability Decision Record is stale for the current issue title/body" -def test_daemon_rejects_stale_lhp_payload_hash( +def test_daemon_rejects_stale_lhp_approval_scope_hash( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -618,6 +637,7 @@ def test_daemon_rejects_stale_lhp_payload_hash( allowed_paths=["docs/"], current_body=body, lhp_payload_hash=payload_hash(approved_payload)[:16], + lhp_approval_scope_hash=approved_payload["approval_scope_hash"], ), } ) @@ -629,7 +649,79 @@ def test_daemon_rejects_stale_lhp_payload_hash( report = daemon_once(config, client=gh, feature_runner=lambda **kwargs: pytest.fail("runner should not start")) assert report.outcome == "needs_triage" - assert report.detail == "Reliability Decision Record record-1 LHP payload hash is stale" + assert report.detail == "Reliability Decision Record record-1 LHP approval scope hash is stale" + + +def test_failed_top_issue_is_parked_and_next_cycle_advances_queue(tmp_path: Path) -> None: + repo = "AS215932/engineering-loop" + issues = [ + { + "number": 1, + "title": "First approved issue", + "body": "## Context\nfirst\n## Action items\n1. fail\n## Related\n- test", + "labels": [{"name": "loop:approved"}, {"name": "critical"}], + "url": f"https://github.com/{repo}/issues/1", + "updatedAt": "2026-06-12T00:00:00Z", + }, + { + "number": 2, + "title": "Second approved issue", + "body": "## Context\nsecond\n## Action items\n1. continue\n## Related\n- test", + "labels": [{"name": "loop:approved"}], + "url": f"https://github.com/{repo}/issues/2", + "updatedAt": "2026-06-12T00:00:00Z", + }, + ] + + class StatefulGh: + def __init__(self) -> None: + self.calls: list[list[str]] = [] + + def run(self, args: list[str]) -> str: + self.calls.append(list(args)) + if args[:2] == ["issue", "list"]: + approved = [ + issue + for issue in issues + if any(label["name"] == "loop:approved" for label in issue["labels"]) + ] + return json.dumps(approved) + if args[:2] == ["issue", "view"]: + number = int(args[2]) + issue = next(candidate for candidate in issues if candidate["number"] == number) + return json.dumps({"body": issue["body"]}) + if args[:2] == ["issue", "edit"]: + number = int(args[2]) + issue = next(candidate for candidate in issues if candidate["number"] == number) + names = [label["name"] for label in issue["labels"]] + names.remove("loop:approved") + names.append("loop:needs-human") + issue["labels"] = [{"name": name} for name in names] + return "" + return "[]" + + client = StatefulGh() + config = DaemonConfig( + repos=(repo,), + state_dir=tmp_path / "state", + output_root=tmp_path / "runs", + ) + + def failed_runner(**kwargs: Any) -> dict[str, Any]: + return { + "final_state": {"backend_results": []}, + "failure_summary": {"error_excerpt": "deliberate regression failure"}, + } + + first = daemon_once(config, client=client, feature_runner=failed_runner) + second = daemon_once(config, client=client, feature_runner=failed_runner) + + assert first.issue is not None and first.issue["number"] == 1 + assert second.issue is not None and second.issue["number"] == 2 + assert [label["name"] for label in issues[0]["labels"]] == ["critical", "loop:needs-human"] + assert [label["name"] for label in issues[1]["labels"]] == ["loop:needs-human"] + edit_numbers = [int(call[2]) for call in client.calls if call[:2] == ["issue", "edit"]] + assert edit_numbers == [1, 2] def test_repo_name_for_issue_maps_core_repo_checkout_names() -> None: diff --git a/tests/test_phase28_lhp.py b/tests/test_phase28_lhp.py index bc301b0..1331986 100644 --- a/tests/test_phase28_lhp.py +++ b/tests/test_phase28_lhp.py @@ -9,7 +9,13 @@ import hyrule_engineering_loop.daemon as daemon_module from hyrule_engineering_loop.daemon import DaemonConfig, daemon_once from hyrule_engineering_loop.intake import APPROVED_LABEL -from hyrule_engineering_loop.lhp import LhpClientConfig, fetch_lhp_payload, parse_lhp_pointer, render_lhp_request +from hyrule_engineering_loop.lhp import ( + LhpClientConfig, + fetch_lhp_payload, + parse_lhp_pointer, + payload_hash, + render_lhp_request, +) @pytest.fixture(autouse=True) @@ -53,7 +59,26 @@ def _body() -> str: def _payload() -> dict[str, Any]: - return { + approval_scope = { + "schema_version": "lhp.v1.approval-scope.v1", + "case": {"case_id": "case_1", "occurrence_id": "occurrence_1"}, + "handoff": { + "handoff_id": "handoff_disk_1", + "case_id": "case_1", + "objective": "resolve low root filesystem condition", + "objective_key": "resolve-low-root-filesystem-condition-v1", + "case_type": "proactive_disk_condition", + "resource": {"host": "rtr", "filesystem": "/"}, + "constraints": ["keep human loop:approved gate"], + "acceptance_criteria": ["monitoring alert clears"], + "payload": { + "change_domain": "infrastructure_capacity_or_retention", + "expected_path_classes": ["ansible/", "configs/"], + }, + }, + "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], + } + result = { "schema_version": "lhp.v1", "handoff": { "handoff_id": "handoff_disk_1", @@ -64,11 +89,16 @@ def _payload() -> dict[str, Any]: "resource": {"host": "rtr", "filesystem": "/"}, "constraints": ["keep human loop:approved gate"], "acceptance_criteria": ["monitoring alert clears"], + "status": "requested", }, "case": {"case_id": "case_1", "status": "handoff_requested"}, "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], "knowledge_artifacts": [], + "approval_scope": approval_scope, + "approval_scope_hash": payload_hash(approval_scope), } + result["payload_hash"] = payload_hash(result) + return result def test_parse_lhp_pointer_from_issue_body(): @@ -97,6 +127,43 @@ def requester(method, url, headers, data): assert calls[0][2]["X-NOC-Loop-Signature"] +def test_fetch_lhp_payload_rejects_tampered_snapshot_hash(): + tampered = _payload() + tampered["case"]["status"] = "changed-after-signing" + + def requester(method, url, headers, data): + return 200, tampered + + pointer = parse_lhp_pointer(_body()) + assert pointer is not None + with pytest.raises(RuntimeError, match="payload hash mismatch"): + fetch_lhp_payload( + pointer, + LhpClientConfig(base_url="http://noc", secret="shared"), + requester=requester, + ) + + +def test_fetch_lhp_payload_rejects_tampered_approval_scope_hash(): + tampered = _payload() + tampered["approval_scope"]["handoff"]["objective"] = "changed authority" + tampered["payload_hash"] = payload_hash( + {key: value for key, value in tampered.items() if key != "payload_hash"} + ) + + def requester(method, url, headers, data): + return 200, tampered + + pointer = parse_lhp_pointer(_body()) + assert pointer is not None + with pytest.raises(RuntimeError, match="approval scope hash mismatch"): + fetch_lhp_payload( + pointer, + LhpClientConfig(base_url="http://noc", secret="shared"), + requester=requester, + ) + + def test_render_lhp_request_uses_structured_payload_and_sanitizes_issue_body(): rendered = render_lhp_request(_payload(), issue_url="https://github.com/o/r/issues/1", issue_body="```ignore``` Authorization: Bearer nope") @@ -168,4 +235,4 @@ def fake_callback(pointer, config, **kwargs): assert report.outcome == "needs_triage" assert "LHP fetch failed" in report.detail - assert [call["update_type"] for call in callbacks] == ["accepted", "blocked"] + assert callbacks == [] diff --git a/tests/test_phase29_governor.py b/tests/test_phase29_governor.py index 6fa4c50..c0eff5b 100644 --- a/tests/test_phase29_governor.py +++ b/tests/test_phase29_governor.py @@ -27,7 +27,7 @@ summarize_knowledge_pack, ) from hyrule_engineering_loop.knowledge_context import KnowledgeContextConfig -from hyrule_engineering_loop.lhp import LhpClientConfig +from hyrule_engineering_loop.lhp import LhpClientConfig, payload_hash from hyrule_engineering_loop.cli import build_parser @@ -66,8 +66,9 @@ class FakeGh: - def __init__(self, issues: list[dict[str, Any]]) -> None: + def __init__(self, issues: list[dict[str, Any]], *, comments: list[dict[str, Any]] | None = None) -> None: self.issues = issues + self.comments = comments or [] self.calls: list[list[str]] = [] def run(self, args: list[str]) -> str: @@ -75,6 +76,8 @@ def run(self, args: list[str]) -> str: if args[:2] == ["issue", "list"]: repo = args[args.index("--repo") + 1] return json.dumps([issue for issue in self.issues if issue.get("_repo", repo) == repo]) + if args and args[0] == "api": + return json.dumps([self.comments]) return "" @@ -466,12 +469,67 @@ def rotating_knowledge(_: str, __: Any) -> Any: stored = list((tmp_path / "reliability-governor").glob("*.json")) assert first.records[0].routing_decision == "allow_approved" assert second.records[0].routing_decision == "allow_approved" - assert first.records[0].record_id != second.records[0].record_id + assert first.records[0].record_id == second.records[0].record_id assert len(comment_calls) == 1 assert len(stored) == 1 assert second.skipped == [f"{issue.issue_id}: unchanged decision {first.records[0].record_id}"] +def test_trusted_github_marker_restores_missing_local_state_without_reposting(tmp_path: Path) -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + labels=[APPROVED_LABEL], + ) + expected = govern_issue(issue, registry=default_capability_registry(), knowledge_loader=_knowledge) + gh = FakeGh( + [_issue_json(issue)], + comments=[ + { + "user": {"login": "hyrule-engineering-loop"}, + "body": f"", + } + ], + ) + config = ReliabilityGovernorConfig( + repos=(issue.repo,), + state_dir=tmp_path / "missing-local-state", + dry_run=False, + ) + + report = reliability_governor_once(config, client=gh, knowledge_loader=_knowledge) + + assert not [call for call in gh.calls if call[:2] == ["issue", "comment"]] + assert len(list(config.state_dir.glob("*.json"))) == 1 + assert report.skipped == [f"{issue.issue_id}: unchanged decision {expected.record_id}"] + + +def test_untrusted_github_marker_does_not_suppress_decision_comment(tmp_path: Path) -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + labels=[APPROVED_LABEL], + ) + expected = govern_issue(issue, registry=default_capability_registry(), knowledge_loader=_knowledge) + gh = FakeGh( + [_issue_json(issue)], + comments=[ + { + "user": {"login": "drive-by-commenter"}, + "body": f"", + } + ], + ) + + reliability_governor_once( + ReliabilityGovernorConfig(repos=(issue.repo,), state_dir=tmp_path / "state"), + client=gh, + knowledge_loader=_knowledge, + ) + + assert len([call for call in gh.calls if call[:2] == ["issue", "comment"]]) == 1 + + def test_unchanged_candidate_decision_is_not_reposted(tmp_path: Path) -> None: issue = _issue( title="Update internal service helper", @@ -595,26 +653,54 @@ def _lhp_body() -> str: """ -def _lhp_payload() -> dict[str, Any]: - return { +def _lhp_payload( + *, + objective: str = "resolve disk alert follow-up", + knowledge_artifacts: list[dict[str, Any]] | None = None, +) -> dict[str, Any]: + approval_scope = { + "schema_version": "lhp.v1.approval-scope.v1", + "case": {"case_id": "case_1", "occurrence_id": "occurrence_1"}, + "handoff": { + "handoff_id": "handoff_disk_1", + "case_id": "case_1", + "objective": objective, + "objective_key": "resolve-low-root-filesystem-condition-v1", + "case_type": "proactive_disk_condition", + "resource": {"host": "rtr", "filesystem": "/"}, + "constraints": ["draft PR only"], + "acceptance_criteria": ["monitoring alert clears"], + "payload": { + "change_domain": "infrastructure_capacity_or_retention", + "expected_path_classes": ["ansible/", "configs/"], + }, + }, + "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], + } + result = { "schema_version": "lhp.v1", "handoff": { "handoff_id": "handoff_disk_1", "case_id": "case_1", - "objective": "resolve disk alert follow-up", + "objective": objective, "objective_key": "resolve-low-root-filesystem-condition-v1", "case_type": "proactive_disk_condition", "resource": {"host": "rtr", "filesystem": "/"}, "constraints": ["draft PR only"], "acceptance_criteria": ["monitoring alert clears"], + "status": "requested", }, "case": {"case_id": "case_1", "status": "handoff_requested"}, "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], - "knowledge_artifacts": [], + "knowledge_artifacts": knowledge_artifacts or [], + "approval_scope": approval_scope, + "approval_scope_hash": payload_hash(approval_scope), } + result["payload_hash"] = payload_hash(result) + return result -def test_noc_lhp_handoff_uses_caseservice_payload_and_auto_approves_low_risk() -> None: +def test_noc_disk_lhp_handoff_requires_human_infrastructure_capability() -> None: issue = _issue( title="[noc][lhp] disk handoff", body=_lhp_body(), @@ -638,24 +724,23 @@ def requester(method: str, url: str, headers: dict[str, str] | None, data: bytes assert record.source == "noc" assert record.lhp is not None assert record.lhp.payload_hash != "unfetched" - assert record.routing_decision == "allow_approved" - assert record.intent_type == "monitoring" - assert record.next_loop == "engineering" - assert record.handoff_contract == "github_issue_labels" - assert APPROVED_LABEL in record.labels_to_add + assert record.routing_decision == "needs_human" + assert record.intent_type == "infrastructure_config" + assert record.next_loop == "human" + assert NEEDS_HUMAN_LABEL in record.labels_to_add + assert APPROVED_LABEL not in record.labels_to_add -def test_lhp_payload_hash_is_part_of_record_identity() -> None: +def test_mutable_lhp_snapshot_is_not_part_of_record_identity() -> None: issue = _issue( title="[noc][lhp] disk handoff", body=_lhp_body(), labels=["engineering-handoff"], ) first_payload = _lhp_payload() - changed_payload = { - **_lhp_payload(), - "knowledge_artifacts": [{"kind": "case-note", "id": "changed-without-classification-effect"}], - } + changed_payload = _lhp_payload( + knowledge_artifacts=[{"kind": "case-note", "id": "changed-without-classification-effect"}] + ) def requester(payload: dict[str, Any]) -> Any: def inner( @@ -687,6 +772,46 @@ def inner( assert changed.lhp is not None assert first.lhp.payload_hash != changed.lhp.payload_hash assert first.intent_type == changed.intent_type + assert first.lhp.approval_scope_hash == changed.lhp.approval_scope_hash + assert first.record_id == changed.record_id + + +def test_semantic_lhp_approval_scope_change_creates_new_record_identity() -> None: + issue = _issue( + title="[noc][lhp] disk handoff", + body=_lhp_body(), + labels=["engineering-handoff"], + ) + + def requester(payload: dict[str, Any]) -> Any: + def inner( + method: str, + url: str, + headers: dict[str, str] | None, + data: bytes | None, + ) -> tuple[int, dict[str, Any]]: + return 200, payload + + return inner + + first = govern_issue( + issue, + registry=default_capability_registry(), + knowledge_loader=_knowledge, + lhp_config=LhpClientConfig(base_url="http://noc", secret="shared"), + lhp_requester=requester(_lhp_payload()), + ) + changed = govern_issue( + issue, + registry=default_capability_registry(), + knowledge_loader=_knowledge, + lhp_config=LhpClientConfig(base_url="http://noc", secret="shared"), + lhp_requester=requester(_lhp_payload(objective="resolve a different disk condition")), + ) + + assert first.lhp is not None + assert changed.lhp is not None + assert first.lhp.approval_scope_hash != changed.lhp.approval_scope_hash assert first.record_id != changed.record_id From a915ebca3f0f8bc176c1c0f0451bbe92fc6eb97f Mon Sep 17 00:00:00 2001 From: David Hyrule Date: Tue, 21 Jul 2026 23:07:41 +0200 Subject: [PATCH 2/2] Fail closed across LHP review boundaries --- .../loop/hyrule-reliability-governor.service | 1 + src/hyrule_engineering_loop/cli.py | 14 ++ src/hyrule_engineering_loop/daemon.py | 102 ++++++++++---- src/hyrule_engineering_loop/governor.py | 47 +++++-- src/hyrule_engineering_loop/lhp.py | 62 +++++++- tests/test_phase24_daemon.py | 40 ++++++ tests/test_phase28_lhp.py | 43 +++++- tests/test_phase29_governor.py | 132 +++++++++++++++++- 8 files changed, 386 insertions(+), 55 deletions(-) diff --git a/configs/loop/hyrule-reliability-governor.service b/configs/loop/hyrule-reliability-governor.service index a9cf9db..2fb1691 100644 --- a/configs/loop/hyrule-reliability-governor.service +++ b/configs/loop/hyrule-reliability-governor.service @@ -32,6 +32,7 @@ ExecStart=/opt/engineering-loop/.venv/bin/hyrule-engineering-loop reliability-go --repo AS215932/hyrule-network-proxy \ --repo AS215932/as215932.net \ --registry /opt/engineering-loop/configs/loop/capability-registry.yml \ + --trusted-comment-author Svaag \ --state-dir-path /var/lib/engineering-loop/reliability-governor \ --knowledge-context \ --knowledge-mcp-url http://127.0.0.1:8767/mcp \ diff --git a/src/hyrule_engineering_loop/cli.py b/src/hyrule_engineering_loop/cli.py index 8e12635..5d79a0c 100644 --- a/src/hyrule_engineering_loop/cli.py +++ b/src/hyrule_engineering_loop/cli.py @@ -554,6 +554,14 @@ def governor_command(args: argparse.Namespace) -> int: lhp=LhpClientConfig.from_env(), limit=args.limit, dry_run=args.dry_run, + trusted_comment_authors=tuple( + dict.fromkeys( + ( + *ReliabilityGovernorConfig.trusted_comment_authors, + *(args.trusted_comment_author or ()), + ) + ) + ), ) report = reliability_governor_once(config, client=GhCli()) record_insights( @@ -789,6 +797,12 @@ def _add_reliability_governor_parser( parser.add_argument("--repo", action="append") parser.add_argument("--limit", type=int, default=ReliabilityGovernorConfig.limit) parser.add_argument("--dry-run", action="store_true") + parser.add_argument( + "--trusted-comment-author", + action="append", + metavar="LOGIN", + help="trusted GitHub login whose existing Reliability Decision markers suppress reposting. Repeatable.", + ) parser.add_argument("--state-dir-path", dest="state_dir_path") parser.add_argument("--registry", help="capability registry YAML/JSON") parser.add_argument("--knowledge-context", action="store_true", help="include a read-only AS215932 knowledge context pack") diff --git a/src/hyrule_engineering_loop/daemon.py b/src/hyrule_engineering_loop/daemon.py index e1a00a2..b129a82 100644 --- a/src/hyrule_engineering_loop/daemon.py +++ b/src/hyrule_engineering_loop/daemon.py @@ -628,26 +628,37 @@ def _lhp_approval_scope_hash_error( return None -def _park_issue_for_human(item: IntakeItem, *, client: GhClient) -> bool: +def _park_issue_for_human(item: IntakeItem, *, client: GhClient, attempts: int = 3) -> bool: """Remove a completed attempt from the autonomous queue.""" - try: - client.run( - [ - "issue", - "edit", - str(item.number), - "--repo", - item.repo, - "--remove-label", - APPROVED_LABEL, - "--add-label", - NEEDS_HUMAN_LABEL, - ] - ) - except Exception: - return False - return True + for _ in range(max(1, attempts)): + try: + client.run( + [ + "issue", + "edit", + str(item.number), + "--repo", + item.repo, + "--remove-label", + APPROVED_LABEL, + "--add-label", + NEEDS_HUMAN_LABEL, + ] + ) + except Exception: + continue + return True + return False + + +def _parking_failure_report(item: IntakeItem, *, change_id: str, context: str) -> DaemonReport: + return DaemonReport( + outcome="error", + detail=f"failed to park approved issue before execution: {context}"[:300], + issue={"repo": item.repo, "number": item.number, "title": item.title}, + change_id=change_id, + ) def _change_id_for(item: IntakeItem) -> str: @@ -740,6 +751,7 @@ def daemon_once( """Run one autonomous cycle: pick one approved item, run, publish or journal.""" started = time.monotonic() item: IntakeItem | None = None + issue_parked = False if os.environ.get("GITHUB_ACTIONS"): return _finish( DaemonReport( @@ -801,11 +813,18 @@ def daemon_once( trusted_authors=config.reliability_decision_authors, ) if approval_error is not None or effective_allowed_paths is None: - _park_issue_for_human(item, client=client) + detail = approval_error or "approved issue has no valid Reliability Decision Record" + issue_parked = _park_issue_for_human(item, client=client) + if not issue_parked: + return _finish( + _parking_failure_report(item, change_id=change_id, context=detail), + discord_poster, + icinga_poster, + ) return _finish( DaemonReport( outcome="needs_triage", - detail=(approval_error or "approved issue has no valid Reliability Decision Record")[:200], + detail=detail[:200], issue={"repo": item.repo, "number": item.number, "title": item.title}, change_id=change_id, ), @@ -820,11 +839,18 @@ def daemon_once( lhp_payload = fetch_lhp_payload(lhp_pointer, lhp_config) validated_approval_scope(lhp_payload) except Exception as exc: - _park_issue_for_human(item, client=client) + detail = f"LHP fetch failed: {type(exc).__name__}: {str(exc)[:160]}" + issue_parked = _park_issue_for_human(item, client=client) + if not issue_parked: + return _finish( + _parking_failure_report(item, change_id=change_id, context=detail), + discord_poster, + icinga_poster, + ) return _finish( DaemonReport( outcome="needs_triage", - detail=f"LHP fetch failed: {type(exc).__name__}: {str(exc)[:160]}", + detail=detail, issue={"repo": item.repo, "number": item.number, "title": item.title}, change_id=change_id, ), @@ -833,7 +859,13 @@ def daemon_once( ) lhp_hash_error = _lhp_approval_scope_hash_error(lhp_payload, approval_scope) if lhp_hash_error is not None: - _park_issue_for_human(item, client=client) + issue_parked = _park_issue_for_human(item, client=client) + if not issue_parked: + return _finish( + _parking_failure_report(item, change_id=change_id, context=lhp_hash_error), + discord_poster, + icinga_poster, + ) return _finish( DaemonReport( outcome="needs_triage", @@ -848,7 +880,13 @@ def daemon_once( current_status = str(handoff_state.get("status") or "") if isinstance(handoff_state, dict) else "" if current_status != "requested": detail = f"LHP handoff state is {current_status or 'missing'}, expected requested" - _park_issue_for_human(item, client=client) + issue_parked = _park_issue_for_human(item, client=client) + if not issue_parked: + return _finish( + _parking_failure_report(item, change_id=change_id, context=detail), + discord_poster, + icinga_poster, + ) return _finish( DaemonReport( outcome="needs_triage", @@ -859,6 +897,14 @@ def daemon_once( discord_poster, icinga_poster, ) + issue_parked = _park_issue_for_human(item, client=client) + if not issue_parked: + return _finish( + _parking_failure_report(item, change_id=change_id, context="label transition failed"), + discord_poster, + icinga_poster, + ) + if lhp_pointer is not None: post_lhp_update( lhp_pointer, lhp_config, @@ -965,7 +1011,6 @@ def daemon_once( summary=report.detail, ) - _park_issue_for_human(item, client=client) report.wall_clock_seconds = time.monotonic() - started update_ledger( state_dir, @@ -975,9 +1020,10 @@ def daemon_once( ) return _finish(report, discord_poster, icinga_poster) except Exception as exc: - if item is not None: - _park_issue_for_human(item, client=client) - report = DaemonReport(outcome="error", detail=str(exc)[:300]) + parking_suffix = "" + if item is not None and not issue_parked and not _park_issue_for_human(item, client=client): + parking_suffix = "; failed to park approved issue" + report = DaemonReport(outcome="error", detail=f"{str(exc)}{parking_suffix}"[:300]) report.wall_clock_seconds = time.monotonic() - started return _finish(report, discord_poster, icinga_poster) finally: diff --git a/src/hyrule_engineering_loop/governor.py b/src/hyrule_engineering_loop/governor.py index 14c4c46..82e0e60 100644 --- a/src/hyrule_engineering_loop/governor.py +++ b/src/hyrule_engineering_loop/governor.py @@ -46,6 +46,12 @@ WAKE_EVENT_SCHEMA_VERSION: Literal["reliability-governor.wake.v1"] = "reliability-governor.wake.v1" GOVERNOR_NAME = "Reliability Governor" GOVERNOR_ROLE = "staff_sre_autonomous_operations" + + +class DecisionMarkerLookupError(RuntimeError): + """Remote CDR state could not be determined safely.""" + + CONTROLLED_LOOPS: tuple[str, ...] = ("engineering", "noc", "knowledge") DEFAULT_STRONG_HISTORY_SUCCESSES = 5 LHP_FETCH_ERROR_PREFIX = "fetch_error:" @@ -532,12 +538,16 @@ def governor_once( prior_record = find_matching_decision_record(record, config.state_dir) remote_match = False if prior_record is None: - remote_match = github_has_decision_marker( - issue, - record.record_id, - client=client, - trusted_authors=config.trusted_comment_authors, - ) + try: + remote_match = github_has_decision_marker( + issue, + record.record_id, + client=client, + trusted_authors=config.trusted_comment_authors, + ) + except DecisionMarkerLookupError: + report.skipped.append(f"{issue.issue_id}: decision marker lookup unavailable") + continue if prior_record is not None or remote_match: if remote_match and not path.exists(): path = write_decision_record(record, config.state_dir) @@ -674,8 +684,8 @@ def govern_issue( "status": knowledge.status, "authority_level_used": knowledge.authority_level_used, "policy_result": knowledge.policy_result, - "refs": knowledge.refs, - "reasons": knowledge.reasons, + "refs": sorted(set(knowledge.refs)), + "reasons": sorted(set(knowledge.reasons)), }, "decision": decision, "capability": capability.model_dump(mode="json") if capability is not None else None, @@ -1104,15 +1114,21 @@ def github_has_decision_marker( ] ) decoded = json.loads(raw or "[]") - except (OSError, RuntimeError, ValueError): - return False - pages = decoded if isinstance(decoded, list) else [] + except (OSError, RuntimeError, ValueError) as exc: + raise DecisionMarkerLookupError("GitHub decision-comment lookup failed") from exc + if not isinstance(decoded, list): + raise DecisionMarkerLookupError("GitHub decision-comment lookup returned malformed data") + pages = decoded comments: list[dict[str, Any]] = [] for page in pages: if isinstance(page, list): - comments.extend(comment for comment in page if isinstance(comment, dict)) + if not all(isinstance(comment, dict) for comment in page): + raise DecisionMarkerLookupError("GitHub decision-comment lookup returned malformed comments") + comments.extend(page) elif isinstance(page, dict): comments.append(page) + else: + raise DecisionMarkerLookupError("GitHub decision-comment lookup returned malformed pages") marker = f"" trusted = set(trusted_authors) for comment in comments: @@ -1228,7 +1244,10 @@ def _load_governor_knowledge( def summarize_knowledge_pack(pack: dict[str, Any]) -> KnowledgeSummary: """Reduce a Knowledge context pack to the policy fields the Governor needs.""" - refs = [ref for ref in pack.get("included_refs", []) if isinstance(ref, dict)] + refs = sorted( + (ref for ref in pack.get("included_refs", []) if isinstance(ref, dict)), + key=lambda ref: str(ref.get("concept_id") or "unknown"), + ) ref_ids = [str(ref.get("concept_id", "unknown")) for ref in refs] reasons: list[str] = [] status: KnowledgeStatus = "current" @@ -1270,7 +1289,7 @@ def summarize_knowledge_pack(pack: dict[str, Any]) -> KnowledgeSummary: authority_level_used=authority, policy_result=policy_result, refs=ref_ids, - reasons=reasons, + reasons=sorted(set(reasons)), ) diff --git a/src/hyrule_engineering_loop/lhp.py b/src/hyrule_engineering_loop/lhp.py index d3f3b13..a2b5e82 100644 --- a/src/hyrule_engineering_loop/lhp.py +++ b/src/hyrule_engineering_loop/lhp.py @@ -121,13 +121,15 @@ def validated_approval_scope(payload: dict[str, Any]) -> dict[str, Any]: expected = _hash_token(payload.get("approval_scope_hash"), field_name="approval_scope_hash") if payload_hash(scope) != expected: raise RuntimeError("NOC LHP approval scope hash mismatch") + _validate_approval_scope_binding(payload, scope) return scope def render_lhp_request(payload: dict[str, Any], *, issue_url: str, issue_body: str) -> str: - handoff = _dict_value(payload.get("handoff")) - case = _dict_value(payload.get("case")) - objectives = _list_value(payload.get("verification_objectives")) + approval_scope = validated_approval_scope(payload) + handoff = _dict_value(approval_scope.get("handoff")) + case = _dict_value(approval_scope.get("case")) + objectives = _list_value(approval_scope.get("verification_objectives")) lines = [ f"# {safe_text(handoff.get('objective') or 'NOC LHP request')}", "", @@ -156,6 +158,60 @@ def render_lhp_request(payload: dict[str, Any], *, issue_url: str, issue_body: s return "\n".join(lines) +def _validate_approval_scope_binding(payload: dict[str, Any], scope: dict[str, Any]) -> None: + """Ensure audit snapshot fields cannot diverge from approved execution fields.""" + + current_handoff = _dict_value(payload.get("handoff")) + approved_handoff = _dict_value(scope.get("handoff")) + for field, approved_value in approved_handoff.items(): + if field == "payload_hash": + current_hash = payload_hash(_dict_value(current_handoff.get("payload"))) + if current_hash != str(approved_value or "").strip().lower(): + raise RuntimeError("NOC LHP handoff payload is outside the approved scope") + continue + if field == "payload": + if not _projection_matches(_dict_value(current_handoff.get("payload")), _dict_value(approved_value)): + raise RuntimeError("NOC LHP handoff routing payload is outside the approved scope") + continue + if current_handoff.get(field) != approved_value: + raise RuntimeError(f"NOC LHP handoff field {field} is outside the approved scope") + + current_objectives = [item for item in _list_value(payload.get("verification_objectives")) if isinstance(item, dict)] + for approved in _list_value(scope.get("verification_objectives")): + if not isinstance(approved, dict): + raise RuntimeError("NOC LHP approval scope objective is invalid") + objective_id = str(approved.get("objective_id") or "") + objective_key = str(approved.get("objective_key") or "") + current = next( + ( + item + for item in current_objectives + if (objective_id and str(item.get("objective_id") or "") == objective_id) + or (not objective_id and str(item.get("objective_key") or "") == objective_key) + ), + None, + ) + if current is None: + raise RuntimeError("NOC LHP approved verification objective is missing") + for field, approved_value in approved.items(): + if field == "payload_hash": + if payload_hash(_dict_value(current.get("payload"))) != str(approved_value or "").strip().lower(): + raise RuntimeError("NOC LHP objective payload is outside the approved scope") + elif current.get(field) != approved_value: + raise RuntimeError(f"NOC LHP objective field {field} is outside the approved scope") + + +def _projection_matches(current: dict[str, Any], approved: dict[str, Any]) -> bool: + for key, approved_value in approved.items(): + current_value = current.get(key) + if isinstance(approved_value, dict): + if not isinstance(current_value, dict) or not _projection_matches(current_value, approved_value): + return False + elif current_value != approved_value: + return False + return True + + def post_lhp_update( pointer: LhpPointer, config: LhpClientConfig, diff --git a/tests/test_phase24_daemon.py b/tests/test_phase24_daemon.py index c2e63b1..be546aa 100644 --- a/tests/test_phase24_daemon.py +++ b/tests/test_phase24_daemon.py @@ -724,6 +724,46 @@ def failed_runner(**kwargs: Any) -> dict[str, Any]: assert edit_numbers == [1, 2] +def test_daemon_aborts_before_execution_when_queue_parking_fails(tmp_path: Path) -> None: + repo = "AS215932/engineering-loop" + + class ParkingFailureGh(FakeGh): + def run(self, args: list[str]) -> str: + self.calls.append(list(args)) + if args[:2] == ["issue", "edit"]: + raise RuntimeError("temporary label API failure") + key = " ".join(args[:2]) + return self.responses.get(key, "[]") + + client = ParkingFailureGh( + { + "issue list": _approved_issue_json(1, repo=repo, labels=["loop:approved"]), + "issue view": json.dumps( + { + "body": "## Context\nfirst\n## Action items\n1. fail closed\n## Related\n- test" + } + ), + } + ) + runner_called = False + + def runner(**kwargs: Any) -> dict[str, Any]: + nonlocal runner_called + runner_called = True + return {"final_state": {}} + + report = daemon_once( + DaemonConfig(repos=(repo,), state_dir=tmp_path / "state", output_root=tmp_path / "runs"), + client=client, + feature_runner=runner, + ) + + assert report.outcome == "error" + assert "failed to park approved issue before execution" in report.detail + assert runner_called is False + assert len([call for call in client.calls if call[:2] == ["issue", "edit"]]) == 3 + + def test_repo_name_for_issue_maps_core_repo_checkout_names() -> None: cases = { "AS215932/engineering-loop": "engineering-loop", diff --git a/tests/test_phase28_lhp.py b/tests/test_phase28_lhp.py index 1331986..e86d263 100644 --- a/tests/test_phase28_lhp.py +++ b/tests/test_phase28_lhp.py @@ -59,6 +59,10 @@ def _body() -> str: def _payload() -> dict[str, Any]: + handoff_payload = { + "change_domain": "infrastructure_capacity_or_retention", + "expected_path_classes": ["ansible/", "configs/"], + } approval_scope = { "schema_version": "lhp.v1.approval-scope.v1", "case": {"case_id": "case_1", "occurrence_id": "occurrence_1"}, @@ -71,12 +75,16 @@ def _payload() -> dict[str, Any]: "resource": {"host": "rtr", "filesystem": "/"}, "constraints": ["keep human loop:approved gate"], "acceptance_criteria": ["monitoring alert clears"], - "payload": { - "change_domain": "infrastructure_capacity_or_retention", - "expected_path_classes": ["ansible/", "configs/"], - }, + "payload": handoff_payload, + "payload_hash": payload_hash(handoff_payload), }, - "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], + "verification_objectives": [ + { + "objective_key": "disk_clear", + "name": "disk alert clears", + "payload_hash": payload_hash({}), + } + ], } result = { "schema_version": "lhp.v1", @@ -90,9 +98,12 @@ def _payload() -> dict[str, Any]: "constraints": ["keep human loop:approved gate"], "acceptance_criteria": ["monitoring alert clears"], "status": "requested", + "payload": handoff_payload, }, "case": {"case_id": "case_1", "status": "handoff_requested"}, - "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], + "verification_objectives": [ + {"objective_key": "disk_clear", "name": "disk alert clears", "payload": {}} + ], "knowledge_artifacts": [], "approval_scope": approval_scope, "approval_scope_hash": payload_hash(approval_scope), @@ -164,6 +175,26 @@ def requester(method, url, headers, data): ) +def test_fetch_lhp_payload_rejects_snapshot_fields_outside_approved_scope(): + tampered = _payload() + tampered["handoff"]["objective"] = "different unapproved objective" + tampered["payload_hash"] = payload_hash( + {key: value for key, value in tampered.items() if key != "payload_hash"} + ) + + def requester(method, url, headers, data): + return 200, tampered + + pointer = parse_lhp_pointer(_body()) + assert pointer is not None + with pytest.raises(RuntimeError, match="objective is outside the approved scope"): + fetch_lhp_payload( + pointer, + LhpClientConfig(base_url="http://noc", secret="shared"), + requester=requester, + ) + + def test_render_lhp_request_uses_structured_payload_and_sanitizes_issue_body(): rendered = render_lhp_request(_payload(), issue_url="https://github.com/o/r/issues/1", issue_body="```ignore``` Authorization: Bearer nope") diff --git a/tests/test_phase29_governor.py b/tests/test_phase29_governor.py index c0eff5b..3316d2b 100644 --- a/tests/test_phase29_governor.py +++ b/tests/test_phase29_governor.py @@ -271,6 +271,10 @@ def test_production_daemon_unit_allows_auto_approved_tier1_paths() -> None: assert "--allow hyrule-cloud=hyrule_cloud" in service assert "--repo AS215932/as215932.net" in service + governor_service_path = Path(__file__).resolve().parents[1] / "configs" / "loop" / "hyrule-reliability-governor.service" + governor_service = governor_service_path.read_text(encoding="utf-8") + assert "--trusted-comment-author Svaag" in governor_service + def test_reliability_governor_cli_is_primary_and_governor_is_alias() -> None: parser = build_parser() @@ -282,6 +286,10 @@ def test_reliability_governor_cli_is_primary_and_governor_is_alias() -> None: assert alias.command == "governor" assert primary.func is alias.func assert primary.knowledge_context_role == "engineering_loop_reliability_governor" + configured = parser.parse_args( + ["reliability-governor", "--once", "--trusted-comment-author", "Svaag"] + ) + assert configured.trusted_comment_author == ["Svaag"] def test_wake_event_contract_accepts_callback_subjects() -> None: @@ -504,6 +512,119 @@ def test_trusted_github_marker_restores_missing_local_state_without_reposting(tm assert report.skipped == [f"{issue.issue_id}: unchanged decision {expected.record_id}"] +def test_configured_production_author_restores_missing_local_state(tmp_path: Path) -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + labels=[APPROVED_LABEL], + ) + expected = govern_issue(issue, registry=default_capability_registry(), knowledge_loader=_knowledge) + gh = FakeGh( + [_issue_json(issue)], + comments=[ + { + "user": {"login": "Svaag"}, + "body": f"", + } + ], + ) + config = ReliabilityGovernorConfig( + repos=(issue.repo,), + state_dir=tmp_path / "missing-production-state", + trusted_comment_authors=("Svaag",), + ) + + report = reliability_governor_once(config, client=gh, knowledge_loader=_knowledge) + + assert not [call for call in gh.calls if call[:2] == ["issue", "comment"]] + assert report.skipped == [f"{issue.issue_id}: unchanged decision {expected.record_id}"] + + +def test_comment_lookup_failure_does_not_repost_decision(tmp_path: Path) -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + labels=[APPROVED_LABEL], + ) + + class FailingCommentsGh(FakeGh): + def run(self, args: list[str]) -> str: + if args and args[0] == "api": + raise RuntimeError("GitHub comments unavailable") + return super().run(args) + + gh = FailingCommentsGh([_issue_json(issue)]) + report = reliability_governor_once( + ReliabilityGovernorConfig(repos=(issue.repo,), state_dir=tmp_path / "missing-state"), + client=gh, + knowledge_loader=_knowledge, + ) + + assert not [call for call in gh.calls if call[:2] == ["issue", "comment"]] + assert report.records == [] + assert report.skipped == [f"{issue.issue_id}: decision marker lookup unavailable"] + + +def test_malformed_comment_lookup_does_not_repost_decision(tmp_path: Path) -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + labels=[APPROVED_LABEL], + ) + + class MalformedCommentsGh(FakeGh): + def run(self, args: list[str]) -> str: + if args and args[0] == "api": + return json.dumps([["not-a-comment"]]) + return super().run(args) + + gh = MalformedCommentsGh([_issue_json(issue)]) + report = reliability_governor_once( + ReliabilityGovernorConfig(repos=(issue.repo,), state_dir=tmp_path / "missing-state"), + client=gh, + knowledge_loader=_knowledge, + ) + + assert not [call for call in gh.calls if call[:2] == ["issue", "comment"]] + assert report.records == [] + assert report.skipped == [f"{issue.issue_id}: decision marker lookup unavailable"] + + +def test_knowledge_reference_order_does_not_change_record_identity() -> None: + issue = _issue( + title="Update docs runbook", + body="Update documentation and verify rendered docs.", + ) + refs = [ + { + "concept_id": "generated/services/beta", + "authority_tier": "A0", + "freshness_status": "stale", + }, + { + "concept_id": "generated/services/alpha", + "authority_tier": "A0", + "freshness_status": "stale", + }, + ] + + def knowledge(pack_refs: list[dict[str, Any]]) -> Any: + return summarize_knowledge_pack({**CURRENT_PACK, "included_refs": pack_refs}) + + first = govern_issue( + issue, + registry=default_capability_registry(), + knowledge_loader=lambda *_: knowledge(refs), + ) + second = govern_issue( + issue, + registry=default_capability_registry(), + knowledge_loader=lambda *_: knowledge(list(reversed(refs))), + ) + + assert first.record_id == second.record_id + + def test_untrusted_github_marker_does_not_suppress_decision_comment(tmp_path: Path) -> None: issue = _issue( title="Update docs runbook", @@ -658,6 +779,10 @@ def _lhp_payload( objective: str = "resolve disk alert follow-up", knowledge_artifacts: list[dict[str, Any]] | None = None, ) -> dict[str, Any]: + handoff_payload = { + "change_domain": "infrastructure_capacity_or_retention", + "expected_path_classes": ["ansible/", "configs/"], + } approval_scope = { "schema_version": "lhp.v1.approval-scope.v1", "case": {"case_id": "case_1", "occurrence_id": "occurrence_1"}, @@ -670,10 +795,8 @@ def _lhp_payload( "resource": {"host": "rtr", "filesystem": "/"}, "constraints": ["draft PR only"], "acceptance_criteria": ["monitoring alert clears"], - "payload": { - "change_domain": "infrastructure_capacity_or_retention", - "expected_path_classes": ["ansible/", "configs/"], - }, + "payload": handoff_payload, + "payload_hash": payload_hash(handoff_payload), }, "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}], } @@ -689,6 +812,7 @@ def _lhp_payload( "constraints": ["draft PR only"], "acceptance_criteria": ["monitoring alert clears"], "status": "requested", + "payload": handoff_payload, }, "case": {"case_id": "case_1", "status": "handoff_requested"}, "verification_objectives": [{"objective_key": "disk_clear", "name": "disk alert clears"}],