Skip to content

test(feedback): allow retry fixture to finish (#759) - #761

Open
Kewe63 wants to merge 2 commits into
nansen-ai:mainfrom
Kewe63:fix/feedback-fixture-timeout
Open

Kewe63 wants to merge 2 commits into
nansen-ai:mainfrom
Kewe63:fix/feedback-fixture-timeout

Conversation

@Kewe63

@Kewe63 Kewe63 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #759.

The feedback preference retry regression deliberately waits 1, 2, and 4 seconds before its fourth request. The shared CLI process fixture previously killed each subprocess after exactly 10 seconds, leaving too little headroom for four HTTP exchanges, Node startup, module loading, output collection, and shutdown on slower hosts.

This PR:

  • gives the feedback CLI fixture a named 15-second process deadline;
  • documents why the deadline must exceed the seven-second retry schedule;
  • matches the existing 15-second subprocess deadline in feedback-recovery.test.js;
  • leaves production retry behavior and assertions unchanged.

Testing

RED on the unmodified base:

src/__tests__/feedback.test.js (68 tests | 2 failed | 66 skipped)
× keeps ask-first guidance after all preference retries {"flags":[]} 10030ms
× keeps ask-first guidance after all preference retries {"flags":["--json"]} 10020ms
Error: CLI fixture timed out

GREEN after the patch:

npx vitest run src/__tests__/feedback.test.js \
  -t 'keeps ask-first guidance after all preference retries'
Test Files  1 passed (1)
Tests       2 passed | 66 skipped (68)
npx vitest run src/__tests__/feedback.test.js
Test Files  1 passed (1)
Tests       68 passed (68)
npm run lint
eslint .

Full local suite:

npm test
Test Files  116 passed (116)
Tests       4984 passed | 16 skipped (5000)

GitHub's checks provide the additional repository review and security gates.

Compatibility and risk

  • Test-only change; production retry timing is unchanged.
  • The 15-second fixture deadline remains below Vitest's 30-second per-test limit.
  • The test still requires four requests and the final fail-closed guidance, so coverage is not weakened.

Checklist

  • Relevant tests pass
  • src/schema.json updated if new commands or flags were added — not applicable
  • README.md updated if new top-level commands or categories were added — not applicable
  • Changeset added for user-visible changes — not applicable; test-only portability fix

Give the CLI harness enough process headroom for the intentional 1, 2, and 4 second preference retry schedule.\n\nFixes nansen-ai#759
@nansen-pr-reviewer

nansen-pr-reviewer Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

nansen-pr-reviewer summary

This test-only change increases the feedback CLI subprocess timeout from 10 to 15 seconds to accommodate the documented preference retry backoff. It does not alter production behavior, command functionality, or security-sensitive code.

Deterministic check: success — No issues found

Risk: 1/5 (Trivial)

✅ Auto-approved — Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 1/5 within threshold 3.


Token usage: 11,283 input, 356 output, 3,206 cache read | Usage Guide

Cooldown: for the next 10 minutes (counting from when this review finished), new pushes to this PR will not trigger another review — the next push after the window expires will. Need a fresh review sooner? Comment @nansen-pr-reviewer[bot] re-review.

@nansen-pr-reviewer nansen-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved by Nansen review

Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 1/5 within threshold 3.

  • The deterministic severity check passed: No issues found
  • Risk level 1/5 (Trivial)
  • The model recommended: approve

Approval was decided by deterministic gates, not by the model. The model's recommendation can block auto-approval but never cause it.

@Kewe63 Kewe63 changed the title test(feedback): allow retry fixture to finish test(feedback): allow retry fixture to finish (#759) Oct 22, 2026

@nansen-pr-reviewer nansen-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved by Nansen review

Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 1/5 within threshold 3.

  • The deterministic severity check passed: No issues found
  • Risk level 1/5 (Trivial)
  • The model recommended: approve

Approval was decided by deterministic gates, not by the model. The model's recommendation can block auto-approval but never cause it.

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.

Feedback preference retry tests exceed the fixture's 10-second timeout

1 participant