Skip to content

feat: /add-audit skill — opt-in local audit log for the ncl surface#2987

Closed
moshe-nanoco wants to merge 1 commit into
feat/guard-seamfrom
feat/audit-ncl-skill
Closed

feat: /add-audit skill — opt-in local audit log for the ncl surface#2987
moshe-nanoco wants to merge 1 commit into
feat/guard-seamfrom
feat/audit-ncl-skill

Conversation

@moshe-nanoco

@moshe-nanoco moshe-nanoco commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Updated — rebased onto the latest feat/guard-seam (tip c144587) and hardened per a max-effort review.
The base advanced again (a docs(guard) comment-only commit — no functional delta); this PR was rebased onto the new tip with no audit runtime change from that rebase. Separately, a max-effort code review drove one design change and a handful of fixes, folded into the single commit:

  • Value-free recording model (replaces the key-pattern redactor outright — the redactor file is deleted). The event now records WHO did WHICH action to WHAT target and the outcome; details carries only the flag names that were passed plus a small allowlist of governance enum values. No free-form argument value is ever written, so a secret can't leak from a value that was never stored.
  • emit-on-throw — the middleware brackets the dispatcher, so a command that throws still leaves a failure record (then re-throws).
  • An approved replay records outcome approved (was success); rejected is deferred to the approval-lifecycle increment.
  • --help probes record under a neutral cli.help action instead of masquerading as the real verb succeeding.
  • --limit 0 is honored as zero rows; --since/--until parse as UTC; --format ndjson streams the full window (no silent 100-row cap).
  • Exporter init() seam hardened — a hook whose module loads after boot still runs its one-time init(). The seam was import-order-sensitive (onEvent/maintain/shutdown already read the live list; init() did not), so late registration now self-heals. No live impact today — zero hooks ship — but it closes the trap the advertised registerAuditHook seam would set for the first exporter.

Re-verified green in both states (counts below).

What

The local audit log as an install skill (/add-audit), scoped to the ncl command surface: every dispatch outcome on both transports — host socket and container — including scope denials, approval holds, and approved replays, recorded as SIEM-shaped, append-only NDJSON day-files under data/audit/<UTC-day>.ndjson. Read back with ncl audit list (filters + --format ndjson); in-process exporters plug in via registerAuditHook. Off by default — AUDIT_ENABLED=true is the master switch; nothing is persisted (and data/audit/ is never created) until an operator opts in.

Stacked on feat/guard-seam — the diff is one commit, 19 files, all additive: 18 under .claude/skills/add-audit/ plus one CHANGELOG bullet. Trunk code is untouched — audit is not core NanoClaw; it ships as a skill (per the audit-implementations review verdict, hub: engineering/reviews/audit-log-implementations).

Why

"Who changed this group's config, when, and was it approved?" has no durable answer today: logs/nanoclaw.log is free text interleaved with routing noise, and pending_approvals rows are deleted once decided — working state, not history. Design refs on the hub: engineering/requirements/audit-log and engineering/discovery/audit-log-decisions (authoritative for every design choice here).

