release: v0.3.2 - #134
Conversation
* fix: always close upstreams after server close errors * test: preserve containment close error identity
* test: diagnose Windows descendant readiness boundary * test: diagnose cold Windows provider helper settlement * test: share provider readiness wait helper
chore: prepare v0.3.2 release
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe release adds origin-pinned PostHog command classification, shared preview enforcement, resilient shutdown cleanup, synchronized Windows provider tests, and version 0.3.2 release and toolchain updates. ChangesSecurity and policy flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MiftahServer
participant OperationPipeline
participant PosthogCommandClassifier
Client->>MiftahServer: request miftah_route_preview
MiftahServer->>PosthogCommandClassifier: classify bounded command metadata
PosthogCommandClassifier-->>MiftahServer: return risk and provenance
MiftahServer->>OperationPipeline: evaluate policy enforcement
OperationPipeline-->>MiftahServer: return allowed or POLICY_BLOCKED
MiftahServer-->>Client: return preview and enforcement
Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server/miftah-server.ts (1)
476-502: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSolid containment fix; consider surfacing all close failures, not just the first.
Each shutdown step now runs regardless of earlier failures, which correctly fixes the containment gap.
closeFailure ??= { error }only preserves the first error though — ifunsubscribeResourceSubscriptionsandupstreams.close()both fail, the second failure is silently dropped.♻️ Optional: aggregate all close failures
- let closeFailure: { readonly error: unknown } | undefined; - try { - await this.unsubscribeResourceSubscriptions(() => true); - } catch (error) { - closeFailure = { error }; - } - try { - await this.server.close(); - } catch (error) { - closeFailure ??= { error }; - } - try { - await this.upstreams.close(); - } catch (error) { - closeFailure ??= { error }; - } - if (closeFailure !== undefined) throw closeFailure.error; + const closeErrors: unknown[] = []; + try { + await this.unsubscribeResourceSubscriptions(() => true); + } catch (error) { + closeErrors.push(error); + } + try { + await this.server.close(); + } catch (error) { + closeErrors.push(error); + } + try { + await this.upstreams.close(); + } catch (error) { + closeErrors.push(error); + } + if (closeErrors.length > 0) throw new AggregateError(closeErrors, "Failed to close one or more shutdown resources");🤖 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 `@src/mcp/server/miftah-server.ts` around lines 476 - 502, Update close() to collect every error thrown by unsubscribeResourceSubscriptions, server.close, and upstreams.close instead of retaining only the first via closeFailure. After all shutdown steps complete, surface the aggregated failures while preserving the existing execution order and lifecycle audit behavior.
🤖 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 `@tests/secret-providers.test.ts`:
- Around line 412-437: Update the test around waitForProviderEntered, observed,
and commandSettled to enforce the two-phase handshake: make the fixture block
immediately after writing the provider-entry marker, assert commandSettled is
still false before releasing that block, then release the fixture and await the
existing settlement race. Preserve the current success and rejection assertions
after the release.
---
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 476-502: Update close() to collect every error thrown by
unsubscribeResourceSubscriptions, server.close, and upstreams.close instead of
retaining only the first via closeFailure. After all shutdown steps complete,
surface the aggregated failures while preserving the existing execution order
and lifecycle audit behavior.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: c91b685a-a456-40ce-89d5-257a25ccd173
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
CHANGELOG.mddocs/architecture.mddocs/config.mddocs/presets-and-clients.mddocs/security.mdpackage.jsonsrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/policy/policy-types.tssrc/policy/posthog-command-wrapper.tssrc/policy/risk-classifier.tssrc/policy/risk-name-patterns.tstests/fixtures/fake-secret-provider.mjstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/package-contract.test.tstests/posthog-command-wrapper.test.tstests/preset-docs-contract.test.tstests/release-version.test.tstests/risk-classification-docs-contract.test.tstests/secret-providers.test.ts
| let commandSettled = false; | ||
| const observed = pending.then( | ||
| (result) => { | ||
| commandSettled = true; | ||
| return { status: "fulfilled" as const, result }; | ||
| }, | ||
| (error: unknown) => { | ||
| commandSettled = true; | ||
| return { status: "rejected" as const, error }; | ||
| } | ||
| ); | ||
| let settlementTimer: NodeJS.Timeout | undefined; | ||
|
|
||
| try { | ||
| await waitForProviderEntered(providerReadyPath, "the cold fake provider to enter through the Windows helper"); | ||
| const outcome = await Promise.race([ | ||
| observed, | ||
| new Promise<{ status: "pending" }>((resolve) => { | ||
| settlementTimer = setTimeout(() => resolve({ status: "pending" }), 2_000); | ||
| }) | ||
| ]); | ||
| if (outcome.status === "pending") { | ||
| throw new Error("The provider entered through the Windows helper but the helper did not settle within 2000ms"); | ||
| } | ||
| if (outcome.status === "rejected") throw outcome.error; | ||
| expect(outcome.result.stdout.toString("utf8")).toBe("fixture-provider-secret"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the required provider-entry/settlement ordering.
This can pass when observed settled while waitForProviderEntered() was polling: commandSettled is tracked but never checked before the race. Use a two-phase fixture handshake—block after writing the entry marker—then assert the command is pending before releasing the fixture and awaiting settlement.
🤖 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 `@tests/secret-providers.test.ts` around lines 412 - 437, Update the test
around waitForProviderEntered, observed, and commandSettled to enforce the
two-phase handshake: make the fixture block immediately after writing the
provider-entry marker, assert commandSettled is still false before releasing
that block, then release the fixture and await the existing settlement race.
Preserve the current success and rejection assertions after the release.
test: make npm failure diagnostics deterministic
test: diagnose Windows cold provider entry timing
|
Superseded by a fresh v0.3.2 promotion PR after #135 and #137 merged into development. This PR retains historic CI/review state from the earlier release head, including a CodeRabbit changes-requested decision, so it will not be used for release. The replacement will be reviewed and validated against the exact current development head before any merge to main. |
1 participant
Fixes #127
Release promotion
Promotes the validated
developmentcommitd65f3f16db35d5b5c0ae37b6f0992ec8840d7766tomainfor@lubab/miftahv0.3.2.Included fixes
Evidence before promotion
After this promotion PR is green and merged, I will tag this exact
maincommit and create GitHub Releasev0.3.2; the protected OIDC workflow will performnpm publish --provenance.Summary by CodeRabbit