docs(security): add audit log integrity to threat model by Benkapner · Pull Request #2010 · fullsend-ai/fullsend

Benkapner · 2026-06-08T09:55:56Z

The security architecture lists auditability as a cross-cutting principle: every agent action is logged, attributable, and reviewable. But the audit log itself is not protected against tampering.

The current findings.jsonl audit trail is append-only by convention, not by enforcement. In the sandbox model (ADR 0017), the agent has write access to the workspace, which includes the audit log directory. A compromised agent that cannot exfiltrate credentials (because credential isolation works as designed) can still cover its tracks by deleting entries that record suspicious activity, modifying findings to downgrade severity, inserting fabricated entries to create a false trail, or truncating the log to remove evidence of a compromise window.

This compounds with agent self-report unreliability: if an agent can both misrepresent its own actions and tamper with the log that would catch the misrepresentation, the combination defeats both controls.

This matters most for forensics. When investigating a security incident, the first question is "what did the agent actually do?" If the answer depends on a log the agent could have tampered with, the investigation starts from zero.

What this PR does

1. Threat model addition (docs/problems/security-threat-model.md): Adds a new "Cross-cutting concern: audit log integrity" section with four defense options and their trade-offs:

Cryptographic hash chaining makes tampering detectable (each entry includes a hash of the previous entry; modifying or deleting any entry breaks the chain). Note: hash chaining alone does not detect tail truncation; detecting that requires an external record of the expected chain length or latest hash.
Write-once external sink makes tampering impossible (stream events to an append-only store the sandbox cannot modify)
Post-run harness verification checks integrity outside the sandbox before the run is considered complete
Signed entries use a key the agent does not control to sign each audit entry (the harness signs entries before writing, avoiding tension with ADR 0017's credential isolation)

2. Implementation (internal/security/trace.go): Adds SHA-256 hash chaining to TracedFinding and AppendFinding. Each JSONL entry now includes prev_hash and hash fields forming a chain. Modifying or deleting any entry breaks the chain from that point forward. Adds VerifyChain() for post-run integrity verification.

Backward compatible: entries without hash fields (from before this change) are skipped during verification. No changes needed to existing callers since AppendFinding computes the hashes internally.

3. Tests (internal/security/trace_test.go): Covers chain construction, tampering detection, entry deletion detection, and empty file handling.

Relates to #1685 (commit signing with gitsign). Audit log integrity is a prerequisite: if the process log cannot be trusted, signing the output commit provides provenance but not accountability. Hash-chained audit logs are a simpler first step that does not require external signing infrastructure.

Adds a cross-cutting concern section on audit log integrity, covering hash chaining, write-once sinks, post-run verification, and signed entries as defense options. References issue fullsend-ai#1685 (commit signing). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

fullsend-ai-review · 2026-06-08T10:05:34Z

Review

Findings

Medium

[logic-error] internal/security/trace.go:155 — VerifyChain skips legacy entries (no hash/prev_hash fields) but still counts them in Entries. Combined with run.go checking cv.Entries > 0, a file containing only legacy entries reports "Audit log integrity verified (N entries)" despite zero hashes being checked. Additionally, lastHash and VerifyChain handle interleaved legacy/chained entries inconsistently: lastHash reads only the last line (returning seedHash if it is legacy), while VerifyChain tracks prev from the last chained entry. This causes false-positive chain breaks when legacy and chained entries are interleaved.
Remediation: Add a Verified counter to ChainVerification that increments only when an entry's hash is actually validated. Use cv.Verified > 0 in run.go. Either forbid interleaving or make lastHash scan backward for the last chained entry.
[logic-error] internal/cli/run.go:975 — Race between host-side output scan and sandbox download clobbering the same file. scanOutputFiles writes hash-chained entries to runDir/security/findings.jsonl via AppendFinding. Then sandbox.Download copies the sandbox's .security/ directory into the same runDir/security/ path. If the sandbox produced its own findings.jsonl, the download overwrites the host-written chained file. VerifyChain then runs on whatever file remains, which may be the sandbox's (potentially tampered) version.
Remediation: Download sandbox findings to a separate path or merge them via AppendFinding after download.

Low

[audit-log-integrity-gap] internal/security/hooks — Multiple Python hooks inside the sandbox write directly to findings.jsonl using plain JSON without computing prev_hash/hash fields. These entries are treated as legacy and skipped by VerifyChain, meaning sandbox-originated findings have no chain integrity. Combined with the legacy-entry logic above, the verification step currently provides no tamper-evidence for sandbox-side findings. Note: hash chaining inside the sandbox would not add meaningful protection since a sandbox attacker already controls the workspace; the real fix is host-side merge and chaining.
Remediation: Have hooks write to a separate staging file and merge/chain them on the host side before verification runs.
[error-handling] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). While the current Finding struct fields cannot practically fail marshaling, this is a deviation from defensive coding practice for an integrity mechanism.
Remediation: Change computeHash to return (string, error) and propagate the marshal error.
[error-handling] internal/security/trace.go:63 — lastHash() returns seedHash on all error paths (file open failure, empty file, unmarshal error, empty hash field). Silently falling back to seedHash means a corrupted or truncated file will start a new chain segment without any indication. VerifyChain would catch the resulting chain break downstream, but the error origin is obscured.
Remediation: Change lastHash() to return (string, error) and propagate errors to callers.
[race-condition] internal/security/trace.go:87 — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. The existing caller in run.go calls sequentially, so this is not currently triggered.
Remediation: Document the concurrency limitation in the function's doc comment.
[fail-open] internal/cli/run.go:981 — Verification is guarded by os.Stat: if findings.jsonl does not exist, verification is silently skipped. An attacker who deletes the entire file avoids verification.
[tail-truncation] internal/security/trace.go:134 — VerifyChain validates internal chain consistency but cannot detect tail truncation. The threat model documents this as a known limitation, but the run.go message "Audit log integrity verified" slightly overstates the guarantee.
Remediation: Update the message to "chain consistency verified" rather than "integrity verified."
[misleading-label] PR title uses docs(security): prefix but includes 135 lines of production Go code and 221 lines of test code. Per COMMITS.md, docs type is for documentation-only changes. This should use feat(security): to appear in release notes under Features.
Remediation: Change PR title to feat(security): add audit log hash chaining for tamper detection.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. See also: docs/plans/universal-harness-access.md:1281 references AppendFinding file permissions without noting the expanded behavioral contract.
Remediation: Add a note that AppendFinding now includes hash chaining.

