Repository navigation
feat: MCP-first data tools with a model-free CLI fallback and workspace skills - #11
Conversation
Do not merge: experiment for evaluating direct altimate-code tools in Claude Code. - Bring in the MCP registration, analyst agent and experimental listing from the plugin draft branches - Skill and analyst agent use the altimate-code MCP tools when present, otherwise `tool context` then `tool run` - SessionStart hook picks MCP or CLI mode without starting the CLI and adds routing guidance only in CLI mode - Opt-in PreToolUse guard for direct warehouse clients (`ALTIMATE_ROUTE_GUARD=1`) - Tests: `tests/data-mode.sh`, `tests/warehouse-guard.sh` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bfc4992 to
6ff19bf
Compare
- Start the MCP server with --workspace-tools - Pass --workspace-tools on every tool run/list/schema call in CLI mode (not tool context) - Skill: workspace_context, workspace_skill and workspace_execute in both modes, receipt refresh, setup statuses, engine install on request, linking without memory upload, skill list/refresh/publish, memory save only on explicit request - Analyst agent, /altimate command and SessionStart text route workspace questions to the workspace tools - Tests: tests/workspace-tools.sh Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n both modes Do not merge: experiment. - SessionStart copies the linked workspace's skills into `.claude/skills` in MCP and CLI mode (ported from the workspace-skills draft: command blocks neutralised, signed-in tenant only, user skills never overwritten); `ALTIMATE_CLAUDE_SKILL_SYNC=0` disables it - Starts a bounded background `workspace refresh` when the snapshot is missing or older than an hour - `.mcp.json` runs `mcp serve --workspace-tools`; skill and analyst agent describe workspace context, skills and reads in both modes - Tests: `tests/session-start.sh`, `tests/test_sync_workspace_skills.py`; version 1.4.0 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and on demand Do not merge: experiment. - `hooks-handlers/refresh-skills.sh` refreshes the linked workspace's skills and copies them into `.claude/skills`: `--if-stale` runs in the background when the snapshot is missing or over an hour old, `--wait` refreshes now and reports the copies; one refresh per project at a time - SessionStart copies the snapshot already on disk and starts the background refresh without waiting; Claude Code loads skills added mid-session - Stop hook runs the same hourly background refresh in long sessions - `/altimate-code:refresh-skills` command for a manual refresh - Tests: `tests/refresh-skills.sh` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…refresh Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hkaltai
changed the title
The `altimate-code-experimental` listing, its packaging script, docs and smoke tests belong to the separate pilot PR; this branch changes only the `altimate-code` plugin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…shes, tighten docs and guard Workspace skill copies are reconciled under a per-project lock and swapped with rollback; a snapshot that cannot be read leaves copies alone; symlinked SKILL.md and tenant-less manifests are not served. Failed background refreshes retry hourly, --wait keeps another refresh's lock, the mode check avoids here-docs, the warehouse guard checks chained commands, and the analyst agent no longer reads .env. PRIVACY and README describe the workspace skill data flow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ned commands in the guard header Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| if [[ "$CMD" =~ ${PREFIX}(psql|trino|clickhouse-client|clickhouse|snowsql|mysql|sqlcmd|duckdb)([[:space:]]|$) ]]; then | ||
| # Interactive or help invocations without a query are left alone. | ||
| if [[ "$CMD" =~ (^|[[:space:]])(-c|--command|-e|--execute|-q|--query|-f|--file|-x)([[:space:]=]|$) ]] || [[ "$CMD" =~ (\<\<|\|[[:space:]]*(psql|trino|clickhouse|snowsql|mysql|duckdb)|\<[[:space:]]*[^[:space:]]+\.sql) ]]; then |
There was a problem hiding this comment.
WARNING: \< is a GNU/glibc word-start assertion, so this branch is always true on Linux
[[ ... =~ ]] compiles with glibc regcomp, where \< matches the start of a word. \<\< therefore matches the empty string at any word start, so the second condition fires for every command that reached line 43. On glibc systems every command-position psql, trino, mysql, duckdb, snowsql, sqlcmd or clickhouse-client is denied - including the read-only invocations this file promises to leave alone (psql --version, psql -h db). The allow 'psql --version' case in tests/warehouse-guard.sh cannot pass there. BSD regex (macOS) treats \< as a literal <, so behaviour also differs by platform. Match the heredoc/redirect forms with plain << / < instead of backslash-escaped ones.
There was a problem hiding this comment.
Already fixed before this review landed: QUERY_INPUT uses plain << / < (no \<), so the check behaves the same with glibc and BSD regex. allow 'psql --version' and allow 'psql -h db' pass in tests/warehouse-guard.sh.
| # A client is only matched in command position (start, or after ; & | ( or sudo / docker exec), | ||
| # so file operations that merely mention a client name pass. | ||
| START='(^|[;&|(]|&&|\|\|)[[:space:]]*(sudo[[:space:]]+)?' | ||
| DOCKER='(docker|podman|kubectl)[[:space:]]+(exec|compose[[:space:]]+exec)[^;&|]*[[:space:]]' |
There was a problem hiding this comment.
SUGGESTION: the docker branch is not anchored to command position
$START carries the (^|[;&|(]) anchor, but the $DOCKER alternative does not, so docker exec ... <client> matches anywhere in the string. A command that merely mentions one, e.g. git commit -m 'docker exec -i pg psql -c select 1', is denied, contradicting the comment above about matching only in command position. Wrap the docker branch in the same anchor.
There was a problem hiding this comment.
Fixed in 8cd1e4d: DOCKER now sits inside the $START-anchored repetition (${START}(${KEYWORD}|${WRAP}|${ASSIGN}|${DOCKER})*), so docker/podman/kubectl exec counts only in command position. Tests added: git commit -m 'docker exec -i pg psql …' is allowed; sudo docker exec, env X=1 docker exec, bash -c "docker exec …", kubectl exec … -- psql, and docker after && / ; are still denied.
| find "$LOCK" -maxdepth 0 -mmin +10 -exec rmdir {} \; 2>/dev/null | ||
| if [ "$MODE" = "--wait" ]; then | ||
| # A refresh already in flight finishes first; this one then picks up anything newer. | ||
| for _ in $(seq 1 120); do mkdir "$LOCK" 2>/dev/null && break; sleep 1; done |
There was a problem hiding this comment.
WARNING: this wait loop does not record whether the lock was actually acquired
If an in-flight refresh outlasts the 120 attempts, the loop falls through, the command runs workspace refresh concurrently with it, and line 45 then rmdirs a lock owned by the other process, letting a third refresh start. The stale-lock cleanup on line 38 only reclaims locks older than ten minutes, so after a crashed refresh the two-minute wait always ends unlocked and two refreshes write the same snapshot. Separately, seq is not installed on stock macOS, where the loop body never runs at all. Set a LOCKED=1 flag when mkdir succeeds and only rmdir in that case, and use a portable counter.
There was a problem hiding this comment.
Already fixed: --wait sets LOCKED=1 only when mkdir succeeds, exits with a message if it never gets the lock, and only the holder runs rmdir. On seq: it ships with macOS at /usr/bin/seq (checked on macOS 26.6), and the --wait path in tests/refresh-skills.sh passes there.
| ids = list(manifest.get("skills", {})) | ||
| owner = (manifest.get("tenant"), str(manifest.get("apiUrl", "")).rstrip("/")) | ||
| except (OSError, ValueError, AttributeError): | ||
| return [] |
There was a problem hiding this comment.
WARNING: an unreadable snapshot is treated as another tenant and deletes every copied skill
return [] here - and signed_in_as() returning None when ~/.altimate/altimate.json cannot be read or parsed - makes sync() remove all marked .claude/skills/<name> copies. A manifest caught mid-write by a concurrent workspace refresh, or temporarily unreadable credentials, therefore wipes the team skills from the project. Because refresh-skills.sh --if-stale skips while .synced-at is under an hour old, they are not restored until the next stale refresh. This also contradicts the module docstring's 'a failure leaves existing copies as they were'. Return a sentinel (e.g. None) on read/parse failure and skip the removal pass instead of deleting.
There was a problem hiding this comment.
Already fixed: an unreadable manifest or unreadable credentials now raise Unreadable, read_snapshot returns None, and sync() leaves existing copies untouched. Only a missing snapshot, or one owned by another account, removes copies. Covered by test_unreadable_manifest_keeps_copies.
| MANIFEST = ".manifest.json" | ||
| SKILLS_DIR = os.path.join(".claude", "skills") | ||
| MARKER = ".altimate-workspace-skill" | ||
| SNAPSHOT_META = {MANIFEST, ".gitignore", ".synced-at"} |
There was a problem hiding this comment.
SUGGESTION: SNAPSHOT_META is never used
Hidden files inside the bundle are already skipped by filename.startswith('.') in build_copy, so this constant is dead. Remove it or use it here so the snapshot-metadata invariant lives in one place.
There was a problem hiding this comment.
Fixed in 8cd1e4d: removed the unused SNAPSHOT_META; build_copy's hidden-file skip is the single rule.
| printf ' "hookEventName": "SessionStart",\n' | ||
| printf ' "additionalContext": "The altimate-code plugin ships two skills in this session. (1) The `altimate-code` skill routes data-engineering work to the altimate-code CLI — column-level lineage, downstream-impact, cross-database validation, FinOps, PII from sampled rows, query cost attribution, schema diffs, dev-vs-prod diffing. In a project linked to an Altimate workspace it also answers from the shared skills, knowledge and memory of that workspace, so questions about team conventions, standards or best practices fit it too. It is not well-suited for pure file/code edits to dbt models and SQL files that do not touch warehouse state — native Bash/Edit/Write handles those faster. (2) The `review` skill runs the altimate-code dbt PR reviewer on dbt project changes (models/**.sql, schema.yml, dbt_project.yml, sources.yml, macros, snapshots, seeds). It emits a signed verdict envelope with column lineage, query equivalence, dbt anti-pattern detection, and PII screening — none of which native tools or a generic code-review skill can compute. When the user asks to review dbt / SQL / warehouse changes, this is the specialized reviewer available. For non-dbt code review (TypeScript, Python, Go), the `review` skill declines."\n' | ||
| if [ "$MODE" = "cli" ]; then | ||
| printf ' "additionalContext": "The altimate-code plugin ships two skills in this session. (1) The `altimate-code` skill describes the model-free data tools of altimate-code: column-level lineage, downstream-impact analysis, PII detection, data-diff between tables or databases, FinOps and query cost, query equivalence, dialect translation, dbt manifest and lineage, and read-only queries and schema inspection on connected warehouses. They reach Claude in two ways. When the session has the plugin MCP tools (named `mcp__plugin_altimate-code_altimate-code__*`), those are the tools. When it does not, because the organisation disallows MCP servers or the server did not start, the same tools run from the shell through the altimate CLI `tool run` subcommand, which also returns structured output without a second agent. Neither mode runs a second LLM, and calls are read-only unless the user approves writes. In a project linked to an Altimate workspace, the `workspace_context`, `workspace_skill` and `workspace_execute` tools (MCP, or `tool run` with `--workspace-tools`) give the team conventions, knowledge, shared skills, project memory and read-only queries on the workspace warehouse integrations. Long multi-step jobs such as dbt builds and migrations can be delegated to the altimate-code agent. Plain file edits to dbt models and SQL files need none of this. (2) The `review` skill runs the altimate-code dbt PR reviewer on dbt project changes (models/**.sql, schema.yml, dbt_project.yml, sources.yml, macros, snapshots, seeds). It emits a signed verdict envelope with column lineage, query equivalence, dbt anti-pattern detection, and PII screening, which native tools and a generic code-review skill do not compute. For non-dbt code review (TypeScript, Python, Go), the `review` skill declines. In this session the altimate-code MCP server is not available, so the following project guidance applies. Data work in this project: the warehouses and dbt project of this repository are connected to Altimate Code through its CLI. When a request needs warehouse data or schema, column lineage, downstream impact, PII checks, data diffs or warehouse cost, use the Altimate Code CLI instead of ad-hoc database clients: run `altimate-code tool context` once, then `altimate-code tool run <tool> --workspace-tools --args <json>`; find a tool with `altimate-code tool list --workspace-tools` and its arguments with `altimate-code tool schema <tool> --workspace-tools`. `tool context` also names the linked workspace and lists its team skills. Questions about the team conventions, definitions or workspace data start with `tool run workspace_context --workspace-tools`; warehouse reads through the workspace engine use `tool run workspace_execute --workspace-tools`. It uses the configured connections and is read-only by default. Long multi-step jobs (dbt builds, migrations, backfills) use `altimate-code run`. Code edits, docs, tests and git work that need no warehouse data or lineage use the normal tools.%s"%s\n' "$SKILLS_NOTE" "$RELOAD" |
There was a problem hiding this comment.
SUGGESTION: the ~2.4 KB additionalContext text is duplicated
Line 57 repeats this entire string for MCP mode; the branches differ only in the trailing CLI-only guidance sentence. Two copies of a 2.4 KB prompt that already differ in intent will drift. Build the shared prefix once in a variable and append the mode-specific sentence plus $SKILLS_NOTE/$RELOAD.
There was a problem hiding this comment.
Fixed in 8cd1e4d: one additionalContext string, with the CLI-only guidance appended through MODE_NOTE. I checked the hook output byte-for-byte before and after, in both CLI and MCP mode: identical.
| 5. For long multi-step jobs, or when you invoke **`/altimate-code:altimate <task>`** for work that needs the agent, Claude Code runs `altimate run "<your task>"` locally with the `builder` agent (or `analyst` for a data question when MCP is unavailable), which uses your configured LLM provider. | ||
| 6. Warehouse queries and dbt commands run **locally under your credentials**. If the installed CLI predates the `tool` subcommand, the skill falls back to delegation. | ||
|
|
||
| Connections come from project and global `.altimate-code/connections.json` files and `ALTIMATE_CODE_CONN_*` environment variables. To pin a file for the MCP server, change the `args` in `.mcp.json` to `["mcp", "serve", "--connections", "/absolute/path/to/connections.json"]`. |
There was a problem hiding this comment.
SUGGESTION: this example drops --workspace-tools
Following it disables the linked-workspace MCP tools this PR adds: .mcp.json and tests/workspace-tools.sh both require --workspace-tools, and CHANGELOG 1.4.0 states the server starts with it. Show ["mcp", "serve", "--workspace-tools", "--connections", "/absolute/path/to/connections.json"].
There was a problem hiding this comment.
Fixed in 8cd1e4d: the example is now ["mcp", "serve", "--workspace-tools", "--connections", "/absolute/path/to/connections.json"].
| grep -q "$tool" "$PLUGIN/skills/altimate-code/SKILL.md" || fail "SKILL.md does not describe $tool" | ||
| done | ||
|
|
||
| out="$(env -i HOME="$(mktemp -d)" PATH="/usr/bin:/bin" ALTIMATE_DATA_MODE=cli bash "$PLUGIN/hooks-handlers/session-start.sh")" |
There was a problem hiding this comment.
SUGGESTION: this runs the real SessionStart hook against the caller's working directory
With env -i but no CLAUDE_PROJECT_DIR and no cd, the hook falls back to $PWD, so it syncs .claude/skills in the checkout the suite was started from. If that checkout has a linked .altimate-code directory it also starts a real background workspace refresh and can remove marked skill copies outside the temp dir. The other hook tests pin CLAUDE_PROJECT_DIR to a mktemp project; do the same here (and clean up the mktemp -d HOME).
There was a problem hiding this comment.
Fixed in 8cd1e4d: the hook now runs with CLAUDE_PROJECT_DIR and HOME set to one mktemp -d directory, which is removed afterwards, so it never touches the caller's checkout.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (7 files)
Previous Review Summaries (11 snapshots, latest commit a74e074)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a74e074)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit 6038100)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (12 files)
Previous review (commit f36468b)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (2 files)
Previous review (commit 2cd0163)This review did not finish. The model reached its output limit before it Previous review (commit 2cd0163)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files, incremental diff a25dfdd..2cd0163)
Previous review (commit a25dfdd)Status: No Issues Found | Recommendation: Merge Files Reviewed (12 files, incremental diff e1e1c73..a25dfdd)
Previous review (commit e1e1c73)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 37670fa)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 8cd1e4d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit 0cdccbd)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (18 files)
Fix these issues in Kilo Cloud Previous review (commit d6d440c)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (28 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 71.9K · Output: 19.6K · Cached: 872.1K |
…uring swaps, and judge warehouse clients by their own flags Workspace skill copies are served only for the signed-in account (tenant, URL and the CLI's credential digest), survive the CLI's snapshot swap, are never written through a symlinked .claude, are removed when copying is turned off, and carry a named marker. Refresh freshness is per account. The MCP mode check follows Claude Code's policy rules (managed-mcp.json, managed-settings.d, scoped plugin server name, serverCommand, merged allowlists, /mcp toggles by git root), and CLI-mode guidance names the executable on PATH. The opt-in warehouse guard checks each client call against its own query flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…recognise its staging names Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and partial reads Skills are served only to a signed-in account for a manifest the CLI would attribute; credentials or a snapshot that cannot be read change nothing, and a manifest replaced mid-sync stops the removal pass. The shared mkdir lock is renewed, taken over by rename and released only by its holder. BOM, symlink, Windows and special-file handling now give the same copies as the CLI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…; junctions on every Python version Each copy is swapped in only while the lock is held and the snapshot is the one read and still the signed-in account's. Swap detection uses the staging directory's mtime (Windows ctime is creation time). Junctions are refused at every path via st_reparse_tag, a SKILL.md that is not a regular file skips that skill, and marker lines split like the CLI's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # A refresh already in flight finishes first; this one then picks up anything newer. | ||
| LOCKED= | ||
| for _ in $(seq 1 120); do mkdir "$LOCK" 2>/dev/null && LOCKED=1 && break; sleep 1; done | ||
| [ -n "$LOCKED" ] || { say "Another workspace skill refresh is still running; try again when it finishes."; exit 1; } |
There was a problem hiding this comment.
WARNING: seq is not installed on macOS, so the new LOCKED guard makes --wait fail there
seq ships with GNU coreutils, not macOS, so $(seq 1 120) expands to nothing and the loop body never runs. With the new [ -n "$LOCKED" ] check, /altimate-code:refresh-skills now prints "Another workspace skill refresh is still running" and exits 1 on every macOS run; before this change it fell through and refreshed. Use a portable counter (i=0; while [ "$i" -lt 120 ]; do ...; i=$((i + 1)); done) or jot.
There was a problem hiding this comment.
Not an issue: seq ships with macOS at /usr/bin/seq (BSD seq, present on every release since 10.5; checked on macOS 26.6), so $(seq 1 120) expands normally and the --wait path gets the lock. tests/refresh-skills.sh passes on macOS.
There was a problem hiding this comment.
Changed in e1e1c73 to a shell counter (i=0; while [ "$i" -lt 120 ]; …; i=$((i + 1)); done), so the loop no longer runs seq at all. For the record, current macOS does include /usr/bin/seq in the base system (root:wheel on the system volume, checked on macOS 26.6). The counter just removes the question.
| - **Command** — `plugins/altimate-code/commands/altimate.md` — `/altimate-code:altimate` slash command for explicit delegation. | ||
| - **Routing skill** — `plugins/altimate-code/skills/altimate-code/SKILL.md` — use the MCP tools when available, otherwise `tool run` (after `tool context`); delegate long jobs with `run`. | ||
| - **Analyst agent** — `plugins/altimate-code/agents/analyst.md` — read-only data analyst that works in both modes. | ||
| - **MCP registration** — `plugins/altimate-code/.mcp.json` — launches the local stdio server; builds without `mcp serve` fail to start it and the CLI mode applies instead. |
There was a problem hiding this comment.
SUGGESTION: CLI mode does not apply when the build lacks mcp serve
data-mode.sh chooses the mode from command -v altimate-code and the MCP policy settings only; it never runs the CLI, so it cannot detect a build without mcp serve. Such a build stays in MCP mode, the server fails to start, and the CLI routing guidance is not added (the PR's "Known limitations" says the same). plugins/altimate-code/README.md:14 repeats the claim. Reword both to say the skill's tool run fallback applies instead, or probe altimate-code mcp serve --help at session start.
There was a problem hiding this comment.
Fixed in 8cd1e4d: CONTRIBUTING and the plugin README now say that a build without mcp serve stays in MCP mode, without the routing guidance, and that the skill falls back to tool run. This matches the PR's Known limitations.
| return False | ||
| if not os.path.isdir(target_root): | ||
| # Nothing to remove; create the directory only when there is something to copy. | ||
| snapshot = None if withdraw else read_snapshot(project) |
There was a problem hiding this comment.
SUGGESTION: read_snapshot parses the whole snapshot twice on the first sync
On a project without .claude/skills this reads the credentials and every SKILL.md, discards the result, then repeats the same read after the lock at line 428. The pre-lock call only decides whether the directory should be created, so a cheap read_manifest(project) is not None check would avoid the duplicate work.
There was a problem hiding this comment.
Keeping as is: the pre-lock read decides more than whether a snapshot exists. It must also know there is at least one skill to copy, or .claude/skills would be created for an empty or other-account snapshot. read_manifest(...) is not None can't tell. The extra read happens only on a project's first sync and reads a handful of small files, and the read after the lock is required anyway because the snapshot can change while we wait.
| self.assertEqual(self.run_sync()[0], "changed") | ||
| self.assertFalse(os.path.exists(self.skill_path("staging-conventions"))) | ||
|
|
||
| def test_snapshot_without_a_tenant_is_not_served(self): |
There was a problem hiding this comment.
SUGGESTION: this test cannot detect a regression in the manifest check it names
The credentials written here (only altimateApiKey) make signed_in_as() raise Unreadable, so read_snapshot returns None and nothing is copied regardless of what owner_of does: deleting the tenant/apiUrl/account checks entirely would still pass. Keep the valid credentials from setUp, sync first, then strip tenant/apiUrl from the manifest and assert the existing copy is removed.
There was a problem hiding this comment.
Fixed in 8cd1e4d: the test keeps the valid credentials from setUp, syncs first and checks the copy exists, then strips tenant/apiUrl from the manifest and checks the copy is removed. I verified it catches the regression: with serves() changed to ignore the manifest owner, the test fails.
…en docs and tests - warehouse-guard: docker/podman/kubectl exec counts only in command position, so a commit message that mentions one is no longer denied. - session-start: one additionalContext string, with the CLI guidance appended in CLI mode. - Drop the unused SNAPSHOT_META constant. - README: the pinned-connections example keeps --workspace-tools; README and CONTRIBUTING say a build without mcp serve stays in MCP mode and the skill falls back to tool run. - tests: the workspace-tools hook run uses a temp project; the tenant test syncs first so it fails if the manifest ownership check is dropped; more guard positions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| WRAP='([^[:space:]]*/)?(sudo|env|command|time|exec|nice|nohup|timeout)([[:space:]]+(-[^[:space:]]*([[:space:]]+[^-[:space:]][^[:space:]]*)?|[0-9][^[:space:]]*))*[[:space:]]+' | ||
| ASSIGN='[A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+' | ||
| DOCKER='(docker|podman|kubectl)[[:space:]]+(exec|compose[[:space:]]+exec)[^;&|]*[[:space:]]' | ||
| PREFIX="${START}(${KEYWORD}|${WRAP}|${ASSIGN}|${DOCKER})*" |
There was a problem hiding this comment.
SUGGESTION: anchoring DOCKER loses bash -lc / sh -ec wrappers
Moving DOCKER behind $START is the right fix for commit messages, but START only recognises bash|sh|zsh with a bare -c (line 41). A genuine command position that uses combined flags, e.g. bash -lc "docker exec -i pg psql -c 'select 1'" or sh -ec "docker exec pg psql -c 1", now no longer matches DOCKER and is allowed, while the old unanchored DOCKER alternative did deny it. Widening the shell alternative to combined short flags (e.g. (bash|sh|zsh)[[:space:]]+-[a-z]*c[[:space:]]+[\"']) keeps those command positions caught.
There was a problem hiding this comment.
Good catch, that was a regression from the anchoring. Fixed in 37670fa: START now accepts combined short flags ending in c ((bash|sh|zsh)[[:space:]]+-[A-Za-z]*c[[:space:]]+["']). Tests added: bash -lc "docker exec … psql …", sh -ec 'docker exec pg psql …' and zsh -lc "psql -c 1" are denied.
…sition in the warehouse guard Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ncing past an undecodable staging .gitignore - warehouse-guard: the regex matcher is replaced by warehouse-guard.py, which splits the command into shell words (quoted text is never a command), skips keywords, VAR=value, wrappers and docker/podman/docker-compose/kubectl exec with each tool's own option table, and judges bash/sh/zsh/dash/ksh -c strings, env -S strings and $(...)/`...` substitutions as commands. warehouse-guard.sh only checks the opt-in and python3. - sync-workspace-skills: a staging-named directory whose .gitignore is not UTF-8 is left alone instead of stopping the whole sync and withdrawal. - Docs: workspace status prints no page URL; only DROP DATABASE, DROP SCHEMA and TRUNCATE are always blocked; only curl/PowerShell installs older than v0.12.8 lack altimate-code; engineStatus unavailable; guard needs python3 and never treats quoted text as a command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ll does - A heredoc body is data: it is skipped (only substitutions in the body of an unquoted delimiter are judged), so an apostrophe in SQL no longer stops the check and backticks in a quoted commit or PR body are not taken for commands. - Newlines separate commands; a backslash-newline joins lines; # starts a comment. - `)|` counts as a pipe; env -S is read only among env's own options; a parse error in one substitution no longer stops the rest of the command being judged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| while not quoted and k < len(line): | ||
| k = substitution(line, k, subs) if line.startswith("$(", k) or line[k] == "`" else k + 1 | ||
| heredocs = [] | ||
| elif c == "#" and (i == 0 or text[i - 1] in " \t;&|()"): |
There was a problem hiding this comment.
WARNING: a # at the start of a line is not treated as a comment, which both hides and invents queries
This check is i == 0 or text[i - 1] in " \t;&|()", but the newline in front of the # is only rewritten in out, not in text, so for any comment line after the first text[i - 1] is still "\n". Two consequences in a multi-line command:
printf 'echo hi\n# $(psql -c 1)\ntrue'is denied: the shell treats the line as a comment, but the scanner still reaches the$(branch and judgespsql -c 1.printf 'echo hi\n# don'"'"'t use psql\npsql -c 1'is allowed: the apostrophe opens a quote that swallows the rest, soshlexraises,judgereturnsFalse, and the realpsql -c 1is never seen.
The same word-start problem exists right after an escaped space (echo a\ # ; psql -c 1, where the shell sees # inside the word but text[i - 1] is " "). Test for a real word boundary instead of the raw previous character (at minimum add "\n").
There was a problem hiding this comment.
Fixed in aeec2ec. # now starts a comment only at a real word start, which the scanner tracks from what it has emitted, so a newline counts and an escaped space or a substitution does not. Tests cover echo hi\n# $(psql -c 1) (allowed), echo hi\n# note\npsql -c 1 (denied), echo x\ #$(psql -c 1) and $(echo hi)#$(psql -c 1) (both denied).
| break | ||
| k = 0 | ||
| while not quoted and k < len(line): | ||
| k = substitution(line, k, subs) if line.startswith("$(", k) or line[k] == "`" else k + 1 |
There was a problem hiding this comment.
WARNING: heredoc-body substitutions are scanned one line at a time and ignore backslash escapes
substitution(line, k, subs) only sees the current body line and does not honour the escaping an unquoted heredoc body applies (like a double-quoted string, \$, \` and \\ suppress expansion):
cat <<EOF\nUse \$(psql -c 1) literally\nEOFprints the text and runs nothing, yet the scan recordspsql -c 1and denies the command.cat <<EOF\n$(echo hi\npsql -c 1)\nEOFrunspsql -c 1in the substitution, but the first line yields onlyecho hand the second line never starts a substitution, so the command is allowed.
Scan the remaining body as a whole (the heredoc terminator is already known) and skip \ before $(/` when looking for substitutions.
There was a problem hiding this comment.
Fixed in aeec2ec. An unquoted heredoc body is now collected whole and scanned with backslash escapes, as the shell expands it: \$(psql -c 1) is literal and allowed, \\$(psql -c 1) runs and is denied, and a substitution spanning lines ($(echo hi / psql -c 1)) is judged.
| i += 1 | ||
| elif nested and c in "()" and (depth := depth + (1 if c == "(" else -1)) < 0: | ||
| return "".join(out), subs, i + 1 | ||
| elif (heredoc := HEREDOC.match(text, i)) and not text.startswith("<<<", i): |
There was a problem hiding this comment.
SUGGESTION: not text.startswith("<<<", i) is redundant
HEREDOC cannot match a herestring: after << the delimiter alternative accepts only a non-<> character, a quoted string or a backslash escape, and at <<< the next character is <, so HEREDOC.match() already returns None. The extra condition is always true when the walrus matches, so drop it (or note the herestring case in the comment above) to keep one rule.
There was a problem hiding this comment.
Kept, and made explicit in aeec2ec. HEREDOC.match can't match at the first < of <<<, but it does match one character later (<<"$HOME"), so without the guard a here-string was read as a heredoc and swallowed the following lines. A separate review round confirmed this. <<< now has its own branch, with tests: cat <<< x\npsql -c 1 and psql <<< "select 1" are denied.
…ere-strings and continued pipelines - # starts a comment only at a word start (not after an escaped space or a substitution). - A heredoc delimiter is unquoted as the shell does; an unquoted heredoc body is scanned as a whole, honouring backslash escapes, so a substitution may span lines. - <<< is a here-string, not a heredoc. - A newline after | or & continues the pipeline or list. - env -S is also read inside a flag cluster (env -iS'...'). - The no-python3 test checks the exit status as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nes and backslash-newlines - A newline after a trailing | or & continues the line even after a comment, blank lines or a backslash-newline; tracked as it is emitted instead of looking back. - A heredoc delimiter that cannot be unquoted no longer stops the check, and a quoted delimiter never runs across lines. - env - (and env [options] -) is read as env -i. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude Code shows skill descriptions only while its skill listing fits a size budget (about 1% of the context window). Past it, bundled skills keep their descriptions and project skills, including the workspace skill copies, are listed as bare names that Claude cannot match to a request. - `sync-workspace-skills.py --index` prints one `- name: description` line per workspace skill copy (marked, non-symlinked), 250 characters per description and 4,000 in all, JSON-escaped for the hook output. - `session-start.sh` adds that index to the skills note; user-written skills are not listed. - Tests cover the index caps, the user-skill exclusion and the hook output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Consensus review (5 reviewers, 1 convergence round) of 845085c, with the CLI contract checked against the companion CLI PR head.
Requesting changes for one MAJOR: PRIVACY.md does not describe the workspace-tool data flows, and it says the MCP server and tool run make no LLM calls, but ask_knowledge_base is answered by Altimate's backend models. This is a doc-only fix.
The five minors (inline) do not block:
- SessionStart context over 10,000 characters: edge input (~100+ skills).
- Windows managed MCP policy not read: degrades to the skill's
tool runfallback; follow-up. - Hardcoded
serverCommandin the policy check: edge input. - Guard denies write workloads: opt-in advisory guard.
- Background refresh unbounded / killed-refresh lock: needs a hung network call; follow-up.
What was verified:
- All shell suites pass under macOS
/bin/bash3.2.57, andpytest tests/passes (62 tests). - Every CLI command, flag, exit code, status and tool id the plugin uses matches the companion CLI.
- The mirror constants match
claude-skill-mirror.ts. reloadSkillsand${CLAUDE_PLUGIN_ROOT}in command bodies are both documented Claude Code behaviour.- The SessionStart JSON stays valid with hostile skill descriptions.
!blocks andallowed-toolsare stripped from copied skills, the copies stay out of git, and user skills are never touched.
Nits are in a separate PR comment.
| | Schema metadata, sampled column values, and query results returned by tools | Local MCP server → Claude Code conversation → your configured model provider | Governed by Claude Code and your model provider's policies | | ||
| | Scratch tables and spilled query results | Local session storage or spill files; any content Claude reads enters the conversation and reaches your model provider | Local artifacts; conversation copies follow Claude Code and provider policies | | ||
|
|
||
| The MCP server makes no separate LLM calls. Delegation uses the separate provider path below. |
There was a problem hiding this comment.
MAJOR — PRIVACY.md omits the workspace-tool data flows and says the server makes no LLM calls
.mcp.json always starts the server with --workspace-tools. workspace_context, workspace_skill and workspace_execute send the question, SQL and workspace receipt to the Altimate tenant, and ask_knowledge_base is answered by Altimate's backend models. The companion CLI's own tool description says "knowledge requests may use backend models", and skills/altimate-code/SKILL.md:271 and the README say the same. This file has no row for that flow, and it states here and at lines 12-13 that the MCP server and tool run make no LLM calls, without the exception. As written, the public privacy policy contradicts the README.
Fix: add a "Data flows through workspace tools" table (questions, SQL and arguments → Altimate tenant; knowledge answers from Altimate's backend models; results → conversation → your model provider), and qualify the "no LLM calls" sentences with the ask_knowledge_base exception.
There was a problem hiding this comment.
Fixed in eb4c0e8.
PRIVACY.mdnow has a "Data flows for workspace tools" section. Questions, SQL and other arguments sent throughworkspace_context,workspace_skillandworkspace_executego to your Altimate tenant, and knowledge questions (ask_knowledge_base) are answered by Altimate's backend models. Results then enter the conversation and reach your model provider.- The "no LLM calls" sentences are qualified with that exception.
- It also explains that a project not linked to a workspace sends no questions or SQL.
- The effective date is updated.
| description = " ".join(((fields or {}).get("description") or "").split()) | ||
| if len(description) > INDEX_DESCRIPTION_CHARS: | ||
| description = description[: INDEX_DESCRIPTION_CHARS - 1].rsplit(" ", 1)[0] + "…" | ||
| line = f"- {entry}: {description}" if description and used < INDEX_TOTAL_CHARS else f"- {entry}" |
There was a problem hiding this comment.
MINOR — SessionStart additionalContext can exceed Claude Code's 10,000-character cap
INDEX_TOTAL_CHARS caps only the descriptions. After 4,000 characters every remaining skill is still listed by name, with no limit. Measured in CLI mode with 24-character names: 2,957 characters with no skills, 9,172 with 80 skills, 10,252 with 120, and 19,664 with 300. Over the cap, Claude Code injects only a file path and a 2,000-character preview. The CLI-mode guidance starts at about character 1,777, so it is lost along with the index.
Fix: cap the whole index, for example ending with … and N more in .claude/skills/, so the total stays under 10,000.
Not a blocker: it takes roughly 100+ workspace skills (fewer with long names).
There was a problem hiding this comment.
Fixed in eb4c0e8. The whole index, measured as JSON-escaped text, is capped, and skills past the cap are counted in a last - … and N more in .claude/skills/ line. Measured: 300 skills with long names and descriptions give 6,903 characters of SessionStart context in CLI mode (5,722 in MCP mode), well under 10,000. Tests cover the cap, the closing line, and a large non-ASCII index.
| local project="${CLAUDE_PROJECT_DIR:-$PWD}" | ||
| local config="${CLAUDE_CONFIG_DIR:-$HOME/.claude}" | ||
| local args=() dir f | ||
| for dir in "/Library/Application Support/ClaudeCode" "/etc/claude-code"; do |
There was a problem hiding this comment.
MINOR — Native Windows: managed MCP policy is not read
Only /Library/Application Support/ClaudeCode and /etc/claude-code are probed. The documented Windows files, C:\Program Files\ClaudeCode\managed-settings.json and managed-mcp.json, are not. On Windows, an organisation that disallows MCP still gets MCP mode and no CLI guidance. Separately, in Git Bash command -v altimate-code also finds the npm altimate-code.cmd shim, which a stdio spawn without cmd /c may fail to start (not verified on Windows).
Fix: also probe /c/Program Files/ClaudeCode (or $(cygpath "$PROGRAMFILES")/ClaudeCode).
Not a blocker: the base SessionStart text and the skill already tell Claude to use tool run when the MCP tools are missing.
There was a problem hiding this comment.
Fixed in eb4c0e8: data-mode.sh also probes $PROGRAMFILES/ClaudeCode (default C:/Program Files/ClaudeCode) for managed-settings.json and managed-mcp.json, with tests. The npm .cmd shim under a stdio spawn is not verified on Windows, so it is listed as a follow-up.
| python3 -I -c ' | ||
| import json, sys | ||
| names = {"plugin:altimate-code:altimate-code"} | ||
| command = ["altimate-code", "mcp", "serve", "--workspace-tools"] |
There was a problem hiding this comment.
MINOR — The serverCommand policy check uses a hardcoded command
The README tells users to add --connections … to the .mcp.json args. Claude Code matches serverCommand exactly, every argument in order. After that edit, an allow or deny serverCommand entry is checked against this hardcoded array instead of the real one, so the mode can be misdetected either way.
Fix: read command and args from the plugin's .mcp.json, falling back to this default.
Not a blocker: it needs both custom args and a serverCommand policy.
There was a problem hiding this comment.
Agreed. Follow-up: reading command/args from the plugin's .mcp.json only matters when both custom args and a serverCommand policy are set. It is in the PR description's follow-ups.
| import shlex | ||
| import sys | ||
|
|
||
| HINT = ( |
There was a problem hiding this comment.
MINOR — The guard blocks write workloads and points them at a read-only tool
These are all denied: psql -X -1 -f migrate.sql, mysql -h db -u u -p db < schema.sql, duckdb new.duckdb ".read init.sql" and pg_dump a | psql b. In each case this hint names the read-only sql_execute. A migration or a local database bootstrap then has no route except unsetting ALTIMATE_ROUTE_GUARD.
Fix: mention --allow-write / altimate run in the hint, or leave -f and < file forms alone.
Not a blocker: the guard is opt-in and advisory.
There was a problem hiding this comment.
Changed in eb4c0e8: the guard's hint now says that a write (a migration or a load) needs the user's approval for --allow-write, or can be delegated with altimate run. The guard stays opt-in.
| exit "$STATUS" | ||
| fi | ||
|
|
||
| if mkdir "$LOCK" 2>/dev/null; then |
There was a problem hiding this comment.
MINOR — The background refresh has no time limit, and a killed refresh leaves its lock
A hung workspace refresh keeps running after the session ends. After 10 minutes the next hook removes its lock (line 46) and starts another refresh, so hung refreshes can pile up hourly from the Stop hook. The first refresh's rmdir "$LOCK" can later remove the newer holder's lock. A subshell killed mid-refresh leaves the lock in place for 10 minutes, because the subshell has no trap.
Fix: add trap 'rmdir "$LOCK" 2>/dev/null' EXIT inside the subshell, and bound the refresh with a portable timeout or a PID recorded in the lock.
Not a blocker: it needs a hung network call. Safe to do in a follow-up.
There was a problem hiding this comment.
Partly fixed in eb4c0e8.
- Both refresh paths now remove their lock on exit, including on HUP, INT and TERM, so a stopped refresh no longer leaves it for 10 minutes (checked by terminating a running refresh).
--waitwaits at most 60 s for the lock, inside the Bash tool's two-minute default.- A time limit on the refresh itself is a follow-up, in the PR description.
|
Nits from the consensus review (none blocking):
|
…ndex and address review findings - PRIVACY: workspace tools send questions, SQL and arguments to the Altimate tenant, and knowledge questions are answered by Altimate's backend models; the "no LLM calls" statements are qualified accordingly. - The skill index at session start is capped as JSON-escaped text and ends with "… and N more", so the SessionStart context stays well under Claude Code's limit. - Managed MCP policy is also read from %PROGRAMFILES%\ClaudeCode on Windows. - The background refresh removes its lock however it ends, --wait waits at most 60 s, and the CLI is asked to mirror skills (ALTIMATE_CLAUDE_SKILL_SYNC=1) only from the refresh. - The warehouse guard's hint also routes writes; changelog and test name fixed for disabledMcpjsonServers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. The MAJOR (
Follow-ups (Windows |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if not NAME_PATTERN.fullmatch(entry) or not is_ours(path): | ||
| continue | ||
| try: | ||
| fields, _ = parse_frontmatter(read_text_no_follow(os.path.join(path, "SKILL.md"), "utf-8-sig")) |
There was a problem hiding this comment.
WARNING: index() re-reads the JSON-escaped copy, so non-ASCII descriptions reach the session as escape text
render_skill() writes the copy's frontmatter with json.dumps(description), but parse_frontmatter() only undoes \"; it does not decode \uXXXX or \\. index() therefore repeats the escaped text: a skill described as Café is listed as - team: Caf\u00e9 (literal backslash-u) in additionalContext, a backslash in the description is doubled, and a | block scalar shows a literal \n. The 250-character cut is applied to the escaped form too, so a non-ASCII description truncates after about 40 real characters and each non-ASCII character consumes ~6 characters of the 4,000 budget.
Repro: sync a skill with description: Café, then run python3 sync-workspace-skills.py --index <project>; the output contains Caf\\u00e9.
Decode the quoted scalar (json.loads when the value is a JSON string, which is how render_skill writes it), or read the already-decoded description from the snapshot.
There was a problem hiding this comment.
Fixed in a74e074. index() now decodes each copy's description as the JSON string it is stored as (by render_skill, and by the CLI the same way), so Café, backslashes and newlines read as written, and the 250/4,000-character caps measure real characters. A test covers non-ASCII text, a backslash and quotes, and fails without the decoding.
| local project="${CLAUDE_PROJECT_DIR:-$PWD}" | ||
| local config="${CLAUDE_CONFIG_DIR:-$HOME/.claude}" | ||
| local args=() dir f | ||
| for dir in "/Library/Application Support/ClaudeCode" "/etc/claude-code" "${PROGRAMFILES:-C:/Program Files}/ClaudeCode"; do |
There was a problem hiding this comment.
WARNING: the Windows managed-settings directory is %PROGRAMDATA%\ClaudeCode, not %PROGRAMFILES%\ClaudeCode
Claude Code's machine-wide managed files are /Library/Application Support/ClaudeCode (macOS), /etc/claude-code (Linux/WSL) and C:\ProgramData\ClaudeCode (Windows). This probe adds ${PROGRAMFILES:-C:/Program Files}/ClaudeCode, so an organisation's managed-settings.json / managed-mcp.json under ProgramData is still not read on Windows, and the session stays in MCP mode without CLI guidance. tests/data-mode.sh:50-56 writes the same wrong directory, so the new tests pass regardless.
Also, on Linux/macOS the :- default is a relative path (C:/Program Files/...), so a project directory literally named C: would be read as managed policy. Gate the entry on a Windows shell and use ${PROGRAMDATA:-C:/ProgramData}/ClaudeCode.
There was a problem hiding this comment.
The directory stays. Claude Code's managed-settings docs list C:\Program Files\ClaudeCode\ for Windows and state that it "doesn't read the legacy Windows path C:\ProgramData\ClaudeCode\managed-settings.json".
Your second point was right, and it's fixed in a74e074: the Windows directory is probed only when PROGRAMFILES is set, which Windows always sets. So a relative C:/Program Files folder is never read on macOS or Linux, with a test for that case.
| i=0 | ||
| while [ "$i" -lt 60 ]; do mkdir "$LOCK" 2>/dev/null && LOCKED=1 && break; sleep 1; i=$((i + 1)); done | ||
| [ -n "$LOCKED" ] || { say "Another workspace skill refresh is still running; try again when it finishes."; exit 1; } | ||
| trap 'rmdir "$LOCK" 2>/dev/null' EXIT |
There was a problem hiding this comment.
WARNING: --wait releases the lock twice, so it can remove a newer holder's lock
The new trap 'rmdir \"$LOCK\" 2>/dev/null' EXIT fires on every exit, but line 59 still runs rmdir \"$LOCK\" right after the sync. Once line 59 frees the lock, another --wait (line 52) or a Stop-hook --if-stale refresh (line 65) can mkdir the same lock; when this script then exits, the EXIT trap removes that holder's lock, and a third refresh can start alongside it, which is the overlap the lock exists to prevent. The window covers the find/wc/say after line 59.
Drop the explicit rmdir at line 59 (the trap covers the normal path) or clear the trap (trap - EXIT) before it.
There was a problem hiding this comment.
Fixed in a74e074: --wait now drops its EXIT/signal trap before the explicit release. Exiting later can no longer remove a lock another refresh took after that point.
|
|
||
| - Team workspace skills as Claude Code skills, in both the MCP and CLI modes. At session start `hooks-handlers/sync-workspace-skills.py` copies the skills of the Altimate workspace linked to the project (from `.altimate-code/skill/_workspace/`) into `.claude/skills/`. Copies keep only `name` and `description` from the frontmatter, have `!` command blocks disarmed, leave out files Claude Code reads as hooks, agents, commands or settings, are served only for the Altimate account the user is signed in to (tenant, URL and key digest), never replace a skill the user wrote (copies carry a marker file; a name clash skips the workspace skill), and get a `.gitignore` so they stay out of version control. When the workspace drops a skill, its copy is removed. The SessionStart context mentions the project skills when copies exist. | ||
| - A project that was never synced, or not for an hour, is refreshed by a detached `altimate workspace refresh` that the hook does not wait for. It is bounded by a per-project lock that expires after ten minutes, and it is skipped silently when no CLI is on PATH or the project has no `.altimate-code` directory. Claude Code's skill watcher loads the copies when it finishes. | ||
| - The SessionStart context lists each workspace skill copy with its description (250 characters per skill, 4,000 in all; past that, names only). Claude Code shows skill descriptions only while its skill listing fits a size budget (about 1% of the context window), and past it project skills appear as bare names that Claude cannot match to a request. |
There was a problem hiding this comment.
SUGGESTION: index() never lists names past the cap
The loop drops every skill after the 4,000-character budget and appends only - … and N more in .claude/skills/ (sync-workspace-skills.py:606-612); the omitted names are not printed. Say "past that, a count of the rest" instead of "names only".
There was a problem hiding this comment.
Fixed in a74e074: the changelog now says "past that, a count of the rest".
…nly on Windows, release the --wait lock once - The SessionStart index decodes each copy's JSON-stored description, so non-ASCII text, backslashes and newlines read as written and are measured as such. - C:\Program Files\ClaudeCode is probed only when PROGRAMFILES is set, so a relative "C:/Program Files" folder is never read as managed policy elsewhere. - refresh-skills.sh --wait drops its exit trap before releasing the lock, so it cannot remove a lock another refresh took meanwhile; changelog wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Re-review at a74e0743 (three commits since the earlier review at 845085c). Approved.
The earlier MAJOR is fixed. PRIVACY.md now has a "Data flows for workspace tools" section: questions, SQL and arguments go to the Altimate tenant, ask_knowledge_base is answered by Altimate's backend models, and results enter the conversation. The "no LLM calls" sentences and the BYOK note are qualified. I checked this against the companion CLI: an unlinked project only lists workspaces, and copying skills into .claude/skills now happens only when ALTIMATE_CLAUDE_SKILL_SYNC=1, which the refresh now passes. The earlier SessionStart size, guard hint and changelog items are fixed too. All 6 shell suites and 64 pytest cases pass at this head. 300 skills with 64-character names give a CLI-mode context under 7,200 characters.
Minors, none blocking:
- Index budget counts escaped length (non-ASCII descriptions get about 1/6 of the room): errs on the safe side; cosmetic for non-ASCII workspaces.
- Decoded descriptions keep control characters: edge input, with no way to break out of the line.
- Windows
.cmdshim (rest of the earlier Windows finding): safe to do in a follow-up; the skill falls back totool run. - Hardcoded
serverCommand(earlier): edge input; listed as a follow-up. - Refresh time limit and stale-lock race (rest of the earlier lock finding): needs a refresh that hangs for 10+ minutes; listed as a follow-up.
Nits are in a separate comment.
| if len(description) > INDEX_DESCRIPTION_CHARS: | ||
| description = description[: INDEX_DESCRIPTION_CHARS - 1].rsplit(" ", 1)[0] + "…" | ||
| line = f"- {entry}: {description}" if description else f"- {entry}" | ||
| size = len(json.dumps(line)[1:-1]) + 2 |
There was a problem hiding this comment.
MINOR: the index budget counts \uXXXX-escaped length, so non-ASCII descriptions get about a sixth of the room
json.dumps(line) uses ensure_ascii, so 漢 counts as 6 characters and " as 2. Claude Code applies its cap to the decoded string. Measured with 300 copies that have 64-character names:
- ASCII descriptions: 12 skills listed with descriptions (3,803 decoded characters)
- CJK descriptions: 2 skills (673 decoded characters)
- descriptions full of quotes: 6 skills
It never goes over the cap. But a non-English workspace loses most of the listing that the index exists for. The 250-character per-description cap does count real characters. The 4,000 cap does not, and the changelog's "4,000 in all" reads as characters.
Fix: size each line with len(line) + 1 (or len(json.dumps(line, ensure_ascii=False)[1:-1]) + 2). Keep ensure_ascii=True for the printed output so the hook JSON stays ASCII.
Not a blocker: the error is on the safe side, and only non-ASCII workspaces list fewer skills.
There was a problem hiding this comment.
Fixed in a754e0a: each line is sized by its decoded length (len(line) + 1), so the 4,000 cap counts real characters and the printed hook output stays ASCII. A test with 120 CJK descriptions now lists at least 15 skills within 4,000 characters, and fails with the escaped sizing.
| description = json.loads(quoted.group(1)) | ||
| except ValueError: | ||
| pass | ||
| description = " ".join(description.split()) |
There was a problem hiding this comment.
MINOR: decoded descriptions keep control characters
" ".join(description.split()) collapses newlines, tabs and U+2028, so a description cannot add headings or list items. I checked this: "ok.\n\n## SYSTEM\n…" comes out as one bullet line. But ESC, NUL, BEL and zero-width characters get through. Before this commit a stored \u001b showed as literal text. Now it reaches additionalContext as a real control character.
Fix: after decoding, strip them, e.g. re.sub(r"[\x00-\x1f\x7f-\x9f\u200b-\u200f\u2060\ufeff]", "", description).
Not a blocker: it needs a workspace author to put control characters in a description, and they cannot break out of the line.
There was a problem hiding this comment.
Fixed in a754e0a: control (C0/C1 except whitespace), zero-width and bidi characters are removed after decoding and before whitespace is collapsed, so no double space is left. A test with ESC, BEL, ZWSP and U+202E fails without the strip.
| local dirs=("/Library/Application Support/ClaudeCode" "/etc/claude-code") | ||
| # Windows (Git Bash, MSYS, Cygwin): C:\Program Files\ClaudeCode. PROGRAMFILES is set only there, | ||
| # so a relative "C:/..." path is never probed on macOS or Linux. | ||
| [ -n "${PROGRAMFILES:-}" ] && dirs+=("$PROGRAMFILES/ClaudeCode") |
There was a problem hiding this comment.
MINOR (earlier finding, partly fixed): Windows
Fixed: C:\Program Files\ClaudeCode is now read, and only when PROGRAMFILES is set. WSL correctly keeps reading /etc/claude-code, and the test covers a relative C:/Program Files folder on other platforms.
Still open: the npm altimate-code.cmd shim under a stdio spawn. It is listed as a follow-up in the PR description.
Not a blocker: deferring it is reasonable, because the skill falls back to tool run when the MCP tools are missing.
There was a problem hiding this comment.
Thanks for checking. The npm altimate-code.cmd shim under a stdio spawn stays a follow-up in the PR description, since the skill falls back to tool run when the MCP tools are missing.
| python3 -I -c ' | ||
| import json, sys | ||
| names = {"plugin:altimate-code:altimate-code"} | ||
| command = ["altimate-code", "mcp", "serve", "--workspace-tools"] |
There was a problem hiding this comment.
MINOR (earlier finding, still open, deferred): serverCommand is still matched against this hardcoded array
The plugin README still suggests adding --connections … to the .mcp.json args. With that edit, an exact serverCommand allow or deny entry is checked against the wrong command. It is listed as a follow-up.
Not a blocker: it needs both custom args and a serverCommand policy.
There was a problem hiding this comment.
Agreed. Reading command/args from the plugin's .mcp.json stays a follow-up in the PR description.
| # Claude Code watches only skill directories that exist when it starts. | ||
| [ -L "$PROJECT_DIR/.claude" ] || mkdir -p "$PROJECT_DIR/.claude/skills" 2>/dev/null | ||
|
|
||
| find "$LOCK" -maxdepth 0 -mmin +10 -exec rmdir {} \; 2>/dev/null |
There was a problem hiding this comment.
MINOR (earlier finding, partly fixed): refresh lock
Fixed: the traps at :54-55 and :71-72 now remove the lock when a refresh exits or is stopped. I checked this: the lock goes as soon as the refresh child exits (bash runs the trap after the foreground child finishes).
Still open: there is no time limit on workspace refresh. After 10 minutes this line breaks a hung refresh's lock and lets a second refresh start. When the hung one finally exits, its EXIT trap removes the lock the second one now holds. The time limit is listed as a follow-up. The fix would be a bounded refresh, or the holder's PID recorded in the lock and checked before breaking or releasing it.
Not a blocker: it needs a network call that hangs for more than 10 minutes.
There was a problem hiding this comment.
Agreed. A time limit on workspace refresh (or the holder's PID recorded in the lock) stays a follow-up in the PR description. In a754e0a, tests/refresh-skills.sh now stops a running background refresh and checks that its lock is gone, and checks that --wait leaves none.
|
Nits from the consensus re-review (none blocking):
|
…i characters, and align wording - The SessionStart index caps decoded characters, so non-ASCII workspaces list as many skills as ASCII ones; control, zero-width and bidi characters are removed before whitespace is collapsed. - README, SessionStart text and changelog say workspace knowledge questions are answered by Altimate's backend models; --wait reports the copies present; 1.4.0 lists the Windows policy path, lock release and guard hint. - Test: a stopped background refresh and --wait both leave no lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the re-review. The inline items are answered in their threads; the fixes are in a754e0a. On these nits:
|
Summary
Gives Claude Code direct access to altimate-code's data tools instead of delegating each task to a second altimate-code agent with its own model. The plugin uses the altimate-code MCP server when it is allowed, falls back to the model-free
altimate-code tool runCLI when an organisation blocks MCP servers, and brings the linked Altimate workspace (knowledge, conventions, memory and team skills) into the session in both modes. Delegation to the altimate-code agent remains for long multi-step dbt jobs.Requires an altimate-code build with
mcp serveand thetoolCLI: AltimateAI/altimate-code#1609. Builds on #6 (MCP registration, analyst agent).Changes
Data tools: MCP first, CLI fallback
.mcp.jsonregistersaltimate-code mcp serve --workspace-tools(read-only by default).hooks-handlers/data-mode.sh) picks the mode without starting the CLI: CLI mode whenaltimate-codeis not on PATH, when managed, user or project settings deny the server (deniedMcpServers, orallowedMcpServerswithout it; on macOS, Linux and Windows), or withALTIMATE_DATA_MODE=cli. Only CLI mode adds routing guidance (tool context, thentool run <tool> --args <json>).altimate-codeskill, theanalystagent and/altimate-code:altimateuse the MCP tools when present andtool runotherwise; long jobs still use thebuilderagent.ALTIMATE_ROUTE_GUARD=1, needspython3) points direct warehouse client commands attool run. It splits the command into shell words, so quoted text is never taken for a command, and judges commands behind sudo/env/timeout, docker/podman/compose/kubectl exec,sh -cstrings and$(...)substitutions.Linked Altimate workspace in both modes
workspace_context,workspace_skillandworkspace_executeover MCP ortool run --workspace-tools, with handling for each setup status (needs_auth,needs_workspace,disabled,unavailable,needs_engine). Memory is saved only on an explicit request..claude/skills/, sanitised (frontmatter reduced to name and description,!command blocks disarmed, no hook, agent, command or settings files), only for the signed-in tenant, never replacing a user-written skill, and kept out of git. When the plugin refreshes the workspace it asks altimate-code (#1609) to write the copies too (ALTIMATE_CLAUDE_SKILL_SYNC=1)./altimate-code:refresh-skillson demand.ALTIMATE_CLAUDE_SKILL_SYNC=0turns copying and refresh off.Also included
plugins/altimate-code/CHANGELOG.md.The automatic pilot listing (
altimate-code-experimental) from #9 is not part of this PR; it stays in #9.Validation
tests/data-mode.sh,tests/session-start.sh,tests/refresh-skills.sh,tests/warehouse-guard.sh,tests/resolve-cli.sh,tests/workspace-tools.shpass;python3 -m pytest tests/test_sync_workspace_skills.py60/60;claude plugin validatepasses for the marketplace and the plugin./altimate-code:refresh-skillsruns and reports the copies; MCP mode and CLI mode each answered workspace questions correctly against a #1609 build.mainat 6ca6ecf), with a build of #1609 on PATH and a local DuckDB warehouse:altimate-codeis on PATH;ALTIMATE_DATA_MODE=cli, withdeniedMcpServersnaming the server (by nameplugin:altimate-code:altimate-codeor by command), and withallowedMcpServersthat leaves it out;allowedMcpServersnames it.ALTIMATE_ROUTE_GUARD=1):psql -c,snowsql -q,bq query,docker exec ... clickhouse-clientandecho $(psql -c ...);altimate tool run ...,dbt runandgit commit -m 'psql -c ...';refresh-skills.sh --if-stale: exits cleanly when the project is not linked.claude --plugin-dir), about $0.47 for both:altimate-code tool runthrough Bash with the same answers. The DELETE was refused ("Re-run with --allow-write"), and the row was still there afterwards.Review round (human consensus review of 845085c)
All 6 review threads and the nits comment have replies. Fixed in eb4c0e8 and 6038100:
PRIVACY.mdworkspace-tool data flows, including the backend models behindask_knowledge_base.… and N more: 300 skills give 6,903 characters in CLI mode.--wait.disabledMcpjsonServerscorrected.Tests:
tests/*.shpass andpytest tests/63 pass.Follow-ups: read
serverCommandfrom.mcp.json; a time limit onworkspace refresh; the npm.cmdshim under a stdio spawn on Windows; YAML double-quote escapes in copied descriptions (together with the CLI).Known limitations
altimate-codewithoutmcp serveis on PATH, the hook still picks MCP mode; the server fails to start and Claude Code caches that failure until restarted.altimate; MCP mode needs thealtimate-codename on PATH (CLI mode works with either).🤖 Generated with Claude Code