feat(security): add tamper-evident audit chain logger#892
feat(security): add tamper-evident audit chain logger#892gemini2026 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a tamper-evident, append-only audit logging module with SHA-256 hash chaining persistence via JSONL format. Includes a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
docs/reference/audit-chain.md (3)
39-40: One sentence per line.Lines 39–40 contain two sentences on the same line.
Split them for cleaner diffs.The `chain_id` is a random 24-character hex string generated when the logger creates a new file. It remains constant across all entries in a single chain and persists when the logger resumes from an existing file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/audit-chain.md` around lines 39 - 40, The line describing `chain_id` currently contains two sentences on the same line—split it into two separate lines so each sentence is its own line: one line stating "The `chain_id` is a random 24-character hex string generated when the logger creates a new file." and the next line stating "It remains constant across all entries in a single chain and persists when the logger resumes from an existing file." so diffs are clean.
85-86: One sentence per line.Multiple sentences appear on a single line here.
Split each sentence onto its own line for readable diffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/audit-chain.md` around lines 85 - 86, The documentation line describing the constructor currently contains multiple sentences on one line; split each sentence into its own line so diffs are readable—e.g., separate "The constructor creates parent directories if they do not exist." and "If the file already contains entries, the logger resumes the chain from the last entry." into two lines under the constructor documentation.
113-114: One sentence per line.Lines 113–114 and 119–120 each have two sentences on the same line.
Split them for cleaner diffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/audit-chain.md` around lines 113 - 114, Split the two combined-sentence lines so each sentence is on its own line: change the line that currently contains "When `limit` is omitted or zero, all matching entries are returned. Malformed lines are silently skipped." into two lines ("When `limit` is omitted or zero, all matching entries are returned." and "Malformed lines are silently skipped.") and do the same for the other occurrence around lines 119–120; ensure each sentence is a separate line to keep diffs clean.nemoclaw/src/security/audit-chain.test.ts (1)
558-578: Test name "concurrent writes" is misleading.This test performs rapid sequential writes from a single thread, not concurrent writes from multiple threads.
SinceappendFileSyncis synchronous, the writes are serialized.
Consider renaming to"rapid sequential writes"or similar to accurately reflect what's being tested.True concurrency testing would require multiple worker threads writing simultaneously.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/audit-chain.test.ts` around lines 558 - 578, The test title "concurrent writes" is misleading because it performs serialized writes using AuditLogger (which uses appendFileSync); rename the describe or it block to accurately reflect the behavior (e.g., change describe("concurrent writes") or it("handles rapid sequential writes without data loss") to "rapid sequential writes" or similar) so the test name matches the implementation, referencing the AuditLogger, appendFileSync, readEntries, and verifyChain symbols to locate the test to update.nemoclaw/src/security/audit-chain.ts (2)
346-350: Consider a structured logging approach instead ofconsole.error.Using
console.errorduringreadTailStatecan produce unexpected output in production or library consumers.
Options:
- Return a diagnostic in the
TailStateresult for the caller to handle.- Accept an optional logger callback.
- Silently skip (current behavior minus the log).
Given this is a security module where malformed lines could indicate tampering, silent skipping may be appropriate since
verifyChainhandles detection separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/audit-chain.ts` around lines 346 - 350, The console.error in readTailState should be removed and replaced with a structured diagnostic approach: update the readTailState signature to either accept an optional logger callback (e.g., logger?: (level: string, msg: string, meta?: any) => void) or add a diagnostic field on the returned TailState (e.g., diagnostics: Array<{line: string, error: string}>), then use that mechanism instead of console.error when skipping a malformed line; ensure callers (or verifyChain) can inspect diagnostics if needed and that default behavior remains silent if no logger is provided.
110-113: No file locking for multi-process writes.
appendFileSyncprovides atomicity at the OS level for small writes, but concurrent appends from multiple processes can interleave or corrupt entries on some platforms.If multi-process logging is a future requirement, consider:
- Advisory file locking (
flockequivalent)- A separate log aggregator process
- A database-backed alternative
For single-process use as currently tested, this is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/audit-chain.ts` around lines 110 - 113, The current append in AuditChain.write (appendFileSync(this.path, ...)) can interleave or corrupt when multiple processes write; replace the direct append with a multi-process-safe approach such as acquiring an advisory lock before writing (e.g., flock via a native binding or fs-ext's flockSync), or implement a dedicated log-writer/aggregator process or DB-backed store; ensure the code acquires the lock on this.path (or on a separate .lock file), performs the append of JSON.stringify(entry)+"\n", updates this.seq and this.prevHash only after a successful write, and always releases the lock even on error.docs/reference/injection-scanner.md (1)
23-25: One sentence per line.Lines 23-24 contain multiple sentences on single lines. Per coding guidelines, each sentence should be on its own line to make diffs readable.
The injection scanner detects prompt injection patterns in agent tool inputs and outputs. -It applies text normalization and pattern matching to identify attempts to override system prompts, inject instructions, manipulate tools, or exfiltrate data. +It applies text normalization and pattern matching to identify attempts to override system prompts, inject instructions, manipulate tools, or exfiltrate data.(The second sentence is already on its own line, but verify the source has one sentence per line throughout.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/injection-scanner.md` around lines 23 - 25, The two sentences in the README paragraph should be split so each sentence is on its own line: replace the single line containing "The injection scanner detects prompt injection patterns in agent tool inputs and outputs. It applies text normalization and pattern matching to identify attempts to override system prompts, inject instructions, manipulate tools, or exfiltrate data." with two lines — one containing "The injection scanner detects prompt injection patterns in agent tool inputs and outputs." and a second containing "It applies text normalization and pattern matching to identify attempts to override system prompts, inject instructions, manipulate tools, or exfiltrate data."; also scan the rest of the document to ensure every sentence follows the one-sentence-per-line guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/injection-scanner.md`:
- Around line 113-115: The documentation for maxSeverity incorrectly states its
return type as `Severity | ""`; update the doc to match the implementation and
tests by changing the return type to `Severity | null` and the description to
say it returns null for an empty findings array; reference the maxSeverity
function and the existing test that asserts null to ensure consistency.
---
Nitpick comments:
In `@docs/reference/audit-chain.md`:
- Around line 39-40: The line describing `chain_id` currently contains two
sentences on the same line—split it into two separate lines so each sentence is
its own line: one line stating "The `chain_id` is a random 24-character hex
string generated when the logger creates a new file." and the next line stating
"It remains constant across all entries in a single chain and persists when the
logger resumes from an existing file." so diffs are clean.
- Around line 85-86: The documentation line describing the constructor currently
contains multiple sentences on one line; split each sentence into its own line
so diffs are readable—e.g., separate "The constructor creates parent directories
if they do not exist." and "If the file already contains entries, the logger
resumes the chain from the last entry." into two lines under the constructor
documentation.
- Around line 113-114: Split the two combined-sentence lines so each sentence is
on its own line: change the line that currently contains "When `limit` is
omitted or zero, all matching entries are returned. Malformed lines are silently
skipped." into two lines ("When `limit` is omitted or zero, all matching entries
are returned." and "Malformed lines are silently skipped.") and do the same for
the other occurrence around lines 119–120; ensure each sentence is a separate
line to keep diffs clean.
In `@docs/reference/injection-scanner.md`:
- Around line 23-25: The two sentences in the README paragraph should be split
so each sentence is on its own line: replace the single line containing "The
injection scanner detects prompt injection patterns in agent tool inputs and
outputs. It applies text normalization and pattern matching to identify attempts
to override system prompts, inject instructions, manipulate tools, or exfiltrate
data." with two lines — one containing "The injection scanner detects prompt
injection patterns in agent tool inputs and outputs." and a second containing
"It applies text normalization and pattern matching to identify attempts to
override system prompts, inject instructions, manipulate tools, or exfiltrate
data."; also scan the rest of the document to ensure every sentence follows the
one-sentence-per-line guideline.
In `@nemoclaw/src/security/audit-chain.test.ts`:
- Around line 558-578: The test title "concurrent writes" is misleading because
it performs serialized writes using AuditLogger (which uses appendFileSync);
rename the describe or it block to accurately reflect the behavior (e.g., change
describe("concurrent writes") or it("handles rapid sequential writes without
data loss") to "rapid sequential writes" or similar) so the test name matches
the implementation, referencing the AuditLogger, appendFileSync, readEntries,
and verifyChain symbols to locate the test to update.
In `@nemoclaw/src/security/audit-chain.ts`:
- Around line 346-350: The console.error in readTailState should be removed and
replaced with a structured diagnostic approach: update the readTailState
signature to either accept an optional logger callback (e.g., logger?: (level:
string, msg: string, meta?: any) => void) or add a diagnostic field on the
returned TailState (e.g., diagnostics: Array<{line: string, error: string}>),
then use that mechanism instead of console.error when skipping a malformed line;
ensure callers (or verifyChain) can inspect diagnostics if needed and that
default behavior remains silent if no logger is provided.
- Around line 110-113: The current append in AuditChain.write
(appendFileSync(this.path, ...)) can interleave or corrupt when multiple
processes write; replace the direct append with a multi-process-safe approach
such as acquiring an advisory lock before writing (e.g., flock via a native
binding or fs-ext's flockSync), or implement a dedicated log-writer/aggregator
process or DB-backed store; ensure the code acquires the lock on this.path (or
on a separate .lock file), performs the append of JSON.stringify(entry)+"\n",
updates this.seq and this.prevHash only after a successful write, and always
releases the lock even on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1657469-f7fc-48b6-b329-fb8a4cbfad82
⛔ Files ignored due to path filters (2)
nemoclaw/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/pr.yamlDockerfiledocs/reference/audit-chain.mddocs/reference/injection-scanner.mdinstall.shnemoclaw/src/security/audit-chain.test.tsnemoclaw/src/security/audit-chain.tsnemoclaw/src/security/injection-scanner.test.tsnemoclaw/src/security/injection-scanner.ts
💤 Files with no reviewable changes (1)
- .github/workflows/pr.yaml
Dockerfile
Outdated
| RUN curl -fsSL -o /usr/local/bin/gosu "https://github.com/tianon/gosu/releases/download/1.19/gosu-amd64" \ | ||
| && echo "52c8749d0142edd234e9d6bd5237dff2d81e71f43537e2f4f66f75dd4b243dd0 /usr/local/bin/gosu" | sha256sum -c - \ | ||
| && chmod +x /usr/local/bin/gosu \ | ||
| && gosu --version |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat DockerfileRepository: NVIDIA/NemoClaw
Length of output: 9506
🏁 Script executed:
# Check what gosu releases are available for version 1.19
web_search
gosu 1.19 releases github tianon available architecturesRepository: NVIDIA/NemoClaw
Length of output: 152
🌐 Web query:
gosu 1.19 github releases available architectures arm64
💡 Result:
Yes, gosu version 1.19 on GitHub (tianon/gosu releases) provides binaries for multiple architectures, including arm64. The release has 22 assets, which include architecture-specific binaries like gosu-amd64 and corresponding ones for other arches such as arm64, armhf, ppc64le, s390x, riscv64, mips64el, and more, based on the standard Debian dpkg architecture naming convention used in the project (e.g., gosu-arm64). This is confirmed by the release page listing assets and the project's build process documented in Debian sources, which explicitly builds for arm64 (GOARCH=arm64) among others. Wolfi also provides gosu-1.19-r0.apk for aarch64.
Citations:
- 1: https://github.com/tianon/gosu/releases
- 2: https://sources.debian.org/src/gosu/1.19-1/Dockerfile
- 3: https://wolfi.pkgs.org/rolling/wolfi-base-aarch64/gosu-1.19-r0.apk.html
- 4: https://packages.debian.org/sid/admin/gosu
Hardcoded amd64 gosu binary breaks arm64 builds.
Lines 29–32 unconditionally download gosu-amd64. When the build runs on an arm64 builder or with --platform=linux/arm64, line 32 will fail with "exec format error" trying to execute an incompatible binary. gosu 1.19 provides arm64 binaries (gosu-arm64). Restore architecture-aware selection to download the correct binary for the build architecture, or explicitly pin both FROM stages to --platform=linux/amd64 if arm64 support is intentionally dropped.
docs/reference/injection-scanner.md
Outdated
| ### `maxSeverity(findings: Finding[]): Severity | ""` | ||
|
|
||
| The highest severity level present in the findings array, or an empty string if the array is empty. |
There was a problem hiding this comment.
Documentation-code mismatch: maxSeverity returns null, not "".
The documented return type Severity | "" does not match the implementation, which returns Severity | null. The test at injection-scanner.test.ts:623-625 confirms maxSeverity([]) returns null via .toBeNull(). Callers following this documentation would incorrectly check for an empty string instead of null.
📝 Proposed fix
-### `maxSeverity(findings: Finding[]): Severity | ""`
+### `maxSeverity(findings: Finding[]): Severity | null`
-The highest severity level present in the findings array, or an empty string if the array is empty.
+The highest severity level present in the findings array, or `null` if the array is empty.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### `maxSeverity(findings: Finding[]): Severity | ""` | |
| The highest severity level present in the findings array, or an empty string if the array is empty. | |
| ### `maxSeverity(findings: Finding[]): Severity | null` | |
| The highest severity level present in the findings array, or `null` if the array is empty. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/injection-scanner.md` around lines 113 - 115, The
documentation for maxSeverity incorrectly states its return type as `Severity |
""`; update the doc to match the implementation and tests by changing the return
type to `Severity | null` and the description to say it returns null for an
empty findings array; reference the maxSeverity function and the existing test
that asserts null to ensure consistency.
SHA-256 hash-chained JSONL audit logger with verify, export, and tail utilities. Each entry includes the hash of the previous entry, making any tampering detectable via the verifyChain function. Includes sequence continuity and chain_id consistency checks to detect block-deletion and splice attacks. Graceful handling of malformed JSONL lines in all read paths.
gemini2026
force-pushed
the
feat/audit-chain
branch
from
66b520e to
f68e97c
Compare
1 participant
Closes #891
Summary
Adds a tamper-evident, hash-chained JSONL audit logger to NemoClaw's security toolkit.
Hash chain:
Entry structure:
seq— monotonic sequence numberchain_id— 24-char random hex identifier unique per audit instanceprev_hash— hash of previous entry (empty for first)entry_hash—sha256:<64 hex chars>computed from canonical JSON payloadtype— event type stringtime— ISO 8601 UTC timestampdata— arbitrary JSON payloadAPI:
AuditLoggerclass withlog(type, data)methodverifyChain(path)— validates entire hash chain, detects any tampering or gapsexportEntries(path, since, limit?)— export entries by sequence number rangetailEntries(path, n?)— return last N entries (defaults to 50)Hardening:
readonlyon all output interface fieldsappendFileSyncFull Vitest test coverage (35 tests) including tamper detection, error paths, boundary conditions, and rapid-write stress test.
Self-contained under
nemoclaw/src/security/with no dependencies on existing NemoClaw internals.Reference documentation at
docs/reference/audit-chain.md.Test plan
cd nemoclaw && npx vitest run src/security/audit-chain.test.ts— 35/35 tests passnpm test— all tests pass, 0 regressionstsc --noEmit— clean type checkmake check— all linters and hooks passSummary by CodeRabbit
New Features
Documentation
Tests