Adds whole-Store backup/restore primitives so an external CI/CD job can
mirror the Store to a git repo (and restore back from one).
REST:
- GET /api/store/bundle.zip — deterministic ZIP of all (filtered) Store
entities. Layout: manifest.json + entities/<id>/{plugin,assets}/.
Manifest carries owner_email for cross-instance restore. Auth: any
authenticated user (Store is community-open).
- POST /api/store/import-bundle — admin-only restore. Modes
merge|replace|skip; owner resolution by email with stub-disabled-user
fallback when the email is unknown on the target instance.
CLI:
- agnes store update <id> [--description X] [--zip PATH] ... — in-place
edit (server PUT permits owner OR admin per F4). Closes the missing
edit affordance for analysts who want to fix a typo or push a new
ZIP without losing install_count.
- agnes store pull [-o store.zip] [--unpack DIR] — download the bundle.
--unpack streams + extracts so an external git-backup workflow can
drop the tree straight into a repo and `git add .`.
- agnes store info [--json] — counts + size summary.
- agnes admin store push <zip-or-dir> [--mode ...] — admin-only restore.
Auto-zips a directory client-side so a working-tree → server
round-trip is one command.
cli/v2_client.py gains api_get_stream helper for binary downloads.
Tests: 5 new server tests (bundle shape + filters + round-trip + stub
user creation + skip mode + admin-only gate) + 11 new CLI tests
(update, pull/unpack, info, admin push). 66/66 store-related tests
green locally.
create_entity + update_entity created the `scratch` temp dir inside one
try/finally but cleaned it up in a separate one. Validation HTTPExceptions
raised by _safe_zip_extract (zip_unsafe_path, zip_too_large_uncompressed)
or the BadZipFile→422 conversion exited the first scope, and the second
finally was never entered → temp dir leaked on every failed upload.
Devin flagged this on the F2 commit. The leak pre-existed (zip_unsafe_path
was the original vector); F2 added zip_too_large_uncompressed to the same
broken cleanup path. Fixed by collapsing scratch creation + cleanup into
one outer try/finally that covers both extraction AND metadata/bake; the
inner try/except/finally still handles BadZipFile→422 + tmp file cleanup.
Same restructure in update_entity. Regression test
`test_scratch_dir_cleaned_up_after_failed_extraction` triggers a
zip_unsafe_path 422 and asserts tmp/agnes_store_* contains no leaked
dirs.
Three independent reviews of PR #180 surfaced four real defects in the new
Store / my-ai-stack surface. CHANGELOG entries detail each; one-liners:
- F1 video_url XSS: any authenticated user could upload a Store entity
with `video_url=javascript:...` and pop XSS in any viewer's session via
the `<a href=...>` "Watch video" link in store_detail.html. Jinja2
autoescape doesn't block URI schemes inside attribute values. Fixed by
scheme-validating to http(s) only on create + update; 400 invalid_video_url.
- F2 ZIP decompression bomb: _safe_zip_extract checked path-traversal but
not declared file_size totals — a 50 MB compressed upload at 1:1000
ratio decompresses to 50 GB and DOS the host disk. Fixed by summing
zinfo.file_size across infolist() and refusing > 200 MB before
extractall touches disk. 413 zip_too_large_uncompressed.
- F4 admin authz parity: PUT /api/store/entities/{id} was owner-only while
DELETE allowed owner OR admin; the store-detail page hid Edit/Delete
buttons from admin even though DELETE was permitted. Fixed by allowing
admin on PUT and passing is_admin to the template; gate is now
is_owner OR is_admin everywhere.
- F5 cross-owner suffix collision: sanitize_username is many-to-one
(alice.smith / alice_smith both → alice-smith). Two such users uploading
entities with the same display name produced identical
`<name>-by-<username>` suffixes, silently colliding in the served
agnes-store-bundle on-disk paths AND the manifest catalog (Claude Code
dedupes by plugin.json `name`). Fixed by enforcing global uniqueness on
the suffixed value at create_entity; 409 conflict_global_suffix.
F3 (ZIP symlink members) was investigated and confirmed to be a
false-positive — Python's stdlib ZipFile.extractall does not honor
symlink mode bits, so no exploit exists.
9 new regression tests in tests/test_store_api.py::TestStoreSecurityFixes
covering all four. Test run locally: 60/60 store-related tests pass.
Per CLAUDE.md vendor-agnostic OSS guidance — replace the real
groupon.com email used as a sanitize_username() example with a
placeholder (alice_smith@example.com).
`compute_default_agent_prompt` (which renders the install commands in
the setup prompt's marketplace block) was calling
`resolve_allowed_plugins` — the admin-only feed that predates the
v25 Store/opt-out layer. Result: a user with 2 opted-out curated
plugins + 2 Store skills saw the original 4 admin grants in the
install list (including the opted-out ones, with cross-marketplace
duplicates), and no `agnes-store-bundle` install line for the skills.
Now we call `resolve_user_marketplace` — the same resolver that
`/marketplace.zip` + `/marketplace.git/` serve from. The install
commands now match the served catalog exactly: admin grants minus the
user's opt-outs, plus the `agnes-store-bundle` synth plugin (which
wraps every installed Store skill + agent into one plugin entry) and
any standalone Store plugin uploads.
Dedup by `manifest_name` because two upstream marketplaces shipping a
plugin with the same name collide in the synth marketplace.json by
design (CLAUDE.md "Same-named plugins ... collide in the catalog by
design"). A duplicate `claude plugin install <name>@agnes` would be a
no-op anyway, so it's just visual noise to keep emitting both.
`_read_agnes_ca_pem()` decides whether the served fullchain.pem needs
trust-bootstrapping in the rendered setup prompt. Pre-fix it only
checked the leaf's *immediate* issuer against `certifi`'s trust store.
For Let's Encrypt that's the intermediate (R13), which `certifi` does
not ship — only roots are in trust stores. So a publicly-trusted LE
chain still tripped the "needs bootstrap" path and the setup prompt
emitted a step-0 TLS trust block + clone-fallback marketplace block
that no client actually needs (Bun-compiled `claude.exe`, system git,
Python via certifi all validate the chain through the bundled ISRG
Root X1).
Now we walk every cert in the fullchain (leaf + intermediates) and
return None the first time any cert's issuer is in the certifi trust
store — that captures the standard "leaf signed by intermediate signed
by publicly-trusted root" shape. Trusted subjects are read once into
a set for O(1) lookup. Self-signed (leaf.issuer == leaf.subject) and
private-CA chains (no chain link's issuer in certifi) keep their
previous "return PEM" behavior, so deployments that genuinely need
the bootstrap still get it.
Validated end-to-end against the live VM at
agnes-marustamyan.groupondev.com (LE R13 → ISRG Root X1):
- Let's Encrypt fullchain → has_ca=False (was True)
- Self-signed cert → has_ca=True
- Corporate-CA chain (private root) → has_ca=True
- Missing fullchain.pem → has_ca=False
Past migration finalize steps RENAME / DROP COLUMN / ALTER on the
`users` table (e.g. _v12_to_v13_finalize, _v13_to_v14_finalize,
_v17_to_v18_finalize, the v5 backfill). DuckDB rejects an ALTER on a
table that any other table references via FOREIGN KEY, so the new
store_entities / user_store_installs / user_plugin_optouts entries —
which the self-heal pass writes to _SYSTEM_SCHEMA before the migration
ladder runs — broke 6 legacy-migration tests with:
Cannot alter entry "users" because there are entries that depend on it
Pre-existing convention (see personal_access_tokens at v6) is to omit
FK constraints to `users` and validate user existence at the app
layer. Sync the three v25 tables with that convention. Same edit in
both _SYSTEM_SCHEMA and _V24_TO_V25_MIGRATIONS so fresh installs and
upgraded installs land in the same shape.
App-level cascade behavior is unchanged: store entity DELETE explicitly
deletes user_store_installs rows in app/api/store.py, and the admin
grant-deletion hook explicitly deletes user_plugin_optouts rows for the
plugin. The dropped FK constraints were defense-in-depth, not the only
guard.
Adds a community-driven Store where any authenticated user uploads
skills/agents/plugins as ZIPs, plus /my-ai-stack as the per-user
composition view. The served Claude Code marketplace is now:
(admin_granted ∖ opt_outs) ∪ store_installs
Skill + agent installs are merged into a single `agnes-store-bundle`
plugin in the served marketplace; type=plugin uploads stay standalone.
Names are suffixed with `-by-<owner-username>` at upload time so two
owners can use the same display name without colliding in Claude Code's
flat skill/agent namespace.
Schema v23 → v24 adds three tables:
- store_entities — community-uploaded skills/agents/plugins
- user_store_installs — what each user has chosen to install
- user_plugin_optouts — opt-out overlay on top of admin grants
Admin grant-delete drops every user's opt-out for that plugin so
re-grant resets cleanly to enabled (no sticky personal preference).
UI:
- /store — e-commerce-style listing with type/category/owner
filters, search, pagination, owner-aware [Install]
buttons, clickable cards
- /store/new — 2-step upload wizard with drag & drop, preview
validation (POST /api/store/entities/preview), docs
multi-upload, photo + video URL
- /store/{id} — detail page with hero, file list, docs, owner
actions (Edit/Delete) for the uploader
- /my-ai-stack — Granted plugins (toggle opt-out) + From the Store
(uninstall) sections
- Admin nav: Marketplaces moved into Admin dropdown, renamed to
"Curated Marketplaces"
Validation hardening: type-mismatch guards reject skill ZIP uploaded as
agent (or vice versa), and plugin ZIPs masquerading as skills/agents.
Human-readable error messages mapped client-side from machine codes.
Cross-source naming: Store entity-id-prefixed dirs (`plugins/store-<id>/`)
plus the bundle (`plugins/store-bundle/`) avoid collisions with admin
marketplaces (whose `store` slug is reserved by `is_valid_slug`).
Bundle composition is content-hashed at serve time — install/uninstall
or owner re-upload bumps the bundle's plugin.json `version`, so Claude
Code's auto-update toggle picks up changes.
Tests: 50+ new tests across naming, repositories, filter (admin ∪ store
∪ bundle), API (upload/install/uninstall/delete/preview/docs), end-to-end
marketplace.zip with bundle merging.
Pre-fix: when v24 migration found rows to migrate but
data_source.bigquery.project was empty, it logged a warning per row
and returned normally. Schema_version then bumped to 24 unconditionally
→ next start's 'if current < 24:' gate skipped _v23_to_v24_finalize
forever, leaving rows in DuckDB-flavor SQL that the new
_wrap_admin_sql_for_jobs_api wrapping path rejects.
Devin escalated this from advisory ("idempotent retry") to critical
on rescan after my reply. The reply was wrong — the LIKE filter inside
the function gives idempotency IF the function is called again, but
the schema-version gate prevents that call from happening.
Fix (Devin's recommended Approach 1): raise RuntimeError BEFORE the
schema-version bump when rows need migration but project_id is empty.
The schema_version stays at 23, so on next start the 'if current < 24:'
gate fires and the migration runs again — this time with project_id
configured.
Side effect: a BQ-using deployment that hasn't set the project blocks
startup until they do. That's the right call for a config error that
would otherwise silently break all materialized tables. The error
message points at the right knob (data_source.bigquery.project +
restart).
No-rows-no-block invariant preserved: the early 'if not rows: return'
at the top of _v23_to_v24_finalize means non-BQ deployments are
unaffected.
Tests:
- test_v24_raises_when_project_not_configured_and_rows_need_migration:
asserts raise + schema_version stays at 23 (the load-bearing
invariant for retry-on-next-start to work)
- test_v24_skips_clean_when_no_rows_match_even_without_project:
asserts non-BQ deployments don't block startup
- Existing 3 tests still pass
Three items from operator feedback after running the actual flow:
(1) Help docstring lied: "--bucket / --source-table ignored" for
materialized rows. Reality: --bucket is load-bearing because
`agnes schema <name>` builds the BQ identifier as
`bq.<bucket>.<source_table>`. An empty bucket registered the row but
broke schema/describe with HTTP 400 "unsafe BQ identifier in
registry". Fix: docstring rewritten to reflect reality, plus
client-side validation rejects materialized + empty bucket with a
clear error pointing at the right knob.
(2) Post-register UX cliff: `agnes pull` after register-table reports
"Updated 0 tables (1 total)" because registration adds a registry
row but does NOT trigger a parquet build. Operators routinely
assume something's broken when they need to run
`agnes setup first-sync` to kick off the materialization. Hint
emitted on success now points at first-sync.
(3) RBAC gotcha: `agnes catalog` is RBAC-filtered via
`resource_grants`, so non-admin users don't see freshly-registered
rows until a grant is created. Hint emitted on success now points at
`agnes admin grant create <group> table <name>`.
Tests: 8/8 in test_cli_admin_materialized.py, including two new
regression tests for the validation + the hint output.
The iterative bare-name rewriter (one re.sub per name, longest-first)
was vulnerable to cross-contamination when the GCP project ID contained
a registered table name as a hyphen-delimited word.
Concrete repro:
project = 'my-ue-project'
registered = ['orders', 'ue']
user SQL = 'SELECT * FROM orders JOIN ue ON ...'
iter 1 (orders): produces 'FROM `my-ue-project.fin.orders` JOIN ue ...'
iter 2 (ue): '\bue\b' matches 'ue' INSIDE 'my-ue-project' (hyphen
creates word boundary on both sides) — corrupts
the iter-1 path
Fallback at query.py:576 caught the resulting BQ parse error and fell
back to per-table SELECT * estimate, so impact was over-estimation,
not fail-open — but the #171 partition-pruning fix silently degraded
to pre-fix behavior whenever a project name shared a hyphen-segment
with a registered table.
Fix: single re.sub call with an alternation regex sorted longest-first.
Single-pass means each source position is processed exactly once, so
freshly-inserted backticked text from one match isn't re-scanned by
later names in the alternation.
Regression test
test_rewrite_helper_does_not_corrupt_when_project_id_contains_registered_name
covers the exact Devin repro.
`_try_acquire_file_lock` opened the lock file with `open(mode='w')`
BEFORE the mtime check, which truncated the file and refreshed mtime
to now. The subsequent age check always saw ~0, so the TTL reclaim
branch was never reachable and `materialize.lock_ttl_seconds` was
a silently no-op config knob.
Repro:
before open(w): mtime age = 100000s
after open(w): mtime age = 0s
Fix: stat the lock path BEFORE any open(). If pre-probe mtime is
older than TTL, unlink (forcing a fresh inode for the open + flock
that follows). Order is now stat-then-decide-then-probe, not
probe-then-stat-then-decide.
Two regression tests added in tests/test_bq_materialize_concurrency.py:
- test_stale_held_lock_is_reclaimed_despite_live_holder — exercises
the full reclaim path with a still-living fcntl holder. Pre-fix
this returned None (in_flight forever); post-fix returns a holder
fd on a new inode.
- test_failed_probe_does_not_self_refresh_lock_mtime — sister test
pins that a failed acquisition's mode='w' truncate doesn't
pathologically loop.
Residual cross-process risk (genuinely overrunning materialize past
TTL races a fresh attempt — both write to the same parquet.tmp,
inode-level flock independence means new acquisition succeeds while
old holder is still alive) stays documented in the helper docstring.
In-process threading.Lock keyed on table_id blocks the single-process
race; cross-process protection relies on TTL being well above
longest plausible COPY (24h default).
Adds `test_unified_flow_uses_only_agnes_verbs` that asserts no `da `
substring (with trailing space, to dodge false positives on `Darwin` /
`database` / `adapter`) appears in any of the four
`resolve_lines()` shapes:
- bare (no plugins, no ca)
- plugins only
- ca only
- plugins + ca
Also pins the `agnes init --server-url … --token …` shape — commit
8784f10a's stale-on-disk-token fix relies on `init` receiving an
explicit `--token` argument; if a future refactor drops the flag from
the emitted command the test fails loudly instead of silently
regressing to 401-on-stale-token in production.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 10.
Replace the analyst-vs-admin `?role=` design summary with the unified
flow we're shipping: single tile, single PAT-mint shape (general /
90 d), `agnes init` mandatory for everyone, marketplace block gated
by `resource_grants`, pre-flight check now validates both git and
claude.
The intro paragraph references the 10-task unification follow-up and
the `?role=` introduction-and-removal cycle so a future operator
reading the diff doesn't think they missed a release.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 9.
Drops the `<nav class="role-tiles">` block (Analyst / Admin tiles),
the `_show_admin_tile` flag, the `const ROLE = {{ role | tojson }};`
JS line, and the role-aware PAT-mint ternary. The setupNewClaude
button now mints a uniform PAT for everyone:
{ name: defaultTokenName(), expires_in_days: 90 }
…against the existing `POST /auth/tokens` endpoint. No new endpoint,
no role-locked TTL clamp. The `bootstrap-analyst` 1-hour scope is no
longer used from /setup (it broke the install flow anyway — saved PATs
expired before the user opened Claude Code; tracked as a separate
cleanup issue).
Also removes the now-unused `.role-tiles` / `.role-tile` CSS rules so
the stylesheet doesn't carry dead selectors.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 6.
The `/setup` route no longer accepts `?role=analyst|admin`. The route
signature drops the `Literal[...] = Query(...)` parameter and the
silent admin-downgrade block (`if role == "admin" and not is_admin:
role = "analyst"`). The `role` ctx variable threaded into install.html
also goes away — Task 6 cleans up the template's role-tile UI and the
JS PAT-mint ternary.
`?role=` is silently ignored by FastAPI for unknown query params, so
existing bookmarks (none in production — the param was added in this
PR and never shipped) just degrade to the unified layout. No
RedirectResponse shim needed.
Tests: drop the entire `tests/test_setup_page_roles.py` file (eight
role-branching tests that no longer apply) and add
`tests/test_setup_page_unified.py` with three tests:
- `test_setup_page_renders_unified_layout`
- `test_setup_page_ignores_role_query_param`
- `test_setup_page_renders_marketplace_for_user_with_grants`
- `test_install_legacy_path_redirects_to_setup`
Also replace the role-aware `test_install_preview_*` tests in
test_web_ui.py with unified-layout assertions.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 5.
Removes the `role: Literal["analyst", "admin"] = "admin"` parameter from
`compute_default_agent_prompt`. The same RBAC pass
(`marketplace_filter.resolve_allowed_plugins`) now runs for every user —
admin or not. Users with no `resource_grants` rows get the
no-marketplace layout; users with grants get the marketplace block
inserted. Admin-vs-analyst is no longer a layout branch.
`render_agent_prompt_banner` no longer derives a `role` from
`user.is_admin`; it just delegates to `compute_default_agent_prompt`.
Two `compute_default_agent_prompt(...role=role)` call sites in
`app/web/router.py::setup_page` are updated to drop the keyword so the
route keeps rendering — Task 5 will remove the `?role=` query
parameter and the silent admin-downgrade block from the route signature
itself.
Tests: drop role-aware assertions from test_welcome_template_renderer
and test_welcome_template_api. Both files now assert the unified
default contains `agnes init` + `uv tool install` and bans the legacy
`agnes auth import-token` / `agnes auth whoami` verbs.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 4.
Renames `_git_check_block` to `_preflight_block` and adds a
`claude --version` check beside `git --version`. Both binaries are
required by the marketplace step — git for the clone fallback,
claude for `claude plugin marketplace add` / `claude plugin install` —
so checking them together gives one clear failure instead of two
confusing downstream errors.
Install hints: `npm i -g @anthropic-ai/claude-code` for Linux / WSL
plus a doc URL (https://docs.claude.com/claude-code) for the native
macOS / Windows installers. We don't try to one-line a native
installer; the canonical instructions live upstream.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 3.
Adds `_step_numbers(*, has_marketplace, has_skills)` so step numbering
lives in one place instead of being split across three branches in
`resolve_lines`. Pins the unified layout in the tests:
No plugins: 1 install, 2 init, 3 catalog, 4 diagnose, 5 skills, 6 confirm
With plugins: 1, 2, 3, 4 preflight, 5 marketplace, 6 diagnose, 7 skills, 8 confirm
`agnes auth import-token` / `agnes auth whoami` are now banned from the
rendered prompt — `agnes init` subsumes them. The renamed
`test_resolve_lines_no_plugins_unified_six_step_layout` asserts those
strings are absent and that the new step headers (`Bootstrap your Agnes
workspace`, `Verify the data is queryable`) are present.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 2.
Removes the `role: Literal["analyst", "admin"]` parameter from
`resolve_lines` / `render_setup_instructions` and deletes the
`_resolve_analyst_lines`, `_analyst_init_lines`, `_analyst_finale_lines`
helpers. The unified flow now always emits `agnes init` (the
workspace-rails delivery mechanism) in place of the legacy
`agnes auth import-token` + `agnes auth whoami` pair, and uses
`agnes catalog` as the smoke-verify step.
`agnes init` already verifies the PAT internally, and `agnes catalog`
doubles as a data-plane smoke check, so dropping `agnes auth whoami`
costs no signal.
Drops the now-redundant `tests/test_setup_instructions_analyst.py` and
patches the one ordering test in `tests/test_setup_instructions.py` that
referenced the old "Log in" / "Verify the login" headers. Also strips
the `role=role` kwarg from `compute_default_agent_prompt`'s call into
`resolve_lines` so the welcome-template render path keeps working;
welcome_template.py's own role param is removed in a follow-up task.
Plan: docs/superpowers/plans/2026-05-04-unified-setup-prompt.md task 1.
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.
Sweep operator runbooks (docs/QUICKSTART, docs/HEADLESS_USAGE,
docs/architecture, docs/sample-data, docs/agent-workspace-prompt,
docs/metrics/metrics.yml, dev_docs/server, dev_docs/disaster-recovery),
the corporate-memory service README, the jira connector README + backfill
scripts, the deploy skill, and test docstrings. Replaces `da sync` →
`agnes pull`, `da analyst setup` → `agnes init`, `da metrics ...` →
`agnes catalog --metrics` / `agnes admin metrics ...`, `da fetch` →
`agnes snapshot create`, plus the matching docker-compose admin
invocations.
Vendor-specific `/opt/data-analyst/` install paths in jira backfill /
consistency scripts and operator docs are replaced with the
placeholder `<install-dir>` and a new `AGNES_ENV_FILE` env-var override
that lets a deployment inject its actual install path without a code
change. Aligns with the OSS vendor-agnostic policy in CLAUDE.md.
CHANGELOG `### Internal` entry summarizes the audit and reaffirms the
intentional stale-marker tuples (`_LEGACY_STRINGS`, `_OUR_COMMAND_MARKERS`)
that must keep referencing `da sync` / `da fetch` / etc. for hook upgrade
and override-detection logic.
Pre-fix `agnes pull` decided what to download from sync_state hash
equality alone:
if server_hash != local_hash or tid not in local_tables or not server_hash:
to_download.append(tid)
If the recorded local hash matched server but the actual parquet had
been deleted from disk, the download was skipped. The next DuckDB
view rebuild then fails on a missing file. Repro: `rm
server/parquet/X.parquet && agnes pull` → 'Updated 0 tables', X
still missing.
Failure modes that produce hash-equal-but-file-missing:
- manual `rm` of a single parquet
- operator-side cleanup of `server/parquet/`
- two workspaces sharing one user's
`~/.config/agnes/sync_state.json` (TODO(workspace-scoped-sync-state)
in pull.py): one workspace writes its parquets, the other reads
sync_state and concludes 'I already have these'
- disk corruption / partial restore from backup
Fix: existence check runs alongside the hash compare. Missing file
forces a re-download regardless of hash equality. `parquet_dir` is
hoisted above the loop so the existence check is in scope when the
download set is built.
Tests: regression test for the hash-equal-but-missing-file case +
counterpart for the fast-path (hash-equal-and-file-present must
still skip).
Bring admin UI, audit-log messages, code comments, and analyst-facing
skill docs in line with the post-bootstrap CLI surface (`agnes pull`,
`agnes push`, `agnes init`, `agnes snapshot create`). The legacy
`_LEGACY_STRINGS` detection tuple in `app/api/claude_md.py` and the hook
upgrade markers in `cli/lib/hooks.py` are intentionally left as-is —
they exist precisely to flag pre-rewrite content for re-authoring.
Strip "(folded from `da metrics list`)" / "(lifted from `da metrics
show`)" / "Replaces the old `da analyst status`" docstring noise — the
rename history is in CHANGELOG.md, not in module docstrings.
Closes#171. The /api/query cost guardrail used to dry-run a synthetic
`SELECT * FROM <table>` for each registered remote-BQ row referenced
by the user SQL — which made BigQuery estimate a full table scan, with
column projection, predicate pushdown, and partition pruning all
disabled. Narrow queries on big partitioned/clustered tables (the
documented happy path for `agnes query --remote`) hit ~30,000×
over-estimates and got rejected with 400 `remote_scan_too_large` even
when BQ's own dry-run reported single-digit MB.
Pavel's report on #171 traced the root cause and proposed the fix:
rewrite the user SQL to BQ-native syntax and dry-run it as a single
job, exactly the way `bq query --dry_run` works.
Implementation:
- New helper _rewrite_user_sql_for_bq_dry_run rewrites bare registered
names (word-boundary, case-insensitive, longest-first to avoid prefix
collisions) + bq."<ds>"."<tbl>" forms to backticked
`<project>.<ds>.<tbl>` paths.
- _bq_quota_and_cap_guard runs ONE dry-run on the rewritten SQL. Cap
check uses the real estimate.
- Fallback path: if BQ rejects with bq_bad_request (e.g. DuckDB-only
syntax like ::INT casts), the guard falls back to the pre-fix
per-table SELECT * approach so non-portable queries still get a
(loose) cap estimate instead of fail-opening. Non-parse BQ errors
(forbidden, upstream) still propagate as 502.
- _bq_guardrail_inputs now also returns name_lookups so the rewriter
has the (registered_name, bucket, source_table) mapping it needs.
- Per-table breakdown is unavailable from a composite dry-run; total
bytes are pinned to dry_run_set[0] for the post-flight
record_bytes(sum(...)) call to keep returning the right total.
Tests (7 new, 3 existing still pass):
- dry-run receives rewritten user SQL with WHERE clause intact (the
load-bearing assertion for #171)
- single dry-run per request even with multiple registered tables
(JOIN, UNION) referenced
- fallback to per-table SELECT * on bq_bad_request
- non-parse BQ errors (forbidden) still 502
- rewriter unit tests: bare + bq.path in same SQL, longest-name-wins
on prefix collision, case-insensitive bare-name match
The renderer no longer emits the legacy "da analyst setup" verb (the analyst
flow uses `agnes init`, the admin flow uses `agnes auth import-token`). The
disjunction assertions ("da analyst setup" OR "agnes auth" OR "curl") were
permissive and would have silently kept passing even if the renderer
regressed. Replace them with role-aware assertions that match the actual
emitted markers and explicitly check that no legacy verb survives.
Four call sites where #174 (branched from main before the agnes rename
fully landed in some files) emitted or referenced `da fetch`. None are
operator-visible runtime crashes — but `extractor.py` logs a stale
verb to the operator log and `DATA_SOURCES.md` is current docs:
- connectors/bigquery/extractor.py:431,434 (operator-facing log line on
unverified BQ entity_type — was suggesting `da fetch`).
- docs/DATA_SOURCES.md:77,85 (current public docs, two refs to
`da fetch` in the workflow + the BQ scope description).
- tests/test_cli_query_render.py:7 (module docstring listed
`da fetch / agnes schema / etc.` — now `agnes snapshot create / agnes
schema / etc.`).
- tests/test_cli_snapshot_create.py:1 (docstring referenced `(folded
from `da fetch`)` — historical, removed; no value once the rename
landed).
Pre-existing stale `da` references elsewhere in the branch (templates,
operator runbooks, internal comments) are not touched by this commit —
they live outside the merge surface and are a separate cleanup task.
Verified: 10/10 across the affected test files pass.
Ports Minas's PR #172 (against pre-rename `da` CLI on main) and applies
the principle to the post-rename `agnes` CLI. Two distinct failure modes
on Windows consoles whose default codepage is cp1250 (cs-CZ) / cp1252
(en-US):
1. `agnes pull` and other Rich-progress codepaths
UnicodeEncodeError on Braille spinner glyphs. Fix: `cli/main.py`
reconfigures stdout/stderr to UTF-8 with errors='replace' at import
time on `sys.platform == 'win32'` so Rich's legacy-Windows render
path emits decodable bytes. Wrapped in try/except so pytest's
captured streams (which aren't TextIOWrapper) don't break.
2. `agnes skills list` and `agnes skills show`
UnicodeDecodeError when reading skill markdown containing em-dashes /
accented chars. Default `Path.read_text()` uses
locale.getpreferredencoding(False), which is the broken codepage on
Windows. Fix: every call site passes encoding='utf-8' explicitly.
Broader scope than #172 because:
- The bootstrap rewrite renamed/removed several files Minas's PR
patched (`cli/commands/analyst.py` -> rolled into init.py;
`cli/commands/sync.py` -> split into pull/push). Those targets no
longer exist; the equivalent code lives in init.py.
- Other call sites Minas didn't touch (still bare in his branch) are
patched here too — config.py / update_check.py / snapshot_meta.py /
setup.py / skills.py — so the codebase has zero locale-default text
I/O in cli/.
Side cleanup: stale `Run `da`` reference in snapshot_meta.py:88 fixed
to `agnes` while touching the file.
The dashed identifier is what the test exercises (backticks required for
dashed BQ project IDs); the literal string can be any synthetic value.
'prj-grp' is too close to a real customer-prefix pattern that the OSS
vendor-scrub regex flags. 'my-project' matches placeholders used elsewhere
in the project.
CI failure: test_readers_in_pre_init_dir asserted no Traceback in stderr
when running `agnes snapshot create x --as y --estimate` in a folder
that never saw `agnes init`. The estimate-guard fix in 3d587681 let
`--estimate` skip the local_db check and reach `api_post_json`, but the
existing `except V2ClientError` doesn't cover transport-layer failures.
With no server configured the URL defaults to http://localhost:8000;
httpx raises ConnectError → ConnectError isn't a V2ClientError → the
exception bubbles up through Typer/rich as a full traceback.
Add `except httpx.HTTPError` next to V2ClientError so connection /
DNS / TLS / timeout failures all render the friendly hint
`Run `agnes init …` first` instead of leaking transport noise.
Real bug: `agnes push` was reading `<workspace>/user/sessions/`, but
Claude Code writes session jsonls to `~/.claude/projects/<encoded-cwd>/`
and nothing on the analyst side ever copies them across. The SessionEnd
hook ran `agnes push` happily and uploaded zero sessions every time.
`cli/lib/claude_sessions.py` probes both Claude Code encoding variants
(older `/`→`-` keeping spaces+tildes; newer all-non-alphanumeric→`-`
with collapsed runs) and unions whichever exist. Users who upgraded
Claude Code mid-project end up with both encoded dirs side-by-side on
disk; the union ensures no session is left behind. Same-named jsonl in
both dirs → newest mtime wins. `<workspace>/user/sessions/` survives as
a fallback for any setup that explicitly mirrors sessions there.
Verified on real disk: helper returns 2 dirs + 8 unioned session files
for the Agnes-test workspace where the previous code returned 0.
Three coupled UX fixes for the analyst-onboarding flow:
1. Dashboard "Setup a new Claude Code" CTA was rendering admin paste
prompt for everyone (analysts couldn't actually execute the marketplace
plugin install / skills setup steps). render_agent_prompt_banner now
picks role based on user.is_admin — analysts get the analyst flow.
2. /setup default role changed from admin to analyst. Most visitors are
analysts; admin layout is opt-in via the admin tile or ?role=admin.
3. Admin tile is admin-only on the role-tile nav. Non-admins see only
the analyst tile. Server-side: non-admin requesting ?role=admin is
silently downgraded to analyst (otherwise they'd see admin paste
prompt despite no tile).
Tests:
- New: test_setup_page_admin_tile_hidden_for_non_admin (anonymous client
can't see "Admin CLI" or role=admin link)
- New: test_setup_page_admin_role_downgraded_for_non_admin (anonymous
?role=admin → analyst layout, no marketplace step in clipboard)
- New: test_install_preview_default_role_is_analyst (admin signing in to
bare /setup gets analyst clipboard by default)
- Renamed: test_setup_page_default_role_is_admin → ..._is_analyst
- Updated: test_setup_page_admin_clipboard_renders_admin_layout uses
FastAPI dependency_overrides to inject admin user (admin layout is
now admin-gated)
- Updated: test_install_preview_visible_for_signed_in_user explicitly
passes ?role=admin to exercise admin layout
Caught by my own broader test scope after Devin fixes — three test files
asserted on user-visible strings that were renamed by the bootstrap PR
but the assertions weren't updated:
- tests/test_api_query_guardrail.py:110 — asserted `da fetch in suggestion`
on /api/query 400 response. Renamed to `agnes snapshot create`.
- tests/test_query_materialized_error_message.py:56 — asserted `da sync`
in materialized-not-yet error detail. Renamed to `agnes pull`.
- tests/test_cli_error_render.py:71 — fixture data + assertion both
carried `da fetch`. Updated to `agnes snapshot create`.
Plus an actual content miss: docs/setup/claude_settings.json (a template
shipped to operators) still installed `da sync` / `da sync --upload-only`
hooks. The companion test file (tests/test_setup_hooks_template.py) was
asserting that legacy state. Updated both:
- Template hooks: `agnes pull --quiet` / `agnes push --quiet`
- Test assertions + function name match the new commands
13 Devin findings across 10 files:
🔴 Critical:
- app/api/v2_catalog.py:42 — `_fetch_hint` returns `da fetch` in /api/v2/catalog
responses (user-visible in every catalog list)
- cli/skills/agnes-data-querying.md — 11 stale `da fetch`/`da sync` refs in the
bundled skill markdown
- config/claude_md_template.txt:38 — referenced `agnes pull --docs-only` flag
that does NOT exist in agnes pull (removed; spec only ships --quiet/--json/
--dry-run)
🟡 Important:
- app/api/admin.py:252 — `da fetch` in bq_max_scan_bytes hint
- cli/commands/auth.py:119 — `da sync` in import-token docstring (--help text)
- cli/commands/tokens.py:48 — "Export it so `da` can use it" prose
- ARCHITECTURE.md — 4 stale rows in CLI commands table
- README.md — stale paragraphs for analysts (da sync, da analyst setup)
🚩 Substantive observations addressed:
- app/api/query.py:249,302,489 — server-side error/help strings still said
`da sync`/`da fetch` (returned in API responses to clients)
- cli/commands/snapshot.py:235-241 — DuckDB existence guard incorrectly
blocked `--estimate` (server-side dry-run that never opens local DB).
Added test ensuring estimate path skips the guard.
Skipped (intentionally historical):
- app/api/admin.py:2377,2429,2437 — historical comments describing past
manifest-vs-sync_state bug; past tense, accurate to keep as `da sync`.
Adds three sections to the E2E plan:
- "Coverage honesty" — explicit list of what the plan reveals (✅) and
what it does NOT (❌, with reasoning per gap)
- "Recommended additional coverage layers" — Tier 1/2/3 with realistic
coverage estimates (~70 % / ~80 % / ~95 % / ~98 %)
- "Prerequisites" table — what's needed on the VM, with fallback
behavior per missing item
The plan is intentionally not exhaustive. Goal is to surface the worst
contract violations fast, not to prove correctness across all real-world
environments. Documenting the gap explicitly so operators don't ship
on a false sense of "tests passed = production-ready."
Typer/rich emits ANSI styling in CI's --help output (e.g. `--metrics`
becomes `-\x1b[0m\x1b[1;36m-metrics`), so literal substring asserts
like `assert "--metrics" in result.output` fail. Locally the test runner
auto-detects no-TTY and produces plain text, masking the issue.
Add a small `_clean()` helper per test file that strips ANSI escape
codes (`\x1b\[[0-9;]*m`) before substring containment checks.
- _v23_to_v24_finalize: wrap row-update loop in BEGIN/COMMIT/ROLLBACK
to match the project's transactional-finalizer pattern (compare
_v12_to_v13_finalize, _v17_to_v18_finalize, _v18_to_v19_finalize).
Pre-fix a process crash mid-loop left the schema_version unchanged
but partially-converted rows persisted across restart — idempotent
overall but inconsistent with project convention.
- _v23_to_v24_finalize: re.sub replacement now uses a function-form
(lambda) instead of an f-string, so any future project_id with a
backslash sequence isn't misinterpreted as a group reference.
- tests: add a Keboola-source materialized row case asserting the
SELECT's source_type filter prevents non-BQ rewrites.
