fix(harness-testkit): bound unbounded testkit awaits with p-timeout, named per call site (#390) - #393
Closed
omridevk wants to merge 2 commits into
Closed
fix(harness-testkit): bound unbounded testkit awaits with p-timeout, named per call site (#390)#393omridevk wants to merge 2 commits into
omridevk wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe harness testkit adds a shared 20-second deadline. It applies timeout handling to session resolution, chat approval operations, and polling. Tests cover stalled predicates and asynchronous predicate success. ChangesTestkit deadlines
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/embed/test/panel-focus.it.test.ts`:
- Around line 79-96: Wrap the test body after successful serveHost/page creation
in a try/finally block, and move page.close() and host.close() into the
finalizer. Preserve the existing releaseSessionList cleanup, ensuring all
acquired resources are closed even when an assertion or navigation step rejects.
- Around line 78-94: Update the test around holdFirstSessionList and openPanel
so the launcher is clicked while the session-list request remains blocked, then
assert composer(page) is visible before calling releaseSessionList(). Keep the
existing session-pill visibility, composer focus, and text-entry assertions
after release, preserving the cleanup in the finally block.
In `@packages/harness-testkit/src/call-tool.ts`:
- Line 123: Update the pump handling around drainApprovals so approval-deadline
failures from rpc.chat.permissionDecision are identified and rethrown as a
tagged deadline error, while ordinary pump errors remain swallowed. Ensure the
later await of pump observes the propagated deadline failure, and add coverage
for an expired approval deadline.
In `@packages/harness-testkit/src/deadline.ts`:
- Around line 3-17: Update deadline in packages/harness-testkit/src/deadline.ts
(lines 3-17) to expose cancellation or expiration to the work operation and
ensure cleanup after timeout. In packages/harness-testkit/src/call-tool.ts
(lines 117-122), abort the subscription when the deadline expires; in
packages/extension-testkit/src/boot-server.ts (lines 20-34), stop engines that
resolve after timeout; in
packages/extension-testkit/src/get-extension-test-api.ts (lines 63-66), always
run close() and stop() cleanup even when browser closing fails; and in
packages/harness-testkit/src/until.ts (lines 28-32), stop polling and clear
pending waits when the hang guard expires.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: fbbcfd32-9550-46ca-9b75-9f1c3a483e35
📒 Files selected for processing (19)
apps/conciv/src/composer/actions.tsxapps/conciv/src/composer/model-selector.tsxapps/conciv/src/composer/session-selector.tsxapps/conciv/src/data/settled-data.tsapps/conciv/src/pane/chat-pane.tsxapps/conciv/src/routes/__root.tsxapps/conciv/src/routes/panel.$sessionId.tsxapps/conciv/src/routes/quick.tsxpackages/embed/test/panel-focus.it.test.tspackages/extension-testkit/src/boot-server.tspackages/extension-testkit/src/get-extension-test-api.tspackages/harness-testkit/package.jsonpackages/harness-testkit/src/call-tool.tspackages/harness-testkit/src/deadline.tspackages/harness-testkit/src/session.tspackages/harness-testkit/src/testkit.tspackages/harness-testkit/src/until.tspackages/harness-testkit/test/deadline.test.tspackages/harness-testkit/test/until.test.ts
Comment on lines
+78
to
+94
| it('paints the shell while the session list is still loading, then focuses the composer', async () => { | ||
| const host = await serveHost(() => | ||
| hostPage({apiBase: suite.kit().base, widget: '{"quickTerminal":false,"transport":"fetch"}'}), | ||
| ) | ||
| const page = await suite.browser().newPage() | ||
| const releaseSessionList = await holdFirstSessionList(page) | ||
| await page.goto(host.base, {waitUntil: 'domcontentloaded'}) | ||
| try { | ||
| await expectLocator(page.getByRole('button', {name: 'Open conciv chat'})).toBeVisible({timeout: 15_000}) | ||
| } finally { | ||
| releaseSessionList() | ||
| } | ||
| await openPanel(page) | ||
| await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000}) | ||
| await expectLocator(composer(page)).toBeFocused({timeout: 10_000}) | ||
| await page.keyboard.type('typed after the session list resolved') | ||
| await expectLocator(composer(page)).toHaveText('typed after the session list resolved') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Open the panel before releasing the session-list request.
Lines 86-90 only verify that the launcher renders while the request is held. openPanel(page) runs after releaseSessionList(). The test does not verify panel rendering while session data is pending.
Click the launcher and assert that the composer is visible before releasing the request. Keep the session-pill, focus, and text-entry assertions after release.
Proposed test change
try {
- await expectLocator(page.getByRole('button', {name: 'Open conciv chat'})).toBeVisible({timeout: 15_000})
+ const opener = page.getByRole('button', {name: 'Open conciv chat'})
+ await expectLocator(opener).toBeVisible({timeout: 15_000})
+ await opener.click()
+ await expectLocator(composer(page)).toBeVisible({timeout: 15_000})
} finally {
releaseSessionList()
}
- await openPanel(page)
await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('paints the shell while the session list is still loading, then focuses the composer', async () => { | |
| const host = await serveHost(() => | |
| hostPage({apiBase: suite.kit().base, widget: '{"quickTerminal":false,"transport":"fetch"}'}), | |
| ) | |
| const page = await suite.browser().newPage() | |
| const releaseSessionList = await holdFirstSessionList(page) | |
| await page.goto(host.base, {waitUntil: 'domcontentloaded'}) | |
| try { | |
| await expectLocator(page.getByRole('button', {name: 'Open conciv chat'})).toBeVisible({timeout: 15_000}) | |
| } finally { | |
| releaseSessionList() | |
| } | |
| await openPanel(page) | |
| await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000}) | |
| await expectLocator(composer(page)).toBeFocused({timeout: 10_000}) | |
| await page.keyboard.type('typed after the session list resolved') | |
| await expectLocator(composer(page)).toHaveText('typed after the session list resolved') | |
| it('paints the shell while the session list is still loading, then focuses the composer', async () => { | |
| const host = await serveHost(() => | |
| hostPage({apiBase: suite.kit().base, widget: '{"quickTerminal":false,"transport":"fetch"}'}), | |
| ) | |
| const page = await suite.browser().newPage() | |
| const releaseSessionList = await holdFirstSessionList(page) | |
| await page.goto(host.base, {waitUntil: 'domcontentloaded'}) | |
| try { | |
| const opener = page.getByRole('button', {name: 'Open conciv chat'}) | |
| await expectLocator(opener).toBeVisible({timeout: 15_000}) | |
| await opener.click() | |
| await expectLocator(composer(page)).toBeVisible({timeout: 15_000}) | |
| } finally { | |
| releaseSessionList() | |
| } | |
| await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000}) | |
| await expectLocator(composer(page)).toBeFocused({timeout: 10_000}) | |
| await page.keyboard.type('typed after the session list resolved') | |
| await expectLocator(composer(page)).toHaveText('typed after the session list resolved') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/embed/test/panel-focus.it.test.ts` around lines 78 - 94, Update the
test around holdFirstSessionList and openPanel so the launcher is clicked while
the session-list request remains blocked, then assert composer(page) is visible
before calling releaseSessionList(). Keep the existing session-pill visibility,
composer focus, and text-entry assertions after release, preserving the cleanup
in the finally block.
Comment on lines
+79
to
+96
| const host = await serveHost(() => | ||
| hostPage({apiBase: suite.kit().base, widget: '{"quickTerminal":false,"transport":"fetch"}'}), | ||
| ) | ||
| const page = await suite.browser().newPage() | ||
| const releaseSessionList = await holdFirstSessionList(page) | ||
| await page.goto(host.base, {waitUntil: 'domcontentloaded'}) | ||
| try { | ||
| await expectLocator(page.getByRole('button', {name: 'Open conciv chat'})).toBeVisible({timeout: 15_000}) | ||
| } finally { | ||
| releaseSessionList() | ||
| } | ||
| await openPanel(page) | ||
| await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000}) | ||
| await expectLocator(composer(page)).toBeFocused({timeout: 10_000}) | ||
| await page.keyboard.type('typed after the session list resolved') | ||
| await expectLocator(composer(page)).toHaveText('typed after the session list resolved') | ||
| await page.close() | ||
| await host.close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the page and host in a finalizer.
If an assertion rejects after serveHost() succeeds, lines 95-96 do not run. The open page and HTTP server can then keep the test process active until its file-level timeout.
Wrap the test body in try/finally. Close both resources from the finalizer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/embed/test/panel-focus.it.test.ts` around lines 79 - 96, Wrap the
test body after successful serveHost/page creation in a try/finally block, and
move page.close() and host.close() into the finalizer. Preserve the existing
releaseSessionList cleanup, ensuring all acquired resources are closed even when
an assertion or navigation step rejects.
omridevk
force-pushed
the
fix/390-testkit-deadlines
branch
from
August 10, 2026 00:23
a8482a8 to
98203db
Compare
This was referenced Aug 10, 2026
Merged
…ir stage (#390) The testkit awaited the bare oRPC link with no bound at sessions.resolve, chat.subscribe, chat.permissionDecision, the approval pump drain, engine start/stop and browser close, so a wedged call surfaced only as the runner's file-level timeout with no frame. One shared deadline(label, budgetMs, work) now bounds each of those stages at a named 20s budget and rejects with the stage and the budget it blew. until had the same hole from the other side: it awaited the predicate before checking the deadline, so a predicate that never settled defeated hangGuardMs entirely. The poll loop now runs inside the same deadline, so until always throws its stall error by the guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
omridevk
force-pushed
the
fix/390-testkit-deadlines
branch
from
August 10, 2026 18:38
98203db to
ee2e5dc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/harness-testkit/src/call-tool.ts`:
- Line 143: Update the deadline-wrapped pump await in the call-tool flow to
await a derived promise that suppresses ordinary iterator and shutdown-abort
rejections while preserving and propagating deadline errors. Keep the existing
testkit deadline behavior intact, and add coverage for both swallowed pump
errors and propagated deadline failures.
- Around line 121-125: Ensure the subscription created in withAutoApproval is
always cancelled when deadline rejects: move rpc.chat.subscribe into a
try/finally that invokes abort.abort(), or explicitly abort the controller in
the timeout path before propagating the error. Preserve normal stream handling
and add a regression test covering a stalled subscription.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 16bdb1db-fe6c-4610-ad2e-0679a43e603e
📒 Files selected for processing (10)
packages/extension-testkit/src/boot-server.tspackages/extension-testkit/src/get-extension-test-api.tspackages/harness-testkit/package.jsonpackages/harness-testkit/src/call-tool.tspackages/harness-testkit/src/deadline.tspackages/harness-testkit/src/session.tspackages/harness-testkit/src/testkit.tspackages/harness-testkit/src/until.tspackages/harness-testkit/test/deadline.test.tspackages/harness-testkit/test/until.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/harness-testkit/package.json
- packages/harness-testkit/test/deadline.test.ts
- packages/extension-testkit/src/boot-server.ts
- packages/harness-testkit/src/until.ts
- packages/harness-testkit/test/until.test.ts
- packages/harness-testkit/src/session.ts
- packages/harness-testkit/src/testkit.ts
- packages/extension-testkit/src/get-extension-test-api.ts
- packages/harness-testkit/src/deadline.ts
Comment on lines
+121
to
+125
| const stream = await deadline( | ||
| 'testkit chat.subscribe', | ||
| TESTKIT_DEADLINE_MS, | ||
| rpc.chat.subscribe({sessionId: session}, {signal: abort.signal}), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'chat\.subscribe|withAutoApproval|abort\.abort\(\)' .Repository: conciv-dev/conciv
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- candidate skill files ---'
find .. -type f \( -iname 'SKILL.md' -o -iname '*tanstack*intent*' \) -print 2>/dev/null | head -80
printf '%s\n' '--- relevant file outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline packages/harness-testkit/src/call-tool.ts
ast-grep outline packages/harness-testkit/src/deadline.ts 2>/dev/null || true
ast-grep outline packages/harness-testkit/test/deadline.test.ts
else
echo 'ast-grep unavailable'
fi
printf '%s\n' '--- relevant source ---'
sed -n '1,190p' packages/harness-testkit/src/call-tool.ts
printf '%s\n' '--- deadline implementation and tests ---'
rg -l 'function deadline|export.*deadline|const deadline' packages/harness-testkit
sed -n '1,180p' packages/harness-testkit/test/deadline.test.tsRepository: conciv-dev/conciv
Length of output: 11894
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- repository review skill ---'
sed -n '1,240p' .claude/skills/conciv-review/SKILL.md
printf '%s\n' '--- code review skill ---'
sed -n '1,240p' .github/skills/code-review/SKILL.md
printf '%s\n' '--- source outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline packages/harness-testkit/src/call-tool.ts
deadline_file="$(rg -l 'function deadline|export.*deadline|const deadline' packages/harness-testkit/src | head -1 || true)"
if [ -n "$deadline_file" ]; then ast-grep outline "$deadline_file"; fi
ast-grep outline packages/harness-testkit/test/deadline.test.ts
fi
printf '%s\n' '--- call-tool.ts ---'
sed -n '1,190p' packages/harness-testkit/src/call-tool.ts
printf '%s\n' '--- deadline locations ---'
rg -n -l 'function deadline|export.*deadline|const deadline' packages/harness-testkit/src
printf '%s\n' '--- deadline tests ---'
sed -n '1,180p' packages/harness-testkit/test/deadline.test.tsRepository: conciv-dev/conciv
Length of output: 27012
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- deadline implementation ---'
cat -n packages/harness-testkit/src/deadline.ts
printf '%s\n' '--- RPC client implementation ---'
cat -n packages/harness-testkit/src/session.ts
printf '%s\n' '--- changed-file summary ---'
git diff --stat main...HEAD 2>/dev/null || git diff --stat
printf '%s\n' '--- standalone deadline behavior probe ---'
node - <<'JS'
function deadline(label, budgetMs, work) {
return Promise.race([
work,
new Promise((_, reject) => {
setTimeout(() => reject(new Error(`${label} exceeded ${budgetMs}ms`)), budgetMs)
}),
])
}
const abort = new AbortController()
let subscriptionSettled = false
const subscription = new Promise((resolve) => {
setTimeout(() => {
subscriptionSettled = true
resolve('stream')
}, 100)
})
deadline('testkit chat.subscribe', 10, subscription).catch((error) => {
console.log(JSON.stringify({
rejected: error.message,
signalAbortedAtDeadline: abort.signal.aborted,
subscriptionStillPendingAtDeadline: !subscriptionSettled,
}))
})
setTimeout(() => {}, 30)
JSRepository: conciv-dev/conciv
Length of output: 2344
Abort the chat subscription when its deadline expires.
When deadline('testkit chat.subscribe', ...) rejects, execution leaves withAutoApproval before its try/finally, so abort.abort() does not run. The RPC request and stream can remain active. Move subscription creation into cleanup that always aborts the controller, or abort it in the timeout path. Add a regression test for a stalled subscription.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-testkit/src/call-tool.ts` around lines 121 - 125, Ensure the
subscription created in withAutoApproval is always cancelled when deadline
rejects: move rpc.chat.subscribe into a try/finally that invokes abort.abort(),
or explicitly abort the controller in the timeout path before propagating the
error. Preserve normal stream handling and add a regression test covering a
stalled subscription.
| } finally { | ||
| abort.abort() | ||
| await pump | ||
| await deadline('testkit approval pump drain', TESTKIT_DEADLINE_MS, pump) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve swallowing for ordinary pump errors.
pump.catch(() => {}) does not change the original pump promise. Line 143 still awaits that original promise, so every pump rejection propagates. A normal iterator error or an abort error during shutdown can fail withAutoApproval.
Await a derived promise that swallows non-deadline errors and preserves deadline errors. Add coverage for both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-testkit/src/call-tool.ts` at line 143, Update the
deadline-wrapped pump await in the call-tool flow to await a derived promise
that suppresses ordinary iterator and shutdown-abort rejections while preserving
and propagating deadline errors. Keep the existing testkit deadline behavior
intact, and add coverage for both swallowed pump errors and propagated deadline
failures.
11 tasks
omridevk
changed the title
…named per call site Replace the hand-rolled deadline() helper with p-timeout at every call site (sessions.resolve, chat.subscribe/permissionDecision, approval pump drain, until()'s hang guard), keeping the same named labels. TESTKIT_DEADLINE_MS stays as a constants-only export. Revert the extension-testkit deadline wiring: #412 owns that package now and would conflict. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Closing unmerged, by Omri's call. The RCA-backed content of this PR (the dispose-chain hang class from #390/#361) is now owned elsewhere: browser close is bounded by the file-scoped fixture + p-timeout in PR #412, the static server force-closes stragglers in #412, embed's whole lifecycle moves to @playwright/test (runner-bounded teardown) in the parallel migration, and engine stop's drain was already bounded product-side (RUN_DRAIN_TIMEOUT_MS = 5s, core/src/app.ts). What remained here after the p-timeout rework was per-await deadline wrapping on test-body RPC calls that vitest's testTimeout already bounds — a nonstandard pattern that adds machinery without covering a real hang class. Known residual, tracked on #361: the per-disposer loop in core dispose (app.ts:516) is error-safe but not time-bounded. |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #390 (testkit-wide silent hangs — RCA on the issue).
What
p-timeout(pTimeout(work, {milliseconds, message})), named per call site:sessions.resolve,chat.subscribe,chat.permissionDecisioninside the approval pump, the pump drain inwithAutoApproval's finally, anduntil()'s hang guard. Each stall now rejects with its stage name instead of riding silently to the 200s vitest ceiling. Healthy runs are byte-identical in behavior.deadline()helper in favor ofp-timeout(added as a regular dependency,^7.0.1, matching the lockfile's existing resolution viap-queue).TESTKIT_DEADLINE_MSstays insrc/deadline.tsas a constants-only module (no./deadlinesubpath export — nothing outside the package needs it anymore).until()awaited the predicate before checking its deadline, so a never-settling predicate defeatedhangGuardMsentirely. The poll now races insidepTimeout(); a never-resolving predicate fails at the guard with the named stall error (unit-tested both ways, revert-check verified).Not in this PR
packages/extension-testkithardening (boot-server.ts,get-extension-test-api.ts) moved to #412 to avoid conflicting with that PR's ownership of the package.Verified
typecheck green for
@conciv/harness-testkitand@conciv/extension-testkit; harness-testkit 31 tests green (until.test.tsunchanged and green); oxlint + oxfmt clean; fallow 0 introduced.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests