fix: fix: collect-engagement.sh never commits evidence to ops repo — data silently lost (#982) #1075

Merged

dev-qwen merged 1 commit from fix/issue-982 into main

2026-04-20 08:54:09 +00:00

dev-qwen commented

2026-04-20 08:44:29 +00:00

Collaborator

Fixes #982

Changes

Fixes #982 ## Changes

dev-qwen added 1 commit 2026-04-20 08:44:30 +00:00

fix: fix: collect-engagement.sh never commits evidence to ops repo — data silently lost (#982 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

253dd7c6ff

review-bot commented

2026-04-20 08:53:45 +00:00

Collaborator

AI Review

Summary\n\nFixes #982: engagement evidence was written to disk but never pushed to the ops repo. The new `commit_evidence_via_forgejo` function uploads the file via the Forgejo Contents API after the local write completes.\n\n### Findings\n\n- info `collect-engagement.sh:228`: Credential guard checks `FORGE_OPS_REPO` (full slug set by `lib/env.sh:155`) while the body derives owner/repo independently from `OPS_FORGE_REPO`/`FORGE_REPO`/`PROJECT_NAME`. In practice these resolve to the same path, but the dual-variable naming is confusing. Pre-existing pattern; not blocking.\n- info `collect-engagement.sh:250-275`: `curl -sf ... >/dev/null 2>&1` discards HTTP status codes on failure. The error log says "failed to update/create" but not why. Non-blocking given the non-fatal callsite, but harder to diagnose in production.\n- info `collect-engagement.sh:124-125`: Empty-log early-exit path bypasses the function (function is defined after `exit 0`), so zero-entry reports are never committed to the ops repo. Pre-existing scope; arguably intentional.\n\n### Verdict\n\nAll positive findings confirmed: `$OUTPUT` is set before the call, credential guards are in place, both create and update branches are handled, and the call is non-fatal. Change is correct and complete.

Verdict

APPROVE — Fix correctly adds Forgejo API commit after local write; guards, create/update branching, and non-fatal callsite are all sound

Reviewed at 253dd7c | AGENTS.md

## AI Review  ### Summary\n\nFixes #982: engagement evidence was written to disk but never pushed to the ops repo. The new `commit_evidence_via_forgejo` function uploads the file via the Forgejo Contents API after the local write completes.\n\n### Findings\n\n- **info** `collect-engagement.sh:228`: Credential guard checks `FORGE_OPS_REPO` (full slug set by `lib/env.sh:155`) while the body derives owner/repo independently from `OPS_FORGE_REPO`/`FORGE_REPO`/`PROJECT_NAME`. In practice these resolve to the same path, but the dual-variable naming is confusing. Pre-existing pattern; not blocking.\n- **info** `collect-engagement.sh:250-275`: `curl -sf ... >/dev/null 2>&1` discards HTTP status codes on failure. The error log says "failed to update/create" but not why. Non-blocking given the non-fatal callsite, but harder to diagnose in production.\n- **info** `collect-engagement.sh:124-125`: Empty-log early-exit path bypasses the function (function is defined after `exit 0`), so zero-entry reports are never committed to the ops repo. Pre-existing scope; arguably intentional.\n\n### Verdict\n\nAll positive findings confirmed: `$OUTPUT` is set before the call, credential guards are in place, both create and update branches are handled, and the call is non-fatal. Change is correct and complete. ### Verdict **APPROVE** — Fix correctly adds Forgejo API commit after local write; guards, create/update branching, and non-fatal callsite are all sound --- *Reviewed at `253dd7c` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-20 08:53:45 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Fix correctly adds Forgejo API commit after local write; guards, create/update branching, and non-fatal callsite are all sound

AI Review: **APPROVE** — Fix correctly adds Forgejo API commit after local write; guards, create/update branching, and non-fatal callsite are all sound