chore: Updating agents instructions + Adding Claude Code ones. #124
Conversation
nicolasnoble
had a problem deploying
to
GITLAB
GitHub Actions
Failure
There was a problem hiding this comment.
Pull request overview
This PR updates AI agent instruction files to provide consistent guidance across multiple AI development tools (Claude Code, GitHub Copilot, and Cursor). It adds a new CLAUDE.md file with comprehensive guidance and updates existing instruction files to include a new section on adding CLI arguments and a reminder to keep all AI agent instruction files synchronized.
- Adds comprehensive CLAUDE.md file with build commands, architecture overview, and coding standards
- Adds "Adding CLI Arguments" section to all three instruction files with guidance on ClientArgs structure
- Adds "AI Agent Instructions" section reminding contributors to update all instruction files when introducing new patterns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| CLAUDE.md | New comprehensive instruction file for Claude Code with build commands, architecture details, and coding standards |
| .github/copilot-instructions.md | Added CLI argument guidelines and reminder about synchronizing instruction files |
| .cursor/rules/rust.mdc | Added CLI argument guidelines and reminder about synchronizing instruction files |
| # CLAUDE.md | ||
|
|
||
| This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. | ||
|
|
||
| ## Build and Development Commands | ||
|
|
||
| ```bash | ||
| # Build the project | ||
| cargo build | ||
|
|
||
| # Build in release mode | ||
| cargo build --release | ||
|
|
||
| # Run the server | ||
| cargo run --bin modelexpress-server | ||
|
|
||
| # Run tests | ||
| cargo test | ||
|
|
||
| # Run integration tests (starts server, runs test client) | ||
| ./run_integration_tests.sh | ||
|
|
||
| # Run a specific test client | ||
| cargo run --bin test_client -- --test-model "google-t5/t5-small" | ||
|
|
||
| # Run clippy (required before submitting code) | ||
| cargo clippy | ||
|
|
||
| # Generate sample configuration file | ||
| cargo run --bin config_gen -- --output model-express.yaml | ||
| ``` | ||
|
|
||
| ## Architecture | ||
|
|
||
| ModelExpress is a Rust-based model cache management service that accelerates inference by caching HuggingFace models. It can be deployed standalone or as a sidecar alongside inference solutions like NVIDIA Dynamo. | ||
|
|
||
| ### Workspace Structure | ||
|
|
||
| The project is a Rust workspace with three crates: | ||
|
|
||
| - **`modelexpress_server`** (`modelexpress-server`): gRPC server providing model services | ||
| - `services.rs`: Implements `HealthService`, `ApiService`, and `ModelService` gRPC services | ||
| - `database.rs`: SQLite-based model status persistence via `ModelDatabase` | ||
| - `cache.rs`: Cache eviction and management | ||
| - Uses global `MODEL_TRACKER` (`LazyLock<ModelDownloadTracker>`) for tracking download state | ||
|
|
||
| - **`modelexpress_client`** (`modelexpress-client`): Client library and CLI tool | ||
| - `lib.rs`: Main `Client` struct with gRPC clients for health, API, and model services | ||
| - `bin/cli.rs`: HuggingFace CLI replacement for model downloads | ||
| - Supports automatic fallback to direct download when server unavailable | ||
|
|
||
| - **`modelexpress_common`** (`modelexpress-common`): Shared code and protobuf definitions | ||
| - `grpc/` module contains generated proto code (health, api, model) | ||
| - `providers/huggingface.rs`: HuggingFace download implementation | ||
| - `download.rs`: Provider-agnostic download orchestration | ||
| - `cache.rs`, `config.rs`, `client_config.rs`: Configuration types | ||
|
|
||
| ### gRPC Services | ||
|
|
||
| Protocol definitions are in `modelexpress_common/proto/`: | ||
| - `health.proto`: Health check endpoint | ||
| - `api.proto`: Generic request/response API | ||
| - `model.proto`: Model download with streaming status updates | ||
|
|
||
| ### Key Patterns | ||
|
|
||
| - Download status tracked in SQLite database with compare-and-swap for concurrent request handling | ||
| - Streaming gRPC responses for download progress updates via `ModelStatusUpdate` | ||
| - `CacheConfig::discover()` finds cache configuration from environment or config files | ||
| - Configuration layering: CLI args > environment variables > config files > defaults | ||
|
|
||
| ### Adding CLI Arguments | ||
|
|
||
| Client CLI arguments and environment variables are defined in a shared struct to avoid duplication: | ||
|
|
||
| 1. **`ClientArgs`** in `modelexpress_common/src/client_config.rs`: | ||
| - Single source of truth for shared client arguments (endpoint, timeout, cache settings, etc.) | ||
| - Add new arguments here with `#[arg(long, env = "MODEL_EXPRESS_...")]` | ||
| - Avoid `-v` short flag (reserved for CLI's verbose) | ||
|
|
||
| 2. **`ClientConfig::load()`** in the same file: | ||
| - Apply the new argument to the config struct in the "APPLY CLI ARGUMENT OVERRIDES" section | ||
|
|
||
| 3. **`Cli`** in `modelexpress_client/src/bin/modules/args.rs`: | ||
| - Embeds `ClientArgs` via `#[command(flatten)]` | ||
| - Only add CLI-specific arguments here (e.g., `--format`, `--verbose`) | ||
|
|
||
| 4. **Tests**: Add tests in `client_config.rs` for argument parsing and config loading | ||
|
|
||
| ## Code Standards | ||
|
|
||
| - **No `unwrap()`**: Strictly forbidden except in benchmarks. Use `match`, `?`, or `expect()` (tests only) | ||
| - **All dependencies in root `Cargo.toml`**: Sub-crates use workspace dependencies exclusively | ||
| - **Clippy enforced**: `cargo clippy` must pass with no warnings (multiple lints set to deny) | ||
| - **No emojis in code** | ||
| - **No markdown documentation files for code changes** | ||
|
|
||
| ## AI Agent Instructions | ||
|
|
||
| When introducing new patterns, conventions, or architectural decisions that affect how code should be written, update ALL AI agent instruction files: | ||
| - `CLAUDE.md` (Claude Code) | ||
| - `.github/copilot-instructions.md` (GitHub Copilot) | ||
| - `.cursor/rules/rust.mdc` (Cursor) |
There was a problem hiding this comment.
This markdown file appears to be documenting code architecture and development patterns, which contradicts the repository's coding guideline that states "Do not create markdown files to document code changes or decisions." While CLAUDE.md is intended for AI agent instructions (which may have different rules), it goes beyond simple instructions and provides extensive architectural documentation. Consider whether this exception is intentional or if the architectural content should be integrated into existing documentation like README.md or CONTRIBUTING.md.
| - Embeds `ClientArgs` via `#[command(flatten)]` | ||
| - Only add CLI-specific arguments here (e.g., `--format`, `--verbose`) |
There was a problem hiding this comment.
The statement "Embeds ClientArgs via #[command(flatten)]" is inaccurate. The Cli struct in modelexpress_client/src/bin/modules/args.rs does not use #[command(flatten)] to embed ClientArgs. Instead, the CLI defines its own arguments and manually constructs a ClientArgs struct from them (see modelexpress_client/src/bin/cli.rs lines 26-38). This documentation should be corrected to accurately reflect the current implementation.
| - Embeds `ClientArgs` via `#[command(flatten)]` | |
| - Only add CLI-specific arguments here (e.g., `--format`, `--verbose`) | |
| - Defines CLI-specific arguments (e.g., `--format`, `--verbose`, model identifiers) | |
| - Values from `Cli` are used in `modelexpress_client/src/bin/cli.rs` to manually construct a `ClientArgs` instance (no `#[command(flatten)]` embedding) |
| 1. **`ClientArgs`** in `modelexpress_common/src/client_config.rs`: | ||
| - Single source of truth for shared client arguments (endpoint, timeout, cache settings, etc.) | ||
| - Add new arguments here with `#[arg(long, env = "MODEL_EXPRESS_...")]` | ||
| - Avoid `-v` short flag (reserved for CLI's verbose) |
There was a problem hiding this comment.
The guidance to "Avoid -v short flag (reserved for CLI's verbose)" is contradicted by the actual code. ClientArgs in modelexpress_common/src/client_config.rs line 33 uses short = 'v' for log_level. This creates a conflict with the CLI's use of -v for verbose mode. Either the documentation should acknowledge this existing conflict, or the code should be updated to remove the -v short flag from one of these usages.
| - Avoid `-v` short flag (reserved for CLI's verbose) | |
| - Note: `ClientArgs` currently uses `-v` as the short flag for `log_level`, and the CLI also uses `-v` for `--verbose`; avoid introducing any additional uses of `-v` and prefer long-only flags for new options until this duplication is refactored. |
| 3. **Do NOT duplicate in `Cli`** (`modelexpress_client/src/bin/modules/args.rs`): | ||
| - `Cli` embeds `ClientArgs` via `#[command(flatten)]` | ||
| - Only add CLI-specific arguments there (e.g., `--format`, `--verbose`) | ||
|
|
There was a problem hiding this comment.
The statement "Cli embeds ClientArgs via #[command(flatten)]" is inaccurate. The Cli struct in modelexpress_client/src/bin/modules/args.rs does not use #[command(flatten)] to embed ClientArgs. Instead, the CLI defines its own arguments and manually constructs a ClientArgs struct from them (see modelexpress_client/src/bin/cli.rs lines 26-38). This documentation should be corrected to accurately reflect the current implementation.
| 3. **Do NOT duplicate in `Cli`** (`modelexpress_client/src/bin/modules/args.rs`): | |
| - `Cli` embeds `ClientArgs` via `#[command(flatten)]` | |
| - Only add CLI-specific arguments there (e.g., `--format`, `--verbose`) | |
| 3. **Wire CLI arguments into `ClientArgs`**: | |
| - Define user-facing flags in `Cli` (`modelexpress_client/src/bin/modules/args.rs`) | |
| - In `modelexpress_client/src/bin/cli.rs` (see lines 26–38), construct a `ClientArgs` from the `Cli` fields | |
| - Keep `ClientArgs` as the single source of truth for shared client configuration; only add CLI-specific arguments to `Cli` (e.g., `--format`, `--verbose`) |
| 1. **Add to `ClientArgs`** in `modelexpress_common/src/client_config.rs`: | ||
| - This is the single source of truth for shared arguments | ||
| - Use `#[arg(long, env = "MODEL_EXPRESS_...")]` for environment variable support | ||
| - Do NOT use `-v` short flag (reserved for CLI's verbose) |
There was a problem hiding this comment.
The guidance to "Do NOT use -v short flag (reserved for CLI's verbose)" is contradicted by the actual code. ClientArgs in modelexpress_common/src/client_config.rs line 33 uses short = 'v' for log_level. This creates a conflict with the CLI's use of -v for verbose mode. Either the documentation should acknowledge this existing conflict, or the code should be updated to remove the -v short flag from one of these usages.
| - Do NOT use `-v` short flag (reserved for CLI's verbose) | |
| - Do NOT introduce new `-v` short flags (reserved for CLI verbosity and currently used by `log_level`) |
| - `Cli` embeds `ClientArgs` via `#[command(flatten)]` | ||
| - Only add CLI-specific arguments there (e.g., `--format`, `--verbose`) |
There was a problem hiding this comment.
The statement "Cli embeds ClientArgs via #[command(flatten)]" is inaccurate. The Cli struct in modelexpress_client/src/bin/modules/args.rs does not use #[command(flatten)] to embed ClientArgs. Instead, the CLI defines its own arguments and manually constructs a ClientArgs struct from them (see modelexpress_client/src/bin/cli.rs lines 26-38). This documentation should be corrected to accurately reflect the current implementation.
| - `Cli` embeds `ClientArgs` via `#[command(flatten)]` | |
| - Only add CLI-specific arguments there (e.g., `--format`, `--verbose`) | |
| - `Cli` defines its own CLI-facing arguments; `modelexpress_client/src/bin/cli.rs` is responsible for constructing a `ClientArgs` instance from them | |
| - Keep shared configuration fields in `ClientArgs` and only add CLI-specific options to `Cli` (e.g., `--format`, `--verbose`) |
| 1. **Add to `ClientArgs`** in `modelexpress_common/src/client_config.rs`: | ||
| - This is the single source of truth for shared arguments | ||
| - Use `#[arg(long, env = "MODEL_EXPRESS_...")]` for environment variable support | ||
| - Do NOT use `-v` short flag (reserved for CLI's verbose) |
There was a problem hiding this comment.
The guidance to "Do NOT use -v short flag (reserved for CLI's verbose)" is contradicted by the actual code. ClientArgs in modelexpress_common/src/client_config.rs line 33 uses short = 'v' for log_level. This creates a conflict with the CLI's use of -v for verbose mode. Either the documentation should acknowledge this existing conflict, or the code should be updated to remove the -v short flag from one of these usages.
| - Do NOT use `-v` short flag (reserved for CLI's verbose) | |
| - Avoid introducing new uses of the `-v` short flag; it is reserved for the top-level CLI `--verbose` option (there is a legacy use in `ClientArgs` for `log_level` that will be cleaned up separately). |
Signed-off-by: Nicolas Pixel Noble <pixel@nobis-crew.org>
nicolasnoble
force-pushed
the
nnoble/agents-config
branch
from
5fc7551 to
facbbc0
Compare
nicolasnoble
had a problem deploying
to
GITLAB
GitHub Actions
Failure
No description provided.