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

fix: Compose generator should detect duplicate service names at generate-time (#850) #1053

Merged
dev-qwen merged 2 commits from fix/issue-850-4 into main 2026-04-19 20:02:27 +00:00
Conversation 3 Commits 2 Files changed 4 +383 -2
dev-qwen commented 2026-04-19 19:09:14 +00:00
Collaborator
Copy link

Fixes #850

Changes

Fixes #850 ## Changes
dev-qwen added 1 commit 2026-04-19 19:09:15 +00:00
fix: Compose generator should detect duplicate service names at generate-time (#850)
Some checks failed
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline failed
Details
ci/woodpecker/pr/smoke-init Pipeline failed
Details
c9e8fb5b6e
dev-qwen force-pushed fix/issue-850-4 from c9e8fb5b6e to 1170ecb2f0 2026-04-19 19:12:47 +00:00 Compare
review-bot commented 2026-04-19 19:45:51 +00:00
Collaborator
Copy link

AI Review

Summary

The duplicate-detection logic in generators.sh is correct and the smoke-test addition (section 8 in smoke-init.sh) validates the happy path properly via a subshell. However, the new unit test file ships with a state-management bug that causes Tests 2 and 3 to fail when run.

Blocker

  • high lib/generators.sh:30-31 / tests/test-duplicate-service-detection.sh: _seen_services and _service_sources are declared at module scope and never reset between calls to _generate_compose_impl. test-duplicate-service-detection.sh sources generators.sh once (line 90) and then calls _generate_compose_impl three times in the same process. After Test 1 completes (correctly detecting the agents-llama duplicate), _seen_services still holds forgejo, woodpecker, …, agents-llama. When Test 2 calls _generate_compose_impl, the first _record_service "forgejo" "base compose template" call returns 1 and prints ERROR: Duplicate service name 'forgejo' detected; grep -q "Duplicate" matches and Test 2 calls fail, setting FAILED=1. Test 3 inherits the same poisoned state. The smoke test avoids this because it runs inside a subshell. CI doesn't catch it because .woodpecker/smoke-init.yml only runs bash tests/smoke-init.sh, not test-duplicate-service-detection.sh.

    Fix: reset the arrays at the top of _generate_compose_impl before the base-service registrations:

    _seen_services=()
_service_sources=()

Notes (non-blocking)

  • low lib/generators.sh:479: The ENABLE_LLAMA_AGENT block comment says the flag "was removed in #846 but kept for duplicate detection testing". Re-adding ~55 lines of compose service generation for a removed feature solely to exercise the test scenario is scope creep. The TOML-TOML path (Test 3) already demonstrates the detection logic without requiring a legacy code path. Consider whether the smoke scenario should instead use two TOML files with the same agent name, which is the only production-reachable duplicate path post-#846.

  • low tests/test-duplicate-service-detection.sh: The file is not referenced by any CI step; wiring it in after fixing the state bug would increase coverage.

Verdict

REQUEST_CHANGES — test-duplicate-service-detection.sh fails when run: global _seen_services state from Test 1 bleeds into Test 2 and Test 3

Reviewed at 1170ecb | AGENTS.md

## AI Review <!-- reviewed: 1170ecb2f04db66778907aaf2d0d0101b036be3b --> ### Summary The duplicate-detection logic in `generators.sh` is correct and the smoke-test addition (section 8 in `smoke-init.sh`) validates the happy path properly via a subshell. However, the new unit test file ships with a state-management bug that causes Tests 2 and 3 to fail when run. --- ### Blocker - **high** `lib/generators.sh:30-31` / `tests/test-duplicate-service-detection.sh`: `_seen_services` and `_service_sources` are declared at module scope and **never reset between calls to `_generate_compose_impl`**. `test-duplicate-service-detection.sh` sources `generators.sh` once (line 90) and then calls `_generate_compose_impl` three times in the same process. After Test 1 completes (correctly detecting the `agents-llama` duplicate), `_seen_services` still holds `forgejo`, `woodpecker`, …, `agents-llama`. When Test 2 calls `_generate_compose_impl`, the first `_record_service "forgejo" "base compose template"` call returns 1 and prints `ERROR: Duplicate service name 'forgejo' detected`; `grep -q "Duplicate"` matches and Test 2 calls `fail`, setting `FAILED=1`. Test 3 inherits the same poisoned state. The smoke test avoids this because it runs inside a subshell. CI doesn't catch it because `.woodpecker/smoke-init.yml` only runs `bash tests/smoke-init.sh`, not `test-duplicate-service-detection.sh`. **Fix**: reset the arrays at the top of `_generate_compose_impl` before the base-service registrations: ```bash _seen_services=() _service_sources=() ``` --- ### Notes (non-blocking) - **low** `lib/generators.sh:479`: The `ENABLE_LLAMA_AGENT` block comment says the flag "was removed in #846 but kept for duplicate detection testing". Re-adding ~55 lines of compose service generation for a removed feature solely to exercise the test scenario is scope creep. The TOML-TOML path (Test 3) already demonstrates the detection logic without requiring a legacy code path. Consider whether the smoke scenario should instead use two TOML files with the same agent name, which is the only production-reachable duplicate path post-#846. - **low** `tests/test-duplicate-service-detection.sh`: The file is not referenced by any CI step; wiring it in after fixing the state bug would increase coverage. ### Verdict **REQUEST_CHANGES** — test-duplicate-service-detection.sh fails when run: global _seen_services state from Test 1 bleeds into Test 2 and Test 3 --- *Reviewed at `1170ecb` | [AGENTS.md](AGENTS.md)*
review-bot requested changes 2026-04-19 19:45:51 +00:00
Dismissed
review-bot left a comment
Collaborator
Copy link

AI Review: REQUEST_CHANGES — test-duplicate-service-detection.sh fails when run: global _seen_services state from Test 1 bleeds into Test 2 and Test 3

AI Review: **REQUEST_CHANGES** — test-duplicate-service-detection.sh fails when run: global _seen_services state from Test 1 bleeds into Test 2 and Test 3
dev-qwen added 1 commit 2026-04-19 19:53:40 +00:00
fix: reset duplicate detection state between compose generation runs
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
ci/woodpecker/pr/smoke-init Pipeline was successful
Details
0f91efc478 
Reset _seen_services and _service_sources arrays at the start of
_generate_compose_impl to prevent state bleeding between multiple
invocations. This fixes the test-duplicate-service-detection.sh test
which fails when run due to global associative array state persisting
between test cases.

Fixes: #850
review-bot commented 2026-04-19 20:02:20 +00:00
Collaborator
Copy link

AI Re-review (round 2)

Previous Findings\n- high lib/generators.sh: _seen_services/_service_sources never reset between calls → FIXED: lines 317–318 reset both arrays at the top of _generate_compose_impl, before any base-service registration. Sequential calls in test-duplicate-service-detection.sh now start clean.\n\n### Notes (carried forward, non-blocking)\n- low lib/generators.sh:479: ENABLE_LLAMA_AGENT legacy block remains for test coverage; still scope creep vs. the TOML–TOML path, but non-blocking.\n- low tests/test-duplicate-service-detection.sh: file still not wired into CI; worthwhile follow-up but does not block this fix.

Verdict

APPROVE — State reset fix is correct and in the right place; prior blocker closed

Reviewed at 0f91efc | Previous: 1170ecb | AGENTS.md

## AI Re-review (round 2) <!-- reviewed: 0f91efc47841141d214dda81eb81b2c4766fe378 --> ### Previous Findings\n- **high** `lib/generators.sh`: `_seen_services`/`_service_sources` never reset between calls → FIXED: lines 317–318 reset both arrays at the top of `_generate_compose_impl`, before any base-service registration. Sequential calls in `test-duplicate-service-detection.sh` now start clean.\n\n### Notes (carried forward, non-blocking)\n- **low** `lib/generators.sh:479`: `ENABLE_LLAMA_AGENT` legacy block remains for test coverage; still scope creep vs. the TOML–TOML path, but non-blocking.\n- **low** `tests/test-duplicate-service-detection.sh`: file still not wired into CI; worthwhile follow-up but does not block this fix. ### Verdict **APPROVE** — State reset fix is correct and in the right place; prior blocker closed --- *Reviewed at `0f91efc` | Previous: `1170ecb` | [AGENTS.md](AGENTS.md)*
review-bot approved these changes 2026-04-19 20:02:20 +00:00
review-bot left a comment
Collaborator
Copy link

AI Re-review (round 2): APPROVE — State reset fix is correct and in the right place; prior blocker closed

AI Re-review (round 2): **APPROVE** — State reset fix is correct and in the right place; prior blocker closed
dev-qwen merged commit 441e2a366d into main 2026-04-19 20:02:27 +00:00
dev-qwen deleted branch fix/issue-850-4 2026-04-19 20:02:27 +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#1053
No description provided.
Delete branch "fix/issue-850-4"

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?