Skip to content

fix: address release review findings - #102

Merged
mohanagy merged 2 commits into
developmentfrom
fix/release-101-review
Jul 18, 2026
Merged

fix: address release review findings#102
mohanagy merged 2 commits into
developmentfrom
fix/release-101-review

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Summary

Addresses the valid CodeRabbit findings from release-promotion PR #101 before 0.3.0 can advance to main.

  • Corrects miftah_list_approvals metadata to an observational, read-only local tool.
  • Clarifies that delegated approval bearers are only exposed when form elicitation is unavailable.
  • Strengthens descriptor, delegated-tool reservation, and per-issue changelog contract coverage.
  • Records the corrected miftah_list_approvals behavior in the pending 0.3.0 notes.

Test-first evidence

  • The documentation regression failed before the clarification.
  • The management descriptor regression failed before the metadata correction.
  • The changelog scoping contracts failed before the bounded issue-entry helper.

Validation

  • npm test
  • npm run test:core
  • npm run test:coverage — 95.47% statements/lines, 92.00% branches
  • npm run lint, npm run typecheck, npm run build, npm run smoke:cli
  • npm run check:pack, npm run test:package

No approval authorization flow, token handling, profile behavior, or release version changes are included.

Summary by CodeRabbit

  • New Features

    • miftah_list_approvals is now presented as a read-only observation tool, with metadata accurately indicating that it does not modify state.
    • Management tools publish consistent behavioral annotations for improved client handling.
  • Documentation

    • Clarified delegated-agent approval behavior, including safe approval responses and connection-bound, one-time credentials when form elicitation is unavailable.
    • miftah init --client claude-code now provides exact manual permission guidance without changing Claude Code settings.
    • Updated release notes to identify miftah_list_approvals as read-only.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The MCP approval-listing tool now publishes observational, read-only metadata. Approval documentation and release notes were clarified, while changelog contract tests now validate individual issue entries through a shared helper.

Changes

Management contract and documentation

Layer / File(s) Summary
Management metadata and release validation
src/mcp/server/management-tools.ts, tests/management-tools-contract.test.ts, tests/mcp-wrapper.test.ts, docs/config.md, tests/approval-docs-contract.test.ts, CHANGELOG.md, tests/release-version.test.ts
miftah_list_approvals is classified as observational and read-only, with expanded descriptor, compatibility, approval-documentation, changelog, and release-note assertions.
Issue-scoped changelog assertions
tests/helpers/changelog.ts, tests/changelog-issue-entry.test.ts, tests/*-docs-contract.test.ts
Added changelogIssueEntry and migrated changelog checks to validate content within specific issue entries.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • mohanagy/miftah#99: Also updates the centralized miftah_list_approvals management-tool metadata and approval contract tests.
  • mohanagy/miftah#65: Introduces the approval-management machinery associated with miftah_list_approvals.
  • mohanagy/miftah#100: Also updates 0.3.0 changelog and release-version validation.

Poem

A bunny checks the tools in line,
“Read-only” marks the audit sign.
Changelog bullets now stay neat,
Each issue gets its own small seat.
Hop, test, merge—the contract’s sweet!


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ❌ Error Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description covers summary, validation, and test-first evidence, but it omits the required Security impact section from the template. Add a dedicated Security impact section and state the reviewed boundaries for credentials, routing, policy, redaction, audit data, subprocesses, dependencies, and failure behavior.
Title check ❓ Inconclusive The title is relevant but too generic to identify the actual metadata, docs, and test changes. Use a more specific title like “Fix miftah_list_approvals metadata and delegated approval docs/tests.”
✅ Passed checks (2 passed)
Check name Status Explanation
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/release-101-review

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

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 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/management-tools-contract.test.ts`:
- Around line 23-28: Add an exact-length assertion in the delegated-mode test
around managementTools({ delegatedAgentApproval: true }), verifying the returned
delegated collection length equals MANAGEMENT_TOOL_DESCRIPTORS.length before or
alongside the existing descriptor loop. Keep the per-descriptor presence and
annotation assertions unchanged.
🪄 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: 9e2fc4ee-12ee-434a-9285-22760acfe2ae

📥 Commits

Reviewing files that changed from the base of the PR and between 246ec92 and 5789be5.

📒 Files selected for processing (13)
  • CHANGELOG.md
  • docs/config.md
  • src/mcp/server/management-tools.ts
  • tests/approval-docs-contract.test.ts
  • tests/changelog-issue-entry.test.ts
  • tests/helpers/changelog.ts
  • tests/management-tools-contract.test.ts
  • tests/mcp-wrapper.test.ts
  • tests/preset-docs-contract.test.ts
  • tests/profile-runtime-isolation-docs-contract.test.ts
  • tests/profile-state-docs-contract.test.ts
  • tests/release-version.test.ts
  • tests/routing-context-docs-contract.test.ts

Comment thread tests/management-tools-contract.test.ts
@mohanagy
mohanagy merged commit 08b478e into development Jul 18, 2026
12 checks passed
@mohanagy mohanagy mentioned this pull request Jul 18, 2026
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