Skip to content

feat: add fetch audit logging for remote resource tracking - #1094

Merged
ggallen merged 1 commit into
mainfrom
feat/fetch-audit-logging-v2
May 26, 2026
Merged

feat: add fetch audit logging for remote resource tracking#1094
ggallen merged 1 commit into
mainfrom
feat/fetch-audit-logging-v2

Conversation

@ggallen

@ggallen ggallen commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

  • Add FetchAuditEntry struct and AppendFetchAudit function for JSONL audit logging
  • Mirrors existing security.AppendFinding pattern
  • Foundation for ADR-0038 universal harness access — no callers yet

Test plan

  • Unit tests for single/multiple entries, directory creation, JSON field names
  • go test ./internal/fetch/... passes
  • make lint passes

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

Site preview

Preview: https://e3b0644a-site.fullsend-ai.workers.dev

Commit: 8e8f7431f706c905d5a9ca1d3f238a85ad5da841

@fullsend-ai-review

fullsend-ai-review Bot commented May 18, 2026

Copy link
Copy Markdown

Review

Findings

No findings.

Clean implementation of AppendFetchAudit in internal/fetch/audit.go — mirrors security.AppendFinding pattern with the addition of os.MkdirAll for directory creation. File permissions (0o600) match the security package convention. Tests cover single/multiple entries, nested directory creation, and JSON field verification. Plan doc updated to reflect the implemented API.

Previous run

Review

Findings

Low

  • [style] internal/fetch/audit.go:11 — Exported struct FetchAuditEntry and function AppendFetchAudit lack godoc comments. The mirrored AppendFinding in internal/security/trace.go includes a doc comment (// AppendFinding writes a traced finding as a JSON line to the given file path.). Adding similar comments would maintain consistency across the codebase.
    Remediation: Add godoc comments to FetchAuditEntry (describing its purpose as a JSONL audit record for remote resource fetches) and AppendFetchAudit (describing it as appending a fetch audit entry to the given log path).
Previous run (2)

Review

Findings

No findings.

Clean implementation that mirrors the established security.AppendFinding pattern. File permissions (0o600), error wrapping, and JSONL append logic are consistent with existing conventions. The caller-provides-path design is a good separation-of-concerns improvement over the plan doc's original env-var-based approach. Tests cover the key scenarios (single/multiple entries, directory creation, JSON field names). Plan document updated to match the implementation.

Previous run (3)

Review

Findings

Low

  • [docs-currency] docs/plans/universal-harness-access.md:1190 — The implementation plan (section 8, "Audit Logging") describes this component as internal/audit/fetch_log.go with a FetchLog struct and LogFetch function, but the actual implementation uses internal/fetch/audit.go with FetchAuditEntry and AppendFetchAudit. The plan also specifies FULLSEND_AUDIT_DIR env-var-based path resolution, while the implementation takes logPath as a parameter (better separation of concerns). The plan should be updated to reflect the actual implementation to avoid confusion for future contributors.
    Remediation: Update section 8 of the implementation plan to match the actual package path, struct name, function signature, and the addition of the CacheHit field.
Previous run (4)

Review

Findings

Low

  • [platform-security] internal/fetch/audit.go:25 — Audit log file created with 0o644 permissions (world-readable), while the analogous security.AppendFinding uses 0o600. Audit logs may contain URLs of fetched resources, which could expose information about what the system accesses.
    Remediation: Consider using 0o600 to match the security package's pattern, unless downstream tooling requires broader read access.

Info

  • [style] internal/fetch/audit.go:35 — Minor: uses f.Write(append(data, '\n')) which allocates a new slice on each call. The existing security.AppendFinding uses fmt.Fprintf(f, "%s\n", data) which avoids the allocation. No practical impact at expected call volumes.

Comment thread internal/fetch/audit.go Outdated
Comment thread internal/fetch/audit.go Outdated
@fullsend-ai-review

Copy link
Copy Markdown

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

  • #1098 — Audit log file created with 0o644 permissions (world-readable), while the analogous security.AppendFinding uses 0o600. Audit logs may contain URLs of fetched resources.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 19, 2026
Comment thread internal/fetch/audit.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 26, 2026
Add JSONL audit logging for remote resource fetches, mirroring the
existing security.AppendFinding pattern. Includes FetchAuditEntry
struct, AppendFetchAudit function, and tests. Updates the design
plan to match the implementation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 26, 2026
@ggallen
ggallen added this pull request to the merge queue May 26, 2026
Merged via the queue into main with commit 2ff8417 May 26, 2026
9 of 10 checks passed
@ggallen
ggallen deleted the feat/fetch-audit-logging-v2 branch May 26, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants