Skip to content

chore: tune Copilot PR review + restructure AGENTS.md as workspace-wide#56

Merged
bglusman merged 5 commits intomainfrom
chore/copilot-review-instructions
Apr 27, 2026
Merged

chore: tune Copilot PR review + restructure AGENTS.md as workspace-wide#56
bglusman merged 5 commits intomainfrom
chore/copilot-review-instructions

Conversation

@bglusman
Copy link
Copy Markdown
Owner

@bglusman bglusman commented Apr 26, 2026

Three related changes for AI-agent guidance on this repo:

  1. .github/copilot-instructions.md — global per-repo instructions Copilot reads on every PR review (per GitHub's docs).
  2. .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.
  3. AGENTS.md restructure — root file was titled "Calciforge Host-Agent" and carried host-agent-only build/security rules. Moved to crates/host-agent/AGENTS.md (with git mv so history is preserved); new root AGENTS.md is 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:

Key rules adopted:

  • "If uncertain, do not comment" — borrowed from deno's copilot-instructions, the single highest-leverage anti-noise rule.
  • Verify-before-flag — don't claim a missing test/handler/import without grepping the diff and surrounding repo first.
  • Priority hierarchy — Correctness → Security → Resource & error handling → Performance (if measurable) → Style (skip).
  • Skip-list for tools that already gate (cargo fmt, cargo clippy, gitleaks).
  • Project-specific NOT-a-bug context{{secret:NAME}} sentinel, FnoxClient subprocess-by-design, clashd daemon adapter, zeroclaw_* upstream refs, mixed Rust edition.
  • Past bug classes worth pattern-matching — substitution-after-bypass, (url, None) dest_host bug, bearer logged at info!, fnox set argv leak.
  • Cross-refs AGENTS.md (workspace-wide) and CLAUDE.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/expect heuristics, // SAFETY: requirement, edition 2024 set_var unsafety, std::sync::Mutex across .await, blocking I/O in async, select! cancellation safety, kill_on_drop(true) on long-running children, JoinHandle panic-swallowing, #[expect] over #[allow], #[non_exhaustive] / #[must_use], &str over &String, LazyLock<Regex> for hot paths, plus an explicit "what NOT to flag" list.

AGENTS.md restructure

AGENTS.md is the emerging vendor-neutral standard (Codex, OpenAI cloud agent, etc.) for repo-level agent guidance — analogous to CLAUDE.md for Claude Code. The existing root file was 50 lines of host-agent-only content (ZFS delegation, mTLS CN→Unix user mapping, clash-agent user model). Agents working in any other crate were reading rules that didn't apply and missing the workspace-wide ones.

New root AGENTS.md:

  • Crate inventory with one-line role descriptions
  • Project vocabulary (Calciforge / Calcifer / Moving Castle / Doors / {{secret:NAME}} sentinel / zeroclaw_*)
  • 7 mandatory rules every agent must follow (CLAUDE.md secret discipline pointer, pre-commit gate, sentinel contract, substitution boundary order, no-secret-values-in-logs, fnox set stdin mode)
  • Workspace build/test commands (incl. loom)
  • Pointers into per-area files

Host-agent specifics now at crates/host-agent/AGENTS.md (preserved via git 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-neutral AGENTS.md at root.

Note on commit history

Four commits on the branch:

  1. v1 of copilot-instructions.md
  2. v2 rewrite (motivated partly by an unverified ~4000-char limit assumption — turned out to be the older Copilot Chat feature, not code review)
  3. Restored AGENTS.md / CLAUDE.md cross-refs that v2 dropped on that incorrect premise
  4. AGENTS.md workspace restructure

Squash-on-merge will collapse this; left unsquashed it shows the iteration honestly.

🤖 Generated with Claude Code

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.
Copilot AI review requested due to automatic review settings April 26, 2026 22:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/copilot-instructions.md Outdated
Comment thread .github/copilot-instructions.md Outdated
- **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.
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- **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`.

Copilot uses AI. Check for mistakes.
Comment thread .github/copilot-instructions.md Outdated
Comment on lines +21 to +24
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)
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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)

Copilot uses AI. Check for mistakes.
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.
Copilot AI review requested due to automatic review settings April 26, 2026 23:14
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .github/copilot-instructions.md Outdated

- `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.
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- `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.

Copilot uses AI. Check for mistakes.
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.
@bglusman bglusman changed the title chore(.github): add copilot-instructions.md to tune PR-review behavior chore: tune Copilot PR review + restructure AGENTS.md as workspace-wide Apr 27, 2026
…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.
Copilot AI review requested due to automatic review settings April 27, 2026 02:06
@bglusman
Copy link
Copy Markdown
Owner Author

Triage of Copilot's 4 inline comments:

# Claim Verdict
1 Wrong link to docs/architecture-review-2026-04-25.md (real path research/reviews/...) Outdated — that whole MEDIUM section was removed in v2
2 crate::env shim doesn't exist in crates/calciforge Outdated + would have been correct — also removed in v2; the shim genuinely doesn't exist, so v1 was wrong, but the line is gone
3 Pre-commit hook is local-only, not auto-on-every-commit Outdated nit — v2 already says "pre-commit blocks commit" / "pre-commit + CI"
4 +15555550100, 7000000001, eyJ0eXAi... are not in .gitleaks.toml allowlist Real bug in v2 — fixed in 0080fbc. Real mechanism is path-based + a small regex list. 7000000001 is in auth.rs non-allowlisted code.

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.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread AGENTS.md
Comment on lines +17 to +19
| Crate | Role |
|---|---|
| `calciforge` | Channel router, identity, command dispatch, model gateway. The user-facing binary. |
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
- Tokio async runtime
- Axum for HTTP/mTLS
- No `unsafe` code
- All async functions must be `Send + Sync`
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- 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

Copilot uses AI. Check for mistakes.
- Audit: append-only with `chattr +a`

## Spec Reference
`/root/.openclaw/workspace/specs/CALCIFORGE-HOST-AGENT-SPEC.md`
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
`/root/.openclaw/workspace/specs/CALCIFORGE-HOST-AGENT-SPEC.md`
`../../specs/CALCIFORGE-HOST-AGENT-SPEC.md`

Copilot uses AI. Check for mistakes.
@bglusman bglusman merged commit b523ffe into main Apr 27, 2026
18 checks passed
@bglusman bglusman deleted the chore/copilot-review-instructions branch May 1, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants