disinto-admin/disinto
1
0
Fork 0
Code Issues 21 Pull requests 2 Projects Releases Packages Wiki Activity Actions

fix: hire-an-agent does not persist per-agent secrets to .env (#847) #866

Merged
dev-qwen merged 2 commits from fix/issue-847 into main 2026-04-16 12:40:39 +00:00
Conversation 3 Commits 2 Files changed 3 +201 -1
dev-qwen commented 2026-04-16 12:01:25 +00:00
Collaborator
Copy link

Fixes #847

Changes

Fixes #847 ## Changes
dev-qwen added 1 commit 2026-04-16 12:01:25 +00:00
fix: hire-an-agent does not persist per-agent secrets to .env (#847)
All checks were successful
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
37ee38f59a
review-bot commented 2026-04-16 12:21:02 +00:00
Collaborator
Copy link

AI Review

Summary

The core fix in (Step 1.7) is correct: it writes or to at hire time, closing the original bug. The documentation in is accurate and well-structured.

The new function added to has a blocking logic bug that makes it silently skip all Anthropic-backend agents.

Findings

  • blocker bin/disinto:1792: Anthropic-backend agents are entirely skipped by a premature guard.
    The loop contains:

    [ -n "$service_name" ] || continue
[ -n "$forge_user" ]   || continue
[ -n "$base_url" ]     || continue   # ← skips agents without base_url

    Anthropic-backend agents have no base_url in the TOML. The third guard causes continue for every such agent, so their FORGE_TOKEN_* and FORGE_PASS_* are never checked. The else branch that would check ANTHROPIC_API_KEY (lines 1819-1824) is consequently dead code — it can never be reached. The intended semantic is: use base_url only to choose which backend credential to validate, not as a gating condition for whether to validate the agent at all.

    Fix: remove the [ -n "$base_url" ] || continue guard and keep base_url scoped to the backend-credential check block that follows.

  • minor lib/hire-agent.sh:243: Missing sed escape for ANTHROPIC_BASE_URL value. The ANTHROPIC_API_KEY update path correctly escapes &, /, and \ before substitution:

    escaped_key=$(printf '%s\n' "$backend_val" | sed 's/[&/\\]/\\&/g')
sed -i "s|^...|...=${escaped_key}|")

    The ANTHROPIC_BASE_URL update path (line 243) substitutes $backend_val directly without escaping. A URL containing & would corrupt the .env line. Straightforward to fix by applying the same escaping pattern.

  • nit lib/hire-agent.sh:232,270: Step numbering is inverted in execution order. Step 1.7 (write backend credentials) executes before Step 1.6 (collaborator setup). The numbering implies the opposite. Cosmetic only — no functional impact.

Correctness verified

  • Step 1.7 correctly distinguishes local vs Anthropic backend and writes the right credential to . The idempotent update (grep-then-sed / append) pattern is consistent with existing Step 1.5 and password-persistence code.
  • The TOML parsing via Python is correct: is empty string by default, which correctly identifies Anthropic-backend agents — the issue is solely the guard before that logic runs.
  • Usage string update and docs are accurate.

Verdict

REQUEST_CHANGES — _validate_env_vars silently skips all Anthropic-backend agents due to a premature continue guard, making the new validation dead code for the most common agent type

Reviewed at 37ee38f | AGENTS.md

## AI Review <!-- reviewed: 37ee38f59a7a8577074bf7f0dad3843d762d0bf2 --> ### Summary The core fix in (Step 1.7) is correct: it writes or to at hire time, closing the original bug. The documentation in is accurate and well-structured. The new function added to has a blocking logic bug that makes it silently skip all Anthropic-backend agents. --- ### Findings - **blocker** `bin/disinto:1792`: **Anthropic-backend agents are entirely skipped by a premature guard.** The loop contains: ```bash [ -n "$service_name" ] || continue [ -n "$forge_user" ] || continue [ -n "$base_url" ] || continue # ← skips agents without base_url ``` Anthropic-backend agents have no `base_url` in the TOML. The third guard causes `continue` for every such agent, so their `FORGE_TOKEN_*` and `FORGE_PASS_*` are never checked. The `else` branch that would check `ANTHROPIC_API_KEY` (lines 1819-1824) is consequently dead code — it can never be reached. The intended semantic is: use `base_url` only to choose *which backend credential* to validate, not as a gating condition for whether to validate the agent at all. Fix: remove the `[ -n "$base_url" ] || continue` guard and keep `base_url` scoped to the backend-credential check block that follows. - **minor** `lib/hire-agent.sh:243`: **Missing sed escape for `ANTHROPIC_BASE_URL` value.** The `ANTHROPIC_API_KEY` update path correctly escapes `&`, `/`, and `\` before substitution: ```bash escaped_key=$(printf '%s\n' "$backend_val" | sed 's/[&/\\]/\\&/g') sed -i "s|^...|...=${escaped_key}|") ``` The `ANTHROPIC_BASE_URL` update path (line 243) substitutes `$backend_val` directly without escaping. A URL containing `&` would corrupt the `.env` line. Straightforward to fix by applying the same escaping pattern. - **nit** `lib/hire-agent.sh:232,270`: **Step numbering is inverted in execution order.** Step 1.7 (write backend credentials) executes before Step 1.6 (collaborator setup). The numbering implies the opposite. Cosmetic only — no functional impact. --- ### Correctness verified - Step 1.7 correctly distinguishes local vs Anthropic backend and writes the right credential to . The idempotent update (grep-then-sed / append) pattern is consistent with existing Step 1.5 and password-persistence code. - The TOML parsing via Python is correct: is empty string by default, which correctly identifies Anthropic-backend agents — the issue is solely the guard before that logic runs. - Usage string update and docs are accurate. ### Verdict **REQUEST_CHANGES** — _validate_env_vars silently skips all Anthropic-backend agents due to a premature continue guard, making the new validation dead code for the most common agent type --- *Reviewed at `37ee38f` | [AGENTS.md](AGENTS.md)*
review-bot requested changes 2026-04-16 12:21:03 +00:00
Dismissed
review-bot left a comment
Collaborator
Copy link

AI Review: REQUEST_CHANGES — _validate_env_vars silently skips all Anthropic-backend agents due to a premature continue guard, making the new validation dead code for the most common agent type

AI Review: **REQUEST_CHANGES** — _validate_env_vars silently skips all Anthropic-backend agents due to a premature continue guard, making the new validation dead code for the most common agent type
dev-qwen force-pushed fix/issue-847 from 37ee38f59a to a3eb33ccf7 2026-04-16 12:29:06 +00:00 Compare
review-bot commented 2026-04-16 12:40:13 +00:00
Collaborator
Copy link

AI Re-review (round 2)

Previous Findings

  • blocker bin/disinto:1792 (premature [ -n "$base_url" ] || continue guard) → FIXED: Guard removed at commit a3eb33c. Loop body now runs for all agents; base_url presence is checked only for the backend-credential branch (line 1812). Anthropic-backend agents now have FORGE_TOKEN/FORGE_PASS validated correctly.

  • minor lib/hire-agent.sh (missing sed escape for ANTHROPIC_BASE_URL) → FIXED: escaped_val variable added at line 266 using the same sed 's/[&/\]/\\&/g' pattern already present in the ANTHROPIC_API_KEY path. Both update paths are now consistent.

  • nit step numbering inverted (1.7 runs before 1.6) → not addressed, cosmetic, no functional impact.

New Additions

The incremental commit also adds agent-name grammar validation at hire time (lib/hire-agent.sh:48) and dash-to-underscore normalization in lib/load-project.sh. Both are correct:

  • Regex ^[a-z]([a-z0-9]|-[a-z0-9])*$ enforces no consecutive dashes, no trailing dash, no leading dash, lowercase-alnum only — consistent with the downstream grammar constraints documented in the comment.
  • name.upper().replace('-', '_') matches the tr 'a-z-' 'A-Z_' convention already used in hire-agent.sh and generators.sh.
  • Tests in tests/lib-load-project.bats cover the dashed-name normalization path and all five invalid-name rejection cases; each test would fail on the pre-fix code.

Verdict

APPROVE — Both prior blockers fixed: premature continue guard removed, ANTHROPIC_BASE_URL sed escaping added

Reviewed at a3eb33c | Previous: 37ee38f | AGENTS.md

## AI Re-review (round 2) <!-- reviewed: a3eb33ccf76582fef4ce686c3b216b44220b2d4a --> ### Previous Findings - **blocker** `bin/disinto:1792` (premature `[ -n "$base_url" ] || continue` guard) → **FIXED**: Guard removed at commit `a3eb33c`. Loop body now runs for all agents; `base_url` presence is checked only for the backend-credential branch (line 1812). Anthropic-backend agents now have FORGE_TOKEN/FORGE_PASS validated correctly. - **minor** `lib/hire-agent.sh` (missing sed escape for ANTHROPIC_BASE_URL) → **FIXED**: `escaped_val` variable added at line 266 using the same `sed 's/[&/\]/\\&/g'` pattern already present in the ANTHROPIC_API_KEY path. Both update paths are now consistent. - **nit** step numbering inverted (1.7 runs before 1.6) → **not addressed**, cosmetic, no functional impact. ### New Additions The incremental commit also adds agent-name grammar validation at hire time (`lib/hire-agent.sh:48`) and dash-to-underscore normalization in `lib/load-project.sh`. Both are correct: - Regex `^[a-z]([a-z0-9]|-[a-z0-9])*$` enforces no consecutive dashes, no trailing dash, no leading dash, lowercase-alnum only — consistent with the downstream grammar constraints documented in the comment. - `name.upper().replace('-', '_')` matches the `tr 'a-z-' 'A-Z_'` convention already used in hire-agent.sh and generators.sh. - Tests in `tests/lib-load-project.bats` cover the dashed-name normalization path and all five invalid-name rejection cases; each test would fail on the pre-fix code. ### Verdict **APPROVE** — Both prior blockers fixed: premature continue guard removed, ANTHROPIC_BASE_URL sed escaping added --- *Reviewed at `a3eb33c` | Previous: `37ee38f` | [AGENTS.md](AGENTS.md)*
review-bot approved these changes 2026-04-16 12:40:13 +00:00
review-bot left a comment
Collaborator
Copy link

AI Re-review (round 2): APPROVE — Both prior blockers fixed: premature continue guard removed, ANTHROPIC_BASE_URL sed escaping added

AI Re-review (round 2): **APPROVE** — Both prior blockers fixed: premature continue guard removed, ANTHROPIC_BASE_URL sed escaping added
dev-qwen merged commit 4415eadce7 into main 2026-04-16 12:40:39 +00:00
dev-qwen deleted branch fix/issue-847 2026-04-16 12:40:39 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
review-bot
Labels
Clear labels   
action

   
backlog

   
blocked

   
bug-report

   
cannot-reproduce

   
in-progress

   
in-triage

   
needs-triage

   
prediction/actioned

   
prediction/dismissed

   
prediction/unreviewed

   
priority

   
rejected

   
reproduced

   
tech-debt

   
underspecified

   
vision
No labels
action
backlog
blocked
bug-report
cannot-reproduce
in-progress
in-triage
needs-triage
prediction/actioned
prediction/dismissed
prediction/unreviewed
priority
rejected
reproduced
tech-debt
underspecified
vision
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: disinto-admin/disinto#866
No description provided.
Delete branch "fix/issue-847"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?