feat: harden approval and management tool controls

[mohanagy]

Summary

• default protected actions now fail closed when a client cannot present a human form; bearer approval is explicitly opt-in through security.approvalMode: delegated-agent
• generated GitHub multi-profile configurations require profile-switch confirmation and explicit destructive-profile selection
• centralize management-tool schemas, annotations, exposure, and exact Claude Code permission guidance
• bind approvals to their form or delegated mechanism and audit disabled delegated controls as policy denials

Security impact

• Default non-form clients cannot obtain or self-confirm bearer approvals.
• Delegated approval remains an explicit configuration choice and approval records cannot cross into another confirmation mechanism.
• Client permission snippets are exact allow-list guidance only; Miftah remains the server-side authorization boundary.

Test-first and validation

• Added focused failing regressions for cross-mechanism approval reuse and disabled-delegation audit status before the implementation fix.
• Updated configuration, contracts, examples, and security documentation where the public behavior changed.
• Completed focused security and management-tool review, including the stateful approval-expiry behavior behind miftah_list_approvals.
• npm test
• npm run test:core
• npm run test:coverage
• npm run lint
• npm run typecheck
• npm run build
• npm run smoke:cli
• npm run check:pack
• npm run test:package

Closes #96
Closes #97
Closes #98

Summary by CodeRabbit

• New Features
  • Added configurable security.approvalMode with human (default) and delegated-agent options.
  • Human approvals now fail closed when confirmation forms aren’t available.
  • Delegated approval tools are only exposed when explicitly enabled.
  • miftah init --client claude-code now prints reviewed Claude Code permission guidance without modifying client settings.
  • GitHub multi-profile presets require exact profile-switch and destructive-action confirmations.
• Documentation
  • Updated configuration, security, routing, CLI, presets, and audit documentation to reflect mechanism-specific approval visibility and fail-closed behavior.
  • Audit entries now record which approval mechanism was used.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0510c888-3dca-4253-b1fc-7984843b8913

📥 Commits

Reviewing files that changed from the base of the PR and between fa4f65d and 069dfc7.

📒 Files selected for processing (4)

• src/approvals/approval-store.ts
• src/cli/client-snippets.ts
• src/cli/init.ts
• src/mcp/server/miftah-server.ts

---

📝 Walkthrough

Walkthrough

Miftah adds explicit approval mechanisms, fail-closed human confirmation, delegated-agent gating, centralized management-tool descriptors and annotations, Claude Code permission guidance, and stricter GitHub multi-profile preset defaults.

Changes

Security and onboarding controls

