agnes-the-ai-analyst/cli/lib/pull.py
ZdenekSrotyr 8784f10a6b fix(devin-review): stale-token override + status sessions counter + lock comment
Three Devin Review findings on PR #173 addressed in one commit since
they're in adjacent code paths:

1. cli/commands/init.py:99 (\u{1F534}): `agnes init --token NEW` ran
   step 2 verify against the OLD on-disk token because `get_token()`
   read `~/.config/agnes/token.json` before the env var, and
   `_override_server_env` only set the env var. So `agnes init --force`
   on a machine with a stale token.json failed 401 with a confusing
   'token expired' even though the --token arg was valid.

   Fix: ContextVar-based override in `cli.config._token_override`
   checked by `get_token()` BEFORE the on-disk read.
   `_with_token_override` context manager scopes the override.
   `_override_server_env` now also sets the contextvar via
   `_with_token_override(token)`, so both env var and contextvar
   carry the override (env for back-compat with anything bypassing
   get_token; contextvar is the authoritative source).
   Async-safe (each task sees its own override) and leak-proof
   (resets on context exit).
   2 new tests: regression on stale-disk-token + scope leak guard.

2. cli/commands/status.py:43 (\u{1F7E1}): sessions_pending_upload only
   checked legacy `<workspace>/user/sessions/` and always reported 0
   in workspaces bootstrapped with `agnes init` (Claude Code writes
   to `~/.claude/projects/`, not the legacy path). Same bug we fixed
   for `agnes push` in 08e49591.

   Fix: route through `cli.lib.claude_sessions.list_session_files()`
   so status and push agree on what counts as a pending session.

3. connectors/bigquery/extractor.py:111 (\u{1F7E1}): docstring claimed
   "a live holder still wins the second flock attempt" — incorrect on
   Linux. After `unlink()` + `open()`, the new file is a new inode;
   fcntl.flock keys per-inode, so the old holder's lock does NOT block
   the new acquisition. In a genuine TTL-overrun scenario two writers
   CAN race the parquet.tmp.

   Fix: documentation only. Comment now honestly describes the
   inode-recreation behavior, names the threading.Lock as the actual
   in-process guard, and flags pid-gating as the next-iteration fix
   if real corruption surfaces. The 24h default TTL is well above
   typical COPY durations so the practical risk is low.

Tests: 17/17 across test_cli_init.py + test_lib_pull.py + the broader
regression set.
2026-05-04 21:26:30 +02:00

401 lines
16 KiB
Python

