PR Review Summary

(0) Total Issues | Risk: Low

Re-review Summary

This is a re-review triggered by pull_request:synchronize. The head SHA (0e30315) matches the previously approved state. No new PR-specific code changes since the last approval.

This PR has been thoroughly reviewed across 10 prior review cycles, all resulting in approval. The comprehensive review history includes:

Prior Feedback Status

All 5 prior inline threads are resolved:

🕐 Pending Recommendations (0)

All prior feedback has been addressed.

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The architecture is clean, error handling is appropriate for CLI context, tests cover core scenarios, CI integration is correct, and documentation accurately reflects the new behavior. All prior feedback has been addressed through 10 comprehensive review cycles. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Delta Re-review Summary

This is a delta-scoped re-review triggered by pull_request:synchronize. The single commit since the last approved review is:

• 9c285249d — fix(ci): limit Turbo concurrency to prevent OOM on cache-miss runs

Delta scope: .github/workflows/ci.yml only (+3/-2 lines)

Technical Assessment:

• ✅ Correct fix — Limiting Turbo concurrency to 8 on a 4-vCPU runner is a reasonable heuristic to prevent OOM kills
• ✅ Well-documented — Comment clearly explains why the limit exists (cache-miss runs with concurrent Next.js builds)
• ✅ No functional impact — This affects CI performance/stability only, not the setup module behavior
• ✅ Idiomatic — --concurrency=N is the standard Turborepo flag for limiting parallelism

Prior Feedback Status

All 5 prior inline threads are resolved:

🕐 Pending Recommendations (0)

All prior feedback has been addressed.

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The PR has been thoroughly reviewed across 11 prior review cycles with all substantive issues either fixed or appropriately declined. This sync event contains a single CI fix that limits Turbo concurrency to prevent OOM kills — a straightforward operational improvement with no impact on the setup module's functionality. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Delta Re-review Summary

This is a delta-scoped re-review following 1 new commit since the last approved review:

• c344a827c — cleanup: remove orphaned scripts/setup.sh

Delta scope: scripts/setup.sh deletion only (−222 lines)

Assessment:

• ✅ Correct cleanup — The file was orphaned when package.json was updated to use node scripts/setup-dev.js instead of sh scripts/setup.sh
• ✅ Already noted — This cleanup was listed in the PR description under "Known cleanup" as work to be done
• ✅ No functional impact — The file was no longer referenced by any script or workflow
• ✅ Completes the PR — Removes the last vestige of the old bash-based setup flow

Prior Feedback Status

All 5 prior inline threads are resolved:

🕐 Pending Recommendations (0)

All prior feedback has been addressed.

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The PR has been thoroughly reviewed across 12 review cycles with all substantive issues either fixed or appropriately declined. This final commit removes the orphaned scripts/setup.sh that was no longer referenced after the unified setup module was introduced — completing the cleanup. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Delta Re-review Summary

This is a delta-scoped re-review following 1 new commit since the last approval:

• cee4e1eb2 — fix: separate exec timeout from port conflict in setup, add non-compose fix suggestion

Delta scope: packages/agents-core/src/setup/setup.ts only (+39/-29 lines)

Technical Assessment

Change 1: Non-compose container fix suggestion (lines 200-206)

const standaloneContainers = occupiedPorts
  .filter(({ project }) => project === null)
  .map(({ container }) => container);
for (const name of standaloneContainers) {
  fixes.push(`  docker rm -f ${name}`);
}

• ✅ Correct fix — Addresses the UX gap identified in the PR description where standalone containers (not managed by Docker Compose) received no actionable fix suggestion
• ✅ Safe filtering — Only targets containers where project === null (no compose label)
• ✅ Consistent formatting — Uses the same indentation pattern as the compose project suggestions

Change 2: Timeout separated from port conflict (lines 340-365)

• ✅ Correct logic — isTimeout is now explicitly checked before treating the error as a port conflict
• ✅ Timeout increased — 60s → 120s is reasonable for first-run Docker image pulls
• ✅ Graceful recovery — On timeout, health checks still run (containers may have started despite exec timeout)

Change 3: Health checks moved outside try/catch (lines 367-385)

• ✅ Cleaner control flow — Health polling now always runs after successful docker compose up -d or timeout (but not after port conflict return)
• ✅ No behavioral regression — The dual-failure safeguard (process.exit(1) if both DBs fail health checks) is preserved

Test coverage verified:

it('should continue when docker compose times out during startup', async () => {
  // Mocks killed: true, signal: SIGTERM
  // Verifies migrations still run after timeout
});

Prior Feedback Status

All 5 prior inline threads are resolved:

🕐 Pending Recommendations (0)

