Skip to content

fix(observability): avoid exposing live AMS ledgers#5471

Merged
JSONbored merged 2 commits into
mainfrom
codex/fix-exposure-of-ams-ledgers-in-grafana
Jul 12, 2026
Merged

fix(observability): avoid exposing live AMS ledgers#5471
JSONbored merged 2 commits into
mainfrom
codex/fix-exposure-of-ams-ledgers-in-grafana

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The original provisioning wired Grafana directly to the miner's live SQLite ledgers, which can expose free-form reason/payload_json and private prediction signals to any Grafana principal with query capability, creating a confidentiality risk.
  • The project already uses a safer pattern where Grafana reads a redacted reporting export (see sqlite.yml), so the AMS datasources should follow that pattern instead of mounting live app files.

Description

  • Repoint the AMS Grafana datasources from the live miner paths to redacted reporting-export paths by changing the datasource paths to /reporting/ams-attempt-log.sqlite and /reporting/ams-prediction-ledger.sqlite in grafana/provisioning/datasources/ams-ledgers.yml.
  • Update the miner observability documentation in packages/gittensory-miner/docs/observability.md to require publishing sanitized reporting exports into Grafana rather than mounting GITTENSORY_MINER_CONFIG_DIR into Grafana.
  • Strengthen the unit test test/unit/ams-ledgers-datasource.test.ts to assert the provisioned datasource paths live under /reporting and do not reference /ams-ledgers or the raw ledger filenames.

Testing

  • Ran npm run selfhost:validate-observability, which completed successfully and reported valid dashboards/alert rules. (passed)
  • Ran the updated unit test with npm test -- --run test/unit/ams-ledgers-datasource.test.ts, which ran the three tests and passed. (passed)
  • Ran npm run docs:drift-check to verify documentation parity, which passed. (passed)
  • Attempted the full gate with npm run test:ci; it progressed through many checks but stopped at the Cloudflare typegen check because worker-configuration.d.ts was reported stale (this change did not modify Cloudflare bindings). (stopped/failure)
  • npm audit --audit-level=moderate was attempted but the registry audit endpoint returned 403 Forbidden (external network/service issue), so the audit step could not complete. (skipped/failed)

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored
JSONbored marked this pull request as draft July 12, 2026 21:51
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-12 22:38:29 UTC

7 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR closes a real confidentiality gap by repointing the AMS Grafana datasources from live miner SQLite ledgers (which carry free-form `reason`/`payload_json`) to a new redacted reporting export, mirroring the existing `sqlite.yml` pattern. The new export script, compose service, docs, and tests are well-targeted and the datasource/test changes are correct as shown. However, the export script's core `ATTACH`/`INSERT` step in `export_ledger()` is unguarded against failure under `set -eu`, which contradicts the script's own stated fail-open/isolation invariant.