"""`run_pull` — pure data-refresh primitive lifted from `cli/commands/sync.py`.
Pulls the RBAC-filtered manifest from the server, downloads parquets whose
MD5 hash differs from local state, rebuilds DuckDB views, and syncs the
corporate memory bundle to `<workspace>/.claude/rules/km_*.md`.
Contract — Task 8:
- Pure function: no Typer, no stdout, no `sys.exit`. Caller decides what to print.
- Returns a `PullResult` dataclass.
- `dry_run=True` -> no disk writes anywhere (no DB file, no parquet dir,
no rules dir, no sync_state).
- Lazy mkdir: `server/parquet/` is created inside the per-table loop on
first write; `.claude/rules/` is only created when the bundle has at
least one mandatory item or non-empty approved list. Empty inputs leave
the workspace tree alone.
- The DuckDB file at `<workspace>/user/duckdb/analytics.duckdb` is the
load-bearing artifact for every downstream reader (CLI query, hooks),
so it gets created even with zero parquets.
The api_get/stream_download helpers in `cli/client.py` read server URL and
token from `cli.config` (via the `AGNES_SERVER` and `AGNES_TOKEN` env
overrides). To keep `run_pull` callable with explicit `server_url` /
`token` arguments without rewriting the HTTP layer, this module sets those
env vars for the duration of the call and restores the prior values on
exit. That's the cheapest adapter that doesn't bleed into client.py.
"""
from __future__ import annotations
import hashlib
import os
import re
import time
from contextlib import contextmanager
from dataclasses import dataclass, field
from datetime import datetime, timezone
from pathlib import Path
from typing import Iterator
from cli.client import api_get, stream_download
from cli.config import get_sync_state, save_sync_state
@dataclass
class PullResult:
"""Outcome of a `run_pull` invocation.
Fields:
- `tables_updated`: count of parquets actually re-downloaded this run.
- `parquets_total`: count of non-remote tables visible in the manifest.
- `rules_count`: number of `km_*.md` files written to `.claude/rules/`.
- `duration_s`: wall time of the call.
- `errors`: list of `{"table": ..., "error": ...}` (or
`{"stage": "memory_bundle", "error": ...}`) — best-effort flow,
individual failures don't abort the whole pull.
"""
tables_updated: int = 0
parquets_total: int = 0
rules_count: int = 0
duration_s: float = 0.0
errors: list[dict] = field(default_factory=list)
_SAFE_ID_RE = re.compile(r"^[a-zA-Z0-9_\-]{1,128}$")
@contextmanager
def _override_server_env(server_url: str, token: str) -> Iterator[None]:
"""Set AGNES_SERVER + scoped token override for the duration of the call.
`cli.config.get_server_url` honors `AGNES_SERVER`, so the server URL is
swapped via env-var. The TOKEN override is routed through
`cli.config._with_token_override` (a ContextVar), which is checked by
`get_token()` BEFORE the on-disk `~/.config/agnes/token.json`. This is
load-bearing: `agnes init --token NEW` runs the verify call in step 2
while the file still holds an OLD token from a prior install — without
the override, the verify uses the stale on-disk token and fails 401.
`AGNES_TOKEN` env var is also set as a back-compat hint for any code
path that bypasses `get_token()` (none in `cli/` at last audit, but
third-party hooks may), but the contextvar is the authoritative source.
Restores prior values on exit so the caller's environment isn't
mutated permanently. Not safe for concurrent invocation across threads;
single-threaded use only.
"""
from cli.config import _with_token_override
prev_server = os.environ.get("AGNES_SERVER")
prev_token = os.environ.get("AGNES_TOKEN")
os.environ["AGNES_SERVER"] = server_url
if token:
os.environ["AGNES_TOKEN"] = token
try:
with _with_token_override(token):
yield
finally:
if prev_server is None:
os.environ.pop("AGNES_SERVER", None)
else:
os.environ["AGNES_SERVER"] = prev_server
if prev_token is None:
os.environ.pop("AGNES_TOKEN", None)
else:
os.environ["AGNES_TOKEN"] = prev_token
def run_pull(
server_url: str,
token: str,
workspace: Path,
*,
dry_run: bool = False,
) -> PullResult:
"""Refresh local parquets + corporate memory rules from the server.
Mirrors the `_sync_quiet` flow in `cli/commands/sync.py`, minus all
Typer/Rich UI. Returns a `PullResult` summary; never raises for
network/server errors (records them under `errors` instead) so the
caller can decide whether a partial pull is fatal.
"""
started = time.monotonic()
result = PullResult()
workspace = Path(workspace)
with _override_server_env(server_url, token):
# 1. Fetch manifest. A failure here means we can't tell what to
# download at all — record the error and bail out empty-handed.
try:
resp = api_get("/api/sync/manifest")
resp.raise_for_status()
manifest = resp.json()
except Exception as exc:
result.errors.append({"stage": "manifest", "error": str(exc)})
result.duration_s = time.monotonic() - started
return result
server_tables = manifest.get("tables", {}) or {}
local_state = get_sync_state()
local_tables = local_state.get("tables", {})
# 2. Compute the download set, skipping remote-mode tables (no
# parquet on the server) and unchanged hashes.
#
# The parquet-existence check is load-bearing: a stale `sync_state.json`
# entry (hash matches server) is NOT proof the file is on disk. The
# file can disappear between runs — manual rm, disk corruption, an
# operator nuking `server/parquet/` during cleanup, a different
# workspace sharing the same `~/.config/agnes/sync_state.json`
# (TODO(workspace-scoped-sync-state) below) writing one workspace's
# parquets while another reads sync_state and assumes "I already
# have these." Without the existence guard, `agnes pull` would skip
# the download and the downstream DuckDB view rebuild fails on a
# missing file. Hash-equal-but-file-missing → force re-download.
to_download: list[str] = []
non_remote_total = 0
parquet_dir = workspace / "server" / "parquet"
for tid, info in server_tables.items():
if info.get("query_mode") == "remote":
continue
non_remote_total += 1
local_hash = local_tables.get(tid, {}).get("hash", "")
server_hash = info.get("hash", "")
target = parquet_dir / f"{tid}.parquet"
if (
server_hash != local_hash
or tid not in local_tables
or not server_hash
or not target.exists()
):
to_download.append(tid)
result.parquets_total = non_remote_total
# 3. Dry-run short-circuit — touch nothing on disk.
if dry_run:
result.tables_updated = 0 # by definition no writes happened
result.duration_s = time.monotonic() - started
return result
# 4. Download parquets. Lazy mkdir: only create server/parquet/
# when we have at least one table to write into it.
for tid in to_download:
if not parquet_dir.exists():
parquet_dir.mkdir(parents=True, exist_ok=True)
target = parquet_dir / f"{tid}.parquet"
expected_hash = server_tables[tid].get("hash", "")
try:
stream_download(f"/api/data/{tid}/download", str(target))
if expected_hash:
actual_hash = _file_md5(target)
if actual_hash != expected_hash:
target.unlink(missing_ok=True)
raise ValueError(
f"hash mismatch: expected {expected_hash[:12]}, got {actual_hash[:12]}"
)
elif not _is_valid_parquet(target):
target.unlink(missing_ok=True)
raise ValueError("not a valid parquet (missing PAR1 magic)")
local_tables[tid] = {
"hash": expected_hash,
"rows": server_tables[tid].get("rows", 0),
"size_bytes": server_tables[tid].get("size_bytes", 0),
}
result.tables_updated += 1
except Exception as exc:
result.errors.append({"table": tid, "error": str(exc)})
# 5. Persist sync state (only on real runs).
# TODO(workspace-scoped-sync-state): currently saved to
# ~/.config/agnes/sync_state.json (per legacy sync.py behavior).
# Two workspaces sharing one user account share this state.
# Future: scope to <workspace>/.agnes/sync_state.json so workspace
# bootstrap leaves no residue outside <workspace>/.
local_state["tables"] = local_tables
local_state["last_sync"] = datetime.now(timezone.utc).isoformat()
save_sync_state(local_state)
# 6. Rebuild DuckDB views — unconditional. The DB file is the
# load-bearing artifact for downstream readers.
_rebuild_duckdb_views(workspace, parquet_dir)
# 7. Fetch corporate-memory bundle and lazily write
# `.claude/rules/km_*.md`. Best-effort: a server outage on this
# endpoint must not fail the whole pull.
try:
written = _fetch_and_write_rules(workspace)
result.rules_count = written
except Exception as exc:
result.errors.append({"stage": "memory_bundle", "error": str(exc)})
result.duration_s = time.monotonic() - started
return result
# ---------------------------------------------------------------------------
# Helpers — copied verbatim from cli/commands/sync.py with the lazy-mkdir
# fix in `_fetch_and_write_rules`. Task 18 deletes sync.py; until then the
# two copies coexist (no behavior drift, copy not move).
# ---------------------------------------------------------------------------
def _file_md5(path: Path) -> str:
"""MD5 of a file, same chunking as app/api/sync.py:_file_hash so the
client-side verification matches the manifest hash byte-for-byte."""
h = hashlib.md5()
with open(path, "rb") as f:
for chunk in iter(lambda: f.read(8192), b""):
h.update(chunk)
return h.hexdigest()
def _is_valid_parquet(path: Path) -> bool:
"""Cheap structural check — parquet files begin and end with `PAR1`.
Used as a fallback when the manifest has no hash (legacy snapshots) and
during view rebuild to skip obviously-broken files. Does not guarantee
the footer is well-formed — that's DuckDB's job at CREATE VIEW time.
"""
try:
size = path.stat().st_size
if size < 8:
return False
with open(path, "rb") as f:
head = f.read(4)
f.seek(-4, 2)
tail = f.read(4)
return head == b"PAR1" and tail == b"PAR1"
except OSError:
return False
def _rebuild_duckdb_views(workspace: Path, parquet_dir: Path) -> None:
"""Recreate DuckDB views from downloaded parquets. Preserve user tables.
The DuckDB file at `<workspace>/user/duckdb/analytics.duckdb` is
created unconditionally (even on an empty pull) — downstream readers
expect the file to exist. The parquet rebuild loop is a no-op when
`parquet_dir` is missing.
"""
import duckdb
db_path = workspace / "user" / "duckdb" / "analytics.duckdb"
db_path.parent.mkdir(parents=True, exist_ok=True)
conn = duckdb.connect(str(db_path))
try:
# Existing user-created BASE TABLEs we must not shadow with views.
try:
existing_tables = {
row[0] for row in conn.execute(
"SELECT table_name FROM information_schema.tables "
"WHERE table_type='BASE TABLE'"
).fetchall()
}
except Exception:
existing_tables = set()
# Drop all current views so the rebuild is from a clean slate.
try:
views = conn.execute(
"SELECT table_name FROM information_schema.tables WHERE table_type='VIEW'"
).fetchall()
for (view_name,) in views:
conn.execute(f'DROP VIEW IF EXISTS "{view_name}"')
except Exception:
pass
# Recreate views for each parquet file. One broken file (corrupt
# download, partial write left over from a previous run, ...) must
# not abort the whole rebuild — skip and keep going.
if parquet_dir.exists():
for pq_file in parquet_dir.rglob("*.parquet"):
view_name = pq_file.stem
if view_name in existing_tables:
continue
if not _is_valid_parquet(pq_file):
continue
abs_path = str(pq_file.resolve())
try:
conn.execute(
f'CREATE VIEW "{view_name}" AS '
f"SELECT * FROM read_parquet('{abs_path}')"
)
except duckdb.Error:
continue
finally:
conn.close()
def _item_to_md(item: dict) -> str:
"""Render a knowledge item as a Markdown rule file."""
lines = [f"# {item.get('title', 'Untitled')}"]
if item.get("domain"):
lines.append(f"_Domain: {item['domain']}_")
if item.get("category"):
lines.append(f"_Category: {item['category']}_")
lines.append("")
lines.append(item.get("content", ""))
return "\n".join(lines)
def _fetch_and_write_rules(workspace: Path) -> int:
"""Fetch /api/memory/bundle and write `.claude/rules/km_*.md` files.
Returns the count of rule files actually written.
Lazy mkdir contract — Task 8 fix vs. legacy `cli/commands/sync.py`:
the rules directory is created only when the bundle has at least one
mandatory item or a non-empty approved list. An empty bundle leaves
the workspace untouched (no `.claude/rules/` shell, no `km_approved.md`
cleanup attempt against a directory that doesn't exist).
The km_*.md namespace in `.claude/rules/` is server-managed: this
function is the only writer, and it prunes any stale km_*.md files on
every run that materializes the directory. Do not create km_*.md
files manually — they will be removed on next pull.
"""
rules_dir = workspace / ".claude" / "rules"
resp = api_get("/api/memory/bundle")
resp.raise_for_status()
bundle = resp.json()
mandatory = bundle.get("mandatory", []) or []
approved = bundle.get("approved", []) or []
# Lazy mkdir — empty bundle leaves the workspace tree alone.
if not mandatory and not approved:
return 0
rules_dir.mkdir(parents=True, exist_ok=True)
written: set[str] = set()
# One file per mandatory item.
for item in mandatory:
item_id = item.get("id", "")
if not _SAFE_ID_RE.match(item_id):
# Silently skip unsafe ids — caller has no Typer.echo here.
continue
fname = f"km_{item_id}.md"
(rules_dir / fname).write_text(_item_to_md(item), encoding="utf-8")
written.add(fname)
# Approved items roll up into a single file.
if approved:
lines = ["# Approved Corporate Knowledge\n"]
for item in approved:
lines.append(f"## {item.get('title', 'Untitled')}\n")
lines.append(item.get("content", "") + "\n")
(rules_dir / "km_approved.md").write_text("\n".join(lines), encoding="utf-8")
written.add("km_approved.md")
else:
stale = rules_dir / "km_approved.md"
if stale.exists():
stale.unlink()
# Prune stale per-item files no longer mandatory.
for existing in rules_dir.glob("km_*.md"):
if existing.name not in written and existing.name != "km_approved.md":
existing.unlink()
return len(written)