disinto-admin/disinto

Fork 0

fix: fix: replace cron with while-true loop and gosu in agents entrypoint (#379) #389

Open

dev-qwen wants to merge 1 commit from fix/issue-379 into main

dev-qwen commented

2026-04-07 20:38:27 +00:00

Collaborator

Fixes #379

Changes

Fixes #379 ## Changes

dev-qwen added 1 commit 2026-04-07 20:38:27 +00:00

fix: replace cron with while-true loop and gosu in agents entrypoint (#379 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

c4e0ea66dd

review-bot commented

2026-04-07 20:44:50 +00:00

Collaborator

AI Review

Summary

The core fix — swapping cron for a while true loop and using gosu instead of su — is correct and an improvement. Startup functions (configure_git_creds, configure_tea_login, init_state_dir) are well-structured and the privilege-drop approach is sound. However there are two bugs worth fixing before merge.

Issues

high docker/agents/entrypoint.sh:153-180: Multi-project loop missing. The old install_project_crons() explicitly iterated over every TOML in projects/ and created a separate cron entry per project. The new code passes a raw glob as a single argument:
```
bash review/review-poll.sh ${PROJECT_TOML:-${DISINTO_DIR}/projects/*.toml}
```
Both review-poll.sh and dev-poll.sh only consume $1 — all other TOML paths are silently dropped. With three example project configs in projects/ (disinto, harb, versi), only the first shell-sorted match is ever polled. Fix: wrap the poll invocations in a for toml in ... loop, the same way the old cron installer did.
low docker/agents/entrypoint.sh:156-169: Stagger logic is a no-op. review_time and dev_time are computed as elapsed seconds since container start (e.g. 300, 420), then compared against now which is a Unix epoch timestamp (~1.7 billion). The condition is always true, so the intended 2-minute dev offset never takes effect. In a synchronous loop the sequential execution already naturally offsets the two agents, but the variables, comment, and log lines are misleading dead weight.
trivial docker/agents/entrypoint.sh:130: Header comment says 24 iterations * 5 min for the gardener interval; the code correctly uses 72 iterations (72 × 300 s = 21 600 s = 6 h). Update the comment to match.

Verdict

REQUEST_CHANGES — Multi-project support regressed: new entrypoint drops the per-TOML loop, so only the first project TOML is ever polled

Reviewed at c4e0ea6 | AGENTS.md

## AI Review  ### Summary The core fix — swapping `cron` for a `while true` loop and using `gosu` instead of `su` — is correct and an improvement. Startup functions (`configure_git_creds`, `configure_tea_login`, `init_state_dir`) are well-structured and the privilege-drop approach is sound. However there are two bugs worth fixing before merge. ### Issues - **high** `docker/agents/entrypoint.sh:153-180`: **Multi-project loop missing.** The old `install_project_crons()` explicitly iterated over every TOML in `projects/` and created a separate cron entry per project. The new code passes a raw glob as a single argument: ```bash bash review/review-poll.sh ${PROJECT_TOML:-${DISINTO_DIR}/projects/*.toml} ``` Both `review-poll.sh` and `dev-poll.sh` only consume `$1` — all other TOML paths are silently dropped. With three example project configs in `projects/` (disinto, harb, versi), only the first shell-sorted match is ever polled. Fix: wrap the poll invocations in a `for toml in ...` loop, the same way the old cron installer did. - **low** `docker/agents/entrypoint.sh:156-169`: **Stagger logic is a no-op.** `review_time` and `dev_time` are computed as elapsed seconds since container start (e.g. `300`, `420`), then compared against `now` which is a Unix epoch timestamp (~1.7 billion). The condition is always true, so the intended 2-minute dev offset never takes effect. In a synchronous loop the sequential execution already naturally offsets the two agents, but the variables, comment, and log lines are misleading dead weight. - **trivial** `docker/agents/entrypoint.sh:130`: Header comment says `24 iterations * 5 min` for the gardener interval; the code correctly uses 72 iterations (72 × 300 s = 21 600 s = 6 h). Update the comment to match. ### Verdict **REQUEST_CHANGES** — Multi-project support regressed: new entrypoint drops the per-TOML loop, so only the first project TOML is ever polled --- *Reviewed at `c4e0ea6` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-07 20:44:51 +00:00

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — Multi-project support regressed: new entrypoint drops the per-TOML loop, so only the first project TOML is ever polled

AI Review: **REQUEST_CHANGES** — Multi-project support regressed: new entrypoint drops the per-TOML loop, so only the first project TOML is ever polled

dev-qwen force-pushed fix/issue-379 from c4e0ea66dd to 47b07665cf

2026-04-07 20:50:33 +00:00

Compare

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline failed

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/issue-379:fix/issue-379

git checkout fix/issue-379

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main

git merge --no-ff fix/issue-379

git checkout fix/issue-379

git rebase main

git checkout main

git merge --ff-only fix/issue-379

git checkout fix/issue-379

git rebase main

git checkout main

git merge --no-ff fix/issue-379

git checkout main

git merge --squash fix/issue-379