All prior feedback has been addressed.

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The PR has been thoroughly reviewed across 13 prior review cycles with all substantive issues either fixed or appropriately declined. This latest commit correctly addresses the two remaining findings from the PR description's test plan: (1) exec timeout is no longer misclassified as a port conflict, and (2) non-compose containers now get an actionable docker rm -f fix suggestion. Clean implementation, no issues. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Delta Re-review Summary

This is a delta-scoped re-review following 1 new commit since the last approved review:

• 22d593481 — fix: detect localhost DB URLs before skipping Docker, add SpiceDB health polling

Delta scope: 2 files, +68/-13 lines

Human Feedback Resolution

Technical Assessment

1. Localhost URL detection (lines 48-58):

• isLocalDatabaseUrl() regex /[@/](localhost|127\.0\.0\.1)[:/]/ correctly matches localhost URLs
• hasExternalDatabases() returns true only when both URLs are set AND at least one points to non-local host
• This correctly addresses the .env.example default scenario identified by the reviewer

2. SpiceDB health check (lines 377-393):

• Added to parallel health check array with 30s timeout (matches postgres-db)
• Failure is logged as warning, not fatal — appropriate since SpiceDB is optional for some workflows
• Only doltgres + postgres dual failure triggers exit

3. Test coverage:

• New test verifies SpiceDB health check is invoked
• New test verifies localhost URLs + unavailable Docker → exit(1)
• New test verifies external URLs + unavailable Docker → graceful skip

🕐 Pending Recommendations (1)

• 🟡 setup.ts:482 waitForServerReady duplicates functionality in e2e utils (valid observation, low priority)

The waitForServerReady functions in setup.ts and packages/create-agents/src/__tests__/e2e/utils.ts are similar but not identical (different polling strategies, logging, success conditions). Consolidation is reasonable future work but not blocking — the duplication is ~20 lines of stable polling logic in different contexts (user-facing setup vs. test infrastructure).

Summary: This delta correctly addresses both human review comments from @miles-kt-inkeep. The localhost URL detection logic is sound and properly handles the .env.example default scenario. The SpiceDB health polling is correctly integrated with appropriate timeout and non-fatal failure handling. The waitForServerReady duplication is acknowledged as valid feedback for future cleanup. This PR has been thoroughly reviewed across 14 review cycles — ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Delta Re-review Summary

This is a delta-scoped re-review following 1 new commit since the last approved review:

• ed98c2327 — fix: skip Docker check in CI where databases are service containers

Delta scope: 2 files, +27/-2 lines

Technical Assessment

Logic change (setup.ts lines 56-58):

// In CI, databases are always externally managed (GitHub Actions service containers, etc.)
if (process.env.CI) return true;

• ✅ Correct fix — In CI environments (GitHub Actions, GitLab CI, etc.), databases are typically provided as service containers, not Docker Compose. The previous code would fail because it saw localhost URLs and expected Docker to be available.
• ✅ Minimal change — Single conditional check added before the localhost URL check
• ✅ No side effects — Only affects the hasExternalDatabases() helper used in pre-flight checks

Test coverage:

• ✅ New test case 'should skip docker in CI even with localhost database URLs' validates the behavior
• ✅ beforeEach now explicitly clears process.env.CI to ensure other tests don't accidentally inherit CI behavior

Prior Feedback Status

All prior inline threads remain resolved:

🕐 Pending Recommendations (1)

• 🟡 setup.ts:485 waitForServerReady duplicates functionality in e2e utils (valid observation, low priority — acknowledged as future cleanup)

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The PR has been thoroughly reviewed across 15 review cycles with all substantive issues either fixed or appropriately declined. This latest commit correctly handles CI environments where databases are provided as service containers rather than Docker Compose — a clean, focused fix with appropriate test coverage. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

Re-review Summary

This is a re-review triggered by pull_request:synchronize. The head SHA (073a4716b) matches the previously approved state. No new PR-specific code changes since the last approval.

This PR has been thoroughly reviewed across 15+ prior review cycles, all resulting in approval. The comprehensive review history includes:

Prior Feedback Status

All 5 prior automated inline threads are resolved:

Human Review Status

Human reviewer @miles-kt-inkeep approved on 2026-02-18T22:49:50Z with 3 comments:

• ✅ Localhost URL detection — Fixed (added isLocalDatabaseUrl() helper)
• ✅ SpiceDB health check — Fixed (added to parallel health polling)
• 🟡 waitForServerReady duplication — Acknowledged as valid future cleanup (low priority)

🕐 Pending Recommendations (1)

• 🟡 setup.ts:485 waitForServerReady duplicates functionality in e2e utils — valid observation for future consolidation, low priority

Summary: This PR successfully unifies ~600 lines of duplicated setup logic into a shared @inkeep/agents-core/setup module. The architecture is clean, error handling is appropriate for CLI context, tests cover core scenarios (17 test cases), CI integration is correct, and documentation accurately reflects the new behavior. Human reviewer has approved. All substantive feedback has been addressed. Ship it! 🚀