api: standalone read-only HTTP service for insight metrics + concepts#36
Conversation
A thin Starlette front-end over the exact functions the `hyrule-knowledge
insights` CLI uses, so the CLI and the API never compute different numbers.
Read-only — the durable insight ledger is still written only by the
git-tracked, reviewed sync flow.
GET /healthz
GET /v1/insights/metrics?loop=&source=ledger|collector
ledger -> canonical IDQ/CGS/silence over committed inputs
collector -> live metrics over the collector stream
GET /v1/knowledge/concepts/{id} -> title+body+authority+source_refs
Matches the mcp_server pattern: starlette+uvicorn under a new optional `api`
extra (already in the lock via mcp), console script hyrule-knowledge-api,
config via HYRULE_KNOWLEDGE_* env. Tests cover source validation, the
collector-live path (CGS=1.0 on a matched decision+label, loop filter), the
ledger path shape, and graceful 503 when the export is absent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9887313669
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| [project.optional-dependencies] | ||
| mcp = ["mcp>=1.27.0"] | ||
| api = ["starlette>=0.37", "uvicorn>=0.30"] |
There was a problem hiding this comment.
The new api extra is added in pyproject, but uv.lock was not updated, so frozen installs cannot select it. I checked uv sync --help: --extra includes optional dependencies by extra name and --frozen syncs without updating uv.lock; with the current lock metadata, uv sync --extra api --frozen --offline --no-install-project fails with Extra api is not defined, which blocks users/CI from installing the advertised API service via the checked-in lockfile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84cbd2d — the uv.lock update (adding the api extra to the project metadata) was left uncommitted; now committed. uv sync --extra api --frozen (the deploy role command) succeeds. Good catch — that would have broken the deploy.
| collector_url, | ||
| token=os.environ.get("HYRULE_KNOWLEDGE_COLLECTOR_TOKEN") or None, | ||
| ) | ||
| except InsightSyncError as exc: |
There was a problem hiding this comment.
When HYRULE_KNOWLEDGE_COLLECTOR_URL points at a down or timing-out collector, fetch_collector_insights can raise urllib transport OSError/URLError before it raises InsightSyncError; the CLI sync path catches (InsightSyncError, OSError), but this API handler only catches InsightSyncError, so source=collector returns a 500 instead of the intended 502 collector error in that operational case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84cbd2d — the collector branch now catches (InsightSyncError, OSError) and returns 502, matching the CLI sync path. Test: test_metrics_collector_down_returns_502.
| items = fetch_collector_insights( | ||
| collector_url, | ||
| token=os.environ.get("HYRULE_KNOWLEDGE_COLLECTOR_TOKEN") or None, | ||
| ) |
There was a problem hiding this comment.
For source=collector, this async Starlette handler calls the synchronous urllib-based fetch_collector_insights directly; when the collector is slow or waits until its 10s timeout, the uvicorn worker's event loop is blocked, so unrelated requests like /healthz and concept lookups also hang. Run this blocking fetch in a threadpool or make the route synchronous so live collector polling cannot stall the whole API process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84cbd2d — the blocking urllib fetch now runs via starlette.concurrency.run_in_threadpool, so a slow/down collector (up to its 10s timeout) no longer blocks the event loop / /healthz / concept lookups.
| except InsightSyncError as exc: | ||
| return JSONResponse({"error": f"collector: {exc}"}, status_code=502) | ||
| decisions = [ | ||
| item.get("record", {}) for item in items if item.get("record_type") == "decision" |
There was a problem hiding this comment.
If the collector returns a decision item whose record is missing required fields such as insight_id, this comprehension passes the raw dict through and insight_policy_metrics later indexes row["insight_id"], producing a 500 for a malformed collector payload. The sync path validates these same records with the agent-core contracts before use, so the live API path should either validate and return a collector error or skip rejected records rather than crashing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84cbd2d — live collector records are now validated with the same _validate_decision / _validate_label the ledger path uses (agent-core contracts + safety + created_at); malformed rows are skipped rather than crashing insight_policy_metrics. Test: test_metrics_collector_skips_malformed_records (bad record dropped, valid pair still scores CGS 1.0).
| ) | ||
|
|
||
| if source == "ledger": | ||
| decisions, labels = load_insight_policy_fixtures() |
There was a problem hiding this comment.
When the API is launched as an installed service from a working directory that does not contain evals/fixtures/insights and ledger/insights, this call still asks load_insight_policy_fixtures() to read those relative paths from cwd, even if HYRULE_KNOWLEDGE_CONFIG points at the real repository/export. In that deployment, source=ledger silently reports empty canonical metrics rather than the committed insight rows, so resolve the ledger fixture paths relative to the configured bundle/repo instead of process cwd.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84cbd2d — main() now os.chdirs to the configured bundle repo (parent of HYRULE_KNOWLEDGE_CONFIG) before serving, so source=ledger resolves ledger/ + evals/fixtures/ against the real checkout, not the launch cwd. (The deploy also sets WorkingDirectory to the checkout, but this makes it robust regardless.)
- Commit uv.lock with the `api` extra (was left uncommitted, so `uv sync --extra api --frozen` — the deploy command — failed to select it). - Run the blocking urllib collector fetch via run_in_threadpool so a slow/down collector can't stall /healthz + concept lookups on the event loop. - Catch OSError/URLError (down collector) as 502, matching the CLI, not a 500. - Validate live collector records with the same contracts the ledger path uses; skip malformed rows instead of letting insight_policy_metrics crash on them. - main() chdirs to the configured bundle repo so source=ledger resolves ledger/ + evals/fixtures/ regardless of launch cwd. Tests: collector-down -> 502; malformed record skipped (valid pair still scores). 157 passed, ruff + mypy clean, uv sync --extra api --frozen green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84cbd2d660
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| try: | ||
| out.append(validator(item.get("record", {}), _COLLECTOR_SOURCE)) | ||
| except InsightPolicyError: |
There was a problem hiding this comment.
When the collector returns a decision or label whose record has created_at but still fails the agent-core model contract, _validate_decision/_validate_label raises pydantic's validation error rather than InsightPolicyError, so this helper lets it escape and source=collector returns a 500 instead of skipping/rejecting the bad collector row. Fresh evidence after the earlier malformed-record thread: the new helper only catches InsightPolicyError, while the model validation call inside the reused validators is still outside that exception type.
Useful? React with 👍 / 👎.
| except (InsightSyncError, OSError) as exc: | ||
| return JSONResponse({"error": f"collector: {exc}"}, status_code=502) |
There was a problem hiding this comment.
If the collector responds with HTTP 200 but an invalid JSON body (or bytes that cannot decode as UTF-8), fetch_collector_insights raises JSONDecodeError/UnicodeDecodeError, which is not caught here, so source=collector returns a 500 even though the upstream collector response is the failing dependency. The same handler already maps bad collector transport and shape errors to 502, so include decode failures in that collector-error path as well.
Useful? React with 👍 / 👎.
| decisions = _validated_records(items, "decision", _validate_decision) | ||
| labels = _validated_records(items, "label", _validate_label) |
There was a problem hiding this comment.
When the live collector response contains duplicate decision or label items for the same insight_id/label_id (for example from overlapping replay/retry data, which the sync path explicitly dedupes), these raw lists are passed straight into insight_policy_metrics. That function counts raw decisions for decision_count/action counts and raw matching labels for IDQ, so source=collector can inflate counts and weight duplicated labels differently from the reviewed ledger for the same records.
Useful? React with 👍 / 👎.
| # committed rows regardless of where the service was launched. | ||
| config = Path(_config_path()) | ||
| if config.exists(): | ||
| os.chdir(config.resolve().parent) |
There was a problem hiding this comment.
When HYRULE_KNOWLEDGE_CONFIG is a relative path from the service launch directory, this chdir makes later calls to _config_path() reuse that same relative string from the new cwd. For example, launching from /srv with HYRULE_KNOWLEDGE_CONFIG=knowledge/knowledge.config.yml changes into /srv/knowledge, then request handlers try to load /srv/knowledge/knowledge/knowledge.config.yml, so concept lookups and store-backed metrics can 500 after startup; keep the resolved config path or update the env before changing cwd.
Useful? React with 👍 / 👎.
1 participant
Context
Operator wants the insight metrics steerable from the Observatory instead of SSHing to run
hyrule-knowledge insights .... Per the agreed design (network-operations #TBD): reads become a fast API, writes stay git. This is the read API — a standalone service (UNIX-philosophy), not folded into knowledge-mcp.Change
New
hyrule_knowledge/api.py— a thin Starlette front-end over the same functions the CLI uses (insight_policy.insight_policy_metrics,load_insight_policy_fixtures,insight_sync.fetch_collector_insights,KnowledgeStore), so CLI and API can't drift.mcp_serverpattern:starlette+uvicornunder a new optionalapiextra (already resolved in the lock viamcp), console scripthyrule-knowledge-api, config viaHYRULE_KNOWLEDGE_*env.sourcesplit is the crux:collectorgives the operator instant feedback after labelling;ledgeris the deterministic number gate promotions must use.Tests
tests/test_api.py: source validation (400), collector-without-URL (503), the live collector path (matched decision+label → CGS 1.0, loop filter drops the soc record), the ledger-source shape, and a graceful 503 when the sqlite export is absent (never a 500). Full suite 155 passed; ruff + mypy clean;uv sync --group dev --frozenunchanged.Next (separate PRs)
hyrule-knowledge-apion loop (mirrorknowledge_mcp), firewall + monitoring./insights/metricspage consumingsource=collector+source=ledger, and a "Sync now" button that dispatches the reviewed sync job.🤖 Generated with Claude Code