Labels: PR adds SHA-256 hash chaining to the security trace subsystem with harness integration.

Previous run

Review

Findings

Medium

[logic-error] internal/cli/run.go:988 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. ChainVerification lacks a field to distinguish hash-verified entries from legacy-skipped entries, so the success message is misleading after a migration where pre-existing entries have no hashes.
Remediation: Add a Verified (or HashChecked) counter to ChainVerification that increments only when an entry's hash is actually validated. Only print the "verified" message when cv.Verified > 0.
[logic-error] internal/cli/run.go:975 — Race between host-side output scan and sandbox download clobbering the same file. scanOutputFiles writes hash-chained entries to runDir/security/findings.jsonl via AppendFinding. Then sandbox.Download copies the sandbox's .security/ directory into the same runDir/security/ directory. If the sandbox wrote its own findings.jsonl, the download may overwrite the host-written chained file. VerifyChain then runs on whatever file remains.
Remediation: Write host-side and sandbox-side findings to separate files, or download sandbox findings before writing host-side entries.

Low

[audit-log-integrity-gap] internal/security/hooks — Multiple Python hooks inside the sandbox write directly to findings.jsonl using plain JSON without computing or appending prev_hash/hash fields. These entries are treated as legacy and skipped by VerifyChain, meaning sandbox-originated findings have no chain integrity. Combined with the legacy-entry logic error above, the verification step currently provides no tamper-evidence for sandbox-side findings. Note: hash chaining inside the sandbox would not add meaningful protection since a sandbox attacker already controls the workspace; the real fix is host-side merge and chaining.
Remediation: Have hooks write to a separate staging file and merge/chain them on the host side before verification runs.
[error-handling] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). While the current Finding struct fields cannot practically fail marshaling, this is a deviation from defensive coding practice for an integrity mechanism.
Remediation: Change computeHash to return (string, error) and propagate the marshal error.
[error-handling] internal/security/trace.go:63 — lastHash() returns seedHash on all error paths (file open failure, empty file, unmarshal error, empty hash field). Silently falling back to seedHash means a corrupted or truncated file will start a new chain segment without any indication. VerifyChain would catch the resulting chain break downstream, but the error origin is obscured.
Remediation: Change lastHash() to return (string, error) and propagate errors to callers.
[audit-log-integrity-bypass] internal/security/trace.go:34 — The hash chain uses a hardcoded, publicly known seed hash and SHA-256 without any keyed MAC. An attacker with write access can recompute the entire chain from scratch. The threat model doc acknowledges this as an open question and discusses HMAC/signed entries as future considerations.
[race-condition] internal/security/trace.go:87 — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. The existing caller in run.go calls sequentially, so this is not currently triggered.
Remediation: Document the concurrency limitation in the function's doc comment.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false silently, and lastHash returns seedHash. Additionally, scanner.Err() is not checked after the scan loop.
[tail-truncation] internal/security/trace.go:134 — VerifyChain validates internal chain consistency but cannot detect tail truncation. The threat model documents this as a known limitation, but the run.go message "Audit log integrity verified" slightly overstates the guarantee.
Remediation: Update the message to "chain consistency verified" rather than "integrity verified."
[fail-open] internal/cli/run.go:981 — Verification is guarded by os.Stat: if findings.jsonl does not exist, verification is silently skipped. An attacker who deletes the entire file avoids verification.
[misleading-label] PR title uses docs(security): prefix but includes 135 lines of production Go code and 221 lines of test code. Per COMMITS.md, docs type is for documentation-only changes. This should use feat(security): to appear in release notes under Features.
Remediation: Change PR title to feat(security): add audit log hash chaining for tamper detection.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. See also: docs/plans/universal-harness-access.md:1281 references AppendFinding file permissions without noting the expanded behavioral contract.
Remediation: Add a note that AppendFinding now includes hash chaining.

Previous run (2)

Review

Findings

Medium

[logic-error] internal/cli/run.go:988 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. ChainVerification lacks a field to distinguish hash-verified entries from legacy-skipped entries.
Remediation: Add a Verified (or HashChecked) counter to ChainVerification that increments only when an entry's hash is actually validated. Only print the "verified" message when cv.Verified > 0.
[logic-error] internal/cli/run.go:975 — Race between host-side output scan and sandbox download clobbering the same file. scanOutputFiles writes hash-chained entries to runDir/security/findings.jsonl via AppendFinding. Then sandbox.Download copies the sandbox's .security/ directory into the same runDir/security/ directory. If the sandbox wrote its own findings.jsonl, the download may overwrite the host-written chained file. VerifyChain then runs on whatever file remains.
Remediation: Write host-side and sandbox-side findings to separate files, or download sandbox findings before writing host-side entries.
[audit-log-integrity-gap] internal/security/hooks — Multiple Python hooks inside the sandbox write directly to findings.jsonl using plain JSON without computing or appending prev_hash/hash fields. These entries are treated as legacy and skipped by VerifyChain, meaning sandbox-originated findings — arguably the most security-critical — have no chain integrity. Combined with the legacy-entry logic error above, the verification step currently provides no tamper-evidence for sandbox-side findings.
Remediation: Either provide a shared Python utility that computes the same hash chain, or have hooks write to a separate file and merge on the host side.

