Skip to content

Integrate Knowledge Loop with LHP v2#41

Draft
Svaag wants to merge 4 commits into
mainfrom
feat/agentic-coordination-v2
Draft

Integrate Knowledge Loop with LHP v2#41
Svaag wants to merge 4 commits into
mainfrom
feat/agentic-coordination-v2

Conversation

@Svaag

@Svaag Svaag commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a signed LHP-v2 Knowledge coordinator worker for governed context and gap requests
  • add the SOC-specific read-only soc_shadow policy and security context-pack sections
  • stage approved loop learning as sanitized A4 proposals for the existing reviewed Knowledge PR flow
  • index SOC, agent-core, and Agentic Observatory as source repositories

Safety boundaries

  • caller roles derive from authenticated loop identity
  • coordinator intake cannot promote A1/A2 knowledge
  • raw logs, command output, secrets, and uncited learning fail validation
  • all proposed learning retains adversarial validation and human PR review

Validation

  • 179 pytest tests
  • Ruff and strict mypy
  • OKF validation, quality, export, eval, ledger, lifecycle, and secret scan

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 88
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Determinism Risk

The _resolve_context method opens a KnowledgeStore connection inside the synchronous _handle method, but the SQLite store path is a class-level default (exports/knowledge.sqlite). If the store is concurrently written by another process (e.g., a generator run) between the heartbeat and the context assembly, the context pack output could differ between runs with identical inputs. This breaks the byte-stable regeneration guarantee required by export --check and the loop's volatile-churn detection. The store should be opened once at worker startup or the connection should be held read-only for the duration of the cycle.

with KnowledgeStore(self.store_path) as store:
    pack = build_context_pack(
        task=task,
        role=role,
        store=store,
        risk_level=envelope.risk_level,
        token_budget=6000,
        max_chars=20_000,
        task_id=envelope.work_item_id or envelope.handoff_id,
        policy_path=self.policy_path,
    )
Missing Input Validation

In _stage_learning_proposal, the event_type is extracted from envelope.payload.get("event_type") and validated against allowed_event_types, but the producer field in the learning event is hardcoded from the profile lookup (producer, allowed_event_types = profile). The caller can supply a producer in the payload that conflicts with the derived producer. If the caller's payload contains "producer": "engineering_loop" while the source loop is soc, the event will be staged with the wrong producer. This could allow a SOC loop to masquerade as an engineering loop in the learning ledger.

producer, allowed_event_types = profile
event_type = _text(envelope.payload.get("event_type"), limit=100)
if event_type not in allowed_event_types:
    raise ValueError(
        f"{envelope.source_loop} may not propose learning event type {event_type!r}"
    )
citations = _citations(record)
if not citations:
    raise ValueError("learning proposal requires at least one cited source or context pack")
context_pack_ids = [
    citation["context_pack_id"] for citation in citations if "context_pack_id" in citation
]
raw_lessons = envelope.payload.get("lessons")
lessons = (
    [_text(item, limit=1000) for item in raw_lessons[:20]]
    if isinstance(raw_lessons, list)
    else []
)
raw_metrics = envelope.payload.get("metrics")
metrics = _bounded(raw_metrics) if isinstance(raw_metrics, dict) else {}
event = build_local_learning_event(
    producer=producer,
    event_type=event_type,
    subject=_text(
        envelope.payload.get("subject") or envelope.case_id or envelope.work_item_id,
        limit=500,
    ),
    summary=_text(
        envelope.payload.get("summary") or envelope.summary or envelope.intent,
        limit=4000,
    ),
    citations=citations,
    status="proposed",
    authority_tier="A4",
    context_pack_ids=context_pack_ids,
    metrics=metrics,
    lessons=lessons,
    metadata={
        "coordination_handoff_id": envelope.handoff_id,
        "coordination_scope_hash": envelope.scope_hash,
        "source_loop": envelope.source_loop,
        "adversarial_validation": "passed",
        "direct_promotion": False,
    },
)
Race Condition

The _write_proposal method uses a temporary file with os.replace for atomic writes, but the collision check (if target.exists()) and the write are not atomic. If two concurrent worker instances process the same proposal (e.g., due to a coordinator re-delivery), both could pass the existence check, write different temporary files, and the second os.replace would silently overwrite the first. This could lead to data loss or inconsistent proposal states. The collision check should be inside the exclusive file creation.

def _write_proposal(self, event: dict[str, Any]) -> Path:
    self.proposal_dir.mkdir(parents=True, exist_ok=True, mode=0o750)
    target = self.proposal_dir / f"{event['id']}.json"
    encoded = json.dumps(event, sort_keys=True, indent=2) + "\n"
    if target.exists():
        if target.read_text(encoding="utf-8") != encoded:
            raise LearningLedgerError(f"proposal id collision: {event['id']}")
        return target
    temporary = target.with_name(f".{target.name}.{uuid4().hex}.tmp")
    descriptor = os.open(temporary, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
    try:
        with os.fdopen(descriptor, "w", encoding="utf-8") as handle:
            handle.write(encoded)
            handle.flush()
            os.fsync(handle.fileno())
        os.replace(temporary, target)
    finally:
        if temporary.exists():
            temporary.unlink()
    return target

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safely access optional attribute

The HandoffEnvelope may not always have a risk_level attribute. If it is missing,
this will raise an AttributeError. Use getattr with a safe default to avoid crashes
when the field is absent.

src/hyrule_knowledge/coordination.py [200-219]

-role = _CONTEXT_ROLES.get(envelope.source_loop)
+risk_level = getattr(envelope, 'risk_level', None)
 ...
 with KnowledgeStore(self.store_path) as store:
     pack = build_context_pack(
         task=task,
         role=role,
         store=store,
-        risk_level=envelope.risk_level,
+        risk_level=risk_level,
         ...
     )
Suggestion importance[1-10]: 6

__

Why: The suggestion adds defensive access for the risk_level attribute, but in this PR the HandoffEnvelope is consistently expected to have that field, so it's a low-impact precaution. Score capped for error handling suggestions.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant