Skip to content

fix(poll): retain feedback until acknowledged - #312

Open
withally wants to merge 1 commit into
kunchenguid:mainfrom
withally:fm/lavish-upstream-durability-pr-u1
Open

fix(poll): retain feedback until acknowledged#312
withally wants to merge 1 commit into
kunchenguid:mainfrom
withally:fm/lavish-upstream-durability-pr-u1

Conversation

@withally

@withally withally commented Sep 2, 2026

Copy link
Copy Markdown

Retains each feedback batch under a stable delivery_id until explicit --ack; a 30-second lease releases the identical batch for consumers that never ACK, and ACK wakes a waiting poll when newer feedback remains. This addresses the still-reproducible durability defect described in #228 and carries forward only the lease-and-ACK portion of the transport work introduced in #212 and reverted in #214. Regression coverage includes no-ACK redelivery, ACK consumption, post-ACK wakeup, retry idempotence, and an isolated real-browser annotation round-trip through a killed response consumer; pnpm run check passes.

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The ACK-and-reply partial-application path should be fixed before merging so feedback cannot be consumed while its associated browser reply is lost.

The delivery store’s lease and reconciliation behavior is well covered, but the CLI commits acknowledgement before issuing the independent reply request, leaving a reachable failure window between the two operations.

Files Needing Attention: src/cli.js, src/server.js, src/session-store.js

Comments Outside Diff (1)

  1. src/cli.js, line 786-801 (link)

    P1 ACK can outrun reply

    When a combined --ack and --agent-reply command successfully acknowledges feedback but the process or connection fails before the separate reply request completes, the delivery is permanently consumed while the browser never receives the agent's reply. Persist the acknowledgement and associated reply as one recoverable operation so retrying cannot leave this partial state.

    Knowledge Base Used:

    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!

Reviews (1): Last reviewed commit: "fix(poll): retain feedback until acknowl..." | Re-trigger Greptile

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Diff-reviewed head 47a5515d (withally fork; first PR from this author). Files touched: AGENTS.md, README.md, src/cli.js, src/server.js, src/session-store.js, and matching tests — no workflow-file changes. Not malware. Greptile already ran (confidence 4/5); not pinging.

Fork CI / Guard / Require no-mistakes: still action_required on runs 33575463526 / 33575463525 / 33575463524. Diff review says they are safe to approve; the approve step did not land from this pass, so CI has not started. This is waiting on first-time fork workflow approval + green CI, not on an author code change yet.

contract-class: new-default. Claimed as a durability fix for #228 / #296 (lease-and-ACK; portion of #212 transport work that #214 reverted), but a bug-fix claim does not make a replaced default path restore. Today poll delivery consumes on return (takeFeedback clears; restore only covers disconnect-during-take). This PR changes the agent contract: every feedback batch gets a stable delivery_id, stays leased ~30s, and is cleared only by POST /api/:key/ack / --ack. Help, next_step, and presence guidance all teach ACK as the normal loop. That is a new default poll protocol, so no auto-merge even when CI is green — captain decision after green checks.

Security: no Firstmate security flag from this diff. New /ack matches the existing agent-side session-key trust model of poll / agent-reply (key is derived, not secret — already documented). No landing-page / session-enumeration change (unlike #310). Bounded ACK / agent-reply id histories. No secrets or workflow injection.

Greptile P1 (author note, not a captain hold): CLI posts --ack before --agent-reply. If ACK succeeds and the reply request fails, feedback is consumed while the browser may miss that reply until a retry. Your idempotent retry test covers the happy retry path; making ACK+reply one recoverable server operation (or reply-before-ACK with clear rules) would close the window. Optional for this PR — call it out so you know it is not why CI is waiting.

VISION.md (per rule)

  • The artifact stays the author's — aligns. Store/CLI/poll contract only; no extra injection into saved HTML.
  • Interaction beats prose — aligns. Reviewer interaction unchanged; agent recovery of lost poll payloads.
  • An artifact's design is chosen, never defaulted into — aligns. No design-guidance / styling change.
  • Nothing interrupts the human — aligns. Detection still does not wake polls; ACK is agent-initiated. Lease redelivery does not nag the reviewer.
  • Every token is spent on purpose — aligns with a caveat. Durability earns redelivery tokens when a harness drops stdout; agents that never ACK will see the same batch again after ~30s (guidance updated to teach ACK). Prefer that over silent loss (poll delivery is destructive and unrecoverable: a truncated payload silently destroys user feedback and still exits 0 #228).
  • The instructions are the product — aligns. CLI / next_step / README own the new ACK contract instead of a parallel mechanism.
  • Scope — aligns. Still one person, one agent, local review loop; no hosting, MCP, or multi-human collab.

What happens next

  1. First-time fork CI / Guard / NM need approval, then checks must go green (no-mistakes blocking).
  2. Once green and still safe: captain-decision hold for new-default (not waiting on you for the product call).
  3. Optional: address Greptile's ACK-before-reply partial-failure note if you want that tightened before captain review.

No merge from this pass. #216 / #295 untouched. Related: open #296; closed #228; history #212/#214.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants