feat: !secure chat commands for secret set/list (#31)#20
Closed
feat: !secure chat commands for secret set/list (#31)#20
Conversation
Adds `!secure` as a post-auth chat command dispatched before any agent
sees the message. Subcommands:
- `!secure set NAME=value` — stores the secret in fnox and replies
with "Stored `NAME`" + a retention-warning. Never echoes the value.
- `!secure list` — lists stored secret names (not values). Parses
fnox's output defensively; extra columns that look like values are
stripped before replying.
- `!secure help` / `!secure` — usage string.
Dispatch model (matches existing `!switch`/`!sessions`/`!default`):
- `CommandHandler::is_secure_command(&str)` — static, case-insensitive.
- `CommandHandler::handle_secure(text, identity_id) -> String` —
async (shells out to fnox), takes identity for future per-identity
audit/ACL though not used yet. Wired into telegram.rs in both
`handle_message_nonblocking` and `handle_message` paths, and added
to the unknown-command exclusion list so `!secure` doesn't get
intercepted as unknown before hitting the post-auth handler.
Subprocess approach today — `Command::new("fnox").args(["set", …])`.
Acknowledged-cost: three places now shell out to fnox (vault.rs,
secure_set, secure_list), which means three places that need an
availability check. **Follow-up PR (in flight) will migrate all three
to the fnox library crate via `fnox = { git = "https://github.com/jdx/fnox" }`**
— fnox already exposes public modules (secret_resolver, commands,
etc.) at `src/lib.rs`, so no upstream PR is needed.
Redaction discipline:
- The success reply names the stored secret but never echoes the
value; tested.
- The failure reply surfaces fnox's stderr (user needs to know why
it failed) but doesn't carry the value; tested.
- The telegram channel's `debug!` logs only that a !secure command
was handled, without the message text. A `!secure set FOO=bar`
would otherwise leak the value into ops logs.
- Name validation: `[A-Za-z0-9_-]+` only (matches the substitution
engine's accepted ref-name syntax — so a stored secret can be
immediately referenced as `{{secret:NAME}}` without a rename).
Tested with three invalid-char cases.
5 given/when/then behavioral tests:
- `secure_set_reply_includes_name_but_not_value`
- `secure_set_surfaces_fnox_error_without_echoing_value`
- `secure_unknown_subcommand_returns_help`
- `secure_set_rejects_invalid_name_chars`
- `secure_list_returns_names_only`
All use a fake-fnox shell script installed on PATH via TempDir, so
the tests are hermetic regardless of whether the real fnox is
installed on the dev/CI machine.
Explicit non-goals for this PR (follow-ups):
- `!secure remove NAME` — pending a decision on whether fnox's
own remove command or a config-file edit is the right approach.
- `!secure request NAME` — the out-of-band localhost-paste flow
that avoids chat-transport retention entirely (see
docs/rfcs/agent-secret-gateway.md §5).
- Matrix/WhatsApp channel wiring — same pattern as telegram, but
adds separate verification surface. Doing in a follow-up to keep
this PR reviewable.
- Library-based fnox integration (swap subprocess → lib calls) —
committed as next PR per user direction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a post-auth !secure chat command to ZeroClawed so secrets can be set/listed without ever being routed into an agent’s context (currently wired for Telegram only).
Changes:
- Introduces
CommandHandler::is_secure_commandandhandle_secureplussecure_set/secure_list/secure_helpimplementations. - Adds hermetic tests using a fake
fnoxbinary injected viaPATH. - Wires
!securedispatch into both Telegram message handling paths and excludes it from the unknown-command filter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crates/zeroclawed/src/commands.rs | Implements !secure parsing/dispatch, shells out to fnox, and adds tests ensuring no value is echoed back. |
| crates/zeroclawed/src/channels/telegram.rs | Routes !secure messages to the new handler (without logging raw text) and prevents unknown-command handling from intercepting it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+288
to
294
| let cmd = trimmed.split(' ').next().unwrap_or("").to_lowercase(); | ||
| cmd == "!secure" | ||
| } | ||
|
|
||
| /// Respond to unknown commands with a helpful message. | ||
| pub fn unknown_command(&self, text: &str) -> String { | ||
| let cmd = text.trim().split(' ').next().unwrap_or("").to_string(); |
There was a problem hiding this comment.
is_secure_command splits on a literal space (split(' ')). If a user sends !secure followed by a tab/newline (common when pasting from mobile), this will fail to detect the command and it may fall through to the unknown-command path (which can echo the entire first “token”, potentially including the secret). Use split_whitespace() (or a char::is_whitespace split) to reliably parse the leading command word.
Suggested change
| let cmd = trimmed.split(' ').next().unwrap_or("").to_lowercase(); | |
| cmd == "!secure" | |
| } | |
| /// Respond to unknown commands with a helpful message. | |
| pub fn unknown_command(&self, text: &str) -> String { | |
| let cmd = text.trim().split(' ').next().unwrap_or("").to_string(); | |
| let cmd = trimmed.split_whitespace().next().unwrap_or("").to_lowercase(); | |
| cmd == "!secure" | |
| } | |
| /// Respond to unknown commands with a helpful message. | |
| pub fn unknown_command(&self, text: &str) -> String { | |
| let cmd = text | |
| .trim() | |
| .split_whitespace() | |
| .next() | |
| .unwrap_or("") | |
| .to_string(); |
Comment on lines
+876
to
+880
| // `!secure ...` — split off the subcommand word. | ||
| let mut parts = trimmed.splitn(3, ' '); | ||
| let _lead = parts.next(); // `!secure` | ||
| let sub = parts.next().map(|s| s.to_lowercase()).unwrap_or_default(); | ||
| let rest = parts.next().unwrap_or("").trim(); |
There was a problem hiding this comment.
handle_secure uses splitn(..., ' ') to parse !secure <subcommand> .... Even if is_secure_command is fixed, this still won’t handle tabs/newlines or repeated whitespace between words, which can make set/list/help parsing fail for pasted input. Consider parsing the leading words with split_whitespace() and then deriving rest from the original string via slicing (or by joining the remaining iterator) to preserve the value portion.
Suggested change
| // `!secure ...` — split off the subcommand word. | |
| let mut parts = trimmed.splitn(3, ' '); | |
| let _lead = parts.next(); // `!secure` | |
| let sub = parts.next().map(|s| s.to_lowercase()).unwrap_or_default(); | |
| let rest = parts.next().unwrap_or("").trim(); | |
| // `!secure ...` — parse the leading words using whitespace, but preserve | |
| // the remainder for `set NAME=value`. | |
| let mut parts = trimmed.split_whitespace(); | |
| let lead = parts.next().unwrap_or_default(); // `!secure` | |
| let sub_raw = parts.next().unwrap_or_default(); | |
| let sub = sub_raw.to_lowercase(); | |
| let rest = if sub_raw.is_empty() { | |
| "" | |
| } else { | |
| let after_lead = &trimmed[lead.len()..]; | |
| if let Some(sub_start) = after_lead.find(sub_raw) { | |
| let after_sub = &after_lead[sub_start + sub_raw.len()..]; | |
| after_sub.trim() | |
| } else { | |
| "" | |
| } | |
| }; |
Comment on lines
+1217
to
+1219
| // Only allow the same name-shape substitution accepts, so callers | ||
| // can immediately use the stored name in a `{{secret:NAME}}` ref. | ||
| // See crates/security-proxy/src/substitution.rs. |
There was a problem hiding this comment.
The reference crates/security-proxy/src/substitution.rs appears to be stale/nonexistent in this repo, so the comment doesn’t provide a valid pointer for why the name regex is chosen. Please update it to the actual location that defines the secret-name syntax (or remove the reference if the check is self-contained here).
Suggested change
| // Only allow the same name-shape substitution accepts, so callers | |
| // can immediately use the stored name in a `{{secret:NAME}}` ref. | |
| // See crates/security-proxy/src/substitution.rs. | |
| // Only allow names that are immediately usable in a `{{secret:NAME}}` | |
| // reference. The accepted syntax here is ASCII alphanumeric plus `_` | |
| // and `-`, matching the validation enforced below. |
Comment on lines
+1230
to
+1233
| use tokio::process::Command; | ||
| let output = match Command::new("fnox") | ||
| .args(["set", &name, &value]) | ||
| .output() |
There was a problem hiding this comment.
fnox set is invoked with the secret value as a command-line argument (args(["set", &name, &value])). This exposes the secret to host-level process inspection (e.g., ps, procfs, process monitors) even though you avoid routing it to an agent. If fnox supports it, prefer passing the value via stdin (or a temp file descriptor / env var) to keep it out of argv; otherwise this should be called out as an explicit limitation until the library-based integration lands.
Comment on lines
+874
to
+876
| pub async fn handle_secure(&self, text: &str, _identity_id: &str) -> String { | ||
| let trimmed = text.trim(); | ||
| // `!secure ...` — split off the subcommand word. |
There was a problem hiding this comment.
handle_secure accepts an identity_id parameter but it’s currently unused. Either remove this parameter to avoid implying auditing/attribution, or use it (e.g., include it in an audit log entry or pass it to fnox metadata if supported) so the call sites’ “post-auth for audit” rationale is actually realized.
Comment on lines
+260
to
+267
| // agent. Runs post-auth so we can audit who set what; doesn't yet | ||
| // gate by role (open for any authenticated identity). The handler | ||
| // is async because it shells out to `fnox`. | ||
| // | ||
| // We deliberately do NOT log `text` here or in the handler — the | ||
| // `!secure set NAME=value` form contains a secret value that must | ||
| // not appear in ops logs. debug! logs only that a !secure command | ||
| // was handled; the handler never logs `text` either. |
There was a problem hiding this comment.
The comment says !secure runs post-auth “so we can audit who set what”, but handle_secure(..., identity_id) currently ignores the identity and secure_set/secure_list don’t emit any audit record. Please either implement auditing/attribution (at least a structured log with identity + secret name, never the value) or adjust the comment to avoid overstating the current behavior.
Suggested change
| // agent. Runs post-auth so we can audit who set what; doesn't yet | |
| // gate by role (open for any authenticated identity). The handler | |
| // is async because it shells out to `fnox`. | |
| // | |
| // We deliberately do NOT log `text` here or in the handler — the | |
| // `!secure set NAME=value` form contains a secret value that must | |
| // not appear in ops logs. debug! logs only that a !secure command | |
| // was handled; the handler never logs `text` either. | |
| // agent. Runs post-auth so the authenticated identity is available | |
| // to downstream handling; doesn't yet gate by role (open for any | |
| // authenticated identity). The handler is async because it shells | |
| // out to `fnox`. | |
| // | |
| // We deliberately do NOT log `text` here or in the handler — the | |
| // `!secure set NAME=value` form contains a secret value that must | |
| // not appear in ops logs. This site logs only that a !secure | |
| // command was handled, not a structured audit record of the secret | |
| // operation itself. |
a7bff45 to
a4a09ff
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+859
to
+861
| /// Handle `!secure <subcommand>`. Dispatches to `fnox` as a | ||
| /// subprocess; nothing about the secret value transits an agent's | ||
| /// context. Never echoes back values — responses are name-only. |
There was a problem hiding this comment.
PR description mentions three fnox subprocess call-sites (including vault.rs::get_secret_from_fnox), but in this branch the only Rust fnox subprocess usage appears to be in secure_set/secure_list (no other Command::new("fnox") call-sites found). Either update the PR description to match the actual change set, or include the other referenced migration in this PR if it was intended.
Comment on lines
+1276
to
+1278
| // that looks like a secret name (non-whitespace leading token) | ||
| // and present a name-only summary. Don't attempt structured | ||
| // parsing; bail to a raw echo if the output isn't lines-of-tokens. |
There was a problem hiding this comment.
The comment says to “bail to a raw echo if the output isn't lines-of-tokens”, but the current implementation always tokenizes every non-empty/non-# line and never falls back to raw output. Either implement the described fallback (e.g., detect unexpected formats) or adjust the comment to match the actual behavior.
Suggested change
| // that looks like a secret name (non-whitespace leading token) | |
| // and present a name-only summary. Don't attempt structured | |
| // parsing; bail to a raw echo if the output isn't lines-of-tokens. | |
| // that looks like a secret name (the first non-whitespace token on | |
| // each non-empty, non-comment line) and present a name-only summary. | |
| // Keep parsing permissive rather than attempting a strict structured | |
| // format check. |
Comment on lines
+1262
to
+1266
| async fn secure_list() -> String { | ||
| use tokio::process::Command; | ||
| let output = match Command::new("fnox").arg("list").output().await { | ||
| Ok(o) => o, | ||
| Err(e) => { |
There was a problem hiding this comment.
Both secure_set and secure_list await Command::output() with no timeout. If fnox blocks (waiting for input, stuck on backend, etc.) these spawned tasks can hang indefinitely and accumulate under load. Consider wrapping the subprocess in a tokio::time::timeout and returning a clear error on expiry (and/or killing the child process).
Comment on lines
+2002
to
+2014
| unsafe { | ||
| std::env::set_var("PATH", new_path); | ||
| } | ||
| Self { original } | ||
| } | ||
| } | ||
| impl Drop for PathGuard { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| match &self.original { | ||
| Some(p) => std::env::set_var("PATH", p), | ||
| None => std::env::remove_var("PATH"), | ||
| } |
There was a problem hiding this comment.
The unsafe { std::env::set_var(...) } block is unnecessary in this crate (edition 2021), so it will trigger the unused_unsafe warning; with CI running cargo clippy -- -D warnings, this likely fails the build. Either remove the unsafe blocks, or add a targeted #[allow(unused_unsafe)]/#[allow(clippy::unused_unsafe)] if you want to keep the code forward-compatible with Rust 2024’s set_var signature change.
Suggested change
| unsafe { | |
| std::env::set_var("PATH", new_path); | |
| } | |
| Self { original } | |
| } | |
| } | |
| impl Drop for PathGuard { | |
| fn drop(&mut self) { | |
| unsafe { | |
| match &self.original { | |
| Some(p) => std::env::set_var("PATH", p), | |
| None => std::env::remove_var("PATH"), | |
| } | |
| std::env::set_var("PATH", new_path); | |
| Self { original } | |
| } | |
| } | |
| impl Drop for PathGuard { | |
| fn drop(&mut self) { | |
| match &self.original { | |
| Some(p) => std::env::set_var("PATH", p), | |
| None => std::env::remove_var("PATH"), |
Comment on lines
+2010
to
+2014
| unsafe { | ||
| match &self.original { | ||
| Some(p) => std::env::set_var("PATH", p), | ||
| None => std::env::remove_var("PATH"), | ||
| } |
There was a problem hiding this comment.
Same unused_unsafe issue in Drop for PathGuard: the unsafe { ... } block currently contains only safe calls in edition 2021 and is likely to fail -D warnings in CI. Remove the unsafe or suppress the lint in a narrowly-scoped way.
Suggested change
| unsafe { | |
| match &self.original { | |
| Some(p) => std::env::set_var("PATH", p), | |
| None => std::env::remove_var("PATH"), | |
| } | |
| match &self.original { | |
| Some(p) => std::env::set_var("PATH", p), | |
| None => std::env::remove_var("PATH"), |
This was referenced Apr 25, 2026
Owner
Author
|
Codex integration sweep note: I reviewed the inline comments on this PR. GitHub rejected direct inline replies for these older/outdated review comments with HTTP 422, so I am responding top-level instead: 3139403790, 3139403820, 3139403838, 3139403851, 3139403866, 3139403882, 3141251479, 3141251485, 3141251494, 3141251502, 3141251509.\n\nI did not edit this branch. Items that overlap the secure/fnox/host-agent/digest integration work are addressed in draft PR #38 (codex-integration-code), including stdin-based fnox set, bounded fnox waits, whitespace-safe !secure parsing, identity-aware !secure audit logs, valid-input host-agent properties, real WhatsApp HMAC verification, loopback OneCLI default bind, and race-free digest temp paths. Remaining PR-specific findings stay actionable for this branch owner or a follow-up. |
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
Cross-cutting triage finding (PRs #20, #21, #23, #26, #28, #31): the !secure parser used `splitn(' ')` which mis-shapes multi-space input — `"!secure set NAME=v"` parsed as sub="" rest="set NAME=v", silently routing through the unknown-subcommand path with the secret in the help-error message. Switch to `split_whitespace()`. Also wires the previously-unused `identity_id` into an audit log so the `_` prefix can drop. The log records identity + subcommand, never the value (`rest`). Closes the "audit-claim docstring with no audit implementation" finding from triage.
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
…ning (#44) Squash-merge of integration/super-combined — 4 weeks of feature work + cross-PR security fixes + codex agent's hardening, all green CI (14/14 checks). ## Features landing - **fnox secret-resolver integration** (#15) + FnoxClient subprocess wrapper (#21) - **Adversarial commit-reviewer + mechanical pre-commit gate** (#18) - **{{secret:NAME}} substitution engine** in security-proxy URL/headers/body (#19) - **Per-secret destination allowlist** (#22) — RFC §11.1 attack defense - **!secure chat commands** (set/list) on Telegram (#20), Matrix (#28), WhatsApp (#31) - **zeroclawed-mcp** scaffold — agent-facing secret discovery server (#23) - **install.sh wires MCP** into Claude Code agent configs (#26) - **zeroclawed-secret-paste** — localhost web UI for one-shot secret input (#34) - **Bulk paste UI** — .env-style multi-secret onboarding with per-line results - **LAN-friendly defaults** — bind 0.0.0.0 + RFC 1918 Origin acceptance - **WhatsApp HMAC verification** (was always-true placeholder before — codex hardening) ## Security fixes folded in - /vault/:secret bearer auth + 127.0.0.1 default bind (#39) - URL-embedded secrets honor destination allowlist (#41) - Paste-flow: bearer URL only at debug, fnox set via stdin not argv (#40) - Paste-flow: graceful shutdown, exit-on-submit, reject Origin: null (#43) - Subprocess timeouts + kill_on_drop on FnoxClient - BrokenPipe-tolerant stdin write (Linux CI surface) - Header-value log redaction - OneCLI bound to 127.0.0.1 by default - Sanitized real API token + Telegram IDs from sample configs (#36) ## Architecture / refactors - Consolidated onecli binary into security-proxy (#17) - Hardcoded vault URL removed from onecli-client - security-proxy resolver wired into hot path - Extracted build_app router; migrated /vault/:secret route - !secure parser uses split_whitespace (was splitn), audit-logs invocations ## Test coverage added - security-proxy substitution engine + body/headers tests - onecli-client retry + Http(_) variant + adversarial fallthrough suite - onecli-client client.rs rewritten from tautologies to wiremock-backed - config/validator coverage (was zero, now 290-line module covered) - 16 zeroclawed-secret-paste tests including bulk-mode cases ## Docs / RFCs - agent-secret-gateway holistic architecture - consolidation-findings (what #28 must address) - secret-input-web-ui RFC (input-only, new-by-default) - browser-harness integration spike - test-quality-audit Round 1+2+3 (host-agent + zeroclawed priority files) ## Codex agent's hardening cherry-picks - Subprocess timeouts on fnox calls - map_spawn_error helper - Validator hardening + atomic-counter digest race fix - WhatsApp HMAC implementation + tests - proxy header-value log redaction CI: all 14 checks green at squash time. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Owner
Author
|
Subsumed by #44 (squashed to |
bglusman
added a commit
that referenced
this pull request
Apr 25, 2026
…ning (#44) Squash-merge of integration/super-combined — 4 weeks of feature work + cross-PR security fixes + codex agent's hardening, all green CI (14/14 checks). ## Features landing - **fnox secret-resolver integration** (#15) + FnoxClient subprocess wrapper (#21) - **Adversarial commit-reviewer + mechanical pre-commit gate** (#18) - **{{secret:NAME}} substitution engine** in security-proxy URL/headers/body (#19) - **Per-secret destination allowlist** (#22) — RFC §11.1 attack defense - **!secure chat commands** (set/list) on Telegram (#20), Matrix (#28), WhatsApp (#31) - **zeroclawed-mcp** scaffold — agent-facing secret discovery server (#23) - **install.sh wires MCP** into Claude Code agent configs (#26) - **zeroclawed-secret-paste** — localhost web UI for one-shot secret input (#34) - **Bulk paste UI** — .env-style multi-secret onboarding with per-line results - **LAN-friendly defaults** — bind 0.0.0.0 + RFC 1918 Origin acceptance - **WhatsApp HMAC verification** (was always-true placeholder before — codex hardening) ## Security fixes folded in - /vault/:secret bearer auth + 127.0.0.1 default bind (#39) - URL-embedded secrets honor destination allowlist (#41) - Paste-flow: bearer URL only at debug, fnox set via stdin not argv (#40) - Paste-flow: graceful shutdown, exit-on-submit, reject Origin: null (#43) - Subprocess timeouts + kill_on_drop on FnoxClient - BrokenPipe-tolerant stdin write (Linux CI surface) - Header-value log redaction - OneCLI bound to 127.0.0.1 by default - Sanitized real API token + Telegram IDs from sample configs (#36) ## Architecture / refactors - Consolidated onecli binary into security-proxy (#17) - Hardcoded vault URL removed from onecli-client - security-proxy resolver wired into hot path - Extracted build_app router; migrated /vault/:secret route - !secure parser uses split_whitespace (was splitn), audit-logs invocations ## Test coverage added - security-proxy substitution engine + body/headers tests - onecli-client retry + Http(_) variant + adversarial fallthrough suite - onecli-client client.rs rewritten from tautologies to wiremock-backed - config/validator coverage (was zero, now 290-line module covered) - 16 zeroclawed-secret-paste tests including bulk-mode cases ## Docs / RFCs - agent-secret-gateway holistic architecture - consolidation-findings (what #28 must address) - secret-input-web-ui RFC (input-only, new-by-default) - browser-harness integration spike - test-quality-audit Round 1+2+3 (host-agent + zeroclawed priority files) ## Codex agent's hardening cherry-picks - Subprocess timeouts on fnox calls - map_spawn_error helper - Validator hardening + atomic-counter digest race fix - WhatsApp HMAC implementation + tests - proxy header-value log redaction CI: all 14 checks green at squash time. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
bglusman
added a commit
that referenced
this pull request
Apr 26, 2026
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.
bglusman
added a commit
that referenced
this pull request
Apr 27, 2026
…de (#56) * chore(.github): add copilot-instructions.md to tune PR-review behavior 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. * chore(.github): tighten copilot-instructions + add path-scoped Rust file 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. * chore(.github): restore AGENTS.md + CLAUDE.md cross-refs in copilot instructions 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. * docs: split AGENTS.md into workspace-wide root + host-agent crate file 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. * fix(.github): correct gitleaks allowlist description in copilot-instructions 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
!secureas a post-auth chat command dispatched before anyagent sees the message. Subcommands:
`Stored `NAME`` + a retention warning. Never echoes the value.
fnox's output defensively; extra columns are stripped before
replying.
Wired into Telegram (both `handle_message` paths) and excluded from
the unknown-command pre-auth filter. Matrix/WhatsApp wiring is a
follow-up (same pattern, separate verification surface).
Redaction discipline
doesn't carry the value.
(the text contains the value).
engine's accepted ref-name syntax so stored secrets can be
immediately used as `{{secret:NAME}}`.
Tests
5 given/when/then in `commands::tests::secure_*`:
All use a fake-fnox shell script installed on `PATH` via `TempDir` —
hermetic, works whether or not real fnox is installed.
Known tradeoff
Subprocess approach means three call-sites now shell out to fnox
(`vault.rs::get_secret_from_fnox`, `secure_set`, `secure_list`) —
three availability checks. Follow-up PR migrates all three to the
fnox library crate (`fnox = { git = "https://github.com/jdx/fnox" }`
— lib.rs already public, no upstream PR needed).
Non-goals (follow-ups)
`docs/rfcs/agent-secret-gateway.md` §5.
🤖 Generated with Claude Code