Improve Codex reliability, quota, and Windows pet behavior - #15
Improve Codex reliability, quota, and Windows pet behavior#15ziyuezhou1 wants to merge 7 commits into
Conversation
|
Follow-up fix in 866083e: packaged LLMPET now registers login startup, failed hook delivery rate-limits an automatic app restart, and the Windows installer explicitly runs the app after setup. Validation: the full npm test suite passed, and a Windows kill/recovery drill confirmed that the next Node hook launched LLMPET.exe --hook-recovery and restored the authenticated localhost IPC endpoint with HTTP 200. |
myunwang
left a comment
There was a problem hiding this comment.
Thanks for the substantial contribution. The overall direction is valid: Codex hooks and account/rateLimits/read are documented interfaces, and I independently ran the PR's test suite successfully.
I'm requesting changes before merge because there are four blocking integration/reliability issues:
-
The hook event that triggers recovery is lost. When
postStatefails, the hook starts LLMPET and exits without queueing or replaying the current payload. If that event is the onlyStop, the recovered app never receives the completion state—especially important on Codex builds where rollout JSONL is not a reliable fallback. Please persist/retry the event after the local server becomes healthy, and add a regression test that verifies delivery rather than only process spawning. -
Packaged users cannot reliably keep LLMPET closed. Startup currently enables
openAtLoginunconditionally, and any later Claude/Codex hook failure respawns the app. An intentional Quit therefore becomes non-sticky. Please gate startup/recovery behind an explicit persisted user setting and record intentional shutdown so hooks do not resurrect the app after Quit. -
The branch must be rebased onto the latest
main. GitHub currently reports the PR as conflicting.mainrecently gained the travel system andtest/travel.js, while this branch'spackage.jsontest command does not run that suite. Core features have been moving quickly recently, so please rebase, preserve the travel implementation/tests, resolve the README/package conflicts, and run the complete current-main suite. -
Popup resizing can move content off-screen. The new geometry deliberately avoids horizontal clamping. The test accepts
x=1476, width=520on a 1920px work area, leaving 76px outside the visible screen. Please keep the outer window inside the work area and preserve the pet's visual position using an internal pet/content offset; add left/right-edge coverage.
After these changes, please include evidence for:
- the first failed hook event being replayed after recovery;
- explicit Quit remaining closed;
- the full latest-main test suite including travel;
- popup content remaining fully visible at both horizontal screen edges.
The PR is worth continuing, so I am leaving it open for revision rather than closing it.
# Conflicts: # README_EN.md # README_JA.md # package.json
# Conflicts: # package.json
866083e to
09f580e
Compare
|
Thank you for the substantial follow-up. I re-reviewed the updated branch and independently ran the complete current test suite; it passes on macOS. The four blockers from my previous review are now addressed at the code/test level:
However, the later Windows Terminal work adds a separate privileged subsystem and has expanded this PR to 43 files / roughly 4,000 added lines. I do not want that security and installer surface coupled to the core reliability fixes. I created an isolated integration branch for it:
Please split the work as follows:
For the experimental Windows PR, please also cover these acceptance points:
The earlier fixes are valuable and worth merging once the privileged Windows portion is separated. No direct repository write access is needed—please continue from your fork and target the experimental branch with the second PR. |
|
I also want to add a more personal thank-you. You clearly put a great deal of time and care into this contribution. After the first review, you did not dismiss the concerns or make superficial changes—you worked through every blocker, rebased onto the moving My request to split the Windows Terminal broker is not a rejection of your work or a lack of trust in the effort behind it. It is a maintainer decision about keeping a privileged, installer-level subsystem reviewable and giving it the dedicated Windows validation it deserves. The core reliability improvements are valuable, and I want to make sure your contribution can enter the project in a form that is safe, maintainable, and properly credited. Thank you again for continuing to improve the PR after detailed feedback and for being willing to work with a project whose core features have been changing quickly. Contributions with this much thought and follow-through are rare, and they are sincerely appreciated. |
70b9c92 to
7e877c0
Compare
Problems addressed
LastWriteTimedid not advance.What changed
LastWriteTimeis only a compatibility fallback.The privileged Windows Terminal subsystem was removed from this PR and is now isolated in #16, targeting
experimental/windows-terminal-focus. No release version bump is included.Validation
npm testsuite passes on Windows, including the travel suite.LastWriteTime, last-event-time startup recovery, weekly-bucket detection, drag handling, and fully visible popup content at both screen edges.