Tests for the routing policy; honour --backend on route --role reviewer - #5
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23e2b5dade
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rr = resolveRoute({ ...argv, role: "reviewer", backend: r.backend, _composed: true, depth: undefined, diff: undefined }); | ||
| const reviewers = reviewComposition(depth, author, kind, argv.backend && argv.backend !== true ? String(argv.backend) : null).map((r) => { | ||
| // a slot the user pinned with --backend keeps it even when that CLI is missing; composed slots may fall back | ||
| const rr = resolveRoute({ ...argv, role: "reviewer", backend: r.backend, _composed: !r.pinned, depth: undefined, diff: undefined }); |
There was a problem hiding this comment.
When an explicit reviewer backend is unavailable, setting _composed: false correctly prevents fallback, but cmdRoute then discards resolveRoute().why and only preserves messages containing falls back. For example, route --role reviewer --parent codex --backend claude without the Claude CLI returns an external invocation that cannot run, with no indication that the CLI is missing. Preserve the not installed warning in the reviewer slot so callers can act on it.
Useful? React with 👍 / 👎.
| // `pinned` is an explicit --backend: it takes slot A, and the alternation runs from there. | ||
| // Before it was honoured, `route --role reviewer --backend X` was silently overridden here. | ||
| function reviewComposition(depth, author, kind, pinned = null) { | ||
| const first = pinned || otherBackend(author); |
There was a problem hiding this comment.
Allowing --backend to select slot A introduces a new exception to the documented independence policy: references/review.md:7-15 still says only CLI availability can move A off the family opposite the author, while this branch permits an explicit same-family reviewer. Update the policy documentation and the contradictory comment immediately above this function so coordinators do not continue treating cross-family slot A as guaranteed.
Useful? React with 👍 / 👎.
…бход lock'ов По второму раунду ревью PR #4 (Macroscope High ×3): - внешний и нативный писатель одного репозитория считали слоты под разными замками: agent-run теперь берёт и репозиторный mkdir-мьютекс, общий с agent-wt lock; порядок машинный → репозиторный, agent-wt берёт только второй — цикла нет; - ENOENT между EEXIST и чтением pid (держатель отпустил) — повтор, не падение; - agent-wt считает lock'и всех linked worktree из общего .git, а не только каталога <repo>.worktrees: писатель `agent-run --cwd` может сидеть где угодно; - writeJson через tmp + rename: читатель никогда не видит обрезанный meta.json. tests/caps.sh: трупы обоих мьютексов, process-lock в чужом worktree — 47 проверок. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…остав панели Решения «кто, где, сколько ревьюеров» жили без единой проверки; каждая правка политики проверялась руками. Стенд детерминирован: --parent, PATH с заглушками CLI, DELEGATE_KIT_HOME с config.json, синтетические diff'ы ровно на границах порогов (399/400 строк, 10 файлов, 2 модуля, risk zone по пути и по строке, lockfile как шум, --kind mechanical). 43 проверки, шаг в CI. Стенд вскрыл дефект: `route --role reviewer --backend X` молча затирался композицией панели. Теперь явный --backend занимает слот A, чередование идёт от него, и закреплённый слот не переезжает на другое семейство при отсутствии CLI — как и обещает external.md для явных флагов. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…о --backend на слоте A По ревью PR #5 (Codex P2 ×2): закреплённый --backend без CLI возвращал невыполнимый внешний вызов без предупреждения — note теперь сохраняет «not installed»; review.md называл единственной причиной сдвига слота A отсутствие CLI — добавлен явный --backend как выбор пользователя. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
23e2b5d to
618f0a5
Compare
1 participant
Stacked on #4 (base branch
fix/writer-cap; retargets tomainwhen #4 merges).Problem
The decisions
routemakes — family per role, native vs external, CLI-missing fallback, review depth from a diff, panel composition — had no tests. Every policy change was checked by hand.Change
tests/route.sh: 43 checks, deterministic without any model call (--parent, PATH with CLI shims,DELEGATE_KIT_HOMEwithconfig.json, synthetic diffs exactly at the depth thresholds: 399/400 lines, 10 files, 2 modules, risk zone by path and by added line, lockfile as noise,--kind mechanical, explicit--depthas the user's yes). CI step added.route --role reviewer --backend Xwas silently overridden by the panel composition. Now an explicit--backendtakes slot A, the alternation runs from it, and a pinned slot stays on its family even when that CLI is missing — asexternal.mdpromises for explicit flags.Checks
bash skills/delegate-kit/tests/route.sh— 43/43bash skills/delegate-kit/tests/caps.sh— 43/43,delivery.sh— 25/25node --check,bash -n🤖 Generated with Claude Code