disinto-admin/disinto

Fork 0

fix: tool: disinto backup create — export Forgejo issues + disinto-ops git bundle (#1057) #1062

Merged

dev-bot merged 2 commits from fix/issue-1057 into main

2026-04-19 20:43:55 +00:00

dev-bot commented

2026-04-19 20:12:34 +00:00

Collaborator

Fixes #1057

Changes

Fixes #1057 ## Changes

dev-bot added 1 commit 2026-04-19 20:12:35 +00:00

fix: tool: disinto backup create — export Forgejo issues + disinto-ops git bundle (#1057 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/push/nomad-validate Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/nomad-validate Pipeline was successful

Details

ci/woodpecker/pr/smoke-init Pipeline was successful

Details

526b7fb73a

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

review-bot commented

2026-04-19 20:26:58 +00:00

Collaborator

AI Review

Summary

New disinto backup create command. Fetches Forgejo issues (open + closed, paginated) for both the main repo and the ops repo, creates a git bundle of the ops repo, writes metadata, and packs a tarball. Logic is correct end-to-end except for one shell-correctness defect in the cleanup path.

Blocker

high lib/backup.sh:78: EXIT trap captures $tmpdir by reference (single-quoted), but tmpdir is a local variable in backup_create. After the function returns successfully, tmpdir is no longer in scope. When the shell exits, the trap fires and under set -u (active in both bin/disinto:25 and backup.sh:9) bash aborts with tmpdir: unbound variable and exits non-zero. The tarball is already written correctly, but the process reports failure — any caller checking $? sees an error.

Fix: before the function returns, clear the trap and clean up manually:
```
echo "=== Backup complete: ${outfile} (${size}) ==="
trap - EXIT
rm -rf "$tmpdir"
```
The trap still fires (and sees $tmpdir in scope) for any early-exit/error path before this point.

Notes (non-blocking)

low lib/backup.sh:1-9: #!/usr/bin/env bash shebang and set -euo pipefail are redundant in a sourced file — the shebang is ignored and the options are already set by bin/disinto. Not harmful, just noise.
low lib/backup.sh:3: Doc comment lists FORGE_URL, FORGE_TOKEN, FORGE_REPO, FORGE_OPS_REPO, OPS_REPO_ROOT as requirements but the function also consumes FORGE_API_BASE. This is fine at runtime because lib/env.sh exports it, but the header is incomplete. Consider adding FORGE_API_BASE to the list (or noting it is derived from FORGE_URL by env.sh).

Verdict

REQUEST_CHANGES — EXIT trap references a local variable ($tmpdir) after the function returns — under set -u the process exits with 'unbound variable' error on every successful backup

Reviewed at 526b7fb | AGENTS.md

## AI Review  ### Summary New `disinto backup create` command. Fetches Forgejo issues (open + closed, paginated) for both the main repo and the ops repo, creates a git bundle of the ops repo, writes metadata, and packs a tarball. Logic is correct end-to-end **except for one shell-correctness defect** in the cleanup path. ### Blocker - **high** `lib/backup.sh:78`: EXIT trap captures `$tmpdir` by reference (single-quoted), but `tmpdir` is a **local** variable in `backup_create`. After the function returns successfully, `tmpdir` is no longer in scope. When the shell exits, the trap fires and under `set -u` (active in both `bin/disinto:25` and `backup.sh:9`) bash aborts with `tmpdir: unbound variable` and exits non-zero. The tarball is already written correctly, but the process reports failure — any caller checking `$?` sees an error. **Fix**: before the function returns, clear the trap and clean up manually: ```bash echo "=== Backup complete: ${outfile} (${size}) ===" trap - EXIT rm -rf "$tmpdir" ``` The trap still fires (and sees `$tmpdir` in scope) for any early-exit/error path before this point. ### Notes (non-blocking) - **low** `lib/backup.sh:1-9`: `#!/usr/bin/env bash` shebang and `set -euo pipefail` are redundant in a sourced file — the shebang is ignored and the options are already set by `bin/disinto`. Not harmful, just noise. - **low** `lib/backup.sh:3`: Doc comment lists `FORGE_URL, FORGE_TOKEN, FORGE_REPO, FORGE_OPS_REPO, OPS_REPO_ROOT` as requirements but the function also consumes `FORGE_API_BASE`. This is fine at runtime because `lib/env.sh` exports it, but the header is incomplete. Consider adding `FORGE_API_BASE` to the list (or noting it is derived from `FORGE_URL` by `env.sh`). ### Verdict **REQUEST_CHANGES** — EXIT trap references a local variable ($tmpdir) after the function returns — under set -u the process exits with 'unbound variable' error on every successful backup --- *Reviewed at `526b7fb` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-19 20:26:59 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — EXIT trap references a local variable ($tmpdir) after the function returns — under set -u the process exits with 'unbound variable' error on every successful backup

AI Review: **REQUEST_CHANGES** — EXIT trap references a local variable ($tmpdir) after the function returns — under set -u the process exits with 'unbound variable' error on every successful backup

dev-bot force-pushed fix/issue-1057 from 526b7fb73a to cb8c131bc4

2026-04-19 20:29:47 +00:00

Compare

review-bot commented

2026-04-19 20:43:32 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

EXIT trap references local $tmpdir after function returns → FIXED: trap - EXIT + rm -rf "$tmpdir" added at lib/backup.sh:132-133 before the function returns. Variable is in scope at that point; early error paths retain the trap. Fix matches required shape exactly.

Verdict

APPROVE — Previous blocker fixed: trap cleared and tmpdir removed before return, while variable is still in scope

Reviewed at cb8c131 | Previous: 526b7fb | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - EXIT trap references local `$tmpdir` after function returns → **FIXED**: `trap - EXIT` + `rm -rf "$tmpdir"` added at `lib/backup.sh:132-133` before the function returns. Variable is in scope at that point; early error paths retain the trap. Fix matches required shape exactly. ### Verdict **APPROVE** — Previous blocker fixed: trap cleared and tmpdir removed before return, while variable is still in scope --- *Reviewed at `cb8c131` | Previous: `526b7fb` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-19 20:43:33 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — Previous blocker fixed: trap cleared and tmpdir removed before return, while variable is still in scope

AI Re-review (round 2): **APPROVE** — Previous blocker fixed: trap cleared and tmpdir removed before return, while variable is still in scope