Skip to content

test: verify retry identities and complete boundary outputs - #33491

Merged
OrcaWin merged 1 commit into
mainfrom
audit/test-value-batch338
Oct 25, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
audit/test-value-batch338

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​61 $\color{#cf222e}{\Huge{\mathbf{−}}}$​34 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​27
Prod 0 0 0 0

Several tests could pass with unrelated retry IDs, truncated sort results, or missing rollback guidance. Compare recovery IDs with the actual socket request, require complete sorted results, and check rollback guidance in rendered help.

Also use the production status projector in the shared-control fixture, replace an arbitrary source-count threshold with shipped-source and scanner-fixture checks, and correct launch test titles to describe their injected boundaries. Seven test files change; production code is unchanged.

Audited 49 sources/336 authored declarations; no removals. Original and candidate runs each passed 385 cases. Four Darwin-only cases skipped, two fish cases were scope-excluded, and two fish/node-pty cases were deferred locally. Changed-file lint/format and independent evidence validation passed; deliberate owner faults confirm the strengthened assertions. Full PR CI supplies typecheck and ast-grep validation.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The changes update tests across CLI runtime, command help, runtime feature sorting, execution-host registry, and source scanning. The tests now compare recovered request identifiers with captured values, check recovery launch eligibility for configured environment variables, and assert formatted rollback help output. Other tests check result lengths, convert runtime-host snapshots through a shared function, and verify source-scan inclusion and exclusion behavior.

Priority: Low

Merge Risk: Low · up to f060f

This test-only PR has a bounded coverage gap; add a distinct required-feature fixture before relying on it to guard merge behavior.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the main test changes and reports testing results, but it omits most required template sections. It does not provide the required ELI5, What Changed, Why, Linked Issue, Visual… Complete the required template sections. Add a plain-language ELI5 summary, structured change and rationale details, a linked issue or maintainer exception, Visual Proof with N/A and a reason, AI Disclosure when applicable, Review and bound…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main changes: stronger retry identity checks and complete boundary-output verification.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the main test changes and reports testing results, but it omits most required template sections. It does not provide the required ELI5, What Changed, Why, Linked Issue, Visual Proof, AI Disclosure, Review, Notes, or Checklist content.

Resolution

Complete the required template sections. Add a plain-language ELI5 summary, structured change and rationale details, a linked issue or maintainer exception, Visual Proof with N/A and a reason, AI Disclosure when applicable, Review and boundary-impact notes, and the required checklist confirmations.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments: 1
src/shared/ephemeral-vm-runtime-feature-sorting.test.ts: 1

30-35: Trivial · Functional Correctness · Quick win

Cover required-feature insertion.

required has the same id, recipeId, and createdAt as features[0]; replacement is not part of featureIdentity. The test therefore covers replacement only. A merge regression that drops a required feature with a new identity would pass these assertions.

Suggested fix
   const required = { ...features[0], replacement: true }
+  const additionalRequired = { ...features[0], id: 'vm-required', replacement: true }
   const merged = new Map(features.map((entry) => [featureIdentity(entry), entry]))
   merged.set(featureIdentity(required), required)
+  merged.set(featureIdentity(additionalRequired), additionalRequired)
   const expectedMerged = [...merged.values()].sort((a, b) =>
     featureIdentity(a).localeCompare(featureIdentity(b))
   )
   reads = 0
-  const actual = mergeRuntimeFeatures(features, [required])
+  const actual = mergeRuntimeFeatures(features, [required, additionalRequired])

Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ac9c053f-4897-4bf4-9bbb-ffca2a07aae4
Commits

Reviewing files that changed from the base of the PR and between 49bde1c and f060f42.

Files selected for processing: 7
  • src/cli/runtime/client-recovery.test.ts
  • src/cli/runtime/profile-state-recovery-launch.test.ts
  • src/cli/runtime/serve-orcad-launch.test.ts
  • src/cli/specs/profile-state.test.ts
  • src/shared/ephemeral-vm-runtime-feature-sorting.test.ts
  • src/shared/execution-host-registry-shared-control-connect.test.ts
  • src/shared/export-let-function-initializer-ban.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@OrcaWin
OrcaWin merged commit 3ff1f00 into main Oct 25, 2026
37 checks passed
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.

1 participant