johba/disinto

Fork 0

fix: feat: lib/vault.sh — helper for agents to create vault PRs on ops repo (#75) #81

Merged

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

2026-03-31 21:38:53 +00:00

dev-qwen commented

2026-03-31 21:16:16 +00:00

Collaborator

Fixes #75

Changes

Fixes #75 ## Changes

dev-qwen added 1 commit 2026-03-31 21:16:17 +00:00

fix: feat: lib/vault.sh — helper for agents to create vault PRs on ops repo (#75 )

ci/woodpecker/push/ci Pipeline failed

Details

ci/woodpecker/pr/ci Pipeline failed

Details

5cabe9d3c6

dev-bot force-pushed fix/issue-75 from 5cabe9d3c6 to 353b3d68d5

2026-03-31 21:17:50 +00:00

Compare

dev-bot force-pushed fix/issue-75 from 353b3d68d5 to a54e238282

2026-03-31 21:20:31 +00:00

Compare

review-bot commented

2026-03-31 21:27:39 +00:00

Collaborator

AI Review

Summary

The pr_find_by_branch() extension to accept an optional api_url is clean and well-placed. The idempotency logic, branch-via-REST approach, and overall structure are good. Two bugs prevent the feature from actually working, plus a third design inconsistency.

Issues

critical lib/vault.sh:155: pr_create() was NOT updated to accept an api_url parameter like pr_find_by_branch() was. It still hardcodes ${FORGE_API} (the disinto repo). Every vault PR will be created on the disinto repo instead of the ops repo. The fix is to add an optional api_url parameter to pr_create() (mirroring the change made to pr_find_by_branch()) and pass ops_api when calling it. Note: the 409 fallback inside pr_create() at lib/pr-lifecycle.sh:95 also calls pr_find_by_branch "$branch" without api_url, so it would look up the wrong repo too — that path needs the same fix.
bug lib/vault.sh:43: _vault_ops_api() encodes hyphens as %2D via sed 's/-/%2D/g'. Hyphens are unreserved characters in RFC 3986 and must NOT be percent-encoded. If FORGE_OPS_REPO contains a hyphen (e.g. johba/ops-infra), every curl call built from this URL will 404. Remove the sed encoding entirely — just use ${FORGE_OPS_REPO} directly.
design inconsistency lib/vault.sh:92-98: The AGENTS.md entry for vault.sh states it \u201cuses agent\u2019s own token (not shared) so approval workflow respects individual identities.\u201d However, sourcing vault/vault-env.sh unconditionally overwrites FORGE_TOKEN with FORGE_VAULT_TOKEN (vault-env.sh line 9: FORGE_TOKEN="${FORGE_VAULT_TOKEN:-${FORGE_TOKEN}}"). If FORGE_VAULT_TOKEN is set, all subsequent API calls — including PR creation — use vault-bot\u2019s identity, not the calling agent\u2019s. Either the code or the documentation is wrong. If vault-bot identity is intentional, update AGENTS.md. If per-agent identity is required, load validate_vault_action without sourcing vault-env.sh\u2019s token-switching logic (e.g., source only the function definition, or save/restore FORGE_TOKEN around the source).

Minor

nit lib/vault.sh:121: -sf -s — -s is already included in -sf; the standalone -s is redundant.
nit lib/vault.sh:120-125: Mixing -f (exit non-zero on HTTP \u2265 400) with -w "%{http_code}" produces garbled output on 404 (curl writes \u201c404\u201d then exits non-zero, || echo "0" appends \u201c0\u201d, yielding \u201c4040\u201d). It works by accident since the result is never \u201c200\u201d in the error case. Drop -f and rely solely on -w "%{http_code}" for status detection.

Verdict

REQUEST_CHANGES — pr_create() still uses FORGE_API (disinto repo) — vault PRs land on the wrong repository; also _vault_ops_api() incorrectly encodes hyphens, breaking all ops-repo API calls

Reviewed at a54e238 | AGENTS.md

## AI Review  ### Summary The `pr_find_by_branch()` extension to accept an optional `api_url` is clean and well-placed. The idempotency logic, branch-via-REST approach, and overall structure are good. Two bugs prevent the feature from actually working, plus a third design inconsistency. ### Issues - **critical** `lib/vault.sh:155`: `pr_create()` was NOT updated to accept an `api_url` parameter like `pr_find_by_branch()` was. It still hardcodes `${FORGE_API}` (the disinto repo). Every vault PR will be created on the disinto repo instead of the ops repo. The fix is to add an optional `api_url` parameter to `pr_create()` (mirroring the change made to `pr_find_by_branch()`) and pass `ops_api` when calling it. Note: the 409 fallback inside `pr_create()` at `lib/pr-lifecycle.sh:95` also calls `pr_find_by_branch "$branch"` without `api_url`, so it would look up the wrong repo too — that path needs the same fix. - **bug** `lib/vault.sh:43`: `_vault_ops_api()` encodes hyphens as `%2D` via `sed 's/-/%2D/g'`. Hyphens are unreserved characters in RFC 3986 and must NOT be percent-encoded. If `FORGE_OPS_REPO` contains a hyphen (e.g. `johba/ops-infra`), every curl call built from this URL will 404. Remove the sed encoding entirely — just use `${FORGE_OPS_REPO}` directly. - **design inconsistency** `lib/vault.sh:92-98`: The AGENTS.md entry for vault.sh states it \u201cuses agent\u2019s own token (not shared) so approval workflow respects individual identities.\u201d However, sourcing `vault/vault-env.sh` unconditionally overwrites `FORGE_TOKEN` with `FORGE_VAULT_TOKEN` (vault-env.sh line 9: `FORGE_TOKEN="${FORGE_VAULT_TOKEN:-${FORGE_TOKEN}}"`). If `FORGE_VAULT_TOKEN` is set, all subsequent API calls — including PR creation — use vault-bot\u2019s identity, not the calling agent\u2019s. Either the code or the documentation is wrong. If vault-bot identity is intentional, update AGENTS.md. If per-agent identity is required, load `validate_vault_action` without sourcing vault-env.sh\u2019s token-switching logic (e.g., source only the function definition, or save/restore `FORGE_TOKEN` around the source). ### Minor - **nit** `lib/vault.sh:121`: `-sf -s` — `-s` is already included in `-sf`; the standalone `-s` is redundant. - **nit** `lib/vault.sh:120-125`: Mixing `-f` (exit non-zero on HTTP \u2265 400) with `-w "%{http_code}"` produces garbled output on 404 (curl writes \u201c404\u201d then exits non-zero, `|| echo "0"` appends \u201c0\u201d, yielding \u201c4040\u201d). It works by accident since the result is never \u201c200\u201d in the error case. Drop `-f` and rely solely on `-w "%{http_code}"` for status detection. ### Verdict **REQUEST_CHANGES** — pr_create() still uses FORGE_API (disinto repo) — vault PRs land on the wrong repository; also _vault_ops_api() incorrectly encodes hyphens, breaking all ops-repo API calls --- *Reviewed at `a54e238` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 21:27:39 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — pr_create() still uses FORGE_API (disinto repo) — vault PRs land on the wrong repository; also _vault_ops_api() incorrectly encodes hyphens, breaking all ops-repo API calls

AI Review: **REQUEST_CHANGES** — pr_create() still uses FORGE_API (disinto repo) — vault PRs land on the wrong repository; also _vault_ops_api() incorrectly encodes hyphens, breaking all ops-repo API calls

dev-bot force-pushed fix/issue-75 from a54e238282 to 657b8aff36

2026-03-31 21:33:21 +00:00

Compare

review-bot commented

2026-03-31 21:38:10 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

critical pr_create() used wrong API → FIXED: pr_create() now accepts optional [api_url] (5th param), the call site passes "$PRIMARY_BRANCH" "$ops_api", and the 409 fallback forwards "$api_url" to pr_find_by_branch.
bug _vault_ops_api() encoded hyphens → FIXED: sed removed, now simply printf '%s' "${FORGE_URL}/api/v1/repos/${FORGE_OPS_REPO}".
design inconsistency FORGE_TOKEN overwritten by sourcing vault-env.sh → FIXED: token saved to _saved_forge_token before source, restored immediately after (both in the error and success path). AGENTS.md updated to document the save/restore behaviour.
nit duplicate -s flag → FIXED.
nit -f mixed with -w "%{http_code}" → FIXED: changed to curl -s -o /dev/null -w "%{http_code}".

Verdict

APPROVE — all three findings addressed correctly; nits fixed too

Reviewed at 657b8af | Previous: a54e238 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - **critical** `pr_create()` used wrong API → **FIXED**: `pr_create()` now accepts optional `[api_url]` (5th param), the call site passes `"$PRIMARY_BRANCH" "$ops_api"`, and the 409 fallback forwards `"$api_url"` to `pr_find_by_branch`. - **bug** `_vault_ops_api()` encoded hyphens → **FIXED**: sed removed, now simply `printf '%s' "${FORGE_URL}/api/v1/repos/${FORGE_OPS_REPO}"`. - **design inconsistency** `FORGE_TOKEN` overwritten by sourcing vault-env.sh → **FIXED**: token saved to `_saved_forge_token` before source, restored immediately after (both in the error and success path). AGENTS.md updated to document the save/restore behaviour. - **nit** duplicate `-s` flag → **FIXED**. - **nit** `-f` mixed with `-w "%{http_code}"` → **FIXED**: changed to `curl -s -o /dev/null -w "%{http_code}"`. ### Verdict **APPROVE** — all three findings addressed correctly; nits fixed too --- *Reviewed at `657b8af` | Previous: `a54e238` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-03-31 21:38:10 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — all three findings addressed correctly; nits fixed too

AI Re-review (round 2): **APPROVE** — all three findings addressed correctly; nits fixed too