How it works

  • The skill copies src/audit/ — a domain-free leaf: event schema (schema_version: 1, projects onto OCSF/ECS), env config, day-file store (appendFileSync, host single-writer, bucketed by event day), fail-open + loud emit, post-write hooks, newest-first reader, boot/maintenance wiring — plus one CLI adapter (src/cli/dispatch.audit.ts) and the read-only ncl audit resource.
  • Recording model — value-free. One canonical event per command carries actor (host:<os-user> or the agent group), origin (transport, session, channel), the dotted action from the guard catalog (e.g. groups.config.add-mcp-server), touched/attempted resources, outcome (success · failure · denied · pending · approved), correlation_id, and details. details never holds raw argument values — it carries the sorted list of flag names that were passed, plus a small allowlist of governance-relevant enum values (role, mode, session_mode, cli_scope, access, engage_mode, sender_scope, provider, model) echoed verbatim when present. A failure keeps the error code, never the free-text message. Target identifiers (ids, users, groups) surface structurally in resources, not in details. There is no value redactor: a secret in an error string, a malformed-JSON arg, or an unanticipated key can't land, because no free-form value is ever written.
  • Exactly two core reach-ins, applied at install and guarded by a shipped wiring test (TS-AST + registration behavior + conformance + emit-invariant sweep):
    1. src/cli/dispatch.ts: export const dispatch = withAudit(dispatchInner) at the definition site — one composition covers the socket server, the container cli_request path, and the in-module approved-replay call.
    2. src/cli/resources/index.ts: import './audit.js';
  • Correlation is the approval id: an approved replay carries the approval row as its guard grant (opts.grant.approval_id), and a fresh hold is correlated by recovering the row it just created (payload frame.id match) — so --correlation <approval-id> returns the whole gated chain (pending → terminal).
  • audit is deliberately not in GROUP_SCOPE_RESOURCES: group-scoped agents fail closed; host + cli_scope: global callers only. A disabled box answers ncl audit list with audit log is disabled — set AUDIT_ENABLED=true instead of an empty list, and an enabled box refuses to boot if data/audit/ isn't writable.
  • Guard conformance stays clean: registerResource auto-derives the audit.list catalog entry (access: 'open' — it never holds, so it cannot introduce an approve-continuation gap), verified by the shipped wiring test that mirrors guard's CI conformance check.

Deviations from the decisions doc (ncl-only v1 scope)

  1. Pending events are emitted by the dispatch middleware — the approval-pending response is this increment's record of a hold. The decisions doc assigns pending events to a requestApproval decorator; that machinery belongs to the approval-lifecycle increment (depends on the approval-contract branch) and attaches later as pure adds (new *.audit.ts adapters + observer subscriptions — src/audit/ needs zero rework). Consequence: the pending event does not name the picked approver yet, and terminal approved/rejected decision events are deferred to that increment (this PR records the approved replay as approved).
  2. Env vars live in src/audit/config.ts (still read via readEnvFile), not core config.ts — one fewer core edit for the skill to apply.
  3. Retention prune is self-contained — boot prune + an hourly unref'd timer over the once-per-UTC-day throttle — instead of a host-sweep hook. Likewise the boot writability assert fires at the adapter's module load (still before the host accepts any work), so no index.ts edit.
  4. Action names are the guard catalog's (commandGuardAction, fallback cli.<name>), not the doc's older spellings; a --help probe records under the neutral cli.help action rather than the real verb.
  5. Stricter than the doc on privacy. The decisions doc specifies a value redactor; this increment goes further and records no free-form argument values at all (the value-free model above) — a strict superset of the doc's intent. The requirements' "denied worked example" is still satisfied: a cross-group denial records the action, the attempted target as a resource, and denied — the foreign id the caller aimed at is still visible, structurally, without echoing any raw arg value.

