test: guard fresh startup release verification#147
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds startup-log verification and a post-release packaged-smoke startup step: the verify-release tool now accepts a startup log file, parses the latest "app starting" section, checks Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Astro-Han
left a comment
There was a problem hiding this comment.
Overall the implementation is solid and addresses the startup verification gap. A few minor suggestions:
There was a problem hiding this comment.
Code Review
This pull request introduces a startup verification mechanism to the release process by adding a smoke test script to the release checklist and updating the release verification script to validate application startup logs. The feedback suggests improving the robustness of the log verification by ensuring the log version matches the expected release version, which prevents false positives from older log entries. Additionally, reviewers recommended including Windows-specific paths in the release checklist for better cross-platform support and updating the test suite to cover version mismatch scenarios.
4f2d6c8 to
2f1b9c0
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Summary
Good addition for catching startup hangs. Found some test strictness issues worth addressing.
P1 Issues
- Test assertions check only specific failure messages but don't verify the complete failure set, which could miss unexpected failures
P2 Issues
- Regex pattern edge case awareness
readStartupLogFiletest only checks error message prefix
2f1b9c0 to
ee137b8
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Review Summary
Overall the PR is well-structured with good test coverage. Found several issues.
P1 (suggest fixing): 3 issues
P2 (record only): 7 issues
P3 (nitpick): 2 issues
See inline comments for details.
Astro-Han
left a comment
There was a problem hiding this comment.
Review Summary
Overall the PR is well-structured with good test coverage. Found several issues.
P1 (suggest fixing): 3 issues
P2 (record only): 7 issues
P3 (nitpick): 2 issues
See inline comments for details.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/RELEASE_CHECKLIST.md:
- Around line 145-146: The default startup_log points to $HOME which can read a
different profile than the smoke test (PAWWORK_CI_SMOKE_HOME), causing stale
logs to be used; update the startup_log default to respect PAWWORK_CI_SMOKE_HOME
(or the local smoke_home) so the smoke-run uses the correct profile log—modify
the startup_log assignment (the startup_log variable in the diff) to use
${PAWWORK_CI_SMOKE_HOME:-$HOME} or the equivalent smoke_home-aware expansion so
that when PAWWORK_CI_SMOKE_HOME/ smoke_home is set the log path resolves under
that directory rather than $HOME/Library/Logs/PawWork/main.log.
In `@packages/desktop-electron/scripts/verify-release.test.ts`:
- Around line 316-319: The test "reports unreadable startup log files with the
path" uses a fixed path that may exist on some machines; change the test to
generate and use a unique nonexistent temp path when calling readStartupLogFile
(e.g., build a path using os.tmpdir() plus a timestamp/pid/random suffix or
create a temporary directory with fs.mkdtemp and use a guaranteed-nonexistent
filename), ensure the path does not exist before invoking readStartupLogFile,
and keep the same expected rejects.toThrow pattern so the failure message still
includes the generated path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe9ddfbc-e916-4c8e-9b3e-1463ea2d84ac
📒 Files selected for processing (3)
.github/RELEASE_CHECKLIST.mdpackages/desktop-electron/scripts/verify-release.test.tspackages/desktop-electron/scripts/verify-release.ts
ee137b8 to
8b11f72
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Release verification hardening is a worthwhile goal, and the test matrix is thorough. Surfacing a few issues — one P1 where the documented macOS smoke script will not find the log file as written, one P2 alias bug in the server ready check that defeats the stated hang detection goal, plus smaller polish items. See inline.
Astro-Han
left a comment
There was a problem hiding this comment.
Review Summary
P0 (Blocking): None - code logic is sound
P1 (Should Fix): 1 issue
escapeRegExphelper defined only in test file, buthasStartupVersionuses the same logic inline. Consider consolidating.
P2 (Nice to Fix): 2 issues
- RELEASE_CHECKLIST.md: script fails silently after 60s timeout without explicit
Timed out waiting for ready filemessage - cleanup() does not verify if
app_pidprocess still exists beforekill
P3 (Nit): 3 issues
[^\n]in regex literals - minor readability concern- No test for version strings containing regex special chars
- Error message prefix inconsistency (
Invalid release tagvsLatest startup log does not include)
Overall: good coverage, well-tested. P1 is a minor code organization issue, P2s are documentation/script robustness nits.
fad9578 to
6047797
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/RELEASE_CHECKLIST.md:
- Around line 185-203: Save the original values of PAWWORK_CI_SMOKE,
PAWWORK_CI_SMOKE_HOME, and PAWWORK_RELEASE_STARTUP_LOG into temporary variables
before setting them, and restore those originals in the finally block (before
removing $smokeHome and stopping $app) so the caller session environment is
returned to its prior state; update the block around the Start-Process /
try...finally (referencing $env:PAWWORK_CI_SMOKE, $env:PAWWORK_CI_SMOKE_HOME,
$env:PAWWORK_RELEASE_STARTUP_LOG, $app, and the finally cleanup) to perform the
restore unconditionally.
In `@packages/desktop-electron/src/main/index-sidecar-source.test.ts`:
- Around line 7-10: Add a raw-source guard that asserts the startup readiness
log with payload is present: update the test alongside the existing toContain
checks (the ones asserting "app.setPath(\"logs\"", "const needsMigration =
false", and 'logger.log("init done")') to also toContain the `server ready`
emission including the `{ url:` payload (the same contract `verifyStartupLog()`
expects). Keep using the raw-source toContain style and place this check near
the other expectations so future logging refactors that remove or change the
`server ready { url: ... }` line will fail the source-contract test; reference
the `verifyStartupLog` contract and the `server ready`/`url` tokens when adding
the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6239ae3-d781-4cd1-8cfa-d7ee54e512de
📒 Files selected for processing (5)
.github/RELEASE_CHECKLIST.mdpackages/desktop-electron/scripts/verify-release.test.tspackages/desktop-electron/scripts/verify-release.tspackages/desktop-electron/src/main/index-sidecar-source.test.tspackages/desktop-electron/src/main/index.ts
6047797 to
d0083be
Compare
Summary
Adds release verification coverage for fresh packaged startup logs so a build that reaches
server readybut never finishes desktop initialization is caught before closing startup-blocking issues.Why
PawWork v0.2.5 exposed a first-launch loading hang tracked by #144 and confirmed again by #145. The runtime fix is already on
dev; this PR hardens the release verification path so the same startup failure mode is checked before the next release is marked done.Related Issue
Fixes #144
Fixes #145
How To Verify
I also ran
git diff --check.Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Documentation
Tests
Chores