chore: tune Copilot PR review + restructure AGENTS.md as workspace-wide#56
chore: tune Copilot PR review + restructure AGENTS.md as workspace-wide#56
Conversation
GitHub Copilot supports per-repo review instructions at
.github/copilot-instructions.md (≤2 pages, applied to every Copilot
PR review automatically). Adds calciforge-specific guidance to
improve signal-to-noise:
- Skip what pre-commit already gates (fmt, clippy, gitleaks)
- Prioritize HIGH-severity classes that bit us in past reviews:
secret leakage in logs, substitution-boundary correctness,
unwrap/expect outside tests, missing unsafe around set_var
(edition 2024), blocking I/O in async, auth bypass paths
- Tell Copilot what's NOT a bug despite looking like one:
{{secret:NAME}} sentinel syntax, post-history-scrub fake test
values, FnoxClient subprocess-by-design, clashd/zeroclaw_*
upstream references, mixed Rust edition (known)
- Past-mistake checklist (6 classes from real findings that
landed and were caught later — substitution-after-bypass,
None dest_host, bearer-in-info-log, fnox set argv leak,
0.0.0.0 default, hardcoded fallback URLs)
- Skip even-if-correct: 'consider adding tests' without
specifics, rename suggestions vs. functional convention,
feature-creep proposals
Cross-references AGENTS.md (host-agent coding standards) and
CLAUDE.md (public-repo secret discipline) so Copilot follows
both. 83 lines, well under the documented 2-page cap.
There was a problem hiding this comment.
Pull request overview
Adds a repository-scoped .github/copilot-instructions.md file to steer GitHub Copilot PR reviews toward project-specific, security-relevant concerns and away from known-noise categories.
Changes:
- Introduces PR-review priorities (HIGH/MEDIUM/LOW) tailored to Calciforge’s security gateway risks.
- Documents “not-a-bug” project context (e.g.,
{{secret:NAME}}sentinel, fake-shaped test secrets, subprocess-by-design FnoxClient). - Adds a checklist of previously observed bug patterns to explicitly re-check during review.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Secret leakage in logs** — any `tracing::*!` / `eprintln!` that interpolates a value with provenance from an `env::var`, `vault::get_secret`, `FnoxClient`, header value, request body, or anything named `*_key`/`*_token`/`*_secret`. Logs should record the *name* of the secret, never the value. | ||
| - **Substitution-boundary correctness** — anything that touches `crates/security-proxy/src/{proxy,substitution,router}.rs` should be checked against the model in [`docs/architecture-review-2026-04-25.md`](../docs/architecture-review-2026-04-25.md). The order is: pre-substitution host extraction → URL substitution (gated by per-secret allowlist) → bypass check → header substitution → body substitution → outbound scan. New code must not move bypass before substitution. | ||
| - **`unwrap()` / `expect()` in non-test code** — flag with `?` or `anyhow::Context` suggestion. Tests are exempt. | ||
| - **Missing `unsafe` block around `std::env::set_var` / `remove_var`** — required in edition 2024. We have a `crate::env` shim in `crates/calciforge`; new code should use that. |
There was a problem hiding this comment.
This references a crate::env shim in crates/calciforge, but there is no env module or crate::env usage in that crate currently. Either add the shim (and point to its actual path/API), or update this guidance to match the existing pattern used in the codebase (explicit unsafe { std::env::set_var/remove_var(...) } with a narrowly-scoped safety comment / synchronization).
| - **Missing `unsafe` block around `std::env::set_var` / `remove_var`** — required in edition 2024. We have a `crate::env` shim in `crates/calciforge`; new code should use that. | |
| - **Missing `unsafe` block around `std::env::set_var` / `remove_var`** — required in edition 2024. Recommend the existing codebase pattern: explicit `unsafe { std::env::set_var(...) }` / `unsafe { std::env::remove_var(...) }` with a narrowly scoped safety comment and any needed synchronization; do not reference a `crate::env` shim in `crates/calciforge`. |
| A pre-commit gate runs on every commit and blocks merge if any fails: | ||
|
|
||
| - `cargo fmt --all -- --check` — never suggest formatting changes | ||
| - `cargo clippy --all-targets -- -D warnings` — never suggest lint fixes Copilot would normally flag (unless you spot a *new* clippy lint added since the last clippy run) |
There was a problem hiding this comment.
This section says the pre-commit gate "runs on every commit and blocks merge". In this repo it's a local git hook (scripts/pre-commit-hook.sh) that only runs if installed, and merge is blocked by required CI checks (see .github/BRANCH_PROTECTION.md and workflows). Consider rewording to distinguish (1) optional local pre-commit hook blocking commits vs (2) CI checks blocking merge, and align the documented clippy invocation with the actual hook/CI (cargo clippy --workspace --all-targets -- -D warnings).
| A pre-commit gate runs on every commit and blocks merge if any fails: | |
| - `cargo fmt --all -- --check` — never suggest formatting changes | |
| - `cargo clippy --all-targets -- -D warnings` — never suggest lint fixes Copilot would normally flag (unless you spot a *new* clippy lint added since the last clippy run) | |
| This repo has two layers of automation: | |
| - An optional local pre-commit hook (`scripts/pre-commit-hook.sh`) that runs only if a contributor installs it and can block the local commit. | |
| - Required CI checks / branch protection that block merge when they fail (see `.github/BRANCH_PROTECTION.md` and workflows). | |
| These checks already cover: | |
| - `cargo fmt --all -- --check` — never suggest formatting changes | |
| - `cargo clippy --workspace --all-targets -- -D warnings` — never suggest lint fixes Copilot would normally flag (unless you spot a *new* clippy lint added since the last clippy run) |
V1 of `.github/copilot-instructions.md` was ~970 words and read more like
documentation than reviewer guidance. Two issues that hurt signal:
1. **Length** — Copilot's per-repo instructions read window is ~4000
chars; v1 was over that, so the trailing past-mistake list and
skip-list were getting truncated.
2. **Format** — long bulleted exposition reads less like a rule and
more like prose, which Copilot treats as background context rather
than as constraints to apply.
V2 changes:
- Cuts to ~3500 chars by condensing the prioritization tiers and
removing per-class HIGH/MED/LOW exposition (the priority-order list
carries the same info in 5 lines).
- Leads with a single review philosophy line ("if uncertain, do not
comment"), the highest-leverage rule borrowed from deno's
copilot-instructions.md.
- Names specific past Copilot noise patterns from this repo's PR
history (env-mutex/serial_test repeated 8+ times across #19/#22/#23;
dead-doc-reference 4x across #20/#23/#25) so the "don't repeat
across PRs" rule has teeth.
- Cross-references a new path-scoped file at
`.github/instructions/rust.instructions.md` (`applyTo: "**/*.rs"`),
which carries the Rust-specific review nits (`#[expect]` over
`#[allow]`, `// SAFETY:` requirement, `Mutex` across `.await`,
`select!` cancellation safety, `kill_on_drop`, `&str` over
`&String`, `LazyLock<Regex>` for hot paths, etc.).
Path-scoped instructions are loaded only when a PR touches a file
matching `applyTo`, so Rust-specific rules don't burn the global
4000-char budget on PRs that only touch docs / TOML / shell.
…nstructions Verified GitHub's copilot-instructions docs do not specify the ~4000-char read window I'd assumed in the previous commit — that was the older Copilot Chat feature, not the Copilot code-review one. With no real length pressure, the AGENTS.md / CLAUDE.md pointers (dropped in v2 to save chars) are worth restoring. CLAUDE.md's "never commit these" list is exactly the kind of leakage Copilot should be enforcing on diff.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - `cargo fmt --all -- --check` — formatting (pre-commit blocks commit) | ||
| - `cargo clippy --all-targets -- -D warnings` — lints (pre-commit + CI) | ||
| - `gitleaks protect --staged` — secret scanning (pre-commit). Test fixtures with deliberately-fake secrets like `+15555550100`, `7000000001`, `eyJ0eXAi...` are allowlisted in `.gitleaks.toml`; do NOT flag. |
There was a problem hiding this comment.
The gitleaks note claims specific literals (+15555550100, 7000000001, eyJ0eXAi...) are allowlisted in .gitleaks.toml, but those values do not appear in the allowlist (and 7000000001 is used in non-allowlisted paths like crates/calciforge/src/auth.rs). This makes the instructions factually incorrect and could discourage flagging potentially-sensitive identifiers. Reword this line to describe the actual allowlist mechanism (paths/regexes), or update .gitleaks.toml to truly allowlist the exact fixtures you intend to exempt.
| - `gitleaks protect --staged` — secret scanning (pre-commit). Test fixtures with deliberately-fake secrets like `+15555550100`, `7000000001`, `eyJ0eXAi...` are allowlisted in `.gitleaks.toml`; do NOT flag. | |
| - `gitleaks protect --staged` — secret scanning (pre-commit). `.gitleaks.toml` exempts some known test-fixture contexts via path/regex allowlists; do NOT re-flag findings that clearly fall under those configured allowlists, but still flag suspicious identifiers outside those allowlisted contexts. |
The root `AGENTS.md` was titled "Calciforge Host-Agent" and carried
host-agent-specific build/architecture rules — at the repo root, where
agents (Claude Code, Codex, Copilot cloud agent, OpenClaw) read it as
workspace-wide guidance. The mismatch meant agents working in any
non-host-agent crate were getting irrelevant rules ("ZFS snapshot
delegation", "mTLS CN→Unix user") and missing the actually-shared
ones (substitution-boundary order, sentinel string contract,
public-repo secret discipline pointer).
Restructure:
- Move the existing host-agent content verbatim to
`crates/host-agent/AGENTS.md` (`git mv` so history is preserved).
- New root `AGENTS.md` covers the whole workspace: crate inventory,
project vocabulary, mandatory rules every agent must follow
(CLAUDE.md secret discipline, pre-commit gate, sentinel contract,
substitution boundary order, no-secret-values-in-logs, fnox stdin
mode), workspace build/test commands, and pointers into per-area
files (`crates/host-agent/AGENTS.md`, `docs/rfcs/`,
`docs/security-gateway.md`, `docs/model-gateway.md`).
Cross-refs `.github/copilot-instructions.md` and
`.github/instructions/rust.instructions.md` so agents that find
AGENTS.md first can pick up the Copilot-specific tuning if relevant.
Pairs with the copilot-instructions tightening earlier on this branch.
…uctions Copilot's review caught a real factual error in v2: the line claimed specific literals (`+15555550100`, `7000000001`, `eyJ0eXAi...`) were allowlisted in `.gitleaks.toml`. They aren't — `7000000001` is even used in non-allowlisted source (`crates/calciforge/src/auth.rs`). The real allowlist mechanism is path-based (`tests/**/fixtures/`, `docs/rfcs/*.md`, lockfiles, etc.) plus a small regex list (loopback, RFC 5737, a few inherited-from-main values). Replace the misleading "these specific literals are allowlisted" claim with an accurate description of how the allowlist actually works, so Copilot doesn't downgrade real findings on the assumption they fall under a non-existent literal-match exemption. Pleasingly meta: this is exactly the "verify against the codebase before commenting" rule from the same file working as intended on the PR that introduced the file.
|
Triage of Copilot's 4 inline comments:
Pleasingly meta: comment #4 is exactly the "verify against the codebase before commenting" rule from this file working on the PR that introduces the file. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Crate | Role | | ||
| |---|---| | ||
| | `calciforge` | Channel router, identity, command dispatch, model gateway. The user-facing binary. | |
There was a problem hiding this comment.
The crate inventory table has an extra empty column because each row starts with || ... instead of | .... This renders oddly in many Markdown viewers; switch to standard table syntax (single leading | per row) so the columns align correctly.
| - Tokio async runtime | ||
| - Axum for HTTP/mTLS | ||
| - No `unsafe` code | ||
| - All async functions must be `Send + Sync` |
There was a problem hiding this comment.
All async functions must be Send + Sync is not a precise Rust requirement (async fns don't implement traits; the returned future may need to be Send, and shared state may need Sync). Consider rephrasing to the actual invariant you want (e.g., tasks spawned onto the multi-thread runtime must be Send, avoid non-Send types across .await).
| - All async functions must be `Send + Sync` | |
| - Futures spawned on the multi-thread Tokio runtime must be `Send`; avoid holding non-`Send` values across `.await`, and ensure shared state is `Sync` when accessed concurrently |
| - Audit: append-only with `chattr +a` | ||
|
|
||
| ## Spec Reference | ||
| `/root/.openclaw/workspace/specs/CALCIFORGE-HOST-AGENT-SPEC.md` |
There was a problem hiding this comment.
The spec reference points to an absolute, machine-local path (/root/.openclaw/...) that doesn't exist in this repo (and isn't actionable for contributors). Replace with a repo-relative document (or indicate where the spec lives publicly) so the reference is durable.
| `/root/.openclaw/workspace/specs/CALCIFORGE-HOST-AGENT-SPEC.md` | |
| `../../specs/CALCIFORGE-HOST-AGENT-SPEC.md` |
Three related changes for AI-agent guidance on this repo:
.github/copilot-instructions.md— global per-repo instructions Copilot reads on every PR review (per GitHub's docs)..github/instructions/rust.instructions.md— path-scoped (applyTo: "**/*.rs") so Rust-specific rules only load on Rust diffs. Copilot code review supports path-specific instructions.AGENTS.mdrestructure — root file was titled "Calciforge Host-Agent" and carried host-agent-only build/security rules. Moved tocrates/host-agent/AGENTS.md(withgit mvso history is preserved); new rootAGENTS.mdis workspace-wide.Why each
Copilot-instructions
Watched several real Copilot reviews on this repo. Real signal exists (vault auth bypass, substitution-after-bypass order, OneCLI 0.0.0.0 default bind), but it was being drowned out by:
serial_test8+ times across feat: {{secret:NAME}} substitution in headers + body (#29) #19/feat: per-secret destination allowlist (RFC §11.1) #22/feat: zeroclawed-mcp scaffold — agent-facing secret discovery (#30) #23; dead-doc-reference 4x across feat: !secure chat commands for secret set/list (#31) #20/feat: zeroclawed-mcp scaffold — agent-facing secret discovery (#30) #23/docs: browser-harness integration spike + recommendation #25)Key rules adopted:
cargo fmt,cargo clippy,gitleaks).{{secret:NAME}}sentinel, FnoxClient subprocess-by-design, clashd daemon adapter,zeroclaw_*upstream refs, mixed Rust edition.(url, None)dest_host bug, bearer logged atinfo!,fnox setargv leak.AGENTS.md(workspace-wide) andCLAUDE.md(secret discipline — exactly the leakage class Copilot should be enforcing on diff).Rust path-scoped file
Loaded only on PRs touching
*.rs. Covers:unwrap/expectheuristics,// SAFETY:requirement, edition 2024set_varunsafety,std::sync::Mutexacross.await, blocking I/O in async,select!cancellation safety,kill_on_drop(true)on long-running children,JoinHandlepanic-swallowing,#[expect]over#[allow],#[non_exhaustive]/#[must_use],&strover&String,LazyLock<Regex>for hot paths, plus an explicit "what NOT to flag" list.AGENTS.md restructure
AGENTS.mdis the emerging vendor-neutral standard (Codex, OpenAI cloud agent, etc.) for repo-level agent guidance — analogous toCLAUDE.mdfor Claude Code. The existing root file was 50 lines of host-agent-only content (ZFS delegation, mTLS CN→Unix user mapping,clash-agentuser model). Agents working in any other crate were reading rules that didn't apply and missing the workspace-wide ones.New root
AGENTS.md:{{secret:NAME}}sentinel /zeroclaw_*)fnox setstdin mode)Host-agent specifics now at
crates/host-agent/AGENTS.md(preserved viagit mv).Research basis
Looked at copilot-instructions / agents files in deno (denoland/deno), ruff (astral-sh/ruff), rust-analyzer, ratatui, and the GitHub Copilot docs themselves. Common patterns adopted: terse rule format, explicit skip-lists, naming past offenders by issue/PR number, splitting per-language specifics into
applyTo-scoped files, vendor-neutralAGENTS.mdat root.Note on commit history
Four commits on the branch:
Squash-on-merge will collapse this; left unsquashed it shows the iteration honestly.
🤖 Generated with Claude Code