Repository navigation
fix(session): stop resubmitting rejected reviews to unchanged daemons - #1199
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
 [Medium impact] The change affects how Hunk review sessions reconnect to their local daemon. The retry changes appear sound, but the new tests must satisfy the repository’s environment-access rule before merging. Findings
Fix with agent prompt### Issue 1
packages/hunk/src/session/broker/brokerClient.test.ts:256
The new fixtures write directly to `process.env` here and in the real-daemon test. The repository guide says “Never access `process.env` directly” and requires the approved environment layer or parameters instead. Move these settings behind that boundary before merging to satisfy the repository requirement.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryStops resending rejected reviews to an unchanged healthy daemon.
Intentional behavior acknowledged by benvinegar: missing launch metadata does not release a refusal while the daemon remains healthy. This prevents repeat submissions when daemon identity cannot be distinguished. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Authenticated review rejected] --> B[Record payload wait]
B --> C[Poll daemon health]
C --> D{Healthy and same fingerprint?}
D -->|Yes, including null-to-null| C
D -->|No| E[Release wait and reconnect]
B --> F[Successful explicit review replacement]
F --> E
B --> G[Snapshot-only update]
G --> C
Reviews (1) · Last reviewed commit: "fix(session): stop resubmitting rejected..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Authenticate observed sockets with real signed hellos while keeping retry time deterministic. */ | ||
| async function createRefusalLifecycleTest(withMetadata = false, subscribe = false) { | ||
| const runtimeDir = mkdtempSync(join(tmpdir(), "hunk-payload-refusal-")); | ||
| process.env.XDG_RUNTIME_DIR = runtimeDir; |
There was a problem hiding this comment.
The new fixtures write directly to process.env here and in the real-daemon test. The repository guide says “Never access process.env directly” and requires the approved environment layer or parameters instead. Move these settings behind that boundary before merging to satisfy the repository requirement.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hunk/src/session/broker/brokerClient.test.ts
Line: 256
Comment:
**Tests bypass environment boundary**
The new fixtures write directly to `process.env` here and in the real-daemon test. The repository guide says “Never access `process.env` directly” and requires the approved environment layer or parameters instead. Move these settings behind that boundary before merging to satisfy the repository requirement.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
A large
hunk diff mainreview repeatedly sent an approximately 8.3 MB registration with 5,501 files to a daemon whose registration limit is 5,000 files. Authentication succeeded, registration was rejected, and reconnecting submitted the same invalid payload again. The review never appeared in the session list.Changes
This belongs in the existing session client lifecycle. It does not change payload limits, transport/protocol shapes, or transient-disconnect retry behavior. The wire snapshot remains unchanged.
Verification
Rebased onto current
main(483ea47d) in an isolated worktree and installed its committed lockfile withbun install --frozen-lockfile; no dependency upgrades are included. Tested on macOS with Bun 1.4.3.bun test packages/hunk/src/session/broker/brokerClient.test.ts— 56 passed.bun test packages/hunk/src/session/client/daemonSkew.test.ts— 9 passed.bun test packages/hunk/src/session/broker/wire.snapshot.test.ts— 3 passed.bun run test:session-broker-node— 4 passed.bun run typecheck— passed.bun run deps:check— passed.oxfmt --check,oxlint --deny-warnings, andgit diff --check— passed.Linux and Windows were not tested locally. The status-line wording is the only intended visual change; its existing 80-column notice test passes.
Scope of the incident finding
The registration/retry loop was confirmed, but it is not proven to explain the original process's full memory footprint. An isolated 3,000-iteration rejection/reconnect probe did not reproduce substantial retained memory. This PR fixes retry churn, not a claimed complete memory-leak diagnosis; resize geometry retention is addressed independently in #1200.