Skip to content

fix: support positional installs and trace discovery#504

Merged
mohanagy merged 1 commit into
nextfrom
fix/install-platform-and-trace-discovery
Jun 4, 2026
Merged

fix: support positional installs and trace discovery#504
mohanagy merged 1 commit into
nextfrom
fix/install-platform-and-trace-discovery

Conversation

@mohanagy
Copy link
Copy Markdown
Owner

@mohanagy mohanagy commented Jun 4, 2026

Summary

  • allow madar install <platform> alongside --platform and update the help/usage contract
  • keep duplicate or conflicting install platform selectors rejected with one shared usage string
  • preserve exact deferred-Madar ToolSearch positions so compare traces do not hide earlier broad exploration, and only exempt select:mcp__madar__... lookups

Testing

  • npm run test:run -- tests/unit/cli.test.ts tests/unit/compare.test.ts
  • npm run typecheck
  • npm run build

Summary by CodeRabbit

  • Bug Fixes

    • Install command now validates for duplicate platform arguments and provides clearer error messages.
  • New Features

    • Improved trace analysis accuracy with enhanced tool discovery metadata tracking.
  • Tests

    • Added test coverage for install command validation and trace classification scenarios.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR enhances the Madar CLI with stricter platform argument validation and refines trace analysis to distinguish Madar tool-discovery queries from broad exploration. The install command now rejects duplicate platform arguments and updates help text; the trace classifier tracks and excludes deferred Madar tool selections from exploration counts, improving exploration-outcome categorization.

Changes

CLI Install Argument Validation

Layer / File(s) Summary
Install command help text and parser constants
src/cli/main.ts, src/cli/parser.ts
Help text revised to show `[platform
Platform argument validation and duplicate detection
src/cli/parser.ts
parseInstallArgs tracks platform provision to detect and reject duplicates; validates each form (--platform, --platform=value, positional) and throws UsageError(INSTALL_USAGE) for invalid or duplicate values.
CLI validation tests
tests/unit/cli.test.ts
Added assertions for positional platform parsing and multi-platform error cases; updated help text expectations to match revised syntax and example invocation.

Madar Tool-Discovery Metadata & Trace Classification

Layer / File(s) Summary
Trace turn summary interface and discovery helpers
src/infrastructure/compare.ts
CompareMadarTraceTurnSummary extended with optional madar_tool_discovery_count and madar_tool_discovery_tool_indexes fields; helper functions normalize toolsearch names and recognize deferred Madar selection queries (select:mcp__madar__...).
Discovery metadata derivation and exploration classification
src/infrastructure/compare.ts
During Madar trace analysis, derives per-turn discovery tool indexes and skips matching toolsearch calls from broad exploration counts, enabling precise classification of exploration patterns vs. Madar invocation.
Discovery tracking during trace extraction
src/infrastructure/compare.ts
extractMadarTrace accumulates discovery counts and tool-use indexes matching the deferred Madar pattern; merges these fields when aggregating multiple parsed events, with index offsets applied during turn consolidation.
Tool-discovery vs. broad exploration classification tests
tests/unit/compare.test.ts
Three new tests verify deferred Madar selections are excluded from pre-Madar broad exploration counts, non-deferred selections are included, and post-Madar discovery does not suppress earlier exploration—validating the refined classification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • mohanagy/madar#347: Both modify src/infrastructure/compare.ts trace/tool-use classification logic around toolsearch and mcp__madar__* categorization.
  • mohanagy/madar#321: Both refine src/infrastructure/compare.ts trace classification by handling toolsearch differently—this PR adds discovery indexing/suppression while the other adds exploration-outcome refinements.

Poem

A parser stands guard at the gate,
Checking platforms with measured weight,
While traces unfold with metadata bright,
Madar's first step now lit in the right light. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 clearly summarizes the main changes: support for positional install arguments and trace discovery improvements.
Description check ✅ Passed The description covers the main objectives and testing approach, but omits the full testing checklist items and documentation updates 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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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/install-platform-and-trace-discovery

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/unit/cli.test.ts (1)

1112-1115: ⚡ Quick win

Add one more duplicate-selector test path for --platform=.

Please add a case like parseInstallArgs(['--platform=claude', 'cursor'], 'windows') (and/or repeated flag forms) to cover the new duplicate-detection branch introduced for equals syntax.

🤖 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/unit/cli.test.ts` around lines 1112 - 1115, Add a unit test to cover
the duplicate-platform detection for the equals-style flag in parseInstallArgs:
add an assertion that calling parseInstallArgs(['--platform=claude', 'cursor'],
'windows') throws the same usage error, and optionally add a similar case for
repeated equals flags (e.g., ['--platform=claude', '--platform=cursor']) to
exercise the new duplicate-detection branch; reference the parseInstallArgs
function in your test file and use the same expected error string 'Usage: madar
install [platform|--platform P]'.
🤖 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.

Nitpick comments:
In `@tests/unit/cli.test.ts`:
- Around line 1112-1115: Add a unit test to cover the duplicate-platform
detection for the equals-style flag in parseInstallArgs: add an assertion that
calling parseInstallArgs(['--platform=claude', 'cursor'], 'windows') throws the
same usage error, and optionally add a similar case for repeated equals flags
(e.g., ['--platform=claude', '--platform=cursor']) to exercise the new
duplicate-detection branch; reference the parseInstallArgs function in your test
file and use the same expected error string 'Usage: madar install
[platform|--platform P]'.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 690e6b6a-8f99-490a-8144-0f123159dce2

📥 Commits

Reviewing files that changed from the base of the PR and between 5983882 and c2ed886.

📒 Files selected for processing (5)
  • src/cli/main.ts
  • src/cli/parser.ts
  • src/infrastructure/compare.ts
  • tests/unit/cli.test.ts
  • tests/unit/compare.test.ts

@mohanagy mohanagy merged commit 091a226 into next Jun 4, 2026
7 checks passed
@mohanagy mohanagy deleted the fix/install-platform-and-trace-discovery branch June 4, 2026 04:45
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