Low

[error-handling] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). While the current Finding struct fields cannot practically fail marshaling, this is a minor deviation from defensive coding practice for an integrity mechanism.
Remediation: Change computeHash to return (string, error) and propagate the marshal error.
[error-handling] internal/security/trace.go:63 — lastHash() returns seedHash on all error paths (file open failure, empty file, unmarshal error, empty hash field). Silently falling back to seedHash means a corrupted or truncated file will start a new chain segment without any indication. VerifyChain would catch the resulting chain break downstream, but the error origin is obscured.
Remediation: Change lastHash() to return (string, error) and propagate errors to callers.
[audit-log-integrity-bypass] internal/security/trace.go:34 — The hash chain uses a hardcoded, publicly known seed hash and SHA-256 without any keyed MAC. An attacker with write access can recompute the entire chain from scratch. The threat model doc acknowledges this as an open question and discusses HMAC/signed entries as future considerations.
[race-condition] internal/security/trace.go:87 — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. The existing caller in run.go calls sequentially, so this is not currently triggered.
Remediation: Document the concurrency limitation in the function's doc comment.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash. Additionally, scanner.Err() is not checked after the scan loop.
[tail-truncation] internal/security/trace.go:134 — VerifyChain validates internal chain consistency but cannot detect tail truncation. The threat model documents this as a known limitation, but the run.go message "Audit log integrity verified" slightly overstates the guarantee.
Remediation: Update the message to "chain consistency verified" rather than "integrity verified."
[fail-open] internal/cli/run.go:981 — Verification is guarded by os.Stat: if findings.jsonl does not exist, verification is silently skipped. An attacker who deletes the entire file avoids verification.
[misleading-label] PR title uses docs(security): prefix but includes 135 lines of production Go code and 221 lines of test code. Per COMMITS.md, docs type is for documentation-only changes. This should use feat(security): to appear in release notes under Features.
Remediation: Change PR title to feat(security): add audit log hash chaining for tamper detection.
[unauthorized-scope-expansion] internal/security/trace.go — 135-line implementation without a dedicated issue. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference, not authorization. The threat model section presents options with trade-offs, which partially addresses the concern.
Remediation: File a tracking issue for audit log integrity.
[api-shape-pattern] internal/security/trace.go — TracedFinding adds PrevHash and Hash as string fields. VerifyChain handles missing fields by checking empty strings, which works and is consistent with the existing struct pattern.
[naming-convention] internal/security/trace.go — Private function names computeHash and lastHash are somewhat generic, though adequately descriptive in context of the single file.
[naming-convention] internal/security/trace.go — The constant name seedHash could be slightly more descriptive (e.g., chainSeedHash).
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence.
[stale-doc] docs/plans/universal-harness-access.md:1281 — States file permissions "match security.AppendFinding" when describing AppendFetchAudit. The reference is accurate for permissions but AppendFinding now has additional functionality not mentioned.

Labels: PR adds security infrastructure (SHA-256 hash chaining for audit logs) with harness integration

Labels: PR adds SHA-256 hash chaining for audit log integrity with harness integration

Previous run (3)

Review

Findings

Medium

[logic-error] internal/cli/run.go:981 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. Currently, all sandbox-side Python hooks write findings without prev_hash/hash fields, so this scenario is realistic. ChainVerification lacks a field to distinguish hash-verified entries from legacy-skipped entries.
Remediation: Add a Verified (or HashChecked) counter to ChainVerification that increments only when an entry's hash is actually validated. Only print the "verified" message when cv.Verified > 0.
[logic-error] internal/cli/run.go:975 — Race between host-side output scan and sandbox download clobbering the same file. scanOutputFiles writes hash-chained entries to runDir/security/findings.jsonl via AppendFinding. Then sandbox.Download copies the sandbox's .security/ directory into the same runDir/security/ directory. If the sandbox wrote its own findings.jsonl, the download may overwrite the host-written chained file. VerifyChain then runs on whatever file remains.
Remediation: Write host-side and sandbox-side findings to separate files, or download sandbox findings before writing host-side entries.
[error-handling] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). While the current Finding struct fields cannot practically fail marshaling, this deviates from the package's established error-handling pattern where AppendFinding checks json.Marshal errors.
Remediation: Change computeHash to return (string, error) and propagate the marshal error.
[error-handling] internal/security/trace.go:69 — lastHash() returns seedHash on all error paths (file open failure, empty file, unmarshal error, empty hash field). Silently falling back to seedHash means a corrupted or truncated file will start a new chain segment without any indication. VerifyChain would catch the resulting chain break downstream, but the error origin is obscured.
Remediation: Change lastHash() to return (string, error) and propagate errors to callers, or at minimum log when falling back to seedHash on a non-empty file.

Low