Layer / File(s)	Summary
Approval mechanisms and fail-closed enforcement src/approvals/approval-store.ts, src/mcp/server/miftah-server.ts, src/audit/*, tests/approval-*	Approval records and audit events distinguish form from delegated-agent; non-form human-mode requests fail closed, while approval tools require explicit delegated-agent mode.
Management descriptors and Claude Code guidance src/mcp/server/management-tools.ts, src/cli/client-snippets.ts, src/cli/init.ts, tests/management-tools-contract.test.ts, tests/client-snippets.test.ts	Management tools are generated from centralized descriptors with annotations and visibility rules, and Claude Code permissions.ask guidance is printed without writing client settings.
Approval schema and multi-profile preset defaults src/config/schema.ts, src/config/types.ts, src/config/presets.ts, examples/github.miftah.json, tests/presets.test.ts	Configuration accepts security.approvalMode, while generated GitHub multi-profile configurations require profile confirmation and explicit destructive selection.
Security documentation and contract alignment README.md, docs/*, CHANGELOG.md, tests/*docs-contract*	Documentation and contract tests describe approval boundaries, management-tool guidance, multi-profile safety defaults, and updated changelog validation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• mohanagy/miftah#65 — Earlier connection-bound approval implementation extended here with explicit approval mechanisms and audit tracking.
• mohanagy/miftah#47 — Related management-tool discovery and routing paths.
• mohanagy/miftah#53 — Related audit event model and approval metadata.

Poem

A bunny guards the approval gate,
With forms and badges labeled straight.
Tools wear notes, presets choose,
Claude gets rules—but settings snooze.
“Exact and safe!” the rabbit sings,
While audits record the proper things.

---

Caution

Pre-merge checks failed

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

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Docstring Coverage	❌ Error	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the PR’s main theme: hardening approval and management-tool controls.
Description check	✅ Passed	The description covers Summary, Security impact, and Validation, with only minor template-format gaps in the validation details.
Linked Issues check	✅ Passed	The changes address #96, #97, and #98 with mechanism-bound approvals, hardened multi-profile defaults, and typed management-tool descriptors plus permission guidance.
Out of Scope Changes check	✅ Passed	Most changes directly support the linked security, profile, and management-tool goals, with no clearly unrelated code introduced.

✨ 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 feat/issues-96-98-security

---

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

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

tests/profile-leases-docs-contract.test.ts (1)

2-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve Unreleased-section scoping in both changelog contract tests.

Both replacements scan the entire changelog, allowing historical entries to satisfy assertions intended to validate the current release notes.

• tests/profile-leases-docs-contract.test.ts#L2-L3,L35-L35: restore section-scoped extraction or constrain the [#28] match to ## [Unreleased].
• tests/profile-runtime-isolation-docs-contract.test.ts#L2-L2,L35-L35: restore section-scoped extraction or constrain the [#29] match to ## [Unreleased].

🤖 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/profile-leases-docs-contract.test.ts` around lines 2 - 3, Update both
changelog contract tests, tests/profile-leases-docs-contract.test.ts lines 2-3
and 35, and tests/profile-runtime-isolation-docs-contract.test.ts lines 2 and
35, so their assertions inspect only the ## [Unreleased] section. Restore
section-scoped extraction or constrain the profile leases [`#28`] and profile
runtime isolation [`#29`] matches to that section; do not allow historical
changelog entries to satisfy either test.

src/approvals/approval-store.ts (1)

106-119: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Make the approval mechanism explicit and immutable.

request() defaults to "delegated-agent" while the bearer-safety callback defaults to accepting any bearer. A caller that omits mechanism therefore silently creates delegated-agent authorization, contrary to the explicit opt-in contract. Pending-record reuse also ignores mechanism, so the same binding can be requested under a different mechanism while the stored summary retains the original one. Require the mechanism and reject mismatched reuse (or bind it through the consume path) to prevent authorization and audit inconsistencies.

🤖 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/approvals/approval-store.ts` around lines 106 - 119, The request method
must require an explicit, immutable approval mechanism instead of defaulting to
"delegated-agent". Remove the mechanism default in request, and ensure
pending-record reuse only returns a token when the stored mechanism matches the
requested mechanism; otherwise reject or proceed through a mechanism-bound
creation/consume path so the stored summary and authorization remain consistent.

🤖 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 `@src/mcp/server/management-tools.ts`:
- Around line 190-201: Update the miftah_list_approvals tool definition to
classify its non-mutating approval metadata listing as observational: change
interaction to observational, set readOnlyHint to true, destructiveHint to
false, and idempotentHint to true; leave its availability and other fields
unchanged.

In `@src/mcp/server/miftah-server.ts`:
- Around line 1696-1701: Update auditStatus() to classify MiftahError code
APPROVAL_DELEGATION_DISABLED as an audit denial rather than a failure,
preserving the existing handling for other error codes and the
assertDelegatedAgentApprovalEnabled() behavior.

---

Outside diff comments:
In `@src/approvals/approval-store.ts`:
- Around line 106-119: The request method must require an explicit, immutable
approval mechanism instead of defaulting to "delegated-agent". Remove the
mechanism default in request, and ensure pending-record reuse only returns a
token when the stored mechanism matches the requested mechanism; otherwise
reject or proceed through a mechanism-bound creation/consume path so the stored
summary and authorization remain consistent.

In `@tests/profile-leases-docs-contract.test.ts`:
- Around line 2-3: Update both changelog contract tests,
tests/profile-leases-docs-contract.test.ts lines 2-3 and 35, and
tests/profile-runtime-isolation-docs-contract.test.ts lines 2 and 35, so their
assertions inspect only the ## [Unreleased] section. Restore section-scoped
extraction or constrain the profile leases [`#28`] and profile runtime isolation
[`#29`] matches to that section; do not allow historical changelog entries to
satisfy either test.

🪄 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: 5d44142a-9c2a-417e-80ab-01f925efb8c2

📥 Commits

Reviewing files that changed from the base of the PR and between cbfde7a and 77b446d.

📒 Files selected for processing (37)

• CHANGELOG.md
• README.md
• docs/architecture.md
• docs/cli.md
• docs/config.md
• docs/library-api.md
• docs/presets-and-clients.md
• docs/security.md
• docs/threat-model.md
• examples/github.miftah.json
• src/approvals/approval-store.ts
• src/audit/audit-trail.ts
• src/audit/audit-types.ts
• src/cli/client-snippets.ts
• src/cli/exit-codes.ts
• src/cli/init.ts
• src/config/presets.ts
• src/config/schema.ts
• src/config/types.ts
• src/mcp/server/management-tools.ts
• src/mcp/server/miftah-server.ts
• src/utils/errors.ts
• tests/approval-fallback.test.ts
• tests/audit-outcomes.test.ts
• tests/cli-exit-codes.test.ts
• tests/client-snippets.test.ts
• tests/config-schema-contract.test.ts
• tests/identity-docs-contract.test.ts
• tests/init-command.test.ts
• tests/management-tools-contract.test.ts
• tests/mcp-wrapper.test.ts
• tests/preset-docs-contract.test.ts
• tests/presets.test.ts
• tests/profile-leases-docs-contract.test.ts
• tests/profile-runtime-isolation-docs-contract.test.ts
• tests/profile-state-docs-contract.test.ts
• tests/routing-context-docs-contract.test.ts

[mohanagy]

CodeRabbit follow-up for the outside-diff comments:\n\n- Approval mechanism handling is fixed in 07b45f4: the mechanism is explicit at runtime and in TypeScript, pending records reject cross-mechanism reuse, and consumption is mechanism-bound.\n- The #28 and #29 assertions intentionally inspect retained release history. Those entries are published in 0.2.0, not current Unreleased notes. The prior helper only happened to return released history when Unreleased was empty; requiring old entries in every unrelated Unreleased section would make the contract incorrect.\n\nThe focused regressions and the complete release-quality suite pass on 07b45f4.

[mohanagy]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mohanagy]

@coderabbitai run pre-merge checks

[coderabbitai]

✅ Actions performed

Running Pre-merge checks. Results will be posted in the summary comment once complete.

[mohanagy]

@coderabbitai run pre-merge checks

[coderabbitai]

✅ Actions performed

Running Pre-merge checks. Results will be posted in the summary comment once complete.

[mohanagy]

CodeRabbit pre-merge exception record for current head 069dfc7:

• CodeRabbit's current status check is green and its fresh review reports no actionable comments; both review threads are resolved.
• I invoked @coderabbitai run pre-merge checks twice. The bot acknowledged both requests, but both results have remained Running for more than ten minutes with no current-head score.
• The only visible 37.50% result predates the final named-function documentation commit. A focused TypeScript AST audit of the current PR source finds JSDoc on 25 of 29 changed named declarations (86.21%).
• CodeRabbit's organization-managed metric does not expose its candidate list, so there is no further evidence-based source change to make without adding noise documentation to anonymous callbacks.

This records a demonstrably stuck, opaque pre-merge metric rather than disregarding actionable feedback. I will proceed only after the exact-head Windows retry and all other required CI checks are green.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@mohanagy Acknowledged. The record clearly documents that the stale 37.50% metric predates the current head and that the current review has no actionable findings. Waiting for the exact-head Windows retry and all remaining required CI checks is the appropriate merge gate.

🐇⏳