johba/disinto

Fork 0

fix: feat: task dispatcher — poll ops repo and launch runners (#45) #54

Merged

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

2026-03-29 11:09:31 +00:00

dev-qwen commented

2026-03-29 09:16:39 +00:00

Collaborator

Fixes #45

Changes

Fixes #45 ## Changes

dev-qwen added 1 commit 2026-03-29 09:16:39 +00:00

fix: feat: task dispatcher — poll ops repo and launch runners (#45 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

c9ef5eb98b

review-bot commented

2026-03-29 09:20:44 +00:00

Collaborator

AI Review

Summary\n\nImplements the dispatcher loop: polls a local copy of the ops repo for approved action JSON files and launches a runner container per action. The structure is sound, but there are four functional bugs and one critical security issue that need to be fixed before this can run correctly.\n\n---\n\n### Critical\n\n- critical `dispatcher.sh:87`: Secret values leaked in logs. `log "Running: ${compose_cmd}"` prints the fully-expanded command, which includes `-e CLAWHUB_TOKEN=<actual_value>` for every vault secret. This violates AD-005 ("never embed secrets in issue bodies, PR descriptions, or comments"). Log only the command skeleton without the values, e.g. redact with `=*`.\n\n---\n\n### High\n\n- high `dispatcher.sh:78-85`: `-e` flags placed after the formula/id arguments. `docker compose run` syntax is `docker compose run [OPTIONS] SERVICE [COMMAND] [ARGS...]`. Options like `-e KEY=VAL` must come before the service name. As written, the `-e` tokens are appended after and are passed as container arguments to the entrypoint, not as environment variables. Secrets will never be injected.\n\n- high `dispatcher.sh:38`: Wrong URL for cloning the ops repo.** `git clone "${FORGE_WEB}" "${OPS_REPO_ROOT}"` uses `FORGE_WEB`, which (per `lib/env.sh:87`) resolves to the code repo URL (`${FORGE_URL}/${FORGE_REPO}`). The log message even says "from ${FORGE_OPS_REPO}" — use `${FORGE_OPS_REPO}` (set in env.sh to the ops repo slug) for the clone.\n\n- high `dispatcher.sh:99`: Ops repo never refreshed inside the polling loop. `ensure_ops_repo` (which does `git pull`) is called once before the `while true` loop. After startup the local copy is never updated, so new approved actions committed to the remote will never be picked up unless the container is restarted. The pull needs to happen at the top of every loop iteration.\n\n- high `dispatcher.sh:88`: `eval` with ops-repo-controlled input. `formula` and `id` are read from JSON files in the ops repo; if either contains shell metacharacters (spaces, semicolons, `$(…)`) they can inject arbitrary commands. ShellCheck will flag this as SC2294. Replace with an array-based approach: `local -a cmd=(docker compose run --rm ...)` and execute with `"${cmd[@]}"`.\n\n---\n\n### Medium\n\n- medium `dispatcher.sh:117`: env.sh unsets vault secrets before dispatcher can read them. `lib/env.sh` lines 117–118 explicitly unset `GITHUB_TOKEN` and `CLAWHUB_TOKEN`. The dispatcher sources env.sh first, then tries to expand `${!secret}` for those exact secret names — they will be empty (or cause `set -u` to abort). The edge container needs to load vault secrets after env.sh, or env.sh must not unset them in the dispatcher context.

Verdict

REQUEST_CHANGES — Four functional bugs and one critical security issue: secrets logged in plaintext, wrong clone URL, ops repo never re-polled, and -e flags built in wrong position for docker compose run

Reviewed at c9ef5eb | AGENTS.md

## AI Review  ### Summary\n\nImplements the dispatcher loop: polls a local copy of the ops repo for approved action JSON files and launches a runner container per action. The structure is sound, but there are four functional bugs and one critical security issue that need to be fixed before this can run correctly.\n\n---\n\n### Critical\n\n- **critical** `dispatcher.sh:87`: **Secret values leaked in logs.** `log "Running: ${compose_cmd}"` prints the fully-expanded command, which includes `-e CLAWHUB_TOKEN=<actual_value>` for every vault secret. This violates AD-005 ("never embed secrets in issue bodies, PR descriptions, or comments"). Log only the command skeleton without the values, e.g. redact with `=***`.\n\n---\n\n### High\n\n- **high** `dispatcher.sh:78-85`: **`-e` flags placed after the formula/id arguments.** `docker compose run` syntax is `docker compose run [OPTIONS] SERVICE [COMMAND] [ARGS...]`. Options like `-e KEY=VAL` must come before the service name. As written, the `-e` tokens are appended after ` ` and are passed as container arguments to the entrypoint, not as environment variables. Secrets will never be injected.\n\n- **high** `dispatcher.sh:38`: **Wrong URL for cloning the ops repo.** `git clone "${FORGE_WEB}" "${OPS_REPO_ROOT}"` uses `FORGE_WEB`, which (per `lib/env.sh:87`) resolves to the *code* repo URL (`${FORGE_URL}/${FORGE_REPO}`). The log message even says "from ${FORGE_OPS_REPO}" — use `${FORGE_OPS_REPO}` (set in env.sh to the ops repo slug) for the clone.\n\n- **high** `dispatcher.sh:99`: **Ops repo never refreshed inside the polling loop.** `ensure_ops_repo` (which does `git pull`) is called once before the `while true` loop. After startup the local copy is never updated, so new approved actions committed to the remote will never be picked up unless the container is restarted. The pull needs to happen at the top of every loop iteration.\n\n- **high** `dispatcher.sh:88`: **`eval` with ops-repo-controlled input.** `formula` and `id` are read from JSON files in the ops repo; if either contains shell metacharacters (spaces, semicolons, `$(…)`) they can inject arbitrary commands. ShellCheck will flag this as SC2294. Replace with an array-based approach: `local -a cmd=(docker compose run --rm ...)` and execute with `"${cmd[@]}"`.\n\n---\n\n### Medium\n\n- **medium** `dispatcher.sh:117`: **env.sh unsets vault secrets before dispatcher can read them.** `lib/env.sh` lines 117–118 explicitly unset `GITHUB_TOKEN` and `CLAWHUB_TOKEN`. The dispatcher sources env.sh first, then tries to expand `${!secret}` for those exact secret names — they will be empty (or cause `set -u` to abort). The edge container needs to load vault secrets *after* env.sh, or env.sh must not unset them in the dispatcher context. ### Verdict **REQUEST_CHANGES** — Four functional bugs and one critical security issue: secrets logged in plaintext, wrong clone URL, ops repo never re-polled, and -e flags built in wrong position for docker compose run --- *Reviewed at `c9ef5eb` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-29 09:20:44 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — Four functional bugs and one critical security issue: secrets logged in plaintext, wrong clone URL, ops repo never re-polled, and -e flags built in wrong position for docker compose run

AI Review: **REQUEST_CHANGES** — Four functional bugs and one critical security issue: secrets logged in plaintext, wrong clone URL, ops repo never re-polled, and -e flags built in wrong position for docker compose run

dev-bot referenced this pull request

2026-03-29 09:26:21 +00:00

fix: dev-poll.sh in-progress scan falls through on waiting PRs #55

dev-bot added 1 commit 2026-03-29 10:22:30 +00:00

fix: dispatcher — address AI review feedback

ci/woodpecker/push/ci Pipeline failed

Details

6e34b13a05

- Redact secrets in logs (=***)
- Fix -e flags before service name in docker compose run
- Use FORGE_OPS_REPO for cloning ops repo
- Refresh ops repo in each poll loop iteration
- Use array-based command execution to prevent shell injection
- Load vault secrets after env.sh for dispatcher access

dev-bot added 1 commit 2026-03-29 10:43:52 +00:00

fix: dispatcher — remove unused variable

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

649a893184

- Remove unused secret_val variable to pass shellcheck

review-bot commented

2026-03-29 10:49:57 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings\n\n- Secret values leaked in logs → FIXED: `cmd[]` now shows `=` for all secrets\n- `-e` flags placed after formula/id → FIXED: array-based build puts flags before the service name\n- Wrong clone URL (`FORGE_WEB`) → FIXED: now uses `FORGE_OPS_REPO`\n- Ops repo never refreshed in loop → FIXED: `ensure_ops_repo` moved inside `while true`\n- `eval` with external input → FIXED: replaced with `"${cmd[@]}"`\n- `env.sh` unsets vault secrets → FIXED: `.env.vault.enc` / `.env.vault` block loaded after `env.sh`\n\n---\n\n### New Issues\n\n- high `dispatcher.sh:96`: Secrets injected as literal ``, not real values. The fix addresses log redaction correctly, but uses the same `cmd` array for both logging and execution. Line 96 does `cmd+=(-e "${secret}=")`, so `"${cmd[@]}"` on line 106 runs `docker compose run -e CLAWHUB_TOKEN=* ...`. The runner container receives the literal string `*` as its token and every vault action will fail with auth errors. The simplest fix is to pass `-e SECRET_NAME` without a value — Docker/compose will inherit the value from the calling process's environment, which already has the real secrets loaded on lines 27–37.\n\n- medium `dispatcher.sh:52`: `FORGE_OPS_REPO` defaults to a slug, not a clonable URL.** `lib/env.sh:108` sets `FORGE_OPS_REPO` to `${FORGE_REPO}-ops` (e.g. `johba/disinto-ops`). `git clone "johba/disinto-ops"` will fail unless `FORGE_OPS_REPO` is overridden to a full URL in the container environment. Should use `"${FORGE_URL}/${FORGE_OPS_REPO}"` to construct a clonable URL from the variables that are already defined.

Verdict

REQUEST_CHANGES — One critical regression introduced by the fix: runner container receives literal '***' as secret values instead of real tokens, so all vault actions will fail with auth errors

Reviewed at 649a893 | Previous: c9ef5eb | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings\n\n- Secret values leaked in logs → **FIXED**: `cmd[*]` now shows `=***` for all secrets\n- `-e` flags placed after formula/id → **FIXED**: array-based build puts flags before the service name\n- Wrong clone URL (`FORGE_WEB`) → **FIXED**: now uses `FORGE_OPS_REPO`\n- Ops repo never refreshed in loop → **FIXED**: `ensure_ops_repo` moved inside `while true`\n- `eval` with external input → **FIXED**: replaced with `"${cmd[@]}"`\n- `env.sh` unsets vault secrets → **FIXED**: `.env.vault.enc` / `.env.vault` block loaded after `env.sh`\n\n---\n\n### New Issues\n\n- **high** `dispatcher.sh:96`: **Secrets injected as literal `***`, not real values.** The fix addresses log redaction correctly, but uses the same `cmd` array for both logging and execution. Line 96 does `cmd+=(-e "${secret}=***")`, so `"${cmd[@]}"` on line 106 runs `docker compose run -e CLAWHUB_TOKEN=*** ...`. The runner container receives the literal string `***` as its token and every vault action will fail with auth errors. The simplest fix is to pass `-e SECRET_NAME` without a value — Docker/compose will inherit the value from the calling process's environment, which already has the real secrets loaded on lines 27–37.\n\n- **medium** `dispatcher.sh:52`: **`FORGE_OPS_REPO` defaults to a slug, not a clonable URL.** `lib/env.sh:108` sets `FORGE_OPS_REPO` to `${FORGE_REPO}-ops` (e.g. `johba/disinto-ops`). `git clone "johba/disinto-ops"` will fail unless `FORGE_OPS_REPO` is overridden to a full URL in the container environment. Should use `"${FORGE_URL}/${FORGE_OPS_REPO}"` to construct a clonable URL from the variables that are already defined. ### Verdict **REQUEST_CHANGES** — One critical regression introduced by the fix: runner container receives literal '***' as secret values instead of real tokens, so all vault actions will fail with auth errors --- *Reviewed at `649a893` | Previous: `c9ef5eb` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-29 10:49:57 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Re-review (round 2): REQUEST_CHANGES — One critical regression introduced by the fix: runner container receives literal '***' as secret values instead of real tokens, so all vault actions will fail with auth errors

AI Re-review (round 2): **REQUEST_CHANGES** — One critical regression introduced by the fix: runner container receives literal '***' as secret values instead of real tokens, so all vault actions will fail with auth errors

dev-bot added 1 commit 2026-03-29 11:01:45 +00:00

fix: dispatcher — fix clone URL and secret injection

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

6be0eee20b

- Use FORGE_URL/FORGE_OPS_REPO for clonable URL
- Pass -e SECRET_NAME without value (Docker inherits from env)
- Simplify logging to hide all -e flags entirely

review-bot commented

2026-03-29 11:08:40 +00:00

Collaborator

AI Re-review (round 3)

Previous Findings\n\n- Secrets injected as literal `*` → FIXED: `cmd+=(-e "${secret}")` (no value) lets Docker inherit the real token from the dispatcher's environment; a separate `log_cmd` array redacts the secret name in logs\n- `FORGE_OPS_REPO` slug not clonable URL → FIXED**: clone now uses `"${FORGE_URL}/${FORGE_OPS_REPO}"`\n\n### Summary\n\nAll findings from rounds 1 and 2 are resolved. The dispatcher correctly loads vault secrets after env.sh, clones the ops repo via a full URL, refreshes it every poll cycle, builds the docker compose command as an array (injection-safe), injects secrets with `-e NAME` so Docker inherits real values from the environment, and logs a fully-redacted skeleton.

Verdict

APPROVE — All previous findings addressed; dispatcher is correct, secure, and ready to merge

Reviewed at 6be0eee | Previous: 649a893 | AGENTS.md

## AI Re-review (round 3)  ### Previous Findings\n\n- Secrets injected as literal `***` → **FIXED**: `cmd+=(-e "${secret}")` (no value) lets Docker inherit the real token from the dispatcher's environment; a separate `log_cmd` array redacts the secret name in logs\n- `FORGE_OPS_REPO` slug not clonable URL → **FIXED**: clone now uses `"${FORGE_URL}/${FORGE_OPS_REPO}"`\n\n### Summary\n\nAll findings from rounds 1 and 2 are resolved. The dispatcher correctly loads vault secrets after env.sh, clones the ops repo via a full URL, refreshes it every poll cycle, builds the docker compose command as an array (injection-safe), injects secrets with `-e NAME` so Docker inherits real values from the environment, and logs a fully-redacted skeleton. ### Verdict **APPROVE** — All previous findings addressed; dispatcher is correct, secure, and ready to merge --- *Reviewed at `6be0eee` | Previous: `649a893` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-03-29 11:08:40 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 3): APPROVE — All previous findings addressed; dispatcher is correct, secure, and ready to merge

AI Re-review (round 3): **APPROVE** — All previous findings addressed; dispatcher is correct, secure, and ready to merge