[race-condition] internal/security/trace.go:42 — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. The existing caller in run.go calls sequentially, so this is not currently triggered, but the API makes no concurrency restriction visible.
Remediation: Document the concurrency limitation in the function's doc comment.
[tail-truncation] internal/security/trace.go:134 — VerifyChain validates internal chain consistency but cannot detect tail truncation. The threat model documents this as a known limitation, but the run.go message "Audit log integrity verified" slightly overstates the guarantee.
Remediation: Update the message to "chain consistency verified" rather than "integrity verified."
[fail-open] internal/cli/run.go:981 — Verification is guarded by os.Stat: if findings.jsonl does not exist, verification is silently skipped. The file may legitimately not exist when no findings are produced, but a warning when security scanning ran with no output file could be useful.
[misleading-label] PR title uses docs(security): prefix but includes 135 lines of production Go code and 221 lines of test code. Per COMMITS.md, this should use feat(security): to appear in release notes under Features.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash. Additionally, scanner.Err() is not checked after the scan loop (though it IS checked in VerifyChain).
[weak-seed] internal/security/trace.go:34 — The seedHash is a hardcoded all-zeros string. The threat model raises the open question of whether the seed should be derived from the run's trace ID to bind the chain to a specific run.
[naming-convention] internal/security/trace_test.go — Custom splitLines/split helpers instead of strings.Split as used in other test files (audit_test.go, run_test.go).
[unauthorized-scope-expansion] internal/security/trace.go — 135-line implementation without a dedicated issue. The PR's threat model section does present multiple options with trade-offs, which partially addresses the concern. Consider filing a tracking issue for audit log integrity.

Previous run (4)

Review

Findings

Medium

