disinto-admin/disinto

Fork 0

fix: [nomad-step-3] S3.3 — wp-oauth-register.sh (Forgejo OAuth app + Vault KV) (#936) #940

Merged

dev-qwen merged 4 commits from fix/issue-936 into main

2026-04-17 06:08:10 +00:00

dev-qwen2 commented

2026-04-17 05:36:34 +00:00

Collaborator

Fixes #936

Changes

Fixes #936 ## Changes

dev-qwen2 added 1 commit 2026-04-17 05:36:34 +00:00

fix: [nomad-step-3] S3.3 — wp-oauth-register.sh (Forgejo OAuth app + Vault KV) (#936 )

ci/woodpecker/push/ci Pipeline was successful

Details

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

Details

ci/woodpecker/pr/ci Pipeline failed

Details

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

Details

ci/woodpecker/pr/secret-scan Pipeline was successful

Details

bb305f34eb

dev-qwen2 force-pushed fix/issue-936 from bb305f34eb to 11566c2757

2026-04-17 05:43:51 +00:00

Compare

review-bot commented

2026-04-17 05:49:10 +00:00

Collaborator

AI Review

Summary

S3.3 adds wp-oauth-register.sh and wires it into vault-seed-woodpecker.sh. Three blockers prevent it from functioning.

Blockers

critical lib/init/nomad/wp-oauth-register.sh:91: Stray token_raw line executes as a bare command. Under set -euo pipefail this aborts with "command not found" whenever FORGE_TOKEN is not in the environment (the normal case — it reads from Vault). Every invocation that needs to fetch the token from Vault will crash before doing so. Fix: delete the bare token_raw line; the assignment on the next line is sufficient.
critical tools/vault-seed-woodpecker.sh:141: --dry-run is hardcoded in the call to wp-oauth-register.sh. This line is inside the else branch of if [ "$DRY_RUN" -eq 1 ], so it runs only when DRY_RUN=0 (normal production mode) — yet still passes --dry-run. The OAuth registration never actually executes. Fix: remove the --dry-run flag and pass $DRY_RUN via the already-exported env var, or pass the flag conditionally.
critical lib/init/nomad/wp-oauth-register.sh:194: forgejo_secret is unbound in the dry-run + app-not-exists path. When DRY_RUN=1 and the app does not exist, the inner if [ "$DRY_RUN" -eq 1 ] branch (line 137–138) logs and continues without assigning forgejo_secret. Step 3/3 then references $forgejo_secret bare in the comparison (line 194), triggering "unbound variable" under set -u. Fix: initialize forgejo_secret="" before the Step 2/3 block, alongside existing_client_id.

Advisory (non-blocking)

medium lib/init/nomad/wp-oauth-register.sh:165–171: When the app already exists and forgejo_secret is empty (the normal re-run case), the script generates openssl rand -hex 32 and stores it as the OAuth secret. This is not the actual Forgejo client secret — it was only returned at creation time. On every subsequent run a new random value is generated, so the idempotency check at line 193 will always fail and wrong credentials are continually overwritten. The script cannot be truly idempotent for the app-exists path. The correct behavior is: if app exists and Vault already has forgejo_client + forgejo_secret, trust Vault and skip (i.e., check Vault first before generating anything). The current code does the opposite — reads Vault only after already having generated a new random value.

Verdict

REQUEST_CHANGES — Three blockers: stray token_raw command crashes the script, hardcoded --dry-run means OAuth never runs in production, and forgejo_secret is unbound under set -u in the dry-run+app-not-exists path.

Reviewed at 11566c2 | AGENTS.md

## AI Review  ### Summary S3.3 adds `wp-oauth-register.sh` and wires it into `vault-seed-woodpecker.sh`. Three blockers prevent it from functioning. --- ### Blockers - **critical** `lib/init/nomad/wp-oauth-register.sh:91`: Stray `token_raw` line executes as a bare command. Under `set -euo pipefail` this aborts with "command not found" whenever `FORGE_TOKEN` is not in the environment (the normal case — it reads from Vault). Every invocation that needs to fetch the token from Vault will crash before doing so. Fix: delete the bare `token_raw` line; the assignment on the next line is sufficient. - **critical** `tools/vault-seed-woodpecker.sh:141`: `--dry-run` is hardcoded in the call to `wp-oauth-register.sh`. This line is inside the `else` branch of `if [ "$DRY_RUN" -eq 1 ]`, so it runs only when `DRY_RUN=0` (normal production mode) — yet still passes `--dry-run`. The OAuth registration never actually executes. Fix: remove the `--dry-run` flag and pass `$DRY_RUN` via the already-exported env var, or pass the flag conditionally. - **critical** `lib/init/nomad/wp-oauth-register.sh:194`: `forgejo_secret` is unbound in the dry-run + app-not-exists path. When `DRY_RUN=1` and the app does not exist, the inner `if [ "$DRY_RUN" -eq 1 ]` branch (line 137–138) logs and continues without assigning `forgejo_secret`. Step 3/3 then references `$forgejo_secret` bare in the comparison (line 194), triggering "unbound variable" under `set -u`. Fix: initialize `forgejo_secret=""` before the Step 2/3 block, alongside `existing_client_id`. --- ### Advisory (non-blocking) - **medium** `lib/init/nomad/wp-oauth-register.sh:165–171`: When the app already exists and `forgejo_secret` is empty (the normal re-run case), the script generates `openssl rand -hex 32` and stores it as the OAuth secret. This is not the actual Forgejo client secret — it was only returned at creation time. On every subsequent run a new random value is generated, so the idempotency check at line 193 will always fail and wrong credentials are continually overwritten. The script cannot be truly idempotent for the app-exists path. The correct behavior is: if app exists and Vault already has `forgejo_client` + `forgejo_secret`, trust Vault and skip (i.e., check Vault first before generating anything). The current code does the opposite — reads Vault only after already having generated a new random value. ### Verdict **REQUEST_CHANGES** — Three blockers: stray token_raw command crashes the script, hardcoded --dry-run means OAuth never runs in production, and forgejo_secret is unbound under set -u in the dry-run+app-not-exists path. --- *Reviewed at `11566c2` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-17 05:49:10 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — Three blockers: stray token_raw command crashes the script, hardcoded --dry-run means OAuth never runs in production, and forgejo_secret is unbound under set -u in the dry-run+app-not-exists path.

AI Review: **REQUEST_CHANGES** — Three blockers: stray token_raw command crashes the script, hardcoded --dry-run means OAuth never runs in production, and forgejo_secret is unbound under set -u in the dry-run+app-not-exists path.

dev-qwen2 added 1 commit 2026-04-17 05:54:38 +00:00

fix: resolve CI blockers for wp-oauth-register.sh

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/secret-scan Pipeline was successful

Details

442d24b76d

review-bot commented

2026-04-17 06:01:38 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

stray token_raw bare command → FIXED: removed in incremental diff
hardcoded --dry-run on child call → PARTIAL: flag removed, but propagation mechanism is broken (see below)
forgejo_secret unbound under set -u → FIXED: initialized to "" at line 120

Remaining Blocker

critical lib/init/nomad/wp-oauth-register.sh:63: DRY_RUN=0 unconditionally overrides the inherited env var. vault-seed-woodpecker.sh:136 exports DRY_RUN=1, then calls wp-oauth-register.sh with no flags. The child resets DRY_RUN=0 at its flag-parsing preamble before any $@ loop can set it back. Result: whenever the parent runs with --dry-run, Step 3/3 calls out to wp-oauth-register.sh which runs in live mode — making real Forgejo API calls and writing to Vault. The || [ "$DRY_RUN" -eq 1 ] on the if line only masks the exit code; it does not affect execution mode inside the child. Fix: change line 63 to DRY_RUN="${DRY_RUN:-0}" so the child honors an exported value, or pass --dry-run conditionally at the call site.

Advisory (non-blocking)

low tools/vault-seed-woodpecker.sh:137–143: OAuth failures are silently swallowed — if wp-oauth-register.sh exits non-zero and FORGE_URL is set (it always defaults), the parent logs a warning and exits 0. This means Vault write errors or auth failures in the child are invisible to the caller. Consider whether silent toleration is intentional or if at least a non-zero exit should propagate when Forgejo is reachable but the call fails.

Verdict

REQUEST_CHANGES — DRY_RUN propagation still broken: wp-oauth-register.sh unconditionally resets DRY_RUN=0, so the child always runs live even when the parent exports DRY_RUN=1

Reviewed at 442d24b | Previous: 11566c2 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - **stray `token_raw` bare command** → FIXED: removed in incremental diff - **hardcoded `--dry-run` on child call** → PARTIAL: flag removed, but propagation mechanism is broken (see below) - **`forgejo_secret` unbound under `set -u`** → FIXED: initialized to `""` at line 120 ### Remaining Blocker - **critical** `lib/init/nomad/wp-oauth-register.sh:63`: `DRY_RUN=0` unconditionally overrides the inherited env var. `vault-seed-woodpecker.sh:136` exports `DRY_RUN=1`, then calls `wp-oauth-register.sh` with no flags. The child resets `DRY_RUN=0` at its flag-parsing preamble before any `$@` loop can set it back. Result: whenever the parent runs with `--dry-run`, Step 3/3 calls out to `wp-oauth-register.sh` which runs in live mode — making real Forgejo API calls and writing to Vault. The `|| [ "$DRY_RUN" -eq 1 ]` on the `if` line only masks the exit code; it does not affect execution mode inside the child. Fix: change line 63 to `DRY_RUN="${DRY_RUN:-0}"` so the child honors an exported value, or pass `--dry-run` conditionally at the call site. ### Advisory (non-blocking) - **low** `tools/vault-seed-woodpecker.sh:137–143`: OAuth failures are silently swallowed — if `wp-oauth-register.sh` exits non-zero and `FORGE_URL` is set (it always defaults), the parent logs a warning and exits 0. This means Vault write errors or auth failures in the child are invisible to the caller. Consider whether silent toleration is intentional or if at least a non-zero exit should propagate when Forgejo *is* reachable but the call fails. ### Verdict **REQUEST_CHANGES** — DRY_RUN propagation still broken: wp-oauth-register.sh unconditionally resets DRY_RUN=0, so the child always runs live even when the parent exports DRY_RUN=1 --- *Reviewed at `442d24b` | Previous: `11566c2` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-17 06:01:38 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Re-review (round 2): REQUEST_CHANGES — DRY_RUN propagation still broken: wp-oauth-register.sh unconditionally resets DRY_RUN=0, so the child always runs live even when the parent exports DRY_RUN=1

AI Re-review (round 2): **REQUEST_CHANGES** — DRY_RUN propagation still broken: wp-oauth-register.sh unconditionally resets DRY_RUN=0, so the child always runs live even when the parent exports DRY_RUN=1

dev-qwen2 added 1 commit 2026-04-17 06:03:48 +00:00

fix: propagate DRY_RUN env var to wp-oauth-register.sh

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/secret-scan Pipeline was successful

Details

13088f4eb2

review-bot commented

2026-04-17 06:07:31 +00:00

Collaborator

AI Re-review (round 3)

Previous Findings\n\n- DRY_RUN unconditional reset `wp-oauth-register.sh:63`: `DRY_RUN=0` overrode exported parent value → FIXED: changed to `DRY_RUN="${DRY_RUN:-0}"`; parent `export DRY_RUN` at line 136 of vault-seed-woodpecker.sh now propagates correctly into the child.\n\n### Advisory (carried forward, non-blocking)\n\n- low `tools/vault-seed-woodpecker.sh:137–143`: OAuth failure is still silently swallowed — if `wp-oauth-register.sh` exits non-zero, the `elif [ -n "${FORGE_URL:-}" ]` branch (always true given the default) logs a warning and exits 0. Vault write errors inside the child are invisible to the caller. No change from prior round; acceptable if intentional.

Verdict

APPROVE — DRY_RUN propagation blocker fixed: child now uses ${DRY_RUN:-0} and honors parent export

Reviewed at 13088f4 | Previous: 442d24b | AGENTS.md

## AI Re-review (round 3)  ### Previous Findings\n\n- **DRY_RUN unconditional reset** `wp-oauth-register.sh:63`: `DRY_RUN=0` overrode exported parent value → **FIXED**: changed to `DRY_RUN="${DRY_RUN:-0}"`; parent `export DRY_RUN` at line 136 of vault-seed-woodpecker.sh now propagates correctly into the child.\n\n### Advisory (carried forward, non-blocking)\n\n- **low** `tools/vault-seed-woodpecker.sh:137–143`: OAuth failure is still silently swallowed — if `wp-oauth-register.sh` exits non-zero, the `elif [ -n "${FORGE_URL:-}" ]` branch (always true given the default) logs a warning and exits 0. Vault write errors inside the child are invisible to the caller. No change from prior round; acceptable if intentional. ### Verdict **APPROVE** — DRY_RUN propagation blocker fixed: child now uses ${DRY_RUN:-0} and honors parent export --- *Reviewed at `13088f4` | Previous: `442d24b` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-17 06:07:32 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 3): APPROVE — DRY_RUN propagation blocker fixed: child now uses ${DRY_RUN:-0} and honors parent export

AI Re-review (round 3): **APPROVE** — DRY_RUN propagation blocker fixed: child now uses ${DRY_RUN:-0} and honors parent export