How tested

  • 833/833 tests green on an applied checkout (773 base + 60 shipped with the skill, 84 files); 773/773 unapplied (79 files); pnpm run build green in both states; zero changes under container/.
  • The shipped src/cli/dispatch.audit.test.ts drives the real wrapped dispatch through the real registry and real guard (audit force-enabled, the store's append captured) — asserting the recorded event for every outcome: host success (socket origin), agent success after group auto-fill, scope denial (attempted-resource type recorded, value-free details), handler failure (error code only), a hold recorded as pending correlated to the approval row it created, an uncorrelated hold (no approver), a full approved replay recorded as approved sharing the grant's approval id, a --help probe recorded as cli.help, and an emit-on-throw case (a thrown handler still leaves a failure record). The value-free guarantee is asserted in the stored bytes: details.args holds only flag names; the allowlist echoes an enum value (role: 'admin') while the accompanying --user value is absent from details and surfaces only as a resource; and a secret passed as an arg value never appears in the written line.
  • The src/audit-wiring.test.ts AST check pins the reach-in shape (export const dispatch = withAudit(dispatchInner), inner kept unexported), the resource's absence from the group-scope allowlist, guard conformance staying clean with the resource registered, and the emit-invariant sweep (emitAuditEvent only in src/audit/ and *.audit.ts).
  • The pre-rework branch was additionally live-smoked on a booted host (host-socket + agent + denied + full hold→approve→replay cycle grep-able from the day-file, every ncl audit list filter, disabled-box error, group-scoped refusal). Those runtime plumbing paths — transports, correlation, filters, boot/disabled behavior — are unchanged by the rebase and the value-free rework; the recording-model change itself is covered by the shipped unit suite above (which asserts the stored bytes are value-free) rather than re-derived from the older smoke.

Usage

Run /add-audit (copies files, applies the two edits, enables AUDIT_ENABLED=true + AUDIT_RETENTION_DAYS=90, builds, restarts). Then:

ncl audit list --outcome denied --since 7d
ncl audit list --correlation <approval-id>
ncl audit list --action groups.config --format ndjson

Removal: .claude/skills/add-audit/REMOVE.md reverses every change (day-files are left in place — they're the operator's records).

🤖 Generated with Claude Code

@moshe-nanoco moshe-nanoco added the PR: Skill Skill package or skill-related changes label Jul 9, 2026
@github-actions github-actions Bot added the core-team PR opened by a core team member label Jul 9, 2026
@moshe-nanoco moshe-nanoco force-pushed the feat/audit-ncl-skill branch from 22c597c to 8bf9684 Compare July 9, 2026 11:26
@moshe-nanoco moshe-nanoco marked this pull request as draft July 9, 2026 13:15
@moshe-nanoco moshe-nanoco force-pushed the feat/audit-ncl-skill branch from 8bf9684 to 2c5dcaf Compare July 9, 2026 13:34
@moshe-nanoco moshe-nanoco force-pushed the feat/audit-ncl-skill branch from 2c5dcaf to 8bf9684 Compare July 9, 2026 15:40
@moshe-nanoco moshe-nanoco force-pushed the feat/audit-ncl-skill branch from 8bf9684 to b56f9c5 Compare July 11, 2026 21:53
@moshe-nanoco moshe-nanoco force-pushed the feat/audit-ncl-skill branch from b56f9c5 to 84dd32b Compare July 12, 2026 11:29
Ships the local audit log as an install skill, scoped to the ncl command
surface: one withAudit(dispatch) composition at the definition site covers
both transports and the grant-carrying approved replay; holds are recorded as
pending events correlated by approval id (recovered from the hold row), so
--correlation returns the whole gated chain. src/audit/ installs as a
domain-free leaf — schema v1, NDJSON day-files under data/audit/ (bucketed by
event time), fail-open + loud emit that brackets the dispatcher so a thrown
command still leaves a record, post-write exporter hooks, boot writability
refusal, self-contained retention prune. `ncl audit list` is host +
global-scope only (off the group-scope allowlist, fails closed), validates its
filter flags (a typo is rejected, not silently ignored), reads --since/--until
as UTC, and streams the full window on --format ndjson (no silent 100-row cap).
Core reach-ins: the dispatch composition and the resource-barrel import, both
guarded by a shipped AST/behavior wiring test that also pins guard conformance
clean.

Value-free recording model (hardened per the max-effort review): the event
stores WHO did WHICH action to WHAT target and the outcome — never raw
argument values. `details` carry the flag names that were passed plus a small
allowlist of governance-relevant enum values (role, mode, session_mode,
cli_scope, access, engage_mode, sender_scope, provider, model) echoed
verbatim; every other value is name-only, and a failure keeps the error CODE,
never the free-text message. This replaces the key-pattern redactor outright —
a secret in an error message, a malformed-JSON arg, or an innocent key can no
longer land, because no free-form value is ever written. Target ids surface
structurally in resources.

Other review fixes: emit-on-throw; an approved replay records as `approved`
(rejected dropped until the approval-lifecycle increment); --help probes
record under a neutral cli.help action instead of masquerading as the real
verb succeeding; --limit 0 is honored as zero rows; and an exporter hook whose
module loads after boot still runs its one-time init() — the registration seam
is import-order-insensitive (onEvent/maintain/shutdown already read the live
list; init() now self-heals on late registration).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@moshe-nanoco

Copy link
Copy Markdown
Contributor Author

Obsolete — the base branch feat/guard-seam is merged to main, and the audit skill now ships in the enterprise repo (nanocoai/nanoclaw-ent#12). Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-team PR opened by a core team member PR: Skill Skill package or skill-related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant