feat(core): upload the snapshot log to the logs endpoint, not as a resource (PPLT-6034)

[this-is-shivamsingh]

What

The per-snapshot CLI log was shipped as a /percy.*.log snapshot resource, which the API (PPLT-5969, percy-api#6761) has to bridge with a Redis hint to route it into percy-debug-logs. This PR uploads the log directly to POST /api/v1/logs with service_name='snapshot' and reference_id=<snapshot_id> right after the snapshot is created — the API writes it straight to the log bucket as {build}_{snapshot}_snapshot_{sha}, with no manifest/redirect dependency.

Changes

• @percy/client: add sendSnapshotLog(buildId, snapshotId, content) → POST /logs with service_name: 'snapshot'.
• @percy/core discovery: stop pushing the log resource (drop createLogResource).
• @percy/core snapshot: after sendSnapshot, best-effort uploadSnapshotLog keyed by response.data.id — web snapshots only (automate/app comparisons carry no snapshot log, and their response.data.id is a comparison id).
• Update discovery specs that asserted the log resource; add client + core specs.

Backward compatibility

Old CLIs keep sending the log as a resource, so the API's PPLT-5969 redirect still serves them. The read path (fetch_cli_snapshot_logs, NetworkLogService.cli_log_file_name, and @percy/cli-support percy/percy-cli-support#370) already accepts both key shapes (/percy.*\.log/ and _snapshot_). No API change needed; the percy-api Redis-hint cleanup is a follow-up once old CLIs age out.

Testing

• eslint: clean on all changed files.
• @percy/client suite: passes (new #sendSnapshotLog specs green).
• uploadSnapshotLog logic verified standalone (guard clauses, seed→upload, best-effort). Full core browser suite validated in CI.

Related: percy-api PPLT-5969 (#6761), percy/percy-cli-support#370.

🤖 Generated with Claude Code

[this-is-shivamsingh]

no need for this commits

[this-is-shivamsingh]

Done — removed the comment (the method name + args are self-explanatory).

[this-is-shivamsingh]

remove this comment

[this-is-shivamsingh]

Done — removed the comment.

[this-is-shivamsingh]

Note:
// The per-snapshot CLI log is no longer shipped as a resource; it is uploaded
// directly to the logs endpoint after the snapshot is created (see
// uploadSnapshotLog in snapshot.js), keyed by the snapshot id.

[this-is-shivamsingh]

Done — removed that comment block; the removal is explained in the commit/PR instead.

[this-is-shivamsingh]

At this point, does the snapshot is ran, and all the logs are added in the logger ?

[this-is-shivamsingh]

Yes. This runs inside the snapshot upload task, right after sendSnapshot resolves — so asset discovery and snapshot processing are complete and every log entry for this snapshot (matched by meta.snapshot.name + testCase) is already in the logger; logger.query returns them. It's actually a more complete capture than the old approach, which built the log resource during discovery (earlier). Verified live: support:debug on a new-CLI snapshot downloaded the log with 47 real entries.

[this-is-shivamsingh]

Code Review — PPLT-6034: snapshot log to dedicated logs endpoint

Summary: Stops shipping per-snapshot CLI logs as a fake /percy.<ts>.log resource attached to the DOM snapshot, and instead uploads them directly to the logs endpoint keyed by snapshot id (service_name: 'snapshot'), mirroring the existing build-log egress path. Net change is 4 source files: client/src/client.js, core/src/discovery.js, core/src/snapshot.js, core/src/utils.js.

Findings

Severity	Location	Issue	Suggestion
Medium	core/test/snapshot.test.js (uploadSnapshotLog spec)	The security-critical redactSecrets step on this new egress path is only implicitly covered — the happy-path test asserts jasmine.any(String) for content, so a regression that dropped redaction would not fail any test.	Add a test that seeds a log line containing a PERCY_TOKEN-shaped secret and asserts the decoded payload does not contain it.
Low	core/src/snapshot.js + core/src/percy.js (sendBuildLogs)	Snapshot logs now egress twice: once per-snapshot via sendSnapshotLog, and still inside the build-level sendBuildLogs aggregate (the main messages Set retains everything).	Confirm this is intended (build log = full aggregate, snapshot log = per-snapshot slice for the UI). If so, no change — flagging only to rule out an accidental leftover of the old resource path.
Low	client/src/client.js (sendSnapshotLog)	No if (!process.env.PERCY_TOKEN) return guard that sendBuildLogs has.	Mitigated in practice — only reached after a successful sendSnapshot (already authed), and this.post sends auth headers regardless. Safe to leave; noting for symmetry.

Checklist result

• Critical — Secrets: ✅ redactSecrets(logger.snapshotLogs(...)) runs before egress (CWE-532), matching the build-log path.
• Critical — API: ✅ Additive only — new sendSnapshotLog method; no breaking flag/signature change.
• High — Errors / Daemon: ✅ uploadSnapshotLog is fully best-effort (try/catch, never throws), so a log-upload failure can't fail the snapshot. finally { evictSnapshot } bounds memory — this ticket improves daemon resource pressure.
• Correctness (verified):
  • response.data.id is the snapshot id — consistent with existing JobData(response.data.id, …) usage. ✅
  • evictSnapshot deletes only the snapshotLogs cache + flags a rescan; it does not touch the main messages Set, so the end-of-build sendBuildLogs aggregate is not starved. ✅
  • Gated on send === 'sendSnapshot', so automate/app comparisons (no snapshot log) are correctly skipped. ✅
  • createLogResource fully removed — zero surviving callers repo-wide; snapshotLogName/logger imports in discovery.js are still used; pako is a declared core dependency. ✅
• High — Tests: ✅ Error/edge paths covered: missing ids, empty meta, happy path keyed by snapshot id, best-effort rejection; client-side happy path + validateId failures.

Verdict

Approve — clean, well-scoped, correctly best-effort, and the two subtle correctness risks (build-log starvation via eviction, snapshot-id source) both check out. Only pre-merge ask is the Medium: an explicit redaction test, since this is a new secret-egress path that shouldn't silently regress.

_{🤖 Repo-specific review via the percy-cli-code-review skill (Claude Code).}

[this-is-shivamsingh]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[this-is-shivamsingh]

[Medium] Snapshot log cache not evicted when the snapshot send fails

logger.evictSnapshot(meta.snapshot) now runs only inside uploadSnapshotLog's finally, which is reached only after let response = yield percy.client[send](build.id, snapshot) (line 533) succeeds. If that send throws, the generator jumps straight to the queue's 'error' handler and this line — and the finally — never run, so the cached log entries for that snapshot key are never evicted (and never sent). Previously evictSnapshot ran unconditionally during discovery, before any upload, so a failed send still evicted. This is a bounded-per-build resource leak on the failure path.

Suggestion: also call logger.evictSnapshot(meta.snapshot) in the queue's 'error' handler, or wrap the client[send] + evict in a try/finally in the task handler so eviction always happens. Add a test for the send-failure path.

Reviewer: stack-code-reviewer

[this-is-shivamsingh]

[Medium] Leftover build-test scaffolding, unrelated to PPLT-6034

This file (Example Page → bing.com, Google Homepage → github.com) isn't referenced by any test, script, or package, and its only history is the trailing "percy build testing" commits. It's scratch content used to trigger a Percy build run, not part of the log-upload feature, and should not land on master.

Suggestion: git rm yamls/snapshot.yaml and drop it (plus the three build-test commits) from the PR before merge.

Reviewer: stack-code-reviewer

[this-is-shivamsingh]

Claude Code PR Review

PR: #2417 • Head: 62dbc5d • Reviewers: stack-code-reviewer

Summary

Moves the per-snapshot CLI log off the DOM-resource path (/percy.<ts>.log) and uploads it directly to the logs endpoint via a new best-effort PercyClient.sendSnapshotLog, keyed by the snapshot id returned from sendSnapshot (gzip + base64, secrets redacted before egress). The endpoint migration itself is correct, well-tested, and consistent with the existing sendBuildLogs conventions; the open items are a failure-path cache-eviction gap and stray scratch artifacts on the branch.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets added; yamls/snapshot.yaml holds only public URLs.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched; reuses existing client auth.
High	Security	Input validation and sanitization	Pass	sendSnapshotLog calls validateId on build & snapshot ids.
High	Security	No IDOR — resource ownership validated	N/A	Server-side concern; client keys by ids it already owns.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL in the CLI.
High	Correctness	Logic is correct, handles edge cases	Pass	response.data.id keying and send === 'sendSnapshot' guard verified correct; no-id / no-log early returns present.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	uploadSnapshotLog is intentionally best-effort (try/catch/finally) with debug logging — swallow is deliberate and documented.
High	Correctness	No race conditions or concurrency issues	Pass	Runs inside the existing snapshot task generator; no new shared state.
Medium	Testing	New code has corresponding tests	Pass	Client + core unit tests + discovery integration test added.
Medium	Testing	Error paths and edge cases tested	Fail	No test for eviction/no-eviction when the outer sendSnapshot throws (Finding 1).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Resource-index shifts in discovery/snapshot tests are correct mechanical fixups (one fewer resource per snapshot).
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One POST per web snapshot; gzip keeps payload small.
Medium	Performance	Long-running tasks use background jobs	Pass	Best-effort, non-blocking to snapshot success.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors sendBuildLogs (gzip/base64/redact).
Medium	Quality	Changes are focused (single concern)	Fail	Stray yamls/snapshot.yaml + 3 build-test commits are unrelated to the feature (Findings 2 & 3).
Low	Quality	Meaningful names, no dead code	Pass	createLogResource and its now-unused imports removed cleanly; lint clean.
Low	Quality	Comments explain why, not what	Pass	Comments explain the egress/redaction rationale.
Low	Quality	No unnecessary dependencies added	Pass	Reuses pako already used elsewhere in core.

Findings

• File: packages/core/src/snapshot.js:540 (call site) / packages/core/src/snapshot.js uploadSnapshotLog finally
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: logger.evictSnapshot(meta.snapshot) used to run unconditionally in discovery.js during resource processing, before any upload. It now runs only inside uploadSnapshotLog's finally, which is reached only if let response = yield percy.client[send](build.id, snapshot) (snapshot.js:533) succeeds. If that send throws, the generator jumps straight to the queue's 'error' handler and uploadSnapshotLog never runs — so the cached log entries for that snapshot key are never evicted (and never sent, since there's no snapshot id). For builds with many failing/retried snapshots this is a bounded-per-build resource leak that did not exist before.
• Suggestion: Also call logger.evictSnapshot(meta.snapshot) in the queue's 'error' handler, or wrap the client[send] call + evict in a try/finally in the task handler so eviction always happens regardless of send outcome. Add a test covering the send-failure path.

---

• File: yamls/snapshot.yaml (new file)
• Severity: Medium (hygiene)
• Reviewer: stack-code-reviewer
• Issue: Leftover personal build-test scaffolding (Example Page → bing.com, Google Homepage → github.com). Not referenced by any test, script, or package; its only history is the trailing "percy build testing" commits. Unrelated to PPLT-6034 and should not land on master.
• Suggestion: git rm yamls/snapshot.yaml and drop it from the PR before merge.

---

• File: (commit history)
• Severity: Low (hygiene)
• Reviewer: stack-code-reviewer
• Issue: 6a164ef / b0a6575 ("chore: empty commit for percy build testing" ×2) and f8fcfb1 ("chore: change Example Page url to bing.com for percy build test") add no functional value and are tied to the stray yaml above.
• Suggestion: Squash/drop these three commits (or squash-merge the PR) so master history reads as a clean feat + test sequence.

---

• File: packages/core/src/snapshot.js:540 → packages/client/src/client.js:946
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The full snapshot task meta is spread into the request options in sendSnapshotLog ({ identifier: 'snapshot.send_log', ...meta }). This matches the existing sendSnapshot/sendComparison convention, but a meta object carrying an identifier key would clobber 'snapshot.send_log' in logging/metrics. Precedented and low-risk; flagged for awareness.
• Suggestion: No change required unless meta shape changes; if hardening, spread meta first then set identifier last.

Verified clean (no action needed)

• response.data.id keying — correct: send is 'sendSnapshot' only for web/DOM snapshots (not 'sendComparison'), and comparison-tag snapshots (Automate/App) never flowed through processSnapshotResources, so they never had a per-snapshot log resource before — no regression, guard correctly scoped.
• Secret-redaction parity — redactSecrets(logger.snapshotLogs(...)) before gzip/base64 matches the sendBuildLogs CWE-532 mitigation; snapshotLogs() returns shallow copies and redactSecrets mutates only .message, consistent with the existing contract.
• Lint & dangling imports — clean; createLogResource/redactSecrets removed from discovery.js with no unused references left.

Reconciliation with human reviewers

• @rishigupta1599 approved the PR (2026-09-03).
• The author's own inline threads (remove-comment nits on client.js/discovery.js, and the "are all logs in the logger at this point?" question on snapshot.js:540) were all addressed/answered in-thread — nothing outstanding to carry.

---

Verdict: PASS — the endpoint migration is correct and well-tested; no High/Critical findings. Please still address the Medium items before merge: fix (or ticket) the eviction-on-send-failure gap, and strip the stray yamls/snapshot.yaml and the three build-test commits so they don't land on master.