Blockers

  • scripts/export-ams-reporting-db.sh: inside `export_ledger()` the `sqlite3 -cmd ".timeout 5000" "$src" "ATTACH ...; INSERT ...; DETACH report;"` call (around the block right before the `PRAGMA quick_check` check) has no `|| true`/error guard, so under `set -eu` (line 2) a busy/locked live ledger (plausible per the miner's own documented `busy_timeout`/concurrent-writer behavior) or a transient insert failure aborts the whole script immediately — breaking the explicit claim that 'a bad one must not block the other' since the second ledger's `export_ledger` call never runs.
  • scripts/export-ams-reporting-db.sh:105 computes `fingerprint="script=$SCRIPT_VERSION;$(append_only_fingerprint "$src" "$tbl" "$time_col")"` under `set -e`, so a locked/corrupt source DB makes the whole script exit before the second ledger is exported; catch that failure inside `export_ledger` and return 0 after preserving last-good output, e.g. `if ! fp_tail="$(append_only_fingerprint "$src" "$tbl" "$time_col" 2>/dev/null)"; then echo "[ams-reporting:$label] export skipped: unable to fingerprint $src; preserving last-good $out" >&2; return 0; fi; fingerprint="script=$SCRIPT_VERSION;$fp_tail"`.
Nits — 5 non-blocking
  • scripts/export-ams-reporting-db.sh: `hash_stdin()` is defined but never called anywhere in the script — dead code that should be removed or wired in.
  • docker-compose.yml and .env.example continue to grow past ~800/500 lines respectively; not introduced by this diff but worth a future split if more profiles keep landing here.
  • Wrap the risky `sqlite3 ATTACH/INSERT/DETACH` invocation in `export_ledger()` with `|| { echo '[ams-reporting:$label] export failed: ...' >&2; rm -f "$tmp" ...; return 0; }` so a locked/busy source ledger degrades to 'preserve last-good' instead of killing the whole export run.
  • test/unit/selfhost-ams-reporting.test.ts: add a test that simulates a locked/busy source db (e.g. hold a transaction open) for the attempt-log export and asserts the prediction-ledger export still completes — this would have caught the `set -eu` propagation gap.
  • Remove or use `hash_stdin()` in scripts/export-ams-reporting-db.sh.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue — Link the relevant issue (for example `Closes #123`) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 44 registered-repo PR(s), 36 merged, 435 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 44 PR(s), 435 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ⚠️ ℹ️ Insufficient signal risk: clean · value: insufficient-signal — Nothing measurable for the structural-improvement analyzers on this PR (e.g. no code files changed).
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 44 PR(s), 435 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.72%. Comparing base (29c18f2) to head (58f9af0).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5471   +/-   ##
=======================================
  Coverage   94.72%   94.72%           
=======================================
  Files         559      559           
  Lines       44664    44664           
  Branches    14664    14664           
=======================================
  Hits        42308    42308           
  Misses       1621     1621           
  Partials      735      735           
Flag Coverage Δ
shard-1 44.04% <ø> (ø)
shard-2 35.20% <ø> (ø)
shard-3 32.06% <ø> (+0.05%) ⬆️
shard-4 31.99% <ø> (-0.06%) ⬇️
shard-5 32.73% <ø> (+0.05%) ⬆️
shard-6 43.69% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored force-pushed the codex/fix-exposure-of-ams-ledgers-in-grafana branch from 3410f9b to bab9f57 Compare July 12, 2026 22:06
@JSONbored JSONbored self-assigned this Jul 12, 2026
…rafana

Completes the fix this PR only half-implemented: the datasource/docs already
pointed at /reporting/ams-attempt-log.sqlite and /reporting/ams-prediction-ledger.sqlite,
but nothing produced those files, and docker-compose.yml never mounted the live
ledger directory anywhere -- the datasource was already inert out of the box.

Adds scripts/export-ams-reporting-db.sh (mirrors export-grafana-reporting-db.sh's
incremental-fingerprint + atomic-write shape, simplified for two independent
SQLite-only, insert-only sources) and a new ams-reporting-exporter service under
a dedicated ams-observability compose profile: it mounts the miner's live
GITTENSORY_MINER_CONFIG_DIR read-only (Grafana never gets this mount), drops
attempt_log_events' free-form reason/payload_json columns, and writes the
redacted snapshot into the same shared reporting volume Grafana already reads.
predictions/blocker_codes_json/warning_codes_json pass through unchanged --
already bounded, engine-defined codes, not free text.

Also documents the new profile/env vars in .env.example and observability.md.
@JSONbored
JSONbored marked this pull request as ready for review July 12, 2026 22:28
@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 12, 2026
@JSONbored
JSONbored merged commit 2fd9196 into main Jul 12, 2026
19 checks passed
@JSONbored
JSONbored deleted the codex/fix-exposure-of-ams-ledgers-in-grafana branch July 12, 2026 23:01
JSONbored added a commit that referenced this pull request Jul 14, 2026
…serve-last-good (#5757)

An instance whose ams-attempt-log source has never existed (an engine-only
deployment with no co-located miner) creates its one-and-only export under
whichever DDL was current at that moment, then export_ledger's "source
missing, output already exists" fail-open path preserves it forever --
the fingerprint/SCRIPT_VERSION rebuild check is unreachable while the
source stays absent, so a later DDL widening (PR #5637 added
provider/cost_usd/tokens_used to attempt_log_events a day after PR #5471
first shipped this script) was previously invisible to such an instance
permanently. Every miner-usage.json panel selecting those columns then
hard-fails with "no such column" instead of just showing empty. Fixed by
reconciling the preserved output's schema against the current DDL's
declared columns (ALTER TABLE ADD COLUMN, mirroring
packages/gittensory-miner/lib/attempt-log.js's own ensureOutcomeColumns
pattern) whenever a preserve-last-good path fires -- additive only, so
existing rows and their data are never touched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant