Repository navigation
Conversation
…s out A database helper that never prints its ready line now fails with the container's status, exit code, engine error and log tail, read before the attach is killed so `--rm` has not removed it yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMySvvbM4B1zdmhw3pp1wA
|
/ai-review Generated by Claude Code |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Confirmed both Claude findings as minor diagnostic-quality issues: the logs diagnostic omits stderr, and diagnostic failures lose their reasons. Existing attached-process stderr capture partly mitigates the first issue. Codex's independent review completed with no findings.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | packages/stack/src/storage/DockerDatabaseStorage.ts:651 |
error-handling |
claude | The helper logs diagnostic drops container stderr. Docker logs routes container stderr to the CLI's stderr, but engineCommand returns only stdout on success. Consequently, stderr-only logs appear as 'helper logs: (empty)', limiting recovery of startup errors when the attached stream stalls. |
| 🟡 MINOR | packages/stack/src/storage/DockerDatabaseStorage.ts:655 |
error-handling |
claude | Every inspect or logs failure becomes the same 'unavailable' string, making a diagnostic timeout indistinguishable from a removed helper or another engine failure. |
Stats
Claude findings: 2 · Codex findings: 0 · Confirmed: 2 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews completed. Verified all three Claude findings: two minor diagnostic omissions and one test-coverage nit. Codex reported no findings. No major or critical issues were identified.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | packages/stack/src/storage/DockerDatabaseStorage.ts:655 |
error-handling |
claude | Failed diagnostic commands replace useful engine errors with "unavailable", making a missing helper indistinguishable from an unresponsive daemon. |
| 🟡 MINOR | packages/stack/src/storage/DockerDatabaseStorage.ts:651 |
error-handling |
claude | The new helper logs section omits the container's stderr because engineCommand returns only stdout on successful commands. |
| ⚪ NIT | packages/stack/src/storage/DockerDatabaseStorage.integration.test.ts:491 |
test-coverage |
claude | The updated timeout test does not protect diagnostic collection before attach termination, and it does not exercise the "unavailable" or "(empty)" fallbacks. |
Stats
Claude findings: 3 · Codex findings: 0 · Confirmed: 3 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMySvvbM4B1zdmhw3pp1wA
Flaky test check: ✅ no flaky testsCommit Each execution is a separate Vitest process; |
TL;DR
A database helper whose readiness times out now reports the container's state and log tail, so the next stall shows why it stalled.
DockerDatabaseStorage.integration.test.ts › labels its volume with the configured test run:start --attachprinted no ready line within 30s, and stderr was empty. The error said onlyDatabase helper did not become ready within 30 seconds. That cannot tell apart a helper that never started, one that exited, and one that printed its ready line that the attach never relayed. On a readiness timeout, the helper start now reads the container's status, exit code and engine error (inspect) and its last 20 log lines (logs), each with its own 5s bound. It reads them before killing the attach, because--rmremoves the helper once the attach ends. Both go into the error and the state onto the span. The existing helper-timeout test now asserts on this output.ref
spotted in:
Generated by Claude Code