[logic-error] internal/cli/run.go:742 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. ChainVerification lacks a field to distinguish "N entries hash-verified" from "N entries legacy-skipped." If sandbox-side hooks write findings without hash chaining to the same findings.jsonl, verification reports success without any tamper-evidence check.
Remediation: Add a Verified (or Skipped) field to ChainVerification so the caller knows how many entries actually had their hash checked. In run.go, only print the verified message when cv.Verified > 0, and emit a distinct info message when all entries were legacy.
[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call. At minimum, document the concurrency limitation in the function's doc comment.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with 135-line SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go, 221 lines), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope and ensure it appears in release notes under Features.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + harness integration + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated. For an integrity mechanism, silent data loss in the hash computation would undermine the security guarantee.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This provides backward compatibility but means legacy entries can be deleted, inserted, or modified without detection. See also: an attacker could insert entries with both hash fields omitted to bypass chain verification entirely. A known trade-off worth documenting in code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded. See also: docs/plans/universal-harness-access.md:1165 which references AppendFinding file permissions.

Previous run (5)

Review

Findings

Medium

[logic-error] internal/cli/run.go:740 — VerifyChain failure calls printer.StepFail but does not return an error or set a non-zero exit code. Every other StepFail call in run.go is followed by return fmt.Errorf(...) to actually halt the run. The threat model document added in this PR states: "After each agent run, a harness-level step verifies the audit log's integrity before the run is considered complete. If verification fails, the run is flagged for investigation regardless of whether the agent's output otherwise looks clean." The current implementation contradicts this — a tampered log produces a print-only message that does not affect the run outcome.
Remediation: Return an error when cv.Valid is false, consistent with the pattern used by all other StepFail calls in this function. If soft-fail is intentional for the initial rollout, add a code comment and a TODO referencing the threat model's open question about synchronous vs. asynchronous verification.
[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call. At minimum, document the concurrency limitation in the function's doc comment.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with 135-line SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go, 221 lines), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope and ensure it appears in release notes under Features.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + harness integration + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated. For an integrity mechanism, silent data loss in the hash computation would undermine the security guarantee.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This is internally consistent with lastHash (which also returns seedHash for entries without hashes) and provides backward compatibility. However, it means legacy entries can be deleted, inserted, or modified without detection — a known trade-off worth documenting in the code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded. See also: docs/plans/universal-harness-access.md:1165 which references AppendFinding file permissions.

Previous run (6)

Review

Findings

Medium

[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Per CLAUDE.md, non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This is internally consistent with lastHash (which also returns seedHash for entries without hashes) and provides backward compatibility. However, it means legacy entries can be deleted, inserted, or modified without detection — a known trade-off worth documenting in the code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded.

Previous run (7)

Review

Findings

Medium

[missing-authorization] docs/problems/security-threat-model.md — This is a non-trivial change (36 new lines introducing a new security concern section) without a linked issue. The PR body mentions this is a prerequisite for Investigate using gitsign to produce GPG signatures on agent-generated commits #1685 but does not link an issue that authorizes the audit log integrity addition itself. Non-trivial changes to the threat model should have explicit authorization. (Severity tempered from high: the change is documentation-only to a design exploration document with a thorough PR body explanation, and the content is architecturally coherent with existing ADRs and principles.)

Low

[edge-case-correctness] docs/problems/security-threat-model.md:445 — The section lists log truncation as an attack vector ("Truncate the log to remove evidence of a compromise window") but the hash chaining defense does not address tail truncation. If an attacker removes the last N entries, the remaining chain is internally consistent — it just ends earlier. Detection of tail truncation requires an external record of the expected chain length or latest hash, which is not mentioned. Consider adding a note that hash chaining alone does not detect tail truncation.
[internal-consistency] docs/problems/security-threat-model.md:437 — The new section is closely related to the adjacent "agent self-report unreliability" section (which references JSONL traces via ADR 0021 as a partial mitigation), but does not cross-reference it. If an agent can both misrepresent its actions (self-report unreliability) AND tamper with the audit log that would catch the misrepresentation, the combination is more dangerous than either alone. A cross-reference would strengthen both sections.

Info

[technical-accuracy] docs/problems/security-threat-model.md:449 — The "Signed entries" defense option mentions a key could be "injected at sandbox creation and revoked at sandbox teardown." Per ADR 0017, the sandbox model is designed so that credentials never enter the sandbox. A signing key injected into the sandbox would be a credential inside the sandbox, creating tension with ADR 0017's core principle. The parenthetical alternative ("the harness signs entries before writing") avoids this tension.

Previous run (8)

Review

Findings

Medium

[logic-error] internal/cli/run.go:981 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. Currently, all sandbox-side Python hooks write findings without prev_hash/hash fields, so this scenario is realistic. ChainVerification lacks a field to distinguish hash-verified entries from legacy-skipped entries.
Remediation: Add a Verified (or HashChecked) counter to ChainVerification that increments only when an entry's hash is actually validated. Only print the "verified" message when cv.Verified > 0.
[logic-error] internal/cli/run.go:975 — Race between host-side output scan and sandbox download clobbering the same file. scanOutputFiles writes hash-chained entries to runDir/security/findings.jsonl via AppendFinding. Then sandbox.Download copies the sandbox's .security/ directory into the same runDir/security/ directory. If the sandbox wrote its own findings.jsonl, the download may overwrite the host-written chained file. VerifyChain then runs on whatever file remains.
Remediation: Write host-side and sandbox-side findings to separate files, or download sandbox findings before writing host-side entries.
[error-handling] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). While the current Finding struct fields cannot practically fail marshaling, this deviates from the package's established error-handling pattern where AppendFinding checks json.Marshal errors.
Remediation: Change computeHash to return (string, error) and propagate the marshal error.
[error-handling] internal/security/trace.go:69 — lastHash() returns seedHash on all error paths (file open failure, empty file, unmarshal error, empty hash field). Silently falling back to seedHash means a corrupted or truncated file will start a new chain segment without any indication. VerifyChain would catch the resulting chain break downstream, but the error origin is obscured.
Remediation: Change lastHash() to return (string, error) and propagate errors to callers, or at minimum log when falling back to seedHash on a non-empty file.

Low

[race-condition] internal/security/trace.go:42 — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. The existing caller in run.go calls sequentially, so this is not currently triggered, but the API makes no concurrency restriction visible.
Remediation: Document the concurrency limitation in the function's doc comment.
[tail-truncation] internal/security/trace.go:134 — VerifyChain validates internal chain consistency but cannot detect tail truncation. The threat model documents this as a known limitation, but the run.go message "Audit log integrity verified" slightly overstates the guarantee.
Remediation: Update the message to "chain consistency verified" rather than "integrity verified."
[fail-open] internal/cli/run.go:981 — Verification is guarded by os.Stat: if findings.jsonl does not exist, verification is silently skipped. The file may legitimately not exist when no findings are produced, but a warning when security scanning ran with no output file could be useful.
[misleading-label] PR title uses docs(security): prefix but includes 135 lines of production Go code and 221 lines of test code. Per COMMITS.md, this should use feat(security): to appear in release notes under Features.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash. Additionally, scanner.Err() is not checked after the scan loop (though it IS checked in VerifyChain).
[weak-seed] internal/security/trace.go:34 — The seedHash is a hardcoded all-zeros string. The threat model raises the open question of whether the seed should be derived from the run's trace ID to bind the chain to a specific run.
[naming-convention] internal/security/trace_test.go — Custom splitLines/split helpers instead of strings.Split as used in other test files (audit_test.go, run_test.go).
[unauthorized-scope-expansion] internal/security/trace.go — 135-line implementation without a dedicated issue. The PR's threat model section does present multiple options with trade-offs, which partially addresses the concern. Consider filing a tracking issue for audit log integrity.

Labels: PR adds security infrastructure (SHA-256 hash chaining for audit logs) with harness integration

Previous run (9)

Review

Findings

Medium

[logic-error] internal/cli/run.go:742 — VerifyChain returns Valid=true with Entries>0 when all entries are legacy (no hash fields). The harness prints "Audit log integrity verified (N entries)" despite having verified zero hashes. ChainVerification lacks a field to distinguish "N entries hash-verified" from "N entries legacy-skipped." If sandbox-side hooks write findings without hash chaining to the same findings.jsonl, verification reports success without any tamper-evidence check.
Remediation: Add a Verified (or Skipped) field to ChainVerification so the caller knows how many entries actually had their hash checked. In run.go, only print the verified message when cv.Verified > 0, and emit a distinct info message when all entries were legacy.
[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call. At minimum, document the concurrency limitation in the function's doc comment.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with 135-line SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go, 221 lines), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope and ensure it appears in release notes under Features.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + harness integration + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated. For an integrity mechanism, silent data loss in the hash computation would undermine the security guarantee.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This provides backward compatibility but means legacy entries can be deleted, inserted, or modified without detection. See also: an attacker could insert entries with both hash fields omitted to bypass chain verification entirely. A known trade-off worth documenting in code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded. See also: docs/plans/universal-harness-access.md:1165 which references AppendFinding file permissions.

Previous run (10)

Review

Findings

Medium

[logic-error] internal/cli/run.go:740 — VerifyChain failure calls printer.StepFail but does not return an error or set a non-zero exit code. Every other StepFail call in run.go is followed by return fmt.Errorf(...) to actually halt the run. The threat model document added in this PR states: "After each agent run, a harness-level step verifies the audit log's integrity before the run is considered complete. If verification fails, the run is flagged for investigation regardless of whether the agent's output otherwise looks clean." The current implementation contradicts this — a tampered log produces a print-only message that does not affect the run outcome.
Remediation: Return an error when cv.Valid is false, consistent with the pattern used by all other StepFail calls in this function. If soft-fail is intentional for the initial rollout, add a code comment and a TODO referencing the threat model's open question about synchronous vs. asynchronous verification.
[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call. At minimum, document the concurrency limitation in the function's doc comment.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with 135-line SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go, 221 lines), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope and ensure it appears in release notes under Features.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + harness integration + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated. For an integrity mechanism, silent data loss in the hash computation would undermine the security guarantee.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This is internally consistent with lastHash (which also returns seedHash for entries without hashes) and provides backward compatibility. However, it means legacy entries can be deleted, inserted, or modified without detection — a known trade-off worth documenting in the code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded. See also: docs/plans/universal-harness-access.md:1165 which references AppendFinding file permissions.

Previous run (11)

Review

Findings

Medium

[race-condition] internal/security/trace.go — TOCTOU race between lastHash() reading the file and AppendFinding() opening it to append. If two goroutines or processes call AppendFinding concurrently on the same file, both read the same lastHash, compute hashes with the same prev_hash, and silently fork the chain. The existing caller in run.go:1450 calls sequentially in a loop, so this is not currently triggered, but the API makes no concurrency restriction visible and any future concurrent caller will corrupt the chain without error.
Remediation: Use advisory file locking (e.g., flock) around the read+append sequence, or use a stateful writer that tracks prev in memory rather than re-reading the file on every call.
[misleading-label] PR title uses docs(security): prefix per Conventional Commits, but the change includes production Go code (internal/security/trace.go with SHA-256 hash chaining implementation) and a new test file (internal/security/trace_test.go), not just documentation. Per the repo's Conventional Commits convention, this should use feat(security): to accurately reflect scope.
[missing-authorization] Non-trivial change (135-line Go implementation + 221-line test file + threat model section) without a linked issue providing authorization. The PR body says "Relates to Investigate using gitsign to produce GPG signatures on agent-generated commits #1685" which is a cross-reference to existing work on gitsign, not authorization for this specific implementation. Per CLAUDE.md, non-trivial changes should have explicit authorization through a linked issue.

Low

[error-handling-gap] internal/security/trace.go:55 — computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). The current Finding struct fields (strings + int) cannot practically fail marshaling, but this deviates from the package's established error-handling pattern where all errors are wrapped with fmt.Errorf and propagated.
[edge-case] internal/security/trace.go:69 — lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain without any error surfaced to the caller. Additionally, scanner.Err() is not checked after the scan loop.
[edge-case] internal/security/trace.go:140 — VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. This is internally consistent with lastHash (which also returns seedHash for entries without hashes) and provides backward compatibility. However, it means legacy entries can be deleted, inserted, or modified without detection — a known trade-off worth documenting in the code comments.
[stale-doc] docs/plans/universal-harness-access-phase1.md:88 — Describes AppendFetchAudit as mirroring security.AppendFinding. With hash chaining now internal to AppendFinding, a developer following this pattern would create an append function without tamper-evidence. The reference is still technically correct (the function still appends JSONL) but the behavioral contract has expanded.

Previous run (12)

Review

Findings

Medium

[missing-authorization] docs/problems/security-threat-model.md — This is a non-trivial change (36 new lines introducing a new security concern section) without a linked issue. The PR body mentions this is a prerequisite for Investigate using gitsign to produce GPG signatures on agent-generated commits #1685 but does not link an issue that authorizes the audit log integrity addition itself. Non-trivial changes to the threat model should have explicit authorization. (Severity tempered from high: the change is documentation-only to a design exploration document with a thorough PR body explanation, and the content is architecturally coherent with existing ADRs and principles.)

Low

[edge-case-correctness] docs/problems/security-threat-model.md:445 — The section lists log truncation as an attack vector ("Truncate the log to remove evidence of a compromise window") but the hash chaining defense does not address tail truncation. If an attacker removes the last N entries, the remaining chain is internally consistent — it just ends earlier. Detection of tail truncation requires an external record of the expected chain length or latest hash, which is not mentioned. Consider adding a note that hash chaining alone does not detect tail truncation.
[internal-consistency] docs/problems/security-threat-model.md:437 — The new section is closely related to the adjacent "agent self-report unreliability" section (which references JSONL traces via ADR 0021 as a partial mitigation), but does not cross-reference it. If an agent can both misrepresent its actions (self-report unreliability) AND tamper with the audit log that would catch the misrepresentation, the combination is more dangerous than either alone. A cross-reference would strengthen both sections.

Info

[technical-accuracy] docs/problems/security-threat-model.md:449 — The "Signed entries" defense option mentions a key could be "injected at sandbox creation and revoked at sandbox teardown." Per ADR 0017, the sandbox model is designed so that credentials never enter the sandbox. A signing key injected into the sandbox would be a credential inside the sandbox, creating tension with ADR 0017's core principle. The parenthetical alternative ("the harness signs entries before writing") avoids this tension.

Add tamper-evident hash chaining to TracedFinding and AppendFinding. Each JSONL entry now includes prev_hash and hash fields forming a SHA-256 chain. Modifying or deleting any entry breaks the chain from that point forward. Add VerifyChain() to validate chain integrity, and tests covering chain construction, tampering detection, and deletion detection. Backward compatible: entries without hash fields are skipped during verification. No changes needed to existing callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

Add tail truncation limitation note to hash chaining defense. Add cross-reference to agent self-report unreliability section. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

Benkapner · 2026-06-08T10:15:16Z

Thanks for the review. Addressed the findings in cfe2e01 and updated the PR description. Here's a response to each:

[missing-authorization] This PR is motivated by #1685 (commit signing). The doc section positions audit log integrity as a prerequisite for commit signing: signing the output commit provides provenance, but if the process log cannot be trusted, provenance alone does not give you accountability. Happy to open a dedicated issue if the maintainers prefer a separate tracking item.

[edge-case-correctness] Good catch. Added an explicit limitation note to the hash chaining defense: "hash chaining alone does not detect tail truncation; detecting that requires an external record of the expected chain length or latest hash." This is also why the doc lists write-once external sinks and post-run verification as complementary defenses rather than positioning hash chaining as sufficient on its own.

[internal-consistency] Added a cross-reference to the agent self-report unreliability section in the opening paragraph: "if an agent can both misrepresent its own actions and tamper with the log that would catch the misrepresentation, the combination defeats both controls."

[technical-accuracy] Agreed. The parenthetical "the harness signs entries before writing" is the intended model, keeping signing keys outside the sandbox consistent with ADR 0017. Clarified this in the updated PR description as well.

Call VerifyChain() on the findings JSONL after extracting sandbox security findings. Logs a failure if the hash chain is broken, confirming the audit trail was not tampered with during the run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

Benkapner · 2026-06-08T10:26:58Z

Added post-run verification in 5e3d64b.

VerifyChain() is now called after extracting sandbox security findings in internal/cli/run.go. If the JSONL hash chain is broken, the run logs a failure. This closes the loop: hash chaining makes tampering detectable, and post-run verification makes that detection automatic rather than requiring a manual audit.

The verification runs only when security is enabled and a findings file exists. If the chain is intact, it logs the entry count. If broken, it logs exactly which entry failed and why.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-06-08T10:39:00Z

+			if verifyErr != nil {
+				printer.StepWarn("Audit log verification error: " + verifyErr.Error())
+			} else if !cv.Valid {
+				printer.StepFail(fmt.Sprintf("Audit log integrity check FAILED: %s", cv.BrokenMsg))


[medium] logic-error

VerifyChain failure calls printer.StepFail but does not return an error or set a non-zero exit code. Every other StepFail call in run.go is followed by return fmt.Errorf(...) to actually halt the run. The threat model document added in this PR states post-run verification should flag the run for investigation if verification fails. The current implementation contradicts this — a tampered log produces a print-only message that does not affect the run outcome.

Suggested fix: Return an error when cv.Valid is false, consistent with the pattern used by all other StepFail calls in this function. If soft-fail is intentional for the initial rollout, add a code comment and a TODO referencing the threat model open question about synchronous vs. asynchronous verification.

Fixed in 8abd8e2. StepFail is now followed by return fmt.Errorf(...), consistent with the rest of run.go. A tampered log halts the run

fullsend-ai-review · 2026-06-08T10:39:00Z

+	payload := struct {
+		TraceID   string `json:"trace_id"`
+		Timestamp string `json:"timestamp"`
+		Phase     string `json:"phase"`


[low] error-handling-gap

computeHash discards the json.Marshal error with data, _ := json.Marshal(payload). Deviates from the package established error-handling pattern.

The payload struct contains only string fields. json.Marshal cannot fail on a struct of strings (no channels, functions, or unsupported types). Accepting the _ here avoids an error path that can never trigger.

fullsend-ai-review · 2026-06-08T10:39:00Z

+}
+
+// lastHash reads the final line of the JSONL file and extracts the hash field.
+// Returns seedHash if the file does not exist or is empty.


[low] edge-case

lastHash uses bufio.Scanner with the default 64KB buffer. Lines exceeding this limit cause scanner.Scan() to return false, and lastHash silently returns seedHash — forking the chain. scanner.Err() is not checked after the scan loop.

valid point, i think in practice, a single JSONL finding entry is unlikely to exceed 64KB (typical entries are under 1KB), but adding scanner.Err() check and a larger buffer would make this more robust. Happy to add if the maintainers want it

fullsend-ai-review · 2026-06-08T10:39:00Z

+	scanner := bufio.NewScanner(f)
+	prev := seedHash
+	idx := 0
+


[low] edge-case

VerifyChain skips legacy entries (both Hash and PrevHash empty) without updating prev. Legacy entries can be deleted, inserted, or modified without detection — a known trade-off worth documenting in code comments.

Intentional. Legacy entries are skipped for backward compatibility so that existing logs from before hash chaining was added don't break verification. The trade-off is documented in the threat model's open questions section (per-run vs. global chains). Once all entries are hashed, this path becomes dead code

VerifyChain failure now returns an error and halts the run, consistent with the StepFail pattern used elsewhere in run.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

github-actions · 2026-06-19T09:49:16Z

Site preview

Preview: https://83265bb9-site.fullsend-ai.workers.dev

Commit: 627e603638de00fd227b94da60dd059db399767a

rh-hemartin · 2026-06-19T09:50:17Z

Hello! Sorry for the delay. Taking a look (also locally).

rh-hemartin · 2026-06-19T10:31:35Z

I'm wondering how do I test this locally... do you have a way to generate findings?

Benkapner · 2026-06-21T06:35:20Z

Thanks @rh-hemartin. you can view it with unit tests: you can run go test ./internal/security/... to see it in action. the tests create fake findings, chain them, tamper with them, and verify the chain catches it. you don't need a real agent run to validate the behavior.

rh-hemartin · 2026-06-22T06:07:04Z

I wanted to see an agent actively trying to forge the chain and fail to do it because of the safeguards you introduced, but I guess it is OK. I'm not very familiar with this part and wanted to give it a go.

github-actions · 2026-06-22T08:06:38Z

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

fullsend-ai-review · 2026-06-22T08:10:00Z

🤖 Review · ❌ Terminated · Started 8:09 AM UTC · Ended 8:27 AM UTC
Commit: 4e21a60 · View workflow run →

Benkapner · 2026-06-22T12:03:56Z

ok, @rh-hemartin, should be clean now

fullsend-ai-review · 2026-06-22T12:07:02Z

🤖 Review · ❌ Terminated · Started 12:07 PM UTC · Ended 12:23 PM UTC
Commit: 4e21a60 · View workflow run →

codecov · 2026-06-22T12:11:07Z

Codecov Report

❌ Patch coverage is 65.11628% with 30 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/security/trace.go	73.68%	13 Missing and 7 partials ⚠️
internal/cli/run.go	0.00%	10 Missing ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-06-22T12:23:55Z

🤖 Finished Review · ❌ Failure · Started 12:07 PM UTC · Completed 12:23 PM UTC
Commit: 9811c00 · View workflow run →

ralphbean

LGTM.

fullsend-ai-review · 2026-06-22T14:32:27Z

🤖 Finished Review · ❌ Failure · Started 2:32 PM UTC · Completed 2:48 PM UTC
Commit: c07c7f7 · View workflow run →

ralphbean

Actually, you're failing commit-lint. See #2514 for why it wasn't caught sooner, but it's failing in the merge queue now.

It's your style(..)... commit. Re-do that commit as chore(..)....

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

Benkapner · 2026-06-22T18:08:55Z

@ralphbean fixed, changed to chore(security):. (thanks for that)

fullsend-ai-review · 2026-06-22T18:11:44Z

🤖 Finished Review · ❌ Failure · Started 6:11 PM UTC · Completed 6:30 PM UTC
Commit: 627e603 · View workflow run →

fullsend-ai-retro · 2026-06-22T19:00:18Z

🤖 Finished Retro · ✅ Success · Started 7:00 PM UTC · Completed 7:11 PM UTC
Commit: 627e603 · View workflow run →

fullsend-ai-retro · 2026-06-22T19:11:25Z

Retro: PR #2010 — Audit log hash chaining

This was a human-authored PR (Benkapner, with Claude co-authoring) adding SHA-256 hash chaining to the security audit log. The review agent performed well overall.

What went well

Review agent caught a real bug. The second review run (10:39 UTC, Jun 8) identified that StepFail in run.go was not followed by return fmt.Errorf(...), meaning a tampered audit log would produce a warning but not halt the run. The author fixed this within 12 minutes.
Architectural feedback was high quality. The first review run (10:05 UTC) identified a genuine race condition between host-side output scanning and sandbox download that could overwrite the chained findings file with a potentially tampered sandbox version.
Author engagement was strong. Benkapner responded to all findings with clear reasoning, accepting valid ones and pushing back with good justification on debatable ones.

What could go better

Time to resolution: 14 days, dominated by 11 days waiting for human review. Not an agent issue.
False positive on infallible operation. The review agent flagged data, _ := json.Marshal(payload) as an error-handling gap in both review runs. The author correctly noted that json.Marshal cannot fail on a struct with only string fields — this is a well-known Go pattern. See proposal below.
Duplicate finding across review runs. The json.Marshal error-handling finding appeared in both the first review (issue comment) and second review (inline comment). This is already tracked by #956 (resolve inline comments from previous reviews on re-review).
Commit convention issue. The review agent flagged the PR title using docs(security): instead of feat(security):, but a different commit convention issue (style(..) vs chore(..)) is what actually blocked the merge queue. Existing issues #2225 and #1529 cover this space.

Proposals filed

Review agent should recognize provably infallible Go operations before flagging ignored errors (in fullsend-ai/fullsend)

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 8, 2026

Benkapner and others added 2 commits June 8, 2026 13:10

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 8, 2026

fullsend-ai-review Bot suggested changes Jun 8, 2026

View reviewed changes

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 8, 2026

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 8, 2026

rh-hemartin self-assigned this Jun 19, 2026

github-actions Bot deployed to site-preview June 19, 2026 09:49 View deployment

rh-hemartin approved these changes Jun 22, 2026

View reviewed changes

rh-hemartin added this pull request to the merge queue Jun 22, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 22, 2026

rh-hemartin added this pull request to the merge queue Jun 22, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 22, 2026

Merge remote-tracking branch 'origin/main' into docs/audit-log-integrity

965cb17

github-actions Bot deployed to site-preview June 22, 2026 08:09 View deployment

github-actions Bot deployed to site-preview June 22, 2026 12:04 View deployment

rh-hemartin added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jun 22, 2026

ralphbean approved these changes Jun 22, 2026

View reviewed changes

This was referenced Jun 22, 2026

e2e: actions/checkout@v7 rejects fork PR checkouts in pull_request_target workflow #2504

Closed

fix(#2504): allow checkout@v7 to check out fork PR code in e2e workflow #2505

Closed

github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jun 22, 2026

github-actions Bot deployed to site-preview June 22, 2026 14:30 View deployment

ralphbean added this pull request to the merge queue Jun 22, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 22, 2026

ralphbean added this pull request to the merge queue Jun 22, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 22, 2026

ralphbean mentioned this pull request Jun 22, 2026

fix(ci): lint individual commits on PRs to catch invalid prefixes early #2514

Open

2 tasks

ralphbean requested changes Jun 22, 2026

View reviewed changes

chore(security): gofmt trace.go

627e603

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>

Benkapner force-pushed the docs/audit-log-integrity branch from c07c7f7 to 627e603 Compare June 22, 2026 18:07

github-actions Bot deployed to site-preview June 22, 2026 18:09 View deployment

ralphbean approved these changes Jun 22, 2026

View reviewed changes

ralphbean added this pull request to the merge queue Jun 22, 2026

Merged via the queue into fullsend-ai:main with commit 0fb80c9 Jun 22, 2026
11 of 12 checks passed

fullsend-ai-retro Bot mentioned this pull request Jun 22, 2026

Review agent should recognize provably infallible Go operations before flagging ignored errors #2529

Open

Conversation

Benkapner commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Uh oh!

fullsend-ai-review Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Info

Uh oh!

Benkapner commented Jun 8, 2026

Uh oh!

Benkapner commented Jun 8, 2026

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Benkapner Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

fullsend-ai-review Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Benkapner Jun 8, 2026

Choose a reason for hiding this comment

Benkapner commented Jun 8, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 8, 2026 •

edited

Loading

github-actions Bot commented Jun 19, 2026 •

edited

Loading

github-actions Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 22, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 22, 2026 •

edited

Loading